Fix choppy scroll chrome at page boundaries#704
Conversation
…-stability-20260717 # Conflicts: # docs/branch-review-ledger.md
…-stability-20260717
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
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)
📝 WalkthroughWalkthroughMobile phone scroll-hide behavior now uses scroll-range metrics and sustained directional intent. Composer-specific layout state was removed, bottom spacing remains reserved during hide transitions, and unit/UI tests verify stable geometry during composer hiding and in-flow header collapse. ChangesPhone scroll stability
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ScrollContainer
participant ScrollHideReporter
participant MasterSearchHeader
participant MainContent
ScrollContainer->>ScrollHideReporter: report offset, maxOffset, and source
ScrollHideReporter->>MasterSearchHeader: update hidden state after directional intent
ScrollHideReporter->>MainContent: preserve scroll geometry during layout changes
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/clinical-dashboard/global-search-shell.tsx (1)
450-457: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftDo not mix offsets from different scroll containers in one intent state.
The reporter retains
lastOffsetand direction globally, but this handler forwards offsets from every nested scroller. Switching frommain.scrollTop = 500to a descendant nearscrollTop = 0becomes a false upward gesture and reveals the chrome even when the user scrolls down. Track intent per target or explicitly rebase when the active scroll target changes, and add a nested-scroller regression test.🤖 Prompt for 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. In `@src/components/clinical-dashboard/global-search-shell.tsx` around lines 450 - 457, Update the scroll-intent handling around onScrollCapture and reportPhoneScrollHideRef so offsets and direction are not carried between different scroll containers; track the active target’s previous offset or explicitly rebase state when event.target changes before reporting the new offset. Preserve correct hide/reveal behavior for continued scrolling within one container, and add a regression test covering a switch from a deeply scrolled main container to a near-zero nested scroller.
🤖 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.
Outside diff comments:
In `@src/components/clinical-dashboard/global-search-shell.tsx`:
- Around line 450-457: Update the scroll-intent handling around onScrollCapture
and reportPhoneScrollHideRef so offsets and direction are not carried between
different scroll containers; track the active target’s previous offset or
explicitly rebase state when event.target changes before reporting the new
offset. Preserve correct hide/reveal behavior for continued scrolling within one
container, and add a regression test covering a switch from a deeply scrolled
main container to a near-zero nested scroller.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6715b8dc-d090-498d-9c1a-5c89aac894b6
📒 Files selected for processing (7)
docs/branch-review-ledger.mdsrc/components/ClinicalDashboard.tsxsrc/components/clinical-dashboard/global-search-shell.tsxsrc/components/clinical-dashboard/master-search-header.tsxsrc/components/clinical-dashboard/use-hide-on-scroll.tstests/ui-smoke.spec.tstests/use-hide-on-scroll.test.ts
💤 Files with no reviewable changes (1)
- src/components/clinical-dashboard/master-search-header.tsx
|
Addressed the valid CodeRabbit outside-diff finding in 35e74dd: scroll intent now rebases when the active scroll container changes, with focused unit and nested-scroller UI regressions. Local verification: 9/9 focused Vitest, TypeScript, scoped ESLint, Prettier, and diff checks passed. |
…-stability-20260717 # Conflicts: # docs/branch-review-ledger.md # src/components/clinical-dashboard/use-hide-on-scroll.ts # tests/use-hide-on-scroll.test.ts
1 participant
Summary
Why
Changing the scroll viewport while chrome was hiding caused scrollTop to be clamped near the bottom. That synthetic upward movement was interpreted as user intent, producing repeated hide/show movement and choppy scrolling.
Verification
npm run test -- tests/use-hide-on-scroll.test.ts— 8/8 passednpm run typecheck— passedgit diff --check— passedLocal limitations
npm run verify:cheapreached the full Vitest phase but was stopped after making no progress under concurrent local Vitest workloads.node_modulesjunction in this isolated worktree, and the Webpack fallback did not reach the project identity endpoint. Hosted exact-head UI and required CI are therefore merge gates.No Supabase, OpenAI, or other live-provider workflow was run.
Summary by CodeRabbit