Update: react-monaco-editor and the hooks issue

[codebykat]

Fix

In the course of the multiple upgrades in #3183, we noticed that the editor sometimes failed to focus the text area on load or on creating a new note. I eventually traced this to the version bump of monaco-react-editor, specifically the refactor to use hooks, which was included in versions higher than 0.50.1.

After some experimentation I reordered the code in such a way that it seems to work (moving the initialization from componentDidMount into editorReady), but I'm a little worried about this change, because the existing codebase is written with a Redux (action dispatch) architecture that largely predates React hooks, and I'm not sure whether it might introduce unintended side effects. I'm also not sure if we still need to use a boot timer, or if this refactor incidentally solves that.

The discussion at the above-referenced issue does not contain any real workarounds nor is there an update to the component that reverts or fixes the behavior, which is somewhat concerning. However a quick search for alternatives to react-monaco-editor didn't unearth any viable options (the other major package is written to load the editor from a CDN on load to avoid configuration, which is definitely a choice).

Test

Since this change affects the loading order of the editor component, we need to make sure it doesn't have any unintended side effects, so smoke-test all the things: switching notes, creating a new note, syncing changes, etc.

Release

Fixed a bug causing the text area not to be focused on load or on new note creation.

[dmsnell]

If this tests well I don't see a problem with the proposed patch. These are still imperative APIs from the editor, no orthogonal to React and its hooks. Redux actions and updates still work fine in a hooks world too; the only thing to watch out for are the normal inherent gotcha's with hooks and stale data, when supplied to Redux code, as improper use of hooks can lead to corruption deeper in the code.

If the delay isn't causing noticeable harm maybe it's best to leave it in. Take it out in another PR where it can be tested by itself. I don't remember the reason it's there but I would hope it's in a commit message.

[codebykat]

Thanks @dmsnell!

I don't remember the reason it's there but I would hope it's in a commit message.

Unfortunately, as with so many things around here, almost all the blame points to a giant changeset that was itself a refactor of something else: fc89212 🫠

[dmsnell]

@codebykat the answer is in the following

https://github.com/Automattic/simplenote-electron/blame/d036a087370589a884f03ef32955e1842caf26ac/lib/note-content-editor.tsx#L126-L137

We were dealing with UI jank when navigating through notes because of the shift that occurs when initializing Monaco. It would render the checkboxes and that bogged down scrolling. The initial state then was the "fast" mode, which rendered plaintext, and when stopping at a single note, after the delay, would shift into "full" mode with the replaced checkboxes and full Monaco functionality.

So it's not necessary if the experience of switching between notes remains fast. It's a workaround to not having a faster UI.