Repository navigation
fix(app-shell): give InspectorSelectField the unknown-value rule eight call sites hand-rolled (objectui#8488) - #8863
Conversation
…d of a blank trigger (objectui#8488) Radix renders a controlled value matching no option as nothing at all, so a stale or off-spec metadata value painted an EMPTY `InspectorSelectField` — indistinguishable from "unset". `InspectorSelectField` now synthesises a selectable, flagged row for it; the optional `unknownValueLabel` prop keeps the wording with the call site. Eight hand-rolled copies of the rule are deleted in favour of it: three that flagged the row (`ActionTargetField`, the view-column field picker, `FlowNodeConfigField`) and five that appended the raw value UNFLAGGED (`ReportDefaultInspector` type/dataset/chartX/chartY, `ViewVariantInspector` type), which made "stored" and "offered" the same picture. The placeholder is deliberately not the repair — it would assert "nothing is stored" about a field that is storing something. objectui#8450 keeps the empty state; a case pins that the two never impersonate each other. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01611D6ZaRaMmwTNQmSbk8MH
✅ 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
|
PM 评审 —— PR #8863(卡 #8488):通过,已翻 ready、auto-merge 已武装
⛔ 我的前提是不完整的 —— 三份,实际是八份我的裁决写的前提是:「三份手搓副本在规则上一致,只在措辞上不同」,并且给了「若在规则上分歧就报回、⛔ 不要强行统一」的作废条件。 规则那一半成立了。但population 是错的。 我在 四处 ⭐ 这比我的前提更有价值:我数的是「三处已经修好的」,dev 数的是「所有做同一件事的」。⛔ 我数的是解法,不是问题。 那五处从来没被人当成同类项,因为它们没有可供 grep 的标记 —— 而这正是它们有缺陷的原因。 卡片正文的一处具体断言也被证伪了,我同样复核过:正文说 ⭐ 45 vs 61 的争议解决了,而且两个数字都是对的我自己重算,与 dev 的对账逐字吻合: ⇒ 两份测量都没数错,它们数的是不同的 population,而两边都没说自己数的是哪个。 这场争议横跨两个座位和一张卡,⛔ 不是因为谁算错了。派发里那句「重推时把读法写出来,别只给数字」正是为此,而它这一次真的付了钱。 消融是这一班的标准形状,而且它的不对称才是关键摘掉合成分支(盘上先证:blob ⭐ 更重要的是没有变红的那些:#8450 的十条空态用例全绿,外加 还款到位的两处:钉的是渲染出来的 trigger 文本(⛔ 不是选项数组)—— 缺陷的形状本来就是「 裁决的边界,逐条守住
位置上他们统一为 prepend 并给了理由(Radix 打开时把选中项滚入视野,于是真实 roster 正好落在陈旧行下方,替换值就在手边)。只有 Clause-②
|
Fixes #8488.
What was wrong
InspectorSelectFieldhands Radix a controlled value. Radix renders a controlled value matching noSelectItemas nothing at all — no fallback, no raw value, no marker. So a stored value the current roster does not offer painted an empty trigger, pixel-identical to "this field is unset". The author saw an empty control, picked a value to fill it in, and silently overwrote a key they were never shown.objectui#8450 fixed the EMPTY half (no value at all now draws the placeholder) and deliberately left this half alone.
The direction taken
The primitive owns the rule now: a non-empty value the roster does not contain is synthesised as a selectable, flagged row, labelled
VALUE (not found)by default, overridable per call site through a new optionalunknownValueLabelprop.⛔ Explicitly not the placeholder. Drawing the placeholder over an unknown value would assert "nothing is stored" about a field that IS storing something — confidently wrong rather than merely blank. The two states are pinned as never impersonating each other.
⛔ Explicitly not direction 2 (raw value painted on the trigger, not an option). One of the pins reads the flagged text off the trigger precisely because that text can only get there by way of a mounted
SelectItemthe controlled value matched — which is what makes the row re-pickable rather than decorative.Re-derived call-site count — the reading, not the number
The card carried a disputed 45 vs 61. Both are correct; they count different populations of the same grep, and they reconcile exactly:
The grep pattern is a LEFT-ANGLE-BRACKET immediately followed by
InspectorSelectField— the JSX opening tag. It is spelled in words here because a literal angle-bracket fragment is silently eaten out of GitHub bodies:(substitute the real one-character prefix for
OPENTAG_)_shared.select.test.tsxrecorded for objectui#8450.*.test.tsx(2 files).The falsifiable premise — measured, and it did not hold as written
The dispatch's premise was: the three hand-rolled copies agree on the RULE and differ only in WORDING. Measured:
Where it held. All three test the same predicate (non-empty value AND not in the roster), all three synthesise a selectable row labelled
VALUE (suffix), and none of them treats falsy-but-present differently — every one coerces to a string first, so0andfalsearrive as non-empty strings. So the rule really is one rule, and(deprecated)needed to be overridable, not flattened. It is:FlowNodeConfigFieldkeeps its sourced wording (framework#4278 / ADR-0090 D3) through the new prop, and so does the view-column picker's(not in object).Where it did not hold — the premise was incomplete. There are not three copies. Sweeping the whole membership-test shape across the call sites of this primitive found eight:
ActionDefaultInspector·ActionTargetField(not found)ViewColumnInspector· field picker(not in object)FlowNodeConfigField· select branch(deprecated)ReportDefaultInspector· report typeReportDefaultInspector· datasetReportDefaultInspector· chart X axisReportDefaultInspector· chart Y axisViewVariantInspector· view typeThe five unflagged ones append the raw stored value with no marker — which is direction 2 implemented locally: "stored" and "offered" become the same picture on screen. All eight are deleted in favour of the primitive, so the five now flag.
A specific claim in the card body is falsified by that. The card says
datasetName"leaves the binding blank rather than saying so". It does not:datasetOptionswas already appending the raw name. The binding was never blank there — it was unmarked, which is a different defect and the one actually repaired.Position also differed (two prepend, one appends). The primitive prepends: Radix scrolls the selected item into view on open, so the real roster then sits directly under the stale row, which is where a replacement gets picked. Only
FlowNodeConfigField's row moves.Acceptance — the class predicate
Pinned by rendered trigger text, never by an options array — the bug's whole shape was a correct
valueprop that reached no pixels._shared.unknownValue.test.tsx(new, 6 cases) — the flagged render; empty-vs-unknown side by side in one case; a known value left alone; the call-site wording override; the flagged row proved to be a real matched option; and a no-op case for a caller still passing its own row.ReportDefaultInspector.test.tsx(2 new cases) — a call site that never hand-rolled a flag, pinned end to end: a dropped dataset readsretired_dataset (not found), a catalogued one keeps its own label with nothing flagged._shared.select.test.tsx(objectui#8450's 10 cases) unchanged and green — the empty half still belongs to it.Ablation — from the committed tree, on-disk proof before the run
Mutation: the synthesis branch in
_shared.tsxreplaced byconst shownOptions = options;.Landed on disk, proved before running anything (script carried
trap ... EXIT INT TERMwith absolute paths, and refused to proceed unless every check below passed):Red by test-case name — 5 failed, 23 passed:
The failure text is the bug itself:
expected '' to be 'retired_group (not found)'— the ablated trigger renders the empty string.The asymmetry is the point. All 10 of objectui#8450's empty-state cases stayed green, as did
leaves a KNOWN value alone,is a no-op when the call site already synthesised the row itself, andstill shows a catalogued dataset by its own label. The pins fail for this bug's own reason and for no other.Restore leg, symmetric and proved by state, not by an exit code:
git checkout HEAD -- ABSOLUTE_PATH, thengit diff HEADempty andgit hash-objectback toe694b0bd8db8d5bd350b5bfa9051b1afa6513144.Verification
All at
3c1e997fb, the final commit.pnpm exec vitest run packages/app-shell/src/views/metadata-admin/pnpm --filter @object-ui/app-shell type-checktsc --noEmit && tsc -p tsconfig.test.json, so the new test files are type-checked toopnpm --filter @object-ui/app-shell lintpnpm exec eslint . --no-inline-config --format json(whole repo, not narrowed)node scripts/check-changeset-presence.mjsnode scripts/check-governed-queue-guard.mjs --testThe repo-wide eslint run is the full population, so no narrowing argument is owed for it.
Clause-2 — measured
nopackages/app-shell/package.jsonexportscarries only.and./styles.css.packages/app-shell/src/index.tshas zeroexport *and does not nameInspectorSelectFieldor_sharedanywhere.packages/typesorpackages/components.So
unknownValueLabelis a new prop on a module-internal component and never reaches the published type surface. No contract review is needed, and noneeds:contract-reviewlabel was applied.验收备注
ViewColumnInspector's hand-rolled copy already had it onmain, and it is reproducible there today. Closing it needs a loading contract on the primitive's inputs, which is a different decision than this card's.FlowObjectListField.tsxcarries a ninth copy of the same rule ((deprecated), same ADR-0090 D3 lineage), but on its own Radix select rather than this primitive — outside this card's scope, which is the primitive and its call sites. Successor: none known.engine.form.notInObject, with a zh-CN entry), so localising the inspector suffix is a real follow-up. Not a defect, a contract violation, or a metadata trap, so it is noted rather than filed. Successor: none known.DashboardWidgetInspectorhas the same unflagged-append shape for its dataset name, but it does not render through this primitive. Successor: none known.🤖 Generated with Claude Code
https://claude.ai/code/session_01611D6ZaRaMmwTNQmSbk8MH
Generated by Claude Code