Skip to content

fix(SQLLab): most recent queries at the top in Query History without refreshing the page #36359

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
EnxDev merged 3 commits into from
Dec 3, 2025
Merged

fix(SQLLab): most recent queries at the top in Query History without refreshing the page #36359

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b71700e
Add sorting
FelipeGLopez Dec 1, 2025
56a6936
Add tests
FelipeGLopez Dec 1, 2025
e1058b5
Fix linting issues
FelipeGLopez Dec 3, 2025
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
109 changes: 109 additions & 0 deletions superset-frontend/src/SqlLab/components/QueryHistory/QueryHistory.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,3 +133,112 @@ test('fetches the query history by the tabViewId', async () => {
expect(queryResultText).toBeInTheDocument();
isFeatureEnabledMock.mockClear();
});

test('displays multiple queries with newest query first', async () => {
const isFeatureEnabledMock = mockedIsFeatureEnabled.mockImplementation(
featureFlag => featureFlag === FeatureFlag.SqllabBackendPersistence,
);

const multipleQueriesApiResult = {
count: 2,
ids: [694, 693],
result: [
{
changed_on: '2024-03-12T20:10:02.497775',
client_id: 'd2ZDzRYzn',
database: {
database_name: 'examples',
id: 1,
},
end_time: '1710274202496.047852',
error_message: null,
executed_sql: 'SELECT COUNT(*) from "FCC 2018 Survey"\nLIMIT 1001',
id: 694,
limit: 1000,
limiting_factor: 'DROPDOWN',
progress: 100,
results_key: null,
rows: 1,
schema: 'main',
select_as_cta: false,
sql: 'SELECT COUNT(*) from "FCC 2018 Survey"',
sql_editor_id: '22',
start_time: '1710274202445.992920',
status: QueryState.Success,
tab_name: 'Untitled Query 2',
tmp_table_name: null,
tracking_url: null,
user: {
first_name: 'admin',
id: 1,
last_name: 'user',
},
},
{
changed_on: '2024-03-12T20:01:02.497775',
client_id: 'b0ZDzRYzn',
database: {
database_name: 'examples',
id: 1,
},
end_time: '1710273662496.047852',
error_message: null,
executed_sql: 'SELECT * from "FCC 2018 Survey"\nLIMIT 1001',
id: 693,
limit: 1000,
limiting_factor: 'DROPDOWN',
progress: 100,
results_key: null,
rows: 443,
schema: 'main',
select_as_cta: false,
sql: 'SELECT * from "FCC 2018 Survey"',
sql_editor_id: '22',
start_time: '1710273662445.992920',
status: QueryState.Success,
tab_name: 'Untitled Query 1',
tmp_table_name: null,
tracking_url: null,
user: {
first_name: 'admin',
id: 1,
last_name: 'user',
},
},
],
};

const editorQueryApiRoute = `glob:*/api/v1/query/?q=*`;
fetchMock.get(editorQueryApiRoute, multipleQueriesApiResult);
const { container } = render(setup(), { useRedux: true, initialState });

await waitFor(() =>
expect(fetchMock.calls(editorQueryApiRoute).length).toBe(1),
);

expect(screen.getByTestId('listview-table')).toBeVisible();
expect(screen.getByRole('table')).toBeVisible();

const tableRows = container.querySelectorAll(
'table > tbody > tr:not(.ant-table-measure-row)',
);
expect(tableRows).toHaveLength(2);

// Check that both queries are present
const olderQueryRow = screen.getByText('443');
const newerQueryElements = screen.getAllByText('1');
expect(olderQueryRow).toBeInTheDocument();
expect(newerQueryElements.length).toBeGreaterThan(0);
Comment on lines +229 to +231

@codeant-ai-for-open-source codeant-ai-for-open-source Bot Dec 3, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggestion: Using screen.getAllByText('1') is fragile and can match other '1' occurrences in the DOM (headers, timestamps, ids), causing flakiness; instead, explicitly search the table rows (which you already query) to assert that at least one data row contains the expected '1' value. [possible bug]

Severity Level: Critical 🚨

Suggested change
const newerQueryElements = screen.getAllByText('1');
expect(olderQueryRow).toBeInTheDocument();
expect(newerQueryElements.length).toBeGreaterThan(0);
const hasNewerQuery = Array.from(
container.querySelectorAll('table > tbody > tr:not(.ant-table-measure-row)'),
).some(row => row.textContent?.includes('1'));
expect(olderQueryRow).toBeInTheDocument();
expect(hasNewerQuery).toBe(true);
Why it matters? ⭐

This is a useful and practical improvement. Using getAllByText('1') is brittle because small numeric strings often appear elsewhere (headers, timestamps). Scoping the assertion to the queried table rows (which the test already selects) makes the check more robust and directly verifies the rendered data. The improved_code correctly uses the rendered container to assert presence within the table rows.

Prompt for AI Agent 🤖 
This is a comment left during a code review.

**Path:** superset-frontend/src/SqlLab/components/QueryHistory/QueryHistory.test.tsx
**Line:** 229:231
**Comment:**
	*Possible Bug: Using `screen.getAllByText('1')` is fragile and can match other '1' occurrences in the DOM (headers, timestamps, ids), causing flakiness; instead, explicitly search the table rows (which you already query) to assert that at least one data row contains the expected '1' value.

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.


// Verify ordering: newer query (1 row) should appear before older query (443 rows)
// Find the actual row elements to check their order
const firstDataRow = tableRows[0];
const secondDataRow = tableRows[1];

// The newer query should be in the first row (has 1 result row)
expect(firstDataRow).toHaveTextContent('1');
// The older query should be in the second row (has 443 result rows)
expect(secondDataRow).toHaveTextContent('443');

isFeatureEnabledMock.mockClear();

@codeant-ai-for-open-source codeant-ai-for-open-source Bot Dec 3, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggestion: Using mockClear() only clears call history but does not remove the mock implementation; this can leave isFeatureEnabled returning the mocked value in subsequent tests and cause test leakage. Replace the single-line cleanup with a call that resets the mock implementation (for example mockReset() on the original mock) so the mock state is fully cleared between tests. [possible bug]

Severity Level: Critical 🚨

Suggested change
isFeatureEnabledMock.mockClear();
mockedIsFeatureEnabled.mockReset();
Why it matters? ⭐

This suggestion is valid: mockClear() only clears call history and keeps the mock implementation, which can cause test leakage if any test relies on the default/unmocked behavior. Using mockReset() (or mockRestore()/jest.resetAllMocks() in a shared afterEach) will clear both history and implementation making tests more isolated. The suggested improved_code (calling mockReset on the underlying jest mock) is appropriate.

Prompt for AI Agent 🤖 
This is a comment left during a code review.

**Path:** superset-frontend/src/SqlLab/components/QueryHistory/QueryHistory.test.tsx
**Line:** 243:243
**Comment:**
	*Possible Bug: Using `mockClear()` only clears call history but does not remove the mock implementation; this can leave `isFeatureEnabled` returning the mocked value in subsequent tests and cause test leakage. Replace the single-line cleanup with a call that resets the mock implementation (for example `mockReset()` on the original mock) so the mock state is fully cleared between tests.

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.

});
6 changes: 5 additions & 1 deletion superset-frontend/src/SqlLab/components/QueryHistory/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,11 @@ const QueryHistory = ({
editorId,
)
.concat(data.result)
.reverse()
.sort((a, b) => {
const aTime = a.startDttm || 0;
const bTime = b.startDttm || 0;
return aTime - bTime;

@codeant-ai-for-open-source codeant-ai-for-open-source Bot Dec 3, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggestion: Sorting direction is reversed: the comparator returns aTime - bTime which orders oldest-first; to show most recent queries at the top the comparator must return bTime - aTime (descending order). [logic error]

Severity Level: Minor ⚠️

Suggested change
return aTime - bTime;
return bTime - aTime;
Why it matters? ⭐

The current comparator orders oldest-first (ascending). For a query history UI you almost always want most-recent-first. Reversing the subtraction to bTime - aTime fixes the ordering logic. This is a direct logic correction to the visible bug in this hunk and doesn't require broader changes.

Prompt for AI Agent 🤖 
This is a comment left during a code review.

**Path:** superset-frontend/src/SqlLab/components/QueryHistory/index.tsx
**Line:** 97:97
**Comment:**
	*Logic Error: Sorting direction is reversed: the comparator returns aTime - bTime which orders oldest-first; to show most recent queries at the top the comparator must return bTime - aTime (descending order).

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.

})
: getEditorQueries(queries, editorId),
[queries, data, editorId],
);
Expand Down
Loading