Skip to content

Commit 2414d56

Browse files
committed
Throw on a corrupt per-tab value and recover at the seam
parseSessionJson swallowed both JSON and schema-validation errors into an indistinguishable null, hiding corruption and conflating it with a legitimate absent value. Separate detecting corruption from deciding what to do about it: deserialize now returns null only for an absent value and throws (SyntaxError or ZodError) on a present-but-invalid one. createSessionScopedAtom owns seeding policy, so it is the seam that catches: a corrupt per-tab value (or a sessionStorage read that throws a SecurityError when DOM storage is blocked) is logged and treated as a miss, falling through to the breadcrumb then the default rather than crashing app startup. Update the per-tab-storage ADR to describe the throw-and-recover flow.
1 parent cc929ee commit 2414d56

5 files changed

Lines changed: 77 additions & 21 deletions

File tree

docs/adr/20260630-per-tab-session-scoped-storage-primitive.md

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ Extract the per-tab + breadcrumb mechanism into one primitive, `createSessionSco
2222

2323
`createActiveConfigurationAtom` is refactored onto the primitive rather than left as a parallel implementation, so the seed-and-claim logic lives in exactly one place with one set of tests.
2424

25-
A per-tab value crosses two backings with different serialization needs, so the primitive takes a **`SessionValueCodec<T>`**: the breadcrumb keeps the native value (structured clone preserves a `Set`), while `sessionStorage` holds only strings. The codec's `deserialize` validates the parsed string with **zod** and returns `null` for a missing, unparseable, or wrong-shape value, so a stale or hand-edited per-tab value falls through to the breadcrumb instead of seeding a bad shape. Graph-view layout's codec serializes its `activeToggles` `Set` as an array and rebuilds it on read via a zod `.transform`; schema-view layout is plain JSON. Active Connection's value is a bare id string, so its codec passes the string through and skips zod.
25+
A per-tab value crosses two backings with different serialization needs, so the primitive takes a **`SessionValueCodec<T>`**: the breadcrumb keeps the native value (structured clone preserves a `Set`), while `sessionStorage` holds only strings. The codec's `deserialize` returns `null` only for an absent value (a legitimate miss) and validates a present value with **zod**, _throwing_ on an unparseable or wrong-shape value rather than swallowing it. Detecting corruption is thus separate from deciding what to do about it: the seam (`createSessionScopedAtom`) catches the throw, logs it, and treats it as a miss so a stale or hand-edited per-tab value falls through to the breadcrumb instead of seeding a bad shape or crashing startup. Graph-view layout's codec serializes its `activeToggles` `Set` as an array and rebuilds it on read via a zod `.transform`; schema-view layout is plain JSON. Active Connection's value is a bare id string, so its codec passes the string through and skips zod.
2626

2727
## Considered Options
2828

@@ -35,7 +35,7 @@ A per-tab value crosses two backings with different serialization needs, so the
3535

3636
- **No migration.** The breadcrumb keeps each concept's existing localForage key and native shape; the codec only governs the per-tab `sessionStorage` round-trip. Existing stored layouts seed a fresh tab unchanged.
3737
- **Cold start can resume a layout set in a different tab** (last-writer-wins breadcrumb), the same honest "resume the most recent" semantics Active Connection accepts. Per the storage model — read once at creation, never re-read (`per-key-diff-merge` context) — no scope here has ever had live cross-tab sync; an already-open tab does not reflect another tab's change until it reloads.
38-
- **A corrupt or stale per-tab value self-heals.** `deserialize` returning `null` falls through to the breadcrumb then the default; breadcrumb **write** failures still surface through the persistence-status path (`storage-layer-owns-persistence-failure`). Appropriate for non-critical view preferences.
38+
- **A corrupt or stale per-tab value self-heals.** `deserialize` throws on a present-but-invalid value (and reading a blocked `sessionStorage` can throw a `SecurityError`); the seam catches either, logs a warning, and falls through to the breadcrumb then the default rather than crashing startup. Breadcrumb **write** failures still surface through the persistence-status path (`storage-layer-owns-persistence-failure`). Appropriate for non-critical view preferences.
3939
- **Rule for new atoms.** A concept that should diverge per tab uses `createSessionScopedAtom` with a codec; a shared Map-keyed collection uses `reconcileMapByKey` (`per-key-diff-merge`); a shared scalar uses plain `atomWithLocalForage`. Scope is now a visible choice at the atom's creation, not an implicit consequence of which factory was reached for.
4040
- The `sessionStorage` backing stays injectable, so per-tab isolation is tested directly with separate stores and separate `sessionStorage` mocks over one shared mock localForage.
4141
- This primitive is the `perTab` adapter that spike #1876 proposes to formalize alongside the other two scopes. This ADR records the **scope** decision; #1876 may later relocate where the logic lives without re-deciding it.

packages/graph-explorer/src/core/StateProvider/graphViewLayoutDefaults.test.ts

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -32,9 +32,13 @@ describe("graphViewLayoutCodec", () => {
3232
).toStrictEqual(defaultGraphViewLayout);
3333
});
3434

35-
test("treats a missing or corrupt value as a miss", () => {
35+
test("treats an absent value as a miss", () => {
3636
expect(graphViewLayoutCodec.deserialize(null)).toBeNull();
37-
expect(graphViewLayoutCodec.deserialize("{ not json")).toBeNull();
38-
expect(graphViewLayoutCodec.deserialize("{}")).toBeNull();
37+
expect(graphViewLayoutCodec.deserialize("")).toBeNull();
38+
});
39+
40+
test("throws on a corrupt value so the seam can discard it", () => {
41+
expect(() => graphViewLayoutCodec.deserialize("{ not json")).toThrow();
42+
expect(() => graphViewLayoutCodec.deserialize("{}")).toThrow();
3943
});
4044
});

packages/graph-explorer/src/core/StateProvider/schemaViewLayoutDefaults.test.ts

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -29,9 +29,13 @@ describe("schemaViewLayoutCodec", () => {
2929
).toStrictEqual(defaultSchemaViewLayout);
3030
});
3131

32-
test("treats a missing or corrupt value as a miss", () => {
32+
test("treats an absent value as a miss", () => {
3333
expect(schemaViewLayoutCodec.deserialize(null)).toBeNull();
34-
expect(schemaViewLayoutCodec.deserialize("{ not json")).toBeNull();
35-
expect(schemaViewLayoutCodec.deserialize("{}")).toBeNull();
34+
expect(schemaViewLayoutCodec.deserialize("")).toBeNull();
35+
});
36+
37+
test("throws on a corrupt value so the seam can discard it", () => {
38+
expect(() => schemaViewLayoutCodec.deserialize("{ not json")).toThrow();
39+
expect(() => schemaViewLayoutCodec.deserialize("{}")).toThrow();
3640
});
3741
});

packages/graph-explorer/src/core/StateProvider/sessionScopedStorage.test.ts

Lines changed: 26 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,10 @@
11
import { createStore } from "jotai";
22
import localForage from "localforage";
3-
import { beforeEach, describe, expect, test } from "vitest";
3+
import { beforeEach, describe, expect, test, vi } from "vitest";
44
import { z } from "zod";
55

6+
import { logger } from "@/utils";
7+
68
import {
79
defaultGraphViewLayout,
810
type GraphViewLayout,
@@ -113,7 +115,7 @@ describe("createSessionScopedAtom", () => {
113115
expect(store.get(atom)).toStrictEqual({ count: 42 });
114116
});
115117

116-
test("treats a corrupt session value as a miss and falls back to the breadcrumb", async () => {
118+
test("discards a corrupt session value with a warning and falls back to the breadcrumb", async () => {
117119
await localForage.setItem<Counter>(KEY, { count: 7 });
118120
const sessionStorage = createInMemorySessionStorage();
119121
sessionStorage.setItem(KEY, "{ not valid json");
@@ -127,6 +129,28 @@ describe("createSessionScopedAtom", () => {
127129

128130
const store = createStore();
129131
expect(store.get(atom)).toStrictEqual({ count: 7 });
132+
expect(vi.mocked(logger.warn)).toHaveBeenCalledOnce();
133+
});
134+
135+
test("recovers when reading sessionStorage throws, falling back to the breadcrumb", async () => {
136+
await localForage.setItem<Counter>(KEY, { count: 7 });
137+
const sessionStorage = createInMemorySessionStorage();
138+
// DOM storage blocked: accessing the value throws a SecurityError rather
139+
// than returning null. The seam must treat this as a miss, not crash boot.
140+
vi.spyOn(sessionStorage, "getItem").mockImplementation(() => {
141+
throw new DOMException("blocked", "SecurityError");
142+
});
143+
144+
const atom = await createSessionScopedAtom<Counter>({
145+
key: KEY,
146+
defaultValue: { count: 0 },
147+
codec: counterCodec,
148+
sessionStorage,
149+
});
150+
151+
const store = createStore();
152+
expect(store.get(atom)).toStrictEqual({ count: 7 });
153+
expect(vi.mocked(logger.warn)).toHaveBeenCalledOnce();
130154
});
131155

132156
test("writing updates this tab synchronously and the breadcrumb in the background", async () => {

packages/graph-explorer/src/core/StateProvider/sessionScopedStorage.ts

Lines changed: 35 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,8 @@ import type { z } from "zod";
22

33
import localForage from "localforage";
44

5+
import { logger } from "@/utils";
6+
57
import { persistThroughQueue } from "./persistence";
68
import { resolveSessionStorage } from "./safeSessionStorage";
79
import { createWriteThroughAtom } from "./writeThroughAtom";
@@ -22,10 +24,12 @@ export type SessionValueCodec<T> = {
2224
};
2325

2426
/**
25-
* Parses a sessionStorage JSON string against `schema`, returning `null` for a
26-
* missing, unparseable, or schema-invalid value so the caller treats it as a
27-
* miss. This keeps a stale or hand-edited per-tab value from seeding the atom
28-
* with the wrong shape.
27+
* Parses a sessionStorage JSON string against `schema`. Returns `null` only for
28+
* an absent value (`null` or empty string) — a legitimate miss. A present but
29+
* unparseable or schema-invalid value is corrupt and **throws** (`SyntaxError`
30+
* from `JSON.parse` or `ZodError` from the schema); the seam that owns seeding
31+
* (`createSessionScopedAtom`) catches it, so detecting corruption stays separate
32+
* from deciding what to do about it.
2933
*/
3034
export function parseSessionJson<T>(
3135
raw: string | null,
@@ -34,12 +38,7 @@ export function parseSessionJson<T>(
3438
if (raw === null || raw === "") {
3539
return null;
3640
}
37-
try {
38-
const result = schema.safeParse(JSON.parse(raw));
39-
return result.success ? result.data : null;
40-
} catch {
41-
return null;
42-
}
41+
return schema.parse(JSON.parse(raw));
4342
}
4443

4544
/**
@@ -75,7 +74,7 @@ export async function createSessionScopedAtom<T>({
7574
codec: SessionValueCodec<T>;
7675
sessionStorage?: Storage;
7776
}) {
78-
let seedValue = codec.deserialize(sessionStorage.getItem(key));
77+
let seedValue = readSessionSeed(sessionStorage, key, codec);
7978
if (seedValue === null) {
8079
// Cold start: seed from the shared breadcrumb and claim it into this tab's
8180
// sessionStorage, so a later reload reads this value back rather than
@@ -101,6 +100,31 @@ export async function createSessionScopedAtom<T>({
101100
);
102101
}
103102

103+
/**
104+
* Reads and decodes this tab's per-tab seed, recovering from a corrupt value.
105+
*
106+
* `codec.deserialize` throws on a present-but-invalid value (and reading
107+
* sessionStorage itself can throw a `SecurityError` when DOM storage is
108+
* blocked). Either is a recoverable miss: a stale or hand-edited per-tab value
109+
* should not crash app startup, so it is logged and treated as absent, letting
110+
* the caller fall through to the shared breadcrumb then the default.
111+
*/
112+
function readSessionSeed<T>(
113+
sessionStorage: Storage,
114+
key: string,
115+
codec: SessionValueCodec<T>,
116+
): T | null {
117+
try {
118+
return codec.deserialize(sessionStorage.getItem(key));
119+
} catch (error) {
120+
logger.warn(
121+
`Discarding corrupt per-tab value for "${key}"; falling back to the persisted breadcrumb.`,
122+
error,
123+
);
124+
return null;
125+
}
126+
}
127+
104128
/** Applies a serialized value to sessionStorage, removing the key for `null`. */
105129
function writeSession(
106130
sessionStorage: Storage,

0 commit comments

Comments
 (0)