codeant-ai-for-open-source[bot] commented on code in PR #41285:
URL: https://github.com/apache/superset/pull/41285#discussion_r3454032271
##########
superset-frontend/packages/superset-core/src/sqlLab/index.ts:
##########
@@ -62,6 +62,14 @@ export interface Tab {
*/
id: string;
+ /**
+ * The stable backend-assigned identifier for this tab. Exposed as an opaque
+ * string so the public extension API does not leak the backend's internal
+ * numeric tab id. Set once the tab has been persisted to the backend;
+ * undefined for new tabs before the first backend sync.
+ */
+ backendId?: string;
Review Comment:
**Suggestion:** The new public `backendId` field is declared but the runtime
`Tab` objects are still created without setting it, so extensions will always
receive `undefined` and cannot correlate tabs with backend `tabstateview` rows
as documented. Populate this field when building `Tab` instances (for example
from the persisted tab identifier) so the API contract is actually fulfilled.
[incomplete implementation]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Extensions cannot map tabs to backend tabstateview rows.
- ⚠️ Extensions must re-derive mappings from internal Redux state.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Extensions access the SQL Lab API via `window.superset.sqlLab` as wired in
`ExtensionsStartup`
(superset-frontend/src/extensions/ExtensionsStartup.tsx:65-76), which
exposes the `sqlLab` implementation from `src/core/sqlLab/index.ts`.
2. A caller invokes `sqlLab.createTab()` or listens to
`sqlLab.onDidCreateTab`, whose
implementations in `src/core/sqlLab/index.ts:217-233` and `239-38` construct
`Tab`
instances by calling `makeTab(...)` (same file, lines 18-31) or
`getTab(...)` (lines
33-40).
3. `makeTab(...)` returns `new Tab(id, name, dbId, catalog, schema,
editorGetter, panels)`
(src/core/sqlLab/index.ts:18-31), using the `Tab` class defined in
`src/core/sqlLab/models.ts:34-70`, which only declares `id`, `title`,
`databaseId`,
`catalog`, `schema`, `panels`, and `getEditor()` — it has no `backendId`
field and never
assigns one.
4. The public API type in this PR adds `backendId?: string;` to `sqlLab.Tab`
(superset-frontend/packages/superset-core/src/sqlLab/index.ts:65-71), but
since neither
`Tab` in `models.ts` nor `makeTab(...)`/`getTab(...)` ever set that
property, any
extension reading `tab.backendId` from `getCurrentTab()`, `getTabs()`, or
`onDidCreateTab`
will always see `undefined`, making the new API field unusable.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=1edd73deea6348ac9808de6dd6a67dbd&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=1edd73deea6348ac9808de6dd6a67dbd&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/packages/superset-core/src/sqlLab/index.ts
**Line:** 71:71
**Comment:**
*Incomplete Implementation: The new public `backendId` field is
declared but the runtime `Tab` objects are still created without setting it, so
extensions will always receive `undefined` and cannot correlate tabs with
backend `tabstateview` rows as documented. Populate this field when building
`Tab` instances (for example from the persisted tab identifier) so the API
contract is actually fulfilled.
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=86e7389754e06ca54d459b34e9133385c293a76ed2ee4cc6d22a9152b304e5e2&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41285&comment_hash=86e7389754e06ca54d459b34e9133385c293a76ed2ee4cc6d22a9152b304e5e2&reaction=dislike'>👎</a>
##########
superset-frontend/src/SqlLab/contributions.ts:
##########
@@ -46,5 +46,27 @@ export const ViewLocations = {
statusBar: 'sqllab.statusBar',
results: 'sqllab.results',
queryHistory: 'sqllab.queryHistory',
+ // Extensions can register a full-pane replacement here. SqlEditor renders
+ // the registered view instead of the default editor+SouthPane split when
+ // a tab was opened in that mode.
+ northPane: 'sqllab.northPane',
+ // Extensions register tab-type commands here. When any are present the
+ // "+" new-tab button becomes a dropdown listing all registered tab types
+ // plus the built-in SQL Editor option.
+ newTab: 'sqllab.newTab',
},
} as const;
+
+/**
+ * localStorage key an extension sets before calling createTab() to declare
+ * which northPane view the new tab should open with. The value must be the
+ * view ID passed to views.registerView() (e.g. "my-ext.northPane"). SqlEditor
+ * consumes and removes this key during initialization, then persists the
chosen
+ * view ID under a per-tab key so the mode survives page reloads.
+ *
+ * @example
+ * // In an extension's newTab command handler:
+ * localStorage.setItem(PENDING_NORTH_PANE_VIEW_KEY, 'my-ext.northPane');
+ * sqlLab.createTab({ title: 'My View' });
+ */
+export const PENDING_NORTH_PANE_VIEW_KEY = 'sqllab.pendingNorthPaneView';
Review Comment:
**Suggestion:** Using a single global localStorage key for pending
north-pane selection introduces a race: two extension tab creations in quick
succession can overwrite each other before `SqlEditor` consumes the key,
causing the wrong tab mode to open. Use a per-request/per-tab tokenized key (or
pass the view id through tab creation payload/state) to avoid cross-tab
collisions. [race condition]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ North-pane tabs can show another extension's view.
- ⚠️ Tab layout becomes nondeterministic under rapid tab creation.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Extensions register custom north-pane views under
`ViewLocations.sqllab.northPane` and
follow the documented pattern in
`superset-frontend/src/SqlLab/contributions.ts:61-71`,
setting the global `localStorage` key `sqllab.pendingNorthPaneView` via
`PENDING_NORTH_PANE_VIEW_KEY` before calling
`window.superset.sqlLab.createTab({ title:
... })`.
2. `sqlLab.createTab()` is implemented in
`superset-frontend/src/core/sqlLab/index.ts:239-38`; it dispatches
`ADD_QUERY_EDITOR` and
the new `QueryEditor` entries are rendered by `TabbedSqlEditors`
(superset-frontend/src/SqlLab/components/TabbedSqlEditors/index.tsx:273-288),
which for
each `queryEditor` mounts a corresponding `<SqlEditor queryEditor={qe} ...
/>`.
3. Each `SqlEditor` instance initializes its `northPaneViewId` state by
reading the
*global* pending key: the initializer in
`superset-frontend/src/SqlLab/components/SqlEditor/index.tsx:292-305` does
`const
pendingViewId = localStorage.getItem(PENDING_NORTH_PANE_VIEW_KEY);` and, if
present,
removes that key and writes the value to a per-tab key
`NORTH_PANE_VIEW_KEY(northPaneStorageId)`.
4. If two different extension commands run in quick succession (e.g., two
`sqllab.newTab`
menu items from separate extensions, activated via the `NewTabButton`
dropdown at
`TabbedSqlEditors/index.tsx:113-147` and
`commands.executeCommand(item.command)`), they
each overwrite the same global `PENDING_NORTH_PANE_VIEW_KEY` before both
`SqlEditor`
instances have run their initialization. As a result, at least one tab will
read a stale
or mismatched `pendingViewId` (or none at all), causing that tab to open
with the wrong
north-pane view or fall back to the default editor layout, despite the
extension having
set a different desired view for its tab.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=faf89980cbf145fa9cce8fca09c402cd&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=faf89980cbf145fa9cce8fca09c402cd&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/contributions.ts
**Line:** 72:72
**Comment:**
*Race Condition: Using a single global localStorage key for pending
north-pane selection introduces a race: two extension tab creations in quick
succession can overwrite each other before `SqlEditor` consumes the key,
causing the wrong tab mode to open. Use a per-request/per-tab tokenized key (or
pass the view id through tab creation payload/state) to avoid cross-tab
collisions.
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=b9cb7e3dc042f479f2d3115c757ef8bd303d76f0da5fdfd9076352056e2a65ca&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41285&comment_hash=b9cb7e3dc042f479f2d3115c757ef8bd303d76f0da5fdfd9076352056e2a65ca&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]
