Repository navigation
fix(app-shell): store only the patch in a view personalization overlay (#5233) - #5772
Merged
Merged
Conversation
#5233) `persistViewPatch` wrote `{ ...baseViewDef, ...patch }`, so an overlay written by a mere column drag froze the source view's effective `filter` — and its `columns`, `label`, `type`, `isDefault` — into the stored row. The display merge is `{ ...source, ...override }`, so that copy then outranked the source view forever. The write now goes through `buildPersistedViewBody`: for a system view's overlay it is the patch alone (plus `viewKind` when the active tab carries one — identity, not content); a saved view's own row is deliberately still written whole, because for it the body IS the view and `saveItem` is a whole-document PUT. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EuPCi56cnGyykygi3z9w4m
…rite shape
Adds the write-side pins beside the read-side ones: the overlay body carries the
patch and nothing the source view owns, the stored row carries no source key at
rest, a reader outside objectui (no `narrowPersonalizationOverlay`) now sees the
admin's edit, the next write strips a pre-fix fat row, and — the guard that must
SURVIVE — a saved view's own row keeps its definition.
Also tense-corrects the comments across both packages that stated
`{ ...baseViewDef, ...patch }` as current fact. Assertions are untouched there;
only the claim about what the write shape IS today.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EuPCi56cnGyykygi3z9w4m
Contributor
✅ Console Performance Budget
The eager closure is every chunk the entry reaches through static imports — what the browser fetches and parses before the app renders. The entry chunk on its own is a small fraction of it. 📦 Bundle Size Report
Size Limits
|
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 #5233
The write half of the maintainer's 2026-08-12 ruling (objectstack#7494, comment
5261754173), unblocked by objectstack#9933 landing in@objectstack/spec17.1.0.The read half (
narrowPersonalizationOverlay) merged earlier as PR #5272 and stays.Premise check first — this card's own history says it is step one
The 2026-08-18 dispatch measured the ruling's premise false and escalated instead
of proceeding:
ViewMetadataSchemarefused acolumnState-only patch, so writing thepatch alone would have made a column drag 422 rather than persist. The unlock sweep was
explicit that admitting
columnStateas a runtime-only overlay key and making itauthor-writable are two different spec changes, and only the first shipped — so the
first thing measured here was which one the pin gives us.
Probed against the installed
@objectstack/spec17.1.0 (resolved frompackages/app-shell, version read off the resolvedpackage.json, not the lockfile):{ object, name, viewKind, columnState }{ … sort }/{ … rowHeight }/{ … hiddenFields }/{ … inlineEdit }alonecolumnStates(typo neighbour)viewbody: no recognizedviewkey"columnstate(typo neighbour)zzzNotAKeycolumnState: { widths: { a: 'wide' } }.strip()edcolumnStatewith noobject/ noviewKindViewItemcarryingcolumnStateThe refusals are the control the accepts need: the same probe still says no, so the
five accepts are a reading and not a schema that stopped discriminating. The admission
is the runtime-only one the ruling asked for, not authorability — the spec's own
text says so:
columnStateis declared onflattenedViewOverlayFields()(the overlayvalidation face) and
viewItemWireFields(), and the authoring door carries a rejectionmessage for it by name ("Column order/widths are per-user runtime personalization …
Remove it from authored metadata"). Premise holds. Proceeding was correct.
Instrument limits: this probes the schema
saveMetaItemvalidates against, in-process.It does not exercise a live server, so it cannot speak to identity inheritance actually
running server-side — that is why the write still sends
viewKindwhen the active tabhas one, rather than relying on inheritance.
Landing site — the triage line is inverted, re-measured on
9850c6e4eTriage says the fix lands in
packages/data-objectstack. It does not:persistViewPatchis defined atpackages/app-shell/src/views/ObjectView.tsx:738.ObjectView.tsx:766-771—dataSource.updateViewConfig(objectName, viewIdLocal, { ...baseViewDef, ...merged }, { isSavedView: targetIsSavedView }).packages/data-objectstack/src/index.ts:1337-1345holds a docblock that describesthe defect. The adapter is not the defect; it persists what it is given, and that is
correct. Only that docblock changed there — no adapter behaviour.
What "store only the patch" means here — and where it deliberately stops
mergedis the debounced accumulation of the third argument of everypersistViewPatchcall. All seven call sites pass a single-key object literal, one owned key each
(
rowHeight,sort,hiddenFields,columnState,inlineEdit) — somergedisthe patch, and it is written verbatim rather than filtered through
VIEW_OVERLAY_OWNED_KEYS: a filter here would silently drop a sixth key someone laterpersists, which is the drift the existing
PERSIST_CALLSratchet exists to make loud.The line between owned and copied is not redrawn here — it was drawn on the read
side by PR #5272 and this write now produces exactly the shape that reader keeps:
owned keys plus
VIEW_OVERLAY_IDENTITY_KEYS.object,nameand the overlay markerare stamped by
updateViewConfigitself;viewKindis carried from the active tab whenit has one (identity, not content) and omitted otherwise.
labelis not kept — theplatform inherits it from the shadowed registry entry, so a stored one is a snapshot of
the source view's label at write time, which is the class of frozen key the whole card
is about. The ruling's own examples of the frozen copy —
filter,columns,label,type,isDefault— are all gone.The branch, and why narrowing both sides would be the worse error
updateViewConfigpersists throughclient.meta.saveItem, which is aPUTof thewhole document. The adapter's own
updateViewreads the current row and merges ontoit precisely because
saveItemdoes not — its 404 path says so in as many words("there is nothing to merge onto. Fail loudly instead of emitting the partial write").
So the body handed to
updateViewConfigis what the row becomes.isSavedView: false) — the row is an overlay laid over a definitionthat lives elsewhere. Nothing in it but the patch is an opinion the user expressed.
Narrowed. This is the card.
isSavedView: true) — the row is the view. There is no sourceunderneath it for a copied key to shadow, so the ruled harm cannot arise; a patch-only
PUT would not narrow this row, it would delete the user's view definition. Left
byte-identical to the pre-fix write.
That is not an argument, it is measured — see ablation C below, which quotes the
destroyed
config. Considered and rejected: routing the saved-view branch throughupdateView(read-modify-write) would narrow it honestly, but it changes which adaptermethod the toolbar uses, doubles the requests, throws where the current path upserts,
and is outside both this ruling and this fence.
Existing rows: both dispositions, neither silent
Tolerate on read (PR #5272) still holds — and because this write replaces the whole
document, the next toolbar toggle also strips a pre-fix fat row at rest. So the
issue's "strip on next write" arrives as a consequence rather than as a second
mechanism. No migration. Both are pinned.
The demonstration
Same scenario twice through the real adapter write, the real batch read and the
real consumer merge — a user drags a column on
crm_lead.default, then an adminedits that view's filter from
status=opentostatus=qualified:Read this honestly: through objectui's own switcher the two rows already behave the
same — that is PR #5272 doing exactly the job it was merged for. What this PR changes
is the bytes at rest and, with them, every reader that does not run
narrowPersonalizationOverlay: Studio and the server-side switcher merge the storeddocument over the source view with no such pass, and for them the frozen copy still won.
That is the half the maintainer called a transition rather than an end state, and the
"laundering an unrecognized key past validation" the ruling names.
Both legs are pinned as tests, not just printed:
a reader OUTSIDE objectui — no read-side narrowing — now sees the admin edit tooruns the pre-fix row first as its owncontrol, so the post-fix assertions are a measurement rather than a tautology.
The existing guard, moved deliberately
ObjectView.overlayPatchOnly.test.tspinned the current fat-write shape. What moved,and what did not:
as "EXACTLY what
persistViewPatchsends". It now says USED TO send, and the caseis retitled
a PRE-FIX fat overlay no longer shadows the source view on read. The bodyis still produced by the real adapter write rather than dropped into the store, so
it models a row a real write path once produced. Its assertions are unchanged and still
pass — the read-side half is not weakened by an inch.
PERSIST_CALLSratchet over everypersistViewPatch(…)call site.The fix is inside the callee, so all seven call sites are byte-identical and the ratchet
reads exactly as before.
updateViewConfigcall site must route its bodythrough
buildPersistedViewBody(and must not contain...baseViewDef. Ablation Bbelow is why: without it, reverting the wiring alone leaves every behavioural test
green.
Sibling assumptions checked, as the dispatch asked:
ObjectView.overlayFilterRecovery.test.tsx:150andObjectView.defaultViewIdentity.test.tsx:238— neither becomes false.defaultViewIdentity's docblock is about the persistence key (name: viewId), whichis unchanged, so it is left alone.
overlayFilterRecovery's comment claimed the copy inthe present tense and is tense-corrected; its legacy-triple case still routes past the
narrowing (no marker, no
viewKind) and still passes.Reverse verification — three ablations, predictions recorded before running
Each: commit first, mutate, confirm on disk by anchored counts in both directions
(the deleted anchor and an injected
ABLATION_xmarker), run, restore undertrap … EXIT INT TERM, then confirm the restore in both directions too. No rebuild isinvolved or possible to get wrong here: the root
vitest.config.mtsaliases@object-ui/data-objectstacktopackages/data-objectstack/src, andObjectView.tsxis imported from source — the ablations turning red is the proof of that resolution.
{ ...baseViewDef, ...patch }expected undefined to deeply equal { type: 'kanban', …(1) }Leg B is the one worth reading twice: it is the gap a behavioural-only test suite cannot
see, and the reason the second ratchet exists. Leg C is the control that must survive
legs A and B and must die here — it is what says the narrowing stopped where it
should, and its failure text is the user's deleted view.
Verification
At
9a5fce746(the final commit; the union below was run on this tree).pnpm --workspace-concurrency=2 --filter '@object-ui/app-shell^...' build→ exit 0vitest run packages/app-shell/ packages/data-objectstack/Test Files 543 passed (543)·Tests 5475 passed | 1 skipped (5476)Test Files 342 passed (342)·Tests 3496 passed | 1 skipped (3497), 0 failuresTest Files 1 passed (1)·Tests 17 passed (17)type-check(both packages)packages/data-objectstack type-check$ tsc --noEmit … Done,packages/app-shell type-check$ tsc --noEmit && tsc -p tsconfig.test.json … Done— so neither was a zero-match silent passcheck:control-bytes✅ check-control-bytes: OK (scanned 4803 tracked text file(s); skipped 85 binary)check:phantom-deps✅ Every in-scope import is declared by the package that publishes it.check:self-import✅ No package names itself inside its own src/.check:esm-specifiersSpecifier leg: no un-ledgered package emits an extensionless relative specifier.check:spec-symbols✅ spec symbol derivation: 1298 files scanned against 4966 spec export namescheck-changeset-no-major.mjscheck-changeset-presence.mjs✅ 9 source file(s) of 2 released package(s) changed, and this change declares 1 changeset(s)check-lint-coverage.mjs✅ lint coverage: 46/46 packages linted, 0 with outstanding errors (0 total)persistViewPatchhitsLint was narrowed, and the narrowing is declared with its evidence.
pnpm lintisturbo run lint, i.e. each package's owneslint .. Run for the two packages thatcontain changed files: 935 files / 0 errors / 2560 warnings (app-shell) and 48 files
/ 0 errors / 368 warnings (data-objectstack), plus
lint:root→ 0 errors. (1) Thepopulation is eslint's own —
eslint .under each package resolving the rooteslint.config.jsand its ignores, not a file list I chose; (2) the counts are read off--format json; (3) the narrowing excludes nothing, because type-aware linting is notenabled — the config extends
tseslint.configs.recommended, notrecommendedTypeChecked,and
languageOptionsdeclares noproject/projectService, so no file outside those twopackages can have its verdict moved by this diff. Warnings are pre-existing
no-explicit-anyin anany-typed file; eslint exits 0 (no--max-warningsanywhere),and
+2of them are the new seam's own loosely-typed record signature, matching thesurrounding file.
Build Docsis red onmain(#5668) and this diff touches neitherapps/site/norcontent/, so a ~10-second skip there is expected and is not a reading that the docssite builds.
Fence
Written:
packages/app-shell/src/views/ObjectView.tsx· fourpackages/app-shell/src/views/ObjectView.*.test.*·packages/data-objectstack/src/index.ts(the
:1337-1345docblock only) · fourpackages/data-objectstack/src/*.test.ts(comment tense only) ·
.changeset/*. Nothing underapps/console/.env.*,examples/console-starter/**orpackages/auth/**.narrowPersonalizationOverlay's owndocblock was left alone on purpose: its "strip-on-next-write heals a row only when its
user touches that view again" is still a true statement about that disposition, and it is
outside the granted docblock.
Generated by Claude Code