fix(components): resolve the declared action type for a bar-hosted action:icon - #6362
Merged
os-support-ai merged 1 commit intoAug 25, 2026
Merged
Conversation
…tion:icon (objectui#6306) `action:bar` does not route members through `SchemaRenderer` — it pulls each member's renderer off the registry and RENAMES the declared type as it spreads it onto the child: `type` becomes the component id and the real declaration moves to `actionType`. `action:button` has always resolved that pair when it forwards (`schema.actionType || schema.type`); `action:icon` read `schema.type` alone and dropped `actionType`, handing the runner `type: 'action:icon'`. `ActionRunner.execute` resolves its handler from `action.type || action.actionType || action.name`, and `'action:icon'` binds no registered handler and no builtin. For a declaration carrying `target` rather than `endpoint` it does not reach the legacy `navigate`/`api` fallback either, so it fell through to `executeActionSchema` and the authored action never ran — no error, no toast, the objectstack#2169 "Mark Done does nothing" shape. One authored action executed or did nothing purely as a function of which `component` the host picked for it. The `|| schema.type` leg is load-bearing in the other direction: rendered standalone, `action:icon` never sees an `actionType` and its own registry `inputs` declare `type` as the action type. `actionType` joins the props intersection for the same reason it sits on `action:button`'s: it is the host-composed override read first, and it is not a `UIActionSchema` key.
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-support-ai
marked this pull request as ready for review
August 25, 2026 16:11
os-support-ai
deleted the
claude/issue-6306-action-icon-type-resolution
branch
August 25, 2026 16:24
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 #6306
An
action:iconrendered as a member of anaction:barforwarded the component id as the action type, so the click resolved no handler: nothing happened, with no error and no toast — the objectstack#2169 "Mark Done does nothing" shape.All gate readings below were taken on the final commit of this branch,
afb35717b.Premise, re-derived on
origin/main@194fae184Not taken on trust from the card. Every line number in the report reproduces exactly on the ref this branch is cut from:
action:bardoes not route members throughSchemaRenderer. It pulls each member's renderer off the registry and renames the declared type as it spreads it onto the child:typebecomes the component id, and the real declaration moves toactionType.action:buttonresolves that pair when it forwards;action:iconreadschema.typealone and droppedactionTypeentirely, handing the runnertype: 'action:icon'.'action:icon'binds no registered handler and no builtin. For a declaration carryingtargetrather thanendpointit does not reach the legacynavigate/apifallback either, so it falls through toexecuteActionSchemaand the authored action never runs. The bug was a function of the layout, not the declaration — one authored action executed or did nothing depending only on whichcomponentthe host picked for it.Census (a control, not an assumption)
type: schemaappears in exactly two files underrenderers/action/:action-button.tsx(already correct) andaction-icon.tsx.action:group(:249) andaction:menu(:220) forwardtype: action.typefrom their own member array — the member's declared type is used directly and never renamed, which is why the construct does not appear there. They are a genuinely different composition, not a third instance. No third case was found; nothing else was touched.The fix
action-icon.tsxgets the same resolutionaction-button.tsxalready had.actionTypejoins the props intersection for the same reason it sits onaction:button's — it is the host-composed override read first, and it is not aUIActionSchemakey. No accept set is widened; this restores declared behaviour only.The
|| schema.typeleg is load-bearing in the other direction: rendered standalone,action:iconnever sees anactionType, and its own registryinputsdeclaretypeas the action type.The pin executes clicks, and carries a positive control
The reason this shipped is that the existing icon coverage (
action-bar-member-visible-gate.test.tsx:195-213) rendersaction:iconbar members three times and asserts onlyvisible/enabled— it never asserts that a click reaches a handler. The new pin asserts execution, not props.Positive control. Every row that reads the icon member's zero renders a sibling
action:buttonmember of the same declaration in the same bar, with anapihandler registered throughActionProvider, and reads its one first. This is what makes a zero a reading rather than a dead probe: a harness that executes nothing at all — an unmounted renderer, a member pushed into the overflow menu, an assertion racing the asyncexecute— reports zero on both members, so the control tells the two apart from the failure message alone. The two members differ in exactly one authored key,component;name/labeldiffer only as addressing handles, and the runner never consults them here (typeis always truthy on this path, so its|| action.nameleg is unreachable).Two further design points:
handlers: { api, 'action:icon': trap }) makes the defect two-sided rather than merely absent — the unfixed renderer produces a positive artefact instead of only a missing call. Measured on the unfixed tree:expected "vi.fn()" to not be called at all, but actually been called 1 times. That is direct evidence the component id reached the runner as the action type, and the row fails in 22 ms instead of timing out at ~1000 ms.maxVisible: 10andmobileMaxVisible: 10). The inline/overflow split readsmobileMaxVisible ?? 1whenuseIsMobile()is true, so pinning onlymaxVisiblewould leave the split at the mercy of the environment's viewport — a zero that is not about type resolution at all.schema.actionTypealone, which would trade this defect for its mirror image on the surface wheretypeis the action type.Ablation — predicted before running, then compared
Prediction was recorded before the first run: 5 rows, 3 red / 2 green before the fix, 5 green after, with the zero caused by fall-through to
executeActionSchema(silent, no throw).action:buttonmember reaches the handleraction:iconmember reaches the same handlerexpected "vi.fn()" to be called 2 times, but got 1 timesexpected "vi.fn()" to be called 2 times, but got 1 timesaction:iconObserved:
Tests 3 failed | 2 passed (5)— rows and counts exactly as predicted. After the fix:Tests 5 passed (5).Where the prediction was wrong. In the trap row the
apiwaitForwas written first, so it timed out and the trap's own assertion never evaluated — the "two-sided" half was asserted rather than measured. The row was restructured to settle on either path (api.mock.calls.length + trap.mock.calls.length) and assert the trap first, and the ablation was re-run: the trap then fired observably (quoted above). The two-sided claim above is measured, not inferred.check:action-forward-parityis blind to this defect — and its green run is not evidencetypeis in the forward whitelist, and the gate diffs key presence against the owed set. This is a wrong-value defect behind a present key, a class the gate has no opinion on by construction.This was measured, not merely argued. Running the gate against the unfixed
action-icon.tsx(restored from194fae184, mutation confirmed on disk, then restored and verified byte-identical toHEAD) produces output identical to the fixed tree, including the per-surface counts:Same
owes 25, forwards 19, same✅, same exit 0 in both worlds — because the fix changes a value expression, not the key set.Could the gate be taught to catch it? Report only —
scripts/check-action-forward-parity.mjsisdomain:devxterritory and is not modified here. The observation, for whoever owns that call: the gate already parses each forward site's payload object literal, so it has the expression for each key in hand and currently uses only the key's presence. A narrow extension would be a per-key value contract for keys the host is known to rename — specifically, that any surface reachable as anaction:barmember must spelltypeasschema.actionType || schema.type, sinceaction-bar.tsxis the single composer of that rename (exactly one site repo-wide). That is a real design decision with a false-positive surface (a renderer legitimately composingtypesome other way), which is why it is filed as an observation rather than attempted here.Gates
Derived by reading the CI job step lists under
.github/workflows/(ci.ymlType Check / Test jobs,lint.yml,changeset-presence.yml,changeset-guard.yml,control-bytes.yml,vi-mock-specifiers.yml), not from memory. Exit codes captured before any pipe; each row quotes the gate's own verdict line.vitest run packages/components/Test Files 191 passed (191)/Tests 1736 passed (1736)Test Files 1 passed (1)/Tests 5 passed (5)@object-ui/components type-checktsc --noEmit && tsc -p tsconfig.test.json— no diagnosticscheck:action-forward-parity✅ action forward parity: 5 surfaces checked against 39 runtime-read keys from 4 consumerscheck:vi-mock-specifiers✅ check-vi-mock-specifiers: OK (3753 tracked source file(s), 2065 test-named; …)check:control-bytes✅ check-control-bytes: OK (scanned 5235 tracked text file(s); skipped 85 binary).check-changeset-presence.mjs✅ 1 source file(s) of 1 released package(s) changed, and this change declares 1 changeset(s)changeset:check✅ All workspace packages are in the changeset fixed group.+✅ No changeset declares amajorbump.pnpm lint(repo-wide)The typecheck green is a real measurement of this diff.
tsconfig.test.jsonincludessrc/**/*.test.tsx, and--listFilesconfirms both the new test file and the fixedaction-icon.tsxare program inputs (1 hit each) — so this is not the "typecheck excludes tests" shape where a green says nothing about new coverage.Lint, stated honestly. The repo-wide
pnpm lint(turbo run lint) was attempted and the container's ~10-minute foreground cap killed it withSIGTERM(exit 143) — that reads as not measured, not as red. It got through 46 of 47 tasks first, and@object-ui/components:lintwas among them:✖ 912 problems (0 errors, 912 warnings). The single task cut off was@object-ui/app-shell#lint, which this diff does not touch. No completed task reported any errors.The narrowed measurement, with the three things that make a narrowing a measurement rather than a skip: (1) the population is read from eslint's own config — 3752 files; (2) file counts come from
--format json— both changed files linted, 0 errors; (3)eslint.config.jsenables no type-aware linting (noprojectService, noparserOptions.project— zero matches), so this diff cannot move the verdict of any file it does not touch. The 12 warnings onaction-icon.tsxare pre-existing and were measured against the base file rather than assumed: 12 before, 12 after, delta 0.One caution for anyone re-running this: a bare
eslint .from the repo root is not the gate and reports 89 pre-existing errors across 74 files. The rootlint:rootscript deliberately ignorespackages/*/**, so that invocation lints package sources under the wrong config. The gate isturbo run lint(per-packageeslint .), which is what the table reports.Scope
packages/components/src/renderers/action/action-icon.tsxplus its new coverage and one changeset.renderers/overlay/andpackages/app-shellare untouched. No labels attached.Generated by Claude Code