Speed up site testing selection and CI Production UI shards - #1686
Conversation
Document the testing-speed playbook, reuse one Playwright build root across phone-chrome browser stages, fail closed when verify:pr-local's selected build is refused, switch PR UI to duration-aware explicit shards with parity guards, trim redundant viewport matrix cells, and detect Cloud Playwright browser revision drift without forcing mismatched executables. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR adds duration-aware Playwright sharding, browser revision validation, shared phone-Chrome build roots, and fail-closed local verification reporting. CI, tests, scripts, and verification documentation use these workflows. ChangesVerification hardening
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CI
participant PackageScripts
participant PlaywrightPrShards
participant Playwright
CI->>PackageScripts: invoke test:e2e:pr:shard with shard number
PackageScripts->>PlaywrightPrShards: resolve validated production files
PlaywrightPrShards->>Playwright: run selected Chromium specs
sequenceDiagram
participant CLI
participant BrowserRevisionCheck
participant PlaywrightCore
participant BrowserRoot
CLI->>BrowserRevisionCheck: request revision validation
BrowserRevisionCheck->>PlaywrightCore: read expected Chromium revision
BrowserRevisionCheck->>BrowserRoot: inspect installed revisions
BrowserRevisionCheck-->>CLI: return validation status
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
CI triageCI failed on this PR. Automated classification of the 3 failed job(s):
Compared with main CI run #8920 (cancelled). Classification is evidence routing, not permission to ignore a failure. Exact quarantined Playwright identities remain governed by the flake ledger. |
Blank lines after #259 ended the Markdown archive table, so check:outstanding-issues treated the PR resolutions as deleted. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Autopilot + Bugbot pass (PR #1686)Final tip: Snapshot (post-push)
Actions taken (priority order)
Verification
Blockers / notes
No merge performed. |
1 similar comment
Autopilot + Bugbot pass (PR #1686)Final tip: Snapshot (post-push)
Actions taken (priority order)
Verification
Blockers / notes
No merge performed. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Pull request overview
This PR refines the repository’s local/CI verification ergonomics for Playwright-heavy UI work: it makes PR Production UI sharding duration-aware (explicit spec groups with a parity contract), adds a fail-closed guard so verify:pr-local can’t appear green when a selected build refuses to run, and documents a testing-speed playbook plus Cloud/container browser drift handling.
Changes:
- Replace CI Production UI
--shard=i/Nwith explicit, validated shard file groups (scripts/playwright-pr-shards.mjs) and update CI wiring accordingly. - Ensure
verify:pr-localfails closed and prints a closing summary whenbuildis selected but refused due to a running dev server (dedicated exit code 76 /BUILD_REFUSED_DEV_SERVER). - Speed-focused phone-chrome improvements: reuse a single Playwright build root across multiple browser stages (then clean it), trim redundant viewport matrices, and add a fail-closed Playwright browser-revision drift check + docs.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/verify-pr-local.test.ts | Adds unit coverage for fail-closed verify:pr-local build refusal summary/exit behavior. |
| tests/verify-phone-chrome.test.ts | Verifies shared Playwright build-root reuse across multiple browser stages and cleanup behavior. |
| tests/ui-tools.spec.ts | Removes a redundant tablet viewport from the tools launcher matrix (keeps representative breakpoints). |
| tests/ui-smoke.spec.ts | Trims duplicate phone viewport cells in the dashboard smoke matrix. |
| tests/test-runner-safety.test.ts | Updates CI contract assertions to require shard-runner usage and reject old --shard=i/3 wiring. |
| tests/playwright-pr-shards.test.ts | Adds parity tests ensuring explicit shard groups cover every production spec exactly once and stay aligned with config intent. |
| tests/guard-next-build.test.ts | Pins the dedicated refused-build exit code (76) to prevent “green-looking” refusals. |
| tests/check-playwright-browser-revision.test.ts | Adds tests for the new fail-closed container Chromium revision drift check. |
| scripts/verify-pr-local.mjs | Exports selection + adds run-summary and fail-closed execution path for selected scripts (notably build refusal). |
| scripts/verify-phone-chrome.mjs | Adds shared build-root env wiring across multiple browser stages and ensures cleanup runs before exit. |
| scripts/playwright-pr-shards.mjs | Introduces duration-aware explicit Production UI shard groups plus validation and CI runner entrypoint. |
| scripts/guard-next-build.mjs | Adds distinct exit code + explicit refusal marker to make dev-server build refusals unambiguous. |
| scripts/check-playwright-browser-revision.mjs | Adds fail-closed detection of /opt/pw-browsers revision drift vs locked Playwright expectations. |
| package.json | Adds test:e2e:pr:shard, shard validation, and browser revision check scripts. |
| docs/testing.md | Documents the testing-speed playbook, local Playwright keep-root, refuted levers, and drift delegation to CI. |
| docs/scripts-index.md | Updates script counts and indexes the new scripts. |
| docs/process-hardening.md | Adds a pointer to the testing-speed playbook and updates CI-sharding guidance to “shipped” state. |
| docs/outstanding-issues.md | Archives resolved items (#167 and #255) with outcomes matching the new guards/docs. |
| docs/codex-cloud.md | Documents Playwright browser readiness and drift handling in Cloud/container environments. |
| docs/branch-review-ledger.md | Records the work/review entry for this PR branch. |
| AGENTS.md | Adds a drift-handling note linking to the testing-speed playbook guidance. |
| .github/workflows/ci.yml | Switches Production UI job from Playwright --shard to the explicit shard runner (test:e2e:pr:shard). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/codex-cloud.md`:
- Line 59: Correct or remove the `#255` tracking reference for the Playwright
browser-readiness work in docs/codex-cloud.md lines 59-59, AGENTS.md lines
1015-1015, and tests/check-playwright-browser-revision.test.ts lines 26-26;
ensure all three references use the same valid tracking ID or omit the ID
consistently.
In `@scripts/guard-next-build.mjs`:
- Around line 39-40: Remove or replace the incorrect (`#167`) issue reference in
the comments and test descriptions for DEV_SERVER_BUILD_REFUSED_EXIT_CODE, the
verify-pr-local summary, and the related guard-next-build and verify-pr-local
tests. Apply the correction in scripts/guard-next-build.mjs lines 39-40,
scripts/verify-pr-local.mjs lines 112-117, tests/guard-next-build.test.ts lines
29-32, and tests/verify-pr-local.test.ts lines 67-85; no behavioral changes are
needed.
In `@tests/check-playwright-browser-revision.test.ts`:
- Line 20: Update the two playwrightBrowserRevisionCheck fixtures in
tests/check-playwright-browser-revision.test.ts at lines 20 and 39-42 to include
NODE_ENV: "test" in their env objects while preserving all existing PLAYWRIGHT_*
variables. Leave the runPhoneChromeStages fixtures in
tests/verify-phone-chrome.test.ts at lines 162 and 192 unchanged.
In `@tests/playwright-pr-shards.test.ts`:
- Around line 31-36: Update the shard iteration in the test around
filesForPrUiShard so shard is inferred as the literal union 1 | 2 | 3 rather
than number, allowing type-safe indexing into prUiShardGroups while preserving
the existing assertions.
🪄 Autofix
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: 2952c10f-75ce-4e99-b67f-acb4aa3d0266
📒 Files selected for processing (22)
.github/workflows/ci.ymlAGENTS.mddocs/branch-review-ledger.mddocs/codex-cloud.mddocs/outstanding-issues.mddocs/process-hardening.mddocs/scripts-index.mddocs/testing.mdpackage.jsonscripts/check-playwright-browser-revision.mjsscripts/guard-next-build.mjsscripts/playwright-pr-shards.mjsscripts/verify-phone-chrome.mjsscripts/verify-pr-local.mjstests/check-playwright-browser-revision.test.tstests/guard-next-build.test.tstests/playwright-pr-shards.test.tstests/test-runner-safety.test.tstests/ui-smoke.spec.tstests/ui-tools.spec.tstests/verify-phone-chrome.test.tstests/verify-pr-local.test.ts
Resolved docs/outstanding-issues.md conflict by keeping both sides' resolutions (#167, #247, #255 resolved; both removed from the open queue) and taking main's newer #253 text (reflects #247's resolution). Fixed TS errors surfaced by the merged main tsconfig: NODE_ENV missing from partial env objects in check-playwright-browser-revision.test.ts and verify-phone-chrome.test.ts, and a non-const shard array losing its literal type in playwright-pr-shards.test.ts.
|
@cursoragent Work the current open PR end-to-end. Confirm the PR number and GitHub head first from context. If more than one open PR could apply, stop and say which one you would use and why. Fetch and start from the remote tip that matches that GitHub head. If the named branch ref is missing or stale, use the PR head ref. Preserve unrelated local WIP, including any local-only ledger commits; do not discard dirty work, and do not treat a local-ahead commit as the reviewed tip. Do not merge the PR, force-push, rebase, or arm auto-merge unless I explicitly ask. No provider-backed gates without separate approval. If you cannot push or resolve threads, diagnose and comment only; if inline replies fail, resolve when possible and put dispositions in the summary comment. If auto-merge is already armed, push only for a real blocker, and avoid pushes that would cancel in-flight required CI unless the push itself clears that blocker. If the PR is already merged or closed: confirm the head and merge commit, note required-CI outcome, post one summary, and stop. Goal: deep review plus Bugbot, fix actionable issues with the smallest correct changes, clear merge / required-CI / thread blockers, run strong local offline verification, push fixes, append the review ledger, and post one PR summary. Prefer thoroughness over speed. Regenerate large assets only when a fix requires it; then run the asset check and keep compatibility aliases byte-identical where the repo uses them. Snapshot the GitHub head SHA: tip, base, behind/ahead, mergeable state, merge-tree versus origin/main (real conflict versus behind-but-clean), required checks on that tip including Production UI when selected, advisory separately, unresolved actionable threads. Missing checks while dirty are not green. If the tip moves mid-work, re-snapshot and continue from the new head. Ledger-lookup against that GitHub head under the heavy review-and-fix scope for this PR. Already reviewed at this head with clean merge-tree, green required checks, and no new actionable threads → summarize, comment, stop unless I asked for a fresh superseding pass. Follow the repo review protocol. Unblock once: real conflict → merge origin/main (prefer main’s shared queues; keep this PR’s notes); behind-but-clean → one sync, late if required CI is in flight; clean → leave. Labels like skip-branch-sync do not block a manual sync needed to clear a real blocker. No sync thrash. After any sync or push, re-snapshot tip, merge-tree, and required checks before declaring done. Dedupe the ledger if a merge touched it. Review high-confidence delta risks only. Separate PR-introduced defects from pre-existing re-emitted debt. For generated assets, review contracts, aliases, cache, and manifests—not every generated line. If protected RAG or ranking surfaces are touched, say so before editing; fix PR-body policy text only when wrong or missing. Ignore bot noise. No nit spam or broad rewrites. Escalate verification by touched risk: clinical / RAG / privacy / migrations / auth → domain check plus production-readiness when warranted; UI / phone-chrome / routing / styling → ensure plus phone-chrome or UI gates when warranted; generated assets or docs inventory/links → asset or docs checks when warranted. Fix P0/P1 always; clear scoped P2 when locally provable; else disposition and resolve. Required-check failures on this tip only; ignore advisory. Prefer reverting a bad autofix commit. After fixes: format and commit before push; smallest targeted proof; repo cheap gate; repo PR-local gate. No release, lighthouse, live eval, or live provider gates without approval. Push only this PR’s fix commits. Append ledger for the final GitHub head under the heavy scope. Never push a ledger-only tip—include ledger in a real fix push, or leave a no-change ledger append local and unpushed. One PR comment: tip, sync/merge-tree, fixed versus dispositioned, threads, required CI, decisive local gate lines, residual risks; merge left to me. Inline only for remaining human-needed P0/P1. Stop when merge-tree is clean on the current GitHub tip, actionable threads are clear, required checks are green or clearly in progress, heavy gates for scope passed, summary posted, merge left to me. |
PR #1686 — review-and-fix completePR: #1686 Snapshot
No sync was needed — tree merge is clean. Main is 2 commits ahead ( Review outcomeNo code push — the head is already in good shape from prior autopilot work (lint fix, main sync, CodeRabbit dispositions:
Local verification (decisive lines)Ledger & comment
Merge is yours. Mark ready when you want it out of draft. Optional: sync   |
Cursor review-and-fix pass (PR #1686)GitHub head: Sync / merge-treeNo sync performed this pass. Fixed vs dispositionedNo code push — head already green on required CI; prior commits addressed lint, main sync,
Required CI (tip
|
Pull request was converted to draft
Main added tests/ui-phone-scroll-document-rail.spec.ts as a production e2e:pr spec; the explicit shard inventory in playwright-pr-shards.mjs did not list it, so tests/playwright-pr-shards.test.ts failed on orphans.
* docs(ledger): record Run PR sweep outcomes for open PRs Append one superseding Run PR sweep row per PR touched in this pass (conflict syncs, CI fixes, and review-thread code fixes). * docs(ledger): supersede Run PR rows for #1648 and #1686 post-merge CI fixes --------- Co-authored-by: Cursor Agent <cursoragent@cursor.com>
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Return a structured error when browsers.json is malformed, and pass an explicit projectRoot into shared Playwright build-root cleanup so cwd cannot leave temp dirs behind. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/testing.md (2)
34-34: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument the required verification timing.
The
PR readyrow can imply that formatting and verification are required only at final handoff. State thatnpm run formatmust run before every push, with the whole-tree result committed. Keepverify:pr-localas the final handoff gate when it covers the change.As per coding guidelines, run
npm run formatbefore every push, commit the formatting result, and runnpm run verify:pr-localor the smallest covering gate.🤖 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/testing.md` at line 34, Update the “PR ready” row in the testing documentation to state that npm run format must be run before every push and its whole-tree result committed. Retain npm run verify:pr-local as the final handoff gate when it covers the change, or identify the smallest covering gate.Source: Coding guidelines
191-195: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winRequire an exact
chromeVersionmatch in the refresh checklist.Lighthouse stores the complete
hostUserAgent, but the refresh check only requires one distinct non-empty value. A different full browser identity can pass the checklist and fail the next budget comparison, even with the same Chrome major.🤖 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/testing.md` around lines 191 - 195, Update the refresh checklist around the “one distinct chromeVersion” requirement to require an exact match against the pinned HeadlessChrome/<major> value, not merely one non-empty or same-major value. Clarify that the complete stored browser identity must match the expected pinned value before the refreshed baseline is considered usable.
🤖 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/branch-review-ledger.md`:
- Line 716: Update the 2026-08-07 verification entry in the branch review ledger
to replace the bare “build” gate with the decisive successful build output line
emitted by the build command. Preserve the other verification results and do not
report only a gate name or exit code.
- Line 716: Move the 2026-08-07 ledger record for
claude/search-bar-mobile-layout-buu0io from its current location to the end of
docs/branch-review-ledger.md, preserving every existing row unchanged. Use npm
run ledger:append for the new review entry or --supersede if correcting an
existing record, and restore append-only ordering.
---
Outside diff comments:
In `@docs/testing.md`:
- Line 34: Update the “PR ready” row in the testing documentation to state that
npm run format must be run before every push and its whole-tree result
committed. Retain npm run verify:pr-local as the final handoff gate when it
covers the change, or identify the smallest covering gate.
- Around line 191-195: Update the refresh checklist around the “one distinct
chromeVersion” requirement to require an exact match against the pinned
HeadlessChrome/<major> value, not merely one non-empty or same-major value.
Clarify that the complete stored browser identity must match the expected pinned
value before the refreshed baseline is considered usable.
🪄 Autofix
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: e7a14e33-9d81-4489-8ae8-d35be80d628b
📒 Files selected for processing (16)
.github/workflows/ci.ymlAGENTS.mddocs/branch-review-ledger.mddocs/codex-cloud.mddocs/process-hardening.mddocs/scripts-index.mddocs/testing.mdpackage.jsonscripts/check-playwright-browser-revision.mjsscripts/playwright-pr-shards.mjsscripts/verify-phone-chrome.mjstests/check-playwright-browser-revision.test.tstests/playwright-pr-shards.test.tstests/ui-smoke.spec.tstests/ui-tools.spec.tstests/verify-phone-chrome.test.ts
🚧 Files skipped from review as they are similar to previous changes (14)
- docs/codex-cloud.md
- tests/ui-smoke.spec.ts
- tests/verify-phone-chrome.test.ts
- docs/scripts-index.md
- tests/ui-tools.spec.ts
- package.json
- tests/playwright-pr-shards.test.ts
- tests/check-playwright-browser-revision.test.ts
- .github/workflows/ci.yml
- scripts/check-playwright-browser-revision.mjs
- AGENTS.md
- docs/process-hardening.md
- scripts/playwright-pr-shards.mjs
- scripts/verify-phone-chrome.mjs
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Summary
docs/testing.mdwith a pointer fromdocs/process-hardening.md.verify:phone-chromebrowser stages, then clean it on exit.verify:pr-local's selectedbuildis refused while the Clinical KB dev server is running (BUILD_REFUSED_DEV_SERVERexit 76 + closing summary) — closes#167.--shard=i/3to duration-aware explicit file groups (scripts/playwright-pr-shards.mjs) with a parity contract.ui-smoke/ui-tools(keep distinct layout owners).check:playwright-browser-revisionand docs for Cloud/container Chromium drift — closes#255without forcing mismatched executables.Verification
verify-phone-chrome,verify-pr-local,guard-next-build,playwright-pr-shards,check-playwright-browser-revision,test-runner-safety(67 passed)npm run test:ci-workflows(222 passed)node scripts/playwright-pr-shards.mjs --validatenpm run check:playwright-browser-revisionnpm run check:outstanding-issuesnpm run check:github-actionsnpm run docs:check-scriptsnpm run format(committed)npm run verify:pr-local/verify:ui— heavy path (lint+full unit+UI) not required for this gate/docs/test-infra change beyond the focused contracts above; CI Production UI will exercise the new shard runner.Risk and rollout
--shard=i/Nand prior viewport matrices.Notes
workers: 1,fullyParallel: false,retries: 0.Summary by CodeRabbit
New Features
Documentation
Bug Fixes