diff --git a/docs/DATA-MODEL.md b/docs/DATA-MODEL.md index 5d33883..35de805 100644 --- a/docs/DATA-MODEL.md +++ b/docs/DATA-MODEL.md @@ -563,7 +563,18 @@ event whose lane is missing. Import **merges** every set: a mark present in either the file or the current device survives, and import never removes one. `daily` merges as a **union of days per ID** — every day either side -recorded is a day the reader actually played. An export written before daily checklists existed +recorded is a day the reader actually played. + +**Which copy wins a conflict differs by store, and the difference is the shape of the data.** For +`ignored` (and the legacy `completions`) the **earlier** mark wins: `at` records when the reader made +it, membership in the set is the whole fact, and the oldest timestamp is the truest answer to when +they said it. For `progress` the **later** record wins, because there the record *is* the data — a +status, an effort, a note, whether it repeats — and `at` is when one of those last changed. Keeping +the earlier copy there discards every edit made after it, in both directions an import happens in: +restoring a backup taken before an evening's work would roll that evening back, and importing an old +file onto a device with newer progress would roll the device back. Taking the maximum of the two +timestamps keeps the merge order-independent and idempotent either way, and removes nothing. +`mergeProgress` is pure and `test/progress.test.ts` pins it. An export written before daily checklists existed simply has no `daily` key, which is not an error. Losing a user's marks to a bad import is unrecoverable, so the merge is deliberately one-directional. A file whose `format` is unrecognised is refused outright rather than partly applied. diff --git a/src/client/state/useProgress.ts b/src/client/state/useProgress.ts index 8a8e78b..b08403a 100644 --- a/src/client/state/useProgress.ts +++ b/src/client/state/useProgress.ts @@ -62,6 +62,50 @@ function isEmpty(p: Progress): boolean { ); } +/** + * Union merge on import, keeping whichever copy was touched last. Never removes. + * + * `useMarkSet` keeps the **earlier** of two marks and is right to. There `at` is + * when the reader made the mark, membership in the set is the whole fact, and + * the oldest timestamp is the truest answer to "when did they say this?" — + * nothing is lost by preferring it. + * + * This store is the opposite shape, and it was merging the same way. Here the + * record *is* the data — a status, an effort, a note, whether it repeats — and + * `at` is when they last changed one of those. Keeping the earlier copy + * therefore discards every edit made after it, in both of the directions an + * import actually happens in: restoring a backup taken before an evening's work + * rolls that evening back, and importing an old file into a device with newer + * progress rolls the device back. Neither is recoverable, because nothing else + * holds a copy. + * + * So the later record wins. Nothing is removed either way — an id present on + * only one side always survives — which is the guarantee docs/DATA-MODEL.md + * § Import makes, and taking the maximum of two timestamps keeps the merge + * order-independent and idempotent exactly as the old rule was. + * + * A record whose `at` is not a string is an import that has been edited or + * truncated. It can still land under an id nothing holds yet, but it never wins + * a comparison against a record that does carry one. + */ +export function mergeProgress( + current: ProgressMap, + incoming: ProgressMap, +): ProgressMap { + const touchedAt = (p: Progress): string => + typeof p.at === "string" ? p.at : ""; + + const next = { ...current }; + for (const [id, value] of Object.entries(incoming)) { + const existing = next[id]; + next[id] = + existing === undefined || touchedAt(value) > touchedAt(existing) + ? value + : existing; + } + return next; +} + export function useProgress() { const [progress, setProgress] = useState(load); @@ -112,17 +156,8 @@ export function useProgress() { [patch], ); - /** Union merge on import, keeping the earlier entry. Never removes. */ const merge = useCallback((incoming: ProgressMap) => { - setProgress((prev) => { - const next = { ...prev }; - for (const [id, value] of Object.entries(incoming)) { - const existing = next[id]; - next[id] = - existing === undefined || value.at < existing.at ? value : existing; - } - return next; - }); + setProgress((prev) => mergeProgress(prev, incoming)); }, []); return { diff --git a/test/progress.test.ts b/test/progress.test.ts new file mode 100644 index 0000000..103930d --- /dev/null +++ b/test/progress.test.ts @@ -0,0 +1,72 @@ +import { describe, expect, test } from "bun:test"; +import { mergeProgress } from "../src/client/state/useProgress.ts"; +import type { ProgressMap } from "../src/client/state/useProgress.ts"; + +/** + * How an imported file meets the progress already on the device. + * + * This store is the only copy of what the reader has said about an event — + * status, effort, note, whether it repeats — and there is no account and no + * server holding a second one. So the merge has exactly two obligations: never + * drop an id, and never roll an answer back to an older one. Both directions of + * that second clause are real, because an import is as often a backup being + * restored as it is a second device arriving. + */ + +const at = (iso: string) => `2026-08-${iso}T12:00:00.000Z`; + +describe("mergeProgress", () => { + test("keeps an id that only one side has, from either side", () => { + const device: ProgressMap = { a: { status: "done", at: at("10") } }; + const file: ProgressMap = { b: { status: "doing", at: at("11") } }; + expect(Object.keys(mergeProgress(device, file)).sort()).toEqual(["a", "b"]); + expect(Object.keys(mergeProgress(file, device)).sort()).toEqual(["a", "b"]); + }); + + test("the later record wins, so a newer edit is not rolled back", () => { + // The bug this replaces kept the *earlier* copy, which is right for a mark + // — where `at` is when it was made — and wrong here, where the record is + // the data and `at` is when it last changed. Restoring a backup taken + // before an evening's work would have undone the evening. + const older: ProgressMap = { a: { status: "doing", at: at("10") } }; + const newer: ProgressMap = { + a: { status: "done", effort: "grind", note: "two more runs", at: at("14") }, + }; + + expect(mergeProgress(older, newer).a).toEqual(newer.a); + // And the same answer whichever way round it is applied, so restoring an + // old file over newer progress does not roll the device back either. + expect(mergeProgress(newer, older).a).toEqual(newer.a); + }); + + test("merging is idempotent and order-independent", () => { + // Taking the maximum of two timestamps keeps both properties, which is what + // makes importing the same file twice harmless. + const a: ProgressMap = { x: { status: "done", at: at("10") } }; + const b: ProgressMap = { x: { status: "doing", at: at("12") } }; + const once = mergeProgress(a, b); + expect(mergeProgress(once, b)).toEqual(once); + expect(mergeProgress(b, a)).toEqual(once); + }); + + test("a record with no timestamp lands, but never wins", () => { + // An import is untrusted input: a file edited by hand or truncated can carry + // a record with no `at`. It is still the reader's data, so it is kept under + // an id nothing holds — but it must not overwrite a record that does say + // when it was touched. + const broken = { at: undefined } as unknown as ProgressMap[string]; + const device: ProgressMap = { a: { status: "done", at: at("10") } }; + + expect(mergeProgress(device, { a: broken }).a?.status).toBe("done"); + expect(mergeProgress(device, { fresh: broken }).fresh).toBe(broken); + }); + + test("nothing is ever removed, whatever the file says", () => { + // The one guarantee docs/DATA-MODEL.md § Import actually makes. + const device: ProgressMap = { + a: { status: "done", at: at("10") }, + b: { note: "later", at: at("11") }, + }; + expect(Object.keys(mergeProgress(device, {})).sort()).toEqual(["a", "b"]); + }); +});