fix(web): mobile task switcher sheet skeleton while snapshot loads (#860)

* fix(web): show skeleton in mobile task switcher sheet while snapshot loads When the user opens the mobile task switcher inside a task page before the workflow snapshot has hydrated kanban, the sheet rendered "No tasks yet." instead of a loading state — and stayed that way if the fetch silently failed. useWorkflowSnapshot now drives kanban.isLoading, useTasks exposes it as { tasks, isLoading }, and the sheet wires that into TaskSwitcher's existing skeleton. Fetch errors are surfaced via console.warn so an indefinite empty list isn't silent anymore. * fix(web): guard snapshot finally on cancel and drop redundant subscription Skip the workflow-snapshot fetch's finally block when the effect was superseded by a newer workflowId — otherwise the old fetch's settle would clear the loading flag the new effect just set, briefly flashing the empty-state UI mid-switch. Also drop the redundant kanbanIsLoading subscription in useSheetData (useTasks already encapsulates it) and make the setState mock in use-tasks.test.ts actually mutate state so useWorkflowSnapshot's side effects are exercised by the test. * fix(web): scope snapshot finally clear to the effect that raised the flag If kanban already matched the requested workflowId, the effect skipped raising isLoading=true — but its finally still cleared it on settle, collapsing a skeleton a concurrent caller (e.g. workspace switch) had set independently. Track whether this effect raised the flag and only clear in that case. Also collapse the multi-line useTasks comment per CLAUDE.md style. * fix(web): suppress superseded snapshot fetch warnings Skip the catch-handler console.warn when the effect has been cancelled — a stale fetch shouldn't surface noise after a newer workflow has taken over. * test(web): flush microtasks explicitly in race test waitFor on a never-true assertion resolves immediately, hiding the intent. Use explicit Promise.resolve() flushes to land the superseded fetch's .then/.finally chain before asserting. * style(web): collapse multi-line comments per CLAUDE.md
2026-05-10 17:02:40 +01:00
parent deb12d31b2
commit 21202ef437
6 changed files with 289 additions and 13 deletions
@@ -87,11 +87,10 @@ export function useSheetData(workspaceId: string | null, workflowId: string | nu
  const gitStatusByEnvId = useAppStore((state) => state.gitStatus.byEnvironmentId);
  const envIdBySessionId = useAppStore((state) => state.environmentIdBySessionId);
  const messagesBySession = useAppStore((state) => state.messages.bySession);
-  const { tasks } = useTasks(workflowId);
+  const { tasks, isLoading: tasksLoading } = useTasks(workflowId);
  const steps = useAppStore((state) => state.kanban.steps);
  const workspaces = useAppStore((state) => state.workspaces.items);
  const repositoriesByWorkspace = useAppStore((state) => state.repositories.itemsByWorkspaceId);
-  const kanbanIsLoading = useAppStore((state) => state.kanban.isLoading ?? false);

  const selectedTaskId = useMemo(() => {
    if (activeSessionId) return sessionsById[activeSessionId]?.task_id ?? activeTaskId;
@@ -157,7 +156,8 @@ export function useSheetData(workspaceId: string | null, workflowId: string | nu
    selectedTaskId,
    steps,
    workspaces,
-    kanbanIsLoading,
+    // Skeleton while snapshot hydrates kanban — otherwise shows "No tasks yet." even when tasks exist.
+    tasksLoading,
    tasksWithRepositories,
    dialogSteps,
  };
@@ -114,7 +114,7 @@ export const SessionTaskSwitcherSheet = memo(function SessionTaskSwitcherSheet({
            onArchiveTask={actions.handleArchiveTask}
            onDeleteTask={actions.handleDeleteTask}
            deletingTaskId={actions.deletingTaskId}
-            isLoading={data.kanbanIsLoading}
+            isLoading={data.tasksLoading}
          />
        </div>
      </SheetContent>
@@ -0,0 +1,121 @@
+import { describe, it, expect, vi, beforeEach } from "vitest";
+import { renderHook } from "@testing-library/react";
+
+const mockHydrate = vi.fn();
+const mockSetState = vi.fn();
+const mockFetchWorkflowSnapshot = vi.fn();
+
+type Task = { id: string; title: string };
+type MockState = {
+  connection: { status: string };
+  kanban: { workflowId: string | null; tasks: Task[]; steps: unknown[]; isLoading?: boolean };
+  hydrate: typeof mockHydrate;
+};
+
+let mockState: MockState = {
+  connection: { status: "connected" },
+  kanban: { workflowId: null, tasks: [], steps: [] },
+  hydrate: mockHydrate,
+};
+
+vi.mock("@/components/state-provider", () => ({
+  useAppStore: (selector: (s: MockState) => unknown) => selector(mockState),
+  useAppStoreApi: () => ({
+    getState: () => mockState,
+    setState: (updater: (s: MockState) => MockState) => {
+      // Apply the updater so useWorkflowSnapshot's isLoading transitions are
+      // visible to subsequent reads in the same render cycle. Without this
+      // the mock would silently swallow side effects.
+      mockState = updater(mockState);
+      mockSetState(mockState);
+    },
+  }),
+}));
+
+vi.mock("@/lib/api", () => ({
+  fetchWorkflowSnapshot: (...args: unknown[]) => mockFetchWorkflowSnapshot(...args),
+}));
+
+vi.mock("@/lib/ssr/mapper", () => ({
+  snapshotToState: (snapshot: unknown) => ({ snapshot }),
+}));
+
+import { useTasks } from "./use-tasks";
+
+function setMockState(patch: Partial<MockState["kanban"]> & { workflowId?: string | null }) {
+  mockState = {
+    ...mockState,
+    kanban: { ...mockState.kanban, ...patch },
+  };
+}
+
+describe("useTasks — loading and matching", () => {
+  beforeEach(() => {
+    vi.clearAllMocks();
+    // Never resolves so we observe pre-resolution loading state.
+    mockFetchWorkflowSnapshot.mockReturnValue(new Promise(() => {}));
+    mockState = {
+      connection: { status: "connected" },
+      kanban: { workflowId: null, tasks: [], steps: [], isLoading: false },
+      hydrate: mockHydrate,
+    };
+  });
+
+  it("returns empty list and not-loading when workflowId is null", () => {
+    const { result } = renderHook(() => useTasks(null));
+    expect(result.current.tasks).toEqual([]);
+    expect(result.current.isLoading).toBe(false);
+  });
+
+  it("reports loading once useWorkflowSnapshot has flipped kanban.isLoading", () => {
+    setMockState({ workflowId: null, isLoading: true });
+    const { result } = renderHook(() => useTasks("wf-1"));
+    expect(result.current.tasks).toEqual([]);
+    expect(result.current.isLoading).toBe(true);
+  });
+
+  it("does not report loading when fetch has settled but workflowId mismatches", () => {
+    // Simulates a failed snapshot fetch — `kanban.isLoading` is back to false
+    // and `kanban.workflowId` never advanced to the requested id. Caller
+    // shows the empty-state UI rather than an infinite skeleton.
+    setMockState({ workflowId: null, isLoading: false });
+    const { result } = renderHook(() => useTasks("wf-1"));
+    expect(result.current.tasks).toEqual([]);
+    expect(result.current.isLoading).toBe(false);
+  });
+
+  it("returns tasks and not-loading once kanban.workflowId matches", () => {
+    setMockState({
+      workflowId: "wf-1",
+      tasks: [
+        { id: "t1", title: "One" },
+        { id: "t2", title: "Two" },
+      ],
+      isLoading: false,
+    });
+    const { result } = renderHook(() => useTasks("wf-1"));
+    expect(result.current.tasks).toHaveLength(2);
+    expect(result.current.isLoading).toBe(false);
+  });
+
+  it("filters out tasks that belong to a different workflow snapshot", () => {
+    setMockState({
+      workflowId: "wf-other",
+      tasks: [{ id: "t1", title: "One" }],
+      isLoading: false,
+    });
+    const { result } = renderHook(() => useTasks("wf-1"));
+    expect(result.current.tasks).toEqual([]);
+    expect(result.current.isLoading).toBe(false);
+  });
+
+  it("respects kanban.isLoading even when workflowId already matches", () => {
+    setMockState({
+      workflowId: "wf-1",
+      tasks: [{ id: "t1", title: "One" }],
+      isLoading: true,
+    });
+    const { result } = renderHook(() => useTasks("wf-1"));
+    expect(result.current.isLoading).toBe(true);
+  });
+});
@@ -6,14 +6,14 @@ export function useTasks(workflowId: string | null) {
  useWorkflowSnapshot(workflowId);

  const kanbanWorkflowId = useAppStore((state) => state.kanban.workflowId);
+  const kanbanIsLoading = useAppStore((state) => state.kanban.isLoading ?? false);
  const tasks = useAppStore((state) => state.kanban.tasks);

-  const workflowTasks = useMemo(() => {
-    if (!workflowId || kanbanWorkflowId !== workflowId) {
-      return [];
-    }
-    return tasks;
-  }, [workflowId, kanbanWorkflowId, tasks]);
+  const matchesActive = !!workflowId && kanbanWorkflowId === workflowId;
+  const workflowTasks = useMemo(() => (matchesActive ? tasks : []), [matchesActive, tasks]);

-  return { tasks: workflowTasks };
+  // Loading only while a snapshot fetch is in-flight; settles to false on success/error to avoid an infinite skeleton.
+  const isLoading = !!workflowId && kanbanIsLoading;
+
+  return { tasks: workflowTasks, isLoading };
 }
@@ -0,0 +1,139 @@
+import { describe, it, expect, vi, beforeEach } from "vitest";
+import { renderHook, waitFor } from "@testing-library/react";
+
+const mockHydrate = vi.fn();
+const mockFetchWorkflowSnapshot = vi.fn();
+const mockSetState = vi.fn();
+
+type MockKanban = {
+  workflowId: string | null;
+  tasks: unknown[];
+  steps: unknown[];
+  isLoading?: boolean;
+};
+type MockState = {
+  connection: { status: string };
+  kanban: MockKanban;
+  hydrate: typeof mockHydrate;
+};
+
+let mockState: MockState = {
+  connection: { status: "connected" },
+  kanban: { workflowId: null, tasks: [], steps: [], isLoading: false },
+  hydrate: mockHydrate,
+};
+
+vi.mock("@/components/state-provider", () => ({
+  useAppStore: (selector: (s: MockState) => unknown) => selector(mockState),
+  useAppStoreApi: () => ({
+    getState: () => mockState,
+    setState: (updater: (s: MockState) => MockState) => {
+      const next = updater(mockState);
+      mockSetState(next);
+      mockState = next;
+    },
+  }),
+}));
+
+vi.mock("@/lib/api", () => ({
+  fetchWorkflowSnapshot: (...args: unknown[]) => mockFetchWorkflowSnapshot(...args),
+}));
+
+vi.mock("@/lib/ssr/mapper", () => ({
+  snapshotToState: (snapshot: unknown) => ({ snapshot }),
+}));
+
+import { useWorkflowSnapshot } from "./use-workflow-snapshot";
+
+function resetState(kanban: Partial<MockKanban> = {}) {
+  vi.clearAllMocks();
+  mockState = {
+    connection: { status: "connected" },
+    kanban: {
+      workflowId: null,
+      tasks: [],
+      steps: [],
+      isLoading: false,
+      ...kanban,
+    },
+    hydrate: mockHydrate,
+  };
+}
+
+describe("useWorkflowSnapshot — kanban.isLoading", () => {
+  beforeEach(() => resetState());
+
+  it("flips isLoading true while a fetch for an un-hydrated workflow is in flight", () => {
+    mockFetchWorkflowSnapshot.mockReturnValue(new Promise(() => {}));
+    renderHook(() => useWorkflowSnapshot("wf-1"));
+    expect(mockState.kanban.isLoading).toBe(true);
+  });
+
+  it("flips isLoading back to false after the fetch resolves", async () => {
+    mockFetchWorkflowSnapshot.mockResolvedValue({ steps: [], tasks: [] });
+    renderHook(() => useWorkflowSnapshot("wf-1"));
+    await waitFor(() => expect(mockHydrate).toHaveBeenCalled());
+    await waitFor(() => expect(mockState.kanban.isLoading).toBe(false));
+  });
+
+  it("flips isLoading back to false after the fetch rejects", async () => {
+    mockFetchWorkflowSnapshot.mockRejectedValue(new Error("nope"));
+    renderHook(() => useWorkflowSnapshot("wf-1"));
+    await waitFor(() => expect(mockState.kanban.isLoading).toBe(false));
+    expect(mockHydrate).not.toHaveBeenCalled();
+  });
+
+  it("does not flip isLoading true if the requested workflow is already hydrated", () => {
+    resetState({ workflowId: "wf-1", isLoading: false });
+    mockFetchWorkflowSnapshot.mockReturnValue(new Promise(() => {}));
+    renderHook(() => useWorkflowSnapshot("wf-1"));
+    expect(mockState.kanban.isLoading).toBe(false);
+  });
+
+  it("does not clear isLoading on settle when it didn't raise the flag (already-hydrated re-fetch)", async () => {
+    // Mimic a workspace switch having set isLoading=true after the snapshot
+    // hydrated for this workflowId. A silent re-fetch (e.g. WS reconnect)
+    // must not collapse that skeleton.
+    resetState({ workflowId: "wf-1", isLoading: true });
+    mockFetchWorkflowSnapshot.mockResolvedValue({ steps: [], tasks: [] });
+    renderHook(() => useWorkflowSnapshot("wf-1"));
+    await waitFor(() => expect(mockHydrate).toHaveBeenCalled());
+    expect(mockState.kanban.isLoading).toBe(true);
+  });
+
+  it("does nothing when workflowId is null", () => {
+    renderHook(() => useWorkflowSnapshot(null));
+    expect(mockFetchWorkflowSnapshot).not.toHaveBeenCalled();
+    expect(mockState.kanban.isLoading).toBe(false);
+  });
+
+  it("does not clear isLoading when an old fetch settles after workflowId changes", async () => {
+    // First fetch never settles synchronously — we resolve it manually
+    // *after* re-rendering with a new workflowId, simulating the race where
+    // the user switches workflows mid-fetch.
+    let resolveFirst!: (snapshot: { steps: unknown[]; tasks: unknown[] }) => void;
+    const firstFetch = new Promise<{ steps: unknown[]; tasks: unknown[] }>((r) => {
+      resolveFirst = r;
+    });
+    const secondFetch = new Promise<{ steps: unknown[]; tasks: unknown[] }>(() => {});
+    mockFetchWorkflowSnapshot.mockReturnValueOnce(firstFetch).mockReturnValueOnce(secondFetch);
+
+    const { rerender } = renderHook(({ id }: { id: string | null }) => useWorkflowSnapshot(id), {
+      initialProps: { id: "wf-1" as string | null },
+    });
+    expect(mockState.kanban.isLoading).toBe(true);
+
+    // User switches to wf-2 before wf-1 finishes loading
+    rerender({ id: "wf-2" });
+    expect(mockState.kanban.isLoading).toBe(true);
+
+    // Old fetch lands now; flush its .then/.finally microtask chain so we
+    // can assert the negatives without relying on waitFor (which would
+    // resolve immediately for never-true assertions).
+    resolveFirst({ steps: [], tasks: [] });
+    await Promise.resolve();
+    await Promise.resolve();
+    expect(mockHydrate).not.toHaveBeenCalled();
+    expect(mockState.kanban.isLoading).toBe(true);
+  });
+});
@@ -9,12 +9,28 @@ export function useWorkflowSnapshot(workflowId: string | null) {

  useEffect(() => {
    if (!workflowId) return;
+    let cancelled = false;
+    const setLoading = store.getState().kanban.workflowId !== workflowId;
+    if (setLoading) {
+      store.setState((state) => ({ ...state, kanban: { ...state.kanban, isLoading: true } }));
+    }
    fetchWorkflowSnapshot(workflowId, { cache: "no-store" })
      .then((snapshot) => {
+        if (cancelled) return;
        store.getState().hydrate(snapshotToState(snapshot));
      })
-      .catch(() => {
-        // Ignore snapshot errors — will retry on WS reconnect.
+      .catch((error) => {
+        // Suppress superseded-fetch noise; retry happens on WS reconnect.
+        if (cancelled) return;
+        console.warn("[useWorkflowSnapshot] failed to load snapshot:", error);
+      })
+      .finally(() => {
+        // Only clear the flag this effect raised; skip when cancelled or when a concurrent caller owns it.
+        if (cancelled || !setLoading) return;
+        store.setState((state) => ({ ...state, kanban: { ...state.kanban, isLoading: false } }));
      });
+    return () => {
+      cancelled = true;
+    };
  }, [workflowId, store, connectionStatus]);
 }