Desktop: Fixes #8960: Fix cursor jumps to the top of the note editor on sync #10456
+66
−21
Conversation
This file contains 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
…e on sync This commit fixes a race condition caused by the editor's form note's hasChanged property not being up-to-date (or checked) while refreshing the note. The `.hasChanged` property was previously checked only before running async logic that updated the note. If the note changed during this async logic, this could change the value passed down to the editor as its content. If the editor had recently updated its content, the content prop would then **not match** the current content of the editor. The CodeMirror 5 (and also the rich text?) editors handle this by 1) changing the editor's content 2) clearing the editor's undo history and 3) resetting the cursor position. The CodeMirror 6 editor only does 1 and 2, so the issue was most significant in the CodeMirror 5-based editor. This commit fixes the issue by 1) storing a refrence to the latest formNote in a ref and 2) checking ref.current.hasChanged just before saving changes to the note.
personalizedrefrigerator
changed the title
|
That's great, thanks for looking into this! Let's merge and see what feedback we get, hopefully that fixes it for good! |
|
Good job! Thanks a lot! |
|
The issue is still present: #8960 (comment) |
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.
Summary
Note
Update: This does not seem to have resolved the issue.
This pull request fixes a race condition in useFormNote.ts that sometimes caused the editor cursor to jump to the top of a note after a sync finishes.
May fix #8960.
Details
More specifically, the issue was caused by
formNote.hasChangedbeing out-of-date.In
NoteEditor.tsx,.hasChangedis set to true by changing the value of a state variable (setFormNote). This is done in a callback fired when the current note updates:joplin/packages/app-desktop/gui/NoteEditor/NoteEditor.tsx
Line 186 in faf332a
Previously,
.hasChangedwas checked just before scheduling a refresh:joplin/packages/app-desktop/gui/NoteEditor/utils/useFormNote.ts
Line 167 in faf332a
After a refresh completed,
.hasChangedwas set tofalse:joplin/packages/app-desktop/gui/NoteEditor/utils/useFormNote.ts
Line 105 in faf332a
The issue can occur if
.hasChangedbecomestruejust after a refresh is scheduled. In this case, the refresh would still happen. The refresh would load the current copy of the note from the database (n).Because the editor's recent changes haven't had time to be written to the database,
n.bodywould not match the content of the editor. The CodeMirror 5 (and also the rich text?) editors handle this by changing the editor's content, clearing the editor's undo history, and resetting the cursor position. (The CodeMirror 6 editor handles cursor positioning differently).Adding an additional
.hasChanged-based check doesn't seem to be enough to fix the issue (see comment). It seems to be necessary to set.hasChangedimmediately, without waiting for a re-render to update its value. This pull request does this by:onSetFormNotecallback that wrapssetFormNote.formNoteRefwithinonSetFormNote.formNoteRef.current.hasChangedjust before setting the new form note.Testing plan
The original issue is difficult to reproduce. As such, the following testing plan focuses on avoiding regressions:
This has been tested successfully on Ubuntu 24.04.
Note: Additional manual testing was done with an equivalent fix on this branch with additional debug logging and more frequent auto-sync enabled.