From e780d807090b60602a37d5afa6573c95fb03108f Mon Sep 17 00:00:00 2001 From: StefanFVogel <58847673+StefanFVogel@users.noreply.github.com> Date: Wed, 1 Jul 2026 16:33:02 +0200 Subject: [PATCH] fix: refresh sidebar tasks when the active workspace changes (#1564) --- .../app-sidebar/app-sidebar.test.tsx | 4 + .../components/app-sidebar/app-sidebar.tsx | 8 + .../hooks/domains/kanban/use-kanban-data.ts | 6 +- .../kanban/use-workspace-sidebar-tasks.ts | 6 + .../settings/use-workflow-settings.test.ts | 24 +++ .../domains/settings/use-workflow-settings.ts | 8 +- apps/web/hooks/use-workflows.test.ts | 157 ++++++++++++++++++ apps/web/hooks/use-workflows.ts | 45 ++++- 8 files changed, 248 insertions(+), 10 deletions(-) create mode 100644 apps/web/hooks/use-workflows.test.ts diff --git a/apps/web/components/app-sidebar/app-sidebar.test.tsx b/apps/web/components/app-sidebar/app-sidebar.test.tsx index 1414895b18..b86d78ba69 100644 --- a/apps/web/components/app-sidebar/app-sidebar.test.tsx +++ b/apps/web/components/app-sidebar/app-sidebar.test.tsx @@ -71,6 +71,10 @@ vi.mock("@/hooks/use-in-office", () => ({ useInOffice: () => officeRouteMock.inOffice, })); +vi.mock("@/hooks/use-workflows", () => ({ + useEnsureWorkspaceWorkflows: () => {}, +})); + const storeState = { appSidebar: { collapsed: false, diff --git a/apps/web/components/app-sidebar/app-sidebar.tsx b/apps/web/components/app-sidebar/app-sidebar.tsx index 5aa312d151..963c55a67c 100644 --- a/apps/web/components/app-sidebar/app-sidebar.tsx +++ b/apps/web/components/app-sidebar/app-sidebar.tsx @@ -3,6 +3,7 @@ import { useCallback, useEffect, useRef } from "react"; import { usePathname } from "@/lib/routing/client-router"; import { useAppStore } from "@/components/state-provider"; +import { useEnsureWorkspaceWorkflows } from "@/hooks/use-workflows"; import { useInOffice } from "@/hooks/use-in-office"; import { cn } from "@/lib/utils"; import { @@ -62,6 +63,13 @@ export function AppSidebar() { const pathname = usePathname(); const inOffice = useInOffice(); + // Keep `state.workflows.items` in sync with the active workspace at the top + // of the always-mounted sidebar. Downstream consumers (the workspace picker, + // Tasks section, kanban board) all assume this state exists for the current + // workspace — hoisting it above the collapsible Tasks section is required so + // a user with the section collapsed still gets fresh workflows on a switch. + useEnsureWorkspaceWorkflows(); + const handleResize = useCallback( (e: React.MouseEvent) => { e.preventDefault(); diff --git a/apps/web/hooks/domains/kanban/use-kanban-data.ts b/apps/web/hooks/domains/kanban/use-kanban-data.ts index b828d489e6..05338816be 100644 --- a/apps/web/hooks/domains/kanban/use-kanban-data.ts +++ b/apps/web/hooks/domains/kanban/use-kanban-data.ts @@ -2,7 +2,6 @@ import { useMemo, useSyncExternalStore } from "react"; import { useAppStore } from "@/components/state-provider"; -import { useWorkflows } from "@/hooks/use-workflows"; import { useWorkflowSnapshot } from "@/hooks/use-workflow-snapshot"; import { useUserDisplaySettings } from "@/hooks/use-user-display-settings"; import { filterTasksByRepositories } from "@/lib/kanban/filters"; @@ -26,8 +25,9 @@ export function useKanbanData({ const enablePreviewOnClick = useAppStore((state) => state.userSettings.enablePreviewOnClick); const repositoriesByWorkspace = useAppStore((state) => state.repositories.itemsByWorkspaceId); - // Data fetching hooks - useWorkflows(workspaceState.activeId, true); + // Data fetching hooks. `state.workflows.items` is loaded by `AppSidebar` via + // `useEnsureWorkspaceWorkflows` (unconditional, above any collapsible), so + // the kanban page only needs the workflow snapshot here. useWorkflowSnapshot(workflowsState.activeId); // User settings hook diff --git a/apps/web/hooks/domains/kanban/use-workspace-sidebar-tasks.ts b/apps/web/hooks/domains/kanban/use-workspace-sidebar-tasks.ts index 0f1c3ad2ae..d3a5d58044 100644 --- a/apps/web/hooks/domains/kanban/use-workspace-sidebar-tasks.ts +++ b/apps/web/hooks/domains/kanban/use-workspace-sidebar-tasks.ts @@ -20,6 +20,12 @@ export type WorkspaceSidebarTasksResult = AggregatedSidebarTasks & { * single active `kanban` slice for tasks that arrived via WS before their * snapshot resolved). Snapshots from other workspaces are filtered out so a * stale hydration doesn't leak across workspace switches. + * + * Assumes `state.workflows.items` is kept in sync with the active workspace by + * an always-mounted caller (`useEnsureWorkspaceWorkflows` from `AppSidebar`). + * Do not add the fetch back here — this hook only runs when the Tasks section + * accordion is expanded, so co-locating the fetch would recreate the original + * "sidebar stale after workspace switch" bug for collapsed-section users. */ export function useWorkspaceSidebarTasks(workspaceId: string | null): WorkspaceSidebarTasksResult { useAllWorkflowSnapshots(workspaceId); diff --git a/apps/web/hooks/domains/settings/use-workflow-settings.test.ts b/apps/web/hooks/domains/settings/use-workflow-settings.test.ts index 22a23dbb39..64092b42c7 100644 --- a/apps/web/hooks/domains/settings/use-workflow-settings.test.ts +++ b/apps/web/hooks/domains/settings/use-workflow-settings.test.ts @@ -12,6 +12,7 @@ type StoreWorkflow = { name: string; description?: string | null; hidden?: boolean; + style?: "kanban" | "office" | "custom"; }; type MockState = { workflows: { items: StoreWorkflow[] } }; @@ -46,6 +47,29 @@ beforeEach(() => { setStore([]); }); +describe("useWorkflowSettings — store boundary filters", () => { + it("does not merge office-style workflows from the global store", () => { + // The sidebar's `useEnsureWorkspaceWorkflows` populates the store with every + // workflow — including office-style ones — on all routes. The settings UI + // is kanban-only (ADR-0004), so office workflows must be dropped at the + // store boundary (matching the SSR-side filter). Otherwise they land in + // `workflowItems`, which is what "Export All" serialises → workflow-import- + // export e2e regresses. + const officeInB: StoreWorkflow = { + id: "wf-b-office", + workspaceId: "ws-b", + name: "Office Only Workflow", + style: "office", + }; + setStore([officeInB]); + + const { result } = renderHook(() => useWorkflowSettings([], "ws-b")); + + expect(result.current.workflowItems).toHaveLength(0); + expect(result.current.savedWorkflowItems).toHaveLength(0); + }); +}); + describe("useWorkflowSettings", () => { it("does not include workflows from other workspaces present in the global store", () => { // Store has a workflow from workspace A (e.g. user previously visited it) diff --git a/apps/web/hooks/domains/settings/use-workflow-settings.ts b/apps/web/hooks/domains/settings/use-workflow-settings.ts index e73efba5f4..2c0e941cf0 100644 --- a/apps/web/hooks/domains/settings/use-workflow-settings.ts +++ b/apps/web/hooks/domains/settings/use-workflow-settings.ts @@ -17,10 +17,12 @@ export function useWorkflowSettings(initialWorkflows: Workflow[], workspaceId?: // Hidden workflows (e.g. the system "Improve Kandev" template) are loaded // into the global store with `includeHidden: true` so the kanban can resolve // them when a task references one, but they must never surface in the - // settings management UI. Filter them out at the store boundary so all - // downstream merging logic remains hidden-agnostic. + // settings management UI. Office-style workflows are managed from the Office + // surface (ADR-0004) and must be excluded the same way — the SSR-side filter + // in `workspace-workflows-client.tsx` already drops them; the store-boundary + // filter must match so live WS/fetch updates cannot merge them back in. const scopedStoreWorkflows = useMemo(() => { - const visible = storeWorkflows.filter((w) => !w.hidden); + const visible = storeWorkflows.filter((w) => !w.hidden && w.style !== "office"); return workspaceId ? visible.filter((w) => w.workspaceId === workspaceId) : visible; }, [storeWorkflows, workspaceId]); const [workflowItems, setWorkflowItems] = useState(initialWorkflows); diff --git a/apps/web/hooks/use-workflows.test.ts b/apps/web/hooks/use-workflows.test.ts new file mode 100644 index 0000000000..a9084f6dfd --- /dev/null +++ b/apps/web/hooks/use-workflows.test.ts @@ -0,0 +1,157 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; +import { renderHook, waitFor } from "@testing-library/react"; + +const mockListWorkflows = vi.fn(); +const mockSetWorkflows = vi.fn(); + +type MockState = { + workflows: { items: Array<{ id: string; workspaceId: string; name: string }> }; + workspaces: { activeId: string | null }; + setWorkflows: typeof mockSetWorkflows; +}; + +let mockState: MockState = { + workflows: { items: [] }, + workspaces: { activeId: null }, + setWorkflows: mockSetWorkflows, +}; + +vi.mock("@/components/state-provider", () => ({ + useAppStore: (selector: (s: MockState) => unknown) => selector(mockState), +})); + +vi.mock("@/lib/api", () => ({ + listWorkflows: (...args: unknown[]) => mockListWorkflows(...args), +})); + +import { useWorkflows, useEnsureWorkspaceWorkflows } from "./use-workflows"; + +function makeWorkflow(id: string, workspaceId: string) { + return { + id, + workspace_id: workspaceId, + name: id, + description: null, + sort_order: 0, + agent_profile_id: null, + hidden: false, + style: null, + }; +} + +describe("useWorkflows — stale response guard", () => { + beforeEach(() => { + vi.clearAllMocks(); + mockState = { + workflows: { items: [] }, + workspaces: { activeId: null }, + setWorkflows: mockSetWorkflows, + }; + }); + + it("discards a stale in-flight response when the workspace switches mid-fetch", async () => { + let resolveStale: (v: unknown) => void = () => {}; + mockListWorkflows.mockImplementationOnce( + () => + new Promise((res) => { + resolveStale = res; + }), + ); + + const { rerender } = renderHook( + ({ workspaceId }: { workspaceId: string | null }) => useWorkflows(workspaceId, true), + { initialProps: { workspaceId: "ws-A" } }, + ); + await waitFor(() => expect(mockListWorkflows).toHaveBeenCalledWith("ws-A", expect.anything())); + + // Switch to workspace B before A's fetch resolves; B resolves first. + mockListWorkflows.mockResolvedValueOnce({ workflows: [makeWorkflow("wf-B", "ws-B")] }); + rerender({ workspaceId: "ws-B" }); + await waitFor(() => + expect(mockSetWorkflows).toHaveBeenCalledWith([expect.objectContaining({ id: "wf-B" })]), + ); + + // Now let A resolve. It must NOT overwrite the store with A's workflows. + resolveStale({ workflows: [makeWorkflow("wf-A", "ws-A")] }); + for (let i = 0; i < 5; i++) await Promise.resolve(); + + const written = mockSetWorkflows.mock.calls.map((call) => call[0]); + const wroteA = written.some( + (list: Array<{ id: string }>) => list.length > 0 && list.some((w) => w.id === "wf-A"), + ); + expect(wroteA).toBe(false); + }); + + it("does not clear workflows when a stale fetch fails after the workspace switched", async () => { + let rejectStale: (e: Error) => void = () => {}; + mockListWorkflows.mockImplementationOnce( + () => + new Promise((_res, rej) => { + rejectStale = rej; + }), + ); + + const { rerender } = renderHook( + ({ workspaceId }: { workspaceId: string | null }) => useWorkflows(workspaceId, true), + { initialProps: { workspaceId: "ws-A" } }, + ); + await waitFor(() => expect(mockListWorkflows).toHaveBeenCalledWith("ws-A", expect.anything())); + + // Switch to workspace B; B resolves with data. + mockListWorkflows.mockResolvedValueOnce({ workflows: [makeWorkflow("wf-B", "ws-B")] }); + rerender({ workspaceId: "ws-B" }); + await waitFor(() => + expect(mockSetWorkflows).toHaveBeenCalledWith([expect.objectContaining({ id: "wf-B" })]), + ); + + // A's fetch fails after B already succeeded. Catch must NOT wipe the store. + rejectStale(new Error("network")); + for (let i = 0; i < 5; i++) await Promise.resolve(); + + const cleared = mockSetWorkflows.mock.calls.some( + (call) => Array.isArray(call[0]) && call[0].length === 0, + ); + expect(cleared).toBe(false); + }); + + it("does not clear hydrated workflows when the fetch for the current workspace fails", async () => { + // The sidebar mounts on every route (task detail, settings, ...) and boot + // hydrates `state.workflows.items` before the sidebar's refresh fetch + // fires. If that fetch flakes, blowing the store away leaves the sidebar, + // board, and kanban scoping with no workflow IDs until another success. + mockListWorkflows.mockRejectedValueOnce(new Error("network")); + + renderHook(() => useWorkflows("ws-A", true)); + + await waitFor(() => expect(mockListWorkflows).toHaveBeenCalledWith("ws-A", expect.anything())); + for (let i = 0; i < 5; i++) await Promise.resolve(); + + expect(mockSetWorkflows).not.toHaveBeenCalled(); + }); +}); + +describe("useEnsureWorkspaceWorkflows", () => { + beforeEach(() => { + vi.clearAllMocks(); + mockListWorkflows.mockResolvedValue({ workflows: [] }); + mockState = { + workflows: { items: [] }, + workspaces: { activeId: "ws-A" }, + setWorkflows: mockSetWorkflows, + }; + }); + + it("fetches workflows for the store's active workspace on mount", async () => { + renderHook(() => useEnsureWorkspaceWorkflows()); + await waitFor(() => expect(mockListWorkflows).toHaveBeenCalledWith("ws-A", expect.anything())); + }); + + it("re-fetches when the store's active workspace changes", async () => { + const { rerender } = renderHook(() => useEnsureWorkspaceWorkflows()); + await waitFor(() => expect(mockListWorkflows).toHaveBeenCalledWith("ws-A", expect.anything())); + + mockState = { ...mockState, workspaces: { activeId: "ws-B" } }; + rerender(); + await waitFor(() => expect(mockListWorkflows).toHaveBeenCalledWith("ws-B", expect.anything())); + }); +}); diff --git a/apps/web/hooks/use-workflows.ts b/apps/web/hooks/use-workflows.ts index 9c399c8c6b..08559e7c76 100644 --- a/apps/web/hooks/use-workflows.ts +++ b/apps/web/hooks/use-workflows.ts @@ -1,15 +1,28 @@ import { useEffect } from "react"; import { useAppStore } from "@/components/state-provider"; import { listWorkflows } from "@/lib/api"; +import type { WorkflowsState } from "@/lib/state/slices"; -export function useWorkflows(workspaceId: string | null, enabled = true) { - const workflows = useAppStore((state) => state.workflows.items); - const setWorkflows = useAppStore((state) => state.setWorkflows); +type StoreWorkflow = WorkflowsState["items"][number]; +type SetWorkflows = (workflows: StoreWorkflow[]) => void; +/** + * Fire-and-forget fetch effect. Kept internal so callers that only need to + * populate `state.workflows.items` (e.g. `useEnsureWorkspaceWorkflows`) don't + * also subscribe to the store slice they wrote to — that would re-render the + * caller on every fetch and defeats the "top-level layout" placement. + */ +function useWorkflowsFetchEffect( + workspaceId: string | null, + enabled: boolean, + setWorkflows: SetWorkflows, +) { useEffect(() => { if (!enabled || !workspaceId) return; + let cancelled = false; listWorkflows(workspaceId, { cache: "no-store", includeHidden: true }) .then((response) => { + if (cancelled) return; const mapped = response.workflows.map((workflow) => ({ id: workflow.id, workspaceId: workflow.workspace_id, @@ -22,8 +35,32 @@ export function useWorkflows(workspaceId: string | null, enabled = true) { })); setWorkflows(mapped); }) - .catch(() => setWorkflows([])); + // Do not clear on error — the sidebar mounts on every route, and boot + // hydrates workflows before the refresh fires. Blowing the slice away on + // a network flake would leave the sidebar and board with no workflow IDs + // until another success. The next successful fetch replaces the slice. + .catch(() => {}); + return () => { + cancelled = true; + }; }, [enabled, setWorkflows, workspaceId]); +} +/** + * Load workflows for the active workspace. Call from a component that stays + * mounted independently of any collapsible section, so `state.workflows.items` + * follows the active workspace even when the sidebar's Tasks section is + * collapsed and its children (which consume workflows) are unmounted. + */ +export function useEnsureWorkspaceWorkflows() { + const workspaceId = useAppStore((state) => state.workspaces.activeId); + const setWorkflows = useAppStore((state) => state.setWorkflows); + useWorkflowsFetchEffect(workspaceId, true, setWorkflows); +} + +export function useWorkflows(workspaceId: string | null, enabled = true) { + const workflows = useAppStore((state) => state.workflows.items); + const setWorkflows = useAppStore((state) => state.setWorkflows); + useWorkflowsFetchEffect(workspaceId, enabled, setWorkflows); return { workflows }; }