fix: stop the needs-you inbox from flashing Loading on background refresh (#3685)

Co-authored-by: nova28 <17953305+nova28@users.noreply.github.com> Co-authored-by: Carlos Florencio <carlosmflorencio@gmail.com>
2026-09-16 01:52:53 +08:00
parent b586a80cc3
commit 186e354116
12 changed files with 621 additions and 38 deletions
@@ -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(<NeedsYouInboxPageClient />);

-    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(<NeedsYouInboxPageClient />);

-    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(<NeedsYouInboxPageClient />);

    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(<NeedsYouInboxPageClient />);
+
+    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(<NeedsYouInboxPageClient />);
+
+    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(<NeedsYouInboxPageClient />);
+
+    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(<NeedsYouInboxPageClient />);
+
+    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(<NeedsYouInboxPageClient />);
+
+    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(<NeedsYouInboxPageClient />);
+
+    expect(screen.getByTestId(EMPTY_TESTID)).not.toBeNull();
+    expect(screen.queryByRole("status")).toBeNull();
+  });
 });
@@ -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 (
    <PageShell title={t("sidebar:inbox")} contentClassName="space-y-4 p-6">
@@ -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<void>((resolve) => {
+      resolveHeld = resolve;
+    });
+    let resolveReleaseAll: (() => void) | undefined;
+    const releaseAll = new Promise<void>((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();
+  });
+});
@@ -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<string, Set<WsHandler>>(),
+}));
+
+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<WsHandler>();
+      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);
+  });
+});
@@ -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<void>,
+  storeApi: ReturnType<typeof useAppStoreApi>,
+) {
+  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<number | undefined>(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);
@@ -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<void>;

@@ -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");
@@ -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,
      };
    }),

@@ -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;
 }
@@ -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 = {
@@ -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
@@ -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