Repository navigation
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
This was referenced Aug 18, 2026
Merged
This was referenced Sep 25, 2026
akarma-synetal
pushed a commit
to akarma-synetal/objectui
that referenced
this pull request
Sep 28, 2026
…arker only (objectui#10210, ruling B) (objectstack-ai#10427) Fixes objectstack-ai#10210 Clause-②: no This carries out the maintainer's ruling B on objectui#10210 (ruling comment 5824008636, 「10210 同意」). `data-objectstack` retires `isLegacyOverlayRow`, so a view row counts as a personalization overlay only when it carries the `_isOverride` marker. The ruling puts the B work on this card and asks for no separate execution card. The write half landed earlier as objectui#10332 (`Part of`). Implemented under the `domain:ui` seat 4 claim 5824191795 on branch `claude/issue-10210-retire-overlay-shape-guess`, session `https://claude.ai/code/session_01BP8CMtACxTdLjqR6rhd33C`. ## What changed - `packages/data-objectstack/src/index.ts`: `isLegacyOverlayRow` is deleted. That was the shape guess: a flat row (no nested `config`) with `viewKind: 'list'`. `isPersonalizationOverlayRow` now reads only the marker, on the item or on its `{list: …}` body. Four comments the change made false are corrected: the marker docblock, `VIEW_OVERLAY_OWNED_KEYS`, `narrowPersonalizationOverlay`, and the comment in the `listViews` filter. No export, key, schema or signature moves. - Five existing pins are rewritten with the reason, and none is deleted (counts below). Three are the ones the ruling names; the other two are the app-shell pins described under "Surface breach". - A new pin, `packages/data-objectstack/src/viewOverlayMarkerOnly-10210.test.ts`, covers the heal, the accepted exposure and a same-shape marker control. - `.changeset/10210-overlay-marker-only.md` declares a `patch` for `@object-ui/data-objectstack`: a fix to how the view list reads rows, with no API change. It says broken views recover on read, and it names the one exposed class exactly as the ruling words it. ## Mechanism assumptions, measured - **A1 holds.** On `origin/main` (378a4f6), `isPersonalizationOverlayRow` returned `isLegacyOverlayRow(item, spec)` when the marker was absent. `git grep` finds two callers of the classifier, both in `index.ts`. Here is what each did with a row the guess classified: - `listViews()` filter: dropped it from the saved-view list, so the app-shell tab was stamped `readonly` (`isSystem = !saved`). Now the row is returned as a saved view: a flat row as stored, with `_draft` kept in draft preview. - `narrowPersonalizationOverlay()`, which app-shell's `sanitizeViewOverride` calls for both `loadViewOverrides` read branches (the display merge): narrowed it to identity plus the five owned keys. Now it returns the row by reference, whole. - The write side of objectui#10332, as merged: `updateViewConfig` still stamps `_isOverride` last on a system-view target (pinned unchanged in `viewOverlayMarker.test.ts`), and the config save now writes a nested-`config` envelope. So no current writer produces an unmarked overlay. - **A2 holds.** The broken row is built the way the card measured it: the pre-objectui#10332 flat panel draft, landed through a store that inherits `viewKind`/`object`/`label` from the registry entry it shadows, as the platform's `viewIdentityPatch` does. Its keys are `{label,type,columns,name,isDefault,id,viewKind,object}`, plus `filter` because the draft edited one. `listViews()` returns it with the edited label, columns and filter, in both the published read and the `?preview=draft` read, and `narrowPersonalizationOverlay` returns it whole. Through the real app-shell pipeline (`listViews` into `buildViewTabs` into `isSavedViewId`), the rewritten `ObjectView.overrideMasquerade.test.ts` case asserts that the tab is **not** read-only, that the mutating-handler guard admits it, and that the tab shows the edited label and columns. - **A3 holds and is pinned as the accepted trade-off.** A toolbar overlay written before the marker and never touched since has the same shape. It now reads back from `listViews()` with its frozen label, columns and filter. Merged over a newer code definition, that frozen copy wins. This is asserted in the new pin, in `viewOverlayPatchOnly.test.ts`, and end to end in app-shell's `ObjectView.overlayPatchOnly.test.ts`, where the admin's filter, columns and label edit is covered by the frozen copy. - **A4 is partly falsified: the pins that asserted the guess are five, not the three named, and one of the three named asserted nothing about it.** I measured this by running every suite with only the source hunk applied. Four assertions went red: one in `listViews.test.ts`, one in `viewOverlayPatchOnly.test.ts`, one in app-shell `ObjectView.overlayPatchOnly.test.ts` and one in app-shell `ObjectView.overrideMasquerade.test.ts`. `viewOverlayMarker.test.ts` stayed 8/8 green. Only its header prose described the guess, so the header is the part rewritten there. Each rewritten file carries a comment naming this ruling. - **A5 holds.** The source hunk was reverted to the base blob while the tests stayed at HEAD. The trap restored from `HEAD` and verified the restore by blob hash and an empty `git diff HEAD`. Result: **9 red, 46 green** over the six files. All 4 rewritten assertions and 5 of the 6 new cases went red. The sixth new case is the same-shape marker control, which is green on both trees by design. The 8 cases in `viewOverlayMarker.test.ts` stayed green because nothing there was rewritten. No `dist` is involved: the data-objectstack tests import `./index`, and app-shell resolves `@object-ui/data-objectstack` to `src` through the root vitest alias. `it(` counts before and after (text count, then executed-test count from the vitest JSON report): | file | before | after | |:--|:--|:--| | `data-objectstack/src/listViews.test.ts` | 12 / 12 | 12 / 12 | | `data-objectstack/src/viewOverlayMarker.test.ts` | 8 / 8 | 8 / 8 | | `data-objectstack/src/viewOverlayPatchOnly.test.ts` | 8 / 8 | 8 / 8 | | `app-shell/src/views/ObjectView.overlayPatchOnly.test.ts` | 17 / 17 | 17 / 17 | | `app-shell/src/views/ObjectView.overrideMasquerade.test.ts` | 2 `it(` + 1 `it.each(` with 2 rows / 4 | 3 `it(` + 1 `it.each(` with 1 row / 4 | | `data-objectstack/src/viewOverlayMarkerOnly-10210.test.ts` (new) | none | 6 / 6 | In `ObjectView.overrideMasquerade.test.ts`, the unmarked row moved out of the `it.each` table into its own `it(`, because the table's shared body asserts exclusion. The executed count is unchanged. ## Surface breach: a bounded in-place fix, declared The claim's surface is `data-objectstack/src/index.ts`, the three named pins, a new pin and one changeset. The measurement above found two more pins that asserted the guess through the real adapter, both in app-shell: `ObjectView.overrideMasquerade.test.ts` (its `legacy unmarked row` case) and `ObjectView.overlayPatchOnly.test.ts` (`a PRE-MARKER legacy row is narrowed …`). Leaving them would leave this PR red in CI. I rewrote them in place, the same way as the named pins. The dispatch said "stop on breach"; the role file allows a bounded in-place fix when all four conditions hold, and it wins when the two conflict. That conflict is raised in the report, not settled silently here. The four conditions: 1. Same defect class: both pin the retired shape guess itself. 2. Mechanical, with the shape fixed by the ruling: marker-only, the same rewrite as the named pins. 3. No other holder: the file lists of all 16 open PRs were read; none touches either file or `packages/data-objectstack/`. The release PR's 1732 files were read in full, matching its `changed_files`. 4. Same gate family: app-shell vitest was already a named gate, and nothing new needs verifying. The seat needs to add these two paths to the claim surface. The same edit also repairs a stale cross-file line citation in `ObjectView.overrideMasquerade.test.ts` (it cited the `savedViews` normalization by an old line range; it now cites it by content), as AGENTS.md objectstack-ai#11 asks for any file a change touches anyway. ## Pending changesets corrected in place (`check-changeset-overwrite`, case 2: prose correction; front matter unchanged) - `.changeset/10210-view-config-save-envelope.md` (objectui#10332, still pending). Its⚠️ paragraph said that already-broken views are **not** repaired by this release and keep reading back as read-only. That becomes false once this change ships with it, so the paragraph now says such views are repaired on read, edits kept, by this `data-objectstack` change. One present-tense clause ("is the shape the list reader treats as …") is put in the past tense. - `.changeset/view-overlay-write-patch-only-5233.md` said "rows written before this land are still tolerated on read". That is now true only for rows carrying the marker, so the sentence is scoped to them and points to this change for rows older than the marker. - `.changeset/hollow-view-overlay-hydration-pin-5773.md` names `packages/data-objectstack/src/index.ts`. I re-read it, and no sentence becomes false: its fixtures are real `updateViewConfig` writes, which carry the marker, and `InterfaceListPage`'s hydration does not narrow. ## Verification, all at HEAD `9736e8d20` - **Tests.** Under the shared verify lock, `pnpm exec vitest run --maxWorkers=2` ran the whole `packages/data-objectstack/` package plus the 12 app-shell and console suites that name `listViews(`, `_isOverride`, `narrowPersonalizationOverlay`, `sanitizeViewOverride`, `loadViewOverrides` or `listViewOverrides`: **1072 passed, 0 failed** (the base had 1066 of 1066; the difference is the 6 new cases). A second run covered 5 more suites that import the adapter and name `listViews` (`metadataReadWarningToast`, core `element-data-source`, and the kanban, list and timeline `elementDataSource` suites): **66 passed**. Every suite that uses the real adapter and names an overlay reader was run: 19 of 19. - **Type-check.** `pnpm --workspace-concurrency=2 --filter '@object-ui/data-objectstack^...' run build` (exit 0), then `pnpm --filter @object-ui/data-objectstack type-check` (exit 0). Its program lists all 4 touched data-objectstack test files (`--listFilesOnly`). Next, `turbo run build --filter='@object-ui/app-shell^...' --concurrency=2`: 28 of 28 tasks succeeded. Then `pnpm --filter @object-ui/app-shell type-check` (`tsc --noEmit && tsc -p tsconfig.test.json`) exited 0, and `tsconfig.test.json` lists both edited app-shell test files. - **Gates**, all exit 0: `check-changeset-presence` ("7 source file(s) of 2 released package(s) changed, and this change declares 1 changeset(s)"), `check-changeset-no-major`, `check-changeset-fixed`, `check-changeset-overwrite` (report-only; it names the two corrections above), `check-changeset-claims` ("No pending changeset names a file this change touches"; the self-contradiction reading is clean over 3 bodies), `check-pending-changeset-literals`, `check-new-cross-file-line-citations` ("VERDICT new-cross-file-line-citations: 0 new citation(s)"), `check-control-bytes`, `check-shell-escape-residue`, `check-test-path-roots`, `check-vi-mock-specifiers`, `check-vi-mock-inherit`, `check-vi-mock-override-shape`, `check-unreferenced-sources` and `check-object-metadata-write-doors`. `check-governed-queue-guard --test` over the 10 paths says NOT GOVERNED. - **Lint, narrowed and stated as a measurement.** - Scope: the root `eslint.config.js` `**/*.{ts,tsx}` and `**/*.test.{ts,tsx}` blocks, the same config each package's `eslint .` resolves to. - Run: `eslint --format json` over the 7 touched TypeScript files gives 7 files, **0 errors**, 183 warnings, all `no-explicit-any`. - Same measurement on the base blobs through `--stdin`: `index.ts` 124 → 122, `ObjectView.overrideMasquerade.test.ts` 7 → 8, the other four unchanged, and 10 in the new pin, in line with its sibling pins. - Invariance: the config enables no type-aware linting (no `parserOptions.project` or `projectService`), so this diff cannot move the verdict for any untouched file. - **NOT MEASURED:** the full app-shell package run (roughly 760 files), which is beyond the foreground cap. CI's sharded `pnpm test` runs it. ## Acceptance notes (not filed) - Comments outside this diff that still describe the retired net in the present tense. Carrier: none. - The `buildViewConfigSaveBody` docblock in `packages/app-shell/src/views/ObjectView.tsx`: "a flat row carrying `viewKind` is exactly the shape the adapter's legacy-overlay net reads as a personalization overlay". - The same sentence in the header of `ObjectView.viewConfigSaveEnvelope-10210.test.ts`. - The `buildPersistedViewBody` docblock: "already harmless on read since PR objectstack-ai#5272 narrowed the merge" now holds only for marked rows. Each describes history accurately and is stale only in tense or scope. This is drift, not a defect. - Reasoned, not measured: a pre-marker overlay row (the exposed class) now reads as a saved view. The next toolbar toggle on that view therefore takes `persistViewPatch`'s saved-view branch (`isSavedViewId` is true), writing the row whole and without the marker. Unlike before the ruling, the row does not re-mark itself on the next touch; it stays a plain row. This follows the ruling's "reads as a plain row", and I record it only so it is not rediscovered later. --- _Generated by [Claude Code](https://claude.ai/code/session_01BP8CMtACxTdLjqR6rhd33C)_ Co-authored-by: Claude <noreply@anthropic.com>
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.
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