fix(ui): preserve default modal resize handles when config is partial - #43806
fix(ui): preserve default modal resize handles when config is partial#43806deepujain wants to merge 3 commits into
Conversation
Code Review Agent Run #249c7eActionable Suggestions - 0Additional Suggestions - 1
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 |
There was a problem hiding this comment.
🟢 Approval recommended
The change is scoped, matches the stated issue/root cause, and includes focused unit tests covering the new merge behavior.
Pull request overview
Fixes resizable/draggable modal “drift” by ensuring callers that pass a partial resizableConfig don’t accidentally discard the default resize-handle enable guards in the shared Modal component.
Changes:
- Add
mergeResizableConfig()helper to merge callerresizableConfigoverrides withdefaultResizableConfig, preserving default resize-handle restrictions unless explicitly overridden. - Update
Modalto use the merged config viauseMemo. - Add unit tests validating default-handle preservation and explicit
enableoverrides.
File summaries
| File | Description |
|---|---|
| superset-frontend/packages/superset-ui-core/src/components/Modal/Modal.tsx | Merges default resizable settings with caller overrides to preserve resize-handle guards in draggable modals. |
| superset-frontend/packages/superset-ui-core/src/components/Modal/Modal.test.tsx | Adds targeted tests for the new merge behavior to prevent regressions. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Adding an explicit return type to To implement this, you can import the export function mergeResizableConfig(
hideFooter: boolean | undefined,
overrides: ModalProps['resizableConfig'] = {},
): ModalProps['resizableConfig'] {
const defaults = defaultResizableConfig(hideFooter);
// ... implementation
}superset-frontend/packages/superset-ui-core/src/components/Modal/Modal.tsx |
753a9c3 to
6bdc758
Compare
Code Review Agent Run #81ab04Actionable 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 |
a708e0c to
8159ee0
Compare
Code Review Agent Run #5bdff6Actionable 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #43806 +/- ##
=======================================
Coverage 79.45% 79.46%
=======================================
Files 2895 2895
Lines 168167 168175 +8
Branches 38995 38999 +4
=======================================
+ Hits 133624 133633 +9
+ Misses 32044 32043 -1
Partials 2499 2499
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:
|
a4c36bb to
f820ab1
Compare
|
Fixed the current frontend type-check failure by making Validated on the pushed change:
|
49d9671 to
cb209a8
Compare
Code Review Agent Run #7e9f9bActionable 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 |
cb209a8 to
53803d0
Compare
Code Review Agent Run #c9bc85Actionable Suggestions - 0Additional Suggestions - 1
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 |
Drill and chart modals pass only min/defaultSize in resizableConfig, which dropped the default enable map and exposed all handles, causing drift from non-bottom-right corners. Merge overrides with defaults instead. Fixes apache#43320
Guard ResizableProps.enable === false so partial merges do not silently re-enable default handles. Add regression test and explicit return type. Signed-off-by: Deepak Jain <deepujain@gmail.com>
The helper always returns a concrete config, so expose that contract to keep strict TypeScript checks from treating test and caller dereferences as optional. Signed-off-by: Deepak Jain <deepujain@gmail.com>
53803d0 to
339fc44
Compare
|
The latest Bito additional suggestion about |
|
|
||
| const isDragged = () => !!document.querySelector('.react-draggable-dragged'); | ||
|
|
||
| describe('mergeResizableConfig', () => { |
There was a problem hiding this comment.
These tests only call the extracted helper, so they still pass if CustomModal stops passing the merged configuration to Resizable; partial callers would then regain the guarded top/left handles. Could this render Modal with the partial caller config and assert the rendered handle set?
Code Review Agent Run #c2cd37Actionable 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 |
SUMMARY
Drill to detail, Drill by, and View as table modals pass a partial
resizableConfig(min size and default height only).Modalreplaced the full default config with that partial object, which dropped theenablemap that restricts resize handles to bottom, bottom-right, and right.With all handles enabled inside a draggable wrapper, resizing from top or left edges drifts and anchors from the wrong corner (#43320).
This change merges caller overrides with the default resizable config so handle guards stay in place unless explicitly overridden.
TESTING INSTRUCTIONS
npm test -- --testPathPattern=packages/superset-ui-core/src/components/Modal/Modal.test.tsxADDITIONAL INFORMATION
Fixes #43320