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: [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]
