fix: Playwright browser preflight prevents #120 false greens - #1432
Conversation
Root-cause #120: exit-0 with "N failed" came from shell pipelines without pipefail; Node runners already propagated Playwright status. Add a Chromium headless-shell preflight before the heavy lock/build, announce phone-chrome stage failures on stderr, and document the measurement trap. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 24 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 (2)
📝 WalkthroughWalkthroughAdds Playwright browser-binary preflight checks before build startup, refactors phone-chrome stage execution to fail fast, strengthens related tests and documentation, and updates phone-scroll focus assertions and review records. ChangesRunner hardening and reliability
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant PlaywrightRunner
participant BrowserPreflight
participant Filesystem
PlaywrightRunner->>BrowserPreflight: validate requested projects
BrowserPreflight->>Filesystem: check browser executable paths
Filesystem-->>BrowserPreflight: return path availability
BrowserPreflight-->>PlaywrightRunner: continue or exit with failure
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Comment |
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
![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: a419a1ef6b
ℹ️ 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".
…ht-1392' into codex/review-pr1432
CI triageCI failed on this PR. Automated classification of the 3 failed job(s):
Compared with main CI run #6895 (cancelled). Classification is evidence routing, not permission to ignore a failure. Exact quarantined Playwright identities remain governed by the flake ledger. |
# Conflicts: # docs/scripts-index.md
|
@cursoragent resolve merge conflict |
|
Merge conflict on PR #1432 is resolved and pushed ( Only conflict was in
  |
# Conflicts: # docs/outstanding-issues.md
# Conflicts: # docs/scripts-index.md
This was a REAL conflict, not staleness, and it is why PR #1466 had no `CI` workflow run at all — only the four `pull_request_target` checks. GitHub could not build refs/pull/1466/merge. That is #116's signature: a missing check list is a conflict signal, not a green pass. `git merge-tree` exited 1 on tests/ui-phone-scroll.spec.ts while `PR mergeability` still showed success from before main advanced. main's #1432 edited a hunk inside the `phoneOwner` loop — precisely the region this branch moved to ui-phone-scroll-page-owned.spec.ts — so git saw one side delete the region and the other modify it. Resolved by taking this branch's shell-only ui-phone-scroll.spec.ts and re-applying #1432's change at its new home. That change is a genuine flake fix (sheet focus restoration retries after 50 ms when focus falls through to <body>, so a bare blur races the retry and can re-pin the header), and dropping it in a merge would have silently reintroduced the flake — the same fails-by-running-nothing class this branch exists to close. It went to the occurrence in the section-sheet round-trip, matched by the hunk's own context, not the similar-looking line 95 lines earlier. No content lost, proven rather than asserted: every non-trivial line of main's ui-phone-scroll.spec.ts is present across the three split files plus the shared helper. The only 22 differences are declarations that gained an `export ` prefix and the two imports whose path changed depth. Verified on the merged tree: verify:cheap exit 0, Test Files 437 passed (437), Tests 4588 passed | 4 skipped (4592); tsc --noEmit clean; prettier --check . clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JHLPEV4o1rzipPDqshCSHY
Earlier today I recorded on #121 that a sandboxed remote session has only two options for the Chromium 1194-vs-1234 mismatch: get the /opt write permission, or decline to claim browser evidence. That was an over-generalisation from a single blocked mkdir, and it is wrong. PR #1432 landed a preflight whose own failure message names the route I had missed: PLAYWRIGHT_CHROMIUM_EXECUTABLE_PATH, read by playwright.config.ts:11 and honoured by scripts/playwright-browser-preflight.mjs:101. It needs no filesystem write. Verified by launching rather than by reading the flag — the container's existing 1194 headless_shell drives fine under the repo's Playwright 1.62 client (version 141.0.7390.37, page rendered, boundingBox measured). #121 keeps the wrong sentence with the retraction beside it, because which claim was wrong and why is the part worth carrying forward. #130's recorded blocker for the pre-paint guard is marked LIFTED: that guard is buildable in a remote session after all. Also records #1432 itself, which no row referenced. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018K7sEKH35KZkWxvCnQcNN2
2 participants
Summary
verify:phone-chromeappearing to exit 0 while loggingN failed).N failedsymptom matches a shell pipeline withoutpipefail(cmd 2>&1 | tee …reports tee’s0while the log still endsN failed).scripts/playwright-browser-preflight.mjsfails before the heavy lock / production build whenchrome-headless-shell(orPLAYWRIGHT_CHROMIUM_EXECUTABLE_PATH) is missing;verify-phone-chromeannounces stage failures on stderr and unit-tests abort-on-first-failure;docs/testing.mddocuments the pipefail measurement trap.run-playwrightscripts as infrastructure inputs.mainresolved (7fbf7131): kept archived#110(from main) and#120(this branch); dropped both stale open rows that collided at the same table position.npm run check:outstanding-issuesandgit merge-tree --write-tree origin/main HEADare clean.Test plan
PLAYWRIGHT_CHROMIUM_EXECUTABLE_PATH=/tmp/missing-… node scripts/run-playwright.mjs --project=chromium→ exit1with preflight message (no build).npm run test -- tests/playwright-browser-preflight.test.ts tests/verify-phone-chrome.test.ts tests/test-runner-safety.test.ts→42 passed.npm run check:outstanding-issues→ passed after archiving Merge codex/RAG_FIX modes (Differentials/Services/Forms/Favourites) restyled to Clinical White theme #120 / after merge resolution.npm run verify:phone-chrome -- --dry-run --files scripts/playwright-browser-preflight.mjs,…selects lock/runtime/contracts.git merge origin/mainconflict resolved;git merge-tree --write-tree origin/main HEADclean.static-pr/pr-requiredon this branch.Summary by CodeRabbit
Bug Fixes
Documentation