-
Notifications
You must be signed in to change notification settings - Fork 16.6k
fix: SQL Lab tab events #35105
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
fix: SQL Lab tab events #35105
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/SqlLab/utils/reduxStateToLocalStorageHelper.ts | ✅ |
| superset-frontend/src/SqlLab/types.ts | ✅ |
| superset-frontend/src/SqlLab/reducers/getInitialState.ts | ✅ |
| superset-frontend/src/SqlLab/fixtures.ts | ✅ |
| superset-frontend/src/core/sqlLab.ts | ✅ |
| superset-frontend/src/SqlLab/actions/sqlLab.js | ✅ |
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.
villebro
left a comment
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.
LGTM with a minor test fixture comment. I'm curious, do you know wht the ids are mutable? I would have thought the main id would stay unchanged throughout the tab's lifecycle.
I think it's because they are updated when the tab is saved on the server. @justinpark will probably know better but I'm hoping that we can improve the logic now that an immutable id is present and maybe keep the remote id in a separate variable. |
villebro
left a comment
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.
Approving but hoping for remaining fixtures to also be updated!
ae193c8 to
59401a7
Compare
(cherry picked from commit e729b2d)
(cherry picked from commit e729b2d)
SUMMARY
This adds a unique
immutableIdfield to theQueryEditortype that persists throughout the tab's lifecycle, enabling reliable event filtering and tab identification for SQL Lab extensions while maintaining backward compatibility. Previously, SQL Lab tabs could only be identified by their mutable id field, which made it difficult for extensions to reliably track events and maintain stable references to specific editor instances across state changes and persistence operations.The addition of
immutableIdfor tabs might help improving the current logic that uses mutableidandtabViewIdintroduced in #34720 to reduce re-renders. I'll let @justinpark analyze the code and check if things can be improved in a follow-up PR.TESTING INSTRUCTIONS
Check that the events are triggered for the correct tabs.
ADDITIONAL INFORMATION