fix(data-objectstack): read view overrides from the type='view' namespace they are written to (#3774)

.changeset/view-overrides-readback.md

@@ -0,0 +1,13 @@
+---
+"@object-ui/data-objectstack": patch
+"@object-ui/app-shell": patch
+"@object-ui/types": patch
+---
+
+Fix saved list-view preferences never reading back (density, column widths, sort, hidden columns, inline edit)
+
+`listViewOverrides` in the ObjectStack adapter enumerated `GET /api/v1/meta/{objectName}` — putting the object name in the metadata **type** slot — while `updateViewConfig` persists under `type='view'`. The two key spaces are disjoint, so the batch map came back empty for every object and every personalization a user saved on a list view was written to the server but never read back, showing up as "the setting didn't save".
+
+The read now enumerates `type='view'` once and narrows to the object client-side, through the same accessor `listViews()` uses over the same rows — the metadata index is name-only, so there is no server-side `?object=` filter to push it into.
+
+Second half: the batch read no longer swallows its own failures into an empty map. An empty map is an authoritative "this object has no overrides" and callers may still trust it and skip the per-view reads (the batch optimization is intact), but a transport failure now rejects, so the per-view `getView` fallback it was silently disabling becomes reachable again. `DataSource.listViewOverrides` documents both terms so other adapters implement the same contract.

packages/app-shell/src/views/InterfaceListPage.tsx

@@ -293,8 +293,16 @@ export function InterfaceListPage({ page, className, onConfigChange, reserveEdit
         const ds: any = dataSource;
         let full: any = null;
         if (typeof ds?.listViewOverrides === 'function') {
-          const all = await ds.listViewOverrides(cfg.source);
-          full = all?.[resolvedViewKey] ?? null;
+          // Scoped catch: the batch enumeration REJECTS on a transport failure
+          // rather than answering an authoritative-looking `{}` (objectui#3774
+          // / the `DataSource.listViewOverrides` contract). That failure must
+          // not skip the per-view `getView` below — it is precisely the case
+          // the fallback exists for. Without this the outer catch would abort
+          // the whole hydration and the view would stay hollow.
+          try {
+            const all = await ds.listViewOverrides(cfg.source);
+            full = all?.[resolvedViewKey] ?? null;
+          } catch { /* fall through to the per-view read */ }
         }
         if (!hasColumns(full) && typeof ds?.getView === 'function') {
           full = await ds.getView(cfg.source, resolvedViewKey);

packages/app-shell/src/views/ObjectView.tsx

@@ -156,6 +156,67 @@ export function defaultListColumnsFromObject(
     return [];
 }
 
+/**
+ * Read the persisted per-view overrides (density, column widths, sort, hidden
+ * columns, inlineEdit …) for `ids`, preferring the adapter's one-request batch
+ * enumeration and falling back to a per-view `getView`.
+ *
+ * Extracted from the effect below so the three-way branch is pinnable on its
+ * own (objectui#3774) — the file's existing precedent for this is
+ * {@link defaultListColumnsFromObject}.
+ *
+ * The batch/fallback choice hangs entirely on how `listViewOverrides` ANSWERS,
+ * so the two are spelled out:
+ *
+ * - RESOLVES (including to an empty map) → authoritative. The adapter looked
+ *   and reports what exists; an object whose views were never customized
+ *   legitimately has no overrides, and we take that answer and skip N GETs.
+ *   That is the whole point of the batch call — do not "helpfully" re-probe
+ *   per view on an empty map, that reinstates the 404 flurry it removes.
+ * - REJECTS → the adapter could not tell, which is NOT the same fact as
+ *   "nothing is there". Fall through to the per-view reads.
+ *
+ * An adapter that swallows its own failures into `{}` collapses those two into
+ * the authoritative branch and makes the fallback below unreachable code —
+ * that was objectui#3774's second half, fixed in `@object-ui/data-objectstack`.
+ */
+export async function loadViewOverrides(
+    dataSource: any,
+    objectName: string,
+    ids: string[],
+): Promise<Record<string, any>> {
+    if (typeof dataSource?.listViewOverrides === 'function') {
+        try {
+            const all = await dataSource.listViewOverrides(objectName);
+            if (all && typeof all === 'object') {
+                const map: Record<string, any> = {};
+                for (const id of ids) {
+                    if (all[id]) map[id] = all[id];
+                }
+                return map;
+            }
+        } catch {
+            // fall through to per-view fetch
+        }
+    }
+    if (typeof dataSource?.getView !== 'function') return {};
+    const entries = await Promise.all(
+        ids.map(async (id) => {
+            try {
+                const v = await dataSource.getView(objectName, id);
+                return [id, v] as const;
+            } catch {
+                return [id, null] as const;
+            }
+        })
+    );
+    const map: Record<string, any> = {};
+    for (const [id, v] of entries) {
+        if (v && typeof v === 'object') map[id] = v;
+    }
+    return map;
+}
+
 export function ObjectView({ dataSource, objects, onEdit, externalRefreshKey }: any) {
     const { objectName } = useParams();
     const { t } = useObjectTranslation();
@@ -543,11 +604,9 @@ function ObjectViewInner({ dataSource, objects, onEdit, externalRefreshKey }: an
     // `dataSource.updateViewConfig` and read back here so toggle preferences
     // survive a hard reload. Keyed by viewId → partial view config to merge.
     //
-    // Use the batch listViewOverrides() when available — fires one HTTP
-    // GET per object instead of N (one per defined view), avoiding a flurry
-    // of 404s for objects whose views have never been customized. Falls
-    // back to per-view getView() for adapters that don't support the batch
-    // method.
+    // Reading strategy (batch first, per-view fallback) lives in the exported
+    // `loadViewOverrides` above so it can be pinned directly — see its doc for
+    // why an empty batch result is authoritative but a rejected one is not.
     const [viewOverrides, setViewOverrides] = useState<Record<string, any>>({});
     useEffect(() => {
         let cancelled = false;
@@ -567,40 +626,7 @@ function ObjectViewInner({ dataSource, objects, onEdit, externalRefreshKey }: an
             return;
         }
 
-        const loadBatch = async (): Promise<Record<string, any>> => {
-            if (typeof (dataSource as any).listViewOverrides === 'function') {
-                try {
-                    const all = await (dataSource as any).listViewOverrides(objectName);
-                    if (all && typeof all === 'object') {
-                        const map: Record<string, any> = {};
-                        for (const id of ids) {
-                            if (all[id]) map[id] = all[id];
-                        }
-                        return map;
-                    }
-                } catch {
-                    // fall through to per-view fetch
-                }
-            }
-            if (typeof dataSource.getView !== 'function') return {};
-            const entries = await Promise.all(
-                ids.map(async (id) => {
-                    try {
-                        const v = await dataSource.getView!(objectName, id);
-                        return [id, v] as const;
-                    } catch {
-                        return [id, null] as const;
-                    }
-                })
-            );
-            const map: Record<string, any> = {};
-            for (const [id, v] of entries) {
-                if (v && typeof v === 'object') map[id] = v;
-            }
-            return map;
-        };
-
-        loadBatch().then((map) => {
+        loadViewOverrides(dataSource, objectName, ids).then((map) => {
             if (!cancelled) setViewOverrides(map);
         });
         return () => { cancelled = true; };

packages/app-shell/src/views/ObjectView.viewOverrides.test.tsx

@@ -0,0 +1,139 @@
+/**
+ * ObjectUI
+ * Copyright (c) 2024-present ObjectStack Inc.
+ *
+ * This source code is licensed under the MIT license found in the
+ * LICENSE file in the root directory of this source tree.
+ */
+
+/**
+ * objectui#3774, second half — the batch read must not answer "I don't know"
+ * with a value that looks authoritative.
+ *
+ * `loadViewOverrides` prefers the adapter's one-request `listViewOverrides`
+ * and falls back to a per-view `getView`. Which branch runs depends entirely
+ * on how the batch call ANSWERS, and the two answers below are different facts:
+ *
+ *   resolves (even to `{}`)  → authoritative "this is what exists" → take it,
+ *                              skip N GETs. That IS the batch optimization.
+ *   rejects                  → "could not tell" → fall through to per-view.
+ *
+ * `@object-ui/data-objectstack` used to `catch {}` its own failures into `{}`,
+ * collapsing the second case into the first. The fallback below was therefore
+ * unreachable code on that adapter — which is why the wrong-namespace read
+ * (fixed in the same PR) surfaced as a silent "settings didn't save" instead of
+ * an error the fallback could recover from.
+ *
+ * REVERSE VERIFICATION — direction predicted before running, both observed:
+ *   • restore the adapter's swallow (`catch { return {} }` in
+ *     `listViewOverrides`) → the adapter suite's "a FAILING enumeration
+ *     rejects" pin goes RED, and the fall-through case here stays green while
+ *     being unreachable in production — which is exactly why the adapter-side
+ *     pin has to exist and this file alone is not enough.
+ *   • re-inline an unconditional `return map` (dropping the `catch`) here →
+ *     'falls through to per-view getView when the batch read REJECTS' goes RED.
+ */
+
+import { describe, it, expect, vi } from 'vitest';
+import { loadViewOverrides } from './ObjectView';
+
+const IDS = ['crm_lead.all', 'crm_lead.pipeline'];
+
+const OVERRIDE = { name: 'crm_lead.all', object: 'crm_lead', density: 'compact' };
+
+describe('loadViewOverrides — batch first (#3774)', () => {
+  it('uses the batch result and skips the per-view reads entirely', async () => {
+    const getView = vi.fn(async () => ({ never: true }));
+    const dataSource = {
+      listViewOverrides: vi.fn(async () => ({ 'crm_lead.all': OVERRIDE })),
+      getView,
+    };
+
+    const map = await loadViewOverrides(dataSource, 'crm_lead', IDS);
+
+    expect(map).toEqual({ 'crm_lead.all': OVERRIDE });
+    expect(dataSource.listViewOverrides).toHaveBeenCalledWith('crm_lead');
+    expect(getView).not.toHaveBeenCalled();
+  });
+
+  it('an EMPTY successful batch is authoritative — it still returns early', async () => {
+    const getView = vi.fn(async () => ({ density: 'comfortable' }));
+    const dataSource = { listViewOverrides: vi.fn(async () => ({})), getView };
+
+    const map = await loadViewOverrides(dataSource, 'crm_lead', IDS);
+
+    // "No overrides exist" is a real answer. Re-probing per view here would
+    // reinstate the 404 flurry the batch call exists to remove.
+    expect(map).toEqual({});
+    expect(getView).not.toHaveBeenCalled();
+  });
+
+  it('keeps only the ids the caller asked about', async () => {
+    const dataSource = {
+      listViewOverrides: vi.fn(async () => ({
+        'crm_lead.all': OVERRIDE,
+        'crm_lead.retired': { name: 'crm_lead.retired' },
+      })),
+      getView: vi.fn(),
+    };
+
+    expect(Object.keys(await loadViewOverrides(dataSource, 'crm_lead', IDS))).toEqual(['crm_lead.all']);
+  });
+});
+
+describe('loadViewOverrides — the fallback must stay reachable (#3774)', () => {
+  it('falls through to per-view getView when the batch read REJECTS', async () => {
+    const dataSource = {
+      listViewOverrides: vi.fn(async () => { throw new Error('503 metadata store unavailable'); }),
+      getView: vi.fn(async (_o: string, id: string) =>
+        id === 'crm_lead.all' ? OVERRIDE : null),
+    };
+
+    const map = await loadViewOverrides(dataSource, 'crm_lead', IDS);
+
+    // The point of the whole issue: a failed batch read recovers, it does not
+    // silently render as "the user never customized anything".
+    expect(map).toEqual({ 'crm_lead.all': OVERRIDE });
+    expect(dataSource.getView).toHaveBeenCalledTimes(IDS.length);
+  });
+
+  it('falls through when the batch read resolves to a non-object', async () => {
+    const dataSource = {
+      listViewOverrides: vi.fn(async () => null),
+      getView: vi.fn(async () => OVERRIDE),
+    };
+
+    expect(await loadViewOverrides(dataSource, 'crm_lead', IDS)).toEqual({
+      'crm_lead.all': OVERRIDE,
+      'crm_lead.pipeline': OVERRIDE,
+    });
+  });
+
+  it('survives a per-view read failing on its own', async () => {
+    const dataSource = {
+      getView: vi.fn(async (_o: string, id: string) => {
+        if (id === 'crm_lead.pipeline') throw new Error('404');
+        return OVERRIDE;
+      }),
+    };
+
+    expect(await loadViewOverrides(dataSource, 'crm_lead', IDS)).toEqual({ 'crm_lead.all': OVERRIDE });
+  });
+});
+
+describe('loadViewOverrides — adapters without the batch method (#3774)', () => {
+  it('an adapter exposing only getView is unaffected', async () => {
+    const dataSource = { getView: vi.fn(async () => OVERRIDE) };
+
+    expect(await loadViewOverrides(dataSource, 'crm_lead', IDS)).toEqual({
+      'crm_lead.all': OVERRIDE,
+      'crm_lead.pipeline': OVERRIDE,
+    });
+    expect(dataSource.getView).toHaveBeenCalledTimes(IDS.length);
+  });
+
+  it('an adapter with neither method yields an empty map instead of throwing', async () => {
+    expect(await loadViewOverrides({}, 'crm_lead', IDS)).toEqual({});
+    expect(await loadViewOverrides(undefined, 'crm_lead', IDS)).toEqual({});
+  });
+});