diff --git a/apps/web/app/needs-you-inbox/needs-you-inbox-page-client.test.tsx b/apps/web/app/needs-you-inbox/needs-you-inbox-page-client.test.tsx index 407b30698b..98fbef76a9 100644 --- a/apps/web/app/needs-you-inbox/needs-you-inbox-page-client.test.tsx +++ b/apps/web/app/needs-you-inbox/needs-you-inbox-page-client.test.tsx @@ -8,20 +8,24 @@ const mocks = vi.hoisted(() => ({ })); const EMPTY_TESTID = "needs-you-inbox-empty"; +const ERROR_TESTID = "needs-you-inbox-error"; let state: { status: "idle" | "loading" | "ready" | "error"; bundles: ClarificationInboxBundle[]; hiddenCount: number; hasMore: boolean; + appliedGeneration?: number; + lastAppliedOk?: boolean; }; +let workspacesActiveId: string | null; vi.mock("@/components/state-provider", () => ({ // eslint-disable-next-line @typescript-eslint/no-explicit-any useAppStore: (selector: (s: any) => unknown) => selector({ needsYouInbox: { byWorkspaceId: { w1: state } }, - workspaces: { activeId: "w1" }, + workspaces: { activeId: workspacesActiveId }, bumpNeedsYouInboxRefreshTick: mocks.bumpRefreshTick, }), })); @@ -82,6 +86,7 @@ function bundle(id: string): ClarificationInboxBundle { beforeEach(() => { mocks.bumpRefreshTick.mockReset(); state = { status: "idle", bundles: [], hiddenCount: 0, hasMore: false }; + workspacesActiveId = "w1"; }); afterEach(() => cleanup()); @@ -116,7 +121,7 @@ describe("NeedsYouInboxPageClient", () => { state = { status: "error", bundles: [], hiddenCount: 0, hasMore: false }; render(); - expect(screen.getByTestId("needs-you-inbox-error")).not.toBeNull(); + expect(screen.getByTestId(ERROR_TESTID)).not.toBeNull(); expect(screen.queryByTestId(EMPTY_TESTID)).toBeNull(); }); @@ -124,7 +129,7 @@ describe("NeedsYouInboxPageClient", () => { state = { status: "ready", bundles: [], hiddenCount: 0, hasMore: true }; render(); - expect(screen.getByTestId("needs-you-inbox-error")).not.toBeNull(); + expect(screen.getByTestId(ERROR_TESTID)).not.toBeNull(); expect(screen.queryByTestId(EMPTY_TESTID)).toBeNull(); }); @@ -148,12 +153,127 @@ describe("NeedsYouInboxPageClient", () => { expect(screen.getByTestId("stub-hidden-panel").textContent).toBe("2"); }); +}); +describe("NeedsYouInboxPageClient loading vs. settled/refresh states", () => { it("shows a loading indicator on the very first read, not the empty state", () => { - state = { status: "loading", bundles: [], hiddenCount: 0, hasMore: false }; + state = { + status: "loading", + bundles: [], + hiddenCount: 0, + hasMore: false, + appliedGeneration: 0, + }; render(); expect(screen.getByRole("status")).not.toBeNull(); expect(screen.queryByTestId(EMPTY_TESTID)).toBeNull(); }); + + it("keeps the settled empty state during a background refresh, instead of re-flashing loading", () => { + state = { + status: "loading", + bundles: [], + hiddenCount: 0, + hasMore: false, + appliedGeneration: 1, + lastAppliedOk: true, + }; + render(); + + expect(screen.getByTestId(EMPTY_TESTID)).not.toBeNull(); + expect(screen.queryByRole("status")).toBeNull(); + }); + + it("keeps the settled list during a background refresh, instead of re-flashing loading", () => { + state = { + status: "loading", + bundles: [bundle("p1")], + hiddenCount: 0, + hasMore: false, + appliedGeneration: 1, + lastAppliedOk: true, + }; + render(); + + expect(screen.getAllByTestId("stub-row")).toHaveLength(1); + expect(screen.queryByRole("status")).toBeNull(); + }); + + it("does not flash the error state for a truncated boot payload before the first read resolves", () => { + state = { + status: "loading", + bundles: [], + hiddenCount: 0, + hasMore: true, + appliedGeneration: 0, + lastAppliedOk: false, + }; + render(); + + expect(screen.getByRole("status")).not.toBeNull(); + expect(screen.queryByTestId(ERROR_TESTID)).toBeNull(); + }); + + // R2-F1: a refresh that follows a FAILED read reaches resolveViewMode as + // the same (loading, 0 bundles, 0 hidden, hasMore=false) tuple as a refresh + // over an already-settled empty inbox -- only `lastAppliedOk` (false here, + // since the last applied response was the error, not a page) tells them + // apart. Getting this wrong renders a false "Nothing needs your attention" + // over a read that is still failing. + it("shows the loading indicator, not a false empty state, during a refresh that follows a failed read", () => { + state = { + status: "loading", + bundles: [], + hiddenCount: 0, + hasMore: false, + appliedGeneration: 3, + lastAppliedOk: false, + }; + render(); + + expect(screen.getByRole("status")).not.toBeNull(); + expect(screen.queryByTestId(EMPTY_TESTID)).toBeNull(); + expect(screen.queryByTestId(ERROR_TESTID)).toBeNull(); + }); + + // R2-F5: the boot seed applies before the first read starts, so `status` + // can observably be "idle" (not yet "loading") with a truncation flag + // already set. That must read the same as any other pre-first-read state: + // the honest spinner, not a false empty. + it("shows the loading indicator, not a false empty state, for an idle truncated boot seed", () => { + state = { + status: "idle", + bundles: [], + hiddenCount: 0, + hasMore: true, + appliedGeneration: 0, + lastAppliedOk: false, + }; + render(); + + expect(screen.getByRole("status")).not.toBeNull(); + expect(screen.queryByTestId(EMPTY_TESTID)).toBeNull(); + expect(screen.queryByTestId(ERROR_TESTID)).toBeNull(); + }); + + // R3-F1: with no active workspace the controller never issues a read (every + // trigger is guarded on `workspaceId`), so `lastAppliedOk` can never flip + // and the pre-read "idle"/`!lastAppliedOk` spinner would otherwise never + // resolve. This must render the empty state, not a permanent spinner. + it("renders the empty state, not a permanent spinner, when no active workspace resolves", () => { + workspacesActiveId = null; + state = { + status: "idle", + bundles: [], + hiddenCount: 0, + hasMore: false, + appliedGeneration: 0, + lastAppliedOk: false, + }; + render(); + + expect(screen.getByTestId(EMPTY_TESTID)).not.toBeNull(); + expect(screen.queryByRole("status")).toBeNull(); + }); }); diff --git a/apps/web/app/needs-you-inbox/needs-you-inbox-page-client.tsx b/apps/web/app/needs-you-inbox/needs-you-inbox-page-client.tsx index eab4dd0351..3c43229631 100644 --- a/apps/web/app/needs-you-inbox/needs-you-inbox-page-client.tsx +++ b/apps/web/app/needs-you-inbox/needs-you-inbox-page-client.tsx @@ -8,9 +8,11 @@ import { selectNeedsYouInboxBundles, selectNeedsYouInboxHasMore, selectNeedsYouInboxHiddenCount, + selectNeedsYouInboxLastAppliedOk, selectNeedsYouInboxRevision, selectNeedsYouInboxStatus, } from "@/lib/state/slices/needs-you-inbox/selectors"; +import type { NeedsYouInboxReadStatus } from "@/lib/state/slices/needs-you-inbox/types"; import type { ClarificationInboxBundle } from "@/lib/types/clarification-inbox"; import { NeedsYouInboxRow } from "@/components/needs-you-inbox/needs-you-inbox-row"; import { NeedsYouInboxEmptyState } from "@/components/needs-you-inbox/needs-you-inbox-empty-state"; @@ -19,17 +21,27 @@ import { NeedsYouInboxHiddenPanel } from "@/components/needs-you-inbox/needs-you type ViewMode = "error" | "loading" | "empty" | "list"; -// A page reporting truncation while listing zero rows means enrichment -// emptied a page the query had filled, so this resolves to the retryable -// "error" state rather than "empty" (design-01#Data-and-contracts). +// `status` alone can't distinguish a refresh after success from one after a +// failure, since `beginNeedsYouInboxRead` overwrites it to "loading" without +// touching what preceded it; `lastAppliedOk` carries that distinction and +// gates the loading view. `hasActiveWorkspace` short-circuits to the empty +// view, since with no active workspace no controller trigger ever applies a +// response and `lastAppliedOk` can never flip. A truncated page with zero +// rows is enrichment having emptied a filled page, not an empty inbox, so it +// resolves to the retryable error view instead. function resolveViewMode( - status: string, + status: NeedsYouInboxReadStatus, bundleCount: number, - hiddenCount: number, hasMore: boolean, + lastAppliedOk: boolean, + hasActiveWorkspace: boolean, ): ViewMode { - if (status === "error" || (bundleCount === 0 && hasMore)) return "error"; - if (status === "loading" && bundleCount === 0 && hiddenCount === 0) return "loading"; + if (!hasActiveWorkspace) return "empty"; + if ((status === "loading" || status === "idle") && !lastAppliedOk) { + return "loading"; + } + if (status === "error") return "error"; + if (bundleCount === 0 && hasMore) return "error"; if (bundleCount === 0) return "empty"; return "list"; } @@ -76,10 +88,18 @@ export function NeedsYouInboxPageClient() { const hiddenCount = useAppStore(selectNeedsYouInboxHiddenCount); const listRevision = useAppStore(selectNeedsYouInboxRevision); const hasMore = useAppStore(selectNeedsYouInboxHasMore); + const lastAppliedOk = useAppStore(selectNeedsYouInboxLastAppliedOk); + const hasActiveWorkspace = useAppStore((s) => s.workspaces.activeId !== null); const bumpRefreshTick = useAppStore((s) => s.bumpNeedsYouInboxRefreshTick); const retry = useCallback(() => bumpRefreshTick(), [bumpRefreshTick]); - const viewMode = resolveViewMode(status, bundles.length, hiddenCount, hasMore); + const viewMode = resolveViewMode( + status, + bundles.length, + hasMore, + lastAppliedOk, + hasActiveWorkspace, + ); return ( diff --git a/apps/web/e2e/tests/chat/needs-you-inbox-background-refresh.spec.ts b/apps/web/e2e/tests/chat/needs-you-inbox-background-refresh.spec.ts new file mode 100644 index 0000000000..2d8e3cd3a6 --- /dev/null +++ b/apps/web/e2e/tests/chat/needs-you-inbox-background-refresh.spec.ts @@ -0,0 +1,80 @@ +// AC-UI-NEEDS-YOU-INBOX-001: a workspace-wide session event must not flash +// the first-load "Loading…" state over an already-settled (here: empty) +// Inbox. Regression coverage for the `lastAppliedOk`-gated view mode in +// needs-you-inbox-page-client.tsx. +import { test, expect } from "../../fixtures/test-base"; +import { watchWs, waitForHttp } from "../../helpers/causal-waits"; + +test.describe("Needs-you Inbox background refresh", () => { + test("keeps the settled empty state during a WS-triggered background refresh", async ({ + testPage, + apiClient, + seedData, + }) => { + test.setTimeout(60_000); + + // Arm before the first navigation: `page.on("websocket")` only sees + // sockets opened after this call (see causal-waits.ts). + const wsWatcher = watchWs(testPage); + + const initialInboxLoaded = waitForHttp(testPage, "GET", /\/api\/v1\/clarification-inbox$/); + await testPage.goto("/needs-you-inbox"); + await initialInboxLoaded; + const emptyState = testPage.getByTestId("needs-you-inbox-empty"); + await expect(emptyState).toBeVisible(); + const loadingText = testPage.getByText("Loading…", { exact: true }); + await expect(loadingText).toHaveCount(0); + + // Hold every clarification-inbox read that starts from here on, + // deterministically, instead of racing a guessed wall-clock delay + // against the real refresh. `/e2e:simple-message` fires several + // workspace-wide session.state_changed / pending_action_changed events in + // quick succession (CREATED, STARTING, ...); without holding every one of + // them, a later un-held read resolves fast enough to re-settle the view + // before the assertion below ever samples the DOM, hiding the exact race + // this test exists to catch. + let resolveHeld: (() => void) | undefined; + const held = new Promise((resolve) => { + resolveHeld = resolve; + }); + let resolveReleaseAll: (() => void) | undefined; + const releaseAll = new Promise((resolve) => { + resolveReleaseAll = resolve; + }); + await testPage.route("**/api/v1/clarification-inbox?*", async (route) => { + const response = await route.fetch(); + resolveHeld?.(); + await releaseAll; + await route.fulfill({ response }); + }); + + const stateChanged = wsWatcher.waitForEvent("session.state_changed"); + + // Any session transition in the workspace broadcasts `session.state_changed` + // workspace-wide (design-02#Control-flow) and bumps the Inbox's refresh + // tick, regardless of whether the task ever produces a clarification. + const title = "Needs-you Inbox Background Refresh"; + await apiClient.createTaskWithAgent(seedData.workspaceId, title, seedData.agentProfileId, { + description: "/e2e:simple-message", + workflow_id: seedData.workflowId, + workflow_step_id: seedData.startStepId, + repository_ids: [seedData.repositoryId], + }); + + await stateChanged; + await held; + + // At least one background read is held in flight and none has resolved: + // the settled empty view must survive it, and the first-load spinner + // must not reappear. + await expect(emptyState).toBeVisible(); + await expect(loadingText).toHaveCount(0); + + const backgroundReadResolved = waitForHttp(testPage, "GET", /\/api\/v1\/clarification-inbox$/); + resolveReleaseAll?.(); + await backgroundReadResolved; + // Still empty once the background reads actually resolve: the new task + // never produced a clarification, so nothing should have changed. + await expect(emptyState).toBeVisible(); + }); +}); diff --git a/apps/web/hooks/domains/needs-you-inbox/use-needs-you-inbox-controller.test.tsx b/apps/web/hooks/domains/needs-you-inbox/use-needs-you-inbox-controller.test.tsx index c3b4293ef0..e865d9f3c1 100644 --- a/apps/web/hooks/domains/needs-you-inbox/use-needs-you-inbox-controller.test.tsx +++ b/apps/web/hooks/domains/needs-you-inbox/use-needs-you-inbox-controller.test.tsx @@ -9,8 +9,26 @@ vi.mock("@/lib/api/domains/clarification-inbox-api", () => ({ listClarificationInbox: (...args: unknown[]) => listClarificationInboxMock(...args), })); +const SESSION_STATE_CHANGED = "session.state_changed"; + +type WsHandler = () => void; +const wsMocks = vi.hoisted(() => ({ + handlers: new Map>(), +})); + +function emitWsEvent(type: string) { + for (const handler of wsMocks.handlers.get(type) ?? []) handler(); +} + vi.mock("@/lib/ws/connection", () => ({ - getWebSocketClient: () => null, + getWebSocketClient: () => ({ + on: (type: string, handler: WsHandler) => { + const handlers = wsMocks.handlers.get(type) ?? new Set(); + handlers.add(handler); + wsMocks.handlers.set(type, handlers); + return () => handlers.delete(handler); + }, + }), })); const readBootPayloadMock = vi.fn(); @@ -58,6 +76,7 @@ beforeEach(() => { listClarificationInboxMock.mockResolvedValue(page()); readBootPayloadMock.mockReset(); readBootPayloadMock.mockReturnValue({ initialState: {} }); + wsMocks.handlers.clear(); }); afterEach(() => { @@ -239,3 +258,129 @@ describe("useNeedsYouInboxController boot-hydration seed (AC .34, .40, .41)", () expect(result.current.getState().needsYouInbox.byWorkspaceId[WORKSPACE_ID]).toBeUndefined(); }); }); + +describe("useNeedsYouInboxController WS event coalescing", () => { + it("coalesces a burst into a leading read now and one trailing read for what changed inside the window", async () => { + vi.useFakeTimers(); + listClarificationInboxMock.mockResolvedValueOnce(page()); + listClarificationInboxMock.mockResolvedValueOnce(page()); + listClarificationInboxMock.mockResolvedValue(page({ count: 3 })); + const { result } = renderController(true); + await act(async () => { + await vi.advanceTimersByTimeAsync(0); + }); + expect(listClarificationInboxMock).toHaveBeenCalledTimes(1); + + // Burst of two distinct signals plus a repeat, all inside the window: + // `session.state_changed` and `session.pending_action_changed` are + // distinct signals from distinct sessions, not duplicates of one browser + // event, so every one of them after the leading read must be queued, not + // dropped. + act(() => { + emitWsEvent(SESSION_STATE_CHANGED); + emitWsEvent("session.pending_action_changed"); + emitWsEvent(SESSION_STATE_CHANGED); + }); + await act(async () => { + await vi.advanceTimersByTimeAsync(0); + }); + expect(listClarificationInboxMock).toHaveBeenCalledTimes(2); + + // Still inside the window: no second read has fired yet, but one is + // armed for the remainder of it. + await act(async () => { + await vi.advanceTimersByTimeAsync(249); + }); + expect(listClarificationInboxMock).toHaveBeenCalledTimes(2); + + // The window elapses: the trailing read fires and its result is applied, + // so nothing from the burst is silently lost. + await act(async () => { + await vi.advanceTimersByTimeAsync(1); + }); + expect(listClarificationInboxMock).toHaveBeenCalledTimes(3); + expect(result.current.getState().needsYouInbox.byWorkspaceId[WORKSPACE_ID]?.count).toBe(3); + + // Well past the window: a later, separate event still causes its own + // leading read. + await act(async () => { + await vi.advanceTimersByTimeAsync(250); + }); + act(() => { + emitWsEvent(SESSION_STATE_CHANGED); + }); + await act(async () => { + await vi.advanceTimersByTimeAsync(0); + }); + expect(listClarificationInboxMock).toHaveBeenCalledTimes(4); + }); + + it("clears a pending trailing read on unmount", async () => { + vi.useFakeTimers(); + const { unmount } = renderController(true); + await act(async () => { + await vi.advanceTimersByTimeAsync(0); + }); + expect(listClarificationInboxMock).toHaveBeenCalledTimes(1); + + act(() => { + emitWsEvent(SESSION_STATE_CHANGED); + emitWsEvent(SESSION_STATE_CHANGED); + }); + await act(async () => { + await vi.advanceTimersByTimeAsync(0); + }); + expect(listClarificationInboxMock).toHaveBeenCalledTimes(2); + + // A trailing timer is now armed for the second (queued) event. A read + // count alone can't tell whether unmount actually cleared it: the effect + // that would turn a leaked timer's tick bump into a read is unmounted + // right along with it, so the count stays flat either way. Assert the + // timer itself. + expect(vi.getTimerCount()).toBeGreaterThan(0); + + unmount(); + + expect(vi.getTimerCount()).toBe(0); + }); + + // R2-F3: `trailingBumpTimeoutRef` must survive an effect re-run triggered + // by something other than unmount (here, a connectionStatus change that is + // not itself a reconnect-to-"connected" edge) -- the effect's own cleanup + // only tears down the WS listeners it registered, and must not silently + // drop a still-pending trailing bump. + it("does not drop a pending trailing read when the WS effect re-runs for an unrelated reason", async () => { + vi.useFakeTimers(); + listClarificationInboxMock.mockResolvedValueOnce(page()); + listClarificationInboxMock.mockResolvedValueOnce(page()); + listClarificationInboxMock.mockResolvedValue(page({ count: 3 })); + const { result } = renderController(true); + await act(async () => { + await vi.advanceTimersByTimeAsync(0); + }); + expect(listClarificationInboxMock).toHaveBeenCalledTimes(1); + + act(() => { + emitWsEvent(SESSION_STATE_CHANGED); + emitWsEvent(SESSION_STATE_CHANGED); + }); + await act(async () => { + await vi.advanceTimersByTimeAsync(0); + }); + expect(listClarificationInboxMock).toHaveBeenCalledTimes(2); + + // A connectionStatus change that is not a "disconnected -> connected" + // edge (so it does not itself trigger a reconnect read) still re-runs the + // effect the trailing timer lives inside. + act(() => { + result.current.getState().setConnectionStatus("reconnecting"); + }); + + await act(async () => { + await vi.advanceTimersByTimeAsync(250); + }); + + expect(listClarificationInboxMock).toHaveBeenCalledTimes(3); + expect(result.current.getState().needsYouInbox.byWorkspaceId[WORKSPACE_ID]?.count).toBe(3); + }); +}); diff --git a/apps/web/hooks/domains/needs-you-inbox/use-needs-you-inbox-controller.ts b/apps/web/hooks/domains/needs-you-inbox/use-needs-you-inbox-controller.ts index 921eeee251..836ecb75f4 100644 --- a/apps/web/hooks/domains/needs-you-inbox/use-needs-you-inbox-controller.ts +++ b/apps/web/hooks/domains/needs-you-inbox/use-needs-you-inbox-controller.ts @@ -1,7 +1,7 @@ import { useCallback, useEffect, useLayoutEffect, useRef } from "react"; import { useAppStore, useAppStoreApi } from "@/components/state-provider"; import { useFeature } from "@/hooks/domains/features/use-feature"; -import { useForegroundRefresh } from "@/hooks/use-foreground-refresh"; +import { FOREGROUND_EVENT_COALESCE_MS, useForegroundRefresh } from "@/hooks/use-foreground-refresh"; import { getWebSocketClient } from "@/lib/ws/connection"; import { listClarificationInbox } from "@/lib/api/domains/clarification-inbox-api"; import { selectNeedsYouInboxNextSnoozeExpiry } from "@/lib/state/slices/needs-you-inbox/selectors"; @@ -48,6 +48,85 @@ function useNeedsYouInboxBootSeed( }, [enabled, workspaceId, storeApi]); } +// Trigger: WS action-based (session.pending_action_changed, +// session.state_changed) and edge-detected WS (re)connect. Both live in one +// effect keyed on connectionStatus so a client created after this hook mounts +// is picked up the moment status first changes. +function useNeedsYouInboxWsRefresh( + enabled: boolean, + workspaceId: string | null, + connectionStatus: string, + refresh: (targetWorkspaceId: string) => Promise, + storeApi: ReturnType, +) { + const wasConnectedRef = useRef(false); + // session.state_changed is broadcast workspace-wide, so a burst of + // unrelated session transitions must not cause a burst of reads: coalesce + // with the same window use-foreground-refresh.ts uses for its own bursty + // browser events. Unlike that hook's duplicate browser events, though, + // session.state_changed and session.pending_action_changed are distinct + // signals from distinct sessions -- a read triggered by one cannot reflect + // a change the other carries -- so an event landing inside the window is + // queued for one trailing bump at the end of it, never dropped. + const lastWsBumpAtRef = useRef(-Infinity); + // Held in a ref alongside `lastWsBumpAtRef`, not effect-local: this effect + // re-runs on every `connectionStatus` change (e.g. a reconnect), and its own + // cleanup only tears down the WS listeners it just registered -- clearing a + // still-pending trailing bump there would drop it silently on a re-run, + // exactly the "dropped, not deferred" bug this coalescing exists to avoid. + // Only a true unmount (below) clears it. + const trailingBumpTimeoutRef = useRef(undefined); + useEffect(() => { + if (!enabled) return; + const isConnected = connectionStatus === "connected"; + if (isConnected && !wasConnectedRef.current && workspaceId) { + void refresh(workspaceId); + } + wasConnectedRef.current = isConnected; + + const client = getWebSocketClient(); + if (!client) return; + const doBump = () => { + lastWsBumpAtRef.current = Date.now(); + storeApi.getState().bumpNeedsYouInboxRefreshTick(); + }; + const bump = () => { + const elapsed = Date.now() - lastWsBumpAtRef.current; + if (elapsed >= FOREGROUND_EVENT_COALESCE_MS) { + // A pending trailing timer due but not yet fired loses the race to + // this immediate path; cancel it so the burst yields one read here, + // not one here and a second when the timer catches up. + if (trailingBumpTimeoutRef.current !== undefined) { + window.clearTimeout(trailingBumpTimeoutRef.current); + trailingBumpTimeoutRef.current = undefined; + } + doBump(); + return; + } + if (trailingBumpTimeoutRef.current !== undefined) return; + trailingBumpTimeoutRef.current = window.setTimeout(() => { + trailingBumpTimeoutRef.current = undefined; + doBump(); + }, FOREGROUND_EVENT_COALESCE_MS - elapsed); + }; + const offPending = client.on("session.pending_action_changed", bump); + const offStateChanged = client.on("session.state_changed", bump); + return () => { + offPending(); + offStateChanged(); + }; + }, [enabled, connectionStatus, workspaceId, refresh, storeApi]); + + useEffect(() => { + return () => { + if (trailingBumpTimeoutRef.current !== undefined) { + window.clearTimeout(trailingBumpTimeoutRef.current); + trailingBumpTimeoutRef.current = undefined; + } + }; + }, []); +} + /** * Owns every Needs-you Inbox refresh trigger (design-02#Control-flow) and the * snooze-expiry timer. Mounted once, unconditionally of route, so the badge @@ -95,29 +174,7 @@ export function useNeedsYouInboxController() { void refresh(workspaceId); }, [enabled, workspaceId, refresh]); - // Trigger: WS action-based (session.pending_action_changed, - // session.state_changed) and edge-detected WS (re)connect. Both live in one - // effect keyed on connectionStatus so a client created after this hook - // mounts is picked up the moment status first changes. - const wasConnectedRef = useRef(false); - useEffect(() => { - if (!enabled) return; - const isConnected = connectionStatus === "connected"; - if (isConnected && !wasConnectedRef.current && workspaceId) { - void refresh(workspaceId); - } - wasConnectedRef.current = isConnected; - - const client = getWebSocketClient(); - if (!client) return; - const bump = () => storeApi.getState().bumpNeedsYouInboxRefreshTick(); - const offPending = client.on("session.pending_action_changed", bump); - const offStateChanged = client.on("session.state_changed", bump); - return () => { - offPending(); - offStateChanged(); - }; - }, [enabled, connectionStatus, workspaceId, refresh, storeApi]); + useNeedsYouInboxWsRefresh(enabled, workspaceId, connectionStatus, refresh, storeApi); // Applies the WS-triggered refresh tick bumped above. const lastTickRef = useRef(refreshTick); diff --git a/apps/web/hooks/use-foreground-refresh.ts b/apps/web/hooks/use-foreground-refresh.ts index f6b35bd6fe..255addb844 100644 --- a/apps/web/hooks/use-foreground-refresh.ts +++ b/apps/web/hooks/use-foreground-refresh.ts @@ -1,6 +1,6 @@ import { useCallback, useEffect, useRef } from "react"; -const FOREGROUND_EVENT_COALESCE_MS = 250; +export const FOREGROUND_EVENT_COALESCE_MS = 250; type Refresh = () => void | Promise; diff --git a/apps/web/lib/state/slices/needs-you-inbox/needs-you-inbox-slice.test.ts b/apps/web/lib/state/slices/needs-you-inbox/needs-you-inbox-slice.test.ts index 8143e314b7..99a051047a 100644 --- a/apps/web/lib/state/slices/needs-you-inbox/needs-you-inbox-slice.test.ts +++ b/apps/web/lib/state/slices/needs-you-inbox/needs-you-inbox-slice.test.ts @@ -107,6 +107,41 @@ describe("needs-you-inbox slice", () => { expect(state.count).toBe(0); }); + // R2-F1: `resolveViewMode` needs to tell a refresh following success apart + // from one following a failure, and `status` alone can't -- a refresh in + // flight is "loading" either way, since `beginNeedsYouInboxRead` only ever + // writes `status`. `lastAppliedOk` is the field that survives that + // overwrite untouched. + it("marks lastAppliedOk true after a successful page and false after a failed read, unaffected by the loading transition between them", () => { + const store = newStore(); + const okGeneration = store.getState().beginNeedsYouInboxRead("w1"); + store.getState().setNeedsYouInboxPage("w1", okGeneration, { + bundles: [bundle()], + count: 1, + hiddenCount: 0, + nextSnoozeExpiry: null, + hasMore: false, + }); + expect(store.getState().needsYouInbox.byWorkspaceId.w1.lastAppliedOk).toBe(true); + + const failGeneration = store.getState().beginNeedsYouInboxRead("w1"); + // Mid-refresh: status flips to "loading" but the prior success must still + // be visible until the read actually settles one way or the other. + expect(store.getState().needsYouInbox.byWorkspaceId.w1.status).toBe("loading"); + expect(store.getState().needsYouInbox.byWorkspaceId.w1.lastAppliedOk).toBe(true); + + store.getState().setNeedsYouInboxError("w1", failGeneration); + expect(store.getState().needsYouInbox.byWorkspaceId.w1.lastAppliedOk).toBe(false); + + // A refresh started after the failure is "loading" again, but now with no + // successful page behind it -- this is the exact tuple `resolveViewMode` + // must not render as a false "empty". + store.getState().beginNeedsYouInboxRead("w1"); + const state = store.getState().needsYouInbox.byWorkspaceId.w1; + expect(state.status).toBe("loading"); + expect(state.lastAppliedOk).toBe(false); + }); + it("drops a stale error response the same way it drops a stale page response", () => { const store = newStore(); const staleGeneration = store.getState().beginNeedsYouInboxRead("w1"); diff --git a/apps/web/lib/state/slices/needs-you-inbox/needs-you-inbox-slice.ts b/apps/web/lib/state/slices/needs-you-inbox/needs-you-inbox-slice.ts index 3a878e6b28..5e49e788aa 100644 --- a/apps/web/lib/state/slices/needs-you-inbox/needs-you-inbox-slice.ts +++ b/apps/web/lib/state/slices/needs-you-inbox/needs-you-inbox-slice.ts @@ -22,6 +22,7 @@ const emptyWorkspaceState = (): NeedsYouInboxWorkspaceState => ({ hasMore: false, status: "idle", appliedGeneration: 0, + lastAppliedOk: false, }); // Typed against the full `AppState` (the zustand slices-pattern shape), not @@ -63,6 +64,7 @@ export const createNeedsYouInboxSlice = (set: ImmerSet): NeedsYouInboxSlice => ( hasMore: page.hasMore, status: "ready", appliedGeneration: generation, + lastAppliedOk: true, }; }), @@ -93,6 +95,7 @@ export const createNeedsYouInboxSlice = (set: ImmerSet): NeedsYouInboxSlice => ( hasMore: seed.hasMore, status: "idle", appliedGeneration: 0, + lastAppliedOk: false, }; }), diff --git a/apps/web/lib/state/slices/needs-you-inbox/selectors.ts b/apps/web/lib/state/slices/needs-you-inbox/selectors.ts index b17435ad5d..af0020ba5b 100644 --- a/apps/web/lib/state/slices/needs-you-inbox/selectors.ts +++ b/apps/web/lib/state/slices/needs-you-inbox/selectors.ts @@ -12,6 +12,7 @@ const EMPTY_WORKSPACE_STATE: NeedsYouInboxWorkspaceState = { hasMore: false, status: "idle", appliedGeneration: 0, + lastAppliedOk: false, }; function selectActiveWorkspaceState(state: AppState): NeedsYouInboxWorkspaceState { @@ -44,6 +45,14 @@ export function selectNeedsYouInboxRevision(state: AppState): number { return selectActiveWorkspaceState(state).appliedGeneration; } +/** Whether a successful page (as opposed to an error, or no response yet) is + * the last thing actually applied to this workspace's rows -- see + * `resolveViewMode`'s use of this for why `status`/`appliedGeneration` alone + * cannot answer it during a refresh. */ +export function selectNeedsYouInboxLastAppliedOk(state: AppState): boolean { + return selectActiveWorkspaceState(state).lastAppliedOk; +} + export function selectNeedsYouInboxHasMore(state: AppState): boolean { return selectActiveWorkspaceState(state).hasMore; } diff --git a/apps/web/lib/state/slices/needs-you-inbox/types.ts b/apps/web/lib/state/slices/needs-you-inbox/types.ts index 83f7c1a10e..d6c1c6a89c 100644 --- a/apps/web/lib/state/slices/needs-you-inbox/types.ts +++ b/apps/web/lib/state/slices/needs-you-inbox/types.ts @@ -23,6 +23,13 @@ export type NeedsYouInboxWorkspaceState = { // The generation of the last response actually applied to this workspace's // rows, for the stale-response guard. appliedGeneration: number; + // Whether the last applied response was a successful page, as opposed to an + // error or no response ever having applied. `status` alone cannot answer + // this during a refresh: `beginNeedsYouInboxRead` overwrites `status` to + // "loading" without touching what preceded it, so a refresh that follows a + // failed read and a refresh that follows a successful one are otherwise the + // same tuple. + lastAppliedOk: boolean; }; export type NeedsYouInboxSliceState = { diff --git a/docs/plans/needs-you-inbox/plan.md b/docs/plans/needs-you-inbox/plan.md index 4a7bf53103..0c6465dc94 100644 --- a/docs/plans/needs-you-inbox/plan.md +++ b/docs/plans/needs-you-inbox/plan.md @@ -53,8 +53,11 @@ document's own "Out of scope" section for the full list. - [x] [Task 01: Needs-you Inbox delivery](task-01-needs-you-inbox-delivery.md) - [x] [Task 02: Inbox feature-toggle terminology](task-02-inbox-label-alignment.md) +- [x] [Task 03: Fix background-refresh loading state](task-03-fix-background-refresh-loading-state.md) Task 02 depends on Task 01 and records the follow-up display-name alignment. +Task 03 depends on Task 01 and fixes a `resolveViewMode` / WS-coalescing +regression against AC .19-.21; see its own file for scope and verification. ## Verification results @@ -74,3 +77,15 @@ route, and runtime behavior unchanged. The runtime flag and public configuration reference now use `Inbox`, matching the destination copy contract in Part 3. Follow-up PR: https://github.com/kdlbs/kandev/pull/3673 + +## Task 03 results + +`resolveViewMode` could not distinguish a background refresh that followed a +successful read from one that followed a failure, so the Inbox re-flashed +"Loading..." on every WS/periodic refresh trigger and never resolved at all +with no active workspace. Fixed via a `lastAppliedOk` slice field that +survives the `status -> "loading"` overwrite a refresh performs, a +`hasActiveWorkspace` short-circuit to the empty view, and trailing-edge +queueing for WS refresh triggers that land inside the coalescing window. + +Follow-up PR: https://github.com/kdlbs/kandev/pull/3685 diff --git a/docs/plans/needs-you-inbox/task-03-fix-background-refresh-loading-state.md b/docs/plans/needs-you-inbox/task-03-fix-background-refresh-loading-state.md new file mode 100644 index 0000000000..4d0bab1720 --- /dev/null +++ b/docs/plans/needs-you-inbox/task-03-fix-background-refresh-loading-state.md @@ -0,0 +1,92 @@ +--- +id: "03-fix-background-refresh-loading-state" +title: "Fix Needs-you Inbox background-refresh loading state" +status: done +wave: 2 +depends_on: + - "01-needs-you-inbox-delivery" +plan: "plan.md" +requirements: + - REQ-UI-NEEDS-YOU-INBOX-001 +acceptance_criteria: + - AC-UI-NEEDS-YOU-INBOX-001.19 + - AC-UI-NEEDS-YOU-INBOX-001.20 + - AC-UI-NEEDS-YOU-INBOX-001.21 +system_design: + - ../../specs/ui/system-design/needs-you-inbox-02.md +--- + +# Task 03: Fix background-refresh loading state + +## Summary + +`resolveViewMode` (`apps/web/app/needs-you-inbox/needs-you-inbox-page-client.tsx`) +implemented AC .20/.21 by branching on `status` alone, which cannot tell a +refresh that just succeeded from one that just failed: both leave `status` +momentarily at `"loading"` while the request that will resolve it is in +flight. The observable defect was the Inbox re-showing "Loading..." on every +background refresh trigger from +[Control flow](../../specs/ui/system-design/needs-you-inbox-02.md#control-flow) +(WS event, reconnect, tab-visibility regain, or the 60-second periodic +re-read) even when the list was already settled and empty, and — with no +active workspace resolved — never leaving the loading view at all, because no +read is ever issued to resolve it. + +Separately, the WS refresh trigger's coalescing window was leading-edge only: +an `ws.ActionSessionPendingActionChanged` / `ws.ActionSessionStateChanged` +event that landed inside the 250 ms window was dropped rather than queued, +so the re-read AC .19 requires ("no exit shall be able to strand a row by +emitting no event... the row set shall converge from re-reading the bundle +path") did not always run. + +## In scope + +- `needsYouInbox` slice: add `lastAppliedOk` (true after a successful list + apply, false after an error, boot-seed, or default), surviving the + `status -> "loading"` overwrite a refresh performs. +- `resolveViewMode(status, bundleCount, hasMore, lastAppliedOk, +hasActiveWorkspace)`: gate the loading view on `!lastAppliedOk` instead of + `status` alone, and short-circuit to the empty view when + `hasActiveWorkspace === false` (previously: no trigger ever re-resolves the + view, so the loading state was permanent). +- `use-needs-you-inbox-controller.ts` (`useNeedsYouInboxWsRefresh`): keep the + leading-edge read but queue one trailing read for a trigger that lands + inside the coalescing window, instead of dropping it. +- E2E: `apps/web/e2e/tests/chat/needs-you-inbox-background-refresh.spec.ts`. + +## Out of scope + +Any change to the read contract, the row/count derivation, or the five +refresh triggers themselves (still exactly the ones +[Control flow](../../specs/ui/system-design/needs-you-inbox-02.md#control-flow) +names). This task fixes how the client renders the interval between a +trigger firing and its response landing; it defines no new trigger and reads +no new field. + +## Acceptance + +- AC .19: a WS refresh trigger landing inside the coalescing window is queued + and still runs, rather than being silently dropped — regression test in + `use-needs-you-inbox-controller.test.tsx`. +- AC .20: with `hasActiveWorkspace === false`, the Inbox renders the empty + view rather than an unresolvable loading view. +- AC .21: after a failed read settles with `status === "error"`, + `resolveViewMode` renders the error view and never claims that the Inbox is + empty. While a retry is pending (`status === "loading"` and + `lastAppliedOk === false`), it renders the loading view until the retry + succeeds or fails. + +## Verification + +- `npx vitest run` (3 touched files): 36/36 passed. +- `pnpm run typecheck`: clean. +- `pnpm e2e:run --project chromium -- tests/chat/needs-you-inbox-background-refresh.spec.ts`: + passed. +- Manual dev-instance check (flag forced on): empty state settles with no + loading flash on first load, reload, or background-refresh tick. +- Own analysis enumerated all 12 reachable `(status, bundleCount, hasMore, +lastAppliedOk, hasActiveWorkspace)` tuples against `resolveViewMode`, all + correct; independently cross-checked by a second model with no production + defect found. + +PR: https://github.com/kdlbs/kandev/pull/3685