fix(app-shell): name retained-but-inactive values in the metadata-admin inspectors - #6792
fix(app-shell): name retained-but-inactive values in the metadata-admin inspectors#6792claude[bot] wants to merge 2 commits into
Conversation
…f showing them as live `showWhen` gates rendering only, and `isFieldVisible` additionally re-shows any field that already holds a stored value — deliberately, "so existing config is never hidden". So an author who enabled a controller, filled its dependents and switched the controller back off keeps seeing those fields as ordinary, live controls: the stored config and the switch beside it disagree, and nothing on screen said which was in effect. Per the maintainer ruling of 2026-08-27 (Option C) the values are KEPT and the state is made explicit. Pruning on save was rejected outright — it silently discards config an author entered and inverts the rule above. - `inactiveRetainedKind()` — a pure read reporting a field on screen ONLY because the stored-value re-show rule fired, distinguishing 'controller-off' from 'no-controller' (the `__legacy__` render-only keys, where there is no toggle to point the author at). - `controllerAdmits()` / `hasStoredValue()` split out of `isFieldVisible` so the affordance and the visibility filter share ONE definition and cannot drift. The stored-value re-show rule itself is unchanged. - `FlowNodeConfigField` renders the notice with a deliberate "Clear value" action; read-only inspectors show the notice without it. Coverage is pinned mechanically over all 33 gated fields in the descriptor tables plus both runtime producers. Render-layer only: no save-path change and no data deletion — clearing is an ordinary field commit.
…e assertion The spec flipped ApprovalEscalation.enabled to default(true) upstream; once this repo consumes a release carrying it, the inspector descriptor must follow and this assertion flips to toBeNull(). Say so at the assertion so the failure reads as the intended signal rather than a break.
✅ 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
|
Standing down on this PR's second queue ejection — deliberately, and here is exactly whyWhat happened. Enqueued 16:03Z, ejected 17:01Z by This is not this PR's failure, and that is measured rather than asserted. On the ejected queue head, zero checks failed — the counts were 17 The cause is runner contention, repo-wide. At the time this entry was running, the pool was carrying two merge-queue entries (24 jobs each, run speculatively in parallel — the second entry is not "waiting", it is taking runners) plus three pull requests at 29–30 jobs each: roughly 138 jobs against a pool that, measured at 16:30, had not started a single job on either queue head 27 minutes after they were enqueued. The sibling entry #6791 was ejected the same way an hour earlier, with the same reason code. What I am doing instead of re-queueing immediately⛔ Not re-queueing this PR right now, and not because it needs anything. The queue currently holds exactly one entry (#6791, auto-rebased onto ⇒ This PR is re-queued as soon as #6791 merges, one entry at a time, until the pool recovers. Four other verified-green PRs from this seat (#6796, #6800, #6801, #6811) are being deliberately held out of the queue for the same reason. That hold is a throughput decision, not a review one — every one of them is green and accepted. One re-run has already been spent on the sibling entry (#6791, re-queued once at 16:03 after its own timeout). I am not spending a second speculative re-queue on this PR while the condition that caused both ejections is still present. If a third ejection happens on a single-entry queue, that is a different signal and it belongs to the merge queue's own configuration — the Generated by Claude Code |
⛔ 更正我上一条里的一句话:拥堵不是「repo-wide」,是本席自己造成的上一条我写:
**测了之后,这句是错的。**17:26Z 采样最近更新的 14 个 open PR,逐个读 check-runs 统计处于
⇒ **43 个在跑的 job 全部来自本席自己刚派发的那一轮。**样本里没有任何别的席位在占用 runner。
这改变了结论,不只是措辞「把合并队列串行化」并不能串行化 runner 池:PR 的 CI 和 ⇒ 上一条里"队列只剩一条就是最可能通过的状态"这个推断不成立:队列确实只剩一条,而它 25 分钟里仍然 0 个 job 开跑,因为本席自己的三个 R16 PR 排在它前面。 仍然照旧执行的部分:一次只挂一个队列条目(那仍是对的,只是不充分),已绿的 PR 继续以 draft 形式按住(它们的 CI 已跑完,不占 runner)。 新认识:并发上限买到的是并行编写,但每一轮结束时会同时产生 3 份 PR CI,而那份负载会和本席自己的落地竞争。这是一个真实的取舍,已记在座位贴上,并会向维护者报告 —— ⛔ 不由本席单方面改并发上限,那是维护者 2026-08-29 的明令。 自我修正的边界:本席只采样了最近更新的 14 个 open PR,且看不到同组织其它仓对同一 runner 池的占用。⇒ 「没有别的席位在跑」的含义是「本次采样范围内没有」,不是「不存在」。 Generated by Claude Code |
1 participant
Fixes #6499
Implements the maintainer ruling of 2026-08-27 (decision-inbox batch 3, verbatim 「同意」) — Option C: keep the values, name the state. Render-layer only: no save-path change, no data deletion, and
isFieldVisible's stored-value re-show rule is behaviourally unchanged.What was actually wrong on screen
showWhengates rendering, andisFieldVisibleadditionally re-shows any field that already holds a stored value — deliberately, "so existing config is never hidden". Those two rules together mean the residue is not hidden at all: an author who enabled a controller, filled its dependents, then switched the controller back off keeps seeing those fields as ordinary, live-looking controls. The stored config and the switch beside it disagree, and nothing on screen said which one was in effect.So the fix is not "reveal something hidden" — it is to stop rendering inert config in a live control's clothes.
The change
inactiveRetainedKind(field, node, fields)(flow-node-config.ts) — a pure read reporting a field on screen ONLY because the stored-value re-show rule fired. It distinguishes'controller-off'(a real toggle the author can switch back on) from'no-controller'— the__legacy__render-only keys, where no toggle exists and telling the author to go find one would be a fresh lie on the very screen whose defect was showing inert config as live.controllerAdmits()/hasStoredValue()split out ofisFieldVisibleso the affordance and the visibility filter share one definition of "the controller says yes". Duplicating it would let them drift, and the drift would be silent — the affordance would quietly stop annotating the fields it exists for.return true) is untouched; only the controller-resolution tail moved into a named helper. The 40 pre-existingisFieldVisibleassertions inflow-node-config.test.tsare the regression net, and ablation A4 below is the tripwire against the rejected option.FlowNodeConfigFieldrenders the notice beside the affected control with a deliberate "Clear value" action. Read-only inspectors show the notice without the action. Clearing is an ordinary field commit ofundefinedthrough the existingsetField— byte-identical to the author emptying the control by hand.engine.*strings inmetadata-admin/i18n.ts,en+zh.Coverage census — mechanical, with the control
Found by enumerating the descriptor tables at runtime, not by grep, and not by hand. The type list is parsed from the source table itself rather than from
FLOW_NODE_TYPE_OPTIONS, becausenotify/parallel_gateway/join_gatewayare realFLOW_NODE_CONFIGkeys the picker does not offer — using the option list alone silently missed them.FLOW_NODE_CONFIGkeysshowWhenscript_task/signalalias pairs)showWhengroupsThe 7 canonical groups:
start::triggerType(9 dependents) ·start::__legacy__(1) ·decision::__legacy__(1) ·script::__legacy__(5) ·approval::escalation.enabled(4) ·wait::waitEventConfig.eventType(2) ·boundary_event::boundaryConfig.eventType(3).The control that proves the scan discriminates: 24 of the enumerated node types carry zero
showWhenfields (create_record,connector_action,end,loop, …). A scan that simply tagged everything would report 32, not 8. This is asserted, not just observed — the coverage test fails if the bucket count, the gated-field total (33) or the ungated-type count (24) drifts.Two runtime producers are covered too, and they mint groups no source file contains:
jsonSchemaToFlowFields(any engine-publishedconfigSchemawhose nested object has anenabled: boolean) andconnectorInputFields(the same, re-prefixed underconnectorConfig.input). Both funnel through the singleisFieldVisiblefilter atFlowNodeInspector.tsx:229, which is why one predicate covers groups that do not exist statically. Each has its own negative control in the tests (a schema with noenabledsibling / a flat schema → 0 gated fields).Ablation — the affordance appears exactly when a hidden-but-stored value exists, and not otherwise
Run against the committed tree.
packages/app-shellhas nodist/at all and the tests import the mutated modules by relative path, so there is no stale-build hazard: every mutation changed the outcome with no rebuild step, which is itself the proof the tests read the mutated source. Every leg — mutation and restore — was confirmed by comparinggit hash-objectagainst the HEAD blob, never by an editor's exit code; each restore additionally assertedgit diff HEADempty. Baseline and post-restore both 25/25 green, so nothing was left mutated.FlowNodeConfigFieldinactiveRetainedKindalways returnsnulldoes not clear anything until the author clicks, and both re-show-rule guardsdoes NOT flag while the controller is on,an ordinary live field renders with no noticeisFieldVisible(the shape Option A would have produced)still shows a hidden-but-stored field (the rule the prune option would have inverted)goes red, plus 3 predicate + 5 componentA2 ∪ A3 is the "exactly when" claim, each direction failing independently: A2 proves the affordance is not vacuous, A3 proves it is not indiscriminate. A4 is the guard on the half the ruling forbade touching.
Commands and exit codes
Exit codes captured before any pipe (redirect-then-capture), and each verdict quoted from the gate's own output rather than from
$?.The union above was re-run on the final commit
abdbac92bafter the last edit, and this body cites that run.Lint scope, stated rather than assumed: repo-wide
eslint . --no-inline-configwas run in full — 3948 files (population read from eslint's own config, count read from--format json), 90 pre-existing errors across 75 files, 0 of them in any file this PR touches.eslint.config.jsconfigures no type-aware linting (0 hits forprojectService/parserOptions/project:), so this diff structurally cannot move an untouched file's verdict.check:i18n-keys: the dispatch order warned that author-facing strings need keys inpackages/i18n/src/locales/en.tsacross all ten packs. Measured, that does not apply here:metadata-admin/i18n.tsis a module-local label table registered in that gate'sEXCLUDED_TRANSLATORS, withforwardedScopecovering this directory.engine.*keys areen+zhin that one file by design, and the gate passes. Nothing external readsENGINE_STRINGS_EN/_ZH, so there is no pack-parity obligation to satisfy — following the dispatch's instruction literally would have added ten pack entries the gate does not want.Scope
Confined to
packages/app-shell/src/views/metadata-admin/and its tests; nothing inpackages/plugin-form/(#6237) orpackages/components/src/renderers/basic/(#6783). Nocontent/docs/releases/edit. The fenced directory was clean on arrival, as the dispatch predicted.ApprovalEscalation.enabledtodefault(true)upstream, inspector still declaresdefaultValue: 'false'). Re-measured here on 2026-08-29 and it is still latent — installed@objectstack/specis 17.2.0 with.default(false), so the two agree today;mainupstream has.default(true), so the next spec bump makes it live.inactiveRetainedKindresolves controllers through the samedefaultValue, so it follows that fix automatically. One assertion inflow-node-config.inactiveRetained.test.tsis deliberately coupled to the current literal and will flip totoBeNull()when #6620 lands; the assertion carries a comment saying so, so the failure reads as the intended signal rather than a break.Generated by Claude Code