codeant-ai-for-open-source[bot] commented on code in PR #36760:
URL: https://github.com/apache/superset/pull/36760#discussion_r2664830007
##########
superset-frontend/src/SqlLab/actions/sqlLab.ts:
##########
@@ -430,11 +613,19 @@ export function postStopQuery(query) {
};
}
-export function setDatabases(databases) {
+export function setDatabases(
+ databases: Record<string, Database>,
Review Comment:
**Suggestion:** The `setDatabases` action creator is typed to accept a
`Record<string, Database>`, while the reducer's `SET_DATABASES` handler treats
`action.databases` as an array and calls `.forEach` on it; if callers follow
the action's type and pass a record, this will cause a runtime `TypeError:
action.databases.forEach is not a function`. To keep the contract consistent
and avoid this failure, the action should accept a `Database[]`, matching what
the reducer expects. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
export function setDatabases(databases: Database[]): SqlLabAction {
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The PR adds a setDatabases action creator typed as Record<string, Database>
but the reducer (superset-frontend/src/SqlLab/reducers/sqlLab.ts) clearly
treats action.databases as an array and iterates over it:
(action.databases as any[])!.forEach((db: any) => { ... });
This is a real typing/contract mismatch. If callers provide a record
(matching the current action signature) the reducer will throw
"action.databases.forEach is not a function" at runtime. Changing the action
signature to accept Database[] brings the action type in-line with the
reducer's expectation and prevents this class of runtime error. The change is
not merely stylistic — it fixes a real inconsistency verified in the codebase.
</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/actions/sqlLab.ts
**Line:** 616:617
**Comment:**
*Logic Error: The `setDatabases` action creator is typed to accept a
`Record<string, Database>`, while the reducer's `SET_DATABASES` handler treats
`action.databases` as an array and calls `.forEach` on it; if callers follow
the action's type and pass a record, this will cause a runtime `TypeError:
action.databases.forEach is not a function`. To keep the contract consistent
and avoid this failure, the action should accept a `Database[]`, matching what
the reducer expects.
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/SaveDatasetModal/index.tsx:
##########
@@ -344,9 +344,9 @@ export const SaveDatasetModal = ({
dispatch(
createDatasource({
sql: datasource.sql,
- dbId: datasource.dbId || datasource?.database?.id,
- catalog: datasource?.catalog,
- schema: datasource?.schema,
+ dbId: (datasource.dbId || datasource?.database?.id) as number,
Review Comment:
**Suggestion:** The `dbId` fallback currently uses the `||` operator, which
treats `0` as falsy and will incorrectly ignore a valid `dbId` of `0` in favor
of `datasource.database.id`. This can lead to sending the wrong `database` id
to the `/api/v1/dataset/` endpoint and associating the new virtual dataset with
an unintended database. Using nullish coalescing (`??`) ensures that only
`null`/`undefined` trigger the fallback while preserving legitimate numeric
ids. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
dbId: (datasource.dbId ?? datasource?.database?.id) as number,
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The current code uses the logical OR operator which treats 0 as falsy. If a
valid database id of 0 is possible in this environment, the expression will
incorrectly fall back to datasource?.database?.id, potentially associating the
new dataset with the wrong database. Replacing `||` with the nullish coalescing
operator `??` preserves legitimate numeric 0 while still falling back only on
null/undefined, so this is a real logic bug fix rather than a cosmetic change.
</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/SaveDatasetModal/index.tsx
**Line:** 347:347
**Comment:**
*Logic Error: The `dbId` fallback currently uses the `||` operator,
which treats `0` as falsy and will incorrectly ignore a valid `dbId` of `0` in
favor of `datasource.database.id`. This can lead to sending the wrong
`database` id to the `/api/v1/dataset/` endpoint and associating the new
virtual dataset with an unintended database. Using nullish coalescing (`??`)
ensures that only `null`/`undefined` trigger the fallback while preserving
legitimate numeric ids.
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/TableElement/index.tsx:
##########
@@ -171,7 +171,7 @@ const TableElement = ({ table, ...props }:
TableElementProps) => {
dispatch(
tableApiUtil.invalidateTags([{ type: 'TableMetadatas', id: name }]),
);
- dispatch(syncTable(table, tableData));
+ dispatch(syncTable(table, tableData, undefined));
Review Comment:
**Suggestion:** The refresh handler calls the persistence action with an
explicit `undefined` `finalQueryEditorId`, which causes `syncTable` to
overwrite the table's existing `queryEditorId` with `undefined` in the payload,
potentially breaking the association with its tab state and causing backend
persistence errors when SQL Lab backend persistence is enabled. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
dispatch(syncTable(table, tableData, currentQueryEditorId));
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The current code explicitly passes undefined as finalQueryEditorId into
syncTable. Looking at syncTable (src/SqlLab/actions/sqlLab.ts), it builds
finalTable = { ...table, queryEditorId: finalQueryEditorId } and will therefore
overwrite the table's existing queryEditorId with undefined when backend
persistence is enabled. That will sever the table ↔ tab association and can
cause backend persistence/migration to behave incorrectly. Elsewhere in this
same component the code uses currentQueryEditorId when syncing during
initialization, which preserves the correct tab id. Replacing undefined with
currentQueryEditorId fixes a real logic bug (not just stylistic) and keeps
behavior consistent with the rest of the component.
</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/TableElement/index.tsx
**Line:** 174:174
**Comment:**
*Logic Error: The refresh handler calls the persistence action with an
explicit `undefined` `finalQueryEditorId`, which causes `syncTable` to
overwrite the table's existing `queryEditorId` with `undefined` in the payload,
potentially breaking the association with its tab state and causing backend
persistence errors when SQL Lab backend persistence is enabled.
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/SqlEditorLeftBar/index.tsx:
##########
@@ -195,7 +195,8 @@ const SqlEditorLeftBar = ({ queryEditorId }:
SqlEditorLeftBarProps) => {
const handleDbList = useCallback(
(result: DatabaseObject) => {
- dispatch(setDatabases(result));
+ // eslint-disable-next-line @typescript-eslint/no-explicit-any
Review Comment:
**Suggestion:** The database list callback receives an array of database
objects from `DatabaseSelector`, but it forwards this array directly to
`setDatabases`, which expects a map keyed by database id; this shape mismatch
can cause lookups like `databases[dbId]` to fail at runtime, breaking features
such as table preview and async-run checks that rely on retrieving a database
by its id. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
(result: DatabaseObject[]) => {
const databasesById = Object.fromEntries(
result.map(db => [db.id, db]),
);
// eslint-disable-next-line @typescript-eslint/no-explicit-any
dispatch(setDatabases(databasesById as any));
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The current PR code types the callback parameter as a single DatabaseObject
but then passes it directly to setDatabases as any.
The action creator setDatabases in this codebase expects a map keyed by
database id (the rest of the file accesses databases[dbId]).
If the upstream component actually supplies an array of DatabaseObject,
dispatching that array will produce a runtime shape mismatch and subsequent
lookups like databases[dbId] will fail.
The proposed change converts an incoming array into an id-keyed object,
matching the shape consumed elsewhere and preventing runtime lookup errors.
This is a functional fix, not merely stylistic.
</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/SqlEditorLeftBar/index.tsx
**Line:** 197:198
**Comment:**
*Logic Error: The database list callback receives an array of database
objects from `DatabaseSelector`, but it forwards this array directly to
`setDatabases`, which expects a map keyed by database id; this shape mismatch
can cause lookups like `databases[dbId]` to fail at runtime, breaking features
such as table preview and async-run checks that rely on retrieving a database
by its id.
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]
