Desktop: Fixes #8960: Fix cursor jumps to the top of the note editor on sync (#10456)

packages/app-desktop/gui/NoteEditor/NoteEditor.tsx

@@ -10,7 +10,7 @@ import useMessageHandler from './utils/useMessageHandler';
 import useWindowCommandHandler from './utils/useWindowCommandHandler';
 import useDropHandler from './utils/useDropHandler';
 import useMarkupToHtml from './utils/useMarkupToHtml';
-import useFormNote, { OnLoadEvent, SetFormNote } from './utils/useFormNote';
+import useFormNote, { OnLoadEvent, OnSetFormNote } from './utils/useFormNote';
 import useEffectiveNoteId from './utils/useEffectiveNoteId';
 import useFolder from './utils/useFolder';
 import styles_ from './styles';
@@ -68,7 +68,7 @@ function NoteEditor(props: NoteEditorProps) {
 	const isMountedRef = useRef(true);
 	const noteSearchBarRef = useRef(null);
 
-	const setFormNoteRef = useRef<SetFormNote>();
+	const setFormNoteRef = useRef<OnSetFormNote>();
 	const { saveNoteIfWillChange, scheduleSaveNote } = useScheduleSaveCallbacks({
 		setFormNote: setFormNoteRef, dispatch: props.dispatch, editorRef,
 	});

packages/app-desktop/gui/NoteEditor/utils/useFormNote.ts

@@ -1,4 +1,4 @@
-import { useState, useEffect, useCallback, RefObject } from 'react';
+import { useState, useEffect, useCallback, RefObject, useRef } from 'react';
 import { FormNote, defaultFormNote, ResourceInfos } from './types';
 import { clearResourceCache, attachedResources } from './resourceHandling';
 import AsyncActionQueue from '@joplin/lib/AsyncActionQueue';
@@ -8,13 +8,15 @@ import Setting from '@joplin/lib/models/Setting';
 import usePrevious from '../../hooks/usePrevious';
 import ResourceEditWatcher from '@joplin/lib/services/ResourceEditWatcher/index';
 
-const { MarkupToHtml } = require('@joplin/renderer');
+import { MarkupToHtml } from '@joplin/renderer';
 import Note from '@joplin/lib/models/Note';
-import { reg } from '@joplin/lib/registry';
 import ResourceFetcher from '@joplin/lib/services/ResourceFetcher';
 import DecryptionWorker from '@joplin/lib/services/DecryptionWorker';
 import { NoteEntity } from '@joplin/lib/services/database/types';
 import { focus } from '@joplin/lib/utils/focusHandler';
+import Logger from '@joplin/utils/Logger';
+
+const logger = Logger.create('useFormNote');
 
 export interface OnLoadEvent {
 	formNote: FormNote;
@@ -32,7 +34,8 @@ export interface HookDependencies {
 	onAfterLoad(event: OnLoadEvent): void;
 }
 
-export type SetFormNote = ReturnType<typeof useState<FormNote>>[1];
+type MapFormNoteCallback = (previousFormNote: FormNote)=> FormNote;
+export type OnSetFormNote = (newFormNote: FormNote|MapFormNoteCallback)=> void;
 
 // eslint-disable-next-line @typescript-eslint/ban-types -- Old code before rule was applied
 function installResourceChangeHandler(onResourceChangeHandler: Function) {
@@ -78,11 +81,14 @@ export default function useFormNote(dependencies: HookDependencies) {
 	const previousNoteId = usePrevious(formNote.id);
 	const [resourceInfos, setResourceInfos] = useState<ResourceInfos>({});
 
+	const formNoteRef = useRef(formNote);
+	formNoteRef.current = formNote;
+
 	// Increasing the value of this counter cancels any ongoing note refreshes and starts
 	// a new refresh.
 	const [formNoteRefreshScheduled, setFormNoteRefreshScheduled] = useState<number>(0);
 
-	async function initNoteState(n: NoteEntity) {
+	const initNoteState = useCallback(async (n: NoteEntity, isNewNote: boolean) => {
 		let originalCss = '';
 
 		if (n.markup_language === MarkupToHtml.MARKUP_LANGUAGE_HTML) {
@@ -107,22 +113,39 @@ export default function useFormNote(dependencies: HookDependencies) {
 			encryption_applied: n.encryption_applied,
 		};
 
+		logger.debug('Initializing note state');
+
 		// Note that for performance reason,the call to setResourceInfos should
 		// be first because it loads the resource infos in an async way. If we
 		// swap them, the formNote will be updated first and rendered, then the
 		// the resources will load, and the note will be re-rendered.
-		setResourceInfos(await attachedResources(n.body));
+		const resources = await attachedResources(n.body);
+
+		// If the user changes the note while resources are loading, this can lead to
+		// a note being incorrectly marked as "unchanged".
+		if (!isNewNote && formNoteRef.current?.hasChanged) {
+			logger.info('Cancelled note refresh -- form note changed while loading attached resources.');
+			return null;
+		}
+
+		setResourceInfos(resources);
 		setFormNote(newFormNote);
 
+		logger.debug('Resource info and form note set.');
+
 		await handleResourceDownloadMode(n.body);
 
 		return newFormNote;
-	}
+	}, []);
 
 	useEffect(() => {
 		if (formNoteRefreshScheduled <= 0) return () => {};
+		if (formNoteRef.current.hasChanged) {
+			logger.info('Form note changed between scheduling a refresh and the refresh itself. Cancelling the refresh.');
+			return () => {};
+		}
 
-		reg.logger().info('Sync has finished and note has never been changed - reloading it');
+		logger.info('Sync has finished and note has never been changed - reloading it');
 
 		let cancelled = false;
 
@@ -134,11 +157,12 @@ export default function useFormNote(dependencies: HookDependencies) {
 			// it would not have been loaded in the editor (due to note selection changing
 			// on delete)
 			if (!n) {
-				reg.logger().warn('Trying to reload note that has been deleted:', noteId);
+				logger.warn('Trying to reload note that has been deleted:', noteId);
 				return;
 			}
 
-			await initNoteState(n);
+			await initNoteState(n, false);
+
 			setFormNoteRefreshScheduled(0);
 		};
 
@@ -147,7 +171,7 @@ export default function useFormNote(dependencies: HookDependencies) {
 		return () => {
 			cancelled = true;
 		};
-	}, [formNoteRefreshScheduled, noteId]);
+	}, [formNoteRefreshScheduled, noteId, initNoteState]);
 
 	const refreshFormNote = useCallback(() => {
 		// Increase the counter to cancel any ongoing refresh attempts
@@ -164,7 +188,9 @@ export default function useFormNote(dependencies: HookDependencies) {
 		const syncJustEnded = prevSyncStarted && !syncStarted;
 
 		if (!decryptionJustEnded && !syncJustEnded) return;
-		if (formNote.hasChanged) return;
+		if (formNoteRef.current.hasChanged) return;
+
+		logger.debug('Sync or decryption finished with an unchanged formNote.');
 
 		// Refresh the form note.
 		// This is kept separate from the above logic so that when prevSyncStarted is changed
@@ -173,7 +199,7 @@ export default function useFormNote(dependencies: HookDependencies) {
 	}, [
 		prevSyncStarted, syncStarted,
 		prevDecryptionStarted, decryptionStarted,
-		formNote.hasChanged, refreshFormNote,
+		refreshFormNote,
 	]);
 
 	useEffect(() => {
@@ -186,7 +212,7 @@ export default function useFormNote(dependencies: HookDependencies) {
 
 		let cancelled = false;
 
-		reg.logger().debug('Loading existing note', noteId);
+		logger.debug('Loading existing note', noteId);
 
 		function handleAutoFocus(noteIsTodo: boolean) {
 			if (!isProvisional) return;
@@ -206,11 +232,11 @@ export default function useFormNote(dependencies: HookDependencies) {
 			const n = await Note.load(noteId);
 			if (cancelled) return;
 			if (!n) throw new Error(`Cannot find note with ID: ${noteId}`);
-			reg.logger().debug('Loaded note:', n);
+			logger.debug('Loaded note:', n);
 
 			await onBeforeLoad({ formNote });
 
-			const newFormNote = await initNoteState(n);
+			const newFormNote = await initNoteState(n, true);
 
 			setIsNewNote(isProvisional);
 
@@ -267,5 +293,24 @@ export default function useFormNote(dependencies: HookDependencies) {
 		};
 	}, [formNote.body]);
 
-	return { isNewNote, formNote, setFormNote, resourceInfos };
+	// Currently, useFormNote relies on formNoteRef being up-to-date immediately after the editor
+	// changes, with no delay during which async code can run. Even a small delay (e.g. that introduced
+	// by a setState -> useEffect) can lead to a race condition. See https://github.com/laurent22/joplin/issues/8960.
+	const onSetFormNote: OnSetFormNote = useCallback(newFormNote => {
+		if (typeof newFormNote === 'function') {
+			const newNote = newFormNote(formNoteRef.current);
+			formNoteRef.current = newNote;
+			setFormNote(newNote);
+		} else {
+			formNoteRef.current = newFormNote;
+			setFormNote(newFormNote);
+		}
+	}, [setFormNote]);
+
+	return {
+		isNewNote,
+		formNote,
+		setFormNote: onSetFormNote,
+		resourceInfos,
+	};
 }

packages/app-desktop/gui/NoteEditor/utils/useScheduleSaveCallbacks.ts

@@ -6,12 +6,12 @@ import ExternalEditWatcher from '@joplin/lib/services/ExternalEditWatcher';
 import Note from '@joplin/lib/models/Note';
 import type { Dispatch } from 'redux';
 import eventManager, { EventName } from '@joplin/lib/eventManager';
-import type { SetFormNote } from './useFormNote';
+import type { OnSetFormNote } from './useFormNote';
 
 const logger = Logger.create('useScheduleSaveCallbacks');
 
 interface Props {
-	setFormNote: RefObject<SetFormNote>;
+	setFormNote: RefObject<OnSetFormNote>;
 	dispatch: Dispatch;
 	editorRef: RefObject<NoteBodyEditorRef>;
 }