Skip to content

fix(sqllab): Skip progress bar on no data #37652

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
justinpark merged 1 commit into from
Feb 5, 2026
+81 −16
Merged

fix(sqllab): Skip progress bar on no data #37652

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 34 additions & 0 deletions superset-frontend/src/SqlLab/components/SouthPane/Results.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,11 @@ import { denormalizeTimestamp } from '@superset-ui/core';
import { LOCALSTORAGE_MAX_QUERY_AGE_MS } from 'src/SqlLab/constants';
import Results from './Results';

jest.mock('@superset-ui/core', () => ({
...jest.requireActual('@superset-ui/core'),
isFeatureEnabled: (flag: string) => flag === 'SQLLAB_BACKEND_PERSISTENCE',
}));

const mockedProps = {
queryEditorId: defaultQueryEditor.id,
latestQueryId: 'LCly_kkIN',
Expand All @@ -43,6 +48,11 @@ const mockedExpiredProps = {
latestQueryId: 'expired_query_id',
};

const mockedNoStoredResultsProps = {
...mockedEmptyProps,
latestQueryId: 'no_stored_results_query_id',
};

const latestQueryProgressMsg = 'LATEST QUERY MESSAGE - LCly_kkIN';
const expireDateTime = Date.now() - LOCALSTORAGE_MAX_QUERY_AGE_MS - 1;

Expand Down Expand Up @@ -103,6 +113,19 @@ const mockState = {
sqlEditorId: defaultQueryEditor.id,
sql: 'select * from table4',
},
no_stored_results_query_id: {
cached: false,
changed_on: denormalizeTimestamp(new Date().toISOString()),
db: 'main',
dbId: 1,
id: 'no_stored_results_query_id',
startDttm: Date.now(),
sqlEditorId: defaultQueryEditor.id,
sql: 'select * from table5',
state: 'success',
resultsKey: null,
results: null,
},
},
},
};
Expand Down Expand Up @@ -132,3 +155,14 @@ test('should pass latest query down to ResultSet component', async () => {
});
expect(getByText(latestQueryProgressMsg)).toBeVisible();
});

test('Renders alert when query succeeded but has no stored results', async () => {
const { getByText } = render(<Results {...mockedNoStoredResultsProps} />, {
useRedux: true,
initialState: mockState,
});
const alertText = getByText(
/no stored results found, you need to re-run your query/i,
);
expect(alertText).toBeVisible();
});
34 changes: 18 additions & 16 deletions superset-frontend/src/SqlLab/components/SouthPane/Results.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,25 +83,27 @@ const Results: FC<Props> = ({
!latestQuery.resultsKey &&
!latestQuery.results;

if (hasNoStoredResults) {
return (
<Alert
type="info"
message={t('No stored results found, you need to re-run your query')}
/>
);
}

return (
<>
<QueryStatusBar key={latestQueryId} query={latestQuery} />
{hasNoStoredResults ? (
<Alert
type="info"
message={t('No stored results found, you need to re-run your query')}
/>
) : (
<ResultSet
search
queryId={latestQuery.id}
database={databases[latestQuery.dbId]}
displayLimit={displayLimit}
defaultQueryLimit={defaultQueryLimit}
showSql
showSqlInline
/>
)}
<ResultSet
search
queryId={latestQuery.id}
database={databases[latestQuery.dbId]}
displayLimit={displayLimit}
defaultQueryLimit={defaultQueryLimit}
showSql
showSqlInline
/>
</>
);
};
Expand Down
22 changes: 22 additions & 0 deletions superset-frontend/src/SqlLab/reducers/sqlLab.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -572,6 +572,28 @@ describe('sqlLabReducer', () => {
test('should refresh queries when polling returns empty', () => {
newState = sqlLabReducer(newState, actions.refreshQueries({}));
});
test('should set state to fetching when sync query succeeds without results', () => {
const syncQuery = {
...query,
id: 'sync-query',
state: QueryState.Pending,
runAsync: false,
results: null,
};
newState = sqlLabReducer(
{
...newState,
queries: { 'sync-query': syncQuery },
},
actions.refreshQueries({
'sync-query': {
...syncQuery,
state: QueryState.Success,
},
}),
);
expect(newState.queries['sync-query'].state).toBe(QueryState.Fetching);
});
});
// eslint-disable-next-line no-restricted-globals -- TODO: Migrate from describe blocks
describe('CLEAR_INACTIVE_QUERIES', () => {
Expand Down
7 changes: 7 additions & 0 deletions superset-frontend/src/SqlLab/reducers/sqlLab.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -759,6 +759,13 @@ export default function sqlLabReducer(
? prevState
: currentState,
};
if (
newQueries[id].state === QueryState.Success &&
newQueries[id].runAsync === false &&
Copy link
Contributor

@codeant-ai-for-open-source codeant-ai-for-open-source bot Feb 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

!newQueries[id].results
) {
newQueries[id].state = QueryState.Fetching;
}
if (
shallowEqual(
omit(newQueries[id], ['extra']),
Copy link
Contributor

@codeant-ai-for-open-source codeant-ai-for-open-source bot Feb 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Expand Down
Loading