Conversation
db6f162 to
b182877
Compare
8347655 to
b2f6ab7
Compare
The two view-layout atoms shared one localForage value across tabs, so opening the app in a second tab clobbered the first tab's sidebar and toggle state (last-writer-wins). Extract the per-tab sessionStorage + shared localForage breadcrumb trick that createActiveConfigurationAtom() used into a generic createSessionScopedAtom<T>, and refactor active-config onto it so the subtle seed logic lives in one named place. Point both layout atoms at it with zod-validated codecs co-located with each model: schema-view is plain JSON; graph-view serializes its activeToggles Set as an array. parseSessionJson() rejects a stale or hand-edited per-tab value with the wrong shape, falling through to the breadcrumb. The breadcrumb key/shape are unchanged, so existing stored layouts are reused with no migration.
Add an ADR recording the per-tab session-scoped storage primitive: the three named cross-tab scopes (per-tab / shared-reconciled / shared-blind-write), why layout is per-tab, and how it relates to the active-connection and per-key-diff-merge ADRs and spike #1876. Note in the per-key-diff-merge ADR that layout has since moved from a shared scalar to per-tab session scope, so it is no longer a standing example of a shared blind-write scalar. Add a test exercising createSessionScopedAtom with the real graphViewLayoutCodec over the cold-start claim path: a breadcrumb holding a native activeToggles Set is seeded as a Set and claimed into sessionStorage as its array-serialized form. Previously the generic helper and the Set-bearing codec were only tested in isolation.
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.
- Move the graphViewLayout activeToggles Set rebuild from an object-level transform onto the activeToggles field, so the transform sits on the field it converts and the object schema infers the runtime shape. - Correct the SessionValueCodec.deserialize contract doc: it returns null only for an absent value and throws on a corrupt one, which the seam catches. Do not swallow errors in the codec. - Drop the unused session() accessor from the test tab harness. - Add cross-tab coverage for both layout codecs through the real codecs (graph view proves the activeToggles Set survives the array round-trip across a write-then-cold-start sequence; schema view covers isolation and cold-start seeding), matching the active-connection assurances.
The global test setup already gives every test a fresh IndexedDB backend (dropInstance + new IDBFactory per test), so the per-describe beforeEach(localForage.clear()) blocks added no isolation.
createSessionScopedAtom runs a ReadTransform on the shared breadcrumb before claiming it, which is the only path a shape retired by a newer app version can arrive by. Cover that it normalizes and claims the normalized value, that it never touches this tab's own zod-validated session value or the default, and that a stored nodes-styling sidebar item lands on the combined styles panel.
c3c888e to
3c1fb70
Compare
resolveSessionStorage only guards the initial access, so a later setItem could still throw QuotaExceededError when storage fills, or SecurityError where DOM storage is blocked. That throw escaped the Jotai setter and would take down the React subtree that set the atom. writeSession now owns serialization and swallows a write failure with a warning. The atom has already updated in memory and the shared breadcrumb still persists through the queue, so the only cost is this tab's warm-reload value.
The per-tab storage ADR named the three cross-tab scopes but the glossary never picked them up, leaving Graph View Layout, Schema View Layout, and Storage Scope itself undefined. Also corrects the Graph Database entry, which still described layout as IndexedDB-persisted app state.
The assertion computed its expected value with the same serialize call it was testing, so a serialize that dropped a field would have changed both sides together and still passed. Write the on-disk JSON out instead.
Moving both layouts onto createSessionScopedAtom left several docs describing the shape it replaced. The backward-compatibility rule in testing.md only triggered on atomWithLocalForage, so it no longer covered either layout type it was written for, and its cross-tab example is typed to that factory and cannot open a per-tab atom. product.md still listed layout as IndexedDB-persisted. Three ADRs still said the per-tab mechanism and the read transform live where this branch moved them from. Also trims the Storage Scope glossary entry to the definition, since it had duplicated the ADR's per-scope atom inventory and the two copies already disagreed, and corrects that inventory to the glossary's own term for Styles. Drops the stale "one set of tests" claim from the new ADR: the active-connection tests stay as behavior-preservation evidence.
writeSession guarded codec.serialize alongside the storage call, so a codec defect was logged as a write failure and the per-tab layer silently stopped working for the rest of the tab's life. A serialize throw is a defect, not a storage condition. Only setItem and removeItem are guarded now.
The type and the parser were two hand-written declarations of one shape, and nothing tied them together: adding an optional field to the type compiled clean, then zod stripped it on read. The field survived in the breadcrumb through structured clone but vanished on every warm reload of the tab that set it. The schema is now the only declaration, matching what schemaViewLayoutSchema already did for the schema view.
"Layout" meant two things: the Cytoscape algorithm that positions vertices, and the per-tab sidebar and toggle state this branch made per-tab. The bare word now belongs to the algorithm, and the per-tab state is a View Layout, which matches the graphViewLayout and schemaViewLayout identifiers. Also documents the per-tab scope for users in both feature docs, mirroring the wording connections.md already uses for the Active Connection, and dates the storage-scope ADR to when it landed so it sorts after the ADR it cites.
The shared comment claimed both layout suites existed because their codecs carry risk. That holds for graph view, whose activeToggles Set has to survive the array round trip, and not for schema view, whose codec is structurally the counter codec. Name what each one actually guards so neither reads as redundant with the other.
| const breadcrumb = await localForage.getItem<T>(key); | ||
| if (breadcrumb !== null) { | ||
| seedValue = transform ? transform(breadcrumb) : breadcrumb; | ||
| writeSession(sessionStorage, key, codec, seedValue); |
There was a problem hiding this comment.
The per-tab leg is hardened and the breadcrumb leg is trusted, so the self-heal guarantee holds on one side only.
readSessionSeed wraps codec.deserialize in try/catch, logs, and returns null so a bad per-tab value falls through — that is the documented behavior and the corrupt-value and SecurityError tests pin it. This branch has no equivalent. localForage.getItem<T>(key) asserts the breadcrumb as T without parsing it, then transform and writeSession both run outside any try.
That matters because of the interaction with two other lines. writeSession deliberately places const serialized = codec.serialize(value) outside its own try at line 160 (correct, for a genuine defect), and graphViewLayoutCodec.serialize spreads [...layout.activeToggles] at graphViewLayoutDefaults.ts:100. transformGraphViewLayout only touches activeSidebarItem, so a breadcrumb whose activeToggles is absent or non-iterable reaches that spread untouched, throws TypeError, and rejects this createSessionScopedAtom call — which storageAtoms.ts awaits inside a top-level Promise.all with no surrounding try. That is a blank app rather than a degraded one.
The ADR states the self-heal narrowly and correctly, scoping it to "a corrupt or stale per-tab value." The PR description generalizes it to "a stale or hand-edited value can never crash the top-level-await startup," and that word is what I am questioning.
Likelihood is bounded, which is why I read this as important rather than blocking: activeToggles has been a Set in the persisted shape since before the user-layout to graph-view-layout rename, and migrateUserLayout copies the legacy value verbatim, so a well-formed legacy breadcrumb round-trips fine. The reachable triggers are a hand-edited IndexedDB value — the same input class the per-tab leg was hardened against — and a restored config backup or rolled-back app version writing a different native shape, which is precisely the path "no migration" exists to protect.
Two ways out, and I don't have a strong preference:
- Parse the breadcrumb through the same schema before claiming it, so both legs share one corruption seam and a throw falls through to
defaultValue. Costs one parse per cold start plus a corrupt-breadcrumb test, and makes the general claim true. - Narrow the PR description to match what the ADR already says. Costs nothing, but leaves the hand-edit and rollback vectors able to take down boot.
Either is defensible for a non-critical view preference. Worth noting there is currently no test for a corrupt breadcrumb, only for a corrupt per-tab value.
| - **Styles** are scoped per **Vertex Type** (**Vertex Styles**) and **Edge Type** (**Edge Styles**) | ||
| - The **Graph View**, **Data Table View**, and **Schema View** all render from the same **Session** and **Schema** | ||
| - Each browser tab has its own **View Layout** per view, the same divergence as **Active Connection** | ||
| - A **Layout** positions **Vertices** on the **Graph View** canvas and is not part of any **View Layout** |
There was a problem hiding this comment.
A question about how absolute you want this, because my in-flight #2144 collides with it and I am not assuming the glossary is the side that should bend.
This invariant, plus the Layout entry's _Avoid_: View Layout (a different concept) and the closed enumeration in Schema View Layout ("active sidebar panel, sidebar width, and the details-auto-open preference"), together say a Layout is never part of a View Layout. #2144 adds a required layoutAlgorithm: LayoutName field directly to SchemaViewLayout and defaults it in transformSchemaViewLayout. The moment it lands, a Layout is part of a View Layout and that enumeration is incomplete. Since this PR is sequenced first, the text would be merged as settled fact and then falsified.
I can see two resolutions and I think the choice is yours as the owner of the domain model:
- The invariant is what you meant, and Persist schema view layout selection #2144 is wrong. Then
layoutAlgorithmdoes not belong inSchemaViewLayoutat all and I move it to its own atom in Persist schema view layout selection #2144. Costs me an atom and keeps the concept boundary sharp, which is a real benefit: it would mean "View Layout" stays purely UI chrome. - The invariant is over-stated. Then scoping it to the Graph View ("it is the algorithm, not part of the Graph View Layout's UI state") and opening the
Schema View Layoutlist with "such as" costs one line here and saves a doc-correcting commit in Persist schema view layout selection #2144.
For what it is worth, #2145 does not conflict — its Graph Arrangement entry also treats Layout as the algorithm only, carrying _Avoid_: Layout (only the algorithm). It does add a sixth Layout-adjacent term to this region of the glossary, so whichever way this goes, the six read better if they agree on where the algorithm lives.
Happy to take the work in #2144 either way. I just want the decision made here rather than discovered later.
| **Storage Scope**: | ||
| The cross-tab behavior a persisted atom picks at creation, so scope is a visible decision rather than a side effect of which factory was reached for. Three named scopes: **per-tab**, where tabs diverge and a fresh tab starts from the value most recently used; **shared-reconciled**, where a Map-keyed collection is merged per key across tabs; and **shared-blind-write**, where each write is the whole value. See the `per-tab-session-scoped-storage-primitive` ADR for which atoms use which, and `per-key-diff-merge-cross-tab-reconciliation` for the merge rule. | ||
| _Avoid_: Persistence mode, storage strategy |
There was a problem hiding this comment.
Minor, and the term itself clearly earns its place — it names a decision the code really does make at atom creation.
The length is what I'd push back on. CONTEXT-FORMAT.md asks for "one or two sentences max" and "define what it IS, not what it does," and docs/agents/domain.md splits what into the glossary and why into ADRs. This entry enumerates all three scopes with their merge and divergence mechanics and points at two ADRs, which reads closer to an ADR abstract than a definition. The neighbouring Layout and View Layout entries are tight by comparison.
Something like: "The cross-tab persistence behavior a persisted atom picks at creation — per-tab (tabs diverge), shared-reconciled (merged per key), or shared-blind-write. See the storage ADRs for which atoms use which." The per-scope mechanics are already in the new ADR, so restating them here is the part that could go.
Entirely your call on which detail an agent needs inline versus behind a link — that is the judgement the glossary exists to encode, so I'm not going to be confident about it from outside.
| - **Shared-reconciled** — `atomWithLocalForage` with `reconcileMapByKey`. Map-keyed collections genuinely shared across tabs, merged per key (`per-key-diff-merge`). Backs Connections, Schema, Vertex and Edge Styles, Sessions. | ||
| - **Shared-blind-write** — `atomWithLocalForage` with no reconciler. Scalars where each write is the whole intended value and tabs need not diverge. Backs the boolean/number settings (e.g. `showDebugActions`). | ||
|
|
||
| `createActiveConfigurationAtom` is refactored onto the primitive rather than left as a parallel implementation, so the seed-and-claim logic lives in exactly one place. Its own tests stay, narrowed to what the wrapper still owns: the empty-string-is-a-miss codec rule, the bare-string round trip, and the `resolveSessionStorage` fallback. They also stand as behavior-preservation evidence for the refactor. |
There was a problem hiding this comment.
Small factual correction: nothing was narrowed. git diff bfc06442 HEAD -- packages/graph-explorer/src/core/StateProvider/activeConnectionStorage.test.ts is empty — all ten tests are unchanged, including the multi-tab cases.
Worth fixing because the reality is stronger than the claim. Retaining the full pre-existing suite unmodified is better behavior-preservation evidence for the refactor than retaining a narrowed subset would be, and it is the thing that made me comfortable with the createActiveConfigurationAtom rewrite. Suggest rewording to say the tests were kept intact as behavior-preservation evidence.
| }); | ||
| const store = createStore(); | ||
|
|
||
| expect(() => store.set(atom, { count: 5 })).toThrow(TypeError); |
There was a problem hiding this comment.
Small one, and it is the only one of the three new type-form assertions where your justifying comment does not reach.
docs/agents/testing.md:79 asks for the instance form (toThrow(new FooError(a, b))) over toThrow(FooError), because the type form passes even when the code built the error with the wrong data. The two in graphViewLayoutDefaults.test.ts and schemaViewLayoutDefaults.test.ts are correctly exempt and you said why inline: SyntaxError messages are V8-specific and ZodError's issues shape moved between zod 3 and 4, so pinning an instance there would couple those tests to engine and library versions.
This one is different. The TypeError is thrown by the test's own brokenCodec at line 214 with a fixed message, so there is no version coupling to avoid and the instance form is available:
expect(() => store.set(atom, { count: 5 })).toThrow(
new TypeError("activeToggles is not iterable"),
);Strictly stronger here: it would catch a regression where writeSession swallowed the codec's error and rethrew a different TypeError, which is exactly the laundering this test exists to prevent. As written, any TypeError from anywhere in the setter satisfies it.
Context rather than criticism: these three are the repo's first type-form assertions. The other eight toThrow sites all use the instance form.
This branch has not been deployed
2 participants
Description
The graph-view and schema-view layout state (active sidebar tab, sidebar width, view toggles, details-auto-open preference) was persisted as a single localForage value shared across all browser tabs. Opening the app in a second tab clobbered the first tab's layout — last-writer-wins over the whole value.
This scopes both layouts to the browser tab using the same per-tab mechanism the active connection already uses: the live value lives in
sessionStorage(survives this tab's reload, never leaks to other tabs), with a shared localForage breadcrumb that seeds a freshly-opened tab from the last-used layout on cold start.Core change: the per-tab + breadcrumb "trick" that was inlined in
activeConnectionStorage.tsis extracted into a genericcreateSessionScopedAtom<T>.createActiveConfigurationAtomis refactored onto it (so the subtle seed-and-claim logic lives in one place), and both layout atoms now route through it with zod-validated codecs.Notable details:
sessionStorageholds strings, so each layout has a codec.deserializereturnsnullfor an absent value but throws on a corrupt/wrong-shape one; the seam (createSessionScopedAtom) catches, logs, and falls back to the breadcrumb then the default — so a stale or hand-edited value can never crash the top-level-await startup. Detecting corruption is kept separate from deciding what to do about it.Setserialization. Graph-view'sactiveTogglesis a runtimeSet; its codec serializes it as an array for JSON and rebuilds theSeton read. Schema-view is plain JSON.The new per-tab scope is recorded in an ADR alongside the two existing shared scopes (reconciled maps, blind-write scalars).
Validation
pnpm checkspasses (types, lint, format).pnpm testpasses (full suite), including new coverage insessionScopedStorage.test.ts(seed order, corrupt-value +SecurityErrorrecovery, theSet-bearing cold-start claim, and cross-tab isolation/seeding through the real graph-view and schema-view codecs).activeTogglesSetround-trips across a write-then-cold-start sequence.How to read
parseSessionJsonand the corrupt-value recovery seamactiveTogglesSet↔ array serializationRelated Issues
Check List
pnpm checkspasses with no errors.pnpm testpasses with no failures.