From 2dbcfc6c82305f0bf724d2487e1abe7fdeec2e66 Mon Sep 17 00:00:00 2001 From: "Molecule AI Dev Engineer A (Kimi)" Date: Sat, 13 Jun 2026 21:24:07 +0000 Subject: [PATCH] fix(mobile-inbox): clear rows on tab switch and derive actions from row.kind (#2766) - RequestsInbox now clears items immediately when changes and uses a generation counter so stale responses from a previous tab cannot overwrite the list after a tab switch. - RequestItem derives its layout and primary action from instead of the selected tab, so an approval row can never be actioned as action="done" even if it somehow renders under Tasks. - Added regression tests for immediate clear, row-kind-derived actions, and ignoring a stale fetch that resolves after switching tabs. Fixes #2766 Test plan: - npx vitest run src/components/concierge/__tests__/RequestsInbox.test.tsx - npx vitest run src/components/concierge Co-Authored-By: Claude --- .../components/concierge/RequestsInbox.tsx | 36 +++++++++-- .../__tests__/RequestsInbox.test.tsx | 61 ++++++++++++++++++- 2 files changed, 90 insertions(+), 7 deletions(-) diff --git a/canvas/src/components/concierge/RequestsInbox.tsx b/canvas/src/components/concierge/RequestsInbox.tsx index b4d452e25..76a344bf2 100644 --- a/canvas/src/components/concierge/RequestsInbox.tsx +++ b/canvas/src/components/concierge/RequestsInbox.tsx @@ -111,6 +111,11 @@ export function RequestsInbox({ kind, onCountChange }: RequestsInboxProps) { // TODO(multi-user): when the canvas grows real per-action attribution, plumb // the acting user through props instead of a single module-level resolve. const responderIdRef = useRef("admin"); + // core#2766: monotonic generation counter so a stale `/requests/pending` + // response from a previous `kind` cannot overwrite the list after a tab + // switch (or after a later live-refresh load). + const loadGenRef = useRef(0); + useEffect(() => { let cancelled = false; fetchSession() @@ -123,14 +128,31 @@ export function RequestsInbox({ kind, onCountChange }: RequestsInboxProps) { return () => { cancelled = true; }; }, []); + // Increment the generation on unmount so any in-flight load cannot call + // setItems after this component instance is gone. + useEffect(() => { + return () => { + loadGenRef.current += 1; + }; + }, []); + const load = useCallback(() => { + const gen = ++loadGenRef.current; + // core#2766: clear stale rows the moment the kind changes so the user + // never sees approval cards under the Tasks tab (or vice-versa) while the + // new list is still fetching. This prevents the wrong-action race where a + // stale approval row could be actioned with action="done". + setItems([]); + onCountChange?.(0); api.get(`/requests/pending?kind=${kind}`) .then((r) => { + if (gen !== loadGenRef.current) return; // stale response const list = r ?? []; setItems(list); onCountChange?.(list.length); }) .catch(() => { + if (gen !== loadGenRef.current) return; // stale response setItems([]); onCountChange?.(0); }); @@ -197,7 +219,6 @@ export function RequestsInbox({ kind, onCountChange }: RequestsInboxProps) { void; @@ -223,10 +243,14 @@ interface RequestItemProps { * approval layout reuses the .apprCard classes. Both share the inline * More-Info thread panel. */ function RequestItem({ - row, kind, acting, threadOpen, onRespond, onToggleThread, responderId, + row, acting, threadOpen, onRespond, onToggleThread, responderId, }: RequestItemProps) { const badge = statusLabel(row.status); - const isApproval = kind === "approval"; + // core#2766: derive the card layout and primary action from the ROW'S kind, + // not from the selected tab. During a tab switch the old tab's rows are + // cleared, but if any stale render slips through it must not expose a + // task "Done" button on an approval row. + const isApproval = row.kind === "approval"; // "Responder identity" on a resolved row (shows only if a resolved item // ever renders in this pending view — defensive, since pending excludes @@ -296,7 +320,7 @@ function RequestItem({ if (isApproval) { return ( -
+
@@ -320,7 +344,7 @@ function RequestItem({ } return ( -
+
diff --git a/canvas/src/components/concierge/__tests__/RequestsInbox.test.tsx b/canvas/src/components/concierge/__tests__/RequestsInbox.test.tsx index 443afbd27..8ed06260a 100644 --- a/canvas/src/components/concierge/__tests__/RequestsInbox.test.tsx +++ b/canvas/src/components/concierge/__tests__/RequestsInbox.test.tsx @@ -12,7 +12,7 @@ * vi.resetModules() in afterEach undoes the mocks for other files. */ import React from "react"; -import { render, screen, fireEvent, cleanup, act } from "@testing-library/react"; +import { render, screen, fireEvent, cleanup, act, waitFor } from "@testing-library/react"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { RequestsInbox, type RequestRow } from "../RequestsInbox"; import { showToast } from "@/components/Toaster"; @@ -185,6 +185,65 @@ describe("RequestsInbox — More Info thread", () => { }); }); +describe("RequestsInbox — tab-switch wrong-action race (core#2766)", () => { + it("clears stale rows immediately on kind switch before the new fetch resolves", async () => { + mockApiGet.mockResolvedValue([approvalRow("a1")]); + const { rerender } = render(); + await waitFor(() => + expect(screen.getByText("Delete production volume")).toBeTruthy(), + ); + expect(screen.getByRole("button", { name: /approve/i })).toBeTruthy(); + + // Switch to the Tasks tab with a fetch that never resolves in this test. + mockApiGet.mockImplementation(() => new Promise(() => {})); + rerender(); + + // The stale approval row must be gone instantly; otherwise the user could + // see approval cards under the Tasks tab and the wrong primary action. + expect(screen.queryByText("Delete production volume")).toBeNull(); + expect(screen.queryByTestId("request-item")).toBeNull(); + expect(screen.queryByRole("button", { name: /approve/i })).toBeNull(); + }); + + it("ignores a stale approval fetch that resolves after switching to Tasks", async () => { + let resolveApproval: (value: RequestRow[]) => void = () => {}; + mockApiGet.mockImplementation((path: string) => { + if (path === "/requests/pending?kind=approval") { + return new Promise((res) => { + resolveApproval = res; + }); + } + return Promise.resolve([]); + }); + + const { rerender } = render(); + // The approval fetch is in flight but has not resolved yet. + expect(screen.queryByText("Delete production volume")).toBeNull(); + + // Switch to Tasks before the approval response lands. + rerender(); + + // The old approval response finally arrives; it must be ignored. + await act(async () => { + resolveApproval([approvalRow("a1")]); + }); + expect(screen.queryByText("Delete production volume")).toBeNull(); + expect(screen.queryByTestId("request-item")).toBeNull(); + expect(screen.queryByRole("button", { name: /approve/i })).toBeNull(); + }); + + it("derives action buttons from row.kind, not the selected tab", async () => { + // Defensive: even if a mismatched row somehow reaches the list, its action + // must be driven by row.kind so an approval can never be actioned as "done". + mockApiGet.mockResolvedValue([approvalRow("a1")]); + await act(async () => { render(); }); + + expect(screen.queryByRole("button", { name: /done/i })).toBeNull(); + expect(screen.getByRole("button", { name: /approve/i })).toBeTruthy(); + expect(screen.getByRole("button", { name: /reject/i })).toBeTruthy(); + }); +}); + describe("RequestsInbox — live refresh + toasts", () => { it("subscribes to the socket bus on mount", async () => { await act(async () => { render(); }); -- 2.52.0