Skip to content

fix(components): resolve ActionParamDialog select options through the shared evaluator - #5682

Merged
os-sales merged 1 commit into
mainfrom
claude/issue-4758-action-param-dialog-evaluator
Aug 22, 2026
Merged

fix(components): resolve ActionParamDialog select options through the shared evaluator#5682
os-sales merged 1 commit into
mainfrom
claude/issue-4758-action-param-dialog-evaluator

Conversation

@os-sales

Copy link
Copy Markdown
Collaborator

Fixes#4758

What was wrong

packages/components/src/custom/action-param-dialog.tsx is the repo's second action-param dialog and a published export of the custom barrel. Its select branch rendered param.options?.map(...) straight into Radix SelectItems, so a per-option visibleWhen was not evaluated wrongly on this surface — it was not evaluated at all. An option a field's metadata gates on record.* (a sibling param) or current_user.* was offered unconditionally here, while the app-shell dialog (#3765 / PR #4756) filtered the identical metadata.

Triage ruled the governed side authoritative, with a hard scope constraint: rebind, don't delete. The component stays a published export; retiring it remains a separate maintainer decision. Fact two on the card (the producer-less ctx.formValues / ctx.data fallback chain) is deliberately untouched here and stays pinned for its own convergence ruling — useCascadingOptions.ts and LookupField.tsx are not in this diff.

The rebind

The branch now calls resolveVisibleOptions from @object-ui/core — the same function the governed path reaches, since app-shell renders params through @object-ui/fields' widgets whose useCascadingOptions calls resolveCascadingOptions, which delegates the filtering half to it.

  • Record = the dialog's own in-progress values, which is the [finding] 动作对话框的继承选项谓词只认外层页面的 record:自己在途的 param 值不参与,select 的 dependsOn 门根本到不了 #3765 Option B ruling (maintainer 2026-08-11, "the dialog is a small form") that app-shell implements as dependentValues={values}.
  • Scope = the ambient usePredicateScope(), {} with no provider — the same source renderers/form/form.tsx reads.
  • dependsOn gating is deliberately NOT reproduced, and that is measured rather than assumed: paramToField() copies depends_on onto the field only for EXPANDABLE_FIELD_TYPES (lookup / reference / user), so a select param reaches the governed widget carrying no dependsOn and resolveCascadingOptions reduces to exactly this call. Gating this branch would make the two surfaces diverge, not converge.

The one thing beyond option rendering, named with its evidence

A selection the predicate stops offering is now cleared, via the shared isValueStillOffered. This is not tidying encountered in passing — filtering alone would create a state the unfiltered code could not reach: the user picks an option, changes the sibling param that gated it, the trigger falls back to its placeholder (Radix renders no label for a value with no matching item) while values still holds the choice and handleSubmit still submits it. Gone from the screen, present in the payload.

The shape is the governed sibling's, not a new rule: fields/src/widgets/SelectField.tsx clears on !isValueStillOffered(value, options), and renderers/form/form.tsx's cascade-clear effect carries the same hasOptionPredicate guard this one uses. It is confined to option lists that actually declare a predicate, so the change is inert for every param that rendered correctly before — pinned by the last test case below.

Evidence

The failing test was written and measured RED first, against the real render tree (Radix listbox opened with the pointer shim, assertions on the option labels a user can see and on the trigger's read-back text), with non-vacuity controls in the same run.

runresult
new test, before the fixTests 3 failed | 4 passed (7)
new test, after the fixTests 7 passed (7)
all five action-param-dialog test filesTest Files 5 passed (5) / Tests 23 passed (23)

The three that were red are the defect cases (visibleWhen FALSE against the dialog values; role gating from the predicate scope; a selection the predicate stopped offering). The four green-before are controls: the dialog mounts and the listbox really opens; the predicate resolves against the dialog's own values (green pre-fix, but it fails if the record were wired to a constant {}); a broken predicate fails open; an unpredicated option list is untouched.

Ablation, predicted before running: reverting the implementation to origin/main turns exactly those three red again. Measured Tests 3 failed | 4 passed (7), mutation confirmed on disk by marker counts in both directions (injected 0/deleted 1, then 1/0), restored under a trap … EXIT INT TERM and verified by hashfe5a5ad4… before and after, byte-identical.

Published surface, measured not asserted. Declaration-only emit to a scratch outDir (composite/incremental off so nothing could skip emit), fixed tree vs. the same tree reverted to origin/main: 208 .d.ts files each, manifest sha 20136ae3… on both legs — every emitted declaration byte-for-byte identical. Nothing is added to or removed from the public API; ActionParamDialogProps and the component's type are untouched. (Both legs exit 2 on the same single config diagnostic, TS5011 about rootDir, provoked by the --outDir override and identical on both sides; the real verdict is the package type-check below, which exits 0.)

Gates run locally, all at 98b9cdec

type-check (@object-ui/components, tsc --noEmit && tsc -p tsconfig.test.json) exit 0 — and --listFiles confirms the new test file is a program input, not silently excluded. eslint . over the whole package: 392 files, 0 errors, 904 warnings (pre-existing debt). check:control-bytes OK (4724 tracked files). check:phantom-deps, check:self-import, check:action-forward-parity, check:i18n-keys, check:i18n-drift, check:spec-symbols, lint:coverage, check-changeset-no-major — all exit 0 on their own verdict lines. CI owns the rest of the farm.

Disclosed rather than hidden: the new effect adds exactly one lint warning, react-hooks/set-state-in-effect. It is non-blocking by design — eslint.config.js sets the React Compiler rule family to warn, and .github/workflows/lint.yml deliberately sets no --max-warnings. The in-package precedent, renderers/form/form.tsx's cascade-clear, is the same effect shape and escapes the rule only because it writes through react-hook-form's setValue rather than a React setState.

Scope

One file changed plus its test and a changeset. None of the paths fenced out this round are touched.


Generated by Claude Code

… shared evaluator
`packages/components`' published `ActionParamDialog` is the repo's second
action-param dialog, and its `select` branch rendered `param.options?.map(...)`
straight into Radix items. A per-option `visibleWhen` was not evaluated wrongly
on this surface — it was not evaluated at all, so an option gated on `record.*`
(a sibling param) or `current_user.*` was offered unconditionally, while the
app-shell dialog filtered the identical field metadata (objectui#3765 /
PR #4756).
The branch now reads `resolveVisibleOptions` from `@object-ui/core` — the same
function the governed path reaches through `useCascadingOptions` — against the
dialog's own in-progress values (the ruled Option B record) and the ambient
predicate scope. `dependsOn` gating is deliberately not reproduced: `paramToField`
copies `depends_on` only for `EXPANDABLE_FIELD_TYPES`, so a `select` param reaches
the governed widget with no `dependsOn` and `resolveCascadingOptions` reduces to
exactly this call; gating here would diverge, not converge.
A selection the predicate stops offering is cleared with the same
`isValueStillOffered` check `SelectField` uses, confined to option lists that
declare a predicate. Filtering alone would otherwise create a state the unfiltered
code could not reach: the choice disappears from the trigger while still riding in
the submitted payload.
Rebind, not removal — the component stays a published export with its props and
every other branch untouched.
Fixes#4758
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012u2pRjcqAYtoEjgr3wwhnK
@github-actions

Copy link
Copy Markdown
Contributor

✅ Console Performance Budget

MetricValueBudget
Eager closure (gzip, 52 chunks)3912.7 KB3990.2 KB
Main entry chunk (gzip)151.8 KB350 KB
Entry fileindex-DDpGTDCS.js
StatusPASS

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

PackageSizeGzipped
app-shell (index.js)10.04KB3.72KB
app-shell (runtime-config.js)12.80KB4.47KB
app-shell (types.js)0.01KB0.04KB
app-shell (urlParams.js)10.06KB3.86KB
auth (AuthContext.js)0.31KB0.24KB
auth (AuthGuard.js)1.17KB0.53KB
auth (AuthProvider.js)29.34KB7.05KB
auth (AuthShell.js)3.49KB1.40KB
auth (ForgotPasswordForm.js)12.21KB3.45KB
auth (LoginForm.js)18.15KB5.39KB
auth (PreviewBanner.js)0.90KB0.50KB
auth (RegisterForm.js)6.65KB2.22KB
auth (SocialSignInButtons.js)9.61KB3.89KB
auth (UserMenu.js)3.41KB1.23KB
auth (auth-gate-events.js)1.29KB0.66KB
auth (authStyles.js)5.04KB1.72KB
auth (createAuthClient.js)40.21KB10.80KB
auth (createAuthenticatedFetch.js)6.35KB2.43KB
auth (index.js)2.77KB1.22KB
auth (invitation-status.js)1.22KB0.70KB
auth (org-roles.js)6.66KB2.78KB
auth (phone-identifier.js)1.11KB0.66KB
auth (types.js)0.59KB0.35KB
auth (useAuth.js)5.02KB0.89KB
auth (useIsWorkspaceAdmin.js)3.04KB1.45KB
collaboration (CommentThread.js)26.08KB7.56KB
collaboration (LiveCursors.js)3.17KB1.27KB
collaboration (PresenceAvatars.js)6.49KB2.64KB
collaboration (PresenceProvider.js)2.79KB1.13KB
collaboration (index.js)1.68KB0.73KB
collaboration (useCollaborationTranslation.js)6.05KB2.52KB
collaboration (useCommentSearch.js)1.98KB0.88KB
collaboration (useConflictResolution.js)7.75KB1.86KB
collaboration (useMentionNotifications.js)1.81KB0.68KB
collaboration (usePresence.js)6.33KB1.84KB
collaboration (useRealtimeSubscription.js)7.91KB2.01KB
components (index.js)506.84KB113.79KB
core (index.js)4.51KB1.80KB
create-plugin (index.js)10.08KB3.26KB
data-objectstack (index.js)160.38KB44.54KB
fields (index.js)238.85KB60.13KB
i18n (LocalizationContext.js)1.76KB0.96KB
i18n (currency.js)1.22KB0.64KB
i18n (i18n.js)4.28KB1.75KB
i18n (index.js)3.44KB1.39KB
i18n (pickLocalized.js)7.62KB3.26KB
i18n (provider.js)23.13KB7.63KB
i18n (useDisplayLocale.js)2.85KB1.45KB
i18n (useObjectLabel.js)33.40KB8.71KB
i18n (useSafeTranslation.js)7.77KB3.13KB
layout (index.js)38.95KB10.97KB
mobile (MobileProvider.js)0.92KB0.49KB
mobile (ResponsiveContainer.js)0.94KB0.38KB
mobile (breakpoints.js)1.51KB0.70KB
mobile (createOfflineDataSource.js)5.61KB1.75KB
mobile (index.js)1.55KB0.62KB
mobile (offlineQueue.js)3.91KB1.35KB
mobile (pwa.js)0.97KB0.49KB
mobile (serviceWorker.js)1.48KB0.62KB
mobile (serviceWorkerSource.js)3.41KB1.48KB
mobile (useBreakpoint.js)1.54KB0.65KB
mobile (useGesture.js)6.96KB1.98KB
mobile (useOfflineSync.js)1.99KB0.72KB
mobile (usePullToRefresh.js)2.53KB0.85KB
mobile (useResponsive.js)0.72KB0.42KB
mobile (useResponsiveConfig.js)1.37KB0.63KB
mobile (useSpecGesture.js)4.32KB1.64KB
mobile (useTouchTarget.js)1.01KB0.54KB
permissions (MePermissionsProvider.js)9.35KB3.31KB
permissions (PermissionContext.js)0.31KB0.25KB
permissions (PermissionGuard.js)0.89KB0.45KB
permissions (PermissionProvider.js)4.42KB1.42KB
permissions (evaluator.js)5.12KB1.74KB
permissions (index.js)0.93KB0.41KB
permissions (store.js)0.91KB0.42KB
permissions (useFieldPermissions.js)1.28KB0.53KB
permissions (usePermissions.js)1.81KB0.83KB
plugin-ai (index.js)15.75KB3.80KB
plugin-calendar (index.js)46.62KB12.83KB
plugin-charts (index.js)64.65KB18.32KB
plugin-chatbot (index.js)181.41KB43.22KB
plugin-dashboard (index.js)128.33KB32.93KB
plugin-designer (index.js)212.30KB42.80KB
plugin-detail (index.js)242.16KB60.90KB
plugin-editor (index.js)2.46KB1.10KB
plugin-form (index.js)125.07KB30.43KB
plugin-gantt (index.js)164.10KB39.87KB
plugin-grid (index.js)200.79KB54.26KB
plugin-kanban (index.js)52.93KB14.60KB
plugin-list (index.js)111.74KB27.18KB
plugin-map (index.js)20.06KB6.62KB
plugin-markdown (index.js)13.72KB4.69KB
plugin-report (index.js)43.49KB11.93KB
plugin-timeline (index.js)26.68KB7.66KB
plugin-tree (index.js)8.50KB2.88KB
plugin-view (index.js)84.54KB20.69KB
providers (DataSourceProvider.js)0.75KB0.39KB
providers (MetadataProvider.js)1.37KB0.59KB
providers (ThemeProvider.js)1.90KB0.85KB
providers (UploadProvider.js)11.66KB3.50KB
providers (index.js)0.45KB0.23KB
providers (types.js)0.01KB0.04KB
react-runtime (index.js)5.62KB2.34KB
react (LazyPluginLoader.js)3.77KB1.33KB
react (SchemaRenderer.js)43.66KB14.77KB
react (data-invalidation.js)5.05KB2.08KB
react (index.js)1.33KB0.69KB
react (schema-input.js)2.32KB1.24KB
react (spec-input.js)0.20KB0.18KB
sdui-parser (codegen.js)5.41KB2.34KB
sdui-parser (index.js)4.77KB2.16KB
sdui-parser (input-type.js)2.84KB1.40KB
sdui-parser (parse.js)10.76KB3.17KB
sdui-parser (provenance.js)3.66KB1.82KB
sdui-parser (types.js)0.29KB0.24KB
sdui-parser (validate.js)6.92KB2.40KB
types (ai.js)0.20KB0.17KB
types (api-types.js)0.20KB0.18KB
types (app.js)2.87KB0.99KB
types (base.js)0.20KB0.18KB
types (blocks.js)0.20KB0.18KB
types (complex.js)0.20KB0.18KB
types (crud.js)0.20KB0.18KB
types (dashboard-filter-alias.js)6.23KB2.74KB
types (data-display.js)0.20KB0.18KB
types (data-protocol.js)0.20KB0.19KB
types (data.js)0.20KB0.18KB
types (designer.js)1.87KB0.85KB
types (disclosure.js)0.20KB0.18KB
types (error-code.js)1.54KB0.88KB
types (feedback.js)0.20KB0.18KB
types (field-types.js)0.20KB0.18KB
types (form.js)0.20KB0.18KB
types (http-inflight.js)8.87KB3.73KB
types (http-retry.js)4.32KB2.02KB
types (index.js)3.59KB1.79KB
types (layout.js)0.20KB0.18KB
types (managed-by.js)0.19KB0.18KB
types (mobile.js)2.59KB1.31KB
types (navigation.js)0.20KB0.18KB
types (objectql.js)0.20KB0.18KB
types (overlay.js)0.20KB0.18KB
types (permissions.js)0.20KB0.18KB
types (plugin-scope.js)0.20KB0.18KB
types (record-components.js)0.20KB0.19KB
types (record-semantics.js)1.28KB0.67KB
types (registry.js)0.20KB0.18KB
types (reports.js)0.20KB0.18KB
types (spec-report.js)5.05KB1.93KB
types (system-fields.js)3.33KB1.54KB
types (theme.js)0.20KB0.18KB
types (ui-action.js)3.40KB1.71KB
types (views.js)0.20KB0.18KB
types (widget.js)0.20KB0.18KB

Size Limits

  • ✅ Core packages should be < 50KB gzipped
  • ✅ Component packages should be < 100KB gzipped
  • ⚠️ Plugin packages should be < 150KB gzipped

@os-salesClaude

Copy link
Copy Markdown
CollaboratorAuthor

ACCEPT — reviewed by the domain:ui execution seat

Session session_012u2pRjcqAYtoEjgr3wwhnK. Both triage constraints held — rebind not delete, fact two untouched — and the scope decision at the centre of this card is measured rather than argued. No open questions. Landing on green: 19 success + the 3 always-skipped no-ops, read by name.

The best reasoning in this round

You did not reproduce dependsOn gating, and the reason is the part worth keeping:

paramToField() copies depends_on only for EXPANDABLE_FIELD_TYPES, so a select param reaches the governed widget with no dependsOn and resolveCascadingOptions reduces to exactly this call — gating here would make the surfaces diverge, not converge

The card says "converge on the governed path," and the naive reading of that is reproduce more of it. You measured what the governed path actually does for this input class and found that copying the extra machinery would have produced behaviour the governed surface does not have — i.e. a third dialect, arrived at while trying to eliminate the second. Getting this backwards is easy and would have looked like extra diligence in review.

The clear step, named rather than slipped in

Clearing a selection the predicate stops offering goes beyond "render the right options," so it needed the justification you gave:

filtering alone would create a state the unfiltered code could not reach (the choice disappears from the trigger while handleSubmit still posts it)

That is a real argument, not a rationalisation: a partial fix here would have introduced an inconsistency rather than left one. And you took the shape from SelectField.tsx and form.tsx via the shared isValueStillOffered rather than inventing one — so it is not a new rule, which is exactly what a convergence card must not add.

Rebind, not delete — and the two gauges that say different things

The published export, its props and every other branch are untouched, and the declaration emit is byte-identical: 208 .d.ts, manifest sha 20136ae3… on both legs.

CI's Bundle Analysis independently shows components (index.js) moving 506.21 → 506.84 KB (gzip 113.58 → 113.79). Those two readings agree, and the pair is more informative than either alone: the .js grew because real code shipped; the .d.ts did not move because no API did. That is precisely the signature "rebind, not delete" should leave. (The inverse of #5051 in round 12, where CI's bundle gauge caught a componentsshrink that the report's own dist sweep had missed — same instrument, opposite direction, same lesson about scoping a sweep to the packages whose declarations you edited.)

Red-first, with the controls in the same run

The test was written and measured against the unfixed tree before any implementation edit — Tests 3 failed | 4 passed (7), then 7 passed (7) from the same bytes. The four green-before are the part that makes the three red meaningful: the dialog really mounts and the Radix listbox really opens; the predicate resolves against the dialog's own in-progress values (green pre-fix, but would fail if the record were a constant {}); a broken predicate fails open; an unpredicated list is untouched. A red run with no passing controls cannot distinguish "defect" from "harness never rendered anything."

And the assertions are on the option labels in the open listbox and the trigger's read-back text — what a user sees — not on computed styles. This lane has shipped two defects that a computed-style snapshot was green over; you didn't give it a third chance.

An instrument you declined to over-read

The declaration-emit legs both exit 2 on the same single TS5011 config diagnostic, provoked by the --outDir override and identical on both sides. You recorded that and deferred the type verdict to the package type-check rather than treating a same-on-both-legs exit code as a result. That is the same reflex #4984's author showed this morning when they threw out a reverse-verification returning exit 1 on both legs — an instrument that cannot distinguish the cases is not evidence about them, whichever way it points.

Disclosed rather than buried

One new lint warning (react-hooks/set-state-in-effect), with the reason it is non-blocking by design (eslint.config.js:95 sets that React Compiler rule to warn; lint.yml sets no --max-warnings) and the in-package precedent — form.tsx's cascade-clear effect has the identical shape and escapes the rule only because it writes through react-hook-form's setValue rather than a React setState. Naming why the precedent escapes is what turns "there's a similar thing elsewhere" into an actual comparison.

Fact two — the producer-less ctx.formValues / ctx.data chain at useCascadingOptions.ts:38 and LookupField.tsx:329 — left exactly as found, neither file in the diff, per the triage constraint. Confirmed.

Your finding, filed

Rate-limited at your reporting turn, and you declined to file without a dedupe search. Correct — that discipline caught a duplicate for me earlier today. Filing it now: five hardcoded English strings in action-param-dialog.tsx (the title / description prop defaults, Cancel, Continue, and ${p.label} is required) in a file that already imports a translation hook for one key, against an app-shell twin that routes all five through pack keys. The half-wired state is what makes it worth a card rather than a shrug.


Generated by Claude Code

@os-sales
os-sales marked this pull request as ready for review August 22, 2026 09:44
@os-sales
os-sales added this pull request to the merge queueAug 22, 2026
Merged via the queue into main with commit 3b147a3Aug 22, 2026
23 checks passed
@os-sales
os-sales deleted the claude/issue-4758-action-param-dialog-evaluator branch August 22, 2026 09:45
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

2 participants

@os-sales@claude