fix: refresh sidebar tasks when the active workspace changes (#1564)
This commit is contained in:
@@ -71,6 +71,10 @@ vi.mock("@/hooks/use-in-office", () => ({
|
||||
useInOffice: () => officeRouteMock.inOffice,
|
||||
}));
|
||||
|
||||
vi.mock("@/hooks/use-workflows", () => ({
|
||||
useEnsureWorkspaceWorkflows: () => {},
|
||||
}));
|
||||
|
||||
const storeState = {
|
||||
appSidebar: {
|
||||
collapsed: false,
|
||||
|
||||
@@ -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();
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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);
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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<Workflow[]>(initialWorkflows);
|
||||
|
||||
@@ -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()));
|
||||
});
|
||||
});
|
||||
@@ -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 };
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user