fix(chrome): hide and return the universal header on scroll at every breakpoint#1222
Merged
Conversation
Renames the phone-gated active helper, adds a resetKey so hosts can rebase the reporter on a geometry switch, and adds a document-scroll reporter for shells whose page scrolls the document above the phone breakpoint. readChromeCollapseBudget now only counts the header strip while the collapse wrapper is actually a grid - the sticky/translate mode releases no layout.
Adds hideOnScroll.wide to the collapse strategy. Hosts whose scrollport is internal at every width collapse the header row at every width; hosts that hand scrolling back to the document above the phone breakpoint pin the collapse wrapper to the viewport top and translate it away. The wrapper - not the header, which had no sticky travel inside its header-height parents - now owns that pinning.
…esktop GlobalSearchShell now reports document scroll as well as #main-content scroll and opts the header into every breakpoint, unwrapping the chrome-visibility div above phones so the sticky wrapper has travel. ClinicalDashboard hides in every mode rather than answer mode only. The phone bottom search dock is untouched - MasterSearchHeader still gates it on the phone layout - so tablet and desktop composers stay put.
Adds the Chromium sweep the reported defect needed - the reveal must clear while the reader is still hundreds of pixels down, with the bar back on screen - plus a static contract over the wiring that proof depends on: scroll source per width, collapse-versus-sticky per host, and the display:contents ancestor that gives the sticky wrapper travel. Also drops the now-redundant sticky rule from header#search wherever its row collapses, and documents the model in search-chrome-behaviour.md.
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
Contributor
|
Warning Review limit reached
Next review available in: 58 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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 Plus Run ID: 📒 Files selected for processing (9)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…-4fd7' into cursor/global-header-scroll-hide-4fd7
BigSimmo
added
the
skip-branch-sync
Cursor
BigSimmo
marked this pull request as ready for review
July 25, 2026 16:37
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
BigSimmo
enabled auto-merge (squash)
July 25, 2026 16:44
2 tasks
BigSimmo
added a commit
that referenced
this pull request
Jul 25, 2026
BigSimmo
added a commit
that referenced
this pull request
Jul 25, 2026
BigSimmo
added a commit
that referenced
this pull request
Jul 25, 2026
* fix(ui): synchronize mobile answer chrome Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com> * test(ui): require answer scroll hide without blur Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com> * fix(ui): release stale answer composer focus Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com> * fix(ui): bind composer autofocus retries Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com> * chore(ui): stay within dashboard line budget Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com> * fix(ui): hide chrome on compact answer results Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com> * fix(ui): hold chrome through fractional scroll clamp Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com> * docs(review): record mobile chrome Bugbot pass Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com> * test(ui): distinguish range clamps from upward intent Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com> * fix(ui): distinguish layout clamps from upward scroll Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com> * refactor(ui): compact bound composer focus retry Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com> * docs(ui): codify geometry-based clamp handling Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com> * style: format use-hide-on-scroll.ts to satisfy prettier check Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com> * docs(review): record current PR Bugbot pass Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com> * docs(review): record document scroll behavior findings Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com> * fix(ui): clear dock focus latch and refuse near-bottom reserve hides Prevent composerChromeFocused from pinning both chrome edges after the phone dock tears down without blur, and require reserve-only hide decisions to keep the current offset inside the post-collapse range. * docs(review): record PR #1192 Bugbot P1/P2 fixes * style: format search-chrome-behaviour.md * docs(review): record PR #1192 review after Bugbot fixes and main sync * docs(review): record final PR #1192 tip after sync * chore(ui): stay within ClinicalDashboard line budget * chore(ui): compress 3-line comment to restore line budget Merging origin/main expanded a JSX comment from 2 to 3 lines while restructuring heroComposerBreakpoint/heroOwnsPhoneComposer declarations, putting ClinicalDashboard.tsx at 4141 vs the 4140 no-growth budget. Compact the comment back to 2 lines; zero behaviour change. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com> * docs(ledger): record PR #1192 pr-ci-fix (maintainability budget) Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com> * chore(ui): extract composer focus helper for line budget * fix(ui): gate focus pins at sharedChromePinned to fix react-hooks/set-state-in-effect Two useEffect blocks were calling setState synchronously in the effect body (react-hooks/set-state-in-effect), triggering the React Compiler lint rule. Fix: move heroComposerOwnsPhones, phoneBottomSearchDockActive, and hideOnScrollEnabled before sharedChromePinned; gate headerChromeFocused and composerChromeFocused by their active surfaces directly in sharedChromePinned; remove the two effects entirely. This is the idiomatic React pattern — derive clamped values at the consumer rather than resetting state in effects — and eliminates the cascading render. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com> * docs(ledger): record PR #1192 pr-ci-fix (react-hooks/set-state-in-effect) Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com> * fix(ui): clear chrome focus latch without setState-in-effect Derive focus pins from active dock/hide-on-scroll ownership and clear stale latch state via queueMicrotask. Suppress focus=1 autofocus after any mode search submit (and on run=1 bootstrap) so result views can hide phone chrome. * docs(review): record PR #1192 recommended-fix pass * test(ui): pin focus=1 submit and chrome latch contracts * fix(ui): defer stale chrome focus cleanup Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com> * chore(ci): re-trigger checks after main sync Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com> * chore: retrigger PR checks Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com> * chore: retrigger PR checks after branch sync Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com> * fix: apply CodeRabbit auto-fixes Fixed 2 file(s) based on 2 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai> * Revert "fix: apply CodeRabbit auto-fixes" This reverts commit 30dbdce. * test: make short-runway scroll deterministic Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com> * docs: deduplicate merged review ledger row Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com> * test: assert near-bottom hide refusal and floor the short-runway hide offset Production UI failed because the short-runway smoke test raced PageDown's smooth scroll against the near-bottom reserve guard; the hide only fired when an animation frame happened to sample the 32-40px intent window. Drive the designed paths deterministically instead: a jump onto the bottom edge must keep chrome visible (guard coverage in the browser), and a floored post-collapse-offset scroll hides both edges. * test: gesture upward in separated steps after the bottom clamp On a starved CI renderer the single upward scrollTop write coalesced into the trailing bottom-clamp evaluation, and the shrinking-range hold rebased it away as geometry feedback, so the reveal never fired. A real drag always emits follow-up events; model that with two separated upward steps that each yield frames. * fix(ui): release clamp detection when user scroll exceeds reveal threshold RAF debouncing coalesces a layout-clamp scroll event with any immediately following user scroll into one RAF evaluation. The hook reads the final offset (newMaxOffset - userScroll) but lastMaxOffsetRef still holds the pre-collapse maximum so the clamp detection incorrectly fires: lastOffset (V) > newMaxOffset (V-72) ← old position above new max offset (V-96) < lastOffset (V) ← apparent upward movement maxOffset (V-72) < previousMaxOffset (V) ← range shrank All three conditions are true even though the upward movement includes a genuine 24px user scroll on top of the geometry-driven clamp. Guard the clamp detection with offset >= maxOffset - revealIntentDistance: only treat the movement as pure geometry feedback when the net position lands within revealIntentDistance (12px) of the new bottom edge. If the combined offset is already more than 12px below the new max the user has supplied enough upward intent to reveal regardless of the clamp. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com> * chore: record PR #1192 scroll clamp detection fix in branch-review-ledger Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com> * chore: retrigger PR checks * docs(ledger): drop duplicate records created by the main merge Merging current main into this branch let the union driver append two records that both sides already carried, which fails check:branch-review-ledger. Both copies of each record were byte identical, so the later copy goes and the unique record set is unchanged at 938. * docs: deduplicate ledger after main merge Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com> * docs(ledger): record the #1222 reconciliation review of PR #1192 * docs(ledger): drop the duplicate record from the latest main merge * fix: apply CodeRabbit auto-fixes Fixed 1 file(s) based on 1 unresolved review comment. Co-authored-by: CodeRabbit <noreply@coderabbit.ai> --------- Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
This was referenced Jul 25, 2026
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
1 participant
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
max-sm, and above that the header either never hid (ClinicalDashboard) or scrolled off with the page and only came back once the reader reached the very top (GlobalSearchShell— the reported defect).usesPhoneSearchLayout, and the new browser sweep asserts no composer scroll-hides at tablet or desktop.useScrollHideReporterper host, so they can never disagree about scroll direction.Two distinct root causes, one per scroll-ownership model:
GlobalSearchShell#main-contenton phones, the document above#main-content's ReactonScrollnever fires above phones, so nothing reported scroll at all; thestickyrule onheader#searchhad zero travel inside its two header-height parentsuseDocumentScrollHideReporterfeeds window scroll into the same reporter;wide: "sticky"moves the pin to the collapse wrapper, whose ancestor becomesdisplay: contentsso it can resolve against the viewportClinicalDashboard<main>at every widthmax-sm-only, so the header simply never hid above phoneswide: "collapse"releases the header row at every width, where the released strip goes straight to the contentSupporting changes:
readChromeCollapseBudgetcharges the header's height against the scroll runway only while the wrapper is actually a grid (chrome that sticks and translates releases no layout, so charging it would suppress valid hides);useScrollHideReportergained aresetKeysoClinicalDashboardrebases whensearchModeswaps<main>'s header reserve;header#searchdrops its now-redundant sticky rule wherever its row collapses.Verification
Re-run after each
mainsync; the results below are from the current head.npm run verify:cheap— pass except one pre-existing local failure intests/pdf-extractor.test.ts(Python OCR prerequisites missing on this workstation). Reproduced identically at the base commit1aa64e94in a scratch worktree, and hostedUnit coverageis green, so it is environmental rather than a regression.npm run verify:ui— 284/284 Chromium tests, including the existingui-phone-scrollsweep (no phone regression) and the 12 newui-chrome-scrolltests.npm run typecheckplus both scroll specs (30/30) aftermainbrought the fix: clean rebuild of PR #1188 maintainability remediation #1213ClinicalDashboardrefactor and the fix: audit SQL remediation (worker URL + grants) after main sync #1197 SQL sync;check:migration-role,check:function-grants, andcheck:branch-review-ledgeralso pass.1aa64e94and the new tests kept, all 12 Playwright tests and 8 of the 10 static contract assertions fail; with the fix restored, all pass.npx prettier --checkon every changed file.npm run verify:pr-local— not run as a single command;verify:cheapcovered lint/typecheck/unit andverify:uibuilt and served a production bundle.Provider-backed gates (
eval:*,check:supabase-project,verify:release) were not run and are not applicable: this change touches presentation chrome only, with no retrieval, ranking, answer, ingestion, or source-governance surface in the diff.New coverage
tests/ui-chrome-scroll.spec.ts— tablet (834x1112) and desktop (1440x900) across three surfaces (/tools,/formulation/worry,/?mode=prescribing&q=a&run=1). The load-bearing assertion is the reveal mid-page: hidden must clear while the reader is still hundreds of pixels down, with the bar measurably back on screen. Asserting onlydata-scroll-hiddenwould have passed against the old sticky header, which flipped the attribute while sitting far off the viewport top.tests/header-scroll-hide-contract.test.ts— static contract over the wiring that browser proof depends on: scroll source per width, collapse-versus-sticky per host, thedisplay: contentsancestor, and the phone-only gate on the dock.docs/search-chrome-behaviour.mdgains a "Scroll hide/reveal" section with the ownership table and the five rules that keep it working.Risk and rollout
/tools,/formulation/worry,/differentials,/services/13yarn,/documents/:id, and/?mode=*at tablet and desktop — all hide and return correctly. Short pages (/favourites,/documents/searchat these widths) have no runway and correctly never hide.Notes
skip-branch-syncwas applied because repeatedpr-branch-syncbot merges left CIaction_requiredon every new head, so required checks could never finish (same pattern recorded for fix(theme): salvage dark-mode audit from PR #1190 without clinical regressions #1214 indocs/branch-review-ledger.md). Syncing tomainis being done manually instead./safety-planrenders noheader#searchat all; that is pre-existing and out of scope here.