-
Notifications
You must be signed in to change notification settings - Fork 15.8k
fix(DatasourceModal): replace imperative modal updates with declarative state #35256
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've completed my review and didn't find any issues.
Files scanned
File Path | Reviewed |
---|---|
superset-frontend/src/dashboard/components/EmbeddedModal/index.tsx | ✅ |
superset-frontend/src/components/Datasource/DatasourceModal/index.tsx | ✅ |
superset-frontend/src/pages/ThemeList/index.tsx | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Check out our docs on how you can make Korbit work best for you and your team.
…ve state Fixes DOM error "Failed to execute 'insertBefore' on 'Node'" by replacing imperative modal.update() calls with React state management. Changes: - Convert syncColumnsRef to syncColumns state variable - Remove confirmModal state and imperative update calls - Simplify modal creation to not store instance - Add tests to verify useModal hook usage and prevent regressions - Remove unused useRef import The modal content now updates declaratively through React re-renders instead of direct DOM manipulation, eliminating insertion conflicts. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
ebbf139
to
b44ec86
Compare
🎪 Showtime deployed environment on GHA for b44ec86 • Environment: http://52.12.3.102:8080 (admin/admin) |
superset-frontend/src/components/Datasource/DatasourceModal/DatasourceModal.useModal.test.tsx
Outdated
Show resolved
Hide resolved
Follow testing guidelines to avoid nesting when testing. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
Summary
Fixes DOM error
NotFoundError: Failed to execute 'insertBefore' on 'Node'
in DatasourceModal by replacing imperative modal updates with declarative React state management.Problem
The DatasourceModal component was storing modal instances from
Modal.useModal()
and calling.update()
on them imperatively. This caused DOM manipulation conflicts with React's reconciliation process, leading to the error when React tried to insert DOM nodes that were no longer children of their expected parents.Solution
syncColumnsRef
tosyncColumns
state variable for proper React reactivityconfirmModal.update()
calls that manipulated DOM outside React's controlgetSaveDialog
callbackTest Plan
Modal.useModal
hook usage instead of directModal.confirm
Technical Details
Before: Modal content was updated imperatively:
After: Modal content updates declaratively:
🤖 Generated with Claude Code