From 47f875a61e0cefc253c75b119e59cf1f57c5cc80 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Almeida?= <57680069+zeval@users.noreply.github.com> Date: Fri, 1 May 2026 00:19:50 +0100 Subject: [PATCH] fix(web): plan panel misses agent updates emitted before WS connects (#749) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(web): plan panel misses agent updates emitted before WS connects Agent auto-start can write a plan via MCP within ~100 ms of task creation, faster than the browser opens its WS connection — the `task_plan.created` broadcast is delivered to zero clients and the Plan tab never appears because `useTaskPlan` only fetches once the panel is mounted (chicken-egg). Eagerly fetch the plan from `usePlanPanelAutoOpen` once the WS is connected so the store is populated regardless of the panel's mount state. Also acknowledge the plan when the panel is restored from a saved layout with no `lastSeen` recorded — that's a page-reload hydrate, not a fresh agent update, so a stale indicator should not flash. * test(web): cover the new eager-fetch + reload-acknowledge branches Mock the new store selectors and `getTaskPlan` API in `use-plan-panel-auto-open.test.ts`, restore the original `panelExists → return` short-circuit (the previous version slipped through to `addPlanPanel` on a live update against an open panel), and add three cases: eager-fetch on cold load, fetch gated on WS connect, and the page-reload acknowledge. * fix(web): guard the eager plan fetch against an infinite retry loop `isLoading` flipping during the fetch was re-triggering the effect via its own dep update — on persistent failure it would keep firing because `isLoaded` never flips. Switch to a ref-keyed attempt set so each task is fetched at most once per WS-connected session, and clear the set on WS reconnect so transient failures still recover. Also drop the redundant `activeTaskId` guard inside the auto-open effect (`plan` is already derived from `activeTaskId`) and clarify the catch-block comment. Addresses Greptile P1 + Claude review suggestions on PR #749. * test(web): cover the disconnect→reconnect retry + tighten code comments Address Claude review on PR #749: - attemptedRef comment now reflects what the clear actually does (clears all task IDs on WS disconnect, not per-task on navigation). - Catch comment points at the disconnect/reconnect effect as the actual retry trigger instead of the misleading "useTaskPlan retries on panel mount" claim. - Reload-acknowledge comment is honest about the lastSeen ambiguity: optimistic suppression of the indicator flash, with live-update path re-arming after a post-reload update. - Added a regression test for the disconnect → reconnect → re-fetch path, which the previous test suite did not exercise. * docs(web): clarify reload-acknowledge comment to match the actual control flow * test(web): split test file under the 100-line per-function lint cap The new eager-fetch + reconnect-retry tests pushed the top-level describe callback to 114 lines, tripping max-lines-per-function. Split into a sibling describe for the eager-fetch scope. * fix(web): don't overwrite a WS-delivered plan with a stale null fetch The HTTP fetch and the WS \`task_plan.created\` event can resolve in either order on a fast auto-start: when the fetch was issued first (server didn't have the plan yet, returns null) and the WS event arrives mid-flight, the unconditional \`setTaskPlan(taskId, null)\` would clobber the real plan that just landed. Read the live store state inside the \`then\` and skip the write when null overlaps with a populated plan. Adds a regression test for the race. * docs(web): fix the disconnect/reconnect comment wording Effect fires on disconnect, not reconnect — wording was inverted. * fix(web): also guard the eager fetch against stale non-null overwrites CodeRabbit caught a residual race: when a newer plan lands via WS while the HTTP fetch is in flight, an older non-null HTTP response could still clobber it (regressing updated_at + content). Compare timestamps and skip the write when the live store entry is already at-or-newer. Adds a regression test, splits the test file into a third sibling describe so each callback stays under the 100-line lint cap. * style(web): apply prettier formatting --- .../hooks/use-plan-panel-auto-open.test.ts | 180 ++++++++++++++++-- apps/web/hooks/use-plan-panel-auto-open.ts | 71 ++++++- 2 files changed, 236 insertions(+), 15 deletions(-) diff --git a/apps/web/hooks/use-plan-panel-auto-open.test.ts b/apps/web/hooks/use-plan-panel-auto-open.test.ts index 67e24c2477..6b5d5af67b 100644 --- a/apps/web/hooks/use-plan-panel-auto-open.test.ts +++ b/apps/web/hooks/use-plan-panel-auto-open.test.ts @@ -4,25 +4,38 @@ import type { TaskPlan } from "@/lib/types/http"; const mockAddPlanPanel = vi.fn(); const mockGetPanel = vi.fn(); +const mockSetTaskPlan = vi.fn(); +const mockSetTaskPlanLoading = vi.fn(); +const mockMarkTaskPlanSeen = vi.fn(); +const mockGetTaskPlan = vi.fn(); let mockActiveTaskId: string | null = "task-1"; let mockPlan: TaskPlan | null = null; let mockLastSeen: string | undefined = undefined; +let mockIsLoaded = true; +let mockConnectionStatus = "connected"; let mockIsRestoringLayout = false; let mockApi: { getPanel: typeof mockGetPanel } | null = { getPanel: mockGetPanel }; +function buildState() { + return { + tasks: { activeTaskId: mockActiveTaskId }, + taskPlans: { + byTaskId: mockActiveTaskId && mockPlan ? { [mockActiveTaskId]: mockPlan } : {}, + loadedByTaskId: mockActiveTaskId ? { [mockActiveTaskId]: mockIsLoaded } : {}, + lastSeenUpdatedAtByTaskId: + mockActiveTaskId && mockLastSeen !== undefined ? { [mockActiveTaskId]: mockLastSeen } : {}, + }, + connection: { status: mockConnectionStatus }, + setTaskPlan: mockSetTaskPlan, + setTaskPlanLoading: mockSetTaskPlanLoading, + markTaskPlanSeen: mockMarkTaskPlanSeen, + }; +} + vi.mock("@/components/state-provider", () => ({ - useAppStore: (selector: (state: Record) => unknown) => - selector({ - tasks: { activeTaskId: mockActiveTaskId }, - taskPlans: { - byTaskId: mockActiveTaskId && mockPlan ? { [mockActiveTaskId]: mockPlan } : {}, - lastSeenUpdatedAtByTaskId: - mockActiveTaskId && mockLastSeen !== undefined - ? { [mockActiveTaskId]: mockLastSeen } - : {}, - }, - }), + useAppStore: (selector: (state: Record) => unknown) => selector(buildState()), + useAppStoreApi: () => ({ getState: () => buildState() }), })); vi.mock("@/lib/state/dockview-store", () => ({ @@ -34,6 +47,10 @@ vi.mock("@/lib/state/dockview-store", () => ({ }), })); +vi.mock("@/lib/api/domains/plan-api", () => ({ + getTaskPlan: (...args: unknown[]) => mockGetTaskPlan(...args), +})); + import { usePlanPanelAutoOpen } from "./use-plan-panel-auto-open"; const TS = "2026-04-20T00:00:00Z"; @@ -57,9 +74,12 @@ describe("usePlanPanelAutoOpen", () => { mockActiveTaskId = "task-1"; mockPlan = agentPlan(); mockLastSeen = undefined; + mockIsLoaded = true; + mockConnectionStatus = "connected"; mockIsRestoringLayout = false; mockApi = { getPanel: mockGetPanel }; mockGetPanel.mockReturnValue(null); + mockGetTaskPlan.mockResolvedValue(null); }); it("opens plan panel for unseen agent plan", () => { @@ -110,3 +130,141 @@ describe("usePlanPanelAutoOpen", () => { expect(mockAddPlanPanel).not.toHaveBeenCalled(); }); }); + +describe("usePlanPanelAutoOpen — eager fetch", () => { + beforeEach(() => { + vi.clearAllMocks(); + mockActiveTaskId = "task-1"; + mockPlan = agentPlan(); + mockLastSeen = undefined; + mockIsLoaded = true; + mockConnectionStatus = "connected"; + mockIsRestoringLayout = false; + mockApi = { getPanel: mockGetPanel }; + mockGetPanel.mockReturnValue(null); + mockGetTaskPlan.mockResolvedValue(null); + }); + + it("eagerly fetches the plan when not yet loaded", () => { + mockIsLoaded = false; + mockPlan = null; + renderHook(() => usePlanPanelAutoOpen()); + expect(mockGetTaskPlan).toHaveBeenCalledWith("task-1"); + expect(mockSetTaskPlanLoading).toHaveBeenCalledWith("task-1", true); + }); + + it("does not fetch when WS is disconnected", () => { + mockIsLoaded = false; + mockConnectionStatus = "connecting"; + renderHook(() => usePlanPanelAutoOpen()); + expect(mockGetTaskPlan).not.toHaveBeenCalled(); + }); + + it("acknowledges the plan on hydrate when the panel was restored", () => { + mockGetPanel.mockReturnValue({ id: "plan" }); + mockLastSeen = undefined; + renderHook(() => usePlanPanelAutoOpen()); + expect(mockMarkTaskPlanSeen).toHaveBeenCalledWith("task-1"); + expect(mockAddPlanPanel).not.toHaveBeenCalled(); + }); + + it("does not acknowledge a live update when lastSeen is already recorded", () => { + mockGetPanel.mockReturnValue({ id: "plan" }); + mockLastSeen = TS; + mockPlan = agentPlan(TS_LATER); + renderHook(() => usePlanPanelAutoOpen()); + expect(mockMarkTaskPlanSeen).not.toHaveBeenCalled(); + expect(mockAddPlanPanel).not.toHaveBeenCalled(); + }); + + it("does not retry the eager fetch after a failure", async () => { + mockIsLoaded = false; + mockPlan = null; + let rejectFn: (err: Error) => void = () => {}; + mockGetTaskPlan.mockImplementation( + () => + new Promise((_, reject) => { + rejectFn = reject; + }), + ); + const { rerender } = renderHook(() => usePlanPanelAutoOpen()); + expect(mockGetTaskPlan).toHaveBeenCalledTimes(1); + rejectFn(new Error("boom")); + await new Promise((r) => setTimeout(r, 0)); + rerender(); + rerender(); + expect(mockGetTaskPlan).toHaveBeenCalledTimes(1); + }); +}); + +describe("usePlanPanelAutoOpen — race guards", () => { + beforeEach(() => { + vi.clearAllMocks(); + mockActiveTaskId = "task-1"; + mockPlan = agentPlan(); + mockLastSeen = undefined; + mockIsLoaded = true; + mockConnectionStatus = "connected"; + mockIsRestoringLayout = false; + mockApi = { getPanel: mockGetPanel }; + mockGetPanel.mockReturnValue(null); + mockGetTaskPlan.mockResolvedValue(null); + }); + + it("does not overwrite a newer WS-delivered plan with an older HTTP result", async () => { + mockIsLoaded = false; + mockPlan = null; + let resolveFn: (v: TaskPlan | null) => void = () => {}; + mockGetTaskPlan.mockImplementation( + () => + new Promise((resolve) => { + resolveFn = resolve; + }), + ); + renderHook(() => usePlanPanelAutoOpen()); + // WS delivers the latest plan while the HTTP fetch is in flight. + mockPlan = agentPlan(TS_LATER); + // HTTP resolves with an older snapshot of the same plan. + resolveFn(agentPlan(TS)); + await new Promise((r) => setTimeout(r, 0)); + expect(mockSetTaskPlan).not.toHaveBeenCalled(); + }); + + it("does not overwrite a WS-delivered plan when the fetch resolves with null", async () => { + mockIsLoaded = false; + mockPlan = null; + let resolveFn: (v: TaskPlan | null) => void = () => {}; + mockGetTaskPlan.mockImplementation( + () => + new Promise((resolve) => { + resolveFn = resolve; + }), + ); + renderHook(() => usePlanPanelAutoOpen()); + // Simulate a WS event populating the store while the HTTP fetch is in flight. + mockPlan = agentPlan(); + resolveFn(null); + await new Promise((r) => setTimeout(r, 0)); + expect(mockSetTaskPlan).not.toHaveBeenCalled(); + }); + + it("retries the eager fetch after WS reconnects following a failure", async () => { + mockIsLoaded = false; + mockPlan = null; + mockGetTaskPlan.mockRejectedValueOnce(new Error("boom")); + mockGetTaskPlan.mockResolvedValueOnce(null); + + const { rerender } = renderHook(() => usePlanPanelAutoOpen()); + expect(mockGetTaskPlan).toHaveBeenCalledTimes(1); + await new Promise((r) => setTimeout(r, 0)); + + // WS disconnect — clears the attempted set + mockConnectionStatus = "connecting"; + rerender(); + + // WS reconnect — fetches again + mockConnectionStatus = "connected"; + rerender(); + expect(mockGetTaskPlan).toHaveBeenCalledTimes(2); + }); +}); diff --git a/apps/web/hooks/use-plan-panel-auto-open.ts b/apps/web/hooks/use-plan-panel-auto-open.ts index 6cdc396359..d6220aca1f 100644 --- a/apps/web/hooks/use-plan-panel-auto-open.ts +++ b/apps/web/hooks/use-plan-panel-auto-open.ts @@ -1,8 +1,9 @@ "use client"; -import { useEffect } from "react"; -import { useAppStore } from "@/components/state-provider"; +import { useEffect, useRef } from "react"; +import { useAppStore, useAppStoreApi } from "@/components/state-provider"; import { useDockviewStore } from "@/lib/state/dockview-store"; +import { getTaskPlan } from "@/lib/api/domains/plan-api"; /** * Watches the active task's plan and opens the Plan panel quietly (without @@ -18,19 +19,81 @@ import { useDockviewStore } from "@/lib/state/dockview-store"; export function usePlanPanelAutoOpen() { const activeTaskId = useAppStore((s) => s.tasks.activeTaskId); const plan = useAppStore((s) => (activeTaskId ? s.taskPlans.byTaskId[activeTaskId] : null)); + const isLoaded = useAppStore((s) => + activeTaskId ? (s.taskPlans.loadedByTaskId[activeTaskId] ?? false) : false, + ); const lastSeen = useAppStore((s) => activeTaskId ? s.taskPlans.lastSeenUpdatedAtByTaskId[activeTaskId] : undefined, ); + const setTaskPlan = useAppStore((s) => s.setTaskPlan); + const setTaskPlanLoading = useAppStore((s) => s.setTaskPlanLoading); + const markTaskPlanSeen = useAppStore((s) => s.markTaskPlanSeen); + const connectionStatus = useAppStore((s) => s.connection.status); + const storeApi = useAppStoreApi(); const api = useDockviewStore((s) => s.api); const isRestoringLayout = useDockviewStore((s) => s.isRestoringLayout); const addPlanPanel = useDockviewStore((s) => s.addPlanPanel); + // Track tasks we've already attempted to fetch so a transient failure + // doesn't put us in an infinite retry loop. Cleared on WS disconnect so a + // reconnect can retry any failed or not-yet-attempted tasks. + const attemptedRef = useRef>(new Set()); + + // Eagerly fetch the plan on task load. The Plan panel mounts `useTaskPlan` + // only after the panel exists, so without this fetch a plan written by the + // agent before the browser's WS connected (fast auto-start path) would never + // populate the store and the auto-open below would never fire. + useEffect(() => { + if (!activeTaskId || connectionStatus !== "connected") return; + if (isLoaded) return; + if (attemptedRef.current.has(activeTaskId)) return; + const taskId = activeTaskId; + attemptedRef.current.add(taskId); + setTaskPlanLoading(taskId, true); + getTaskPlan(taskId) + .then((fetched) => { + // Race guard: if a WS event populated the store while our HTTP + // request was in flight, don't overwrite a real plan with a + // stale response — neither a `null` (server didn't have it yet + // at fetch time) nor an older non-null version (HTTP saw an + // earlier write than the WS event we already applied). + const live = storeApi.getState().taskPlans.byTaskId[taskId]; + if (live) { + if (fetched === null) return; + if (Date.parse(fetched.updated_at) < Date.parse(live.updated_at)) return; + } + setTaskPlan(taskId, fetched); + }) + .catch(() => { + /* swallow — the disconnect/reconnect effect clears `attemptedRef` + * so a transient failure retries automatically after recovery. */ + }) + .finally(() => setTaskPlanLoading(taskId, false)); + }, [activeTaskId, connectionStatus, isLoaded, setTaskPlan, setTaskPlanLoading, storeApi]); + + // Clear the attempt set on WS disconnect so that when the WS reconnects + // the fetch effect can retry any tasks that previously failed or were pending. + useEffect(() => { + if (connectionStatus === "connected") return; + attemptedRef.current.clear(); + }, [connectionStatus]); + useEffect(() => { if (!api || isRestoringLayout) return; if (!plan || plan.created_by !== "agent") return; if (lastSeen === plan.updated_at) return; - if (api.getPanel("plan")) return; + if (api.getPanel("plan")) { + // Page-reload case: panel restored from saved layout and there is no + // recorded `lastSeen` (not persisted across sessions). We can't tell + // whether the plan was acknowledged before reload or not, so we + // optimistically mark it seen to avoid a stale indicator flash on + // every reload. When a live update arrives after the reload, + // `lastSeen` is defined so we don't suppress it — PlanTab reads the + // store directly and re-arms the indicator. + if (lastSeen === undefined) markTaskPlanSeen(plan.task_id); + return; + } addPlanPanel({ quiet: true, inCenter: true }); - }, [api, isRestoringLayout, plan, lastSeen, addPlanPanel]); + }, [api, isRestoringLayout, plan, lastSeen, addPlanPanel, markTaskPlanSeen]); }