Feat/acp v1#4741
Merged
Merged
Conversation
- Restore original components to pre-ACP state (ChatHistory, ChatMentionInput, mention-input) - Create ACP-specific components in acp/components/ directory - AcpChatMentionInput: recursive workspace file loading (limit 50) - AcpChatHistory: no delete button (server-managed sessions) - Register ACP components via IChatAgentViewService contribution point - Dynamic component selection in chat.view.tsx based on supportsAgentMode flag - Add design document: docs/plans/2026-04-07-acp-components-refactor.md Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add AcpMCPFooterButton, AcpRulesFooterButton, and AcpSlashCommandFooter components with chat-input-footer registry for extensible footer items. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ibution Move slash command footer item registration from AcpChatMentionInput's useEffect into AcpFooterContribution (ClientAppContribution pattern), and remove MCP/Rules footer items that are no longer needed. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- Replace prepending slash text with cursor-position insertion - Render slash commands as styled tags (non-editable span) instead of plain text - Reuse built-in "/" mention panel instead of separate footer dropdown - Add opensumi-chat-input-open-slash-panel event for external triggers Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ttribute selector - Remove space in nameWithSlash getter: "/ Explain" -> "/Explain" - Fix slash tag serialization by using span[data-command] selector instead of CSS class selector which failed due to CSS modules composes behavior - Remove debug console.log Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…g logs Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Re-add 8 test files covering chat agent service, chat model, diff computer, multi-line decoration, tree-sitter, and MCP servers. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…dropdown When a slash command is selected from the MentionInput dropdown, only a visual DOM tag was inserted but the parent's command state was never updated. This caused AcpChatAgent to receive an empty command string, skipping the custom invoke handler. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Removed 10 verbose console.log/logger.log debug statements and 3 noisy dispose lifecycle logs across the ACP module. Error and warn level logs are preserved. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
… mention input - Remove dead code (unreachable return) and add null-safe fallbacks in acp-session-provider.ts - Remove duplicate `localize` import in ChatMentionInput.acp.tsx Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ing space Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- Remove the slashCommand useEffect in MentionInput that duplicated the slash tag alongside the custom event insertion path - Guard CodeBlockWrapperInput to skip rendering the command prop tag when it was already extracted from the message text Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ervice Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add @opensumi/di mock to prevent DI container errors in unit tests - Make AcpPermissionHandler.initStorage() lazy (ensureInitialized pattern) - Fix auditLog assertion to match actual timestamp format - Fix getSmartTitle assertion for undefined kind Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The else branch in getItems was using a shadowed const folders (string[]) instead of the outer MentionItem[] variable, causing a type error when passed to expandFolderPaths which expects string[]. Renamed the intermediate variable to folderPaths and assigned the expanded result to folders. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
ChatReply: when condition was inverted — button was always rendered and then overwritten. Now properly checks !when or when matches before rendering. Also adds missing AgentProcessConfig import in test. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Add 242 tests across 8 test suites covering: - acp-cli-client.service.ts (NDJSON transport, JSON-RPC) - acp-cli-process-manager.ts (process lifecycle) - acp-permission-caller.service.ts (permission routing, option sorting) - acp-agent.service.ts (session management, notifications) - agent-request.handler.ts (request routing delegation) - file-system.handler.ts (file ops, workspace sandboxing) - terminal.handler.ts (PTY terminal lifecycle) Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The async branch of getChatComponentDeferred omitted messageId, causing inconsistent props compared to the synchronous path. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- Remove unused imports (AcpAgentServiceToken, SimpleMessage) - Remove unused mockStream variable - Fix property shorthand lint error - Add tests for history, images, error handling, and content conversion Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
Walkthrough在浏览器与 Node 侧新增并接入 ACP Agent 模式：实现 NDJSON RPC 客户端、进程管理、会话与消息流；权限桥接与弹窗；文件系统/终端处理器；聊天视图/输入/注册表与默认/ACP 代理；偏好与配置、样式及大量测试。 ChangesACP Agent 模式与权限对话集成
Sequence Diagram(s)sequenceDiagram
participant UI as Browser UI (Chat / PermissionDialog)
participant Bridge as AcpPermissionBridgeService
participant RPC as AcpPermissionRpcService
participant Client as AcpCliClientService
participant NodeHandlers as Agent Handlers (FS/Terminal)
UI->>Bridge: onDidRequestPermission(params)
Bridge->>UI: render dialog
UI->>Bridge: user decision
Bridge->>RPC: $showPermissionDialog(params)
RPC->>Client: JSON-RPC -> agent process
Client->>NodeHandlers: handleRead/Write/CreateTerminal...
NodeHandlers-->>Client: result/error
Client-->>RPC: notification / session updates
RPC-->>UI: stream/notification for chat/permission
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ minutes Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/ai-native/src/browser/layout/ai-layout.tsx (1)
25-42:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winReact Hooks 规则违反：条件返回后调用 useMemo
在第 25-36 行的条件返回之后，第 39 行调用了
useMemo，这违反了 React Hooks 的规则。Hooks 必须在组件顶层调用，不能在条件返回之后。这会导致在移动端和桌面端之间切换时，hooks 调用顺序不一致，可能引发运行时错误。🐛 建议的修复方案
export const AILayout = () => { const { layout } = getStorageValue(); const designLayoutConfig = useInjectable(DesignLayoutConfig); - // 判断是否应该显示完整布局 - const shouldShowFullLayout = !isMobileDevice(); - - // 移动端模式：只渲染 AI_CHAT_VIEW_ID，添加 mobile class - if (!shouldShowFullLayout) { - return ( - <SlotRenderer - slot={AI_CHAT_VIEW_ID} - isTabbar={true} - defaultSize={layout['AI-Chat']?.currentId ? layout['AI-Chat']?.size || 360 : 0} - maxResize={420} - minResize={280} - minSize={0} - /> - ); - } - - // 正常模式：渲染完整布局 const defaultRightSize = useMemo( () => (designLayoutConfig.useMergeRightWithLeftPanel ? 0 : 49), [designLayoutConfig.useMergeRightWithLeftPanel], ); + // 判断是否应该显示完整布局 + const shouldShowFullLayout = !isMobileDevice(); + + // 移动端模式：只渲染 AI_CHAT_VIEW_ID + if (!shouldShowFullLayout) { + return ( + <SlotRenderer + slot={AI_CHAT_VIEW_ID} + isTabbar={true} + defaultSize={layout['AI-Chat']?.currentId ? layout['AI-Chat']?.size || 360 : 0} + maxResize={420} + minResize={280} + minSize={0} + /> + ); + } + + // 正常模式：渲染完整布局 return ( <BoxPanel direction='top-to-bottom'>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ai-native/src/browser/layout/ai-layout.tsx` around lines 25 - 42, The hook call useMemo for defaultRightSize is placed after an early conditional return (when !shouldShowFullLayout), violating React Hooks rules; move the useMemo call to the top level of the component (before the conditional return) so hooks run unconditionally, e.g., compute defaultRightSize using useMemo(designLayoutConfig.useMergeRightWithLeftPanel) above the if (!shouldShowFullLayout) block and then reference defaultRightSize inside both the SlotRenderer return and the full-layout rendering; ensure all other hooks follow the same top-level placement to keep hook order stable (symbols: shouldShowFullLayout, SlotRenderer, AI_CHAT_VIEW_ID, defaultRightSize, useMemo, designLayoutConfig.useMergeRightWithLeftPanel).
🟡 Minor comments (8)
packages/ai-native/src/browser/acp/components/AcpChatMentionInput.tsx-498-535 (1)
498-535:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRule 搜索逻辑与排序条件不一致，描述匹配永远不会生效。
当前先按
rule.text过滤，导致仅命中 description 的规则被提前丢弃。建议修复
- return mappedRules - .filter((rule) => rule.text.toLocaleLowerCase().includes(lowerSearchText)) + return mappedRules + .filter((rule) => { + const text = rule.text.toLocaleLowerCase(); + const desc = (rule.description || '').toLocaleLowerCase(); + return text.includes(lowerSearchText) || desc.includes(lowerSearchText); + })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ai-native/src/browser/acp/components/AcpChatMentionInput.tsx` around lines 498 - 535, The current filtering only checks rule.text so items that match only in description are dropped; update the filter used on mappedRules (the block using lowerSearchText and .filter(...)) to include rules where either rule.text or rule.description (use toLocaleLowerCase safely, defaulting to '') includes lowerSearchText, and keep the existing sort logic that already checks aDescMatch/bDescMatch so description matches can be promoted; ensure you reference searchText, lowerSearchText, mappedRules, and the comparator that computes aTextMatch/aDescMatch/bTextMatch/bDescMatch when making the change.packages/ai-native/src/browser/acp/components/AcpChatInput.tsx-74-106 (1)
74-106:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win指令候选列表可能不会随选中 Agent 变化。
options计算使用了selectedAgentId和chatAgentViewService，但依赖数组缺少这两项，导致过滤结果不会随选中 Agent 的变化而更新。建议修复
- }, [trigger, chatAgentService]); + }, [trigger, chatAgentService, chatAgentViewService, selectedAgentId]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ai-native/src/browser/acp/components/AcpChatInput.tsx` around lines 74 - 106, The useMemo that computes options (function options in AcpChatInput.tsx) is missing dependencies so the candidate list won't update when selectedAgentId or chatAgentViewService changes; update the dependency array of the useMemo to include selectedAgentId and chatAgentViewService (and keep chatAgentService and trigger) so getRenderAgents() and the filter based on selectedAgentId re-run when those values change; ensure the dependencies cover getRenderAgents/getCommands if they are methods on objects that may change.packages/ai-native/src/browser/acp/permission-dialog-container.tsx-300-300 (1)
300-300:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winCSS 变量表达式括号不闭合
Line 300 的
backgroundColor少了一个右括号，最终 CSS 值非法，背景色不会按预期生效。建议修改
- backgroundColor: 'var(--kt-popover-background, var(--popover-background, var(--app-background))', + backgroundColor: 'var(--kt-popover-background, var(--popover-background, var(--app-background)))',🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ai-native/src/browser/acp/permission-dialog-container.tsx` at line 300, The inline style for backgroundColor in permission-dialog-container (the style object where backgroundColor is set to 'var(--kt-popover-background, var(--popover-background, var(--app-background))') has a missing closing parenthesis making the CSS var expression invalid; fix it by adding the final right parenthesis so the nested var() calls are properly closed (update the backgroundColor value in the style object in PermissionDialogContainer / permission-dialog-container.tsx).packages/ai-native/src/browser/chat/chat-input-footer.registry.ts-25-48 (1)
25-48:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win同一
id重复注册会累积旧 disposable。当前替换旧条目时只
splice了贡献项，但不会释放上一轮注册句柄；频繁刷新贡献项会导致 registry 内部 disposable 持续增长。建议修改
export class ChatInputFooterRegistry extends Disposable implements IChatInputFooterRegistry { private contributions: ChatInputFooterContribution[] = []; + private contributionDisposables = new Map<string, IDisposable>(); private readonly onDidChangeEmitter = new Emitter<void>(); readonly onDidChange: Event<void> = this.onDidChangeEmitter.event; registerFooterItem(id: string, item: ChatInputFooterItem): IDisposable { + this.contributionDisposables.get(id)?.dispose(); const existing = this.contributions.findIndex((c) => c.id === id); if (existing !== -1) { this.contributions.splice(existing, 1); } ... const disposable = Disposable.create(() => { const idx = this.contributions.indexOf(entry); if (idx !== -1) { this.contributions.splice(idx, 1); this.onDidChangeEmitter.fire(); } + this.contributionDisposables.delete(id); }); + this.contributionDisposables.set(id, disposable); this.addDispose(disposable);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ai-native/src/browser/chat/chat-input-footer.registry.ts` around lines 25 - 48, registerFooterItem replaces an existing contribution entry in this.contributions but never disposes the previous registration's disposable, causing disposables to accumulate; update registerFooterItem (and related data structures) to track the disposable per contribution (e.g., keep a Map<string, IDisposable> or attach the disposable to the ChatInputFooterContribution entry) and when an existing id is found dispose the previous disposable (call .dispose()) before splicing and overwriting the entry, then register and store the new disposable via Disposable.create and this.addDispose as before.packages/ai-native/src/browser/chat/chat.internal.service.acp.ts-139-143 (1)
139-143:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
dispose()漏掉_onAvailableCommandsChange的释放。当前未释放该 emitter，可能残留订阅。请在销毁时一并 dispose。
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ai-native/src/browser/chat/chat.internal.service.acp.ts` around lines 139 - 143, The dispose() override is missing disposal of the _onAvailableCommandsChange emitter; update the override in chat.internal.service.acp.ts (the dispose() method) to call this._onAvailableCommandsChange.dispose() alongside this._onModeChange.dispose(), this._onSessionLoadingChange.dispose(), and this._onSessionModelChange.dispose() (before calling super.dispose()) so the emitter is properly cleaned up and no subscriptions remain.packages/ai-native/src/browser/components/acp/mention-input.module.less-3-4 (1)
3-4:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win存在空样式块，触发 stylelint
block-no-empty建议删除空的
.popover_icon {}，或补充实际样式，避免流水线失败。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ai-native/src/browser/components/acp/mention-input.module.less` around lines 3 - 4, 删除或填充空的样式块以修复 stylelint 的 block-no-empty 错误：在 mention-input.module.less 中找到空选择器 .popover_icon 并要么直接移除该空规则，要么为 .popover_icon 添加实际样式（如尺寸、display、margin 等）以避免空块；提交时确保不再保留空的大括号以通过流水线检查。packages/ai-native/src/browser/components/ChatMentionInput.acp.tsx-401-404 (1)
401-404:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win规则搜索过滤条件与排序意图不一致
当前先按
rule.text过滤，导致“按描述命中”的规则在排序前已被剔除，描述匹配逻辑无法生效。Also applies to: 407-435
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ai-native/src/browser/components/ChatMentionInput.acp.tsx` around lines 401 - 404, The filter currently only checks rule.text against lowerSearchText, removing items that might match rule.description before sorting; update the filtering to include both rule.text and rule.description (e.g., check if either toLocaleLowerCase contains lowerSearchText) and then adjust the sort comparator in the mappedRules.sort call to rank exact/partial matches in rule.text higher than matches in rule.description (fall back to other tie-breakers like name or original order). Apply the same filtering+sorting change to the other occurrence covering lines 407-435 so both blocks use the unified match-on-text-or-description and sort-by-relevance logic.packages/ai-native/src/browser/components/ChatHistory.acp.tsx-120-139 (1)
120-139:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win时间分组文案未走国际化
这里直接返回英文字符串，会在非英文语言环境下出现混杂文案。建议改为
localizekey。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ai-native/src/browser/components/ChatHistory.acp.tsx` around lines 120 - 139, getTimeKey currently returns hard-coded English strings; change it to return localization keys and/or formatted localized messages instead. Update the getTimeKey function to call the project's localization utility (e.g., localize or useIntl.formatMessage) for each case (Just now, Xm ago, Xh ago, Xd ago, Xw ago, Xmo ago, Xy ago), supplying the numeric value as a variable where needed, and replace direct string literals inside getTimeKey with calls like localize('chat.time.justNow') or localize('chat.time.minutesAgo', { minutes }) so the UI uses translated text across locales.
🧹 Nitpick comments (8)
packages/ai-native/__test__/node/acp-cli-process-manager.test.ts (1)
1-228: ⚡ Quick win该测试套与另一份
CliAgentProcessManager用例高度重复，建议合并。与
packages/ai-native/__tests__/node/acp/cli-agent-process-manager.test.ts存在大量同类场景（start/stop/kill/isRunning/wrapError 等）重复，后续容易漂移并增加 CI 耗时。建议保留一套主测试并把另一套改为差异化场景或删除。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ai-native/__test__/node/acp-cli-process-manager.test.ts` around lines 1 - 228, This test file largely duplicates scenarios already covered in the other test suite for CliAgentProcessManager; consolidate to one canonical test and remove or convert this file to only cover unique edge cases. Specifically, keep a single comprehensive test file exercising CliAgentProcessManager's core behaviors (startAgent, stopAgent, killAgent, isRunning, getExitCode, listRunningAgents, killAllAgents, handleProcessExit, killProcessGroup, wrapError) and either delete this duplicate file or refactor it into a small focused file that only tests the differing scenarios not present in packages/ai-native/__tests__/node/acp/cli-agent-process-manager.test.ts (e.g., unique error paths or race conditions), ensuring references to createMockChildProcess, mockSpawn, and the class CliAgentProcessManager remain intact while removing overlapping test cases.packages/ai-native/__test__/node/acp-cli-client.test.ts (1)
446-446: ⚡ Quick win避免使用固定延时等待异步处理，当前用例有抖动风险。
Line 446、Line 461、Line 478 依赖
setTimeout(10)等待请求处理完成，在 CI 负载波动时容易出现偶发失败。建议改为基于明确事件/回调完成信号的等待（例如等待mockStdin.write被调用的 Promise）。Also applies to: 461-461, 478-478
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ai-native/__test__/node/acp-cli-client.test.ts` at line 446, Replace the fragile fixed delays (await new Promise(r => setTimeout(r, 10))) with a deterministic wait that resolves when the stdin write actually occurs: wrap or replace mockStdin.write with a small helper that returns/fulfills a Promise (e.g., create a let writeDone; const writePromise = new Promise(res => writeDone = res); then set mockStdin.write = (...args) => { originalWrite(...args); writeDone(); }; await writePromise) and use that in place of the setTimeout calls so the test waits for the actual mockStdin.write invocation rather than a fixed timeout.packages/ai-native/src/browser/chat/chat-agent.service.ts (1)
148-148: ⚡ Quick win错误信息建议改为输出可用 agent id 列表。
${this.agents}可读性很低，排障价值有限。建议直接输出keys()，便于快速定位注册状态。建议修改
- throw new Error(`No agent with id ${id},this.agents ${this.agents}`); + throw new Error( + `No agent with id ${id}, available agents: ${Array.from(this.agents.keys()).join(', ')}`, + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ai-native/src/browser/chat/chat-agent.service.ts` at line 148, The thrown error currently interpolates this.agents (low readability); change the error to list available agent ids by extracting the keys from this.agents (e.g., using this.agents.keys() or Array.from(this.agents.keys())) so the message shows the missing id and a readable list of registered agent ids; update the throw site that references id and this.agents to include that keys list in the error text.packages/ai-native/src/browser/components/permission-dialog-widget.tsx (1)
127-127: ⚡ Quick winLine 127 建议与模块样式对齐使用本地 class。
如果按样式文件中的修复改为
.focused本地类，这里应改为styles.focused，避免依赖全局类名。建议修复
- className={cls(styles.option_button, isFocused && 'focused')} + className={cls(styles.option_button, isFocused && styles.focused)}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ai-native/src/browser/components/permission-dialog-widget.tsx` at line 127, 在 PermissionDialogWidget 里将本地样式用于聚焦状态：把 className={cls(styles.option_button, isFocused && 'focused')} 改为使用本地类名 styles.focused（即 cls(styles.option_button, isFocused && styles.focused)），以避免引用全局类并与样式模块保持一致；定位相关代码可通过 className 属性、styles.option_button、isFocused 标志和 styles.focused 标识符查找并替换。packages/ai-native/src/node/acp/acp-agent.service.ts (2)
522-524: ⚡ Quick win错误被静默吞掉，建议记录日志
cancelRequest方法中的 catch 块为空，错误被完全忽略。即使取消请求失败不应阻断流程，也建议记录日志以便调试。♻️ 建议的修复
try { await this.clientService.cancel(cancelNotification); - } catch (error) {} + } catch (error) { + this.logger?.warn('[AcpAgentService] cancelRequest failed:', error); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ai-native/src/node/acp/acp-agent.service.ts` around lines 522 - 524, The catch block swallowing errors after calling this.clientService.cancel(cancelNotification) should log the error instead of being empty; inside the cancelRequest (or the method containing that try/catch) replace the empty catch with a logging call (e.g., this.logger.error(...) or, if no instance logger exists, console.error(...)) that includes a clear message and the caught error/stack plus context (e.g., the cancelNotification or request id) so failures to cancel are recorded but still do not throw.
213-213: 🏗️ Heavy lift使用固定延时等待通知是不可靠的模式
第 213 行和第 342 行使用固定延时（2000ms 和 500ms）等待异步通知到达。这种模式不可靠：在高负载或慢网络下可能超时，在快速响应时又浪费时间。建议改用基于事件的方式，等待特定通知或超时。
Also applies to: 342-342
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ai-native/src/node/acp/acp-agent.service.ts` at line 213, Replace the unreliable fixed-delay waits (the occurrences of "await new Promise((resolve) => setTimeout(resolve, 2000));" and the 500ms variant) with an event-driven wait: implement or use a helper like waitForNotification(notificationType, predicate?, timeoutMs) that returns a Promise which resolves when the specific notification/event is emitted (or the predicate matches) and rejects or resolves false on timeout; inside the existing flow (where those awaits are now) attach a one-time listener to the notification emitter, resolve the promise when the expected notification arrives, ensure you remove the listener on resolve/timeout to avoid leaks, and use a reasonable timeout fallback instead of always sleeping.packages/ai-native/src/browser/layout/ai-layout.tsx (1)
1-1: 💤 Low value移除未使用的导入
useEffect和useState已导入但未在组件中使用。♻️ 建议的修复
-import React, { useEffect, useMemo, useState } from 'react'; +import React, { useMemo } from 'react';🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ai-native/src/browser/layout/ai-layout.tsx` at line 1, Remove the unused imports useEffect and useState from the React import in ai-layout.tsx: update the import line (currently "import React, { useEffect, useMemo, useState } from 'react';") to only include the actually used symbols (e.g., "import React, { useMemo } from 'react';"), ensuring no other references to useEffect/useState remain in the file.packages/ai-native/src/common/prompts/empty-prompt-provider.ts (1)
68-71: ⚡ Quick win重复的异步调用导致性能浪费
getACPCurrentFileInfo()在provideContextPrompt(第 21 行) 和buildACPPrompt(第 68 行) 中被重复调用。由于这是一个异步方法，涉及编辑器状态访问，建议将结果作为参数传递以避免重复调用。♻️ 建议的重构
async provideContextPrompt(context: SerializedContext, userMessage: string): Promise<string> { const hasContextFields = context.globalRules.length > 0 || context.attachedFolders.length > 0 || context.attachedFiles.length > 0 || context.attachedRules.length > 0; + const currentFileInfo = await this.getACPCurrentFileInfo(); + if (!hasContextFields) { - const currentFileInfo = await this.getACPCurrentFileInfo(); const hasCurrentFile = currentFileInfo && !context.attachedFiles.some((file) => file.path === currentFileInfo.path); if (!hasCurrentFile) { return userMessage; } } - return this.buildACPPrompt(context, userMessage); + return this.buildACPPrompt(context, userMessage, currentFileInfo); } - private async buildACPPrompt(context: SerializedContext, userMessage: string): Promise<string> { + private buildACPPrompt( + context: SerializedContext, + userMessage: string, + currentFileInfo: Awaited<ReturnType<typeof this.getACPCurrentFileInfo>> + ): string { const sections: string[] = []; // ... rest of the method - let currentFileInfo = await this.getACPCurrentFileInfo(); + let effectiveFileInfo = currentFileInfo; if (currentFileInfo && context.attachedFiles.some((file) => file.path === currentFileInfo!.path)) { - currentFileInfo = null; + effectiveFileInfo = null; } - if (currentFileInfo) { - let currentFileSection = `Current file: ${currentFileInfo.path}`; - if (currentFileInfo.currentLine && currentFileInfo.lineContent) { - currentFileSection += ` (line ${currentFileInfo.currentLine}: \`${currentFileInfo.lineContent}\`)`; + if (effectiveFileInfo) { + let currentFileSection = `Current file: ${effectiveFileInfo.path}`; + if (effectiveFileInfo.currentLine && effectiveFileInfo.lineContent) { + currentFileSection += ` (line ${effectiveFileInfo.currentLine}: \`${effectiveFileInfo.lineContent}\`)`; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ai-native/src/common/prompts/empty-prompt-provider.ts` around lines 68 - 71, The code calls the async method getACPCurrentFileInfo() twice (in provideContextPrompt and buildACPPrompt); refactor so the caller obtains currentFileInfo once and passes it into buildACPPrompt (or have provideContextPrompt accept a currentFileInfo param) to avoid duplicate async/editor access; update signatures for buildACPPrompt (and any intermediary calls) to accept the pre-fetched currentFileInfo and remove the second getACPCurrentFileInfo() call (referencing provideContextPrompt, buildACPPrompt, and getACPCurrentFileInfo).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 21ecf915-8f11-411a-a9f2-f2ac6832f8b3
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (107)
package.jsonpackages/ai-native/__test__/browser/acp/acp-permission-rpc.service.test.tspackages/ai-native/__test__/browser/acp/permission-bridge.service.test.tspackages/ai-native/__test__/browser/acp/permission-dialog-container.test.tspackages/ai-native/__test__/browser/acp/permission.handler.test.tspackages/ai-native/__test__/browser/chat/chat-model.test.tspackages/ai-native/__test__/node/acp-agent-request-handler.test.tspackages/ai-native/__test__/node/acp-agent.service.test.tspackages/ai-native/__test__/node/acp-cli-back.test.tspackages/ai-native/__test__/node/acp-cli-client.test.tspackages/ai-native/__test__/node/acp-cli-process-manager.test.tspackages/ai-native/__test__/node/acp-file-system-handler.test.tspackages/ai-native/__test__/node/acp-permission-caller.test.tspackages/ai-native/__test__/node/acp-terminal-handler.test.tspackages/ai-native/__tests__/node/acp/cli-agent-process-manager.test.tspackages/ai-native/package.jsonpackages/ai-native/src/browser/acp/acp-permission-rpc.service.tspackages/ai-native/src/browser/acp/components/AcpChatHistory.tsxpackages/ai-native/src/browser/acp/components/AcpChatInput.tsxpackages/ai-native/src/browser/acp/components/AcpChatMentionInput.tsxpackages/ai-native/src/browser/acp/components/AcpChatViewHeader.tsxpackages/ai-native/src/browser/acp/components/AcpChatViewWrapper.tsxpackages/ai-native/src/browser/acp/components/AcpFooterButtons.tsxpackages/ai-native/src/browser/acp/components/AcpFooterContribution.tspackages/ai-native/src/browser/acp/index.tspackages/ai-native/src/browser/acp/permission-bridge.service.tspackages/ai-native/src/browser/acp/permission-dialog-container.module.lesspackages/ai-native/src/browser/acp/permission-dialog-container.tsxpackages/ai-native/src/browser/acp/permission-dialog.module.lesspackages/ai-native/src/browser/acp/permission-dialog.view.tsxpackages/ai-native/src/browser/acp/permission.handler.tspackages/ai-native/src/browser/ai-core.contribution.tspackages/ai-native/src/browser/chat/acp-chat-agent.tspackages/ai-native/src/browser/chat/acp-session-provider.tspackages/ai-native/src/browser/chat/chat-agent.service.tspackages/ai-native/src/browser/chat/chat-input-footer.registry.tspackages/ai-native/src/browser/chat/chat-manager.service.acp.tspackages/ai-native/src/browser/chat/chat-manager.service.tspackages/ai-native/src/browser/chat/chat-model.tspackages/ai-native/src/browser/chat/chat-proxy.service.acp.tspackages/ai-native/src/browser/chat/chat-proxy.service.tspackages/ai-native/src/browser/chat/chat.history.registry.tspackages/ai-native/src/browser/chat/chat.input.registry.tspackages/ai-native/src/browser/chat/chat.internal.service.acp.tspackages/ai-native/src/browser/chat/chat.internal.service.tspackages/ai-native/src/browser/chat/chat.module.lesspackages/ai-native/src/browser/chat/chat.render.registry.tspackages/ai-native/src/browser/chat/chat.view.acp.tsxpackages/ai-native/src/browser/chat/chat.view.registry.tspackages/ai-native/src/browser/chat/default-acp-config-provider.tspackages/ai-native/src/browser/chat/default-chat-agent.tspackages/ai-native/src/browser/chat/get-default-agent-type.tspackages/ai-native/src/browser/chat/local-storage-provider.tspackages/ai-native/src/browser/chat/pick-workspace-dir.tspackages/ai-native/src/browser/chat/session-provider-registry.tspackages/ai-native/src/browser/chat/session-provider.tspackages/ai-native/src/browser/components/ChatEditor.tsxpackages/ai-native/src/browser/components/ChatHistory.acp.tsxpackages/ai-native/src/browser/components/ChatMentionInput.acp.tsxpackages/ai-native/src/browser/components/acp/ChatReply.tsxpackages/ai-native/src/browser/components/acp/MentionInput.tsxpackages/ai-native/src/browser/components/acp/chat-history.module.lesspackages/ai-native/src/browser/components/acp/mention-input.module.lesspackages/ai-native/src/browser/components/acp/types.tspackages/ai-native/src/browser/components/components.module.lesspackages/ai-native/src/browser/components/mention-input/mention-input.module.lesspackages/ai-native/src/browser/components/mention-input/mention-input.tsxpackages/ai-native/src/browser/components/mention-input/mention-item.tsxpackages/ai-native/src/browser/components/mention-input/types.tspackages/ai-native/src/browser/components/permission-dialog-widget.module.lesspackages/ai-native/src/browser/components/permission-dialog-widget.tsxpackages/ai-native/src/browser/contrib/terminal/ai-terminal.service.tspackages/ai-native/src/browser/contrib/terminal/decoration/terminal-decoration.tsxpackages/ai-native/src/browser/index.tspackages/ai-native/src/browser/layout/ai-layout.tsxpackages/ai-native/src/browser/mcp/base-apply.service.tspackages/ai-native/src/browser/preferences/schema.tspackages/ai-native/src/browser/types.tspackages/ai-native/src/common/index.tspackages/ai-native/src/common/prompts/empty-prompt-provider.tspackages/ai-native/src/node/acp/acp-agent.service.tspackages/ai-native/src/node/acp/acp-cli-back.service.tspackages/ai-native/src/node/acp/acp-cli-client.service.tspackages/ai-native/src/node/acp/acp-permission-caller.service.tspackages/ai-native/src/node/acp/cli-agent-process-manager.tspackages/ai-native/src/node/acp/handlers/agent-request.handler.tspackages/ai-native/src/node/acp/handlers/constants.tspackages/ai-native/src/node/acp/handlers/file-system.handler.tspackages/ai-native/src/node/acp/handlers/terminal.handler.tspackages/ai-native/src/node/acp/index.tspackages/ai-native/src/node/index.tspackages/core-browser/src/ai-native/ai-config.service.tspackages/core-common/package.jsonpackages/core-common/src/log.tspackages/core-common/src/settings/ai-native.tspackages/core-common/src/storage.tspackages/core-common/src/types/ai-native/acp-types.tspackages/core-common/src/types/ai-native/agent-types.tspackages/core-common/src/types/ai-native/index.tspackages/file-service/package.jsonpackages/i18n/src/common/en-US.lang.tspackages/i18n/src/common/zh-CN.lang.tspackages/startup/entry/sample-modules/ai-native/WelcomePage.module.lesspackages/startup/entry/sample-modules/ai-native/WelcomePage.tsxpackages/startup/entry/sample-modules/ai-native/ai-native.contribution.tspackages/startup/entry/web/server.tstools/playwright/src/tests/search-view.test.ts
💤 Files with no reviewable changes (1)
- packages/ai-native/src/browser/components/mention-input/mention-input.module.less
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4741 +/- ##
==========================================
- Coverage 52.96% 52.92% -0.05%
==========================================
Files 1686 1720 +34
Lines 105031 107381 +2350
Branches 22996 23525 +529
==========================================
+ Hits 55634 56833 +1199
- Misses 41043 42023 +980
- Partials 8354 8525 +171
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Types
Background or solution
Changelog
Summary by CodeRabbit
新增功能
测试