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/NotesModule.qml | 128 ++++++++++++++++++++++++---------- 1 file changed, 91 insertions(+), 37 deletions(-) (limited to 'desktop/modules/notes/NotesModule.qml') 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; -- cgit v1.2.3