Skip to content

fix(OneDataCollection): guard persisted state and the Save view chip - #5291

Draft
sauldom102 wants to merge 1 commit into
mainfrom
fix/data-collection-storage-hydration-guards
Draft

fix(OneDataCollection): guard persisted state and the Save view chip#5291
sauldom102 wants to merge 1 commit into
mainfrom
fix/data-collection-storage-hydration-guards

Conversation

@sauldom102

Copy link
Copy Markdown
Collaborator

Description

Closes two holes around the persisted collection status. Hydration applied the stored payload verbatim, so state that no longer matches the collection — a renamed filter, a dropped visualization, or a payload written by a different collection under the same storage key — went straight into the data adapter; it is now validated against the declared definition first, the same way seedFromStorage already validates the same payload for the item-navigation seeding path. Separately, the "Save view" chip was offered from state divergence alone and never checked that a view could actually be persisted, so on a collection without an id (or with storage={false}) a saved view lived in component state and vanished on unmount with no signal to the user. In the Factorial monorepo that is 149 of 226 OneDataCollection instances, 74 of them page-level lists.

Screenshots (if applicable)

No new UI. The one visual change is a removal: on a collection with no storage key, the dashed "Save view" chip no longer appears when the view diverges from its baseline. Collections that pass an id are unaffected.

Implementation details

  • fix: validate the stored status against the collection's declared definition before hydrating it

    What gets checked

    New pruneStoredStatus runs on the payload returned by the storage handler, before any featureProvider.setValue:

    • filters — pruned to declared keys. An explicitly stored {} is the user having cleared the filters and is honored; empty by pruning is schema drift, so the declared defaults survive instead.
    • visualizationFilters — each entry pruned the same way, entries for visualizations that no longer exist dropped. Validated against the collection-level filter keys rather than the per-visualization override, so state valid for another visualization is never deleted.
    • sortings — kept only for a declared field. null is preserved: that is an explicit "clear sorting", not drift.
    • grouping — kept only when the field is a declared groupBy.
    • search — kept only when the definition enables search.
    • visualization — kept only for an in-range integer index.
    • navigationFilters — pruned to declared keys.
    • settings and customPresets pass through: stale column ids are inert, and a saved view is user-authored data whose pruning is a separate, more invasive change.

    A non-object payload from a misbehaving handler now yields an empty status instead of throwing on mount.

  • fix: stop offering "Save view" when there is nowhere to persist it

    Why the chip and the shared link, both

    presetActionState now returns "none" unless a storage key is resolved (id present and storage !== false) — mirroring useDataCollectionStorage, which is inactive under exactly those conditions. The shared-link (dc_shared_view) create dialog is gated on the same flag, since saving is the only way a shared view materializes and it was the other route into the save flow. The link param is still stripped either way, so a reload does not retry.

    This overlaps the existing savingViewsDisabled opt-out but is not the same thing: that prop is a consumer choice, this is a capability check.

  • test: cover pruneStoredStatus per feature (23 cases) and the chip/shared-link gates in the presets suite

Known remaining holes

Both are the same class of bug and out of scope here:

  • Value-shape drift. Pruning is key-level. A declared filter whose stored value shape changed still hydrates as-is — e.g. navigation-filter Date values that JSON round-trip into strings.
  • Key collisions. Nothing yet stops two collections from sharing a storage key; pruning only limits the blast radius when they do. A provisional auto-derived key (pathname template + schema fingerprint) plus copy-forward migration when a real id lands is the intended follow-up, and it depends on this PR to be safe.

Two holes around the persisted collection status:

Stored state was applied verbatim on hydration — `featureProvider.setValue`
straight from the handler payload, with no check against the collection's
declared definition. Persisted state that predates a schema change (renamed
or removed filters, a dropped visualization), or that was written by a
different collection under the same key, went directly into the data adapter.
`seedFromStorage` already validates the same payload for the item-navigation
seeding path; hydration now does too, via `pruneStoredStatus`.

The "Save view" chip was offered from state divergence alone, never checking
that there is somewhere to persist a view. On a collection without an `id`
(or with `storage={false}`) the saved view lived in component state and was
lost on unmount, with no signal to the user. It is now gated on a resolved
storage key, as is the shared-link create dialog — the other way into the
save flow.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@github-actions github-actions Bot added fix react Changes affect packages/react labels Aug 28, 2026
@github-actions

Copy link
Copy Markdown
Contributor

🔍 Review policy: Code change

Default rule: any other change needs one approval from f0-devs (rule 4).

Required approvals

Team Why Status
@factorialco/f0-devs Every code change needs a dev approval ⏳ pending
How this was decided
  • PRs touching only sds/ modules require their owners and nothing else.
  • Otherwise, docs-only changes (*.md, *.mdx, *.stories.tsx, anything in __stories__/) → one f0-general approval.
  • Otherwise, feat: titles → one f0-devs and one f0-designers approval. Not a feature? Fix the title prefix.
  • Anything else → one f0-devs approval.
  • Add the needs-design-review label to also request a design approval on any PR.
  • Creating a new sds/ module (new package.yml) additionally requires an f0-general approval.

Policy source: ownership/review-policy.ts · Team members: ownership/teams.yml

@github-actions

Copy link
Copy Markdown
Contributor

✅ No untranslated copy added

Every user-visible string in this PR comes from the i18n layer. Codebase total unchanged at 133.

@github-actions

Copy link
Copy Markdown
Contributor

✅ No New Circular Dependencies

No new circular dependencies detected. Current count: 0

@github-actions

github-actions Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

✅ Storybook docs — no pages lost

Every page reachable on main is still reachable here.

Links point at this PR's Storybook build — browse the full Storybook.

Snapshot of the Storybook index (docs pages + stories) compared against main. Non-blocking.

@github-actions

Copy link
Copy Markdown
Contributor

📦 Alpha Package Version Published

Use pnpm i github:factorialco/f0#npm/alpha-pr-5291 to install the package

Use pnpm i github:factorialco/f0#a3dbb3463b9d50ef0b30ba9f7e627d8d6a189929 to install this specific commit

@github-actions

Copy link
Copy Markdown
Contributor

🔍 Visual review for your branch is published 🔍

Here are the links to:

@github-actions

Copy link
Copy Markdown
Contributor

✅ No breaking public API changes

No public exports were removed, renamed, or had existing props/types changed in a breaking way compared to main.

Comparing f0, experimental and ai against main. Adding components, types, or optional props is safe. This check is non-blocking.

⚠️ Could not analyze component-status (no-base) — a build may have failed; results may be incomplete.

@github-actions

Copy link
Copy Markdown
Contributor

♿ Accessibility (axe) — components changed in this PR

7 issues across 4 stories — all non-blocking (todo).

Story Rule WCAG Impact Nodes Mode
Patterns/Data Collection/Miscellaneous / With Progress Bar color-contrast WCAG 1.4.3 AA (2.0) serious 2 🟡 todo
Patterns/Data Collection/Empty State / Error Example nested-interactive WCAG 4.1.2 A (2.0) serious 1 🟡 todo
Patterns/Data Collection/Empty State / Error Example target-size WCAG 2.5.8 AA (2.2) serious 1 🟡 todo
Patterns/Data Collection/Empty State / Custom Messages And Actions nested-interactive WCAG 4.1.2 A (2.0) serious 1 🟡 todo
Patterns/Data Collection/Empty State / Custom Messages And Actions target-size WCAG 2.5.8 AA (2.2) serious 1 🟡 todo
Patterns/Data Collection/Empty State / Empty To Data Example nested-interactive WCAG 4.1.2 A (2.0) serious 1 🟡 todo
Patterns/Data Collection/Empty State / Empty To Data Example target-size WCAG 2.5.8 AA (2.2) serious 1 🟡 todo

Scope: only stories in the files/component folders this PR changed. It can't yet flag downstream ripple from shared-code/token changes, or diff against main (planned: base-vs-head delta).

@github-actions

Copy link
Copy Markdown
Contributor

Coverage Report for packages/react

Status Category Percentage Covered / Total
🔵 Lines 69.21% 29372 / 42435
🔵 Statements 68.17% 31077 / 45584
🔵 Functions 62.07% 7004 / 11284
🔵 Branches 61.93% 22075 / 35640
File Coverage
File Stmts Branches Functions Lines Uncovered Lines
Changed Files
packages/react/src/patterns/OneDataCollection/OneDatacollection.tsx 89.1% 74.39% 88.46% 90.88% 356, 434, 440-442, 452, 469, 562, 615, 621-622, 640-642, 647, 719, 756-760, 769-772, 823, 854, 885-886, 904-910, 946-949, 1065-1071, 1126, 1181, 1185, 1237, 1305, 1333, 1341, 1365, 1393, 1405, 1419, 1623, 1630, 1846-1870
packages/react/src/patterns/OneDataCollection/hooks/useDataColectionStorage/pruneStoredStatus.ts 100% 98% 100% 100%
packages/react/src/patterns/OneDataCollection/hooks/useDataColectionStorage/useDataCollectionStorage.ts 91.48% 87.5% 100% 91.3% 65-69, 157, 173-174
Generated in workflow #17600 for commit cb56c85 by the Vitest Coverage Report Action

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fix react Changes affect packages/react

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant