feat(sqllab): primary/secondary action extensions #36644
Conversation
| await this.initializeExtension(extension); | ||
| }), | ||
| ); | ||
| try { |
There was a problem hiding this comment.
Why do you need to add a try/catch here? Isn't this handled by the caller?
There was a problem hiding this comment.
ops. it's not supposed to be here.
superset-frontend/src/SqlLab/components/SqlEditorTopBar/index.tsx
Outdated
Show resolved
Hide resolved
justinpark
force-pushed
the
feat--superset--extensions-actions
branch
from
6580e9e to
7aac09b
Compare
| */ | ||
| export enum ViewContribution { | ||
| RightSidebar = 'sqllab.rightSidebar', | ||
| SouthPanels = 'sqllab.panels', |
There was a problem hiding this comment.
It would be nice to rename this to Panels to match the identifier as the other ones.
| `} | ||
| > | ||
| <ActionPanel viewId={ViewContribution.Editor} primary compactMode> | ||
| {defaultPrimaryActions} |
There was a problem hiding this comment.
I think primary actions should come before secondary actions like VSCode (different from what's in Kasia's prototype). We should also sort the actions based on their use. Run should be the first action on the left.
| queryEditorId: string; | ||
| } | ||
|
|
||
| const StatusBar = ({ queryEditorId }: StatusBarProps) => { |
There was a problem hiding this comment.
The StatusBar component should not have any logic in itself part from rendering items on the left and right sides. It's a layout component that receives built-in and external extensions. One built-in extension is the Timer. It would also be nice to implement the Timer logic using the events from @apache-superset/core instead of depending on Redux.
|
@kasiazjc There was some feedback regarding the placement of the primary button at the top of the editor. First of all, I think the Run button is currently too far from the editor, which means the mouse has to travel quite a distance to click it. 
That's why I placed the primary and secondary buttons at the top left corner, similar to other locations in the existing interface. updated run primary action layout: 
updated south panel tabs: 
|
dosubot
bot
added
change:frontend
codeant-ai-for-open-source
bot
added
the
size:XXL
justinpark
force-pushed
the
feat--superset--extensions-actions
branch
from
2132dd8 to
012da74
Compare
Nitpicks 🔍
|
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| queryEditorId, | ||
| queryState, | ||
| overlayCreateAsMenu, | ||
| runQuery, |
There was a problem hiding this comment.
Suggestion: The component's props include allowAsync but the function does not destructure it from props, so allowAsync is not available inside the component; this leaves the prop unused and prevents forwarding it to runQuery, causing behavioral regression when callers rely on allowAsync. [logic error]
Severity Level: Minor
| runQuery, | |
| allowAsync, |
Why it matters? ⭐
The props interface includes allowAsync but the component does not destructure it, so any callers setting allowAsync will have no effect. This is a real behavioral regression risk and the suggested destructuring is the correct, minimal fix.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/SqlLab/components/RunQueryActionButton/index.tsx
**Line:** 102:102
**Comment:**
*Logic Error: The component's props include `allowAsync` but the function does not destructure it from props, so `allowAsync` is not available inside the component; this leaves the prop unused and prevents forwarding it to `runQuery`, causing behavioral regression when callers rely on `allowAsync`.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| data-test="run-query-action" | ||
| onClick={() => | ||
| onClick(shouldShowStopBtn, allowAsync, runQuery, stopQuery, logAction) | ||
| onClick(shouldShowStopBtn, runQuery, stopQuery, logAction) |
There was a problem hiding this comment.
Suggestion: runQuery accepts an optional boolean (c?: boolean) but the call site now invokes runQuery with no argument; the new allowAsync prop should be forwarded to runQuery to preserve previous behavior — wrap runQuery in a lambda that passes allowAsync. [logic error]
Severity Level: Minor
| onClick(shouldShowStopBtn, runQuery, stopQuery, logAction) | |
| onClick(shouldShowStopBtn, () => runQuery(allowAsync), stopQuery, logAction) |
Why it matters? ⭐
runQuery has signature (c?: boolean) => void. The PR now calls runQuery with no args which can change behavior if callers expected allowAsync to be passed. Wrapping runQuery as () => runQuery(allowAsync) (and destructuring allowAsync) preserves prior semantics — this fixes a real logic issue.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/SqlLab/components/RunQueryActionButton/index.tsx
**Line:** 145:145
**Comment:**
*Logic Error: `runQuery` accepts an optional boolean (`c?: boolean`) but the call site now invokes `runQuery` with no argument; the new `allowAsync` prop should be forwarded to `runQuery` to preserve previous behavior — wrap `runQuery` in a lambda that passes `allowAsync`.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| setShowSave={() => true} | ||
| overlayMenu={overlayMenu} | ||
| onSaveAsExplore={onSaveAsExplore} | ||
| />, | ||
| ); | ||
|
|
There was a problem hiding this comment.
Suggestion: The tests call userEvent.click without awaiting it; modern userEvent calls are asynchronous and should be awaited to avoid flaky tests — convert userEvent.click(...) to await userEvent.click(...) wherever a click leads to DOM updates or assertions immediately afterward. [possible bug]
Severity Level: Critical 🚨
| setShowSave={() => true} | |
| overlayMenu={overlayMenu} | |
| onSaveAsExplore={onSaveAsExplore} | |
| />, | |
| ); | |
| await userEvent.click(caretBtn); |
Why it matters? ⭐
userEvent.click is asynchronous in modern testing-library; awaiting it prevents race conditions between the click and subsequent assertions. This change reduces flakiness and is a correct improvement to the test.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/SqlLab/components/SaveDatasetActionButton/SaveDatasetActionButton.test.tsx
**Line:** 47:51
**Comment:**
*Possible Bug: The tests call `userEvent.click` without awaiting it; modern userEvent calls are asynchronous and should be awaited to avoid flaky tests — convert `userEvent.click(...)` to `await userEvent.click(...)` wherever a click leads to DOM updates or assertions immediately afterward.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| onClick={() => setShowSave(true)} | ||
| popupRender={() => overlayMenu} | ||
| popupRender={() => ( | ||
| <Menu | ||
| items={[ | ||
| { | ||
| label: t('Save dataset'), | ||
| key: 'save-dataset', | ||
| onClick: onSaveAsExplore, | ||
| }, | ||
| ]} | ||
| /> |
There was a problem hiding this comment.
Suggestion: Logic bug: onClick={() => setShowSave(true)} is attached to the DropdownButton, so clicking the dropdown to open the menu will immediately open the save modal instead of waiting for the user to select the "Save dataset" menu item; move the setShowSave(true) call into the menu item's click handler (and ensure the menu item handler also calls onSaveAsExplore) so the modal opens only after selecting the menu action. [logic error]
Severity Level: Minor
| onClick={() => setShowSave(true)} | |
| popupRender={() => overlayMenu} | |
| popupRender={() => ( | |
| <Menu | |
| items={[ | |
| { | |
| label: t('Save dataset'), | |
| key: 'save-dataset', | |
| onClick: onSaveAsExplore, | |
| }, | |
| ]} | |
| /> | |
| popupRender={() => ( | |
| <Menu | |
| items={[ | |
| { | |
| label: t('Save dataset'), | |
| key: 'save-dataset', | |
| onClick: () => { | |
| setShowSave(true); | |
| onSaveAsExplore?.(); | |
| }, |
Why it matters? ⭐
The current code calls setShowSave(true) on the DropdownButton's onClick, so simply opening the dropdown will immediately open the save modal — that's a real UX/logic bug. Moving the setShowSave call into the menu item's onClick (and calling onSaveAsExplore there) fixes the behaviour so the modal opens only after the user selects the menu action.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/SqlLab/components/SaveDatasetActionButton/index.tsx
**Line:** 65:75
**Comment:**
*Logic Error: Logic bug: `onClick={() => setShowSave(true)}` is attached to the `DropdownButton`, so clicking the dropdown to open the menu will immediately open the save modal instead of waiting for the user to select the "Save dataset" menu item; move the `setShowSave(true)` call into the menu item's click handler (and ensure the menu item handler also calls `onSaveAsExplore`) so the modal opens only after selecting the menu action.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| (ExtensionsManager as any).instance = undefined; | ||
| }); | ||
|
|
||
| afterEach(() => { |
There was a problem hiding this comment.
Suggestion: Tests only null out the singleton ExtensionsManager.instance but do not call any cleanup on an existing instance, which can leave internal state, timers or subscriptions from a prior instance alive and cause test flakiness or memory leaks; call an explicit cleanup/dispose on any existing instance before resetting to undefined. [resource leak]
Severity Level: Minor
| (ExtensionsManager as any).instance = undefined; | |
| }); | |
| afterEach(() => { | |
| const existing = (ExtensionsManager as any).instance as ExtensionsManager | undefined; | |
| if (existing && typeof (existing as any).dispose === 'function') { | |
| (existing as any).dispose(); | |
| } | |
| (ExtensionsManager as any).instance = undefined; | |
| }); | |
| afterEach(() => { | |
| const existing = (ExtensionsManager as any).instance as ExtensionsManager | undefined; | |
| if (existing && typeof (existing as any).dispose === 'function') { | |
| (existing as any).dispose(); | |
| } |
Why it matters? ⭐
This is a sensible improvement for test hygiene. The current tests just null the singleton which can leave timers/subscriptions alive if the manager holds any. Calling a dispose (guarded by a typeof check) is defensive and won't change behavior when dispose is absent. It's not speculative — it prevents potential flakiness and memory leaks in long test runs.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/components/ViewExtension/ViewExtension.test.tsx
**Line:** 97:100
**Comment:**
*Resource Leak: Tests only null out the singleton `ExtensionsManager.instance` but do not call any cleanup on an existing instance, which can leave internal state, timers or subscriptions from a prior instance alive and cause test flakiness or memory leaks; call an explicit cleanup/dispose on any existing instance before resetting to undefined.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| rerender( | ||
| <ExtensionsProvider> | ||
| <ViewExtension viewId={VIEW_ID_B} /> | ||
| </ExtensionsProvider>, | ||
| ); |
There was a problem hiding this comment.
Suggestion: In the rerender call you re-wrap the UI with a new ExtensionsProvider instance instead of relying on the original test wrapper; this can create nested/different provider instances and make the test behavior inconsistent — rerender with only the new ViewExtension element so the original wrapper remains in effect. [possible bug]
Severity Level: Critical 🚨
| rerender( | |
| <ExtensionsProvider> | |
| <ViewExtension viewId={VIEW_ID_B} /> | |
| </ExtensionsProvider>, | |
| ); | |
| rerender(<ViewExtension viewId={VIEW_ID_B} />); |
Why it matters? ⭐
Correct — passing a new provider wrapper to rerender may create a different provider instance or nesting, altering context state and making the test flaky. Rerendering the inner component so the original test wrapper remains in effect is the correct and minimal fix.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/components/ViewExtension/ViewExtension.test.tsx
**Line:** 194:198
**Comment:**
*Possible Bug: In the rerender call you re-wrap the UI with a new `ExtensionsProvider` instance instead of relying on the original test wrapper; this can create nested/different provider instances and make the test behavior inconsistent — rerender with only the new `ViewExtension` element so the original wrapper remains in effect.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.|
|
||
| render(<MenuExtension viewId={TEST_VIEW_ID} primary />); | ||
|
|
||
| const button = screen.getByText('test.action Title').closest('button')!; |
There was a problem hiding this comment.
Suggestion: Locating the primary action's button via .closest('button') on a text node can return null and cause a runtime error; use a role-based query that finds the actual button by accessible name to avoid a potential TypeError and make the test less brittle. [possible bug]
Severity Level: Critical 🚨
| const button = screen.getByText('test.action Title').closest('button')!; | |
| const button = screen.getByRole('button', { name: 'test.action Title' }); |
Why it matters? ⭐
The change makes the test more robust and idiomatic: querying by role with the accessible name avoids a potential null from .closest() and better reflects how users interact with the UI. It's a simple, valid improvement that doesn't change behaviour and reduces flakiness.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/components/MenuExtension/MenuExtension.test.tsx
**Line:** 217:217
**Comment:**
*Possible Bug: Locating the primary action's button via `.closest('button')` on a text node can return null and cause a runtime error; use a role-based query that finds the actual button by accessible name to avoid a potential TypeError and make the test less brittle.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| const contributions = | ||
| ExtensionsManager.getInstance().getViewContributions(viewId) || []; |
There was a problem hiding this comment.
Suggestion: The call to getViewContributions(viewId) || [] assumes the method returns an array or falsy value; if it returns a non-array truthy value the code will break when iterating. Coerce the result to an array (using Array.isArray) to ensure contributions is always an array. [type error]
Severity Level: Minor
| const contributions = | |
| ExtensionsManager.getInstance().getViewContributions(viewId) || []; | |
| const maybeContributions = ExtensionsManager.getInstance().getViewContributions(viewId); | |
| const contributions = Array.isArray(maybeContributions) ? maybeContributions : []; |
Why it matters? ⭐
Coercing the result with Array.isArray prevents runtime errors if getViewContributions unexpectedly returns a non-array truthy value. The improved code is a small, defensive fix that prevents .map from failing.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/components/ViewExtension/index.tsx
**Line:** 27:28
**Comment:**
*Type Error: The call to `getViewContributions(viewId) || []` assumes the method returns an array or falsy value; if it returns a non-array truthy value the code will break when iterating. Coerce the result to an array (using `Array.isArray`) to ensure `contributions` is always an array.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| ExtensionsManager.getInstance().getViewContributions(viewId) || []; | ||
| const { getView } = useExtensionsContext(); | ||
|
|
||
| return <>{contributions.map(contribution => getView(contribution.id))}</>; |
There was a problem hiding this comment.
Suggestion: Rendering a list of views without stable React keys can produce UI bugs and warnings; mapping contributions directly to elements returns an array of children without keys which can cause incorrect element reuse and rendering issues. Wrap each rendered view in an element with a stable key (e.g., the contribution id) and guard against missing ids. [possible bug]
Severity Level: Critical 🚨
| return <>{contributions.map(contribution => getView(contribution.id))}</>; | |
| return ( | |
| <> | |
| {contributions | |
| .filter(contribution => contribution && typeof contribution.id !== 'undefined') | |
| .map(contribution => ( | |
| <span key={String(contribution.id)}>{getView(contribution.id)}</span> | |
| ))} | |
| </> | |
| ); |
Why it matters? ⭐
Mapping an array of children without stable keys can produce React warnings and incorrect element reuse. The suggested change adds a stable key per item which addresses the real issue. Note: the proposed wrapper uses a which will add extra DOM; prefer using <React.Fragment key={...}> to avoid altering markup or ensure getView already returns keyed elements.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/components/ViewExtension/index.tsx
**Line:** 31:31
**Comment:**
*Possible Bug: Rendering a list of views without stable React keys can produce UI bugs and warnings; mapping `contributions` directly to elements returns an array of children without keys which can cause incorrect element reuse and rendering issues. Wrap each rendered view in an element with a stable `key` (e.g., the contribution id) and guard against missing ids.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
|
CodeAnt AI finished reviewing your PR. |
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #bb862d
Actionable Suggestions - 4
-
superset-frontend/src/components/MenuExtension/index.tsx - 2
- Undefined Command Access · Line 64-67
- Undefined Command Access · Line 91-94
-
superset-frontend/src/SqlLab/components/StatusBar/index.tsx - 1
- CSS Property Typo · Line 44-44
-
superset-frontend/src/SqlLab/components/TabbedSqlEditors/index.tsx - 1
- Invisible border due to missing style · Line 61-61
Additional Suggestions - 14
-
superset-frontend/src/SqlLab/components/SqlEditorTopBar/useDatabaseSelector.test.ts - 1
-
Type mismatch in test data · Line 199-199The test passes a single DatabaseObject to getDbList, but the setDatabases action expects an array of DatabaseObject. This type mismatch could cause the reducer to fail at runtime when processing databases. Pass an array instead and remove the 'as any' cast.
-
-
superset-frontend/src/SqlLab/components/SqlEditorTopBar/useDatabaseSelector.ts - 1
-
Type mismatch in onDbChange · Line 59-61The onDbChange function's parameter type doesn't match the expected (db: DatabaseObject) => void from DatabaseSelectorProps. This could lead to type inconsistencies when called by components like TableSelectorMultiple. Update to accept the full DatabaseObject for better type safety.
Code suggestion
suggestion const onDbChange = (db: DatabaseObject) => { dispatch(queryEditorSetDb(queryEditor, db.id)); };
-
-
superset-frontend/src/components/MenuExtension/index.tsx - 1
-
Type Ignore Violation · Line 68-68The // @ts-ignore suppresses type checking, violating the no 'any' types rule; remove it after fixing the import.
-
-
superset-frontend/src/components/ViewExtension/index.tsx - 1
-
React Key Missing · Line 31-31The map function in the return statement lacks a key prop, which can lead to React rendering issues and console warnings. Add key={contribution.id} to each mapped element.
-
-
superset-frontend/src/SqlLab/components/SqlEditorTopBar/index.tsx - 1
-
Unused prop in interface · Line 30-30The queryEditorId prop is defined in SqlEditorTopBarProps but not used in the component or passed to MenuExtension. This appears to be an unused parameter that could confuse maintainers. Consider removing it from the interface, component props, and the parent SqlEditor usage to keep the API clean.
-
-
superset-frontend/src/SqlLab/components/SqlEditorTopBar/SqlEditorTopBar.test.tsx - 1
-
Missing compactMode test coverage · Line 25-50The mock misses compactMode, so tests can't verify the component passes compactMode=true to primary MenuExtension, reducing coverage of prop forwarding.
-
-
superset-frontend/src/components/DatabaseSelector/types.ts - 1
-
Missing test coverage for horizontalMode · Line 51-51The new horizontalMode prop changes the component's layout from vertical to horizontal, but the existing tests don't cover this behavior. Consider adding a test to ensure the horizontal rendering works as expected.
-
-
superset-frontend/src/components/ViewExtension/ViewExtension.test.tsx - 4
-
Unnecessary Type Assertion · Line 93-93The `as any` assertion on ExtensionsProvider in the render wrapper is unnecessary, as React.FC is compatible with React.ComponentType. This violates the no-`any` rule and can be safely removed.
Code suggestion
@@ -92,3 +92,3 @@ const renderWithExtensionsProvider = (ui: ReactElement) => { - return render(ui, { wrapper: ExtensionsProvider as any }); + return render(ui, { wrapper: ExtensionsProvider }); }; -
Type Safety Violation · Line 72-76The test accesses private properties `contextIndex` and `extensionContributions` on ExtensionsManager using `as any`, which violates the project's TypeScript standards prohibiting `any` types. While this enables testing of internal state, it reduces type safety and maintainability. Consider exposing a test-specific API or using spies on public methods to achieve the same test coverage without type assertions.
-
Redundant Provider Wrapping · Line 92-94The renderWithExtensionsProvider function wraps components with ExtensionsProvider, but the project's custom render (from spec/helpers/testing-library) already includes this provider. This creates unnecessary nesting and inconsistency. Consider using render directly for all tests.
-
Inconsistent Test Rendering · Line 194-198In the 'renders views for different viewIds independently' test, the initial render uses renderWithExtensionsProvider, but rerender manually wraps with ExtensionsProvider. This inconsistency could lead to unexpected behavior. Standardize on one approach.
-
-
superset-frontend/src/SqlLab/components/QueryLimitSelect/index.tsx - 1
-
Type Inconsistency in Number Formatter · Line 37-47The convertToShortNum function has inconsistent return types (number for small/large values, string for K/M), which reduces type safety. Make it always return string for consistency.
-
-
superset-frontend/src/SqlLab/components/SqlEditor/index.tsx - 1
-
Potential layout issue with negative margin · Line 187-187The negative margin on .sql-container extends the element beyond its parent .north-pane, which may cause unexpected layout behavior. If this is intended for full-width editor, it's fine; otherwise, consider removing it.
-
-
superset-frontend/src/SqlLab/components/StatusBar/index.tsx - 1
-
Unused Type Definition · Line 39-41The StatusBarProps interface defines queryEditorId but the component function doesn't use it. If props are needed later, they can be added then; for now, this is unused code.
-
Review Details
-
Files reviewed - 29 · Commit Range:
8666ffd..012da74- superset-frontend/packages/superset-ui-core/src/components/AsyncAceEditor/index.tsx
- superset-frontend/packages/superset-ui-core/src/components/Icons/AntdEnhanced.tsx
- superset-frontend/src/SqlLab/components/AppLayout/index.tsx
- superset-frontend/src/SqlLab/components/QueryLimitSelect/QueryLimitSelect.test.tsx
- superset-frontend/src/SqlLab/components/QueryLimitSelect/index.tsx
- superset-frontend/src/SqlLab/components/RunQueryActionButton/index.tsx
- superset-frontend/src/SqlLab/components/SaveDatasetActionButton/SaveDatasetActionButton.test.tsx
- superset-frontend/src/SqlLab/components/SaveDatasetActionButton/index.tsx
- superset-frontend/src/SqlLab/components/SaveQuery/index.tsx
- superset-frontend/src/SqlLab/components/ShareSqlLabQuery/index.tsx
- superset-frontend/src/SqlLab/components/SouthPane/index.tsx
- superset-frontend/src/SqlLab/components/SqlEditor/SqlEditor.test.tsx
- superset-frontend/src/SqlLab/components/SqlEditor/index.tsx
- superset-frontend/src/SqlLab/components/SqlEditorLeftBar/index.tsx
- superset-frontend/src/SqlLab/components/SqlEditorTopBar/SqlEditorTopBar.test.tsx
- superset-frontend/src/SqlLab/components/SqlEditorTopBar/index.tsx
- superset-frontend/src/SqlLab/components/SqlEditorTopBar/useDatabaseSelector.test.ts
- superset-frontend/src/SqlLab/components/SqlEditorTopBar/useDatabaseSelector.ts
- superset-frontend/src/SqlLab/components/StatusBar/StatusBar.test.tsx
- superset-frontend/src/SqlLab/components/StatusBar/index.tsx
- superset-frontend/src/SqlLab/components/TabbedSqlEditors/index.tsx
- superset-frontend/src/SqlLab/constants.ts
- superset-frontend/src/SqlLab/contributions.ts
- superset-frontend/src/components/DatabaseSelector/index.tsx
- superset-frontend/src/components/DatabaseSelector/types.ts
- superset-frontend/src/components/MenuExtension/MenuExtension.test.tsx
- superset-frontend/src/components/MenuExtension/index.tsx
- superset-frontend/src/components/ViewExtension/ViewExtension.test.tsx
- superset-frontend/src/components/ViewExtension/index.tsx
-
Files skipped - 0
-
Tools
- Eslint (Linter) - ✔︎ Successful
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ 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 [email protected].
Documentation & Help
AI Code Review powered by
| const command = | ||
| ExtensionsManager.getInstance().getCommandContribution( | ||
| contribution.command, | ||
| )!; |
There was a problem hiding this comment.
If getCommandContribution returns undefined, accessing command.icon will throw at runtime; add a guard or filter.
Code Review Run #bb862d
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| const command = | ||
| ExtensionsManager.getInstance().getCommandContribution( | ||
| contribution.command, | ||
| )!; |
There was a problem hiding this comment.
If getCommandContribution returns undefined, accessing command.title will throw at runtime; add a guard or filter.
Code Review Run #bb862d
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| } | ||
|
|
||
| const StatusBar = () => ( | ||
| <Container align="center" justify="space-bewteen"> |
There was a problem hiding this comment.
The justify prop has a typo: "space-bewteen" should be "space-between" to apply correct CSS flexbox justification and ensure proper horizontal spacing of status bar items.
Code Review Run #bb862d
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| border-top-color: ${({ theme }) => theme.colorPrimaryActive} !important; | ||
| border-right-color: ${({ theme }) => theme.colorPrimaryActive} !important; | ||
| box-shadow: 0 0 2px ${({ theme }) => theme.colorPrimaryActive} !important; | ||
| border-top: 2px; |
There was a problem hiding this comment.
The CSS shorthand 'border-top: 2px;' sets the width but defaults the style to 'none', preventing any visible border from rendering. Since 'border-top-color' is already set to the theme's primary active color, adding 'solid' to the shorthand will make the intended 2px top border appear for active tabs.
Review Rule
Incomplete CSS Border Shorthand Declaration for repo: apache/superset, language: TSX. Bito will avoid suggestions that match this rule. You can manage review rules here.Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
Code Review Agent Run #1bc81bActionable 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 |
Code Review Agent Run #6ebfccActionable 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 |
|
CodeAnt AI is running Incremental review Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
| } | ||
| return runQuery(false); | ||
| if (isStopAction) return stopQuery(); | ||
| runQuery(); |
There was a problem hiding this comment.
Removed allowAsync since runQuery does not reference the value anymore.
superset/superset-frontend/src/SqlLab/components/SqlEditor/index.tsx
Lines 313 to 317 in e5489bd
Code Review Agent Run #5bbb49Actionable Suggestions - 0Additional Suggestions - 2
Review 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 |
Code Review Agent Run #b83febActionable 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 |
|
CodeAnt AI is running Incremental review Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
Code Review Agent Run #97d76cActionable 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 |
|
Thank you for the latest changes @justinpark. I have some design suggestions that could be addressed in follow-up PRs but I think it would be easier to do them in this PR so that @kasiazjc can review the final form of the screen. 
Something like this: 
@kasiazjc These are just suggestions and I bet you can get to something even better. |
| export interface StatusBarProps { | ||
| queryEditorId: string; | ||
| } |
There was a problem hiding this comment.
Unused currently.
| export interface StatusBarProps { | |
| queryEditorId: string; | |
| } |
| viewId: string; | ||
| } | ||
|
|
||
| const ViewExtension = ({ viewId }: ViewExtensionProps) => { |
There was a problem hiding this comment.
The name ViewExtension is a bit misleading as it receives a viewId but returns a list of views.
| </Splitter.Panel> | ||
| )} | ||
| </Splitter> | ||
| {statusBarContributions.length > 0 && <StatusBar />} |
There was a problem hiding this comment.
Inside StatusBar you're also checking the contributions registry. I wonder if we should let StatusBar render the registry lookup and render as null if needed instead of duplicating the logic here.
| setShowSave: (arg0: boolean) => void; | ||
| overlayMenu: JSX.Element | null; | ||
| onSaveAsExplore?: () => void; | ||
| compactMode?: boolean; |
There was a problem hiding this comment.
compactMode comes from SaveQuery which always sets it to true. Should we remove it?
| interface ShareSqlLabQueryProps { | ||
| queryEditorId: string; | ||
| addDangerToast: (msg: string) => void; | ||
| compactMode?: boolean; |
There was a problem hiding this comment.
Same here. compactMode is always true.
| ${({ theme }) => ` | ||
| const DatabaseSelectorWrapper = styled.div<{ horizontal?: boolean }>` | ||
| ${({ theme, horizontal }) => | ||
| horizontal |
There was a problem hiding this comment.
I was not able to find any place in the code setting DatabaseSelector's horizontalMode. Should we remove the property?
justinpark
force-pushed
the
feat--superset--extensions-actions
branch
from
061b7f4 to
c689c41
Compare
|
CodeAnt AI is running Incremental review Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Code Review Agent Run #09d82e
Actionable Suggestions - 7
-
superset-frontend/src/SqlLab/components/SqlEditor/SqlEditor.test.tsx - 1
- Test assertion mismatch · Line 324-326
-
superset-frontend/src/components/ViewExtension/ViewExtension.test.tsx - 1
- Test Logic Bug · Line 194-198
-
superset-frontend/src/SqlLab/components/SqlEditorTopBar/useDatabaseSelector.test.ts - 5
- Incomplete dispatch assertion · Line 107-130
- Incomplete dispatch assertion · Line 132-154
- Incomplete dispatch assertion · Line 156-178
- Type mismatch in getDbList test · Line 180-208
- Incomplete dispatch assertion · Line 298-320
Additional Suggestions - 10
-
superset-frontend/src/components/MenuExtension/index.tsx - 1
-
Inconsistent null handling · Line 60-85In primaryActions, there's no null check for command, unlike secondaryActions. If getCommandContribution returns null despite the ! assertion, it could cause runtime errors accessing command.icon. Add the check for consistency and safety.
-
-
superset-frontend/src/SqlLab/components/SqlEditorTopBar/useDatabaseSelector.ts - 2
-
Type Safety Risk · Line 89-89The type cast `result as unknown as Database[]` bypasses TypeScript's type checking, which could lead to runtime errors if DatabaseObject and Database have incompatible structures. Consider verifying type compatibility or using a type guard.
-
Misleading Naming · Line 102-102The variable name 'bool' is misleading since it holds a string (or null) from URLSearchParams.get('db'), not a boolean. A more descriptive name would improve readability.
-
-
superset-frontend/src/components/ViewExtension/index.tsx - 1
-
Missing React key in map · Line 31-31The map over contributions lacks a key prop on the returned elements, which will trigger React console warnings and may cause inefficient re-renders. Consider wrapping getView(contribution.id) in ... and importing React if needed.
-
-
superset-frontend/src/SqlLab/components/QueryLimitSelect/index.tsx - 1
-
Incomplete number formatting · Line 37-48The convertToShortNum function should format all large numbers, including billions, and return a consistent string type to avoid potential type issues in usage.
Code suggestion
@@ -37,12 +37,14 @@ - export function convertToShortNum(num: number) { - if (num < 1000) { - return num; - } - if (num < 1_000_000) { - return `${num / 1000}K`; - } - if (num < 1_000_000_000) { - return `${num / 1000_000}M`; - } - return num; - } + export function convertToShortNum(num: number) { + if (num < 1000) { + return num.toString(); + } + if (num < 1_000_000) { + return `${num / 1000}K`; + } + if (num < 1_000_000_000) { + return `${num / 1000_000}M`; + } + if (num < 1_000_000_000_000) { + return `${num / 1_000_000_000}B`; + } + return num.toString(); + }
-
-
superset-frontend/src/SqlLab/components/SqlEditorTopBar/useDatabaseSelector.test.ts - 2
-
Unnecessary rerender · Line 81-105rerender() not needed; effect sets db on initial render. Remove for clarity.
Code suggestion
diff --git a/superset-frontend/src/SqlLab/components/SqlEditorTopBar/useDatabaseSelector.test.ts b/superset-frontend/src/SqlLab/components/SqlEditorTopBar/useDatabaseSelector.test.ts index 0000000..0000000 100644 --- a/superset-frontend/src/SqlLab/components/SqlEditorTopBar/useDatabaseSelector.test.ts +++ b/superset-frontend/src/SqlLab/components/SqlEditorTopBar/useDatabaseSelector.test.ts @@ -91,10 +91,7 @@ test('returns database when dbId exists in store', () => { const { result, rerender } = renderHook( () => useDatabaseSelector(defaultQueryEditor.id), { wrapper: createWrapper({ useRedux: true, store, }), }, ); - // Trigger effect by rerendering - rerender(); - expect(result.current.db).toEqual(mockDatabase); });
-
Unnecessary rerender · Line 238-276rerender() unnecessary; effect sets db from localStorage on mount. Remove.
Code suggestion
diff --git a/superset-frontend/src/SqlLab/components/SqlEditorTopBar/useDatabaseSelector.test.ts b/superset-frontend/src/SqlLab/components/SqlEditorTopBar/useDatabaseSelector.test.ts index 0000000..0000000 100644 --- a/superset-frontend/src/SqlLab/components/SqlEditorTopBar/useDatabaseSelector.test.ts +++ b/superset-frontend/src/SqlLab/components/SqlEditorTopBar/useDatabaseSelector.test.ts @@ -254,8 +254,6 @@ test('reads database from localStorage when URL has db param', () => { const { result, rerender } = renderHook( () => useDatabaseSelector(defaultQueryEditor.id), { wrapper: createWrapper({ useRedux: true, store, }), }, ); - rerender(); - expect(result.current.db).toEqual(localStorageDb); expect(localStorageHelpers.setItem).toHaveBeenCalledWith( localStorageHelpers.LocalStorageKeys.Database, null, ); });
-
-
superset-frontend/src/SqlLab/components/StatusBar/StatusBar.test.tsx - 1
-
Incomplete Test Coverage · Line 38-43The test suite covers rendering when status bar contributions are present, but misses the conditional branch where no contributions exist, potentially leaving untested logic in the component's fragment return.
-
-
superset-frontend/src/SqlLab/components/SouthPane/index.tsx - 1
-
Hardcoded padding value · Line 231-231The padding in the Flex component uses a hardcoded `8px` value. For consistency with the theme, consider using `theme.sizeUnit` instead.
-
-
superset-frontend/src/components/ViewExtension/ViewExtension.test.tsx - 1
-
Unnecessary Type Cast · Line 93-93The 'as any' cast on ExtensionsProvider is unnecessary, as React.FC components are compatible with the render wrapper. Removing it improves type safety.
-
Filtered by Review Rules
Bito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
-
superset-frontend/src/SqlLab/components/TabbedSqlEditors/index.tsx - 1
- Invalid CSS border declaration · Line 61-61
Review Details
-
Files reviewed - 29 · Commit Range:
9512504..c689c41- superset-frontend/packages/superset-ui-core/src/components/AsyncAceEditor/index.tsx
- superset-frontend/packages/superset-ui-core/src/components/Icons/AntdEnhanced.tsx
- superset-frontend/src/SqlLab/components/AppLayout/index.tsx
- superset-frontend/src/SqlLab/components/EstimateQueryCostButton/index.tsx
- superset-frontend/src/SqlLab/components/QueryLimitSelect/QueryLimitSelect.test.tsx
- superset-frontend/src/SqlLab/components/QueryLimitSelect/index.tsx
- superset-frontend/src/SqlLab/components/RunQueryActionButton/RunQueryActionButton.test.tsx
- superset-frontend/src/SqlLab/components/RunQueryActionButton/index.tsx
- superset-frontend/src/SqlLab/components/SaveDatasetActionButton/SaveDatasetActionButton.test.tsx
- superset-frontend/src/SqlLab/components/SaveDatasetActionButton/index.tsx
- superset-frontend/src/SqlLab/components/SaveQuery/index.tsx
- superset-frontend/src/SqlLab/components/ShareSqlLabQuery/index.tsx
- superset-frontend/src/SqlLab/components/SouthPane/index.tsx
- superset-frontend/src/SqlLab/components/SqlEditor/SqlEditor.test.tsx
- superset-frontend/src/SqlLab/components/SqlEditor/index.tsx
- superset-frontend/src/SqlLab/components/SqlEditorLeftBar/index.tsx
- superset-frontend/src/SqlLab/components/SqlEditorTopBar/SqlEditorTopBar.test.tsx
- superset-frontend/src/SqlLab/components/SqlEditorTopBar/index.tsx
- superset-frontend/src/SqlLab/components/SqlEditorTopBar/useDatabaseSelector.test.ts
- superset-frontend/src/SqlLab/components/SqlEditorTopBar/useDatabaseSelector.ts
- superset-frontend/src/SqlLab/components/StatusBar/StatusBar.test.tsx
- superset-frontend/src/SqlLab/components/StatusBar/index.tsx
- superset-frontend/src/SqlLab/components/TabbedSqlEditors/index.tsx
- superset-frontend/src/SqlLab/constants.ts
- superset-frontend/src/SqlLab/contributions.ts
- superset-frontend/src/components/MenuExtension/MenuExtension.test.tsx
- superset-frontend/src/components/MenuExtension/index.tsx
- superset-frontend/src/components/ViewExtension/ViewExtension.test.tsx
- superset-frontend/src/components/ViewExtension/index.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 [email protected].
Documentation & Help
AI Code Review powered by
| fireEvent.click(await findByText('Limit')); | ||
| expect(await findByText('10 000')).toBeInTheDocument(); | ||
| }); |
There was a problem hiding this comment.
The change to findByText('Limit') correctly updates the test to match the component's button text, but the related assertion on line 326 expects '10 000' while the component renders '10 000 ' (with trailing space) in the dropdown options. This could cause the test to fail after opening the dropdown.
Code suggestion
Check the AI-generated fix before applying
| fireEvent.click(await findByText('Limit')); | |
| expect(await findByText('10 000')).toBeInTheDocument(); | |
| }); | |
| fireEvent.click(await findByText('Limit')); | |
| expect(await findByText('10 000 ')).toBeInTheDocument(); | |
| }); |
Citations
- Rule Violated: dev-standard.mdc:97
Code Review Run #09d82e
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| rerender( | ||
| <ExtensionsProvider> | ||
| <ViewExtension viewId={VIEW_ID_B} /> | ||
| </ExtensionsProvider>, | ||
| ); |
There was a problem hiding this comment.
The rerender in this test incorrectly wraps the component with ExtensionsProvider again, leading to double wrapping since the initial render already uses this wrapper. This could cause unexpected behavior in context or state. Change to rerender() instead.
Code Review Run #09d82e
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| test('dispatches QUERY_EDITOR_SETDB action on onDbChange', () => { | ||
| const store = mockStore(createInitialState()); | ||
| const { result } = renderHook( | ||
| () => useDatabaseSelector(defaultQueryEditor.id), | ||
| { | ||
| wrapper: createWrapper({ | ||
| useRedux: true, | ||
| store, | ||
| }), | ||
| }, | ||
| ); | ||
|
|
||
| act(() => { | ||
| result.current.onDbChange({ id: 2 }); | ||
| }); | ||
|
|
||
| const actions = store.getActions(); | ||
| expect(actions).toContainEqual( | ||
| expect.objectContaining({ | ||
| type: 'QUERY_EDITOR_SETDB', | ||
| dbId: 2, | ||
| }), | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Test checks dispatch type but not payload.dbId. Add expect.objectContaining({ dbId: 2 }).
Code suggestion
Check the AI-generated fix before applying
--- a/superset-frontend/src/SqlLab/components/SqlEditorTopBar/useDatabaseSelector.test.ts
+++ b/superset-frontend/src/SqlLab/components/SqlEditorTopBar/useDatabaseSelector.test.ts
@@ -124,6 +124,7 @@ test('dispatches QUERY_EDITOR_SETDB action on onDbChange', () => {
expect(actions).toContainEqual(
expect.objectContaining({
type: 'QUERY_EDITOR_SETDB',
+ dbId: 2,
}),
);
});
Citations
- Rule Violated: AGENTS.md:21
Code Review Run #09d82e
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| test('dispatches queryEditorSetCatalog action on onCatalogChange', () => { | ||
| const store = mockStore(createInitialState()); | ||
| const { result } = renderHook( | ||
| () => useDatabaseSelector(defaultQueryEditor.id), | ||
| { | ||
| wrapper: createWrapper({ | ||
| useRedux: true, | ||
| store, | ||
| }), | ||
| }, | ||
| ); | ||
|
|
||
| act(() => { | ||
| result.current.onCatalogChange('new_catalog'); | ||
| }); | ||
|
|
||
| const actions = store.getActions(); | ||
| expect(actions).toContainEqual( | ||
| expect.objectContaining({ | ||
| type: 'QUERY_EDITOR_SET_CATALOG', | ||
| }), | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Test checks type but not payload.catalog. Add expect.objectContaining({ catalog: 'new_catalog' }).
Code suggestion
Check the AI-generated fix before applying
--- a/superset-frontend/src/SqlLab/components/SqlEditorTopBar/useDatabaseSelector.test.ts
+++ b/superset-frontend/src/SqlLab/components/SqlEditorTopBar/useDatabaseSelector.test.ts
@@ -149,6 +149,7 @@ test('dispatches queryEditorSetCatalog action on onCatalogChange', () => {
expect(actions).toContainEqual(
expect.objectContaining({
type: 'QUERY_EDITOR_SET_CATALOG',
+ catalog: 'new_catalog',
}),
);
});
Citations
- Rule Violated: AGENTS.md:21
Code Review Run #09d82e
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| test('dispatches queryEditorSetSchema action on onSchemaChange', () => { | ||
| const store = mockStore(createInitialState()); | ||
| const { result } = renderHook( | ||
| () => useDatabaseSelector(defaultQueryEditor.id), | ||
| { | ||
| wrapper: createWrapper({ | ||
| useRedux: true, | ||
| store, | ||
| }), | ||
| }, | ||
| ); | ||
|
|
||
| act(() => { | ||
| result.current.onSchemaChange('new_schema'); | ||
| }); | ||
|
|
||
| const actions = store.getActions(); | ||
| expect(actions).toContainEqual( | ||
| expect.objectContaining({ | ||
| type: 'QUERY_EDITOR_SET_SCHEMA', | ||
| }), | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Test checks type but not payload.schema. Add expect.objectContaining({ schema: 'new_schema' }).
Code suggestion
Check the AI-generated fix before applying
--- a/superset-frontend/src/SqlLab/components/SqlEditorTopBar/useDatabaseSelector.test.ts
+++ b/superset-frontend/src/SqlLab/components/SqlEditorTopBar/useDatabaseSelector.test.ts
@@ -173,6 +173,7 @@ test('dispatches queryEditorSetSchema action on onSchemaChange', () => {
expect(actions).toContainEqual(
expect.objectContaining({
type: 'QUERY_EDITOR_SET_SCHEMA',
+ schema: 'new_schema',
}),
);
});
Citations
- Rule Violated: AGENTS.md:21
Code Review Run #09d82e
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| test('dispatches setDatabases action on getDbList', () => { | ||
| const store = mockStore(createInitialState()); | ||
| const { result } = renderHook( | ||
| () => useDatabaseSelector(defaultQueryEditor.id), | ||
| { | ||
| wrapper: createWrapper({ | ||
| useRedux: true, | ||
| store, | ||
| }), | ||
| }, | ||
| ); | ||
|
|
||
| const newDatabase = { | ||
| id: 3, | ||
| database_name: 'test_db', | ||
| backend: 'postgresql', | ||
| }; | ||
|
|
||
| act(() => { | ||
| result.current.getDbList(newDatabase as any); | ||
| }); | ||
|
|
||
| const actions = store.getActions(); | ||
| expect(actions).toContainEqual( | ||
| expect.objectContaining({ | ||
| type: 'SET_DATABASES', | ||
| }), | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Test passes single object to getDbList, but hook expects DatabaseObject[]. Change to [newDatabase] and assert payload.databases.
Code suggestion
Check the AI-generated fix before applying
--- a/superset-frontend/src/SqlLab/components/SqlEditorTopBar/useDatabaseSelector.test.ts
+++ b/superset-frontend/src/SqlLab/components/SqlEditorTopBar/useDatabaseSelector.test.ts
@@ -192,7 +192,7 @@ test('dispatches setDatabases action on getDbList', () => {
const newDatabase = {
id: 3,
database_name: 'test_db',
backend: 'postgresql',
};
act(() => {
- result.current.getDbList(newDatabase as any);
+ result.current.getDbList([newDatabase]);
});
const actions = store.getActions();
expect(actions).toContainEqual(
expect.objectContaining({
type: 'SET_DATABASES',
+ databases: [newDatabase],
}),
);
});
Citations
- Rule Violated: AGENTS.md:21
Code Review Run #09d82e
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| test('handles null catalog change', () => { | ||
| const store = mockStore(createInitialState()); | ||
| const { result } = renderHook( | ||
| () => useDatabaseSelector(defaultQueryEditor.id), | ||
| { | ||
| wrapper: createWrapper({ | ||
| useRedux: true, | ||
| store, | ||
| }), | ||
| }, | ||
| ); | ||
|
|
||
| act(() => { | ||
| result.current.onCatalogChange(null); | ||
| }); | ||
|
|
||
| const actions = store.getActions(); | ||
| expect(actions).toContainEqual( | ||
| expect.objectContaining({ | ||
| type: 'QUERY_EDITOR_SET_CATALOG', | ||
| }), | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Test checks type but not payload.catalog === null. Add to expect.objectContaining.
Code suggestion
Check the AI-generated fix before applying
--- a/superset-frontend/src/SqlLab/components/SqlEditorTopBar/useDatabaseSelector.test.ts
+++ b/superset-frontend/src/SqlLab/components/SqlEditorTopBar/useDatabaseSelector.test.ts
@@ -315,6 +315,7 @@ test('handles null catalog change', () => {
expect(actions).toContainEqual(
expect.objectContaining({
type: 'QUERY_EDITOR_SET_CATALOG',
+ catalog: null,
}),
);
});
Citations
- Rule Violated: AGENTS.md:21
Code Review Run #09d82e
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
Code Review Agent Run #9d6b54Actionable Suggestions - 0Additional Suggestions - 1
Review 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 |
|
CodeAnt AI is running Incremental review Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
There was a problem hiding this comment.
Code Review Agent Run #edfa64
Actionable Suggestions - 1
-
superset-frontend/src/SqlLab/components/StatusBar/StatusBar.test.tsx - 1
- Jest Mock Missing __esModule · Line 22-29
Review Details
-
Files reviewed - 29 · Commit Range:
f5cc11b..f6e1549- superset-frontend/packages/superset-ui-core/src/components/AsyncAceEditor/index.tsx
- superset-frontend/packages/superset-ui-core/src/components/Icons/AntdEnhanced.tsx
- superset-frontend/src/SqlLab/components/AppLayout/index.tsx
- superset-frontend/src/SqlLab/components/EstimateQueryCostButton/index.tsx
- superset-frontend/src/SqlLab/components/QueryLimitSelect/QueryLimitSelect.test.tsx
- superset-frontend/src/SqlLab/components/QueryLimitSelect/index.tsx
- superset-frontend/src/SqlLab/components/RunQueryActionButton/RunQueryActionButton.test.tsx
- superset-frontend/src/SqlLab/components/RunQueryActionButton/index.tsx
- superset-frontend/src/SqlLab/components/SaveDatasetActionButton/SaveDatasetActionButton.test.tsx
- superset-frontend/src/SqlLab/components/SaveDatasetActionButton/index.tsx
- superset-frontend/src/SqlLab/components/SaveQuery/index.tsx
- superset-frontend/src/SqlLab/components/ShareSqlLabQuery/index.tsx
- superset-frontend/src/SqlLab/components/SouthPane/index.tsx
- superset-frontend/src/SqlLab/components/SqlEditor/SqlEditor.test.tsx
- superset-frontend/src/SqlLab/components/SqlEditor/index.tsx
- superset-frontend/src/SqlLab/components/SqlEditorLeftBar/index.tsx
- superset-frontend/src/SqlLab/components/SqlEditorTopBar/SqlEditorTopBar.test.tsx
- superset-frontend/src/SqlLab/components/SqlEditorTopBar/index.tsx
- superset-frontend/src/SqlLab/components/SqlEditorTopBar/useDatabaseSelector.test.ts
- superset-frontend/src/SqlLab/components/SqlEditorTopBar/useDatabaseSelector.ts
- superset-frontend/src/SqlLab/components/StatusBar/StatusBar.test.tsx
- superset-frontend/src/SqlLab/components/StatusBar/index.tsx
- superset-frontend/src/SqlLab/components/TabbedSqlEditors/index.tsx
- superset-frontend/src/SqlLab/constants.ts
- superset-frontend/src/SqlLab/contributions.ts
- superset-frontend/src/components/MenuExtension/MenuExtension.test.tsx
- superset-frontend/src/components/MenuExtension/index.tsx
- superset-frontend/src/components/ViewExtension/ViewExtension.test.tsx
- superset-frontend/src/components/ViewExtension/index.tsx
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ 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 [email protected].
Documentation & Help
AI Code Review powered by
| jest.mock('src/extensions/ExtensionsManager', () => { | ||
| const getInstance = jest.fn().mockReturnValue({ | ||
| getViewContributions: jest | ||
| .fn() | ||
| .mockReturnValue([{ id: 'test-status-bar' }]), | ||
| }); | ||
| return { getInstance }; | ||
| }); |
There was a problem hiding this comment.
The Jest mock for the default export of ExtensionsManager is missing __esModule: true, which Jest requires for ES module mocking. Without it, the import may not resolve correctly, potentially causing test failures. Other mocks in the codebase, like ViewExtension, include this property.
Code suggestion
Check the AI-generated fix before applying
| jest.mock('src/extensions/ExtensionsManager', () => { | |
| const getInstance = jest.fn().mockReturnValue({ | |
| getViewContributions: jest | |
| .fn() | |
| .mockReturnValue([{ id: 'test-status-bar' }]), | |
| }); | |
| return { getInstance }; | |
| }); | |
| jest.mock('src/extensions/ExtensionsManager', () => { | |
| const getInstance = jest.fn().mockReturnValue({ | |
| getViewContributions: jest | |
| .fn() | |
| .mockReturnValue([{ id: 'test-status-bar' }]), | |
| }); | |
| return { __esModule: true, getInstance }; | |
| }); |
Code Review Run #edfa64
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
@justinpark @michael-s-molina thank you for all the work and changes you did! I think moving the buttons to the left (and adding the text to the primary) make sense. Had to do a little bit of digging to confirm that this is also a pattern that is used, so if that's the case - I am on board :) Some quicks notes on the things I noticed in the screenshots below! Buttons for secondary actionsI think looking at it now, the number of different highlighted buttons/components seem a bit overwhelming - especially the number of secondary actions. Icons also have different colors, which adds to the inconsistency. 
What do you think about changing the secondary action buttons to just icons/dropdown triggers (possibly with dividers)? I think in ant design they are called ghost buttons. Found this example in JetBrains and I really like the clean look. I would still keep run as primary action though. 
SQL AreaSo in here I will be honest - I am not a fan of the gray background for the main/central areas, as not only it can cause accessibility problems, but also it seems outdated. I know I myself put it in here myself, but in the screenshots it seems darker than designs which I think is a problem - so updated proposal below. I would suggest going with a clean, minimalist look, which would be a white background. Was trying to see what some other tools are doing and it's like half and half (white or gray). So basically:
This would also help with seeing which row is actually being highlighted as currently it blends with the gray background. 
Top tabsMaking them smaller - makes sense to me Tabs for results etcWith the gray that is being used for the background (which I didn't consider before, sorry about that) it seems like the tabs are "under" the content, or indented in a way. I would again do white background and a little bit more padding at the top. I know that in general we want to have as much items etc as possible, but with this number of actions it starts to get crowded. 
I think that is all for now, let me know what you think! I am open for discussion 😌 |
|
@kasiazjc I love the proposed changes! I also have a big problem with grey, extra borders, or things that decrease visual clarity. I really prefer a clean look, specially in SQL Lab given that we have so many features available. |
|
Updated design: 
|
|
CodeAnt AI is running Incremental review Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
There was a problem hiding this comment.
Code Review Agent Run #dd9e59
Actionable Suggestions - 1
-
superset-frontend/src/SqlLab/components/SaveQuery/index.tsx - 1
- Icon color styling issue · Line 69-69
Additional Suggestions - 7
-
superset-frontend/src/SqlLab/components/AceEditorWrapper/index.tsx - 2
-
Remove debug console.log · Line 154-154This console.log appears to be debug code for logging cursor position during editor load. It should be removed before merging to avoid cluttering production console output.
Code suggestion
--- a/superset-frontend/src/SqlLab/components/AceEditorWrapper/index.tsx +++ b/superset-frontend/src/SqlLab/components/AceEditorWrapper/index.tsx @@ -152,7 +152,6 @@ // setTimeout(() => { const { row, column } = cursorPosition; - console.log('moving cursor to', { row, column }); editor.moveCursorToPosition({ row, column }); editor.focus(); editor.scrollToLine(row, true, true); // }, 100);
-
Custom CSS addition · Line 195-214The added CSS styles for Ace Editor theming use antd tokens, which is good, but the AGENTS.md guidelines recommend avoiding custom CSS whenever possible. Since this is for a third-party component (Ace Editor), it may be necessary, but verify if Ace's theme configuration can achieve the same result.
-
-
superset-frontend/src/SqlLab/components/SqlEditorTopBar/index.tsx - 1
-
Unconditional UI dividers · Line 51-57The dividers are rendered unconditionally, which may show extra separators if defaultSecondaryActions is empty (since MenuListExtension returns null in that case). Consider checking `defaultSecondaryActions?.length > 0` before rendering the divider, secondary component, and final divider to avoid UI clutter.
-
-
superset-frontend/src/SqlLab/components/EstimateQueryCostButton/index.tsx - 4
-
Unused prop removal · Line 40-40The removal of the unused compactMode prop from the interface cleans up the code without affecting functionality, as it was not passed in any usage.
-
Unused parameter removal · Line 54-54Removing the unused compactMode parameter from the function signature maintains the same behavior, as the default was false and no callers passed it.
-
API modernization · Line 115-117Updating Button props from custom buttonStyle to Antd's native color and variant aligns with modernization guidelines, as verified in Antd 5.26 docs.
-
Conditional cleanup · Line 126-126Removing the conditional rendering of btnText, as compactMode was removed and defaulted to false, ensures text is always shown without changing behavior.
-
Review Details
-
Files reviewed - 31 · Commit Range:
f6e1549..a5b30ad- superset-frontend/packages/superset-ui-core/src/components/AsyncAceEditor/index.tsx
- superset-frontend/packages/superset-ui-core/src/components/Icons/AntdEnhanced.tsx
- superset-frontend/src/SqlLab/components/AceEditorWrapper/index.tsx
- superset-frontend/src/SqlLab/components/AppLayout/index.tsx
- superset-frontend/src/SqlLab/components/EstimateQueryCostButton/EstimateQueryCostButton.test.tsx
- superset-frontend/src/SqlLab/components/EstimateQueryCostButton/index.tsx
- superset-frontend/src/SqlLab/components/QueryLimitSelect/QueryLimitSelect.test.tsx
- superset-frontend/src/SqlLab/components/QueryLimitSelect/index.tsx
- superset-frontend/src/SqlLab/components/RunQueryActionButton/RunQueryActionButton.test.tsx
- superset-frontend/src/SqlLab/components/RunQueryActionButton/index.tsx
- superset-frontend/src/SqlLab/components/SaveDatasetActionButton/SaveDatasetActionButton.test.tsx
- superset-frontend/src/SqlLab/components/SaveDatasetActionButton/index.tsx
- superset-frontend/src/SqlLab/components/SaveQuery/index.tsx
- superset-frontend/src/SqlLab/components/ShareSqlLabQuery/index.tsx
- superset-frontend/src/SqlLab/components/SouthPane/index.tsx
- superset-frontend/src/SqlLab/components/SqlEditor/SqlEditor.test.tsx
- superset-frontend/src/SqlLab/components/SqlEditor/index.tsx
- superset-frontend/src/SqlLab/components/SqlEditorLeftBar/index.tsx
- superset-frontend/src/SqlLab/components/SqlEditorTopBar/SqlEditorTopBar.test.tsx
- superset-frontend/src/SqlLab/components/SqlEditorTopBar/index.tsx
- superset-frontend/src/SqlLab/components/SqlEditorTopBar/useDatabaseSelector.test.ts
- superset-frontend/src/SqlLab/components/SqlEditorTopBar/useDatabaseSelector.ts
- superset-frontend/src/SqlLab/components/StatusBar/StatusBar.test.tsx
- superset-frontend/src/SqlLab/components/StatusBar/index.tsx
- superset-frontend/src/SqlLab/components/TabbedSqlEditors/index.tsx
- superset-frontend/src/SqlLab/constants.ts
- superset-frontend/src/SqlLab/contributions.ts
- superset-frontend/src/components/MenuListExtension/MenuExtension.test.tsx
- superset-frontend/src/components/MenuListExtension/index.tsx
- superset-frontend/src/components/ViewListExtension/ViewExtension.test.tsx
- superset-frontend/src/components/ViewListExtension/index.tsx
-
Files skipped - 0
-
Tools
- Eslint (Linter) - ✔︎ Successful
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ 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 [email protected].
Documentation & Help
AI Code Review powered by
| display: flex; | ||
| margin: 0; | ||
| color: ${({ theme }) => theme.colorIcon}; | ||
| svg { |
There was a problem hiding this comment.
The removal of the color property from the span may cause icons in the save query modal to lose their theme-based color, potentially making them invisible or incorrectly colored. Based on similar patterns in the codebase, the color should be applied to the svg element instead.
Citations
- Rule Violated: AGENTS.md:13
- Rule Violated: AGENTS.md:14
Code Review Run #dd9e59
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
4 participants
User description
SUMMARY
Doc Link
Following changes in the SQL Lab layout structure, the existing SQL Lab actions have been modified so that they are now arranged through the Extension Manager.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION
CodeAnt-AI Description
Pluggable SQL Lab panels and compact editor controls with new tests
What Changed
Impact
✅ Clearer extension-driven editor actions✅ Shorter, icon-first toolbar for compact editor layouts✅ Easier database selection in horizontal/compact UIs💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.