From 3eb8d2eeabf4e2e8840b2d83ec618df0f502949e Mon Sep 17 00:00:00 2001
From: Frederic Cabassut <cabassut.frederic@gmail.com>
Date: Wed, 5 Aug 2026 10:18:34 +0200
Subject: [PATCH] fix: close note-switching race conditions and restore
 offline-first note open
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Three issues found in review, all around switching to a different note
while the editor is already open (via a note-link or backlink chip):

- TiptapEditor's onTransaction/onUpdate had no guard against writing
  state after the component is torn down. Removing a focused editor's
  DOM node fires a native blur, which ProseMirror turns into one last
  transaction synchronously during that same teardown pass, crashing
  with state_unsafe_mutation. Deferred the onTransaction write to a
  microtask (checked against a destroyed flag) so it lands after
  Svelte's current pass finishes; guarded onUpdate synchronously.

- openEditor/openNoteById in NotesView.svelte awaited a network fetch
  before ever showing the editor, for every note open (not just
  link-driven navigation) — a regression for an offline-first app, and
  the source of a real race: two rapid note-link clicks left two
  fetches in flight with no ordering guarantee, so whichever resolved
  last won, not whichever was clicked last. Reverted to opening
  instantly from local/IndexedDB data (with a request-id guard against
  the residual race), and instead fetch backlinks lazily in
  NoteEditor.svelte only if not already present — mirroring the
  existing attachments-fetch pattern already in that file. Also
  applied the same post-teardown guard to that pre-existing attachments
  effect, since remounting on note-switch newly exposes it to the same
  class of race.

- FormattingToolbar's closeDropdowns() cleared search results but never
  cancelled the pending debounce timer, so a stale query could
  repopulate results after the dropdown had already closed. Added the
  missing clearTimeout, plus a try/catch around the search fetch for
  consistency with the pattern used elsewhere in the app.

Verified: pnpm check (0 errors), full unit suite (316/316), and the
existing e2e suite including all note-linking scenarios (19/19) all
pass against these changes.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
---
 src/lib/components/FormattingToolbar.svelte | 15 ++++--
 src/lib/components/NoteEditor.svelte        | 54 +++++++++++++++++++--
 src/lib/components/NotesView.svelte         | 53 +++++++++-----------
 src/lib/components/TiptapEditor.svelte      | 17 ++++++-
 4 files changed, 96 insertions(+), 43 deletions(-)

diff --git a/src/lib/components/FormattingToolbar.svelte b/src/lib/components/FormattingToolbar.svelte
index 24b1474..60a2bcf 100644
--- a/src/lib/components/FormattingToolbar.svelte
+++ b/src/lib/components/FormattingToolbar.svelte
@@ -62,11 +62,15 @@
 			noteSearchResults = [];
 			return;
 		}
-		const res = await fetch(`/api/search?q=${encodeURIComponent(query)}`);
-		if (requestId !== noteSearchRequestId) return;
-		if (!res.ok) return;
-		const results: { id: string; title: string }[] = await res.json();
-		noteSearchResults = results.filter((n) => n.id !== currentNoteId);
+		try {
+			const res = await fetch(`/api/search?q=${encodeURIComponent(query)}`);
+			if (requestId !== noteSearchRequestId) return;
+			if (!res.ok) return;
+			const results: { id: string; title: string }[] = await res.json();
+			noteSearchResults = results.filter((n) => n.id !== currentNoteId);
+		} catch {
+			// Search failed silently
+		}
 	}
 
 	let noteSearchRequestId = 0;
@@ -115,6 +119,7 @@
 	function closeDropdowns() {
 		openDropdown = null;
 		dropdownAnchorEl = null;
+		clearTimeout(noteSearchTimer);
 		noteSearchResults = [];
 	}
 
diff --git a/src/lib/components/NoteEditor.svelte b/src/lib/components/NoteEditor.svelte
index 3573a86..404e79e 100644
--- a/src/lib/components/NoteEditor.svelte
+++ b/src/lib/components/NoteEditor.svelte
@@ -3,7 +3,7 @@
 </script>
 
 <script lang="ts">
-	import { onMount } from 'svelte';
+	import { onMount, onDestroy } from 'svelte';
 	import { pushState } from '$app/navigation';
 	import { page } from '$app/state';
 	import ColorPicker from './ColorPicker.svelte';
@@ -19,7 +19,7 @@
 	import { getIsDarkMode } from '$lib/utils/theme.svelte.js';
 	import { getPreferences } from '$lib/stores/preferences.svelte.js';
 	import type { Editor } from '@tiptap/core';
-	import type { Note, NoteColor, Attachment, Collaborator } from '$lib/types/index.js';
+	import type { Note, NoteColor, Attachment, Collaborator, NoteBacklink } from '$lib/types/index.js';
 	import Palette from 'lucide-svelte/icons/palette';
 	import SquareCheck from 'lucide-svelte/icons/square-check';
 	import ImageIcon from 'lucide-svelte/icons/image';
@@ -165,8 +165,22 @@
 		return () => window.removeEventListener('beforeunload', handleBeforeUnload);
 	});
 
+	// Safety net for note-link/backlink navigation: NotesView.svelte remounts this
+	// component (rather than reusing the instance) whenever the user switches to a
+	// different note while the editor is open. That teardown skips the debounced
+	// autosave timer's cleanup (it only clears the timer, it doesn't flush) and
+	// beforeunload (which never fires for an in-SPA unmount), so a very recent edit
+	// could otherwise be silently dropped.
+	onDestroy(() => {
+		if (hasUnsavedChanges() && (title.trim() || content.trim())) {
+			performSave();
+		}
+	});
+
 	// svelte-ignore state_referenced_locally
 	let attachmentsList = $state<Attachment[]>(note?.attachments ?? []);
+	// svelte-ignore state_referenced_locally
+	let backlinksList = $state<NoteBacklink[]>(note?.backlinks ?? []);
 
 	let viewportHeight = $state('100dvh');
 	let viewportTop = $state('0px');
@@ -326,15 +340,45 @@
 		}
 	}
 
-	// Fetch attachments for existing notes if not pre-populated (e.g. loaded from IDB)
+	// Fetch attachments for existing notes if not pre-populated (e.g. loaded from IDB).
+	// Guarded against post-teardown writes: since NotesView.svelte now remounts this
+	// component on every note-switch (rather than reusing the instance), a fetch that
+	// resolves after teardown would otherwise write to $state on an already-destroyed
+	// instance racing the newly-mounted one.
 	$effect(() => {
 		if (noteId && !currentlyNew && (!note?.attachments || note.attachments.length === 0)) {
+			let destroyed = false;
 			fetch(`/api/notes/${noteId}/attachments`)
 				.then((res) => res.ok ? res.json() : [])
 				.then((data: Attachment[]) => {
+					if (destroyed) return;
 					if (data.length > 0) attachmentsList = data;
 				})
 				.catch(() => {});
+			return () => {
+				destroyed = true;
+			};
+		}
+	});
+
+	// Fetch backlinks for existing notes if not pre-populated (e.g. loaded from IDB) —
+	// note.backlinks is only ever attached server-side by getNote(), which nothing
+	// else in the client calls when opening a note (the bulk notes store and
+	// IndexedDB never carry it), so this mirrors the attachments effect above to
+	// fill it in on open. Same post-teardown guard as above.
+	$effect(() => {
+		if (noteId && !currentlyNew && (!note?.backlinks || note.backlinks.length === 0)) {
+			let destroyed = false;
+			fetch(`/api/notes/${noteId}`)
+				.then((res) => res.ok ? res.json() : null)
+				.then((data: Note | null) => {
+					if (destroyed) return;
+					if (data?.backlinks && data.backlinks.length > 0) backlinksList = data.backlinks;
+				})
+				.catch(() => {});
+			return () => {
+				destroyed = true;
+			};
 		}
 	});
 
@@ -619,11 +663,11 @@
 		{/if}
 
 		<!-- Backlinks -->
-		{#if note?.backlinks && note.backlinks.length > 0}
+		{#if backlinksList.length > 0}
 			<div class="border-t border-[var(--border-subtle)] px-4 py-2 shrink-0">
 				<p class="mb-1 text-xs font-medium text-[var(--text-muted)]">Referenced by</p>
 				<div class="flex flex-wrap gap-1.5">
-					{#each note.backlinks as backlink (backlink.id)}
+					{#each backlinksList as backlink (backlink.id)}
 						<button
 							type="button"
 							onclick={() => onOpenNote(backlink.id)}
diff --git a/src/lib/components/NotesView.svelte b/src/lib/components/NotesView.svelte
index 2abca02..6ae9981 100644
--- a/src/lib/components/NotesView.svelte
+++ b/src/lib/components/NotesView.svelte
@@ -32,18 +32,8 @@
 	// Use SvelteKit's replaceState (not the raw history API) so the router keeps
 	// ownership of the entry — NoteEditor's close-on-back relies on popstate being
 	// handled by SvelteKit.
-	async function openEditor(note: Note) {
-		try {
-			const res = await fetch(`/api/notes/${note.id}`);
-			if (res.ok) {
-				const full = await res.json();
-				editingNote = { ...note, backlinks: full.backlinks ?? [] };
-			} else {
-				editingNote = note;
-			}
-		} catch {
-			editingNote = note;
-		}
+	function openEditor(note: Note) {
+		editingNote = note;
 		replaceState(`#${note.id}`, {});
 	}
 
@@ -54,25 +44,18 @@
 		replaceState(location.pathname, {});
 	}
 
-	async function openNoteById(noteId: string) {
-		showNewNote = false;
-		newNoteChecklist = false;
-		editingNote = null;
+	// Reads from the local IndexedDB cache rather than fetching over the network —
+	// this keeps note-link navigation instant and offline-safe (matching how the
+	// rest of the app opens notes), and backlinks are filled in progressively by
+	// NoteEditor's own fetch-if-not-pre-populated effect once the editor is open,
+	// the same pattern already used there for attachments.
+	let openNoteRequestId = 0;
 
-		try {
-			const res = await fetch(`/api/notes/${noteId}`);
-			if (res.ok) {
-				const full = await res.json();
-				editingNote = full;
-				replaceState(`#${noteId}`, {});
-			}
-		} catch {
-			const note = await getIdbNote(noteId);
-			if (note) {
-				editingNote = note;
-				replaceState(`#${noteId}`, {});
-			}
-		}
+	async function openNoteById(noteId: string) {
+		const requestId = ++openNoteRequestId;
+		const note = await getIdbNote(noteId);
+		if (requestId !== openNoteRequestId) return; // a newer navigation superseded this one
+		if (note) openEditor(note);
 	}
 
 	function handleReorder(noteIds: string[]) {
@@ -173,5 +156,13 @@
 {/if}
 
 {#if editingNote}
-	<NoteEditor note={editingNote} onClose={closeEditor} onOpenNote={openNoteById} />
+	<!-- Forces a full destroy+remount on note-switch (e.g. clicking a note-link
+	     while the editor is already open) instead of reusing the same instance
+	     with a new `note` prop — a persisting instance's own back-navigation
+	     effect misreads `openEditor`'s replaceState call as "browser back was
+	     pressed" and closes the editor, and its once-only local state never
+	     resyncs to the new note otherwise. -->
+	{#key editingNote.id}
+		<NoteEditor note={editingNote} onClose={closeEditor} onOpenNote={openNoteById} />
+	{/key}
 {/if}
diff --git a/src/lib/components/TiptapEditor.svelte b/src/lib/components/TiptapEditor.svelte
index 4df4d07..3f79d14 100644
--- a/src/lib/components/TiptapEditor.svelte
+++ b/src/lib/components/TiptapEditor.svelte
@@ -108,12 +108,25 @@
 				],
 				content,
 				onUpdate: ({ editor: e }) => {
+					if (destroyed) return;
 					// eslint-disable-next-line @typescript-eslint/no-explicit-any
 					onUpdate((e.storage as Record<string, any>).markdown.getMarkdown());
 				},
 				onTransaction: () => {
-					editor = editorInstance;
-					onTransaction?.();
+					// Removing this component's DOM node (e.g. a {#key}-driven remount
+					// in NotesView.svelte when switching to a different note) fires a
+					// native blur on the still-focused contenteditable, which
+					// ProseMirror's blur plugin turns into one last transaction —
+					// synchronously, from inside Svelte's own render/effect pass that's
+					// doing the removal. Mutating $state right there crashes with
+					// state_unsafe_mutation; the `destroyed` check alone isn't enough
+					// since this can fire before onMount's own cleanup runs. Defer to a
+					// microtask so the write lands after Svelte's current pass finishes.
+					queueMicrotask(() => {
+						if (destroyed) return;
+						editor = editorInstance;
+						onTransaction?.();
+					});
 				}
 			});
 			editor = editorInstance;