fix(ui): restore phone Safari and PWA edge chrome#1270
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
Warning Review limit reached
Next review available in: 55 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 (5)
📝 WalkthroughWalkthroughPhone browser and standalone layouts now use different scroll owners, shared scroll utilities, and frame/portal-based phone footer layers. Chrome hide/reveal wiring, responsive shell CSS, focus clearance, and extensive contract and Playwright coverage were updated. ChangesPhone scroll and chrome behavior
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
BigSimmo
added
the
skip-branch-sync
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
tests/dashboard-scroll-padding.test.ts (1)
44-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThese regexes can match across rule boundaries.
\.phone-scroll-surface\s*\{[\s\S]*?overflow-y:\s*visible;is unbounded by the closing brace, so aoverflow-y: visibledeclared in any later rule satisfies it — the guard would keep passing after the contract is broken.tests/ui-overlay-css-contract.test.tsalready solves this with brace-depth block extraction (cssBlock); extracting the block first (or restricting the gap with[^}]*?) makes this assertion actually load-bearing.🤖 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/dashboard-scroll-padding.test.ts` around lines 44 - 47, The regex assertions in the dashboard scroll-padding test are not scoped to the `.phone-scroll-surface` rule and can match declarations in later rules. Update the assertions near `globalStyles` to extract or constrain the rule block, reusing the existing `cssBlock` approach from `ui-overlay-css-contract.test.ts` or limiting matches to characters before `}`, while preserving the desktop `visible` and standalone mobile `auto` expectations.src/components/clinical-dashboard/phone-footer-layer-portal.tsx (1)
38-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the exported phone breakpoint constant before reusing it.
use-hide-on-scroll.tshas the matching 639px query, but it is currently private. ExportphoneMediaQueryand import it here so the footer portal is gated the same way the scroll-hide paths use the shared phone seam.🤖 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/phone-footer-layer-portal.tsx` around lines 38 - 45, Export the existing phone breakpoint constant phoneMediaQuery from use-hide-on-scroll.ts, then import and use it in the phone-footer portal’s useLayoutEffect instead of the duplicated "(max-width: 639px)" query. Preserve the existing matchMedia synchronization and cleanup 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.
Inline comments:
In `@src/components/ClinicalDashboard.tsx`:
- Around line 372-375: Move the reportChromeScrollHideRef.current assignment out
of the render body and into a useEffect keyed by chromeScrollHide.reportScroll,
matching the latest-report ref pattern used by the sibling implementation. Keep
useDocumentScrollHideReporter unchanged.
In `@tests/ui-chrome-scroll.spec.ts`:
- Around line 176-189: Update the test around “desktop document scrolling does
not blur the focused page search” so it verifies the actual scroll owner using
readPrimaryScrollGeometry(page).owner before and after scrolling, or move the
assertion into the phone-sized document-scroll coverage where the phone-gated
focus behavior applies. Ensure the test no longer labels bounded `#main-content`
scrolling as document scrolling.
In `@tests/ui-smoke.spec.ts`:
- Around line 2004-2022: Update the geometry assertions around
readPrimaryScrollGeometry and the paired page.evaluate calls so both scroll and
DOM measurements are captured in one sequential snapshot rather than
Promise.all. Apply the same change to the corresponding blocks near the other
referenced assertions, preserving the existing geometry values and expectations
while eliminating split DOM moments.
---
Nitpick comments:
In `@src/components/clinical-dashboard/phone-footer-layer-portal.tsx`:
- Around line 38-45: Export the existing phone breakpoint constant
phoneMediaQuery from use-hide-on-scroll.ts, then import and use it in the
phone-footer portal’s useLayoutEffect instead of the duplicated "(max-width:
639px)" query. Preserve the existing matchMedia synchronization and cleanup
behavior.
In `@tests/dashboard-scroll-padding.test.ts`:
- Around line 44-47: The regex assertions in the dashboard scroll-padding test
are not scoped to the `.phone-scroll-surface` rule and can match declarations in
later rules. Update the assertions near `globalStyles` to extract or constrain
the rule block, reusing the existing `cssBlock` approach from
`ui-overlay-css-contract.test.ts` or limiting matches to characters before `}`,
while preserving the desktop `visible` and standalone mobile `auto`
expectations.
🪄 Autofix (Beta)
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: 4c66ae16-b07c-4e8b-80f7-44e05799b907
📒 Files selected for processing (27)
docs/branch-review-ledger.mddocs/search-chrome-behaviour.mdsrc/app/globals.csssrc/components/ClinicalDashboard.tsxsrc/components/DocumentViewer.tsxsrc/components/calculators/search-page.tsxsrc/components/clinical-dashboard/global-search-shell.tsxsrc/components/clinical-dashboard/master-search-header.tsxsrc/components/clinical-dashboard/phone-footer-layer-portal.tsxsrc/components/clinical-dashboard/scroll-surface.tssrc/components/clinical-dashboard/use-document-viewer-chrome-scroll.tssrc/components/clinical-dashboard/use-hide-on-scroll.tssrc/components/differentials/differential-presentation-workflow-page.tsxsrc/components/therapy-compass/therapy-compass.csssrc/components/ui-primitives.tsxtests/clinical-dashboard-merge-artifacts.test.tstests/dashboard-scroll-padding.test.tstests/header-scroll-hide-contract.test.tstests/playwright-scroll.tstests/ui-chrome-scroll.spec.tstests/ui-formulation.spec.tstests/ui-overlay-css-contract.test.tstests/ui-phone-scroll.spec.tstests/ui-smoke.spec.tstests/ui-stress.spec.tstests/ui-therapy-nav-scroll.spec.tstests/ui-tools.spec.ts
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
CI triageCI failed on this PR. Automated classification of the 2 failed job(s):
Compared with main CI run #5913 (success). Classification is evidence routing, not permission to ignore a failure. Exact quarantined Playwright identities remain governed by the flake ledger. |
BigSimmo
removed
the
skip-branch-sync
1 participant
Summary
Root cause
Browser Safari and installed standalone mode require different scroll ownership. Browser pages were trapped in a fixed inner scroller, preventing Safari toolbar collapse, while page-owned PWA footers were rendered inside that same inner scroller. Hidden chrome also retained paint/reserve paths that could leave an opaque band. The shared frame/portal contract now assigns each surface one scroll and footer owner.
Verification
npm run verify:cheap- all 25 gates; 393 files; 3,532 tests passed, 2 skipped/formshydration failure reproduced and fixed - exact production journey 10/10PR requiredSUCCESS and Production UI 323/3238c84b87e-6345-490b-b20b-89d9c844ea4eSUCCESS; readiness and health HTTP 200Privateaddress pill are native Safari controls outside the page viewportnpm run check:branch-review-ledgergit diff --checknpm run verify:pr-localpreviously passed runtime, formatting, lint, typecheck, the full unit suite, offline RAG fixtures, and its standalone production build; one aggregate wrapper invocation exited silently during Next's post-compile TypeScript phase. The identical build passed independently, and exact-head hosted Build plus Production UI are green.Physical acceptance
The installed PWA now passes the requested edge-to-edge header/footer behavior with seamless hide/reveal and no retained app band. In normal Safari, the page also owns the full web viewport; iOS still renders its status row and floating
Privateaddress control outside that viewport. CSS cannot remove or paint through those system-owned controls, so they are recorded as the platform boundary rather than an unresolved app defect.Risk and rollback
Risk is limited to phone chrome/scroll ownership and iOS safe-area compositing. Tablet and desktop retain inline ownership. Roll back by reverting the squash merge if physical or production smoke checks regress.
Merging this PR to
mainautomatically deploys the Railway production app.RAG impact: no retrieval behaviour change — UI-only phone chrome and scroll ownership changes.
Summary by CodeRabbit
Bug Fixes
Documentation
Tests