korbit-ai[bot] commented on code in PR #35986:
URL: https://github.com/apache/superset/pull/35986#discussion_r2511309199
##########
superset-frontend/packages/superset-ui-core/src/components/AsyncAceEditor/index.tsx:
##########
@@ -207,6 +208,49 @@ export function AsyncAceEditor(
}
}, [keywords, setCompleters]);
+ // Move autocomplete popup to the nearest parent container with
data-ace-container
+ useEffect(() => {
+ const editorInstance = (ref as React.RefObject<AceEditor>)?.current
+ ?.editor;
+ if (!editorInstance) return;
+
+ const moveAutocompleteToContainer = () => {
+ const editorContainer = editorInstance.container;
+ const autocompletePopup =
+ (editorContainer?.querySelector(
+ '.ace_autocomplete',
+ ) as HTMLElement) ||
+ (document.querySelector('.ace_autocomplete') as HTMLElement);
+ if (autocompletePopup) {
+ const targetContainer =
+ editorContainer?.closest('#ace-editor') ||
+ editorContainer?.parentElement;
+ if (targetContainer && targetContainer !== document.body) {
+ targetContainer.appendChild(autocompletePopup);
+ autocompletePopup.setAttribute('data-ace-autocomplete',
'true');
+ }
+ }
+ };
+
+ // Hook into Ace's command execution to detect when autocomplete
starts
+ const handleAfterExec = (e: Ace.Operation) => {
+ if (
+ e.command.name &&
+ ['insertstring', 'startAutocomplete'].includes(e.command.name)
+ ) {
+ moveAutocompleteToContainer();
+ }
+ };
Review Comment:
### Missing validation for Ace operation structure <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The event handler doesn't validate the structure of the Ace.Operation
object, which could cause runtime errors if the expected properties don't exist.
###### Why this matters
If the Ace.Operation object doesn't have the expected command structure,
accessing e.command.name could throw a TypeError, breaking the autocomplete
functionality.
###### Suggested change ∙ *Feature Preview*
Add proper validation for the operation object:
```typescript
const handleAfterExec = (e: Ace.Operation) => {
if (
e?.command?.name &&
['insertstring', 'startAutocomplete'].includes(e.command.name)
) {
moveAutocompleteToContainer();
}
};
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/05c11c70-a9be-4e88-b04e-8424580f9baf/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/05c11c70-a9be-4e88-b04e-8424580f9baf?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/05c11c70-a9be-4e88-b04e-8424580f9baf?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/05c11c70-a9be-4e88-b04e-8424580f9baf?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/05c11c70-a9be-4e88-b04e-8424580f9baf)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:cd1c1e40-7797-48c5-9057-cab9e893756d -->
[](cd1c1e40-7797-48c5-9057-cab9e893756d)
##########
superset-frontend/packages/superset-ui-core/src/components/AsyncAceEditor/index.tsx:
##########
@@ -207,6 +208,49 @@ export function AsyncAceEditor(
}
}, [keywords, setCompleters]);
+ // Move autocomplete popup to the nearest parent container with
data-ace-container
+ useEffect(() => {
+ const editorInstance = (ref as React.RefObject<AceEditor>)?.current
+ ?.editor;
+ if (!editorInstance) return;
+
+ const moveAutocompleteToContainer = () => {
+ const editorContainer = editorInstance.container;
+ const autocompletePopup =
+ (editorContainer?.querySelector(
+ '.ace_autocomplete',
+ ) as HTMLElement) ||
+ (document.querySelector('.ace_autocomplete') as HTMLElement);
Review Comment:
### Unsafe type assertions on nullable DOM queries <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The code uses unsafe type assertions that could cause runtime crashes when
the querySelector returns null.
###### Why this matters
If neither querySelector finds the autocomplete popup element, the code will
attempt to call methods on null, leading to TypeError crashes at runtime.
###### Suggested change ∙ *Feature Preview*
Add proper null checks before type assertions:
```typescript
const autocompletePopup =
editorContainer?.querySelector('.ace_autocomplete') ||
document.querySelector('.ace_autocomplete');
if (!autocompletePopup) return;
const popup = autocompletePopup as HTMLElement;
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5fbf4c93-2750-4fd6-9a03-a5eaa4c9d61a/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5fbf4c93-2750-4fd6-9a03-a5eaa4c9d61a?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5fbf4c93-2750-4fd6-9a03-a5eaa4c9d61a?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5fbf4c93-2750-4fd6-9a03-a5eaa4c9d61a?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5fbf4c93-2750-4fd6-9a03-a5eaa4c9d61a)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:a08cd591-d5cf-4acf-bd7d-d5751c9e9841 -->
[](a08cd591-d5cf-4acf-bd7d-d5751c9e9841)
##########
superset-frontend/packages/superset-ui-core/src/components/AsyncAceEditor/index.tsx:
##########
@@ -207,6 +208,49 @@ export function AsyncAceEditor(
}
}, [keywords, setCompleters]);
+ // Move autocomplete popup to the nearest parent container with
data-ace-container
+ useEffect(() => {
+ const editorInstance = (ref as React.RefObject<AceEditor>)?.current
+ ?.editor;
+ if (!editorInstance) return;
+
+ const moveAutocompleteToContainer = () => {
+ const editorContainer = editorInstance.container;
+ const autocompletePopup =
+ (editorContainer?.querySelector(
+ '.ace_autocomplete',
+ ) as HTMLElement) ||
+ (document.querySelector('.ace_autocomplete') as HTMLElement);
+ if (autocompletePopup) {
+ const targetContainer =
+ editorContainer?.closest('#ace-editor') ||
+ editorContainer?.parentElement;
+ if (targetContainer && targetContainer !== document.body) {
+ targetContainer.appendChild(autocompletePopup);
+ autocompletePopup.setAttribute('data-ace-autocomplete',
'true');
+ }
+ }
+ };
Review Comment:
### Uncached DOM queries in command handler <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
DOM queries are performed on every command execution without caching,
causing unnecessary performance overhead.
###### Why this matters
Multiple DOM queries (querySelector, closest, parentElement) execute
repeatedly for each 'insertstring' or 'startAutocomplete' command, creating
performance bottlenecks especially during rapid typing or frequent autocomplete
triggers.
###### Suggested change ∙ *Feature Preview*
Cache DOM elements outside the handler function and only re-query when
necessary:
```typescript
let cachedAutocompletePopup: HTMLElement | null = null;
let cachedTargetContainer: Element | null = null;
const moveAutocompleteToContainer = () => {
if (!cachedAutocompletePopup) {
cachedAutocompletePopup =
editorContainer?.querySelector('.ace_autocomplete') as HTMLElement ||
document.querySelector('.ace_autocomplete') as
HTMLElement;
}
if (cachedAutocompletePopup && !cachedTargetContainer) {
cachedTargetContainer = editorContainer?.closest('#ace-editor') ||
editorContainer?.parentElement;
}
// ... rest of logic
};
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/eeb0626a-9b7c-4a0d-af58-59e6ee16f3e0/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/eeb0626a-9b7c-4a0d-af58-59e6ee16f3e0?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/eeb0626a-9b7c-4a0d-af58-59e6ee16f3e0?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/eeb0626a-9b7c-4a0d-af58-59e6ee16f3e0?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/eeb0626a-9b7c-4a0d-af58-59e6ee16f3e0)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:0abf76dd-e38e-4c6f-8176-47b9191670aa -->
[](0abf76dd-e38e-4c6f-8176-47b9191670aa)
##########
superset-frontend/packages/superset-ui-core/src/components/AsyncAceEditor/index.tsx:
##########
@@ -207,6 +208,49 @@ export function AsyncAceEditor(
}
}, [keywords, setCompleters]);
+ // Move autocomplete popup to the nearest parent container with
data-ace-container
+ useEffect(() => {
+ const editorInstance = (ref as React.RefObject<AceEditor>)?.current
+ ?.editor;
+ if (!editorInstance) return;
+
+ const moveAutocompleteToContainer = () => {
+ const editorContainer = editorInstance.container;
+ const autocompletePopup =
+ (editorContainer?.querySelector(
+ '.ace_autocomplete',
+ ) as HTMLElement) ||
+ (document.querySelector('.ace_autocomplete') as HTMLElement);
+ if (autocompletePopup) {
+ const targetContainer =
+ editorContainer?.closest('#ace-editor') ||
+ editorContainer?.parentElement;
+ if (targetContainer && targetContainer !== document.body) {
+ targetContainer.appendChild(autocompletePopup);
+ autocompletePopup.setAttribute('data-ace-autocomplete',
'true');
+ }
+ }
+ };
+
+ // Hook into Ace's command execution to detect when autocomplete
starts
+ const handleAfterExec = (e: Ace.Operation) => {
+ if (
+ e.command.name &&
+ ['insertstring', 'startAutocomplete'].includes(e.command.name)
+ ) {
+ moveAutocompleteToContainer();
+ }
+ };
Review Comment:
### Inefficient command name checking <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Array.includes() is called on every command execution for a small, static
array.
###### Why this matters
Using includes() for a small set of known values creates unnecessary array
iteration overhead when called frequently during typing, especially when a
simple equality check or Set lookup would be more efficient.
###### Suggested change ∙ *Feature Preview*
Use a Set for O(1) lookup or direct equality comparison:
```typescript
const AUTOCOMPLETE_COMMANDS = new Set(['insertstring', 'startAutocomplete']);
const handleAfterExec = (e: Ace.Operation) => {
if (e.command.name && AUTOCOMPLETE_COMMANDS.has(e.command.name)) {
moveAutocompleteToContainer();
}
};
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2ff82daf-6bdd-48fd-a26b-46c49ad8eadd/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2ff82daf-6bdd-48fd-a26b-46c49ad8eadd?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2ff82daf-6bdd-48fd-a26b-46c49ad8eadd?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2ff82daf-6bdd-48fd-a26b-46c49ad8eadd?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2ff82daf-6bdd-48fd-a26b-46c49ad8eadd)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:93062c6a-749c-4405-8e90-312d70c1d8b2 -->
[](93062c6a-749c-4405-8e90-312d70c1d8b2)
--
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]
