test(phone-scroll): split the 2100-line spec, and fix two coverage holes it exposed

[BigSimmo]

Summary

• Split tests/ui-phone-scroll.spec.ts (2104 lines, 56 tests) into three files over a shared tests/helpers/phone-scroll.ts. Test bodies moved byte-identical; the seam is the page-owner model docs/search-chrome-behaviour.md already defines.
• Fixed a pre-existing coverage hole: nothing under tests/helpers/ was in uiPatterns, so editing a shared Playwright fixture skipped Production UI entirely.
• Fixed scripts/phone-chrome-plan.mjs, whose ownership stage would have selected zero tests after the split.
• Recorded that this split's stated justification is refuted — it does not rebalance the shards, and the measurement says why.
• Separately: made the run-coordinator's JSON writes atomic (fecd190). Unrelated to the split, but it failed this PR's first Unit coverage run and is a real defect, not a re-roll candidate. Its own commit, independently revertible.

RAG impact: no retrieval behaviour change — nothing under src/lib/rag/**, clinical-search, retrieval-selection, ranking-config, answer-ranking, the eval harness, or the golden fixture is touched.

File	Tests	Covers
ui-phone-scroll.spec.ts	21	shared shell header hide/reveal, per-mode top-edge release, universal collapse owner
ui-phone-scroll-routes.spec.ts	19	per-route breadth sweep (mode homes, dashboard scrollers, long pages)
ui-phone-scroll-page-owned.spec.ts	16	document-viewer composer, standalone frame-owned footers, calculators dock, Services canvas

The reason for this split is refuted, and that is the useful result

It was justified — in docs/process-hardening.md and in PR #1453 — as "the only lever that rebalances the shards, because --shard cannot divide a single file." Measured against a live server, it rebalances nothing:

	Before	After
Project total	342 tests, 18 files	342 tests, 20 files
Shard 1	121 (56 phone-scroll)	121 (56 phone-scroll)
Shard 2	111 (0)	111 (0)
Shard 3	110 (0)	110 (0)

--shard does not distribute files. It walks them in alphabetical collection order and cuts at test-count boundaries, so three siblings named ui-phone-scroll* sort adjacently and land in the same shard. The shards were already count-balanced — the imbalance is duration:

shard 1  121 tests / 10 files — phone-scroll(56), chrome-scroll(17), accessibility(15), overlap(14), …
shard 2  111 tests /  4 files — ui-smoke(92), route-coverage(12), specifiers(5), pwa(2)
shard 3  110 tests /  6 files — ui-tools(87), universal-search(16), stress(3), …

Shard 1 is slow because it holds ten slow-per-test files, while shards 2 and 3 are each dominated by one fast-per-test file. The only lever that would actually rebalance is explicit per-shard spec groups in ci.yml instead of --shard=i/N. docs/process-hardening.md now carries that trade rather than the change: perfect balance is ~7m37 against a measured 9m36 largest shard — about 2 min of a 13m39 critical path — bought with a hand-maintained file list in the required UI job whose miss mode is a spec silently running nowhere. It also carries a stop rule against the tempting hack of renaming specs to game the sort order.

Two coverage holes, both the "second hand-maintained list" shape

1. tests/helpers/** was invisible to ui_changed — pre-existing, and the more serious of the two. Three of the four files in that directory back ui-*.spec.ts journeys, and zero-touch.ts alone backs six of them including ui-smoke. Editing any of them reported ui_changed=false, so Production UI skipped and the change reached no browser at all, on a green pull request. Matched at directory level rather than by filename — a hand-list is exactly what failed here — and the one current non-UI file (supabase-round-trip-counter.ts) costs one extra UI run when it changes, against a miss that is silent.

2. scripts/phone-chrome-plan.mjs would have selected zero tests. Its ownership stage greps four journey titles out of ui-phone-scroll.spec.ts; all four moved to the page-owned file. Redirected, and phoneChromeBrowserSpecPattern widened so a changed sibling is recognised as a phone-chrome browser spec at all. playwrightHelper now includes the new shared fixture so editing it selects the phone-chrome plan.

Config regexes. playwright.config.ts's productionSpecPattern and testMatch both carried an exact phone-scroll alternative that matches neither sibling — they would have been collected by nothing. Both arms are now open (phone-scroll(?:-[a-z0-9-]+)?) so a future sibling runs rather than quietly not running, and tests/playwright-project-isolation.test.ts asserts every ui-phone-scroll*.spec.ts on disk is matched by both. It reads the regex literals out of the config source (the config cannot be imported — it resolves a base URL at module scope) and fails closed if either constant is renamed.

The coordinator write race (fecd190)

This PR's first CI run failed Unit coverage with SyntaxError: Unexpected end of JSON input at tests/test-runner-safety.test.ts:321, in "gives a queued heavyweight process priority over later focused work" — 4581 passed, 1 failed, and nothing in the split's diff goes near the run lock.

writeJson in scripts/test-run-lock.mjs published every coordinator record with a plain writeFileSync, and every reader of those records is concurrent by construction. The crash was the mild symptom: the coordinator's own reader has always swallowed the parse error (readJson returns null, queueRecords filters it out), so a torn read does not surface at all — the queued ticket simply disappears for that poll and hasTickets reports false while a queued exclusive waiter is sitting right there. That is the priority inversion the queue exists to prevent, and the tolerant reader was hiding it.

Now written to a sibling temp file and renamed over the target. renameSync is atomic within a filesystem; the temp name deliberately does not end in .json so listJsonFiles cannot pick it up mid-flight; a failed write removes the temp rather than leaving residue. The test's poll is also made tolerant, matching the coordinator's own reader — it was the only reader treating an unparseable file as fatal.

Verification

• npm run test:e2e -- tests/ui-phone-scroll.spec.ts tests/ui-phone-scroll-routes.spec.ts tests/ui-phone-scroll-page-owned.spec.ts --project=chromium — 56 passed (4.3m), 0 failed, 16/19/21 per file
• Collection parity against a live server: 342 tests in 18 files / 56 phone-scroll before, 342 tests in 20 files / 56 after
• npm run verify:cheap — Test Files 436 passed (436), Tests 4578 passed | 4 skipped (4582), exit 0
• tests/test-runner-safety.test.ts run 5× consecutively — 27 passed each time
• npm run check:ci-scope — self-test passes including the new playwright-shared-fixtures case
• npx prettier --check . — All matched files use Prettier code style!
• node scripts/check-outstanding-issues.mjs — 144 rows (70 open, 74 archived), unique ids, next-id=147 above the highest

Two local failures were diagnosed and are not defects in this branch:

• The first browser run failed 56/56 at browserType.launch in 3ms — that is #121 (container Chromium 1194 vs pinned 1234). The green result above is the re-run with PLAYWRIGHT_CHROMIUM_EXECUTABLE_PATH=/opt/pw-browsers/chromium.
• A local verify:cheap failed installed-lock-parity with expected 10000000 to be less than or equal to 40000 — stale node_modules (brace-expansion 1.1.16) against the lockfile this branch merged from main's build(deps): patch brace-expansion denial of service #1456. npm ci --include=dev brought it to 1.1.18 and the gate passed. That guard did exactly its job.

verify:phone-chrome not run as a whole: its browser stage is the same three specs already run directly above, and its contract stage is inside the verify:cheap unit run.

Not run, deliberately: verify:release, eval:*, check:supabase-project are provider-backed and not authorised here.

Risk and rollout

• Risk: low for behaviour, moderate for wiring — the split itself is a byte-identical move proven by a green 56/56 run and identical collection counts, but it moves files that four other places name. All four are updated in this diff and two of them now have guards. The tests/helpers/** widening can only cause Production UI to run more often, never less. The atomic write can only narrow a race.
• Rollback: git revert any of the three commits independently. The uiPatterns widening and the coordinator fix are both independently valuable and address defects that predate this branch.
• Provider or production effects: None. Test and CI-scoping code only; no application source, no schema, no provider calls.

Notes

#127 and #146 both cite the old file; their rows now name the new one, with the reminder that the exact test title is the durable identity because declaration lines drift.

The pattern across the last three changes on this branch is the same defect wearing different clothes: a list of spec or component names maintained in two places, where the copy that goes stale fails by running nothing rather than by going red. Three instances found in one day (mockup components, phone-chrome ownership journeys, shared Playwright fixtures). Each is now guarded individually; a general rule — that any list naming test files must be derived from or asserted against the filesystem — is probably the real fix, and is not attempted here.

Summary by CodeRabbit

• Bug Fixes

  • Improved phone scrolling and browser chrome behavior across routes, standalone mode, and page-owned layouts.
  • Prevented focus loss, layout shifts, footer misplacement, and near-bottom hide/reveal oscillation.
  • Made CI queue records safer during concurrent reads and writes.

• Tests

  • Expanded Playwright coverage for phone scrolling, pinned chrome, footer geometry, focus behavior, and route handling.
  • Ensured all phone-scroll test suites are collected and correctly included in affected test runs.

• Documentation

  • Clarified Playwright shard assignment behavior and updated phone-scroll coverage guidance.

[coderabbitai]

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 30 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 @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 2f886285-514e-4552-a8ea-7fa1e1395f8f

📥 Commits

Reviewing files that changed from the base of the PR and between 1878588 and 93cc68a.

📒 Files selected for processing (9)

• docs/branch-review-ledger.md
• docs/process-hardening.md
• scripts/ci-change-scope.mjs
• scripts/ensure-codex-cloud-git-remote.mjs
• scripts/phone-chrome-plan.mjs
• tests/codex-cloud-setup.test.ts
• tests/test-runner-safety.test.ts
• tests/ui-phone-scroll-page-owned.spec.ts
• tests/verify-phone-chrome.test.ts

📝 Walkthrough

Walkthrough

Changes

Phone scroll coverage and execution hardening

Layer / File(s)	Summary
Shared phone-scroll fixtures and UI coverage tests/helpers/phone-scroll.ts, tests/ui-phone-scroll*.spec.ts	Phone-scroll utilities are centralized and used by expanded route, page-owned chrome, focus, footer, geometry, and near-bottom behavior tests.
Playwright collection and shard guidance playwright.config.ts, tests/playwright-project-isolation.test.ts, docs/process-hardening.md, docs/search-chrome-behaviour.md	Phone-scroll filename matching and collection assertions are expanded, and shard behavior and documented coverage are corrected.
CI scope and phone-chrome routing scripts/ci-change-scope.mjs, scripts/phone-chrome-plan.mjs, tests/verify-phone-chrome.test.ts	Shared helper changes now select UI and phone-scroll consumer coverage, with synchronization tests for the consumer list.
Atomic test-run record publication scripts/test-run-lock.mjs, tests/test-runner-safety.test.ts	Coordinator JSON writes use temporary files and renames, while polling ignores temporarily invalid records.

Estimated code review effort: 4 (Complex) | ~45 minutes

Possibly related PRs

• BigSimmo/Database#1264: Both modify the shared phone-scroll test utilities and related UI coverage.
• BigSimmo/Database#1279: Both update phone-scroll helper consumer routing in the phone-chrome plan.
• BigSimmo/Database#705: Both modify test-run locking infrastructure.

Suggested labels: codex

Suggested reviewers: cursoragent, claude

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 46.67% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main change: splitting the phone-scroll spec and addressing coverage gaps it exposed.
Description check	✅ Passed	The description follows the template with Summary, Verification, Risk and rollout, and Notes, and the nonclinical sections are filled out.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Create stacked PR
• Commit on current branch

---

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

CI triage

CI failed on this PR. Automated classification of the 2 failed job(s):

• Production UI (1) — needs investigation: inspect the failing step and uploaded diagnostics; rerun only after classifying the cause.
• PR required — needs investigation: inspect the failing step and uploaded diagnostics; rerun only after classifying the cause.

Compared with main CI run #7314 (cancelled).

Classification is evidence routing, not permission to ignore a failure. Exact quarantined Playwright identities remain governed by the flake ledger.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6320250c6b

ℹ️ 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".

[Copilot]

@copilot Fix the code for all comments in this review thread.

When a review comment includes a suggested change, apply t...

Fixed in fecd190-adjacent commit. The doc comment now names tests/playwright-project-isolation.test.ts as the actual guard and describes what it asserts — the references to the non-existent phoneScrollSpecFiles constant and check-phone-scroll-specs.mjs script are gone.

[coderabbitai]

Actionable comments posted: 1

🤖 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/process-hardening.md`:
- Around line 189-193: Specify the `text` language on the fenced code block
containing the shard listing, changing its opening fence while preserving the
listing contents and closing fence.

🪄 Autofix (Beta)

✅ Autofix completed

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 38fb3b60-b31a-45b1-9da9-a536370a81a4

📥 Commits

Reviewing files that changed from the base of the PR and between db50738 and 1878588.

📒 Files selected for processing (13)

• docs/process-hardening.md
• docs/search-chrome-behaviour.md
• playwright.config.ts
• scripts/ci-change-scope.mjs
• scripts/phone-chrome-plan.mjs
• scripts/test-run-lock.mjs
• tests/helpers/phone-scroll.ts
• tests/playwright-project-isolation.test.ts
• tests/test-runner-safety.test.ts
• tests/ui-phone-scroll-page-owned.spec.ts
• tests/ui-phone-scroll-routes.spec.ts
• tests/ui-phone-scroll.spec.ts
• tests/verify-phone-chrome.test.ts

[coderabbitai]

Note

Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it.

Fixes Applied Successfully

Fixed 1 file(s) based on 1 unresolved review comment.

Files modified:

• docs/process-hardening.md

Commit: 5bcef7d9e10ef385fa17bbc805c1c8727e8f9485

The changes have been pushed to the claude/ci-testing-review-2l8klp branch.

Time taken: 2m 24s

[BigSimmo]

Temporarily paused during the owner-authorized ordered merge sweep so main cannot move between exact-head review and merge. The branch and all commits are preserved; this PR will be reopened at its verified place in the queue.

[BigSimmo]

@cursoragent this PR will be re-opened soon. Please resolve any failing CI, run bugbot and resolve any bugbot comments against current repo and fix any merge conflicts and also fix any errors and address and fix all comments or suggestions that are likely to improve the PR and Codebase. Please implement smallest required fix according to best code practices. Check your work. Leave this PR closed for now but still make all these changes and run the review. I want it ready to go for when I re-open it.

[cursor]

PR #1466 is ready to reopen and remains CLOSED. Branch tip: f6a27bc6 (contains origin/main).

Done

• Merged origin/main cleanly (was 7 behind).
• Bugbot: no cursor[bot] findings.
• Review threads: all 3 resolved (Codex helper consumers, Copilot doc refs, CodeRabbit MD040 — already fixed in tree).
• Last hosted CI on the pre-close tip was green; closed PR UI still shows frozen head 5bcef7d until reopen.

Fixes landed

1. phoneContract sibling miss — ui-phone-scroll-routes / -page-owned now select contracts + phoneRelevant like the base file; pinned in verify-phone-chrome.test.ts (ae74f57e).
2. Codex Cloud origin inspect — use configured remote.origin.url, not git remote get-url (insteadOf token rewrite was breaking verify:cheap here) (fa304a53).

Verification

Test Files  444 passed (444)
Tests  4652 passed | 4 skipped (4656)

prettier --check . pass · check:ci-scope pass · review ledger recorded for fa304a53.

On reopen, GitHub should move the PR head to f6a27bc6 and CI will re-run on that tip.

  

[supabase]

This pull request has been ignored for the connected project sjrfecxgysukkwxsowpy because there are no changes detected in supabase directory. You can change this behaviour in Project Integrations Settings ↗︎.

---

Preview Branches by Supabase.
Learn more about Supabase Branching ↗︎.

[BigSimmo]

@cursoragent This PR was just reopened. Do not merge, force-push, rebase, or arm auto-merge
unless I explicitly ask.

1. Confirm GitHub is on the current branch tip (not the old closed head).
   Snapshot head SHA, behind/ahead, mergeable state, required checks, and
   unresolved actionable threads.

2. Mergeability on this tip only:

   • Real conflict (merge-tree dirty): smallest fix, push.
   • Behind-but-clean: if required CI is in flight, wait and sync once late;
     otherwise sync once now. Prefer human/operator identity for update-branch;
     else git merge origin/main + push. No sync thrash.
   • Clean/current: leave it.
     Missing checks while mergeability is dirty ≠ green.

3. Fresh required CI on this tip only — ignore pre-close rollup. On failure:
   smallest fix, push, re-check. Don’t rerun unless a fix landed or a check
   is stuck. No provider-backed gates without approval.

4. New/unresolved actionable Codex/Bugbot/Copilot/human threads: smallest fix
   or short already-fixed/not-actionable reply, then resolve. Ignore
   rate-limit/summary noise.

5. Stop when: merge-tree clean, no actionable threads, required checks green
   or clearly in progress on this tip. Report tip SHA, sync done/not needed,
   CI status, threads handled, residual risks. Leave the merge decision to me.

[cursor]

1. Snapshot (reopen)

Tip	93cc68a9 (after sync; was a76601d5)
GitHub head	matches tip
Was	1 behind / 27 ahead · GitHub CONFLICTING/DIRTY
merge-tree	clean (false DIRTY — behind-but-clean)
Threads	0 unresolved
Checks on old tip	only PR mergeability FAILURE + thin pull_request_target set (merge ref unbuildable)

2. Sync

Synced once: git merge origin/main + push (#1510). No content conflict (docs/branch-review-ledger.md auto-merged).
gh identity unavailable for update-branch; sync:pr-branches skipped this PR (skip-branch-sync label) — merge+push used instead.

3. CI on this tip

Fresh runs queued/in progress on 93cc68a9:

• CI pending · Gitleaks/Semgrep queued · PR Policy in progress · mergeability re-queued
  Now MERGEABLE / BLOCKED (checks not finished). Pre-close rollup ignored.

4. Threads

None actionable. Prior Codex/Copilot/CodeRabbit threads stay resolved.

5. Stop

• merge-tree clean vs origin/main (0 behind)
• 0 unresolved actionable threads
• required checks in progress on this tip

Tip: 93cc68a9 · Sync: done once · CI: running · Threads: none · Merge: left to you

Residual: skip-branch-sync will block automated sync next time main moves; phone-scroll still lives in slow shard 1 (documented).