Uh oh!
There was an error while loading. Please reload this page.
fix(data-objectstack,app-shell): a view overlay contributes only the keys it owns - #5272
Merged
Merged
Conversation
…keys it owns (#5233) `ObjectView.persistViewPatch` sends `{ ...baseViewDef, ...patch }`, so an overlay written by a column drag or a sort change copies the view's whole body — its effective `filter` included — into the stored row. The display merge is `{ ...source, ...override }`, so that snapshot then outranks the source view forever: an admin edits the view's filter and every user who once resized a column keeps the old one, with nothing reporting it. `narrowPersonalizationOverlay` (new export, alongside `VIEW_OVERLAY_OWNED_KEYS`) reduces a personalization overlay row to the five keys the toolbar actually writes, and `sanitizeViewOverride` applies it at the one seam both of `loadViewOverrides`' read branches pass through — so rows already stored heal on the next read, with no migration and nothing rewritten at rest. Saved views' own bodies are untouched, classified by the same predicate `listViews()` excludes overlay rows by. Ruled by the maintainer 2026-08-12 on objectstack#7494 (comment 5261754173). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RV6yuVCxymHYE16PL9vQkE
… be able to fail (#5233) Found by the ablation leg that makes `narrowPersonalizationOverlay` inert: the fixture carried only identity plus one owned key, so its key set was already the narrowed one and the assertion held against a narrowing that never ran. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RV6yuVCxymHYE16PL9vQkE
Contributor
✅ Console Performance Budget
📦 Bundle Size Report
Size Limits
|
os-support-ai
marked this pull request as ready for review
August 18, 2026 23:36
Uh oh!
There was an error while loading. Please reload this page.
This was referenced Aug 18, 2026
Merged
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Part of #5233
Not
Fixes— deliberately. This lands the half that is entirely ours (the read), andescalates the half the ruling names (the write at rest) with the measurement that blocks
it. Details in "What does not ship" below; the issue should stay open on that decision.
The defect
persistViewPatchsends{ ...baseViewDef, ...patch }, so an overlay written by a merecolumn drag copies the view's current effective
filter— and itscolumns,label,type,isDefault— into the stored row. The display merge is{ ...source, ...override }(
buildViewTabs/viewEntry), so that snapshot then outranks the source viewindefinitely: an admin edits the view's filter and everyone who once resized a column keeps
the old filter, with nothing anywhere reporting it.
Ruled by the maintainer on 2026-08-12 (objectstack#7494, comment
5261754173):Premise re-verified on
main— one correctionThe card attributes
persistViewPatchtopackages/data-objectstack/src/index.ts. Oncurrent
mainthe function — and the{ ...baseViewDef, ...patch }spread — live inpackages/app-shell/src/views/ObjectView.tsx(theuseCallbackaround line 698, the writearound line 727). The adapter's
updateViewConfigis what persists that body verbatim.The defect itself reproduces exactly as described; only the file attribution was off. The
declared file surface for this claim is amended accordingly (comment on the issue), and it
is why this PR touches app-shell as well as the adapter.
What ships: an overlay contributes only the keys it owns
@object-ui/data-objectstackgains two exports, because it is the package that owns theoverlay row's shape (it stamps the
_isOverridemarker thatlistViews()excludes rows by):VIEW_OVERLAY_OWNED_KEYS—rowHeight,sort,hiddenFields,columnState,inlineEdit: one perpersistViewPatchcall site, read off the tree.narrowPersonalizationOverlay(row)— reduces a personalization overlay row toidentity plus those five. A saved view's own body is returned by reference, untouched;
classification is the existing
isPersonalizationOverlayRow, so a row cannot be anoverlay for one reader and a saved view for another.
sanitizeViewOverride(app-shell) applies it. That is the one seam both branches ofloadViewOverridespass through — already pinned as such byObjectView.overlayFilterRecovery.test.tsx— and it is where a row is read as a patch.Deliberately not applied inside
listViewOverrides/getView: both answer with thestored document, their equality is itself pinned ("same key space, same document",
listViewOverrides.test.ts), andInterfaceListPagehydrates a hollow view out of it.Narrowing the document read would have broken that pin and that hydration.
labelis dropped from the overlay on purpose, even though the platform stamps it there:viewIdentityPatch(@objectstack/metadata-protocol, #2555) inheritsviewKind/object/labelfrom the registry entry an overlay shadows, so a storedlabelis a snapshot of thesource view's label — content, not identity. Same for
isDefault, which the tab carries andthe fat write therefore froze.
The existing-rows decision: tolerate on read — explicitly, and pinned
Rows already stored carry the frozen body. Of the three dispositions the card names:
and leaves the frozen filter authoritative until then. It also cannot ship on its own
right now (see below).
sys_metadatarows an operator maynot know exist, and buys nothing the read-side narrowing does not already deliver.
page load: no migration, no rewrite at rest, no user action.
The card forbids silent tolerance, not tolerance. This one is documented at the function,
declared here, and pinned by tests that say which decision they encode
(
ObjectView.overlayPatchOnly.test.ts, "rows written BEFORE this fix (the disposition,pinned)"), including the pre-marker legacy shape.
What does not ship, and why: the write at rest
The ruling's own words are "store the patch only". Measured against the platform's live
contract before writing any of it, that shape is refused for one of the five keys:
Measured with
@objectstack/spec17.0.0 as installed here;columnStateappears nowhere inthe framework's
packages/spec/srceither.sort,rowHeight,hiddenFieldsandinlineEditall parse fine on their own —columnStatedoes not, because it is objectui'sown non-author runtime key (
plugin-grid/src/__tests__/gridNonAuthorKeys.test.tsx).saveMetaItemvalidates everyviewwrite (afternormalizeViewMetadatainherits identityfrom the registry baseline), so a patch-only write for a column resize or reorder — the
most frequent toolbar toggle, and one that travels alone — would come back 422
INVALID_METADATAand the user's resize would not persist at all. Today it survives onlybecause the copied base rides along and supplies a recognized key.
So the storage-shape half needs a decision that is not this card's to take:
viewschema the runtime overlay keys, then narrow the write(contract-first: the refusal is the producer's, and the row is a real, already-persisted
shape it declines to describe). Costs a framework change and a release.
columnStatepersistence until (1) lands. Cheapest towrite, but it removes a working feature.
reaches a user, but the stale bytes stay at rest and any reader outside objectui (Studio,
server-side switcher) still sees a snapshot.
viewmetadata namespace entirely — the parked per-userdirection on objectstack#7494 / Implement the objectstack#7494 ruling: view config is explicitly org-wide — console wording/UX + permission-gated write path #5232.
Recommendation: (1) with (3) shipped now as the harm-elimination half. (2) trades a silent
staleness for a visible regression; (4) is parked and much larger.
Tests
packages/app-shell/src/views/ObjectView.overlayPatchOnly.test.ts— the defect pin, roundtrip through the real adapter write + read and the real consumer merge
(
loadViewOverrides→buildViewTabs): a column drag writes the overlay, the admin thenedits the source view's filter and columns, and the tab shows the new ones while keeping
the
columnState/sort/hiddenFields/rowHeight/inlineEditthe overlay was writtenfor. Plus the disposition pins, the two "must NOT be narrowed" controls, and a ratchet
that reads
ObjectView.tsxand fails ifpersistViewPatchever writes a key the overlayis not allowed to own (the drift that would otherwise silently drop a user's setting).
packages/data-objectstack/src/viewOverlayPatchOnly.test.ts— the policy itself, and thedocument/patch boundary this PR deliberately keeps.
Reverse verification (predicted before running)
Two legs, both against committed code. No rebuild needed and none can hide a stale result:
the root
vitest.config.mtsaliases@object-ui/data-objectstacktopackages/data-objectstack/src, so tests resolve source, notdist— and the ablationsturning red is itself the proof of that resolution.
sanitizeViewOverridestatus: openwherequalifiedwas expectednarrowPersonalizationOverlaymade inertThe miss is reported rather than smoothed over, and it found a real weakness: "omits an
owned key the row does not carry" used a fixture of identity plus one owned key, whose key
set is already the narrowed one — so it held against a narrowing that never ran. The
fixture now carries a base key (second commit) and the leg re-run gives 4 red / 4 green.
Gates
Run at
670380763, the tree this PR points at:pnpm exec vitest run packages/data-objectstack/— 41 files, 556 tests, all pass.pnpm exec vitest run packages/app-shell/— 447 files, 4313 pass, 1 skipped (run atf0b6bf59a; the only later change is five lines inside one data-objectstack test file,re-run green, so no app-shell input moved).
pnpm --filter @object-ui/data-objectstack --filter @object-ui/app-shell type-checkandlint— pass (app-shell'stype-checkcovers the test tsconfig; both script names echoedin the output, so neither was a zero-match silent pass).
pnpm check:control-bytes,check:phantom-deps(this PR adds a cross-package import),check:self-import,check:spec-symbols,check:esm-specifiers, pluscheck-changeset-no-major.mjsandcheck-changeset-fixed.mjs— all pass.Changeset:
patchfor@object-ui/data-objectstackand@object-ui/app-shell.Generated by Claude Code
Generated by Claude Code