Skip to content

refactor(ui): unify composer model selection with Astryx Selector - #6158

Merged
Astro-Han merged 8 commits into
apache:mainfrom
Astro-Han:refactor/model-picker-unify
Oct 19, 2026
Merged

Astro-Han merged 8 commits into
apache:mainfrom
Astro-Han:refactor/model-picker-unify

Conversation

@Astro-Han

@Astro-Han Astro-Han commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Model selection used nested radio menus, a custom ACP Agent rail/search panel, a compact WorkHub wheel, and a separate Codex plugin selector. Context capacity and selected-model summaries also appeared repeatedly.

Use one Composer-owned Astryx Selector, with search and connection/Agent groups. Thinking effort uses the shared adjacent selector. Remove context capacity and duplicate menu summaries.

Paths unified

Path Result
Native conversations and expanded WorkHub Searchable connection groups and the shared thinking control.
New tasks/home and side chat Native models, ACP Agents and Codex models share one selector.
Existing ACP sessions The same selector, scoped to the current Agent, retaining opaque model variants, modes and session recovery.
Codex client plugin Supplies its remote model catalog, provider identity and loading/retry callback through the existing slot's renderPicker; Composer owns rendering, shared provider logos, native/ACP composition, mutations and pending-send admission.
Compact WorkHub The same controls use Astryx's bottom sheet.
Model-recovery action Opens the production selector directly.

Settings retains its existing Astryx ModelPicker and shared native option shaping. Runtime Host, storage and dependency contracts are unchanged.

Integration and recovery

The slot delegates rendering to Composer with plugin-owned catalog data. One filtered executor catalog defines the scope: editable new tasks retain all configured Agents, while fixed sessions retain only their current Agent. Model options and recovery actions use that same catalog; selecting one Agent does not lock a new task to it.

Unavailable and authentication-required Agents expose Retry or Sign in both before selection and while a different ready Agent is selected. Recovery uses the existing callbacks without changing the current selection. Existing-session restore/history actions keep their existing callbacks. Loading uses Astryx's native loading state; new-task triggers retain Agent identity. Recovery notices wrap onto a separate row so they do not squeeze the model label on narrow screens. Fixed ACP sessions do not activate the Codex contribution or expose recovery actions for other Agents.

Removed implementations

  • Delete chat-model-switcher.tsx, composer-options-menu.tsx, executor-model-picker.tsx, model-picker-panel.tsx and model-wheel-picker.tsx, their obsolete styles and exports.
  • Remove the Codex client's native connection grouping, custom option/value rendering, separate model/reasoning Selectors, wheel branch/state and injected mark styling.
  • Replace old menu/rail/wheel tests and the Codex controls-shape mock with focused behavior tests through the real Composer, slot registry and actual Codex client bundle. Existing Electron checks retain Host-confirmed changes and native floating-window bounds; browser stories own search/focus/layout checks.

The cumulative diff adds 1,042 lines and removes 5,356 across 37 files (net −4,314).

Client integration migration

conversation.composer.model-selection now receives renderPicker, hasModels, fixedExecutorId and the current executor target. Contributions call renderPicker with their own model catalog instead of rendering replacement controls. The previous full-control owner props and ModelWheelPicker client export are removed. The repository-only Codex example is migrated in this PR; existing installations must be reinstalled from the matching Maka checkout. Its README documents this requirement. No compatibility picker or duplicate execution state is retained.

Refs #5787, #5789, #6103.

Verification

  • UI build and all 783 UI workspace tests passed. The focused Composer/slot/runtime set contains 25 passing tests.
  • Actual Codex client bundle + production Composer/slot integration covers failed discovery → Retry, keyboard opening, native → Codex → ACP → native, exact native connection identity, reasoning changes, pending-send admission, and fixed ACP model/mode controls.
  • ACP recovery tests cover no selection and an already-selected ready Agent; Sign in and Retry invoke the existing callbacks with zero selection writes. The fixed-session test asserts the exact Agent group and no other Agent's recovery button, retaining failed-write/confirmed-model checks. Removing the native-option guard or the catalog's fixed-session filter independently fails that isolation test. Earlier ACP-composition and pre-selection-recovery mutations also failed their regressions.
  • All 11 Codex adapter protocol tests passed. Removed tests protected the retired wheel interaction and mocked control structure; their supported selection/recovery obligations now run through the shared component.
  • Desktop typecheck, static Storybook build, 19 focused browser runs, format/lint, Desktop/UI Knip, surface inventory and renderer architecture checks passed.
  • Four additional CDP checks cover light/dark Codex composition and narrow recovery before and after selecting another ready Agent: no page overflow, search owns focus, no radio controls, shared provider logos are rendered, and recovery remains visible. The existing narrow story now selects Antigravity while retaining Claude Agent's Sign in action. Browser stories assert the Codex logo and narrow label geometry.
  • Full CI passed on the final head 68a62a530, which M4n5ter approved after rechecking the recovery and fixed-session paths. All three inline review threads are resolved. Electron E2E and packaged-app checks were not run locally.

Visual comparison

Same real product-workhub--standard-composer story, fixtures, Chinese locale and 1000 × 900 viewport. Left: separately built main at 286ffa820; right: this branch at 500b83641. Each image contains the complete composer and open popup.

Light theme: nested menu before, unified Astryx Selector after

Dark theme: nested menu before, unified Astryx Selector after

Current Codex contribution through the shared picker

The story loads the actual Codex client bundle. Native, Codex and ACP groups share the same searchable list; longer catalogs scroll inside Astryx's popup.

Codex contribution through the unified picker, light

Codex contribution through the unified picker, dark

AI use

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope: Codex assisted with component and plugin consolidation, legacy removal, regression tests, Storybook/CDP verification and this description.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

@github-actions github-actions Bot added the effort/XXL Over 2500 readable lines label Oct 19, 2026

@M4n5ter M4n5ter left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

English

I found two cases where the consolidation seems to drop existing behavior:

  1. External Agent controls disappear when a model plugin owns the slot — composer.tsx:2509

    Moving the entire picker into the slot’s fallback means it no longer renders when the Codex model plugin takes this slot. New tasks lose the other configured ACP Agents, and existing ACP sessions lose their model/mode controls and recovery actions. Previously, the external picker wrapped the slot.

    Could we preserve those Agent controls when a plugin replaces the model-selection slot?

  2. An unavailable Agent has no Retry action before it’s selected — composer-model-picker.tsx:254

    Discovery can return an unavailable entry without setting executor.error. In a new task there’s no selected Agent yet, so this produces a disabled row with no Retry action. Reopening the picker doesn’t refresh discovery either, leaving the user stuck after a transient failure.

    Could we keep a reachable Retry action here, using the existing onRetry callback?

I reproduced both with base/PR comparisons. The eight focused tests pass, but don’t cover these two paths.

中文

对照新旧实现时,发现两个原有行为可能丢失的场景:

  1. 模型插件接管 slot 后,外部 Agent 控件会消失 — composer.tsx:2509

    整个 picker 移进 slot 的 fallback 后,Codex 模型插件一旦接管这个 slot,它就不会再渲染。新任务看不到其他已配置的 ACP Agent,已有 ACP 会话也会丢失模型、模式和恢复操作。之前外部 picker 是包在 slot 外面的。

    这里能否保留插件替换 slot 时的外部 Agent 控件?

  2. 尚未选中的不可用 Agent 没有重试入口 — composer-model-picker.tsx:254

    发现过程可能返回 unavailable,但不设置 executor.error。新任务此时还没有选中 Agent,最终只显示一个禁用选项,没有 Retry;重新打开列表也不会重新发现,临时故障恢复后用户仍无法在这里继续。

    这里能否保留一个可操作的 Retry,复用现有的 onRetry 回调?

两处都做了基线与 PR 的对照复现。现有 8 个定向测试均通过,不过没有覆盖这两个场景。

@liugddx liugddx left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The consolidation correctly centralises native/external option shaping, exact connection identity, opaque model variants, and pending-write handling. I reviewed head 82a35ec0e89925097a5f90bfb5c5f24b6661602a and am requesting changes for two integration regressions below.

P1 — A plugin-owned model-selection slot can hide the executor picker for non-Codex Agent sessions. The Codex plugin claims this slot whenever onExecutorTargetChange exists, but it only renders native/Codex choices. With the new fallback containing the whole ComposerModelPicker, an existing non-Codex ACP session loses its current Agent model, mode, and recovery actions; the old ExecutorModelPickerBoundary kept that outer state visible. Retain the executor boundary around the slot, or make the slot contract carry and render the executor catalog and recovery state.

P2 — Unselected unavailable Agents are rendered as disabled rows with no recovery action. New tasks provide an executorPicker before a selection exists, but sign-in, restore, new-task, and retry actions are only rendered for executor.selection. This regresses the old browsed-Agent recovery surface for authentication_required, restorable, history_only, history_gap, and unavailable entries. Add a small per-entry recovery surface or route these rows to the existing callbacks.

P3 — Initial executor discovery has no loading state in the new Selector, and non-fixed external selections lose the Agent name from the closed trigger. These are non-blocking clarity regressions.

Tests: keep the multiline textContent assertion in apps/desktop/e2e/workhub-layout.spec.ts; the explicit click/focus immediately before fill() is redundant. Add tests for an unselected unavailable Agent and for a plugin-owned conversation.composer.model-selection slot. The current recovery test uses fixed: true with an existing selection, so it cannot catch the first regression.

The exact native IDs, opaque variant selection, mode reset, and pending-send gate otherwise look preserved. CI test is green; I did not run the full suite locally against this SHA.

Comment thread packages/ui/src/composer.tsx Resolved
@Astro-Han
Astro-Han requested review from M4n5ter and liugddx October 19, 2026 03:19

@M4n5ter M4n5ter left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

English

The plugin composition issue looks fixed now. There’s one remaining case in the Agent recovery fix: composer-model-picker.tsx:311.

If a new task already has a ready Agent A selected, another Agent B that is unavailable or authentication_required still has no Retry / Sign in action. recoveryEntries branches on executor.selection, so it only checks A and leaves B as a disabled option. The old picker allowed recovering B without changing the selection of A.

Could we limit the selected-Agent-only behavior to fixed sessions and keep the other Agents’ recovery actions available in new tasks? Extending the current recovery test with A already selected would cover this.

I reproduced the difference against base and the updated head.

中文

插件组合的问题看起来已经修好了。Agent 恢复这里还剩一个场景:composer-model-picker.tsx:311。

新任务已经选中正常的 Agent A 后,另一个处于 unavailable 或 authentication_required 状态的 Agent B 仍然没有 Retry/Sign in。recoveryEntries 根据 executor.selection 分支,只检查了 A,B 就只剩一个禁用选项。旧选择器允许在保留 A 的选择时恢复 B。

这里能否只对 fixed 会话限制为当前 Agent,新任务则继续保留其他 Agent 的恢复入口?在现有恢复测试里补一个“已经选中 A”的场景就能覆盖。

已对照基线和更新后的 head 复现这个差异。

@liugddx liugddx left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI review — OpenAI Codex, posted on behalf of @liugddx. This is not an independent human committer approval.

COMMENTED: the earlier blocking findings are addressed; no new P1/P2 findings. Reviewed f231aeb0fbc4a452f8efed123961517ca1254bfb against 286ffa8206cfaa9f58c0d50e806ab6287c457575.

The Codex contribution now supplies model data to the Composer-owned renderer, preserving native and ACP controls. Fixed ACP sessions bypass unrelated Codex discovery. Unselected authentication-required and unavailable Agents retain Sign in/Retry without selecting an unusable executor. Loading and Agent identity are restored as well.

P3 (non-blocking): the fixed-ACP isolation assertion checks the wrong group name. At packages/ui/src/__tests__/composer-model-picker-recovery.test.tsx:274, the assertion excludes OpenAI, while the fixture's native connection groups are named first and second. I deliberately allowed native options into a fixed ACP session: the original test still passed. An assertion against the actual first group failed with that mutation and passed with the unmodified implementation. Assert both native groups are absent, or assert that the only model group is Agent. Keep the surrounding failed-write/confirmed-model test.

Approach: centralizing rendering and mutations in Composer is the right layer and removes the plugin's duplicate implementation. The current design is sufficiently small; no additional abstraction is warranted. Keep the replacement integration tests, particularly the actual Codex client bundle through the slot registry. Useful remaining coverage would extend existing tests for rejected mode writes and search focus across asynchronous catalogue arrival.

Validation: Core/UI builds and all 783 UI tests passed. The Codex adapter suite had 5 passes and 6 failures on Windows; the base reproduced the same six failing cases, beginning with direct execution of its .mjs fixture failing with spawn EFTYPE. These are existing platform failures, not new findings against this PR. git diff --check passed. Browser/Electron E2E was not rerun in this review. The hosted test check passes at the reviewed head.

The smallest correct version is the current implementation with the isolation assertion corrected. My earlier P1/P2 objections no longer need to block this change.

@liugddx
liugddx dismissed their stale review October 19, 2026 03:34

AI follow-up review by OpenAI Codex on behalf of @liugddx: the earlier P1/P2 findings are resolved at f231aeb. See follow-up review 5566618394; only a non-blocking P3 test assertion remains. Withdrawing my previous blocking review; this is not human committer approval.

@Astro-Han

Copy link
Copy Markdown
Contributor Author

@M4n5ter Fixed the remaining recovery case in 68a62a530. Selection presence no longer determines the recovery scope: one catalog filtered by fixed drives both model options and recovery actions. The original plugin composition fix remains covered by the actual Codex client bundle and slot registry.

I extended the existing recovery test to keep ready Agent A selected while signing in or retrying Agent B, with zero selection writes. The existing narrow story now selects Antigravity while Claude Agent's Sign in action remains visible; light/dark CDP checks passed. All 783 UI tests, Desktop typecheck and the Storybook build passed locally. CI for the new commit is tracked separately.

@M4n5ter M4n5ter left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

English

Looks good to me on 68a62a530. Both issues from the earlier reviews are addressed, and I didn’t find any new correctness or maintainability concerns in this follow-up.

I rechecked the case with a ready Agent A already selected: another Agent’s Retry / Sign in action remains available without changing the current selection. Fixed sessions still expose only their own model and recovery controls. Sharing the filtered catalog between model options and recovery keeps those two paths consistent.

The original reproduction now passes, all 25 focused Composer/slot/runtime tests passed in the local temporary runner, and CI passed on this revision. I haven’t rerun the full build or local Electron E2E.

中文

看过 68a62a530,之前两项问题都已修复,这次补充改动没有发现新的正确性或可维护性问题。

重新验证了已经选中正常 Agent A 的场景:其他 Agent 的 Retry/Sign in 仍然可用,且不会改变当前选择。fixed 会话也仍然只显示自身的模型和恢复控件。模型列表与恢复操作共用过滤后的 catalog,使两条路径的范围判断保持一致。

原复现场景现在通过,临时本地 runner 中的 25 个 Composer/slot/runtime 定向测试全部通过,当前提交 CI 也已通过。本轮没有重跑完整构建或本地 Electron E2E。

@Astro-Han
Astro-Han marked this pull request as ready for review October 19, 2026 04:50
@Astro-Han
Astro-Han merged commit 2333c1a into apache:main Oct 19, 2026
2 checks passed
@Astro-Han
Astro-Han deleted the refactor/model-picker-unify branch October 19, 2026 04:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/XXL Over 2500 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants