codeant-ai-for-open-source[bot] commented on code in PR #41303:
URL: https://github.com/apache/superset/pull/41303#discussion_r3456334293
##########
superset-frontend/src/features/home/RightMenu.test.tsx:
##########
@@ -412,6 +416,10 @@ test('Logs out and clears local storage item redux', async
() => {
expect(localStorage.getItem('redux')).toBeNull();
expect(sessionStorage.getItem('login_attempted')).toBeNull();
});
+ // The namespaced Cache API store is purged on logout.
+ expect(deleteMock).toHaveBeenCalledWith('@SUPERSET-UI/CONNECTION');
+
+ delete (global as any).caches;
Review Comment:
**Suggestion:** This test mutates a global (`global.caches`) and only
restores it at the end of the happy path; if any assertion throws earlier,
cleanup is skipped and later tests inherit the mocked Cache API, causing
cross-test pollution and flaky failures. Move restoration into guaranteed
teardown (e.g., `afterEach`/`finally`) and restore any prior value instead of
unconditional delete. [possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Caching tests fail if logout test aborts early.
- ⚠️ Jest suite can show order-dependent, flaky failures.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open `superset-frontend/src/features/home/RightMenu.test.tsx` and locate
the test `Logs
out and clears local storage item redux` where `global.caches` is mocked at
line 406 and
only cleaned up with `delete (global as any).caches;` at line 422.
2. Cause an assertion in this test to fail before line 422 (for example, by
changing the
expectations inside the `waitFor` block at lines 415–418 so they no longer
match), then
run the Jest test suite so the test throws and aborts before executing the
`delete (global
as any).caches;` cleanup.
3. In the same Jest run, execute the cache-related tests in
`superset-frontend/packages/superset-ui-core/test/connection/callApi/callApi.test.ts`,
which rely on the global Cache API and call `await
caches.delete(constants.CACHE_KEY);` in
`beforeEach` at lines 10–13 and `await caches.open(constants.CACHE_KEY);` at
lines 20 and
33.
4. Observe that because `global.caches` was left as `{ delete: deleteMock }`
when the
RightMenu logout test failed, subsequent uses of `caches.open` in
`callApi.test.ts` now
throw `TypeError: caches.open is not a function`, causing unrelated tests to
fail
depending on execution order, demonstrating cross-test pollution from the
incomplete
global cleanup.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=ffc5208754814751bf7c3ab16b5b50cd&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=ffc5208754814751bf7c3ab16b5b50cd&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/features/home/RightMenu.test.tsx
**Line:** 406:422
**Comment:**
*Possible Bug: This test mutates a global (`global.caches`) and only
restores it at the end of the happy path; if any assertion throws earlier,
cleanup is skipped and later tests inherit the mocked Cache API, causing
cross-test pollution and flaky failures. Move restoration into guaranteed
teardown (e.g., `afterEach`/`finally`) and restore any prior value instead of
unconditional delete.
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%2F41303&comment_hash=7af9a4c5fc4a26b2407674b5dfd3f15c54085c66dd2da35d41d0e0489f71a807&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41303&comment_hash=7af9a4c5fc4a26b2407674b5dfd3f15c54085c66dd2da35d41d0e0489f71a807&reaction=dislike'>👎</a>
##########
superset-frontend/src/features/home/RightMenu.tsx:
##########
@@ -353,6 +354,14 @@ const RightMenu = ({
try {
window.localStorage.removeItem('redux');
window.sessionStorage.removeItem('login_attempted');
+ // Purge the namespaced Cache API store so cached GET responses are not
+ // retained on the device after the session ends. Best-effort: the
+ // returned promise is not awaited since logout navigates away.
+ if (typeof caches !== 'undefined') {
+ caches.delete(CACHE_KEY).catch(() => {
+ /* best-effort: ignore cache deletion failures */
+ });
+ }
Review Comment:
**Suggestion:** The cache purge is started asynchronously but not awaited,
while logout is an immediate link navigation. In real browsers, navigation can
terminate in-flight async work, so the Cache API store may remain undeleted and
the fix becomes unreliable. Make logout wait for the deletion to settle (or
delay navigation until purge attempt completes) so cleanup actually happens.
[cache]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Logout may leave @SUPERSET-UI/CONNECTION cache undeleted.
- ❌ New sessions can reuse prior ETagged GET responses.
- ⚠️ Risk of cross-user data exposure via cached responses.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In the Superset frontend, trigger a GET request that is cacheable via
`callApi` (for
example, a dashboard or dataset listing) so that
`packages/superset-ui-core/src/connection/callApi/callApi.ts` executes the
ETag cache path
at lines 4–43, opening `caches.open(CACHE_KEY)` and storing the response in
the
`@SUPERSET-UI/CONNECTION` Cache API store via `supersetCache.put(url,
response.clone())`.
2. While logged in as User A over HTTPS, confirm that the response is cached
by invoking
the same GET again and observing that `callApi` reads from Cache API and
uses the cached
ETag (lines 13–20 and 27–31 of `callApi.ts`).
3. Click the "Logout" menu item rendered by `RightMenu` in
`superset-frontend/src/features/home/RightMenu.tsx`, which is wired at lines
512–518 via
`<Typography.Link href={ensureAppRoot(navbarRight.user_logout_url)}>` with
`onClick:
handleLogout`; this invokes `handleLogout` at lines 34–49, where
`caches.delete(CACHE_KEY).catch(() => {})` is called without being awaited,
immediately
followed by browser navigation to the logout URL.
4. If the page navigates away and the document is torn down before the
asynchronous
`caches.delete(CACHE_KEY)` at lines 41–43 completes (e.g., on a slow device
or heavily
loaded browser), the `@SUPERSET-UI/CONNECTION` Cache API namespace is never
actually
deleted, so when a new session is established and `callApi` is used again,
it can still
see and reuse entries under `CACHE_KEY`, meaning cached GET responses from
the prior
session can persist across logout despite the intended purge.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=df2a464b863b4eeca8bdd8248cae548c&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=df2a464b863b4eeca8bdd8248cae548c&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/features/home/RightMenu.tsx
**Line:** 357:364
**Comment:**
*Cache: The cache purge is started asynchronously but not awaited,
while logout is an immediate link navigation. In real browsers, navigation can
terminate in-flight async work, so the Cache API store may remain undeleted and
the fix becomes unreliable. Make logout wait for the deletion to settle (or
delay navigation until purge attempt completes) so cleanup actually happens.
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%2F41303&comment_hash=735d8b056fd3524711fc634774d79ab4dc4d3436d17ab5d0e647238d6c7f1d45&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41303&comment_hash=735d8b056fd3524711fc634774d79ab4dc4d3436d17ab5d0e647238d6c7f1d45&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]
