codeant-ai-for-open-source[bot] commented on code in PR #41285:
URL: https://github.com/apache/superset/pull/41285#discussion_r3488903734
##########
superset-frontend/src/SqlLab/components/TabbedSqlEditors/index.tsx:
##########
@@ -93,6 +96,105 @@ const TabTitle = styled.span`
// Get the user's OS
const userOS = detectOS();
+const newTabTooltip =
+ userOS === 'Windows' ? t('New tab (Ctrl + q)') : t('New tab (Ctrl + t)');
+
+const PlusIcon = (
+ <Icons.PlusOutlined
+ iconSize="l"
+ css={css`
+ vertical-align: middle;
+ `}
+ data-test="add-tab-icon"
+ />
+);
+
+function NewTabButton({ onAddSqlEditor }: { onAddSqlEditor: () => void }) {
+ const [open, setOpen] = useState(false);
+
+ const dropdownItems = useMemo<MenuItemType[]>(() => {
+ if (!open) return [];
+ const primaryItems =
+ menus.getMenu(ViewLocations.sqllab.newTab)?.primary ?? [];
+ return [
+ {
+ key: 'sql-editor',
+ label: t('SQL Editor'),
+ icon: <Icons.TableOutlined iconSize="m" />,
+ onClick: () => {
+ setOpen(false);
+ onAddSqlEditor();
+ },
+ },
+ ...primaryItems.map(item => {
+ const command = commands.getCommand(item.command);
+ const Icon = command?.icon
+ ? ((Icons as Record<string, typeof Icons.FileOutlined>)[
+ command.icon
+ ] ?? Icons.FileOutlined)
+ : Icons.FileOutlined;
+ return {
+ key: command?.id ?? item.command,
+ label: command?.title ?? item.command,
+ icon: <Icon iconSize="m" />,
+ onClick: () => {
+ setOpen(false);
+ commands.executeCommand(item.command);
+ },
+ } as MenuItemType;
Review Comment:
**Suggestion:** The menu item handler executes `item.command` even when
`getCommand` returned `undefined`. If an extension contributes a menu item
before its command is registered (or the command failed to load), clicking the
item throws `Command "...\" not found` and breaks the add-tab flow. Filter out
missing commands or guard execution and show a non-fatal fallback. [api
mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ SQL Lab new-tab menu crashes when command missing.
- ⚠️ Extension-provided tab actions fail, reducing usability.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. An extension contributes a new tab type into the `sqllab.newTab` menu, so
that
`menus.getMenu(ViewLocations.sqllab.newTab)?.primary` returns an item with
`item.command`
in `NewTabButton`
(`superset-frontend/src/SqlLab/components/TabbedSqlEditors/index.tsx:115-121`).
2. At the moment the SQL Lab `TabbedSqlEditors` component renders, assume
the extension
has not yet registered its command handler (or registration failed), so
`commands.getCommand(item.command)` in the dropdown items mapper returns
`undefined`
(`TabbedSqlEditors/index.tsx:129-131`), while the label and icon fall back
to defaults.
3. Despite `command` being `undefined`, the code still builds a
`MenuItemType` whose
`onClick` handler calls `commands.executeCommand(item.command)` without any
guard
(`TabbedSqlEditors/index.tsx:136-143`).
4. When the user opens the new-tab dropdown and clicks this contributed tab
type,
`commands.executeCommand` in
`superset-frontend/src/core/commands/index.ts:52-59` looks up
the command in `commandRegistry`, fails to find it, and throws
`Error('Command "..." not
found.')`, causing a runtime error that interrupts the add-tab flow for that
extension-provided tab type.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=f72d0080e84c4c798d67c1f14f8d20ed&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=f72d0080e84c4c798d67c1f14f8d20ed&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<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:** 129:144
**Comment:**
*Api Mismatch: The menu item handler executes `item.command` even when
`getCommand` returned `undefined`. If an extension contributes a menu item
before its command is registered (or the command failed to load), clicking the
item throws `Command "...\" not found` and breaks the add-tab flow. Filter out
missing commands or guard execution and show a non-fatal fallback.
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.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41285&comment_hash=af053530bfba6f2755c4362d48fead2dfed34ba8c21865ac591d142aeb651010&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41285&comment_hash=af053530bfba6f2755c4362d48fead2dfed34ba8c21865ac591d142aeb651010&reaction=dislike'>👎</a>
##########
superset-frontend/src/SqlLab/components/SqlEditor/index.tsx:
##########
@@ -271,6 +302,48 @@ const SqlEditor: FC<Props> = ({
const logAction = useLogAction({ queryEditorId: queryEditor.id });
const isActive = currentQueryEditorId === queryEditor.id;
+
+ // Re-renders when an extension registers a northPane view after async load.
+ const northPaneViews = useViews(ViewLocations.sqllab.northPane) || [];
+
+ // Resolve the per-tab localStorage key the same way every other SQL Lab
+ // consumer does (`tabViewId ?? id`), so the value written, read back, and
+ // observed via the `storage` event all agree once a tab is
backend-persisted.
+ const northPaneStorageId = queryEditor.tabViewId ?? queryEditor.id;
+
+ // ID of the northPane view active for this tab, or null for the default
+ // SQL editor layout. Set by an extension via PENDING_NORTH_PANE_VIEW_KEY
+ // before calling createTab(); persisted per-tab in localStorage.
+ const [northPaneViewId, setNorthPaneViewId] = useState<string | null>(() => {
+ const pendingViewId = readNorthPaneStorage(PENDING_NORTH_PANE_VIEW_KEY);
+ if (pendingViewId) {
+ writeNorthPaneStorage(PENDING_NORTH_PANE_VIEW_KEY, null);
+ writeNorthPaneStorage(
+ NORTH_PANE_VIEW_KEY(northPaneStorageId),
+ pendingViewId,
+ );
+ return pendingViewId;
+ }
+ return readNorthPaneStorage(NORTH_PANE_VIEW_KEY(northPaneStorageId));
+ });
+
+ useEffect(() => {
+ writeNorthPaneStorage(
+ NORTH_PANE_VIEW_KEY(northPaneStorageId),
+ northPaneViewId,
+ );
+ }, [northPaneStorageId, northPaneViewId]);
Review Comment:
**Suggestion:** This effect writes the current mode immediately whenever the
storage key changes, but `northPaneStorageId` can switch from client `id` to
backend `tabViewId` after hydration while `northPaneViewId` is still `null`. In
that transition, the code removes the existing persisted value under the
backend key before ever reading it, so a previously saved extension pane mode
is lost on reload. Read/migrate the new key first (or gate writes until
initialization for the new key completes) before writing/removing. [cache]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ NorthPane extension mode lost after backend persistence migration.
- ⚠️ Extensions cannot rely on consistent northPane mode storage.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open SQL Lab and create or use a tab that will be persisted via backend
tab state
(FeatureFlag.SqllabBackendPersistence), so that `EditorAutoSync` is active
(`superset-frontend/src/SqlLab/components/EditorAutoSync/index.tsx:69-184`)
and will later
call `syncQueryEditor` for unsaved editors (`lines 161-168`).
2. Install an extension that configures a northPane view by setting
`PENDING_NORTH_PANE_VIEW_KEY` before creating a new tab, as documented in
`superset-frontend/src/SqlLab/contributions.ts:60-72`, so that `SqlEditor`
persists the
chosen northPane view id into localStorage via
`writeNorthPaneStorage(NORTH_PANE_VIEW_KEY(northPaneStorageId),
pendingViewId)` in the
`useState` initializer
(`superset-frontend/src/SqlLab/components/SqlEditor/index.tsx:317-325`).
3. Observe that `SqlEditor` derives the per-tab storage key as
`northPaneStorageId =
queryEditor.tabViewId ?? queryEditor.id` (`index.tsx:309-312`) and initially
reads only
`NORTH_PANE_VIEW_KEY(northPaneStorageId)` in the state initializer
(`index.tsx:317-328`),
while `EditorAutoSync` later migrates the same `QueryEditor` to the backend
by setting
`tabViewId` in `syncQueryEditor`
(`superset-frontend/src/SqlLab/actions/sqlLab.ts:24-47`),
causing `northPaneStorageId` to switch from the temporary `id` to the
backend `tabViewId`.
4. When `tabViewId` is populated after this migration while
`northPaneViewId` in React
state remains `null` for that tab, the effect at
`SqlEditor/index.tsx:330-335` runs with
the new `northPaneStorageId` and writes `null` via
`writeNorthPaneStorage(NORTH_PANE_VIEW_KEY(northPaneStorageId),
northPaneViewId)`, which
calls `localStorage.removeItem`, erasing any previously persisted northPane
view id under
the backend key so that subsequent reloads cannot restore the
extension-specific northPane
layout for that tab.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=3c5596c8e0d44f419300a31a0f65f9ea&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=3c5596c8e0d44f419300a31a0f65f9ea&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<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/index.tsx
**Line:** 330:335
**Comment:**
*Cache: This effect writes the current mode immediately whenever the
storage key changes, but `northPaneStorageId` can switch from client `id` to
backend `tabViewId` after hydration while `northPaneViewId` is still `null`. In
that transition, the code removes the existing persisted value under the
backend key before ever reading it, so a previously saved extension pane mode
is lost on reload. Read/migrate the new key first (or gate writes until
initialization for the new key completes) before writing/removing.
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.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41285&comment_hash=103334efa3560374713fb6e40a5ae899629a555f9f0cb6d02be3a91054e964df&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41285&comment_hash=103334efa3560374713fb6e40a5ae899629a555f9f0cb6d02be3a91054e964df&reaction=dislike'>👎</a>
--
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]
