Skip to content

fix: remove toolbar window listeners on unmount#2783

Open
ArturQuirino wants to merge 1 commit intosuperdoc-dev:mainfrom
ArturQuirino:fix/toolbar-unmount-window-listeners
Open

fix: remove toolbar window listeners on unmount#2783
ArturQuirino wants to merge 1 commit intosuperdoc-dev:mainfrom
ArturQuirino:fix/toolbar-unmount-window-listeners

Conversation

@ArturQuirino
Copy link
Copy Markdown

Summary

Ensures the default editor Toolbar removes its window resize and keydown listeners when the component is destroyed, not only when a <KeepAlive> boundary deactivates it.

Problem

onDeactivated runs for kept-alive components when they are hidden, but not on a normal unmount. Global listeners registered in onMounted could therefore stay attached after the toolbar was torn down.

Solution

  • Introduce a small teardownWindowListeners() helper and call it from both onDeactivated and onBeforeUnmount.
  • Add Toolbar.test.js to assert removeEventListener is invoked with the same handler references used in addEventListener (important because resize uses a throttled wrapper).

Test plan

  • pnpm --filter @superdoc/super-editor exec vitest run src/editors/v1/components/toolbar/Toolbar.test.js
  • (optional) pnpm --filter @superdoc/super-editor exec vitest run src/editors/v1/tests/toolbar/

Copy link
Copy Markdown
Contributor

@caio-pizzol caio-pizzol left a comment

Choose a reason for hiding this comment

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

@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);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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)', () => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants