fix(core): align Drill to detail table headers with scrollable body - #43472
fix(core): align Drill to detail table headers with scrollable body#43472anishtilekar wants to merge 5 commits into
Conversation
VirtualTable renders its header as a plain antd <thead> separate from the react-window Grid that renders the body. When the Grid's vertical scrollbar appears, it eats into the body's visible column width without shrinking the header, so header and data columns drift out of alignment - most noticeable in the Drill to detail modal, which always paginates and often needs to scroll. Reserve the scrollbar's width when computing column widths whenever the current page's rows would overflow the visible height, mirroring the equivalent fix already applied to the non-virtualized table's sticky header in useSticky.tsx. Fixes apache#43383
…il-header-alignment
dosubot
Bot
added
change:frontend
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #e49b9f
Actionable Suggestions - 1
-
superset-frontend/packages/superset-ui-core/src/components/Table/utils/getScrollBarSize.ts - 1
- Duplicate measureScrollBarSize logic · Line 1-59
Additional Suggestions - 1
-
superset-frontend/packages/superset-ui-core/src/components/Table/VirtualTable.test.tsx - 1
-
Mock cleanup placement · Line 172-172The `resizeSpy.mockRestore()` call at line 172 is correctly placed to clean up the spy. However, if more tests are added that mock useResizeDetector, consider moving the cleanup to a beforeEach/afterEach block to ensure isolation.
-
Review Details
-
Files reviewed - 4 · Commit Range:
014cc0f..014cc0f- superset-frontend/packages/superset-ui-core/src/components/Table/VirtualTable.test.tsx
- superset-frontend/packages/superset-ui-core/src/components/Table/VirtualTable.tsx
- superset-frontend/packages/superset-ui-core/src/components/Table/utils/getScrollBarSize.test.ts
- superset-frontend/packages/superset-ui-core/src/components/Table/utils/getScrollBarSize.ts
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- Eslint (Linter) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers an incremental AI Review. -
/review full- Manually triggers a full AI Review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
| /** | ||
| * Licensed to the Apache Software Foundation (ASF) under one | ||
| * or more contributor license agreements. See the NOTICE file | ||
| * distributed with this work for additional information | ||
| * regarding copyright ownership. The ASF licenses this file | ||
| * to you under the Apache License, Version 2.0 (the | ||
| * "License"); you may not use this file except in compliance | ||
| * with the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, | ||
| * software distributed under the License is distributed on an | ||
| * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
| * KIND, either express or implied. See the License for the | ||
| * specific language governing permissions and limitations | ||
| * under the License. | ||
| */ | ||
|
|
||
| let cached: number | undefined; | ||
|
|
||
| const css = (x: TemplateStringsArray) => x.join('\n'); | ||
|
|
||
| function measureScrollBarSize() { | ||
| const inner = document.createElement('div'); | ||
| const outer = document.createElement('div'); | ||
| inner.style.cssText = css` | ||
| width: auto; | ||
| height: 100%; | ||
| overflow: scroll; | ||
| `; | ||
| outer.style.cssText = css` | ||
| position: absolute; | ||
| visibility: hidden; | ||
| overflow: hidden; | ||
| width: 100px; | ||
| height: 50px; | ||
| `; | ||
| outer.append(inner); | ||
| document.body.append(outer); | ||
| const size = outer.clientWidth - inner.clientWidth; | ||
| outer.remove(); | ||
| return size; | ||
| } | ||
|
|
||
| // Measures the browser/OS native scrollbar width. `VirtualTable`'s | ||
| // react-window `Grid` body doesn't apply any custom `::-webkit-scrollbar` | ||
| // styling, so its actual vertical scrollbar always renders at this native | ||
| // size - used to keep the (separately rendered) header in sync with how | ||
| // much horizontal space the scrollbar steals from the body's columns. | ||
| export default function getScrollBarSize(forceRefresh = false) { | ||
| if (typeof document === 'undefined') { | ||
| return 0; | ||
| } | ||
| if (cached === undefined || forceRefresh) { | ||
| cached = measureScrollBarSize(); | ||
| } | ||
| return cached; | ||
| } |
There was a problem hiding this comment.
Lines 20-58 (the entire implementation) are a semantic duplicate of plugins/plugin-chart-table/src/DataTable/utils/getScrollBarSize.ts (lines 23-70). The only substantive difference is removal of the custom-scrollbar API. This duplication creates maintenance risk — both copies must stay in sync if measureScrollBarSize changes. The existing implementation already handles the non-custom case used by VirtualTable; the caller at line 171 of VirtualTable.tsx does not need custom scrollbar behavior. A single shared implementation (with re-export if needed) eliminates divergence risk.
Code Review Run #e49b9f
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| const rowCount = pageSize ?? dataSource?.length ?? 0; | ||
| const hasVerticalScroll = rowCount * cellSize > availableHeight; | ||
| const scrollBarSize = hasVerticalScroll ? getScrollBarSize() : 0; |
There was a problem hiding this comment.
Suggestion: hasVerticalScroll only compares row content height with the nominal Grid height. When the columns are wider than the Grid, a horizontal scrollbar consumes part of the vertical viewport; rows that otherwise exactly fit can then trigger a vertical scrollbar as well. Because that case is not included here, the header does not reserve the vertical scrollbar width and becomes misaligned. Determine scrollbar presence using both horizontal and vertical overflow, including the scrollbar footprint. [incorrect condition logic]
Severity Level: Major ⚠️
- ⚠️ Wide virtualized tables can gain an unreserved vertical scrollbar.
- ⚠️ Header cells misalign when horizontal overflow reduces Grid height.
- ⚠️ Drill-detail columns may appear clipped or shifted.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/packages/superset-ui-core/src/components/Table/VirtualTable.tsx
**Line:** 169:171
**Comment:**
*Incorrect Condition Logic: `hasVerticalScroll` only compares row content height with the nominal Grid height. When the columns are wider than the Grid, a horizontal scrollbar consumes part of the vertical viewport; rows that otherwise exactly fit can then trigger a vertical scrollbar as well. Because that case is not included here, the header does not reserve the vertical scrollbar width and becomes misaligned. Determine scrollbar presence using both horizontal and vertical overflow, including the scrollbar footprint.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
There was a problem hiding this comment.
Agreed—the horizontal scrollbar reduces the Grid's client height. A page whose rows fit the outer height can then gain a vertical scrollbar with no header reservation, recreating the alignment problem. Should the detection include the horizontal scrollbar footprint?
There was a problem hiding this comment.
Yes. The detection should account for the horizontal scrollbar’s height before deciding whether the vertical scrollbar is present.
Use the Grid’s actual column width to detect horizontal overflow, then reduce the effective height by the scrollbar footprint:
const scrollBarSize = getScrollBarSize();
const hasHorizontalScroll = totalWidth > tableWidth;
const effectiveHeight =
availableHeight - (hasHorizontalScroll ? scrollBarSize : 0);
const hasVerticalScroll = rowCount * cellSize > effectiveHeight;
const columnSizingWidth = Math.max(
tableWidth - (hasVerticalScroll ? scrollBarSize : 0),
0,
);Because totalWidth currently depends on columnSizingWidth, the width calculation should be structured in two passes (or otherwise determine horizontal overflow from the unadjusted column widths) to avoid a circular dependency. This covers the edge case where rows exactly fit the nominal height but a horizontal scrollbar reduces the Grid’s client height and causes vertical overflow.
Codecov Report❌ Patch coverage is
❌ Your project check has failed because the head coverage (99.97%) is below the target coverage (100.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## master #43472 +/- ##
=======================================
Coverage 78.86% 78.87%
=======================================
Files 2876 2877 +1
Lines 164623 164648 +25
Branches 38019 38028 +9
=======================================
+ Hits 129835 129861 +26
+ Misses 32345 32344 -1
Partials 2443 2443
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:
|
Code Review Agent Run #9f46fdActionable Suggestions - 0Filtered 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 |
…il-header-alignment
508250f to
8717947
Compare
There was a problem hiding this comment.
Code Review Agent Run #5493ea
Actionable Suggestions - 1
-
superset-frontend/packages/superset-ui-core/src/components/Table/VirtualTable.tsx - 1
- Incomplete size enum mapping · Line 152-152
Review Details
-
Files reviewed - 4 · Commit Range:
cc8ded7..8717947- superset-frontend/packages/superset-ui-core/src/components/Table/VirtualTable.test.tsx
- superset-frontend/packages/superset-ui-core/src/components/Table/VirtualTable.tsx
- superset-frontend/packages/superset-ui-core/src/components/Table/utils/getScrollBarSize.test.ts
- superset-frontend/packages/superset-ui-core/src/components/Table/utils/getScrollBarSize.ts
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- Eslint (Linter) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers an incremental AI Review. -
/review full- Manually triggers a full AI Review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
| const { ref } = useResizeDetector({ onResize }); | ||
| const theme = useTheme(); | ||
|
|
||
| const cellSize = size === TableSize.Middle ? MIDDLE : SMALL; |
There was a problem hiding this comment.
size === TableSize.Middle ? MIDDLE : SMALL silently maps both TableSize.Small and TableSize.Large to 39px, but TableSize.Large is a distinct, user-visible size that should render a taller row. If Large is intentionally collapsed into Small's height, document it; otherwise branch on TableSize.Large or adjust the default accordingly. Also note dataSource is already extracted at line 142, so moving cellSize to after the destructuring block is safe.
Code Review Run #5493ea
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| pagination && typeof pagination === 'object' | ||
| ? pagination.pageSize | ||
| : undefined; | ||
| const rowCount = pageSize ?? dataSource?.length ?? 0; |
There was a problem hiding this comment.
When a paginated table's final page contains fewer rows than pagination.pageSize, this reserves scrollbar width for the configured page capacity even though the mounted rows fit. The header then becomes narrower than the body and reintroduces the alignment problem on short pages. Should this use the actual mounted row count when determining vertical overflow?
2 participants
SUMMARY
In the Drill to detail modal,
VirtualTable(used byDrillDetailPane) renders its header as a plain antd<thead>inside a<table>, separate from thereact-windowGridthat renders the body rows. Column widths for both are derived from the same measured container width.When the current page's rows are tall enough to need vertical scrolling, the Grid's own scrollbar eats into the body's visible column width, but the header isn't shrunk to match, so header cells drift out of alignment with their data columns (and can appear clipped/truncated). This is the same class of bug already fixed for the non-virtualized table's sticky header in
useSticky.tsx(#36190, #36891, #42573), butVirtualTable.tsxuses a completely different header/body split and wasn't covered by those fixes.This PR reserves the scrollbar's width when sizing columns whenever the current page's row count would overflow the visible height, so the header and the Grid's columns stay in sync - mirroring the approach used in
useSticky.tsx.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A - the bug only reproduces with a live scrollable dataset; see reproduction steps in the linked issue.
TESTING INSTRUCTIONS
npx jest packages/superset-ui-core/src/components/Table/VirtualTable.test.tsx packages/superset-ui-core/src/components/Table/utils/getScrollBarSize.test.tsfromsuperset-frontend/.ADDITIONAL INFORMATION