Copilot commented on code in PR #38156:
URL: https://github.com/apache/superset/pull/38156#discussion_r2836878547
##########
superset-frontend/src/dashboard/actions/dashboardState.ts:
##########
@@ -190,13 +206,21 @@ export function saveFaveStar(id: number, isStarred:
boolean) {
return apiCall
.then(() => {
- dispatch(toggleFaveStar(!isStarred));
+ // Only update state if this is still the current dashboard
+ const currentId = getState().dashboardInfo?.id;
+ if (currentId === id) {
+ dispatch(toggleFaveStar(!isStarred));
+ }
})
- .catch(() =>
- dispatch(
- addDangerToast(t('There was an issue favoriting this dashboard.')),
- ),
- );
+ .catch(() => {
+ // Only show error if this is still the current dashboard
+ const currentId = getState().dashboardInfo?.id;
+ if (currentId === id) {
+ dispatch(
+ addDangerToast(t('There was an issue favoriting this dashboard.')),
+ );
+ }
+ });
};
}
Review Comment:
The race condition fix in `saveFaveStar` should be covered by tests to
ensure the stale response handling works correctly. The test should verify that
when the dashboard ID changes between the API call and the response, no state
updates or error toasts are dispatched. This is a critical behavior change that
prevents user-facing bugs.
Consider adding test cases that:
1. Verify actions are dispatched when dashboard ID matches
2. Verify actions are NOT dispatched when dashboard ID changes (the race
condition scenario)
3. Cover both success and error response paths
The file `superset-frontend/src/dashboard/actions/dashboardState.test.ts`
already has comprehensive tests for other thunks like `saveDashboardRequest`
and would be the appropriate location for these tests.
##########
superset-frontend/src/dashboard/actions/dashboardState.ts:
##########
@@ -160,27 +160,43 @@ export function toggleFaveStar(isStarred: boolean):
ToggleFaveStarAction {
}
export function fetchFaveStar(id: number) {
- return function fetchFaveStarThunk(dispatch: AppDispatch) {
+ return function fetchFaveStarThunk(
+ dispatch: AppDispatch,
+ getState: GetState,
+ ) {
return SupersetClient.get({
endpoint: `/api/v1/dashboard/favorite_status/?q=${rison.encode([id])}`,
})
.then(({ json }: { json: JsonObject }) => {
- dispatch(toggleFaveStar(!!(json?.result as JsonObject[])?.[0]?.value));
+ // Only update state if this is still the current dashboard
+ // This prevents stale responses from affecting the UI after navigation
+ const currentId = getState().dashboardInfo?.id;
+ if (currentId === id) {
+ dispatch(
+ toggleFaveStar(!!(json?.result as JsonObject[])?.[0]?.value),
+ );
+ }
})
- .catch(() =>
- dispatch(
- addDangerToast(
- t(
- 'There was an issue fetching the favorite status of this
dashboard.',
+ .catch(() => {
+ // Only show error if this is still the current dashboard
+ // This prevents error toasts from appearing for dashboards the user
+ // has already navigated away from (e.g., deleted dashboards)
+ const currentId = getState().dashboardInfo?.id;
+ if (currentId === id) {
+ dispatch(
+ addDangerToast(
+ t(
+ 'There was an issue fetching the favorite status of this
dashboard.',
+ ),
),
- ),
- ),
- );
+ );
+ }
+ });
};
}
Review Comment:
The race condition fix in `fetchFaveStar` should be covered by tests to
ensure the stale response handling works correctly. The test should verify that
when the dashboard ID changes between the API call and the response, no state
updates or error toasts are dispatched. This is a critical behavior change that
prevents user-facing bugs.
Consider adding test cases that:
1. Verify actions are dispatched when dashboard ID matches
2. Verify actions are NOT dispatched when dashboard ID changes (the race
condition scenario)
3. Cover both success and error response paths
The file `superset-frontend/src/dashboard/actions/dashboardState.test.ts`
already has comprehensive tests for other thunks like `saveDashboardRequest`
and would be the appropriate location for these tests.
--
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]
