Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DeleteDashboard: Redirect to home after deleting a dashboard #78918

Merged
merged 2 commits into from
Dec 1, 2023

Conversation

ivanortegaalba
Copy link
Contributor

Fixes #78810

Diagnosis
The useDeleteDashboard was listening to when the result was present in the state.value from useAsyncFn giving a cb to be called onDeleteDashboard. The callback returned by useAsyncFn is used inside another useAsyncFn that hides the modal once the onDeleteDashboard is resolved.

Because onDeleteDashboard is resolved before the next re-render is triggered, the useEffect declared in useDeleteDashboard is never called, so the variables and the replace is never invoked.

Solution
I just simplified this piece of code, since the custom hook useDeleteDashboard is never used. I approached moving everything into a single async callback called when the user clicks confirm. I also removed the duplicated notification to show only one since it did not add much value.

@ivanortegaalba ivanortegaalba requested a review from a team as a code owner November 30, 2023 17:10
@ivanortegaalba ivanortegaalba requested review from axelavargas, kaydelaney, dprokop and evictorero and removed request for a team November 30, 2023 17:10
@grafana-delivery-bot grafana-delivery-bot bot added this to the 10.3.x milestone Nov 30, 2023
@ivanortegaalba ivanortegaalba changed the title Fix: Redirect to home after deleting a dashboard DeleteDashboard: Redirect to home after deleting a dashboard Nov 30, 2023
Copy link
Contributor

@evictorero evictorero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tested it and works as expected. Left some comments.

public/app/features/manage-dashboards/state/actions.ts Outdated Show resolved Hide resolved
public/app/features/manage-dashboards/state/actions.ts Outdated Show resolved Hide resolved
@ivanortegaalba
Copy link
Contributor Author

I've tested it and works as expected. Left some comments.

Fixed, sorry

Copy link
Contributor

@evictorero evictorero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Shouldn't be backported since it is a regression?

Copy link
Member

@dprokop dprokop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works as expected, nice!

Did you manage to figure out when/what introduced this regression? Also, I think this may deserve a backport to 10.2.x

@ivanortegaalba ivanortegaalba added backport v10.2.x and removed no-backport Skip backport of PR labels Dec 1, 2023
@ivanortegaalba
Copy link
Contributor Author

Sorry, I got confused with the backport policy vs unscheduled releases. Marked as backport.

@dprokop, it was caused because the hiddenModal function was hiding the modal before the modal re-renders since it is upper in the shadow dom hierarchy. So the hook was not triggering the callbacks under if (state.value).

When this changes, I don't know. The code from our side hasn't been modified in 2 years. My only guess is they fixed something in the execution order of the useAsyncFn.

@ivanortegaalba ivanortegaalba merged commit 8d7314b into main Dec 1, 2023
19 checks passed
@ivanortegaalba ivanortegaalba deleted the fix-delete-dashboard-rediret branch December 1, 2023 09:15
grafana-delivery-bot bot pushed a commit that referenced this pull request Dec 1, 2023
ivanortegaalba added a commit that referenced this pull request Dec 1, 2023
#78936)

DeleteDashboard: Redirect to home after deleting a dashboard (#78918)

(cherry picked from commit 8d7314b)

Co-authored-by: Ivan Ortega Alba <ivanortegaalba@gmail.com>
@aangelisc aangelisc modified the milestones: 10.3.x, 10.2.3 Dec 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Dashboards: Deleting a dashboard from settings is not redirecting to home
4 participants