feat(web-shell): complete inline terminal command UI#4710
Conversation
…uto-scroll fix - Parse insight protocol JSON from ACP session into typed messages (insight_progress / insight_ready) and render inline progress bar with spinner matching CLI display - Consolidate multiple progress updates to show only the latest; hide progress bar once the report is ready - Add slash command message rendering: /stats, /model, /memory, /mcp, /agents, /btw, /status, /user-shell with dedicated cards - Fix auto-scroll breaking when tool cards, SubAgent panels, or TodoList cards appear by adding scroll cooldown mechanism - Extend daemon SDK with agent management and MCP workspace APIs - Add slash command completions with inline descriptions
📋 Review SummaryThis is a substantial PR (9220 additions, 68 changed files) that moves slash command flows ( 🔍 General Feedback
🎯 Specific Feedback🔴 Critical
🟡 High
🟢 Medium
🔵 Low
✅ Highlights
|
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Test coverage gaps — several new code paths have zero test coverage:
GET /session/:id/statsendpoint inserver.ts— no tests inserver.test.tsPOST /workspace/mcp/:server/{enable,disable,authenticate,clear-auth}— 4 new mutation endpoints with no testsPOST /workspace/agents/generate— no tests inworkspaceAgents.test.ts- Bridge methods
getSessionStatsStatus,manageMcpServer,generateWorkspaceAgentinbridge.ts— no tests inbridge.test.ts - Insight parsing pipeline (
splitInsightSegments,parseInsightJson,extractJsonObject) intranscriptToMessages.ts— no tests
Additional suggestion: loadMcpTools in packages/webui/src/daemon/workspace/actions.ts has an over-broad catch-all (line 77) that swallows timeouts, auth failures, and network errors, returning a synthetic "daemon does not expose MCP tool details" response. The catch should be narrowed to only the "method not found" case (TypeError), letting other errors propagate with their actual messages.
— qwen3.7-max via Qwen Code /review
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
[Suggestion] Dead code: MemoryDialog.tsx (555 lines), ModelDialog.tsx (304 lines), and McpDialog.tsx (625 lines) are no longer imported or rendered anywhere after this PR moved their functionality to inline MemoryMessage, ModelMessage, and McpStatusMessage components. ~1,484 lines of dead code. Consider deleting them (and their CSS modules) in this PR or a follow-up.
— claude-opus-4-6 via Qwen Code /review
Verification Report — PR #4710Reviewer: wenshao 1. Build
2. Unit Tests
Failing tests (introduced by this PR):
Root cause: 3. Daemon + API Integration (tmux session)Started daemon via
4. Web-Shell Frontend
5. Code Quality Observations
6. Verdict
Recommendation: The 2 test failures are caused by a missing — wenshao |
- Add insight_error protocol type to stop spinner on generation failure - Fix insight_ready id duplication with per-segment counter - Add useEffect cleanup for McpStatusMessage panel active dispatch - Extend MCP OAuth authenticate timeout to 10 minutes - Add TODO for process-wide metrics limitation in stats - Fix AbortController misleading try/finally in ACP agent generation - Add appendLocalUserMessage to /btw and /bug handlers - Add popup blocker check for /bug window.open - Add try/catch + dispatchActionError to getStats() - Replace raw addEventListener with useDelayedGlobalKeyDown in MCP panel - Return generic error in workspaceAgents 500 response - Align description length validation (4096 chars) at HTTP layer - Restore isUserShell to use isShellToolName() for expand button - Use per-server try/catch in /mcp to allow partial failure - Remove unimplemented /mcp completion subcommands - Translate btw.empty to Chinese - Increase virtualizer overscan from 5 to 20
|
Re: Test coverage gaps (review by wenshao) Acknowledged — tests for the new endpoints ( Re: The broad catch is intentional: this code path supports older daemons that don't expose the |
|
Re: Dead code Dialog files (review by wenshao)
|
The web-shell previously used a hardcoded version constant. Pass the resolved CLI package version through the capabilities envelope so clients can display the actual daemon version.
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
Code Review Overview (AI Generated)
PR: #4710 feat(web-shell): complete inline terminal command UI
Type: Feature (large UI + backend expansion)
Change size: +9315/-1050 across 87 files, 6 commits
Multi-Round Review (Rounds 0-6): Clean — 0 findings
Round 0 (Design): Well-scoped feature PR with clear organization. Inline command panels, insight progress, /btw support, slash command completions, user shell separation, stats/MCP management/agent generation APIs, and auto-scroll fix — all coherently implemented following established patterns (approval-mode, model switching, MCP restart).
Round 1 (Architecture):
- Backend routes in
server.tsandworkspaceAgents.tsfollow existing patterns with proper validation (serverName length, description 4096-char limit at HTTP layer, boolean sync flags) - Bridge methods (
getSessionStatsStatus,manageMcpServer,generateWorkspaceAgent) use standardPromise.race+withTimeout+getChannelClosedRejectpattern buildWorkspaceMcpStatuscorrectly changed to async for per-server OAuth token lookups viaMCPOAuthTokenStorage, with non-fatal error handling matching CLI behavior- SDK types (
DaemonSessionStatsStatus,DaemonMcpManageResult, etc.) properly mirror bridge types - Insight protocol parsing (
splitInsightSegments,parseInsightJson,extractJsonObject) correctly handles progress/ready/error message types with dedup
Round 2 (Robustness):
- All wenshao Critical/Major findings verified as fixed in commit 6 (beab7e2):
- insight_ready id uniqueness:
${block.id}-ir-${readyCount++} - insight_error protocol type added for spinner termination
- McpStatusMessage useEffect cleanup:
return () => dispatchActive(id, false) - getStats() try/catch + dispatchActionError matching other actions
- /btw appendLocalUserMessage before runVisibleBtw
- /mcp partial failure: per-server try/catch in Promise.all
- Agent description 4096-char limit at HTTP layer (before 256KB byte-length check)
- workspaceAgents 500 response: generic error string, details only to stderr
- MCP OAuth authenticate timeout: 10 minutes in webUI actions matching bridge budget
- insight_ready id uniqueness:
- MCP manage action validation: enable/disable/authenticate/clear-auth all properly validated with scope-aware settings persistence
- OAuth authenticate uses proper try/finally for appEvents listener cleanup
Round 3 (Security): serverName validated for presence + length, description validated at both HTTP and ACP layers, clientId properly parsed via parseAndValidateWorkspaceClientId, OAuth tokens stored via MCPOAuthTokenStorage with per-server isolation.
Round 4 (Performance): buildWorkspaceMcpStatus uses Promise.all for parallel OAuth lookups across servers. splitInsightSegments short-circuits for non-insight text (zero allocations on normal assistant messages). Virtualizer overscan increased from 5 to 20 for smoother scrolling.
Round 5 (Feature): All features described in PR body are implemented. Known limitations documented: uiTelemetryService process-wide metrics (TODO added), bridgeTypes inline types (circular dependency avoidance), splitInsightSegments performance (not a hot path).
Round 6 (Undirected): Cross-file consistency verified — bridge types match SDK types, ext method constants consistent across status.ts/bridge.ts/acpAgent.ts, timeout budgets aligned between layers.
Outstanding Items (non-blocking, for follow-up)
- Test coverage gaps (5 areas identified by wenshao): stats endpoint, MCP mutation endpoints, agent generation, bridge methods, insight parsing pipeline
- Dead code removal: MemoryDialog.tsx, ModelDialog.tsx, McpDialog.tsx (~1484 lines)
LGTM!
This review was generated by QoderWork AI
Verification ReportEnvironment: macOS Darwin 25.4.0 (Apple Silicon), Node.js, tmux parallel execution Test Results
Total: 986 tests passed across 39 test files, 0 failures. PR-Changed File Test Coverage
Build Observations
Verdict✅ Ready to merge — All 986 tests pass, full build and bundle succeed. The PR is a large web-shell UI overhaul (87 files) with comprehensive test coverage across web-shell, SDK, and webui packages. Verified by: wenshao |
- Fix window.open returning null due to noopener flag (App.tsx) - Use Buffer.byteLength for description length check (workspaceAgents.ts) - Remove stale MCP subcommands from EN slash tree (slashCompletion.ts) - Increase MCP action timeout from 30s to 5min (workspace/actions.ts) - Add counter to insight_error id for uniqueness (transcriptToMessages.ts) - Remove duplicate error reporting in /stats handler (App.tsx) - Fix CSS variable name --color-error to --error-color (MessageItem.tsx) - Remove duplicate echo in /btw command (App.tsx)
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
Re-review at HEAD 259ccef — APPROVE
独立审计 wenshao 两轮 CHANGES_REQUESTED
第一轮（commit e4e65675, 6月2日）— 11 findings:
| Finding | 严重度 | 状态 | 验证 |
|---|---|---|---|
buildSessionStatsStatus 使用进程级 telemetry |
Critical | ✅ Non-blocking | 作者加了 TODO，per-session 需要 core 层改造，超出本 PR 范围 |
/btw 缺少 appendLocalUserMessage |
Suggestion | ✅ Fixed | 已解决：/btw 路径不 append（避免和 BtwMessage 标题重复显示） |
getStats() 缺 try/catch |
Suggestion | ✅ Fixed | HEAD 已有 dispatchActionError 包裹 |
| bridge 内联类型 vs SDK 类型重复 | Suggestion | Won't fix | 合理：引入循环依赖 |
| MCP endpoint 不检查 control chars | Suggestion | Won't fix | 合理：URL path param 不可能出现裸控制字符 |
| description 校验不一致 (chars vs bytes) | Suggestion | ✅ Fixed | HTTP 层增加了 4096-char 限制 |
OAuth authenticate 超时 |
严重 | ✅ Fixed | 10 * 60_000 timeout 匹配 bridge 侧 |
/mcp Promise.all 单 server 失败拖垮面板 |
建议 | ✅ Fixed | 每个 loadMcpTools 独立 try/catch |
splitInsightSegments 性能 O(K·L) |
建议 | Won't fix | 合理：已有 INSIGHT_PREFIXES 短路 |
| McpStatusMessage useEffect 无 cleanup | Critical | ✅ Fixed | 已添加 return () => dispatchActive(id, false) |
| insight parsing 零测试 | Suggestion | Deferred | follow-up material |
第二轮（commit 572a5c0e, 6月3日）— 6 findings:
| Finding | 严重度 | 状态 | 验证 |
|---|---|---|---|
| description.length vs Buffer.byteLength 不一致 | Suggestion | ✅ Fixed | 同上，HTTP 层已加 char limit |
| 非 authenticate actions 使用 30s 默认 timeout | Suggestion | Non-blocking | restart 已有 bridge 侧 MCP_RESTART_DEFAULT_TIMEOUT_MS；enable/disable/clear-auth 是同步操作 |
window.open(noopener) 永远返回 null |
严重 | ✅ Fixed | HEAD App.tsx:1566 已移除 'noopener,noreferrer' 第三参数，改用 win.opener = null |
insight_error React key 重复 |
建议 | ✅ Fixed | HEAD transcriptToMessages.ts:83,98 已加 errorCount++ 计数器 |
--color-error CSS 变量不存在 |
建议 | ✅ Fixed | HEAD MessageItem.tsx:86 已改为 --error-color |
/btw double display |
建议 | ✅ Fixed | /btw 路径不 append，避免重复 |
第三轮（HEAD 259ccef 新增）— 3 findings:
| Finding | 严重度 | 状态 |
|---|---|---|
| EN completion tree 有 stale 子命令 | Suggestion | ✅ Fixed（已移除 nodesc/auth/noauth） |
/stats 双重 error reporting |
建议 | ✅ Fixed（移除了 reportError 调用） |
VS Code companion 不处理 insight_error |
建议 | Deferred（超出本 PR 范围） |
遗留 follow-up 项（均 non-blocking）
- 新 endpoint 测试覆盖（
/session/:id/stats, MCP mutation endpoints, agents endpoints） - Dead code 清理（
MemoryDialog.tsx,ModelDialog.tsx,McpDialog.tsx~1,484 行） insightProtocol单元测试- VS Code companion
insight_error处理
3 participants
Summary
/agents、/memory、/model、/mcp、/stats、/status等 web-shell 命令从弹窗改为消息流内联面板，交互时仍保留上方对话列表可见。/insight流式进度支持，将 ACP insight 协议数据解析为独立的 progress/ready 消息类型，内联展示 spinner 和进度条，仅保留最新进度，并在报告生成后移除进度。/btw支持，抽离BtwMessage和UserShellMessage组件，并限制长/btw回答最高半屏、内容可滚动。Validation
/agents/memory/model/mcp/stats/status/btw <question>!<command>npm run build仅出现已有 VS Code companioncurlywarning 和 Browserslist 数据过期提示。npm run bundle成功复制 sandbox profiles、vendor assets、bundled skills、docs 和 locales 到dist/。/mcp、/agents、/memory、/model、/stats、/status、/btw <question>和!pwd。Scope / Risk
Testing Matrix
Testing matrix notes:
npm run build和npm run bundle验证。npx流程本轮未验证。Linked Issues / Bugs