fix(settings): prevent confirm dialog persisting after leaving settings#754
Conversation
Replace the global router.beforeEach guard in CategorizationSettings with a beforeRouteLeave guard on the parent Settings component. The global guard required manual cleanup in beforeDestroy, which could fail to execute in time during route transitions, causing the unsaved changes dialog to appear on every subsequent navigation even after leaving the settings page. beforeRouteLeave is automatically managed by Vue Router and only fires when leaving the /settings route, eliminating the zombie guard issue. The window.beforeunload handler remains in CategorizationSettings for browser tab/window close protection. Fixes ActivityWatch/activitywatch#626
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to b049dff in 8 seconds. Click for details.
- Reviewed
89lines of code in2files - Skipped
0files when reviewing. - Skipped posting
0draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
Workflow ID: wflow_JMBcKALfoyXutpC0
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Greptile SummaryThis PR fixes a critical bug where the unsaved changes confirmation dialog persisted on every navigation after leaving the settings page. The fix refactors the navigation guard from a global Key Changes:
Benefits:
The logic is sound and properly addresses the root cause described in ActivityWatch/activitywatch#626. Confidence Score: 5/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[User navigates away from /settings] --> B{Is there unsaved category changes?}
B -->|Yes| C[Show confirmation dialog]
B -->|No| D[Allow navigation]
C -->|User clicks OK| D
C -->|User clicks Cancel| E[Stay on /settings]
F[User closes/refreshes browser tab] --> G{Is there unsaved category changes?}
G -->|Yes| H[Browser shows beforeunload warning]
G -->|No| I[Allow close/refresh]
style A fill:#e1f5ff
style F fill:#e1f5ff
style B fill:#fff4e1
style G fill:#fff4e1
style C fill:#ffe1e1
style H fill:#ffe1e1
style D fill:#e1ffe1
style I fill:#e1ffe1
style E fill:#ffe1e1
Last reviewed commit: b049dff |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #754 +/- ##
=======================================
Coverage 26.08% 26.08%
=======================================
Files 29 29
Lines 1698 1698
Branches 294 294
=======================================
Hits 443 443
Misses 1233 1233
Partials 22 22 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Summary
router.beforeEachtobeforeRouteLeaveon the parentSettingscomponentProblem
When modifying categories without saving and navigating away from the settings page, the "unsaved changes" confirmation dialog keeps appearing on every subsequent navigation, even when the user is no longer on the settings page.
Root Cause
CategorizationSettings.vueregistered a globalrouter.beforeEachguard that required manual cleanup inbeforeDestroy. If the cleanup didn't execute in time during route transitions (race condition betweennext()completing andbeforeDestroyfiring), the guard persisted as a "zombie" — blocking all future navigations with the confirm dialog.Fix
Replace the global
router.beforeEachguard withbeforeRouteLeaveon the parentSettings.vueroute component.beforeRouteLeaveis:/settingsroute (not on every navigation)The
window.beforeunloadhandler remains inCategorizationSettings.vuefor browser tab/window close protection (separate concern).Test plan
Fixes ActivityWatch/activitywatch#626
Important
Fixes persistent unsaved changes dialog by moving navigation guard to
beforeRouteLeaveinSettings.vue.router.beforeEachtobeforeRouteLeaveinSettings.vue./settingsroute.router.beforeEachguard fromCategorizationSettings.vue.beforeRouteLeaveguard toSettings.vueto handle unsaved changes.window.beforeunloadhandler inCategorizationSettings.vuefor tab/window close protection.This description was created by
for b049dff. You can customize this summary. It will automatically update as commits are pushed.