fix(sqllab): Skip progress bar on no data #37652
Open
+81
−16
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Code Review Agent Run #e4189cActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| }; | ||
| if ( | ||
| newQueries[id].state === QueryState.Success && | ||
| newQueries[id].runAsync === false && |
Contributor
There was a problem hiding this 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]
Severity Level: Major ⚠️
- ⚠️ Query status UI shows success with no results.
- ⚠️ Progress bar and no-data state may overlap.
- ⚠️ Backend omission of runAsync causes visual bug.
Suggested change
| newQueries[id].runAsync === false && | |
| !newQueries[id].runAsync && |
Steps of Reproduction ✅
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.Prompt for AI Agent 🤖
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.| } | ||
| if ( | ||
| shallowEqual( | ||
| omit(newQueries[id], ['extra']), |
Contributor
There was a problem hiding this 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]
Severity Level: Critical 🚨
- ❌ Reducer throws during REFRESH_QUERIES processing.
- ❌ SQL Lab query list UI can fail to update.
- ⚠️ Polling/updates drop for affected query ids.
Suggested change
| omit(newQueries[id], ['extra']), | |
| state.queries[id] && |
Steps of Reproduction ✅
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.)Prompt for AI Agent 🤖
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.
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
michael-s-molina
approved these changes
Feb 4, 2026
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
SUMMARY
There is an issue where the query progress bar and the "no data" state are displayed together, which can cause confusion. This occurs because, in sync mode, when a query is executed, the batch query/updated_since is triggered at the moment the query results are being parsed. As a result, the query state changes to "success" before the results are actually populated.
This commit modifies the process so that, just like in async mode, the state changes to "fetching" during this period. This prevents misunderstanding by displaying the correct status until the results are available.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before:
After:
TESTING INSTRUCTIONS
specs are added
ADDITIONAL INFORMATION