Uh oh!
There was an error while loading. Please reload this page.
fix(app-shell): a stored id can no longer rename a view tab, so set-default writes again (#4211) - #4224
Merged
Merged
Conversation
…-default writes again (#4211) "Set as default" could fire no adapter write at all. The filer measured that `ObjectStackAdapter.updateView` has no early return between its read and its `saveItem`, so the cause is above it, in the switcher's identity seam. Views reach `ObjectView` through two independent reads of the same `type='view'` metadata namespace — `objectDef.listViews` (keyed by the composer's `<object>.<key>` identity) and the adapter's `listViews()` overlay rows. Everything that decides whether a view is mutable asks whether a tab's id is among the overlay keys, so the two reads must spell one view's identity identically. The overlay side stamped `id` last and was safe; the metadata side built `{ id: <key>, ...body, ...override }` — `id` FIRST — so an `id` key inside the stored body or the stored override replaced it. Both really carry one: `persistViewPatch` writes the whole tab object (its `id` included) back through `updateViewConfig`, and a duplicated view copies its source artifact's `id` verbatim. The tab then sat under an id the overlay read had never heard of, was classified as a system view, and — because the set-default / rename / delete entries render only under `!readonly` — the menu entry was ABSENT rather than present-and-inert. Hence "nothing happened" rather than "refused": the guard's toast was never reached because there was no control left to click. One spelling replaces three: `viewRowId` answers a row's identity for the producer and every reader and is idempotent across the overlay normalization; `viewEntry` stamps identity last at all three tab-building sites; and `isSavedViewId` is the single predicate behind both the tab's `readonly` flag and the handler guard, so the menu and the handler agree by construction. `buildViewTabs` / `setDefaultViewPatches` are extracted from the `views` memo and the handler so the seam and the write set are assertable without mounting the view. Reverse verification: restoring the clobberable spelling turns 7 cases red across both divergence blocks with the zero-write signature quoted (`expected [ 'crm_lead.default', 'my_view', … ]`, `isSavedViewId expected false to be true`); all 15 control and seam cases stay green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017Qqyix2QcnpUC9XeYVDzx3
The latest updates on your projects. Learn more about Vercel for GitHub. |
Contributor
✅ Console Performance Budget
📦 Bundle Size Report
Size Limits
|
Picks up the rc.6 spec bump (#4169) and the vite alias completion (#4218) so this branch is verified against the same @objectstack/spec CI resolves. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017Qqyix2QcnpUC9XeYVDzx3
Contributor
✅ Console Performance Budget
📦 Bundle Size Report
Size Limits
|
yinlianghui
marked this pull request as ready for review
August 11, 2026 05:43
Uh oh!
There was an error while loading. Please reload this page.
Contributor
✅ Console Performance Budget
📦 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 freeto 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#4211
Premise: confirmed, and the undetermined half is now settled
The card's premise held on
origin/main@b42558a4c(after #4214 and #4212 landed). The adapter measurement stands —ObjectStackAdapter.updateViewhas no early return between its read and itssaveItem, so the zero-write symptom cannot originate there — and the two candidate routes are both real. What the card left open was which route the QA run hit and why the ids diverged. Both are answered below.Which route: route 2 (the menu entry is absent)
Route 1 (
handleSetDefaultViewtoasts and returns) is unreachable through the UI, and this is decidable statically rather than needing the running app:readonlyflag and the handler'sisSavedViewguard consult the same predicate over the same array —ObjectView.tsx:2022computessaved = savedViews.find(sv => viewRowId(sv) === view.id)/isSystem = !saved, and:1003's guard asks the same question.!isReadonly:ViewTabBar.tsx:564,:667andManageViewsDialog.tsx:300,:361.So whenever the ids diverge the entry is never rendered, and the toast is never reached — there is no control left to click. That is exactly why the symptom reads as "nothing happened" rather than "refused". Route 1 survives only as the handler's own guard against a caller that ignores
readonly; it is pinned as such, not as the QA repro.Why the ids diverged: two measured shapes, one mechanism
Views reach
ObjectViewthrough two independent reads of the sametype='view'metadata namespace:objectDef.listViews(MetadataProvider.applyViewItem)objectName.viewKeyidentitylistViews()overlay rowsnameEverything that decides whether a view is mutable asks whether a tab's id is among the overlay keys, so the two must spell one view's identity identically. They had three spellings to do it with, and one of them was clobberable:
:706):sv.name || sv.idsv.id || sv._id{ id: key, ...body, ...override }—idFIRST, so anidkey inside the merged body or the stored override silently replaced it.Both of those merged bodies are stored documents that really do carry an
id:Shape A — the persisted override row.
persistViewPatch(viewDef.id, viewDef, patch)writes the whole tab object, itsidincluded, throughupdateViewConfig, which stores it undername: viewId.listViewOverridesreads that row back keyed byname,loadViewOverrideshands it to the tab list, and itsidwon. Any view the user personalised (density, sort, hidden columns, column widths, inline edit) has such a row.Shape B — a duplicated view. It copies its source artifact's
idverbatim into the view body;applyViewItemspreads that body intoobjectDef.listViews[key], so the entry carries the foreignid. ObjectView's own overlay-side comment at:709already anticipated this shape ("a duplicate'sidfield … may have been copied verbatim from the source artifact") — the metadata side had no such protection.Either way the tab landed under an id the overlay read had never heard of,
isSavedViewanswered false for a view that is saved, and the tab was presented as a system view with its mutating entries hidden.Measured before the fix, shape B produced three tabs for two views — the source, a phantom tab under the copied id, and the overlay row pushed separately because its key matched nothing.
The fix: one spelling, identity stamped last
In
packages/app-shell/src/utils/viewIdentity.ts(which already ownsdefaultListViewId):viewRowId(row)— the single answer to "what is this row's identity":name, thenid, then_id, skipping empty strings. Idempotent across the overlay normalization (viewRowId(normalized) === viewRowId(raw)), which is what lets one function serve the producer and every reader.viewEntry(id, ...bodies)— assigns the merged bodies, then stampsid. A tab id is now a property of the key the view was looked up by, never of the data.isSavedViewId(savedViews, vid)— the single predicate behind both the tab'sreadonlyflag and the handler guard, so the menu and the handler agree by construction rather than by coincidence.ObjectView.tsxroutes all nine identity sites through them, and two seams are extracted so they are assertable without mounting the view:buildViewTabs(the tab merge, with the three id-bearing steps) andsetDefaultViewPatches(the write set the handler issues).Deliberately not done: the guard was not loosened and the menu item was not force-shown. Both would hand users an action the handler still refuses. A genuine system view stays read-only with its mutating entries correctly absent — pinned as a control.
Evidence
pnpm exec vitest run packages/app-shell/ packages/plugin-view/ --maxWorkers=2New: 22 cases in
ObjectView.setDefaultViewIdentity.test.tsx, 4 inViewTabBar.setDefaultVisibility.test.tsx.Reverse verification — direction predicted before running, and it held. Restoring the clobberable spelling inside
buildViewTabs(the fix taken out with an in-worktree edit, nevergit stash) turns 7 red / 15 green, with every red inside the two divergence blocks and the zero-write signature quoted in the message:Every control case stayed green under that revert, which is the point — they pin behaviour this change must not move. The working tree was confirmed byte-identical to the pre-revert state afterwards.
Direction stated honestly for one file.
ViewTabBar.setDefaultVisibility.test.tsxis GREEN before the fix and GREEN after.readonlyhiding a mutating entry was already correct — the bug was upstream, in which views got classified as system — so it is a forward-direction control (it exists to catch a later "just force-show the menu item" shortcut), not a red-first pin, and is not claimed as one.Also run:
type-check(app-shell, plugin-view) clean;eslinton all four touched files — 0 errors, and the new files add no new warnings;check:control-bytesOK over 3913 tracked files;changeset:checkOK. Changeset:@object-ui/app-shellpatch. Noskip-changesetlabel (objectui#3724).Out of scope, filed separately
While mapping the seam, the same two-reads-one-namespace structure showed an opposite-direction defect, pre-existing and untouched here:
updateViewConfigstampsobject: objectNameon every personalisation row, andlistViews()narrows by exactly that field, so a system view's override row comes back as a saved-view row. A code-defined view the user has merely personalised therefore reads as user-created and gains rename / delete / set-default. Filed rather than fixed — deciding which rows count as saved views is a separate question from making the two reads agree on identity.Generated by Claude Code