fix(app-shell): carry default and visibleWhen through the object-field options editor - #7629
Merged
Merged
Conversation
…field options editor (objectui#7540) Editing any picklist option in the metadata-admin designer wrote the option back without its `default` or `visibleWhen` key. The payload stayed valid, so nothing surfaced -- silent data loss of two keys the platform honours, one of them (`default`) ruled `enforce` on the object-field face. The loss started in the READER. `readOptions` projected each authored option to exactly `value` / `label` / `color`, so both keys were gone before `patchOptions` ever saw them and no writer-only repair could have carried them. `readOptions` now keeps the whole authored option, parking the keys the editor has no control for on an internal carrier, and `patchOptions` spreads that carrier's contents back into the written document. This mirrors `readFields` one level up, which preserves unknown keys on a field definition and strips only the named retired keys a shipped build actually wrote. There is no counterpart tombstone list for option keys and none is owed: every entry in the registry is field-level, and this editor has only ever written the three keys it displays. No authoring UI changed -- per-option controls for these keys are a separate product question. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KbJQ1y1J12nZxYzFWhP8Q3
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
|
os-sam
marked this pull request as ready for review
September 4, 2026 09:13
This was referenced Sep 4, 2026
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 #7540
Shape 1 only, as scoped by the dispatching PM on the card: the reader keeps the
whole authored option and the writer puts back what it did not display. No
per-option authoring UI was added — see "Deliberately not done".
What changed
One source file:
packages/app-shell/src/views/metadata-admin/inspectors/ObjectFieldInspector.tsx.The loss started in the reader, which is why a writer-only patch could not
have closed it.
readOptionsprojected every authored option down to exactlyvalue/label/color, sodefaultandvisibleWhenwere already gonebefore
patchOptionsever ran — whatever the writer were taught to carry, itcan only carry what the reader handed it.
All three parts move together, as the card's fence requires:
interface Option(module-local) gains arestslot for the authoredkeys the editor has no control for.
readOptionsnow keeps the whole authored option, parking everythingoutside the three displayed keys on
rest. The slot is only attached whenthere is something to carry, so an option with nothing extra produces a row
byte-identical to what this reader used to produce.
patchOptionsspreads that carrier's contents back into the writtendocument, displayed keys last so they stay authoritative for their own slots.
restis editor-internal and never becomes a document key:SelectOptionSchemais strict, so leaking it would 422 every save. The writer therefore emits a
plain document object and never spreads an
Option. A test pins the exact keyset for this.
This mirrors
readFieldsinpreviews/object-fields-io.tsone level up, whichpreserves unknown keys on a field definition and strips only the named retired
keys a shipped build actually wrote (
RETIRED_FIELD_KEYS). No counterparttombstone list for option keys is owed: all five entries in the registry
(
indexed,referenceTo,formula,isSystem,sortOrder) are field-level,and this editor has only ever written the three keys it displays.
The round trip, measured
Not asserted — these are the observed documents from the ablation run below,
same input, same edit ("edit the Label box"), reader reverted vs. repaired.
Input option:
{ value: 'alpha', label: 'Alpha', color: '#00ff00', default: true, visibleWhen: 'record.tier == 2' }{ value: 'alpha', label: 'Alpha II' }— 2 keys{ value: 'alpha', label: 'Alpha II', color: '#00ff00', default: true, visibleWhen: 'record.tier == 2' }— 5 keysAnd the key set directly, from the carrier-leak pin:
Object.keys(option).sort()[ 'label', 'value' ][ 'default', 'label', 'value' ]Re-measured on the installed
@objectstack/spec17.2.0, controls lit before anyrow was trusted (the card's table, reproduced independently here):
Two traps inherited from the card rather than re-earned: a select option's
valuemust be at least 2 characters (a 1-character value poisons every rowwith
too_small@[value]and thevisibleWhenrows then say nothing), and theboolean form of
visibleWhenis refused — it is the string form that isaccepted and canonicalized into the expression envelope.
Ablation
Restored the original
readOptionsprojection only, leaving the repairedwriter in place — the sharpest test of the card's central claim that the reader
is the loss site. Predictions written before the run; observed matched
case-for-case, 10 of 10.
Mutation proven on disk before the run: old-projection anchor count 1 (expected
1), carrier-attach anchor count 0 (expected 0), blob
31c5cf3etob490fcf0. No rebuild leg applies — the test imports./ObjectFieldInspector, the relative source, not a package dist, so vitestresolves the mutated file directly. Restore proven: blob back to
31c5cf3e(equal to HEAD's) and
git diff HEADempty.default: trueon an untouched row survivesexpected undefined to be truedefault: truesurvives on the row being edited…(2) to deeply equal …(3)visibleWhenstring form survives verbatimexpected undefined to be 'record.tier == 2'visibleWhenenvelope form survives verbatimexpected undefined to deeply equal {dialect:'cel',…}…(2) to deeply equal …(4)[ 'label', 'value' ]vs[ 'default', 'label', 'value' ]expected undefined to be 1ObjectFieldInspector.optionLabel.test.tsx(#7014 Q2, 5 cases)Totals:
Tests 7 failed | 8 passed (15)ablated,15 passed (15)repaired.The third row is the one worth reading twice. Dropping both keys still yields
a document the contract ACCEPTS — which is exactly why this bug was silent
rather than a 422, and why an "is the payload valid?" test can never detect it.
That case is deliberately kept in the suite as a non-detector.
Verification
Every run below is on
7001725f, the tree as pushed, with a clean working tree.Exit codes captured by redirecting to a file, never through a pipe.
pnpm exec vitest run …/ObjectFieldInspector.optionKeyPreservation.test.tsx …/ObjectFieldInspector.optionLabel.test.tsxTest Files 2 passed (2)·Tests 15 passed (15)pnpm exec vitest run packages/app-shell/src/views/metadata-admin/Test Files 226 passed (226)·Tests 2341 passed | 1 skipped (2342)pnpm exec vitest related --run …/ObjectFieldInspector.tsxTest Files 53 passed (53)·Tests 414 passed (414)pnpm exec turbo run type-check --concurrency=2(whole repo)Tasks: 81 successful, 81 totalpnpm exec eslintover both changed files✖ 12 problems (0 errors, 12 warnings), exit 0node scripts/check-changeset-presence.mjs✅ 1 source file(s) of 1 released package(s) changed, and this change declares 1 changeset(s)pnpm run changeset:check✅ All workspace packages are in the changeset fixed group.·✅ No changeset declares a major bump.Notes on how to read those:
here:
: anyoccurrences in this file are 1 atorigin/mainand 1 atHEAD(the
raw.map((o: any) …)the original reader already had). 0 errors.tsc --noEmit && tsc -p tsconfig.test.json;--listFileson that secondprogram contains both the new test file and the changed source (count 1 each,
measured — not inferred from the task going green).
Declared narrowing: the full
packages/app-shell/suiteThe suite is 615 test files; the 226-file metadata-admin subset alone took
8m34s on the shared box, so the whole package cannot finish inside this seat's
10-minute foreground ceiling. CI runs it in full regardless. What was run
instead is the complete transitive dependent closure, with the three things
that make a narrowing a measurement rather than a gap:
vitest relatedwalked its ownmodule graph from the changed file and collected the dependents — 53 test
files spanning exactly two directories,
metadata-adminandstudio-design. That is vitest's answer to "what can observe this file".226 files / 2341 tests for the whole containing subtree, run independently
and green.
Its three touched symbols —
interface Option,readOptions,patchOptions— are all module-local (noexport; lines 95, 125, 449). Thefile's only export is the
ObjectFieldInspectorcomponent at line 258, whosesignature is unchanged, and the whole-repo type-check (81/81) confirms no
type consumer moved. So no test outside that closure can observe this change.
Repo-wide
pnpm lintis likewise CI-owned. Its relevance here is bounded by thesame invariance:
eslint.config.jsdeclares noprojectServiceand noparserOptions.project(type-aware linting is off), so a one-file diff cannotmove the verdict on any file it does not contain.
Housekeeping
Clause-②: no. Re-judged from this diff rather than inherited:
the
ObjectFieldInspectorcomponent, and its props type(
MetadataInspectorProps) is untouched.interface Optioncarries noexportmodifier — verified on this diff, at line 95 — so the widened type isnot reachable from any other module, let alone from a published entry point.
@objectstack/specis not touched, and bothkeys were already ACCEPTED by
SelectOptionSchema17.2.0 before this PR(probe table above). The repair stops the editor dropping keys the
contract already takes; it does not ask the contract to take anything new.
One behaviour change a reviewer should see plainly rather than discover, since
it is the honest cost of "preserve what the editor does not display": an option
carrying a key the spec refuses is now carried through instead of being
silently dropped, so a save of such a document surfaces
unrecognized_keys@[options[0]]where it previously succeeded by quietlyrewriting the author's document. I judge this correct and in-contract — the
document was already illegal before the inspector opened it (the pin asserts
exactly that, so the case cannot be misread as this PR creating a 422), the
editor declines to hide a pre-existing violation rather than laundering it, and
it is the same contract
readFieldskeeps one level up. It is a behaviourchange, not a contract change: the published accept set is identical. Flagging
it because it is the one place where reasonable people could want the opposite
default; say so and I will switch to an explicit two-key allowlist.
Deliberately not done
defaultradio across the optionlist, a
CelPredicateFieldper option). Ruled out of scope by the dispatchingPM before work started: it is a genuine product design question about that
panel. Shape 1 is a strict subset of it and forecloses nothing — the reader is
the loss site, so shape 2 could only ever be reached through this change.
There is now no per-option control for either key; both simply survive.
packages/components/src/ui/**(finding(components): packages/components/src/ui/chart.tsx is an unreferenced duplicate of the plugin-charts chart primitives, carrying the legend bug fixed in objectui#7248 #7397),packages/plugin-list/**andpackages/core/src/utils/normalize-list-view.ts(ListView: fold
data={{ provider: 'object', object }}→objectNameand readspecType/typefor the view kind — step 6 of #2890, released by objectstack#14791 ruling B #7477),DashboardView.tsx(fix(dashboard): retire the dashboard-roottitleread on all five surfaces #7622) andcontent/docs/releases/are alluntouched — the diff is 3 files, listed above.
patchOptionslabel repair landed in fix(app-shell): an emptied picklist option Label stays a legal document (Q2) #7536 is left exactly as it is.Different keys, different mechanism. Its 5 pins run green in every run above,
ablated and repaired.
component's write boundary (what
onPatchpersists) and against the contract,not by driving the designer in a real browser.
contract; the gate results above are local runs only.
🤖 Generated with Claude Code
https://claude.ai/code/session_01KbJQ1y1J12nZxYzFWhP8Q3
Generated by Claude Code