diff --git a/.changeset/view-override-masquerade.md b/.changeset/view-override-masquerade.md new file mode 100644 index 0000000000..78fe722a60 --- /dev/null +++ b/.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. diff --git a/packages/app-shell/src/views/ObjectView.overrideMasquerade.test.ts b/packages/app-shell/src/views/ObjectView.overrideMasquerade.test.ts new file mode 100644 index 0000000000..111aa7da17 --- /dev/null +++ b/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); + }); +}); diff --git a/packages/app-shell/src/views/ObjectView.tsx b/packages/app-shell/src/views/ObjectView.tsx index 9d26054a16..d4e58a55ca 100644 --- a/packages/app-shell/src/views/ObjectView.tsx +++ b/packages/app-shell/src/views/ObjectView.tsx @@ -449,9 +449,11 @@ export function buildViewTabs({ }): Array & { 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>>({}); const persistPending = useRef>>({}); + // `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([]); const persistViewPatch = useCallback( (viewIdLocal: string, baseViewDef: Record, patch: Record) => { 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([]); + 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 diff --git a/packages/data-objectstack/src/index.ts b/packages/data-objectstack/src/index.ts index d154c65c2a..4b86b3f960 100644 --- a/packages/data-objectstack/src/index.ts +++ b/packages/data-objectstack/src/index.ts @@ -1033,6 +1033,104 @@ export function viewItemObjectName(item: any): string | undefined { return spec?.data?.object ?? spec?.object ?? spec?.objectName; } +/** + * The explicit discriminant {@link ObjectStackAdapter.updateViewConfig} stamps + * on the rows it writes for a **system**-view target, and + * {@link ObjectStackAdapter.listViews} excludes on read (objectui#4227). + * + * `updateViewConfig` has exactly ONE production caller — `ObjectView`'s + * `persistViewPatch`, invoked only for the toolbar-driven density / sort / + * hiddenFields / columnState / inlineEdit toggle. That single call site is + * NOT itself the explicit "create/save a view" path (that goes through + * {@link ObjectStackAdapter.createView} or the ADR-0034 metadata seam, + * `viewEnvelope` in app-shell) — but it fires for a toggle on EITHER kind of + * active tab, system or already-saved, so "every row this method writes is a + * personalization overlay" is only true for the system-view case. A toggle + * on a genuinely saved view targets that view's own row (same `(type='view', + * name=viewId)` key its create path used), and stamping the marker there + * would make {@link ObjectStackAdapter.listViews} exclude the user's own + * view on the next read (objectui#4227 follow-up, PM review on PR #4713) — + * so `updateViewConfig`'s `opts.isSavedView` withholds the marker for that + * case. The caller passes it from the same `isSavedViewId` classification + * the switcher's readonly gate already computes, rather than this layer + * re-deriving it from the write's shape. + * + * Survives the round trip against a real server: the platform's `view` + * metadata schema `.strip()`s its flattened-overlay members only for + * VALIDATION (an unrecognised top-level key does not fail the parse), and + * `saveMetaItem` persists the AUTHORED body verbatim — never the stripped + * `parsed.data` — specifically so "Studio-only auxiliary fields" (its own + * words for `isPinned`/`isDefault`/`sortOrder`) ride along on the stored + * document. This marker rides through the same door. + * + * The value itself is org-wide, not per-user (objectstack#7494's ruling: the + * overlay this row belongs to is shared view SETTINGS, not a personal + * preference) — the marker's job is only to say WHAT KIND of row this is + * (a settings overlay, not an independently addressable view), never WHO it + * applies to. + */ +const VIEW_OVERLAY_MARKER = '_isOverride' as const; + +/** + * Best-effort classification of a `view` row {@link ObjectStackAdapter.listViews} + * reads back from BEFORE {@link VIEW_OVERLAY_MARKER} existed (objectui#4227) — + * a legacy personalization row written by an older `updateViewConfig` carries + * no discriminant at all. + * + * Measured against the actual write paths, not guessed: + * + * - A genuine saved view is always created with a NESTED `config` — the + * ViewItem-record shape `{name, object, viewKind, config}` (app-shell's + * `viewEnvelope`, and this adapter's own {@link ObjectStackAdapter.createView} + * `fullSpec`). `viewKind` lives OUTSIDE `config` on that shape. + * - A personalization overlay (`updateViewConfig`) is always FLAT — its + * fields sit at the top level, never wrapped in `config`. + * + * `viewKind` on a FLAT row is therefore never something objectui itself + * authors: the only way it gets there is the platform's own server-side + * identity inheritance (`viewIdentityPatch`, `@objectstack/metadata-protocol` + * #2555 / #7741), which fires ONLY when the write's `name` resolves against a + * REGISTRY-backed (i.e. system, code-defined) view. A runtime-created saved + * view has no registry entry to inherit from, so its row — even flattened by + * a later toolbar toggle — never gains a `viewKind`. So "flat body + a + * `viewKind`" is a reliable signature of "override on a system view", while a + * flat row with NO `viewKind` is left alone — exactly the shape the existing + * legacy-bare-spec pin relies on staying a saved view (`listViews.test.ts` — + * "keeps legacy bare specs without a viewKind (saved/list views)"). + * + * Deliberately does NOT try to catch every legacy override: a row the + * CURRENT `persistViewPatch` writes (pre-marker) also copies the system + * view's full body — `type`/`columns`/`data` — into the override, and *that* + * shape is structurally indistinguishable from an untouched saved view's own + * body without this `viewKind` signal or the new marker above. Those rows + * self-heal on their NEXT write (which carries the marker); until then this + * predicate is a best-effort net over the realistic current-state case, not a + * guarantee for every possible legacy row. See the PR description for the + * measured readings this was decided against. + */ +function isLegacyOverlayRow(item: any, spec: any): boolean { + // A ViewItem record (nested `config`) is never an overlay row, regardless + // of what else it carries. + if (spec && spec.config && typeof spec.config === 'object') return false; + const viewKind = item?.viewKind ?? spec?.viewKind; + // 'form' rows are already dropped upstream by the FORM_FAMILY filter before + // this runs; a bare 'list' here is what a system-view override looks like. + return viewKind === 'list'; +} + +/** + * Whether a `view` row {@link ObjectStackAdapter.listViews} enumerated is a + * personalization overlay rather than a saved view — the marker (new writes) + * or the best-effort legacy shape (pre-marker writes). Both layers are + * needed: excluding only the marker would leave every row written before + * this fix still masquerading as a saved view (objectui#4227). + */ +function isPersonalizationOverlayRow(item: any, spec: any): boolean { + if (item?.[VIEW_OVERLAY_MARKER] === true) return true; + if (spec?.[VIEW_OVERLAY_MARKER] === true) return true; + return isLegacyOverlayRow(item, spec); +} + /** * Unwrap a `?state=draft` view read into its bare body, or `null` when there * is nothing pending (#4139). @@ -3199,34 +3297,75 @@ export class ObjectStackAdapter implements DataSource { } /** - * Persist a view definition for an object. + * Persist a toolbar-driven view config patch — density, column widths, + * sort, hidden columns, inline edit. Symmetric counterpart to + * {@link getView}: writes the row to the server metadata store via + * `client.meta.saveItem`, then invalidates the matching cache entry so the + * next {@link getView} reflects the new payload. Returns the persisted item + * when the server echoes it, otherwise undefined. + * + * Called from exactly ONE production site — `ObjectView`'s + * `persistViewPatch`, for the toolbar toggle — but that ONE call site + * fires for BOTH kinds of active tab: a code-defined **system** view (no + * row of its own yet) and a genuinely user-created **saved** view (already + * has a row — the toggle is editing ITS OWN definition, not laying an + * overlay on top of it). Which one a given call means is NOT re-derived + * here from the write's shape (objectui#4227's own lesson: shape inference + * on this namespace is exactly what let a system view masquerade as + * saved) — the caller already knows, via the same `isSavedViewId` + * classification that gates the switcher's readonly flag and its five + * mutating handlers, and passes it as {@link opts.isSavedView}. * - * Symmetric counterpart to {@link getView}: writes the view to the - * server metadata store via `client.meta.saveItem`, then invalidates - * the matching cache entry so the next {@link getView} reflects the - * new payload. Returns the persisted item when the server echoes it, - * otherwise undefined. + * - `isSavedView` false/omitted (system-view target, the common case and + * the default for backward compatibility): stamps + * {@link VIEW_OVERLAY_MARKER} so {@link listViews} excludes the row — + * the original objectui#4227 fix. + * - `isSavedView` true: the marker is withheld. Stamping it here would + * flag the saved view's OWN row as a personalization overlay, and + * `listViews()` would exclude it on the very next read — the user's own + * view would vanish from the switcher the moment they toggled its + * density (objectui#4227 follow-up, PM review on PR #4713, measured: + * `persistViewPatch` has no gate on which kind of tab is active, and + * this method writes to the exact same `(type='view', name=viewId)` key + * {@link createView}/the ADR-0034 `viewEnvelope` seam already used for + * that view, so the write is an upsert onto the saved view's row, not a + * new one). * - * Used by ObjectView for "live" toolbar persistence (density, - * column widths, sort, etc.) and by the View Config Panel for - * explicit saves. + * Per objectstack#7494's ruling, the overlay this writes (system-view + * case) is ORG-WIDE shared view settings, not a per-user preference — a + * true per-user scope is a parked v18 direction on the platform side. * * @param objectName - Object name (e.g. 'lead') * @param viewId - View identifier (e.g. 'all_leads') * @param config - Full view definition to persist + * @param opts.isSavedView - Whether `viewId` already names a saved view + * (vs. a system view being personalized for the first time). Omit / + * `false` for the default overlay-marking behavior. */ async updateViewConfig( objectName: string, viewId: string, - config: Record + config: Record, + opts?: { isSavedView?: boolean } ): Promise | void> { await this.connect(); // ADR-0005 metadata customization overlay: persist views under // `type='view'` (NOT `type=` — that was a pre-overlay // misuse that hit `/api/v1/meta//`, which the // server never wired). The view's `data.object` field is what - // associates it back to the object on read. - const merged = { ...(config || {}), object: (config as any)?.object || objectName, name: viewId }; + // associates it back to the object on read. `VIEW_OVERLAY_MARKER` is + // stamped LAST, alongside `object`/`name`, so nothing in `config` can + // shadow it (objectui#4227) — this is what lets `listViews()` exclude + // the row instead of `savedViews.find` matching it and presenting a + // system view as user-created/mutable. Withheld entirely when the + // caller says this write targets a saved view's own row (see the + // doc comment above). + const merged = { + ...(config || {}), + object: (config as any)?.object || objectName, + name: viewId, + ...(opts?.isSavedView ? {} : { [VIEW_OVERLAY_MARKER]: true }), + }; const result: any = await this.client.meta.saveItem( 'view', viewId, @@ -3289,7 +3428,15 @@ export class ObjectStackAdapter implements DataSource { const spec = v.list ?? v; if (viewItemObjectName(v) !== objectName) return false; const viewKind = v.viewKind ?? spec?.viewKind; - return !(viewKind && FORM_FAMILY.has(viewKind)); + if (viewKind && FORM_FAMILY.has(viewKind)) return false; + // Personalization overlays (density/sort/hiddenFields/columnState/ + // inlineEdit — written by `updateViewConfig`) are NOT saved views: + // returning one here is what let a system view's override row read + // back as user-created and gain Rename/Delete/Set-default/Pin + // (objectui#4227). Marked rows and the best-effort legacy shape are + // both excluded — see {@link isPersonalizationOverlayRow}. + if (isPersonalizationOverlayRow(v, spec)) return false; + return true; }).map((v: any) => { const spec = v.list ?? v; // Preserve the draft provenance flag so the switcher can badge an diff --git a/packages/data-objectstack/src/listViews.test.ts b/packages/data-objectstack/src/listViews.test.ts index f78e5c3d9a..9b97776a6a 100644 --- a/packages/data-objectstack/src/listViews.test.ts +++ b/packages/data-objectstack/src/listViews.test.ts @@ -103,6 +103,68 @@ describe('ObjectStackDataSource.listViews', () => { expect(views.map((v: any) => v.name).sort()).toEqual(['saved_grid', 'wrapped_grid']); }); + // ── objectui#4227 — a personalization override must never read back as a + // saved view (which is what let a system view gain Rename/Delete/ + // Set-default/Pin just because someone toggled its density). ────────── + describe('excludes personalization overlays (objectui#4227)', () => { + it('excludes a row carrying the explicit write-side marker', async () => { + // Exactly what `updateViewConfig` writes today: the marker plus a full + // copy of the system view's body (`persistViewPatch` spreads the whole + // active tab into the write). + const override = { + name: 'crm_lead.default', object: 'crm_lead', type: 'grid', + data: { provider: 'object', object: 'crm_lead' }, columns: ['name'], + rowHeight: 40, _isOverride: true, + }; + const ds = makeDS([override]); + const views = await ds.listViews('crm_lead'); + expect(views).toEqual([]); + }); + + it('excludes a legacy (unmarked) override whose viewKind was backfilled server-side', async () => { + // #7741/#2555: the platform inherits `viewKind`/`object`/`label` from + // the REGISTRY baseline for a personalization PUT against a real + // system view — so a pre-marker row targeting `crm_lead.default` + // still carries `viewKind: 'list'` even though objectui never sent it. + const legacyOverride = { + name: 'crm_lead.default', object: 'crm_lead', viewKind: 'list', + label: 'All', type: 'grid', rowHeight: 40, + }; + const ds = makeDS([legacyOverride]); + const views = await ds.listViews('crm_lead'); + expect(views).toEqual([]); + }); + + it('a marked row is excluded even without the legacy viewKind signal', async () => { + const minimal = { name: 'crm_lead.default', object: 'crm_lead', rowHeight: 40, _isOverride: true }; + const ds = makeDS([minimal]); + expect(await ds.listViews('crm_lead')).toEqual([]); + }); + + it('does NOT exclude a genuine ViewItem-record saved view, even one named like a system view', async () => { + // The nested `config` wrapper is only ever produced by an explicit + // create/save path (createView / the ADR-0034 seam) — never by + // `updateViewConfig`, marker or no marker. + const savedRecord = { + name: 'crm_lead.default', object: 'crm_lead', viewKind: 'list', label: 'My Rebuild', + config: { type: 'grid', data: { provider: 'object', object: 'crm_lead' }, columns: ['name'] }, + }; + const ds = makeDS([savedRecord]); + const views = await ds.listViews('crm_lead'); + expect(views.map((v: any) => v.name)).toEqual(['crm_lead.default']); + }); + + it('does NOT exclude a flat legacy saved view with no viewKind at all', async () => { + // Same fixture family as "keeps legacy bare specs without a viewKind" + // above, restated here to pin the boundary this predicate must respect: + // no `viewKind` ⇒ never treated as an override, regardless of flatness. + const flatSaved = { name: 'crm_lead.my_view', object: 'crm_lead', type: 'grid', columns: ['name'] }; + const ds = makeDS([flatSaved]); + const views = await ds.listViews('crm_lead'); + expect(views.map((v: any) => v.name)).toEqual(['crm_lead.my_view']); + }); + }); + // ── #2767 draft-preview branch ─────────────────────────────────────────── describe('previewDrafts (#2767 P2/P3)', () => { it('reads the draft-overlaid list in a SINGLE preview=draft request', async () => { diff --git a/packages/data-objectstack/src/viewOverlayMarker.test.ts b/packages/data-objectstack/src/viewOverlayMarker.test.ts new file mode 100644 index 0000000000..cc756e5756 --- /dev/null +++ b/packages/data-objectstack/src/viewOverlayMarker.test.ts @@ -0,0 +1,249 @@ +/** + * 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 — a system view's personalization row must never read back + * from `listViews()` as a saved view. Toggling density/sort/hiddenFields/ + * columnState/inlineEdit on a code-defined view stamped `type='view'` / + * `name=` rows that `listViews()` could not tell apart from a + * genuine user-created view, so the switcher tab lost its readonly lock and + * gained Rename/Delete/Set-default/Pin against a view that lives in code. + * + * The fix has two layers, both pinned here end-to-end against a fake + * `sys_metadata` store (round-trip, not a hand-written read fixture — the + * write path is exactly where a shape mismatch would hide): + * + * 1. `updateViewConfig` — the ONLY production writer of personalization + * rows — stamps an explicit `_isOverride` marker on every row it saves. + * 2. `listViews()` excludes any row carrying that marker, AND (for rows + * already persisted before this fix shipped) a best-effort legacy + * shape: flat body + a `viewKind` the platform can only have backfilled + * from a REGISTRY baseline (objectstack#2555 / #7741) — which a + * genuine runtime-created saved view never has. + * + * `listViewOverrides` (the batch personalization reader `ObjectView` merges + * for DISPLAY) is a separate, unchanged consumer of the same rows — it is + * supposed to see overlay rows, so this fix must not touch it. + */ + +import { describe, it, expect, vi } from 'vitest'; +import { ObjectStackAdapter } from './index'; + +/** A stub metadata store keyed the way `sys_metadata` is: `type` + `name`. */ +function makeMetaStore() { + const rows = new Map(); + const meta = { + getItems: vi.fn(async (type: string) => { + const items = [...rows.entries()] + .filter(([k]) => k.startsWith(`${type}::`)) + .map(([, v]) => v); + return { type, items }; + }), + getItem: vi.fn(async (type: string, name: string) => { + const item = rows.get(`${type}::${name}`); + if (!item) { + const err: any = new Error(`Not found: ${type}/${name}`); + err.status = 404; + throw err; + } + return { type, name, item }; + }), + saveItem: vi.fn(async (type: string, name: string, item: any) => { + rows.set(`${type}::${name}`, { ...item }); + return { success: true, item: rows.get(`${type}::${name}`) }; + }), + }; + return { meta, rows }; +} + +function makeDS(meta: 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 }; + return ds; +} + +describe('updateViewConfig stamps the overlay marker (objectui#4227)', () => { + it('every saved item carries `_isOverride: true`', async () => { + const { meta } = makeMetaStore(); + const ds = makeDS(meta); + + await ds.updateViewConfig('crm_lead', 'crm_lead.default', { rowHeight: 40 }); + + expect(meta.saveItem).toHaveBeenCalledWith( + 'view', + 'crm_lead.default', + expect.objectContaining({ _isOverride: true }), + ); + }); + + it('a caller-supplied `_isOverride` cannot un-mark the row — stamped last', async () => { + const { meta } = makeMetaStore(); + const ds = makeDS(meta); + + // `persistViewPatch` spreads the whole active tab into the write; nothing + // in that spread should be able to defeat the marker. + await ds.updateViewConfig('crm_lead', 'crm_lead.default', { _isOverride: false, rowHeight: 40 } as any); + + expect(meta.saveItem).toHaveBeenCalledWith( + 'view', + 'crm_lead.default', + expect.objectContaining({ _isOverride: true }), + ); + }); + + it('createView does NOT stamp the marker — it is a genuine saved view', async () => { + const { meta } = makeMetaStore(); + const ds = makeDS(meta); + + await ds.createView('crm_lead', { name: 'crm_lead.custom', label: 'Custom', type: 'grid' }); + + const [, , saved] = meta.saveItem.mock.calls[0]!; + expect(saved._isOverride).toBeUndefined(); + }); + + it('opts.isSavedView withholds the marker — a toggle on a SAVED view must not flag its own row (objectui#4227 follow-up, PR #4713)', async () => { + const { meta } = makeMetaStore(); + const ds = makeDS(meta); + + // Exactly what `persistViewPatch` sends for a toggle on the ACTIVE tab + // when that tab is already a saved view — `isSavedView: true`. + await ds.updateViewConfig('crm_lead', 'crm_lead.my_pipeline', { + type: 'kanban', label: 'My Pipeline', rowHeight: 40, + }, { isSavedView: true }); + + expect(meta.saveItem).toHaveBeenCalledWith( + 'view', + 'crm_lead.my_pipeline', + expect.not.objectContaining({ _isOverride: true }), + ); + }); +}); + +describe('end-to-end: a toolbar toggle on a system view never masquerades as saved (objectui#4227)', () => { + it('round-trips through the real write + read: excluded from listViews, still present in listViewOverrides', async () => { + const { meta } = makeMetaStore(); + const ds = makeDS(meta); + + // The exact user action the issue reports: density toggle on a + // code-defined view, going through the real adapter write path. + await ds.updateViewConfig('crm_lead', 'crm_lead.default', { + type: 'grid', + data: { provider: 'object', object: 'crm_lead' }, + columns: ['name', 'status'], + rowHeight: 40, + }); + + // `listViews()` — feeds `savedViews` / the switcher's mutability gate — + // must NOT return the override as a saved view. + const savedViews = await ds.listViews('crm_lead'); + expect(savedViews).toEqual([]); + + // `listViewOverrides()` — feeds the DISPLAY merge (viewOverrides) — is a + // different, legitimate reader of the SAME row and must still see it. + const overrides = await ds.listViewOverrides('crm_lead'); + expect(overrides['crm_lead.default']).toMatchObject({ rowHeight: 40 }); + }); + + it('a genuinely created view survives the same round trip as fully manageable', async () => { + const { meta } = makeMetaStore(); + const ds = makeDS(meta); + + await ds.createView('crm_lead', { + name: 'crm_lead.my_pipeline', + label: 'My Pipeline', + type: 'kanban', + }); + + const savedViews = await ds.listViews('crm_lead'); + expect(savedViews.map((v: any) => v.name)).toEqual(['crm_lead.my_pipeline']); + }); + + // ── objectui#4227 follow-up (PM review on PR #4713): the danger sequence + // createView -> toggle its own toolbar -> listViews() -- that PR's own + // round-trip tests covered createView->listViews (no toggle) and a + // toggle on a SYSTEM view's id, but not a toggle on a SAVED view's own + // id, which is exactly what a user does immediately after creating a + // view and adjusting its density. ────────────────────────────────────── + describe('a toggle on a SAVED view must not make it vanish (objectui#4227 follow-up)', () => { + /** The real production shape a runtime "Add View" produces (app-shell's + * `viewEnvelope`, via the ADR-0034 seam) -- nested `config`, `viewKind` + * OUTSIDE it. `ObjectStackAdapter.createView`'s own flat `fullSpec` is + * NOT this shape and is not the path the console's UI actually uses + * (`viewEnvelope` + `persistRuntimeMetadata` write through a separate + * `MetadataClient`, bypassing this adapter's `createView` entirely) -- + * using the real shape here is what makes this a genuine regression + * pin rather than one that only exercises the adapter's own writer. + */ + const savedViewRow = { + name: 'crm_lead.my_pipeline', + object: 'crm_lead', + viewKind: 'list', + label: 'My Pipeline', + config: { + type: 'kanban', + data: { provider: 'object', object: 'crm_lead' }, + columns: ['name', 'status'], + }, + }; + + it('createView -> toggle density on THAT SAME view -> listViews() still returns it (the fix)', async () => { + const { meta, rows } = makeMetaStore(); + rows.set('view::crm_lead.my_pipeline', savedViewRow); + const ds = makeDS(meta); + + // Sanity: the saved view is visible before any toggle. + expect((await ds.listViews('crm_lead')).map((v: any) => v.name)) + .toEqual(['crm_lead.my_pipeline']); + + // `persistViewPatch` spreads the ACTIVE TAB (the flattened saved view + // `listViews()` just returned) plus the toggle's patch, and — with + // this fix — passes `isSavedView: true` because `isSavedViewId` + // already found this id in `savedViews`. + await ds.updateViewConfig('crm_lead', 'crm_lead.my_pipeline', { + type: 'kanban', + label: 'My Pipeline', + columns: ['name', 'status'], + rowHeight: 40, // the toggle itself + }, { isSavedView: true }); + + const afterToggle = await ds.listViews('crm_lead'); + expect(afterToggle.map((v: any) => v.name)).toEqual(['crm_lead.my_pipeline']); + // The tab stays fully manageable: `isSavedViewId`/`viewRowId` match on + // `name`, which the write preserved. + expect(afterToggle[0]).toMatchObject({ name: 'crm_lead.my_pipeline', rowHeight: 40 }); + }); + + it('the same sequence WITHOUT the fix (isSavedView omitted) is the regression this pins against', async () => { + const { meta, rows } = makeMetaStore(); + rows.set('view::crm_lead.my_pipeline', savedViewRow); + const ds = makeDS(meta); + + // Same toggle, but as `updateViewConfig` behaved before this + // follow-up (no `isSavedView` signal) — demonstrates why the signal + // is load-bearing, not decorative. + await ds.updateViewConfig('crm_lead', 'crm_lead.my_pipeline', { + type: 'kanban', + label: 'My Pipeline', + columns: ['name', 'status'], + rowHeight: 40, + }); + + // The user's own saved view has vanished from the switcher. + expect(await ds.listViews('crm_lead')).toEqual([]); + }); + }); +}); diff --git a/packages/types/src/data.ts b/packages/types/src/data.ts index 51dc22b56b..0227f95b46 100644 --- a/packages/types/src/data.ts +++ b/packages/types/src/data.ts @@ -545,9 +545,23 @@ export interface DataSource { * @param objectName - Object name * @param viewId - View identifier (e.g., 'all', 'pipeline') * @param config - The full view configuration to persist + * @param opts.isSavedView - Whether `viewId` already names a saved + * (user-created) view rather than a code-defined one being personalized + * for the first time (objectui#4227 follow-up). A caller that tracks + * which views are saved — the same classification that gates rename/ + * delete/pin/set-default affordances — SHOULD pass this so an + * implementation that distinguishes overlay rows from saved-view rows + * (to keep the two from masquerading as each other) does not mistake + * an edit to a saved view's own row for a new overlay on top of it. + * Omit when the distinction does not apply to a given implementation. * @returns Promise resolving to the persisted config (or void) */ - updateViewConfig?(objectName: string, viewId: string, config: Record): Promise | void>; + updateViewConfig?( + objectName: string, + viewId: string, + config: Record, + opts?: { isSavedView?: boolean }, + ): Promise | void>; /** * List user-created overlay views for an object (ADR-0005 metadata