codeant-ai-for-open-source[bot] commented on code in PR #39528:
URL: https://github.com/apache/superset/pull/39528#discussion_r3503699421
##########
superset-frontend/src/features/home/DashboardTable.tsx:
##########
@@ -248,6 +249,7 @@ function DashboardTable({
getData,
);
setDashboardToDelete(null);
+ onActivityRefresh?.();
Review Comment:
**Suggestion:** `onActivityRefresh` is invoked immediately after starting
deletion, not after deletion succeeds. Because `handleDashboardDelete` is
async, the recents fetch can run before the backend delete/log update finishes,
so the UI can still show stale activity and miss the intended refresh. Trigger
the activity refresh only in the delete success path (or await the returned
promise before calling it). [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Recents section may still show deleted dashboards.
- ⚠️ Recent activity inconsistent with dashboards list after delete.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open the Home page, which renders the `Welcome` component in
`superset-frontend/src/pages/Home/index.tsx` (function `Welcome`, lines
30–80 in that file
snippet) and, for the Dashboards tab, renders `DashboardTable` with
`onActivityRefresh={handleRefreshActivity}` (lines 389–403 in the PR hunk).
2. In `Welcome` (same file), observe `handleRefreshActivity` at lines
181–184: it calls
`setRefreshKey(prev => prev + 1)`, and `refreshKey` is a state dependency
for the
activity-loading `useEffect` (lines 97–179), which calls
`getRecentActivityObjs` and
`getUserOwnedObjects` to populate `activityData`, `dashboardData`, and
related state.
3. In `DashboardTable`
(`superset-frontend/src/features/home/DashboardTable.tsx`), observe
the `DeleteModal` `onConfirm` handler at lines 241–252: it calls
`handleDashboardDelete(dashboardToDelete, refreshData, addSuccessToast,
addDangerToast,
activeTab, user?.userId, getData);` and immediately after that calls
`onActivityRefresh?.();` before setting `dashboardToDelete` to null.
4. In `superset-frontend/src/views/CRUD/utils.tsx`, see
`handleDashboardDelete` defined at
line 362 (per Grep) and shown in the snippet starting at `export function
handleDashboardDelete` (lines 23–65 of the Read output): it returns
`SupersetClient.delete({ endpoint: /api/v1/dashboard/${id} }).then(...)`,
and only inside
the `.then` success callback does it call `refreshData`/`getData` and
`addSuccessToast`,
meaning the delete is asynchronous and `handleDashboardDelete` returns a
Promise.
5. Trigger a real delete by clicking delete on a dashboard card (the
`onDelete` prop in
`DashboardTable` at lines 269–277 sets `dashboardToDelete`, opening the
`DeleteModal`),
then confirm: the UI path runs `handleDashboardDelete` (starting the async
DELETE) and
immediately triggers `handleRefreshActivity` via `onActivityRefresh?.()`,
causing the
`useEffect` in `Welcome` to fetch recent activity (`getRecentActivityObjs`
and
`getUserOwnedObjects`) potentially before the backend DELETE/logging
completes; if the
recents GET completes first, `activityData` and `dashboardData` are updated
with stale
data that still includes the soon-to-be-deleted dashboard, leaving the
Recents/Created
sections inconsistent until a later refresh.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=790ade41abf840eb9b6ab583cbc118e8&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=790ade41abf840eb9b6ab583cbc118e8&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/DashboardTable.tsx
**Line:** 252:252
**Comment:**
*Logic Error: `onActivityRefresh` is invoked immediately after starting
deletion, not after deletion succeeds. Because `handleDashboardDelete` is
async, the recents fetch can run before the backend delete/log update finishes,
so the UI can still show stale activity and miss the intended refresh. Trigger
the activity refresh only in the delete success path (or await the returned
promise before calling it).
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%2F39528&comment_hash=fbfd169805428f588fee1e3fce3549897792b1d50f9bc6e65d5eac5ec2c25933&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39528&comment_hash=fbfd169805428f588fee1e3fce3549897792b1d50f9bc6e65d5eac5ec2c25933&reaction=dislike'>👎</a>
##########
superset-frontend/src/pages/Home/index.tsx:
##########
@@ -290,7 +295,7 @@ function Welcome({ user, addDangerToast }: WelcomeProps) {
]).then(() => {
setIsFetchingActivityData(false);
});
- }, [otherTabFilters]);
+ }, [otherTabFilters, refreshKey]);
Review Comment:
**Suggestion:** Adding `refreshKey` as a fetch trigger introduces
overlapping effect runs when users perform rapid actions, but this effect has
no cancellation or request-order guard. Slower earlier responses can overwrite
newer state and reintroduce stale recent activity/cards. Add request
cancellation or a monotonically increasing request token check before
committing state updates. [race condition]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Older refresh responses can overwrite newer activity state.
- ⚠️ Recents and Created sections intermittently show outdated data.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In `Welcome` (`superset-frontend/src/pages/Home/index.tsx`), observe the
`useEffect` at
lines 97–179 that loads recent activity and user-owned objects: it calls
`getRecentActivityObjs(user.userId!, recent, addDangerToast,
otherTabFilters)` and then
`Promise.all` of three `getUserOwnedObjects` calls to populate
`dashboardData`,
`chartData`, and `queryData`, finally calling
`setIsFetchingActivityData(false)`; this
effect depends on `[otherTabFilters, refreshKey]` (line 298 in the PR hunk).
2. Just above, see `const [refreshKey, setRefreshKey] = useState(0);` and
`const
handleRefreshActivity = () => { setRefreshKey(prev => prev + 1); };` (lines
52–64), and
note that `handleRefreshActivity` is passed down as `onActivityRefresh` to
both
`DashboardTable` and `ChartTable` in the Collapse items for `'dashboards'`
and `'charts'`
(lines 389–403 and 407–421 respectively).
3. In `ChartTable` (`superset-frontend/src/features/home/ChartTable.tsx`),
observe
`handleRefreshData` at lines 99–102: it calls `refreshData(config);` and
then immediately
calls `onActivityRefresh?.();`, meaning any chart action that triggers
`refreshData` (e.g.
delete via `ChartCard` using this `refreshData` prop at line 240) also
increments
`refreshKey` and re-runs the `Welcome` effect; similar paths can be added
for dashboards
via `DashboardTable`’s `onActivityRefresh`.
4. Because each increment of `refreshKey` (from rapid user actions like
deleting or
updating multiple charts/dashboards) kicks off a new asynchronous chain of
`SupersetClient.get` calls in `getRecentActivityObjs` and
`getUserOwnedObjects`
(`superset-frontend/src/views/CRUD/utils.tsx` lines 19–22 and 53–72), and
the `useEffect`
does not use any cancellation or request-order guard, it is possible for an
earlier
refresh (with an older `refreshKey`) to complete after a later one and invoke
`setActivityData`, `setDashboardData`, `setChartData`, and `setQueryData`,
thereby
overwriting state from the newer refresh and leaving Recents and Created
cards showing
stale data that no longer matches the latest deletes/updates.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=78c0c6d2ce3742c88bbe75dfdd29ae69&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=78c0c6d2ce3742c88bbe75dfdd29ae69&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/pages/Home/index.tsx
**Line:** 298:298
**Comment:**
*Race Condition: Adding `refreshKey` as a fetch trigger introduces
overlapping effect runs when users perform rapid actions, but this effect has
no cancellation or request-order guard. Slower earlier responses can overwrite
newer state and reintroduce stale recent activity/cards. Add request
cancellation or a monotonically increasing request token check before
committing state updates.
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%2F39528&comment_hash=c76dfbab72520366b664439112bfc0f812f80ad2e9e562a9f0faa9de15fced38&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39528&comment_hash=c76dfbab72520366b664439112bfc0f812f80ad2e9e562a9f0faa9de15fced38&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]
