codeant-ai-for-open-source[bot] commented on code in PR #37499:
URL: https://github.com/apache/superset/pull/37499#discussion_r2736418026
##########
superset-frontend/src/SqlLab/components/SqlEditor/index.tsx:
##########
@@ -429,73 +429,132 @@ const SqlEditor: FC<Props> = ({
}, [dispatch, queryEditor.sql, startQuery, stopQuery, formatCurrentQuery]);
const hotkeys = useMemo(() => {
- // Get all hotkeys including ace editor hotkeys
+ // Get all hotkeys including editor hotkeys
// Get the user's OS
const userOS = detectOS();
+
+ type EditorHandle = editors.EditorHandle;
+
+ /**
+ * Find the position of a semicolon in the given direction from a starting
position.
+ * Returns the position after the semicolon (for backwards) or at the
semicolon (for forwards).
+ */
+ const findSemicolon = (
+ lines: string[],
+ fromLine: number,
+ fromColumn: number,
+ backwards: boolean,
+ ): { line: number; column: number } | null => {
+ if (backwards) {
+ // Search backwards: start from current position going up
+ for (let line = fromLine; line >= 0; line -= 1) {
+ const lineText = lines[line];
+ const searchEnd = line === fromLine ? fromColumn : lineText.length;
+ // Search from right to left within the line
+ const idx = lineText.lastIndexOf(';', searchEnd - 1);
+ if (idx !== -1) {
+ // Return position after the semicolon
+ return { line, column: idx + 1 };
+ }
+ }
+ } else {
+ // Search forwards: start from current position going down
+ for (let line = fromLine; line < lines.length; line += 1) {
+ const lineText = lines[line];
+ const searchStart = line === fromLine ? fromColumn + 1 : 0;
+ const idx = lineText.indexOf(';', searchStart);
+ if (idx !== -1) {
+ // Return position at the semicolon (end of statement)
+ return { line, column: idx + 1 };
+ }
+ }
+ }
+ return null;
+ };
+
const base = [
...getHotkeyConfig(),
{
name: 'runQuery3',
key: KeyboardShortcut.CtrlShiftEnter,
descr: KEY_MAP[KeyboardShortcut.CtrlShiftEnter],
- func: (editor: AceEditor['editor']) => {
- if (!editor.getValue().trim()) {
+ func: (editor: EditorHandle) => {
+ const value = editor.getValue();
+ if (!value.trim()) {
return;
}
- const session = editor.getSession();
+
+ const lines = value.split('\n');
const cursorPosition = editor.getCursorPosition();
- const totalLine = session.getLength();
- const currentRow = editor.getFirstVisibleRow();
- const semicolonEnd = editor.find(';', {
- backwards: false,
- skipCurrent: true,
- });
- let end;
- if (semicolonEnd) {
- ({ end } = semicolonEnd);
- }
- if (!end || end.row < cursorPosition.row) {
- end = {
- row: totalLine + 1,
- column: 0,
- };
+ const totalLines = lines.length;
+
+ // Find the end of the statement (next semicolon or end of file)
+ const semicolonEnd = findSemicolon(
+ lines,
+ cursorPosition.line,
+ cursorPosition.column,
+ false,
+ );
+ let end: { line: number; column: number };
+ if (semicolonEnd && semicolonEnd.line >= cursorPosition.line) {
Review Comment:
**Suggestion:** Off-by-one / out-of-bounds selection: when no semicolon is
found the code sets the end position to `{ line: totalLines, column: 0 }`.
`totalLines` is `lines.length` so the highest valid line index is `totalLines -
1`. Using `line: totalLines` produces an invalid position and can cause the
editor selection APIs to throw or behave unpredictably. Use the last line index
and its length as the fallback end position (handle empty content safely).
[off-by-one]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Ctrl+Shift+Enter runs current-statement selection fails.
- ⚠️ SqlLab editor hotkey may throw runtime selection errors.
- ⚠️ Affects all editor providers via EditorWrapper selection API.
```
</details>
```suggestion
// No semicolon found forward, use end of file (last line
index)
if (totalLines === 0) {
end = { line: 0, column: 0 };
} else {
const lastLineIndex = totalLines - 1;
end = {
line: lastLineIndex,
column: lines[lastLineIndex]?.length ?? 0,
};
}
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open SqlLab and focus an editor tab that uses SqlEditor component:
superset-frontend/src/SqlLab/components/SqlEditor/index.tsx lines
~429-559 (runQuery3
hotkey logic).
2. Place the cursor in a SQL buffer that contains no semicolon after the
cursor
(typical for single-statement queries without trailing semicolons). This
triggers
the run-statement hotkey path when pressing Ctrl+Shift+Enter.
3. The hotkey handler (runQuery3) at
superset-frontend/src/SqlLab/components/SqlEditor/index.tsx:496-500
executes the
fallback
branch and sets end = { line: totalLines, column: 0 } where totalLines ===
lines.length.
4. Because valid line indices are 0..lines.length-1, setting line:
totalLines produces an
out-of-range cursor/selection position when EditorHandle.setSelection()
is later
invoked
(editor.setSelection(...) after line 540 in the same file). This can
cause the editor
provider's selection API to throw or behave unpredictably.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/SqlLab/components/SqlEditor/index.tsx
**Line:** 499:499
**Comment:**
*Off By One: Off-by-one / out-of-bounds selection: when no semicolon is
found the code sets the end position to `{ line: totalLines, column: 0 }`.
`totalLines` is `lines.length` so the highest valid line index is `totalLines -
1`. Using `line: totalLines` produces an invalid position and can cause the
editor selection APIs to throw or behave unpredictably. Use the last line index
and its length as the fallback end position (handle empty content safely).
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/SqlLab/components/EditorWrapper/index.tsx:
##########
@@ -0,0 +1,365 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { useState, useEffect, useRef, useCallback, useMemo } from 'react';
+import { shallowEqual, useDispatch, useSelector } from 'react-redux';
+import { usePrevious } from '@superset-ui/core';
+import { css, useTheme } from '@apache-superset/core/ui';
+import { Global } from '@emotion/react';
+import type { editors } from '@apache-superset/core';
+
+import { SQL_EDITOR_LEFTBAR_WIDTH } from 'src/SqlLab/constants';
+import { queryEditorSetSelectedText } from 'src/SqlLab/actions/sqlLab';
+import type { KeyboardShortcut } from
'src/SqlLab/components/KeyboardShortcutButton';
+import useQueryEditor from 'src/SqlLab/hooks/useQueryEditor';
+import { SqlLabRootState, type CursorPosition } from 'src/SqlLab/types';
+import { EditorHost } from 'src/core/editors';
+import { useAnnotations } from './useAnnotations';
+import { useKeywords } from './useKeywords';
+
+type EditorHandle = editors.EditorHandle;
+type EditorHotkey = editors.EditorHotkey;
+type EditorAnnotation = editors.EditorAnnotation;
+
+type HotKey = {
+ key: KeyboardShortcut;
+ descr?: string;
+ name: string;
+ func: (editor: EditorHandle) => void;
+};
+
+type EditorWrapperProps = {
+ autocomplete: boolean;
+ onBlur: (sql: string) => void;
+ onChange: (sql: string) => void;
+ queryEditorId: string;
+ onCursorPositionChange: (position: CursorPosition) => void;
+ height: string;
+ hotkeys: HotKey[];
+};
+
+/**
+ * Convert legacy HotKey format to EditorHotkey format.
+ */
+const convertHotkeys = (
+ hotkeys: HotKey[],
+ onRunQuery: () => void,
+): EditorHotkey[] => {
+ const result: EditorHotkey[] = [
+ // Add the built-in run query hotkey
+ {
+ name: 'runQuery',
+ key: 'Alt-enter',
+ description: 'Run query',
+ exec: () => onRunQuery(),
+ },
+ ];
+
+ hotkeys.forEach(keyConfig => {
+ result.push({
+ name: keyConfig.name,
+ key: keyConfig.key,
+ description: keyConfig.descr,
+ exec: keyConfig.func,
+ });
+ });
+
+ return result;
+};
+
+/**
+ * Ace annotation format returned from useAnnotations when data is available.
+ */
+type AceAnnotation = {
+ row: number;
+ column: number;
+ text: string | null;
+ type: string;
+};
+
+/**
+ * Type guard to check if an annotation is in Ace format.
+ */
+const isAceAnnotation = (ann: unknown): ann is AceAnnotation =>
+ typeof ann === 'object' &&
+ ann !== null &&
+ 'row' in ann &&
+ 'column' in ann &&
+ 'text' in ann &&
+ 'type' in ann;
+
+/**
+ * Convert annotation array to EditorAnnotation format.
+ * Handles the union type returned from useAnnotations.
+ */
+const convertAnnotations = (
+ annotations?: unknown[],
+): EditorAnnotation[] | undefined => {
+ if (!annotations || annotations.length === 0) return undefined;
+ // Check if first item is in Ace format (has row, column, text, type)
+ if (!isAceAnnotation(annotations[0])) return undefined;
+ return (annotations as AceAnnotation[]).map(ann => ({
+ line: ann.row,
+ column: ann.column,
+ message: ann.text ?? '',
+ severity: ann.type as EditorAnnotation['severity'],
+ }));
+};
+
+/**
+ * EditorWrapper component that renders the SQL editor using EditorHost.
+ * Uses the default Ace editor or an extension-provided editor based on
+ * what's registered with the editors API.
+ */
+const EditorWrapper = ({
+ autocomplete,
+ onBlur = () => {},
+ onChange = () => {},
+ queryEditorId,
+ onCursorPositionChange,
+ height,
+ hotkeys,
+}: EditorWrapperProps) => {
+ const dispatch = useDispatch();
+ const queryEditor = useQueryEditor(queryEditorId, [
+ 'id',
+ 'dbId',
+ 'sql',
+ 'catalog',
+ 'schema',
+ 'templateParams',
+ 'tabViewId',
+ ]);
+ // Prevent a maximum update depth exceeded error
+ // by skipping access the unsaved query editor state
+ const cursorPosition = useSelector<SqlLabRootState, CursorPosition>(
+ ({ sqlLab: { queryEditors } }) => {
+ const { cursorPosition: pos } = {
+ ...queryEditors.find(({ id }) => id === queryEditorId),
+ };
Review Comment:
**Suggestion:** Spreading the result of `queryEditors.find(...)` can throw a
TypeError if no editor is found (the find returns undefined and spreading
undefined raises). Safely handle a missing find result before
spreading/destructuring to avoid runtime crashes. [type error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ SQL Lab query editor crashes on mount.
- ⚠️ Any EditorHost consumer may unmount unexpectedly.
- ⚠️ Redux selector errors surface in UI render.
```
</details>
```suggestion
const found = queryEditors.find(({ id }) => id === queryEditorId) ??
{};
const { cursorPosition: pos } = found as Partial<{ cursorPosition?:
CursorPosition }>;
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Mount the EditorWrapper component in the running app (file:
superset-frontend/src/SqlLab/components/EditorWrapper/index.tsx) so the
component's render
executes the selector at lines 150-158.
2. Ensure the Redux slice sqlLab.queryEditors does NOT contain an editor
with the prop
queryEditorId passed to EditorWrapper. The selector's callback at lines
151-156 calls
queryEditors.find(({ id }) => id === queryEditorId) (line 153).
3. Because Array.prototype.find returns undefined when no match exists, the
code spreads
that undefined into an object literal at line 152-154 ("const {
cursorPosition: pos } = {
...queryEditors.find(...) }"), which throws a TypeError during the
useSelector callback.
4. Observe the component throwing at runtime during render (stack trace will
point to
superset-frontend/src/SqlLab/components/EditorWrapper/index.tsx:152-154).
This prevents
the EditorWrapper from mounting and breaks the SQL Lab editor UI.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/SqlLab/components/EditorWrapper/index.tsx
**Line:** 152:154
**Comment:**
*Type Error: Spreading the result of `queryEditors.find(...)` can throw
a TypeError if no editor is found (the find returns undefined and spreading
undefined raises). Safely handle a missing find result before
spreading/destructuring to avoid runtime crashes.
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]
