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

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
   )
 

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<typeof filters>
-  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 (
     <OneDataCollection
-      id={id}
+      id={id ?? undefined}
+      storage={storage}
       source={source}
       visualizations={visualizations}
       onStateChange={(state) =>
@@ -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({

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<Record<string, unknown>>
+
+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<string, unknown>,
+      } 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({})
+  })
+})