fix(stores): close persistence fallbacks flagged in PR #358 review#359
Conversation
Three same-class silent-fallback issues where persisted page-store behavior depended on navigation history or skipped a check: 1. Mismatched persist scopes (validate_stores.go): two pages persisting the same store name with the same shape but different scopes (local vs session) share one storage key. The runtime keeps whichever scope initialized first, so the effective backend depended on nav order. Now warns with page_store_persist_scope_conflict (registered + explain entry). 2. Dropped persist config on re-hydration (runtime_assets.go): when a store was first declared without persistence and a later route declared it persisted, registry.init early-returned and silently discarded the persist config, so writes never reached storage. init now adopts the persist config for an existing-but-unpersisted store and restores the saved value, independent of navigation order. 3. Nested secret fields (validate_page.go): the secret-field check only scanned top-level fields, so a persisted store nesting e.g. Profile.Token wrote a credential-shaped value to browser storage with no warning. It now scans nested field paths from FieldTypes. Tests: scope-conflict + nested-secret compiler tests, a node store-runtime step asserting persistence adoption across SPA navigation, and a nested-secret fixture type. CHANGELOG, docs (syntax/components), and explain updated. Full non-example suite green.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 32c10568f8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (persist && persist.scope && persist.key && persist.version && !registry.persist[name]) { | ||
| registry.persist[name] = persist; | ||
| const restored = readPersisted(persist, registry.fields[name]); |
There was a problem hiding this comment.
Use the later seed fields when adopting persistence
When SPA navigation first initializes a store without persistence and later reaches a persisted declaration with the same store name but a different shape, this adoption path restores and later writes using registry.fields[name] from the first, unpersisted seed rather than the persisted route's seed. That can drop all fields from the saved blob and then overwrite storage under the persisted route's version with the wrong field set, corrupting persisted state; either update the registry fields/seeds from state during adoption or reject/warn on unpersisted-to-persisted shape mismatches.
Useful? React with 👍 / 👎.
Addresses the three P2 review findings on #358 (client-side store persistence). Stacked on
feat/store-persistso it contains only the fixes; merge into that branch to resolve the review comments. All three are the same class — persisted-store behavior silently depending on navigation history, or a check that skipped data it should cover.1. Reject same-name stores with different persist scopes
validate_stores.goonly compared struct shape, so two pages persisting the same store name with the same shape but different scopes (persist "local"vspersist "session") passed silently. They share one storage key, and the runtime keeps whichever scope initialized first — so the effective backend depended on navigation order. Now warns withpage_store_persist_scope_conflict(registered in the diagnostics registry +explainentry).2. Preserve persistence when a store is already initialized
registry.initearly-returned for an already-initialized store, so SPA-navigating from a page with an unpersistedcartto a page that declarescartaspersist "local"silently dropped the persist config — subsequent writes never reached storage, while a direct load did persist.initnow adopts the persist config for an existing-but-unpersisted store and restores its saved value, so persistence no longer depends on which route loaded first. Conflicting scopes stay first-wins (and are reported by fix #1), so navigation can't thrash storage.3. Check nested persisted fields for secret-like names
The secret-field check scanned only top-level fields, so a persisted store nesting e.g.
Profile.Tokenwrote a credential-shaped value tolocalStorage/sessionStoragewith nopage_store_persist_secret_fieldwarning. It now scans nested field paths fromgotypes.Struct.FieldTypes(slice/array markers stripped, deduped, sorted), reporting the full path (Account.Token).Tests
internal/compiler:TestValidateWarnsOnPersistedStoreScopeConflict,TestValidatePageWarnsOnNestedPersistedSecretField(+ProfileState/Credentialsnested fixture intestfixture/islands).internal/buildgen: new step in the Node store-runtime harness asserting persistence adoption + storage write-through across SPA navigation.go test ./...(55 non-example packages) green; gofmt + vet clean.Docs
CHANGELOG,
docs/language/syntax.md, anddocs/language/components.mdupdated for the new diagnostic, nested-secret scanning, and the adoption behavior.