fix(core): let UnsavedChangesModal use Ant Design's automatic z-index stacking - #42548
fix(core): let UnsavedChangesModal use Ant Design's automatic z-index stacking#42548rusackas wants to merge 5 commits into
Conversation
Code Review Agent Run #df5c7bActionable 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 |
|
The flagged issue is correct. The current regression test renders both modals simultaneously with To resolve this, update the test to render the Would you like me to implement this fix for you? I can also check the other comments on this PR if you would like me to address them as well. superset-frontend/packages/superset-ui-core/src/components/UnsavedChangesModal/UnsavedChangesModal.test.tsx |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
… stacking UNSAVED_CHANGES_MODAL_Z_INDEX was a hardcoded literal meant to keep this modal above other open modals (e.g. a draggable View query modal). Ant Design already auto-increments z-index for every newly opened Modal off theme.zIndexPopupBase, so any manual override just needs to be bumped again whenever something else's computed z-index creeps past it, which is exactly what happened (#42510). Audited every other zIndex/z-index usage in the frontend: this was the only place hardcoding a z-index override on a Modal-class component. Everything else either doesn't need one, or already derives it from theme.zIndexPopupBase/theme.zIndexBase for a real, local reason (sticky headers inside modal content, the native Fullscreen API's interaction with portaled antd components, custom fixed-position overlays that sit outside Ant Design's own stacking system entirely). None of those are part of this bug and are left untouched. Since this modal is always opened on top of whatever it's interrupting, dropping the override (the constant, the prop, and the pass-through) lets it stack correctly with no number to maintain.
82f9fd7 to
a0851f3
Compare
Ant Design applies the automatically-assigned stacking z-index to the .ant-modal-wrap element, not to the role="dialog" element, so getComputedStyle on the dialog itself returned an empty string. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Code Review Agent Run #b3df60Actionable 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 |
…dex test
rc-util's useId hook always returns the mocked string "test-id" in test
environments, so when two Ant Design modals are open at once their
aria-labelledby ids collide and getByRole('dialog', { name }) can't
distinguish them. Match dialogs by their title text instead.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Code Review Agent Run #c72ef5Actionable 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 |
| }: UnsavedChangesModalProps): ReactElement => ( | ||
| <Modal | ||
| centered | ||
| responsive |
There was a problem hiding this comment.
Ant Design 6.5.1 only renders a higher modal z-index when it inherits a ZIndexContext; this top-level sibling gets no inline z-index, while the View query modal can inherit the dropdown's elevated context. That can leave the unsaved-changes prompt behind the existing modal, so could this use a strategy that is actually above the active overlay context?
There was a problem hiding this comment.
Good catch, fixed with destroyOnHidden. These two modals are always top-level siblings tied on z-index, so the real fix is making sure this one's wrap node always ends up later in the DOM on every open, not trying to out-rank a value that's already tied.
| handleSave: () => void; | ||
| onConfirmNavigation: () => void; | ||
| title?: string; | ||
| body?: string; |
There was a problem hiding this comment.
This component is exported from @superset-ui/core/components, so removing zIndex breaks downstream TypeScript callers and silently drops their overlay override at runtime. Could we keep the optional pass-through while removing only the hardcoded default?
There was a problem hiding this comment.
Checked, nothing in the codebase passes zIndex to this component so there's no compile break there. Keeping the passthrough would let a caller reintroduce the same hardcoded-override footgun this PR is fixing, so I'd rather not bring it back. Added a note in UPDATING.md for any external consumer that was relying on it.
| getComputedStyle(unsavedChangesWrap as HTMLElement).zIndex, | ||
| ); | ||
|
|
||
| expect(unsavedChangesZIndex).toBeGreaterThan(otherZIndex); |
There was a problem hiding this comment.
This assertion currently fails in CI because both computed values are NaN, and it also would not distinguish the old implementation in a browser: the hardcoded 1300 still beats this plain sibling's default layer. Could the regression test recreate the elevated View SQL modal/context so it fails when the hardcoded zIndex is restored and passes with the intended fix?
There was a problem hiding this comment.
Good catch, the test's rewritten now. It renders a second modal and checks DOM order via compareDocumentPosition against .ant-modal-wrap instead of comparing raw zIndex values (which were both NaN, as you found). Fails against the old hardcoded implementation and passes with destroyOnHidden.
… true open-recency Dropping the hardcoded z-index (previous commit) fixes the simple case, but not the general one: two sibling Modals always fall back to the same static z-index and are tie-broken by DOM order, and Ant Design's Modal portal node is otherwise created once, lazily, on first open, and never moves again. If this modal is ever opened once before whatever it's meant to interrupt is opened for the first time, a later reopen goes right back to that stale, now-too-early DOM position and renders behind it again -- reproducing the original bug even with no z-index anywhere. destroyOnHidden tears the portal down on every close so every open recreates it at the end of the document, making DOM order (and thus stacking) always match true open-recency. Replaces the failing regression test (which asserted z-index magnitude, an invariant sibling modals were never guaranteed to hold, and which jsdom can't resolve anyway) with two DOM-order assertions: the original scenario, and the reopen-after-prior-open scenario that only the destroyOnHidden fix actually covers. Adds a permanent Storybook story with a toggle to visually reproduce the bug and the fix side by side.
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #42548 +/- ##
==========================================
- Coverage 65.33% 65.33% -0.01%
==========================================
Files 2803 2803
Lines 158490 158488 -2
Branches 36178 36177 -1
==========================================
- Hits 103557 103554 -3
- Misses 52922 52923 +1
Partials 2011 2011
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 #f27121Actionable 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 |
Addresses review feedback on #42548 about downstream API compat -- document the removed prop instead of re-adding a passthrough that would reintroduce the hardcoded z-index footgun this PR fixes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
3 participants
SUMMARY
Alternate fix for #42510, following up on the discussion in #42546. That PR bumps
UNSAVED_CHANGES_MODAL_Z_INDEXfrom 1100 to 1300 to clear a specific case where the View SQL modal (dynamically z-indexed by Ant Design to around 1200) rendered on top of the unsaved-changes dialog. The problem with bumping the constant is that 1200 isn't a fixed ceiling, it's whatever Ant Design'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 later.This drops
UNSAVED_CHANGES_MODAL_Z_INDEX(and thezIndexprop entirely, nothing ever passed a custom value) instead of bumping it. Ant Design already auto-increments z-index for each newly openedModal, offtheme.zIndexPopupBase, but only in one specific case: when the newModalis nested inside another currently openModal's React tree (seeuseZIndexinantd).UnsavedChangesModaland whatever it's interrupting (the View query modal, an in-progress form, etc.) are always React siblings, never nested in each other, so they both fall back to the same static z-index and are tie-broken by DOM order instead: whichever modal's DOM node was inserted later paints on top.That's fine on its own (later-inserted wins is exactly what we want), except Ant Design's
Modalonly creates its portal DOM node once, lazily, on first open, and then never removes or recreates it, even across later closes/reopens (destroyOnHiddendefaults tofalse). So ifUnsavedChangesModalis ever opened once, anywhere in the app, before the thing it's meant to interrupt is opened for the first time, a later reopen goes right back to that stale, now-too-early DOM position and renders behind it again, reproducing the original bug with no z-index anywhere in sight. Just dropping the hardcoded z-index fixes the specific repro from #42510 (View query opened first, then triggers the unsaved-changes dialog) but not this more general reopen-order case.Setting
destroyOnHiddenonUnsavedChangesModal's internalModalfixes that: it tears the portal DOM node down on every close, so every open recreates it fresh at the end ofdocument.body. DOM order then always tracks true open-recency, and stacking (which falls back to DOM order whenever z-index is tied, which it always is between siblings here) is correct with zero manual z-index management, hardcoded or otherwise. Worth notingModalTrigger(used for the View query modal itself) already defaultsdestroyOnHiddentotrueinternally; this bringsUnsavedChangesModal, built directly on the baseModal, in line with whatModalTrigger-based modals already do.Also audited every other
zIndex/z-indexusage in the frontend to see if this was a wider pattern. It isn't: this was the only place hardcoding a z-index override on a Modal-class component. The rest either don't need one, or already derive it fromtheme.zIndexPopupBase/theme.zIndexBasefor real, local reasons unrelated to this bug (sticky headers inside modal content that need to stay above scrolled sibling content, the native Fullscreen API's interaction with portaled antd components, custom fixed-position overlays like the toast and chat widgets that sit outside Ant Design's own stacking system entirely). None of those needed to change.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A. The original bug's before/after screenshots are already posted on #42546. Added a permanent Storybook story (
Modal.stories.tsx,SiblingModalStacking) with a toggle that reproduces the bug and the fix side-by-side, for interactive visual verification instead.TESTING INSTRUCTIONS
Two regression tests, both asserting DOM order (not z-index magnitude, which two tied siblings were never actually guaranteed to differ on, and which jsdom doesn't reliably resolve anyway):
UnsavedChangesModalopens and ends up later in the DOM.destroyOnHiddenfix specifically covers:UnsavedChangesModalopened and closed once before the other modal is ever opened, then the other modal opens, thenUnsavedChangesModalreopens and still ends up later in the DOM. I verified this test actually fails without thedestroyOnHiddenfix (reverted it locally, confirmed red, restored it) so it isn't a tautology.Manually, or via the new Storybook story: open a dashboard or chart, open the View query/View SQL modal, then trigger an action that shows the unsaved-changes dialog (e.g. edit a control, then try to navigate away). The dialog should render on top, including if you've triggered the unsaved-changes dialog once earlier in the session before ever opening View query.
ADDITIONAL INFORMATION
Disclosure: the local
type-checking-frontendpre-commit hook was skipped (SKIP=type-checking-frontend) for this commit — this worktree'stsc --buildfails on pre-existing TS2742 "inferred type... not portable" errors in unrelated files (Tabs.tsx,SafeMarkdown.tsx,spec/index.tsx), not touched by this change. A targeted typecheck scoped to just the files this PR touches is clean. All other local hooks (prettier, oxlint, custom-rules-frontend, stylelint) passed. CI'sType-Checking (Frontend)job is the real gate for this.