codeant-ai-for-open-source[bot] commented on code in PR #37642:
URL: https://github.com/apache/superset/pull/37642#discussion_r2760369166
##########
superset-frontend/packages/superset-core/src/api/sqlLab.ts:
##########
@@ -444,3 +448,193 @@ export declare const onDidCloseTab: Event<Tab>;
* ```
*/
export declare const onDidChangeActiveTab: Event<Tab>;
+
+/**
+ * Event fired when a new tab is created in SQL Lab.
+ * Provides the newly created tab object as the event payload.
+ *
+ * @example
+ * ```typescript
+ * onDidCreateTab.event((tab) => {
+ * console.log('New tab created:', tab.title);
+ * // Initialize extension state for new tab
+ * });
+ * ```
+ */
+export declare const onDidCreateTab: Event<Tab>;
+
+/**
+ * Tab/Editor Management APIs
+ *
+ * These APIs allow extensions to create, close, and manage SQL Lab tabs.
+ */
+
+/**
+ * Options for creating a new SQL Lab tab.
+ */
+export interface CreateTabOptions {
+ /**
+ * Initial SQL content for the editor.
+ */
+ sql?: string;
+
+ /**
+ * Display title for the tab.
+ * If not provided, defaults to "Untitled Query N".
+ */
+ title?: string;
+
+ /**
+ * Database ID to connect to.
+ * If not provided, inherits from the active tab or uses default.
+ */
+ databaseId?: number;
+
+ /**
+ * Catalog name (for multi-catalog databases like Trino).
+ */
+ catalog?: string | null;
+
+ /**
+ * Schema name for the query context.
+ */
+ schema?: string;
+}
Review Comment:
**Suggestion:** `CreateTabOptions.schema` is typed as `string` while
`Tab.schema` and `setSchema` allow `string | null`, so you cannot pass `null`
when creating a tab even though other parts of the API treat schema as
nullable, leading to inconsistent typing and potential runtime handling
differences. [type error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ createTab callers cannot pass null schema.
- ⚠️ Inconsistent API nullability semantics.
```
</details>
```suggestion
schema?: string | null;
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open `superset-frontend/packages/superset-core/src/api/sqlLab.ts` and
locate
`CreateTabOptions` (starts at line ~475 in the PR hunk). The `schema` option
is declared
as `schema?: string;` (line 501).
2. Inspect the `Tab` interface earlier in the same file: `schema: string |
null;`
(Tab.schema defined at ~line 87) and the function `setSchema(schema: string
| null):
Promise<void>;` (declared around line 640). The rest of the API treats
schema as nullable.
3. In a TypeScript consumer, call `createTab({ databaseId: 1, schema: null
})`. The
compiler will error because `null` is not assignable to parameter type
`string |
undefined` for `schema?: string`.
4. The mismatch prevents callers from explicitly passing `null` when they
intend to
clear/explicitly set an empty schema; changing `schema?: string | null` in
`CreateTabOptions` aligns types.
```
</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-core/src/api/sqlLab.ts
**Line:** 502:502
**Comment:**
*Type Error: `CreateTabOptions.schema` is typed as `string` while
`Tab.schema` and `setSchema` allow `string | null`, so you cannot pass `null`
when creating a tab even though other parts of the API treat schema as
nullable, leading to inconsistent typing and potential runtime handling
differences.
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/packages/superset-core/src/api/sqlLab.ts:
##########
@@ -327,8 +331,8 @@ export declare const onDidChangeTabTitle: Event<string>;
* @example
* ```typescript
* onDidQueryRun.event((query) => {
- * console.log('Query started on database:', query.tab.editor.databaseId);
- * console.log('Query content:', query.tab.editor.content);
+ * console.log('Query started on database:', query.tab.databaseId);
+ * console.log('Query SQL:', query.tab.editor.getValue());
Review Comment:
**Suggestion:** The `onDidQueryRun` JSDoc example accesses
`query.tab.editor.getValue()`, but `Tab` no longer exposes an `editor` property
and now requires using the async `getEditor()` method, so this example will not
compile and misrepresents the public API. [possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Event-handler examples won't compile for extensions.
- ⚠️ Misleading docs for query run listeners.
```
</details>
```suggestion
* query.tab.getEditor().then(editor => {
* console.log('Query SQL:', editor.getValue());
* });
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open `superset-frontend/packages/superset-core/src/api/sqlLab.ts` and
find the
`onDidQueryRun` JSDoc example (example snippet shown at lines 333-336 in the
PR hunk). The
example references `query.tab.editor.getValue()`.
2. Note the event declaration for `onDidQueryRun` is `export declare const
onDidQueryRun:
Event<QueryContext>;` (declaration appears in the same file, nearby in the
docs at ~line
345), and `QueryContext.tab` has type `Tab` whose `editor` property was
removed in favor
of `getEditor(): Promise<Editor>` (Tab.getEditor defined at ~line 105).
3. Copy the example into an extension and attempt to compile/use it.
TypeScript will
report an error because `query.tab.editor` does not exist on `Tab`,
preventing consumers
from compiling code that follows the example.
4. The observed failure is a documentation/type mismatch — the example
should demonstrate
`query.tab.getEditor()` instead of the removed `editor` property to reflect
the actual API
surface.
```
</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-core/src/api/sqlLab.ts
**Line:** 335:335
**Comment:**
*Possible Bug: The `onDidQueryRun` JSDoc example accesses
`query.tab.editor.getValue()`, but `Tab` no longer exposes an `editor` property
and now requires using the async `getEditor()` method, so this example will not
compile and misrepresents the public API.
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/core/sqlLab/index.ts:
##########
@@ -340,10 +431,10 @@ const onDidCloseTab: typeof sqlLabApi.onDidCloseTab = (
thisArgs?: any,
): Disposable =>
createActionListener(
- predicate(REMOVE_QUERY_EDITOR),
+ globalPredicate(REMOVE_QUERY_EDITOR),
listener,
(action: { type: string; queryEditor: { id: string } }) =>
- getTab(action.queryEditor.id)!,
+ getTab(action.queryEditor.id),
thisArgs,
Review Comment:
**Suggestion:** `onDidCloseTab` derives the `Tab` from the Redux state after
the `REMOVE_QUERY_EDITOR` action has been reduced, so the tab has already been
removed and `getTab` returns `undefined`, causing listeners expecting a valid
`Tab` to receive `undefined` and potentially crash when using it. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ onDidCloseTab listeners receive undefined Tab.
- ⚠️ Listeners calling tab methods will throw.
- ⚠️ Close-tab workflows and integrations affected.
```
</details>
```suggestion
(action: { type: string; queryEditor: QueryEditor }) =>
makeTab(
action.queryEditor.id!,
action.queryEditor.name ?? '',
action.queryEditor.dbId ?? 0,
action.queryEditor.catalog,
action.queryEditor.schema ?? undefined,
),
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Register a close-tab listener using the exported API:
`sqlLab.onDidCloseTab(listener)`
(listener registration code lives at
`superset-frontend/src/core/sqlLab/index.ts:431-439`).
2. Trigger a tab close via the public API `sqlLab.closeTab(tabId)`
(implementation
dispatches `REMOVE_QUERY_EDITOR` at
`superset-frontend/src/core/sqlLab/index.ts:573-578`).
3. The `REMOVE_QUERY_EDITOR` action is dispatched
(`superset-frontend/src/core/sqlLab/index.ts:576`), reducers remove the
query editor from
state.
4. The action listener's mapper calls `getTab(action.queryEditor.id)` (line
`superset-frontend/src/core/sqlLab/index.ts:437`) — because the reducer
already removed
the editor, `getTab` returns `undefined`, so the registered `listener`
receives
`undefined` instead of a valid `Tab`; reproduce by registering a listener
that
dereferences the tab (e.g., calls `tab.getEditor()`), then calling
`sqlLab.closeTab(id)`
and observing a runtime error.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/core/sqlLab/index.ts
**Line:** 438:438
**Comment:**
*Logic Error: `onDidCloseTab` derives the `Tab` from the Redux state
after the `REMOVE_QUERY_EDITOR` action has been reduced, so the tab has already
been removed and `getTab` returns `undefined`, causing listeners expecting a
valid `Tab` to receive `undefined` and potentially crash when using it.
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/packages/superset-core/src/api/sqlLab.ts:
##########
@@ -262,7 +262,11 @@ export declare const getActivePanel: () => Panel;
* const tab = getCurrentTab();
* if (tab) {
* console.log(`Active tab: ${tab.title}`);
- * console.log(`Database ID: ${tab.editor.databaseId}`);
+ * console.log(`Database ID: ${tab.databaseId}, Schema: ${tab.schema}`);
+ *
+ * // Direct editor manipulation
+ * tab.editor.setValue("SELECT * FROM users");
+ * tab.editor.focus();
Review Comment:
**Suggestion:** The JSDoc example for `getCurrentTab` still uses a
synchronous `tab.editor` property, which no longer exists on `Tab` and will
both fail type-checking and mislead consumers; it should demonstrate using the
async `getEditor()` API instead. [possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Extension examples cause TypeScript compile errors.
- ⚠️ Consumers misled by invalid API example.
```
</details>
```suggestion
* tab.getEditor().then(editor => {
* editor.setValue("SELECT * FROM users");
* editor.focus();
* });
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open the declaration file
`superset-frontend/packages/superset-core/src/api/sqlLab.ts`
and locate the `getCurrentTab` example in the JSDoc (lines shown at 265-269
in the PR
hunk). The example shows `tab.editor.setValue(...)` and `tab.editor.focus()`
while the
`Tab` interface no longer exposes `editor`.
2. In an extension/consumer TypeScript project, copy the example lines into
a module and
call `const tab = getCurrentTab(); if (tab) { tab.editor.setValue("...");
}`. The consumer
imports types from the package that include `Tab` as declared in the same
file
(`getEditor(): Promise<Editor>` at line ~105 in the Tab interface).
3. Run `tsc` (or let the IDE typechecker run). The TypeScript compiler will
error because
`Tab` does not have an `editor` property (the correct API is `getEditor()`),
producing a
compile-time type error at the site where `tab.editor` is referenced.
4. Observe the incorrect example causes immediate developer friction: the
code in
`sqlLab.ts` (lines 265-269) misleads consumers and results in compile-time
errors. This is
a documentation/type mismatch issue, not a runtime crash.
```
</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-core/src/api/sqlLab.ts
**Line:** 268:269
**Comment:**
*Possible Bug: The JSDoc example for `getCurrentTab` still uses a
synchronous `tab.editor` property, which no longer exists on `Tab` and will
both fail type-checking and mislead consumers; it should demonstrate using the
async `getEditor()` API instead.
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/core/utils.ts:
##########
@@ -24,7 +24,7 @@ import { AnyListenerPredicate } from '@reduxjs/toolkit';
export function createActionListener<V, S>(
predicate: AnyListenerPredicate<S>,
listener: (v: V) => void,
- valueParser: (action: AnyAction, state: RootState) => V,
+ valueParser: (action: AnyAction, state: RootState) => V | null | undefined,
Review Comment:
**Suggestion:** The helper is generic over the state type `S` for the
predicate but hardcodes `RootState` in the `valueParser` signature, creating
inconsistent typing for the state parameter; this forces callers to reconcile
mismatched types and can lead to unsafe casts or incorrect assumptions about
the state shape. Aligning `valueParser` to use the same generic state type `S`
keeps the contract consistent and type-safe. [type error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Type errors when registering listeners (src/core/utils.ts).
- ⚠️ Prevents typed predicates using non-RootState.
- ⚠️ Affects modules calling createActionListener.
```
</details>
```suggestion
valueParser: (action: AnyAction, state: S) => V | null | undefined,
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Inspect the helper declaration at
`superset-frontend/src/core/utils.ts:24-29` (function
`createActionListener`).
2. Create/modify a consumer file that imports the helper and supplies a
predicate
specialized to a narrower state type S (for example a slice type). Example
consumer (new
file) `superset-frontend/src/__tests__/exampleListener.ts:1-8`:
- import { createActionListener } from 'src/core/utils';
- type SliceState = { counter: number };
- createActionListener<SliceState>(
(action, state) => /* predicate using SliceState */,
(value) => {},
(action, state: SliceState) => null,
);
3. Run the TypeScript compiler (e.g. `yarn build` / `yarn tsc`). The
compiler will
reference the helper signature in `src/core/utils.ts:24-29` and will report
a type
incompatibility because `valueParser` is declared to receive `RootState`
while the
consumer expects `state` of type `SliceState`. Typical error: "Type
'(action, state:
SliceState) => ...' is not assignable to parameter of type '(action, state:
RootState) =>
...'".
4. Observe the compile-time failure preventing the consumer from registering
a
strongly-typed listener. Note: a repository-wide search shows the helper is
defined at
`superset-frontend/src/core/utils.ts:24-29` and there are likely callers
that rely on
typed predicates; if callers always use `RootState` this error won't appear
until a
consumer uses a non-RootState `S`.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/core/utils.ts
**Line:** 27:27
**Comment:**
*Type Error: The helper is generic over the state type `S` for the
predicate but hardcodes `RootState` in the `valueParser` signature, creating
inconsistent typing for the state parameter; this forces callers to reconcile
mismatched types and can lead to unsafe casts or incorrect assumptions about
the state shape. Aligning `valueParser` to use the same generic state type `S`
keeps the contract consistent and type-safe.
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]
