korbit-ai[bot] commented on code in PR #35986:
URL: https://github.com/apache/superset/pull/35986#discussion_r2490895552
##########
superset-frontend/packages/superset-ui-core/src/components/AsyncAceEditor/index.tsx:
##########
@@ -207,6 +207,44 @@ export function AsyncAceEditor(
}
}, [keywords, setCompleters]);
+ // Move autocomplete popup to the nearest parent container with
data-ace-container
+ useEffect(() => {
+ const editorInstance = (ref as any)?.current?.editor;
+ if (!editorInstance) return;
+
+ const moveAutocompleteToContainer = () => {
+ const autocompletePopup = document.querySelector(
+ '.ace_autocomplete',
+ ) as HTMLElement;
+
+ if (autocompletePopup) {
+ const editorContainer = editorInstance.container;
+
+ const targetContainer = editorContainer?.parentElement;
Review Comment:
### Missing container validation for autocomplete popup <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The code assumes the parent element is always the correct container for the
autocomplete popup without verifying it has the expected `data-ace-container`
attribute mentioned in the comment.
###### Why this matters
This could cause the autocomplete popup to be moved to an unintended
container, potentially breaking the UI layout or causing positioning issues if
the parent element is not the intended target container.
###### Suggested change ∙ *Feature Preview*
Add validation to find the nearest parent with the `data-ace-container`
attribute:
```typescript
const targetContainer = editorContainer?.closest('[data-ace-container]') ||
editorContainer?.parentElement;
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d3fb7a57-f6be-4439-b4b7-b2f2ae5636dc/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d3fb7a57-f6be-4439-b4b7-b2f2ae5636dc?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d3fb7a57-f6be-4439-b4b7-b2f2ae5636dc?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d3fb7a57-f6be-4439-b4b7-b2f2ae5636dc?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d3fb7a57-f6be-4439-b4b7-b2f2ae5636dc)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:590db06f-8558-4fbc-9703-0c219b383e3c -->
[](590db06f-8558-4fbc-9703-0c219b383e3c)
##########
superset-frontend/packages/superset-ui-core/src/components/AsyncAceEditor/index.tsx:
##########
@@ -207,6 +207,44 @@ export function AsyncAceEditor(
}
}, [keywords, setCompleters]);
+ // Move autocomplete popup to the nearest parent container with
data-ace-container
+ useEffect(() => {
+ const editorInstance = (ref as any)?.current?.editor;
+ if (!editorInstance) return;
+
+ const moveAutocompleteToContainer = () => {
+ const autocompletePopup = document.querySelector(
+ '.ace_autocomplete',
+ ) as HTMLElement;
Review Comment:
### Global selector may affect multiple editors <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Using `document.querySelector` to find the autocomplete popup could select
the wrong popup if multiple Ace editors exist on the same page.
###### Why this matters
In applications with multiple Ace editors, this could move the wrong
autocomplete popup or interfere with other editors' functionality, causing
confusion and broken autocomplete behavior.
###### Suggested change ∙ *Feature Preview*
Scope the search to the current editor's container:
```typescript
const autocompletePopup =
editorContainer?.querySelector('.ace_autocomplete') as HTMLElement ||
document.querySelector('.ace_autocomplete') as HTMLElement;
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0e739290-b608-4d38-9063-d8f764fe1840/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0e739290-b608-4d38-9063-d8f764fe1840?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0e739290-b608-4d38-9063-d8f764fe1840?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0e739290-b608-4d38-9063-d8f764fe1840?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0e739290-b608-4d38-9063-d8f764fe1840)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:b404990a-544d-459e-8e1e-fb95f75415d0 -->
[](b404990a-544d-459e-8e1e-fb95f75415d0)
##########
superset-frontend/packages/superset-ui-core/src/components/AsyncAceEditor/index.tsx:
##########
@@ -207,6 +207,44 @@ export function AsyncAceEditor(
}
}, [keywords, setCompleters]);
+ // Move autocomplete popup to the nearest parent container with
data-ace-container
+ useEffect(() => {
+ const editorInstance = (ref as any)?.current?.editor;
+ if (!editorInstance) return;
+
+ const moveAutocompleteToContainer = () => {
+ const autocompletePopup = document.querySelector(
+ '.ace_autocomplete',
+ ) as HTMLElement;
+
+ if (autocompletePopup) {
+ const editorContainer = editorInstance.container;
+
+ const targetContainer = 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: any) => {
+ if (e.command.name === 'insertstring') {
+ moveAutocompleteToContainer();
+ }
Review Comment:
### Incomplete autocomplete trigger detection <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The autocomplete popup movement is triggered only on 'insertstring' command,
which may not cover all scenarios where the autocomplete popup appears (e.g.,
Ctrl+Space, backspace triggering autocomplete).
###### Why this matters
Users may not see the autocomplete popup in the correct container when
triggered through keyboard shortcuts or other commands, leading to inconsistent
behavior and poor user experience.
###### Suggested change ∙ *Feature Preview*
Listen for multiple autocomplete-related commands or use a more
comprehensive approach:
```typescript
const handleAfterExec = (e: any) => {
if (['insertstring', 'startAutocomplete', 'golineup',
'golinedown'].includes(e.command.name)) {
setTimeout(moveAutocompleteToContainer, 0);
}
};
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7fda21e1-3677-479c-a6e4-fe9f57089a54/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7fda21e1-3677-479c-a6e4-fe9f57089a54?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7fda21e1-3677-479c-a6e4-fe9f57089a54?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7fda21e1-3677-479c-a6e4-fe9f57089a54?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7fda21e1-3677-479c-a6e4-fe9f57089a54)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:b787b8a4-fa81-434f-be37-ce2a8dc88532 -->
[](b787b8a4-fa81-434f-be37-ce2a8dc88532)
--
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]
