From ba3033e929d3186f963c6b7bcff4806158572729 Mon Sep 17 00:00:00 2001 From: Dario Mory Date: Sat, 5 Sep 2026 02:56:30 +0700 Subject: [PATCH] fix(editor): never save a document under a mismatched note id The editor tracked which note a keystroke belonged to in a ref, resynced only by an effect, while the store held the authoritative id. When the two fell out of step the autosave wrote the open document over a different note, replacing its body. - guard the save path on the store's active id, the check the bubble helpers in the same file already make - stop reloadExternal acting on an `active` captured before its awaits, which could reassign the open note behind the editor's back - archive a note's previous text before any save that discards most of it, under .sudonotes/versions/, so a bad write is never unrecoverable --- app/src-tauri/src/lib.rs | 14 +++- app/src-tauri/src/versions.rs | 129 ++++++++++++++++++++++++++++++++++ app/src/components/Editor.tsx | 47 ++++++++----- app/src/store.ts | 21 ++++-- 4 files changed, 189 insertions(+), 22 deletions(-) create mode 100644 app/src-tauri/src/versions.rs diff --git a/app/src-tauri/src/lib.rs b/app/src-tauri/src/lib.rs index 551183f..62c9c9d 100644 --- a/app/src-tauri/src/lib.rs +++ b/app/src-tauri/src/lib.rs @@ -7,6 +7,7 @@ mod note; mod project; mod split; mod vault; +mod versions; mod watcher; use std::collections::{BTreeMap, HashSet}; @@ -707,9 +708,13 @@ fn update_note( .filter(|t| !t.is_empty()) .collect(); note.frontmatter.model = model.map(|m| m.trim().to_string()).filter(|m| !m.is_empty()); - note.body = format!("\n{}\n", body.trim()); + let before = std::mem::replace(&mut note.body, format!("\n{}\n", body.trim())); note.frontmatter.updated = note::now_rfc3339(); + if versions::is_destructive(&before, ¬e.body) { + versions::snapshot(&open.vault.root, &id, &content); + } + note.write_to(&path) .map_err(|e| err("could not write note", e))?; @@ -1694,9 +1699,16 @@ fn save(state: &State, id: &str, edit: impl FnOnce(&mut Note)) -> Resu let content = std::fs::read_to_string(&path).map_err(|e| err("could not read note", e))?; let mut note = Note::parse(&content, &title_from_path(&path)); + let before = note.body.clone(); edit(&mut note); note.frontmatter.updated = note::now_rfc3339(); + // A save that drops most of a note is a rewrite or a bug, and the note + // is the only copy either way. Keep the old text before overwriting it. + if versions::is_destructive(&before, ¬e.body) { + versions::snapshot(&open.vault.root, id, &content); + } + note.write_to(&path) .map_err(|e| err("could not write note", e))?; sync_mirror(¬e); diff --git a/app/src-tauri/src/versions.rs b/app/src-tauri/src/versions.rs new file mode 100644 index 0000000..fe3128c --- /dev/null +++ b/app/src-tauri/src/versions.rs @@ -0,0 +1,129 @@ +//! Last-resort snapshots of a note's previous text. +//! +//! A note is the user's only copy. Editing shrinks and grows a body a line at a +//! time, so a save that discards most of one is not typing — it is a +//! replacement, and the only cases that produce one are a deliberate rewrite +//! and a bug. The rewrite is cheap to archive; the bug is unrecoverable without +//! it, so the previous text is kept before the write either way. +//! +//! This module only ever *adds* files under the vault's own `.sudonotes/` +//! folder. It never edits, moves or deletes a note, and the only files it +//! removes are its own older snapshots for the same note. + +use std::path::{Path, PathBuf}; + +use crate::vault::INDEX_DIR; + +const VERSIONS_DIR: &str = "versions"; + +/// Bodies below this are too small for "half of it went" to mean anything — +/// clearing a two-line note is ordinary editing. +const MIN_BODY_BYTES: usize = 200; + +/// How much of a body a single save may drop before it is archived first. +const KEEP_RATIO: f64 = 0.5; + +/// Snapshots kept per note, newest first. Markdown is tiny; this is generous. +const KEEP_PER_NOTE: usize = 20; + +/// Whether replacing `before` with `after` discards enough of the note to be +/// worth keeping the old text. +pub fn is_destructive(before: &str, after: &str) -> bool { + let before = before.trim(); + if before.len() < MIN_BODY_BYTES { + return false; + } + (after.trim().len() as f64) < (before.len() as f64) * KEEP_RATIO +} + +fn versions_dir(vault_root: &Path, id: &str) -> PathBuf { + vault_root.join(INDEX_DIR).join(VERSIONS_DIR).join(id) +} + +/// Archive `raw` — the note's full file contents as they are on disk right now +/// — before it is overwritten. Best effort: failing to keep a snapshot must +/// never be the reason a save fails. +pub fn snapshot(vault_root: &Path, id: &str, raw: &str) { + let dir = versions_dir(vault_root, id); + if std::fs::create_dir_all(&dir).is_err() { + return; + } + + // Sortable and filename-safe, so rotation can order by name alone. + let stamp = crate::note::now_rfc3339().replace(':', "-"); + let path = dir.join(format!("{stamp}.md")); + // A second save inside the same second must not clobber the first snapshot. + let path = if path.exists() { + dir.join(format!("{stamp}-{}.md", ulid::Ulid::generate())) + } else { + path + }; + if std::fs::write(&path, raw).is_err() { + return; + } + + rotate(&dir); +} + +/// Keep the newest `KEEP_PER_NOTE` snapshots for one note and drop the rest. +fn rotate(dir: &Path) { + let Ok(entries) = std::fs::read_dir(dir) else { + return; + }; + let mut files: Vec = entries + .flatten() + .map(|e| e.path()) + .filter(|p| p.is_file() && p.extension().is_some_and(|ext| ext == "md")) + .collect(); + if files.len() <= KEEP_PER_NOTE { + return; + } + files.sort(); + let cutoff = files.len() - KEEP_PER_NOTE; + for path in &files[..cutoff] { + let _ = std::fs::remove_file(path); + } +} + +#[cfg(test)] +mod tests { + use super::*; + + fn body(n: usize) -> String { + "idea bubble text\n".repeat(n) + } + + #[test] + fn ordinary_editing_is_not_destructive() { + let before = body(30); + // A few lines removed by hand. + let after = body(28); + assert!(!is_destructive(&before, &after)); + } + + #[test] + fn short_notes_are_never_destructive() { + assert!(!is_destructive("a short note", "")); + } + + #[test] + fn losing_most_of_a_long_note_is_destructive() { + assert!(is_destructive(&body(75), &body(3))); + } + + #[test] + fn snapshot_writes_and_rotates() { + let root = std::env::temp_dir().join(format!("sudonotes-versions-{}", ulid::Ulid::generate())); + std::fs::create_dir_all(root.join(INDEX_DIR)).unwrap(); + + for n in 0..KEEP_PER_NOTE + 5 { + snapshot(&root, "note-id", &format!("version {n}")); + } + + let dir = versions_dir(&root, "note-id"); + let kept = std::fs::read_dir(&dir).unwrap().count(); + assert_eq!(kept, KEEP_PER_NOTE); + + std::fs::remove_dir_all(&root).ok(); + } +} diff --git a/app/src/components/Editor.tsx b/app/src/components/Editor.tsx index 60c7541..8fd69d9 100644 --- a/app/src/components/Editor.tsx +++ b/app/src/components/Editor.tsx @@ -3071,24 +3071,39 @@ export function Editor() { } if (!update.docChanged || applying.current) return; const id = activeId.current; - if (id) { - const docStr = update.state.doc.toString(); - const scrollTop = update.view.scrollDOM.scrollTop; - const vaultPath = useStore.getState().vaultPath ?? ""; - - editorStateCache.set(id, { - state: update.state, - scrollTop, - updatedAt: Date.now(), - }); + const active = useStore.getState().active; + // The ref and the store are updated by separate paths, so they can fall + // out of step for a tick. Persisting the document under a mismatched id + // writes one note's text over another's, so nothing is filed under `id` + // until the store agrees it is the note on screen. The reload effect + // resyncs the document, and the keystroke is re-saved from there. + if (!id || !active || active.id !== id) { + if (id && active) { + console.warn( + "sudonotes: dropped a save for", + id, + "while the open note was", + active.id, + ); + } + return; + } - saveHistoryDebounced(vaultPath, id, update.state, scrollTop); - useStore.getState().queueSave(id, docStr); + const docStr = update.state.doc.toString(); + const scrollTop = update.view.scrollDOM.scrollTop; + const vaultPath = useStore.getState().vaultPath ?? ""; - const active = useStore.getState().active; - if (active && active.id === id && active.body !== docStr) { - useStore.setState({ active: { ...active, body: docStr } }); - } + editorStateCache.set(id, { + state: update.state, + scrollTop, + updatedAt: Date.now(), + }); + + saveHistoryDebounced(vaultPath, id, update.state, scrollTop); + useStore.getState().queueSave(id, docStr); + + if (active.body !== docStr) { + useStore.setState({ active: { ...active, body: docStr } }); } }), ]; diff --git a/app/src/store.ts b/app/src/store.ts index b658fa3..f188b80 100644 --- a/app/src/store.ts +++ b/app/src/store.ts @@ -1093,9 +1093,18 @@ export const useStore = create((set, get) => ({ const active = get().active; if (!active) return; + const id = active.id; try { - const fresh = await api.readNote(active.id); + const fresh = await api.readNote(id); + + // The read is a round trip, and the user can pick another note while it + // is in flight. Writing the note captured above back into `active` would + // then reopen a note they have already left — and, worse, leave the + // editor holding one note's text under another's id. Anything stale is + // simply dropped; the note that is open now has its own refresh. + const current = get().active; + if (!current || current.id !== id) return; // Mid-edit, the text on disk is older than what is on screen, so it must // not be taken. The issue states are not the user's to edit and change @@ -1103,7 +1112,7 @@ export const useStore = create((set, get) => ({ // lands between two keystrokes is simply lost, and a bubble keeps showing // a state its issue left minutes ago. if (get().dirty) { - set({ active: { ...active, issueStates: fresh.issueStates } }); + set({ active: { ...current, issueStates: fresh.issueStates } }); return; } @@ -1112,11 +1121,13 @@ export const useStore = create((set, get) => ({ // note on a text change left the editor showing a stale issue state // forever. Only the doc version is gated, because bumping it reloads // CodeMirror and moves the cursor. - const textChanged = fresh.body !== active.body || fresh.title !== active.title; + const textChanged = fresh.body !== current.body || fresh.title !== current.title; set(textChanged ? { active: fresh, docVersion: get().docVersion + 1 } : { active: fresh }); } catch { - // The note was deleted or moved while it was open. - set({ active: null, backlinks: [] }); + // The note was deleted or moved while it was open — unless the user has + // already moved on, in which case this says nothing about what is open. + const current = get().active; + if (current?.id === id) set({ active: null, backlinks: [] }); } },