Skip to content

fix: remove router guard for unsaved category settings#664

Merged
ErikBjare merged 1 commit intoActivityWatch:masterfrom
HomeArchbishop:master
Apr 17, 2025
Merged

fix: remove router guard for unsaved category settings#664
ErikBjare merged 1 commit intoActivityWatch:masterfrom
HomeArchbishop:master

Conversation

@HomeArchbishop
Copy link
Copy Markdown
Contributor

@HomeArchbishop HomeArchbishop commented Apr 4, 2025

In the file CategorizationSettings.vue, there is a note stating // TODO: How to remove this listener?.

This unresolved listener issue leads to an unintended behavior where the confirmation popup reappears every time the route changes, even if the initial confirmation popup was ignored.

This PR addresses the problem to ensure proper removal of the listener and prevent repeated popups during route changes.

Before: repeated popup After: popup only once
repeated popup popup only once

Thanks in advance.


[BTW] This PR does resolve the aforementioned issue. However, I noticed that in src/stores/categories.ts, the state of classes_unsaved_changes remains true even after the confirmation popup is ignored, until the store's load() action is called again.

This behavior might lead to potential issues, but in line with the principle of minimal changes, I have not addressed this in this PR.


Important

Fixes repeated confirmation popup issue in CategorizationSettings.vue by properly removing the router guard.

  • Behavior:
    • Fixes repeated confirmation popup issue in CategorizationSettings.vue by properly removing the router guard.
    • Stores the router guard remover function in routerGuardRemover and calls it in beforeDestroy().
  • Misc:
    • Adds routerGuardRemover to data in CategorizationSettings.vue.

This description was created by Ellipsis for 5d393f8. It will automatically update as commits are pushed.

Copy link
Copy Markdown
Contributor

@ellipsis-dev ellipsis-dev Bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Reviewed everything up to 5d393f8 in 57 seconds

More details
  • Looked at 32 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 6 drafted comments based on config settings.
1. src/views/settings/CategorizationSettings.vue:64
  • Draft comment:
    Good: 'routerGuardRemover' is declared to store the removal function for the router guard.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None
2. src/views/settings/CategorizationSettings.vue:79
  • Draft comment:
    Assigning the router guard removal function to 'this.routerGuardRemover' ensures proper cleanup. Nice fix!
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None
3. src/views/settings/CategorizationSettings.vue:98
  • Draft comment:
    After calling the removal function in beforeDestroy, consider setting 'this.routerGuardRemover = null' for clarity.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None
4. src/views/settings/CategorizationSettings.vue:64
  • Draft comment:
    Initialized 'routerGuardRemover' in data is clear; consider renaming it (e.g. 'unregisterRouterGuard') to clarify its purpose.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None
5. src/views/settings/CategorizationSettings.vue:79
  • Draft comment:
    Assigning the return value of router.beforeEach to routerGuardRemover effectively captures the unregistration function. Verify that your router version returns an unregister function, as this behavior is available in Vue Router 4.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None
6. src/views/settings/CategorizationSettings.vue:98
  • Draft comment:
    The beforeDestroy hook correctly removes both the beforeunload listener and the router guard by invoking routerGuardRemover. Ensure that this cleanup works as expected in your router version.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None

Workflow ID: wflow_cOvV5FS0dM3uXsO7


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 5, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 26.76%. Comparing base (cb9a7ec) to head (5d393f8).
Report is 2 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #664   +/-   ##
=======================================
  Coverage   26.76%   26.76%           
=======================================
  Files          28       28           
  Lines        1655     1655           
  Branches      292      292           
=======================================
  Hits          443      443           
  Misses       1155     1155           
  Partials       57       57           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@ErikBjare
Copy link
Copy Markdown
Member

Very nice, thank you

@ErikBjare ErikBjare merged commit 9842d3e into ActivityWatch:master Apr 17, 2025
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants