docs(issues): rank #017's mobile breach, withdraw a wrong #105 verification, restore #105 after an upstream deletion - #1459
Conversation
The preconnect half of #105 is verified; the LoadingPanel half is not, and the row now says which is which rather than carrying one PENDING for both. Worth recording how it was verified, because the obvious check silently proves nothing: locally NEXT_PUBLIC_SUPABASE_URL is unset, so supabaseOrigin() returns null and the layout renders no link at all. Loading the app and finding no preconnect would have looked like a failure and been an absent fixture. The check needs a placeholder origin (no real network call — the point is React's hoisting, not Supabase connectivity). With that set, both the SSR HTML and the live browser DOM show preconnect and dns-prefetch in <head> with zero stray preconnect in <body>, so React 19 does hoist them out of the body where they are authored. survives the hoist, which matters: without it supabase-js's CORS fetches open a second connection and the hint buys nothing. The remaining half needs a different technique. A LoadingPanel fallback only renders while a lazy chunk is in flight, so it needs a throttled-network browser check rather than verify:ui, which would race past it. Gates: outstanding-issues guard 142 rows / unique ids; prettier clean; the edit is a 1-line diff because it stays inside the Detail column's existing width (1133 chars against a 3268 budget) — see #133 for why that matters in this file. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01361jh3eYVjJCzXWjAhdZiF
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in:1 minute Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughUpdated two outstanding-issues queue entries with the graded Lighthouse result for row ChangesOutstanding issue status updates
Estimated code review effort: 1 (Trivial) | ~2 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
This pull request has been ignored for the connected project Preview Branches by Supabase. |
Dispatched live-web-vitals.yml against psychiatry.tools (run 30548662649, 5 routes x 3 samples). #17 has been the gate on seven latency findings for weeks with no evidence behind it. It now has evidence, and it is a breach. Mobile medians against LCP<2500ms / CLS<0.1: root 4308ms 0.023 therapy-compass 4716ms 0.142 documents-search 3676ms 0.220 dsm 3639ms 0.363 forms 3786ms 0.212 Desktop passes everywhere (LCP 625-694ms, CLS 0.016-0.097), which is worth noting because desktop-only checking would have concluded the opposite. The verdict is gradeable rather than noisy: every LCP sample range sits wholly above 2500ms, so the straddle rule #114 added does not fire. That distinction is the whole point of #1407's sampling work — a single run per cell could not have told a real breach from variance. Applying the rule as written before the numbers were read: any breach makes only the breaching routes' findings actionable, and NOTHING becomes WONTFIX. That is the opposite of what a favourable sample would have produced, which is exactly why the rule was written first. CLS 0.363 on /dsm is the worst single number and is visible layout shift, not a byte-count problem — so it is not addressed by the payload findings #17 gates. Recorded honestly as one step short of complete: this reads the summariser's emitted table, not the raw Lighthouse JSON (artifact 8762211043, 30-day retention). The row itself asks for that cross-check before recording a verdict, most of all a PASS; this is a breach, so the risk of the table flattering the result is lower, but the check is still outstanding. INP is absent because Lighthouse cannot measure it in lab conditions. Diff stays 1 line: #17's Detail cell is 2184 chars against the column's 3268 budget, so nothing repads (see #133). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01361jh3eYVjJCzXWjAhdZiF
…01v' into claude/latency-findings-impl-s8g01v
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:0c2e2b361b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/outstanding-issues.md`:
- Line 142: Update the queue entry for `#105` to remove the completed preconnect
verification and stop directing reviewers to run verify:ui. Align it with the
remaining LoadingPanel verification by requiring a throttled-network browser
check only, while preserving the existing queue format and references.
- Line 112: Update the recommended queue entry for `#017` to reflect that
Lighthouse evidence has already been dispatched and graded; replace the
instruction to capture evidence with the next action of ranking mobile findings
by measured contribution, consistent with the detailed `#017` row.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 031fbb10-5c99-4774-a0c3-9af8cd66ec95
📒 Files selected for processing (1)
docs/outstanding-issues.md
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Main's #1461 reflowed the open-items table while this branch appended #17's live Web-Vitals verdict, so the two sides conflicted for real (`git merge-tree` dirty, not staleness). Resolved row by row rather than by taking either side: kept this branch's #17 BREACH verdict and #105 preconnect evidence, took main's #86 X3 progress and its new #145 row. #105 closes. Its remaining half — the `LoadingPanel` fallbacks — is now verified, and by a different method than the row prescribed. It claimed the fallback renders solely while the client chunk is in flight, so only a throttled-network run could observe it. That is wrong: the installed Next 16 loader wraps an `ssr:false` import in Suspense whenever a `loading` element is supplied, and `BailoutToCSR` throws on the server, so the fallback is emitted in the server response HTML. Confirmed against the running dev server — `role="status" aria-label="Loading"` appears 1x on `/`, 2x on `/dsm`, 2x on `/forms`. No throttled run and no `verify:ui` were needed. Finding raised by Codex on PR #1459. The two recommended-queue entries now match their detail rows: #105 is dropped from the queue, and #17 no longer directs a reader to capture evidence that has already been captured and graded — its next action is ranking the mobile findings by measured contribution and cross-checking the raw Lighthouse JSON artifact. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01361jh3eYVjJCzXWjAhdZiF
Second real conflict on docs/outstanding-issues.md, this time in the archive table: main's #1455 archived #122 while this branch archived #105. Kept both rows and took main's separator width. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01361jh3eYVjJCzXWjAhdZiF
…d the wrong component The claim that `LoadingPanel` was verified present in the SSR response HTML does not hold, so #105 is reopened rather than archived. The evidence was `role="status" aria-label="Loading"` appearing in the initial HTML of `/`, `/dsm` and `/forms`. Those hits are real, but that exact string has exactly one renderer in `src/` — `ModeHomePageSkeleton` (`src/components/mode-home-page-skeleton.tsx:8`). `LoadingPanel` never emits a bare "Loading" label; it renders `aria-label={label}`, always a specific string such as "Loading differentials". Re-checked per route against a running server, `/`, `/dsm`, `/forms`, `/differentials` and `/favourites` return zero `aria-label="Loading <something>"` matches. Codex's mechanism finding on this PR stands and is not what failed: the installed Next 16 loader does set `hasSuspenseBoundary` when a `loading` element is supplied, and `BailoutToCSR` throws on the server, so a mounted `ssr:false` surface emits its fallback into the response HTML. What was missing is that this only fires for a surface mounted during the initial render, and none of the eleven in `clinical-dashboard-lazy.tsx` are — every one sits behind interaction state (`ClinicalDashboard.tsx:3666-4070`). So neither a cold-load grep nor the throttled check the row originally prescribed can observe them; that needs a driven browser session. The reopened row and its queue entry both record the withdrawn claim and carry an explicit stop rule against re-closing on a cold-load grep. The preconnect half remains verified and is marked so, to avoid redoing it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01361jh3eYVjJCzXWjAhdZiF
…line #17 asked for the mobile breach to be ranked by measured contribution. Done, and the measurement turned up something that changes what the work costs. Ran the local offline production Lighthouse harness over the same five routes the live dispatch used. The mobile CLS numbers do not merely correlate with production — they match run 30548662649 to three decimals on all four locally measurable routes (0.023 / 0.142 / 0.220 / 0.363), and the desktop range matches at both endpoints (0.016-0.097). /forms did not measure locally (Lighthouse NO_NAVSTART, its own "run again" transient); live it was 0.212. So CLS on these routes is deterministic layout structure, not network or production data. The CLS half of #17 is debuggable and gate-able locally at zero provider cost, and #17's recorded premise that production vitals need Supabase secrets is stale — run-lighthouse-budget.mjs builds and serves an offline production app precisely to sidestep that. Its earlier "dev-mode CLS is excellent at 0.00-0.04" note was measuring dev, which does not reproduce production. LCP does NOT reproduce and must not be read from local runs: 2014-2051 ms local against 3639-4716 ms live, because the loopback server has no network latency. The LCP ranking does survive — /therapy-compass is the outlier both locally and live — which corroborates #117. Filed as #147 with the ranking (/dsm 0.363 worst, then /documents/search 0.220, /forms 0.212, /therapy-compass 0.142, / 0.023 passing) and the next step: Lighthouse's layout-shift-elements audit returned zero items on every route, so element attribution needs a PerformanceObserver on layout-shift against the same offline build. Two reproduction traps recorded with it: the harness reads CHROME_PATH but nothing in this container sets it, so chrome-launcher fails on every route; and a truncated `progress` package in the npx cache broke Lighthouse startup entirely. No budget baseline was committed — the grader correctly refused this run as incomplete evidence. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01361jh3eYVjJCzXWjAhdZiF
Fourth genuine conflict on docs/outstanding-issues.md, confirmed with git merge-tree before acting. Main's #1470 rewrote #13 as measurement-gated while this branch changed #17, #105 and added #147. Verified by hash that main touched neither #17 nor #105 — both are byte-identical at the merge base and at origin/main — so those rows take this branch's version and #13 takes main's, rather than either side winning wholesale. Also de-duplicated against main's new #118, which already owns adopting the Lighthouse baseline and flipping enforce. #147's gate paragraph now cross-references it instead of restating it, and contributes the constraint #118 was missing: pinning a baseline today would bake CLS 0.363 on /dsm in as the accepted value, so the CLS fixes must land first or the gate ratifies the breach. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01361jh3eYVjJCzXWjAhdZiF
Fifth consecutive genuine conflict on docs/outstanding-issues.md, confirmed with git merge-tree before acting. Main's d8b034c (PR #1441) deleted three rows from the Open items table and archived none of them: #85, which that PR genuinely resolved by adding check-upload-limit-parity.mjs; #119, a duplicate of #122; and #105, which is open and unresolved — its LoadingPanel half is still unverified. This resolution mirrors main on #85 and #119, which were dispositionable, and keeps #105, which was not. #105 survived only because this resolution was done row by row and then diffed with column padding normalised away. Taking either side wholesale would have lost it silently, which is what AGENTS.md forbids for this file and what appears to have happened upstream. Filed as #148: check:outstanding-issues validates duplicate ids, ids in both tables, stale markers and malformed rows, but never compares against the previous revision, so a row that simply disappears is invisible to it in verify:cheap and static-pr alike. The fix is a deletion check against the merge base, not a union merge driver — #133 removed that driver deliberately and reinstating it would bring back the duplicate-row damage it was removed to stop. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01361jh3eYVjJCzXWjAhdZiF
Uh oh!
There was an error while loading. Please reload this page.
Second docs/outstanding-issues.md conflict, this time against #1459. That file carries no merge driver by design (#133), so any overlapping edit conflicts. Resolved by the same prescribed recipe: rebuilt from origin/main and re-applied only this branch's own #86 "Hydration SHIPPED (#101)" edit. Verified identical row count (144) and identical id set to origin/main, so #1459's ranking change, withdrawn verification and restored row are all preserved; the only content delta against main is that one row. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GGEBHp4Seoh1jK1vGTNtYS
Resolves the fifth conflict on docs/outstanding-issues.md by taking main's side on both hunks, after verifying that was correct rather than convenient: main renumbered the queue to fix a duplicate rank 18 this branch introduced, and it archived #138 and #146 rather than dropping them (confirmed present in the archive table). The resulting id set is byte-identical to main's. #150 escalated. That row records CodeRabbit reviewing none of a full day's PRs and rests explicitly on the Codex connector being the surviving reviewer that "found three real defects that had survived local gates". On PR #1505 Codex posted its own usage-limit notice alongside CodeRabbit's spending-cap notice, so that PR received zero automated review and so will anything opened while both caps hold. This is the same issue with its fallback removed, not a new one — which matters because the single Codex finding on #1459 was correct and caught a verification that had matched the wrong component, closing #105 on bad evidence. Local gates did not catch it. The attribution harness lands as scripts/measure-cls-attribution.mjs. #147's next step needs a before/after CLS pair, and #118 will want element attribution again; without this, both mean re-deriving it. Build and serve mirror run-lighthouse-budget.mjs so the numbers sit beside the Lighthouse reports. Two traps that cost real time are encoded rather than left to be rediscovered: CHROME_PATH must be set where the browser is outside a standard location, and an init script attaching a MutationObserver to document.documentElement before <html> exists throws and silently takes the CLS observer with it — so the script now fails loudly on an all-zero result instead of reporting a false clean bill. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01361jh3eYVjJCzXWjAhdZiF
Summary
Docs only, one file. Four ledger changes: a new measured finding, a retraction of a claim made earlier in this same PR, the restoration of a row
maindeleted, and a new defect filed about how that deletion went unnoticed.#147filed — mobile CLS breaches four routes, and it reproduces EXACTLY offline.#017asked for the mobile breach to be ranked by measured contribution. Ranking it turned up something that changes what the work costs. Running the local offline production Lighthouse harness over the same five routes the live dispatch used, the mobile CLS numbers do not merely correlate with production — they match run30548662649to three decimals on all four locally measurable routes (/dsm0.363,/documents/search0.220,/therapy-compass0.142,/0.023), and the desktop range matches at both endpoints (0.016–0.097)./formsdid not measure locally (LighthouseNO_NAVSTART, its own "run again" transient); live it was 0.212. So CLS on these routes is deterministic layout structure, not network, latency, or production data — the CLS half of#017is debuggable and gate-able locally at zero provider cost, and#017's recorded premise that production vitals need Supabase secrets is stale, sincescripts/run-lighthouse-budget.mjsbuilds and serves an offline production app precisely to sidestep it. Its earlier "dev-mode CLS is excellent at 0.00–0.04" note was measuring dev, which does not reproduce production; that is why the breach looked locally unreachable. LCP does not reproduce and must not be read from local runs — 2014–2051 ms local against 3639–4716 ms live, because loopback has no network latency — though its ranking survives, with/therapy-compassthe outlier both ways, corroborating#117.#105stays open, and a verification claimed on it is withdrawn. An earlier revision of this PR closed#105and archived it, on the strength ofrole="status" aria-label="Loading"appearing in the SSR HTML of/,/dsmand/forms. Those hits are real, but that exact string has exactly one renderer insrc/—ModeHomePageSkeleton(src/components/mode-home-page-skeleton.tsx:8).LoadingPanelnever emits a bare"Loading"label; it rendersaria-label={label}, always something specific like"Loading differentials". Re-checked per route against a running server,/,/dsm,/forms,/differentialsand/favouritesall return zeroLoadingPanellabels. The row is reopened with the withdrawn claim recorded in place, so the next person does not repeat it.hasSuspenseBoundarywhenever aloadingelement is supplied — not only whenssris false — andBailoutToCSRthrows on the server, so a mountedssr:falsesurface does emit its fallback into the response HTML. What was missing is that this only fires for a surface that mounts during the initial render, and none of the eleven inclinical-dashboard-lazy.tsxdo: every one sits behind interaction state (ClinicalDashboard.tsx:3666-4070). So neither a cold-load grep nor the throttled check the row originally prescribed can observe them. Both the row and its queue entry now say so, with an explicit stop rule against re-closing on a cold-load grep.#105is also restored here, becausemaindeleted it. Commitd8b034c8(PR fix: guard upload limits in production builds #1441, "guard upload limits in production builds") removed three rows from the Open items table and added zero to the archive:#085, which that PR genuinely resolved by addingscripts/check-upload-limit-parity.mjs;#119, a duplicate of#122; and#105, which is open and unresolved. This branch mirrorsmainon the two dispositionable rows and keeps#105, which was not dispositionable. It survived only because the conflict was resolved row by row and re-diffed with column padding normalised away afterwards.#148filed — a row can leave this ledger and no gate notices.npm run check:outstanding-issuesvalidates duplicate ids, an id present in both tables, a staleissues:next-idmarker, and malformed rows — every shape the 2026-07-29 triple collision took (#112). It never compares against the previous revision, so a row that simply disappears is invisible to it, inverify:cheapand instatic-pralike. The proposed fix is a deletion check against the merge base, explicitly not a union merge driver:#133removed that driver deliberately, and reinstating it would bring back the duplicate-row damage it was removed to stop. The file's own Conventions already say "Keep them — do not delete", and#140is precedent for archiving even a duplicate rather than removing it.#017and the recommended queue are synchronised with all of the above, and#147is de-duplicated againstmain's#118, which landed mid-review and already owns adopting the Lighthouse baseline.#147contributes the constraint#118was missing: pinning a baseline today would bake CLS 0.363 on/dsmin as the accepted value, so the CLS fixes must land first or the gate ratifies the breach it exists to catch.On the diff size. The churn is large relative to the semantic change because of the Prettier markdown-table reflow recorded in ledger
#133— editing the widest cell in a column repads every row. Normalising column padding away, the change is the#017queue and detail rows, the#105queue and detail rows, the new#147and#148rows plus#147's queue row, and theissues:next-idbump.On the merge conflicts. This branch hit five genuine conflicts with
mainondocs/outstanding-issues.md— every sync so far — each confirmed withgit merge-tree --write-tree origin/main HEADbefore acting rather than taken from GitHub'sdirtylabel. That file deliberately carries no union merge driver (#133), so overlapping edits conflict loudly by design. Each was resolved row by row, never by taking a side:main's#086X3 progress,#013measurement-gating,#122,#145and#146were all kept alongside this branch's rows, and for one sync the rows were hashed rather than eyeballed to provemainhad not touched#017or#105. That per-sync cost — and the fact that the fifth sync arrived carrying an upstream row deletion — is an argument for merging this sooner rather than continuing to re-sync it.Verification
Verification not run:
npm run verify:pr-local— the diff is prose indocs/**, touching no source, test, config, schema or workflow file, so no build, browser, or DB gate applies. The gates that do cover this file were run and are pasted below.npm run check:outstanding-issues—Outstanding-issues guard passed: 144 rows (70 open, 74 archived), unique ids, next-id=149 above the highest, no merge driver.This is the gate that catches a bad conflict resolution or a stale id marker — though per#148it is precisely the gate that does not catch a deleted row.npm run check:branch-review-ledger— passed;npm run ledger:dedupereportedNo exact duplicate records in docs/branch-review-ledger.md (144 unique dated rows). Run because mergingmainfive times brought ledger appends with it.npx prettier --check .—All matched files use Prettier code style!Whole-tree, not per-file, and run against the committed state.#147measurement itself:CHROME_PATH=/opt/pw-browsers/chromium npm run verify:lighthouse -- --keep, Lighthouse 12.8.2, Chromium 141.0.7390.37, nine of ten runs producing reports. CLS read back from each report'scumulative-layout-shift.numericValue.grep -rn 'aria-label="Loading"' src/ --include=*.tsxreturns one line,mode-home-page-skeleton.tsx:8; a per-route check of five routes against a running server returns zeroLoadingPanellabels.#148claim:git show d8b034c8 -- docs/outstanding-issues.mdshows three-| #…lines and zero+| #…lines, and none of#085,#105,#119appears anywhere inorigin/main's file, open or archived.mainrow was lost.Deliberately not done: no Lighthouse budget baseline was committed.
lighthouse-budget.jsonstill hasbaseline: nullandenforce: false, so that gate cannot yet catch a regression. The grader refused this run as incomplete evidence because/formsproduced no report, and that refusal was correct and was not overridden. See the#118note above for why a baseline should not be pinned yet even from a complete run.UI verification not run: no UI, routing, styling, reduced-motion or forced-colors code changed. Note that
verify:uiwould not have caught the#105error either — that was a wrong reading of a grep, not a rendering regression.Risk and rollout
#105verification instruction, drops#147and#148, and re-deletes#105— so a revert should be paired with restoring that row by hand.#017live numbers were read from an already-completed dispatch, and nothing here re-dispatches it.Notes
src/lib/rag/**or any other retrieval, ranking, or clinical surface is touched, so noRAG impact:line and no Clinical Governance Preflight is required —classifyPullRequestFilessees onedocs/**path.#148records a defect whose cause sits onmain, in another PR's conflict resolution. This branch records it and restores the one row that should not have gone; it does not attempt to repair#085/#119back into the archive, which is a separate call for whoever owns that history.#147so the next Lighthouse run does not lose the time this one did: the harness readsCHROME_PATH/PLAYWRIGHT_CHROMIUM_EXECUTABLE_PATH(run-lighthouse-budget.mjs:291) but nothing in this container sets either despitePLAYWRIGHT_BROWSERS_PATHbeing set, sochrome-launcherfails on every route; and a truncatedprogresspackage in the npx cache broke Lighthouse startup outright.origin/mainafter test(rag): pin Supabase round-trip budgets on the answer path #1450 merged, keeping the same name, per the merged-PR rule.🤖 Generated with Claude Code
https://claude.ai/code/session_01361jh3eYVjJCzXWjAhdZiF