Desktop: Resolves #16350: External editor: Add additional logic to avoid overwriting a note with older content (#16352)

packages/lib/services/ExternalEditWatcher.test.ts

@@ -1,8 +1,8 @@
 import { setupDatabaseAndSynchronizer, switchClient } from '../testing/test-utils';
 import ExternalEditWatcher from './ExternalEditWatcher';
-import { appendFile } from 'fs/promises';
+import { appendFile, readFile, utimes } from 'fs/promises';
 import Note from '../models/Note';
-import { msleep } from '@joplin/utils/time';
+import { msleep, Second } from '@joplin/utils/time';
 import waitFor from '../testing/waitFor';
 import { NoteEntity } from './database/types';
 
@@ -100,4 +100,41 @@ describe('ExternalEditWatcher', () => {
 			await watcher.stopWatchingAll();
 		}
 	});
+
+	// In some cases, multiple "update" events are received when Joplin updates an external
+	// editor's temporary file. If an update event doesn't change the temp file's content (e.g.
+	// if the update event only adjusts the timestamp), Joplin should ignore the event to avoid
+	// overwriting the note with old content.
+	// See #16350.
+	test('should ignore change events that don\'t change the file\'s content', async () => {
+		const { filePaths, watcher, notes } = await createAndWatchNotes([
+			{ title: 'Test', body: 'Initial content' },
+		]);
+		try {
+			// Apply an edit and wait to ensure that the initial watcher events
+			// have been processed.
+			await appendFile(filePaths[0], ' (updated)');
+			await waitFor(async () => {
+				expect(await Note.load(notes[0].id)).toMatchObject({
+					title: 'Test',
+					body: 'Initial content (updated)',
+				});
+			});
+
+			await Note.save({ id: notes[0].id, body: 'Out-of-sync' });
+			expect(await readFile(filePaths[0], 'utf-8')).toContain('Initial content');
+
+			// Updating the file without changing its content should not overwrite the new content
+			// in the note:
+			await utimes(filePaths[0], new Date(new Date().getTime() + Second), new Date(new Date().getTime() + Second));
+
+			// Should be unchanged after a brief delay
+			await msleep(200);
+			// ...and after any pending change events are applied
+			await watcher.stopWatching(notes[0].id);
+			expect(await Note.load(notes[0].id)).toMatchObject({ body: 'Out-of-sync' });
+		} finally {
+			await watcher.stopWatchingAll();
+		}
+	});
 });

packages/lib/services/ExternalEditWatcher.ts

@@ -9,6 +9,7 @@ import { openFileWithExternalEditor } from './ExternalEditWatcher/utils';
 import AsyncActionQueue from '../AsyncActionQueue';
 import { EventEmitter } from 'events';
 import * as chokidar from 'chokidar';
+const md5 = require('md5');
 const { ErrorNotFound } = require('./rest/utils/errors');
 
 interface ChangeEventContext {
@@ -20,6 +21,9 @@ type DispatchFn = (action: { type: string; [key: string]: unknown })=> void;
 // eslint-disable-next-line @typescript-eslint/no-explicit-any -- Concrete bridge type lives in app-desktop; lib references it structurally
 type BridgeFn = ()=> any;
 
+const hashNoteContent = (content: string) => md5(content);
+type ItemId = string;
+
 export default class ExternalEditWatcher {
 
 	private dispatch: DispatchFn;
@@ -29,7 +33,8 @@ export default class ExternalEditWatcher {
 	private watcher_: any = null;
 	private changeEventQueue_: AsyncActionQueue<ChangeEventContext>;
 	private eventEmitter_: EventEmitter = new EventEmitter();
-	private skipNextChangeEvent_: Record<string, boolean> = {};
+	private skipNextChangeEvent_: Map<ItemId, boolean> = new Map();
+	private lastNoteContentHash_: Map<ItemId, string> = new Map();
 	// eslint-disable-next-line @typescript-eslint/no-explicit-any -- See watcher_ above
 	private chokidar_: any = chokidar;
 
@@ -135,13 +140,12 @@ export default class ExternalEditWatcher {
 				} else if (event === 'change') {
 					const id = this.noteFilePathToId_(path);
 
-					if (!this.skipNextChangeEvent_[id]) {
+					if (!this.skipNextChangeEvent_.has(id)) {
 						this.changeEventQueue_.push(() => this.onNoteChange_(path), { path });
 					} else {
 						this.logger().debug('ExternalEditWatcher: Skipping this event.');
+						this.skipNextChangeEvent_.delete(id);
 					}
-
-					this.skipNextChangeEvent_ = {};
 				} else if (event === 'error') {
 					this.logger().error('ExternalEditWatcher: error');
 				}
@@ -199,6 +203,12 @@ export default class ExternalEditWatcher {
 			if (!noteContent) this.logger().warn(`ExternalEditWatcher: Could not re-read note - user might have purposely deleted note content: ${id}`);
 		}
 
+		const contentHash = hashNoteContent(noteContent);
+		if (contentHash === this.lastNoteContentHash_.get(id)) {
+			this.logger().info('ExternalEditWatcher: No changes since last write');
+			return;
+		}
+
 		this.logger().debug('ExternalEditWatcher: Updating note object.');
 
 		const updatedNote = await Note.unserializeForEdit(noteContent);
@@ -206,6 +216,8 @@ export default class ExternalEditWatcher {
 		updatedNote.parent_id = note.parent_id;
 		await Note.save(updatedNote);
 		this.eventEmitter_.emit('noteChange', { id: updatedNote.id, note: updatedNote });
+
+		this.lastNoteContentHash_.set(id, contentHash);
 	}
 
 	private noteIdToFilePath_(noteId: string) {
@@ -287,6 +299,9 @@ export default class ExternalEditWatcher {
 		const filePath = this.noteIdToFilePath_(noteId);
 		if (this.watcher_) this.watcher_.unwatch(filePath);
 		await shim.fsDriver().remove(filePath);
+
+		this.lastNoteContentHash_.delete(noteId);
+		this.skipNextChangeEvent_.delete(noteId);
 		this.dispatch({
 			type: 'NOTE_FILE_WATCHER_REMOVE',
 			id: noteId,
@@ -304,6 +319,8 @@ export default class ExternalEditWatcher {
 
 		if (this.watcher_) this.watcher_.close();
 		this.watcher_ = null;
+		this.skipNextChangeEvent_.clear();
+		this.lastNoteContentHash_.clear();
 		this.logger().info('ExternalEditWatcher: Stopped watching all files');
 		this.dispatch({
 			type: 'NOTE_FILE_WATCHER_CLEAR',
@@ -322,7 +339,7 @@ export default class ExternalEditWatcher {
 
 		// When the note file is updated programmatically, we skip the next change event to
 		// avoid update loops. We only want to listen to file changes made by the user.
-		this.skipNextChangeEvent_[note.id] = true;
+		this.skipNextChangeEvent_.set(note.id, true);
 
 		await this.writeNoteToFile_(note);
 	}
@@ -335,7 +352,10 @@ export default class ExternalEditWatcher {
 
 		const filePath = this.noteIdToFilePath_(note.id);
 		const noteContent = await Note.serializeForEdit(note);
+
 		await shim.fsDriver().writeFile(filePath, noteContent, 'utf-8');
+		this.lastNoteContentHash_.set(note.id, hashNoteContent(noteContent));
+
 		return filePath;
 	}
 }