codeant-ai-for-open-source[bot] commented on code in PR #37499: URL: https://github.com/apache/superset/pull/37499#discussion_r2737917606
########## superset-frontend/src/core/editors/EditorHost.test.tsx: ########## @@ -0,0 +1,90 @@ +/** + * 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 { render, screen, cleanup } from 'spec/helpers/testing-library'; +import type { editors } from '@apache-superset/core'; + +afterEach(() => { + cleanup(); +}); + +type EditorProps = editors.EditorProps; + +// Mock the AceEditorProvider to avoid loading the full Ace editor in tests +jest.mock('./AceEditorProvider', () => ({ + __esModule: true, + default: ({ id, value, language }: EditorProps) => ( + <div data-test="ace-editor-provider"> + <span data-test="ace-editor-id">{id}</span> + <span data-test="ace-editor-value">{value}</span> + <span data-test="ace-editor-language">{language}</span> Review Comment: **Suggestion:** The mock Ace editor renders attributes using `data-test` but tests query `data-testid` via `getByTestId`; this mismatch causes all `getByTestId` lookups to fail at runtime. Replace `data-test` with `data-testid` in the mock component so the tests query the correct attributes. [logic error] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ EditorHost unit tests fail in CI. - ⚠️ Blocks verification of editor provider behavior. ``` </details> ```suggestion <div data-testid="ace-editor-provider"> <span data-testid="ace-editor-id">{id}</span> <span data-testid="ace-editor-value">{value}</span> <span data-testid="ace-editor-language">{language}</span> ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Run the EditorHost tests: execute Jest for superset-frontend/src/core/editors/EditorHost.test.tsx. 2. Jest evaluates the file and executes the mock factory at superset-frontend/src/core/editors/EditorHost.test.tsx:28-38 which returns JSX with attributes named `data-test`. 3. Tests in the same file assert using screen.getByTestId(...) at superset-frontend/src/core/editors/EditorHost.test.tsx:66,67,71 (e.g., expect(screen.getByTestId('ace-editor-provider')).toBeInTheDocument()). 4. Because the mock renders `data-test` attributes but tests query `data-testid`, getByTestId throws a NotFoundError and those assertions fail, causing the test to fail locally and in CI. ``` </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/editors/EditorHost.test.tsx **Line:** 32:35 **Comment:** *Logic Error: The mock Ace editor renders attributes using `data-test` but tests query `data-testid` via `getByTestId`; this mismatch causes all `getByTestId` lookups to fail at runtime. Replace `data-test` with `data-testid` in the mock component so the tests query the correct attributes. 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/editors/AceEditorProvider.tsx: ########## @@ -0,0 +1,322 @@ +/** + * 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. + */ + +/** + * @fileoverview Default Ace Editor provider implementation. + * + * This module wraps the existing Ace editor components to implement the + * standard EditorProps interface, serving as the default editor when no + * extension provides a custom implementation. + */ + +import { + useRef, + useEffect, + useCallback, + useImperativeHandle, + forwardRef, + type Ref, +} from 'react'; +import type AceEditor from 'react-ace'; +import type { editors } from '@apache-superset/core'; +import { + FullSQLEditor, + JsonEditor, + MarkdownEditor, + CssEditor, + ConfigEditor, + type AceCompleterKeyword, +} from '@superset-ui/core/components'; +import { Disposable } from '../models'; + +type EditorKeyword = editors.EditorKeyword; +type EditorProps = editors.EditorProps; +type EditorHandle = editors.EditorHandle; +type Position = editors.Position; +type Range = editors.Range; +type Selection = editors.Selection; +type EditorAnnotation = editors.EditorAnnotation; +type CompletionProvider = editors.CompletionProvider; + +/** + * Maps EditorLanguage to the corresponding Ace editor component. + */ +const getEditorComponent = (language: string) => { + switch (language) { + case 'sql': + return FullSQLEditor; + case 'json': + return JsonEditor; + case 'markdown': + return MarkdownEditor; + case 'css': + return CssEditor; + case 'yaml': + return ConfigEditor; + default: + return FullSQLEditor; + } +}; + +/** + * Converts EditorAnnotation to Ace annotation format. + */ +const toAceAnnotation = (annotation: EditorAnnotation) => ({ + row: annotation.line, + column: annotation.column ?? 0, + text: annotation.message, + type: annotation.severity, +}); + +/** + * Creates an EditorHandle implementation backed by an Ace editor instance. + */ +const createAceEditorHandle = ( + aceEditorRef: React.RefObject<AceEditor>, + completionProviders: React.MutableRefObject<Map<string, CompletionProvider>>, +): EditorHandle => ({ + focus: () => { + aceEditorRef.current?.editor?.focus(); + }, + + getValue: () => aceEditorRef.current?.editor?.getValue() ?? '', + + setValue: (value: string) => { + aceEditorRef.current?.editor?.setValue(value, -1); + }, + + getCursorPosition: (): Position => { + const pos = aceEditorRef.current?.editor?.getCursorPosition(); + return { + line: pos?.row ?? 0, + column: pos?.column ?? 0, + }; + }, + + moveCursorToPosition: (position: Position) => { + aceEditorRef.current?.editor?.moveCursorToPosition({ + row: position.line, + column: position.column, + }); + }, + + getSelections: (): Selection[] => { + const selection = aceEditorRef.current?.editor?.getSelection(); + if (!selection) return []; + const range = selection.getRange(); + return [ + { + start: { line: range.start.row, column: range.start.column }, + end: { line: range.end.row, column: range.end.column }, + }, + ]; + }, + + setSelection: (range: Range) => { + const editor = aceEditorRef.current?.editor; + if (editor) { + editor.selection.setSelectionRange({ + start: { row: range.start.line, column: range.start.column }, + end: { row: range.end.line, column: range.end.column }, + }); + } + }, + + getSelectedText: () => aceEditorRef.current?.editor?.getSelectedText() ?? '', + + insertText: (text: string) => { + aceEditorRef.current?.editor?.insert(text); + }, + + executeCommand: (commandName: string) => { + const editor = aceEditorRef.current?.editor; + if (editor?.commands) { + editor.commands.exec(commandName, editor, {}); + } + }, + + scrollToLine: (line: number) => { + aceEditorRef.current?.editor?.scrollToLine(line, true, true); + }, + + setAnnotations: (annotations: EditorAnnotation[]) => { + const session = aceEditorRef.current?.editor?.getSession(); + if (session) { + session.setAnnotations(annotations.map(toAceAnnotation)); + } + }, + + clearAnnotations: () => { + const session = aceEditorRef.current?.editor?.getSession(); + if (session) { + session.clearAnnotations(); + } + }, + + registerCompletionProvider: (provider: CompletionProvider): Disposable => { + completionProviders.current.set(provider.id, provider); + return new Disposable(() => { + completionProviders.current.delete(provider.id); + }); + }, +}); + +/** + * Converts generic EditorKeyword to Ace's AceCompleterKeyword format. + */ +const toAceKeyword = (keyword: EditorKeyword): AceCompleterKeyword => ({ + name: keyword.name, + value: keyword.value ?? keyword.name, + score: keyword.score ?? 0, + meta: keyword.meta ?? '', +}); + +/** + * Default Ace-based editor provider component. + * Implements the standard EditorProps interface while wrapping the existing + * Ace editor components. + */ +const AceEditorProvider = forwardRef<EditorHandle, EditorProps>( + (props, ref) => { + const { + id, + value, + onChange, + onBlur, + onCursorPositionChange, + onSelectionChange, + language, + readOnly, + tabSize, + lineNumbers, + wordWrap, + annotations, + hotkeys, + keywords, + height = '100%', + width = '100%', + onReady, + } = props; + + const aceEditorRef = useRef<AceEditor>(null); + const completionProviders = useRef<Map<string, CompletionProvider>>( + new Map(), + ); + + // Create the handle + const handle = createAceEditorHandle(aceEditorRef, completionProviders); + + // Expose handle via ref + useImperativeHandle(ref, () => handle, []); + + // Notify when ready + useEffect(() => { + if (onReady && aceEditorRef.current?.editor) { + onReady(handle); + } + }, [onReady, handle]); + + // Handle editor load + const onEditorLoad = useCallback( + (editor: AceEditor['editor']) => { + // Register hotkeys + if (hotkeys) { + hotkeys.forEach(hotkey => { + editor.commands.addCommand({ + name: hotkey.name, + bindKey: { win: hotkey.key, mac: hotkey.key }, + exec: () => hotkey.exec(handle), + }); + }); + } + + // Set up cursor position change listener + if (onCursorPositionChange) { + editor.selection.on('changeCursor', () => { + const cursor = editor.getCursorPosition(); + onCursorPositionChange({ + line: cursor.row, + column: cursor.column, + }); + }); + } + + // Set up selection change listener + if (onSelectionChange) { + editor.selection.on('changeSelection', () => { + const range = editor.getSelection().getRange(); + onSelectionChange([ + { + start: { line: range.start.row, column: range.start.column }, + end: { line: range.end.row, column: range.end.column }, + }, + ]); + }); + } + + // Focus the editor + editor.focus(); + }, + [hotkeys, onCursorPositionChange, onSelectionChange, handle], + ); + + // Handle blur + const handleBlur = useCallback(() => { + if (onBlur) { + onBlur(value); + } + }, [onBlur, value]); + + // Get the appropriate editor component + const EditorComponent = getEditorComponent(language); + + // Convert annotations to Ace format + const aceAnnotations = annotations?.map(toAceAnnotation); + + // Convert generic keywords to Ace format + const aceKeywords = keywords?.map(toAceKeyword); + + return ( + <EditorComponent + ref={aceEditorRef as Ref<never>} + name={id} + mode={language} Review Comment: **Suggestion:** Prop name mismatch: the public editor contract uses `language` (see EditorProps) but this line passes `mode` to the editor component instead; extension consumers or other editor implementations that expect `language` will not receive it, leading to undefined language handling—replace the prop with `language={language}` so the component receives the documented prop. [possible bug] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ Editor components receive missing language prop. - ❌ Syntax highlighting may use wrong mode. - ⚠️ Affects FullSQLEditor, JsonEditor, MarkdownEditor, CssEditor. ``` </details> ```suggestion language={language} ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Open the AceEditorProvider component in superset-frontend/src/core/editors/AceEditorProvider.tsx and locate the render return (the JSX block that starts around lines 295-306 in the PR hunk). The single-line prop at line 299 is `mode={language}`. 2. Note the editor public contract is declared in packages/superset-core/src/api/editors.ts (file range provided in PR context: Start Line: 97, End Line: 381) where EditorProps includes the required property `language: EditorLanguage;`. EditorComponent type is defined to accept EditorProps. 3. Because AceEditorProvider selects a component via getEditorComponent(...) (mapping to FullSQLEditor, JsonEditor, MarkdownEditor, CssEditor, ConfigEditor imported at the top of AceEditorProvider.tsx) the child editor implementations expect to receive the `language` prop per the shared EditorProps contract. With the current code, those components receive `mode` instead of `language`, so their `props.language` will be undefined at runtime. 4. Reproduce by rendering AceEditorProvider (any place the app mounts editors that use default provider). Inspect a child editor (e.g., FullSQLEditor) props in React DevTools or add a console log inside the child component to observe `props.language` is undefined (traceable to AceEditorProvider.tsx line 299). This demonstrates the prop-name mismatch and the fix is to pass `language={language}`. ``` </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/editors/AceEditorProvider.tsx **Line:** 299:299 **Comment:** *Possible Bug: Prop name mismatch: the public editor contract uses `language` (see EditorProps) but this line passes `mode` to the editor component instead; extension consumers or other editor implementations that expect `language` will not receive it, leading to undefined language handling—replace the prop with `language={language}` so the component receives the documented prop. 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/editors/EditorHost.tsx: ########## @@ -0,0 +1,121 @@ +/** + * 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. + */ + +/** + * @fileoverview EditorHost component for dynamic editor resolution. + * + * This component resolves and renders the appropriate editor implementation + * based on the language and any registered extension providers. If an extension + * has registered an editor for the language, it uses that; otherwise, it falls + * back to the default Ace editor. + */ + +import { useState, useEffect, forwardRef } from 'react'; +import type { editors, contributions } from '@apache-superset/core'; +import { useTheme } from '@apache-superset/core/ui'; +import EditorProviders from './EditorProviders'; +import AceEditorProvider from './AceEditorProvider'; + +type EditorLanguage = contributions.EditorLanguage; +type EditorProps = editors.EditorProps; +type EditorHandle = editors.EditorHandle; + +/** + * Props for EditorHost component. + * Uses the generic EditorProps interface that all editor implementations support. + */ +export type EditorHostProps = EditorProps; + +/** + * Hook to track editor provider changes. + * Returns the provider for the specified language and re-renders when it changes. + */ +const useEditorProvider = (language: EditorLanguage) => { + const manager = EditorProviders.getInstance(); + const [provider, setProvider] = useState(() => manager.getProvider(language)); + + useEffect(() => { + // Subscribe to provider changes + const registerDisposable = manager.onDidRegister(event => { + if (event.provider.contribution.languages.includes(language)) { + setProvider(event.provider); + } + }); + + const unregisterDisposable = manager.onDidUnregister(event => { + if (event.contribution.languages.includes(language)) { + setProvider(manager.getProvider(language)); + } + }); + + // Check for provider on mount (in case it was registered before this component mounted) + const currentProvider = manager.getProvider(language); + setProvider(prev => (currentProvider !== prev ? currentProvider : prev)); Review Comment: **Suggestion:** Race condition: the one-time snapshot `currentProvider` may be stale compared to `prev` when the subscription callbacks run; the existing comparator `currentProvider !== prev` will return true and can overwrite a newer provider that was set by the registration callback — use a safer functional update that prefers an already-installed (`prev`) provider if it differs from the snapshot to avoid reverting to stale state. [race condition] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ EditorHost may render wrong editor implementation. - ❌ SQL Lab editor selection may revert unexpectedly. - ⚠️ Dashboard/CSS editor UX may show incorrect editor. - ⚠️ Extensions registering editors during mount can be affected. ``` </details> ```suggestion setProvider(prev => { // If there is no snapshot provider, keep whatever is currently mounted. if (!currentProvider) { return prev; } // If the currently mounted provider is different from the snapshot, // prefer the currently mounted provider because it may have been set // by subscription callbacks that ran after the snapshot was taken. if (prev && prev !== currentProvider) { return prev; } return currentProvider; }); ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Render EditorHost component (superset-frontend/src/core/editors/EditorHost.tsx). The EditorHost mounts and calls useEditorProvider(language) which runs the effect defined in useEditorProvider (see function body in EditorHost.tsx, lines 49-79). 2. The effect subscribes to provider registration via manager.onDidRegister(...) (subscription callback defined at EditorHost.tsx lines ~55-58). A provider registration event will invoke that callback and call setProvider(event.provider). 3. If an extension registers an editor provider for the language while the component is mounting, the onDidRegister callback runs and sets provider state to the newly registered provider (observed at EditorHost.tsx lines 55-58 where setProvider(event.provider) is called). 4. Immediately after subscribing, the effect reads a one-time snapshot with currentProvider = manager.getProvider(language) and then executes the snapshot-based update at EditorHost.tsx line 69 (the existing line: setProvider(prev => (currentProvider !== prev ? currentProvider : prev));). If the onDidRegister callback ran between subscription and this state update, prev will contain the newer provider but currentProvider will be the older snapshot; the functional update will then revert state back to the stale snapshot provider, causing the component to render the wrong editor. ``` </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/editors/EditorHost.tsx **Line:** 69:69 **Comment:** *Race Condition: Race condition: the one-time snapshot `currentProvider` may be stale compared to `prev` when the subscription callbacks run; the existing comparator `currentProvider !== prev` will return true and can overwrite a newer provider that was set by the registration callback — use a safer functional update that prefers an already-installed (`prev`) provider if it differs from the snapshot to avoid reverting to stale state. 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,363 @@ +/** + * 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 editor = queryEditors.find(({ id }) => id === queryEditorId); + return editor?.cursorPosition ?? { row: 0, column: 0 }; + }, Review Comment: **Suggestion:** Performance regression in selector: using `.find` inside the selector callback runs an O(n) scan on every store update; iterate safely with a guarded loop to avoid creating closure overhead and to bail out early when found, and guard against `queryEditors` being undefined. [performance] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ SQL Lab editor responsiveness with many open query tabs. - ⚠️ Extra CPU work during frequent store updates. ``` </details> ```suggestion const DEFAULT_CURSOR: CursorPosition = { row: 0, column: 0 }; const editorsArr = queryEditors || []; for (let i = 0; i < editorsArr.length; i += 1) { const e = editorsArr[i]; if (e.id === queryEditorId) { return e.cursorPosition ?? DEFAULT_CURSOR; } } return DEFAULT_CURSOR; ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Mount EditorWrapper instances in the SQL Lab UI (component defined at superset-frontend/src/SqlLab/components/EditorWrapper/index.tsx; the component calls `useQueryEditor(...)` at ~line 139 and also uses the cursor selector at lines 152-154). 2. For each EditorWrapper instance, the cursor selector callback executes on every relevant store update and runs `queryEditors.find(...)` (lines 153-154). If many query editors exist, `find` will scan the entire `queryEditors` array each time. 3. The codebase already iterates `queryEditors` in the hook useQueryEditor (src/SqlLab/hooks/useQueryEditor/index.ts lines 24-51) where an Object.fromEntries / map is used to build index structures; thus this additional linear scan duplicates work for each EditorWrapper instance. 4. To reproduce measurable cost: open many SQL Lab tabs so sqlLab.queryEditors contains many entries (tens to hundreds), then trigger a frequent store update (for example, workspace metadata updates). Profile CPU while the cursor selector executes repeatedly; you will observe extra time spent scanning arrays in the selector callback. ``` </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:** 153:154 **Comment:** *Performance: Performance regression in selector: using `.find` inside the selector callback runs an O(n) scan on every store update; iterate safely with a guarded loop to avoid creating closure overhead and to bail out early when found, and guard against `queryEditors` being undefined. 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]
