codeant-ai-for-open-source[bot] commented on code in PR #36359:
URL: https://github.com/apache/superset/pull/36359#discussion_r2585327193
##########
superset-frontend/src/SqlLab/components/QueryHistory/QueryHistory.test.tsx:
##########
@@ -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);
Review Comment:
**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 🚨
```suggestion
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);
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
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.
</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/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.
```
</details>
##########
superset-frontend/src/SqlLab/components/QueryHistory/QueryHistory.test.tsx:
##########
@@ -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);
+
+ // 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();
Review Comment:
**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 🚨
```suggestion
mockedIsFeatureEnabled.mockReset();
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
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.
</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/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.
```
</details>
##########
superset-frontend/src/SqlLab/components/QueryHistory/index.tsx:
##########
@@ -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;
Review Comment:
**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 ⚠️
```suggestion
return bTime - aTime;
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
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.
</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/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.
```
</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]
