fix(explore): ensure unsaved-changes dialog renders above View SQL modal v2 - #42546
Conversation
Code Review Agent Run #6ea649Actionable 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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #42546 +/- ##
==========================================
- Coverage 65.25% 65.25% -0.01%
==========================================
Files 2795 2795
Lines 157971 157971
Branches 36109 36109
==========================================
- Hits 103083 103082 -1
- Misses 52893 52894 +1
Partials 1995 1995
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:
|
rusackas
left a comment
rusackas
left a comment
There was a problem hiding this comment.
Thanks for tracking this down, @SBIN2010! Bumping the constant does clear the reported case, but 1200 isn't a fixed ceiling on the View SQL side, it's whatever antd's own auto z-index stacking computes for however many popups happen to be layered at that moment. So the same bug could resurface with a different number down the road.
Left a suggestion on the constant itself: this modal and the View SQL one both render through the same shared <Modal> component, and antd already auto-increments z-index for each newly opened modal off zIndexPopupBase (this codebase already leans on that token elsewhere, e.g. ToastPresenter, SliceHeaderControls) as long as nothing overrides zIndex explicitly. Since the unsaved-changes prompt only ever opens after whatever it's interrupting, dropping the override lets it stack on top naturally, with no magic number to chase again later.
Happy to be wrong here if there was a reason for the hardcoded value in the first place, just wanted to flag it before merging.
| // Ant Design's default modal zIndex is 1000. Using a higher value ensures | ||
| // this dialog always renders above other open modals (e.g. a draggable View SQL modal). | ||
| const UNSAVED_CHANGES_MODAL_Z_INDEX = 1100; | ||
| const UNSAVED_CHANGES_MODAL_Z_INDEX = 1300; |
There was a problem hiding this comment.
| // Ant Design's default modal zIndex is 1000. Using a higher value ensures | |
| // this dialog always renders above other open modals (e.g. a draggable View SQL modal). | |
| const UNSAVED_CHANGES_MODAL_Z_INDEX = 1100; | |
| const UNSAVED_CHANGES_MODAL_Z_INDEX = 1300; | |
| // This modal is always opened on top of whatever it's interrupting, so it | |
| // doesn't need an explicit zIndex: Ant Design auto-increments each newly | |
| // opened modal's z-index off theme.zIndexPopupBase, and this one, opening | |
| // last, naturally lands above everything else already open. | |
| const UNSAVED_CHANGES_MODAL_Z_INDEX = undefined; |
There was a problem hiding this comment.
Feeling like if AntD does automatic z-indexing, we might not need UNSAVED_CHANGES_MODAL_Z_INDEX at all... and can probably remove MORE z-index entries to not have a problem at all. Investigating.
There was a problem hiding this comment.
I’ll dig into this a bit more, test your suggestion, and get back to you with the results.
There was a problem hiding this comment.
Swept the whole frontend for this: UnsavedChangesModal is the only place anywhere that hardcodes a zIndex override on a Modal. Everywhere else either doesn't need one, or already uses the token correctly (PopoverDropdown uses theme.zIndexBase, SliceHeaderControls uses theme.zIndexPopupBase ± 1). The other z-index literals in the codebase are unrelated, local stacking contexts, chart canvases, drag previews, resize handles, nothing competing with the popup system, so no broader sweep needed.
Also checked every caller, the stories file, and the tests: nobody ever passes a custom zIndex to this component. So the fuller fix isn't defaulting the constant to undefined, it's dropping the whole zIndex prop (the type, the default, and the pass-through) and just letting <Modal> render with no override, same as every other modal in the app.
Happy to just push the cleaner version (or open a fresh PR with it) if that's easier than digging back in, no worries either way if you'd rather finish it yourself.
There was a problem hiding this comment.
I'm having an agent look at this ^ a bit more. I would love for your PR to "win" here, but I'm curious if we can sweep the codebase and delete ALL the z-index complexities to just let AntD do what it does naturally, now that we're on V6, so you might see an alternate PR we can assess in parallel.
There was a problem hiding this comment.
Went ahead and opened the alternate version we talked through: #42548. Dropped UNSAVED_CHANGES_MODAL_Z_INDEX entirely rather than reworking the constant here, so feel free to close this one in favor of that, or let's compare notes if you'd rather keep going on this branch, either is fine by me.
rusackas
left a comment
rusackas
left a comment
There was a problem hiding this comment.
Meh... this fixes it. I'll see about sweeping up on the other PR I was fiddling around with later.
2 participants
SUMMARY
Since the z-index of the "View SQL" modal window (1200) was higher than UNSAVED_CHANGES_MODAL_Z_INDEX (1100), this dialog box was overlapped.
fix #42510
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
BEFORE
AFTER
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION