Repository navigation
Conversation
rusackas
requested review from
eschutho,
geido,
kgabryje,
michael-s-molina and
villebro
as code owners
June 22, 2026 16:10
dosubot
Bot
added
change:frontend
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
Contributor
|
The flagged issue is correct. Exposing internal backend integer IDs in a public API can lead to security risks and tight coupling between the frontend and backend data models. To resolve this, you should use a UUID or a non-exported internal identifier for the tab state. Recommended FixIn /**
* The stable public identifier for this tab.
*/
publicId: string;If you need to map this to the backend ID, keep the backend ID as a private or internal property that is not exposed through the public extension API. Would you like me to fetch all other comments on this PR to validate and implement fixes for them as well? superset-frontend/packages/superset-core/src/sqlLab/index.ts |
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review Agent Run #68d738
Actionable Suggestions - 3
-
superset-frontend/src/SqlLab/components/SqlEditor/index.tsx - 2
- Missing unit tests for northPane feature · Line 124-318
- Storage listener key mismatch · Line 124-318
-
superset-frontend/packages/superset-core/src/sqlLab/index.ts - 1
- Type mismatch with backend API · Line 70-70
Review Details
-
Files reviewed - 5 · Commit Range:
88bc776..88bc776- superset-frontend/packages/superset-core/src/sqlLab/index.ts
- superset-frontend/src/SqlLab/components/SqlEditor/index.tsx
- superset-frontend/src/SqlLab/components/TabbedSqlEditors/index.tsx
- superset-frontend/src/SqlLab/contributions.ts
- superset-frontend/src/extensions/ExtensionsStartup.tsx
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- Eslint (Linter) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #41285 +/- ##
===========================================
+ Coverage 65.62% 84.40% +18.77%
===========================================
Files 3059 3062 +3
Lines 202434 202597 +163
Branches 47448 47496 +48
===========================================
+ Hits 132852 171003 +38151
+ Misses 66587 28336 -38251
- Partials 2995 3258 +263
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:
|
Contributor
There was a problem hiding this comment.
Code Review Agent Run #43c3dc
Actionable Suggestions - 1
-
superset-frontend/src/core/sqlLab/models.ts - 1
- Missing backendId test coverage · Line 37-68
Review Details
-
Files reviewed - 9 · Commit Range:
88bc776..190d77a- superset-frontend/packages/superset-core/src/sqlLab/index.ts
- superset-frontend/src/SqlLab/components/SqlEditor/index.tsx
- superset-frontend/src/SqlLab/components/TabbedSqlEditors/index.tsx
- superset-frontend/src/extensions/ExtensionsLoader.test.ts
- superset-frontend/src/extensions/ExtensionsLoader.ts
- superset-frontend/src/extensions/ExtensionsStartup.test.tsx
- superset-frontend/src/SqlLab/components/SqlEditor/SqlEditor.test.tsx
- superset-frontend/src/core/sqlLab/index.ts
- superset-frontend/src/core/sqlLab/models.ts
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- Eslint (Linter) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
Code Review Agent Run #9c9a7dActionable 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 |
rusackas
force-pushed
the
feat/sqllab-contribution-points-clean
branch
from
June 25, 2026 12:57
25595b0 to
ab9e1d9
Compare
Guards against executeCommand throwing "Command not found" when an extension contributes a menu item before its command finishes loading. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…yboard parity Move the new-tab interception from the inner span to a capture-phase click listener on antd's add button, which is the element that actually receives focus and keyboard activation. A native button synthesizes a click for both mouse and Enter/Space, so one capture listener keeps keyboard and mouse behavior in sync and drops the duplicate focusable span tab stop. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The new-tab "+" button (NewTabButton) had no test for its contributed behavior: the dropdown listing "SQL Editor" plus extension items, the empty-contributions branch that adds a tab directly, and the guard that skips a contributed menu item whose command isn't registered. Add regression tests that register a sqllab.newTab command/menu item and assert each branch, mirroring how SqlEditor.test.tsx registers a view. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…orthPane wrapper Fold Logger.markTimeOrigin() into newQueryEditor so every add-tab entry point (the + button, its dropdown, and antd's onEdit) records add-tab performance telemetry consistently, and swap the northPane view wrapper from a custom flex div to the antd Flex component. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Align getTabs test to actually exercise getTabs - Evaluate northPane extension view before empty-state branch - Handle executeCommand promise rejection in new-tab dropdown Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A stale pending key left behind by a failed createTab() could otherwise be picked up by an unrelated tab mounting later (e.g. on page reload restoring several tabs at once). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…extension load failures, and Tab.backendId
- Close the "+" new-tab dropdown on an outside mousedown or Escape;
trigger={[]} had opted it out of antd's own dismissal, so it stayed open
until re-clicked. Covered by outside-click, Escape, and keyboard tests.
- Resolve the new-tab tooltip at render time rather than module load so
t() runs after the translator is configured.
- Drop the duplicate Logger.markTimeOrigin() in onTabClicked now that
newQueryEditor marks it.
- Remove the orphaned id-keyed northPane localStorage entry once a tab
syncs and its tabViewId arrives.
- Surface partial extension failures: initializeExtensions() resolves to
the names of extensions that failed to load so ExtensionsStartup can
raise a warning toast instead of running the success path; drop the
duplicate error log now that the loader already logs and rethrows.
- Populate Tab.backendId for tabs hydrated from the backend on reload,
which use the backend id as queryEditor.id with no tabViewId.
- Cover the isActive gate on PENDING_NORTH_PANE_VIEW_KEY with a
non-active tab test.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015ybdwS1GbAy3Pkt6YGKCQp
… backendId for query events Address sadpandajoe's review: - Replace the origin-wide PENDING_NORTH_PANE_VIEW_KEY localStorage mailbox with a `northPaneViewId` option on `createTab()`, carried on the new query editor's own state so concurrent createTab() calls can't clobber each other's requested pane. SqlEditor still persists the per-tab localStorage entry so the mode survives a backend rehydrate. - Resolve `Tab.backendId` in `extractBaseData()` (query lifecycle events) through the same `resolveBackendId()` path as every other Tab construction, matching the editor by `id` or `tabViewId` since a synced tab's query carries the backend id as `sqlEditorId`. - Add `northPane` and `newTab` to the public `SqlLabLocation` union so extension manifests declaring those locations type-check. - Surface the actual message/status from a rejected extension-list fetch in the warning toast via `getClientErrorObject()` instead of `String(error)`. Co-Authored-By: Evan Rusackas <evan@preset.io> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
sqlEditorId can be string | null | undefined but the helper only accepts string | undefined; coalesce null to undefined. Co-Authored-By: Evan Rusackas <evan@preset.io> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…test fixture Fall back to the query pane instead of the empty state while a requested northPane view is still registering asynchronously, so the pane isn't blank during extension load. Also fix a test fixture that appended a duplicate query editor id instead of updating the existing entry, which modeled an impossible two-tabs-one-id store state. Co-Authored-By: Evan Rusackas <evan@preset.io> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ring antd's own add-tab handler
Any selection from the "+" dropdown (the built-in "SQL Editor" item or an
extension-contributed one) was also creating a second, unwanted plain tab
alongside the one actually picked. antd's Tabs wraps addIcon in its own
<button onClick={() => onEdit('add')}>, and the dropdown's popup, though
DOM-portaled to document.body, stays a React-tree descendant of that
button. React's synthetic events bubble along the React tree (piercing
straight through the portal), so every dropdown-item click also reached
that ancestor's onEdit('add') and appended a plain tab.
Stops propagation once, in the popup's own wrapper, rather than requiring
every current and future menu-item handler (including extension-contributed
ones) to remember to do it themselves.
Added regression tests for both paths (the built-in SQL Editor item and a
contributed one) asserting exactly one tab is added, not two.
Co-Authored-By: Evan Rusackas <evan@preset.io>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- makeTab: collapse the closed/backendId trailing booleans into an options object so call sites stop passing a bare `false` placeholder. - ExtensionsStartup tests: restore the mocked prototype method in afterEach instead of at the end of each test, so a failed assertion can't leak the mock into later tests; extract a shared toast-text helper to de-duplicate the repeated store.getState() unwrapping. - SqlEditor tests: extract the repeated northPane view registration and storage-key derivation into shared helpers. - TabbedSqlEditors: pass the already-stable newQueryEditor callback directly to NewTabButton instead of wrapping it in a new arrow function on every render. Co-Authored-By: Evan Rusackas <evan@preset.io> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- ExtensionsStartup tests: assert toastType === ToastType.Warning alongside the toast text, so swapping addWarningToast for addInfoToast would actually fail the test. - SqlEditor: extract the northPane view id/localStorage-sync logic into a useNorthPaneView hook, keeping the ~1100-line component focused on layout. Behavior is unchanged. Co-Authored-By: Evan Rusackas <evan@preset.io> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…until extensions are ready ExtensionsLoader.isReady()/onReady() track whether extension initialization has settled; useExtensionsReady() subscribes to that via useSyncExternalStore. Until it's true, NewTabButton can't trust menus.getMenu(sqllab.newTab) to reflect the final set of contributions -- an extension that hasn't finished loading looks identical to "no extensions at all". Without this, clicking "+" during that window fell back to adding a plain SQL tab instead of waiting to show the real dropdown. The button now shows a spinner and ignores clicks until ready. Co-Authored-By: Evan Rusackas <evan@preset.io> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…d ready tracking Reuse the existing 'Loading...' string, attach the add-tab click handler in the empty state, and avoid a dangling rejected promise in ExtensionsLoader. Co-Authored-By: Evan Rusackas <evan@preset.io> Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
custom-rules-frontend's un-awaited-userEvent lint rule (added in #44947) flagged this: userEvent.type() returns a promise, and firing it without awaiting let the test's synthesized Enter keypress race the assertion that follows, same risk the lint rule exists to catch everywhere else. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T1UbxU1PCUDVwqVVUk4Uc5
EnxDev flagged on review that closing a SQL Lab tab never removes its sqllab.northPaneView.<id> localStorage entry, so every tab a user opens and closes over a session leaves a permanent orphaned key behind. Extracted the northPane localStorage key/read/write helpers out of useNorthPaneView.ts into a shared, non-React utils module so both the hook and the two real tab-close paths (the UI's removeQueryEditor action and the extension API's closeTab) can clear a tab's entry when it's actually closed, not just when it migrates from an id-keyed to a tabViewId-keyed entry as before. Co-Authored-By: Evan Rusackas <evan@preset.io> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…diness path Co-Authored-By: Evan Rusackas <evan@preset.io> Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
rusackas
force-pushed
the
feat/sqllab-contribution-points-clean
branch
from
October 28, 2026 00:31
9e52844 to
d23efb2
Compare
github-actions
Bot
removed
i18n
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
SUMMARY
Lets extensions contribute first-class SQL Lab experiences: a full-pane replacement for the default editor/SouthPane split, and their own tab types in the
+new-tab dropdown.SqlLab/contributions.ts:sqllab.northPane(full-pane replacement) andsqllab.newTab(tab types in the+dropdown).PENDING_NORTH_PANE_VIEW_KEY: an extension sets thislocalStoragekey before callingsqlLab.createTab()to declare which northPane view the new tab opens with.SqlEditorconsumes/removes it on init, then persists the choice per-tab so the mode survives reloads.Tab.backendIdon the publicsuperset-coreTabinterface so extensions can correlate UI tabs with theirtabstateviewrow.TabbedSqlEditors: the+button becomes aDropdownwhen extensions contributenewTabitems, listing the built-in SQL Editor plus any contributed tab types.ExtensionsStartup: surfaces extension load errors as warning toasts instead of only logging.This is a clean re-target of #774 onto
apache:master. That PR was opened against a fork branch and stacked on the unmerged Tier 1/2 storage work, which inflated the diff and caused conflicts. The feature itself doesn't depend on the storage code, so this drops the storage stack and rebases just the contribution-surfaces commit (original authorship preserved).Adapted to
master's evolution since the branch forked:SqlEditorconsumesmaster's reactiveuseViews()hook (added in fix(extensions): load extensions async to avoid blocking initial page render #40915) instead of a customonViewsChange()subscription, so a northPane view appears when an extension registers it asynchronously, without blocking initial render.ExtensionsStartupkeepsmaster's non-blocking async extension load (fix(extensions): load extensions async to avoid blocking initial page render #40915) and adds the error-toast surfacing (dropping the originalinitialized-gate, which is no longer needed now that views are reactive).+icon is wrapped inmaster'sAddTabIconWrapperfor styling consistency.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A (extension contribution surfaces; visible only with an extension registering
sqllab.northPane/sqllab.newTabviews).TESTING INSTRUCTIONS
cd superset-frontend && npx jest src/extensions/ExtensionsStartup.test.tsx src/SqlLab/components/SqlEditor/SqlEditor.test.tsx src/SqlLab/components/TabbedSqlEditors/TabbedSqlEditors.test.tsxWith
EnableExtensionson, an extension that registers asqllab.northPaneview (and setsPENDING_NORTH_PANE_VIEW_KEYbeforecreateTab()) opens a tab rendering that view in place of the default editor; registering asqllab.newTabcommand turns the+button into a dropdown.ADDITIONAL INFORMATION
EnableExtensions🤖 Generated with Claude Code