fix(dashboard): stretch Chart Data modal results grid to fill available height - #43454
fix(dashboard): stretch Chart Data modal results grid to fill available height#43454EnxDev wants to merge 1 commit into
Conversation
Code Review Agent Run #de4f11Actionable 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 |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| <Tabs | ||
| fullHeight | ||
| activeKey={activeTabKey} | ||
| onChange={setActiveTabKey} | ||
| items={items} |
There was a problem hiding this comment.
Suggestion: When the number of panes decreases, activeTabKey can still reference a removed tab such as results 2. Since this component now passes that stale key to Tabs, no item is active and the dashboard results area renders blank. Reconcile the active key with resultsPanes and fall back to ResultTypes.Results, as the sibling DataTablesPane does. [incorrect condition logic]
Severity Level: Major ⚠️
- ❌ Chart Data modal can render blank after result panes shrink.
- ⚠️ Mixed and dynamic-query charts can lose remaining results.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/explore/components/DataTablesPane/components/ResultsPaneOnDashboard.tsx
**Line:** 91:95
**Comment:**
*Incorrect Condition Logic: When the number of panes decreases, `activeTabKey` can still reference a removed tab such as `results 2`. Since this component now passes that stale key to `Tabs`, no item is active and the dashboard results area renders blank. Reconcile the active key with `resultsPanes` and fall back to `ResultTypes.Results`, as the sibling `DataTablesPane` does.
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
|
The flagged issue is correct. When the number of result panes decreases, the Here is a suggested fix: const activeKeyExists = items.some(item => item.key === activeTabKey);
const safeActiveKey = activeKeyExists ? activeTabKey : ResultTypes.Results;
return (
<Wrapper>
<Tabs
fullHeight
activeKey={safeActiveKey}
onChange={setActiveTabKey}
items={items}
/>
</Wrapper>
);There are no other comments on this PR to address. Would you like me to perform any other analysis? superset-frontend/src/explore/components/DataTablesPane/components/ResultsPaneOnDashboard.tsx |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #43454 +/- ##
=======================================
Coverage 78.86% 78.86%
=======================================
Files 2876 2876
Lines 164601 164601
Branches 38015 38015
=======================================
Hits 129806 129806
Misses 32348 32348
Partials 2447 2447
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:
|
msyavuz
left a comment
msyavuz
left a comment
There was a problem hiding this comment.
Height chain resolves after this change, and the Explore pane is unaffected since SouthPane already sets .ant-tabs-content.
| <Wrapper> | ||
| <Tabs activeKey={activeTabKey} onChange={setActiveTabKey} items={items} /> | ||
| <Tabs | ||
| fullHeight |
There was a problem hiding this comment.
fullHeight already emits height: 100% on .ant-tabs and .ant-tabs-body, which Wrapper also hardcodes at L31-37 — drop those two rules? .ant-tabs-content was the only missing link.
| activeKey={activeTabKey} | ||
| onChange={setActiveTabKey} | ||
| items={items} | ||
| /> |
There was a problem hiding this comment.
Worth a toHaveStyleRule('height', '100%', { target: '.ant-tabs-content' }) here so removing fullHeight fails loudly? The fix is one prop with no jsdom-observable behavior.
2 participants
SUMMARY
The "View as table" (Chart Data) modal on a dashboard chart could render with its results grid collapsed to a thin sliver, with the "Edit chart"/ "Close" buttons sitting on top of or immediately below it instead of below a properly filled grid.
Root cause:
ResultsPaneOnDashboard's<Tabs>didn't passfullHeight(unlike its sibling usages inTabsRenderer/DashboardContainer), so.ant-tabs-contentnever receivedheight: 100%and collapsed to itsintrinsic size.
Because the collapsed container reports a height of
0to theResizeObserverinuseGridHeight, and that hook only updatesits state
if (h > 0), the results grid silently fell back to a hardcoded 400px height that's disconnected from the modal's real available space. In a large default-sized modal this just wasted blank space; once the modal is smaller than 400px of available body height (e.g. resized down), the grid overflows and visually collides with the footer buttons.Fix: pass
fullHeightto the<Tabs>inResultsPaneOnDashboard,restoring the height chain so the grid's container always reports its real size and the grid tracks it correctly at any modal size.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
before_after_resize_comparison.mp4
TESTING INSTRUCTIONS
dashboard.
"View as table" (Chart Data).
overflowing into the footer buttons, at both default and resized
modal heights.
ADDITIONAL INFORMATION