From 23a7b36d0d20ef1f809db0b5d69f1416021afb91 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Fri, 16 Oct 2026 20:58:18 +0200 Subject: notes: write and commit failures are reported, not assumed A review of the branch found four ways the page could report success for work that never happened, all reachable in ordinary use. Writes: setText is asynchronous unless blockWrites, and its saved()/saveFailed() do not fire for every write (a second setText to the same path loses the signal, which stranded the callback), so a save was reported before it landed and a failed db.json or body write was never seen. Both views now block and carry the outcome, so writeDb and writeBody know the result when they return; a failed create therefore leaves no index entry. The create path also loads the body before the index, so a failed body write cannot leave an entry with no text. Commits: the sync chain swallowed every commit failure with '|| true', so a failing pre-commit hook or a signing failure pushed an unchanged HEAD and reported success. It now stages, commits only when something is staged, and lets 'set -e' fail the chain, with stderr folded in for the whole chain rather than just the push. Duplicate notes: the 'awaiting commit' state lived only in a 3 second timer, so a slower second click re-entered create() and appended a second note. It is now a page property that outlives the timer, and the button follows it. Preview: the shared Switch writes checked itself, dropping the binding, so the switch stopped following page.preview; resynced on change, the way StatusRow.qml does. Also validates a note's file name as digits before read, write or rm, and clears the index error once the index reads again. --- desktop/modules/notes/Notes.js | 9 ++ desktop/modules/notes/NotesModule.qml | 128 +++++++++++++++------- desktop/modules/notes/NotesPage.qml | 72 ++++++------ desktop/modules/notes/README.md | 4 + desktop/modules/notes/test_notes.js | 13 ++- docs/superpowers/specs/2026-10-16-notes-design.md | 4 + 6 files changed, 157 insertions(+), 73 deletions(-) diff --git a/desktop/modules/notes/Notes.js b/desktop/modules/notes/Notes.js index 6bc8c88..4e2ef3d 100644 --- a/desktop/modules/notes/Notes.js +++ b/desktop/modules/notes/Notes.js @@ -44,6 +44,15 @@ function parse(text) { return valid(text) || { notes: [] }; } +// A note's file name as db.json should hold it: digits only, the epoch second +// notes.sh names the file with. Anything else is refused, because the page +// builds a read path, a write path and an rm path from it, and a db.json that +// came off a git remote is not entirely under this user's control. +function validFile(file) { + const f = String(file === undefined || file === null ? "" : file); + return /^\d+$/.test(f) ? f : ""; +} + // Pretty, 2-space, trailing newline. notes.sh reads db.json through jq, which // does not care about the layout. function stringify(db) { diff --git a/desktop/modules/notes/NotesModule.qml b/desktop/modules/notes/NotesModule.qml index 94ef41e..082f02c 100644 --- a/desktop/modules/notes/NotesModule.qml +++ b/desktop/modules/notes/NotesModule.qml @@ -83,21 +83,28 @@ Module { mod.client = l[4] || ""; } - // The store, watched rather than read on open: the tile's count has to - // move when a note is added from either side, the CLI included. A missing - // file is an empty store, not an error worth logging. - // - // fileChanged means the file was replaced, and text() still holds the old - // contents until the reload lands, which is why the reparse lives on - // onLoaded and not on fileChanged (the same split Prefs.qml uses). + // Writing is finished when setText returns. setText is asynchronous unless + // blockWrites is set, and its saved()/saveFailed() cannot be relied on to + // fire for every write (a second setText to the same path loses the + // signal), which would strand a callback and, worse, report a save that + // never landed. blockWrites makes the write complete before returning and + // saveError carries the outcome. The file is tiny and the write happens on + // an explicit click, so the blocking is not felt. property FileView dbView: FileView { path: mod.dbPath atomicWrites: true + blockWrites: true watchChanges: true printErrors: false + property string saveError: "" onFileChanged: reload() onLoaded: mod.reparse() onLoadFailed: mod.reparse() + onSaved: saveError = "" + onSaveFailed: error => { + console.warn("notes: db.json write failed:", error); + saveError = "db.json could not be written"; + } } // Nothing to read until the config has resolved, so the load failed on the @@ -106,8 +113,13 @@ Module { function reparse() { if (mod.dbPath === "") return; const v = Notes.valid(mod.dbView.text()); - if (v !== null) mod.db = v; - else mod.lastError = mod.indexError(); + if (v !== null) { + mod.db = v; + // Once the index is readable again its own complaint is stale. + if (mod.lastError === mod.indexError()) mod.lastError = ""; + } else { + mod.lastError = mod.indexError(); + } mod.ready = true; } @@ -127,11 +139,15 @@ Module { return Notes.valid(mod.dbView.text()); } - // db.json, written atomically. mod.db is assigned first so the page does - // not wait for the watch to come back round. - function writeDb(db) { + // db.json, written atomically and blocking, so the outcome is known when + // this returns. mod.db is assigned first so the page does not wait for the + // watch to come back round. + function writeDb(db, done) { mod.db = db; + mod.dbView.saveError = ""; mod.dbView.setText(Notes.stringify(db)); + const ok = mod.dbView.saveError === ""; + if (done) done(ok, ok ? "" : mod.dbView.saveError); } // A note body. notes.sh keeps the title out of the file, so this is the @@ -139,24 +155,40 @@ Module { // prior load; reading the file back is what would need one, and nothing // here reads it back. property FileView bodyView: FileView { + blockWrites: true printErrors: false + property string saveError: "" + onSaved: saveError = "" + onSaveFailed: error => { + console.warn("notes: note body write failed:", error); + saveError = "the note's file could not be written"; + } } function writeBody(path, text, done) { mod.bodyView.path = path; + mod.bodyView.saveError = ""; mod.bodyView.setText(text); - if (done) done(true, ""); + const ok = mod.bodyView.saveError === ""; + if (done) done(ok, ok ? "" : mod.bodyView.saveError); + } + + // The path of a note's body, "" when the entry's file is not a plain + // epoch-second name: this path is read, written and rm'd, and db.json may + // have come off a remote. + function bodyPath(file) { + const f = Notes.validFile(file); + return f === "" ? "" : mod.notesDir + "/" + f; } function fileIn(db, id) { - for (const n of (db.notes || [])) if (n.id === id) return n.file || ""; + for (const n of (db.notes || [])) if (n.id === id) return Notes.validFile(n.file); return ""; } // --- git --------------------------------------------------------------- // - // Both shells fold stderr into stdout with 2>&1, so one collector holds - // the reason. onExited must not read the collector directly: its + // Both shells fold stderr into stdout, so one collector holds the reason. onExited must not read the collector directly: its // onStreamFinished can fire after the process exits. The collector stores // the text in mod.gitOutput and onExited reads that and the exit code. // The two processes never run at once, so sharing gitOutput is safe. @@ -214,8 +246,7 @@ Module { if (code !== 0) { if (done) done(false, mod.gitOutput); return; } const db = mod.readDbNow(); if (db === null) { if (done) done(false, mod.indexError()); return; } - mod.writeDb(Notes.setLastpull(db, Math.floor(Date.now() / 1000))); - if (done) done(true, ""); + mod.writeDb(Notes.setLastpull(db, Math.floor(Date.now() / 1000)), done); } // Create writes the note after the pull, on the db.json the pull brought @@ -235,13 +266,18 @@ Module { if (db === null) { if (done) done(false, mod.indexError()); return; } const now = Math.floor(Date.now() / 1000); const file = String(now); - let next = Notes.addNote(db, title, file); - // Only when a pull actually happened: with git off there was none, and - // claiming one would suppress notes.sh's own pull later. - if (code === 0 && mod.gitEnabled) next = Notes.setLastpull(next, now); - mod.lastCreatedId = next.notes[next.notes.length - 1].id; - mod.writeDb(next); - mod.writeBody(mod.notesDir + "/" + file, body, () => { if (done) done(true, ""); }); + // The body first. A body write that fails then leaves no index entry + // behind, so the store stays consistent and a retry is not a + // duplicate; the other order leaves an entry with no text to show. + mod.writeBody(mod.notesDir + "/" + file, body, (ok, msg) => { + if (!ok) { if (done) done(false, msg); return; } + let next = Notes.addNote(db, title, file); + // Only when a pull actually happened: with git off there was none, + // and claiming one would suppress notes.sh's own pull later. + if (code === 0 && mod.gitEnabled) next = Notes.setLastpull(next, now); + mod.lastCreatedId = next.notes[next.notes.length - 1].id; + mod.writeDb(next, done); + }); // shortcut: second-granularity filenames, as notes.sh takes date +%s, // so two notes in one second would share a file. Upgrade to a fresh // second or a suffix if that is ever seen. @@ -255,20 +291,27 @@ Module { if (db === null) { if (done) done(false, mod.indexError()); return; } const file = mod.fileIn(db, id); if (file === "") { if (done) done(false, "note not found"); return; } - mod.writeDb(Notes.updateNote(db, id, title)); - mod.writeBody(mod.notesDir + "/" + file, body, () => { if (done) done(true, ""); }); + mod.writeBody(mod.notesDir + "/" + file, body, (ok, msg) => { + if (!ok) { if (done) done(false, msg); return; } + mod.writeDb(Notes.updateNote(db, id, title), done); + }); } - // Delete drops the entry and removes the body file. FileView has no - // delete, so this is the module's one filesystem call. + // Delete drops the entry and removes the body file. The index goes first: + // it is the record, so a failure there must leave the note alone. FileView + // has no delete, so the body removal is the module's one filesystem call, + // detached, so its failure is not observed; a body left behind reads as a + // note without a title, not as a lost note. function remove(id, done) { if (!mod.ready) { if (done) done(false, "notes not loaded yet"); return; } const db = mod.readDbNow(); if (db === null) { if (done) done(false, mod.indexError()); return; } const file = mod.fileIn(db, id); - mod.writeDb(Notes.removeNote(db, id)); - if (file !== "") Quickshell.execDetached(["rm", "--", mod.notesDir + "/" + file]); - if (done) done(true, ""); + mod.writeDb(Notes.removeNote(db, id), (ok, msg) => { + if (!ok) { if (done) done(false, msg); return; } + if (file !== "") Quickshell.execDetached(["rm", "--", mod.notesDir + "/" + file]); + if (done) done(true, ""); + }); } function msg(kind, id) { @@ -277,17 +320,28 @@ Module { return "quickshell - " + mod.client + " removing note ID " + id + "."; } - // The second click of Save or Delete: add everything, commit (a commit - // with nothing staged is not a failure), push. No pull here; creating - // already pulled. + // The second click of Save or Delete: stage everything, commit when there + // is something staged, push. No pull here; creating already pulled. + // + // `exec 2>&1` folds stderr into stdout for the whole chain so the one + // collector holds the reason, and `set -e` makes a failing commit (a + // pre-commit hook, a signing failure) or push exit non-zero instead of + // being reported as a successful sync. "Nothing staged" is tested for + // rather than swallowed with `|| true`, which would hide exactly those + // failures and push an unchanged HEAD. function commitPush(kind, id, done) { if (!mod.gitEnabled) { if (done) done(true, ""); return; } - if (mod.syncing) return; + if (mod.syncing) { if (done) done(false, "a sync is already running"); return; } mod.syncing = true; mod.lastError = ""; mod.gitSync.done = done; mod.gitSync.command = ["sh", "-c", - 'git -C "$1" add . && { git -C "$1" commit -m "$2" || true; } && git -C "$1" push origin HEAD 2>&1', + 'set -e\n' + + 'exec 2>&1\n' + + 'cd "$1"\n' + + 'git add .\n' + + 'if ! git diff --cached --quiet; then git commit -m "$2"; fi\n' + + 'git push origin HEAD\n', "sh", mod.baseDir, mod.msg(kind, id)]; mod.gitSync.running = false; mod.gitSync.running = true; diff --git a/desktop/modules/notes/NotesPage.qml b/desktop/modules/notes/NotesPage.qml index a3d11f6..88382fc 100644 --- a/desktop/modules/notes/NotesPage.qml +++ b/desktop/modules/notes/NotesPage.qml @@ -37,12 +37,19 @@ Item { property string bodyError: "" // A write or git command is in flight; the buttons stand down meanwhile. property bool busy: false + // "create" | "edit" | "delete" once that write has landed and the commit + // and push are still owed. It outlives any timer, so a click after a pause + // pushes the note that was already written instead of writing it again. + property string awaiting: "" + + // The shared Switch writes `checked` itself, which drops the binding + // above; StatusRow.qml documents the same and resyncs the same way. + onPreviewChanged: prevSwitch.checked = page.preview function open(note) { page.bodyError = ""; page.preview = note !== null; - confirm.stop(); - confirmDel.stop(); + page.awaiting = ""; titleField.text = note ? note.title : ""; // Clear before the assignment below: that assignment is what retargets // the body FileView, and its load may land synchronously, so clearing @@ -54,8 +61,7 @@ Item { } function toList() { - confirm.stop(); - confirmDel.stop(); + page.awaiting = ""; page.busy = false; page.editing = null; page.bodyError = ""; @@ -64,27 +70,27 @@ Item { function save() { if (page.busy) return; - const done = (ok, msg) => { - page.busy = false; - if (!ok) { page.owner.lastError = msg; return; } - // Git off: there is no second half, so this was the whole save. - if (!page.owner.gitEnabled) { page.toList(); return; } - confirm.start(); - }; - if (confirm.running) { - confirm.stop(); - const kind = page.editing ? "edit" : "create"; - const id = page.editing ? page.editing.id : page.owner.lastCreatedId; + // The note is already on disk and only the push is owed, so this click + // pushes rather than writing a second note. + if (page.awaiting !== "") { + const id = page.awaiting === "create" ? page.owner.lastCreatedId : page.editing.id; page.busy = true; - page.owner.commitPush(kind, id, (ok, msg) => { + page.owner.commitPush(page.awaiting, id, (ok, msg) => { page.busy = false; - if (!ok) page.owner.lastError = msg; + if (!ok) { page.owner.lastError = msg; return; } page.toList(); }); return; } page.owner.lastError = ""; page.busy = true; + const done = (ok, msg) => { + page.busy = false; + if (!ok) { page.owner.lastError = msg; return; } + // Git off: there is no second half, so this was the whole save. + if (!page.owner.gitEnabled) { page.toList(); return; } + page.awaiting = page.editing ? "edit" : "create"; + }; if (page.editing) page.owner.update(page.editing.id, titleField.text, bodyEdit.text, done); else page.owner.create(titleField.text, bodyEdit.text, done); } @@ -92,25 +98,23 @@ Item { function remove() { if (page.busy || page.editing === null) return; const id = page.editing.id; - const done = (ok, msg) => { - page.busy = false; - if (!ok) { page.owner.lastError = msg; return; } - if (!page.owner.gitEnabled) { page.toList(); return; } - confirmDel.start(); - }; - if (confirmDel.running) { - confirmDel.stop(); + if (page.awaiting === "delete") { page.busy = true; page.owner.commitPush("delete", id, (ok, msg) => { page.busy = false; - if (!ok) page.owner.lastError = msg; + if (!ok) { page.owner.lastError = msg; return; } page.toList(); }); return; } page.owner.lastError = ""; page.busy = true; - page.owner.remove(id, done); + page.owner.remove(id, (ok, msg) => { + page.busy = false; + if (!ok) { page.owner.lastError = msg; return; } + if (!page.owner.gitEnabled) { page.toList(); return; } + page.awaiting = "delete"; + }); } function updateList() { @@ -128,7 +132,7 @@ Item { FileView { id: bodyRead path: (page.view === "editor" && page.editing) - ? page.owner.notesDir + "/" + page.editing.file : "" + ? page.owner.bodyPath(page.editing.file) : "" printErrors: false onLoaded: { bodyEdit.text = text(); page.bodyError = ""; } onLoadFailed: { @@ -137,8 +141,6 @@ Item { } } - Timer { id: confirm; interval: 3000 } - Timer { id: confirmDel; interval: 3000 } Timer { id: updated; interval: 2000 } // --- list -------------------------------------------------------------- @@ -276,9 +278,9 @@ Item { Button { id: saveBtn anchors { top: parent.top; right: parent.right } - enabled: !page.busy || confirm.running - text: page.busy && !confirm.running ? "Saving…" - : confirm.running ? "Commit & push?" : "Save" + enabled: !page.busy + text: page.busy ? "Saving…" + : page.awaiting !== "" ? "Commit & push?" : "Save" onClicked: page.save() } @@ -287,8 +289,8 @@ Item { anchors { top: parent.top; right: saveBtn.left; rightMargin: 8 } visible: page.editing !== null danger: true - enabled: !page.busy || confirmDel.running - text: confirmDel.running ? "Confirm?" : "Delete" + enabled: !page.busy + text: page.awaiting === "delete" ? "Confirm?" : "Delete" onClicked: page.remove() } diff --git a/desktop/modules/notes/README.md b/desktop/modules/notes/README.md index fa1a537..8efd7bc 100644 --- a/desktop/modules/notes/README.md +++ b/desktop/modules/notes/README.md @@ -37,6 +37,10 @@ git to resolve instead. A change made on two machines at once conflicts here for the same reason it conflicts for `notes.sh`: every machine appends to the one `db.json`. +A write that fails is reported rather than assumed: the writes block, so a +refused or failing `setText` is known when it returns, and a failed commit or +push shows git's own message. + **Git order.** `notes.sh` pulls before it writes and commits after, and the page keeps that order: creating a note pulls first, then writes on what the pull brought in, and the commit and push are the second click. The one diff --git a/desktop/modules/notes/test_notes.js b/desktop/modules/notes/test_notes.js index 6a316ce..3f91a5e 100644 --- a/desktop/modules/notes/test_notes.js +++ b/desktop/modules/notes/test_notes.js @@ -18,7 +18,7 @@ const assert = require("assert"); const src = fs.readFileSync(__dirname + "/Notes.js", "utf8") .replace(/^\.pragma library$/m, ""); -const N = new Function(src + "\nreturn { parse, valid, stringify, sorted, formatDate, nextId," +const N = new Function(src + "\nreturn { parse, valid, validFile, stringify, sorted, formatDate, nextId," + " filter, addNote, updateNote, removeNote, setLastpull };")(); const db = { @@ -63,6 +63,17 @@ assert.strictEqual(N.valid('{"a":1}'), null); assert.strictEqual(N.valid("<<<<<<< HEAD\n{}\n=======\n{}\n>>>>>>> other\n"), null); assert.deepStrictEqual(N.parse("not json"), { notes: [] }); // display stays tolerant +// validFile(): the epoch-second name, or "" for anything else. The page builds +// a read path, a write path and an rm path from a note's file, and a db.json +// that came off a git remote is not entirely under this user's control. +assert.strictEqual(N.validFile("1750000000"), "1750000000"); +assert.strictEqual(N.validFile(""), ""); +assert.strictEqual(N.validFile(undefined), ""); +assert.strictEqual(N.validFile("../../.bashrc"), ""); +assert.strictEqual(N.validFile("1750000000\n"), ""); +assert.strictEqual(N.validFile("1e9"), ""); +assert.strictEqual(N.validFile("notes/1750000000"), ""); + // pure transforms preserve everything else and do not mutate const added = N.addNote(db, "T", "1750"); assert.strictEqual(added.notes.length, 4); diff --git a/docs/superpowers/specs/2026-10-16-notes-design.md b/docs/superpowers/specs/2026-10-16-notes-design.md index a9f5420..53b9d2e 100644 --- a/docs/superpowers/specs/2026-10-16-notes-design.md +++ b/docs/superpowers/specs/2026-10-16-notes-design.md @@ -120,6 +120,10 @@ and writing it back would delete every title, so the write instead reports git to resolve. `Notes.valid` is the check; `Notes.parse` stays tolerant for display, returning an empty store for the same input. +A write that fails is reported, not assumed to have landed: both writes block +(`blockWrites`), because `setText` is asynchronous otherwise, and a failure +that nobody observes reports a save that never happened. + - **Create**: the first click pulls first (see Git), then re-reads `db.json` so the append is based on what the pull brought in, not on what was read before it; `notes.sh` reads the id after its own pull for the same reason. -- cgit v1.2.3