codeant-ai-for-open-source[bot] commented on code in PR #38584:
URL: https://github.com/apache/superset/pull/38584#discussion_r3544911473
##########
superset-frontend/src/explore/components/SaveModal.test.tsx:
##########
@@ -266,6 +281,87 @@ test('renders a message when saving as with new
dashboard', () => {
);
});
+test('does not preselect an externally managed dashboard on mount', async ()
=> {
+ const dashboardId = 1;
+ fetchMock.removeRoutes();
+ fetchMock.get(fetchDashboardsEndpoint, mockDashboardData);
+ fetchMock.get(fetchChartEndpoint, { id: 1, dashboards: [1] });
+ fetchMock.get(`glob:*/api/v1/dashboard/${dashboardId}`, {
+ result: {
+ id: dashboardId,
+ dashboard_title: 'Managed Dashboard',
+ owners: [{ id: 1 }],
+ is_managed_externally: true,
+ },
+ });
+
+ const store = mockStore({
+ ...initialState,
+ explore: {
+ ...initialState.explore,
+ slice: {
+ ...initialState.explore.slice,
+ dashboards: [dashboardId],
+ },
+ },
+ });
+
+ const { queryByTestId } = setup(
+ {
+ ...defaultProps,
+ dashboardId,
+ },
+ store,
+ );
+
+ await waitFor(() => {
+ expect(
+ fetchMock.callHistory.calls(`glob:*/api/v1/dashboard/${dashboardId}`),
+ ).toHaveLength(1);
+ });
+
+ const selectInput = queryByTestId('mock-async-select') as HTMLInputElement;
+ expect(selectInput).toBeInTheDocument();
+ expect(
+ fetchMock.callHistory.calls(`glob:*/api/v1/dashboard/${dashboardId}/tabs`),
+ ).toHaveLength(0);
+
+ // Restore default mocks
+ fetchMock.removeRoutes();
+ fetchMock.clearHistory();
+ fetchMock.get(fetchDashboardsEndpoint, mockDashboardData);
+ fetchMock.get(fetchChartEndpoint, { id: 1, dashboards: [1] });
+ fetchMock.get(fetchDashboardEndpoint, {
+ result: [{ id: 'id', dashboard_title: 'dashboard title' }],
+ });
Review Comment:
**Suggestion:** Route cleanup/restoration is done at the end of the test
body instead of guaranteed teardown, so any assertion failure before this block
will leak modified `fetchMock` routes into later tests and create
order-dependent flakiness. Move this cleanup into `afterEach` (or
`try/finally`) so it always executes. [code quality]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Externally managed dashboard tests leak fetchMock routes on failure.
- ⚠️ Later SaveModal tests become flaky and order-dependent.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In `superset-frontend/src/explore/components/SaveModal.test.tsx`, observe
the global
`fetchMock` setup in `beforeAll` at lines 148–154 and the test `test('does
not preselect
an externally managed dashboard on mount', ...)` at lines 284–337, which
reconfigures
routes by calling `fetchMock.removeRoutes()` at line 286 and adding
specialized endpoints
at lines 287–296.
2. Within that test, note the inline cleanup block starting at line 329
labeled `//
Restore default mocks`, which calls `fetchMock.removeRoutes()` and
`fetchMock.clearHistory()` at lines 330–331 and then re-registers the
default endpoints
using `fetchDashboardsEndpoint`, `fetchChartEndpoint`, and
`fetchDashboardEndpoint` at
lines 332–335.
3. If any assertion in this test fails or throws before reaching the cleanup
block—for
example, the expectations on
`fetchMock.callHistory.calls('glob:*/api/v1/dashboard/${dashboardId}')` at
lines 317–321
or the expectation on the tabs endpoint at lines 323–327—Jest aborts the
test and the
cleanup at lines 329–336 is never executed, leaving the modified `fetchMock`
routes and
removed defaults in place.
4. Subsequent tests in the same file, such as `test('updates slice name and
selected
dashboard', ...)` at lines 50–95 or `test('loadDashboards includes
is_managed_externally
filter', ...)` at lines 339–363, rely on the original global `fetchMock`
routes but now
see the leaked configuration from the earlier test, causing later tests to
fail or pass
unpredictably depending on execution order; moving the restoration logic
into `afterEach`
or wrapping the route reconfiguration in `try/finally` would guarantee
cleanup even on
assertion failures.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=162a68e8c8564f4ba4ac0e0386a1d460&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=162a68e8c8564f4ba4ac0e0386a1d460&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/explore/components/SaveModal.test.tsx
**Line:** 329:336
**Comment:**
*Code Quality: Route cleanup/restoration is done at the end of the test
body instead of guaranteed teardown, so any assertion failure before this block
will leak modified `fetchMock` routes into later tests and create
order-dependent flakiness. Move this cleanup into `afterEach` (or
`try/finally`) so it always executes.
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.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38584&comment_hash=aafb424a5f1b9b4162700b0d259759ab3be5be45ba3b2f5b8070bca655653053&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38584&comment_hash=aafb424a5f1b9b4162700b0d259759ab3be5be45ba3b2f5b8070bca655653053&reaction=dislike'>👎</a>
##########
superset-frontend/src/explore/components/SaveModal.test.tsx:
##########
@@ -36,14 +36,29 @@ import SaveModal, {
import { CHART_WIDTH } from 'src/dashboard/constants';
import { GRID_COLUMN_COUNT } from 'src/dashboard/util/constants';
+// Captures the AsyncSelect `options` loader (SaveModal's loadDashboards) so
+// tests can invoke it directly and assert on the request it issues.
+let mockLoadDashboards:
+ | ((search: string, page: number, pageSize: number) => Promise<unknown>)
+ | undefined;
Review Comment:
**Suggestion:** The shared `mockLoadDashboards` function reference is never
reset between tests, so this test can accidentally pass by using a stale loader
captured in an earlier test run. Reset it in `beforeEach`/`afterEach` to avoid
false positives and stale-call behavior. [stale reference]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ SaveModal dashboard filter test can pass with stale loader.
- ⚠️ Regressions in is_managed_externally filter may go undetected.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open `superset-frontend/src/explore/components/SaveModal.test.tsx` and
note the global
`mockLoadDashboards` declaration at lines 41–43 and the `AsyncSelect` Jest
mock at lines
45–62 that assigns `mockLoadDashboards = options` when the component renders.
2. Run an earlier test that renders `SaveModal` with the mocked
`AsyncSelect`, for example
`test('renders a message when a new dashboard is selected', ...)` at lines
250–259, which
calls `setup()` and causes `mockLoadDashboards` to be set to that test run's
loader.
3. Then run `test('loadDashboards includes is_managed_externally filter',
...)` at lines
339–363, which again calls `setup()` at line 354 but never resets
`mockLoadDashboards` and
only waits for `mockLoadDashboards` to be defined at line 356 before
invoking it at line
357.
4. If a regression in `SaveModal` prevents the current `AsyncSelect` from
mounting or
changes its `options` loader, the test can still pass by using the stale
`mockLoadDashboards` captured from the previous test, masking the fact that
the current
dashboard loader no longer includes the `is_managed_externally` filter in
its request URL.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=03bd1539a2ee423789c71319bfebca97&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=03bd1539a2ee423789c71319bfebca97&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/explore/components/SaveModal.test.tsx
**Line:** 41:43
**Comment:**
*Stale Reference: The shared `mockLoadDashboards` function reference is
never reset between tests, so this test can accidentally pass by using a stale
loader captured in an earlier test run. Reset it in `beforeEach`/`afterEach` to
avoid false positives and stale-call behavior.
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.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38584&comment_hash=a96ed73c5f442dd4bb2a9687c3a7582c25cbe1fc1ad7acdf076b1befa534a908&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38584&comment_hash=a96ed73c5f442dd4bb2a9687c3a7582c25cbe1fc1ad7acdf076b1befa534a908&reaction=dislike'>👎</a>
--
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]
