fix(explore): preserve shape size across projection reloads

[FlorinSenoner]

Root cause

Automatic demo-dataset loads used the same replacement semantics as an explicit reset. On reload or a direct projection URL, the fresh explore controller first cleared the dataset-scoped legend record and then applied embedded demo bundle settings with replacement semantics, overwriting a saved Shape size with 30.

Fix

• Distinguish a fresh automatic default load from a later explicit reset using existing controller lifecycle state.
• Preserve existing per-annotation legend settings during automatic startup while seeding embedded demo settings for missing annotations.
• Keep explicit reset-to-demo and imported bundle replacement semantics unchanged.
• Document the behavior in an OpenSpec change.

Reproduction

1. Open the demo explore view.
2. Save Shape size 42.
3. Change to another projection.
4. Reload the page or open the selected projection as a direct URL.

Before this change, Shape size returned to 30 and effective point size returned to 240. After this change, Shape size remains 42 and effective point size remains 336 across both paths. Explicit reset-to-demo still restores the captured demo defaults.

Tests

• pnpm exec vitest run apps/web/src/explore/dataset-controller.eat.test.ts packages/core/src/components/legend/controllers/persistence-controller.test.ts — 38 passed.
• Affected Playwright projects on the isolated worktree server — 29 passed.
• Focused explicit reset-to-demo browser regression — 1 passed.
• pnpm test:ci — 1,873 passed, 1 skipped.
• pnpm precommit — passed before commit and push.
• openspec validate preserve-shape-size-across-projection-navigation --strict — passed.

Closes #340

[tsenoner]

Automated review

Does it solve #340? Yes. I confirmed the failing path is the one this PR targets: the dataset hash is built from sorted protein IDs plus annotation and prediction fingerprints and contains nothing projection-derived, and data-loaded is dispatched only by the data-loader when a bundle is loaded — so switching projection in-session never re-enters clearForNewDataset. What actually reset Shape size was a fresh controller on reload or a direct projection URL treating an automatic demo load as an explicit reset, which the new currentDatasetHash !== null guard now distinguishes. Worth noting for the issue thread that the reporter's literal wording ("changing the projection") was never the failing step on its own; the reload is.

Found 2 issues:

1. shouldClearPersistedState inlines currentDatasetHash !== null, and 39 lines later the same predicate — reading the same value, still ahead of the assignment on line 176 — is given the name hadPreviousDataset. Two spellings of one fact, in a function where ordering relative to line 176 is load-bearing, invites one of them to drift. Hoisting a single const hadPreviousDataset above line 135 and using it in both places removes the duplication.

protspace/apps/web/src/explore/dataset-controller.ts

Lines 134 to 138 in a94ffae

	const datasetHash = generateDatasetHash(data);
	const shouldClearPersistedState =
	(loadMeta.kind === 'default' && currentDatasetHash !== null) ||
	(loadMeta.kind === 'user' && settings != null);

2. The empty-map-to-null normalisation is unobservable and can be dropped. Every consumer of _fileSettings keys by annotation name (tryLoadFileSettings, hasFileSettingsForAnnotation), and for those {} and null already behave identically. The only accessor that distinguishes them is the hasFileSettings getter, which has no production caller — the control bar's hasFileSettings is assigned from the raw settings object in dataset-controller.ts L158, not from the legend.

protspace/packages/core/src/controllers/base-persistence-controller.ts

Lines 96 to 100 in a94ffae

	if (this._fileSettings && Object.keys(this._fileSettings).length === 0) {
	this._fileSettings = null;
	}
	}

🤖 Generated with Claude Code

_{Reviewed at a94ffae against issue #340.}

[FlorinSenoner]

Independent triage at a94ffae17dac61c8c9ee0f6fa97a48b24cdf9b3f:

1. Actionable; follow-up needed. currentDatasetHash !== null at dataset-controller.ts:136 and hadPreviousDataset at line 175 capture the same pre-assignment lifecycle fact. The required direction is to compute hadPreviousDataset once before shouldClearPersistedState and reuse it in both decisions.

2. Non-actionable; keep the normalization. When non-clearing application prunes every annotation already backed by local storage, _fileSettings contains no file-backed entries. ProtspaceLegend.hasFileSettings is a public getter exported through @protspace/core; without {} → null normalization it would report true despite no remaining file settings. The control bar’s separately assigned flag does not make that public API unobservable.

[FlorinSenoner]

Implemented the actionable follow-up in 899b339414b3b931bc7380150a8ab657950b46ab: hadPreviousDataset is now captured once before currentDatasetHash is assigned and reused for both persisted-state clearing and later user-import handling. The focused lifecycle test now covers the initial default load, a subsequent user import with stale tooltip URL state, and explicit demo reset. The {} to null file-settings normalization remains unchanged to preserve the public hasFileSettings contract.

Verification: focused Vitest suites 38/38 passed; full pnpm test:ci 1,873 passed with 1 skipped; pnpm precommit passed; strict OpenSpec validation passed.

[tsenoner]

Adversarial review

Reviewed in an isolated worktree by three independent lenses (code quality, adversarial correctness, issue-resolution audit), with every finding then put through a refuter whose default position was that it is a false positive. 9 raised, 4 survived refutation.

Applied and pushed (9d58a2fd)

Behavior-preserving cleanups, verified green before pushing:

• Fix 1 (reuse): moved getShapeSizeState and setShapeSize into the shared helper module apps/web/tests/helpers/explore.ts (which already imports expect and type Page, so no import additions were needed there); deleted the byte-identical local copy from apps/web/tests/url-view-state.spec.ts and both local copies from apps/web/tests/dataset-reload.spec.ts, adding the two names to each spec's existing './helpers/explore' import list.
• Fix 1 (cont.): replaced the re-inlined open-'Legend settings' / fill #shape-size-input / click 'Save' block in the url-view-state.spec.ts shape-size test with await setShapeSize(page, shapeSize);, which also gives that test the poll-until-applied wait it was missing.
• Fix 2 (simplification): in apps/web/tests/url-view-state.spec.ts added const SHAPE_SIZE_TO_POINT_SIZE = 8; plus expectedShapeState right after const shapeSize = 42;, with the comment pointing at calculatePointSize()/LEGEND_DEFAULTS.symbolSizeMultiplier and the ESM-loader note above waitForView; both duplicated assertion blocks now read await expect.poll(() => getShapeSizeState(page)).toEqual(expectedShapeState);

Follow-up commit (7ef8e846)

• Corrected the multiplier comment I had just added. It claimed to mirror calculatePointSize()
  without noting that helper also clamps via Math.max(10, ...), and pointed at "the note above
  waitForView" when that note is inside it. Now states the clamp divergence and the range where the
  plain multiply is equivalent.

Issue resolution — partially resolves the issue

The root-cause analysis is correct and I verified every load of it independently: the demo bundle
really does embed shapeSize: 30, the default kind really was shared between automatic startup
and explicit reset (unchanged since 5423101c, i.e. it predates the issue), and the fresh-
controller startup really did clear-then-replace. The fix is surgical — the only behavioral mode
that changes is the first default load in a controller; user-import replacement and OPFS restore
semantics are untouched — and the branch is pinned by both a unit test and an e2e test on the reset
side.
Two triage claims deserve pushback. First, the design's assertion that "an in-place projection
change keeps the same explore controller alive and therefore retains Shape size" is correct as far
as I can tell, but the PR treats it as established rather than proving it, and the reporter's issue
text describes exactly that in-place action. The PR is fixing the reload/deep-link variant and
inferring the reporter meant that. That inference is plausible (nothing in the code resets shape
size on a projection change) but it should be confirmed, not assumed, before closing.
Second, hadPreviousDataset is a proxy for "automatic startup", not for "not an explicit reset".
The recovery banner's "Load demo dataset"/"Clear" buttons (startup.ts:33-41) call
loadDefaultDatasetAndClearPersistedFile() while currentDatasetHash is still null, so an
explicitly user-initiated demo load is classified as automatic and preserves state. The outcome is
benign (arguably desirable), but design.md's claim that "an explicit reset occurs after a dataset is
active" is not true of that path.
The bigger scoping question is that the PR fixes #340 by making the entire dataset-scoped legend
record survive startup — hidden values, colors, shapes, palette, z-order, maxVisibleValues, plus the
tooltip-annotations key — which reverses the shipped behavior from #178 while declaring "Modified
Capabilities: None". design.md does acknowledge the whole-record trade-off, so it is a considered
decision, not an oversight, but it is a much larger user-visible change than "keep shape size" and
the reviewer should sign off on it explicitly (especially given the demo-reset button is disabled
while on the demo).
On closing semantics: "Closes #340" is defensible for the projection axis and I would keep it, but I
would open a follow-up for the per-annotation scoping question, since the issue's own justification
("the main reason to set it is based on dataset size") describes a dataset-level preference that
this PR does not deliver.

Gaps found by the issue audit (8)

• Shape size is still scoped per (dataset, annotation), so it resets whenever the color-by annotation changes — which contradicts the issue's stated rationale that it is a dataset-size preference.
  • Why it matters: Storage key is protspace:legend:<hash>:<annotation> (base-persistence-controller.ts:158) and legend.ts:1516 assigns this.shapeSize = settings.shapeSize unconditionally. The demo bundle seeds shapeSize 30 for every annotation, so setting 42 on pfam and switching color-by snaps it back to 30. The new deep-link assertion in url-view-state.spec.ts deliberately navigates to annotation=demoDefaultAnnotation (the same annotation that was saved), so it cannot catch this. The issue text says the reason to change it is dataset size, which implies a dataset-level (or global) preference.
  • Suggested follow-up: Ask the reporter whether shape size should be dataset-scoped; if yes, persist it in a dataset-level record (or fall back to the most recently saved annotation's shapeSize when an annotation has no record) and add an e2e assertion that deep-links with a different annotation.
• The literal symptom in the issue (in-place projection change) is asserted in design.md but never tested, and the PR cannot explain it.
  • Why it matters: My code reading agrees with the author that an in-place projection change preserves shape size on current main, which means the reporter must have reloaded or used a projection URL for the PR to be the fix. If they really saw an in-place reset, [FEATURE] Keep shape size when changing projection #340 would be closed without fixing what they saw. The new test only asserts after page.reload().
  • Suggested follow-up: Add await expect.poll(() => getShapeSizeState(page)).toEqual({pointSize: 336, shapeSize: 42}) immediately after selectProjection(...) and before page.reload() in url-view-state.spec.ts, and confirm the repro with the reporter on a preview deploy before closing.
• Page reload is no longer an escape hatch from a broken demo legend, and the in-app reset is unreachable while the demo is active.
  • Why it matters: control-bar.ts:1002 disables the "Load demo dataset" button with ?disabled=${this.currentDatasetIsDemo}, so the new spec's "Explicit demo reset" scenario is only reachable after importing a custom dataset (exactly the detour the e2e test takes). This PR inverts the e2e assertion added by a8a33582 fix(app): always reset legend state on dataset load (#178), whose issue said the page reload was the user's only workaround. Recovery still exists (legend settings dialog → Reset removes the key, and the next reload reseeds the bundle's curated settings for that annotation) but is undiscoverable.
  • Suggested follow-up: Enable the demo button while on the demo and relabel it "Reset demo dataset" (it already routes to loadDefaultDatasetAndClearPersistedFile, and hadPreviousDataset is true there so it clears correctly), or narrow the preserved-on-startup subset to display settings rather than the whole legend record.
• The change preserves more than the legend record: protspace:tooltip-annotations:<hash> also survives startup now, an untested and undocumented side effect.
  • Why it matters: removeAllStorageItemsByHash matches any protspace:*:<hash>* key (storage-service.ts:52-66), so the demo's tooltip-annotation set was wiped on every startup before this PR — making the readTooltipAnnotations restore branch in dataset-controller.ts:205-220 effectively dead for the demo. It is now live: the demo's tooltip set restores on reload when the URL carries no tooltip param. That is probably the intended design, but nothing in the proposal, spec, or tests covers it.
  • Suggested follow-up: Either add an e2e assertion for demo tooltip restoration across reload or explicitly scope the preserve behavior to the legend prefix; mention it in the spec delta.
• Curated demo bundle settings become permanently stale for returning users.
  • Why it matters: The first-ever load seeds every annotation key from the bundle (base-persistence-controller.ts:94), so on every later visit every key exists, all embedded settings are dropped, and _fileSettings collapses to null. If a future demo bundle ships new curated colors/z-order/hidden values without changing protein ids or annotation values, the dataset hash is unchanged and returning browsers will never pick the new curation up.
  • Suggested follow-up: Store a fingerprint of the bundle's settings alongside the seeded record and reseed (or prompt) when it changes, or exempt fields the user has not explicitly touched.
• The preserve check reads storage under the controller's hash, which is not always the hash the legend writes under.
  • Why it matters: generateDatasetHash(data) in dataset-controller.ts:134 includes annotation_predicted (data-hash.ts predictionFingerprint), but the legend computes its own hash from only {protein_ids, annotations, numeric_annotation_data} (legend.ts:841-845). For an EAT/predicted-bearing dataset the two differ, so the new getStorageItem(key, null) !== null probe would miss the legend's real record and the embedded file settings would win again. The shipped demo bundle has no predicted cells (verified), so this does not bite today.
  • Suggested follow-up: Pass annotation_predicted into the legend's updateDatasetHash call (or drop it from the controller's hash input) so both sides agree; add a regression test with phosphatase_eat.parquetbundle.
• Docs and OpenSpec artifacts understate the change.
  • Why it matters: proposal.md says "Modified Capabilities: None" while the PR reverses shipped, tested [BUG] Reloading the default dataset does not restore original state #178 behavior. docs/explore/legend.md:162-167 lists saved settings without shape/point size and never states that demo legend state now survives a reload or how to return to the pristine demo — and repo convention is to update docs with the change.
  • Suggested follow-up: Add shape/point size to the legend docs' Saved Settings list, document the reload-preserves / explicit-reset-clears contract, and record the [BUG] Reloading the default dataset does not restore original state #178 behavior change in the OpenSpec proposal instead of "None".
• Test placement and helper duplication.
  • Why it matters: The lifecycle regression lives in apps/web/src/explore/dataset-controller.eat.test.ts, an EAT-specific file, and getShapeSizeState is copy-pasted into both dataset-reload.spec.ts and url-view-state.spec.ts rather than apps/web/tests/helpers/explore.ts (a previous commit specifically consolidated duplicate helpers).
  • Suggested follow-up: Move the lifecycle test into a dataset-controller.test.ts (or .lifecycle.test.ts) and hoist getShapeSizeState/setShapeSize into tests/helpers/explore.ts.

Findings needing a decision (2)

These were left for you rather than auto-applied: each changes behavior, needs a product call, or reaches outside this diff.

1. The new unit test re-declares ~35 lines of controller-options scaffolding already present in the sibling test in the same file, and lives under a describe block about EAT settings.

apps/web/src/explore/dataset-controller.eat.test.ts:139 · medium · test-gap

Lines 140-178 duplicate lines 60-99 almost verbatim (controlBar, legendElement,
viewController, the whole options object with its as unknown as Parameters<typeof createDatasetController>[0] cast). Only four fields actually differ:
loadQueue.getLoadMetaForFile, getRunningLoadMeta, getLatestSequence, and whether
getLatestViewRequest reads a mutable variable. Cost: any future field added to
CreateDatasetControllerOptions has to be added to both literals or the second test fails on a cast
that hides the error until runtime; the differences that matter to the test are buried in 35 lines
of noise. Separately, the test is added inside describe('dataset controller EAT settings restore')
(line 52) but tests neither EAT nor anything EAT-adjacent - it asserts legend/control-bar
persistence flags and, at lines 209-213, the unrelated stale-tooltip normalization on user import,
so a failure message will point at the wrong subsystem.

Suggested fix

Keep the setRequestedView assertion at 209-213 — it guards the hadPreviousDataset hoist. For the
label, do NOT create a sibling describe (it would lose the beforeEach at 53-57). Instead rename line
52 to describe('dataset controller', () => { and wrap each it in a nested describe inside it:
describe('EAT settings restore', ...) around the test at line 59 and describe('legend persistence lifecycle', ...) around the test at line 139 — nested describes inherit the outer beforeEach.
Optionally, hoist a function createOptions(overrides: { loadQueue?: object; getLatestViewRequest?: () => ExploreViewRequestState } = {}) above the describe that returns { options, controlBar, legendElement, viewController } with the shared literal and spreads overrides.loadQueue over the
default loadQueue, then rewrite both tests on top of it.

2. Once the demo bundle's legend settings are seeded into localStorage, an updated bundle can never override them for returning visitors.

packages/core/src/controllers/base-persistence-controller.ts:90 · medium · correctness

The new branch skips setStorageItem and deletes the annotation from _fileSettings whenever any
record exists at protspace:legend:<hash>:<annotation>. But the very first automatic default load
itself writes that record (line 94), so from the second visit onward the bundle's settings are
permanently pruned. I verified apps/web/public/data.parquetbundle does ship a settings table
containing {"pfam": {"includeShapes": false, "shapeSize": 30, "sortMode": "manual", ..., "selectedPaletteId": "kellys", "categories": {"PF21947 (Toxin_cobra-type)": {"zOrder": 0, "color": "#F3C300", "shape": "circle"}, ...}}} — i.e. curated per-category colors/shapes. Concrete scenario:
maintainers re-curate the demo's pfam colors and redeploy data.parquetbundle with identical
protein_ids/annotations (so generateDatasetHash is unchanged). Every returning visitor keeps the
old colors forever: on load, getStorageItem(key) !== null -> the new file settings are dropped
and loadFromStorage() returns the stale seeded record. On origin/main the wipe-and-replace made
the bundle authoritative on every default load. The design doc's risk list does not mention this; it
only frames the seeding as benefiting first-time users.

Suggested fix

Minimum (docs-only, safe): add a bullet to the Risks / Trade-offs section of
openspec/changes/preserve-shape-size-across-projection-navigation/design.md, e.g. '- A re-curated
demo bundle no longer reaches returning visitors. The first automatic load seeds the bundle's
legend settings into local storage, and later loads treat any existing record as user-owned. Because
generateDatasetHash ignores the settings table, redeploying data.parquetbundle with new curated
colors/shapes but identical protein_ids/annotations leaves returning visitors on the visit-1 seed.
Changing the demo's data changes the hash and re-seeds.'
Proper fix (behavioral, needs its own tests): make the seed distinguishable from a user edit — e.g.
after the seeding loop in base-persistence-controller.ts write
setStorageItem(buildStorageKey(${this.storageKeyPrefix}-file-seed, hashToUse), fingerprint(settings)), and in the non-clearing branch take the preserve path only when the stored
fingerprint equals the current bundle's; when it differs, fall back to replacement for that hash.
Note the seed key must not collide with findStorageKeysByHash's parts[2] === hash matching.

5 further finding(s) were raised and refuted during verification.

[FlorinSenoner]

Follow-up for the adversarial review at d04ca8f17ddf794145c01c6c15a07789181c6489:

1. In-place projection coverage — implemented. The shape-size browser regression now asserts { pointSize: 336, shapeSize: 42 } immediately after the in-session projection change, before reload, and again after reload/deep-link navigation.
2. Recovery-banner startup path — documented, no behavior change. The OpenSpec design now states that recovery actions before any successful dataset load follow startup semantics; the explicit reset requirement remains scoped to a reset after a dataset is active.
3. Whole-record / [BUG] Reloading the default dataset does not restore original state #178 behavior — intentional and documented. The proposal now explicitly records that browser reload no longer provides [BUG] Reloading the default dataset does not restore original state #178's historical wipe behavior while the in-app Import → Load demo dataset reset after custom data is retained. The legend docs describe both reset paths.
4. Per-annotation Shape size — not changed. Current docs and storage intentionally scope legend settings per dataset and annotation, and the change design explicitly excludes a new dataset/global persistence model. [FEATURE] Keep shape size when changing projection #340 asks for projection retention; changing annotation scope needs a separate product decision.
5. Demo button while already on demo — not changed. Enabling/relabeling that action would expand UI behavior beyond [FEATURE] Keep shape size when changing projection #340. The selected annotation already has Legend settings → Reset; after custom data, the existing demo action remains available and restores bundle defaults.
6. Tooltip-annotation survival — covered and specified. The controller test now proves a silent URL restores the saved tooltip set on automatic startup. The OpenSpec delta/design record that skipping the hash-wide cleanup preserves this existing restore path and that an explicit active-dataset reset still clears hash-scoped keys.
7. Re-curated bundle staleness — documented trade-off; no speculative schema. The design now records that same-hash returning users retain local records because current storage cannot distinguish seeded defaults from edits. Seed fingerprints/provenance would introduce a new schema and precedence policy explicitly outside this change; that policy needs a separate product decision.
8. EAT/controller hash mismatch — implemented with RED/GREEN evidence. The new real-component regression failed with persisted Shape size 42 receiving bundle value 30. The legend now includes annotation_predicted through the final updateDatasetHash() boundary, so it uses the controller's complete hash; the test then passed. The one-time effect on pre-existing prediction-omitting EAT keys is documented.
9. Docs/OpenSpec understatement — addressed. Saved Shape/Point size, automatic reload retention, explicit reset behavior, [BUG] Reloading the default dataset does not restore original state #178's changed reload behavior, tooltip retention, EAT identity, and curated-bundle risk are now recorded.
10. Test placement/scaffolding — addressed. The duplicated controller options now live in one typed harness, and the tests are grouped under EAT settings restore and legend persistence lifecycle. The Playwright helper duplication and multiplier comment were already addressed by 9d58a2fd and 7ef8e846.

Verification on the pushed commit:

• EAT regression RED: expected 42, received 30; GREEN: 11/11.
• Focused Vitest: 50/50.
• Focused Playwright shape-size flow: 1/1.
• Full pnpm test:ci: 1,875 passed, 1 skipped.
• pnpm precommit: passed (including formatting, lint, type-check, knip, docs consistency/build).
• openspec validate preserve-shape-size-across-projection-navigation --strict: passed.

No thread was resolved and no PR state/metadata was changed.