fix: serialize snapshot mutations, catch export/plan throws in event handlers
Fixes the three findings from the final whole-branch review:
- SnapshotStore.mutate/replace now serialize behind a promise queue, so
concurrent mutate() calls (e.g. two independent onChange handlers firing
before either write resolves) each clone from the previous commit instead
of racing on a stale snapshot and silently losing an edit. Preserves
write-before-move-snapshot, caller-visible rejections, structuredClone
identity, and non-wedging on a failed write. Adds tests for concurrent
mutations and a failing write mid-queue.
- DataSection's exportJson/exportOds are wrapped in try/catch and report
failures through the existing import error state/rendering, since a throw
(e.g. MissingRateError from buildOds) inside a plain onClick is invisible
to the error boundary. The pre-import JSON backup now explicitly aborts
the destructive replace if the backup export fails, rather than relying on
an uncaught throw to prevent it.
- PlanEditor no longer calls monthOf()/withPlan() with an empty or partial
date from a native <input type="date">, and requires an integer month
count (with step={1}) before calling withPlan/equalSlices, preventing
both domain throws from firing inside plain event handlers.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
da0d1ac35c
commit
ed1667f8a0
@@ -83,6 +83,73 @@ describe("mutate", () => {
|
||||
).rejects.toThrow("disk full");
|
||||
expect(store.get().posts[0]!.name).toBe("Video Games");
|
||||
});
|
||||
|
||||
test("two concurrent mutate() calls both survive (no lost update)", async () => {
|
||||
// Simulate real persistence latency: each write takes ~20ms, so if the
|
||||
// two mutations clone concurrently instead of queueing, the second
|
||||
// commit silently overwrites the first's change.
|
||||
let stored = createSeedDataset("2026-09");
|
||||
const persistence: Persistence = {
|
||||
async read() {
|
||||
return stored;
|
||||
},
|
||||
async write(dataset) {
|
||||
await new Promise((resolve) => setTimeout(resolve, 20));
|
||||
stored = dataset;
|
||||
},
|
||||
};
|
||||
const store = createSnapshotStore(persistence, "2026-09");
|
||||
await store.load();
|
||||
|
||||
const first = store.mutate((draft) => {
|
||||
draft.months[0]!.income = { amount: 20000, currency: "DKK" };
|
||||
});
|
||||
const second = store.mutate((draft) => {
|
||||
draft.posts[0]!.name = "Renamed";
|
||||
});
|
||||
await Promise.all([first, second]);
|
||||
|
||||
expect(store.get().months[0]!.income.amount).toBe(20000);
|
||||
expect(store.get().posts[0]!.name).toBe("Renamed");
|
||||
expect(stored.months[0]!.income.amount).toBe(20000);
|
||||
expect(stored.posts[0]!.name).toBe("Renamed");
|
||||
});
|
||||
|
||||
test("a failing write mid-queue rejects only its own caller and does not wedge later mutations", async () => {
|
||||
const fake = fakePersistence(createSeedDataset("2026-09"));
|
||||
// The queue processes mutations strictly in order, so failing the
|
||||
// SECOND write call (regardless of when each mutate() is invoked)
|
||||
// reliably targets the middle mutation below.
|
||||
let writeCount = 0;
|
||||
const persistence: Persistence = {
|
||||
read: fake.persistence.read,
|
||||
async write(dataset) {
|
||||
writeCount++;
|
||||
if (writeCount === 2) throw new Error("disk full");
|
||||
await fake.persistence.write(dataset);
|
||||
},
|
||||
};
|
||||
const store = createSnapshotStore(persistence, "2026-09");
|
||||
await store.load();
|
||||
|
||||
const okBefore = store.mutate((draft) => {
|
||||
draft.months[0]!.income = { amount: 111, currency: "DKK" };
|
||||
});
|
||||
const failing = store.mutate((draft) => {
|
||||
draft.posts[0]!.name = "Should not stick";
|
||||
});
|
||||
const okAfter = store.mutate((draft) => {
|
||||
draft.posts[1]!.name = "Should stick";
|
||||
});
|
||||
|
||||
await okBefore;
|
||||
await expect(failing).rejects.toThrow("disk full");
|
||||
await okAfter;
|
||||
|
||||
expect(store.get().months[0]!.income.amount).toBe(111);
|
||||
expect(store.get().posts[0]!.name).not.toBe("Should not stick");
|
||||
expect(store.get().posts[1]!.name).toBe("Should stick");
|
||||
});
|
||||
});
|
||||
|
||||
test("replace swaps the whole dataset, as JSON import needs", async () => {
|
||||
|
||||
+24
-6
@@ -23,6 +23,22 @@ export function createSnapshotStore(
|
||||
let snapshot: Dataset | null = null;
|
||||
const listeners = new Set<Listener>();
|
||||
|
||||
// All mutations run through this chain, one at a time, so a mutation
|
||||
// always clones from the snapshot the previous mutation committed rather
|
||||
// than from a stale snapshot captured before it. Without this, two
|
||||
// mutations issued before either's write resolves both clone the same
|
||||
// starting point and the one that commits last silently discards the
|
||||
// other's change.
|
||||
let queue: Promise<unknown> = Promise.resolve();
|
||||
|
||||
function enqueue<T>(work: () => Promise<T>): Promise<T> {
|
||||
const run = queue.then(work);
|
||||
// Keep the chain alive after a failure, but the caller still sees the
|
||||
// rejection via `run`, which is returned unmodified below.
|
||||
queue = run.catch(() => undefined);
|
||||
return run;
|
||||
}
|
||||
|
||||
function notify() {
|
||||
for (const listener of listeners) listener();
|
||||
}
|
||||
@@ -53,14 +69,16 @@ export function createSnapshotStore(
|
||||
|
||||
get,
|
||||
|
||||
async mutate(fn) {
|
||||
const draft = structuredClone(get());
|
||||
fn(draft);
|
||||
await commit(draft);
|
||||
mutate(fn) {
|
||||
return enqueue(async () => {
|
||||
const draft = structuredClone(get());
|
||||
fn(draft);
|
||||
await commit(draft);
|
||||
});
|
||||
},
|
||||
|
||||
async replace(dataset) {
|
||||
await commit(dataset);
|
||||
replace(dataset) {
|
||||
return enqueue(() => commit(dataset));
|
||||
},
|
||||
|
||||
subscribe(listener) {
|
||||
|
||||
Reference in New Issue
Block a user