Skip to content

fix(cli): fix Space key not working in Arena model selection dialog#4701

Merged
yiliang114 merged 1 commit into
QwenLM:mainfrom
ZijianZhang989:fix/arena-space-key-selection
Jun 3, 2026
Merged

fix(cli): fix Space key not working in Arena model selection dialog#4701
yiliang114 merged 1 commit into
QwenLM:mainfrom
ZijianZhang989:fix/arena-space-key-selection

Conversation

@ZijianZhang989
Copy link
Copy Markdown
Collaborator

@ZijianZhang989 ZijianZhang989 commented Jun 2, 2026

What this PR does

Fix Space key not toggling model checkboxes in the /arena start model selection dialog. Added controlled state (selectedKeys + onSelectedKeysChange) to the MultiSelect component in ArenaStartDialog, so Space key toggles now correctly persist and update the UI.

Why it's needed

When a user runs /arena start without --models, a model selection dialog appears. The prompt says "Space to toggle, Enter to confirm", but pressing Space does nothing — checkboxes never toggle. This is because MultiSelect is a controlled component that requires external state management via selectedKeys and onSelectedKeysChange props, but ArenaStartDialog was not providing them. Without these props, onSelectedKeysChange is undefined, so the toggle call silently fails and the UI never updates.

Reviewer Test Plan

How to verify

  1. Run qwen in any git repository
  2. Type /arena start (without --models flag)
  3. The model selection dialog appears with a list of configured models
  4. Press Space on a model → checkbox should toggle between [ ] and [✓]
  5. Select 2+ models, press Enter → Arena session starts normally with the selected models

Evidence (Before & After)

Before: Pressing Space in the model selection dialog does nothing. Checkboxes remain unchecked regardless of key presses.

After: Pressing Space correctly toggles the checkbox for the highlighted model. Multiple models can be selected and confirmed with Enter.

Tested on

OS Status
🍏 macOS
🪟 Windows ⚠️
🐧 Linux ⚠️

Environment (optional)

Local runtime: npm run bundlenode dist/cli.js

Risk & Scope

  • Main risk or tradeoff: None — this is a minimal 3-line fix adding missing props to an existing component.
  • Not validated / out of scope: Windows and Linux not locally tested, but the fix is platform-independent React state management.
  • Breaking changes / migration notes: None.

Linked Issues

Fixes #4692

中文说明

这个 PR 做了什么

修复了 /arena start 模型选择对话框中 Space 键无法切换复选框的问题。为 ArenaStartDialog 中的 MultiSelect 组件添加了受控状态(selectedKeys + onSelectedKeysChange),使 Space 键切换能正确持久化并更新 UI。

为什么需要这个修复

当用户运行 /arena start(不带 --models 参数)时,会弹出模型选择对话框。提示文字写着"Space to toggle, Enter to confirm",但按下 Space 键什么都不会发生——复选框永远不会切换。这是因为 MultiSelect 是一个受控组件,需要通过 selectedKeysonSelectedKeysChange props 进行外部状态管理,但 ArenaStartDialog 没有提供这些 props。缺少这些 props 时,onSelectedKeysChangeundefined,切换调用静默失败,UI 永远不会更新。

审阅者测试计划

如何验证

  1. 在任意 git 仓库中运行 qwen
  2. 输入 /arena start(不带 --models 参数)
  3. 模型选择对话框出现,显示已配置的模型列表
  4. 按 Space 键 → 复选框应在 [ ][✓] 之间切换
  5. 选择 2 个以上模型,按 Enter → Arena 会话正常启动

测试环境

本地运行:npm run bundlenode dist/cli.js

风险和范围

  • 主要风险:无——这是一个最小的 3 行修复,仅为现有组件添加缺失的 props。
  • 未验证/超出范围:Windows 和 Linux 未在本地测试,但修复内容是与平台无关的 React 状态管理。
  • 破坏性变更/迁移说明:无。

关联 Issue

修复 #4692

The ArenaStartDialog's MultiSelect component was missing the
selectedKeys and onSelectedKeysChange props, causing Space key
toggles to have no effect when selecting models for an Arena session.

Fixes QwenLM#4692
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 2, 2026

📋 Review Summary

This PR fixes a bug where the Space key didn't toggle model checkboxes in the Arena model selection dialog. The fix is minimal and targeted - adding controlled component state management to ArenaStartDialog and passing the required props to MultiSelect. The implementation correctly follows the existing pattern used by the MultiSelect component.

🔍 General Feedback

  • The fix is appropriately minimal - only 3 lines added, no unnecessary changes
  • Correctly identifies the root cause: MultiSelect is a controlled component requiring selectedKeys and onSelectedKeysChange props
  • Follows the existing React pattern already established in the codebase (useState for state management)
  • The test plan is clear and covers the critical interaction paths

🎯 Specific Feedback

No specific issues identified in this review.

✅ Highlights

  • Excellent root cause analysis in the PR description - clearly explains why the Space key wasn't working (controlled component with undefined callback)
  • Minimal change scope - only touches the necessary component with no ripple effects
  • Proper use of TypeScript types - selectedKeys correctly typed as string[] matching the MultiSelectProps interface
  • The fix aligns with how MultiSelect is designed to work (see line 47-48 in MultiSelect.tsx where it expects these props)
  • Good test coverage in the test plan: Space key toggle, multi-selection, and Enter confirmation

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Fixes the /arena start model selection dialog so that pressing Space correctly toggles model checkboxes by wiring ArenaStartDialog up to MultiSelect’s controlled selection state.

Changes:

  • Add local selectedKeys state in ArenaStartDialog.
  • Pass selectedKeys and onSelectedKeysChange to MultiSelect so Space toggles persist and re-render.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@ZijianZhang989
Copy link
Copy Markdown
Collaborator Author

2026-06-03.10.28.43.mp4
2026-06-03.10.31.10.mp4

@tanzhenxin tanzhenxin added the type/bug Something isn't working as expected label Jun 3, 2026
Copy link
Copy Markdown
Collaborator

@yiliang114 yiliang114 left a comment

Choose a reason for hiding this comment

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

Root cause is clear — MultiSelect is a controlled component and ArenaStartDialog wasn't providing selectedKeys/onSelectedKeysChange, so toggles silently no-oped. The fix matches the existing pattern in StatusLineDialog. LGTM.

@yiliang114
Copy link
Copy Markdown
Collaborator

yiliang114 commented Jun 3, 2026

@ZijianZhang989 Welcome to the project and thanks for your first contribution! 🎉

Great job on the clean fix and the detailed PR description. Looking forward to seeing more from you!

Copy link
Copy Markdown
Collaborator

@tanzhenxin tanzhenxin left a comment

Choose a reason for hiding this comment

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

Review

This fixes the Space key not toggling model checkboxes in the /arena start model selection dialog. The underlying multi-select component was refactored a while back from managing its own selection internally to being fully controlled by its parent — selection now lives entirely in props the parent must supply. The arena start dialog was never updated for that change, so it rendered the list without wiring up the selection state, leaving every checkbox permanently empty and the Space toggle a silent no-op. The fix adds the missing local selection state and passes it through, matching the same pattern the status-line dialog already uses.

I verified the root cause against git history and traced the selection end-to-end: toggled models now persist in the dialog's state and flow through to /arena start on Enter. The empty-selection start, the "select at least 2 models" guard, and the skip-disabled-models behavior are all preserved, so there's no regression. The only gap is the absence of a regression test for this dialog — not a blocker for a three-line fix, but worth noting since this exact class of bug (a consumer left un-wired after the component became controlled) has now shipped once.

Verdict

APPROVE — Correct, minimal fix that matches the component's controlled-selection contract; selected models provably reach arena start and no edge case or regression is introduced.

@yiliang114 yiliang114 merged commit ca1bc36 into QwenLM:main Jun 3, 2026
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type/bug Something isn't working as expected

Projects

None yet

Development

Successfully merging this pull request may close these issues.

/arena cna't type space to select model

4 participants