diff --git a/docs/branch-review-ledger.md b/docs/branch-review-ledger.md index 2740e31d4..f0a9ec7db 100644 --- a/docs/branch-review-ledger.md +++ b/docs/branch-review-ledger.md @@ -1305,3 +1305,6 @@ Records before 2026-07-28 were written by hand and had drifted: 146 lines carrie | 2026-07-29 | 1391 | ff4ddfabb8d8594da0c09e04dc37e9286745140b | PR #1391 merge | merged as 39f2bcea. Three review P2s fixed: both ui-smoke retries made idempotent (toPass re-runs after a late-landing click; the mode-menu one clicked a toggle and oscillated) plus a second guard so the <768px branch can recover at all, and #111 archived instead of left open-and-done. One Production UI red on ui-phone-scroll.spec.ts:574 was proven a flake before re-running: the delta from the passing head baecef05 was two ui-smoke guards and one docs row, neither reachable from that spec, and the PR touches no document-viewer phone-scroll surface. Re-run green | verify:cheap exit 0 (429 files / 4404 tests); design-token orphan guard proven red on a reintroduced class; PR required success with UI_RESULT success on re-run | | 2026-07-29 | 1391 | baecef05cac86c4d52af895d483a33ba3c40cd61 | PR #1391 review | reviewed clean — text-4xs retirement confirmed against globals.css (--text-3xs 0.625rem present, --text-4xs absent); orphan guard proven to fail on a reintroduced class; six Playwright retries all retry action-plus-effect so a genuine regression still fails. Resolved the outstanding-issues #108/#109 double-allocation (renumbered to #110/#111, marker to 112) and recorded #111 done | verify:cheap exit 0 (429 files / 4404 tests); design-token-contract 28 passed; check:branch-review-ledger passed | | 2026-07-29 | 1374 | c14edb9c6f0bdbbfb147752503e016f2543fd803 | PR #1374 review + merge | merged as 3704007c — DocumentViewer identity-bound state clear (P1) implemented and verified red without it; all 12 review threads resolved | verify:cheap exit 0 (429 files / 4403 tests); PR required success; Production UI success | +| 2026-07-30 | claude/top-search-design-mockups-w53znc | 7577a1ea60ab5f0918885f90e849bbac754234b1 | PR #1394 search-results-band-adoption + #096/#115 | No P0/P1. Disposition1 partial: isAlwaysStandaloneShellPath fixes services/etc; /tools still layout-false-positive (P2). Disposition2 verified: import-as-rendered deferred as #115 (P3). #096 closure text accurate for root-path; row still open with stale Still-live clause. | vitest tests/search-results-band-adoption.test.ts 6/6; offline gutting repro tools vs services; static read search-route-ownership + outstanding-issues | +| 2026-07-30 | PR #1394 / `claude/top-search-design-mockups-w53znc` | `8c39158d99876338613d5bb3195847fd253ef5ff` | CI/review closeout: /tools page-only roots + thread disposition | FIXED. Layout false-positive for `/tools` closed via `isStandaloneModeHomePath` in reachabilityRoots. Import-as-rendered finding left as `#115` (pre-existing; lint catches the plausible slip). Both Codex threads dispositioned. Merge clean vs main. | vitest adoption 6/6; full unit 4451 passed / 4 skipped; typecheck; prettier; Bugbot pr-bugbot | +| 2026-07-30 | PR #1394 / `claude/top-search-design-mockups-w53znc` | `d658f120253ac0a2dbfe1c9b1a14c6a0744af14d` | CI/review closeout tip (supersedes 8c39158d row) | FIXED layout `/tools` false-positive; `#115` deferred with human disposition; threads resolved; merge-tree clean; CI settling | vitest adoption 6/6; full unit 4451/4; typecheck; Bugbot | diff --git a/docs/outstanding-issues.md b/docs/outstanding-issues.md index e70de00b7..0bdfc446a 100644 --- a/docs/outstanding-issues.md +++ b/docs/outstanding-issues.md @@ -81,7 +81,7 @@ removed after current-main verification; it is not missing recommended work. | 33 | `#103` | A3 | Operator — Supabase schema | Same window as `#102` | 30–60 minutes | Confirm whether the wide `document_table_facts` trigram index from `20260714190000` exists live, then either mirror it into `schema.sql` (retained) or drop it via a forward migration (redundant). **Not the allowlist** — it suppresses live-vs-`schema.sql` findings only and cannot make the migration chain and the mirror agree. Stop: do not drop it without live scan evidence. | | 34 | `#105` | Optional | High — browser/UI verification | When the heavy-run lock is free | 20–40 minutes | Run `verify:ui` over the ten `LoadingPanel` fallbacks and confirm the Supabase `preconnect` reaches `` on a live page. Implementation already shipped; this row is the outstanding verification only. | - + ## Open items @@ -134,7 +134,7 @@ removed after current-main verification; it is not missing recommended work. | #093 | P2 | issue | Next streaming `S:` clone causes Playwright strict-mode violations under CI load | **Outcome:** duplicate-element strict-mode failures stop appearing on loaded CI runs. **Detail:** under full-suite CI load Next.js leaves a hidden duplicate page root in the stream, so a `getByTestId` that is unique locally resolves to 2 elements in CI (seen as `differentials-search-results` on PR #1316, and previously noted on PR #1294 against main). It does not reproduce in isolation, on a single spec, or locally. The documented workaround is to scope the locator to the visible root. **Reproduced locally 2026-07-28** (isolated _production_ build via `run-playwright.mjs`, full `verify:ui`): `ui-tools.spec.ts:563` duplicated `forms-home` and `ui-smoke.spec.ts:3001` duplicated `favourite-row-lithium-monitoring-guideline`; in both, copy 1 is nested under `mobile-composer-reserve-pad`. Both pass when run alone, so it is load/order-dependent, not build-mode dependent — this also corrects an earlier note that CI uses `next dev`; it does not. **Strongest evidence (CI run `30345484316`, 2026-07-28): `ui-overlap.spec.ts:199` on `/` asserted `toHaveCount(1)` successfully and then the same `header#search` locator resolved to 2 a statement later, one of them hidden.** A duplicate that appears _after_ a passing count assertion is a stream/hydration artifact by construction, not a static double mount and not something a CSS or component change can cause. That makes four distinct testids across four specs with the identical shape. **Mitigated, not fixed, on `main` (2026-07-28):** `3a8edb93` rewrapped `gotoHome` in `tests/ui-overlap.spec.ts` to retry count-and-visibility together via `toPass`, so a transient second header no longer trips strict mode there — its own note says "checking count then immediately calling waitFor races that flicker into a strict-mode violation". That hardens one helper; the duplicate root itself is unchanged and other specs remain exposed. **Confirmed pre-existing:** at `631d90d2`, the commit before PR #1316's first commit, that spec already documented "two `header#search` nodes" and "a second transient `header#search` can exist briefly" — so this predates that branch. **Next:** with a full-suite repro now available, bisect the preceding specs to find the state that triggers the second mount, then either scope the shared helpers to the visible root once or fix the mount. **Stop:** do not paper over new occurrences with `.first()` before the duplicate itself is explained. | PR #1316 CI runs; PR #1294 note on main; session 2026-07-28 | 2026-07-28 | | #094 | P2 | rec | Design-system gates assert structure, not rendered effect | **Outcome:** a style contract cannot pass while the style is inert. **Detail:** PR #1316's accent rail shipped inert because `.search-band` sat in `@layer components`, which loses to Tailwind's utilities layer regardless of specificity — and the test asserted `toHaveClass("search-band")`, i.e. class presence, not effect. Computed style showed `1px rgb(229,231,235)` where `2px rgb(11,111,134)` was intended. The same shape of gap let a rail-colour assertion compare a colour against a width and pass unconditionally. **Next:** for contracts where the visual IS the requirement (rails, forced-colors thickness, tap targets), assert `getComputedStyle` in a Playwright case rather than class names in a DOM test, and add the unlayered-component convention to the design-system contract check. **Stop:** do not convert existing passing DOM tests wholesale; add computed-style proof only where the effect carries the meaning. | PR #1316 Codex P2 finding; session 2026-07-28 | 2026-07-28 | | #095 | P3 | issue | `PR required` reports failure for concurrency-cancelled jobs | **Outcome:** a red `PR required` means a real failure. **Detail:** the aggregate calls `require_success` on `coverage`/`production-ui`, so any push that supersedes an in-flight run reports `coverage result was cancelled` → exit 1, indistinguishable at a glance from a genuine failure. Eleven such reds were produced on PR #1316 in one session, and `Production UI` never once ran to completion. **Next:** in `.github/workflows/ci.yml`, either treat `cancelled` distinctly from `failure` in the aggregate, or reduce push frequency against long UI runs. **Stop:** do not relax `require_success` for genuine failures while doing so. | PR #1316 runs 30340972329 / 30341225585; session 2026-07-28 | 2026-07-28 | -| #096 | P2 | task | One PR #1316 review fix is still a live gap on `main` | **Outcome:** the two remaining PR #1316 review findings are fixed on `main` with tests. **Do not chase the commits.** The seven Codex follow-up SHAs (`ff5b682`, `77cfe12`, `9840ed9`, `81ffb86`, `a5d6561`, `967e16c`, `e544d0d`) are **unreachable** — `git fetch origin ` fails for all seven, no open PR or branch carries them, and none was in the squash merge `4bcfeb90`. They were authored in a sandbox on a branch named `work` and never pushed, so the "follow-up PR metadata" each reported does not exist. **Durable source:** the [PR #1316 review threads](https://github.com/BigSimmo/Database/pull/1316/files) persist and describe every fix with file and line detail; re-derive from those, not from the hashes. **Still live on `main` (verified 2026-07-28):** the band adoption gate skips query-backed root modes — `tests/search-results-band-adoption.test.ts:101` returns null for `pathOnly === "/"`, so `/?mode=prescribing` and Documents never enter the route inventory and the root dashboard page is unchecked. **Already fixed independently, no action:** favourites hub counts (`libraryCountsTrusted`), the document-search status derivation, the 401 session-expiry path, and the record-path duplicate notice. **Corrected 2026-07-28 — the Therapy Compass retry-waiter finding is NOT a live defect.** `use-therapy-data.ts:68` `retryWaitersRef` is genuinely unscoped, so a newer request can settle an older retry's promise, but no caller observes it: `useTherapyData` lives in the long-lived `TcProvider` (`bindings.tsx:206`) and `requestKey` derives only from `screen`, so it cannot change without the screen changing; the sole awaiting caller is the band's `AsyncButton` inside `search-screen.tsx:33`, which unmounts on that transition, and `workspace.tsx:36` uses `onClick={b.retryData}` which discards the promise. An earlier note here claimed a visible "Retry stops being busy" symptom — that was wrong and is retained only as the correction. It becomes real if a future caller ever awaits `retry()` from a control that survives a `requestKey` change. **Next:** resolve root-path and href-less modes to `src/app/(search-app)/page.tsx` in the adoption gate, with a negative fixture for a disconnected root route. **Stop:** not user-facing; do not let it block a release, and do not add waiter keying without a reproducer showing a still-mounted control whose busy state clears early. | PR #1316 review sweep; session 2026-07-28 | 2026-07-28 | +| #096 | P2 | task | PR #1316 review follow-ups — adoption-gate coverage closed | **Outcome:** the two remaining PR #1316 review findings are fixed on `main` with tests. **Do not chase the commits.** The seven Codex follow-up SHAs (`ff5b682`, `77cfe12`, `9840ed9`, `81ffb86`, `a5d6561`, `967e16c`, `e544d0d`) are **unreachable** — `git fetch origin ` fails for all seven, no open PR or branch carries them, and none was in the squash merge `4bcfeb90`. They were authored in a sandbox on a branch named `work` and never pushed, so the "follow-up PR metadata" each reported does not exist. **Durable source:** the [PR #1316 review threads](https://github.com/BigSimmo/Database/pull/1316/files) persist and describe every fix with file and line detail; re-derive from those, not from the hashes. **Was live on `main` through 2026-07-28:** the band adoption gate skipped query-backed root modes — `modeHrefToPagePath` returned null for `pathOnly === "/"`, so `/?mode=prescribing` and Documents never entered the route inventory and the root dashboard page was unchecked. Closed on PR #1394 (see Adoption-gate gap closed below). **Already fixed independently, no action:** favourites hub counts (`libraryCountsTrusted`), the document-search status derivation, the 401 session-expiry path, and the record-path duplicate notice. **Corrected 2026-07-28 — the Therapy Compass retry-waiter finding is NOT a live defect.** `use-therapy-data.ts:68` `retryWaitersRef` is genuinely unscoped, so a newer request can settle an older retry's promise, but no caller observes it: `useTherapyData` lives in the long-lived `TcProvider` (`bindings.tsx:206`) and `requestKey` derives only from `screen`, so it cannot change without the screen changing; the sole awaiting caller is the band's `AsyncButton` inside `search-screen.tsx:33`, which unmounts on that transition, and `workspace.tsx:36` uses `onClick={b.retryData}` which discards the promise. An earlier note here claimed a visible "Retry stops being busy" symptom — that was wrong and is retained only as the correction. It becomes real if a future caller ever awaits `retry()` from a control that survives a `requestKey` change. **Adoption-gate gap closed 2026-07-29.** Root-path and href-less modes now resolve to `src/app/(search-app)/page.tsx`. Closing it surfaced two further defects in the same gate that the original finding did not name: the hand-rolled walk was capped at two import hops while the root route's real chain is four (`layout -> shared-search-app-shell -> global-search-shell -> ClinicalDashboard -> document-search-results`), and it followed neither `layout.tsx` — which is where that route's band actually comes from, since the page renders only a pass-through — nor `dynamic(() => import(...))`, which is how the dashboard code-splits its mode workspaces. All three are fixed together with a bounded BFS; each was verified load-bearing by reverting it and watching the gate fail. **Stop:** not user-facing; do not let it block a release, and do not add waiter keying without a reproducer showing a still-mounted control whose busy state clears early. | PR #1316 review sweep; session 2026-07-28 | 2026-07-28 | | #097 | P3 | issue | Gitleaks reports a false red when the PR head moves mid-run | **Outcome:** a red `Gitleaks` means a secret was found, not that someone pushed. **Detail:** on 2026-07-28 the job triggered for head `9bace1d1` checked out that merge ref, then queried the API and built its range against head `40278453` — pushed seconds later and absent from the checkout. Git rejected the range (`fatal: Invalid revision range`), so it scanned `~0 bytes`, logged `no leaks found in partial scan`, and exited 1. The scan did not run at all, which is worse than a normal failure because the natural reading is "noise, ignore it". It cleared on its own once the head stopped moving (`23 commits scanned`, `~198 KB`, `no leaks found`). Both range endpoints resolve in any complete checkout — verified locally against the branch and the PR merge ref — so this is not a `fetch-depth` problem. **Next:** pin the scan to a range the job controls (`base.sha`..the checked-out head) instead of re-querying the API mid-run, so a concurrent push cannot invalidate it. **Related:** same push-churn family as #095. **Stop:** do not weaken the gate to a soft-pass; the fix is a stable range, not a tolerated failure. | PR #1316 runs 30344938800 / 30346797225; session 2026-07-28 | 2026-07-28 | | #098 | P2 | task | Offline round-trip budget harness for the hot routes | **Outcome:** per-scenario Supabase round-trip counts are pinned by a test, so an extra round trip on a hot path is a red gate rather than an inference. **Done 2026-07-29:** the measurement gap is closed — `Server-Timing` now covers `auth`/`ratelimit`/`scope` on `/api/answer`, `auth`/`ratelimit`/`search`/`total` on `/api/search`, and `auth`/`ratelimit` on `/api/answer/stream` (previously the route the UI actually calls emitted no header at all). Headers flush before the first SSE frame, so in-stream stages cannot reach a header and must NOT be routed through the governed `progress`/`final` contract. `tests/answer-route-preamble.test.ts` pins admission-before-scope (no scope call while the limiter is pending or after a deny) and the client-disconnect abort signal. **Next:** generalise it — wrap the Supabase client in a counting proxy and assert per-scenario query budgets over the existing offline suites — `scripts/eval-rag-offline.mjs`, `scripts/test-rag-offline.mjs`, `scripts/rag-offline-contract.mjs` and the contract fixture `scripts/fixtures/rag-offline-contract-tests.json`. **An earlier version of this row named `test-cache-path.mjs` and `check-rag-fixtures.mjs`** (corrected 2026-07-29, PR #1377 review, matching the audit's own retraction): neither exercises a RAG request — the first computes Vitest/TypeScript cache paths, the second only validates fixture manifests — so building the harness on them would have counted nothing. Sequence before #099 and #101: it is the enabler and the standing guard. No providers, no DB. | `docs/audit/latency-audit-2026-07-28.md` measurement plan; `src/lib/server-timing.ts`; `src/lib/answer-stream-contract.ts:18-21` | 2026-07-29 | | #099 | P2 | task | Remove the remaining fixed per-request round trips | **Outcome:** the answer path stops paying avoidable per-request Supabase round trips. **Done 2026-07-29:** shared-cache-hit promotion deferred off the response path with its mid-request staleness guard intact and documented (`rag.ts:3234`, `rag-cache.ts`); scope resolution overlapped with the rate-limit RPC, signal threaded so a client disconnect finally cancels its paginated queries (`answer/route.ts`). **REFUTED on PR #1377 review — do not retry:** the same pass also overlapped scope with the rate-limit RPC and aborted it on deny, claiming the limiter could "deny for free". It cannot. With caller-supplied `filters` or explicit ids, scope passes its zero-query early returns (`search-scope.ts:242,253`) into the paginated `documents` loop at `:269`, and an `AbortSignal` cancels the client request without un-executing a statement Postgres already began — so throttled traffic kept burning database capacity while collecting 429s, against `capacity-review.md:106-113`'s first-soft-failure warning. Scope is behind admission again, pinned by `tests/answer-route-preamble.test.ts`. Re-attempting the overlap requires a non-database admission gate ahead of the durable limiter first. **Remaining:** (a) the 8 `setCachedSearch` awaits — deferring changes `throwIfAborted` semantics and widens a real mutation window because the clone happens after an `await`, so each branch needs discharging individually; (b) batch the anonymous subject+global rate-limit pair, which needs a NEW atomic RPC modelled on `consume_summary_rate_limits_atomic` and cannot be called until the operator applies it — `Promise.all` is the WRONG fix because it consumes the global bucket even when the subject bucket already denied; (c) stop the proxy and route handler resolving identity twice per authenticated request — no in-process memo can do this (different `Request` objects), so the proxy must forward unspoofable verified claims via a header it controls. Cross-references #011: halving auth resolutions eases the ~10-connection Auth cap that `capacity-review.md:106-113` calls the first hard failure. | `docs/audit/latency-audit-2026-07-28.md` L1-1/L1-3/L1-4; `src/lib/api-rate-limit.ts:276-282`; `src/proxy.ts:125` | 2026-07-29 | @@ -152,6 +152,7 @@ removed after current-main verification; it is not missing recommended work. | #112 | P2 | issue | `issues:next-id` has no concurrency protection | **Outcome:** two agents working the same hour cannot allocate the same ledger id. **Detail:** the marker at the top of this file is a plain HTML comment read-modify-written by whoever edits next, with no lock and no post-merge check. On 2026-07-29 it collided **twice in one hour**: PR #1391 claimed `#096`/`#097`, lost them, claimed `#098`/`#099`, lost those too, and its final pair `#108`/`#109` collided a third time with the branch-cleanup work that reached `main` first — resolved during the #1391 merge by renumbering to `#110`/`#111` and bumping the marker to 112. Each collision is silent: `docs/outstanding-issues.md` has NO union merge driver (unlike `docs/branch-review-ledger.md`), so it presents as an ordinary content conflict that a hurried resolution can settle by dropping one side's rows entirely. Nothing checks for duplicate ids afterwards. **Next:** add a duplicate-id and marker-consistency check to the verification gates — the cheapest useful form is a test asserting every `#NNN` id appears exactly once across both tables and that the marker exceeds the max, which turns a silent loss into a red gate. Consider`merge=union`in`.gitattributes` as well, though row-level union does not by itself prevent two rows sharing an id. **Stop:** do not resolve one of these conflicts by taking one side wholesale without diffing the id sets first; that is how rows get dropped. | session 2026-07-29 PR sweep; PR #1391 conflict resolution; `.gitattributes` | 2026-07-29 | | #113 | P2 | issue | `ModeNav` clips its labels at every phone width on `main` | **Outcome:** the mode navigation bar honours its own "labels are never abbreviated" contract, or does not render. **Detail:** measured in Chromium against the running app on 2026-07-29 (PR #1390 review, landed as `8f861bb0`), `span.truncate` `scrollWidth` vs `clientWidth` on `/therapy-compass/search`: 320px viewport clips `Compare` to **15 of 56px**; 360px → 29px; 390px → 39px; 412px → 46px; **430px is the worst case at 3 of 4 labels clipped** (`Compare` 17px, `Recommend` 57 of 77, `Pathways` 57 of 60) because crossing the 26rem band adds a fourth slot to the same space; clean only from ~35rem. Root cause is `grid-auto-columns: 1fr` (`globals.css:2373`) in both lower bands — equal tracks make the WIDEST slot set what every slot needs — combined with `truncate`, so the shortfall is silent. The CSS block's stated budgets are wrong: it claims four labels need 394px, measured intrinsic widths are Search 92.4 / Compare 144.3 / Recommend 125.6 / Pathways 108.5 = **471px**, short by roughly the `0/4` badge plus its gap. **Next:** pick one — (a) content-size every band (adopt the ≥34rem `display: flex` shape throughout) and move thresholds to ~21rem for three slots and ~31rem for four; (b) drop the count badge below the top band, worth ~38px; (c) raise the thresholds so phones keep the collapsed control. (a) is the smallest change and deletes a layout mode. Add the band-boundary browser assertion review asked for, covering the widest label in the set. **Stop:** a hardcoded `rem` threshold cannot guarantee fit for an arbitrary item list, and `ModeNav` is shared — do not treat a Therapy-tuned number as a general solution. | PR #1390 thread `PRRT_kwDOSh5Fis6UyB5X` (open at merge); measured session 2026-07-29 | 2026-07-29 | | #114 | P2 | issue | The live Web-Vitals instrument cannot measure its own noise | **Outcome:** the `#017` baseline rests on evidence whose reproducibility can be checked. **Detail:** `live-web-vitals.yml` (landed `8dbfc5d1`) runs Lighthouse **once** per route/strategy. `#017` asks for reproducible evidence and says to stop when the evidence is too noisy — one sample can neither establish reproducibility nor recognise noise, so the instrument cannot detect the condition its own governing item tells the operator to stop on. The rule is a hard threshold (LCP < 2500ms), so a route near the line resolves to a pass or a breach on run-to-run variance alone, invisibly, and a favourable sample would mark **seven** gated findings WONTFIX. Related and also unclosed: Chrome ships with the `ubuntu-24.04` runner image and is NOT pinned by `LIGHTHOUSE_VERSION`, so a metric shift between a baseline and its follow-up can originate in the browser; the build is now recorded per report in `summary.json` (`chromeVersions`) so a cross-version comparison is visibly invalid, but nothing prevents one. **Next:** N runs per route/strategy with a sample-indexed report name, `expectedRuns`/`incompleteEvidence` reworked to expect N per cell, median as the graded aggregate (the Lighthouse and `lighthouse-ci` convention), and — most important — a spread that straddles a threshold treated as INCOMPLETE EVIDENCE rather than resolved either way. Costs N× dispatch time. Pinning Chrome needs a container or a setup action. **Stop:** do not record an `#017` verdict from a single-sample run, and do not compare baselines whose `chromeVersions` differ. | PR #1385 thread `PRRT_kwDOSh5Fis6U0Zq0` (open at merge); `scripts/summarise-web-vitals.mjs` | 2026-07-29 | +| #115 | P3 | rec | Band adoption gate treats a discovered import as rendered | **Outcome:** the gate fails when a route keeps its results import but stops rendering it. **Detail:** `tests/search-results-band-adoption.test.ts` walks imports and reports adoption if any reachable module contains a band element; it does not track whether the imported binding is used in rendered JSX. Reducing `(search-app)/services/page.tsx` to `
` while retaining its imports keeps the gate green. This is **pre-existing** — the previous two-hop walker had the same flaw — and not a live risk, because `npm run lint` fails the same edit with five `@typescript-eslint/no-unused-vars` warnings under `--max-warnings 0`, so the composite static gate does catch it. The genuinely uncaught shape is an import referenced somewhere non-rendering (a type position, or `void Binding;`), which is a deliberate act rather than a plausible slip. **Next:** if tightened, track which imported bindings appear in JSX element position or as the default export — a re-export like `export default Child` is a real mount with no JSX — and follow `dynamic()` bindings the same way; `@babel/parser` is already used by `tests/route-reachability.test.ts`. **Stop:** do not add a fixture page under `src/app` to prove it — `tests/codebase-index-coverage.test.ts` also walks that tree and Vitest runs files in parallel, so a materialising route can be observed mid-run and left behind on failure. | PR #1394 review; session 2026-07-30 | 2026-07-30 | ## Resolved / archive diff --git a/tests/search-results-band-adoption.test.ts b/tests/search-results-band-adoption.test.ts index 1359e08d0..0edffec31 100644 --- a/tests/search-results-band-adoption.test.ts +++ b/tests/search-results-band-adoption.test.ts @@ -1,9 +1,11 @@ -import { existsSync, readFileSync, readdirSync } from "node:fs"; +import { existsSync, mkdtempSync, readFileSync, readdirSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; import path from "node:path"; import { describe, expect, it } from "vitest"; import { appModeDefinitions } from "@/lib/app-modes"; +import { isAlwaysStandaloneShellPath, isStandaloneModeHomePath } from "@/lib/search-route-ownership"; /** * The shared results band is what stops a search surface asserting "0 matches" @@ -95,34 +97,117 @@ const BAND_ROUTE_ALLOWLIST = new Map([ ], ]); -/** Resolve a mode href like `/services` to its App Router page file when present. */ -function modeHrefToPagePath(href: string): string | null { - const pathOnly = href.split("?")[0]?.trim() ?? ""; - if (!pathOnly.startsWith("/") || pathOnly === "/") return null; - const candidate = path.join(APP_DIR, "(search-app)", pathOnly.slice(1), "page.tsx"); +const ROOT_DASHBOARD_ROUTE = path.join(APP_DIR, "(search-app)", "page.tsx"); + +/** + * Resolve a mode href to its App Router page file. + * + * Query-backed modes are the reason this is not a plain path join. `prescribing` + * declares `href: "/?mode=prescribing"`, whose pathname is `/`, and the previous + * implementation returned null for that — so it, and every href-less mode such + * as Documents, stayed outside the inventory and the root dashboard page was + * never checked at all. + */ +function modeHrefToPagePath(href: string | null): string | null { + const pathOnly = (href ?? "/").split("?")[0]?.trim() || "/"; + if (!pathOnly.startsWith("/")) return null; + const candidate = + pathOnly === "/" ? ROOT_DASHBOARD_ROUTE : path.join(APP_DIR, "(search-app)", pathOnly.slice(1), "page.tsx"); if (!existsSync(candidate)) return null; return path.relative(REPO_ROOT, candidate).replaceAll(path.sep, "/"); } -/** One or two import hops from a route into `@/components/**` that mounts the band. */ -function routeReachesBand(routeSource: string, componentSources: Map) { - if (rendersBand(routeSource)) return true; - const imported = [...routeSource.matchAll(/from "@\/(components\/[^"]+)"/g)].map((match) => match[1]); - for (const specifier of imported) { - const componentPath = `src/${specifier}.tsx`; - const source = componentSources.get(componentPath); - if (!source) continue; - if (rendersBand(source)) return true; - // Page wrappers often re-export a child that mounts the band (e.g. differentials). - const nested = [...source.matchAll(/from "@\/(components\/[^"]+)"/g)].map((match) => match[1]); - if ( - nested.some((nestedSpec) => { - const nestedSource = componentSources.get(`src/${nestedSpec}.tsx`); - return nestedSource ? rendersBand(nestedSource) : false; - }) - ) { - return true; +/** `src/app/(search-app)/services/page.tsx` -> `/services`; the group page -> `/`. */ +function routePathname(routeAbs: string): string { + const rel = path.relative(APP_DIR, routeAbs).replaceAll(path.sep, "/"); + const segments = rel + .replace(/\/page\.tsx$/, "") + .split("/") + .filter((segment) => segment && !segment.startsWith("(")); + return `/${segments.join("/")}`; +} + +/** + * Layouts count toward reachability only for dashboard-owned routes. + * + * In App Router a page's output includes its layouts, and the root dashboard + * route depends on that: `(search-app)/page.tsx` renders only a pass-through and + * its band arrives through the group layout's shared shell. + * + * But that layout transitively imports `ClinicalDashboard`, so following it for + * *every* route made the gate worthless — a namespaced page could be reduced to + * `
` and still "reach" the band. Page-owned result surfaces must reach + * the band through their own page: + * - `isAlwaysStandaloneShellPath` — never mounts the dashboard (services, forms, …) + * - `isStandaloneModeHomePath` — mode homes with their own results page, including + * `/tools`, which is intentionally outside the always-standalone Suspense list + * but still mounts `ApplicationsLauncherPage` rather than the dashboard body + */ +function reachabilityRoots(routeAbs: string): string[] { + const pathname = routePathname(routeAbs); + if (isAlwaysStandaloneShellPath(pathname) || isStandaloneModeHomePath(pathname)) return [routeAbs]; + const layouts: string[] = []; + let dir = path.dirname(routeAbs); + while (dir.startsWith(APP_DIR)) { + const candidate = path.join(dir, "layout.tsx"); + if (existsSync(candidate)) layouts.push(candidate); + if (dir === APP_DIR) break; + dir = path.dirname(dir); + } + return [routeAbs, ...layouts]; +} + +/** `from "x"` and `import("x")`. The lazy form is load-bearing: the dashboard + code-splits its mode workspaces through `dynamic(() => import(...))` in + `clinical-dashboard-lazy.tsx`, so a static-only walk cannot see the band + behind Differentials, Favourites or the prescribing workspace. */ +const IMPORT_SPECIFIER = /(?:from\s*"([^"]+)")|(?:import\(\s*"([^"]+)"\s*\))/g; + +function resolveSpecifier(specifier: string, fromFile: string): string | null { + let base: string; + if (specifier.startsWith("@/")) base = path.join(REPO_ROOT, "src", specifier.slice(2)); + else if (specifier.startsWith(".")) base = path.resolve(path.dirname(fromFile), specifier); + else return null; + const candidates = [`${base}.tsx`, `${base}.ts`, path.join(base, "index.tsx"), path.join(base, "index.ts")]; + return candidates.find((candidate) => existsSync(candidate)) ?? null; +} + +/** + * Breadth-first search from a route (and its layouts) to a rendered band. + * + * The previous implementation hard-coded two hops into `@/components/**`. The + * real chain on the root dashboard route is four — + * `layout → shared-search-app-shell → global-search-shell → ClinicalDashboard → + * document-search-results` — so a fixed hop count silently under-reported + * reachability rather than failing loudly. + */ +const MAX_IMPORT_DEPTH = 8; + +function routeReachesBand(routeAbs: string): boolean { + const roots = reachabilityRoots(routeAbs); + const seen = new Set(roots); + let frontier = roots.map((file) => ({ file, depth: 0 })); + while (frontier.length > 0) { + const next: Array<{ file: string; depth: number }> = []; + for (const { file, depth } of frontier) { + let source: string; + try { + source = readFileSync(file, "utf8"); + } catch { + continue; + } + if (rendersBand(source)) return true; + if (depth >= MAX_IMPORT_DEPTH) continue; + for (const match of source.matchAll(IMPORT_SPECIFIER)) { + const specifier = match[1] ?? match[2]; + if (!specifier) continue; + const target = resolveSpecifier(specifier, file); + if (!target || seen.has(target) || isMockupPath(target)) continue; + seen.add(target); + next.push({ file: target, depth: depth + 1 }); + } } + frontier = next; } return false; } @@ -161,9 +246,11 @@ describe("search results band adoption", () => { // Top-level mode homes such as /services and /favourites also present result // lists. Restricting the inventory to `/search/` left those pages unchecked. + // An href-less mode (Documents) is served by the root dashboard route, so it + // resolves to the same page as the query-backed hrefs rather than dropping out. const modeHomeRoutes = appModeDefinitions .filter((mode) => mode.search.resultsSurface === "results-band") - .map((mode) => ("href" in mode && typeof mode.href === "string" ? modeHrefToPagePath(mode.href) : null)) + .map((mode) => modeHrefToPagePath("href" in mode && typeof mode.href === "string" ? mode.href : null)) .filter((rel): rel is string => Boolean(rel)) .map((rel) => ({ abs: path.join(REPO_ROOT, rel), rel })); @@ -180,16 +267,18 @@ describe("search results band adoption", () => { searchRoutes.some((route) => route.rel === "src/app/(search-app)/services/page.tsx"), "Mode-href discovery must include top-level results pages such as /services.", ).toBe(true); - - const componentSources = new Map( - productionComponents.map(({ abs, rel }) => [rel.replaceAll(path.sep, "/"), readFileSync(abs, "utf8")]), - ); + // Query-backed and href-less modes (`/?mode=prescribing`, Documents) are all + // served by the root dashboard route. Leaving it out is how the gate could + // report "every production search route" while never checking that page. + expect( + searchRoutes.some((route) => route.rel === "src/app/(search-app)/page.tsx"), + "Mode-href discovery must include the root dashboard route that serves query-backed modes.", + ).toBe(true); const orphans: string[] = []; for (const { abs, rel } of searchRoutes) { if (BAND_ROUTE_ALLOWLIST.has(rel)) continue; - const routeSource = readFileSync(abs, "utf8"); - if (!routeReachesBand(routeSource, componentSources)) orphans.push(rel); + if (!routeReachesBand(abs)) orphans.push(rel); } expect( @@ -250,15 +339,63 @@ describe("band adoption detection", () => { expect(mounted.includes(" { - const componentSources = new Map([ - [ - "src/components/orphan-results-page.tsx", - 'export function OrphanResultsPage() { return
; }', - ], - ]); - const routeSource = - 'import { OrphanResultsPage } from "@/components/orphan-results-page";\nexport default OrphanResultsPage;'; - expect(routeReachesBand(routeSource, componentSources)).toBe(false); + it("counts layouts only for dashboard-owned routes", () => { + // The group layout transitively imports ClinicalDashboard, so following it + // for every route would let any page under (search-app) pass while rendering + // nothing. Verified by gutting services/page.tsx to `
`: with layouts + // followed unconditionally the gate still passed; scoped to dashboard-owned + // routes it reports that page as an orphan. + const standalone = path.join(APP_DIR, "(search-app)", "services", "page.tsx"); + expect(reachabilityRoots(standalone)).toEqual([standalone]); + + // `/tools` is a standalone mode home but not always-standalone (Suspense + // boundary differs). It must still be page-only — otherwise gutting + // tools/page.tsx stays green via the layout → dashboard import chain. + const tools = path.join(APP_DIR, "(search-app)", "tools", "page.tsx"); + expect(reachabilityRoots(tools)).toEqual([tools]); + + // The root dashboard route is the case that genuinely needs its layout: the + // page renders only a pass-through and the band arrives via the shared shell. + const rootRoute = path.join(APP_DIR, "(search-app)", "page.tsx"); + const rootRoots = reachabilityRoots(rootRoute); + expect(rootRoots[0]).toBe(rootRoute); + expect(rootRoots.length).toBeGreaterThan(1); + expect(rootRoots.some((file) => file.endsWith(`(search-app)${path.sep}layout.tsx`))).toBe(true); + }); + + it("resolves a route to the band through imports, and reports one that never gets there", () => { + // Real files on disk, because the walker resolves specifiers rather than + // consulting a map. Both directions are asserted: a fixture that only ever + // returns false would pass against a walker that is broken outright. + const dir = mkdtempSync(path.join(tmpdir(), "band-adoption-")); + try { + writeFileSync( + path.join(dir, "child.tsx"), + "export function Child() { return
; }\n", + "utf8", + ); + writeFileSync( + path.join(dir, "orphan-route.tsx"), + 'import { Child } from "./child";\nexport default Child;\n', + "utf8", + ); + expect(routeReachesBand(path.join(dir, "orphan-route.tsx"))).toBe(false); + + // Same shape, but the child mounts the band — and via a lazy import, which + // is how the dashboard code-splits its mode workspaces. + writeFileSync( + path.join(dir, "banded-child.tsx"), + `export function Banded() { return <${BAND_IDENTIFIER} modeId="documents" />; }\n`, + "utf8", + ); + writeFileSync( + path.join(dir, "wired-route.tsx"), + 'const Lazy = () => import("./banded-child");\nexport default Lazy;\n', + "utf8", + ); + expect(routeReachesBand(path.join(dir, "wired-route.tsx"))).toBe(true); + } finally { + rmSync(dir, { recursive: true, force: true }); + } }); });