fix(web-vitals): sample each route, grade the median, refuse a noisy cell - #1407
Merged
Conversation
…cell Ledger #114. The workflow ran Lighthouse ONCE per route/strategy, so it could not measure dispersion — and therefore could not detect the "evidence is too noisy" condition #17 itself tells the operator to stop on. Against a hard 2500ms threshold, a route near the line resolved to a pass or a breach on run-to-run variance alone, invisibly, and a favourable sample would have marked seven gated findings WONTFIX. Each cell is now sampled `SAMPLES` times (default 3, the lighthouse-ci default and the smallest count that yields a median rather than a mean of two), written as `<strategy>-<slug>-<n>.json`. The summariser groups reports into cells and grades the MEDIAN, so one cold-cache outlier cannot drag a cell across the line. The part that actually closes #114 is `straddlesThreshold`: when a cell's samples land on both sides of a threshold, the median is an artefact of which samples fell where and a rerun would grade it differently. That is measured noise, not a verdict, so it is reported as INCOMPLETE EVIDENCE and fails the step — neither a pass nor a breach. A spread entirely on one side is still a real verdict either way; consistently slow is a finding, not noise. Chrome drift, the other half of the row: a mid-dispatch runner-image change means half the matrix was measured by a different browser. That already failed the job, but the step summary still carried the threshold sentence — and the summary is what outlives the run. It is now a disqualifier alongside incomplete evidence and a non-production origin, so no verdict-shaped claim survives it. Found by running main() against fixtures rather than by reading the code. Table is now one row per cell — median plus the spread it came from — so a reader can see whether a number is stable. The samples are all in the artifact. Also separated two failures that had become entangled: a report that measured the WRONG page has real numbers, so it must not count as "no usable metric". Medians are computed from metric validity; `incompleteEvidence` keeps the did-it-measure-the-right-page check. Verified end to end against four fixture directories, not just unit tests: clean -> exit 0, non-closure pass statement 2400/2600/2450 -> exit 1, "too noisy to grade", no verdict prose 3001/3002/3003 -> exit 0, real breach, actionable two Chrome builds -> exit 1, disqualified, no threshold sentence 40 unit tests pass (10 new). verify:cheap exit 0 — Test Files 432 passed (432), Tests 4460 passed | 4 skipped (4464). check:github-actions passed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FvU8z73P6TXUXoYBqN5K1P
Contributor
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 48 minutes 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 (4)
Comment |
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
11 tasks
…-sampling # Conflicts: # docs/outstanding-issues.md
Prettier column widths only; the 115-row cell set is byte-identical. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FvU8z73P6TXUXoYBqN5K1P
BigSimmo
marked this pull request as ready for review
July 30, 2026 05:39
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ce240a7970
ℹ️ 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".
Owner
Author
|
@codex resolve actionable Codex review findings for this pull request and current head using the repository instructions. This is the pull request's single automatic repair pass: do not perform a fresh review, create new standalone findings, or request another review. Work only the existing unresolved Codex threads on the current head. Always fix P0 and P1 findings. For P2 and lower findings, fix only clear, scoped, low-risk issues; otherwise disposition them with a concise reason. After fixing or dispositioning a thread, reply in that thread with as the first line, followed by a concise summary; that marker authorizes the workflow to close that exact thread. If human input or new authorization is required, do not use the marker and leave the thread open with the blocker. Finish only after every actionable thread is fixed or dispositioned and closed, or explicitly left open for a human decision. Do not update the branch from main, address unrelated reviews, broaden scope, or create more than one scoped fix commit. Do not use external APIs, paid services, credentials, dependency changes, or broad refactors unless explicitly authorized. Add targeted tests where behavior changes and run the narrowest relevant validation. |
BigSimmo
enabled auto-merge
July 30, 2026 05:44
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
…noise `samples` is a free-text `workflow_dispatch` input, so the default of 3 was only a default. An operator dispatching `samples=1` got back exactly the single-run matrix this change exists to abolish — one report per cell, a "median" of one, a zero-width range, and therefore a straddle check that could never fire — and the output was indistinguishable from a sound run. Verified against the pre-fix logic: with one sample per cell and every threshold passing, `mobileBreaches` returned 0, `incompleteEvidence` returned 0, and the table printed "Every mobile route is within LCP < 2500ms and CLS < 0.1" in bold. Nothing else in the pipeline objected, because nothing else was looking at the sample count. That matrix could have been recorded against #17 as a verdict. Two samples are refused for the same reason: the median is their mean, and two points cannot distinguish a stable cell from a noisy one whose runs happened to land close together. Three is the `lighthouse-ci` default and the smallest count that yields a real median. Refused in three places, because each has a different failure window: - `main()` exits before reading a single report, so no summary.json is written that a reader could mistake for evidence - `renderTable` adds it to the existing disqualifier list, so the step summary — which outlives the run — cannot carry verdict prose either - the workflow rejects it in seconds, before a full measurement pass against the live domain is spent finding out Six tests, including the decisive one: a one-sample matrix where every threshold passes, the evidence is "complete" for the count requested, and the spread is zero, so only this gate stands between it and a verdict. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FvU8z73P6TXUXoYBqN5K1P
…into claude/fix-web-vitals-sampling
straddlesThreshold takes a cell summary, not (values, threshold). The two-argument call read undefined fields and returned false, so the assertion passed for the wrong reason — typecheck caught what vitest could not. It now asserts the actual point: with one sample the cell's range has zero width, so the straddle check can never fire. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FvU8z73P6TXUXoYBqN5K1P
10 tasks
BigSimmo
pushed a commit
that referenced
this pull request
Jul 30, 2026
The #113 archive row cited `c2edda18` as PR #1405's merge commit. That was the branch tip, which the squash merge discarded, so the claim was already unverifiable when it was written: git merge-base --is-ancestor c2edda1 origin/main -> fails git merge-base --is-ancestor 020c126 origin/main -> ok 020c126 fix(mode-nav): size slots to their content ... (#1405) In a ledger whose whole purpose is durable, auditable resolution history, a resolution nobody can check is worse than no resolution note. The trap is that this repo mixes merge strategies: #1407 and #1410 landed as merge commits, so their branch SHAs stay reachable; #1405 was squashed and its did not. Copying the PR head works three times in four, which is exactly the kind of rule that survives review until it doesn't. Verified after the edit: `c2edda18` no longer appears anywhere in the file, the replacement is an ancestor of main, and `check:outstanding-issues` still passes with `117 rows (55 open, 62 archived), unique ids, next-id=118 above the highest`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FvU8z73P6TXUXoYBqN5K1P
BigSimmo
pushed a commit
that referenced
this pull request
Jul 30, 2026
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
BigSimmo
added a commit
that referenced
this pull request
Jul 30, 2026
…ation, restore #105 after an upstream deletion (#1459) * docs(issues): record #105's preconnect verification in a browser 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. crossOrigin="anonymous" 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 * docs(issues): record #17's live Web-Vitals verdict — mobile breaches 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 * docs(issues): withdraw the #105 LoadingPanel verification — it matched 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 * docs(issues): rank #17's mobile findings — CLS reproduces exactly offline #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 * docs(review): record PR 1459 final reconciliation --------- Co-authored-by: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes ledger
#114. The workflow ran Lighthouse once per route/strategy, so it could not measure dispersion — and therefore could not detect the "evidence is too noisy" condition#017itself tells the operator to stop on. Against a hard 2500 ms threshold, a route near the line resolved to a pass or a breach on run-to-run variance alone, invisibly. A favourable sample would have marked seven gated findings WONTFIX.Sampling. Each cell is measured
SAMPLEStimes (default 3 — thelighthouse-cidefault, and the smallest count that yields a median rather than a mean of two), written as<strategy>-<slug>-<n>.json. The summariser groups reports into cells and grades the median, so one cold-cache outlier cannot drag a cell across the line.The part that actually closes the row is
straddlesThreshold. When a cell's samples land on both sides of a threshold, the median is an artefact of which samples fell where, and a rerun would grade it differently. That is measured noise, not a verdict — so it is reported as incomplete evidence and fails the step, neither a pass nor a breach. A spread entirely on one side is still a real verdict either way: consistently slow is a finding, not noise.Chrome drift, the other half of the row. A mid-dispatch runner-image change means half the matrix was measured by a different browser. That already failed the job, but the step summary still carried the threshold sentence — and the summary is what outlives the run. It is now a disqualifier alongside incomplete evidence and a non-production origin, so no verdict-shaped claim survives it. I found this by running
main()against fixtures, not by reading the code.Table is now one row per cell — median plus the spread it came from — so a reader can see whether a number is stable at a glance. Every sample is still in the uploaded artifact.
Also separated two failures that had become entangled: a report that measured the wrong page has real numbers, so it must not count as "no usable metric". Medians are computed from metric validity;
incompleteEvidencekeeps the did-it-measure-the-right-page check.RAG impact: no retrieval behaviour change — a dispatch-only CI workflow, its standalone summariser, that script's unit tests, and one ledger row; no retrieval, ranking, selection, or answer-generation code is touched.
Verification
Verified end to end against four fixture directories, because the unit tests cannot cover the filename→cell mapping,
summary.json, or the exit codes:01mobile-dsm (samples straddle a threshold: LCP 2400–2600ms … too noisy to grade), no verdict prose0median outside the threshold (LCP 3001–3003ms)1tests/summarise-web-vitals.test.ts— 40 pass (10 new), covering median-vs-outlier, an LCP straddle, a CLS straddle, a stable spread that must not be called noisy, a wholly-over spread that must still breach, a single missing sample, the rendered median+range, and both Chrome-drift directionsexpectedRunsexactly (mobile-root-1.json… 18 files for 3 routes × 2 strategies × 3 samples)npm run verify:cheap— exit 0,Test Files 432 passed (432),Tests 4460 passed | 4 skipped (4464)npm run check:github-actions—GitHub Actions pin check passed.Risk and rollout
Clinical Governance Preflight
No ingestion, answer generation, search/ranking, document access, privacy or clinical output path is touched.
Clinical KB Database(sjrfecxgysukkwxsowpy)Notes
SAMPLESis a dispatch input, so an operator who wants a tighter baseline can raise it without a code change; the summariser grades against what was actually requested rather than the default.Pinning Chrome outright is still not done and I am not claiming it is — on a GitHub-hosted runner that needs a container or a setup action, which is a change to how the job executes rather than a line edit. What this adds is detection: a split build now disqualifies the run, and the recorded
chromeVersionslet a cross-run comparison be rejected at read time.🤖 Generated with Claude Code
https://claude.ai/code/session_01FvU8z73P6TXUXoYBqN5K1P
Generated by Claude Code