fix: resolve header scroll hide behavior - #1354
Conversation
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
…scroll-5b1d' into cursor/fix-mobile-composer-edge-scroll-5b1d Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
📝 WalkthroughWalkthroughThe PR adds geometry-aware scroll-hide metrics and clamp handling, supports wide header collapse modes, coordinates header/composer pinning, improves composer autofocus retention, updates scroll-reporting wiring, and adds unit, UI, documentation, and verification coverage. ChangesMobile chrome scroll and focus behavior
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ac14e66e56
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| * cannot scroll (the phone shell is `fixed inset-0`) no scroll event arrives, | ||
| * so the internal scroller stays the single source at that width. | ||
| */ | ||
| export function useDocumentScrollHideReporter(reportScroll: (metrics: ScrollMetrics) => void) { |
There was a problem hiding this comment.
At viewport widths of 640px or greater, GlobalSearchShell hands scrolling to the document, but it still creates useScrollHideReporter() with the default phone-only gate and never invokes this newly added useDocumentScrollHideReporter; it also passes no wide: "sticky" option to MasterSearchHeader. Consequently, document scrolling can never update the hidden state and the new desktop sticky/transform branch is unreachable. A Playwright check that scrolls window on a desktop standalone-shell route and expects the header's data-scroll-hidden attribute would catch this.
Useful? React with 👍 / 👎.
![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 (2)
src/components/clinical-dashboard/use-hide-on-scroll.ts (1)
199-217: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
collapseKindignores the grid-display test that gatesheaderRelease.
headerReleaseis only counted when the collapse wrapper computes todisplay: grid, butcollapseKindis"in-flow"whenever the element merely exists. For thewide: "sticky"host (GlobalSearchShell above the phone breakpoint) the wrapper is present but sticky/translating, soheaderReleaseis 0 and the only budget is bottom reserve — yet the stricter in-flow runway gate is applied instead of the reserve-only path. Deriving the kind from the same condition keeps the two halves consistent.🔧 Proposed fix
- const headerRelease = - collapse instanceof HTMLElement && window.getComputedStyle(collapse).display === "grid" - ? collapse.getBoundingClientRect().height - : 0; + const collapsesInFlow = collapse instanceof HTMLElement && window.getComputedStyle(collapse).display === "grid"; + const headerRelease = collapsesInFlow ? collapse.getBoundingClientRect().height : 0; @@ return { collapseBudget: headerRelease + reserveRelease, - collapseKind: collapse instanceof HTMLElement ? "in-flow" : reserveRelease > 0 ? "reserve-only" : undefined, + collapseKind: collapsesInFlow ? "in-flow" : reserveRelease > 0 ? "reserve-only" : undefined, };🤖 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/use-hide-on-scroll.ts` around lines 199 - 217, Update the collapseKind derivation in the surrounding scroll-budget logic to use the same HTMLElement-and-computed-display-grid condition that enables headerRelease, rather than element existence alone. Return "in-flow" only when that condition is true; otherwise preserve "reserve-only" when reserveRelease is positive and undefined when it is not.src/components/clinical-dashboard/master-search-header.tsx (1)
367-392: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
composerChromeFocusednever gets set in the widehideOnScrollpaths.composerFocusPropsis only attached to the phone footer dock, so a focused sm+ composer won’t pin the header and can scroll under it. Attach the focus handlers to the sm+ composer placement too.🤖 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/master-search-header.tsx` around lines 367 - 392, The wide-screen composer placement must participate in shared focus pinning. Update the sm+ composer rendering path to attach the existing composerFocusProps handlers, not only the phone footer dock, so composerChromeFocused is set while the composer is focused and prevents header hiding during hideOnScroll.
🧹 Nitpick comments (1)
tests/ui-smoke.spec.ts (1)
2139-2151: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the repeated geometry-probe
evaluate()closures into shared helpers.
edgeGeometry,geometry, andsettledHiddenGeometryeach define an inline DOM-evaluation closure. This matches the pattern already established intests/playwright-scroll.ts(readMobileComposerReservePx,scrollPrimarySurface), which this same test already imports. Extracting these three closures there would reduce duplication and the flagged complexity of this block, and make them reusable for future phone-chrome tests.Based on the line-range change details noting
[code_block_complexity_high]for this segment.Also applies to: 2163-2170, 2190-2201
🤖 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 `@tests/ui-smoke.spec.ts` around lines 2139 - 2151, Extract the repeated DOM geometry probes from the edgeGeometry, geometry, and settledHiddenGeometry evaluate closures into shared helpers in the existing tests/playwright-scroll.ts utility module. Reuse those helpers in tests/ui-smoke.spec.ts while preserving each probe’s returned measurements and the current test behavior.
🤖 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/master-search-header.tsx`:
- Around line 367-392: The wide-screen composer placement must participate in
shared focus pinning. Update the sm+ composer rendering path to attach the
existing composerFocusProps handlers, not only the phone footer dock, so
composerChromeFocused is set while the composer is focused and prevents header
hiding during hideOnScroll.
In `@src/components/clinical-dashboard/use-hide-on-scroll.ts`:
- Around line 199-217: Update the collapseKind derivation in the surrounding
scroll-budget logic to use the same HTMLElement-and-computed-display-grid
condition that enables headerRelease, rather than element existence alone.
Return "in-flow" only when that condition is true; otherwise preserve
"reserve-only" when reserveRelease is positive and undefined when it is not.
---
Nitpick comments:
In `@tests/ui-smoke.spec.ts`:
- Around line 2139-2151: Extract the repeated DOM geometry probes from the
edgeGeometry, geometry, and settledHiddenGeometry evaluate closures into shared
helpers in the existing tests/playwright-scroll.ts utility module. Reuse those
helpers in tests/ui-smoke.spec.ts while preserving each probe’s returned
measurements and the current test behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 73de6cc7-bad8-4f15-a58f-89e24067c5db
📒 Files selected for processing (10)
docs/branch-review-ledger.mddocs/search-chrome-behaviour.mdsrc/components/ClinicalDashboard.tsxsrc/components/clinical-dashboard/global-search-shell.tsxsrc/components/clinical-dashboard/master-search-header.tsxsrc/components/clinical-dashboard/mobile-composer-reserve.tssrc/components/clinical-dashboard/use-hide-on-scroll.tstests/mobile-composer-reserve.test.tstests/ui-smoke.spec.tstests/use-hide-on-scroll.test.ts
|
Reviewed at exact head ac14e66. No replacement hunk. The near-bottom clamp was destructive and current main already has the safer scroll contract. Source branch and worktree are preserved; this PR is closed only to remove the superseded review queue entry. |
2 participants
Summary
RAG impact
Summary by CodeRabbit
Bug Fixes
Enhancements