fix(ui): remove dead scroll on pages that fit the window - #2419
Conversation
Pages whose content already ended still carried a scroll range, so a scrollbar appeared on a page with nothing below the fold and a wheel notch jolted the page into its bottom stop. Measured in Chromium across 39 routes at five viewports: 8px on every standalone page, 38-46px on all 15 shared mode homes, 57px on routes carrying the header nav row, and up to 273px on a tall window. The cause was the same everywhere: page-fill floors written as `calc(100dvh - <chrome estimate>)`. No estimate could be right. `--shell-header-h` (4rem) covers the header's inner bar plus `pb-2` but not the bar's own `pt-[max(0.5rem,var(--safe-area-top))]`; nothing knew about the `header-collapse-addon` nav row on topic routes; nothing knew about `#main-content`'s own `sm:pb-8`; and the dashboard's `11rem` guess had to cover the header block, the wrapper padding, the desktop composer slot and the space-y gap in one number. Surfaces now grow into the box above them instead, which is exact by construction and cannot drift again: - shell `#main-content` grows into `.phone-viewport-frame` (`sm:grow`) - `mobile-composer-reserve-pad` becomes the fill box at sm+ - page shells grow into that pad (`sm:grow`) - the dashboard content wrapper is the fill box for the mode-home canvas, which grows into it (`sm:grow sm:shrink-0`) Phone geometry is untouched: below `sm` the document owns scrolling and there is no bounded box to fill. All 65 phone measurements are byte-identical before and after. Verified in Chromium against 39 routes x 5 viewports: 82 measurements improved, 113 unchanged, 0 increased, and no route gained horizontal overflow. Every page that fits now reports a scroll range of exactly 0; every page with real content reports the same range as before. Adds the "pages that fit the window have no scroll range" guard to tests/ui-chrome-scroll.spec.ts, records the rule as invariant 24 in docs/search-chrome-behaviour.md, and updates the two contract tests that pinned the old floors. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Bbm7tQoyg6etyf7NqoA65d
Warning Review limit reachedNext included review available in 54 minutes. View limit detailsLimit details: You’ve used the included review currently available. Your 100 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (15)
Comment |
This pull request has been ignored for the connected project Preview Branches by Supabase. |
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_c9e42d1a-952c-4095-ba98-cf546141ce2f) |
Uh oh!
There was an error while loading. Please reload this page.
The scroll work landed on main as 603da96; this branch still carries the three phone fixes that were pushed after that merge. Merging main in makes those the only diff a new pull request will show.
Apply #2419's sm:grow page-shell change on the moved searchPageShell recipes so the PR is no longer CONFLICTING. Co-authored-by: Cursor <cursoragent@cursor.com>
#2434) * issues: queue three follow-ups left open by PR #2422 Three append-only intake requests, plus the regenerated snapshot that check:outstanding-issues requires once the inbox is non-empty. - P2 issue: Lighthouse desktop-root LCP read 961/925/900/852 on the #2422 branch against 748/705 on main, same 786ms baseline. It failed the gate once and cost a re-run. Not root-caused; recorded with the evidence on both sides, the one mechanism still testable (whether extracting answer-cancelled-notice.tsx added a chunk to /), the exact command to test it, and the caveat that the gate's 3-sample majority is taken on a single runner and so cannot see runner-level slowness. - P3 issue: /calculators/search keeps a 2px residual scroll range at 1280x1200, deliberately out of scope for #2419 because it is real content rather than the chrome-estimate floor that PR removed. - P3 task: mode-home-page-skeleton.tsx:32,60 still subtract --shell-header-h from 100dvh - the last page-fill estimates in src/ and the pattern invariant 24 retired. Left because the Suspense parent chain has to carry a definite height first. Requests are immutable and merge independently; reconciliation is a separate serial branch and is deliberately not done here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Bbm7tQoyg6etyf7NqoA65d * ledger: record the immutable review for the #2422 branch Owed from the handoff and deliberately not pushed onto the branch while it was green and armed for auto-merge. Records the scope, the merge outcome verified by content rather than by the green tick (git diff c917e0728b1fef is empty, so the squash dropped nothing), the CI evidence on the merged head, and two honest gaps: the Chromium gate could not run in this container under the #255 revision pin, and the desktop-root LCP reading is carried as its own open item rather than reported as cleared. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Bbm7tQoyg6etyf7NqoA65d * snapshot: regenerate repo-awareness after the new review record `Static PR checks` failed on this branch: check:repo-awareness-snapshot reported "review_state differs from the repository". The repo-awareness snapshot mirrors the branch-review records, so appending one leaves it behind; I regenerated the outstanding-issues snapshot in the first commit and missed this second one. Regenerated with the command the check itself names. The diff is only the captured revision, the mirrored copy of the new record, and counts 2644 -> 2645 records / 1612 -> 1613 refs. Nothing unrelated moved. Verified: check:repo-awareness-snapshot now reports "in step ... (195 pages, 481 documents, 2645 reviews)"; the failure reproduced locally first. Also re-ran the rest of the failing job's docs and ledger gates plus the whole-tree format check — docs:check-index, docs:check-inventory, docs:check-scripts, docs:check-links, check:outstanding-issues, check:ledger-write-discipline, check:branch-review-ledger, format:check — all pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Bbm7tQoyg6etyf7NqoA65d --------- Co-authored-by: Claude <noreply@anthropic.com>
Summary
calc(100dvh - <chrome estimate>), and no estimate could be right.--shell-header-h(4rem) covers the header's innermin-h-14bar plus itspb-2but not the bar's ownpt-[max(0.5rem,var(--safe-area-top))]; nothing knew about theheader-collapse-addonnav row that topic routes mount; nothing knew about#main-content's ownsm:pb-8; and the dashboard's11remguess had to cover the header block, the wrapper padding, the desktop composer slot and thespace-ygap in one hard-coded number.#main-contentgrows into.phone-viewport-frame(sm:grow);mobile-composer-reserve-padbecomes the fill box atsm+ (sm:flex sm:min-h-full sm:flex-col); page shells grow into that pad (sm:grow); and the dashboard content wrapper is the fill box for the mode-home canvas, which grows into it (sm:grow sm:shrink-0).smthe document owns scrolling and there is no bounded box to fill, so the phone floors stay as they were. All 65 phone measurements are byte-identical before and after.tests/ui-chrome-scroll.spec.ts— the assertion whose absence let this sit unnoticed.docs/search-chrome-behaviour.mdand corrects the--shell-header-hcomment inglobals.css, which had claimed the token existed so the header height and the page-fill floors "cannot silently drift apart".ui-overlay-css-contract,mobile-interaction-regressions) to pin the new fill contract, including negative assertions so the estimate cannot come back.Verification
npm run verify:pr-localRun locally instead, and reported exactly as run:
npm run typecheck— clean.npm run lint— clean,--max-warnings 0.npm run test(full offline unit suite) — 10,814 passed, 4 skipped, 3 failed. The three failures (clinical-hazard-controls,privacy-readiness-contract,rag-plan-package-parity) were confirmed pre-existing: they fail identically with this branch'ssrc/,tests/anddocs/changes stashed.rag-plan-package-parityfails only because the manifest's reconciled base commit is not present in this shallow clone.npm run verify:phone-chrome— contracts stage passed (9 files, 135 tests passed, includingheader-scroll-hide-contract,use-hide-on-scroll,mobile-composer-reserveandui-overlay-css-contract). Its browser stages could not run here, see below.0; every page with real content reports the same range as before. A separate 1440×2000 pass confirmed the previously hidden cases (/favourites40→0,/dictionary/topics273→0,/calculators/search224→0,/specifiers/map273→0,/differentials/compare273→0). Screenshots of a mode home, a standalone info page, a nav-row route, a results view and a phone home were inspected: nothing shifted, centring still reads correctly, and the background still reaches the bottom edge./,/?mode=documents,/medicationsand/factsheets/topics— all reportmaxScrollTop = 0on both the settled and the stability read.UI verification not run:
npm run verify:uiand theverify:phone-chromebrowser stages could not execute in this container. The lock pins Playwright chromium-1234; the image ships only chromium-1194, and both remedies are blocked by the environment's proxy —scripts/setup-claude-cloud.sh browsersfails on 403 from the apt PPAs, andnpx playwright install chromiuminto the managed cache fails with a download error. Per the#255guard inscripts/check-playwright-browser-revision.mjs, pointingPLAYWRIGHT_CHROMIUM_EXECUTABLE_PATHat the mismatched shell is forbidden, so browser proof is delegated to CI Production UI. The browser measurements above were taken with the container's chromium-1194 against the dev server, not the pinned revision against a production build, and are offered as evidence of the geometry change, not as the Chromium gate.npm run verify:releasebefore release or handoff confidence claimsRisk and rollout
min-heightfloor to flex growth, and two containers become flex columns atsm+ (mobile-composer-reserve-pad, the dashboard content wrapper). The realistic failure mode is a page that no longer stretches to the bottom — visually neutral where it happens, because the shell and every one of these surfaces paint the samevar(--background), but worth a look on wide layouts. Percentage resolution is one level deep in both chains (min-h-fullagainst a parent with a definite height), which is why the nested favourites grid was moved tosm:min-h-fullagainst its now-grown hub rather than left on a viewport calc.Notes
/calculators/searchat 1280×1200 keeps a 2px range. That is the catalogue grid's own content being 2px taller than its space, not a chrome estimate; no floor binds there any more.mode-home-page-skeleton.tsxstill carriessm:min-h-[calc(100dvh-var(--shell-header-h))]. It is a routeloading.tsxplaceholder whose parent chain is a Suspense boundary rather than the reserve pad, so growth would not resolve there. It is transient and cannot produce the persistent scrollbar this PR fixes.getServerSnapshot should be cachedwarning on/favouritesand a 404 for one resource on/. Both reproduce identically with this branch's changes stashed.--shell-header-htoken keeps its value and its other consumers (chrome offsets,--phone-overlay-chrome-h, which adds the top pad back explicitly). Only its use as a page-fill floor is retired.Generated by Claude Code
Note
Medium Risk
Touches shared shell flex geometry (
#main-content, reserve pad, dashboard wrapper); failure mode is pages not stretching to the bottom on wide layouts, though backgrounds should mask it.Overview
Fixes phantom scrollbars on desktop/tablet (
sm+) when page content already fits the viewport. Floors built ascalc(100dvh - <chrome estimate>)systematically overshot because no static number could account for the header’s top safe-area pad, optional header-collapse-addon nav row, or#main-content’s bottom padding — leaving roughly 8–273px of empty scroll range.Layout contract change: at
sm+,#main-contentand themobile-composer-reserve-padbecome a definite flex fill chain (sm:grow,sm:flex sm:min-h-full sm:flex-col); page shells, dashboard content wrapper, mode-home canvas, and shared primitives (searchPageShell,InformationPageShell, therapy/differentials/favourites mains) switch from viewportmin-hcalcs tosm:grow(favourites split rail usessm:min-h-fullagainst the grown hub). Phone viewport calcs are unchanged where the document still owns scrolling.Documents invariant 24, clarifies
--shell-header-his for offsets—not page fill—and adds Playwright “pages that fit the window have no scroll range” plus contract-test pins so the old estimate pattern cannot return.Reviewed by Cursor Bugbot for commit c56e5f5. Configure here.