korbit-ai[bot] commented on code in PR #35216:
URL: https://github.com/apache/superset/pull/35216#discussion_r2373509422
##########
superset-frontend/src/SqlLab/actions/sqlLab.js:
##########
@@ -956,7 +956,7 @@ export function addTable(queryEditor, tableName,
catalogName, schemaName) {
const { dbId } = getUpToDateQuery(getState(), queryEditor, queryEditor.id);
const table = {
dbId,
- queryEditorId: queryEditor.id,
+ queryEditorId: queryEditor.tabViewId ?? queryEditor.id,
Review Comment:
### Inconsistent queryEditorId assignment breaks table-query editor
association <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The addTable function uses tabViewId as queryEditorId but other parts of the
codebase expect queryEditorId to match the actual queryEditor.id for proper
state management and query association.
###### Why this matters
This inconsistency can cause tables to be associated with the wrong query
editor ID, leading to broken functionality when tables are referenced by other
parts of the system that expect queryEditorId to match the queryEditor's actual
ID. This could result in tables not appearing in the correct tabs or being
inaccessible.
###### Suggested change ∙ *Feature Preview*
The code should maintain consistency with how queryEditorId is used
throughout the system. If tabViewId is needed for backend persistence, it
should be handled separately or the entire codebase should be updated to use
tabViewId consistently. Consider using:
```javascript
queryEditorId: queryEditor.id,
tabViewId: queryEditor.tabViewId,
```
Or ensure all related functions that reference queryEditorId are updated to
handle the tabViewId fallback pattern.
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a4360247-a330-4ec5-b398-263fc8541870/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a4360247-a330-4ec5-b398-263fc8541870?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a4360247-a330-4ec5-b398-263fc8541870?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a4360247-a330-4ec5-b398-263fc8541870?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a4360247-a330-4ec5-b398-263fc8541870)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:5dd0e9ae-25e6-4f38-99b1-fad98b42ae14 -->
[](5dd0e9ae-25e6-4f38-99b1-fad98b42ae14)
##########
superset-frontend/src/SqlLab/components/AceEditorWrapper/useKeywords.ts:
##########
@@ -147,7 +148,12 @@ export function useKeywords(
const insertMatch = useEffectEvent((editor: Editor, data: any) => {
if (data.meta === 'table') {
dispatch(
- addTable({ id: queryEditorId, dbId }, data.value, catalog, schema),
+ addTable(
+ { id: queryEditorId, dbId, tabViewId },
+ data.value,
+ catalog,
+ schema,
+ ),
);
}
Review Comment:
### Function with Mixed Responsibilities <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The insertMatch function handles multiple responsibilities: table insertion,
caption formatting, and editor completion. This violates the Single
Responsibility Principle.
###### Why this matters
Mixed responsibilities make the code harder to maintain, test, and modify.
Changes to one aspect might affect others unexpectedly.
###### Suggested change ∙ *Feature Preview*
Split the function into smaller, focused functions:
```typescript
const handleTableInsertion = (data: TableData) => {
dispatch(addTable({ id: queryEditorId, dbId, tabViewId }, data.value,
catalog, schema));
};
const formatCaption = (data: CompletionData) => {
return data.meta === 'table' && data.caption.includes(' ') ?
`"${data.caption}"` : data.caption;
};
const insertMatch = useEffectEvent((editor: Editor, data: CompletionData) =>
{
if (data.meta === 'table') {
handleTableInsertion(data);
}
const caption = formatCaption(data);
editor.completer.insertMatch(`${caption}${['function',
'schema'].includes(data.meta) ? '' : ' '}`);
});
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/31198c54-1f3b-4229-9df0-10d8ccce30d2/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/31198c54-1f3b-4229-9df0-10d8ccce30d2?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/31198c54-1f3b-4229-9df0-10d8ccce30d2?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/31198c54-1f3b-4229-9df0-10d8ccce30d2?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/31198c54-1f3b-4229-9df0-10d8ccce30d2)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:f79df6ce-b675-4db5-9bc3-a776adababba -->
[](f79df6ce-b675-4db5-9bc3-a776adababba)
##########
superset-frontend/src/SqlLab/components/AceEditorWrapper/useKeywords.ts:
##########
@@ -147,7 +148,12 @@ export function useKeywords(
const insertMatch = useEffectEvent((editor: Editor, data: any) => {
if (data.meta === 'table') {
dispatch(
- addTable({ id: queryEditorId, dbId }, data.value, catalog, schema),
+ addTable(
+ { id: queryEditorId, dbId, tabViewId },
+ data.value,
+ catalog,
+ schema,
+ ),
Review Comment:
### Undefined tabViewId passed to addTable <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The addTable action is called with tabViewId even when it might be
undefined, potentially causing inconsistent behavior in table association logic.
###### Why this matters
If tabViewId is undefined, the addTable action may not properly associate
the table with the correct tab, leading to tables being added to the wrong tab
or causing state management issues in multi-tab scenarios.
###### Suggested change ∙ *Feature Preview*
Add a conditional check to ensure tabViewId is only passed when it has a
valid value:
```typescript
addTable(
tabViewId
? { id: queryEditorId, dbId, tabViewId }
: { id: queryEditorId, dbId },
data.value,
catalog,
schema,
),
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/dfa5a85b-4044-49f1-9b10-da63d1836106/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/dfa5a85b-4044-49f1-9b10-da63d1836106?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/dfa5a85b-4044-49f1-9b10-da63d1836106?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/dfa5a85b-4044-49f1-9b10-da63d1836106?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/dfa5a85b-4044-49f1-9b10-da63d1836106)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:8ce0dfc7-bc57-437e-a43c-b4f9f0f765bb -->
[](8ce0dfc7-bc57-437e-a43c-b4f9f0f765bb)
--
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]
