From 49c2de108b02924ba51bde0eee89e41f87f903a7 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Wed, 24 Jun 2026 01:13:33 -0700 Subject: [PATCH] test(dashboard): harden hook tests + drop dead App.tsx re-exports (code review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address code-review findings on the App.tsx module-breakup: - Add behavior-preservation tests: useBoardScrollRestore real double-rAF restore path; useApprovalBanner clear-on-leave-awaiting + mailbox-refresh dedup; useCapacityRiskBanner threshold-change-clears + dismiss-persist assertion; useChatUnreadBadge room-message + cross-project filter; and a new sseSplitIntegration test co-mounting useMailboxUnread + useApprovalBanner to pin the KTD4 SSE split (no double-fire; awaiting-approval refresh exactly once via callback). - Add unmount/teardown assertions (stash interval clear, auth-token listener removal, dashboard-health cancelled flag). - Drop dead type-only re-exports (ApprovalBannerCandidate, CliActionDeps) from App.tsx — zero importers (verified). All hook tests green; App.test.tsx unchanged (5 pre-existing). typecheck + eslint clean. No production behavior change. --- packages/dashboard/app/App.tsx | 2 - .../__tests__/sseSplitIntegration.test.ts | 106 ++++++++++++++++++ .../hooks/__tests__/useApprovalBanner.test.ts | 60 ++++++++++ .../__tests__/useAuthTokenRecovery.test.ts | 20 +++- .../__tests__/useBoardScrollRestore.test.ts | 59 +++++++++- .../__tests__/useCapacityRiskBanner.test.ts | 21 +++- .../__tests__/useChatUnreadBadge.test.ts | 35 ++++++ .../__tests__/useDashboardHealth.test.ts | 23 ++++ .../__tests__/useStashOrphanCount.test.ts | 18 +++ 9 files changed, 336 insertions(+), 8 deletions(-) create mode 100644 packages/dashboard/app/hooks/__tests__/sseSplitIntegration.test.ts diff --git a/packages/dashboard/app/App.tsx b/packages/dashboard/app/App.tsx index beb4a7fb8a..6d9ae38755 100644 --- a/packages/dashboard/app/App.tsx +++ b/packages/dashboard/app/App.tsx @@ -101,8 +101,6 @@ export { isSessionNeedingInputForBanner, getCliActionDisabledReasonForBanner, executeCliSessionBannerAction, - type ApprovalBannerCandidate, - type CliActionDeps, } from "./utils/appLifecycle"; import { subscribeSse } from "./sse-bus"; import { AuthTokenRecoveryDialog } from "./components/AuthTokenRecoveryDialog"; diff --git a/packages/dashboard/app/hooks/__tests__/sseSplitIntegration.test.ts b/packages/dashboard/app/hooks/__tests__/sseSplitIntegration.test.ts new file mode 100644 index 0000000000..1278129acf --- /dev/null +++ b/packages/dashboard/app/hooks/__tests__/sseSplitIntegration.test.ts @@ -0,0 +1,106 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; +import { renderHook, act } from "@testing-library/react"; +import type { Task } from "@fusion/core"; + +interface CapturedSubscription { + url: string; + onReconnect?: () => void; + events: Record void>; +} + +const { subscriptions } = vi.hoisted(() => ({ + subscriptions: [] as CapturedSubscription[], +})); + +vi.mock("../../sse-bus", () => ({ + subscribeSse: vi.fn( + ( + url: string, + sub: { onReconnect?: () => void; events: Record void> }, + ) => { + subscriptions.push({ url, onReconnect: sub.onReconnect, events: { ...sub.events } }); + return () => {}; + }, + ), +})); + +const fetchUnreadCount = vi.fn(async () => ({ unreadCount: 0 })); +vi.mock("../../api", () => ({ + fetchUnreadCount: (...a: unknown[]) => fetchUnreadCount(...a), +})); + +import { useMailboxUnread } from "../useMailboxUnread"; +import { useApprovalBanner } from "../useApprovalBanner"; + +function msg(data: object): MessageEvent { + return { data: JSON.stringify(data) } as MessageEvent; +} + +describe("SSE split (KTD4): mailbox-refresh vs approval-banner", () => { + beforeEach(() => { + subscriptions.length = 0; + fetchUnreadCount.mockReset(); + fetchUnreadCount.mockResolvedValue({ unreadCount: 0 }); + }); + + it("co-mount keeps the awaiting-approval refresh single-fired and the banner independent", async () => { + const mailboxSpy = vi.fn(); + const tasks: Task[] = []; + const onStarPrompt = vi.fn(); + + // Two independent mounts → two subscribeSse calls captured separately so + // the split handlers never overwrite each other. + renderHook(() => useMailboxUnread("p1")); + const approval = renderHook(() => + useApprovalBanner({ + tasks, + currentProjectId: "p1", + gitHubStarPromptShown: true, + onStarPrompt, + onMailboxRefresh: mailboxSpy, + }), + ); + + // Drain the mailbox hook's mount fetch so no setState leaks past the test. + await act(async () => { + await Promise.resolve(); + await Promise.resolve(); + }); + + // Distinguish the two subscriptions: mailbox listens to message:sent, + // the banner listens to task:updated. + const mailboxSub = subscriptions.find((s) => "message:sent" in s.events); + const approvalSub = subscriptions.find((s) => "task:updated" in s.events); + expect(mailboxSub).toBeTruthy(); + expect(approvalSub).toBeTruthy(); + + // (i) approval:requested sets the banner candidate but does NOT fire + // mailbox-refresh; the mailbox hook's approval:requested handler + // (count refresh) is a distinct function from the banner's. + act(() => { + approvalSub!.events["approval:requested"]?.(msg({ id: "a1", updatedAt: "2026-01-01T00:00:00Z" })); + }); + expect(approval.result.current.candidate?.dedupeKey).toBe("approval:a1"); + expect(mailboxSpy).not.toHaveBeenCalled(); + expect(mailboxSub!.events["approval:requested"]).toBeTruthy(); + expect(mailboxSub!.events["approval:requested"]).not.toBe(approvalSub!.events["approval:requested"]); + + // (ii) task:updated → awaiting-approval sets the candidate + fires the + // mailbox refresh exactly once. + act(() => { + approvalSub!.events["task:updated"]?.( + msg({ id: "t1", status: "awaiting-approval", updatedAt: "2026-01-02T00:00:00Z" }), + ); + }); + expect(approval.result.current.candidate?.dedupeKey).toBe("task:t1"); + expect(mailboxSpy).toHaveBeenCalledTimes(1); + + // (iii) a second awaiting-approval for the same task is deduped — no second refresh. + act(() => { + approvalSub!.events["task:updated"]?.( + msg({ id: "t1", status: "awaiting-approval", updatedAt: "2026-01-03T00:00:00Z" }), + ); + }); + expect(mailboxSpy).toHaveBeenCalledTimes(1); + }); +}); diff --git a/packages/dashboard/app/hooks/__tests__/useApprovalBanner.test.ts b/packages/dashboard/app/hooks/__tests__/useApprovalBanner.test.ts index a100879646..d28d24e805 100644 --- a/packages/dashboard/app/hooks/__tests__/useApprovalBanner.test.ts +++ b/packages/dashboard/app/hooks/__tests__/useApprovalBanner.test.ts @@ -136,4 +136,64 @@ describe("useApprovalBanner", () => { }); expect(result.current.candidate).toBeNull(); }); + it("re-triggers after leaving and re-entering awaiting-approval (clear-on-leave)", () => { + const onMailboxRefresh = vi.fn(); + const seedTasks: Task[] = [task("t1", "awaiting-approval")]; + const { result } = renderHook(() => + useApprovalBanner({ + tasks: seedTasks, + currentProjectId: "p1", + gitHubStarPromptShown: true, + onStarPrompt: vi.fn(), + onMailboxRefresh, + }), + ); + + // The seeded awaiting-approval task is already in the seen set, so a repeat + // event for it must NOT trigger. + act(() => { + handlers["task:updated"]?.(msg({ id: "t1", status: "awaiting-approval", updatedAt: "2026-01-01T00:00:00Z" })); + }); + expect(result.current.candidate).toBeNull(); + expect(onMailboxRefresh).not.toHaveBeenCalled(); + + // Task leaves awaiting-approval → the seen-key for t1 is cleared. + act(() => { + handlers["task:updated"]?.(msg({ id: "t1", status: "approved", updatedAt: "2026-01-02T00:00:00Z" })); + }); + expect(result.current.candidate).toBeNull(); + + // Re-entering awaiting-approval re-triggers the candidate + mailbox refresh. + act(() => { + handlers["task:updated"]?.(msg({ id: "t1", status: "awaiting-approval", updatedAt: "2026-01-03T00:00:00Z" })); + }); + expect(result.current.candidate?.dedupeKey).toBe("task:t1"); + expect(onMailboxRefresh).toHaveBeenCalledTimes(1); + }); + + it("dedupes mailbox refresh on a repeated awaiting-approval task:updated", () => { + const onMailboxRefresh = vi.fn(); + const tasks: Task[] = []; + const { result } = renderHook(() => + useApprovalBanner({ + tasks, + currentProjectId: "p1", + gitHubStarPromptShown: true, + onStarPrompt: vi.fn(), + onMailboxRefresh, + }), + ); + + act(() => { + handlers["task:updated"]?.(msg({ id: "t1", status: "awaiting-approval", updatedAt: "2026-01-01T00:00:00Z" })); + }); + expect(result.current.candidate?.dedupeKey).toBe("task:t1"); + expect(onMailboxRefresh).toHaveBeenCalledTimes(1); + + // A second awaiting-approval for the same task is suppressed by seenApprovalKeys. + act(() => { + handlers["task:updated"]?.(msg({ id: "t1", status: "awaiting-approval", updatedAt: "2026-01-04T00:00:00Z" })); + }); + expect(onMailboxRefresh).toHaveBeenCalledTimes(1); + }); }); diff --git a/packages/dashboard/app/hooks/__tests__/useAuthTokenRecovery.test.ts b/packages/dashboard/app/hooks/__tests__/useAuthTokenRecovery.test.ts index 799dbb1b30..54eec1cde2 100644 --- a/packages/dashboard/app/hooks/__tests__/useAuthTokenRecovery.test.ts +++ b/packages/dashboard/app/hooks/__tests__/useAuthTokenRecovery.test.ts @@ -1,9 +1,12 @@ -import { describe, expect, it } from "vitest"; +import { afterEach, describe, expect, it, vi } from "vitest"; import { renderHook, act } from "@testing-library/react"; import { AUTH_TOKEN_RECOVERY_REQUIRED_EVENT } from "../../auth"; import { useAuthTokenRecovery } from "../useAuthTokenRecovery"; describe("useAuthTokenRecovery", () => { + afterEach(() => { + vi.restoreAllMocks(); + }); it("opens when the daemon auth-failure event fires", () => { const { result } = renderHook(() => useAuthTokenRecovery()); @@ -15,4 +18,19 @@ describe("useAuthTokenRecovery", () => { expect(result.current.open).toBe(true); }); + it("removes the daemon auth-failure listener on unmount", () => { + const addSpy = vi.spyOn(window, "addEventListener"); + const removeSpy = vi.spyOn(window, "removeEventListener"); + const { unmount } = renderHook(() => useAuthTokenRecovery()); + + const addedCall = addSpy.mock.calls.find( + ([type]) => type === AUTH_TOKEN_RECOVERY_REQUIRED_EVENT, + ); + expect(addedCall).toBeTruthy(); + const addedHandler = addedCall![1] as EventListener; + + unmount(); + + expect(removeSpy).toHaveBeenCalledWith(AUTH_TOKEN_RECOVERY_REQUIRED_EVENT, addedHandler); + }); }); diff --git a/packages/dashboard/app/hooks/__tests__/useBoardScrollRestore.test.ts b/packages/dashboard/app/hooks/__tests__/useBoardScrollRestore.test.ts index da0ccd12c7..8da0ee7f93 100644 --- a/packages/dashboard/app/hooks/__tests__/useBoardScrollRestore.test.ts +++ b/packages/dashboard/app/hooks/__tests__/useBoardScrollRestore.test.ts @@ -1,13 +1,29 @@ -import { describe, expect, it, vi } from "vitest"; -import { renderHook } from "@testing-library/react"; -import { useBoardScrollRestore } from "../useBoardScrollRestore"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { renderHook, act } from "@testing-library/react"; +import type { TaskView } from "../useViewState"; vi.mock("../../utils/boardScrollSnapshot", () => ({ - captureBoardScrollSnapshot: vi.fn(() => ({ x: 10, columns: {} })), + captureBoardScrollSnapshot: vi.fn(), restoreBoardScrollSnapshot: vi.fn(() => true), })); +import { captureBoardScrollSnapshot, restoreBoardScrollSnapshot } from "../../utils/boardScrollSnapshot"; +import { useBoardScrollRestore } from "../useBoardScrollRestore"; + +const mockedCapture = vi.mocked(captureBoardScrollSnapshot); +const mockedRestore = vi.mocked(restoreBoardScrollSnapshot); + describe("useBoardScrollRestore", () => { + beforeEach(() => { + mockedCapture.mockReset(); + mockedRestore.mockReset(); + mockedRestore.mockReturnValue(true); + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + it("exposes capture and requestRestore without throwing", () => { const { result } = renderHook(() => useBoardScrollRestore("board")); @@ -16,4 +32,39 @@ describe("useBoardScrollRestore", () => { expect(() => result.current.capture()).not.toThrow(); expect(() => result.current.requestRestore()).not.toThrow(); }); + + it("restores the captured snapshot after returning to the board view", () => { + const sentinel = { boardLeft: 42, boardTop: 7, columnTops: { c1: 3 } }; + mockedCapture.mockReturnValue(sentinel); + + // Make the double requestAnimationFrame fire synchronously so the restore + // lands inside the act() that commits the board-view effect. + vi.spyOn(window, "requestAnimationFrame").mockImplementation((cb: FrameRequestCallback) => { + cb(0); + return 0; + }); + + const { result, rerender } = renderHook( + ({ taskView }: { taskView: TaskView }) => useBoardScrollRestore(taskView), + { initialProps: { taskView: "task-detail" } }, + ); + + // Off the board with nothing pending → no restore yet. + expect(mockedRestore).not.toHaveBeenCalled(); + + act(() => { + result.current.capture(); + result.current.requestRestore(); + }); + + // Restore waits for the view to return to "board". + expect(mockedRestore).not.toHaveBeenCalled(); + + act(() => { + rerender({ taskView: "board" }); + }); + + expect(mockedRestore).toHaveBeenCalledTimes(1); + expect(mockedRestore).toHaveBeenCalledWith(sentinel); + }); }); diff --git a/packages/dashboard/app/hooks/__tests__/useCapacityRiskBanner.test.ts b/packages/dashboard/app/hooks/__tests__/useCapacityRiskBanner.test.ts index c830f4a987..dce8981d14 100644 --- a/packages/dashboard/app/hooks/__tests__/useCapacityRiskBanner.test.ts +++ b/packages/dashboard/app/hooks/__tests__/useCapacityRiskBanner.test.ts @@ -7,7 +7,7 @@ vi.mock("../../utils/projectStorage", () => ({ removeScopedItem: vi.fn(), })); -import { getScopedItem, removeScopedItem } from "../../utils/projectStorage"; +import { getScopedItem, removeScopedItem, setScopedItem } from "../../utils/projectStorage"; import { useCapacityRiskBanner } from "../useCapacityRiskBanner"; const base = { @@ -41,6 +41,7 @@ describe("useCapacityRiskBanner", () => { }); expect(result.current.dismissed).toBe(true); + expect(setScopedItem).toHaveBeenCalledWith(expect.any(String), "true", "p1"); }); it("clears a prior dismissal when the banner is re-enabled after hydrate", () => { @@ -58,6 +59,24 @@ describe("useCapacityRiskBanner", () => { // Re-enabling the banner resurrects the dismissed banner. rerender({ enabled: true }); + expect(removeScopedItem).toHaveBeenCalledWith(expect.any(String), "p1"); + expect(result.current.dismissed).toBe(false); + }); + it("clears a prior dismissal when the todo threshold changes after hydrate", () => { + vi.mocked(getScopedItem).mockReturnValue("true"); + const { result, rerender } = renderHook( + (props: { threshold: number }) => + useCapacityRiskBanner({ ...base, capacityRiskTodoThreshold: props.threshold }), + { initialProps: { threshold: 3 } }, + ); + + // First settings load hydrates without clearing. + expect(result.current.dismissed).toBe(true); + expect(removeScopedItem).not.toHaveBeenCalled(); + + // Changing the threshold resurrects the previously-dismissed banner. + rerender({ threshold: 5 }); + expect(removeScopedItem).toHaveBeenCalledWith(expect.any(String), "p1"); expect(result.current.dismissed).toBe(false); }); diff --git a/packages/dashboard/app/hooks/__tests__/useChatUnreadBadge.test.ts b/packages/dashboard/app/hooks/__tests__/useChatUnreadBadge.test.ts index e656a5f34b..5e272d70c3 100644 --- a/packages/dashboard/app/hooks/__tests__/useChatUnreadBadge.test.ts +++ b/packages/dashboard/app/hooks/__tests__/useChatUnreadBadge.test.ts @@ -80,4 +80,39 @@ describe("useChatUnreadBadge", () => { rerender({ taskView: "chat" }); expect(result.current.chatHasUnreadResponse).toBe(false); }); + it("marks unread on a non-user chat:room:message:added", () => { + const { result } = renderHook(() => + useChatUnreadBadge(undefined, { taskView: "board", quickChatOpen: false }), + ); + + act(() => { + handlers["chat:room:message:added"]?.(message({ role: "assistant" })); + }); + + expect(result.current.chatHasUnreadResponse).toBe(true); + }); + + it("ignores user-role chat:room:message:added events", () => { + const { result } = renderHook(() => + useChatUnreadBadge(undefined, { taskView: "board", quickChatOpen: false }), + ); + + act(() => { + handlers["chat:room:message:added"]?.(message({ role: "user" })); + }); + + expect(result.current.chatHasUnreadResponse).toBe(false); + }); + + it("ignores assistant messages scoped to a different project", () => { + const { result } = renderHook(() => + useChatUnreadBadge("p1", { taskView: "board", quickChatOpen: false }), + ); + + act(() => { + handlers["chat:message:added"]?.(message({ role: "assistant", projectId: "p2" })); + }); + + expect(result.current.chatHasUnreadResponse).toBe(false); + }); }); diff --git a/packages/dashboard/app/hooks/__tests__/useDashboardHealth.test.ts b/packages/dashboard/app/hooks/__tests__/useDashboardHealth.test.ts index 6d6184a35f..b99b0f40cd 100644 --- a/packages/dashboard/app/hooks/__tests__/useDashboardHealth.test.ts +++ b/packages/dashboard/app/hooks/__tests__/useDashboardHealth.test.ts @@ -59,4 +59,27 @@ describe("useDashboardHealth", () => { expect(result.current.refreshError).toBe("nope"); expect(result.current.refreshing).toBe(false); }); + it("does not apply the mount fetch after unmount", async () => { + let resolveMount: (value: { status: string }) => void = () => {}; + fetchDashboardHealth.mockImplementation( + () => + new Promise<{ status: string }>((resolve) => { + resolveMount = resolve; + }), + ); + + const { result, unmount } = renderHook(() => useDashboardHealth()); + expect(result.current.health).toBeNull(); + + unmount(); + resolveMount({ status: "ok" }); + + await act(async () => { + await Promise.resolve(); + await Promise.resolve(); + }); + + // The cancelled flag suppresses setState — health stays at its initial null. + expect(result.current.health).toBeNull(); + }); }); diff --git a/packages/dashboard/app/hooks/__tests__/useStashOrphanCount.test.ts b/packages/dashboard/app/hooks/__tests__/useStashOrphanCount.test.ts index cb13cf3700..95ec89cff2 100644 --- a/packages/dashboard/app/hooks/__tests__/useStashOrphanCount.test.ts +++ b/packages/dashboard/app/hooks/__tests__/useStashOrphanCount.test.ts @@ -58,4 +58,22 @@ describe("useStashOrphanCount", () => { }); expect(mockedApi).toHaveBeenCalledTimes(2); }); + it("stops polling once unmounted", async () => { + mockedApi.mockResolvedValue({ count: 1 }); + const { unmount } = renderHook(() => useStashOrphanCount(undefined)); + + // Initial mount fetch. + await act(async () => { + await vi.advanceTimersByTimeAsync(1); + }); + expect(mockedApi).toHaveBeenCalledTimes(1); + + unmount(); + + // Advance well past the 30s interval — the cleared timer must not fire. + await act(async () => { + await vi.advanceTimersByTimeAsync(60_000); + }); + expect(mockedApi).toHaveBeenCalledTimes(1); + }); });