fix(ui): stop phone scroll locking to the bottom on mode homes (collapse-budget gate)#993
Conversation
…pse-budget gate) Since #964 docked the compact composer on phone mode homes, one hide-on-scroll event releases ~180-260px of scroll geometry (72-130px header grid collapse + ~110px reserve-pad shrink, both animated). Mode-home content is short, so the release exceeds the remaining scroll runway: scrollTop clamps straight onto the new bottom edge, a 12px upward drag re-reveals and snaps the geometry back under the finger, and the hide/reveal loop reads as "scroll locks to the bottom" with a blank band below the content (reported on /formulation, iOS installed PWA; reproduced and quantified in Chromium: runway 266->84 on hide, 2 chrome flips in a single gesture). Fix: a collapse-budget gate in computeScrollHideUpdate. Hosts report how much layout the chrome would release if it hid right now (readChromeCollapseBudget: collapsible header strip + every dock-clearance pad above the shared 0.75rem hidden size - shell reserve pad, DocumentViewer content pad, scroller-padding fallback), and hiding is refused unless the remaining runway can absorb the release plus the reveal-intent distance and slack. Short pages keep their chrome and scroll plainly; long pages hide mid-scroll exactly as before but can no longer start a hide at the bottom edge. The existing one-sided bottom-clamp guard is untouched. Consumers that report no budget keep today's behavior. Also fixes a latent DocumentViewer bug the new sweep exposed: its one-shot #main-content lookup could permanently hold a detached node when the shell remounts the scroller, silently killing the viewer's hide-on-scroll. The discovery observer now stays alive for the viewer's lifetime (childList-only, deduped setState). Guardrails: - tests/ui-phone-scroll.spec.ts: 18-test sweep over all 10 standalone mode homes, the 3 dashboard modes, and 4 long routes at phone viewports with reducedMotion "no-preference" (the suite-wide "reduce" default disables the very transitions that caused this bug - that is how #964 shipped green) and simulated PWA safe-area insets. Asserts sole-scrollport, no horizontal overflow, true-bottom reachability, and bottom-edge nudge stability (at most one chrome flip). Verified to fail red against the pre-fix behavior. - tests/use-hide-on-scroll.test.ts: oscillation reproduction harness plus runway-refusal, ample-runway, boundary, and budget-omitted back-compat cases. - ui-smoke: bottom-hide scenario updated to the gate contract (hide with runway remaining, then ride the geometry clamp to the bottom while hidden). Verified: vitest focused suites, ui-phone-scroll + ui-formulation + ui-tools + ui-smoke (chromium), webkit advisory sweep, verify:cheap, verify:ui (264 passed), verify:pr-local. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughPhone scroll hiding now accounts for chrome-collapse geometry, prevents bottom-edge oscillation, keeps the shell scroll container synchronized, and adds unit and Playwright regression coverage. ChangesPhone scroll collapse handling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ClinicalDashboard
participant GlobalSearchShell
participant ChromeBudget
participant ScrollHide
participant MainContent
ClinicalDashboard->>ChromeBudget: measure chrome collapse budget
ChromeBudget-->>ClinicalDashboard: return releasable pixels
ClinicalDashboard->>ScrollHide: report scroll metrics and budget
GlobalSearchShell->>ChromeBudget: measure target or main collapse budget
ChromeBudget-->>GlobalSearchShell: return releasable pixels
GlobalSearchShell->>ScrollHide: report scroll metrics and budget
ScrollHide->>MainContent: apply hide or reveal state
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/components/DocumentViewer.tsx`:
- Around line 1748-1752: Update the sync function in DocumentViewer so it sets
shellScrollContainer to null when `#main-content` is absent, rather than returning
while retaining the detached element; preserve the existing assignment when the
node is present so useHideOnScroll falls back to window during remounts.
In `@tests/ui-phone-scroll.spec.ts`:
- Around line 84-97: Make the phone-scroll test fail loudly when the universal
header selector is missing instead of treating it as absent. Update readGeometry
and installFlipCounter to require the [data-testid="universal-header-collapse"]
element, raising a clear assertion/error before returning fallback geometry or
skipping observer installation, so readFlipCount and headerHidden assertions
remain active.
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0d1bad54-ca30-49c3-b780-0ec09047c6d8
📒 Files selected for processing (11)
docs/branch-review-ledger.mdplaywright.config.tssrc/components/ClinicalDashboard.tsxsrc/components/DocumentViewer.tsxsrc/components/clinical-dashboard/global-search-shell.tsxsrc/components/clinical-dashboard/mobile-composer-reserve.tssrc/components/clinical-dashboard/use-hide-on-scroll.tstests/mobile-composer-reserve.test.tstests/ui-phone-scroll.spec.tstests/ui-smoke.spec.tstests/use-hide-on-scroll.test.ts
…oudly Addresses two CodeRabbit findings on #993: - DocumentViewer kept a detached #main-content node across a two-step shell remount, leaving useHideOnScroll subscribed to a scroller that never fires. Syncing null lets it fall back to window. - installFlipCounter returned early when the header selector missed, pinning the counter at 0 so flip assertions passed vacuously. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
# Conflicts: # docs/branch-review-ledger.md
CI triageCI failed on this PR. Automated classification of the 3 failed job(s):
Compared with main CI run #4054 (success). Classification is evidence routing, not permission to ignore a failure. Exact quarantined Playwright identities remain governed by the flake ledger. |
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Autofix skipped. No unresolved CodeRabbit review comments with fix instructions found. |
The answer home hides its glass header via the overlay strategy (header#search carries data-scroll-hidden; no collapse wrapper exists), so the fail-loudly flip counter must observe whichever chrome element the route renders. Condense the DocumentViewer discovery comments back under the 3166-line no-growth maintainability budget while keeping the remount rationale. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ng CI fixes Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Problem
Since #964 docked the compact composer on phone mode homes, scrolling down on an iPhone (installed PWA) on
/formulation(and every other standalone mode home) "locks to the bottom": scroll is janky, the header/composer thrash, and a large blank band appears below the content.Root cause: one hide-on-scroll event now releases ~180–260px of scroll geometry (72–130px header
grid-template-rowscollapse + ~110px reserve-pad shrink, both animated over 200–240ms). Mode-home content is short, so the release exceeds the remaining scroll runway:scrollTopclamps straight onto the new bottom edge, a 12px upward drag re-reveals and snaps the geometry back under the finger, and the loop reads as "locked to the bottom". Quantified pre-fix in Chromium on/formulation@390×844: runway 266→84 on hide, 2 chrome flips in a single gesture.Why every gate missed it: the Playwright suite runs with
reducedMotion: "reduce", which disables exactly the transitions that produce this failure mode.Fix
computeScrollHideUpdate: hosts report how much layout the chrome would release if it hid right now (readChromeCollapseBudget= collapsible header strip + every dock-clearance pad above the shared 0.75rem hidden size). Hiding is refused unless the remaining runway absorbs the release + reveal-intent distance + slack. Short pages keep their chrome and scroll plainly; long pages hide mid-scroll exactly as before but can never start a hide at the bottom edge. The existing one-sided bottom-clamp guard is untouched; budget-less consumers keep today's behavior.#main-contentlookup could permanently hold a detached node when the shell remounts the scroller, silently killing the viewer's hide-on-scroll. The discovery observer now stays alive (childList-only, deduped setState).Guardrails
tests/ui-phone-scroll.spec.ts(new, registered in both config patterns): 18-test sweep — all 10 standalone mode homes, 3 dashboard modes, 4 long routes — at phone viewports withreducedMotion: "no-preference"and simulated PWA safe-area insets. Asserts sole-scrollport, no horizontal overflow, true-bottom reachability, and bottom-edge nudge stability (≤1 chrome flip). Verified to fail red against the pre-fix behavior.tests/use-hide-on-scroll.test.ts: oscillation repro harness + runway-refusal / ample-runway / boundary / budget-omitted back-compat cases.Verification
verify:cheapgreen (3020 tests)ui-phone-scroll18/18 (chromium) + 18/18 (webkit advisory)ui-formulation+ui-tools+ui-smoke180/180verify:ui264/264;verify:pr-localgreen (format, build, bundle scan, RAG fixtures)🤖 Generated with Claude Code
Summary by CodeRabbit