ci: add dispatch-only live Web-Vitals workflow to discharge #017 - #1385
Conversation
Ledger #17 gates seven client findings from the 2026-07-28 latency audit plus #12/#13/#16, and nothing in the repo could produce the evidence it asks for: in-sandbox vitals are blocked because the production server hard-requires Supabase secrets, and local Playwright timings rank routes against each other without measuring the real origin. Modelled on live-domain-monitor.yml — no secrets, no third-party actions, no providers beyond what an anonymous visitor triggers, LIVE_DOMAIN_URL repo variable override. Dispatch-only rather than scheduled: it is heavier than that monitor's plain GETs, and the decision it feeds is made a few times a year. The decision rule is committed BEFORE any numbers are read, so the gate cannot be rationalised afterwards: mobile LCP < 2500ms and CLS < 0.1 on every route closes#17 as metrics-acceptable and makes the gated payload findings WONTFIX; any breach makes only that route's findings actionable. The summariser is a real script rather than YAML-embedded node so the escaping is not at the mercy of YAML-inside-shell-inside-node quoting, and so the thresholds are testable. It fails closed: a missing LCP or CLS counts as a breach, because an absent number is not evidence of passing — which is the mistake #17 exists to prevent. Verified against four fixtures (breach, pass, missing-metric, empty-directory). Lab metrics only. Lighthouse cannot measure INP in lab conditions; the rule's INP clause is confirmed from CrUX field data, so the pass verdict is stated as provisional on that. Also records the PR #1376 prlanded verification in the branch review ledger. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a manually triggered Lighthouse workflow for configured live routes, a Node CLI that summarizes reports with fail-closed Web Vitals evidence checks and thresholds, expanded tests for evaluation helpers, and ledger updates. ChangesLive Web Vitals Baseline
Branch Review Ledger
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Workflow
participant Lighthouse
participant Summarizer
participant ArtifactStore
Workflow->>Lighthouse: Measure configured routes with mobile and desktop strategies
Lighthouse-->>Workflow: Write JSON reports
Workflow->>Summarizer: Normalize reports and evaluate `#017` evidence
Workflow->>ArtifactStore: Upload web-vitals artifacts
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Comment |
This pull request has been ignored for the connected project Preview Branches by Supabase. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:eab2183503
ℹ️ 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.
The workflow deliberately downgrades a per-route Lighthouse failure to a warning, but the summariser enumerated only the reports that existed and rejected only an entirely empty directory. A directory holding just desktop-root.json therefore produced zero mobile breaches and rendered "Every mobile route is within..." — recommending #17 be closed and its seven gated findings marked WONTFIX on evidence that was never collected. Track the requested route-by-strategy matrix instead: the workflow passes ROUTES through, and any expected mobile run with no report is a breach. The step also exits non-zero when evidence is incomplete, so a partially failed run cannot be read as a pass. An absent number was already treated as a breach; an absent run now is too. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FvU8z73P6TXUXoYBqN5K1P
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:6972f3679f
ℹ️ 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
🧹 Nitpick comments (2)
tests/summarise-web-vitals.test.ts (1)
47-53: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a
missingReport/missingMetricassertion here.This test confirms
mobile-dsmis flagged as a breach when its LCP isnull, but doesn't assert whatmissingReportis set to. Given the gap flagged inscripts/summarise-web-vitals.mjs(Line 137-141) where onlymissingReportgates the process exit, a test asserting the flag value here would have caught that the fail-closed exit doesn't currently cover this case.const breaches = mobileBreaches(rows, DEFAULT_ROUTES); expect(breaches.map((breach) => breach.run)).toEqual(["mobile-dsm"]); + expect(breaches[0].missingReport).toBe(false);🤖 Prompt for 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. In `@tests/summarise-web-vitals.test.ts` around lines 47 - 53, Extend the test for the null LCP case in mobileBreaches to assert the resulting breach’s missingMetric or missingReport flag, using the expected fail-closed value. Keep the existing run-name assertion and ensure the new assertion verifies the flag needed by the process-exit logic..github/workflows/live-web-vitals.yml (1)
88-94: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winPin Lighthouse to an exact patch version
npx --yes lighthouse@12can drift across v12 patch/minor releases, which makes these decision-gate measurements less reproducible; pin an exact version such as12.8.2instead.🤖 Prompt for 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. In @.github/workflows/live-web-vitals.yml around lines 88 - 94, Update the Lighthouse invocation in the workflow to use an exact v12 patch version, such as lighthouse@12.8.2, instead of the floating lighthouse@12 reference; preserve all existing options and failure handling.
🤖 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 @.github/workflows/live-web-vitals.yml:
- Around line 54-55: Update the checkout step using actions/checkout in the live
web vitals workflow to set persist-credentials to false, while preserving the
existing pinned action reference and read-only permissions.
In `@scripts/summarise-web-vitals.mjs`:
- Around line 66-88: Update mobileBreaches so rows with null lcpMs or cls are
marked as fail-closed missing-metric breaches, distinct from ordinary threshold
failures, and update main()’s exit gate to treat both missingReport and
missing-metric breaches as fatal. Preserve the existing handling for genuinely
over-threshold metrics and completely missing mobile reports.
---
Nitpick comments:
In @.github/workflows/live-web-vitals.yml:
- Around line 88-94: Update the Lighthouse invocation in the workflow to use an
exact v12 patch version, such as lighthouse@12.8.2, instead of the floating
lighthouse@12 reference; preserve all existing options and failure handling.
In `@tests/summarise-web-vitals.test.ts`:
- Around line 47-53: Extend the test for the null LCP case in mobileBreaches to
assert the resulting breach’s missingMetric or missingReport flag, using the
expected fail-closed value. Keep the existing run-name assertion and ensure the
new assertion verifies the flag needed by the process-exit logic.
🪄 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: 01611a64-5733-483b-aa05-ff909d283cac
📒 Files selected for processing (4)
.github/workflows/live-web-vitals.ymldocs/branch-review-ledger.mdscripts/summarise-web-vitals.mjstests/summarise-web-vitals.test.ts
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
…uments The documents segment has no page.tsx — it holds only search/, source/ and [id]/ — and there is no rewrite for the bare path, so the default dispatch input would have profiled the Next.js 404 document. docs/site-map.md lists /documents/search as the canonical documents-mode route. This matters more now that a missing mobile report is a hard breach: a route that cannot produce a usable report would block the #17 verdict rather than quietly skew it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FvU8z73P6TXUXoYBqN5K1P
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:a626ef0801
ℹ️ 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.
BigSimmo
commented
Jul 29, 2026
@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. |
Summary
Testing
|
Two holes in the fail-closed gate, both of which exited 0 and let a verdict be recorded on evidence that was not collected. Desktop was never validated. #17 asks for reproducible mobile AND desktop evidence, but the gate only asked mobileBreaches about missing mobile reports. A complete mobile sweep with zero desktop reports printed the metrics-acceptable verdict and exited 0. missingRuns() now covers the full route-by-strategy matrix. A present report with a null LCP/CLS did not fail the step. Those rows are classified under failed with missingReport false, so the gate skipped them and they surfaced only in the table - contradicting the documented guarantee that a missing metric fails closed. They now carry missingMetric and are fatal. An over-threshold number is deliberately still not 'incomplete': that is a real measurement and a real verdict, and the distinction is pinned by a test. Also sets persist-credentials: false on checkout (zizmor artipacked); the job only reads files. 12 tests pass; both gaps were reproduced against the previous gate first. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FvU8z73P6TXUXoYBqN5K1P
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:d860e6320a
ℹ️ 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.
Uh oh!
There was an error while loading. Please reload this page.
Two more holes, both the same root cause: incompleteEvidence() delegated to mobileBreaches(), which is mobile-only, so anything it did not look at was treated as evidence. A desktop report with a null LCP/CLS passed. missingRuns() only checked that the filename existed, and metric validity was only ever checked on mobile, so a complete sweep with desktop-forms.json carrying null metrics returned no incomplete evidence and exited 0. Colliding route slugs silently dropped a route. /a/b and /a-b both slug to a-b, so the second Lighthouse run overwrites the first, and the surviving file then satisfies the expected-run check for both requested routes. The filename scheme cannot represent both, so the evidence can never be complete; this is now fatal. Rather than patch the third instance, incompleteEvidence() now walks the expected route-by-strategy matrix directly and requires each run to carry both graded numbers. Completeness is a property of the whole matrix, so it is computed over the whole matrix. 15 tests pass; both gaps reproduced against the previous code first, each returning [] and exiting 0. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FvU8z73P6TXUXoYBqN5K1P
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:46c85c42f3
ℹ️ 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.
Uh oh!
There was an error while loading. Please reload this page.
LIVE_DOMAIN_URL can point the workflow at a staging cutover. The summariser still printed 'closes#17 as metrics-acceptable', so a staging dry run read as production evidence for a ledger item that asks for psychiatry.tools specifically. The origin is read from each report's own final URL rather than from LIVE_DOMAIN_URL, so a redirect to another host is caught as well as a deliberate override. A non-canonical origin still gets its table and its threshold summary - it is a useful dry run - but the closure sentence is replaced with an explicit non-verdict note naming the origin measured. 18 tests pass; reproduced against the previous code, which emitted the closure sentence for a staging origin. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FvU8z73P6TXUXoYBqN5K1P
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:07d0fdbbdc
ℹ️ 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.
The #17 rule has three clauses - LCP, CLS and INP < 200ms - and Lighthouse cannot measure INP in lab conditions at all. The summariser nonetheless printed 'closes#17 as metrics-acceptable' in bold on LCP and CLS alone and merely asked the operator to confirm INP afterward, so an unavailable or >=200ms CrUX value would have invalidated a closure already announced. The pass branch now states that LCP and CLS are within threshold and that this is explicitly NOT yet an #17 closure until INP is obtained from CrUX. The script can satisfy at most two of the three clauses and no longer claims otherwise. npx --yes lighthouse@12 resolves the range >=12.0.0 <13.0.0-0, so a patch published between a baseline and its follow-up could change metric collection independently of the application and decalibrate the comparison this workflow exists to make. Pinned to 12.8.2 exactly, and summary.json now records the resolved version so a baseline states its own tooling. 18 tests pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FvU8z73P6TXUXoYBqN5K1P
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:8b3904d500
ℹ️ 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.
The origin check passed any report served from psychiatry.tools, but a route that redirects to /login, to a same-origin error page, or to any other path still produces clean metrics for the WRONG page, and the filename cannot reveal it. Reproduced: with every expected run landing on /login, isProductionVerdict returned true and incompleteEvidence returned []. summariseReport now keeps requestedUrl and runtimeError alongside the final URL, and a run counts as evidence only if the final path matches the requested path (trailing slash normalised) and Lighthouse reported no runtime error. Same input now flags all 10 runs. 21 tests pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FvU8z73P6TXUXoYBqN5K1P
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:44a4221f3b
ℹ️ 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.
The path-only comparison accepted a redirect that dropped or rewrote the query, so a dispatch of /documents/search?q=depression that finalised at /documents/search passed as the requested page - presenting an empty-state render as evidence for a results page. Compares normalised path AND query, with params sorted so a reordering redirect is not a false rejection. 23 tests pass, including a dropped-query rejection and a reordered-params acceptance. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FvU8z73P6TXUXoYBqN5K1P
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:93a6955628
ℹ️ 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.
Dropped two union-merge duplicate ledger rows (#88).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:75a2ed8355
ℹ️ 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.
Uh oh!
There was an error while loading. Please reload this page.
BigSimmo
commented
Jul 29, 2026
Resolving the position I left this PR in. I said earlier that after eight rounds of findings I would not self-certify this one, and I meant it — each fix was locally correct and globally incomplete, which is a pattern, not a run of bad luck. What I got wrong was the conclusion I drew from it: I treated "I am no longer confident in my grading of the script" as equivalent to "this PR is not safe to merge". Those are different claims, and on re-examination only the first is true. What merging this actually changes. Nothing runs. Where the residual risk actually lands. It is realised at dispatch time, not merge time: someone runs the workflow and reads a verdict the summariser produced. That is already an explicitly approval-gated action, and I am merging on that basis, and recording the reservation rather than dropping it: the caveat is that this instrument's grading logic needed eight corrections, so the first dispatch should be read as a measurement to be sanity-checked, not as an oracle. I will file that in State on Generated by Claude Code |
The reservation raised during this PR's review belongs where whoever dispatches the workflow will find it, months from now, rather than in a review thread. Records what the instrument is, that the decision rule was committed before any numbers were read, that it fails closed, and that its grading logic took eight rounds of corrections — so the first dispatch is a measurement to sanity-check against the raw artifact, not an oracle, most of all on a PASS that would close seven findings at once. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FvU8z73P6TXUXoYBqN5K1P
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:7ed97850fe
ℹ️ 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.
… one Rounds 9 and 11 found the same root cause from two directions, and both were right. `breaches` and `productionVerdict` chose the verdict sentence, while `incomplete` was appended afterwards as a separate paragraph — so a summary could assert "Every mobile route is within LCP < 2500ms and CLS < 0.1" in bold and then admit two paragraphs later that the evidence was incomplete, or call a staging breach's findings "actionable" for a production ledger item. Whichever line a reader took away, one of them was false, and this text is written to the step summary before the process exits. Rather than gate the two sentences individually — which is what the last two rounds of this kind did, and why a third instance existed — the disqualifiers are now resolved BEFORE any verdict prose. Incomplete evidence and a non-canonical measured origin are collected together, and if either holds the function returns after one non-verdict statement plus the measurements marked "for reference only". The threshold and actionability sentences are unreachable unless every precondition holds, so a fourth instance of this class cannot be written by editing one branch. Round 10's Chrome finding is also right: Chrome ships with the ubuntu-24.04 runner image, not with the pinned LIGHTHOUSE_VERSION, so it moves underneath a baseline and a metric shift can originate in the browser rather than the application. `summariseReport` now keeps `environment.hostUserAgent` — what actually drove the run, rather than what the runner claims — and `main()` records the distinct values in summary.json next to `lighthouseVersion`. Disagreement across reports in one run is itself visible. This is the record-and-compare half of that finding; pinning Chrome outright is not available on a GitHub-hosted runner without a container. Five new assertions, all verified red against the previous shape: × suppresses the threshold claim when a run measured the wrong page × does not call a staging breach actionable × records the Chrome build that produced the report (+2) verify:cheap exit 0 — Test Files 430 passed (430), Tests 4431 passed | 4 skipped. check:github-actions — GitHub Actions pin check passed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FvU8z73P6TXUXoYBqN5K1P
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:b345746e5c
ℹ️ 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: 1
🧹 Nitpick comments (1)
tests/summarise-web-vitals.test.ts (1)
320-338: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDo not encode a full user agent as
chromeVersion.This test freezes the entire
hostUserAgentstring under a field whose name implies a parsed browser version. Rename the field tohostUserAgent, or extractChrome/<version>and keep thechromeVersionname so downstream consumers cannot misinterpret the summary.🤖 Prompt for 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. In `@tests/summarise-web-vitals.test.ts` around lines 320 - 338, The measurement-environment summary currently stores the full host user agent in chromeVersion. Update summariseReport and its tests to either expose this value as hostUserAgent or parse only the Chrome version while retaining chromeVersion; ensure the missing-report case remains null and downstream consumers receive a semantically accurate field.
🤖 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 111: Update the decision rule in the `#017` entry and its reference to `#016`
so a passing mobile LCP/CLS measurement only closes the explicitly measured
payload findings. Keep `#016`’s motion, CSS, client-waterfall, caching, and
dynamic-import items open unless separate evidence validates them, and adjust
the stated closure scope accordingly.
---
Nitpick comments:
In `@tests/summarise-web-vitals.test.ts`:
- Around line 320-338: The measurement-environment summary currently stores the
full host user agent in chromeVersion. Update summariseReport and its tests to
either expose this value as hostUserAgent or parse only the Chrome version while
retaining chromeVersion; ensure the missing-report case remains null and
downstream consumers receive a semantically accurate field.
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
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: c8aea2b2-3497-47d2-92b9-d8f9bf488300
📒 Files selected for processing (4)
docs/branch-review-ledger.mddocs/outstanding-issues.mdscripts/summarise-web-vitals.mjstests/summarise-web-vitals.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/branch-review-ledger.md
- scripts/summarise-web-vitals.mjs
Uh oh!
There was an error while loading. Please reload this page.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FvU8z73P6TXUXoYBqN5K1P
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:5516e634b4
ℹ️ 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/outstanding-issues.md (1)
161-161: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winRestore the missing
Resolvedtable cell.This row has four cells instead of the archive table’s required five, so the resolution date is missing and markdownlint reports MD056.
Proposed fix
-| `#111` | issue | `ui-overlap` phone-inset case still flakes under load | RESOLVED 2026-07-29 (PR `#1391`). The inset measurement now retries inside `expect(async () => …).toPass({ timeout: 15_000 })` (`ui-overlap.spec.ts:159`) — the same retry-the-measurement shape PR `#1375` applied to the eight overlap cases. The 2px symmetry tolerance and the assertions themselves are byte-for-byte unchanged, so a genuinely asymmetric header still fails once the retry budget is spent; only a transient mid-remount sample settles. Full spec green across 4 consecutive runs (14 passed each). Do not relax the tolerance if this recurs — the assertion is the point; make the measurement robust instead. Source: PR `#1375` flake triage; session 2026-07-29 |+| `#111` | issue | `ui-overlap` phone-inset case still flakes under load | RESOLVED 2026-07-29 (PR `#1391`). The inset measurement now retries inside `expect(async () => …).toPass({ timeout: 15_000 })` (`ui-overlap.spec.ts:159`) — the same retry-the-measurement shape PR `#1375` applied to the eight overlap cases. The 2px symmetry tolerance and the assertions themselves are byte-for-byte unchanged, so a genuinely asymmetric header still fails once the retry budget is spent; only a transient mid-remount sample settles. Full spec green across 4 consecutive runs (14 passed each). Do not relax the tolerance if this recurs — the assertion is the point; make the measurement robust instead. Source: PR `#1375` flake triage; session 2026-07-29 | 2026-07-29 |🤖 Prompt for 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. In `@docs/outstanding-issues.md` at line 161, Update the `#111` row in the outstanding-issues archive table to restore the missing Resolved status cell, ensuring the row has the required five cells and includes the resolution date consistently with the existing resolution text.Source: Linters/SAST tools
🤖 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.
Outside diff comments:
In `@docs/outstanding-issues.md`:
- Line 161: Update the `#111` row in the outstanding-issues archive table to
restore the missing Resolved status cell, ensuring the row has the required five
cells and includes the resolution date consistently with the existing resolution
text.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f95759c1-14e1-4ee4-a0e7-6d8f4e46c00b
📒 Files selected for processing (2)
docs/branch-review-ledger.mddocs/outstanding-issues.md
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/branch-review-ledger.md
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. The branch was updated while autofix was in progress. Please try again. |
BigSimmo
commented
Jul 30, 2026
@cursoragent fix merge conflict and all comments resolve |
Merge conflict cleared and all open review threads resolved on PR #1385. Merge: Merged Fixes in
Verification: Threads: 19/19 resolved (0 open). PR head:   |
…s-workflow Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Encode query params when comparing measured vs requested URLs so an encoded separator cannot alias a different page; strip a trailing slash from LIVE_DOMAIN_URL before route append; narrow the #17 closure rule so LCP/CLS cannot WONTFIX all of #16; restore the #111 archive Resolved cell. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Summary
Ledger
#017gates seven client findings from the 2026-07-28 latency audit, plus#012/#013/#016. Nothing in the repo could produce the evidence it asks for — in-sandbox vitals are blocked because the production server hard-requires Supabase secrets, and local Playwright timings rank routes against each other without measuring the real origin. This adds the missing instrument.Modelled on
live-domain-monitor.yml: no secrets, no third-party actions, no providers beyond what an anonymous visitor triggers,LIVE_DOMAIN_URLrepo-variable override, pinned action SHAs. Dispatch-only rather than scheduled — it is heavier than that monitor's plain GETs, and the decision it feeds is made a few times a year.The decision rule is committed before any numbers are read, so the gate cannot be rationalised afterwards:
#017as metrics-acceptable; the gated payload findings become WONTFIXBased on prior evidence I expect the first branch:
#069closed with hosted p90 175–243 ms and "plans are not the multi-second tail", and#017's own row records dev-mode CLS 0.00–0.04. This repo has a track record of inferred perf claims that measurement refuted — which is the point of measuring before optimising.Why the summariser is a script, not inline YAML
scripts/summarise-web-vitals.mjskeeps the escaping out of YAML-inside-shell-inside-node quoting and makes the thresholds testable. It fails closed: a missing LCP or CLS counts as a breach, because an absent number is not evidence of passing — exactly the mistake#017exists to prevent.Lab metrics only. Lighthouse cannot measure INP in lab conditions (it is an interaction metric; TBT is the lab proxy), so the rule's INP clause is confirmed from CrUX field data and the pass verdict is stated as provisional on that.
Also records the PR #1376
prlandedverification in the branch review ledger.Correction — a missing mobile run read as a pass (
6972f367)Review raised at P1, and it was correct. The workflow deliberately downgrades a per-route Lighthouse failure to a warning so one flaky route cannot discard the whole run. But the summariser enumerated only the reports that existed and rejected only an entirely empty directory. A directory holding just
desktop-root.jsontherefore produced zero mobile breaches and rendered "Every mobile route is within…" — recommending#017be closed and its seven gated findings marked WONTFIX on evidence that was never collected.I reproduced it against the original function before changing anything:
That is the exact failure mode
#017exists to prevent, in the script whose stated purpose is to fail closed — "an absent number is not evidence of passing" was implemented, but an absent run was not.The fix tracks the requested route matrix rather than the surviving files. The workflow passes
ROUTESinto the summariser;expectedMobileRuns()derives themobile-<slug>names using the same slug rule as the workflow'ssed, and any expected run with no report is a breach. The summarise step also exits non-zero when reports are missing, so an incomplete run cannot read as a pass at the job level either. Zero mobile reports with no route list supplied is still a breach, so the guard cannot be bypassed by omitting the argument.Verification
check:github-actions(pin check) —GitHub Actions pin check passed.tests/summarise-web-vitals.test.ts— new.Test Files 1 passed (1)/Tests 8 passed (8). Covers the pass case, missing-metric, over-threshold, thedesktop-root.json-only reproducer, and the no-route-list case. Each was checked to fail against the pre-fix shape rather than pass vacuously.Risk and rollout
live-domain-monitor.yml.Notes
Read the summary table in the run's step summary; full Lighthouse JSON is uploaded as a 30-day artifact. Record the
#017verdict indocs/outstanding-issues.mdeither way — a pass is as valuable as a breach here, because it closes seven findings. A run with any missing mobile report is not a verdict at all; rerun it first.RAG impact: no retrieval behaviour change — adds a dispatch-only CI workflow, a standalone summariser script, and its unit test; no retrieval, ranking, selection, or answer-generation code is touched.
Summary by CodeRabbit