Uh oh!
There was an error while loading. Please reload this page.
fix(ui): pin full-height shell screens so only the body scrolls (#331) - #335
Conversation
In the horizontal top-nav layout, ShellFrame stacks the 56px TopNav above <main> in a 100vh flex column, so <main> is only `100vh - 56px` tall. Screens that hardcoded `height: 100vh` were therefore taller than their container, overflowing <main> and scrolling the whole page — dragging chat headers/inputs out of view instead of scrolling just the message body. The sidebar layout was unaffected because SideNav sits beside <main>, giving it the full 100vh. Fix: fill the parent (`height: 100%`) instead of the viewport. Introduce a shared <FullHeightScreen> wrapper and adopt it across the full-height shell screens so the pattern is consistent and can't regress: - new: components/FullHeightScreen.tsx (height:100%, min-height:0) - Learn, Quiz, Library, Social, course-planner: use FullHeightScreen - notetaker: height 100vh -> 100% (root + loading/empty states) - Study: min-height 100vh -> 100% (stays scrollable, now bounded to <main>) Percentage height resolves in both layouts because <main> has a definite height in each, so the sidebar layout remains correct. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Deploying with |
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs | frontend-staging | 40fdc91 | Commit Preview URL Branch Preview URL | Jul 15 2026, 05:48 AM |
Warning Review limit reached
Next review available in:59 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: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
✨ 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 |
…2px) Settings was the screen #331 missed. Its content row hardcoded `height: calc(100vh - 112px)` — the TopBar height subtracted from the viewport, which only held under the sidebar layout where `<main>` is the full viewport. Under the top-nav layout `<main>` is `100vh - 56px`, so the row overflowed it by exactly the 56px TopNav: /settings got a second scrollbar and pushed its bottom 56px below the fold, including the account-deletion controls on the `data` tab. Root the screen in FullHeightScreen and let the content row flex into whatever `<main>` leaves below the TopBar, matching how the other five screens were fixed. This also drops the 112px magic number, so the row no longer silently breaks if the TopBar is ever resized. PreviewModal is `position: fixed`, so it stays out of flow and is unaffected by the root becoming a flex column.
On iOS Safari `100vh` resolves to the *large* viewport — the height with the toolbar collapsed — so a `100vh` shell always extends past the visual viewport while the toolbar is expanded, hiding its own bottom edge. Until now the full-height screens overflowed `<main>` by 56px, and that incidental scroll slack let you drag the clipped edge back into view. #331 removed the slack: screens fit `<main>` exactly, so anything under the toolbar is now unreachable — most visibly the /learn composer. `100dvh` tracks the visual viewport, so the shell ends where the toolbar begins. The usual dvh objection (the value changes as the toolbar collapses, resizing the layout mid-scroll) does not apply here: the shell root is `overflow: hidden` and scrolling happens in inner panes, so the document never scrolls and the toolbar never collapses. No `100vh` fallback: expressing one needs two declarations of the same property, which a React inline style object cannot hold, so it would mean moving the shell root to a CSS class. It would buy nothing — dvh has been Baseline since 2022 (Safari 15.4 / Chrome 108 / Firefox 101), and globals.css already leans unguarded on `color-mix()`, `:has()`, and `overflow-x: clip`, all of which are equal or narrower support. Any browser needing the fallback is already broken by the stylesheet.
`min-height: 0` only does anything on a flex item, where it overrides the `auto` automatic-minimum-size that would otherwise refuse to shrink below the content. FullHeightScreen's documented parent is ShellFrame's `<main>`, a block container, so the root is a block-level box and `min-height: auto` already computes to 0 — the declaration is a no-op at every one of its six call sites. Remove it rather than comment it. Inert CSS on a shared primitive reads as load-bearing and gets copied into places where the author has not checked whether it matters. Callers that genuinely need it can pass it via `style`, and screens that need it on their own inner rows still set it there (see Settings).
AndresL230
commented
Jul 15, 2026
@Darkest-Teddy heads up — I pushed 3 commits to this branch as part of a review sweep, and retargeted the base. Nothing of yours was rewritten (fast-forward only). Happy to back any of it out if you disagree. Base retargeted: |
AndresL230
left a comment
There was a problem hiding this comment.
Approving. The original diff was sound — the 56px TopNav arithmetic checks out and all five converted screens' scroll chains terminate correctly in a flex: 1 + overflowY: auto box, so nothing escapes into <main> and no double-scroll is introduced. The three blockers (dead base branch, missed Settings.tsx, 100vh on iOS) are addressed in the commits above and CI is green on 40fdc91.
Reiterating the caveat from my comment, because the green check overstates what was verified: no test in this repo can observe a layout regression — jsdom has no layout engine. The suite passing proves component contracts are intact, not that the pinning works. A real-device pass on /learn and /settings is worth doing.
Uh oh!
There was an error while loading. Please reload this page.
The comment landed before #335 switched the shell root from 100vh to 100dvh.
* fix(ui): lock body scroll while the shared Dialog is open (#109) Dialog never called the existing useBodyScrollLock hook, so opening any modal built on it let the mobile background scroll behind the overlay. Wire useBodyScrollLock(open) into Dialog — the same pattern the 5 other portal modals (DisclaimerModal, SessionFeedbackFlow, FeedbackFlow, ReportIssueFlow, Library) already use. Part of #109's AC3. The larger #109 work (making Admin/Gradebook/Settings responsive + migrating the ~8 hand-rolled Gradebook modals onto Dialog) is separate and not in this PR. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(ui): refcount the scroll lock and aim it at the real scrollport useBodyScrollLock had two defects that made it unfit for the shared Dialog (#109). 1. It clobbered overlapping locks. Naive save/restore means the second overlay captures the first one's "hidden" as its "previous" value. FeedbackFlow is mounted globally by ShellFrame and auto-opens on a timer, so it routinely overlaps another modal: the first unlock frees the scroll while the second overlay is still up, and the second unlock restores the stale "hidden" -- freezing the page until navigation. Locks are now refcounted, so only 0->1 captures the original value and only 1->0 restores it. 2. It locked the wrong element. Inside the app shell <body> never scrolls: ShellFrame roots both layouts at height:100vh/overflow:hidden and gives <main> flex:1/overflow-y:auto. Setting overflow on <body> there acts on an element with no scrollport, so the lock did nothing for Dialog's only consumer (FlashcardImportModal in Study). The target is now resolved at lock time: the element ShellFrame tags with data-scroll-container, falling back to <body> for the pre-auth pages where <body> genuinely is the scroller. Refcounts are keyed per element (Map<HTMLElement, ...>) so a lock on <main> and a lock on <body> can't hold each other hostage. Only the overflow-x/overflow-y longhands are touched, never the overflow shorthand: ShellFrame sets overflow-y:auto inline via React, and writing the shorthand then restoring it to its previous value ("") would remove that inline declaration outright and leave <main> unable to scroll at all once a modal closed. Renamed to useScrollLock -- the hook no longer necessarily targets <body>, and a name that says otherwise would mislead the next reader. * fix(ui): route the hand-rolled body scroll locks through useScrollLock Nine sites across eight components set document.body.style.overflow by hand, and they disagreed with each other. Six of them (SignInModal, DocumentUploadModal, ManageCoursesModal, Dashboard, Tree, and both landing-page locks) reset to "" unconditionally on cleanup, which breaks stacking exactly the way the old hook did -- close one overlay and any other overlay's lock is silently dropped. The other three saved and restored the previous value, which stacks no better because the value they save may itself be another lock's "hidden". All of them now share the refcounted hook, so overlapping overlays compose instead of fighting. Targeting follows from the hook: the in-shell overlays resolve to <main>, which is what actually scrolls there, while the two pre-auth locks in (public)/page.tsx keep locking <body> -- there is no shell on the landing page, so the fallback is correct and the behavior is unchanged for them. The scroll-lock concern is lifted out of the effects that were also managing Escape listeners and reset state; those effects keep only the work that isn't scroll locking. * test(ui): cover useScrollLock refcounting, targeting, and style round-trip Eleven cases over the bookkeeping that was actually broken: overlapping locks in both release orders, three-deep nesting, a false->true->false transition while another lock is held, unmount-while-active, per-element isolation between <main> and <body>, shell vs pre-auth targeting, and exact restoration of ShellFrame's inline overflow-y:auto. Checked against the old implementation: 8 of the 11 fail on it, so these pin the reported bug rather than just the new code's shape. These tests cannot show the scroll-bleed itself is fixed. jsdom has no layout engine -- nothing here scrolls, has a scrollport, or paints, so only the style bookkeeping is observable. Confirming the background no longer scrolls behind a modal needs a real browser. * docs(ui): correct the scrollport comment after the 100dvh change The comment landed before #335 switched the shell root from 100vh to 100dvh. --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: AndresL230 <190146319+AndresL230@users.noreply.github.com> Co-authored-by: AndresL230 <andreslopez.23061@gmail.com>
Review follow-ups on the journey added with this PR. Two-sided fit: the spec only guarded overshoot, so a future regression that broke `flex: 1` and left the canvas a few pixels tall would have passed — `canvasHeight > 0` is not a floor. It now asserts the canvas settles FLUSH with the scrollport's bottom edge, which is the actual stated invariant, and polls for it because the canvas size is ResizeObserver-driven and starts at Tree.tsx's placeholder 900x600. Gradebook coverage: Landing/Course got the same class of change as Tree with no geometry assertion behind it — the gap that let the identical `calc(100vh - N)` bug survive review on Settings.tsx during #335, since jsdom has no layout engine and cannot catch it. Adds a second test asserting the seeded gradebook does not manufacture scroll it does not need, in both layouts. Renames tree-viewport.spec.ts -> viewport-fit.spec.ts now that it covers two screens. part of #341 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…#487) * fix(ui): replace viewport-math residuals with FullHeightScreen (#341) Three screens still sized themselves by subtracting a magic constant from 100vh. That subtraction can only be correct for ONE of ShellFrame's two layouts — the sidebar variant gives `<main>` the full 100dvh, the TopNav variant gives it 100dvh - 56px — and it is blind to the density preference that retunes the padding tokens above it. Tree.tsx is the one with a user-visible consequence, not just a stray scrollbar: the row sized `calc(100vh - 240px)` wraps the element a ResizeObserver watches, and that contentRect is passed straight through as `<KnowledgeGraph width height>`. A mis-measured row therefore renders the graph CANVAS at the wrong size. It now takes `flex: 1` + `minHeight: 0` inside a FullHeightScreen root, so it absorbs exactly what `<main>` has left after the TopBar and filter row, in either layout and at any density. Gradebook Landing/Course used `calc(100vh - var(--row-h))` — subtracting a DENSITY token (40/34/48px) as though it were a nav height. Both now sit in a FullHeightScreen with `flex: 1 0 auto` on their `<main>`: fill the remaining space, keep growing when the content is taller. Adds frontend/e2e/tree-viewport.spec.ts, which asserts the invariant that was silently violated — on /tree the graph canvas fits inside the shell scrollport — in BOTH layouts, driven off the localStorage layout pref. It lives in its own spec because graph.spec.ts declares itself data-only and reads no geometry by design. part of #341 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(e2e): assert fit in both directions, and cover the gradebook (#341) Review follow-ups on the journey added with this PR. Two-sided fit: the spec only guarded overshoot, so a future regression that broke `flex: 1` and left the canvas a few pixels tall would have passed — `canvasHeight > 0` is not a floor. It now asserts the canvas settles FLUSH with the scrollport's bottom edge, which is the actual stated invariant, and polls for it because the canvas size is ResizeObserver-driven and starts at Tree.tsx's placeholder 900x600. Gradebook coverage: Landing/Course got the same class of change as Tree with no geometry assertion behind it — the gap that let the identical `calc(100vh - N)` bug survive review on Settings.tsx during #335, since jsdom has no layout engine and cannot catch it. Adds a second test asserting the seeded gradebook does not manufacture scroll it does not need, in both layouts. Renames tree-viewport.spec.ts -> viewport-fit.spec.ts now that it covers two screens. part of #341 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(e2e): assert the gradebook adds no phantom height, not that it never scrolls (#341) The first version of this test asserted the gradebook page must not scroll at all. That is wrong: under the topnav layout the scrollport is 56px shorter, and the seeded gradebook's content genuinely needs more than it — scrolling there is the correct answer, and the assertion failed the FIXED build for a legitimate reason (16px at topnav, 0 at sidebar). What the fix actually guarantees is narrower: the container contributes no height of its own beyond the space it was given or the height its content needs. That is now what the test measures — <main> must be no taller than max(available, content), and no shorter than the space available (which would mean flex-grow is broken). The unfixed build overshot by exactly 40px at the default density, which is `--row-h` to the pixel — the density-token-as-nav-height bug, measured. part of #341 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(e2e): measure gradebook content from its children, not scrollHeight (#341) The previous revision of this assertion passed on the UNFIXED build, which makes it worthless as a regression test. Caught by running the spec against the base sha before trusting it green. Cause: it derived the content height from `main.scrollHeight`, which is floored at the element's own client height. Any phantom height the box gave itself was therefore mirrored into the "content" it was being compared against, so `mainHeight <= max(available, content)` held by construction and could never fail. It now measures the content extent from the in-flow children (skipping AmbientOrbs, which is position:fixed) plus the bottom padding — a number the box's own sizing cannot influence. part of #341 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(e2e): close three review findings in the viewport-fit spec (#341) Follow-ups from the review of the spec delta. Gate on the LOADED gradebook grid, not the chrome. `gradebook-transcript-open` renders as soon as there is a user, while `loading` is still swapping a six-card skeleton in for the real grid — measuring that transient made the phantom-height assertion depend on fetch timing. Now waits on role="grid"/"Courses", which only exists once the fetch resolved. Count trailing child margins in the content measurement. A margin-bottom on a direct in-flow child raises <main>'s height without appearing in any child's bounding rect, which would have understated the content and turned a real regression into a pass. Latent today; cheap to close. Assert on the reading that settled. The tree test polled for convergence and then measured AGAIN, reopening the window the poll existed to close. A small settledFit() helper now returns the very reading that satisfied the condition, and every assertion runs on that one. It also returns the last reading on timeout, so a failure reports real numbers instead of a bare "timed out". part of #341 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(e2e): rename useLayout so eslint stops reading it as a React Hook (#341) CI's react-hooks/rules-of-hooks rejected the helper: anything named use* is treated as a Hook, and the spec calls it inside a for-loop over the two shell layouts. Renamed to switchLayout — behaviour identical. Local `npm run lint` reported 0 errors on the same code; the installed eslint-plugin-react-hooks is older than CI's, so this was only visible there. part of #341 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Fix: pin full-height shell screens so only the body scrolls
Closes#331
The bug, in code
ShellFrame(top-nav layout) —<main>is100vh − navHeight:But the screens forced the full viewport inside that shorter
<main>:100vh > (100vh − 56px)→<main>overflows → the whole page scrolls, dragging the chat header/input out of view. The sidebar layout was fine becauseSideNavsits beside<main>(row), so<main>gets the full100vhand100vhmatched exactly.The fix
Fill the parent, not the viewport —
100vh→100%, wrapped in one shared component:height: 100%resolves against<main>(which has a definite height in both layouts), so top-nav is fixed and sidebar is unchanged.The diffs
Standalone pages (
about,terms,privacy,careers,auth,pending,Onboarding) keep100vh— they own the full viewport, correctly.Files changed
#331 checklist
height: 100vh→100%<FullHeightScreen>) applied across affected screens100%resolves to the same100vhthere)Testing
Manual check still recommended:
/learn,/quiz,/library,/social,/notetakerin top-nav layout (header/input pinned, body scrolls) + sidebar layout unchanged.