-
Notifications
You must be signed in to change notification settings - Fork 307
[v1.3?] VSCodeConnect 代码优化 #947
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: release/v1.3
Are you sure you want to change the base?
[v1.3?] VSCodeConnect 代码优化 #947
Conversation
73c1f7f to
40777de
Compare
40777de to
ea13c13
Compare
ea13c13 to
a849db0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
这个 PR 对 VSCodeConnect 服务进行了重构,目的是优化代码结构和提高可维护性。主要改进包括提取类型定义、重构连接逻辑、改进访问修饰符使用,以及增强错误处理。
主要变更:
- 提取了
VSCodeConnectParam类型定义,提高了类型复用性和可读性 - 重构了 WebSocket 连接和重连逻辑,将嵌套的回调改为更清晰的结构
- 添加了私有访问修饰符,改进了封装性
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 14 comments.
| File | Description |
|---|---|
src/app/service/service_worker/client.ts |
更新了 SystemClient 的 connectVSCode 方法类型定义,使用新提取的 VSCodeConnectParam 类型 |
src/app/service/offscreen/vscode-connect.ts |
主要重构文件:提取类型定义、添加访问修饰符、重构连接和重连逻辑、改进消息处理结构 |
src/app/service/offscreen/index.ts |
更新 VSCodeConnect 实例化方式,传递 Server 对象而不是 Group 对象 |
src/app/service/offscreen/client.ts |
更新 VscodeConnectClient 的 connect 方法类型定义 |
| const connectVSCode = () => { | ||
| if (this.ws) return; // 已连接则忽略 | ||
| return new Promise<void>((resolve, reject) => { | ||
| let ws; | ||
| try { | ||
| ws = new WebSocket(url); | ||
| } catch (e: any) { | ||
| this.logger.debug("connect vscode faild", Logger.E(e)); | ||
| reject(e); | ||
| return; | ||
| } | ||
| }); | ||
| let connectOK = false; | ||
| ws.addEventListener("open", () => { | ||
| ws.send('{"action":"hello"}'); | ||
| connectOK = true; | ||
| // 如重复连接,则清除之前的 | ||
| if (this.ws) { | ||
| this.closeExisting(); | ||
| } | ||
| this.ws = ws; | ||
| resolve(); | ||
| this.clearTimer(); | ||
| }); | ||
| ws.addEventListener("message", (ev) => { | ||
| this.handleMessage(ev).catch((err) => { | ||
| this.logger.error("message handler error", Logger.E(err)); | ||
| }); | ||
| }); | ||
|
|
||
| this.wsConnect.addEventListener("error", (e) => { | ||
| this.wsConnect = undefined; | ||
| this.logger.debug("connect vscode faild", Logger.E(e)); | ||
| if (!ok) { | ||
| reject(new Error("connect fail")); | ||
| } | ||
| }); | ||
| ws.addEventListener("error", (e) => { | ||
| this.ws = undefined; | ||
| this.logger.debug("connect vscode faild", Logger.E(e)); | ||
| if (!connectOK) { | ||
| reject(new Error("connect fail")); | ||
| } | ||
| if (reconnect) doReconnect(); | ||
| }); | ||
|
|
||
| this.wsConnect.addEventListener("close", () => { | ||
| this.wsConnect = undefined; | ||
| this.logger.debug("vscode connection closed"); | ||
| ws.addEventListener("close", () => { | ||
| this.ws = undefined; | ||
| this.logger.debug("vscode connection closed"); | ||
| if (reconnect) doReconnect(); | ||
| }); | ||
| // 如 open, close, error 都不发生,30 秒后reject | ||
| this.clearTimer(); | ||
| this.timerId = setTimeout(() => { | ||
| if (!connectOK) { | ||
| reject(new Error("Timeout")); | ||
| try { | ||
| ws.close(); | ||
| } catch (e) { | ||
| console.error(e); | ||
| } | ||
| if (reconnect) doReconnect(); | ||
| } | ||
| }, 30_000); | ||
| }); | ||
| }); | ||
| }; | ||
| // 如果已经连接,断开重连 | ||
| this.closeExisting(); | ||
| // 清理老的定时器 | ||
| this.clearTimer(); | ||
| return Promise.resolve().then(() => connectVSCode()); |
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
函数 connectVSCode 作为嵌套函数定义在 connect 方法内部,但没有返回 Promise。在第 50 行有 return 语句,但在第 111 行调用时使用了 Promise.resolve().then(() => connectVSCode()),这个调用会忽略 connectVSCode 的返回值。应该将 return 语句添加到 connectVSCode 函数声明中,使其显式返回 Promise<void>。
| const doReconnect = () => { | ||
| // 如果已经连接,断开重连 | ||
| if (this.wsConnect) { | ||
| this.wsConnect.close(); | ||
| } | ||
| try { | ||
| this.wsConnect = new WebSocket(url); | ||
| } catch (e: any) { | ||
| this.logger.debug("connect vscode faild", Logger.E(e)); | ||
| reject(e); | ||
| return; | ||
| } | ||
| let ok = false; | ||
| this.wsConnect.addEventListener("open", () => { | ||
| this.wsConnect!.send('{"action":"hello"}'); | ||
| ok = true; | ||
| resolve(); | ||
| }); | ||
| this.wsConnect.addEventListener("message", async (ev) => { | ||
| const data = JSON.parse(ev.data); | ||
| switch (data.action) { | ||
| case "onchange": { | ||
| // 调用安装脚本接口 | ||
| const code = data.data.script; | ||
| this.scriptClient.installByCode(uuidv5(data.data.uri, uuidv5.URL), code, "vscode"); | ||
| break; | ||
| } | ||
| default: | ||
| this.closeExisting(); | ||
| this.clearTimer(); | ||
| this.timerId = setTimeout(connectVSCode, 100); | ||
| }; |
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doReconnect 函数的设计存在问题:它调用 setTimeout 设置延迟后执行 connectVSCode,但 setTimeout 返回的 timerId 被赋值给 this.timerId。然而在 clearTimer() 中使用的是 clearTimeout,这意味着如果在延迟期间调用 clearTimer(),会清除这个定时器。这个行为可能是期望的,但缺乏文档说明。更重要的是,如果 reconnect 为 false,doReconnect 根本不应该被调用,但代码中没有对此进行检查。
| constructor(windowServer: Server, msgSender: MessageSend) { | ||
| this.vscodeConnectGroup = windowServer.group("vscodeConnect"); | ||
| this.scriptClient = new ScriptClient(msgSender); |
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
API 设计不一致:VSCodeConnect 类现在在构造函数中接收 Server 对象并自行创建 group,这与项目中其他 Service 的模式不一致。根据 Architecture Overview 中的描述,Service 应该在构造函数中接收 Group 对象作为依赖注入,而不是接收 Server 然后自己创建 Group。这种改变破坏了依赖注入的模式,降低了可测试性。应该保持原有的设计,在构造函数中接收 Group 对象。
| const connectVSCode = () => { | ||
| if (this.ws) return; // 已连接则忽略 |
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
存在竞态条件:在 connectVSCode() 函数开始时检查 this.ws 是否存在,但在异步的 Promise 执行过程中,this.ws 可能在 open 事件中被设置。如果同时有多个连接尝试,可能会导致多个 WebSocket 实例同时存在。建议在函数开始时就设置一个连接中的标志位,或者使用互斥锁机制防止并发连接。
| // 如重复连接,则清除之前的 | ||
| if (this.ws) { | ||
| this.closeExisting(); | ||
| } | ||
| this.ws = ws; |
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这段逻辑存在问题:在 open 事件处理程序中检查 this.ws 是否已存在,如果存在就关闭旧连接。但此时新的 ws 对象应该就是要赋值给 this.ws 的对象,这个检查在逻辑上不合理。如果担心竞态条件,应该在 connectVSCode 函数开始时就进行检查和处理,而不是在 open 事件中。
| gmApi.init(); | ||
| const vscodeConnect = new VSCodeConnect(this.windowServer.group("vscodeConnect"), this.extMsgSender); | ||
| vscodeConnect.init(); | ||
| const vsCodeConnect = new VSCodeConnect(this.windowServer, this.extMsgSender); |
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
变量命名不一致:在 index.ts 中使用了 camelCase 的 vsCodeConnect(小写 s),但在类型和其他地方都使用 VSCode(大写 VS)。为了保持一致性,建议使用 vscodeConnect(全小写)或 vsCodeConnect,但要在整个项目中保持统一。参考项目中其他地方的命名(如 VscodeConnectClient),应该使用 vscodeConnect。
| }); | ||
| ws.addEventListener("error", (e) => { | ||
| this.ws = undefined; | ||
| this.logger.debug("connect vscode faild", Logger.E(e)); |
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
注释中存在拼写错误:"faild" 应该是 "failed"。
| default: | ||
| this.closeExisting(); | ||
| this.clearTimer(); | ||
| this.timerId = setTimeout(connectVSCode, 100); |
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这里使用了 setTimeout 100ms 延迟,但实际上应该立即调用 connectVSCode()。这个延迟会导致用户在首次连接时无故等待 100ms。如果是为了避免重连风暴,应该只在重连时添加延迟,而不是首次连接时。
| } | ||
| private clearTimer(): void { | ||
| if (this.timerId) { | ||
| clearTimeout(this.timerId); |
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clearTimeout 应该改为 clearInterval。虽然当前代码中 timerId 是通过 setTimeout 设置的,但从语义上看,这个定时器在某些情况下可能需要支持周期性重连(从旧代码的 setInterval 可以看出)。使用 clearTimeout 可能无法正确清理所有类型的定时器。建议明确定时器的类型,或者同时调用 clearTimeout 和 clearInterval 以确保清理。
| try { | ||
| ws = new WebSocket(url); | ||
| } catch (e: any) { | ||
| this.logger.debug("connect vscode faild", Logger.E(e)); |
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
注释中存在拼写错误:"faild" 应该是 "failed"。这个错误在多处出现(第 56、80 行),应该统一修正。
概述 Descriptions
变更内容 Changes
截图 Screenshots