sadpandajoe commented on code in PR #45589:
URL: https://github.com/apache/superset/pull/45589#discussion_r4352039767
##########
superset-frontend/src/pages/Home/index.tsx:
##########
@@ -257,44 +268,56 @@ function Welcome({ user, addDangerToast }: WelcomeProps) {
Promise.all([
getUserEditableObjects(id, 'dashboard')
.then(r => {
- setDashboardData(r);
+ if (!isStale) setDashboardData(r);
return Promise.resolve();
})
.catch((err: unknown) => {
- setDashboardData([]);
- addDangerToast(
- t('There was an issue fetching your dashboards: %s', err),
- );
+ if (!isStale) {
+ setDashboardData([]);
+ addDangerToast(
+ t('There was an issue fetching your dashboards: %s', err),
+ );
+ }
return Promise.resolve();
}),
getUserEditableObjects(id, 'chart')
.then(r => {
- setChartData(r);
+ if (!isStale) setChartData(r);
return Promise.resolve();
})
.catch((err: unknown) => {
- setChartData([]);
- addDangerToast(t('There was an issue fetching your chart: %s', err));
+ if (!isStale) {
+ setChartData([]);
+ addDangerToast(
+ t('There was an issue fetching your chart: %s', err),
+ );
+ }
return Promise.resolve();
}),
canReadSavedQueries
? getUserEditableObjects(id, 'saved_query', ownSavedQueryFilters)
.then(r => {
- setQueryData(r);
+ if (!isStale) setQueryData(r);
return Promise.resolve();
})
.catch((err: unknown) => {
- setQueryData([]);
- addDangerToast(
- t('There was an issue fetching your saved queries: %s', err),
- );
+ if (!isStale) {
+ setQueryData([]);
+ addDangerToast(
+ t('There was an issue fetching your saved queries: %s', err),
+ );
+ }
return Promise.resolve();
})
: Promise.resolve(),
]).then(() => {
- setIsFetchingActivityData(false);
+ if (!isStale) setIsFetchingActivityData(false);
});
- }, [otherTabFilters]);
+
+ return () => {
+ isStale = true;
+ };
+ }, [otherTabFilters, refreshKey]);
Review Comment:
This re-runs Home's activity fetches, but the Edited list in `ActivityTable`
keeps its own `editedCards` state whose effect only depends on `[activeChild,
user.userId]`. If someone is on Recents → Edited and deletes a dashboard or
chart from the section below, the deleted item stays listed with a link to a
resource that no longer exists until they switch tabs or reload. Should
`refreshKey` be passed down so that effect refetches too?
##########
superset-frontend/src/features/home/DashboardTable.test.tsx:
##########
@@ -377,6 +379,11 @@ test('handles dashboard deletion confirmation', async ()
=> {
toggleBulkSelect: jest.fn(),
}));
+ // onActivityRefresh must not fire before the delete-triggered refresh
+ // actually runs, since it's wired to follow refreshData, not the
+ // fire-and-forget delete call itself.
+ expect(onActivityRefresh).not.toHaveBeenCalled();
Review Comment:
This `not.toHaveBeenCalled()` runs before the component is rendered, so it
only checks a fresh `jest.fn()`, and the mocked `handleDashboardDelete` calls
`refreshData` synchronously. A regression that fires `onActivityRefresh`
straight from the confirm handler, before the DELETE succeeds, would still pass
despite the comment above it. Could the test defer the delete promise, assert
the callback hasn't fired after confirming, then assert exactly one call once
it resolves (and none on failure)? The Other-tab `handleGetData` path also has
no `onActivityRefresh` assertion in the existing Other-tab test.
##########
superset-frontend/src/pages/Home/index.tsx:
##########
@@ -214,12 +219,17 @@ function Welcome({ user, addDangerToast }: WelcomeProps) {
useEffect(() => {
if (!otherTabFilters || WelcomeMainExtension) {
- return;
+ return undefined;
}
+ // Guards against out-of-order responses: if `refreshKey` changes again
+ // (e.g. a rapid follow-up delete) before this run's requests resolve,
+ // its results are discarded instead of overwriting newer state.
+ let isStale = false;
const activeTab = getItem(LocalStorageKeys.HomepageActivityFilter, null);
setActiveState(collapseState.length > 0 ? collapseState : DEFAULT_TAB_ARR);
getRecentActivityObjs(user.userId!, recent, addDangerToast,
otherTabFilters)
.then(res => {
+ if (isStale) return;
Review Comment:
Each refresh run snapshots `activeTab` from storage at the start (line 228)
and then calls `setActiveChild` with that snapshot when its requests finish. If
someone deletes a chart while Recents shows Viewed and then clicks Created
before the refresh resolves, the response switches them back to Viewed;
`isStale` doesn't help because changing the tab doesn't re-run this effect.
Could the initial tab selection be limited to the first load (or skipped when
`refreshKey` changed) so a background refresh only updates the data?
--
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]
