You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "michael-s-molina (via GitHub)" <gi...@apache.org> on 2026/02/03 18:10:41 UTC
[PR] feat(extensions): Enhances SQL Lab API [superset]
michael-s-molina opened a new pull request, #37642:
URL: https://github.com/apache/superset/pull/37642
## Summary
This PR extends the SQL Lab API with new capabilities for tab management, query execution, and editor manipulation.
## New APIs
### Editor Access (now available via `Tab.getEditor()`)
- **`Tab.getEditor()`** - Async method to get the editor handle
- **`Editor.getValue()`** - Get the current SQL content
- **`Editor.setValue(sql)`** - Replace the editor content
- **`Editor.insertText(text)`** - Insert text at cursor position
- **`Editor.focus()`** - Focus the editor
- **`Editor.getCursorPosition()`** - Get current cursor position
- **`Editor.getSelections()`** - Get current selections
- **`Editor.setAnnotations(annotations)`** - Set error/warning markers
- **`Editor.registerCompletionProvider(provider)`** - Register custom autocomplete
### Tab Management
- **`createTab(options?)`** - Create a new query editor tab with optional SQL, title, database, catalog, and schema
- **`closeTab(tabId)`** - Close a specific tab
- **`setActiveTab(tabId)`** - Switch to a specific tab
- **`onDidCreateTab`** - Event fired when a new tab is created
### Query Execution
- **`runQuery()`** - Execute the SQL query in the current tab
- **`stopQuery(queryId)`** - Stop a running query
### Tab Context
- **`setDatabase(databaseId)`** - Set the database for the current tab
- **`setCatalog(catalog)`** - Set the catalog for the current tab
- **`setSchema(schema)`** - Set the schema for the current tab
- **`Tab.databaseId`**, **`Tab.catalog`**, **`Tab.schema`** - Direct access to tab context
## API Changes
### Tab Interface
```typescript
// Before
interface Tab {
id: string;
title: string;
editor: Editor; // sync property with content, databaseId, catalog, schema
panels: Panel[];
}
// After
interface Tab {
id: string;
title: string;
databaseId: number;
catalog: string | null;
schema: string | null;
getEditor(): Promise<Editor>; // async method returning EditorHandle
panels: Panel[];
}
```
### Editor Interface
```typescript
// Before
interface Editor {
content: string;
databaseId: number;
catalog: string | null;
schema: string;
table: string | null;
}
// After - extends EditorHandle with full editor manipulation
interface Editor extends EditorHandle {
// Inherited: getValue(), setValue(), insertText(), focus(),
// getCursorPosition(), getSelections(), setAnnotations(), etc.
}
```
### Migration Example
```typescript
// Before
const tab = sqlLab.getCurrentTab();
const sql = tab.editor.content;
const dbId = tab.editor.databaseId;
// After
const tab = sqlLab.getCurrentTab();
const editor = await tab.getEditor();
const sql = editor.getValue();
const dbId = tab.databaseId;
```
## Documentation
Improved JSDoc documentation for the Editor API in `packages/superset-core/src/api/editors.ts`:
- **EditorHandle** - Comprehensive documentation for all methods including behavior details, edge cases, and parameter descriptions
- **Position, Range, Selection** - Added examples and clarified zero-based indexing
- **EditorAnnotation** - Added example and severity color explanations
- **EditorHotkey** - Documented key combination format with example
- **CompletionProvider** - Added example showing async table completion
- **executeCommand()** - Noted that command names are editor-specific (Ace vs Monaco)
## Test Plan
- Create new tabs via `createTab()` with various options
- Verify default tab naming "Untitled Query N"
- Close and switch tabs via API
- Execute and stop queries via API
- Change database/catalog/schema via API
- Access editor via `getEditor()` and manipulate content
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(extensions): Enhances SQL Lab API [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #37642:
URL: https://github.com/apache/superset/pull/37642#issuecomment-3842890210
### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | bbc5ccd7766e600e5a5894a9b6a78242589e8749 |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/69823a220b7e3a00080062e7 |
|<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-37642--superset-docs-preview.netlify.app](https://deploy-preview-37642--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">📱</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-37642--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(extensions): Enhances SQL Lab API [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on PR #37642:
URL: https://github.com/apache/superset/pull/37642#issuecomment-3847306363
> Curious to see how this changes your example editor implementations?
It does not change the editor implementations as the editor API didn't change. What changed was that this API is now available for extensions and built-in features.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(extensions): Enhances SQL Lab API [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(extensions): Enhances SQL Lab API [superset]
Posted by "msyavuz (via GitHub)" <gi...@apache.org>.
msyavuz commented on PR #37642:
URL: https://github.com/apache/superset/pull/37642#issuecomment-3847589322
@michael-s-molina Yeah, i am using your extensions repo as a documentation so i was curious to see how you would use the handle api
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(extensions): Enhances SQL Lab API [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #37642:
URL: https://github.com/apache/superset/pull/37642#issuecomment-3847630414
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(extensions): Enhances SQL Lab API [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #37642:
URL: https://github.com/apache/superset/pull/37642#issuecomment-3843277959
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(extensions): Enhances SQL Lab API [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(extensions): Enhances SQL Lab API [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on PR #37642:
URL: https://github.com/apache/superset/pull/37642#issuecomment-3847606455
> @michael-s-molina Yeah, i am using your extensions repo as a documentation so i was curious to see how you would use the handle api
Oh, got it! We'll need an extension idea that manipulates the editor 😉 The only one I have is the SQL Snippets which uses `insertText` from the handle API.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(extensions): Enhances SQL Lab API [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #37642:
URL: https://github.com/apache/superset/pull/37642#discussion_r2760426966
##########
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:
<div>
<div id="suggestion">
<div id="issue"><b>Outdated API example in comment</b></div>
<div id="fix">
The example in the getCurrentTab comment still references `tab.editor.setValue()` and `tab.editor.focus()`, but `editor` is now an async method `getEditor()`. Update to `const editor = await tab.getEditor(); editor.setValue(...); editor.focus();` to match the new API.
</div>
</div>
<small><i>Code Review Run #07fa32</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/src/core/sqlLab/index.ts:
##########
@@ -392,10 +483,166 @@ 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 runQuery: typeof sqlLabApi.runQuery = async () => {
+ 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;
+
+ store.dispatch(runQueryFromSqlEditor(database, qe, defaultLimit) as any);
+
+ return qe.id;
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Incorrect API return value</b></div>
<div id="fix">
runQuery returns the editor ID (qe.id) but should return the query ID for consistency with stopQuery, which expects a queryId to look up in state.sqlLab.queries. The dispatched runQueryFromSqlEditor generates a unique queryId (nanoid), but this isn't captured or returned.
</div>
</div>
<small><i>Code Review Run #07fa32</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/src/core/sqlLab/index.ts:
##########
@@ -392,10 +483,166 @@ 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>;
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Incorrect state inheritance</b></div>
<div id="fix">
Inheritance logic copies queryEditors[0] before activeQueryEditor, potentially mixing unrelated editor state. If active exists, should inherit only from active.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
// Inherit from active tab or use defaults
const inheritedValues = {
...activeQueryEditor,
...(unsavedQueryEditor?.id === activeQueryEditor?.id && unsavedQueryEditor),
} as Partial<QueryEditor>;
````
</div>
</details>
</div>
<small><i>Code Review Run #07fa32</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
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);
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Outdated API example in comment</b></div>
<div id="fix">
The onDidQueryRun example references `query.tab.editor.getValue()`, but `editor` is now `getEditor()` (async). Change to `(await query.tab.getEditor()).getValue()` to prevent runtime errors.
</div>
</div>
<small><i>Code Review Run #07fa32</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/packages/superset-core/src/api/editors.ts:
##########
@@ -36,69 +43,111 @@ export type { EditorContribution, EditorLanguage };
/**
* Represents a position in the editor (line and column).
+ * Both line and column are zero-based indices.
+ *
+ * @example
+ * // Position at the start of line 5, column 10
+ * const pos: Position = { line: 4, column: 9 };
*/
export interface Position {
- /** Zero-based line number */
+ /** Zero-based line number (first line is 0) */
line: number;
- /** Zero-based column number */
+ /** Zero-based column number (first column is 0) */
column: number;
}
/**
- * Represents a range in the editor with start and end positions.
+ * Represents a contiguous range in the editor defined by start and end positions.
+ * The range is inclusive of the start position and exclusive of the end position.
*/
export interface Range {
- /** Start position of the range */
+ /** Start position of the range (inclusive) */
start: Position;
- /** End position of the range */
+ /** End position of the range (exclusive) */
end: Position;
}
/**
- * Represents a selection in the editor.
+ * Represents a selection in the editor, extending Range with direction information.
+ * A selection is a highlighted range of text that can be manipulated.
*/
export interface Selection extends Range {
- /** Direction of the selection */
+ /**
+ * Direction of the selection.
+ * - 'ltr': Selection was made left-to-right (anchor at start, cursor at end)
+ * - 'rtl': Selection was made right-to-left (anchor at end, cursor at start)
+ */
direction?: 'ltr' | 'rtl';
}
/**
- * Annotation severity levels for editor markers.
+ * Severity levels for editor annotations.
+ * Determines the visual style and icon used to display the annotation.
*/
export type AnnotationSeverity = 'error' | 'warning' | 'info';
/**
- * Represents an annotation (marker/diagnostic) in the editor.
+ * Represents a diagnostic annotation displayed in the editor.
+ * Annotations are used to highlight issues like syntax errors, linting warnings,
+ * or informational messages at specific locations in the code.
+ *
+ * @example
+ * const annotation: EditorAnnotation = {
+ * line: 5,
+ * column: 10,
+ * message: 'Unknown column "user_id"',
+ * severity: 'error',
+ * source: 'sql-validator',
+ * };
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Incorrect zero-based indexing in JSDoc example</b></div>
<div id="fix">
The JSDoc example for EditorAnnotation uses line: 5 and column: 10, but since the interface specifies zero-based indexing (line 0 is the first line, column 0 is the first column), this should be line: 4 and column: 9 to represent the same position. This matches the Position example's convention and prevents confusion for API consumers.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
--- a/superset-frontend/packages/superset-core/src/api/editors.ts
+++ b/superset-frontend/packages/superset-core/src/api/editors.ts
@@ -94,8 +94,8 @@ export type AnnotationSeverity = 'error' | 'warning' | 'info';
* @example
* const annotation: EditorAnnotation = {
* line: 5,
- * column: 10,
+ * line: 4,
+ * column: 9,
* message: 'Unknown column "user_id"',
* severity: 'error',
* source: 'sql-validator',
```
</div>
</details>
</div>
<small><i>Code Review Run #07fa32</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(extensions): Enhances SQL Lab API [superset]
Posted by "msyavuz (via GitHub)" <gi...@apache.org>.
msyavuz commented on code in PR #37642:
URL: https://github.com/apache/superset/pull/37642#discussion_r2763111797
##########
superset-frontend/src/core/sqlLab/index.ts:
##########
@@ -70,39 +78,117 @@ const findQueryEditor = (editorId: string) => {
return editor;
};
-const createTab = (
+/**
+ * Registry for editor handles. Editor components register their handles here
+ * when they mount, allowing the SQL Lab API to access them.
+ */
+const editorHandleRegistry = new Map<string, sqlLabApi.Editor>();
+
+/**
+ * Pending promises waiting for editor handles to be registered.
+ */
+const pendingEditorPromises = new Map<
+ string,
+ Array<(handle: sqlLabApi.Editor) => void>
+>();
+
+/**
+ * Registers an editor handle for a tab. Called by EditorWrapper when it mounts.
+ * Resolves any pending promises waiting for this editor.
+ */
+export const registerEditorHandle = (
+ tabId: string,
+ handle: sqlLabApi.Editor,
+): void => {
+ editorHandleRegistry.set(tabId, handle);
+
+ // Resolve any pending promises waiting for this editor
+ const pending = pendingEditorPromises.get(tabId);
+ if (pending) {
+ pending.forEach(resolve => resolve(handle));
+ pendingEditorPromises.delete(tabId);
+ }
+};
+
+/**
+ * Unregisters an editor handle for a tab. Called when EditorWrapper unmounts.
+ */
+export const unregisterEditorHandle = (tabId: string): void => {
+ editorHandleRegistry.delete(tabId);
+};
+
+/**
+ * Creates a Proxy that always delegates to the current editor handle in the registry.
+ * This handles editor hot-swapping (e.g., Ace to Monaco).
+ */
+const createEditorProxy = (tabId: string): sqlLabApi.Editor =>
+ new Proxy({} as sqlLabApi.Editor, {
+ get(_, prop: keyof sqlLabApi.Editor) {
+ const handle = editorHandleRegistry.get(tabId);
+ if (!handle) {
+ throw new Error(`Editor handle not found for tab ${tabId}`);
+ }
+ const value = handle[prop];
+ return typeof value === 'function' ? value.bind(handle) : value;
+ },
+ });
+
+/**
+ * Gets the editor for a tab, waiting for it to be registered if necessary.
+ * Returns a Proxy that always delegates to the current handle.
+ */
+const getEditorAsync = (tabId: string): Promise<sqlLabApi.Editor> => {
+ const existingHandle = editorHandleRegistry.get(tabId);
+ if (existingHandle) {
+ // Editor already registered, return proxy immediately
+ return Promise.resolve(createEditorProxy(tabId));
+ }
+
+ // Wait for the editor to be registered
+ return new Promise(resolve => {
+ const pending = pendingEditorPromises.get(tabId) ?? [];
+ pending.push(() => resolve(createEditorProxy(tabId)));
+ pendingEditorPromises.set(tabId, pending);
+ });
+};
+
+const makeTab = (
id: string,
name: string,
- sql: string,
dbId: number,
- catalog?: string,
- schema?: string,
- table?: any,
-) => {
- const editor = new Editor(sql, dbId, catalog, schema, table);
+ catalog?: string | null,
+ schema?: string | null,
+): Tab => {
const panels: Panel[] = []; // TODO: Populate panels
- return new Tab(id, name, editor, panels);
+ return new Tab(
+ id,
+ name,
+ dbId,
+ catalog ?? null,
+ schema ?? null,
Review Comment:
Should we default these in the parameters?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(extensions): Enhances SQL Lab API [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #37642:
URL: https://github.com/apache/superset/pull/37642#issuecomment-3842864850
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(extensions): Enhances SQL Lab API [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina merged PR #37642:
URL: https://github.com/apache/superset/pull/37642
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(extensions): Enhances SQL Lab API [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on PR #37642:
URL: https://github.com/apache/superset/pull/37642#issuecomment-3848595244
@msyavuz I updated my extensions to reflect the latest changes.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org