diff --git a/src/app/components/ChartBuilderModal.test.tsx b/src/app/components/ChartBuilderModal.test.tsx index b8b4b95..8201e60 100644 --- a/src/app/components/ChartBuilderModal.test.tsx +++ b/src/app/components/ChartBuilderModal.test.tsx @@ -53,7 +53,9 @@ describe('ChartBuilderModal', () => { now: T, }); useDatasetStore.getState().add(ds); - useChartBuilderStore.getState().init(ds.id); + // `add` reassigns a collision-free id (it is the id authority), so read it back. + const id = useDatasetStore.getState().datasets[0].id; + useChartBuilderStore.getState().init(id); useChartBuilderStore.getState().setMark('bar'); expect(useChartBuilderStore.getState().config.mark).toBe('bar'); diff --git a/src/app/stores/ChartBuilderStore.test.ts b/src/app/stores/ChartBuilderStore.test.ts index a1f390c..3a8edaf 100644 --- a/src/app/stores/ChartBuilderStore.test.ts +++ b/src/app/stores/ChartBuilderStore.test.ts @@ -11,7 +11,9 @@ const T = new Date('2026-06-01T00:00:00Z'); function seedDataset(name: string, data: unknown): number { const ds = createDataset({ name, data, format: 'json', source: 'inline', now: T }); useDatasetStore.getState().add(ds); - return ds.id; + // `add` is the id authority and reassigns a collision-free id, so read it back + // rather than trusting the pre-add createDataset default. + return useDatasetStore.getState().datasets[0].id; } beforeEach(() => { diff --git a/src/app/stores/DatasetStore.test.ts b/src/app/stores/DatasetStore.test.ts index 773c9e9..2469162 100644 --- a/src/app/stores/DatasetStore.test.ts +++ b/src/app/stores/DatasetStore.test.ts @@ -186,3 +186,63 @@ describe('remove & view transitions', () => { expect(store().view).toBe('detail'); }); }); + +describe('id assignment — the store is the collision-free id authority', () => { + test('add reassigns a fresh id even when two creates share a Date.now() id', () => { + // Simulate the tight-loop hazard: two datasets minted with the SAME injected id + // (what Date.now() yields within one millisecond). add must still give them + // distinct, monotonically increasing ids so they cannot collide in IndexedDB. + store().add( + createDataset({ + name: 'A', + data: [{ a: 1 }], + format: 'json', + source: 'inline', + now: T, + id: 42, + }), + ); + store().add( + createDataset({ + name: 'B', + data: [{ a: 1 }], + format: 'json', + source: 'inline', + now: T, + id: 42, + }), + ); + const ids = store().datasets.map((d) => d.id); + expect(new Set(ids).size).toBe(2); + // selectedId tracks the reassigned id of the most recent add, not the input 42. + expect(store().selectedId).toBe(store().datasets[0].id); + }); + + test('addDatasets gives a batch distinct ids past the existing maximum', () => { + store().add( + createDataset({ name: 'Seed', data: [{ a: 1 }], format: 'json', source: 'inline', now: T }), + ); + const seedId = store().datasets[0].id; + store().addDatasets([ + createDataset({ + name: 'X', + data: [{ a: 1 }], + format: 'json', + source: 'inline', + now: T, + id: 42, + }), + createDataset({ + name: 'Y', + data: [{ a: 1 }], + format: 'json', + source: 'inline', + now: T, + id: 42, + }), + ]); + const ids = store().datasets.map((d) => d.id); + expect(new Set(ids).size).toBe(3); + expect(Math.min(...ids.filter((i) => i !== seedId))).toBeGreaterThan(seedId); + }); +}); diff --git a/src/app/stores/DatasetStore.ts b/src/app/stores/DatasetStore.ts index 13b2974..4064608 100644 --- a/src/app/stores/DatasetStore.ts +++ b/src/app/stores/DatasetStore.ts @@ -67,14 +67,19 @@ export interface DatasetState { */ save: (now?: Date) => boolean; - /** Low-level: add a fully-formed dataset and select it. */ + /** + * Low-level: add a fully-formed dataset and select it. The id is (re)assigned + * via `nextDatasetId`, so a `createDataset` default id (`Date.now()`) can never + * reach storage — see `nextDatasetId` for why that matters. + */ add: (dataset: Dataset) => void; /** * Append imported datasets (spec §08 → datasets imported before snippets). Each - * is given a fresh monotonic numeric id so a batch never collides on the - * `Date.now()` default (the `add` TODO) nor with existing ids — safe because - * datasets are referenced by **name**, not id (docs/architecture/07 §1). Names - * are assumed already de-duped by the import service. Selection is unchanged. + * is given a fresh monotonic numeric id (`nextDatasetId`, the same authority as + * `add`) so a batch never collides on the `Date.now()` default nor with existing + * ids — safe because datasets are referenced by **name**, not id + * (docs/architecture/07 §1). Names are assumed already de-duped by the import + * service. Selection is unchanged. */ addDatasets: (incoming: Dataset[]) => void; /** Low-level: merge a patch into a dataset, advancing `modified`. */ @@ -91,6 +96,20 @@ export function byModifiedDesc(a: Dataset, b: Dataset): number { return b.modified.localeCompare(a.modified); } +/** + * Next free numeric id for a dataset: one past the current maximum. This store is + * the **single id authority** — both single (`add`) and batch (`addDatasets`) + * insertion route through it — so a `createDataset` default id (`Date.now()`) + * never reaches IndexedDB, where a tight creation loop (the documented import path) + * could repeat the same millisecond and collide on the numeric key. Dataset ids + * are an internal IndexedDB key only — snippets reference datasets by **name** + * (docs/architecture/07 §1) — so reusing an id freed by a deletion is harmless, + * which is why one-past-the-max suffices without a persistent counter. + */ +function nextDatasetId(datasets: ReadonlyArray): number { + return datasets.reduce((max, d) => Math.max(max, d.id), 0) + 1; +} + /** * Cheap form validation: returns an error message, or `null` when the form is * saveable. Deliberately does **not** `JSON.parse` the input — it only checks @@ -256,18 +275,16 @@ export const useDatasetStore = create((set, get) => ({ return true; }, - // TODO: createDataset ids default to `Date.now()` and `add` does not reassign - // on collision (despite that file's comment). Interactive create is safe, but - // the documented non-interactive import path (naming.ts) creates many datasets - // in a tight loop where `Date.now()` repeats — duplicate numeric keys would - // collide in IndexedDB. Give the store a monotonic id source (or reassign here) - // before wiring import. - add: (dataset) => set((s) => ({ datasets: [dataset, ...s.datasets], selectedId: dataset.id })), + add: (dataset) => + set((s) => { + const withId = { ...dataset, id: nextDatasetId(s.datasets) }; + return { datasets: [withId, ...s.datasets], selectedId: withId.id }; + }), addDatasets: (incoming) => { if (incoming.length === 0) return; set((s) => { - let nextId = s.datasets.reduce((max, d) => Math.max(max, d.id), 0) + 1; + let nextId = nextDatasetId(s.datasets); const withIds = incoming.map((d) => ({ ...d, id: nextId++ })); return { datasets: [...withIds, ...s.datasets] }; });