fix(app-shell,data-objectstack): system view override rows no longer masquerade as saved views - #4713
Merged
Merged
Conversation
…masquerade as saved views A system (code-defined) view's personalization overlay row (density/sort/ hidden columns/column widths/inline-edit, written by `updateViewConfig`) was returned by `listViews()` indistinguishably from a genuine saved view. `ObjectView`'s `isSystem = !saved` check then flipped to false, and the tab gained Rename/Delete/Set-default/Pin against a view that lives in code (objectui#4227). Write side: `updateViewConfig` stamps an explicit `_isOverride: true` discriminant on every row it saves — it has exactly one production caller, the toolbar-driven toggle, so every row it writes is an overlay. Read side: `listViews()` excludes any row carrying that marker, and (for rows persisted before this fix) a best-effort legacy shape — a flat body with a `viewKind` the platform can only have backfilled from a registry (code-defined) baseline, which a genuine runtime-created saved view never has. `listViewOverrides()` is unchanged. Per objectstack#7494's ruling the overlay this stores is org-wide shared view settings, not a per-user preference; prose describing it as "personal" is corrected. Write-side permission gating and true per-user scope are platform-side (out of scope here, per the paired ruling). Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RnQd8iMMUwXQEV1crFmQiQ
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Contributor
✅ Console Performance Budget
📦 Bundle Size Report
Size Limits
|
… view's own row as an overlay PM review on PR #4713 identified an unmeasured truth-table row: `updateViewConfig` has exactly one call site (`ObjectView`'s toolbar-driven toggle), but that site fires for a toggle on EITHER a system view OR an already-saved view, and stamped the `_isOverride` marker unconditionally. A toggle on a saved view's own toolbar writes to that view's own `(type='view', name=viewId)` row -- the same key its create path used -- so the unconditional marker flagged the user's own saved view as an overlay, and `listViews()` then excluded it on the very next read: the user's own view vanished from the switcher the moment they adjusted its density. `updateViewConfig` gains an optional `opts.isSavedView` (also added to the `DataSource` interface in @object-ui/types); the marker is withheld when true. `ObjectView`'s `persistViewPatch` passes it from the same `isSavedViewId` classification its readonly gate and five mutating handlers already use -- mirrored into a ref (`savedViewsRef`) since the callback's `useCallback` deps are evaluated before the `savedViews` state exists further down the component, same pattern as the existing `identityPolicyRef`. New regression coverage in viewOverlayMarker.test.ts pins both directions: the fixed sequence (createView -> toggle on that same saved view -> listViews() still returns it) and, side by side, the same sequence without the flag (demonstrating the view disappearing) so the assertion is not vacuous. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RnQd8iMMUwXQEV1crFmQiQ
Contributor
✅ Console Performance Budget
📦 Bundle Size Report
Size Limits
|
Collaborator
Author
|
Review verdict (patch round): ACCEPT (reviewer of record, PM session The review's danger sequence was confirmed real with line-level evidence and fixed the right way: the caller's existing Will flip ready + enable auto-merge once Lint, Type Check, and Test shards 1–4 conclude Generated by Claude Code |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #4227
What was happening
Toggling any per-view personalization on a code-defined (system) view — density, sort, hidden columns, column widths, inline edit — persists a row under the same
type='view'metadata namespace a genuinely saved view lives in, keyed by the same id (ObjectStackAdapter.updateViewConfig).listViews()returned that row indistinguishably from a real saved view, soObjectView'ssavedViews.find(sv => viewRowId(sv) === view.id)matched it,isSystem = !savedflipped tofalse, and the tab gained Rename / Delete / Set-default / Pin / Edit-view-config against a view that lives in code —handleDeleteViewwould calldataSource.deleteViewon it.PR #4224 (#4211) pinned "a genuine system view stays readonly" as a control, but its fixture's
savedViewswas always[]— it never exercised an override row, so it passed straight through this bug.The fix (per the 2026-08-11 maintainer ruling, paired with objectstack#7494)
Two layers, both needed:
updateViewConfig(the only production writer of personalization overlays —ObjectView's toolbar-driven toggle is its one caller) now stamps an explicit_isOverride: truediscriminant on every row it saves, last in the spread so nothing in the merged config can shadow it.listViews()excludes any row carrying that marker, and additionally excludes a best-effort legacy shape for rows written before this fix shipped: a flat body (no nestedconfig) carrying aviewKind. Measured against the real write paths: a genuine saved view is always created with a nestedconfigwrapper (createView/ the ADR-0034viewEnvelopeseam), withviewKindliving outsideconfig; a personalization overlay is always flat. On a flat row,viewKindis therefore never something objectui itself authored — it can only be the platform's server-side identity inheritance (viewIdentityPatch) firing because the write'snameresolved against a registry-backed (system) view, which a runtime-created saved view has no registry entry to trigger. So "flat body + aviewKind" reliably signals "override on a system view"; a flat row with noviewKindis left alone (this is the existing "legacy bare spec" pin's territory and stays a saved view).This is a best-effort net for the realistic current-state case, not a guarantee for every conceivable legacy row — a pre-marker override that also copies the system view's full body (
persistViewPatchspreads the whole active tab into the write) is structurally indistinguishable from an untouched saved view without theviewKindsignal, and isn't caught. Those rows self-heal on their next write, which now carries the marker.Because
isSystem/readonlyderive fromsavedViews.find(...), which is populated straight fromlistViews(), excluding the override row there is what makes the existingisSystem = !savedgate (and the same-predicate guard on all five mutating handlers) correct again — no change to that predicate itself was needed.listViewOverrides()— the readerObjectViewuses to merge these settings back into the live view for display — is unchanged; it's a different, legitimate consumer of the same rows.Follow-up fix (same PR, post-review — PM review found an unmeasured truth-table row)
persistViewPatch(ObjectView's debounced toolbar-toggle writer, the ONE call site ofupdateViewConfig) has no gate on which kind of tab is active — it fires identically for a system view AND an already-saved view. A toggle on a saved view's own toolbar therefore writes to that view's own(type='view', name=viewId)row (the same key its create path used), and the first commit's unconditional marker stamp flagged that row as an overlay too —listViews()then excluded it on the very next read: the user's own saved view vanished from the switcher the moment they adjusted its density.Fix:
updateViewConfiggains an optionalopts.isSavedView(added to theDataSourceinterface in@object-ui/typestoo, for other implementations of the contract); when true the marker is withheld.persistViewPatchpasses it from the exact sameisSavedViewIdclassification the switcher's readonly gate and its five mutating handlers already use — not a new shape-inference, reusing the existing authoritative signal. (The five mutating handlers — rename/delete/pin/set-default/config-save — were already unaffected: they all route throughdataSource.updateView, the pre-existing non-destructive read-merge-write, never throughupdateViewConfig.)Implementation note:
persistViewPatchis defined before thesavedViewsstate exists further down the component, so referencing it directly in theuseCallbackdependency array would read it in its temporal dead zone. Mirrored into asavedViewsRefinstead (same pattern as the file's existingidentityPolicyRef), read only from inside the callback body.New regression coverage in
viewOverlayMarker.test.tspins both directions against the real production shape (viewEnvelope's nested-configenvelope, not the adapter's own flatcreateView, which the console's UI does not actually call): the fixed sequence (create → toggle on that same saved view →listViews()still returns it, fully manageable) and, side by side, the same sequence with the flag omitted (demonstrating the view disappearing), so the assertion is not vacuous.Scope boundaries (per the paired ruling)
Prose sweep
Console/plugin prose describing this overlay as a "personal preference" was corrected to say it's org-wide shared:
packages/app-shell/src/views/ObjectView.tsx—buildViewTabs's override-wins-per-key comment, the density/sort/etc. hydration comment, and (added for consistency with those two) theviewOverridesstate comment a few hundred lines earlier, which still said "toggle preferences".Swept but found unrelated (left untouched, named here so the search doesn't need repeating):
packages/react/src/hooks/useViewSharing.tsandpackages/plugin-view/src/ViewTabBar.tsx'sprivate/team/organization/publicViewVisibility— a distinct, legitimate feature (who can see a saved view), not the personalization-overlay contract this issue is about.packages/plugin-view/src/ManageViewsDialog.tsx's "per-user personalisation (objectui#1520)" note references a different, unrelated future-work issue. Checked all ten locale packs (packages/i18n/src/locales/*.ts) aroundsystemViewReadonlyand the density/sort/column-visibility toolbar strings — no "personal"/individual-scope framing found there.Tests
packages/data-objectstack/src/listViews.test.ts— 5 cases: excludes a marked row, excludes a legacy unmarked row (flat +viewKind), excludes a minimal marked row with no legacy signal, does not exclude a genuine nested-configViewItem record (even system-view-named), does not exclude a flat legacy saved view with noviewKindat all (pins the existing "legacy bare spec" boundary).packages/data-objectstack/src/viewOverlayMarker.test.ts— write-side: everyupdateViewConfigsave carries the marker by default; a caller-supplied_isOverride: falsecannot un-mark the row;createViewdoes not stamp it;opts.isSavedView: truewithholds it. End-to-end round trips against a fakesys_metadatastore: (a) a toolbar toggle on a system view is excluded fromlistViews()but still present inlistViewOverrides(); (b) a genuinely created view survives the same round trip fully manageable; (c) new — create → toggle on that SAME saved view (viewEnvelope-shaped, nestedconfig) →listViews()still returns it; (d) new, side by side — the same sequence with the flag omitted shows the view vanishing, pinning the regression this follow-up closes.packages/app-shell/src/views/ObjectView.overrideMasquerade.test.ts— runs the real production pipeline (ObjectStackAdapter.listViews()→buildViewTabs/isSavedViewId): a system view with a marked override row, and with a legacy unmarked override row, both stayreadonly=truewith the mutating-handler guard refusing; a positive control shows a genuinely created saved view (even reusing a system-view-shaped label) stays fully manageable; PR fix(app-shell): a storedidcan no longer rename a view tab, so set-default writes again (#4211) #4224's own no-override-row control still holds.Verification
Local, at
6ac6427ec(this PR's current head):CI is not yet observed to converge — reporting at draft-PR time per this repo's dispatch contract; will be re-checked on any follow-up.
Generated by Claude Code