From 022c87033ccb20762d59b5f31c1144a870729e31 Mon Sep 17 00:00:00 2001 From: hshum Date: Fri, 17 Jul 2026 12:34:26 -0700 Subject: [PATCH 1/4] fix(scratchnode): repair red handoff tests and close CI allowlist gap Three ScratchnodeEventsSurface tests failed on clean main with a TypeError because the api mock predated ImportRecapButton's domains.product.scratchnodeImport query, and CI never saw the red because the runtime-smoke job runs an explicit allowlist that omitted the file. The api mock is now a Proxy that degrades unknown function references to unresolved queries (the surface's honest gates render nothing) instead of crashing at property access, the convex/react mock gains the useMutation the component already imports, two scenarios cover the import affordance's published/unresolved gates, and the file joins the runtime-smoke allowlist so future drift turns CI red. Closes #567 Co-Authored-By: Claude Fable 5 --- .github/workflows/ci.yml | 1 + .../ScratchnodeEventsSurface.test.tsx | 88 +++++++++++++++++-- 2 files changed, 83 insertions(+), 6 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 5a17d3d6d..69b42ba47 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -102,6 +102,7 @@ jobs: scripts/__tests__/releaseWorkflowContracts.test.ts convex/domains/redesign/chatRuns.responseShape.test.ts src/features/redesign/components/UniversalComposer.test.tsx + src/features/redesign/surfaces/ScratchnodeEventsSurface.test.tsx scratchnode-launch-gates: name: ScratchNode launch gates diff --git a/src/features/redesign/surfaces/ScratchnodeEventsSurface.test.tsx b/src/features/redesign/surfaces/ScratchnodeEventsSurface.test.tsx index b12320a11..b168ec8d6 100644 --- a/src/features/redesign/surfaces/ScratchnodeEventsSurface.test.tsx +++ b/src/features/redesign/surfaces/ScratchnodeEventsSurface.test.tsx @@ -19,24 +19,52 @@ import { MemoryRouter } from "react-router-dom"; type MockedResponses = { listMyJoinedEvents?: unknown; listMyNotes?: unknown; + scratchnodeImportStatus?: unknown; }; let mockedResponses: MockedResponses = {}; +const mockRunImport = vi.fn(async () => ({ ok: true })); vi.mock("convex/react", () => ({ useQuery: (fnRef: unknown, args: unknown) => { if (args === "skip") return undefined; if (fnRef === "fn:listMyJoinedEvents") return mockedResponses.listMyJoinedEvents; if (fnRef === "fn:listMyNotes") return mockedResponses.listMyNotes; + if (fnRef === "fn:getScratchnodeImportStatus") return mockedResponses.scratchnodeImportStatus; return undefined; }, + useMutation: () => mockRunImport, })); -vi.mock("../../../../convex/_generated/api", () => ({ - api: { - scratchnodeHandoff: { listMyJoinedEvents: "fn:listMyJoinedEvents" }, - notes: { listMyNotes: "fn:listMyNotes" }, - }, -})); +// The api mock is a Proxy so that a component adding a NEW `(api as any).x.y.z` +// reference degrades to an unknown-function query (useQuery returns undefined, +// the surface's honest gates render nothing) instead of throwing a TypeError at +// property access. That exact TypeError shipped red on main for three tests +// while CI's allowlist never ran this file (issue #567). Known functions still +// resolve to stable "fn:" strings so responses stay keyed by name. +vi.mock("../../../../convex/_generated/api", () => { + // Declared inside the factory: vi.mock is hoisted above top-level statements. + const KNOWN_API_FNS: Record = { + "scratchnodeHandoff.listMyJoinedEvents": "fn:listMyJoinedEvents", + "notes.listMyNotes": "fn:listMyNotes", + "domains.product.scratchnodeImport.getScratchnodeImportStatus": + "fn:getScratchnodeImportStatus", + "domains.product.scratchnodeImport.importPublishedWiki": "fn:importPublishedWiki", + }; + const apiNode = (path: string): unknown => { + const known = KNOWN_API_FNS[path]; + if (known) return known; + return new Proxy( + {}, + { + get: (_target, prop) => { + if (typeof prop !== "string") return undefined; + return apiNode(path ? `${path}.${prop}` : prop); + }, + }, + ); + }; + return { api: apiNode("") }; +}); import { ScratchnodeEventsSurface } from "./ScratchnodeEventsSurface"; @@ -228,4 +256,52 @@ describe("ScratchnodeEventsSurface — step 9 NodeBench handoff", () => { expect(queryByTestId("scratchnode-events-empty-no-events")).toBeNull(); expect(queryByTestId("scratchnode-events-empty-no-session")).toBeNull(); }); + + /** + * Scenario 7 — Attendee whose event has a published recap wiki. + * User: returning attendee; the host published the event wiki and the + * attendee wants it inside NodeBench. + * Goal: see the import affordance on that event row and none on rows + * without a published wiki (honest gate: no affordance without a + * KNOWN published recap). + * Failure mode covered: the ImportRecapButton query reads an api path + * (domains.product.scratchnodeImport) that this file's api mock did + * not model — three tests crashed with a TypeError on clean main + * while CI's runtime-smoke allowlist never ran the file (issue #567). + */ + it("renders the import-recap affordance only when a published wiki exists", () => { + setMockedResponses({ + listMyJoinedEvents: { joined: SAMPLE_EVENTS, _truncated: false }, + scratchnodeImportStatus: { + published: true, + imported: false, + documentId: null, + entitySlug: null, + }, + }); + const { getAllByTestId } = renderSurface("session-power-user-9"); + // The status mock is keyed by function (not per-slug), so every rendered + // row reports a published recap — assert one affordance per row. + expect(getAllByTestId(/scratchnode-import-recap-/)).toHaveLength( + SAMPLE_EVENTS.length, + ); + }); + + /** + * Scenario 8 — Import status still loading (or wiki unpublished). + * User: attendee opening the list the moment after join; status query + * has not resolved. + * Goal: no import affordance flashes before the published state is + * KNOWN (mirrors the component's "honest gate" comment). + * Failure mode covered: affordance rendered from undefined status → + * click on a recap that may not exist. + */ + it("renders no import affordance while import status is unresolved", () => { + setMockedResponses({ + listMyJoinedEvents: { joined: SAMPLE_EVENTS, _truncated: false }, + // scratchnodeImportStatus intentionally absent → useQuery undefined + }); + const { queryByTestId } = renderSurface("session-power-user-9"); + expect(queryByTestId(/scratchnode-import-recap-/)).toBeNull(); + }); }); From 5b0b55b45a46a531078f4c0f0712c7bea9407141 Mon Sep 17 00:00:00 2001 From: hshum Date: Fri, 17 Jul 2026 12:36:29 -0700 Subject: [PATCH 2/4] fix(composer): make run cancellation spatially stable, not timer-only MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The submit->Stop same-slot swap meant a double-click slower than the 400ms arming window (Windows accessibility settings allow ~900ms) could still cancel the run it just created. Stop and submit are now both always rendered: Stop holds a reserved slot (visibility:hidden while idle) and submit stays at identical coordinates while streaming, merely disabled — so the second click of a double-click lands on an inert control at every interval. The arming delay remains as defense in depth. Closes #568 Co-Authored-By: Claude Fable 5 --- .../components/UniversalComposer.test.tsx | 17 ++++- .../redesign/components/UniversalComposer.tsx | 65 ++++++++++++------- 2 files changed, 56 insertions(+), 26 deletions(-) diff --git a/src/features/redesign/components/UniversalComposer.test.tsx b/src/features/redesign/components/UniversalComposer.test.tsx index ee5eb3fff..476e8e2b4 100644 --- a/src/features/redesign/components/UniversalComposer.test.tsx +++ b/src/features/redesign/components/UniversalComposer.test.tsx @@ -59,9 +59,17 @@ describe("UniversalComposer runtime controls", () => { fireEvent.click(screen.getByRole("button", { name: "Run research" })); expect(onSubmit).toHaveBeenCalledTimes(1); - // The parent flips streaming after accepting the first click. The control at - // the same coordinates is now Stop, but it stays inert through dblclick #2. + // The parent flips streaming after accepting the first click. The submit + // button STAYS at the same coordinates — disabled, so the second click of + // a double-click is structurally a no-op at any double-click interval. + // Stop lives in its own reserved slot and is also arm-delayed. rerender(); + const submit = screen.getByRole("button", { name: "Run research" }); + expect(submit).toBeDisabled(); + fireEvent.click(submit); + expect(onSubmit).toHaveBeenCalledTimes(1); + expect(onStop).not.toHaveBeenCalled(); + const stop = screen.getByRole("button", { name: "Cancel active run" }); expect(stop).toBeDisabled(); fireEvent.click(stop); @@ -70,6 +78,11 @@ describe("UniversalComposer runtime controls", () => { act(() => vi.advanceTimersByTime(CANCEL_ARM_DELAY_MS)); expect(stop).toBeEnabled(); + // Even armed, the submit slot stays inert — the double-click landing zone + // never becomes a cancel control (issue #568's structural requirement). + fireEvent.click(submit); + expect(onSubmit).toHaveBeenCalledTimes(1); + expect(onStop).not.toHaveBeenCalled(); // Escape remains usable even when focus is no longer in the textarea. fireEvent.keyDown(document, { key: "Escape" }); diff --git a/src/features/redesign/components/UniversalComposer.tsx b/src/features/redesign/components/UniversalComposer.tsx index 1d822be93..db9e40ac8 100644 --- a/src/features/redesign/components/UniversalComposer.tsx +++ b/src/features/redesign/components/UniversalComposer.tsx @@ -718,23 +718,41 @@ export function UniversalComposer({ Chat now )} - {streaming && onStop ? ( + {/* Stop and submit are BOTH always rendered so neither ever moves: + the Stop slot is reserved (visibility:hidden) while idle, and the + submit button stays at identical coordinates while streaming — + merely disabled. The second click of a double-click therefore + lands on the inert submit, never on Stop, regardless of the + user's double-click interval. The CANCEL_ARM_DELAY_MS arming + below stays as defense in depth for pointer replays that do + land on Stop (e.g. Escape spam, automation). */} + {onStop ? ( - ) : ( - - )} + ) : null} + {onRunOnList && batchTargets.length > 0 && (