Skip to content

Conversation

@SiriusXT
Copy link
Member

@SiriusXT SiriusXT commented Nov 20, 2025

Ensure that the editor is properly destroyed when a noteContext is removed.
This prevents issues like #5740 and
potential memory leaks, as editors associated with removed notes would otherwise persist.

This approach seems effective, but I’m not certain if it’s the best solution.

@SiriusXT SiriusXT marked this pull request as ready for review November 20, 2025 09:01
@dosubot dosubot bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Nov 20, 2025
@SiriusXT SiriusXT changed the title fix(editor): clean up editor on note removal fix(editor): clean up editor on noteContext removal Nov 20, 2025
Copy link
Contributor

@eliandoran eliandoran left a comment

Choose a reason for hiding this comment

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

I think you are on the right path with this fix. Nevertheless, the problem with it is that it's necessary to manually handle the cleanup, but Code, EditableText should already handle the clean up on their side. (thus requiring extra work and it's easy to forget that this mechanism exists).

For example for Code, the editor is destroyed in CodeMirror.tsx:33 automatically.

The problem is that the component tree is not properly cleaned when a note context is removed, so I think you can move this event handling in either NoteDetail.tsx (preferred since it's already in React) or in split_note_container.

Instead of manually cleaning up, all we need to do is to remove the component from the tree. In NoteDetail we have noteTypesToRender which caches all the type widgets in a given note split. Perhaps all we need to do here is to clear this array when the note context is removed.

Let me know if this works or if you need any help.

@eliandoran eliandoran marked this pull request as draft December 8, 2025 08:55
@SiriusXT
Copy link
Member Author

SiriusXT commented Dec 9, 2025

@eliandoran Thank you for the detailed explanation. I tried clearing noteTypesToRender in NoteDetail.tsx, but unfortunately I couldn’t get it to work correctly. It turned out to be more complex than I expected, and since I’m not making progress on this at the moment, I’ll go ahead and close the PR.

@SiriusXT SiriusXT closed this Dec 9, 2025
@SiriusXT SiriusXT deleted the siriusbac_editor_patch branch December 9, 2025 06:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:M This PR changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants