rusackas commented on code in PR #45589:
URL: https://github.com/apache/superset/pull/45589#discussion_r4356616326
##########
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:
Good catch. `refreshKey` is now passed to `ActivityTable` and added to the
Edited effect deps, so the list refetches after a delete.
##########
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:
Agreed. Initial tab selection now only runs on the first load (`refreshKey
=== 0`); later refreshes just update `activityData`.
##########
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:
Reworked: the delete promise is deferred, and the test asserts neither
`refreshData` nor `onActivityRefresh` fires before it resolves, then exactly
one call after. The Other-tab test now asserts `onActivityRefresh` on the
`getData` path too.
##########
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);
Review Comment:
Thanks @SBIN2010, the collapse-state restore is now limited to the initial
load (`refreshKey === 0`), so a refresh no longer reopens collapsed panels.
--
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]
