Conversation
The directive only cleaned up on Vue unmount, which does not run on a full page navigation. Also tear down on pagehide so timers, listeners and the field value are cleared when leaving the page.
Keep the watcher armed across back/forward-cache transitions (pagehide with persisted=true) instead of disarming it, guard against arming the same input twice, and add Jest coverage for the teardown paths.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
The directive only reset the DOM value, but its Vue consumers bind the field through v-model. On a bfcache restore (and on the inactivity timeout) the reactive model still held the old value, so the next render wrote the password back into the input. Dispatch a change event on clear so the password Field syncs its model to empty as well.
github-actions
Bot
dismissed
their stale review
July 22, 2026 09:57
Superseded by Codex Review run https://github.com/matomo-org/matomo/actions/runs/29909860634.
Contributor
There was a problem hiding this comment.
🤖 Codex Review: 💬 Low / Polish
Summary
This PR correctly adds shared teardown for normal navigation, bfcache transitions, and Vue unmounting, with focused regression coverage. The implementation appears merge-ready; the only requested cleanup is ensuring each test tears down its directive so intervals and global listeners do not leak between cases.
Findings Overview
| Severity | Count |
|---|---|
| 🚫 Blocking | 0 |
| 0 | |
| 💬 Low / Polish | 1 |
📍 Posted 1 inline finding.
Diagnostics
Detailed review diagnostics are available in the codex-review-output workflow artifact.
The specs mount the directive manually, so clearing the DOM node did not fire the unmounted hook, leaving each test's polling interval and global pagehide listener active for later cases. Explicitly tear each input down in afterEach to keep the suite isolated.
sgiehl
marked this pull request as ready for review
July 23, 2026 15:40
sgiehl
enabled auto-merge (squash)
July 23, 2026 15:40
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The
AutoClearPassworddirective registered its listeners, timers and anelement-level cleanup handle in
mounted, but only tore them down in the Vueunmountedhook.unmounteddoes not run on a full page navigation, so thewatcher (and the value it tracked) was left in place when leaving the page.
This adds a
pagehideteardown path so the directive cleans up on navigation aswell as on component unmount:
input/changelisteners and thepagehidelisteneronUmountedhandle on the elementBoth entry points share a single
teardown()helper so unmount and navigationbehave consistently. On a back/forward-cache transition (
pagehidewithpersisted) the value is dropped but the watcher stays armed, so a restoredpage keeps working. Setup is also guarded so the same input is never armed
twice.
Regression tests are added in
AutoClearPassword.spec.ts.Review
Built bundles (
CoreHome.umd.js,CoreHome.umd.min.js) are included.Checklist