feat: support VSCode API: env.onDidChangeShell #4223
Conversation
bk1012
changed the title
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
warning eslint@8.57.1: This version is no longer supported. Please see https://eslint.org/version-support for other options. Walkthrough此拉取请求引入了一系列与终端事件处理相关的更改。主要包括在 Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
packages/terminal-next/src/browser/terminal.profile.ts (2)
89-96: 建议添加资源清理逻辑！preference 监听器的实现逻辑正确，但建议在服务销毁时清理监听器以防止内存泄漏。
private setupPreferenceListener() { - this.preferenceService.onPreferenceChanged((event: PreferenceChange) => { + const disposable = this.preferenceService.onPreferenceChanged((event: PreferenceChange) => { if (event.preferenceName === TerminalSettingsId.Type) { this.refreshAvailableProfiles(); } }); + this.addDispose(disposable); }
90-93: 建议添加错误处理！在刷新配置文件过程中可能发生错误，建议添加错误处理逻辑。
this.preferenceService.onPreferenceChanged((event: PreferenceChange) => { if (event.preferenceName === TerminalSettingsId.Type) { - this.refreshAvailableProfiles(); + try { + this.refreshAvailableProfiles(); + } catch (error) { + this.logger.error('刷新终端配置文件失败:', error); + } } });packages/extension/src/hosted/api/vscode/ext.host.terminal.ts (1)
375-382: 配置文件更新处理逻辑完善！实现了以下功能：
- 正确比较新旧配置文件路径
- 仅在路径实际变化时触发事件
- 通过避免不必要的事件触发优化了性能
建议为 automation 相关的注释添加英文翻译。
- // 还不知道这个 automation 有啥用 + // TODO: Need to investigate the purpose of automation profile + // 还不知道这个 automation 有啥用
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/extension/src/common/vscode/terminal.ts(1 hunks)packages/extension/src/hosted/api/vscode/env/envApiFactory.ts(1 hunks)packages/extension/src/hosted/api/vscode/ext.host.terminal.ts(3 hunks)packages/terminal-next/src/browser/terminal.profile.ts(4 hunks)packages/types/vscode/typings/vscode.env.d.ts(2 hunks)
🔇 Additional comments (6)
packages/extension/src/hosted/api/vscode/env/envApiFactory.ts (1)
118-118: 实现正确且符合 VSCode API 规范！
该实现正确地将终端 shell 变更事件转发到环境 API 中。
packages/extension/src/common/vscode/terminal.ts (1)
66-67: 接口定义清晰且类型准确！
在 IExtHostTerminal 接口中添加 onDidChangeShell 事件定义，类型声明准确，位置合理。
packages/types/vscode/typings/vscode.env.d.ts (2)
72-76: 事件声明正确且文档完整！
事件声明和文档注释符合 VSCode API 规范。
78-85: 遥测日志器创建函数实现完整！
函数声明和参数文档符合 VSCode API 规范。
packages/extension/src/hosted/api/vscode/ext.host.terminal.ts (2)
58-58: 事件发射器声明正确！
使用了标准的事件发射器模式，并正确设置为私有成员。
176-178: 事件访问器实现正确！
遵循标准的事件访问器模式，提供对私有事件的公共访问。
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4223 +/- ##
=======================================
Coverage 53.99% 53.99%
=======================================
Files 1616 1616
Lines 98108 98129 +21
Branches 20077 20088 +11
=======================================
+ Hits 52972 52987 +15
- Misses 37498 37502 +4
- Partials 7638 7640 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
bk1012
force-pushed
the
feat/support-env.onDidChangeShell
branch
from
4264951 to
a082563
Compare
bk1012
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
packages/extension/src/common/vscode/terminal.ts (1)
66-69: 接口更改符合预期！新增的
onDidChangeShell事件和$setShell方法完全符合 VSCode API 的设计模式。建议为这两个新增的接口成员添加 JSDoc 注释，以便更好地说明其用途和参数类型。建议添加如下文档注释:
+ /** + * 当终端的 shell 发生变化时触发的事件 + */ onDidChangeShell: Event<string>; + /** + * 设置终端的 shell + * @param shell 新的 shell 路径 + */ $setShell(shell: string);packages/terminal-next/src/common/profile.ts (1)
24-24: 接口添加合理！在
ITerminalProfileService中添加onDidChangeDefaultShell事件是合适的，这与服务的职责相符。建议添加文档注释以说明事件的触发时机和参数含义。建议添加如下文档注释:
+ /** + * 当默认 shell 配置发生变化时触发的事件 + * @event + */ onDidChangeDefaultShell: Event<string>;packages/terminal-next/__tests__/browser/mock.service.ts (1)
420-422: 建议增强模拟实现！当前的模拟实现过于简单。为了更好地支持测试场景，建议实现一个完整的事件发射器。
建议修改实现如下:
+ private readonly _onDidChangeDefaultShell = new Emitter<string>(); + - onDidChangeDefaultShell() { - return new Disposable(); - } + onDidChangeDefaultShell = this._onDidChangeDefaultShell.event;这样的实现将允许测试用例触发 shell 变更事件并验证相关行为。
packages/extension/src/browser/vscode/api/main.thread.terminal.ts (1)
129-133: 代码实现正确，建议增加错误处理事件订阅的实现逻辑正确，但建议考虑在调用
proxy.$setShell时添加错误处理。建议修改为:
this.disposable.addDispose( this.profileService.onDidChangeDefaultShell((shell: string) => { + try { this.proxy.$setShell(shell); + } catch (error) { + this.logger.error('Failed to set shell', error); + } }), );packages/extension/src/hosted/api/vscode/ext.host.terminal.ts (2)
189-194: 建议增加空值处理当前实现正确，但建议增加对
shell参数的空值检查。建议修改为:
$setShell(shell: string) { + if (!shell) { + return; + } if (this.shell !== shell) { this.shell = shell; this.onDidChangeShellEvent.fire(shell); } }
383-390: 建议完善实现和文档
- 建议将英文注释翻译为中文
- 建议增加对 profile 参数的空值检查
建议修改为:
public $acceptDefaultProfile(profile: ITerminalProfile, automationProfile?: ITerminalProfile): void { const oldProfile = this._defaultProfile; this._defaultProfile = profile; - // 还不知道这个 automation 有啥用 + // TODO: automation profile 的具体用途待确认 this._defaultAutomationProfile = automationProfile; + if (!profile?.path) { + return; + } if (oldProfile?.path !== profile.path) { this.onDidChangeShellEvent.fire(profile.path); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/extension/src/browser/vscode/api/main.thread.terminal.ts(1 hunks)packages/extension/src/common/vscode/terminal.ts(1 hunks)packages/extension/src/hosted/api/vscode/env/envApiFactory.ts(1 hunks)packages/extension/src/hosted/api/vscode/ext.host.terminal.ts(4 hunks)packages/terminal-next/__tests__/browser/mock.service.ts(1 hunks)packages/terminal-next/src/browser/terminal.profile.ts(6 hunks)packages/terminal-next/src/common/profile.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/extension/src/hosted/api/vscode/env/envApiFactory.ts
- packages/terminal-next/src/browser/terminal.profile.ts
🔇 Additional comments (2)
packages/extension/src/hosted/api/vscode/ext.host.terminal.ts (2)
53-53: 属性声明正确且符合最佳实践
shell 和 onDidChangeShellEvent 的声明遵循了正确的访问修饰符和命名规范。
Also applies to: 59-59
177-179: 事件访问器实现正确
onDidChangeShell 的实现遵循了 VSCode API 的标准模式，与其他事件访问器保持一致。
Types
Background or solution
Changelog
Summary by CodeRabbit
新特性
onDidChangeShell，用于通知终端使用的默认 shell 变化。createTelemetryLogger函数，支持创建符合用户设置的遥测记录器。onDidChangeDefaultShell，用于监听默认 shell 配置的变化。文档
shell常量的注释，以反映设置的变化。