diff --git a/packages/react/src/patterns/OneDataCollection/OneDatacollection.tsx b/packages/react/src/patterns/OneDataCollection/OneDatacollection.tsx index d58065a2c6..f0899e0bc3 100644 --- a/packages/react/src/patterns/OneDataCollection/OneDatacollection.tsx +++ b/packages/react/src/patterns/OneDataCollection/OneDatacollection.tsx @@ -1208,6 +1208,17 @@ const OneDataCollectionComp = < null ) + /** + * Saved views live in the persisted collection status, so they only survive a + * navigation when there is somewhere to write them: a storage key (`id`) and + * storage left enabled. Without both, `useDataCollectionStorage` is inactive + * and a saved view would exist only in this component's state until unmount. + */ + const canPersistViews = useMemo( + () => storage !== false && !!id, + [storage, id] + ) + /** * Whether to offer "Save view" (create a new view): * - a view is selected → "none" (diverging from it auto-deselects via the @@ -1224,6 +1235,9 @@ const OneDataCollectionComp = < // Consumer opted out of saving views (e.g. the org-chart graph): never show // the "Save view" chip regardless of how the view diverges from the baseline. if (savingViewsDisabled) return "none" + // Nothing can be persisted, so offering to save would silently discard the + // view on unmount. + if (!canPersistViews) return "none" // Compares everything except the view mode, so a visualization-only change // does not count as a reason to save a new view. const sameIgnoringVisualization = (a: ViewSnapshot, b: ViewSnapshot) => @@ -1256,6 +1270,7 @@ const OneDataCollectionComp = < return "none" }, [ savingViewsDisabled, + canPersistViews, selectedPresetId, mergedPresets, capturedState, @@ -1411,7 +1426,12 @@ const OneDataCollectionComp = < // just hit Save; strip the param afterwards so a reload doesn't reopen it. useEffect(() => { if (!sharedPreset) return - setPresetDialog({ mode: "create", shared: sharedPreset }) + // Saving is the only way a shared view materializes, so skip the dialog + // when there is nowhere to persist it rather than offer a save that is + // discarded on unmount. The param is still stripped either way. + if (canPersistViews) { + setPresetDialog({ mode: "create", shared: sharedPreset }) + } if (typeof window !== "undefined") { const params = new URLSearchParams(window.location.search) params.delete(SHARED_PRESET_PARAM) @@ -1482,6 +1502,17 @@ const OneDataCollectionComp = < } : {}), }, + { + // The collection-level filters, not `effectiveFilters`: validation must + // use the superset so state stored for one visualization is not dropped + // while another is active. + filters, + sortings, + grouping, + navigationFilters, + search, + visualizationCount: visualizations.length, + }, storage === false ) diff --git a/packages/react/src/patterns/OneDataCollection/__tests__/presets.test.tsx b/packages/react/src/patterns/OneDataCollection/__tests__/presets.test.tsx index 7535d47bf2..9e9a1532c1 100644 --- a/packages/react/src/patterns/OneDataCollection/__tests__/presets.test.tsx +++ b/packages/react/src/patterns/OneDataCollection/__tests__/presets.test.tsx @@ -80,11 +80,14 @@ const visualizations = [ function Harness({ presets, id = "presets-test/v1", + storage, onState, urlSync = false, }: { presets?: PresetsDefinition - id?: string + /** `null` drops the storage key entirely, as a collection with no `id` does. */ + id?: string | null + storage?: false onState?: (state: { filters: unknown sortings: unknown @@ -104,7 +107,8 @@ function Harness({ return ( @@ -206,6 +210,32 @@ describe("OneDataCollection - presets", () => { ).toBeInTheDocument() }) + it("does not offer 'Save view' without a storage key, since nothing could be persisted", async () => { + const user = userEvent.setup() + renderHarness({ id: null }) + + await waitFor(() => expect(screen.getByText("John")).toBeInTheDocument()) + + await sortByName(user) + + expect( + screen.queryByRole("button", { name: "Save view" }) + ).not.toBeInTheDocument() + }) + + it("does not offer 'Save view' when storage is disabled", async () => { + const user = userEvent.setup() + renderHarness({ storage: false }) + + await waitFor(() => expect(screen.getByText("John")).toBeInTheDocument()) + + await sortByName(user) + + expect( + screen.queryByRole("button", { name: "Save view" }) + ).not.toBeInTheDocument() + }) + it("creates a custom preset via the dialog, selecting it and persisting to storage", async () => { const user = userEvent.setup() const { set } = renderHarness() @@ -856,6 +886,27 @@ describe("OneDataCollection - share preset", () => { ).toBeInTheDocument() }) + it("ignores a shared link without a storage key, still stripping the param", async () => { + const encoded = encodeSharedPreset({ + label: "Imported view", + filter: { department: ["eng"] }, + visualization: 0, + }) + window.history.replaceState({}, "", `/?${SHARED_PRESET_PARAM}=${encoded}`) + + renderHarness({ id: null }) + await waitFor(() => expect(screen.getByText("John")).toBeInTheDocument()) + + // Saving is what materializes a shared view, and there is nowhere to save + // it — so no dialog is offered... + expect(screen.queryByLabelText("Title")).not.toBeInTheDocument() + + // ...and the param is dropped anyway, so a reload does not retry. + expect( + new URLSearchParams(window.location.search).has(SHARED_PRESET_PARAM) + ).toBe(false) + }) + it("opens the prefilled create dialog from a shared link and saves the shared config", async () => { const user = userEvent.setup() const encoded = encodeSharedPreset({ diff --git a/packages/react/src/patterns/OneDataCollection/hooks/useDataColectionStorage/__tests__/pruneStoredStatus.test.ts b/packages/react/src/patterns/OneDataCollection/hooks/useDataColectionStorage/__tests__/pruneStoredStatus.test.ts new file mode 100644 index 0000000000..d8dcc56441 --- /dev/null +++ b/packages/react/src/patterns/OneDataCollection/hooks/useDataColectionStorage/__tests__/pruneStoredStatus.test.ts @@ -0,0 +1,203 @@ +import { describe, expect, it } from "vitest" + +import { pruneStoredStatus, StoredStatusDefinition } from "../pruneStoredStatus" +import { DataCollectionStatusComplete } from "../types" + +type Status = DataCollectionStatusComplete> + +const definition: StoredStatusDefinition = { + filters: { department: {}, status: {} }, + sortings: { name: {}, createdAt: {} }, + grouping: { groupBy: { team: {} } }, + navigationFilters: { period: {} }, + search: { enabled: true }, + visualizationCount: 2, +} + +const prune = (stored: Status, override?: StoredStatusDefinition) => + pruneStoredStatus(stored, override ?? definition) + +describe("pruneStoredStatus", () => { + describe("filters", () => { + it("keeps declared keys and drops undeclared ones", () => { + const result = prune({ + filters: { department: ["eng"], removedFilter: ["x"] }, + } as Status) + + expect(result.filters).toEqual({ department: ["eng"] }) + }) + + it("honors an explicitly cleared state", () => { + expect(prune({ filters: {} } as Status).filters).toEqual({}) + }) + + it("drops the whole value when every stored key is undeclared, so the declared defaults survive", () => { + const result = prune({ filters: { removedFilter: ["x"] } } as Status) + + expect(result.filters).toBeUndefined() + }) + + it("drops a non-object value", () => { + const result = prune({ + filters: "corrupt" as unknown as Record, + } as Status) + + expect(result.filters).toBeUndefined() + }) + + it("drops everything when the collection declares no filters", () => { + const result = prune({ filters: { department: ["eng"] } } as Status, { + visualizationCount: 1, + }) + + expect(result.filters).toBeUndefined() + }) + }) + + describe("visualizationFilters", () => { + it("prunes each entry against the collection-level filter keys", () => { + const result = prune({ + visualizationFilters: { + "0": { department: ["eng"], removedFilter: ["x"] }, + "1": { status: ["active"] }, + }, + } as Status) + + expect(result.visualizationFilters).toEqual({ + "0": { department: ["eng"] }, + "1": { status: ["active"] }, + }) + }) + + it("drops entries for visualizations the collection no longer declares", () => { + const result = prune({ + visualizationFilters: { + "0": { department: ["eng"] }, + "5": { department: ["ops"] }, + }, + } as Status) + + expect(result.visualizationFilters).toEqual({ + "0": { department: ["eng"] }, + }) + }) + + it("drops the map when no entry survives", () => { + const result = prune({ + visualizationFilters: { "9": { removedFilter: ["x"] } }, + } as Status) + + expect(result.visualizationFilters).toBeUndefined() + }) + }) + + describe("sortings", () => { + it("keeps a declared field", () => { + const stored = { field: "name", order: "asc" } as const + + expect(prune({ sortings: stored } as Status).sortings).toEqual(stored) + }) + + it("drops an undeclared field", () => { + const result = prune({ + sortings: { field: "removedColumn", order: "asc" }, + } as Status) + + expect(result.sortings).toBeUndefined() + }) + + it("keeps null, which is an explicit user clear rather than drift", () => { + expect(prune({ sortings: null } as Status).sortings).toBeNull() + }) + }) + + describe("grouping", () => { + it("keeps a declared groupBy field", () => { + const stored = { field: "team", order: "asc" } as const + + expect(prune({ grouping: stored } as Status).grouping).toEqual(stored) + }) + + it("drops an undeclared groupBy field", () => { + const result = prune({ grouping: { field: "removedGroup" } } as Status) + + expect(result.grouping).toBeUndefined() + }) + }) + + describe("search", () => { + it("keeps a stored term when search is enabled", () => { + expect(prune({ search: "ada" } as Status).search).toBe("ada") + }) + + it("drops a stored term when search is not enabled", () => { + const result = prune({ search: "ada" } as Status, { + visualizationCount: 1, + }) + + expect(result.search).toBeUndefined() + }) + }) + + describe("visualization", () => { + it("keeps an in-range index", () => { + expect(prune({ visualization: 1 } as Status).visualization).toBe(1) + }) + + it("drops an index past the declared visualizations", () => { + expect( + prune({ visualization: 4 } as Status).visualization + ).toBeUndefined() + }) + + it("drops a negative or non-integer index", () => { + expect( + prune({ visualization: -1 } as Status).visualization + ).toBeUndefined() + expect( + prune({ visualization: 1.5 } as Status).visualization + ).toBeUndefined() + }) + }) + + describe("navigationFilters", () => { + it("keeps declared keys and drops undeclared ones", () => { + const result = prune({ + navigationFilters: { period: { from: "2026-01-01" }, removed: {} }, + } as Status) + + expect(result.navigationFilters).toEqual({ + period: { from: "2026-01-01" }, + }) + }) + }) + + describe("pass-through features", () => { + it("leaves settings untouched, since stale column ids match no column", () => { + const settings = { columns: { removedColumn: { hidden: true } } } + + expect(prune({ settings } as unknown as Status).settings).toEqual( + settings + ) + }) + + it("leaves customPresets untouched, since a saved view is user-authored data", () => { + const customPresets = [ + { id: "mine", label: "Mine", filter: { removedFilter: ["x"] } }, + ] + + expect( + prune({ customPresets } as unknown as Status).customPresets + ).toEqual(customPresets) + }) + }) + + it("omits every feature absent from the stored payload", () => { + expect(prune({} as Status)).toEqual({}) + }) + + it("returns an empty status for a non-object payload", () => { + expect(prune(null as unknown as Status)).toEqual({}) + expect(prune("corrupt" as unknown as Status)).toEqual({}) + }) +}) diff --git a/packages/react/src/patterns/OneDataCollection/hooks/useDataColectionStorage/__tests__/useDataCollectionStorage.test.tsx b/packages/react/src/patterns/OneDataCollection/hooks/useDataColectionStorage/__tests__/useDataCollectionStorage.test.tsx index a35a634d89..1bdec2e3cf 100644 --- a/packages/react/src/patterns/OneDataCollection/hooks/useDataColectionStorage/__tests__/useDataCollectionStorage.test.tsx +++ b/packages/react/src/patterns/OneDataCollection/hooks/useDataColectionStorage/__tests__/useDataCollectionStorage.test.tsx @@ -16,6 +16,7 @@ import { } from "@/lib/providers/datacollection/types" import { TestProviders, zeroRenderHook } from "@/testing/test-utils" +import { StoredStatusDefinition } from "../pruneStoredStatus" import { useDataCollectionStorage } from "../useDataCollectionStorage" import { DataCollectionStorageFeaturesDefinition, @@ -126,6 +127,16 @@ const buildFeatureProviders = ( } } +/** + * Declares every filter key and visualization these tests persist, so hydration + * validation is a no-op here and the assertions stay about the write race. + * `pruneStoredStatus` is covered on its own. + */ +const definition: StoredStatusDefinition = { + filters: { x: {}, y: {}, seeded: {}, fromA: {}, fromB: {} }, + visualizationCount: 2, +} + const wrapperWith = (handler: DataCollectionStorageHandler) => { const Wrapper = ({ children }: { children: React.ReactNode }) => ( @@ -186,7 +197,8 @@ describe("useDataCollectionStorage — pre-hydration write race", () => { useDataCollectionStorage( "test/v1", features, - providers as AnyFeatureProviders + providers as AnyFeatureProviders, + definition ), { wrapper: wrapperWith(handler), @@ -242,7 +254,8 @@ describe("useDataCollectionStorage — pre-hydration write race", () => { useDataCollectionStorage( "test/v1", features, - providers as AnyFeatureProviders + providers as AnyFeatureProviders, + definition ), { wrapper: wrapperWith(handler), @@ -284,7 +297,8 @@ describe("useDataCollectionStorage — pre-hydration write race", () => { useDataCollectionStorage( "test/v1", features, - providers as AnyFeatureProviders + providers as AnyFeatureProviders, + definition ), { wrapper: wrapperWith(handler), @@ -331,7 +345,8 @@ describe("useDataCollectionStorage — pre-hydration write race", () => { useDataCollectionStorage( undefined, features, - buildProviders() as AnyFeatureProviders + buildProviders() as AnyFeatureProviders, + definition ), { wrapper: wrapperWith(handler), @@ -356,6 +371,7 @@ describe("useDataCollectionStorage — pre-hydration write race", () => { "test/v1", features, buildProviders() as AnyFeatureProviders, + definition, true ), { @@ -393,7 +409,8 @@ describe("useDataCollectionStorage — pre-hydration write race", () => { useDataCollectionStorage( storageKey, features, - providers as AnyFeatureProviders + providers as AnyFeatureProviders, + definition ), { wrapper: wrapperWith(handler), diff --git a/packages/react/src/patterns/OneDataCollection/hooks/useDataColectionStorage/pruneStoredStatus.ts b/packages/react/src/patterns/OneDataCollection/hooks/useDataColectionStorage/pruneStoredStatus.ts new file mode 100644 index 0000000000..d22c79fe39 --- /dev/null +++ b/packages/react/src/patterns/OneDataCollection/hooks/useDataColectionStorage/pruneStoredStatus.ts @@ -0,0 +1,181 @@ +import { + FiltersDefinition, + FiltersState, +} from "@/patterns/OneFilterPicker/types" + +import { DataCollectionStatusComplete } from "./types" + +/** + * The declared shape a stored payload is validated against. + * + * Every definition is deliberately typed as a bare `object`: the only thing + * validation needs from it is its set of declared keys, and the real + * definitions (`FiltersDefinition`, `SortingsDefinition`, + * `GroupingDefinition`…) are mapped types whose generics would otherwise + * have to be threaded through this module for no gain. + * + * An absent definition means the collection declares no such feature, so any + * stored value for it is stale by construction and gets dropped. + */ +export type StoredStatusDefinition = { + filters?: object + sortings?: object + grouping?: { groupBy?: object } + navigationFilters?: object + search?: { enabled?: boolean } + /** Number of declared visualizations; a stored index outside it is stale. */ + visualizationCount?: number +} + +const isPlainObject = (value: unknown): value is Record => + typeof value === "object" && value !== null && !Array.isArray(value) + +const declaredKeys = (definition: object | undefined): Set => + new Set(definition ? Object.keys(definition) : []) + +const isDeclaredVisualization = ( + index: unknown, + visualizationCount: number | undefined +): index is number => + typeof index === "number" && + Number.isInteger(index) && + index >= 0 && + (visualizationCount === undefined || index < visualizationCount) + +/** + * Drops the entries of a stored key/value state that the definition no longer + * declares. Returns `undefined` when the whole value should be discarded. + * + * An explicitly stored empty state is the user having cleared everything and is + * honored as-is. Empty *by pruning* — every stored key unknown — is schema + * drift rather than intent, so the collection keeps its declared defaults. + */ +const pruneToDeclared = ( + stored: unknown, + declared: Set +): T | undefined => { + if (!isPlainObject(stored)) return undefined + const kept = Object.fromEntries( + Object.entries(stored).filter(([key]) => declared.has(key)) + ) + if (Object.keys(kept).length === 0 && Object.keys(stored).length > 0) { + return undefined + } + return kept as T +} + +/** + * Validates a stored data collection status against the collection's declared + * definition, dropping everything that no longer applies. + * + * Stored state is untrusted input: it can predate a schema change (renamed or + * removed filters, a dropped visualization) or — when two collections end up + * sharing a storage key — belong to an entirely different collection. Applying + * it verbatim pushes undeclared filter keys straight into the data adapter, so + * every piece is checked against the definition first. + * + * Mirrors the validation `seedFromStorage` already performs for the item + * navigation seeding path, which reads the same persisted payload. + * + * Two features pass through untouched: + * - `settings` carries per-column preferences keyed by ids this module cannot + * resolve; stale entries match no column and are inert. + * - `customPresets` is user-authored data. Pruning the filters captured inside + * a saved view is a separate, more invasive change — a stale preset only + * reaches the adapter when the user explicitly selects it, not on hydration. + */ +export const pruneStoredStatus = < + CurrentFiltersState extends FiltersState, +>( + stored: DataCollectionStatusComplete, + definition: StoredStatusDefinition +): DataCollectionStatusComplete => { + // A handler is free to resolve with anything; a non-object payload carries no + // recoverable state. + if (!isPlainObject(stored)) return {} + + const filterKeys = declaredKeys(definition.filters) + const pruned: DataCollectionStatusComplete = {} + + if (stored.settings !== undefined) { + pruned.settings = stored.settings + } + + if (stored.customPresets !== undefined) { + pruned.customPresets = stored.customPresets + } + + if (stored.filters !== undefined) { + const filters = pruneToDeclared( + stored.filters, + filterKeys + ) + if (filters !== undefined) pruned.filters = filters + } + + if (stored.navigationFilters !== undefined) { + const navigationFilters = pruneToDeclared< + NonNullable + >(stored.navigationFilters, declaredKeys(definition.navigationFilters)) + if (navigationFilters !== undefined) { + pruned.navigationFilters = navigationFilters + } + } + + if (isPlainObject(stored.visualizationFilters)) { + // Validated against the collection-level filter keys rather than the + // narrower per-visualization override: the superset never deletes state + // that is valid for another visualization, and narrowing per view is + // already usePerVisualizationFilters' job. + const perVisualization = Object.entries(stored.visualizationFilters) + .filter(([index]) => + isDeclaredVisualization(Number(index), definition.visualizationCount) + ) + .map( + ([index, filters]) => + [ + index, + pruneToDeclared(filters, filterKeys), + ] as const + ) + .filter( + (entry): entry is readonly [string, CurrentFiltersState] => + entry[1] !== undefined + ) + if (perVisualization.length > 0) { + pruned.visualizationFilters = Object.fromEntries(perVisualization) + } + } + + // `null` is the user having explicitly cleared the sorting and is applied; + // `undefined` leaves the declared defaults in place. + if (stored.sortings === null) { + pruned.sortings = null + } else if ( + stored.sortings && + declaredKeys(definition.sortings).has(String(stored.sortings.field)) + ) { + pruned.sortings = stored.sortings + } + + if ( + stored.grouping?.field !== undefined && + declaredKeys(definition.grouping?.groupBy).has( + String(stored.grouping.field) + ) + ) { + pruned.grouping = stored.grouping + } + + if (typeof stored.search === "string" && definition.search?.enabled) { + pruned.search = stored.search + } + + if ( + isDeclaredVisualization(stored.visualization, definition.visualizationCount) + ) { + pruned.visualization = stored.visualization + } + + return pruned +} diff --git a/packages/react/src/patterns/OneDataCollection/hooks/useDataColectionStorage/useDataCollectionStorage.ts b/packages/react/src/patterns/OneDataCollection/hooks/useDataColectionStorage/useDataCollectionStorage.ts index e4f342ea90..e7d4ea8755 100644 --- a/packages/react/src/patterns/OneDataCollection/hooks/useDataColectionStorage/useDataCollectionStorage.ts +++ b/packages/react/src/patterns/OneDataCollection/hooks/useDataColectionStorage/useDataCollectionStorage.ts @@ -1,4 +1,4 @@ -import { useEffect, useMemo, useState } from "react" +import { useEffect, useMemo, useRef, useState } from "react" import { useDebounceCallback } from "usehooks-ts" import { NavigationFiltersDefinition } from "@/patterns/OneDataCollection/navigationFilters/types" @@ -12,6 +12,7 @@ import { import { useDataCollectionStorage as useDataCollectionStorageProvider } from "@/lib/providers/datacollection/DataCollectionStorageProvider" import { getFeatures } from "./getFeatures" +import { pruneStoredStatus, StoredStatusDefinition } from "./pruneStoredStatus" import { DataCollectionStatus, DataCollectionStorageFeature, @@ -29,6 +30,10 @@ type UseDataCollectionStorage = { * @param key - The storage key * @param featuresDef - The features definition * @param settings - The settings + * @param definition - The collection's declared shape. Stored state is + * validated against it on hydration so state that no longer applies (schema + * drift, or a payload written by a different collection under the same key) + * never reaches the data source. * @returns The settings in storage and the settings storage ready */ @@ -48,6 +53,7 @@ export const useDataCollectionStorage = < Filters, NavigationFilters >, + definition: StoredStatusDefinition, disabled?: boolean ): UseDataCollectionStorage => { const [storageReady, setStorageReady] = useState(false) @@ -76,6 +82,15 @@ export const useDataCollectionStorage = < return !disabled && !!key }, [disabled, key]) + // Latest-value ref so the hydration effect can validate against the current + // definition without re-running every time the (inline) definition object is + // rebuilt. Seeded with the mount-time definition, which is the one hydration + // needs, and refreshed after every render for later key changes. + const definitionRef = useRef(definition) + useEffect(() => { + definitionRef.current = definition + }) + /** Gets the settings in storage when the key and features change */ useEffect(() => { if (!active) { @@ -89,7 +104,8 @@ export const useDataCollectionStorage = < // values left over from the previous key/inactive state. setStorageReady(false) - storageProvider.get(key!).then((status) => { + storageProvider.get(key!).then((rawStatus) => { + const status = pruneStoredStatus(rawStatus, definitionRef.current) Object.entries(featureProviders).forEach( ([featureName, featureProvider]) => { if (