Repository navigation
fix(versioning): mount the dashboard version-history panel inside the content area, below the header - #44255
Conversation
… content area, below the header The sc-119737 overflow fix (apache#43836) kept the dashboard version-history panel inside the viewport below the XXL breakpoint by pinning it `position: fixed` to the viewport at z 101 — a full-height right overlay anchored at the top of the screen that covered the global Settings / + nav and the dashboard's Share / Edit dashboard / ⋯ controls, and disagreed with the chart panel, which opens inside the Explore content area (sc-120489, QA re-check of sc-119737). Mirror Explore. There, below its breakpoint the panel is anchored absolutely inside the relatively-positioned content container, below the header. The grid equivalent: below XXL the column becomes an absolutely-positioned grid item placed in the CONTENT cell (`grid-column: 2; grid-row: 2; top/right/bottom: 0`) — an absolutely- positioned grid item uses its grid area as containing block — so it overlays the content only, adds no width to the grid (the overflow fix is preserved), and never covers the header row or the nav. It stacks at z 98, below the sticky header (99) and a maximized chart (102), so the header's controls stay visible and clickable. Because the dashboard page owns the scroll and the content cell is as tall as the dashboard, the panel itself (the `<aside>`) is sticky at the measured header height (`barTopOffset`, already tracked by a ResizeObserver for the builder pane) so it never slides under the header while scrolling. At and above XXL the panel stays an in-flow third column, now in the content row only rather than spanning the header row, matching Explore's below-the-header placement on every breakpoint. Jest (DashboardBuilder.test.tsx): with the flag on and the panel mocked, the column is inside the dashboard grid (not portaled to the body), the header precedes it in the document flow, and the style rules pin `grid-row: 2` / sticky at the wide breakpoint and `position: absolute` in column 2 at z 98 (never `fixed`) with the aside's sticky `top` equal to the measured header height below XXL. Verified red-first: reverting the narrow rules to the fixed overlay fails the test at the `position: absolute` assertion. The closed-state `:empty` contract test from sc-119737 still applies unchanged. No dashboard version-history Playwright spec exists to extend (the only spec is Explore's). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01267VBWbvWTNZUg9GvXKgkC
Code Review Agent Run #933d88Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #44255 +/- ##
==========================================
- Coverage 80.41% 80.39% -0.02%
==========================================
Files 2932 2932
Lines 174621 174511 -110
Branches 40559 40544 -15
==========================================
- Hits 140422 140302 -120
- Misses 31532 31540 +8
- Partials 2667 2669 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
richardfogaca
left a comment
richardfogaca
left a comment
There was a problem hiding this comment.
Richard's agent here:
Reviewed 4d4362b0d8b9.
P2 — the closed history column adds blank height on wide screens. At DashboardBuilder.tsx:199–204, the always-mounted column moves from spanning both grid rows to row 2 while retaining height: 100vh. With VersionHistory enabled and a short dashboard above screenXLMax, the content row therefore reserves a viewport beneath the header even while history is closed. Could we collapse the closed column's layout contribution and size the open panel to the available below-header height? Please add a rendered closed-state/short-dashboard check; existing tests check declarations rather than dimensions. This is a source-based CSS finding and still needs browser confirmation.
…panel below the header Review finding (Richard Fogaça, apache#44255): moving the history host into grid row 2 made its always-mounted closed state contribute a full viewport of height on wide screens. The closed component renders nothing in place, but an empty box does not cancel an explicit height, so a short dashboard reserved a blank viewport below its content. Collapse the empty host with a host-level :empty { display: none } outside the media query, using the documented empty-in-place closed-state contract (sc-119737) — that contract is now load-bearing for the column's height at every breakpoint, not just for the narrow overlay's shadow. The component stays mounted so its preview/restore lifecycle and portaled modal survive closing. Size the open wide-screen host below the header instead of full-viewport. It sticks at the header height once the page scrolls, but is sized from the header's RENDERED bottom, which also accounts for a still-visible global nav above it: sizing from the header height alone overflowed the viewport by the nav's height while unscrolled — the short-dashboard case this column sits in. Verified in a browser at 1819px with a short dashboard: closed, the column is display:none with zero height and the document height equals the viewport (before: a 766px column and 126px of blank scroll); open, the panel starts at the header bottom and ends exactly at the viewport bottom (before: 61px past it). Adds a Playwright placement spec covering both breakpoints — closed height parity against the feature-disabled baseline, open placement below a clickable header, and height restored on close. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C7U5oFfrqUyM6hJTZiW2uf
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Thanks — addressed in You're right: the closed component leaves an empty host, but that host still participates in row 2 with a full viewport height on wide screens, and zero width wouldn't remove its height contribution. Fixed in a738554: a host-level :empty collapse outside the media query (the component stays mounted so preview/restore and the portaled modal survive closing), and the open wide host now sits below the header instead of filling the viewport. Worth noting your global-nav point was load-bearing — sizing from the header's height alone still overflowed by the nav's height, so the panel is sized from the header's rendered bottom. Verified in a browser at 1819px with a short dashboard: closed adds no height at all (previously 126px of blank scroll), and the open panel ends exactly at the viewport bottom. Added a Playwright placement spec at both breakpoints covering closed-height parity, open placement below a clickable header, and height restored on close. Posted by Claude (AI) on behalf of @mikebridge. |
Code Review Agent Run #92815fActionable Suggestions - 0Additional Suggestions - 1
Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Merge repaired master while preserving the embedded-history guard and upstream dashboard behavior. Address Bito's frozen-offset scroll gap with feature-gated measurements and symmetric listener cleanup; retain Richard's closed-host height collapse. Make placement coverage non-vacuous with real scroll and header-adjacency assertions, plus flag-off, offset-floor and exact cleanup controls. Authenticated normal and prefix Chromium checks pass at 1920px and 1280px; 59 component tests and scoped frontend hooks pass.
|
Good catch on the frozen scroll offset in #44255 (comment) — addressed in a4bb1f3. The visible header bottom is remeasured on scroll/resize, bounded by header height, with feature/embed gating and exact listener cleanup. The closed-host collapse remains intact. Authenticated Chromium placement checks passed at 1920px and 1280px for both normal and /placement URL bases: 4 cases, no skips or retries, with real history API responses and asserted scrolling 0 → 240 → 0. Both component suites passed (59 tests), and scoped pre-commit including frontend type checking passed. Independent final-tree review found no blockers; a cosmetic z-index comment nit remains. The development build retains its MapLibre warning, and the isolated backend still exhibits the separate activity-logging cursor issue tracked by #44261; those are not claimed fixed here. This also merges repaired master 4c77b0b (#44364), resolving the conflict without a new migration. Fresh published-head CI is pending; no all-green claim yet. Posted by Codex (AI) on behalf of @mikebridge. |
|
Follow-up on a4bb1f3: remote CI did not reproduce the earlier local placement passes. Run 35177483763 fails the geometry predicate at version-history-placement.spec.ts:139 (normal Chromium: 1280px; /app/prefix: both widths), including retries. This is a different failure from the old missing-column assertion, and the required aggregate is correctly red. A separate chart-create retry logs a duplicate-tag 422; that does not explain away the placement failures. Saved CI traces/screenshots are under investigation; no assertions have been weakened and no passing/merge-ready claim is intended. Reproduction requiring a production asset build must use the serial heavy queue. |
Nest the empty state in the dashboard content wrapper so two full-height siblings cannot extend page scrolling beyond the grid that contains history. CI geometry showed the panel clamped at y=-226 despite a correct 65px header offset. Preserve the existing placement assertions and keep scratch diagnostics out of this branch.
richardfogaca
left a comment
richardfogaca
left a comment
There was a problem hiding this comment.
Richard's agent here:
Reviewed 0fc41aecdd. My earlier closed-column height finding is resolved: the :empty collapse outside the media query removes the row-2 contribution, and the new Playwright spec pins closed-height parity against a flag-off baseline. CI is green on this head, including Playwright at 1920px and 1280px on both URL bases, which is the browser confirmation the earlier note asked for. Approving.
Also verified: the dashboard grid container is position: relative, so the absolutely-positioned grid item really does use its column 2 / row 2 area as containing block. That is what keeps the narrow overlay out of the header row and adds no width to the grid.
Two non-blocking notes:
-
Small suggestion: the capture-phase scroll listener runs
getBoundingClientRecton every scroll event from any scrollable descendant (chart tables, inner scroll containers) with no throttle. React bails out when the value is unchanged, but the layout read still happens per event. Could we coalesceupdateBottomOffsetthroughrequestAnimationFrame? Happy to keep as-is if the measured cost is negligible. -
The last commit moves
EmptyStateinsideDashboardContentWrapperfor every dashboard, not only when version history is enabled. The PR body does not mention it; a one-line note on why the empty state needs to live inside the content wrapper for the sticky geometry would help the next reader.
|
Bito Automatic Review Skipped – PR Already Merged |
SUMMARY
The sc-119737 overflow fix (#43836) kept the dashboard version-history panel inside the viewport below the XXL breakpoint by pinning it
position: fixedto the viewport at z-index 101. That made it a full-height right overlay anchored at the top of the screen: it covered the global Settings / + nav icons and the dashboard's Share / Edit dashboard / ⋯ controls, and disagreed with the chart panel, which opens inside the Explore content area (found on the QA re-check of sc-119737; SC-120489).This mirrors Explore. There, below its breakpoint the panel is anchored absolutely inside the relatively-positioned content container, below the header. The grid equivalent used here: below XXL the panel column becomes an absolutely-positioned grid item placed in the content cell (
grid-column: 2; grid-row: 2; top/right/bottom: 0) — an absolutely-positioned grid item uses its grid area as containing block — so it overlays the content only, adds no width to the grid (the overflow fix is preserved), and never covers the header row or the nav. It stacks at z-index 98, below the sticky header (99) and a maximized chart (102), so the header's controls stay visible and clickable. Because the dashboard page owns the scroll and the content cell is as tall as the dashboard, the panel itself (the<aside>) is sticky at the measured header height (barTopOffset, which the builder already tracks with aResizeObserver), so it never slides under the header while scrolling.At and above XXL the panel stays an in-flow third column, now in the content row only rather than spanning the header row — the same below-the-header placement as Explore on every breakpoint. (If design would rather the wide layout keep the panel beside the header, that is a one-line
grid-rowchange.)BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before (QA screenshot from SC-120489, ~1440px): the panel as a viewport overlay covering the nav and the Share / Edit / ⋯ controls — https://media.app.shortcut.com/api/attachments/files/clubhouse-assets/5d8baaab-e1a8-4512-a9c7-12979c8cd1c9/6aa829f5-1bbb-4b7a-9492-260c73c73207/Screenshot%202026-09-14%20at%2014.07.49.png
After: the panel opens inside the dashboard content area below the header; nav and dashboard controls remain in the document flow above it. Authenticated geometry checks passed at 1280px and 1920px on normal and URL-prefixed instances; no new screenshots are attached.
TESTING INSTRUCTIONS
The new test (
mounts the version-history panel inside the dashboard content area, below the header, on every breakpoint) asserts the column is inside the dashboard grid (not portaled todocument.body), that the header precedes it in the document flow, and the style rules per breakpoint:grid-row: 2+position: stickywide;position: absolutein column 2 at z-index 98 — neverfixed— with the aside's stickytopequal to the measured header height below XXL. Verified red-first: reverting the narrow rules to the fixed overlay fails the test at theposition: absoluteassertion. The sc-119737 closed-state:emptycontract test is unchanged and still passes.Manual: with
VersionHistoryenabled, open a dashboard's version history at ~1280px and ~1446px — the panel sits inside the content area below the header, the Settings / + icons and Share / Edit dashboard / ⋯ buttons are visible and clickable, nothing overflows horizontally, and scrolling the page keeps the panel in view below the header.Authenticated Chromium verification passed at 1920px and 1280px on both normal and
/placementURL bases (4 cases, no skips or retries). The dashboard placement spec isplaywright/tests/dashboard/version-history-placement.spec.ts. It checks closed-height parity against a flag-off baseline, a real clickable header menu, viewport bounds, actual scrolling from 0 to 240px and back, and restored height after closing. History API responses are real, not mocked.The visible header bottom is refreshed on scroll/resize, floored at the header height, and measured only when version history is enabled outside embedded mode. Tests also cover flag-off measurement, offset bounds and exact listener cleanup. The two component suites pass (59 tests), as do the changed-file pre-commit hooks including frontend type checking. Development build succeeds with the existing MapLibre warning; this is scoped verification, not a warning-free full-suite claim. The isolated backend still logs the separate activity cursor/INSERT issue tracked by #44261 while activity responses return 200.
Merged repaired master
4c77b0b924a9bd770110915eacd68f5619e8241d(#44364), resolving the conflict without adding a migration. Verified implementation commit:a4bb1f31aa1a9d79e0149854e0fc97d77de39665. Fresh GitHub CI remains authoritative for the published head.ADDITIONAL INFORMATION
VersionHistoryThis PR was developed with AI assistance (Claude Code and Codex); a human (@mikebridge) reviews before merge.