fix: remove toolbar window listeners on unmount#2783
fix: remove toolbar window listeners on unmount#2783ArturQuirino wants to merge 1 commit intosuperdoc-dev:mainfrom
Conversation
caio-pizzol
left a comment
There was a problem hiding this comment.
@ArturQuirino clean fix, and it matches how SuperEditor.vue already does this. one thing to think about: onDeactivated removes the listeners but nothing puts them back on show — if the toolbar ever lives inside <KeepAlive>, it'll break after the first hide. not caused by this PR, just worth checking. also suggested one extra test so the two paths stay covered. left inline notes.
| window.addEventListener('keydown', onKeyDown); | ||
| }); | ||
|
|
||
| onDeactivated(teardownWindowListeners); |
There was a problem hiding this comment.
onDeactivated runs when a <KeepAlive> hides this component, but there's no onActivated to put the listeners back when it's shown again. if the toolbar is ever wrapped in <KeepAlive>, the second time it shows up Cmd+F and the resize handling will be dead. if it's never wrapped, you can just drop onDeactivated and keep onBeforeUnmount. worth a look?
| } | ||
|
|
||
| describe('Toolbar', () => { | ||
| it('removes resize and keydown listeners on unmount (not only on KeepAlive deactivate)', () => { |
There was a problem hiding this comment.
the test only checks the unmount path. the whole point of this fix is that both paths matter — could you add one that hides the toolbar inside <KeepAlive> too? that way nobody deletes onDeactivated later thinking it's unused.
Summary
Ensures the default editor
Toolbarremoves itswindowresizeandkeydownlisteners when the component is destroyed, not only when a<KeepAlive>boundary deactivates it.Problem
onDeactivatedruns for kept-alive components when they are hidden, but not on a normal unmount. Global listeners registered inonMountedcould therefore stay attached after the toolbar was torn down.Solution
teardownWindowListeners()helper and call it from bothonDeactivatedandonBeforeUnmount.Toolbar.test.jsto assertremoveEventListeneris invoked with the same handler references used inaddEventListener(important becauseresizeuses a throttled wrapper).Test plan
pnpm --filter @superdoc/super-editor exec vitest run src/editors/v1/components/toolbar/Toolbar.test.jspnpm --filter @superdoc/super-editor exec vitest run src/editors/v1/tests/toolbar/