Merged
Conversation
Without calling `dispose()`, Lexxy leaks references to an entire Lexical reference on a reconnection cycle, for example during turbo navigation when the element is in a `turbo-permanent` tree. Disposable must happen before removal of the bound contenteditable.
Prevented editor `dispose` since the return of `setScrollableTablesActive` is null. Since the editor is registered in a WeakMap, no need to go to the trouble of registering a clean-up function.
Removing tools before editor disposal run their tear-down and handler deregistration, preventing them firing when the editor state changes one last time on disconnect. Fix tableHandler -> tableTools name problem
…itor When removing an element, `disconnectedCallback` on child elements is enqueued after the parent, so various handlers remain in place and fire when the editor is disposed. Calling `remove()` does not fire the event since the element is already gone from the DOM. Extracting to `dispose()` allows synchronously removing the handlers and preventing undesired updates. Given the forced disposal of handlers, tools which are custom elements don't require de-registration as they will be re-connected on re-intertion.
As custom handlers are disposed appropriately, there's no need to remove the elements from the DOM, which will simply re-connect when added back to the DOM. Clean-up toolbar-dropdown initialization Made custom element tool idempotent (or at least closer). Find existing elements or ensure a clean slate before creation
Null the toolbar reference to avoid turbo morphing wiping the toolbar dom while keeping an outdated reference to it. The lifecycle handling could do with a rethink, but this will be a step forward.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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
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.
Called
editor.dispose()on element disconnection, which properly tears down listeners and event handlers.undefinedlistenerdisposefunction to force handler removalAccording to the performance tests we have no more leaked listeners or nodes:
maindisposeProbably removes the need for a lot of #717