Repository navigation
refactor(ui): unify composer model selection with Astryx Selector - #6158
Conversation
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
M4n5ter
left a comment
There was a problem hiding this comment.
English
I found two cases where the consolidation seems to drop existing behavior:
-
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?
-
An unavailable Agent has no Retry action before it’s selected — composer-model-picker.tsx:254
Discovery can return an
unavailableentry without settingexecutor.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
onRetrycallback?
I reproduced both with base/PR comparisons. The eight focused tests pass, but don’t cover these two paths.
中文
对照新旧实现时,发现两个原有行为可能丢失的场景:
-
模型插件接管 slot 后,外部 Agent 控件会消失 — composer.tsx:2509
整个 picker 移进 slot 的 fallback 后,Codex 模型插件一旦接管这个 slot,它就不会再渲染。新任务看不到其他已配置的 ACP Agent,已有 ACP 会话也会丢失模型、模式和恢复操作。之前外部 picker 是包在 slot 外面的。
这里能否保留插件替换 slot 时的外部 Agent 控件?
-
尚未选中的不可用 Agent 没有重试入口 — composer-model-picker.tsx:254
发现过程可能返回
unavailable,但不设置executor.error。新任务此时还没有选中 Agent,最终只显示一个禁用选项,没有 Retry;重新打开列表也不会重新发现,临时故障恢复后用户仍无法在这里继续。这里能否保留一个可操作的 Retry,复用现有的
onRetry回调?
两处都做了基线与 PR 的对照复现。现有 8 个定向测试均通过,不过没有覆盖这两个场景。
liugddx
left a comment
There was a problem hiding this comment.
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.
Generated-by: Codex
Generated-by: Codex
M4n5ter
left a comment
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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.
Generated-by: Codex
|
@M4n5ter Fixed the remaining recovery case in 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
left a comment
There was a problem hiding this comment.
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。
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
renderPicker; Composer owns rendering, shared provider logos, native/ACP composition, mutations and pending-send admission.Settings retains its existing Astryx
ModelPickerand 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
chat-model-switcher.tsx,composer-options-menu.tsx,executor-model-picker.tsx,model-picker-panel.tsxandmodel-wheel-picker.tsx, their obsolete styles and exports.The cumulative diff adds 1,042 lines and removes 5,356 across 37 files (net −4,314).
Client integration migration
conversation.composer.model-selectionnow receivesrenderPicker,hasModels,fixedExecutorIdand the current executor target. Contributions callrenderPickerwith their own model catalog instead of rendering replacement controls. The previous full-control owner props andModelWheelPickerclient 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
Agentgroup 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.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-composerstory, fixtures, Chinese locale and 1000 × 900 viewport. Left: separately builtmainat286ffa820; right: this branch at500b83641. Each image contains the complete composer and open popup.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.
AI use
Tool(s) and scope: Codex assisted with component and plugin consolidation, legacy removal, regression tests, Storybook/CDP verification and this description.
Checklist
Does this PR entail a change in behavior?