fix: improve archive switch reliability (#1325)

2026-06-11 09:50:52 +01:00
parent a03d65fdf8
commit b0482285aa
6 changed files with 256 additions and 68 deletions
@@ -12,6 +12,7 @@ export type SidebarDialogsActions = {
  handleRenameSubmit: (newTitle: string) => Promise<void> | void;
  archivingTask: Target;
  setArchivingTask: (next: Target) => void;
+  archivingTaskId: string | null;
  isArchiving: boolean;
  handleArchiveConfirm: (opts: { cascade: boolean }) => Promise<void> | void;
  deletingTask: Target;
@@ -27,6 +28,7 @@ export function SidebarDialogs({ actions }: { actions: SidebarDialogsActions })
    handleRenameSubmit,
    archivingTask,
    setArchivingTask,
+    archivingTaskId,
    isArchiving,
    handleArchiveConfirm,
    deletingTask,
@@ -52,7 +54,7 @@ export function SidebarDialogs({ actions }: { actions: SidebarDialogsActions })
        taskTitle={archivingTask?.title ?? ""}
        taskId={archivingTask?.id}
        executorType={archivingTask?.executorType}
-        isArchiving={isArchiving}
+        isArchiving={isArchiving && archivingTask?.id === archivingTaskId}
        onConfirm={handleArchiveConfirm}
      />
      <TaskDeleteConfirmDialog
@@ -364,6 +364,7 @@ function useArchiveActions(store: StoreApi) {
    title: string;
    executorType?: string | null;
  } | null>(null);
+  const [archivingTaskId, setArchivingTaskId] = useState<string | null>(null);
  const [isArchiving, setIsArchiving] = useState(false);

  const handleArchiveTask = useCallback(
@@ -382,20 +383,30 @@ function useArchiveActions(store: StoreApi) {
  const handleArchiveConfirm = useCallback(
    async (opts: { cascade: boolean }) => {
      if (!archivingTask) return;
+      const taskId = archivingTask.id;
      setIsArchiving(true);
+      setArchivingTaskId(taskId);
      try {
-        await archiveAndSwitch(archivingTask.id, opts);
+        await archiveAndSwitch(taskId, opts);
      } catch (error) {
        console.error("Failed to archive task:", error);
      } finally {
        setIsArchiving(false);
-        setArchivingTask(null);
+        setArchivingTaskId((current) => (current === taskId ? null : current));
+        setArchivingTask((current) => (current?.id === taskId ? null : current));
      }
    },
    [archivingTask, archiveAndSwitch],
  );

-  return { archivingTask, setArchivingTask, isArchiving, handleArchiveTask, handleArchiveConfirm };
+  return {
+    archivingTask,
+    setArchivingTask,
+    archivingTaskId,
+    isArchiving,
+    handleArchiveTask,
+    handleArchiveConfirm,
+  };
 }

 function useDeleteActions(
@@ -0,0 +1,108 @@
+import { describe, it, expect, vi, beforeEach } from "vitest";
+import { renderHook, act } from "@testing-library/react";
+
+const archiveTaskMock = vi.fn();
+const removeTaskFromBoardMock = vi.fn();
+const getStateMock = vi.fn();
+const storeMock = { getState: getStateMock };
+const replaceTaskUrlMock = vi.fn();
+const setActiveSessionMock = vi.fn();
+const setActiveTaskMock = vi.fn();
+let storeState: {
+  tasks: { activeTaskId: string | null; activeSessionId: string | null };
+  setActiveSession: (...args: unknown[]) => void;
+  setActiveTask: (...args: unknown[]) => void;
+};
+
+vi.mock("@/lib/api", () => ({
+  archiveTask: (...args: unknown[]) => archiveTaskMock(...args),
+  deleteTask: vi.fn(),
+  moveTask: vi.fn(),
+  updateTask: vi.fn(),
+}));
+
+vi.mock("@/lib/links", () => ({
+  replaceTaskUrl: (...args: unknown[]) => replaceTaskUrlMock(...args),
+}));
+
+vi.mock("@/components/state-provider", () => ({
+  useAppStoreApi: () => storeMock,
+}));
+
+vi.mock("@/hooks/use-task-removal", () => ({
+  useTaskRemoval: () => ({
+    removeTaskFromBoard: (...args: unknown[]) => removeTaskFromBoardMock(...args),
+  }),
+}));
+
+import { useArchiveAndSwitchTask } from "./use-task-actions";
+
+function deferred<T>() {
+  let resolve!: (value: T | PromiseLike<T>) => void;
+  let reject!: (reason?: unknown) => void;
+  const promise = new Promise<T>((res, rej) => {
+    resolve = res;
+    reject = rej;
+  });
+  return { promise, resolve, reject };
+}
+
+beforeEach(() => {
+  vi.clearAllMocks();
+  storeState = {
+    tasks: { activeTaskId: "task-A", activeSessionId: "sess-A" },
+    setActiveSession: (...args: unknown[]) => setActiveSessionMock(...args),
+    setActiveTask: (...args: unknown[]) => setActiveTaskMock(...args),
+  };
+  getStateMock.mockReturnValue(storeState);
+  removeTaskFromBoardMock.mockResolvedValue({ switchedTaskId: "task-B" });
+});
+
+describe("useArchiveAndSwitchTask", () => {
+  it("removes and switches away from the active task before archive API resolves", async () => {
+    const archive = deferred<void>();
+    archiveTaskMock.mockReturnValueOnce(archive.promise);
+    const { result } = renderHook(() => useArchiveAndSwitchTask());
+
+    let archiveAndSwitchPromise!: Promise<void>;
+    act(() => {
+      archiveAndSwitchPromise = result.current("task-A");
+    });
+
+    expect(removeTaskFromBoardMock).toHaveBeenCalledWith("task-A", {
+      wasActiveTaskId: "task-A",
+      wasActiveSessionId: "sess-A",
+      switchOnly: true,
+    });
+
+    archive.resolve();
+    await archiveAndSwitchPromise;
+    expect(archiveTaskMock).toHaveBeenCalledWith("task-A", undefined);
+    expect(removeTaskFromBoardMock).toHaveBeenLastCalledWith("task-A", {
+      wasActiveTaskId: "task-A",
+      wasActiveSessionId: "sess-A",
+    });
+  });
+
+  it("restores active task when archive API rejects after switching", async () => {
+    const error = new Error("network error");
+    archiveTaskMock.mockRejectedValueOnce(error);
+    removeTaskFromBoardMock.mockImplementationOnce(async () => {
+      storeState.tasks.activeTaskId = "task-B";
+      return { switchedTaskId: "task-B" };
+    });
+    const { result } = renderHook(() => useArchiveAndSwitchTask());
+
+    await expect(result.current("task-A")).rejects.toThrow("network error");
+
+    expect(removeTaskFromBoardMock).toHaveBeenCalledTimes(1);
+    expect(removeTaskFromBoardMock).toHaveBeenCalledWith("task-A", {
+      wasActiveTaskId: "task-A",
+      wasActiveSessionId: "sess-A",
+      switchOnly: true,
+    });
+    expect(setActiveSessionMock).toHaveBeenCalledWith("task-A", "sess-A");
+    expect(replaceTaskUrlMock).toHaveBeenCalledWith("task-A");
+    expect(archiveTaskMock).toHaveBeenCalledWith("task-A", undefined);
+  });
+});
@@ -1,5 +1,6 @@
 import { useCallback } from "react";
 import { archiveTask, deleteTask, moveTask, updateTask } from "@/lib/api";
+import { replaceTaskUrl } from "@/lib/links";
 import { useAppStoreApi } from "@/components/state-provider";
 import { useTaskRemoval } from "@/hooks/use-task-removal";

@@ -41,8 +42,31 @@ export function useArchiveAndSwitchTask(opts?: { useLayoutSwitch?: boolean }) {
    async (taskId: string, opts?: { cascade?: boolean }) => {
      const { activeTaskId: wasActiveTaskId, activeSessionId: wasActiveSessionId } =
        store.getState().tasks;
-      await archiveTaskById(taskId, opts);
-      await removeTaskFromBoard(taskId, { wasActiveTaskId, wasActiveSessionId });
+
+      const initialSwitch = await removeTaskFromBoard(taskId, {
+        wasActiveTaskId,
+        wasActiveSessionId,
+        switchOnly: true,
+      });
+
+      try {
+        await archiveTaskById(taskId, opts);
+        await removeTaskFromBoard(taskId, { wasActiveTaskId, wasActiveSessionId });
+      } catch (error) {
+        if (
+          wasActiveTaskId &&
+          initialSwitch.switchedTaskId !== null &&
+          store.getState().tasks.activeTaskId === initialSwitch.switchedTaskId
+        ) {
+          if (wasActiveSessionId) {
+            store.getState().setActiveSession(wasActiveTaskId, wasActiveSessionId);
+          } else {
+            store.getState().setActiveTask(wasActiveTaskId);
+          }
+          replaceTaskUrl(wasActiveTaskId);
+        }
+        throw error;
+      }
    },
    [archiveTaskById, removeTaskFromBoard, store],
  );
@@ -88,12 +88,70 @@ function makeStore(init: {
 }

 const nextTask: TaskRow = { id: "task-next", primarySessionId: "sess-next" };
+const recentTaskId = "task-recent";
+const recentSessionId = "sess-recent";

 beforeEach(() => {
  vi.clearAllMocks();
  window.localStorage.clear();
 });

+function makeStoreWithOldAndRecentTasks(activeTaskId: string | null) {
+  const oldTask = { id: "task-old", primarySessionId: "sess-old" };
+  const recentTask = { id: recentTaskId, primarySessionId: recentSessionId };
+  const store = makeStore({
+    activeTaskId,
+    activeSessionId: "sess-A",
+    remainingTasks: [{ id: "task-A", primarySessionId: "sess-A" }, oldTask, recentTask],
+  });
+  store.getRecorded().environmentIdBySessionId = {
+    ...store.getRecorded().environmentIdBySessionId,
+    "sess-old": "env-old",
+    [recentSessionId]: "env-recent",
+  };
+  return store;
+}
+
+function setRecentTaskFirst() {
+  setRecentTasks([
+    {
+      taskId: recentTaskId,
+      title: "Recent task",
+      visitedAt: "2026-06-07T10:00:00Z",
+    },
+    {
+      taskId: "task-A",
+      title: "Removed task",
+      visitedAt: "2026-06-07T09:00:00Z",
+    },
+    {
+      taskId: "task-old",
+      title: "Old task",
+      visitedAt: "2026-06-06T10:00:00Z",
+    },
+  ]);
+}
+
+function setRemovedTaskFirst() {
+  setRecentTasks([
+    {
+      taskId: "task-A",
+      title: "Removed task",
+      visitedAt: "2026-06-07T10:00:00Z",
+    },
+    {
+      taskId: recentTaskId,
+      title: "Recent task",
+      visitedAt: "2026-06-07T09:00:00Z",
+    },
+    {
+      taskId: "task-old",
+      title: "Old task",
+      visitedAt: "2026-06-06T10:00:00Z",
+    },
+  ]);
+}
+
 describe("useTaskRemoval — switch guard (current store wins)", () => {
  it("switches to next task when activeTaskId === taskId (user still on removed task)", async () => {
    const store = makeStore({
@@ -216,10 +274,11 @@ describe("useTaskRemoval — switch guard (WS-clear fallback)", () => {
      const { result } = renderHook(() =>
        useTaskRemoval({ store: store as unknown as StoreApi<never> }),
      );
-      await result.current.removeTaskFromBoard("task-A", {
+      const removeResult = await result.current.removeTaskFromBoard("task-A", {
        wasActiveTaskId: "task-A",
        wasActiveSessionId: "sess-A",
      });
+      expect(removeResult.switchedTaskId).toBeNull();
      expect(hrefSetter).toHaveBeenCalledWith("/");
    } finally {
      Object.defineProperty(window, "location", {
@@ -231,44 +290,9 @@ describe("useTaskRemoval — switch guard (WS-clear fallback)", () => {
 });

 describe("useTaskRemoval — next task selection", () => {
-  const recentTaskId = "task-recent";
-  const recentSessionId = "sess-recent";
-
-  function makeStoreWithOldAndRecentTasks(activeTaskId: string | null) {
-    const oldTask = { id: "task-old", primarySessionId: "sess-old" };
-    const recentTask = { id: recentTaskId, primarySessionId: recentSessionId };
-    const store = makeStore({
-      activeTaskId,
-      activeSessionId: "sess-A",
-      remainingTasks: [{ id: "task-A", primarySessionId: "sess-A" }, oldTask, recentTask],
-    });
-    store.getRecorded().environmentIdBySessionId = {
-      ...store.getRecorded().environmentIdBySessionId,
-      "sess-old": "env-old",
-      [recentSessionId]: "env-recent",
-    };
-    return store;
-  }
-
  it("switches to the most recent remaining task instead of the first snapshot task", async () => {
    const store = makeStoreWithOldAndRecentTasks("task-A");
-    setRecentTasks([
-      {
-        taskId: recentTaskId,
-        title: "Recent task",
-        visitedAt: "2026-06-07T10:00:00Z",
-      },
-      {
-        taskId: "task-A",
-        title: "Removed task",
-        visitedAt: "2026-06-07T09:00:00Z",
-      },
-      {
-        taskId: "task-old",
-        title: "Old task",
-        visitedAt: "2026-06-06T10:00:00Z",
-      },
-    ]);
+    setRecentTaskFirst();

    const { result } = renderHook(() =>
      useTaskRemoval({ store: store as unknown as StoreApi<never> }),
@@ -288,23 +312,7 @@ describe("useTaskRemoval — next task selection", () => {

  it("skips the removed active task when it is first in recent history", async () => {
    const store = makeStoreWithOldAndRecentTasks("task-A");
-    setRecentTasks([
-      {
-        taskId: "task-A",
-        title: "Removed task",
-        visitedAt: "2026-06-07T10:00:00Z",
-      },
-      {
-        taskId: recentTaskId,
-        title: "Recent task",
-        visitedAt: "2026-06-07T09:00:00Z",
-      },
-      {
-        taskId: "task-old",
-        title: "Old task",
-        visitedAt: "2026-06-06T10:00:00Z",
-      },
-    ]);
+    setRemovedTaskFirst();

    const { result } = renderHook(() =>
      useTaskRemoval({ store: store as unknown as StoreApi<never> }),
@@ -321,4 +329,26 @@ describe("useTaskRemoval — next task selection", () => {
    );
    expect(replaceTaskUrlMock).toHaveBeenCalledWith(recentTaskId);
  });
+
+  it("can switch before removing the task from board state", async () => {
+    const store = makeStoreWithOldAndRecentTasks("task-A");
+    setRemovedTaskFirst();
+
+    const { result } = renderHook(() =>
+      useTaskRemoval({ store: store as unknown as StoreApi<never> }),
+    );
+
+    await result.current.removeTaskFromBoard("task-A", {
+      wasActiveTaskId: "task-A",
+      wasActiveSessionId: "sess-A",
+      switchOnly: true,
+    });
+
+    expect(store.getRecorded().setWorkflowSnapshot).not.toHaveBeenCalled();
+    expect(store.getRecorded().setActiveSession).toHaveBeenCalledWith(
+      recentTaskId,
+      recentSessionId,
+    );
+    expect(replaceTaskUrlMock).toHaveBeenCalledWith(recentTaskId);
+  });
 });
@@ -26,6 +26,12 @@ type RemoveFromBoardOptions = {
  wasActiveTaskId?: string | null;
  /** The active session ID captured before the async delete API call. */
  wasActiveSessionId?: string | null;
+  /** Switch away from the task without removing it from board state yet. */
+  switchOnly?: boolean;
+};
+
+type RemoveFromBoardResult = {
+  switchedTaskId: string | null;
 };

 function cachedSessionsHaveEnvIds(sessions: TaskSession[]): boolean {
@@ -95,13 +101,16 @@ function collectRemainingTasks(store: StoreApi<AppState>): KanbanState["tasks"]

 function selectNextTaskAfterRemoval(
  remainingTasks: KanbanState["tasks"],
+  removedTaskId: string,
 ): KanbanState["tasks"][number] | null {
-  const remainingById = new Map(remainingTasks.map((task) => [task.id, task]));
+  const remainingById = new Map(
+    remainingTasks.filter((task) => task.id !== removedTaskId).map((task) => [task.id, task]),
+  );
  for (const recent of getRecentTasks()) {
    const task = remainingById.get(recent.taskId);
    if (task) return task;
  }
-  return remainingTasks[0] ?? null;
+  return remainingTasks.find((task) => task.id !== removedTaskId) ?? null;
 }

 function switchToSessionForTask(params: {
@@ -206,14 +215,16 @@ export function useTaskRemoval({ store, useLayoutSwitch = false }: TaskRemovalOp
   * wins and the captured value is ignored (no auto-switch).
   */
  const removeTaskFromBoard = useCallback(
-    async (taskId: string, opts?: RemoveFromBoardOptions) => {
-      removeTaskFromSnapshots(store, taskId);
+    async (taskId: string, opts?: RemoveFromBoardOptions): Promise<RemoveFromBoardResult> => {
+      if (!opts?.switchOnly) removeTaskFromSnapshots(store, taskId);
      const allRemainingTasks = collectRemainingTasks(store);

-      if (!shouldSwitchAfterRemoval(store, taskId, opts)) return;
+      if (!shouldSwitchAfterRemoval(store, taskId, opts)) {
+        return { switchedTaskId: null };
+      }

      const oldEnvId = resolveOldEnvId(store, opts);
-      const nextTask = selectNextTaskAfterRemoval(allRemainingTasks);
+      const nextTask = selectNextTaskAfterRemoval(allRemainingTasks, taskId);
      if (nextTask) {
        await switchToNextTask({
          store,
@@ -222,9 +233,11 @@ export function useTaskRemoval({ store, useLayoutSwitch = false }: TaskRemovalOp
          useLayoutSwitch,
          loadTaskSessionsForTask,
        });
-      } else {
-        window.location.href = "/";
+        return { switchedTaskId: nextTask.id };
      }
+
+      window.location.href = "/";
+      return { switchedTaskId: null };
    },
    [store, useLayoutSwitch, loadTaskSessionsForTask],
  );