fix(mode-nav): size slots to their content so labels stop being clipped - #1405
Conversation
Ledger #113. `ModeNav` promises a slot shows its real word or folds into More, and it broke that promise at every phone width. The two lower bands used `grid-auto-columns: 1fr`, so the WIDEST slot set what every slot got, and the label's `truncate` hid the shortfall — nothing overflowed, nothing failed, the word was simply gone. Measured in Chromium before changing anything: 320px showed 15 of Compare's 56px, 390px showed 39, and 430px was WORSE at three of four labels clipped, because crossing a band adds a slot to the same space. The bar got less legible as the screen got wider. Only the widest band was clean — the one that already sized slots to their content. So that branch is now the only branch. `display: flex` at every band means no track can be narrower than its own label, which removes the failure mode rather than retuning it, and deletes a layout mode rather than adding one. It is also what the author already chose for wide screens: "slots size to their content and sit left, so the bar reads as a continuation of the header". Thresholds are re-derived from measured intrinsic widths (Search 92.4, Compare 144.3, Recommend 125.6, Pathways 108.5, More ~77, plus the bar's 8px padding) rather than the previous budget, which was ~80px short — about the size of the count badge nobody counted. 16rem -> 21rem for three slots, 26rem -> 31rem for four, 34rem -> 40rem for five. A fixed `rem` threshold still cannot prove fit for an arbitrary item list, and `ModeNav` is shared. `tests/ui-mode-nav-density.spec.ts` holds that gap: at both band boundaries and one pixel either side, every rendered label must be fully visible and the bar must not overflow. A mode with longer labels now fails CI instead of shipping clipped words. Registered in BOTH Playwright allowlists — `testMatch` and `productionSpecPattern` — because a spec missing from either silently never runs. The spec scopes to the nav inside the collapse host rather than the bare test id. That is not incidental: streaming can resolve the id to two elements (ledger #93), and the density decision is a container query that only means anything once the portal has placed the bar in the container it lives in. Verified against the pre-fix CSS with the new spec in place — 6 of 7 red, reporting e.g. `336px: "Compare" needs 56px, has 21px`. verify:cheap exit 0 — Test Files 432 passed (432), Tests 4450 passed | 4 skipped. Chromium, all Therapy/ModeNav journeys — 18 passed (56.7s). Text zoom now behaves as the `rem` mechanism claims: a 390px phone at a 32px root is 12.19rem and falls back to the collapsed control instead of clipping. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FvU8z73P6TXUXoYBqN5K1P
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 39 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 (4)
Comment |
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
CI triageCI failed on this PR. Automated classification of the 2 failed job(s):
Compared with main CI run #6654 (success). Classification is evidence routing, not permission to ignore a failure. Exact quarantined Playwright identities remain governed by the flake ledger. |
CI caught what my local measurement could not: at 496px the boundary case failed by exactly one pixel — `"Compare" needs 59px, has 58px`. Locally the same label measured 56px. The variable face rasterises wider on the runner, so thresholds calibrated to one machine's font metrics are calibrated to nothing. Raised to 22 / 33 / 42rem, roughly 8% over the measured intrinsic widths rather than the pixel or two a single box suggests, so the bands clear the whole spread of environments instead of the one I happened to measure in. This is the same failure the boundary tests exist to catch, so they did their job on the first CI run rather than letting a one-pixel clip ship — the tests at 351/352/353 and 527/528/529 are deliberately the least forgiving place to assert, and that is the point of putting them there. Chromium, tests/ui-mode-nav-density.spec.ts — 7 passed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FvU8z73P6TXUXoYBqN5K1P
CI status on
|
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
The #113 archive row cited `c2edda18` as PR #1405's merge commit. That was the branch tip, which the squash merge discarded, so the claim was already unverifiable when it was written: git merge-base --is-ancestor c2edda1 origin/main -> fails git merge-base --is-ancestor 020c126 origin/main -> ok 020c126 fix(mode-nav): size slots to their content ... (#1405) In a ledger whose whole purpose is durable, auditable resolution history, a resolution nobody can check is worse than no resolution note. The trap is that this repo mixes merge strategies: #1407 and #1410 landed as merge commits, so their branch SHAs stay reachable; #1405 was squashed and its did not. Copying the PR head works three times in four, which is exactly the kind of rule that survives review until it doesn't. Verified after the edit: `c2edda18` no longer appears anywhere in the file, the replacement is an ancestor of main, and `check:outstanding-issues` still passes with `117 rows (55 open, 62 archived), unique ids, next-id=118 above the highest`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FvU8z73P6TXUXoYBqN5K1P
Summary
Fixes ledger
#113.ModeNavpromises that a slot shows its real word or folds intoMore— it never abbreviates. It broke that promise at every phone width.The two lower bands used
grid-auto-columns: 1fr, so the widest slot set what every slot got, and the label'struncatehid the shortfall. Nothing overflowed and nothing failed; the word was simply gone. Measured in Chromium before changing anything:The bar got less legible as the screen got wider, because crossing a band adds a slot to the same space. Only the widest band was clean — the one that already sized slots to their content.
So that branch is now the only branch.
display: flexat every band means no track can be narrower than its own label. That removes the failure mode rather than retuning it, and deletes a layout mode rather than adding one. It is also what the author already chose for wide screens: "slots size to their content and sit left, so the bar reads as a continuation of the header."Thresholds:
16 / 26 / 34rem→22 / 33 / 42rem. The old budget claimed four labels need 394px; measured intrinsic widths are Search 92.4 + Compare 144.3 + Recommend 125.6 + Pathways 108.5 = 471px plus the bar's 8px padding — short by about the size of the count badge nobody counted.The new numbers carry ~8% over that, and that margin is the second thing CI taught me, not padding for its own sake. My first attempt used 21/31rem — measured-plus-a-pixel — and passed locally, then failed CI at the 496px boundary by exactly one pixel:
"Compare" needs 59px, has 58px; "Recommend" needs 81px, has 80px. The same label measured 56px on my box. The variable face rasterises wider on the runner, so a threshold calibrated to one machine's font metrics is calibrated to nothing. 22/33/42rem clears the whole spread rather than one sample of it, and the CSS comment says so.The remaining gap is held by a gate, not by hope. A fixed
remthreshold still cannot prove fit for an arbitrary item list, andModeNavis shared — Therapy is only its first consumer.tests/ui-mode-nav-density.spec.tsasserts that at both band boundaries and one pixel either side, every rendered label is fully visible and the bar does not overflow. A mode whose labels are longer now fails CI instead of shipping clipped words — which is precisely what happened to me on the first run, one commit into this PR.Three details in that spec are deliberate:
testMatchandproductionSpecPattern). A spec missing from either silently never runs — I hit exactly that and gotNo tests found.mode-navto two elements (ledger#093, which I hit), and the density decision is a container query that only means anything once the portal has placed the bar in the container it will live in.RAG impact: no retrieval behaviour change — CSS, one Playwright spec, the Playwright config allowlists, and the mode-nav contract test; nothing under
src/lib/rag/**, clinical-search, ranking-config, the eval harness or the golden fixture is touched.Verification
336px: "Compare" needs 56px, has 21px. The gate is real, not decorative.npm run verify:cheap— exit 0,Test Files 432 passed (432),Tests 4450 passed | 4 skipped (4454)-g "Therapy|mode nav|ModeNav|therapy") —18 passed (56.7s), includingui-therapy-nav-scroll, the fourui-phone-scrollTherapy cases andui-route-coveragetests/ui-mode-nav-density.spec.tsafter the correction —7 passedremmechanism claims — a 390px phone at a 32px root is 12.19rem and falls back to the collapsed control rather than clipping. This is the case my original#113measurement could not test, because that probe injected the root font after navigation and it was silently discarded.npx tsc --noEmit, ESLint andformat:checkcleanci/circleci: verifyis red and it is not this PR. It is red repo-wide: #1403 carries the same failure on0fe40896and was merged intomainanyway (c57318f4). Its config runsformat:check,lint,typecheckandnpm run test, all of which pass on this branch and on a pristinemaincheckout here. I have not diagnosed the job itself — its logs are behind a JS app I cannot read — so I am flagging it rather than claiming it is fine.Risk and rollout
Clinical Governance Preflight
No ingestion, answer generation, search/ranking, document access, privacy or clinical output path is touched.
Clinical KB Database(sjrfecxgysukkwxsowpy)Notes
Noticed while working here, not fixed:
ui-therapy-nav-scroll.spec.tsrefers to amode-nav-bar-anchoring.spec.tsthat does not exist — a stale reference from #1390. Left alone to keep this diff to#113.🤖 Generated with Claude Code
https://claude.ai/code/session_01FvU8z73P6TXUXoYBqN5K1P