mirror of
https://github.com/bretzel-app/crumbs.git
synced 2026-10-12 12:06:57 +02:00
fix: close note-switching race conditions and restore offline-first note open
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>
This commit is contained in:
committed by
Fred Cabassut
co-authored by
Claude Sonnet 5
parent
534d84224d
commit
3eb8d2eeab
@@ -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 = [];
|
||||
}
|
||||
|
||||
|
||||
@@ -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)}
|
||||
|
||||
@@ -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}
|
||||
|
||||
@@ -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;
|
||||
|
||||
Reference in New Issue
Block a user