codeant-ai-for-open-source[bot] commented on code in PR #36644:
URL: https://github.com/apache/superset/pull/36644#discussion_r2655887290
##########
superset-frontend/packages/superset-ui-core/src/components/AsyncAceEditor/index.tsx:
##########
@@ -286,6 +287,7 @@ export function AsyncAceEditor(
/* Adjust gutter colors */
.ace_editor .ace_gutter {
+ z-index: 2;
background-color: ${token.colorBgElevated} !important;
Review Comment:
**Suggestion:** Adding `z-index` on `.ace_gutter` without setting a
positioning context (position: relative/absolute/fixed) is ineffective; make
the gutter positioned so the z-index takes effect and the intended stacking
order is reliable. [logic error]
**Severity Level:** Minor β οΈ
```suggestion
position: relative;
```
<details>
<summary><b>Why it matters? β </b></summary>
Technically z-index only affects stacking order on positioned elements;
adding position: relative ensures the gutter's z-index will be honored. It's a
small, targeted CSS fix to make the intent explicit.
Be mindful that adding positioning can subtly affect layout, but in this
case position: relative is the least intrusive option.
</details>
<details>
<summary><b>Prompt for AI Agent π€ </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/packages/superset-ui-core/src/components/AsyncAceEditor/index.tsx
**Line:** 291:291
**Comment:**
*Logic Error: Adding `z-index` on `.ace_gutter` without setting a
positioning context (position: relative/absolute/fixed) is ineffective; make
the gutter positioned so the z-index takes effect and the intended stacking
order is reliable.
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.
```
</details>
##########
superset-frontend/src/SqlLab/components/SaveDatasetActionButton/SaveDatasetActionButton.test.tsx:
##########
@@ -45,10 +41,11 @@ describe('SaveDatasetActionButton', () => {
});
test('renders a "save dataset" dropdown menu item when user clicks caret
button', async () => {
+ const onSaveAsExplore = jest.fn();
render(
<SaveDatasetActionButton
setShowSave={() => true}
- overlayMenu={overlayMenu}
+ onSaveAsExplore={onSaveAsExplore}
/>,
);
Review 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 π¨
```suggestion
await userEvent.click(caretBtn);
```
<details>
<summary><b>Why it matters? β </b></summary>
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.
</details>
<details>
<summary><b>Prompt for AI Agent π€ </b></summary>
```mdx
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.
```
</details>
##########
superset-frontend/src/SqlLab/components/RunQueryActionButton/index.tsx:
##########
@@ -96,12 +96,12 @@ const StyledButton = styled.span`
`;
const RunQueryActionButton = ({
- allowAsync = false,
queryEditorId,
queryState,
overlayCreateAsMenu,
runQuery,
Review 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 β οΈ
```suggestion
allowAsync,
```
<details>
<summary><b>Why it matters? β </b></summary>
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.
</details>
<details>
<summary><b>Prompt for AI Agent π€ </b></summary>
```mdx
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.
```
</details>
##########
superset-frontend/src/SqlLab/components/StatusBar/index.tsx:
##########
@@ -0,0 +1,49 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { styled } from '@apache-superset/core';
+import { Flex } from '@superset-ui/core/components';
+import ViewExtension from 'src/components/ViewExtension';
+import { SQL_EDITOR_STATUSBAR_HEIGHT } from 'src/SqlLab/constants';
+import { ViewContribution } from 'src/SqlLab/contributions';
+
+const Container = styled(Flex)`
+ flex-direction: row-reverse;
+ height: ${SQL_EDITOR_STATUSBAR_HEIGHT}px;
+ background-color: ${({ theme }) => theme.colorPrimary};
+ color: ${({ theme }) => theme.colorWhite};
+ padding: 0 ${({ theme }) => theme.sizeUnit * 4}px;
+
+ & .ant-tag {
+ color: ${({ theme }) => theme.colorWhite};
+ background-color: transparent;
+ border: 0;
+ }
+`;
+
+export interface StatusBarProps {
+ queryEditorId: string;
+}
+
+const StatusBar = () => (
+ <Container align="center" justify="space-bewteen">
Review Comment:
**Suggestion:** Typo in the `justify` prop value: `"space-bewteen"` is
misspelled and won't match the expected CSS/flexbox value (`"space-between"`),
so the layout will not behave as intended. [logic error]
**Severity Level:** Minor β οΈ
```suggestion
<Container align="center" justify="space-between">
```
<details>
<summary><b>Prompt for AI Agent π€ </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/SqlLab/components/StatusBar/index.tsx
**Line:** 44:44
**Comment:**
*Logic Error: Typo in the `justify` prop value: `"space-bewteen"` is
misspelled and won't match the expected CSS/flexbox value (`"space-between"`),
so the layout will not behave as intended.
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.
```
</details>
##########
superset-frontend/src/components/ViewExtension/ViewExtension.test.tsx:
##########
@@ -0,0 +1,202 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { ReactElement } from 'react';
+import { render, screen } from 'spec/helpers/testing-library';
+import type { contributions, core } from '@apache-superset/core';
+import ExtensionsManager from 'src/extensions/ExtensionsManager';
+import { ExtensionsProvider } from 'src/extensions/ExtensionsContext';
+import ViewExtension from 'src/components/ViewExtension';
+
+function createMockView(
+ id: string,
+ overrides: Partial<contributions.ViewContribution> = {},
+): contributions.ViewContribution {
+ return {
+ id,
+ name: `${id} View`,
+ ...overrides,
+ };
+}
+
+function createMockExtension(
+ options: Partial<core.Extension> & {
+ views?: Record<string, contributions.ViewContribution[]>;
+ } = {},
+): core.Extension {
+ const {
+ id = 'test-extension',
+ name = 'Test Extension',
+ views = {},
+ } = options;
+
+ return {
+ id,
+ name,
+ description: 'A test extension',
+ version: '1.0.0',
+ dependencies: [],
+ remoteEntry: '',
+ exposedModules: [],
+ extensionDependencies: [],
+ contributions: {
+ commands: [],
+ menus: {},
+ views,
+ },
+ activate: jest.fn(),
+ deactivate: jest.fn(),
+ };
+}
+
+function setupActivatedExtension(
+ manager: ExtensionsManager,
+ extension: core.Extension,
+) {
+ const context = { disposables: [] };
+ (manager as any).contextIndex.set(extension.id, context);
+ (manager as any).extensionContributions.set(extension.id, {
+ commands: extension.contributions.commands,
+ menus: extension.contributions.menus,
+ views: extension.contributions.views,
+ });
+}
+
+async function createActivatedExtension(
+ manager: ExtensionsManager,
+ extensionOptions: Parameters<typeof createMockExtension>[0] = {},
+): Promise<core.Extension> {
+ const mockExtension = createMockExtension(extensionOptions);
+ await manager.initializeExtension(mockExtension);
+ setupActivatedExtension(manager, mockExtension);
+ return mockExtension;
+}
+
+const TEST_VIEW_ID = 'test.view';
+
+const renderWithExtensionsProvider = (ui: ReactElement) => {
+ return render(ui, { wrapper: ExtensionsProvider as any });
+};
+
+beforeEach(() => {
+ (ExtensionsManager as any).instance = undefined;
+});
+
+afterEach(() => {
Review 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 β οΈ
```suggestion
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();
}
```
<details>
<summary><b>Why it matters? β </b></summary>
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.
</details>
<details>
<summary><b>Prompt for AI Agent π€ </b></summary>
```mdx
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.
```
</details>
##########
superset-frontend/src/components/ViewExtension/ViewExtension.test.tsx:
##########
@@ -0,0 +1,202 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { ReactElement } from 'react';
+import { render, screen } from 'spec/helpers/testing-library';
+import type { contributions, core } from '@apache-superset/core';
+import ExtensionsManager from 'src/extensions/ExtensionsManager';
+import { ExtensionsProvider } from 'src/extensions/ExtensionsContext';
+import ViewExtension from 'src/components/ViewExtension';
+
+function createMockView(
+ id: string,
+ overrides: Partial<contributions.ViewContribution> = {},
+): contributions.ViewContribution {
+ return {
+ id,
+ name: `${id} View`,
+ ...overrides,
+ };
+}
+
+function createMockExtension(
+ options: Partial<core.Extension> & {
+ views?: Record<string, contributions.ViewContribution[]>;
+ } = {},
+): core.Extension {
+ const {
+ id = 'test-extension',
+ name = 'Test Extension',
+ views = {},
+ } = options;
+
+ return {
+ id,
+ name,
+ description: 'A test extension',
+ version: '1.0.0',
+ dependencies: [],
+ remoteEntry: '',
+ exposedModules: [],
+ extensionDependencies: [],
+ contributions: {
+ commands: [],
+ menus: {},
+ views,
+ },
+ activate: jest.fn(),
+ deactivate: jest.fn(),
+ };
+}
+
+function setupActivatedExtension(
+ manager: ExtensionsManager,
+ extension: core.Extension,
+) {
+ const context = { disposables: [] };
+ (manager as any).contextIndex.set(extension.id, context);
+ (manager as any).extensionContributions.set(extension.id, {
+ commands: extension.contributions.commands,
+ menus: extension.contributions.menus,
+ views: extension.contributions.views,
+ });
+}
+
+async function createActivatedExtension(
+ manager: ExtensionsManager,
+ extensionOptions: Parameters<typeof createMockExtension>[0] = {},
+): Promise<core.Extension> {
+ const mockExtension = createMockExtension(extensionOptions);
+ await manager.initializeExtension(mockExtension);
+ setupActivatedExtension(manager, mockExtension);
+ return mockExtension;
+}
+
+const TEST_VIEW_ID = 'test.view';
+
+const renderWithExtensionsProvider = (ui: ReactElement) => {
+ return render(ui, { wrapper: ExtensionsProvider as any });
+};
+
+beforeEach(() => {
+ (ExtensionsManager as any).instance = undefined;
+});
+
+afterEach(() => {
+ (ExtensionsManager as any).instance = undefined;
+});
+
+test('renders nothing when no view contributions exist', () => {
+ const { container } = renderWithExtensionsProvider(
+ <ViewExtension viewId={TEST_VIEW_ID} />,
+ );
+
+ expect(container.firstChild?.childNodes.length ?? 0).toBe(0);
+});
+
+test('renders placeholder for unregistered view provider', async () => {
+ const manager = ExtensionsManager.getInstance();
+
+ await createActivatedExtension(manager, {
+ views: {
+ [TEST_VIEW_ID]: [createMockView('test-view-1')],
+ },
+ });
+
+ renderWithExtensionsProvider(<ViewExtension viewId={TEST_VIEW_ID} />);
+
+ expect(screen.getByText(/test-view-1/)).toBeInTheDocument();
+});
+
+test('renders multiple view placeholders for multiple contributions', async ()
=> {
+ const manager = ExtensionsManager.getInstance();
+
+ await createActivatedExtension(manager, {
+ views: {
+ [TEST_VIEW_ID]: [
+ createMockView('test-view-1'),
+ createMockView('test-view-2'),
+ ],
+ },
+ });
+
+ renderWithExtensionsProvider(<ViewExtension viewId={TEST_VIEW_ID} />);
+
+ expect(screen.getByText(/test-view-1/)).toBeInTheDocument();
+ expect(screen.getByText(/test-view-2/)).toBeInTheDocument();
+});
+
+test('renders nothing for viewId with no matching contributions', () => {
+ const { container } = renderWithExtensionsProvider(
+ <ViewExtension viewId="nonexistent.view" />,
+ );
+
+ expect(container.firstChild?.childNodes.length ?? 0).toBe(0);
+});
+
+test('handles multiple extensions with views for same viewId', async () => {
+ const manager = ExtensionsManager.getInstance();
+
+ await createActivatedExtension(manager, {
+ id: 'extension-1',
+ views: {
+ [TEST_VIEW_ID]: [createMockView('ext1-view')],
+ },
+ });
+
+ await createActivatedExtension(manager, {
+ id: 'extension-2',
+ views: {
+ [TEST_VIEW_ID]: [createMockView('ext2-view')],
+ },
+ });
+
+ renderWithExtensionsProvider(<ViewExtension viewId={TEST_VIEW_ID} />);
+
+ expect(screen.getByText(/ext1-view/)).toBeInTheDocument();
+ expect(screen.getByText(/ext2-view/)).toBeInTheDocument();
+});
+
+test('renders views for different viewIds independently', async () => {
+ const manager = ExtensionsManager.getInstance();
+ const VIEW_ID_A = 'view.a';
+ const VIEW_ID_B = 'view.b';
+
+ await createActivatedExtension(manager, {
+ views: {
+ [VIEW_ID_A]: [createMockView('view-a-component')],
+ [VIEW_ID_B]: [createMockView('view-b-component')],
+ },
+ });
+
+ const { rerender } = renderWithExtensionsProvider(
+ <ViewExtension viewId={VIEW_ID_A} />,
+ );
+
+ expect(screen.getByText(/view-a-component/)).toBeInTheDocument();
+ expect(screen.queryByText(/view-b-component/)).not.toBeInTheDocument();
+
+ rerender(
+ <ExtensionsProvider>
+ <ViewExtension viewId={VIEW_ID_B} />
+ </ExtensionsProvider>,
+ );
Review 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 π¨
```suggestion
rerender(<ViewExtension viewId={VIEW_ID_B} />);
```
<details>
<summary><b>Why it matters? β </b></summary>
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.
</details>
<details>
<summary><b>Prompt for AI Agent π€ </b></summary>
```mdx
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.
```
</details>
##########
superset-frontend/src/SqlLab/components/QueryLimitSelect/index.tsx:
##########
@@ -34,6 +34,19 @@ export function convertToNumWithSpaces(num: number) {
return num.toString().replace(/(\d)(?=(\d{3})+(?!\d))/g, '$1 ');
}
+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;
Review Comment:
**Suggestion:** The function `convertToShortNum` returns numbers in some
branches and strings in others, producing an inconsistent return type
(number|string). This can cause unexpected behavior in callers that expect a
string (rendering, concatenation, or downstream formatting) and defeats
TypeScript's type-safety; make the function always return a string. [type error]
**Severity Level:** Minor β οΈ
```suggestion
export function convertToShortNum(num: number): string {
if (num < 1000) {
return `${num}`;
}
if (num < 1_000_000) {
return `${num / 1000}K`;
}
if (num < 1_000_000_000) {
return `${num / 1_000_000}M`;
}
return `${num}`;
```
<details>
<summary><b>Why it matters? β </b></summary>
The current implementation returns numbers for small and very large inputs
and strings for K/M branches, which is inconsistent and can surprise callers
and renderers. Making the function always return a string fixes the
type/formatting inconsistency and is the correct, minimal change for a UI
formatting helper.
</details>
<details>
<summary><b>Prompt for AI Agent π€ </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/SqlLab/components/QueryLimitSelect/index.tsx
**Line:** 37:47
**Comment:**
*Type Error: The function `convertToShortNum` returns numbers in some
branches and strings in others, producing an inconsistent return type
(number|string). This can cause unexpected behavior in callers that expect a
string (rendering, concatenation, or downstream formatting) and defeats
TypeScript's type-safety; make the function always return a string.
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.
```
</details>
##########
superset-frontend/src/SqlLab/components/RunQueryActionButton/index.tsx:
##########
@@ -138,11 +138,11 @@ const RunQueryActionButton = ({
);
return (
- <StyledButton>
+ <StyledButton compact={compactMode}>
<ButtonComponent
data-test="run-query-action"
onClick={() =>
- onClick(shouldShowStopBtn, allowAsync, runQuery, stopQuery,
logAction)
+ onClick(shouldShowStopBtn, runQuery, stopQuery, logAction)
Review 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 β οΈ
```suggestion
onClick(shouldShowStopBtn, () => runQuery(allowAsync), stopQuery,
logAction)
```
<details>
<summary><b>Why it matters? β </b></summary>
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.
</details>
<details>
<summary><b>Prompt for AI Agent π€ </b></summary>
```mdx
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.
```
</details>
##########
superset-frontend/src/SqlLab/components/SaveDatasetActionButton/index.tsx:
##########
@@ -19,27 +19,61 @@
import { t } from '@superset-ui/core';
import { useTheme } from '@apache-superset/core/ui';
import { Icons } from '@superset-ui/core/components/Icons';
+import { Menu } from '@superset-ui/core/components/Menu';
import { Button, DropdownButton } from '@superset-ui/core/components';
interface SaveDatasetActionButtonProps {
setShowSave: (arg0: boolean) => void;
- overlayMenu: JSX.Element | null;
+ onSaveAsExplore?: () => void;
+ compactMode?: boolean;
}
const SaveDatasetActionButton = ({
setShowSave,
- overlayMenu,
+ onSaveAsExplore,
+ compactMode,
}: SaveDatasetActionButtonProps) => {
const theme = useTheme();
- return !overlayMenu ? (
+ if (compactMode) {
+ return (
+ <>
+ <Button
+ onClick={() => setShowSave(true)}
+ buttonStyle="secondary"
+ icon={<Icons.SaveOutlined />}
+ tooltip={t('Save query')}
+ />
+ {onSaveAsExplore && (
+ <Button
+ onClick={() => onSaveAsExplore?.()}
+ buttonStyle="secondary"
+ icon={<Icons.TableOutlined />}
+ tooltip={t('Save or Overwrite Dataset')}
+ />
+ )}
+ </>
+ );
+ }
+
+ return !onSaveAsExplore ? (
<Button onClick={() => setShowSave(true)} buttonStyle="primary">
{t('Save')}
</Button>
) : (
<DropdownButton
onClick={() => setShowSave(true)}
- popupRender={() => overlayMenu}
+ popupRender={() => (
+ <Menu
+ items={[
+ {
+ label: t('Save dataset'),
+ key: 'save-dataset',
+ onClick: onSaveAsExplore,
+ },
+ ]}
+ />
Review 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 β οΈ
```suggestion
popupRender={() => (
<Menu
items={[
{
label: t('Save dataset'),
key: 'save-dataset',
onClick: () => {
setShowSave(true);
onSaveAsExplore?.();
},
```
<details>
<summary><b>Why it matters? β </b></summary>
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.
</details>
<details>
<summary><b>Prompt for AI Agent π€ </b></summary>
```mdx
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.
```
</details>
##########
superset-frontend/src/SqlLab/components/TabbedSqlEditors/index.tsx:
##########
@@ -41,6 +41,40 @@ const StyledEditableTabs = styled(EditableTabs)`
height: 100%;
display: flex;
flex-direction: column;
+ & .ant-tabs-nav::before {
+ border-color: ${({ theme }) => theme.colorBorder} !important;
+ }
+ & .ant-tabs-nav-add {
+ border-color: ${({ theme }) => theme.colorBorder} !important;
+ height: 34px;
+ }
+ & .ant-tabs-nav-list {
+ align-items: end;
+ padding-top: 1px;
+ column-gap: ${({ theme }) => theme.sizeUnit}px;
+ }
+ & .ant-tabs-tab-active {
+ border-left-color: ${({ theme }) => theme.colorPrimaryActive} !important;
+ 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;
Review Comment:
**Suggestion:** Invalid CSS shorthand: `border-top: 2px;` is not a valid
complete border declaration and will be ignored by browsers (missing border
style and color), so the intended top border will not render. Replace it with a
full border definition (width, style, color) or set explicit
border-top-width/style/color. [possible bug]
**Severity Level:** Critical π¨
```suggestion
border-top: 2px solid ${({ theme }) => theme.colorPrimaryActive}
!important;
```
<details>
<summary><b>Why it matters? β </b></summary>
The existing declaration `border-top: 2px;` is incomplete (missing style and
color) and will be ignored by browsers β this is a real bug in the styling.
Replacing it with a full declaration such as `2px solid <color>` (as suggested)
will actually render the intended top border and align with the other
border-color rules that are being set.
</details>
<details>
<summary><b>Prompt for AI Agent π€ </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/SqlLab/components/TabbedSqlEditors/index.tsx
**Line:** 61:61
**Comment:**
*Possible Bug: Invalid CSS shorthand: `border-top: 2px;` is not a valid
complete border declaration and will be ignored by browsers (missing border
style and color), so the intended top border will not render. Replace it with a
full border definition (width, style, color) or set explicit
border-top-width/style/color.
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.
```
</details>
##########
superset-frontend/src/SqlLab/components/TabbedSqlEditors/index.tsx:
##########
@@ -41,6 +41,40 @@ const StyledEditableTabs = styled(EditableTabs)`
height: 100%;
display: flex;
flex-direction: column;
+ & .ant-tabs-nav::before {
+ border-color: ${({ theme }) => theme.colorBorder} !important;
+ }
+ & .ant-tabs-nav-add {
+ border-color: ${({ theme }) => theme.colorBorder} !important;
+ height: 34px;
+ }
+ & .ant-tabs-nav-list {
+ align-items: end;
Review Comment:
**Suggestion:** Invalid CSS value: `align-items: end;` is not a standard
value for `align-items` (should be `flex-end`, `center`, etc.), so this rule
will be ignored and tabs may be misaligned; change it to `flex-end` to align
items at the end of the cross axis. [possible bug]
**Severity Level:** Critical π¨
```suggestion
align-items: flex-end;
```
<details>
<summary><b>Why it matters? β </b></summary>
`align-items: end;` is not a valid value for flexbox alignment in standard
browsers β the correct value to align to the cross-axis end is `flex-end`.
Changing to `flex-end` fixes alignment reliably across browsers.
</details>
<details>
<summary><b>Prompt for AI Agent π€ </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/SqlLab/components/TabbedSqlEditors/index.tsx
**Line:** 52:52
**Comment:**
*Possible Bug: Invalid CSS value: `align-items: end;` is not a standard
value for `align-items` (should be `flex-end`, `center`, etc.), so this rule
will be ignored and tabs may be misaligned; change it to `flex-end` to align
items at the end of the cross axis.
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.
```
</details>
##########
superset-frontend/src/SqlLab/components/SouthPane/index.tsx:
##########
@@ -25,9 +25,11 @@ import { css, styled, useTheme } from
'@apache-superset/core/ui';
import { removeTables, setActiveSouthPaneTab } from
'src/SqlLab/actions/sqlLab';
-import { Label } from '@superset-ui/core/components';
+import { Flex, Label } from '@superset-ui/core/components';
Review Comment:
**Suggestion:** Import source mistake: `Label` is imported from
'@superset-ui/core/components' but elsewhere in this codebase `Label` is
provided by '@apache-superset/core/ui'. Importing `Label` from the wrong
package can cause the wrong component (or no component) to be referenced at
runtime; split the imports and import `Label` from '@apache-superset/core/ui'
while keeping `Flex` where it belongs. [possible bug]
**Severity Level:** Critical π¨
```suggestion
import { Flex } from '@superset-ui/core/components';
import { Label } from '@apache-superset/core/ui';
```
<details>
<summary><b>Why it matters? β </b></summary>
The current import groups Flex and Label from
'@superset-ui/core/components'. In this repo most UI primitives like Label come
from the '@apache-superset/core/ui' package (see other imports in this file
such as css, styled, useTheme). Importing Label from the wrong package can pick
a different component or fail at runtime. Switching only the Label import to
'@apache-superset/core/ui' is a targeted, correct fix and resolves a potential
runtime/consistency issue.
</details>
<details>
<summary><b>Prompt for AI Agent π€ </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/SqlLab/components/SouthPane/index.tsx
**Line:** 28:28
**Comment:**
*Possible Bug: Import source mistake: `Label` is imported from
'@superset-ui/core/components' but elsewhere in this codebase `Label` is
provided by '@apache-superset/core/ui'. Importing `Label` from the wrong
package can cause the wrong component (or no component) to be referenced at
runtime; split the imports and import `Label` from '@apache-superset/core/ui'
while keeping `Flex` where it belongs.
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.
```
</details>
##########
superset-frontend/src/SqlLab/components/SqlEditorTopBar/SqlEditorTopBar.test.tsx:
##########
@@ -0,0 +1,130 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { render, screen } from 'spec/helpers/testing-library';
+import { MenuItemType } from '@superset-ui/core/components/Menu';
Review Comment:
**Suggestion:** `MenuItemType` is only used for type annotations in the test
mock; importing it as a value import may inadvertently include runtime module
code. Use a type-only import (`import type { MenuItemType } ...`) to ensure
it's erased at compile time and avoid pulling runtime code into tests.
[possible bug]
**Severity Level:** Critical π¨
```suggestion
import type { MenuItemType } from '@superset-ui/core/components/Menu';
```
<details>
<summary><b>Prompt for AI Agent π€ </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/SqlLab/components/SqlEditorTopBar/SqlEditorTopBar.test.tsx
**Line:** 20:20
**Comment:**
*Possible Bug: `MenuItemType` is only used for type annotations in the
test mock; importing it as a value import may inadvertently include runtime
module code. Use a type-only import (`import type { MenuItemType } ...`) to
ensure it's erased at compile time and avoid pulling runtime code into tests.
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.
```
</details>
##########
superset-frontend/src/SqlLab/components/SqlEditorTopBar/useDatabaseSelector.ts:
##########
@@ -0,0 +1,120 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { useEffect, useCallback, useMemo, useState } from 'react';
+import { useDispatch, useSelector } from 'react-redux';
+
+import { SqlLabRootState } from 'src/SqlLab/types';
+import {
+ queryEditorSetDb,
+ queryEditorSetCatalog,
+ queryEditorSetSchema,
+ setDatabases,
+ addDangerToast,
+} from 'src/SqlLab/actions/sqlLab';
+import { type DatabaseObject } from 'src/components';
+import useQueryEditor from 'src/SqlLab/hooks/useQueryEditor';
+import {
+ getItem,
+ LocalStorageKeys,
+ setItem,
+} from 'src/utils/localStorageHelpers';
+
+export default function useDatabaseSelector(queryEditorId: string) {
+ const databases = useSelector<
+ SqlLabRootState,
+ SqlLabRootState['sqlLab']['databases']
+ >(({ sqlLab }) => sqlLab.databases);
+ const dispatch = useDispatch();
+ const queryEditor = useQueryEditor(queryEditorId, [
+ 'dbId',
+ 'catalog',
+ 'schema',
+ 'tabViewId',
+ ]);
+ const database = useMemo(
+ () => (queryEditor.dbId ? databases[queryEditor.dbId] : undefined),
+ [databases, queryEditor.dbId],
+ );
+ const [userSelectedDb, setUserSelected] = useState<DatabaseObject | null>(
+ null,
+ );
+ const { catalog, schema } = queryEditor;
Review Comment:
**Suggestion:** Potential runtime error: destructuring `queryEditor` with
`const { catalog, schema } = queryEditor;` will throw if `queryEditor` is
undefined. Use optional chaining with safe defaults to avoid a crash when
`queryEditor` is not present. [null pointer]
**Severity Level:** Minor β οΈ
```suggestion
const catalog = queryEditor?.catalog ?? null;
const schema = queryEditor?.schema ?? null;
```
<details>
<summary><b>Prompt for AI Agent π€ </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/SqlLab/components/SqlEditorTopBar/useDatabaseSelector.ts
**Line:** 57:57
**Comment:**
*Null Pointer: Potential runtime error: destructuring `queryEditor`
with `const { catalog, schema } = queryEditor;` will throw if `queryEditor` is
undefined. Use optional chaining with safe defaults to avoid a crash when
`queryEditor` is not present.
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.
```
</details>
##########
superset-frontend/src/SqlLab/components/SqlEditor/SqlEditor.test.tsx:
##########
@@ -321,7 +321,7 @@ describe('SqlEditor', () => {
const defaultQueryLimit = 101;
const updatedProps = { ...mockedProps, defaultQueryLimit };
const { findByText } = setup(updatedProps, store);
- fireEvent.click(await findByText('LIMIT:'));
+ fireEvent.click(await findByText('Limit'));
Review Comment:
**Suggestion:** Using `fireEvent.click` on the "Limit" control can fail to
open a Material-UI (or similar) select/dropdown because those components listen
for the `mouseDown` event to open; the test will be flaky or fail because the
dropdown never appears. Replace the click with `fireEvent.mouseDown` to
reliably open the menu. [logic error]
**Severity Level:** Minor β οΈ
```suggestion
fireEvent.mouseDown(await findByText('Limit'));
```
<details>
<summary><b>Why it matters? β </b></summary>
MaterialβUI Select/Menu components often open on mouseDown rather than
click; replacing the click with fireEvent.mouseDown is a pragmatic, accurate
fix that prevents a flaky test. The existing code matches the PR hunk and the
improved code is syntactically correct and scoped to the test.
</details>
<details>
<summary><b>Prompt for AI Agent π€ </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/SqlLab/components/SqlEditor/SqlEditor.test.tsx
**Line:** 324:324
**Comment:**
*Logic Error: Using `fireEvent.click` on the "Limit" control can fail
to open a Material-UI (or similar) select/dropdown because those components
listen for the `mouseDown` event to open; the test will be flaky or fail
because the dropdown never appears. Replace the click with
`fireEvent.mouseDown` to reliably open the menu.
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.
```
</details>
##########
superset-frontend/src/SqlLab/components/SqlEditorTopBar/useDatabaseSelector.test.ts:
##########
@@ -0,0 +1,320 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import configureStore from 'redux-mock-store';
+import thunk from 'redux-thunk';
+import { renderHook, act } from '@testing-library/react-hooks';
+import { createWrapper } from 'spec/helpers/testing-library';
+import { initialState, defaultQueryEditor } from 'src/SqlLab/fixtures';
+import * as localStorageHelpers from 'src/utils/localStorageHelpers';
+
+import useDatabaseSelector from './useDatabaseSelector';
+
+const middlewares = [thunk];
+const mockStore = configureStore(middlewares);
+
+const mockDatabase = {
+ id: 1,
+ database_name: 'main',
+ backend: 'mysql',
+};
+
+const mockDatabases = {
+ [mockDatabase.id]: mockDatabase,
+};
+
+const createInitialState = (overrides = {}) => ({
+ ...initialState,
+ sqlLab: {
+ ...initialState.sqlLab,
+ databases: mockDatabases,
+ ...overrides,
+ },
+});
+
+beforeEach(() => {
+ jest.spyOn(localStorageHelpers, 'getItem').mockReturnValue(null);
+ jest.spyOn(localStorageHelpers, 'setItem').mockImplementation(() => {});
+});
+
+afterEach(() => {
+ jest.clearAllMocks();
+ jest.restoreAllMocks();
+});
+
+test('returns initial values from query editor', () => {
+ const store = mockStore(createInitialState());
+ const { result } = renderHook(
+ () => useDatabaseSelector(defaultQueryEditor.id),
+ {
+ wrapper: createWrapper({
+ useRedux: true,
+ store,
+ }),
+ },
+ );
+
+ expect(result.current.catalog).toBe(defaultQueryEditor.catalog);
+ expect(result.current.schema).toBe(defaultQueryEditor.schema);
+ expect(typeof result.current.onDbChange).toBe('function');
+ expect(typeof result.current.onCatalogChange).toBe('function');
+ expect(typeof result.current.onSchemaChange).toBe('function');
+ expect(typeof result.current.getDbList).toBe('function');
+ expect(typeof result.current.handleError).toBe('function');
+});
+
+test('returns database when dbId exists in store', () => {
+ const store = mockStore(
+ createInitialState({
+ unsavedQueryEditor: {
+ id: defaultQueryEditor.id,
+ dbId: mockDatabase.id,
+ },
+ }),
+ );
+
+ const { result, rerender } = renderHook(
+ () => useDatabaseSelector(defaultQueryEditor.id),
+ {
+ wrapper: createWrapper({
+ useRedux: true,
+ store,
+ }),
+ },
+ );
+
+ // Trigger effect by rerendering
+ rerender();
Review Comment:
**Suggestion:** Race condition / missing act: the test triggers a state
update in a useEffect by calling `rerender()` but does not await it or wrap it
in `act`, so the assertion can run before React finishes updating state and
cause flaky failures; mark the test async and wrap `rerender()` in `await
act(async () => { ... })`. [race condition]
**Severity Level:** Minor β οΈ
```suggestion
test('returns database when dbId exists in store', async () => {
const store = mockStore(
createInitialState({
unsavedQueryEditor: {
id: defaultQueryEditor.id,
dbId: mockDatabase.id,
},
}),
);
const { result, rerender } = renderHook(
() => useDatabaseSelector(defaultQueryEditor.id),
{
wrapper: createWrapper({
useRedux: true,
store,
}),
},
);
// Trigger effect by rerendering and wait for state updates
await act(async () => {
rerender();
});
```
<details>
<summary><b>Why it matters? β </b></summary>
The test triggers an effect-driven state change via rerender() and does not
wrap/await it in act; that can produce flakiness and React warnings. Changing
the test to async and awaiting the rerender inside act resolves a real
timing/race issue in tests.
</details>
<details>
<summary><b>Prompt for AI Agent π€ </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/SqlLab/components/SqlEditorTopBar/useDatabaseSelector.test.ts
**Line:** 81:102
**Comment:**
*Race Condition: Race condition / missing act: the test triggers a
state update in a useEffect by calling `rerender()` but does not await it or
wrap it in `act`, so the assertion can run before React finishes updating state
and cause flaky failures; mark the test async and wrap `rerender()` in `await
act(async () => { ... })`.
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.
```
</details>
##########
superset-frontend/src/SqlLab/components/SqlEditorTopBar/useDatabaseSelector.ts:
##########
@@ -0,0 +1,120 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { useEffect, useCallback, useMemo, useState } from 'react';
+import { useDispatch, useSelector } from 'react-redux';
+
+import { SqlLabRootState } from 'src/SqlLab/types';
+import {
+ queryEditorSetDb,
+ queryEditorSetCatalog,
+ queryEditorSetSchema,
+ setDatabases,
+ addDangerToast,
+} from 'src/SqlLab/actions/sqlLab';
+import { type DatabaseObject } from 'src/components';
+import useQueryEditor from 'src/SqlLab/hooks/useQueryEditor';
+import {
+ getItem,
+ LocalStorageKeys,
+ setItem,
+} from 'src/utils/localStorageHelpers';
+
+export default function useDatabaseSelector(queryEditorId: string) {
+ const databases = useSelector<
+ SqlLabRootState,
+ SqlLabRootState['sqlLab']['databases']
+ >(({ sqlLab }) => sqlLab.databases);
+ const dispatch = useDispatch();
+ const queryEditor = useQueryEditor(queryEditorId, [
+ 'dbId',
+ 'catalog',
+ 'schema',
+ 'tabViewId',
+ ]);
+ const database = useMemo(
+ () => (queryEditor.dbId ? databases[queryEditor.dbId] : undefined),
+ [databases, queryEditor.dbId],
Review Comment:
**Suggestion:** Logic bug: the truthy check `queryEditor.dbId ? ... :
undefined` treats a valid database id of 0 as falsy and will incorrectly return
undefined instead of the database; also the dependency array reads
`queryEditor.dbId` which will throw if `queryEditor` is null/undefined. Change
to use a null/undefined check and optional chaining in the dependency list so
dbId === 0 is handled and safe when `queryEditor` may be absent. [logic error]
**Severity Level:** Minor β οΈ
```suggestion
() =>
queryEditor?.dbId != null
? databases[queryEditor.dbId]
: undefined,
[databases, queryEditor?.dbId],
```
<details>
<summary><b>Prompt for AI Agent π€ </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/SqlLab/components/SqlEditorTopBar/useDatabaseSelector.ts
**Line:** 51:52
**Comment:**
*Logic Error: Logic bug: the truthy check `queryEditor.dbId ? ... :
undefined` treats a valid database id of 0 as falsy and will incorrectly return
undefined instead of the database; also the dependency array reads
`queryEditor.dbId` which will throw if `queryEditor` is null/undefined. Change
to use a null/undefined check and optional chaining in the dependency list so
dbId === 0 is handled and safe when `queryEditor` may be absent.
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.
```
</details>
##########
superset-frontend/src/SqlLab/components/SqlEditorTopBar/index.tsx:
##########
@@ -0,0 +1,55 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { Flex } from '@superset-ui/core/components';
Review Comment:
**Suggestion:** The file references the type `React.ReactNode` but does not
import React; add an explicit React import so the `React` symbol is available
for type annotations and to avoid relying on global ambient typings or a
specific TS JSX configuration. [type error]
**Severity Level:** Minor β οΈ
```suggestion
import * as React from 'react';
```
<details>
<summary><b>Prompt for AI Agent π€ </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/SqlLab/components/SqlEditorTopBar/index.tsx
**Line:** 19:19
**Comment:**
*Type Error: The file references the type `React.ReactNode` but does
not import React; add an explicit React import so the `React` symbol is
available for type annotations and to avoid relying on global ambient typings
or a specific TS JSX configuration.
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.
```
</details>
##########
superset-frontend/src/SqlLab/components/StatusBar/StatusBar.test.tsx:
##########
@@ -0,0 +1,45 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { render, screen } from 'spec/helpers/testing-library';
+import StatusBar from 'src/SqlLab/components/StatusBar';
+
+jest.mock('src/components/ViewExtension', () => ({
+ __esModule: true,
+ default: ({ viewId }: { viewId: string }) => (
+ <div data-test="mock-view-extension" data-view-id={viewId}>
Review Comment:
**Suggestion:** Test queries use getByTestId (which looks for the
data-testid attribute) but the mocked component sets data-test, so getByTestId
will not find the element and the tests will fail; change the attribute to
data-testid. [possible bug]
**Severity Level:** Critical π¨
```suggestion
<div data-testid="mock-view-extension" data-view-id={viewId}>
```
<details>
<summary><b>Why it matters? β </b></summary>
The tests call screen.getByTestId('mock-view-extension') but the mock sets
data-test, so the queries will fail to locate the mocked element. Changing the
mock to use data-testid fixes a real test failure without side effects.
</details>
<details>
<summary><b>Prompt for AI Agent π€ </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/SqlLab/components/StatusBar/StatusBar.test.tsx
**Line:** 25:25
**Comment:**
*Possible Bug: Test queries use getByTestId (which looks for the
data-testid attribute) but the mocked component sets data-test, so getByTestId
will not find the element and the tests will fail; change the attribute to
data-testid.
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.
```
</details>
##########
superset-frontend/src/SqlLab/components/SqlEditorTopBar/SqlEditorTopBar.test.tsx:
##########
@@ -0,0 +1,130 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { render, screen } from 'spec/helpers/testing-library';
+import { MenuItemType } from '@superset-ui/core/components/Menu';
+import SqlEditorTopBar, {
+ SqlEditorTopBarProps,
+} from 'src/SqlLab/components/SqlEditorTopBar';
+
+jest.mock('src/components/MenuExtension', () => ({
+ __esModule: true,
+ default: ({
+ children,
+ viewId,
+ primary,
+ secondary,
+ defaultItems,
+ }: {
+ children?: React.ReactNode;
+ viewId: string;
+ primary?: boolean;
+ secondary?: boolean;
+ defaultItems?: MenuItemType[];
+ }) => (
+ <div
+ data-test="mock-menu-extension"
Review Comment:
**Suggestion:** The test mock sets the attribute
`data-test="mock-menu-extension"` but the tests call
`screen.getAllByTestId('mock-menu-extension')` which queries elements by
`data-testid`. This mismatch causes the queries to fail at runtime (no elements
found). Change the mock to set `data-testid` instead of `data-test`. [possible
bug]
**Severity Level:** Critical π¨
```suggestion
data-testid="mock-menu-extension"
```
<details>
<summary><b>Prompt for AI Agent π€ </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/SqlLab/components/SqlEditorTopBar/SqlEditorTopBar.test.tsx
**Line:** 41:41
**Comment:**
*Possible Bug: The test mock sets the attribute
`data-test="mock-menu-extension"` but the tests call
`screen.getAllByTestId('mock-menu-extension')` which queries elements by
`data-testid`. This mismatch causes the queries to fail at runtime (no elements
found). Change the mock to set `data-testid` instead of `data-test`.
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.
```
</details>
##########
superset-frontend/src/components/MenuExtension/index.tsx:
##########
@@ -0,0 +1,147 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { useMemo } from 'react';
+import { css, useTheme } from '@apache-superset/core/ui';
+import { Button, Dropdown } from '@superset-ui/core/components';
+import { Menu, MenuItemType } from '@superset-ui/core/components/Menu';
+import { Icons } from '@superset-ui/core/components/Icons';
+import { commands } from 'src/core';
+import ExtensionsManager from 'src/extensions/ExtensionsManager';
+
+export type MenuExtensionProps = {
+ viewId: string;
+} & (
+ | {
+ primary: boolean;
+ secondary?: never;
+ children?: React.ReactNode;
+ defaultItems?: never;
+ compactMode?: boolean;
+ }
+ | {
+ primary?: never;
+ secondary: boolean;
+ children?: never;
+ defaultItems?: MenuItemType[];
+ compactMode?: never;
+ }
+);
+
+const MenuExtension = ({
+ viewId,
+ primary,
+ secondary,
+ defaultItems,
+ children,
+ compactMode,
+}: MenuExtensionProps) => {
+ const theme = useTheme();
+ const iconColor = theme.colorPrimary;
+ const contributions =
+ ExtensionsManager.getInstance().getMenuContributions(viewId);
+
+ const actions = primary ? contributions?.primary : contributions?.secondary;
+ const primaryActions = useMemo(
+ () =>
+ primary
+ ? (actions || []).map(contribution => {
+ const command =
+ ExtensionsManager.getInstance().getCommandContribution(
+ contribution.command,
+ )!;
+ // @ts-ignore
+ const Icon = Icons[command?.icon as IconNameType];
+
+ return (
+ <Button
+ key={contribution.view}
+ onClick={() => commands.executeCommand(command.command)}
+ tooltip={command?.description}
+ icon={<Icon iconSize="m" iconColor={iconColor} />}
+ buttonSize="small"
+ >
+ {!compactMode ? command?.title : undefined}
+ </Button>
+ );
+ })
+ : [],
+ [actions, primary, iconColor, compactMode],
+ );
+ const secondaryActions = useMemo(
+ () =>
+ secondary
+ ? (actions || [])
+ .map(contribution => {
+ const command =
+ ExtensionsManager.getInstance().getCommandContribution(
+ contribution.command,
+ )!;
+ return {
+ key: command.command,
+ label: command.title,
+ title: command.description,
+ onClick: () => commands.executeCommand(command.command),
+ } as MenuItemType;
+ })
+ .concat(...(defaultItems || []))
Review Comment:
**Suggestion:** Secondary menu items mapping uses a non-null assertion and
will crash if `getCommandContribution` returns undefined; also the code uses
`concat(...(defaultItems || []))` which unnecessarily spreads the array and is
error-proneβchange to check for missing commands and use `concat(defaultItems
|| [])`. [possible bug]
**Severity Level:** Critical π¨
```suggestion
const command =
ExtensionsManager.getInstance().getCommandContribution(
contribution.command,
);
if (!command) {
return null;
}
return {
key: command.command,
label: command.title,
title: command.description,
onClick: () => commands.executeCommand(command.command),
} as MenuItemType;
})
.concat(defaultItems || [])
```
<details>
<summary><b>Why it matters? β </b></summary>
This hunk also uses the non-null assertion which can cause the same runtime
crash if a command isn't found. Additionally, .concat(...(defaultItems || []))
is unnecessary and harder to reason about β concat(defaultItems || []) is the
correct, simpler usage. The proposed fix removes the crash vector and
simplifies concatenation.
</details>
<details>
<summary><b>Prompt for AI Agent π€ </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/components/MenuExtension/index.tsx
**Line:** 91:102
**Comment:**
*Possible Bug: Secondary menu items mapping uses a non-null assertion
and will crash if `getCommandContribution` returns undefined; also the code
uses `concat(...(defaultItems || []))` which unnecessarily spreads the array
and is error-proneβchange to check for missing commands and use
`concat(defaultItems || [])`.
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.
```
</details>
##########
superset-frontend/src/components/MenuExtension/index.tsx:
##########
@@ -0,0 +1,147 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { useMemo } from 'react';
+import { css, useTheme } from '@apache-superset/core/ui';
+import { Button, Dropdown } from '@superset-ui/core/components';
+import { Menu, MenuItemType } from '@superset-ui/core/components/Menu';
+import { Icons } from '@superset-ui/core/components/Icons';
+import { commands } from 'src/core';
+import ExtensionsManager from 'src/extensions/ExtensionsManager';
+
+export type MenuExtensionProps = {
+ viewId: string;
+} & (
+ | {
+ primary: boolean;
+ secondary?: never;
+ children?: React.ReactNode;
+ defaultItems?: never;
+ compactMode?: boolean;
+ }
+ | {
+ primary?: never;
+ secondary: boolean;
+ children?: never;
+ defaultItems?: MenuItemType[];
+ compactMode?: never;
+ }
+);
+
+const MenuExtension = ({
+ viewId,
+ primary,
+ secondary,
+ defaultItems,
+ children,
+ compactMode,
+}: MenuExtensionProps) => {
+ const theme = useTheme();
+ const iconColor = theme.colorPrimary;
+ const contributions =
+ ExtensionsManager.getInstance().getMenuContributions(viewId);
+
+ const actions = primary ? contributions?.primary : contributions?.secondary;
+ const primaryActions = useMemo(
+ () =>
+ primary
+ ? (actions || []).map(contribution => {
+ const command =
+ ExtensionsManager.getInstance().getCommandContribution(
+ contribution.command,
+ )!;
+ // @ts-ignore
+ const Icon = Icons[command?.icon as IconNameType];
+
+ return (
+ <Button
+ key={contribution.view}
+ onClick={() => commands.executeCommand(command.command)}
+ tooltip={command?.description}
+ icon={<Icon iconSize="m" iconColor={iconColor} />}
Review Comment:
**Suggestion:** Dynamic icon lookup can yield undefined (or a non-component)
and rendering `<Icon />` will throw "Element type is invalid"; ensure the
looked-up `Icon` is a valid component and render a safe fallback or omit the
icon. [type error]
**Severity Level:** Minor β οΈ
```suggestion
const Icon = Icons[command?.icon as any];
return (
<Button
key={contribution.view}
onClick={() => commands.executeCommand(command.command)}
tooltip={command?.description}
icon={Icon ? <Icon iconSize="m" iconColor={iconColor} /> :
undefined}
```
<details>
<summary><b>Why it matters? β </b></summary>
The Icons lookup can return undefined or a non-component value; rendering
<Icon /> in that case throws "Element type is invalid". The improved code
checks Icon and conditionally renders the icon (or omits it), which prevents
that runtime error and is a minimal, correct safeguard.
</details>
<details>
<summary><b>Prompt for AI Agent π€ </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/components/MenuExtension/index.tsx
**Line:** 69:76
**Comment:**
*Type Error: Dynamic icon lookup can yield undefined (or a
non-component) and rendering `<Icon />` will throw "Element type is invalid";
ensure the looked-up `Icon` is a valid component and render a safe fallback or
omit the icon.
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.
```
</details>
##########
superset-frontend/src/components/ViewExtension/index.tsx:
##########
@@ -16,18 +16,19 @@
* specific language governing permissions and limitations
* under the License.
*/
+import ExtensionsManager from 'src/extensions/ExtensionsManager';
+import { useExtensionsContext } from 'src/extensions/ExtensionsContext';
-/**
- * Jest configuration for @storybook/test-runner
- *
- * This extends the default test-runner config with custom timeouts
- * to handle slow story rendering in CI environments.
- */
-const { getJestConfig } = require('@storybook/test-runner');
-const testRunnerConfig = getJestConfig();
+export interface ViewExtensionProps {
+ viewId: string;
+}
-module.exports = {
- ...testRunnerConfig,
- // Increase timeout from default 15s to 60s for CI environments
- testTimeout: 60000,
+const ViewExtension = ({ viewId }: ViewExtensionProps) => {
+ const contributions =
+ ExtensionsManager.getInstance().getViewContributions(viewId) || [];
+ const { getView } = useExtensionsContext();
+
+ return <>{contributions.map(contribution => getView(contribution.id))}</>;
Review 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 π¨
```suggestion
return (
<>
{contributions
.filter(contribution => contribution && typeof contribution.id !==
'undefined')
.map(contribution => (
<span
key={String(contribution.id)}>{getView(contribution.id)}</span>
))}
</>
);
```
<details>
<summary><b>Why it matters? β </b></summary>
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 <span> which
will add extra DOM; prefer using <React.Fragment key={...}> to avoid altering
markup or ensure getView already returns keyed elements.
</details>
<details>
<summary><b>Prompt for AI Agent π€ </b></summary>
```mdx
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.
```
</details>
##########
superset-frontend/src/components/MenuExtension/MenuExtension.test.tsx:
##########
@@ -0,0 +1,374 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { render, screen, waitFor } from 'spec/helpers/testing-library';
+import userEvent from '@testing-library/user-event';
+import type { contributions, core } from '@apache-superset/core';
+import ExtensionsManager from 'src/extensions/ExtensionsManager';
+import { commands } from 'src/core';
+import MenuExtension from 'src/components/MenuExtension';
+
+jest.mock('src/core', () => ({
+ commands: {
+ executeCommand: jest.fn(),
+ },
+}));
+
+function createMockCommand(
+ command: string,
+ overrides: Partial<contributions.CommandContribution> = {},
+): contributions.CommandContribution {
+ return {
+ command,
+ icon: 'PlusOutlined',
+ title: `${command} Title`,
+ description: `${command} description`,
+ ...overrides,
+ };
+}
+
+function createMockMenuItem(
+ view: string,
+ command: string,
+): contributions.MenuItem {
+ return {
+ view,
+ command,
+ };
+}
+
+function createMockMenu(
+ overrides: Partial<contributions.MenuContribution> = {},
+): contributions.MenuContribution {
+ return {
+ context: [],
+ primary: [],
+ secondary: [],
+ ...overrides,
+ };
+}
+
+function createMockExtension(
+ options: Partial<core.Extension> & {
+ commands?: contributions.CommandContribution[];
+ menus?: Record<string, contributions.MenuContribution>;
+ } = {},
+): core.Extension {
+ const {
+ id = 'test-extension',
+ name = 'Test Extension',
+ commands: cmds = [],
+ menus = {},
+ } = options;
+
+ return {
+ id,
+ name,
+ description: 'A test extension',
+ version: '1.0.0',
+ dependencies: [],
+ remoteEntry: '',
+ exposedModules: [],
+ extensionDependencies: [],
+ contributions: {
+ commands: cmds,
+ menus,
+ views: {},
+ },
+ activate: jest.fn(),
+ deactivate: jest.fn(),
+ };
+}
+
+function setupActivatedExtension(
+ manager: ExtensionsManager,
+ extension: core.Extension,
+) {
+ const context = { disposables: [] };
+ (manager as any).contextIndex.set(extension.id, context);
+ (manager as any).extensionContributions.set(extension.id, {
+ commands: extension.contributions.commands,
+ menus: extension.contributions.menus,
+ views: extension.contributions.views,
+ });
+}
+
+async function createActivatedExtension(
+ manager: ExtensionsManager,
+ extensionOptions: Parameters<typeof createMockExtension>[0] = {},
+): Promise<core.Extension> {
+ const mockExtension = createMockExtension(extensionOptions);
+ await manager.initializeExtension(mockExtension);
+ setupActivatedExtension(manager, mockExtension);
+ return mockExtension;
+}
+
+const TEST_VIEW_ID = 'test.menu';
+
+beforeEach(() => {
+ (ExtensionsManager as any).instance = undefined;
+ jest.clearAllMocks();
+});
+
+afterEach(() => {
+ (ExtensionsManager as any).instance = undefined;
+});
+
+test('renders children when primary mode with no extensions', () => {
+ render(
+ <MenuExtension viewId={TEST_VIEW_ID} primary>
+ <button type="button">Child Button</button>
+ </MenuExtension>,
+ );
+
+ expect(
+ screen.getByRole('button', { name: 'Child Button' }),
+ ).toBeInTheDocument();
+});
+
+test('renders primary actions from extension contributions', async () => {
+ const manager = ExtensionsManager.getInstance();
+
+ await createActivatedExtension(manager, {
+ commands: [createMockCommand('test.action')],
+ menus: {
+ [TEST_VIEW_ID]: createMockMenu({
+ primary: [createMockMenuItem('test-view', 'test.action')],
+ }),
+ },
+ });
+
+ render(<MenuExtension viewId={TEST_VIEW_ID} primary />);
+
+ expect(screen.getByText('test.action Title')).toBeInTheDocument();
+});
+
+test('renders primary actions with children', async () => {
+ const manager = ExtensionsManager.getInstance();
+
+ await createActivatedExtension(manager, {
+ commands: [createMockCommand('test.action')],
+ menus: {
+ [TEST_VIEW_ID]: createMockMenu({
+ primary: [createMockMenuItem('test-view', 'test.action')],
+ }),
+ },
+ });
+
+ render(
+ <MenuExtension viewId={TEST_VIEW_ID} primary>
+ <button type="button">Child Button</button>
+ </MenuExtension>,
+ );
+
+ expect(screen.getByText('test.action Title')).toBeInTheDocument();
+ expect(
+ screen.getByRole('button', { name: 'Child Button' }),
+ ).toBeInTheDocument();
+});
+
+test('hides title in compact mode for primary actions', async () => {
+ const manager = ExtensionsManager.getInstance();
+
+ await createActivatedExtension(manager, {
+ commands: [createMockCommand('test.action')],
+ menus: {
+ [TEST_VIEW_ID]: createMockMenu({
+ primary: [createMockMenuItem('test-view', 'test.action')],
+ }),
+ },
+ });
+
+ render(<MenuExtension viewId={TEST_VIEW_ID} primary compactMode />);
+
+ expect(screen.queryByText('test.action Title')).not.toBeInTheDocument();
+ expect(screen.getByRole('button')).toBeInTheDocument();
+});
+
+test('executes command when primary action button is clicked', async () => {
+ const manager = ExtensionsManager.getInstance();
+
+ await createActivatedExtension(manager, {
+ commands: [createMockCommand('test.action')],
+ menus: {
+ [TEST_VIEW_ID]: createMockMenu({
+ primary: [createMockMenuItem('test-view', 'test.action')],
+ }),
+ },
+ });
+
+ render(<MenuExtension viewId={TEST_VIEW_ID} primary />);
+
+ const button = screen.getByText('test.action Title').closest('button')!;
Review 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 π¨
```suggestion
const button = screen.getByRole('button', { name: 'test.action Title' });
```
<details>
<summary><b>Why it matters? β </b></summary>
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.
</details>
<details>
<summary><b>Prompt for AI Agent π€ </b></summary>
```mdx
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.
```
</details>
##########
superset-frontend/src/SqlLab/components/SqlEditorTopBar/useDatabaseSelector.test.ts:
##########
@@ -0,0 +1,320 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import configureStore from 'redux-mock-store';
+import thunk from 'redux-thunk';
+import { renderHook, act } from '@testing-library/react-hooks';
+import { createWrapper } from 'spec/helpers/testing-library';
+import { initialState, defaultQueryEditor } from 'src/SqlLab/fixtures';
+import * as localStorageHelpers from 'src/utils/localStorageHelpers';
+
+import useDatabaseSelector from './useDatabaseSelector';
+
+const middlewares = [thunk];
+const mockStore = configureStore(middlewares);
+
+const mockDatabase = {
+ id: 1,
+ database_name: 'main',
+ backend: 'mysql',
+};
+
+const mockDatabases = {
+ [mockDatabase.id]: mockDatabase,
+};
+
+const createInitialState = (overrides = {}) => ({
+ ...initialState,
+ sqlLab: {
+ ...initialState.sqlLab,
+ databases: mockDatabases,
+ ...overrides,
+ },
+});
+
+beforeEach(() => {
+ jest.spyOn(localStorageHelpers, 'getItem').mockReturnValue(null);
+ jest.spyOn(localStorageHelpers, 'setItem').mockImplementation(() => {});
+});
+
+afterEach(() => {
+ jest.clearAllMocks();
+ jest.restoreAllMocks();
+});
+
+test('returns initial values from query editor', () => {
+ const store = mockStore(createInitialState());
+ const { result } = renderHook(
+ () => useDatabaseSelector(defaultQueryEditor.id),
+ {
+ wrapper: createWrapper({
+ useRedux: true,
+ store,
+ }),
+ },
+ );
+
+ expect(result.current.catalog).toBe(defaultQueryEditor.catalog);
+ expect(result.current.schema).toBe(defaultQueryEditor.schema);
+ expect(typeof result.current.onDbChange).toBe('function');
+ expect(typeof result.current.onCatalogChange).toBe('function');
+ expect(typeof result.current.onSchemaChange).toBe('function');
+ expect(typeof result.current.getDbList).toBe('function');
+ expect(typeof result.current.handleError).toBe('function');
+});
+
+test('returns database when dbId exists in store', () => {
+ const store = mockStore(
+ createInitialState({
+ unsavedQueryEditor: {
+ id: defaultQueryEditor.id,
+ dbId: mockDatabase.id,
+ },
+ }),
+ );
+
+ const { result, rerender } = renderHook(
+ () => useDatabaseSelector(defaultQueryEditor.id),
+ {
+ wrapper: createWrapper({
+ useRedux: true,
+ store,
+ }),
+ },
+ );
+
+ // Trigger effect by rerendering
+ rerender();
+
+ expect(result.current.db).toEqual(mockDatabase);
+});
+
+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,
+ }),
+ );
+});
+
+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',
+ }),
+ );
+});
+
+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',
+ }),
+ );
+});
+
+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',
+ }),
+ );
+});
+
+test('dispatches addDangerToast action on handleError', () => {
+ const store = mockStore(createInitialState());
+ const { result } = renderHook(
+ () => useDatabaseSelector(defaultQueryEditor.id),
+ {
+ wrapper: createWrapper({
+ useRedux: true,
+ store,
+ }),
+ },
+ );
+
+ act(() => {
+ result.current.handleError('Test error message');
+ });
+
+ const actions = store.getActions();
+ expect(actions).toContainEqual(
+ expect.objectContaining({
+ type: 'ADD_TOAST',
+ payload: expect.objectContaining({
+ toastType: 'DANGER_TOAST',
+ text: 'Test error message',
+ }),
+ }),
+ );
+});
+
+test('reads database from localStorage when URL has db param', () => {
+ const localStorageDb = {
+ id: 5,
+ database_name: 'local_storage_db',
+ backend: 'sqlite',
+ };
+
+ jest.spyOn(localStorageHelpers, 'getItem').mockReturnValue(localStorageDb);
+
+ const originalLocation = window.location;
+ Object.defineProperty(window, 'location', {
+ value: { search: '?db=true' },
+ writable: true,
+ });
+
+ const store = mockStore(createInitialState());
+ 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,
+ );
+
+ Object.defineProperty(window, 'location', {
+ value: originalLocation,
+ writable: true,
+ });
Review Comment:
**Suggestion:** Test state leak / teardown bug: the test mutates
`window.location` and restores it only at the end of the test; if an assertion
throws before the restore, subsequent tests will run with the modified
location. Wrap the location mutation, render, assertions and restore in a
try/finally and await rerender with `act` to ensure proper cleanup and to avoid
flakiness. [possible bug]
**Severity Level:** Critical π¨
```suggestion
test('reads database from localStorage when URL has db param', async () => {
const localStorageDb = {
id: 5,
database_name: 'local_storage_db',
backend: 'sqlite',
};
jest.spyOn(localStorageHelpers, 'getItem').mockReturnValue(localStorageDb);
const originalLocation = window.location;
Object.defineProperty(window, 'location', {
value: { search: '?db=true' },
writable: true,
});
try {
const store = mockStore(createInitialState());
const { result, rerender } = renderHook(
() => useDatabaseSelector(defaultQueryEditor.id),
{
wrapper: createWrapper({
useRedux: true,
store,
}),
},
);
// wait for effect-driven state updates
await act(async () => {
rerender();
});
expect(result.current.db).toEqual(localStorageDb);
expect(localStorageHelpers.setItem).toHaveBeenCalledWith(
localStorageHelpers.LocalStorageKeys.Database,
null,
);
} finally {
// always restore global location
Object.defineProperty(window, 'location', {
value: originalLocation,
writable: true,
});
}
```
<details>
<summary><b>Why it matters? β </b></summary>
Mutating window.location without guaranteeing restoration risks leaking
state across tests if an assertion fails. Wrapping the mutation and restoration
in a try/finally and awaiting effect-driven updates (using act) is a correct
and practical fix for test reliability.
</details>
<details>
<summary><b>Prompt for AI Agent π€ </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/SqlLab/components/SqlEditorTopBar/useDatabaseSelector.test.ts
**Line:** 238:275
**Comment:**
*Possible Bug: Test state leak / teardown bug: the test mutates
`window.location` and restores it only at the end of the test; if an assertion
throws before the restore, subsequent tests will run with the modified
location. Wrap the location mutation, render, assertions and restore in a
try/finally and await rerender with `act` to ensure proper cleanup and to avoid
flakiness.
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.
```
</details>
##########
superset-frontend/src/components/ViewExtension/index.tsx:
##########
@@ -16,18 +16,19 @@
* specific language governing permissions and limitations
* under the License.
*/
+import ExtensionsManager from 'src/extensions/ExtensionsManager';
+import { useExtensionsContext } from 'src/extensions/ExtensionsContext';
-/**
- * Jest configuration for @storybook/test-runner
- *
- * This extends the default test-runner config with custom timeouts
- * to handle slow story rendering in CI environments.
- */
-const { getJestConfig } = require('@storybook/test-runner');
-const testRunnerConfig = getJestConfig();
+export interface ViewExtensionProps {
+ viewId: string;
+}
-module.exports = {
- ...testRunnerConfig,
- // Increase timeout from default 15s to 60s for CI environments
- testTimeout: 60000,
+const ViewExtension = ({ viewId }: ViewExtensionProps) => {
+ const contributions =
+ ExtensionsManager.getInstance().getViewContributions(viewId) || [];
Review 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 β οΈ
```suggestion
const maybeContributions =
ExtensionsManager.getInstance().getViewContributions(viewId);
const contributions = Array.isArray(maybeContributions) ?
maybeContributions : [];
```
<details>
<summary><b>Why it matters? β </b></summary>
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.
</details>
<details>
<summary><b>Prompt for AI Agent π€ </b></summary>
```mdx
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.
```
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
