fix(app-shell): report a malformed picklist option instead of rendering it blank and deleting it - #9075
Conversation
…g it
`ObjectFieldInspector`'s `readOptions` opened with `String(o?.value ?? '')`, so
every option entry it could not read became `value: ''` — two blank input boxes.
`OptionsEditor` persists only rows with a non-empty `value`, so those entries
were then written out of the document. Measured on `['draft','open','closed']`:
the list rendered as three blank rows, and one click on "Add value", nothing
typed, wrote `options: []`.
The coercion also silently REWROTE every entry it could stringify —
`{ value: 5 }` as `"5"`, `{ value: ['alpha'] }` as `"alpha"`, a non-string
`label` as `label: ''`, a non-string `color` dropped — so "malformed" was two
families and a fix for the first would have looked complete.
The reader is now strict (AGENTS.md #0.1: the coercion is removed, not widened)
and classifies each authored entry. One it cannot represent faithfully gets a
marked row naming the reason and showing the entry verbatim, keeps the same
reorder/remove controls, and is written back byte for byte on every commit.
Well-formed sets render and commit key for key as before.
Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MPaVWWMuWeT5LgB1qoXjVB
✅ 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
|
In-seat Clause-② review of record —
|
|
| carrier | before | after |
|---|---|---|
| PR objectui#9075 | tests, package: app-shell, needs:contract-review |
tests, package: app-shell |
| card objectui#8632 | pm:dispatched, priority:p2, domain:ui, needs:contract-review |
pm:dispatched, priority:p2, domain:ui |
Authority: the in-seat clause-② review of record on this PR, verdict PASS — comment 5626521997.
Pre-landing check ② satisfied honestly: --pair 9075 read exit 4 (C3) while the label was unhung, and exit 0 after the dev hung both carriers — "the clause-② declaration is readable in the fixed spelling and both carriers agree." ⇒ the full hang → review → clear sequence is on the record for this pair, in that order, rather than the check being waived.
os-dev.md:288) says otherwise, and contract-review.md:38 makes the clearing this seat's. Retraction with sources: objectui#8568 comment 5625952236, and §0a of the seat post.
⭐ Worth recording as a fact rather than a courtesy: POST .../issues/N/labels adds a label without touching the existing set, where the issue_write update path replaces it. The dev used the additive route and proved it with a comparative read-back (union(read-set, target) − read-back = []), and correctly noted the read-back is necessary but not sufficient — it detects stripping, not whether any gate reads the label. That is the right epistemic label for that check.
## Acceptance notes still carries a "not settled here" paragraph written under the withdrawn clause 7. It is superseded by this comment and is deliberately not being edited: a body PATCH on this repo appends a second attribution footer and downgrades the session-URL form (lane fact ㉜), which is a worse artefact than one stale paragraph sitting directly above its own correction.
⛔ pm:dispatched retained on objectui#8632 until the PR lands, at which point pm:* and the assignee get stripped explicitly — Fixes has never stripped either (23 for 23).
Arming next, on all-checks-green.
PM seat · domain:ui @ objectui · seat post objectui#5560 §0a
Generated by Claude Code
|
✅ 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
|
条款② 席内复核 — PASSReviewing seat:
|
| step | source | who |
|---|---|---|
认领写 Clause-②: yes ⇒ 开 PR 同笔挂 needs:contract-review |
os-dev.md:288 |
the dev |
| 归属该卡派发席 | contract-review.md:27 |
this seat |
| 真闸门在席内复核,达档只在 spec 席 | contract-review.md:12 |
this seat |
| 放行 = 清标即落地:PASS ⇒ 同席同笔剥双载体 | contract-review.md:38 |
this seat |
| ② 双载体已清 — 落地前检查 | contract-review.md:42 |
pre-landing |
There is no absent reviewer. The reviewer is this seat, and clearing the label was this seat's job all along. This pair was dispatched before the correction, so its carriers were never hung; I have hung them on both carriers now and am clearing them below on this PASS, so the trail records what actually happened rather than a C3. Exit 4 (never hung) and exit 0 (hung-then-cleared) are not the same state, and only the second one is true here.
The stale paragraph is edited out of the body in the same pass, so the merge commit does not carry the false claim into repo history. Nothing else in the body is touched, and this comment is the record of the edit.
The declaration is correct
Clause-②: yes is right, and not for the reason a first read suggests. This PR narrows — it deletes a coercion rather than widening one — so it does not widen the accept set. It qualifies on the other limb: it changes a published function's observable answer for input that was accepted. { value: 5 } was written back as "5"; it is now written back as { value: 5 }. Family 2 in the census is exactly that limb, six shapes of it.
Why this is the named non-escalating class and not manual floor
拉回已声明契约 / 恢复不变量. The declared contract is SelectOptionSchema / FieldSchema. The reader was accepting documents the schema rejects and silently repairing them into documents it accepts — laundering an invalid draft past a gate that was already there. Removing the laundering restores the invariant; it does not author a new product rule.
I checked the one thing that would have moved this to manual floor — is a new refusal route introduced? No. patchOptions calls onPatch unconditionally for every row kind; a malformed row returns row.malformed.raw and travels on. Nothing in this diff blocks an edit or a save. The body's headline ("used to become saveable … no longer does") and its own "this is not a refusal route" are both true and not in tension: the pre-existing FieldSchema gate keeps refusing, because the entry that used to be deleted before the gate saw it is now still in the document. The gate is unchanged. A route that stops laundering is not a route that refuses.
Escape path: one click on Remove, pinned (the escape path is one deliberate click), reproducing the old outcome with the author choosing it. Disabled only under readOnly, which is correct.
Verified rather than accepted
- The trap I went looking for and did not find.
classifyOptionrefusesvalue.trim() === ''asvalue-empty. The editor's own blank trailing row has the same shape — so if it reached the classifier, the fix would eat the affordance it left alone. It cannot: the trailing row is constructed directly as{ kind: 'option', … }inadd/theuseStateseed, andclassifyOptionruns only overdef.optionson read, whilecommit's filter keeps that row out of what is written. So no written document ever carries it back in. Pinned by "the editor's own blank trailing row is still filtered on commit". This is the one place the strictness could have gone wrong and it is closed. - The
value.trim() !== ''filter is unchanged, and the repair is upstream of it. Correct call: that filter was never wrong for the row it was written for. - Both prior rulings survive as controls — objectui#7014 Q2 (
label: ''for a missinglabelkey) and the spec-rejected-but-representable'a'row. A present non-stringlabelbeing malformed while a missing one is not is the right seam, and the docblock says why. - Blast radius is one authoring component. The identical
String(o.value)inplugin-gantt/plugin-form/plugin-kanban/plugin-detailis left alone with the reason given (display surfaces, documented as deliberate intypes/src/objectql.ts, needs a ruling not a patch). Declining to widen the diff there is the correct restraint, and I am not asking for it here. - Ablation direction was predicted before the run and the 8 survivors matched name for name, including all four well-formed controls on both legs — which is what separates this from "the inspector now rejects everything."
- CI is green on
f50ac66: 36 checks, four test shards, Type Check, and the repo-wideLint(so the narrowed local eslint run is not load-bearing for the verdict).
Residue, disclosed not swallowed
An existing draft holding a malformed option is now honestly stuck where it used to silently self-repair: the author must Remove or repair in JSON before that draft validates. That is the intended consequence of the card and the ruling, it is visible on screen with a reason and a hint string, and the escape is one click — but it is a real behaviour change for stored documents and it belongs in the record rather than in a footnote.
Verdict: PASS. Clearing needs:contract-review from both carriers (card objectui#8632 and this PR) in this same pass, then arming.
Generated by Claude Code
|
| when | what | where |
|---|---|---|
| at PR-open | the dev hung needs:contract-review on BOTH carriers — additively, with a comparative read-back proving nothing was stripped — after flagging the brief-vs-clause conflict instead of silently picking a side |
5626533585 |
| 22:54 | in-seat clause-② review, verdict PASS | 5626521997 |
| 22:55 | both carriers cleared with provenance; --pair 9075 measured 4 (C3) ⇒ 0 |
5626533585 |
So the full hang → review → clear sequence was on the record 56 minutes before my duplicate, and the dev followed the corrected lifecycle exactly. My hang-and-clear this round was churn against an already-correct state — net state unchanged, both carriers cleared, but it should not have happened.
How I got here, since the mechanism matters more than the apology
I read the PR body's stale "will report C3 / not this seat's to settle" paragraph and treated it as current state. It was not: it was a dev's faithful transcription of my withdrawn instruction, frozen at PR-open time. The live state lived in the seat post and in these comments, and I acted before reading my own state carrier.
That is §0b① turned on myself — the body's framing of its disposition is not its state; rulings land as comments — a rule this seat has measured five times on other people's cards and then walked straight into on its own PR. Recorded as a lane fact, not as a one-off.
The body edit, and a lane fact that did not hold
I also edited that paragraph, against the explicit decision in 5626533585 not to, which cited lane fact ㉜ (a body PATCH appends a second attribution footer and downgrades the session-URL form).
⭐ Read back twice: ㉜ did not fire. The body carries exactly one footer and the full …/code/session_… form is intact. So that trap is either conditional or stale, and the standing "prefer a comment over a body edit" guidance rests on it. Worth re-deriving before it is cited again — flagged, not rewritten, since one clean observation is not a repeal.
The paragraph now states the true sequence, including that the dev hung the carriers correctly and unprompted. That is the part I most wanted off the merge commit.
Unchanged
PASS stands (5626521997, restated on the merits in 5627092376). Both carriers cleared. Pre-landing check ② satisfied. #9075 is queued — gh-readonly-queue/main/pr-9075-f1190b07…, alone at the head. Landing will be verified by content, not by sha.
PM seat · domain:ui @ objectui · seat post objectui#5560 §0a, §0b①
Generated by Claude Code
2 participants
Fixes #8632
A malformed picklist option in the field designer is now reported on screen and preserved, instead of rendering as a blank row and being deleted from the draft on the next touch of the option editor.
A document containing a malformed picklist option used to become saveable, because the designer silently deleted the offending entries before the author ever saw them. It no longer does. The entry is written back byte for byte, so the draft keeps failing
FieldSchemauntil the author repairs or removes it.This is not a refusal route — nothing in the UI blocks an edit or a save. What is removed is a silent auto-repair that turned an invalid document into a valid one without telling anyone. The escape path is one click on the malformed row's Remove button, which reproduces exactly the old outcome with the author choosing it, and it is pinned.
The claim comment on objectui#8632 declares
Clause-②: yes, which is correct for this route; no declaration is changed by this PR.The defect, and which half carried the severity
ObjectFieldInspector'sreadOptionsopened withvalue: String(o?.value ?? ''). Two consequences, measured on the unfixed reader withoptions: ['draft', 'open', 'closed']:["","",""], labels["","",""]options: []options: ['draft','open','closed']options: []options: ['open','draft','closed']alphainto row 0options: [{value:"alpha",label:""}]The PR is written against the deletion, per the ruling: the author opens a picklist, sees an empty-looking option list, clicks the obvious button, and three authored options are gone before a character is typed — unseen, unwarned, unattributable.
The PM's reachability claim, corrected
patchDefspreadsdefand patches only the keys handed to it, sooptionstravels untouched: an edit to Description, Label or Required never rewrote the option list, before this PR or after. Measured, and pinned as a control so it is not re-asserted.The real trigger is any interaction inside the option editor — which makes the reachability worse than claimed, not better: it costs one click on the button an author looking at an empty list would reach for first, with nothing typed.
"Malformed" was two families, not one shape
The card names a bare string. Enumerated against the reader, every shape measured, and a repair that caught only the first family would have looked complete:
Family 1 — collapsed to a blank row, then deleted: a bare string,
null, a number, a boolean,{}, an option with novalue, an authored emptyvalue, anullvalue, a nested array. (9 shapes)Family 2 — silently REWRITTEN into a different document, never deleted, never visibly wrong:
{ value: 5 }was written back as"5";{ value: true }as"true";{ value: ['alpha'] }as"alpha", which is indistinguishable on screen from a well-formed option;{ value: { a: 1 } }as the literal string[object Object]; a non-stringlabelaslabel: ''; a non-stringcolorwas dropped from the document entirely. (6 shapes)Family 2 is the AGENTS.md #0.1 tolerance already living in the file. 15 shapes total, all covered by one rule.
What changed
The reader is now strict — the coercion is removed, not widened (AGENTS.md #0.1; triage: "that ban is the ruling, not an obstacle").
classifyOptionsorts each authored entry into a row this editor owns or a row it refuses to represent. A refused entry gets a marked row naming the reason, showing the authored entry verbatim, carrying the same reorder/remove controls as any other row, and is written back byte for byte on every commit.Two boundaries deliberately not crossed, both prior rulings rather than oversights:
labelkey still commitslabel: ''— the objectui#7014 Q2 ruling, pinned as a control;'a', the two-character minimum) stays an ordinary editable row. The draft validator names it; this reader reports only what it cannot show.The
value.trim() !== ''filter inOptionsEditor.commitis unchanged. It is correct for the row it was written for — this editor's own blank trailing row — and the repair is upstream: an authored entry no longer arrives wearing that row's shape.Ablation — prediction written before the run
Mutation, one line in
OptionsEditor.commit:r.kind === 'malformed' || …becomesr.kind !== 'malformed' && …, restoring the destructive half and leaving the reporting half intact. Direction predicted: turns red.4a98dfb6…; anchor occurrences 1 to 0, injected text 0 to 1; mutated blob45384a3f….4a98dfb6…equals the HEAD blob, anchor back to 1, injected text back to 0,git diff HEADempty for the path; re-run 27 of 27 green.The four well-formed controls staying green on both legs is what separates this repair from "the inspector now rejects everything".
Verification
ObjectFieldInspector.malformedOptions.test.tsx, 27 tests, all refusal-shaped: they assert the WRITTEN document, and the preserved entries are put back throughSelectOptionSchema/FieldSchemato show that preserving an entry is not a claim that it is valid.pnpm exec vitest run packages/app-shell/:Test Files 678 passed (678),Tests 6571 passed | 1 skipped (6572).pnpm --filter '...@object-ui/app-shell', i.e. packages that DEPEND ON app-shell — console plus the two console examples):@object-ui/consoletestsTest Files 98 passed (98),Tests 1132 passed (1132), which includes objectui#6844's registry-resolved designer pin.type-checkgreen for all three dependents after building their closure.pnpm --filter @object-ui/app-shell run type-check(note the hyphen in the script name) exits 0 on both legs, and the new pin is proven inside the typechecked set:tsc -p tsconfig.test.json --listFilesnames the file, 4493 files total.check:i18n-keys,check:i18n-drift,check:i18n-designer-parity,check:i18n-dead-keys,check:designer-field-key-parity,check:control-bytes,check:icon-record-names,check:vi-mock-specifiers,check:vi-mock-inherit,check:vi-mock-override-shape,check:unreferenced-sources,check:shell-escape-residue,check:new-line-citations,check-changeset-presence.eslint --no-inline-config --format jsonover the three changed files: 3 files linted, 0 errors, 11 warnings, every warning a pre-existingno-explicit-anyat a site this diff does not touch (the diff removes oneany). The narrowing is a measurement, not a skipped run:eslint.config.jsdeclares no type-aware program — noproject, noprojectService, notsconfigRootDir— and none of the 9 custom rules ineslint-rules/reads the filesystem, so a change confined to these three files cannot move the verdict on any file it did not touch. The repo-wideeslint .remains CI's run.i18n — the card's "ten locales" is not what this surface uses
en/zhtables inviews/metadata-admin/i18n.tsand is deliberately outside the ten locale packs — its own header andpackages/i18n/README.md("Scope — theengine.*carve-out") both say so, andcheck-i18n-call-site-keys.mjsskips the module by declaration. Seven keys land in each of the two tables, andcheck:i18n-designer-parityis the gate that holds them paired.Acceptance notes
Filed as objectui#9074 —
FieldStub.PicklistStubslices the preview list before filtering it, so entries the filter drops still consume the display budget: a well-formed option sitting behind malformed ones never renders, while the+Ncounter counts them all. Driven directly againstFieldStubwith the projectionObjectFormCanvashands it for['draft','open','closed',{value:'real',label:'Real'}]— the multiselect card renders"Pick one or more…+4"with zero badges and the select card renders"Select…", with the one well-formed option on screen in neither. Kept out of this PR: a read-only preview stub in a different component, destroying nothing, and the repair turns on a product question this card does not answer (should the canvas show a malformed option at all, now that the inspector reports it).noted, not filed — the same
String(o.value)normalisation appears in the runtime renderers (plugin-gantt,plugin-form,plugin-kanban,plugin-detail). Those are display surfaces rather than authoring surfaces, andpackages/types/src/objectql.tsdocuments the normalisation as deliberate, so re-opening it needs a ruling and not a patch. Successor: none — no queued card covers it.条款② — settled in seat, PASS.⚠️ An earlier revision of this section said
check-clause2-carrierswould report C3 and that the carriers were "not this seat's to settle in either direction." That sentence came from this seat's dispatch brief and it was wrong; it is retracted in full on objectui#8568 (comment5625952236).What actually happened, in order, none of it retroactive: the dev hung
needs:contract-reviewon both carriers — additively, and proved it with a comparative read-back — after flagging the conflict between the brief and the standing clause rather than silently picking a side. This seat's in-seat clause-② review then returned PASS (comment5626521997), and both carriers were cleared with provenance (comment5626533585).check-clause2-carriers --pair 9075measured exit 4 (C3) while unhung and exit 0 once hung-then-cleared, so pre-landing check ② (contract-review.md:42) is satisfied on the record rather than waived.Scope
⛔ objectui#6844's sample-content fix and the gate half already handled by objectui#8633 are both out of scope; this is the product half only.
Generated by Claude Code