codeant-ai-for-open-source[bot] commented on code in PR #37642:
URL: https://github.com/apache/superset/pull/37642#discussion_r2763911945
##########
superset-frontend/src/core/sqlLab/index.ts:
##########
@@ -392,10 +492,212 @@ const onDidChangeTabTitle: typeof
sqlLabApi.onDidChangeTabTitle = (
thisArgs,
);
+/**
+ * Event fired when a new tab is created.
+ */
+const onDidCreateTab: typeof sqlLabApi.onDidCreateTab = (
+ listener: (tab: sqlLabApi.Tab) => void,
+ thisArgs?: any,
+): Disposable =>
+ createActionListener(
+ globalPredicate(ADD_QUERY_EDITOR),
+ listener,
+ (action: { type: string; queryEditor: QueryEditor }) =>
+ makeTab(
+ action.queryEditor.id!,
+ action.queryEditor.name ?? '',
+ action.queryEditor.dbId ?? 0,
+ action.queryEditor.catalog,
+ action.queryEditor.schema ?? undefined,
+ ),
+ thisArgs,
+ );
+
+/**
+ * Tab/Editor Management APIs
+ */
+
+const createTab: typeof sqlLabApi.createTab = async (
+ options?: sqlLabApi.CreateTabOptions,
+) => {
+ const {
+ sqlLab: { queryEditors, tabHistory, unsavedQueryEditor, databases },
+ common,
+ } = store.getState() as SqlLabRootState;
+
+ const activeQueryEditor = queryEditors.find(
+ (qe: QueryEditor) => qe.id === tabHistory[tabHistory.length - 1],
+ );
+ const dbIds = Object.values(databases).map(db => db.id);
+ const defaultDbId = common?.conf?.SQLLAB_DEFAULT_DBID;
+ const firstDbId = dbIds.length > 0 ? Math.min(...dbIds) : undefined;
+
+ // Inherit from active tab or use defaults
+ const inheritedValues = {
+ ...queryEditors[0],
+ ...activeQueryEditor,
+ ...(unsavedQueryEditor?.id === activeQueryEditor?.id &&
unsavedQueryEditor),
+ } as Partial<QueryEditor>;
+
+ // Generate default name if no title provided
+ const name =
+ options?.title ??
+ newQueryTabName(
+ queryEditors?.map((qe: QueryEditor) => ({
+ ...qe,
+ ...(qe.id === unsavedQueryEditor?.id && unsavedQueryEditor),
+ })) || [],
+ );
+
+ const newQueryEditor: Partial<QueryEditor> = {
+ dbId:
+ options?.databaseId ?? inheritedValues.dbId ?? defaultDbId ?? firstDbId,
+ catalog: options?.catalog ?? inheritedValues.catalog,
+ schema: options?.schema ?? inheritedValues.schema,
+ sql: options?.sql ?? 'SELECT ...',
+ queryLimit:
+ inheritedValues.queryLimit ?? common?.conf?.DEFAULT_SQLLAB_LIMIT,
+ autorun: false,
+ name,
+ };
+
+ store.dispatch(addQueryEditor(newQueryEditor) as any);
+
+ // Get the newly created tab
+ const updatedState = store.getState() as SqlLabRootState;
+ const newTab =
+ updatedState.sqlLab.queryEditors[
+ updatedState.sqlLab.queryEditors.length - 1
+ ];
+
+ return makeTab(
+ newTab.id,
+ newTab.name ?? '',
+ newTab.dbId ?? 0,
+ newTab.catalog,
+ newTab.schema ?? undefined,
+ );
+};
+
+const closeTab: typeof sqlLabApi.closeTab = async (tabId: string) => {
+ const queryEditor = findQueryEditor(tabId);
+ if (queryEditor) {
+ store.dispatch({ type: REMOVE_QUERY_EDITOR, queryEditor });
+ }
+};
+
+const setActiveTab: typeof sqlLabApi.setActiveTab = async (tabId: string) => {
+ const queryEditor = findQueryEditor(tabId);
+ if (queryEditor) {
+ store.dispatch(setActiveQueryEditor(queryEditor as QueryEditor));
+ }
+};
+
+const executeQuery: typeof sqlLabApi.executeQuery = async options => {
+ const state = store.getState() as SqlLabRootState;
+ const editorId = activeEditorId();
+ const queryEditor = findQueryEditor(editorId);
+
+ if (!queryEditor) {
+ throw new Error('No active query editor');
+ }
+
+ const { databases, unsavedQueryEditor } = state.sqlLab;
+ const qe = {
+ ...queryEditor,
+ ...(queryEditor.id === unsavedQueryEditor?.id && unsavedQueryEditor),
+ } as QueryEditor;
+
+ const database = qe.dbId ? databases[qe.dbId] : null;
+ const defaultLimit = state.common?.conf?.DEFAULT_SQLLAB_LIMIT ?? 1000;
+
+ // Determine SQL to execute
+ let sql: string;
+ let updateTabState = true;
+
+ if (options?.sql !== undefined) {
+ // Custom SQL provided - don't update tab state
+ ({ sql } = options);
+ updateTabState = false;
+ } else if (options?.selectedOnly && qe.selectedText) {
+ // Run selected text only
+ sql = qe.selectedText;
+ updateTabState = false;
+ } else {
+ // Default: use editor content (selected text takes precedence)
+ sql = qe.selectedText || qe.sql;
+ updateTabState = !qe.selectedText;
+ }
+
+ // Merge template params
+ const templateParams = options?.templateParams
+ ? JSON.stringify({
+ ...JSON.parse(qe.templateParams || '{}'),
+ ...options.templateParams,
+ })
+ : qe.templateParams;
+
+ const queryId = nanoid(11);
+
+ const query: Query = {
+ id: queryId,
+ dbId: qe.dbId,
+ sql,
+ sqlEditorId: qe.tabViewId ?? qe.id,
+ sqlEditorImmutableId: qe.immutableId,
+ tab: qe.name,
+ catalog: qe.catalog,
+ schema: qe.schema,
+ tempTable: options?.ctas?.tableName,
+ templateParams,
+ queryLimit: options?.limit ?? qe.queryLimit ?? defaultLimit,
+ runAsync: database ? database.allow_run_async : false,
+ ctas: !!options?.ctas,
+ ctas_method: options?.ctas?.method,
+ updateTabState,
+ };
+
+ store.dispatch(runQueryAction(query));
+
+ return queryId;
+};
+
+const cancelQuery: typeof sqlLabApi.cancelQuery = async (queryId: string) => {
+ const state = store.getState() as SqlLabRootState;
+ const query = state.sqlLab.queries[queryId];
+
+ if (query) {
+ store.dispatch(postStopQuery(query as any) as any);
Review Comment:
**Suggestion:** The `cancelQuery` API dispatches `postStopQuery` instead of
`stopQueryAction`, so no `STOP_QUERY` action is emitted and the server-side
cancellation logic that likely listens to `STOP_QUERY` will not run, meaning
queries may continue running and `onDidQueryStop` listeners will never fire for
cancellations initiated via this API. [logic error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Canceled queries don't trigger STOP_QUERY listeners.
- ❌ onDidQueryStop consumers won't receive cancellations.
- ⚠️ Server-side cancellation flows may not run.
- ⚠️ UI features relying on STOP_QUERY stay inconsistent.
```
</details>
```suggestion
store.dispatch(stopQueryAction(query as any) as any);
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In the running frontend, call the exported API
`sqlLab.cancelQuery(queryId)` (caller
location: any consumer of the embedded SQL Lab API). The implementation
being executed is
in superset-frontend/src/core/sqlLab/index.ts at the cancelQuery function
(lines 665-671).
2. cancelQuery looks up the query in the Redux slice via store.getState()
and finds the
query object (lines 666-667). For an existing query it dispatches
postStopQuery (line 669)
instead of the STOP/stop action variant.
3. Event listeners that subscribe to query stop events use the STOP_QUERY
action path (see
onDidQueryStop which filters predicate(STOP_QUERY) and maps the action to
QueryContext —
the onDidQueryStop registration lives earlier in the same file where
createActionListener
is invoked for STOP_QUERY). Because cancelQuery dispatches postStopQuery,
the STOP_QUERY
action is not emitted and those listeners never receive the stop event.
4. Observed behavior: cancelling a running query via sqlLab.cancelQuery does
not trigger
onDidQueryStop handlers and does not follow the same server cancellation
flow that is
wired to STOP_QUERY listeners. This reproduces reliably by running a query,
invoking
sqlLab.cancelQuery with that query's id, and observing that registered
onDidQueryStop
callbacks are not invoked while postStopQuery is dispatched instead.
```
</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:** 670:670
**Comment:**
*Logic Error: The `cancelQuery` API dispatches `postStopQuery` instead
of `stopQueryAction`, so no `STOP_QUERY` action is emitted and the server-side
cancellation logic that likely listens to `STOP_QUERY` will not run, meaning
queries may continue running and `onDidQueryStop` listeners will never fire for
cancellations initiated via this 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 +433,17 @@ 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)!,
+ (action: { type: string; queryEditor: QueryEditor }) =>
+ // Construct tab from action data since the tab has already been removed
from state
+ makeTab(
+ action.queryEditor.id,
+ action.queryEditor.name ?? '',
+ action.queryEditor.dbId ?? 0,
+ action.queryEditor.catalog,
+ action.queryEditor.schema,
+ ),
Review Comment:
**Suggestion:** In `onDidCloseTab`, the returned `Tab` is built via
`makeTab`, whose `getEditor()` uses `getEditorAsync`; for a tab that has
already been removed there will never be a corresponding editor handle
registration, so any listener that calls `tab.getEditor()` in a close handler
will get a promise that never resolves and leak a resolver in
`pendingEditorPromises`. [resource leak]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ onDidCloseTab handlers calling getEditor hang forever.
- ⚠️ pendingEditorPromises entries leak memory over time.
- ⚠️ Close-tab cleanup code may not complete.
- ⚠️ Consumers must handle closed-tab editor absence explicitly.
```
</details>
```suggestion
(action: { type: string; queryEditor: QueryEditor }) => {
const { id, name, dbId, catalog, schema } = action.queryEditor;
// For closed tabs, expose a Tab whose editor accessor fails fast
instead of waiting on a handle that will never be registered
return new Tab(
id,
name ?? '',
dbId ?? 0,
catalog,
schema,
() =>
Promise.reject(
new Error(`Editor handle not available for closed tab ${id}`),
),
[],
);
},
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Register a close-tab listener via the public API: call
sqlLab.onDidCloseTab(listener).
The onDidCloseTab registration is implemented in
superset-frontend/src/core/sqlLab/index.ts (lines ~435-448) and constructs a
Tab via
makeTab(...) using the removed queryEditor payload.
2. In the listener implementation, call tab.getEditor() (common pattern:
consumer wants to
inspect editor contents on close). The Tab returned by makeTab uses
makeTab(..., () =>
getEditorAsync(id), ...) so tab.getEditor() calls getEditorAsync(tabId).
3. getEditorAsync (defined earlier in the same file) first checks
editorHandleRegistry for
the tabId; for a recently closed tab the editor component has already
unmounted and no
handle will be (re)registered. getEditorAsync instead pushes a resolver into
pendingEditorPromises and returns a Promise that will never resolve.
4. Result: the listener's call to tab.getEditor() returns a never-resolving
Promise and
pendingEditorPromises retains the resolver (memory leak). This reproduces
deterministically when a consumer of sqlLab.onDidCloseTab calls
tab.getEditor() in the
close callback for a tab that has already been removed.
```
</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:** 442:446
**Comment:**
*Resource Leak: In `onDidCloseTab`, the returned `Tab` is built via
`makeTab`, whose `getEditor()` uses `getEditorAsync`; for a tab that has
already been removed there will never be a corresponding editor handle
registration, so any listener that calls `tab.getEditor()` in a close handler
will get a promise that never resolves and leak a resolver in
`pendingEditorPromises`.
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:
##########
@@ -444,3 +450,253 @@ 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 | null;
+}
+
+/**
+ * Creates a new query editor tab in SQL Lab.
+ *
+ * @param options Optional configuration for the new tab
+ * @returns The newly created tab object
+ *
+ * @example
+ * ```typescript
+ * // Create a tab with default settings
+ * const tab = await createTab();
+ *
+ * // Create a tab with specific SQL and database
+ * const tab = await createTab({
+ * sql: "SELECT * FROM users LIMIT 10",
+ * title: "User Query",
+ * databaseId: 1,
+ * schema: "public"
+ * });
+ * ```
+ */
+export declare function createTab(options?: CreateTabOptions): Promise<Tab>;
+
+/**
+ * Closes a specific tab in SQL Lab.
+ *
+ * @param tabId The ID of the tab to close
+ * @returns Promise that resolves when the tab is closed
+ *
+ * @example
+ * ```typescript
+ * const tabs = getTabs();
+ * if (tabs.length > 1) {
+ * await closeTab(tabs[0].id);
+ * }
+ * ```
+ */
+export declare function closeTab(tabId: string): Promise<void>;
+
+/**
+ * Switches to a specific tab in SQL Lab.
+ *
+ * @param tabId The ID of the tab to activate
+ * @returns Promise that resolves when the tab is activated
+ *
+ * @example
+ * ```typescript
+ * const tabs = getTabs();
+ * const targetTab = tabs.find(t => t.title === "My Query");
+ * if (targetTab) {
+ * await setActiveTab(targetTab.id);
+ * }
+ * ```
+ */
+export declare function setActiveTab(tabId: string): Promise<void>;
+
+/**
+ * Query Execution APIs
+ *
+ * These APIs allow extensions to execute and control SQL queries.
+ */
+
+/**
+ * Options for executing a SQL query.
+ */
+export interface QueryOptions {
+ /**
+ * SQL to execute without modifying editor content.
+ * If not provided, uses the current editor content.
+ */
+ sql?: string;
+
+ /**
+ * Run only the selected text in the editor.
+ * Ignored if `sql` option is provided.
+ */
+ selectedOnly?: boolean;
+
+ /**
+ * Override the query row limit.
+ * If not provided, uses the tab's configured limit.
+ */
+ limit?: number;
+
+ /**
+ * Template parameters for Jinja templating.
+ * Merged with existing template parameters from the editor.
+ */
Review Comment:
**Suggestion:** The query execution options use a `templateParams` property
name while the corresponding field in the query context is
`templateParameters`, which is likely to cause confusion and mismatched
payloads; renaming the options field to `templateParameters` aligns the API
surface and reduces integration errors. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Confusing API shape for template parameters.
- ❌ Integration code may miss parameter propagation.
- ⚠️ Docs/examples may be inconsistent.
```
</details>
```suggestion
templateParameters?: Record<string, unknown>;
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Inspect the QueryContext interface in
superset-frontend/packages/superset-core/src/api/sqlLab.ts and note it
defines
`templateParameters: Record<string, any>;` on the context object delivered
to events (see
QueryContext section above QueryOptions).
2. Inspect the QueryOptions definition (at `sqlLab.ts:588-591`) and observe
the option is
named `templateParams?: Record<string, unknown>;`.
3. Create an integration that calls `executeQuery({ templateParams: { foo: 1
} })`
expecting values to be merged into `query.templateParameters` inside
QueryContext; because
of the naming mismatch, off-by-name integration code or documentation that
assumes the
same key will cause confusion or require manual mapping in the
implementation layer.
4. Reproducible by authoring code that uses `executeQuery` and reading
`templateParameters` from the QueryContext in a `onDidQueryRun` listener —
the developer
will have to verify that `templateParams` is actually propagated by the
implementation
(possible source of bugs). The inconsistency is verifiable by searching
these identifiers
in the API file.
```
</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:** 592:592
**Comment:**
*Logic Error: The query execution options use a `templateParams`
property name while the corresponding field in the query context is
`templateParameters`, which is likely to cause confusion and mismatched
payloads; renaming the options field to `templateParameters` aligns the API
surface and reduces integration errors.
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]
