fix(app-shell,data-objectstack): system view override rows no longer masquerade as saved views

.changeset/view-override-masquerade.md

@@ -0,0 +1,20 @@
+---
+'@object-ui/app-shell': patch
+'@object-ui/data-objectstack': patch
+'@object-ui/types': patch
+---
+
+A system (code-defined) view's personalization overlay row no longer masquerades as a user-created saved view.
+
+Toggling density / sort / hidden columns / column widths / inline-edit on a code-defined view persists a row under the same `type='view'` metadata namespace a genuinely saved view lives in, keyed by the same id (`ObjectStackAdapter.updateViewConfig`). `listViews()` previously returned that row indistinguishably from a real saved view, so `ObjectView`'s `isSystem = !saved` check flipped to `false` and the tab gained Rename / Delete / Set-default / Pin against a view that lives in code — `handleDeleteView` would even call `dataSource.deleteView` on it.
+
+Two layers now keep the two kinds of rows apart:
+
+- **Write side**: `updateViewConfig` — the only production writer of personalization overlays — stamps an explicit `_isOverride: true` discriminant on every row it saves, UNLESS the write targets an already-saved view's own row (see below).
+- **Read side**: `listViews()` excludes any row carrying that marker, and (for rows already persisted before this fix shipped) a best-effort legacy shape: a flat body with a `viewKind` the platform can only have server-side-backfilled from a registry (code-defined) baseline — a genuine runtime-created saved view never has one.
+
+`listViewOverrides()` (the reader `ObjectView` uses to merge these settings back into the live view for display) is unchanged — it is supposed to keep seeing overlay rows.
+
+The overlay this stores is **org-wide shared view settings**, not a per-user preference (a true per-user scope is a parked platform-side v18 direction) — comments describing it as "personal" have been corrected to say so.
+
+**Follow-up fix (same card, post-review):** `updateViewConfig`'s ONE call site (`ObjectView`'s toolbar-driven toggle) fires for a toggle on EITHER a system view OR an already-saved view — a saved view whose own toolbar the user toggles writes to that same view's own row. Stamping the overlay marker unconditionally there would flag the user's own saved view as an overlay and make `listViews()` exclude it on the very next read, i.e. the saved view would vanish from the switcher the moment its density was adjusted. `updateViewConfig` gains an optional `opts.isSavedView` parameter (also added to the `DataSource` interface in `@object-ui/types`); `ObjectView` passes it from the same `isSavedViewId` classification its readonly gate and mutating handlers already use, and the marker is withheld when it's true.

packages/app-shell/src/views/ObjectView.overrideMasquerade.test.ts

@@ -0,0 +1,157 @@
+/**
+ * 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#4227 — personalizing a system (code-defined) view must never make
+ * it look user-created.
+ *
+ * Toggling density / sort / hidden columns / column widths / inline edit on a
+ * system view persists a row under the SAME `type='view'` namespace a saved
+ * view lives in, keyed by the SAME id. Before this fix, `ObjectStackAdapter
+ * .listViews()` returned that row indistinguishably from a real saved view,
+ * so `ObjectView`'s `savedViews.find(sv => viewRowId(sv) === view.id)` matched
+ * it, `isSystem`/`readonly` flipped to `false`, and the tab gained Rename /
+ * Delete / Set-default / Pin against a view that lives in code —
+ * `handleDeleteView` would call `dataSource.deleteView` on it.
+ *
+ * PR #4224 (objectui#4211) pinned "a genuine system view stays readonly" as a
+ * control (`ObjectView.setDefaultViewIdentity.test.tsx`), but that fixture's
+ * `savedViews` was always `[]` — it never exercised an override row, so it
+ * passed straight through the bug this issue reports. This file is the
+ * companion fixture that DOES include one, run through the REAL production
+ * pipeline: the adapter's `listViews()` (the actual fix) feeding
+ * `buildViewTabs` / `isSavedViewId` (the actual consumers), exactly as
+ * `ObjectView`'s own effect normalizes them (mirrored from
+ * ObjectView.tsx:967-978).
+ */
+
+import { describe, it, expect, vi } from 'vitest';
+import { ObjectStackAdapter } from '@object-ui/data-objectstack';
+import { buildViewTabs } from './ObjectView';
+import { viewRowId, isSavedViewId } from '../utils/viewIdentity';
+
+const OBJECT_NAME = 'crm_lead';
+
+const DEFINED_VIEWS = {
+ 'crm_lead.default': { name: 'crm_lead.default', label: 'All Leads', type: 'grid' },
+};
+
+const fallbackTab = () => ({ id: 'all', label: 'All records', type: 'grid', columns: [] });
+
+/** `ObjectView.tsx`'s own `savedViews` normalization (ObjectView.tsx:967-978), verbatim. */
+function normalizeSavedViews(rows: any[]) {
+ return rows.map((sv: any) => ({
+ ...sv,
+ id: viewRowId(sv),
+ objectName: sv.objectName || sv.object || OBJECT_NAME,
+ }));
+}
+
+function makeAdapterWithItems(items: any[]) {
+ const ds: any = new ObjectStackAdapter({
+ baseUrl: 'http://test.local',
+ fetch: vi.fn(async () =>
+ new Response(JSON.stringify({ success: true, data: { capabilities: {}, routes: {} } }), {
+ status: 200,
+ headers: { 'Content-Type': 'application/json' },
+ })),
+ });
+ ds.connected = true;
+ ds.connectionState = 'connected';
+ ds.client = { meta: { getItems: vi.fn(async () => ({ items })) } };
+ return ds;
+}
+
+/** The five mutating handlers all short-circuit through this one guard (ObjectView.tsx). */
+const isMutable = (savedViews: any[], vid: string) => isSavedViewId(savedViews, vid);
+const isReadonlyTab = (savedViews: any[], vid: string) => !isMutable(savedViews, vid);
+
+describe('a system view stays readonly even with a personalization row (objectui#4227)', () => {
+ it.each([
+ [
+ 'marked row (new writes, post-fix)',
+ {
+ name: 'crm_lead.default', object: 'crm_lead', type: 'grid',
+ data: { provider: 'object', object: 'crm_lead' }, columns: ['name'],
+ rowHeight: 40, _isOverride: true,
+ },
+ ],
+ [
+ 'legacy unmarked row (viewKind backfilled server-side, pre-marker writes)',
+ {
+ name: 'crm_lead.default', object: 'crm_lead', viewKind: 'list',
+ label: 'All Leads', type: 'grid', rowHeight: 40,
+ },
+ ],
+ ])('%s: excluded from savedViews, tab stays readonly, guard refuses', async (_label, overrideRow) => {
+ const ds = makeAdapterWithItems([overrideRow]);
+
+ // The real fix: `listViews()` must not hand this row back as a saved view.
+ const rawSavedViews = await ds.listViews(OBJECT_NAME);
+ expect(rawSavedViews).toEqual([]);
+
+ const savedViews = normalizeSavedViews(rawSavedViews);
+ const tabs = buildViewTabs({
+ definedViews: DEFINED_VIEWS,
+ primary: undefined,
+ primaryId: undefined,
+ savedViews,
+ viewOverrides: {},
+ fallbackTab,
+ });
+
+ expect(tabs.map((t) => t.id)).toEqual(['crm_lead.default']);
+ // Render-time gate (ObjectView.tsx: `isSystem = !saved`, `readonly: isSystem`).
+ expect(isReadonlyTab(savedViews, 'crm_lead.default')).toBe(true);
+ // The exact predicate all five mutating handlers (rename/delete/pin/
+ // set-default/config) short-circuit on.
+ expect(isMutable(savedViews, 'crm_lead.default')).toBe(false);
+ });
+
+ it('positive control: a genuinely created saved view stays fully manageable, even reusing a system-view-shaped label', async () => {
+ // A real save (createView / the ADR-0034 seam) is always a nested
+ // ViewItem record — the shape `listViews()` must keep letting through.
+ const realSavedView = {
+ name: 'crm_lead.my_pipeline', object: 'crm_lead', viewKind: 'list', label: 'My Pipeline',
+ config: { type: 'kanban', data: { provider: 'object', object: 'crm_lead' }, columns: ['name'] },
+ };
+ const ds = makeAdapterWithItems([realSavedView]);
+
+ const rawSavedViews = await ds.listViews(OBJECT_NAME);
+ expect(rawSavedViews.map((v: any) => v.name)).toEqual(['crm_lead.my_pipeline']);
+
+ const savedViews = normalizeSavedViews(rawSavedViews);
+ const tabs = buildViewTabs({
+ definedViews: DEFINED_VIEWS,
+ primary: undefined,
+ primaryId: undefined,
+ savedViews,
+ viewOverrides: {},
+ fallbackTab,
+ });
+
+ expect(tabs.map((t) => t.id)).toEqual(['crm_lead.default', 'crm_lead.my_pipeline']);
+ expect(isReadonlyTab(savedViews, 'crm_lead.my_pipeline')).toBe(false);
+ expect(isMutable(savedViews, 'crm_lead.my_pipeline')).toBe(true);
+ // The system view beside it is untouched.
+ expect(isReadonlyTab(savedViews, 'crm_lead.default')).toBe(true);
+ });
+
+ it("PR #4224's own control still holds: no override row at all", () => {
+ const tabs = buildViewTabs({
+ definedViews: DEFINED_VIEWS,
+ primary: undefined,
+ primaryId: undefined,
+ savedViews: [],
+ viewOverrides: {},
+ fallbackTab,
+ });
+ expect(tabs.map((t) => t.id)).toEqual(['crm_lead.default']);
+ expect(isReadonlyTab([], 'crm_lead.default')).toBe(true);
+ });
+});

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

@@ -449,9 +449,11 @@ export function buildViewTabs({
 }): Array<Record<string, any> & { id: string }> {
  const viewList = Object.entries(definedViews || {}).map(([key, value]: [string, any]) => {
  const override = viewOverrides[key];
- // Override wins per-key — saved overrides represent user preferences
- // (density, column widths, etc.) that should shadow the embedded
- // definition — but NOT over the tab's identity.
+ // Override wins per-key — saved overrides represent org-wide shared
+ // view settings (density, column widths, etc. — objectstack#7494's
+ // ruling: this store has no per-user scope, so "personal" is not an
+ // accurate description of what it persists) that should shadow the
+ // embedded definition — but NOT over the tab's identity.
  return viewEntry(key, value, override, {
  type: (override?.type) || value?.type || 'grid',
  });
@@ -687,6 +689,12 @@ function ObjectViewInner({ dataSource, objects, onEdit, externalRefreshKey }: an
  // network write.
  const persistTimers = useRef<Record<string, ReturnType<typeof setTimeout>>>({});
  const persistPending = useRef<Record<string, Record<string, any>>>({});
+ // `persistViewPatch` is defined (and its `useCallback` deps evaluated)
+ // BEFORE `savedViews` state exists below — closing over it directly in
+ // the dependency array would read it in its temporal dead zone. Mirror
+ // it into a ref on every render instead (same pattern as
+ // `identityPolicyRef` above), read only from inside the callback body.
+ const savedViewsRef = useRef<any[]>([]);
  const persistViewPatch = useCallback(
  (viewIdLocal: string, baseViewDef: Record<string, any>, patch: Record<string, any>) => {
  if (!dataSource?.updateViewConfig || !objectName || !viewIdLocal) return;
@@ -700,11 +708,26 @@ function ObjectViewInner({ dataSource, objects, onEdit, externalRefreshKey }: an
  const merged = persistPending.current[viewIdLocal] || {};
  delete persistPending.current[viewIdLocal];
  delete persistTimers.current[viewIdLocal];
+ // A toolbar toggle fires through this ONE path for BOTH a
+ // system view (no overlay row yet) and a genuinely saved
+ // view — `baseViewDef` is the active tab either way, and
+ // nothing upstream of here branches on which. Tell the
+ // adapter which case this is via the SAME classification the
+ // switcher's own readonly gate and its five mutating
+ // handlers already use (`isSavedViewId`), rather than
+ // leaving it to (re-)infer from the write's shape: writing
+ // this as an unconditional personalization-overlay marker
+ // (objectui#4227) would flag the saved view's OWN row and
+ // make `listViews()` exclude it on the very next read — the
+ // user's own view would vanish from the switcher after they
+ // merely toggled its density (objectui#4227 follow-up,
+ // PM review on PR #4713).
+ const targetIsSavedView = isSavedViewId(savedViewsRef.current, viewIdLocal);
  Promise.resolve(
  dataSource.updateViewConfig(objectName, viewIdLocal, {
  ...baseViewDef,
  ...merged,
- })
+ }, { isSavedView: targetIsSavedView })
  ).catch((err: any) => {
  console.error('[ObjectView] Failed to persist view config:', err);
  });
@@ -949,6 +972,7 @@ function ObjectViewInner({ dataSource, objects, onEdit, externalRefreshKey }: an
  // into `views` so the ViewTabBar renders them alongside metadata-defined
  // listViews.
  const [savedViews, setSavedViews] = useState<any[]>([]);
+ savedViewsRef.current = savedViews;
  useEffect(() => {
  let cancelled = false;
  if (!objectName) {
@@ -992,8 +1016,10 @@ function ObjectViewInner({ dataSource, objects, onEdit, externalRefreshKey }: an
 
  // Persisted per-view config overrides (e.g. density toggle). Saved
  // separately from `objectDef.listViews` (the embedded definition) via
- // `dataSource.updateViewConfig` and read back here so toggle preferences
- // survive a hard reload. Keyed by viewId → partial view config to merge.
+ // `dataSource.updateViewConfig` and read back here so the toggle state
+ // survives a hard reload. Org-wide shared settings (objectstack#7494's
+ // ruling), not a per-user preference. Keyed by viewId → partial view
+ // config to merge.
  //
  // Reading strategy (batch first, per-view fallback) lives in the exported
  // `loadViewOverrides` above so it can be pinned directly — see its doc for
@@ -1697,10 +1723,13 @@ function ObjectViewInner({ dataSource, objects, onEdit, externalRefreshKey }: an
  // Propagate appearance/view-config properties for live preview
  rowHeight: viewDef.rowHeight ?? listSchema.rowHeight,
  densityMode: viewDef.densityMode ?? listSchema.densityMode,
- // Hydrate persisted user preferences so they survive reload
- // (Airtable-style per-view personal config). All four below go
- // through the same persistViewPatch helper which debounces and
- // batches concurrent toggles.
+ // Hydrate the persisted view settings so they survive reload
+ // (Airtable-style toolbar config — objectstack#7494's ruling:
+ // ORG-WIDE shared, not a per-user preference; a true per-user
+ // scope is a parked v18 direction, not something to fake
+ // client-side). All four below go through the same
+ // persistViewPatch helper which debounces and batches concurrent
+ // toggles.
  sort: (viewDef as any).sort ?? listSchema.sort,
  // The ONE place this view's effective filter is computed (#2890).
  // It used to be computed twice — once here as `filter` for the child