From 1e4ef93b5cc5df93094c2fc7170659f3846c5348 Mon Sep 17 00:00:00 2001 From: Brian Alexander <824344+ironprogrammer@users.noreply.github.com> Date: Sun, 26 Jul 2026 19:47:40 -0700 Subject: [PATCH 1/2] Detect upstream changes before saving MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Troche is often open on more than one machine. Saving was already per song and diff-based, so idle tabs never wrote and songs created elsewhere were never trashed — but a song edited in two places was overwritten by whichever tab saved last, silently, with wp-admin revisions as the only trace. Give the server a per-song version token (a hash of the stored JSON, so an identical re-save doesn't move it) and expose it from a new GET /library/state, which carries tokens only and no content. Clients never compute a token, only compare, so the hash is free to change shape later. The app checks that endpoint when a tab becomes visible — the moment a laptop is most likely stale — and again before each save, closing the debounce window. No background polling. Per-song granularity means most upstream activity isn't a conflict and doesn't need the user: a song changed elsewhere that this tab hasn't touched is adopted, one added elsewhere is pulled in, one trashed elsewhere is dropped. Only a song edited in both places, or edited here and trashed there, surfaces — as a notice naming the song with a choice, since a library-wide "reload" would discard local edits. Those songs are held back from saving until resolved; the rest of the library keeps autosaving. Also fixes an update to a song trashed elsewhere reporting itself as "Offline — changes kept locally". The 404 is now distinguished from a network failure and routed into the same reconcile. --- README.md | 6 + docs/wp-storage-plan.md | 11 +- package.json | 3 +- src/App.jsx | 144 +++++++++- src/components/Header.jsx | 1 + src/components/SyncNotice.jsx | 64 +++++ src/storage.js | 251 ++++++++++++++++-- src/styles.js | 15 ++ tests/README.md | 1 + tests/plugin.php | 36 +++ tests/sync.mjs | 261 +++++++++++++++++++ wp-plugin/includes/class-rest-controller.php | 24 ++ wp-plugin/includes/class-store.php | 78 +++++- wp-plugin/readme.txt | 3 + 14 files changed, 865 insertions(+), 33 deletions(-) create mode 100644 src/components/SyncNotice.jsx create mode 100644 tests/sync.mjs diff --git a/README.md b/README.md index f883a65..715ad35 100644 --- a/README.md +++ b/README.md @@ -79,6 +79,12 @@ each song as a revisioned custom post, and exposes it under a URL slug you choose (default `/troche`). Viewing requires being logged in; editing requires a capability you grant per user on **Settings → Troche**. +Because saving is per song rather than per library, having the app open on two +machines is safe: when a tab comes back into view, and again before it saves, it +checks which songs have moved and folds in anything it hasn't touched itself. +Only a song edited in both places at once needs a decision, and it says so +rather than overwriting quietly. + ### Build ```bash diff --git a/docs/wp-storage-plan.md b/docs/wp-storage-plan.md index c42697b..dc60e69 100644 --- a/docs/wp-storage-plan.md +++ b/docs/wp-storage-plan.md @@ -72,8 +72,15 @@ subpath and the plugin directory. - **Autosave replaces the Save button:** ~10s debounce, fires only when dirty, flush on `visibilitychange`/`pagehide`. Status indicator: "Saved ✓ / Saving… / Offline — changes kept locally." -- **Conflicts: last write wins**; revisions are the safety net. Dirty-only - saving means idle open tabs never clobber. +- **Conflicts: reconcile per song, prompt only when it's genuinely ambiguous.** + Dirty-only saving already means idle open tabs never clobber. On top of that, + the app checks `GET /library/state` (version tokens, no content) when a tab + becomes visible and again before each save. A song changed on another machine + that this tab hasn't touched is adopted silently; one added there is pulled + in; one trashed there is dropped. Only a song edited in *both* places, or + edited here and trashed there, surfaces to the user — those are held back + from saving until resolved, while the rest of the library keeps autosaving. + Whichever copy loses is still recoverable from the song's revisions. - **Share/Export/Import stay functionally untouched** in both modes (one codebase; they're client-side and cost nothing). Export/Import double as the seeding path: each bandmate exports their existing localStorage library and diff --git a/package.json b/package.json index b308b80..ddee09a 100644 --- a/package.json +++ b/package.json @@ -11,8 +11,9 @@ "build": "vite build", "build:wp": "vite build && node scripts/sync-dist.mjs", "preview": "vite preview", - "test": "npm run test:unit && npm run test:plugin && npm run test:zip", + "test": "npm run test:unit && npm run test:sync && npm run test:plugin && npm run test:zip", "test:unit": "node tests/unit.mjs", + "test:sync": "node tests/sync.mjs", "test:plugin": "bash tests/plugin.sh", "test:zip": "bash tests/zip-install.sh" }, diff --git a/src/App.jsx b/src/App.jsx index 071ce12..2aa217a 100644 --- a/src/App.jsx +++ b/src/App.jsx @@ -6,6 +6,9 @@ import { defaultLibrary, defaultSong } from "./defaults.js"; import { loadLibrary, saveLibrary, + syncUpstream, + releaseHold, + forgetHandle, clearBuffer, wpMode, canEdit, @@ -17,6 +20,7 @@ import { Header } from "./components/Header.jsx"; import { Transport } from "./components/Transport.jsx"; import { PartBlock } from "./components/PartBlock.jsx"; import { PrintChart } from "./components/PrintChart.jsx"; +import { SyncNotice } from "./components/SyncNotice.jsx"; import { styles, css } from "./styles.js"; // Matches the CSS mobile breakpoint (see styles.js). Used to drop the @@ -35,6 +39,15 @@ function useIsMobile() { return m; } +// The app renders off an active song, so the library must never reach zero. +// A blank song stands in — the same fallback a fresh, empty install gets, and +// left un-dirty so it's never pushed to everyone's server on its own. +function ensureNonEmpty(lib) { + if (lib.songs.length) return lib; + const blank = defaultSong("New Song"); + return { ...lib, songs: [blank], activeId: blank.id }; +} + export default function App() { const [library, setLibrary] = useState(null); const [loading, setLoading] = useState(true); @@ -71,6 +84,15 @@ export default function App() { // brief confirmation that a share link was copied const [shareFlash, setShareFlash] = useState(false); + // Songs another machine changed that need a decision before they can save + // again: { kind: "conflict" | "orphan", wpId, id, name, theirs? }. Their + // writes are held in storage.js until resolved; the rest of the library + // keeps autosaving normally. + const [flags, setFlags] = useState([]); + + // Count of songs quietly pulled in from another machine, for a passing note. + const [pullFlash, setPullFlash] = useState(0); + useEffect(() => { // Consume the shared payload (if any) before loading from storage. The // hash is stripped regardless of parse success so the URL is clean. @@ -321,12 +343,108 @@ export default function App() { })); }, []); + // Pull down anything another machine changed and fold it into local state. + // Songs this tab hasn't touched are adopted silently; only genuine conflicts + // surface. Returns the library to work from — the merged one when something + // came in, the current one otherwise. + const runSync = useCallback(async () => { + const current = libraryRef.current; + if (!wpMode || !current) return current; + + const result = await syncUpstream(current); + if (!result) return current; + if (result.authExpired) { + setWpStatus("expired"); + return current; + } + + const merged = ensureNonEmpty(result.library); + setLibrary(merged); + setFlags((cur) => { + const incoming = [ + ...result.conflicts.map((c) => ({ ...c, kind: "conflict" })), + ...result.orphans.map((o) => ({ ...o, kind: "orphan" })), + ]; + // Later syncs won't re-flag a song already flagged — the snapshot has + // moved on to the server's copy — so unresolved entries have to be kept + // rather than replaced, or the song's save hold would strand it. + // An orphan supersedes an earlier conflict on the same song: once it's + // been trashed elsewhere, "use theirs" no longer means anything. + const superseded = new Set( + incoming.filter((f) => f.kind === "orphan").map((f) => f.wpId) + ); + const kept = cur.filter((f) => !superseded.has(f.wpId)); + const seen = new Set(kept.map((f) => f.wpId)); + return [...kept, ...incoming.filter((f) => !seen.has(f.wpId))]; + }); + if (result.pulled) { + setPullFlash(result.pulled); + setTimeout(() => setPullFlash(0), 6000); + } + return merged; + }, []); + + const dismissFlag = (wpId) => setFlags((cur) => cur.filter((f) => f.wpId !== wpId)); + + // Conflict → keep this tab's copy. Releasing the hold lets the next save + // overwrite the other machine's version; that version stays recoverable from + // the song's revisions in wp-admin. + const keepMine = (flag) => { + releaseHold(flag.wpId); + dismissFlag(flag.wpId); + setDirty(true); + }; + + // Conflict → take the other machine's copy, dropping this tab's edits to that + // song. Its id stays local so the active-song selection survives. + const useTheirs = (flag) => { + releaseHold(flag.wpId); + setLibrary((cur) => ({ + ...cur, + songs: cur.songs.map((s) => (s.id === flag.id ? { ...flag.theirs, id: s.id } : s)), + })); + dismissFlag(flag.wpId); + }; + + // Orphan → keep a song that was trashed elsewhere. Dropping its server handle + // makes the next save re-create it rather than PUT to a trashed post. + const keepDeleted = (flag) => { + forgetHandle(flag.id, flag.wpId); + setLibrary((cur) => ({ + ...cur, + songs: cur.songs.map((s) => + s.id === flag.id ? (({ wpId, wpToken, ...rest }) => rest)(s) : s + ), + })); + dismissFlag(flag.wpId); + setDirty(true); + }; + + // Orphan → accept the deletion here too. + const discardDeleted = (flag) => { + releaseHold(flag.wpId); + setLibrary((cur) => { + const songs = cur.songs.filter((s) => s.id !== flag.id); + const activeId = songs.some((s) => s.id === cur.activeId) + ? cur.activeId + : songs[0]?.id ?? null; + return ensureNonEmpty({ ...cur, songs, activeId }); + }); + dismissFlag(flag.wpId); + }; + // The single save path used by autosave, the manual Save button (local mode), // and manual retries. Reads the latest library through the ref. const runSave = useCallback( async (opts = {}) => { - const lib = libraryRef.current; + let lib = libraryRef.current; + if (!lib) return; + + // Reconcile first, so a save that has been sitting in the debounce window + // can't push over something that landed from another machine meanwhile. + if (wpMode) lib = await runSync(); if (!lib) return; + setSaving(true); const res = await saveLibrary(lib, opts); setSaving(false); @@ -345,11 +463,15 @@ export default function App() { setWpStatus("expired"); } else if (res.stale) { setWpStatus("stale"); + } else if (res.diverged) { + // A song was trashed elsewhere between the sync and the write. Resync + // to classify it and surface the choice. + runSync(); } else if (res.offline) { setWpStatus("offline"); } }, - [adoptWpIds] + [adoptWpIds, runSync] ); const handleSave = () => { @@ -368,6 +490,12 @@ export default function App() { // Best-effort flush when the tab is hidden or unloaded, so edits inside the // debounce window aren't lost. WP saves use keepalive so they survive unload. + // Coming back to a visible tab is the other half: it's exactly the moment + // this laptop might have gone stale against another one, so reconcile then + // rather than polling in the background. + const syncRef = useRef(runSync); + syncRef.current = runSync; + useEffect(() => { const flush = () => { if (!dirtyRef.current || !libraryRef.current) return; @@ -375,6 +503,7 @@ export default function App() { }; const onVis = () => { if (document.visibilityState === "hidden") flush(); + else syncRef.current(); }; document.addEventListener("visibilitychange", onVis); window.addEventListener("pagehide", flush); @@ -557,6 +686,8 @@ export default function App() { ? "expired" : wpStatus === "stale" ? "stale" + : flags.length + ? "conflict" : wpStatus === "offline" ? "offline" : dirty @@ -606,6 +737,15 @@ export default function App() { onReload={() => window.location.reload()} /> + + , label: "Offline — changes kept locally" }, expired: { icon: , label: "Session expired" }, stale: { icon: , label: "Reload before saving" }, + conflict: { icon: , label: "Changed on another device" }, viewonly: { icon: , label: "View only" }, }; const s = map[state] || map.saved; diff --git a/src/components/SyncNotice.jsx b/src/components/SyncNotice.jsx new file mode 100644 index 0000000..3144c7e --- /dev/null +++ b/src/components/SyncNotice.jsx @@ -0,0 +1,64 @@ +import { TriangleAlert, ArrowDownToLine } from "lucide-react"; + +// Sits under the header in WP mode and reports what came in from another +// machine. +// +// Two very different messages share the strip. The pulled-songs line is +// informational and disappears on its own — those songs were adopted with no +// decision to make. The flags are blocking: each names a song whose saves are +// held until the user picks a side, so each carries its own two buttons rather +// than one library-wide "reload", which would throw away local edits. +export function SyncNotice({ + flags, + pulled, + onKeepMine, + onUseTheirs, + onKeepDeleted, + onDiscardDeleted, +}) { + if (!flags.length && !pulled) return null; + + return ( +
+ {!!pulled && !flags.length && ( +
+ + + Updated {pulled} song{pulled === 1 ? "" : "s"} from another device. + +
+ )} + + {flags.map((flag) => ( +
+ + + {flag.name || "Untitled"}{" "} + {flag.kind === "orphan" + ? "was deleted on another device, but you've edited it here." + : "was also edited on another device."} + + {flag.kind === "orphan" ? ( + <> + + + + ) : ( + <> + + + + )} +
+ ))} +
+ ); +} diff --git a/src/storage.js b/src/storage.js index 28b5194..c29da30 100644 --- a/src/storage.js +++ b/src/storage.js @@ -76,6 +76,19 @@ function writeBuffer(lib) { // songs never issue writes. let serverSnapshot = new Map(); +// The server's own version token per song (wpId -> token), captured alongside +// serverSnapshot. The server mints these from the stored JSON; the client only +// ever compares them for equality, so there's no canonicalization to keep in +// step across the two languages. Diffing these against /library/state is how we +// notice another tab has written since we last looked. +let serverTokens = new Map(); + +// Songs whose writes are suppressed because reconciling them needs the user: +// edited here *and* upstream, or edited here and trashed upstream. Held by +// wpId. Everything else in the library keeps saving normally — a conflict on +// one song is no reason to stall the rest. +let heldWpIds = new Set(); + // Handles assigned to songs created earlier this session, keyed by song id, so // a re-save that fires before the app has adopted the new wpId into state can't // create a duplicate. Reset on every load. @@ -98,11 +111,24 @@ function stableStringify(v) { return JSON.stringify(v); } +// wpId and wpToken are transport handles, not song data — excluded so neither +// one can read as an edit. function songContent(song) { - const { wpId, ...rest } = song; + const { wpId, wpToken, ...rest } = song; return stableStringify(rest); } +function songPayload(song) { + const { wpId, wpToken, ...rest } = song; + return rest; +} + +// A song's server handle: its adopted wpId, or one assigned to it earlier this +// session but not yet reflected in app state. +function handleOf(song) { + return song.wpId ?? createdIds.get(song.id) ?? null; +} + function wpUrl(path) { // rest_url() yields either ".../wp-json/troche/v1" or "...?rest_route=/troche/v1"; // appending the path extends the route correctly in both permalink modes. @@ -123,6 +149,17 @@ async function wpFetch(path, method, body, keepalive) { return res; } +// Read a response body without letting a parse failure unwind a write that +// already landed. A missing token just means the next probe sees a change and +// re-pulls the song, which is harmless — losing the save is not. +async function readJson(res) { + try { + return await res.json(); + } catch { + return null; + } +} + // Thrown to unwind a save when the session/nonce is no longer valid. class AuthError extends Error {} @@ -132,19 +169,60 @@ class AuthError extends Error {} // state and a fresh load rebuilds the snapshot. class StaleSnapshotError extends Error {} -async function loadFromWp() { +// Thrown when an update targets a song the server no longer has — trashed on +// another machine since our last sync. Distinct from a network failure, which +// is what it used to be reported as: a sync reclassifies the song as an orphan +// and asks whether to keep or discard it. +class MissingSongError extends Error {} + +async function fetchLibrary() { const res = await wpFetch("/library", "GET"); if (res.status === 401 || res.status === 403) throw new AuthError(); if (!res.ok) throw new Error("load failed: " + res.status); const data = await res.json(); - const songs = Array.isArray(data?.songs) ? data.songs : []; + return Array.isArray(data?.songs) ? data.songs : []; +} + +// The cheap probe: version tokens only, no song content. +async function fetchState() { + const res = await wpFetch("/library/state", "GET"); + if (res.status === 401 || res.status === 403) throw new AuthError(); + if (!res.ok) throw new Error("state failed: " + res.status); + const data = await res.json(); + const tokens = data?.tokens; + return tokens && typeof tokens === "object" ? tokens : {}; +} - // Rebuild the server snapshot (keyed by wpId) from what we just fetched. +// Record "what the server holds" from a freshly fetched library. Builds new +// Maps rather than mutating, so a caller can hold the previous ones as the +// before-picture of a reconcile. +function recordServerState(songs) { serverSnapshot = new Map(); - createdIds = new Map(); + serverTokens = new Map(); for (const s of songs) { - if (s && typeof s.wpId === "number") serverSnapshot.set(s.wpId, songContent(s)); + if (s && typeof s.wpId === "number") { + serverSnapshot.set(s.wpId, songContent(s)); + serverTokens.set(s.wpId, s.wpToken ?? null); + } } +} + +// True if the token map from /library/state disagrees with what we last saw — +// a song added, removed, or rewritten by someone else. +function upstreamMoved(tokens) { + const keys = Object.keys(tokens); + if (keys.length !== serverTokens.size) return true; + return keys.some((k) => serverTokens.get(Number(k)) !== tokens[k]); +} + +async function loadFromWp() { + const songs = await fetchLibrary(); + recordServerState(songs); + // A full load replaces app state wholesale, so session-local bookkeeping goes + // with it. (syncUpstream deliberately keeps both — it reconciles into the + // library the user is already working in.) + createdIds = new Map(); + heldWpIds = new Set(); return songs; } @@ -158,28 +236,34 @@ async function doWpSave(library, keepalive) { for (const song of songs) { const content = songContent(song); - const payload = (({ wpId: _drop, ...rest }) => rest)(song); - // A song's handle is its wpId, or one assigned to it earlier this session. - const wpId = song.wpId ?? createdIds.get(song.id) ?? null; + const payload = songPayload(song); + const wpId = handleOf(song); if (wpId) { seenWpIds.add(wpId); + // Held songs are awaiting a user decision; writing one would be the + // clobber the hold exists to prevent. Counted as seen above so the trash + // diff below doesn't mistake the skip for a deletion. + if (heldWpIds.has(wpId)) continue; if (serverSnapshot.get(wpId) !== content) { const res = await wpFetch("/songs/" + wpId, "PUT", payload, keepalive); if (res.status === 401 || res.status === 403) throw new AuthError(); + if (res.status === 404) throw new MissingSongError(); if (!res.ok) throw new Error("update failed: " + res.status); serverSnapshot.set(wpId, content); + serverTokens.set(wpId, (await readJson(res))?.wpToken ?? null); } } else { const res = await wpFetch("/songs", "POST", payload, keepalive); if (res.status === 401 || res.status === 403) throw new AuthError(); if (!res.ok) throw new Error("create failed: " + res.status); - const created = await res.json(); + const created = await readJson(res); const newId = created?.wpId; if (newId) { assignedIds[song.id] = newId; createdIds.set(song.id, newId); serverSnapshot.set(newId, content); + serverTokens.set(newId, created?.wpToken ?? null); seenWpIds.add(newId); } } @@ -206,6 +290,7 @@ async function doWpSave(library, keepalive) { // A 404 (already gone) is fine; only hard-fail on other errors. if (!res.ok && res.status !== 404) throw new Error("delete failed: " + res.status); serverSnapshot.delete(wpId); + serverTokens.delete(wpId); } return assignedIds; @@ -236,18 +321,23 @@ export async function loadLibrary() { return readBuffer(); } +// Server round-trips run one at a time: they all read and rewrite the snapshot +// maps, so overlapping calls could double-create songs or reconcile against a +// half-updated picture. +let chain = Promise.resolve(); + +function serialize(fn) { + const result = chain.then(fn, fn); + chain = result.catch(() => {}); + return result; +} + // Persist the library. // Local mode: writes the whole blob. -> { ok } // WP mode: mirrors to the buffer, then diffs and saves per-song. // -> { ok, offline, authExpired, readOnly, assignedIds } -// Saves are serialized (see `chain`) so overlapping calls can't double-create. -let chain = Promise.resolve(); - export function saveLibrary(library, opts = {}) { - const run = () => doSave(library, opts); - const result = chain.then(run, run); - chain = result.catch(() => {}); - return result; + return serialize(() => doSave(library, opts)); } async function doSave(library, opts) { @@ -272,11 +362,138 @@ async function doSave(library, opts) { if (e instanceof StaleSnapshotError) { return { ok: false, stale: true }; } + if (e instanceof MissingSongError) { + return { ok: false, diverged: true }; + } // Network error or server hiccup — changes are safe in the buffer. return { ok: false, offline: true }; } } +// Reconcile this tab against the server, for the case where the same library is +// open on another machine. +// +// Cheap path first: one token request, and if nothing moved upstream we're done +// without transferring a single song. When something did move, the per-song +// diff means most of it still isn't a conflict — a song someone else edited +// that this tab hasn't touched can simply be adopted, and one added elsewhere +// can simply be pulled in. Only a song edited in *both* places, or edited here +// and trashed there, needs the user; those get held back from saving until +// they're resolved. +// +// Returns null when there's nothing to report (including offline — a failed +// probe just means we reconcile later; edits stay safe in the buffer). +// -> { library, pulled, conflicts, orphans } | { authExpired: true } | null +export function syncUpstream(library) { + if (!wpMode || !library) return Promise.resolve(null); + return serialize(() => doSync(library)); +} + +async function doSync(library) { + let prevSnapshot; + let prevTokens; + let serverSongs; + + try { + if (!upstreamMoved(await fetchState())) return null; + // Hold the before-picture: "did this tab edit that song?" has to be asked + // against what the server held at our last sync, not what it holds now. + prevSnapshot = serverSnapshot; + prevTokens = serverTokens; + serverSongs = await fetchLibrary(); + recordServerState(serverSongs); + } catch (e) { + if (e instanceof AuthError) return { authExpired: true }; + return null; + } + + const untouchedHere = (song, wpId) => prevSnapshot.get(wpId) === songContent(song); + + const unclaimed = new Map( + serverSongs.filter((s) => typeof s.wpId === "number").map((s) => [s.wpId, s]) + ); + const merged = []; + const conflicts = []; + const orphans = []; + let pulled = 0; + + // Walk the local library first so this tab's song order survives the merge; + // anything genuinely new to us lands at the end. + for (const local of library.songs) { + const wpId = handleOf(local); + if (wpId == null) { + merged.push(local); // created here, never saved — nothing to reconcile + continue; + } + + const server = unclaimed.get(wpId); + + if (!server) { + // Trashed on the other machine. + if (untouchedHere(local, wpId)) { + pulled++; // drop it here too + continue; + } + orphans.push({ wpId, id: local.id, name: local.name }); + heldWpIds.add(wpId); + merged.push(local); + continue; + } + + unclaimed.delete(wpId); + + if (prevTokens.get(wpId) === server.wpToken) { + merged.push(local); // unchanged upstream — this tab's copy stands + continue; + } + if (untouchedHere(local, wpId)) { + merged.push(server); // changed upstream only — adopt it + pulled++; + continue; + } + + conflicts.push({ wpId, id: local.id, name: local.name, theirs: server }); + heldWpIds.add(wpId); + merged.push(local); + } + + for (const server of unclaimed.values()) { + merged.push(server); // added on the other machine + pulled++; + } + + if (!pulled && !conflicts.length && !orphans.length) return null; + + const activeId = merged.some((s) => s.id === library.activeId) + ? library.activeId + : merged[0]?.id ?? null; + + const reconciled = { ...library, songs: merged, activeId }; + // Keep the offline copy current here too, not just on load and save: a + // reconcile can go a long time without a save behind it (nothing local was + // dirty), and going offline in that window shouldn't roll the tab back to a + // library the server has already moved past. + writeBuffer(reconciled); + + return { library: reconciled, pulled, conflicts, orphans }; +} + +// Resolve a conflict or orphan: release the song's save hold so the next save +// acts on whatever the app settled on. Keeping the local copy makes the next +// diff overwrite theirs; taking theirs leaves nothing to write. +export function releaseHold(wpId) { + heldWpIds.delete(wpId); +} + +// Forget a song's server handle so the next save re-creates it as a fresh post. +// For an orphan the user chose to keep: its old wpId names a trashed post that +// PUT would only 404 on, and the app has stripped wpId from the song — but the +// session-local handle would still resolve it, so that has to go too. +export function forgetHandle(songId, wpId) { + createdIds.delete(songId); + heldWpIds.delete(wpId); +} + // Clear the local buffer (standalone "Reset"). No effect on server data. export function clearBuffer() { try { diff --git a/src/styles.js b/src/styles.js index 631fb26..e2f6f1c 100644 --- a/src/styles.js +++ b/src/styles.js @@ -114,12 +114,27 @@ export const css = ` .sa-savestate.offline { color: #b0692c; } .sa-savestate.expired { color: var(--accent); } .sa-savestate.stale { color: var(--accent); } +.sa-savestate.conflict { color: var(--accent); } /* Clickable states (Unsaved → save now, Offline → retry) read as actions. */ .sa-savestate.clickable { cursor: pointer; } .sa-savestate.clickable.pending { color: var(--accent); } .sa-savestate.clickable:hover { text-decoration: underline; } a.sa-savestate.expired:hover { text-decoration: underline; } +/* Sync notice strip (WP mode) — what arrived from another device, and the + per-song choices for anything that needs one. */ +.sa-sync { + display: flex; flex-direction: column; gap: 6px; + padding: 8px 0 2px; +} +.sa-sync-row { + display: flex; align-items: center; gap: 8px; flex-wrap: wrap; + font-size: 13px; color: var(--accent); +} +.sa-sync-row.info { color: var(--ink-dim); } +.sa-sync-text { flex: 1 1 auto; min-width: 180px; } +.sa-sync-row .sa-btn { padding: 4px 10px; font-size: 12px; } + .sa-switcher { display: flex; align-items: center; gap: 8px; font-family: 'Outfit', sans-serif; font-weight: 600; font-size: 20px; diff --git a/tests/README.md b/tests/README.md index 3d87597..7dfdfa3 100644 --- a/tests/README.md +++ b/tests/README.md @@ -9,6 +9,7 @@ Requires **Node 20+** (same as the build). Run from the repo root. | Command | What it covers | | --- | --- | | `npm run test:unit` | `normalizeLibrary` — duplicate/missing id repair and macOS-style name de-duping. Pure Node, no WordPress. | +| `npm run test:sync` | The WP reconcile path in `storage.js` — what a second open tab adopts silently vs. flags, and that flagged songs are held back from saving. Runs against a fake in-process WordPress. | | `npm run test:plugin` | The plugin source in a fresh WordPress: post type, REST CRUD, the login-to-read and cap-to-edit gates, revisions, trash-on-delete. | | `npm run test:zip` | Builds `troche.zip` and installs it into a fresh WordPress the same way a wp-admin upload would, then verifies it unpacked, activated, and runs. | | `npm test` | All of the above. | diff --git a/tests/plugin.php b/tests/plugin.php index 0e8b795..09708f8 100644 --- a/tests/plugin.php +++ b/tests/plugin.php @@ -86,6 +86,42 @@ function troche_rest( $method, $route, $body = null ) { check( 'song carries wpId', ( $data['songs'][0]['wpId'] ?? 0 ) === $wp_id ); check( 'unicode key round-trips', 'A♭' === ( $data['songs'][0]['musicalKey'] ?? '' ) ); +// ---- version tokens (upstream-change detection) ---- +$token = $data['songs'][0]['wpToken'] ?? ''; +check( 'song carries wpToken', is_string( $token ) && '' !== $token ); +check( 'create response carries wpToken', ! empty( $r->get_data()['wpToken'] ) ); +check( 'create token matches the library token', ( $r->get_data()['wpToken'] ?? null ) === $token ); + +wp_set_current_user( 0 ); +check( 'GET /library/state logged-out -> 401', 401 === troche_rest( 'GET', '/troche/v1/library/state' )->get_status() ); +wp_set_current_user( $sub_id ); +$state = troche_rest( 'GET', '/troche/v1/library/state' ); +check( 'GET /library/state -> 200', 200 === $state->get_status() ); +$tokens = (array) ( $state->get_data()['tokens'] ?? array() ); +check( 'state lists one token, keyed by wpId', array( (string) $wp_id => $token ) === $tokens ); + +// A save that rewrites identical content must not move the token — otherwise +// every idle tab would see a phantom conflict. +troche_rest( 'PUT', '/troche/v1/songs/' . $wp_id, $song ); +$same = (array) troche_rest( 'GET', '/troche/v1/library/state' )->get_data()['tokens']; +check( 'token stable across an identical re-save', $token === ( $same[ (string) $wp_id ] ?? '' ) ); + +// A real edit must move it. +$edited = $song; +$edited['bpm'] = 140; +troche_rest( 'PUT', '/troche/v1/songs/' . $wp_id, $edited ); +$moved = (array) troche_rest( 'GET', '/troche/v1/library/state' )->get_data()['tokens']; +check( 'token changes when content changes', $token !== ( $moved[ (string) $wp_id ] ?? '' ) ); +check( + 'update response token matches the new state token', + ( troche_rest( 'PUT', '/troche/v1/songs/' . $wp_id, $edited )->get_data()['wpToken'] ?? null ) + === ( $moved[ (string) $wp_id ] ?? '' ) +); + +// wpToken is a transport handle, like wpId — it must never be stored. +$stored_song = json_decode( get_post_field( 'post_content', $wp_id ), true ); +check( 'stored content omits wpToken', ! isset( $stored_song['wpToken'] ) ); + // ---- wp-admin cap mapping (post-type actions gate on troche_edit, not core post caps) ---- $editor_id = wp_insert_user( array( diff --git a/tests/sync.mjs b/tests/sync.mjs new file mode 100644 index 0000000..9bdec54 --- /dev/null +++ b/tests/sync.mjs @@ -0,0 +1,261 @@ +// Unit tests for the WP sync/reconcile path in src/storage.js — the logic that +// decides, when the same library is open on two machines, what can be adopted +// silently and what has to be handed to the user. +// +// Runs against a fake in-process WordPress: a Map of song posts behind the same +// four REST routes the plugin serves. No browser, no WordPress. +// node tests/sync.mjs # or: npm run test:sync + +let pass = 0; +let fail = 0; +const check = (label, cond) => { + cond ? pass++ : fail++; + console.log((cond ? "PASS" : "FAIL") + ": " + label); +}; + +// ---- fake server ---- + +// Mirrors class-store.php: songs live keyed by post id, tokens are derived from +// stored content (so an identical re-save doesn't move one), and wpId/wpToken +// are stripped on the way in and decorated on the way out. +function makeServer(initial = {}) { + const posts = new Map(Object.entries(initial).map(([k, v]) => [Number(k), v])); + let nextId = 200; + const token = (song) => "t:" + JSON.stringify(song); + const strip = ({ wpId, wpToken, ...rest }) => rest; + + const server = { + posts, + calls: [], + // Direct mutation, standing in for "the other laptop saved". + edit(wpId, patch) { + posts.set(wpId, { ...posts.get(wpId), ...patch }); + }, + add(song) { + const id = ++nextId; + posts.set(id, song); + return id; + }, + trash(wpId) { + posts.delete(wpId); + }, + decorate(wpId) { + const song = posts.get(wpId); + return { ...song, wpId, wpToken: token(song) }; + }, + }; + + globalThis.fetch = async (url, opts = {}) => { + const route = String(url).replace("http://test/troche/v1", ""); + const method = opts.method || "GET"; + server.calls.push(method + " " + route); + const body = opts.body ? JSON.parse(opts.body) : null; + const ok = (status, data) => ({ ok: true, status, json: async () => data }); + + if (route === "/library") { + return ok(200, { + format: "troche", + version: 1, + songs: [...posts.keys()].sort((a, b) => a - b).map((id) => server.decorate(id)), + }); + } + if (route === "/library/state") { + const tokens = {}; + for (const [id, song] of posts) tokens[String(id)] = token(song); + return ok(200, { tokens }); + } + if (route === "/songs" && method === "POST") { + return ok(201, server.decorate(server.add(strip(body)))); + } + const match = route.match(/^\/songs\/(\d+)$/); + if (match) { + const wpId = Number(match[1]); + if (!posts.has(wpId)) return { ok: false, status: 404, json: async () => ({}) }; + if (method === "DELETE") { + posts.delete(wpId); + return ok(200, { deleted: true, wpId }); + } + posts.set(wpId, strip(body)); + return ok(200, server.decorate(wpId)); + } + throw new Error("unexpected request: " + method + " " + route); + }; + + return server; +} + +// storage.js reads window.trocheWP and computes wpMode at import time, and keeps +// its snapshot maps in module scope — so each scenario gets a fresh instance. +let instance = 0; +async function freshStorage() { + globalThis.window = { trocheWP: { restUrl: "http://test/troche/v1", nonce: "n", canEdit: true } }; + return import("../src/storage.js?case=" + ++instance); +} + +const song = (id, name, bpm = 120) => ({ id, name, bpm, parts: [] }); +const names = (lib) => lib.songs.map((s) => s.name); + +// ---- 1. nothing moved upstream ---- +{ + const server = makeServer({ 1: song("a", "Alpha"), 2: song("b", "Beta") }); + const storage = await freshStorage(); + const lib = await storage.loadLibrary(); + + server.calls.length = 0; + const result = await storage.syncUpstream(lib); + check("quiet server reports nothing to reconcile", result === null); + check( + "quiet sync costs one token request, no content", + server.calls.length === 1 && server.calls[0] === "GET /library/state" + ); +} + +// ---- 2. changed upstream, untouched here -> adopted silently ---- +{ + const server = makeServer({ 1: song("a", "Alpha"), 2: song("b", "Beta") }); + const storage = await freshStorage(); + const lib = await storage.loadLibrary(); + + server.edit(2, { bpm: 155 }); + const result = await storage.syncUpstream(lib); + check("upstream-only edit reconciles", result !== null && !result.conflicts.length); + check("upstream-only edit counts as pulled", result.pulled === 1); + check("upstream-only edit adopts the server value", result.library.songs[1].bpm === 155); + check("upstream-only edit leaves song order alone", String(names(result.library)) === "Alpha,Beta"); +} + +// ---- 3. changed in both places -> conflict, local kept, writes held ---- +{ + const server = makeServer({ 1: song("a", "Alpha"), 2: song("b", "Beta") }); + const storage = await freshStorage(); + const lib = await storage.loadLibrary(); + + server.edit(2, { bpm: 155 }); // the other laptop + const local = { ...lib, songs: [lib.songs[0], { ...lib.songs[1], bpm: 90 }] }; // this one + + const result = await storage.syncUpstream(local); + check("same-song edit is flagged", result.conflicts.length === 1); + check("conflict names the song", result.conflicts[0].name === "Beta"); + check("conflict carries the other copy", result.conflicts[0].theirs.bpm === 155); + check("conflict keeps the local copy in the library", result.library.songs[1].bpm === 90); + check("conflict is not counted as a silent pull", result.pulled === 0); + + server.calls.length = 0; + const saved = await storage.saveLibrary(result.library); + check("save succeeds around a conflict", saved.ok === true); + check("held song is not written", !server.calls.some((c) => c.startsWith("PUT"))); + check("held song is not trashed either", !server.calls.some((c) => c.startsWith("DELETE"))); + check("other laptop's copy survives", server.posts.get(2).bpm === 155); + + // Resolving to "keep mine" releases the hold; the next save overwrites. + storage.releaseHold(result.conflicts[0].wpId); + await storage.saveLibrary(result.library); + check("released song is written on the next save", server.posts.get(2).bpm === 90); +} + +// ---- 4. edits to other songs still save while one is conflicted ---- +{ + const server = makeServer({ 1: song("a", "Alpha"), 2: song("b", "Beta") }); + const storage = await freshStorage(); + const lib = await storage.loadLibrary(); + + server.edit(2, { bpm: 155 }); + const local = { + ...lib, + songs: [{ ...lib.songs[0], bpm: 70 }, { ...lib.songs[1], bpm: 90 }], + }; + + const result = await storage.syncUpstream(local); + await storage.saveLibrary(result.library); + check("a conflict on one song doesn't stall the others", server.posts.get(1).bpm === 70); +} + +// ---- 5. added upstream -> pulled in ---- +{ + const server = makeServer({ 1: song("a", "Alpha") }); + const storage = await freshStorage(); + const lib = await storage.loadLibrary(); + + server.add(song("c", "Gamma")); + const result = await storage.syncUpstream(lib); + check("song added elsewhere is pulled in", String(names(result.library)) === "Alpha,Gamma"); + check("added song counts as pulled", result.pulled === 1); +} + +// ---- 6. trashed upstream, untouched here -> dropped ---- +{ + const server = makeServer({ 1: song("a", "Alpha"), 2: song("b", "Beta") }); + const storage = await freshStorage(); + const lib = await storage.loadLibrary(); + + server.trash(2); + const result = await storage.syncUpstream(lib); + check("song trashed elsewhere is dropped here", String(names(result.library)) === "Alpha"); + check("dropped song raises no flag", result.orphans.length === 0 && result.conflicts.length === 0); +} + +// ---- 7. trashed upstream, edited here -> orphan, writes held ---- +{ + const server = makeServer({ 1: song("a", "Alpha"), 2: song("b", "Beta") }); + const storage = await freshStorage(); + const lib = await storage.loadLibrary(); + + server.trash(2); + const local = { ...lib, songs: [lib.songs[0], { ...lib.songs[1], bpm: 90 }] }; + + const result = await storage.syncUpstream(local); + check("deleted-but-edited song is flagged as an orphan", result.orphans.length === 1); + check("orphan keeps the local copy", result.library.songs.length === 2); + + // An orphan's PUT would 404 — the hold has to stop it reaching the server. + const saved = await storage.saveLibrary(result.library); + check("orphan doesn't fail the save with a 404", saved.ok === true); + + // "Keep mine" drops the dead handle so the song is re-created, not PUT. + const orphan = result.orphans[0]; + storage.forgetHandle(orphan.id, orphan.wpId); + const kept = { + ...result.library, + songs: result.library.songs.map(({ wpId, wpToken, ...rest }) => + rest.id === orphan.id ? rest : { ...rest, wpId, wpToken } + ), + }; + await storage.saveLibrary(kept); + check("kept orphan is re-created as a new song", server.posts.size === 2); +} + +// ---- 8. a song created here but never saved survives a sync ---- +{ + const server = makeServer({ 1: song("a", "Alpha") }); + const storage = await freshStorage(); + const lib = await storage.loadLibrary(); + + server.edit(1, { bpm: 155 }); + const local = { ...lib, songs: [...lib.songs, song("new", "Draft")] }; + + const result = await storage.syncUpstream(local); + check("unsaved local song survives a sync", String(names(result.library)) === "Alpha,Draft"); + check("unsaved local song isn't duplicated", result.library.songs.length === 2); +} + +// ---- 9. a song created this session isn't duplicated by a sync ---- +{ + const server = makeServer({ 1: song("a", "Alpha") }); + const storage = await freshStorage(); + const lib = await storage.loadLibrary(); + + // Save a new song, but don't adopt the returned wpId into the library — + // exactly the window where app state lags the server handle. + const local = { ...lib, songs: [...lib.songs, song("new", "Draft")] }; + const saved = await storage.saveLibrary(local); + check("new song is created on save", saved.ok && Object.keys(saved.assignedIds).length === 1); + + const result = await storage.syncUpstream(local); + check( + "sync before the handle is adopted doesn't duplicate the song", + result === null || result.library.songs.length === 2 + ); +} + +console.log(`\n=== ${pass} passed, ${fail} failed ===`); +process.exit(fail ? 1 : 0); diff --git a/wp-plugin/includes/class-rest-controller.php b/wp-plugin/includes/class-rest-controller.php index 6fcce01..4c2b963 100644 --- a/wp-plugin/includes/class-rest-controller.php +++ b/wp-plugin/includes/class-rest-controller.php @@ -3,6 +3,7 @@ * REST endpoints under troche/v1. * * - GET /library Whole library in the envelope format (login required). + * - GET /library/state Per-song version tokens, no content (login required). * - POST /songs Create a song (troche_edit required). * - PUT /songs/{id} Update a song (troche_edit required). * - DELETE /songs/{id} Trash a song (troche_edit required). @@ -44,6 +45,16 @@ public function register_routes() { ) ); + register_rest_route( + self::NAMESPACE, + '/library/state', + array( + 'methods' => \WP_REST_Server::READABLE, + 'callback' => array( $this, 'get_state' ), + 'permission_callback' => array( $this, 'can_read' ), + ) + ); + register_rest_route( self::NAMESPACE, '/songs', @@ -111,6 +122,19 @@ public function get_library() { return rest_ensure_response( Store::get_library() ); } + /** + * Return per-song version tokens so a client can tell, in one small + * request, whether anything changed under it since its last sync. + * + * @return \WP_REST_Response + */ + public function get_state() { + $response = rest_ensure_response( Store::get_state() ); + // Freshness is the whole point — never let a cache answer this. + $response->header( 'Cache-Control', 'no-store, max-age=0' ); + return $response; + } + /** * Create a song from the request body. * diff --git a/wp-plugin/includes/class-store.php b/wp-plugin/includes/class-store.php index 65e201b..3d5cce8 100644 --- a/wp-plugin/includes/class-store.php +++ b/wp-plugin/includes/class-store.php @@ -100,13 +100,13 @@ static function () { } /** - * The whole library in the envelope format, each song decorated with its - * post id (`wpId`) as the save handle. Trashed songs are excluded. + * Every live song post, oldest first. Shared by get_library() and + * get_state() so the two can never disagree about what's in the library. * - * @return array { format:string, version:int, songs:array[] } + * @return \WP_Post[] */ - public static function get_library() { - $posts = get_posts( + private static function get_song_posts() { + return get_posts( array( 'post_type' => self::POST_TYPE, 'post_status' => 'publish', @@ -116,15 +116,25 @@ public static function get_library() { 'suppress_filters' => false, ) ); + } + /** + * The whole library in the envelope format, each song decorated with its + * post id (`wpId`) as the save handle and its version token (`wpToken`). + * Trashed songs are excluded. + * + * @return array { format:string, version:int, songs:array[] } + */ + public static function get_library() { $songs = array(); - foreach ( $posts as $post ) { + foreach ( self::get_song_posts() as $post ) { $song = self::decode_song( $post->post_content ); if ( null === $song ) { continue; } - $song['wpId'] = (int) $post->ID; - $songs[] = $song; + $song['wpId'] = (int) $post->ID; + $song['wpToken'] = self::token( $post->post_content ); + $songs[] = $song; } return array( @@ -134,6 +144,44 @@ public static function get_library() { ); } + /** + * Version tokens for every live song, keyed by post id — the cheap "has + * anything moved?" probe a second tab polls before it saves. Carries no + * song content, so it stays small however big the library gets. + * + * @return array { tokens: array } + */ + public static function get_state() { + $tokens = array(); + foreach ( self::get_song_posts() as $post ) { + if ( null === self::decode_song( $post->post_content ) ) { + // Skip unparseable posts, exactly as get_library() does, so the + // two views agree on which songs exist. + continue; + } + $tokens[ (string) $post->ID ] = self::token( $post->post_content ); + } + + return array( 'tokens' => (object) $tokens ); + } + + /** + * A song's version token: a hash of its stored JSON. + * + * Content-derived rather than time-derived on purpose. post_modified_gmt + * only has one-second resolution (two saves in the same second look + * identical) and it moves even when a save rewrites byte-identical content, + * which would show up in another tab as a phantom conflict. Clients only + * ever compare tokens for equality — they never compute one — so the hash + * is free to change shape later. + * + * @param string $content post_content. + * @return string + */ + private static function token( $content ) { + return md5( (string) $content ); + } + /** * Create or update one song. * @@ -155,8 +203,8 @@ public static function save_song( array $song, $wp_id, $user ) { $song['id'] = self::generate_id(); } - // The wpId is a server-side handle, not part of the stored envelope. - unset( $song['wpId'] ); + // wpId and wpToken are server-side handles, not part of the stored envelope. + unset( $song['wpId'], $song['wpToken'] ); // wp_insert_post()/wp_update_post() expect slashed input and strip one // level of slashes on the way in; the encoded JSON contains backslashes @@ -188,7 +236,15 @@ public static function save_song( array $song, $wp_id, $user ) { return $result; } - $song['wpId'] = (int) $result; + // Token comes from the post as actually stored, not from the string we + // sent: wp_insert_post()/wp_update_post() run content through save + // filters (kses for users without unfiltered_html, among others), and a + // token that didn't survive those filters would read as a conflict on + // the very next poll. + $stored = get_post( (int) $result ); + + $song['wpId'] = (int) $result; + $song['wpToken'] = self::token( $stored ? $stored->post_content : '' ); return $song; } diff --git a/wp-plugin/readme.txt b/wp-plugin/readme.txt index 7a9fa93..b409767 100644 --- a/wp-plugin/readme.txt +++ b/wp-plugin/readme.txt @@ -27,6 +27,9 @@ This plugin serves the app from your WordPress site and adds: the editing capability can change it. * **Autosave and save history.** Edits save automatically, and every save is a WordPress revision you can review and restore from the dashboard. +* **Safe on two devices at once.** If a bandmate changes a song while you have + it open, the app notices and quietly brings the change in. It only asks you + to choose when the same song was edited in both places. * **Import and export.** Move a library in or out as JSON at any time. The app is served at a URL you choose (default `/troche`), styled entirely by From b94d514722657648e5eb8430f9716e2dce570bd2 Mon Sep 17 00:00:00 2001 From: Brian Alexander <824344+ironprogrammer@users.noreply.github.com> Date: Sun, 26 Jul 2026 20:20:24 -0700 Subject: [PATCH 2/2] Make the upstream check a guarantee, not a probability MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-up on three points. Syncing before a save narrows the clobber window but can't close it: the check and the write aren't one operation, so another machine can land a save in between. PUT now carries the token the client last saw in an X-Troche-Expect-Token header, and the server refuses with 409 if the song has moved since — checked immediately before the write. A refusal is routed into the same reconcile the 404 already used, so a lost race surfaces as the same per-song question rather than an overwrite. A custom header rather than If-Match on purpose: an If-Match a proxy decides to evaluate itself would fail the save outright, whereas a stripped custom header degrades to the unconditional write this endpoint has always done. Fix a re-flagged song keeping the copy it was first flagged with. If the other machine edited again while the notice sat unresolved, the reducer kept the original entry and dropped the newer one, so "Use theirs" adopted a stale copy — which then no longer matched the snapshot and was pushed straight back over their newer version on the next save, silently. Incoming entries now win, keeping their position in the strip; unmentioned flags still survive, so a song's save hold is never stranded. Lifted out of the component as mergeFlags() so it's directly testable. Fix resolving a conflict in the server's favour leaving the offline buffer holding the discarded copy. "Use theirs" and "Delete here too" correctly don't dirty the library — local already matches the server — but the buffer is otherwise only written by a save, so an offline reload resurrected the rejected version. Tests 97 -> 134. Covers the conditional write end to end (refused, accepted, and absent-header back-compat), a re-conflict carrying the newer copy, the diverged result for both 404 and 409 — a headline fix of the parent commit that had no test — the flag reducer's four rules, offline and expired probes, and that /library/state agrees with /library about trashed and unparseable posts, which is what makes deletions propagate at all. --- README.md | 4 +- docs/wp-storage-plan.md | 5 + src/App.jsx | 54 +++++----- src/storage.js | 34 +++++- src/utils.js | 27 +++++ tests/README.md | 6 +- tests/plugin.php | 59 ++++++++++- tests/sync.mjs | 106 ++++++++++++++++++- tests/unit.mjs | 34 +++++- wp-plugin/includes/class-rest-controller.php | 12 ++- wp-plugin/includes/class-store.php | 33 +++++- 11 files changed, 328 insertions(+), 46 deletions(-) diff --git a/README.md b/README.md index 715ad35..90b5d95 100644 --- a/README.md +++ b/README.md @@ -83,7 +83,9 @@ Because saving is per song rather than per library, having the app open on two machines is safe: when a tab comes back into view, and again before it saves, it checks which songs have moved and folds in anything it hasn't touched itself. Only a song edited in both places at once needs a decision, and it says so -rather than overwriting quietly. +rather than overwriting quietly. Saves are conditional on the version the tab +last saw, so even a write that races another machine is refused and turned into +that same question. ### Build diff --git a/docs/wp-storage-plan.md b/docs/wp-storage-plan.md index dc60e69..84d42b6 100644 --- a/docs/wp-storage-plan.md +++ b/docs/wp-storage-plan.md @@ -81,6 +81,11 @@ subpath and the plugin directory. edited here and trashed there, surfaces to the user — those are held back from saving until resolved, while the rest of the library keeps autosaving. Whichever copy loses is still recoverable from the song's revisions. + Checking before a save narrows the window but can't close it — the check and + the write aren't one operation — so `PUT` is conditional: it carries the token + the client last saw in `X-Troche-Expect-Token` and the server refuses with 409 + if the song moved in between. A refusal reconciles and surfaces the same + per-song choice, so a lost race is a question, never a silent overwrite. - **Share/Export/Import stay functionally untouched** in both modes (one codebase; they're client-side and cost nothing). Export/Import double as the seeding path: each bandmate exports their existing localStorage library and diff --git a/src/App.jsx b/src/App.jsx index 2aa217a..16f3546 100644 --- a/src/App.jsx +++ b/src/App.jsx @@ -1,7 +1,7 @@ import { useCallback, useEffect, useRef, useState } from "react"; import { Music, Plus, RotateCcw } from "lucide-react"; import { PALETTE } from "./constants.js"; -import { uid, normalizeLibrary } from "./utils.js"; +import { uid, normalizeLibrary, mergeFlags } from "./utils.js"; import { defaultLibrary, defaultSong } from "./defaults.js"; import { loadLibrary, @@ -9,6 +9,7 @@ import { syncUpstream, releaseHold, forgetHandle, + cacheLibrary, clearBuffer, wpMode, canEdit, @@ -360,23 +361,7 @@ export default function App() { const merged = ensureNonEmpty(result.library); setLibrary(merged); - setFlags((cur) => { - const incoming = [ - ...result.conflicts.map((c) => ({ ...c, kind: "conflict" })), - ...result.orphans.map((o) => ({ ...o, kind: "orphan" })), - ]; - // Later syncs won't re-flag a song already flagged — the snapshot has - // moved on to the server's copy — so unresolved entries have to be kept - // rather than replaced, or the song's save hold would strand it. - // An orphan supersedes an earlier conflict on the same song: once it's - // been trashed elsewhere, "use theirs" no longer means anything. - const superseded = new Set( - incoming.filter((f) => f.kind === "orphan").map((f) => f.wpId) - ); - const kept = cur.filter((f) => !superseded.has(f.wpId)); - const seen = new Set(kept.map((f) => f.wpId)); - return [...kept, ...incoming.filter((f) => !seen.has(f.wpId))]; - }); + setFlags((cur) => mergeFlags(cur, result.conflicts, result.orphans)); if (result.pulled) { setPullFlash(result.pulled); setTimeout(() => setPullFlash(0), 6000); @@ -386,6 +371,16 @@ export default function App() { const dismissFlag = (wpId) => setFlags((cur) => cur.filter((f) => f.wpId !== wpId)); + // Apply a resolution to local state and to the offline buffer together. + // Resolving in favour of the server leaves nothing to save — local already + // matches — and the buffer is otherwise only written by a save, so without + // this it would go on serving the copy the user just rejected to the next + // offline reload. + const applyResolution = (next) => { + setLibrary(next); + cacheLibrary(next); + }; + // Conflict → keep this tab's copy. Releasing the hold lets the next save // overwrite the other machine's version; that version stays recoverable from // the song's revisions in wp-admin. @@ -399,10 +394,11 @@ export default function App() { // song. Its id stays local so the active-song selection survives. const useTheirs = (flag) => { releaseHold(flag.wpId); - setLibrary((cur) => ({ + const cur = libraryRef.current; + applyResolution({ ...cur, songs: cur.songs.map((s) => (s.id === flag.id ? { ...flag.theirs, id: s.id } : s)), - })); + }); dismissFlag(flag.wpId); }; @@ -410,12 +406,13 @@ export default function App() { // makes the next save re-create it rather than PUT to a trashed post. const keepDeleted = (flag) => { forgetHandle(flag.id, flag.wpId); - setLibrary((cur) => ({ + const cur = libraryRef.current; + applyResolution({ ...cur, songs: cur.songs.map((s) => s.id === flag.id ? (({ wpId, wpToken, ...rest }) => rest)(s) : s ), - })); + }); dismissFlag(flag.wpId); setDirty(true); }; @@ -423,13 +420,12 @@ export default function App() { // Orphan → accept the deletion here too. const discardDeleted = (flag) => { releaseHold(flag.wpId); - setLibrary((cur) => { - const songs = cur.songs.filter((s) => s.id !== flag.id); - const activeId = songs.some((s) => s.id === cur.activeId) - ? cur.activeId - : songs[0]?.id ?? null; - return ensureNonEmpty({ ...cur, songs, activeId }); - }); + const cur = libraryRef.current; + const songs = cur.songs.filter((s) => s.id !== flag.id); + const activeId = songs.some((s) => s.id === cur.activeId) + ? cur.activeId + : songs[0]?.id ?? null; + applyResolution(ensureNonEmpty({ ...cur, songs, activeId })); dismissFlag(flag.wpId); }; diff --git a/src/storage.js b/src/storage.js index c29da30..3144e6c 100644 --- a/src/storage.js +++ b/src/storage.js @@ -135,7 +135,7 @@ function wpUrl(path) { return wpConfig.restUrl + path; } -async function wpFetch(path, method, body, keepalive) { +async function wpFetch(path, method, body, keepalive, expectToken) { const res = await fetch(wpUrl(path), { method, credentials: "same-origin", @@ -143,6 +143,11 @@ async function wpFetch(path, method, body, keepalive) { headers: { "Content-Type": "application/json", "X-WP-Nonce": wpConfig.nonce, + // Present only when we know what the server held, so the write can be + // refused if that's no longer true. Omitted (unconditional write) when we + // have no token to offer — an older plugin, or a response we couldn't + // parse — which is exactly the behaviour this had before. + ...(expectToken ? { "X-Troche-Expect-Token": expectToken } : null), }, body: body === undefined ? undefined : JSON.stringify(body), }); @@ -175,6 +180,12 @@ class StaleSnapshotError extends Error {} // and asks whether to keep or discard it. class MissingSongError extends Error {} +// Thrown when the server refuses an update because the song moved under us +// (409). Syncing before a save narrows that window but can't close it — the +// check and the write aren't one operation. Handled exactly like a 404: unwind, +// reconcile, and let the song be flagged if it's a genuine conflict. +class StaleTokenError extends Error {} + async function fetchLibrary() { const res = await wpFetch("/library", "GET"); if (res.status === 401 || res.status === 403) throw new AuthError(); @@ -246,9 +257,16 @@ async function doWpSave(library, keepalive) { // diff below doesn't mistake the skip for a deletion. if (heldWpIds.has(wpId)) continue; if (serverSnapshot.get(wpId) !== content) { - const res = await wpFetch("/songs/" + wpId, "PUT", payload, keepalive); + const res = await wpFetch( + "/songs/" + wpId, + "PUT", + payload, + keepalive, + serverTokens.get(wpId) + ); if (res.status === 401 || res.status === 403) throw new AuthError(); if (res.status === 404) throw new MissingSongError(); + if (res.status === 409) throw new StaleTokenError(); if (!res.ok) throw new Error("update failed: " + res.status); serverSnapshot.set(wpId, content); serverTokens.set(wpId, (await readJson(res))?.wpToken ?? null); @@ -362,7 +380,9 @@ async function doSave(library, opts) { if (e instanceof StaleSnapshotError) { return { ok: false, stale: true }; } - if (e instanceof MissingSongError) { + // Trashed elsewhere, or rewritten elsewhere between our sync and our write. + // Either way the library moved under us: reconcile and let that classify it. + if (e instanceof MissingSongError || e instanceof StaleTokenError) { return { ok: false, diverged: true }; } // Network error or server hiccup — changes are safe in the buffer. @@ -494,6 +514,14 @@ export function forgetHandle(songId, wpId) { heldWpIds.delete(wpId); } +// Mirror the library to the offline buffer without touching the server. For the +// changes that need no save to be correct — resolving a conflict in the +// server's favour leaves local already matching it — where waiting for the next +// save would leave the buffer holding a copy the user has discarded. +export function cacheLibrary(library) { + if (library) writeBuffer(library); +} + // Clear the local buffer (standalone "Reset"). No effect on server data. export function clearBuffer() { try { diff --git a/src/utils.js b/src/utils.js index dbdc34c..25091c6 100644 --- a/src/utils.js +++ b/src/utils.js @@ -57,6 +57,33 @@ export function normalizeLibrary(library) { return { library: { ...library, songs, activeId }, changed }; } +// Fold a sync's conflicts and orphans into the list of songs awaiting a +// decision, keyed by wpId. +// +// A flagged song that this sync didn't mention stays flagged: dismissing a flag +// is what releases that song's save hold, so dropping one would strand it +// unsaveable. A song the sync *did* mention adopts the new entry — the other +// machine has moved again, and "Use theirs" has to mean their copy as it stands +// now, not the one we first saw (taking the stale copy would push it straight +// back over their newer one). Existing entries keep their position so the +// notice strip doesn't reshuffle under the user; genuinely new ones append. +// +// An orphan arriving for a song already flagged as a conflict supersedes it by +// the same rule — once it's been trashed elsewhere there is no "theirs" left. +export function mergeFlags(current, conflicts = [], orphans = []) { + const incoming = [ + ...conflicts.map((c) => ({ ...c, kind: "conflict" })), + ...orphans.map((o) => ({ ...o, kind: "orphan" })), + ]; + const byWpId = new Map(incoming.map((f) => [f.wpId, f])); + const alreadyFlagged = new Set(current.map((f) => f.wpId)); + + return [ + ...current.map((f) => byWpId.get(f.wpId) ?? f), + ...incoming.filter((f) => !alreadyFlagged.has(f.wpId)), + ]; +} + export function clampNum(v, min, max, fallback) { const n = Number(v); if (Number.isNaN(n)) return fallback; diff --git a/tests/README.md b/tests/README.md index 7dfdfa3..999fee5 100644 --- a/tests/README.md +++ b/tests/README.md @@ -8,9 +8,9 @@ Requires **Node 20+** (same as the build). Run from the repo root. | Command | What it covers | | --- | --- | -| `npm run test:unit` | `normalizeLibrary` — duplicate/missing id repair and macOS-style name de-duping. Pure Node, no WordPress. | -| `npm run test:sync` | The WP reconcile path in `storage.js` — what a second open tab adopts silently vs. flags, and that flagged songs are held back from saving. Runs against a fake in-process WordPress. | -| `npm run test:plugin` | The plugin source in a fresh WordPress: post type, REST CRUD, the login-to-read and cap-to-edit gates, revisions, trash-on-delete. | +| `npm run test:unit` | `normalizeLibrary` — duplicate/missing id repair and macOS-style name de-duping — and `mergeFlags`, which decides what an unresolved sync notice does when the next sync lands. Pure Node, no WordPress. | +| `npm run test:sync` | The WP reconcile path in `storage.js` — what a second open tab adopts silently vs. flags, that flagged songs are held back from saving, and that a write onto a song that moved since is refused rather than landing. Runs against a fake in-process WordPress. | +| `npm run test:plugin` | The plugin source in a fresh WordPress: post type, REST CRUD, the login-to-read and cap-to-edit gates, version tokens and conditional writes, revisions, trash-on-delete. | | `npm run test:zip` | Builds `troche.zip` and installs it into a fresh WordPress the same way a wp-admin upload would, then verifies it unpacked, activated, and runs. | | `npm test` | All of the above. | diff --git a/tests/plugin.php b/tests/plugin.php index 09708f8..2620a50 100644 --- a/tests/plugin.php +++ b/tests/plugin.php @@ -20,14 +20,20 @@ function check( $label, $cond ) { $cond ? $pass++ : $fail++; file_put_contents( $out, $line . "\n", FILE_APPEND ); } -function troche_rest( $method, $route, $body = null ) { +function troche_rest( $method, $route, $body = null, $headers = array() ) { $req = new WP_REST_Request( $method, $route ); if ( null !== $body ) { $req->set_header( 'Content-Type', 'application/json' ); $req->set_body( wp_json_encode( $body ) ); } + foreach ( $headers as $name => $value ) { + $req->set_header( $name, $value ); + } return rest_do_request( $req ); } +function troche_tokens() { + return (array) troche_rest( 'GET', '/troche/v1/library/state' )->get_data()['tokens']; +} // ---- post type ---- check( 'CPT troche_song registered', post_type_exists( 'troche_song' ) ); @@ -122,6 +128,53 @@ function troche_rest( $method, $route, $body = null ) { $stored_song = json_decode( get_post_field( 'post_content', $wp_id ), true ); check( 'stored content omits wpToken', ! isset( $stored_song['wpToken'] ) ); +// ---- conditional writes (X-Troche-Expect-Token) ---- +// The client syncs before saving, but that check and the write aren't one +// operation. This is what stops a save landing on a song that moved in between. +$current = troche_tokens()[ (string) $wp_id ] ?? ''; +$stale_write = $song; +$stale_write['bpm'] = 999; + +$refused = troche_rest( + 'PUT', + '/troche/v1/songs/' . $wp_id, + $stale_write, + array( 'X-Troche-Expect-Token' => 'not-the-current-token' ) +); +check( 'PUT with a stale token -> 409', 409 === $refused->get_status() ); +check( 'refused PUT names the conflict', 'troche_stale_token' === ( $refused->get_data()['code'] ?? '' ) ); +check( 'refused PUT hands back the current token', $current === ( $refused->get_data()['data']['wpToken'] ?? '' ) ); +check( 'refused PUT left the song alone', $current === ( troche_tokens()[ (string) $wp_id ] ?? '' ) ); + +$accepted = troche_rest( + 'PUT', + '/troche/v1/songs/' . $wp_id, + $stale_write, + array( 'X-Troche-Expect-Token' => $current ) +); +check( 'PUT with the current token -> 200', 200 === $accepted->get_status() ); +check( 'accepted PUT landed', 999 === (int) ( troche_rest( 'GET', '/troche/v1/library' )->get_data()['songs'][0]['bpm'] ?? 0 ) ); +check( 'accepted PUT moved the token', $current !== ( troche_tokens()[ (string) $wp_id ] ?? '' ) ); + +// A caller that doesn't track tokens (or a header a proxy stripped) still +// writes, exactly as this endpoint always did. +check( 'PUT with no token header still writes', 200 === troche_rest( 'PUT', '/troche/v1/songs/' . $wp_id, $edited )->get_status() ); + +// ---- /library/state agrees with /library about what exists ---- +// A post the library skips must not appear in state, or every probe would read +// as drift and pull the whole library down again. +$junk_id = wp_insert_post( + array( + 'post_type' => 'troche_song', + 'post_status' => 'publish', + 'post_title' => 'Not JSON', + 'post_content' => 'this is not a song', + ) +); +check( 'unparseable post excluded from library', 1 === count( troche_rest( 'GET', '/troche/v1/library' )->get_data()['songs'] ) ); +check( 'unparseable post excluded from state', ! isset( troche_tokens()[ (string) $junk_id ] ) ); +wp_delete_post( $junk_id, true ); + // ---- wp-admin cap mapping (post-type actions gate on troche_edit, not core post caps) ---- $editor_id = wp_insert_user( array( @@ -161,6 +214,10 @@ function troche_rest( $method, $route, $body = null ) { check( 'DELETE /songs/{id} -> 200', 200 === troche_rest( 'DELETE', '/troche/v1/songs/' . $wp_id )->get_status() ); check( 'post moved to trash (not hard-deleted)', 'trash' === get_post_status( $wp_id ) ); check( 'trashed song excluded from library', 0 === count( troche_rest( 'GET', '/troche/v1/library' )->get_data()['songs'] ) ); +// The client reads a song's absence from state as "trashed elsewhere", so this +// is what makes deletions propagate to the other machine at all. +check( 'trashed song excluded from state', ! isset( troche_tokens()[ (string) $wp_id ] ) ); +check( 'PUT to a trashed song -> 404, not 409', 404 === troche_rest( 'PUT', '/troche/v1/songs/' . $wp_id, $song, array( 'X-Troche-Expect-Token' => 'anything' ) )->get_status() ); // ---- server assigns an id when missing ---- $r = troche_rest( 'POST', '/troche/v1/songs', array( 'name' => 'No Id', 'parts' => array() ) ); diff --git a/tests/sync.mjs b/tests/sync.mjs index 9bdec54..dab66ce 100644 --- a/tests/sync.mjs +++ b/tests/sync.mjs @@ -16,8 +16,9 @@ const check = (label, cond) => { // ---- fake server ---- // Mirrors class-store.php: songs live keyed by post id, tokens are derived from -// stored content (so an identical re-save doesn't move one), and wpId/wpToken -// are stripped on the way in and decorated on the way out. +// stored content (so an identical re-save doesn't move one), wpId/wpToken are +// stripped on the way in and decorated on the way out, and a PUT carrying +// X-Troche-Expect-Token is refused with 409 if the song has moved since. function makeServer(initial = {}) { const posts = new Map(Object.entries(initial).map(([k, v]) => [Number(k), v])); let nextId = 200; @@ -27,6 +28,12 @@ function makeServer(initial = {}) { const server = { posts, calls: [], + // Expected-token header per PUT, in call order, so a test can assert the + // client offered one at all — a silently omitted header would downgrade + // every write back to unconditional without failing anything else. + expectations: [], + down: false, // network failure + auth: true, // Direct mutation, standing in for "the other laptop saved". edit(wpId, patch) { posts.set(wpId, { ...posts.get(wpId), ...patch }); @@ -51,6 +58,10 @@ function makeServer(initial = {}) { server.calls.push(method + " " + route); const body = opts.body ? JSON.parse(opts.body) : null; const ok = (status, data) => ({ ok: true, status, json: async () => data }); + const err = (status) => ({ ok: false, status, json: async () => ({}) }); + + if (server.down) throw new TypeError("network error"); + if (!server.auth) return err(403); if (route === "/library") { return ok(200, { @@ -70,11 +81,14 @@ function makeServer(initial = {}) { const match = route.match(/^\/songs\/(\d+)$/); if (match) { const wpId = Number(match[1]); - if (!posts.has(wpId)) return { ok: false, status: 404, json: async () => ({}) }; + if (!posts.has(wpId)) return err(404); if (method === "DELETE") { posts.delete(wpId); return ok(200, { deleted: true, wpId }); } + const expect = (opts.headers || {})["X-Troche-Expect-Token"] ?? null; + server.expectations.push(expect); + if (expect !== null && expect !== token(posts.get(wpId))) return err(409); posts.set(wpId, strip(body)); return ok(200, server.decorate(wpId)); } @@ -257,5 +271,91 @@ const names = (lib) => lib.songs.map((s) => s.name); ); } +// ---- 10. a song rewritten between our sync and our write is refused ---- +// The sync-before-save narrows this window; the conditional PUT closes it. +{ + const server = makeServer({ 1: song("a", "Alpha"), 2: song("b", "Beta") }); + const storage = await freshStorage(); + const lib = await storage.loadLibrary(); + + const local = { ...lib, songs: [lib.songs[0], { ...lib.songs[1], bpm: 90 }] }; + server.edit(2, { bpm: 155 }); // the other laptop, after our last sync + + const res = await storage.saveLibrary(local); + check("a write onto a moved song is refused", res.ok === false && res.diverged === true); + check("refused write isn't reported as offline", !res.offline); + check("the other laptop's copy is intact", server.posts.get(2).bpm === 155); + check("the PUT carried an expected token", server.expectations.every((t) => !!t)); + + // ...and the follow-up sync turns it into a flag the user can act on. + const result = await storage.syncUpstream(local); + check("the refused write becomes a conflict", result.conflicts.length === 1); + check("conflict names the song", result.conflicts[0].name === "Beta"); +} + +// ---- 11. a song trashed between our sync and our write reports diverged ---- +// Used to surface as "Offline — changes kept locally", which it isn't. +{ + const server = makeServer({ 1: song("a", "Alpha"), 2: song("b", "Beta") }); + const storage = await freshStorage(); + const lib = await storage.loadLibrary(); + + const local = { ...lib, songs: [lib.songs[0], { ...lib.songs[1], bpm: 90 }] }; + server.trash(2); + + const res = await storage.saveLibrary(local); + check("a write onto a trashed song reports diverged", res.diverged === true); + check("a trashed song isn't reported as offline", !res.offline); + + const result = await storage.syncUpstream(local); + check("the failed write becomes an orphan", result.orphans.length === 1); +} + +// ---- 12. an unresolved conflict re-reports with the newer copy ---- +// "Use theirs" has to mean their copy as it stands now; adopting the one we +// first saw would push it straight back over their newer one. +{ + const server = makeServer({ 1: song("a", "Alpha"), 2: song("b", "Beta") }); + const storage = await freshStorage(); + const lib = await storage.loadLibrary(); + + server.edit(2, { bpm: 155 }); + const local = { ...lib, songs: [lib.songs[0], { ...lib.songs[1], bpm: 90 }] }; + const first = await storage.syncUpstream(local); + check("first sync flags the conflict", first.conflicts[0].theirs.bpm === 155); + + server.edit(2, { bpm: 175 }); // they edit again while we sit on the notice + const second = await storage.syncUpstream(first.library); + check("a re-edit re-reports the conflict", second.conflicts.length === 1); + check("the re-report carries their current copy", second.conflicts[0].theirs.bpm === 175); + + // Nothing new upstream: the flag isn't re-reported, but the hold stands. + const third = await storage.syncUpstream(second.library); + check("a quiet sync doesn't re-report a held song", third === null); + await storage.saveLibrary(second.library); + check("the hold survives a quiet sync", server.posts.get(2).bpm === 175); +} + +// ---- 13. probe failures don't take anything down with them ---- +{ + const server = makeServer({ 1: song("a", "Alpha") }); + const storage = await freshStorage(); + const lib = await storage.loadLibrary(); + + server.down = true; + check("an offline probe reconciles nothing", (await storage.syncUpstream(lib)) === null); + + server.down = false; + server.auth = false; + check("an expired session is reported, not swallowed", (await storage.syncUpstream(lib))?.authExpired === true); +} + +// ---- 14. no server, nothing to sync ---- +{ + globalThis.window = {}; // no trocheWP: standalone/localStorage mode + const storage = await import("../src/storage.js?case=local"); + check("syncUpstream is a no-op outside WP mode", (await storage.syncUpstream({ songs: [] })) === null); +} + console.log(`\n=== ${pass} passed, ${fail} failed ===`); process.exit(fail ? 1 : 0); diff --git a/tests/unit.mjs b/tests/unit.mjs index 07f1099..3855031 100644 --- a/tests/unit.mjs +++ b/tests/unit.mjs @@ -1,6 +1,6 @@ // Unit tests for pure client logic (no browser, no WordPress). // node tests/unit.mjs # or: npm run test:unit -import { normalizeLibrary } from "../src/utils.js"; +import { normalizeLibrary, mergeFlags } from "../src/utils.js"; let pass = 0; let fail = 0; @@ -49,5 +49,37 @@ check("no double-suffix", !r2.library.songs.some((s) => /\(\d+\) \(\d+\)/.test(s const r3 = normalizeLibrary({ activeId: "a", songs: [{ id: "a", name: "One" }, { id: "b", name: "Two" }] }); check("clean library unchanged", r3.changed === false); +// ---- mergeFlags: the songs a sync says need a decision ---- + +const conflict = (wpId, bpm) => ({ wpId, id: "s" + wpId, name: "S" + wpId, theirs: { bpm } }); +const orphan = (wpId) => ({ wpId, id: "s" + wpId, name: "S" + wpId }); + +let f = mergeFlags([], [conflict(2, 155)], []); +check("a conflict is flagged", f.length === 1 && f[0].kind === "conflict"); +check("a conflict carries their copy", f[0].theirs.bpm === 155); + +// Dismissing a flag is what releases that song's save hold, so a flag the sync +// didn't mention has to survive — dropping it would strand the song unsaveable. +f = mergeFlags(f, [], []); +check("a flag the sync didn't mention survives", f.length === 1 && f[0].wpId === 2); + +// Re-reported: the other machine moved again, so their copy has to move with +// it. Keeping the first one would push a stale version back over their newer. +f = mergeFlags(f, [conflict(2, 175)], []); +check("a re-reported song isn't duplicated", f.length === 1); +check("a re-reported song adopts the newer copy", f[0].theirs.bpm === 175); + +// Trashed elsewhere after the conflict: there's no "theirs" left to take. +f = mergeFlags(f, [], [orphan(2)]); +check("an orphan supersedes a conflict on the same song", f.length === 1 && f[0].kind === "orphan"); + +// Existing entries hold their place; new ones append. +f = mergeFlags([conflict(2, 1), conflict(3, 1)], [conflict(3, 2)], [orphan(9)]); +check( + "flags keep their order, new ones append", + JSON.stringify(f.map((x) => x.wpId)) === JSON.stringify([2, 3, 9]) +); +check("only the re-reported entry changes", f[0].theirs.bpm === 1 && f[1].theirs.bpm === 2); + console.log(`\n=== ${pass} passed, ${fail} failed ===`); process.exit(fail ? 1 : 0); diff --git a/wp-plugin/includes/class-rest-controller.php b/wp-plugin/includes/class-rest-controller.php index 4c2b963..5d3ebc6 100644 --- a/wp-plugin/includes/class-rest-controller.php +++ b/wp-plugin/includes/class-rest-controller.php @@ -8,6 +8,13 @@ * - PUT /songs/{id} Update a song (troche_edit required). * - DELETE /songs/{id} Trash a song (troche_edit required). * + * PUT honours an optional X-Troche-Expect-Token header carrying the version + * token the client last saw; the update is refused with 409 if the song has + * moved since. A custom header rather than If-Match on purpose: an If-Match a + * proxy or cache decides to evaluate itself would fail the save outright, + * whereas a stripped custom header just degrades to the unconditional write + * this endpoint has always done. + * * Auth is cookie + nonce (same-origin); there is no CORS surface. Reads gate on * being logged in; writes gate on the troche_edit capability. * @@ -175,8 +182,11 @@ private function write( \WP_REST_Request $request, $wp_id ) { ); } + // Only meaningful on an update; a create has nothing to be stale against. + $expect = $wp_id ? $request->get_header( 'x_troche_expect_token' ) : null; + $song = Store::sanitize_song( $body ); - $saved = Store::save_song( $song, $wp_id, get_current_user_id() ); + $saved = Store::save_song( $song, $wp_id, get_current_user_id(), $expect ); if ( is_wp_error( $saved ) ) { return $saved; diff --git a/wp-plugin/includes/class-store.php b/wp-plugin/includes/class-store.php index 3d5cce8..17ccbd5 100644 --- a/wp-plugin/includes/class-store.php +++ b/wp-plugin/includes/class-store.php @@ -185,12 +185,17 @@ private static function token( $content ) { /** * Create or update one song. * - * @param array $song Sanitized song object. - * @param int|null $wp_id Existing post id to update, or null to create. - * @param int $user Author id for new posts. + * @param array $song Sanitized song object. + * @param int|null $wp_id Existing post id to update, or null to create. + * @param int $user Author id for new posts. + * @param string|null $expect Version token the caller believes is current. + * When given, the update only lands if the stored + * token still matches; otherwise 409. Null skips + * the check (creates, and callers that don't + * track tokens). * @return array|\WP_Error The saved song (with wpId), or an error. */ - public static function save_song( array $song, $wp_id, $user ) { + public static function save_song( array $song, $wp_id, $user, $expect = null ) { $title = isset( $song['name'] ) && '' !== trim( (string) $song['name'] ) ? (string) $song['name'] : __( 'Untitled Song', 'troche' ); @@ -225,6 +230,26 @@ public static function save_song( array $song, $wp_id, $user ) { array( 'status' => 404 ) ); } + + // Conditional write. The client syncs before saving, but that check + // and this write aren't one operation — another machine can land a + // save in between. Comparing tokens here, immediately before the + // write, is what makes "your edit never disappears" true rather + // than merely likely. A mismatch is not an error the user needs to + // see: the client reconciles and asks, naming the song. + $current = self::token( $existing->post_content ); + if ( null !== $expect && $expect !== $current ) { + return new \WP_Error( + 'troche_stale_token', + __( 'That song changed somewhere else since you last synced.', 'troche' ), + array( + 'status' => 409, + 'wpId' => (int) $wp_id, + 'wpToken' => $current, + ) + ); + } + $postarr['ID'] = (int) $wp_id; $result = wp_update_post( $postarr, true ); } else {