codeant-ai-for-open-source[bot] commented on code in PR #37652:
URL: https://github.com/apache/superset/pull/37652#discussion_r2762122286
##########
superset-frontend/src/SqlLab/reducers/sqlLab.ts:
##########
@@ -759,6 +759,13 @@ export default function sqlLabReducer(
? prevState
: currentState,
};
+ if (
+ newQueries[id].state === QueryState.Success &&
+ newQueries[id].runAsync === false &&
Review Comment:
**Suggestion:** The condition that checks for synchronous queries uses a
strict comparison `runAsync === false`, which will not match cases where
`runAsync` is `undefined` or `null` even though those are effectively non-async
runs, so such queries can incorrectly remain in the "success" state with no
results and still show the confusing UI that this change intends to fix.
Relaxing the check to treat any falsy `runAsync` as synchronous will make the
logic robust against missing or optional `runAsync` values from the backend.
[possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Query status UI shows success with no results.
- ⚠️ Progress bar and no-data state may overlap.
- ⚠️ Backend omission of runAsync causes visual bug.
```
</details>
```suggestion
!newQueries[id].runAsync &&
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Cause REFRESH_QUERIES to run
(superset-frontend/src/SqlLab/reducers/sqlLab.ts, handler
for [actions.REFRESH_QUERIES] around the PR diff lines 749-776).
2. Provide an alteredQueries entry for an existing query id where the
server's
changedQuery object does not include runAsync (i.e., runAsync is undefined
or omitted).
3. Reducer builds newQueries[id] and evaluates the added conditional at
~lines 762-768.
The strict comparison newQueries[id].runAsync === false fails when runAsync
is undefined,
so the branch that sets state = QueryState.Fetching is skipped.
4. The query remains in QueryState.Success with no results populated,
reproducing the UI
confusion (progress bar/no-data overlap) this PR aims to fix. This is
observable in SQL
Lab query rows and the results pane after a refresh from the backend that
omits runAsync.
```
</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/reducers/sqlLab.ts
**Line:** 764:764
**Comment:**
*Possible Bug: The condition that checks for synchronous queries uses a
strict comparison `runAsync === false`, which will not match cases where
`runAsync` is `undefined` or `null` even though those are effectively non-async
runs, so such queries can incorrectly remain in the "success" state with no
results and still show the confusing UI that this change intends to fix.
Relaxing the check to treat any falsy `runAsync` as synchronous will make the
logic robust against missing or optional `runAsync` values from the backend.
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/reducers/sqlLab.ts:
##########
@@ -759,6 +759,13 @@ export default function sqlLabReducer(
? prevState
: currentState,
};
+ if (
+ newQueries[id].state === QueryState.Success &&
+ newQueries[id].runAsync === false &&
+ !newQueries[id].results
+ ) {
+ newQueries[id].state = QueryState.Fetching;
+ }
if (
shallowEqual(
omit(newQueries[id], ['extra']),
Review Comment:
**Suggestion:** In the refresh-queries handler, the equality optimization
dereferences `state.queries[id].extra` without checking if the query already
exists in state, but this block runs even when
`!state.queries.hasOwnProperty(id)` is true, so for new query IDs
`state.queries[id]` is `undefined` and accessing `.extra` will throw at runtime
when `refreshQueries` is called with a previously unseen ID. [null pointer]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Reducer throws during REFRESH_QUERIES processing.
- ❌ SQL Lab query list UI can fail to update.
- ⚠️ Polling/updates drop for affected query ids.
```
</details>
```suggestion
state.queries[id] &&
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In the running app, trigger the REFRESH_QUERIES reducer path by
dispatching the
actions.REFRESH_QUERIES action. The reducer handler is in
superset-frontend/src/SqlLab/reducers/sqlLab.ts inside the
[actions.REFRESH_QUERIES]()
block (the handler begins in the PR diff around lines 749-776).
2. Include an alteredQueries payload that contains a new query id not
present in the
current Redux state. Example payload: { alteredQueries: { "newId": { state:
"success", /*
other fields */ } } }. This causes the forEach branch guarded by the check
at the top of
the loop (the clause that starts with "!state.queries.hasOwnProperty(id)" in
the same
handler) to execute for that id.
3. Execution reaches the equality-optimization block starting at the if (
shallowEqual(...
) && isEqual(... ) ) { ... } portion (the block shown in the diff at ~lines
770-775).
Because state.queries["newId"] is undefined, the expression
state.queries[id].extra is
evaluated and raises a TypeError (cannot read property 'extra' of undefined).
4. Observe a runtime exception thrown from the reducer path in the browser
console and a
broken UI update for SQL Lab queries. The failure occurs deterministically
when
REFRESH_QUERIES contains query IDs not already present in state. (If the
reviewer
considers this a non-issue: the code's earlier condition explicitly allows
this branch for
new ids by design, so the missing guard is a real bug.)
```
</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/reducers/sqlLab.ts
**Line:** 771:771
**Comment:**
*Null Pointer: In the refresh-queries handler, the equality
optimization dereferences `state.queries[id].extra` without checking if the
query already exists in state, but this block runs even when
`!state.queries.hasOwnProperty(id)` is true, so for new query IDs
`state.queries[id]` is `undefined` and accessing `.extra` will throw at runtime
when `refreshQueries` is called with a previously unseen 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]
