From aca25494a5817518e745bd471be294263aca28a0 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Thu, 30 Jul 2026 15:07:45 -0700 Subject: [PATCH] =?UTF-8?q?test(dashboard):=20App.test.tsx=20is=20fully=20?= =?UTF-8?q?green=20=E2=80=94=20an=20incomplete=20mock=20was=20crashing=20N?= =?UTF-8?q?ewTaskModal=20(#2846)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #2829. `App.test.tsx` has been red since **2026-07-25**; #2833 took it 10 failures → 2, and this takes it to **0**. ## The cause, and it was not what I guessed `vi.mock("../../hooks/useViewportMode")` did not export `isShortViewport`, which `NewTaskModal` imports. An incomplete module mock **does not fail at the mock**. It throws inside whichever component imports the missing export, and the nearest `ErrorBoundary` swallows that into *"This section encountered an error"* — so the test failed on a **missing heading** with a DOM that looks perfectly healthy. Structurally the same presentation as the workflow-skeleton bug in #2833: the real failure sits two layers below what the assertion reports. Both remaining tests had this one cause. ## Found by probing, not theorising I had already been wrong twice on this file (`renders nothing`, then i18n), and my standing note said the modal was "plausibly FN-8620's FloatingWindow rework, **unverified**". That would have been a third wrong guess. Instead I dumped what was actually in the DOM at the assertion point: ``` headings: ["Fusion"] | dialogs: 0 newtask-ish: [..., "error-boundary error-boundary--modal"] ``` The `error-boundary--modal` class was the entire answer; reading its text gave the exact missing export. Same technique that cracked #2833 in one shot — the difference between the two halves of this investigation is that I stopped guessing. **App.test.tsx: 141 passed (141).** ## Also patched, and one deliberately not - **`Header.mobile-project-favorites.test.tsx`** mocks the same module with an inline factory and lacks the export. It is **latent, not broken** — its component does not import `isShortViewport` yet, and the day it does, the failure would be this same swallowed crash. One line. - **`FloatingWindow.touch-geometry.test.tsx`** greps as missing but is **not** — it uses `{ ...(await vi.importActual(...)) }`, so its surface is complete by construction. My grep-based classification was a false positive. That spread is the pattern that prevents this class outright, and it is the better fix **where it is available**. It is not available here: App.test.tsx's mock exists precisely so tests need no `window.matchMedia` in jsdom, and spreading the real module would reintroduce that dependency for every unstubbed export. So the scoped one-line stub is the right fix for this file, and the spread is the recommendation for new mocks. A guard asserting "a module mock exports everything the real module does" would close this class repo-wide. I did not build it — 34 files mock this module and only one was genuinely wrong, so the population does not yet justify another instrument, by the same measure-first rule that killed two other candidate gates this week. ## Verification App.test.tsx 141/141 · the two touched siblings 9/9 · `tsc` 0 · lint 0. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) --- .../app/components/__tests__/App.test.tsx | 29 +++++++++++++++++++ .../Header.mobile-project-favorites.test.tsx | 4 +++ 2 files changed, 33 insertions(+) diff --git a/packages/dashboard/app/components/__tests__/App.test.tsx b/packages/dashboard/app/components/__tests__/App.test.tsx index 2b47232e1c..f0476183e8 100644 --- a/packages/dashboard/app/components/__tests__/App.test.tsx +++ b/packages/dashboard/app/components/__tests__/App.test.tsx @@ -639,6 +639,8 @@ vi.mock("../../hooks/useMobileKeyboard", () => ({ // Mock useViewportMode so tests can simulate mobile viewport without // depending on window.matchMedia in jsdom. const mockUseViewportMode = vi.fn(() => "desktop"); +/* `(max-height: 480px)` in production — independent of the width-driven mode. See the note below. */ +const mockIsShortViewport = vi.fn(() => false); vi.mock("../../hooks/useViewportMode", () => ({ MOBILE_MEDIA_QUERY: "(max-width: 768px), (max-height: 480px)", isTabletTouchViewport: (mode?: string) => mode === "tablet", @@ -646,6 +648,30 @@ vi.mock("../../hooks/useViewportMode", () => ({ getViewportMode: () => mockUseViewportMode(), isMobileViewport: () => mockUseViewportMode() === "mobile", isFullScreenSheetViewport: () => mockUseViewportMode() === "mobile", + /* + FNXC:TestViewportMock 2026-07-30-11:20: + An INCOMPLETE module mock does not fail where the export is missing — it throws inside whichever + component imports it, and the nearest ErrorBoundary swallows that into "This section encountered an + error". NewTaskModal adopted `isShortViewport`, this mock did not, and the test failed on a MISSING + HEADING with a healthy-looking DOM. + + FNXC:TestViewportMock 2026-07-30-19:50 (#2846 review — greptile P2, "viewport predicates are conflated"): + SHORT-VIEWPORT IS ITS OWN CONTROL, because in production it is its own MEDIA QUERY. + + The first version keyed it to `mode === "mobile"`, matching how the siblings above are stubbed. The + siblings are width predicates and the mode IS their answer; this one is not. `isShortViewport()` + reads `(max-height: 480px)` alone, while the mobile mode is the OR of width and height — so an + ordinary PORTRAIT PHONE (narrow, tall) is mobile and NOT short, and the mock claimed it was both. + + What that silently mis-tested: `FloatingWindow` suspends geometry PERSISTENCE on a short viewport, + and `PlanningModeModal` picks its compact interview layout and hides the session list from it. Every + mobile test here took those branches, so the ordinary phone case — the most common real viewport — + was never actually exercised, and a regression in the non-short mobile path would have passed. + + Defaults to FALSE rather than to the mode: a test that means "short" now has to say so, which is the + only spelling that can distinguish the two. + */ + isShortViewport: () => mockIsShortViewport(), })); // Mock isIOS so FN-3290 keyboard-open behavior is testable in jsdom @@ -824,6 +850,9 @@ beforeEach(() => { }); mockUseViewportMode.mockReset(); mockUseViewportMode.mockReturnValue("desktop"); + /* Reset alongside the mode: it is a SEPARATE predicate, so a suite that sets it must not leak. */ + mockIsShortViewport.mockReset(); + mockIsShortViewport.mockReturnValue(false); mockAgentStats.todoTaskCount = 0; mockAgentStats.idleNonEphemeralCount = 1; }); diff --git a/packages/dashboard/app/components/__tests__/Header.mobile-project-favorites.test.tsx b/packages/dashboard/app/components/__tests__/Header.mobile-project-favorites.test.tsx index 5f88738be6..5fb9e28aa6 100644 --- a/packages/dashboard/app/components/__tests__/Header.mobile-project-favorites.test.tsx +++ b/packages/dashboard/app/components/__tests__/Header.mobile-project-favorites.test.tsx @@ -14,6 +14,10 @@ vi.mock("../../api", async (importOriginal) => ({ vi.mock("../../hooks/useViewportMode", () => ({ isTabletTouchViewport: (mode?: string) => mode === "tablet", useViewportMode: () => "mobile", + /* FNXC:TestViewportMock 2026-07-30-11:30: keep this mock's surface complete. A missing export does + not fail here — it throws inside the importing component and the nearest ErrorBoundary turns it + into a missing element, which is how App.test.tsx stayed red for five days. */ + isShortViewport: () => false, })); function makeProject(id: string, name: string): ProjectInfo {