fix: an imported backup no longer rolls progress back
The progress store merged like the mark stores do, keeping whichever copy had the earlier timestamp. That is right for a mark — `at` is when the reader made it, membership is the whole fact, and the oldest stamp is the truest one — and wrong here, because this record *is* the data. A status, an effort, a note and whether an event repeats all live in it, and `at` says when one of those last changed. So the earlier copy winning discarded every edit made after it, in both of the directions an import actually happens in: restoring a backup taken before an evening's work undid the evening, and importing an old file onto a device with newer progress rolled the device back. Neither is recoverable — there is no account and no server holding a second copy. The later record wins now. Nothing is removed in either direction, and taking the maximum of two timestamps is as order-independent and idempotent as the rule it replaces. `mergeProgress` came out of the hook to be tested, which is also how the ignored store's opposite rule is now written down rather than assumed. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
This commit is contained in:
co-authored by
Claude Opus 5
parent
7bb7dc843c
commit
f9ef21a366
+12
-1
@@ -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 **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
|
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
|
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
|
deliberately one-directional. A file whose `format` is unrecognised is refused outright rather than
|
||||||
partly applied.
|
partly applied.
|
||||||
|
|||||||
@@ -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() {
|
export function useProgress() {
|
||||||
const [progress, setProgress] = useState<ProgressMap>(load);
|
const [progress, setProgress] = useState<ProgressMap>(load);
|
||||||
|
|
||||||
@@ -112,17 +156,8 @@ export function useProgress() {
|
|||||||
[patch],
|
[patch],
|
||||||
);
|
);
|
||||||
|
|
||||||
/** Union merge on import, keeping the earlier entry. Never removes. */
|
|
||||||
const merge = useCallback((incoming: ProgressMap) => {
|
const merge = useCallback((incoming: ProgressMap) => {
|
||||||
setProgress((prev) => {
|
setProgress((prev) => mergeProgress(prev, incoming));
|
||||||
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;
|
|
||||||
});
|
|
||||||
}, []);
|
}, []);
|
||||||
|
|
||||||
return {
|
return {
|
||||||
|
|||||||
@@ -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"]);
|
||||||
|
});
|
||||||
|
});
|
||||||
Reference in New Issue
Block a user