Uh oh!
There was an error while loading. Please reload this page.
fix(ui): replace viewport-math residuals with FullHeightScreen (#341) - #487
Conversation
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>
This pull request has been ignored for the connected project Preview Branches by Supabase. |
Warning Review limit reached
Next review available in:45 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 (1)
📝 WalkthroughWalkthroughThe PR replaces viewport-based sizing with ChangesViewport-fit shell layout
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
Deploying with |
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs | frontend-staging | caf04b0 | Commit Preview URL Branch Preview URL | Jul 31 2026, 02:59 AM |
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>
…ever 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>
…ght (#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>
AndresL230
commented
Jul 31, 2026
Local e2e cycle — red at base, green at fixRun as two phases inside one Both phases matter: a journey that passes on unfixed code proves nothing, so phase 1 runs the new spec against the base build and requires it to fail. Phase 1 — base Two things worth noting in those numbers:
Phase 2 — fix Two corrections this caughtThe gradebook assertion took two revisions before it was worth keeping, both caught by the base run rather than by review:
It now measures the content extent from the in-flow children (skipping Gates
|
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>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
frontend/e2e/viewport-fit.spec.ts (1)
120-189: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd Gradebook Course screen coverage for the same phantom-height fix.
frontend/e2e/viewport-fit.spec.tsonly runs this geometry check on/gradebook. No other e2e test exercisesGradebookCourseScreenpath with the same<main>content-vs-container height assertion, so add a course-route test or sibling coverage to cover this regression directly.🤖 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 `@frontend/e2e/viewport-fit.spec.ts` around lines 120 - 189, Extend the viewport-fit geometry coverage to navigate to a GradebookCourseScreen course route in addition to /gradebook. Reuse the existing layout loop and <main> content-versus-available-height assertions, ensuring the course screen’s route and seeded data are loaded before measuring so the phantom-height regression is tested directly.
🤖 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.
Nitpick comments:
In `@frontend/e2e/viewport-fit.spec.ts`:
- Around line 120-189: Extend the viewport-fit geometry coverage to navigate to
a GradebookCourseScreen course route in addition to /gradebook. Reuse the
existing layout loop and <main> content-versus-available-height assertions,
ensuring the course screen’s route and seeded data are loaded before measuring
so the phantom-height regression is tested directly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 99a37da4-caf8-470e-ab54-4b0b4d647048
📒 Files selected for processing (4)
frontend/e2e/viewport-fit.spec.tsfrontend/src/components/screens/Gradebook/Course.tsxfrontend/src/components/screens/Gradebook/Landing.tsxfrontend/src/components/screens/Tree.tsx
#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>
Uh oh!
There was an error while loading. Please reload this page.
Jose-Gael-Cruz-Lopez
commented
Jul 31, 2026
Automated post-merge review pass (Claude Code) on One real regression worth a follow-up fix:
Minor follow-ups:
|
Replaces the three surviving
calc(100vh - …)residuals withFullHeightScreen+ flex, and adds the journey that proves the one with a user-visible consequence.Why these were wrong
ShellFramerenders two layouts: a sidebar (its<main>is the full100dvh) or a horizontalTopNavabove<main>(so<main>is100dvh - 56px). Any constant subtracted from100vhcan only be calibrated for one of them — and none of them accounted for the density preference, which retunes the padding tokens of the chrome being subtracted.The consequential one — Tree.tsx
height: calc(100vh - 240px)wrapped the element aResizeObserverwatches; itscontentRectis passed straight through as<KnowledgeGraph width height>. So this was not a stray scrollbar — the graph canvas itself rendered at the wrong size in whichever layout the constant wasn't tuned for.Now:
FullHeightScreenroot +flex: 1/minHeight: 0on the graph row, so it absorbs exactly what<main>has left below the TopBar and filter row. Correct in both layouts, at any density, by construction rather than by calibration.The semantic ones — Gradebook Landing + Course
minHeight: calc(100vh - var(--row-h))subtracted a density token (40/34/48px perglobals.css) as if it were a nav height. Both now sit in aFullHeightScreenwithflex: 1 0 autoon their<main>— fill the remaining space, keep growing when content is taller (theminHeightsemantic, preserved).Test
frontend/e2e/tree-viewport.spec.tsasserts the invariant that was violated: on/treethe graph canvas fits inside the shell scrollport — no overshoot, no scrollable overflow — in both layouts, driven off thesapling_layoutlocalStorage pref. It is a new spec rather than an addition tograph.spec.ts, which declares itself data-only and deliberately reads no x/y.FullscreenGraph(the overlay variant) already usedflex: 1and is untouched.Gates
tsc --noEmit— cleannpm run lint— 0 errors (36 pre-existing warnings)npx vitest run— 55 files, 399 tests passedpart of #341
Summary by CodeRabbit
Bug Fixes
Tests