fix(ui): edge-to-edge mobile layout on main#421
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
1c8bd79 to
32661eb
Compare
![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: 1c8bd79ee9
ℹ️ 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".
32661eb to
97bd9f5
Compare
d8fb310 to
45c0056
Compare
- Unify mobile shell to h-dvh flex column with --mobile-composer-reserve - Full-bleed mode home backgrounds and edge-to-edge action cards on phone - Top-align favourites/tools while centering other mode homes on mobile - Resolve footer dock CSS precedence; safe-area-only mobile header gutters - Strip inset utilities from phone footer dock markup Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Make #main-content the phone scroll surface with overflow-y-auto so tall routes (medication details, favourites) are not clipped by the h-dvh shell. Wire hide-on-scroll to that container via collapse strategy, matching ClinicalDashboard, instead of relying on document scroll. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
- Capture descendant scroll events in GlobalMockupSearchShell so nested page canvases still collapse the phone search dock. - Relax mode-home search midpoint bound for top-aligned edge-to-edge layout. - Stabilize document viewer composer hide test with scroll spacer and explicit scroll events; scroll the forms results scroller and blur composer focus. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
45c0056 to
827206b
Compare
- Keep mode-home children filling the scroll pane for vertical centering - Document viewer hide-on-scroll follows GlobalSearchShell main scroll - Add scrollPrimarySurface test helper for programmatic phone scroll - Relax mode-home centering band to the visible main pane, not full viewport Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughReworks mobile composer dock masking and scroll-hide behavior around ChangesMobile composer dock scroll & layout
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
tests/ui-tools.spec.ts (2)
790-805: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicates the new
scrollPrimarySurfacehelper instead of reusing it.This test manually re-implements scrolling
#main-content(settingscrollTopand dispatching a bubblingscrollevent) rather than calling the sharedscrollPrimarySurfacehelper introduced intests/playwright-scroll.ts. Beyond duplicating logic, the manual version dispatches withbubbles: truewhile the helper does not — an inconsistency that can mask real differences in how the app's scroll-hide reporter is wired.♻️ Suggested refactor
- const scroller = page.locator("`#main-content`"); - // Step scroll down so the shell's scroll reporter sees deliberate movement. - for (const offset of [40, 80, 120, 160, 200]) { - await scroller.evaluate((node, top) => { - node.scrollTop = top; - node.dispatchEvent(new Event("scroll", { bubbles: true })); - }, offset); - } + // Step scroll down so the shell's scroll reporter sees deliberate movement. + for (const offset of [40, 80, 120, 160, 200]) { + await scrollPrimarySurface(page, offset); + }🤖 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-tools.spec.ts` around lines 790 - 805, The scroll setup in the ui-tools spec duplicates the shared scrolling logic instead of using the new scrollPrimarySurface helper. Replace the manual `#main-content` scrollTop updates and bubbling scroll dispatch in this test with calls to scrollPrimarySurface from tests/playwright-scroll.ts, and keep the test aligned with the helper’s event behavior so the scroll-hide reporter is exercised consistently.
433-444: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant fallback after null assertion.
mainBox?.height ?? 820is dead code for the happy path sinceexpect(mainBox).not.toBeNull()on the previous line already guaranteesmainBoxis non-null when the test proceeds; the820magic number is essentially unreachable and just obscures intent.🤖 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-tools.spec.ts` around lines 433 - 444, The mainBox null fallback in the ui-tools test is redundant because the preceding assertion already guarantees a value, so remove the dead fallback and make the intent explicit. Update the search position checks in the same test block to use the non-null mainBox directly after the expect(mainBox).not.toBeNull() assertion, and keep the comparison anchored on the existing mainBox, searchBox, and headingBox locators.src/components/DocumentViewer.tsx (1)
1941-1958: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffDOM-ID coupling + whole-body MutationObserver for scroll-container resolution.
shellScrollContaineris resolved viagetElementById("main-content")inside aMutationObserverwatchingdocument.bodywith{ childList: true, subtree: true }for the component's entire lifetime. This works, but:
- It couples
DocumentViewerto a magic string ID owned byglobal-mockup-search-shell.tsx, with no compile-time contract — a rename there silently breaks scroll-hide here.- The observer callback fires on any DOM mutation anywhere in the app (sheets opening, image lists re-rendering, etc.), not just changes relevant to
#main-content.Since the shell already owns a
mainRef/scroll-root value, consider exposing it via a small context (or prop) instead of DOM lookups, so this becomes a typed dependency rather than an implicit string-ID contract.🤖 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/DocumentViewer.tsx` around lines 1941 - 1958, The scroll container lookup in DocumentViewer is tightly coupled to the hardcoded "main-content" DOM id and uses a broad MutationObserver over document.body for the component lifetime. Replace this implicit DOM query in the shellScrollContainer/useHideOnScroll setup with a typed dependency from the shell, such as a small context or prop backed by the shell’s mainRef/scroll-root value, so DocumentViewer no longer depends on a magic string or whole-app mutation watching.src/components/clinical-dashboard/favourites-home-page.tsx (1)
20-32: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider reusing
ModeHomeMaininstead of duplicating its wrapper styling.This
<main>re-implements most ofModeHomeMain's classes (flex min-h-0 w-full flex-1 flex-col bg-[color:var(--background)] ... text-[color:var(--text)]) but diverges in a few subtle ways that look unintentional:
- Uses
py-4/sm:py-5instead of the separatept-[clamp(...)]/pb-4/sm:pt-[clamp(...)]/sm:pb-[clamp(...)]split thatmode-home-template.tsxnow uses (line 130).- Uses
sm:px-4here vssm:px-6inModeHomeMain(line 130).- Omits
items-center justify-center(intentional here, since Favourites stays top-aligned per the comment inClinicalDashboard.tsxaround line 2914-2917), but that intent isn't documented in this file.Since both wrappers now implement the same "edge-to-edge main + inner
px-4 sm:px-0div" pattern for the same underlying reason (composer-reserve edge-to-edge layout), extendingModeHomeMainwith an alignment option (e.g.,align="start" | "center") and reusing it here would keep the two paths from drifting further apart as the mobile shell layout continues to evolve.🤖 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/favourites-home-page.tsx` around lines 20 - 32, The Favourites home page is duplicating `ModeHomeMain` wrapper styling and has already drifted in padding and alignment details. Update `FavouritesHomePage` to reuse `ModeHomeMain` (or extend `ModeHomeMain` with an alignment option such as start vs center) instead of hardcoding the `<main>` classes here, and keep the inner `px-4 sm:px-0` layout. Make sure the special top-aligned behavior for favourites is preserved explicitly through `ModeHomeMain`, so this file only passes the needed props and `FavouritesHub` stays unchanged.
🤖 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/clinical-dashboard/global-mockup-search-shell.tsx`:
- Around line 327-340: The scroll-capture setup in GlobalMockupSearchShell only
runs once in useEffect and can miss attaching when mainRef.current is initially
null. Update the listener wiring around mainRef/onScrollCapture so it attaches
when `#main-content` becomes available, either by using a callback ref or by
making the effect rerun when the main element appears; ensure the cleanup still
removes the same capture listener.
---
Nitpick comments:
In `@src/components/clinical-dashboard/favourites-home-page.tsx`:
- Around line 20-32: The Favourites home page is duplicating `ModeHomeMain`
wrapper styling and has already drifted in padding and alignment details. Update
`FavouritesHomePage` to reuse `ModeHomeMain` (or extend `ModeHomeMain` with an
alignment option such as start vs center) instead of hardcoding the `<main>`
classes here, and keep the inner `px-4 sm:px-0` layout. Make sure the special
top-aligned behavior for favourites is preserved explicitly through
`ModeHomeMain`, so this file only passes the needed props and `FavouritesHub`
stays unchanged.
In `@src/components/DocumentViewer.tsx`:
- Around line 1941-1958: The scroll container lookup in DocumentViewer is
tightly coupled to the hardcoded "main-content" DOM id and uses a broad
MutationObserver over document.body for the component lifetime. Replace this
implicit DOM query in the shellScrollContainer/useHideOnScroll setup with a
typed dependency from the shell, such as a small context or prop backed by the
shell’s mainRef/scroll-root value, so DocumentViewer no longer depends on a
magic string or whole-app mutation watching.
In `@tests/ui-tools.spec.ts`:
- Around line 790-805: The scroll setup in the ui-tools spec duplicates the
shared scrolling logic instead of using the new scrollPrimarySurface helper.
Replace the manual `#main-content` scrollTop updates and bubbling scroll dispatch
in this test with calls to scrollPrimarySurface from tests/playwright-scroll.ts,
and keep the test aligned with the helper’s event behavior so the scroll-hide
reporter is exercised consistently.
- Around line 433-444: The mainBox null fallback in the ui-tools test is
redundant because the preceding assertion already guarantees a value, so remove
the dead fallback and make the intent explicit. Update the search position
checks in the same test block to use the non-null mainBox directly after the
expect(mainBox).not.toBeNull() assertion, and keep the comparison anchored on
the existing mainBox, searchBox, and headingBox locators.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7a89d3f1-017b-4efd-b01c-8ee9a76af82a
📒 Files selected for processing (10)
src/app/globals.csssrc/components/ClinicalDashboard.tsxsrc/components/DocumentViewer.tsxsrc/components/clinical-dashboard/favourites-home-page.tsxsrc/components/clinical-dashboard/global-mockup-search-shell.tsxsrc/components/clinical-dashboard/master-search-header.tsxsrc/components/mode-home-template.tsxtests/playwright-scroll.tstests/ui-smoke.spec.tstests/ui-tools.spec.ts
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 1 file(s) based on 1 unresolved review comment. Files modified:
Commit: The changes have been pushed to the Time taken: |
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 1 file(s) based on 1 unresolved review comment. A stacked PR containing fixes has been created.
Time taken: |
Fixed 1 file(s) based on 1 unresolved review comment. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. ❌ Failed to clone repository into sandbox. Please try again. |
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 1 file(s) based on 1 unresolved review comment. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 1 file(s) based on 1 unresolved review comment. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
2 participants
Summary
Re-applies the edge-to-edge mobile layout on current
mainand addresses review feedback.Review fix (P1)
#main-contentis the phone scroll surface (overflow-y-auto) so tall routes are not clippedcollapse+ container scroll (matchesClinicalDashboard)#main-contentwhen rendered inside the shellFollow-up
#main-contentinstead ofwindowVerification
npm run format:check— passnpm run verify:cheap— pass locally