ci: shard Production UI, and make the phone-scroll drag report what it did - #1427
Merged
Conversation
…ome hid
CI run 30518866604 failed `ui-phone-scroll.spec.ts:423` on
expect(getByTestId('universal-header-collapse'))
.toHaveAttribute('data-scroll-hidden', 'true') // received ""
after the full 10s auto-retry, and the classifier recorded it as "needs
investigation". The assertion was right; the scroll never happened.
`dragScrollBy` moved the scroller with `scrollTop +=`, which clamps silently
at the end of the range, and returned nothing. When a page lays out shorter
than the test assumed — content still settling under full-suite CI load — a
720px request delivers a fraction of that, the chrome correctly stays visible
because document-detail chrome only hides past `scrollTop > 120`, and the
failure surfaces ten seconds later looking like a product regression. The
helper also resolved the scroll owner once up front, so a mid-drag layout
change left it pushing an element that had stopped scrolling.
- `dragScrollBy` now re-resolves the owner each step and returns the distance
actually travelled.
- `dragScrollUntilHidden` waits for the remaining downward runway (a condition
wait, not a settle sleep), drags, and fails naming the shortfall if the drag
could not cross the threshold. Used at the four sites that assert a hide
immediately after a fixed-distance drag.
- `addPhoneScrollRunway` waits for its 1600px filler to reach layout instead of
sleeping 50ms. All 14 call sites already depend on that runway existing.
Every assertion is byte-identical: a genuinely stuck header still fails exactly
as before, once the drag is proven to have happened. No `.first()` was added
(#93's stop rule) and no tolerance was relaxed.
Measured on 2026-07-30 from the Actions API, two full UI-scope PR runs (30520443076, 30519912667): `Production UI` took 15m26-16m31 of a 16.8-18.6 minute run — 83-89% of wall clock — while every other job finished by minute 4 and then waited. Playwright itself reported `339 passed (13.5m)`; the balance is the isolated production build. That single job is also where the churn cost lands: 42% of PR runs in the sampled window were cancelled (25 of 60 completed), almost all superseded mid-Production-UI. Sharding is across runners, not workers. `workers: 1`, `fullyParallel: false` and `retries: 0` are unchanged inside each shard, so determinism is identical and per-runner load falls — which matters because #93's duplicate page root is load-dependent. `run-playwright.mjs` already forwards argv to `playwright test`, so `--shard` needed no runner change. The shard count is measured, not chosen. `fullyParallel: false` makes a spec file indivisible, so shard sizes are lumpy and more shards is not monotonically faster. Over the 340 required chromium tests: N=3 -> 121/106/113 largest 121 N=4 -> 121/106/96/17 largest 121 (same critical path, one more runner) N=6 -> 65/56/106/5/91/17 largest 106 N=5 -> 121/106/0/96/17 and N=8 -> two empty shards N=4 buys nothing over N=3, and any N with an empty shard would go red because `test:e2e:pr` deliberately omits `--pass-with-no-tests`. Expected critical path ~15.5 -> ~7 min, assuming per-test cost is roughly uniform. `fail-fast: false` so a failing shard cannot cancel its siblings and re-create the cancelled-vs-failed ambiguity #95 removed. Artifact names are shard-scoped because upload-artifact runs with `overwrite: false`. Branch protection requires only the `pr-required` aggregate, and `needs` on a matrix job yields the roll-up of all shards, so the aggregate is unchanged. Also adds `restore-keys` to both Playwright browser caches: without a prefix fallback a lockfile bump forced a cold browser download in every UI job at once, now three times over.
Two inconsistencies found while mapping the pipeline, neither load-bearing but both silent: - `codex-autofix-review-comments.yml` was the only workflow in the repo with no `timeout-minutes` on either job, so both inherited GitHub's 360-minute default for work that reads PR metadata and posts one comment. - `playwright.visual.config.ts` set neither `workers` nor `fullyParallel`, so it inherited Playwright's default `workers = 50% of CPUs`. The production config pins both to serial deliberately; the visual lane was quietly opting out of the anti-flake posture the rest of the suite is configured for.
Both numbers were wrong. `CLAUDE.md` said 24 static/consistency gates against an actual 25 — `check:assets` landed before that line was written, so it was wrong at authoring — and the `gates` skill said "check 2 of 26" against an actual 28. A stale count is not cosmetic here. The skill's whole point at that line is that `verify:cheap` stops at the first failure and everything after it never ran; an agent that believes the chain is 26 long cannot say how much a mid-chain failure skipped. `check:gate-manifest` already derives the real count from `verify:cheap:internal`, so it now asserts the documented numbers against it. The assertions fail closed: if the anchor phrasing disappears, the guard reports a lost anchor rather than passing on a document it no longer checks. Mutation-proven: reverting the skill to "26" fails with ".claude/skills/gates/SKILL.md says 26 where the chain has 28".
Five items from the CI/testing review that should not be changed blind: - #125 `ui_changed` matches all of `src/app`, so an API-only diff pays the 15-minute UI gate. Narrowing it can hide a real regression, so it needs a decision plus a compensating check rather than a quieter filter. - #126 the Playwright build writes to a per-run distDir, so Next's build cache is cold every run (~2 min, now ~29% of the sharded critical path). Fixing it means suppressing the runner's documented always-cleanup, which must not ship without executing the runner. - #127 the advisory UI lane spends ~3 min per UI PR on 5 mockup tests; there are currently zero `@quarantine` tests for it to cover. - #128 CI Triage is complete and self-tested but inert pending a repo variable. - #129 four `changes` outputs are computed and consumed by nothing, and `coverage_changed` fires on any non-doc file.
Contributor
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 19 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 (10)
Comment |
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
BigSimmo
marked this pull request as ready for review
July 30, 2026 08:44
Resolves the conflict that was silently stopping ALL CI on PR #1427: with a real conflict GitHub cannot build refs/pull/1427/merge, so every `pull_request` workflow was skipped with no failing check while `pull_request_target` ran. That is issue #116, and main's new `PR mergeability` check named it. Conflict resolutions: - `.github/workflows/ci.yml` — keep BOTH. Main split the UI gate into `ui-critical-fast` (@critical fail-fast) which `ui-critical` now `needs:`; the shard matrix lands on `ui-critical` only, preserving main's `needs:`/`if:` verbatim. `ui-critical-fast` stays unsharded: it is already the small subset, so sharding it would spend runners without shortening the critical path. - `playwright.visual.config.ts` — take main. It already sets `fullyParallel: false` / `workers: 1`, so this branch's version of that change was redundant and is dropped entirely. - `docs/outstanding-issues.md` — main allocated 125/126/127 concurrently, the exact #112 id race. This branch's five rows renumber to #128-#132, marker to 133, and the cold-cache cross-reference follows. - `docs/process-hardening.md` — keep both sections; the CI-timing note now records that its numbers predate `ui-critical-fast`.
…ate counts The merge changed both numbers this branch had recorded. Shard balance, re-measured against 342 required chromium tests (was 340): N=3 -> 121/111/110 largest 121 N=4 -> 121/106/98/17 largest 121 N=3 remains correct — one 121-test spec group bounds both, so N=4 spends an extra runner for the same critical path. The re-measure command is now in the workflow comment so the next person does not have to rediscover it. Gate counts: merging main added `check:gitleaks-pinned` and `check:pr-mergeability` to `verify:cheap:internal`, so the documented counts went stale the moment the merge landed — 25 -> 27 static, 28 -> 30 total. The guard added earlier in this branch caught it immediately rather than letting the docs drift again, which is the whole reason it exists. Also records the `ui-critical-fast` interaction: the UI critical path is now that 15-test fail-fast job plus the slowest shard, not the full 13.5-minute suite, so neither of this branch's pre-merge timings can be read on its own.
The `merge=union` driver on `docs/outstanding-issues.md` preserves concurrent appends, but when both sides restructure the same region it concatenates them wholesale. Merging the latest main did exactly that: every open row appeared twice and both `issues:next-id` markers survived — 66 duplicate-id errors from `check:outstanding-issues`, which is precisely the failure that gate exists to catch (#112). Resolved by rebuilding on main's canonical file rather than by hand-editing the duplicated table: reset to `origin/main`, then re-apply this branch's five captured rows at #131-#135 (main had advanced its allocation to #130 while this branch was open, so the earlier #128-#132 numbering collided again) and re-apply the #127 narrowing note. Marker bumped to 136. Union merge cannot allocate unique ids; only the structural gate can catch when it has produced an invalid file. It did.
BigSimmo
changed the title
…iew-2l8klp # Conflicts: # docs/branch-review-ledger.md
First real run of the sharded shape (CI 30530618838, all green, whole run 13m39 against a 16.8-18.6 min unsharded baseline): ui-critical-fast 15 tests 3m14 Production UI (1) 121 tests 9m36 Production UI (2) 111 tests 6m54 Production UI (3) 110 tests 6m20 The prediction was wrong by ~40%. ~6.8 min was expected for the largest shard from 121/342 tests x 13.5 min; 9m36 happened. Per-test cost is not uniform — 111 tests took 6m54 while 121 took 9m36 — so a count-balanced split understates the slowest shard whenever the slow specs land in one group. `--shard` can only balance by count; balancing by duration would mean splitting the slow spec files themselves. The win is real but smaller than claimed, and the workflow comment and process-hardening now carry the measured numbers plus the reason the arithmetic misleads, so the next person re-measures instead of re-deriving. Also merges origin/main. The ledger conflict was GitHub-visible only: that file carries merge=union locally, which GitHub does not honour (#129). Resolved by keeping the one genuinely new record and dropping three that main already had elsewhere in the file — append-only forbids dropping a record that exists once, not keeping a second copy. Superseding record appended for this HEAD, since the prior one asserted a root cause that #127's trace evidence refutes.
Contributor
CI triageCI failed on this PR. Automated classification of the 2 failed job(s):
Compared with main CI run #6849 (cancelled). Classification is evidence routing, not permission to ignore a failure. Exact quarantined Playwright identities remain governed by the flake ledger. |
…ocation CI caught what I did not: `static-pr` failed on `check:outstanding-issues` with #131-#134 duplicated and two `issues:next-id` markers. Cause: main's PR #1424 allocated #131-#134 for its own findings at the same time this branch held #131-#135, and `merge=union` did what union does — kept both sides under the same ids. That is #112's documented limit: union preserves concurrent appends but cannot allocate unique ids, so the structural gate is the only thing that catches it. My error was pushing without re-running that gate. The previous push resolved a `docs/branch-review-ledger.md` conflict, and I validated only that file before pushing to win the race against main — but the same merge also touched `docs/outstanding-issues.md`. `verify:cheap` would have caught it locally. Main's rows keep #131-#134 (already merged and referenced elsewhere); this branch's five renumber to #136-#140, one marker at 141, and the cold-cache cross-reference in process-hardening follows its row. Two of main's new rows also make a planned addition here redundant: #134 is the absent ledger merge driver and #133 is the outstanding-issues merge churn — both hit during this branch's work, both already captured upstream, so nothing new is filed for them.
The union merge duplicated the whole open and archive tables again (two header rows, every id twice) because main restructured the file while this branch held rows in it. Same resolution as before and for the same reason: rebuild on main's canonical file rather than hand-editing a doubled table, then re-apply this branch's five rows. Main is now at next-id=135, so they land as #135-#139 with the marker at 140. None of the five is duplicated upstream — checked by summary before re-applying. This is the third renumber of the same five rows in one PR. That is not a mistake being repeated, it is #133 ("outstanding-issues conflicts on nearly every main advance") happening: any branch that holds rows in this file re-collides every time main lands one. Worth weighing whether captures should land in their own PR ahead of the work rather than riding along with it.
BigSimmo
pushed a commit
that referenced
this pull request
Jul 30, 2026
…it asks for A real conflict this time, not staleness: main's PR #1427 rewrote the same helpers this branch touches, and it very likely found the actual cause of #127. `addPhoneScrollRunway` slept 50ms and merely hoped the appended 1600px runway had reached layout; `dragScrollBy` clamped silently at the end of the range while reporting nothing. Under CI load the drag therefore delivered less than it asked for and the chrome was right to stay visible. #1427 polls for the runway, returns the distance actually travelled, and `dragScrollUntilHidden` refuses to expect a hide until remaining runway and delivered travel both clear 160px. Main's helpers are taken whole. This branch keeps only what #1427's own comment says is still missing: "Separating THOSE two still needs the pin state exposed in the DOM; today only the composite `data-scroll-hidden` (`scrollHidden && !sharedChromePinned`) is observable, so both look identical." `data-scroll-signal` publishes the raw signal, and `expectChromeHidden` is cut down to answer only that question, now layered after `dragScrollUntilHidden` rather than duplicating its travel proof. #127 is rewritten again and withdraws a second wrong diagnosis of my own: a short/clamped drag was ruled out early using a maxOffset of 2753 read at a different trace moment than the failing drag, when the pre-runway reading in that same trace was 1153 — and a runway not fully landed puts the offset in the near-bottom band where computeScrollHideUpdate legitimately refuses. That is exactly what #1427 fixes. The row now points at #1427 as the likely fix, keeps the observability gap as the only open part, and says to close it if no recurrence appears on a post-#1427 head. Main also claimed #135 for an unrelated issue, so the union-driver finding renumbers to #140, marker 141. Verified: typecheck 0 errors, lint 0 problems, whole-tree prettier clean, check:outstanding-issues 138 rows / unique ids / next-id=141, and the merged phone-scroll spec 56 passed (4.1m) against an isolated production build — under Chromium 1194, not CI's bundled 1234 (#121). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XrPbbfU9yWuEjEVypCr4ZQ
BigSimmo
pushed a commit
that referenced
this pull request
Jul 30, 2026
Codex review is right: the new check accepted `unset` alongside `unspecified`, and those are not the same state. Per gitattributes, an Unspecified `merge` attribute is the documented default 3-way text merge — the contract this PR establishes — while Unset (`-merge`) takes the current branch's version and declares the merge conflicted, so every two-sided edit becomes a manual resolution. A global or future attributes file could therefore have violated the contract with the gate still printing "no merge driver". Reproduced before fixing: appending `docs/outstanding-issues.md -merge` made `git check-attr` report `merge: unset` and the guard passed. It now fails with a message naming the Unset/Unspecified distinction and telling the reader to drop the negated attribute rather than add one. The acceptance decision moves into an exported `mergeAttributeProblem` so the distinction is unit-tested rather than only reasoned about, with four cases in tests/repo-hygiene.test.ts: `unspecified` accepted; `unset`, `union`/`ledger`, and an empty reading all rejected. The empty case matters because an unparsed check-attr output would otherwise make the whole check vacuous. Verified: with `-merge` present the gate fails on the new message; with it removed it passes "no merge driver". repo-hygiene 47 passed (47). verify:cheap exit 0 — Test Files 435 passed (435), Tests 4508 passed | 4 skipped (4512). Whole-tree prettier clean. (Test total differs from this branch's earlier run because it now carries main's #1423/#1427/#1438; this commit adds four.) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XrPbbfU9yWuEjEVypCr4ZQ
BigSimmo
added a commit
that referenced
this pull request
Jul 30, 2026
* fix(ledger): remove merge=union from the issues ledger, per its own #133 Ledger #133 already recorded union as the wrong driver for this file — "two sides each bumping the marker produce two `next-id` lines, corrupting the file silently where a conflict would fail loudly" — but `.gitattributes` still set it and `check-outstanding-issues.mjs` *required* it, so the repo's own tested conclusion was contradicted by its own config. PR #1430 confirmed the cost at scale: four merges in one session, each reporting success while duplicating the entire open-items table (`#59 appears 2 times (lines 101, 166)` and so on for every row), each needing a manual rebuild from origin/main. Union also makes `git merge-tree` report a clean tree, so the pre-merge conflict check cannot warn. Unlike docs/branch-review-ledger.md — which keeps its custom `merge=ledger` driver, union plus exact-row dedupe — this file allocates IDs by read-modify-write. Concurrent appends therefore need manual renumbering whatever the driver does (hit twice on 2026-07-30: #125 and #135 collisions), so union bought nothing and only hid the overlap. Default 3-way merge conflicts honestly instead. The gate's attribute check is inverted rather than deleted, so a driver reappearing here is a red gate. AGENTS.md, docs/process-hardening.md, .claude/skills/issues/SKILL.md and docs/scripts-index.md are updated to match, and #133's driver half is marked resolved with its still-open half (fixed-width padding making every row edit one hunk) left intact. Verified: reintroducing `docs/outstanding-issues.md merge=union` fails the gate with "must have NO merge driver (found merge=union)", and removing it passes with "no merge driver" — the gate bites, not just passes. verify:cheap exit 0: Test Files 434 passed (434), Tests 4563 passed | 4 skipped (4567). Whole-tree prettier clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XrPbbfU9yWuEjEVypCr4ZQ * fix(gate): reject `-merge` too, not just a named driver Codex review is right: the new check accepted `unset` alongside `unspecified`, and those are not the same state. Per gitattributes, an Unspecified `merge` attribute is the documented default 3-way text merge — the contract this PR establishes — while Unset (`-merge`) takes the current branch's version and declares the merge conflicted, so every two-sided edit becomes a manual resolution. A global or future attributes file could therefore have violated the contract with the gate still printing "no merge driver". Reproduced before fixing: appending `docs/outstanding-issues.md -merge` made `git check-attr` report `merge: unset` and the guard passed. It now fails with a message naming the Unset/Unspecified distinction and telling the reader to drop the negated attribute rather than add one. The acceptance decision moves into an exported `mergeAttributeProblem` so the distinction is unit-tested rather than only reasoned about, with four cases in tests/repo-hygiene.test.ts: `unspecified` accepted; `unset`, `union`/`ledger`, and an empty reading all rejected. The empty case matters because an unparsed check-attr output would otherwise make the whole check vacuous. Verified: with `-merge` present the gate fails on the new message; with it removed it passes "no merge driver". repo-hygiene 47 passed (47). verify:cheap exit 0 — Test Files 435 passed (435), Tests 4508 passed | 4 skipped (4512). Whole-tree prettier clean. (Test total differs from this branch's earlier run because it now carries main's #1423/#1427/#1438; this commit adds four.) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XrPbbfU9yWuEjEVypCr4ZQ --------- Co-authored-by: Claude <noreply@anthropic.com>
This was referenced Jul 30, 2026
BigSimmo
pushed a commit
that referenced
this pull request
Jul 30, 2026
…f sleeping
Root-caused from the failure trace (run 30582678887 artifact 8775836025), not
inferred. Two snapshots either side of the resize:
before anchorTop -138 documentScrollTop 504 headerBottom 0
after anchorTop -7 documentScrollTop 504
`documentScrollTop` is IDENTICAL at 504. The scroll position never moved; the
content shifted down 131 px. And 131 is not arbitrary — the trace records
collapseHeight 72, and gotoPhoneSurface injects --safe-area-top:59px. 72 + 59 =
131. The resize transiently re-shows the shared header, which reclaims its
collapse row AND its safe-area band, and the assertion measured mid-transient.
The cause is the fixed `waitForTimeout(100)` immediately after
`setViewportSize`. That is the same sleep-and-hope pattern PR #1427 removed from
`addPhoneScrollRunway`, and it explains every observation #146 has collected:
- bimodal, never in between: the two states are "settled hidden" (-138) and
"header fully re-shown" (-7), nothing between them
- pixel-identical across runs: -138/-7/131 reproduced on three separate heads
and two different PRs, which timing jitter cannot do
- "under CI load": load pushes the settle past 100 ms, nothing more exotic
- documentScrollTop unchanged: only chrome layout moved
This also retroactively settles the narrowing that #146 WITHDREW. That row once
argued the scroll position held because the sibling documentScrollTop assertion
did not fail — invalid, since Playwright aborts at the first failing expect so
it never ran. The conclusion was right anyway: the trace shows 504 on both
sides. It is now measured rather than inferred.
The fix polls the same geometry the hidden-state assertions already use
(`header#search` bottom <= 1) with a 10 s budget and a message naming the
condition. The tolerance is untouched — #146's stop rule forbids loosening it,
and loosening it would have hidden a real 131 px content jump.
Note what this does NOT claim: if the header ever fails to re-hide rather than
merely settling slowly, the poll times out and the test fails naming that. That
is strictly better than a 100 ms coin flip either way.
Verified: the previously-failing test 3x consecutively (1 passed each), and the
full spec 16 passed (43.0s). Local passes are weak evidence for a load-dependent
race; the mechanism change is the argument.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JHLPEV4o1rzipPDqshCSHY
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A review of the CI testing process for flakiness, speed, and signal quality, measured against the live pipeline rather than inferred from the workflow files. Each item is its own commit and is independently revertible while this PR is open.
ci: shard Production UI across three runners. Sharding is across runners, not workers —workers: 1,fullyParallel: falseandretries: 0are unchanged inside each shard, so determinism is identical and per-runner load falls. Also addsrestore-keysto both Playwright browser caches, which previously forced a cold browser download in every UI job at once on any lockfile bump.test(phone-scroll): make the drag report what it actually did.dragScrollBymoved the scroller withscrollTop +=— which clamps silently — and returned nothing;addPhoneScrollRunwayslept 50 ms instead of waiting for layout. The helper now re-resolves the scroll owner per step and returns the distance travelled,dragScrollUntilHiddenasserts it, and the runway wait is a condition wait. This is a diagnostic and a guard, not a fix — see the correction below.ci: bound the codex auto-resolve jobs.codex-autofix-review-comments.ymlwas the only workflow with notimeout-minuteson either job, so both inherited GitHub's 360-minute default for work that reads PR metadata and posts one comment.chore(gates): pin the documented gate count to the real chain.check:gate-manifestalready derives the real number, so it now asserts the documented ones against it, failing closed if the anchor phrasing disappears. It earned its keep immediately: mergingmainaddedcheck:gitleaks-pinnedandcheck:pr-mergeability, and the guard went red on the stale docs within this PR.docs(issues): capture five deferred findings (#131–#135) that should not be changed blind.RAG impact: no retrieval behaviour change — no file under
src/lib/rag/**, clinical-search, retrieval-selection, ranking-config, answer-ranking, the eval harness, or the golden fixture is touched.classifyPullRequestFilesreturnsragRanking: false,clinicalRisk: false.Correction: the phone-scroll change is not a fix
An earlier revision of this PR claimed it root-caused and fixed the
ui-phone-scrollred — thatdragScrollByclamped, so the drag under-delivered and the chrome legitimately stayed visible. That claim was wrong and is withdrawn.maincarries#127for the same test and the same reduced-motion hide cycle, with trace evidence this PR did not have: the drag delivered in full (documentElement.scrollTop= 1272, exactly the 552 + 720 requested) with ~1300 px of runway spare, and a 10 s non-flip is a latched state, not a race. Under-delivery was not the cause, and this helper would have passed its own checks and failed on the same assertion.What the change does buy is real but narrower: "the drag was short" can no longer be an unfalsifiable explanation for a CI log, so
#127is down toscrollHiddenfalse vssharedChromePinnedlatched. Separating those two needs the pin state exposed in the DOM — today only the compositedata-scroll-hidden(scrollHidden && !sharedChromePinned) is observable, so both look identical. That is now recorded on#127, along with the prime suspect visible in source:composerFocusPinsChromecarries a still-the-active-owner guard andheaderFocusPinsChromecarries none, against the comment directly above both requiring one (master-search-header.tsx:397-398).Exposing that attribute touches shared search chrome, so it belongs in its own change under
docs/search-chrome-behaviour.mdwithverify:phone-chrome— not bolted onto a CI PR.Why this PR ran no CI for two hours
It pushed, opened, and was marked ready for review while producing zero
pull_requestruns — noCI, noGitleaks, noSemgrep— with onlypull_request_target(PR policy) firing. The branch had a real conflict against a base 35 commits ahead, so GitHub could not buildrefs/pull/1427/mergeand skipped everypull_requestworkflow with no failing check.That is
#116, andmain's brand-newPR mergeabilitycheck — shipped by #1416 while this PR sat blocked — caught it and named the cause. Mergingorigin/mainresolved it and CI started on the very next push. The episode is recorded indocs/process-hardening.md: read a missing check list as a conflict signal, never as a pass.The measurement
Job-level timings from two full UI-scope PR runs (
30520443076,30519912667), read from the Actions API:Every other job finished by minute 4 and then waited ~12 minutes. Of 62 PR CI runs sampled in one 83-minute window, 25 of the 60 that completed were cancelled (42%), almost all superseded mid-Production-UI — so that one job is also where the churn cost lands.
The shard count is measured, not chosen.
fullyParallel: falsemakes a spec file indivisible, so shard sizes are lumpy. Re-measured against the merged tree (342 required chromium tests):On the 340-test tree just before, N=5 gave
121/106/0/96/17and N=8 gave two empty shards; an empty shard would go red becausetest:e2e:prdeliberately omits--pass-with-no-tests. N=4 buys nothing over N=3 because one 121-test spec group bounds both.These numbers predate
ui-critical-fast, whichmainadded while this branch was open: the UI critical path is now that 15-test fail-fast job plus the slowest shard, not the full 13.5-minute suite. Both need re-measuring from this PR's own run — it is the first execution of the sharded shape anywhere.Verification
npm run verify:cheapon the merged tree —Test Files 434 passed (434),Tests 4563 passed | 4 skipped (4567), exit 0npm run verify:pr-local(pre-merge) —Test Files 431 passed (431),Tests 4493 passed | 4 skipped (4497), exit 0npx prettier --check .—All matched files use Prettier code style!check:gate-manifest,check:github-actions,check:ci-scope,check:pr-mergeability,check:gitleaks-pinned,check:outstanding-issues— all pass--shard=i/N --listUI verification, partial and stated plainly: browser tests became runnable mid-session because
mainaddedPLAYWRIGHT_CHROMIUM_EXECUTABLE_PATH(retiring#121for this container).tests/ui-phone-scroll.spec.tsran once on the merged tree —56 passed (5.3m), exit 0. I did not complete the three-run flake protocol: that protocol validates a flake fix, and once#127's evidence established this is not one, a green local run cannot say anything about a load-dependent CI failure (#127records56 passed (4.3m)locally too). The single run establishes only that this change does not break the spec. This PR's ownProduction UIshards are the real test.Not run, deliberately:
npm run verify:release,eval:*, andcheck:supabase-projectare provider-backed and were not authorised.Risk and rollout
git revertany single commit — they are independent. Reverting the sharding commit alone restores the single unshardedProduction UIjob and the previous cache keys.Notes
Deferred rather than changed blind, as
#131–#135: theui_changedscope over-firing onsrc/app/api/**, the Playwright build's cold Next cache, the advisory UI lane's cost against zero@quarantinetests, CI Triage sitting inert pending a repo variable, and four deadchangesoutputs.Conflict resolution kept both sides deliberately:
main'sui-critical-fastgating and this branch's shard matrix both survive;playwright.visual.config.tstakesmainentirely (it already serialises, making this branch's version redundant). The captured rows were renumbered twice —mainallocated 125–127 and then 128–130 while this branch was open — and the second merge trippedmerge=unioninto duplicating the whole table, caught bycheck:outstanding-issueswith 66 duplicate-id errors and rebuilt frommain's canonical file. That is#112behaving exactly as documented: union merge preserves concurrent appends but cannot allocate unique ids, so the structural gate is what makes it safe.