feat: support comments on multi-diff view #4567
Conversation
|
/next |
| private readonly multiDiffSourceResolverService: IMultiDiffSourceResolverService; | ||
|
|
||
| @Autowired(EditorCollectionService) | ||
| private readonly collectionService: EditorCollectionServiceImpl; |
There was a problem hiding this comment.
这个依赖关系好像搞反了，是从collection创建editor的
|
🎉 PR Next publish successful! 3.8.3-next-1747817292.0 |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4567 +/- ##
==========================================
- Coverage 52.64% 52.61% -0.03%
==========================================
Files 1685 1687 +2
Lines 104157 104340 +183
Branches 22601 22666 +65
==========================================
+ Hits 54833 54899 +66
- Misses 40993 41081 +88
- Partials 8331 8360 +29
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:
|
|
/next |
|
/next |
|
🎉 PR Next publish successful! 3.8.3-next-1747895552.0 |
…ments-on-multi-diff-widget
|
/next |
|
🎉 PR Next publish successful! 3.9.1-next-1749196667.0 |
|
/next |
|
🎉 PR Next publish successful! 3.9.1-next-1749204089.0 |
…ments-on-multi-diff-widget
erha19
changed the title
|
""" Walkthrough本次变更主要围绕多文件 Diff 编辑器（multi-diff editor）功能的引入与完善，涵盖编辑器集合服务、装饰应用、评论服务、样式、主题色、接口定义等多个模块。包括新增多 diff 编辑器的管理、编辑器装饰应用接口、评论区 race condition 处理、UI 细节优化以及相关依赖注入和事件管理等，涉及多处代码结构和接口调整。 Changes
Sequence Diagram(s)sequenceDiagram
participant 用户
participant EditorCollectionService
participant BrowserMultiDiffEditor
participant DiffEditorPart
participant IEditor
participant CommentsService
用户->>EditorCollectionService: createMultiDiffEditor()
EditorCollectionService->>BrowserMultiDiffEditor: 构造实例并注册
BrowserMultiDiffEditor->>DiffEditorPart: createAndRegisterEditorParts()
DiffEditorPart->>IEditor: 绑定装饰应用、事件监听等
用户->>EditorCollectionService: listMultiDiffEditors()
EditorCollectionService-->>用户: 返回多 diff 编辑器列表
用户->>BrowserMultiDiffEditor: compareMultiple(editor, resource, options)
BrowserMultiDiffEditor->>DiffEditorPart: 注册/更新编辑器部件
DiffEditorPart->>IEditor: updateDocumentModel(uri)
IEditor->>IEditor: 应用装饰、更新选项
用户->>CommentsService: renderCommentRange(editor, selection)
CommentsService->>IEditor: 检查/取消上一次渲染
CommentsService->>IEditor: 应用注释装饰
CommentsService->>IEditor: 更新保留空间
Possibly related PRs
Suggested reviewers
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
yarn install v1.22.22 ✨ Finishing Touches
🧪 Generate Unit Tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 6
🧹 Nitpick comments (12)
packages/extension/__tests__/hosted/api/vscode/ext.host.comments.test.ts (1)
68-73: Mock 服务最小实现可能不足此处仅 stub 了
listEditors，若CommentsService后续调用onDidAddEditor等事件，将导致单测潜在崩溃。
可视情况扩充 mock，或使用jest.fn()捕获未知调用并返回安全默认值。packages/monaco/src/browser/contrib/multi-diff-editor/resource-label.module.less (1)
11-15: 建议：补充 Fallback 颜色变量
color: var(--descriptionForeground)依赖于外部主题变量。若第三方主题未定义该 token，会导致字体颜色回退为inherit，在暗色主题下可能出现对比度不足。可考虑添加后备颜色值：- color: var(--descriptionForeground); + color: var(--descriptionForeground, #8a8a8a);packages/editor/src/browser/editor.less (1)
24-53: 样式逻辑 OK，但.renamed状态颜色疑似误用
.renamed使用了--kt-decoration-deletedResourceForeground，与语义不符（应更接近 modified / added）。请确认设计稿，或调整为专用 token，以避免误导用户。
其他部分（默认M、空状态处理）实现简洁 👍packages/editor/src/browser/decoration-applier.ts (1)
72-74: 将applyDecorationFromProvider提升为public公开此方法有助于外部强制刷新，但请同步更新接口文档，避免误用造成不必要的频繁异步调用。
packages/theme/src/common/color-tokens/multi-diff-editor.ts (1)
15-16: 缺少本地化处理描述字符串直接写了英文常量，而同文件中的其它颜色注册均通过
localize。建议保持一致，避免遗漏国际化。- "The background color of the diff editor's header", + localize('multiDiffEditor.headerBackground', "The background color of the diff editor's header"),packages/editor/src/common/multi-diff.ts (3)
65-65:compareMultiple的首参设计存疑接口自身代表 Multi-Diff Editor，却要求调用方再把
editor: IMultiDiffEditor传进来，容易造成重复或误传。若实现里只需this，建议去掉该参数；如需比较“目标”编辑器，可更明确命名（如targetEditor）。
105-106:multiDiffWidget建议设为只读UI 组件实例应当由实现类内部控制，暴露为
readonly可防止外部误改。- multiDiffWidget: MultiDiffEditorWidget; + readonly multiDiffWidget: MultiDiffEditorWidget;
108-110:highlight应为可选字段目前
options?里highlight必填，调用者若只想传range需额外写highlight: false，不够友好。- reveal(resource: IMultiDiffResourceId, options?: { range?: Range; highlight: boolean }): void; + reveal(resource: IMultiDiffResourceId, options?: { range?: Range; highlight?: boolean }): void;packages/monaco/src/browser/monaco.service.ts (1)
178-190: 缺少资源释放，可能造成内存泄漏
createResourceLabel返回的对象dispose()是空实现，但内部持有的ResourceLabel没有被销毁或从 DOM 移除。应在dispose()中调用resourceLabel.clear()或移除元素，确保事件与引用释放。- dispose: () => {}, + dispose: () => { + resourceLabel.clear(); + },packages/comments/src/browser/comments-thread.ts (1)
278-287: 考虑将延迟时间设为可配置项当前使用硬编码的 100ms 延迟来防止竞态条件。建议将此值提取为配置项或常量，以便根据不同场景进行调整。
+private static readonly WIDGET_SHOW_DELAY_MS = 100; + public showWidgetsIfShowed() { // 清除之前的待执行 timeout，避免竞态条件 this.pendingShowTimeouts.forEach((timeout) => clearTimeout(timeout)); this.pendingShowTimeouts.clear(); for (const editor of this.getEditorsByUri(this.uri)) { let widget = this.widgets.get(editor); // 说明是在新的 group 中打开 if (!widget) { widget = this.addWidgetByEditor(editor); } // 如果标记之前是已经展示的 widget，则调用 show 方法 if (editor.currentUri?.isEqual(this.uri)) { const timeout = setTimeout(() => { // 在异步回调中再次检查编辑器的当前 URI 是否仍然匹配 thread 的 URI // 防止用户快速滚动时 widget 渲染到错误的文件中 if (widget && editor.currentUri?.isEqual(this.uri)) { widget.show(); } // 执行完成后从跟踪集合中移除 this.pendingShowTimeouts.delete(timeout); - }, 100); + }, CommentsThread.WIDGET_SHOW_DELAY_MS); this.pendingShowTimeouts.add(timeout); } } }packages/comments/src/browser/comments.service.ts (2)
313-316: 优化重复编辑器检查逻辑当前的重复检查只基于 Monaco 编辑器 ID。建议同时检查 URI 以确保更准确的重复检测。
const disposer = new Disposable(); -if (this.allEditors.some((e) => e.monacoEditor.getId() === editor.monacoEditor.getId())) { - return this.editorDisposerMap.get(editor.monacoEditor.getId()) || disposer; +const existingEditor = this.allEditors.find((e) => + e.monacoEditor.getId() === editor.monacoEditor.getId() || + (e.currentUri && editor.currentUri && e.currentUri.isEqual(editor.currentUri)) +); +if (existingEditor) { + return this.editorDisposerMap.get(editor.monacoEditor.getId()) || disposer; }
1086-1088: 考虑为 FRAME_THREE 延迟添加注释说明使用
FRAME_THREE延迟可能是为了等待渲染完成，建议添加注释说明使用此延迟的原因。const editor = this.editorCollectionService.getEditorByUri(thread.uri); if (editor) { + // 延迟 3 帧确保编辑器渲染完成后再显示评论组件 setTimeout(() => { thread.show(editor); }, FRAME_THREE);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (29)
packages/ai-native/src/browser/components/ChatEditor.tsx(1 hunks)packages/comments/src/browser/comments-thread.ts(6 hunks)packages/comments/src/browser/comments-zone.view.tsx(1 hunks)packages/comments/src/browser/comments.service.ts(10 hunks)packages/comments/src/common/index.ts(1 hunks)packages/components/src/icon/iconfont/iconMap.ts(1 hunks)packages/components/src/markdown/styles.less(1 hunks)packages/components/src/scrollbars/index.tsx(2 hunks)packages/components/src/scrollbars/styles.less(2 hunks)packages/design/src/browser/theme/light-theme.ts(1 hunks)packages/editor/src/browser/base-editor-wrapper.ts(1 hunks)packages/editor/src/browser/decoration-applier.ts(2 hunks)packages/editor/src/browser/editor-collection.service.ts(9 hunks)packages/editor/src/browser/editor.contribution.ts(2 hunks)packages/editor/src/browser/editor.less(1 hunks)packages/editor/src/browser/multi-diff/multi-diff-editor.ts(7 hunks)packages/editor/src/browser/multi-diff/multi-diff-resource.ts(0 hunks)packages/editor/src/browser/workbench-editor.service.ts(1 hunks)packages/editor/src/common/editor.ts(4 hunks)packages/editor/src/common/multi-diff.ts(3 hunks)packages/extension/__tests__/hosted/api/vscode/ext.host.comments.test.ts(2 hunks)packages/extension/src/browser/vscode/api/main.thread.editor.ts(1 hunks)packages/monaco-enhance/src/browser/zone-widget.ts(1 hunks)packages/monaco/src/browser/contrib/multi-diff-editor/resource-label.module.less(1 hunks)packages/monaco/src/browser/contrib/multi-diff-editor/resource-label.ts(1 hunks)packages/monaco/src/browser/monaco.service.ts(4 hunks)packages/scm/src/browser/dirty-diff/index.ts(1 hunks)packages/theme/src/common/color-tokens/index.ts(1 hunks)packages/theme/src/common/color-tokens/multi-diff-editor.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/editor/src/browser/multi-diff/multi-diff-resource.ts
🧰 Additional context used
🧬 Code Graph Analysis (3)
packages/theme/src/common/color-tokens/multi-diff-editor.ts (1)
packages/theme/src/common/color-tokens/editor.ts (1)
editorBackground(97-101)
packages/editor/src/browser/editor.contribution.ts (1)
packages/editor/src/common/editor.ts (1)
IResourceOpenOptions(625-737)
packages/editor/src/browser/multi-diff/multi-diff-editor.ts (4)
packages/editor/src/browser/types.ts (1)
IConvertedMonacoOptions(474-478)packages/editor/src/common/multi-diff.ts (1)
IMultiDiffEditor(61-111)packages/editor/src/common/editor.ts (2)
ICodeEditor(297-311)IResourceOpenOptions(625-737)packages/utils/src/types.ts (1)
isString(38-44)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: build (ubuntu-latest, 20.x)
- GitHub Check: build (macos-latest, 20.x)
- GitHub Check: unittest (ubuntu-latest, 18.x, jsdom)
- GitHub Check: build-windows
- GitHub Check: unittest (macos-latest, 18.x, jsdom)
- GitHub Check: unittest (ubuntu-latest, 18.x, node)
- GitHub Check: unittest (macos-latest, 18.x, node)
- GitHub Check: ubuntu-latest, Node.js 20.x
🔇 Additional comments (24)
packages/components/src/scrollbars/styles.less (1)
44-56:position: fixed可能导致装饰条脱离容器上下文将
.scrollbar-decoration-*从absolute改为fixed后，装饰条会相对视口定位，而不是相对滚动容器。这在多层滚动容器或组件（如弹窗、嵌套 diff 视图）中可能出现：
- 装饰条漂浮在最外层页面顶部/左侧，遮挡其他 UI。
- 多个滚动区域并存时叠加在一起，z-index 竞争更加明显。
请在实际多 diff 场景下确认：
- 装饰条是否仍然跟随对应滚动区域移动 / 显隐。
- 是否需按滚动容器尺寸裁剪或使用
position: absolute+overflow: hidden的父容器。若只为解决某些 EdgeCase，可考虑仅在特定模式下切换定位或增加
pointer-events: none、contain: layout等保护。packages/components/src/scrollbars/index.tsx (1)
105-109: 阴影透明度轴向对调：请再次确认业务含义现在垂直阴影（左侧竖向渐变）的透明度取
scrollLeft，而水平阴影取scrollTop。如果设计意图是：
- 横向滚动→显示左侧阴影
- 纵向滚动→显示顶部阴影
则实现正确，无需改动。若仅因代码重构误换，请回滚。
packages/scm/src/browser/dirty-diff/index.ts (1)
17-18: 导入路径更新符合模块拆分，OK仅替换
ISumiEditor的来源，功能不变，编译亦能通过。packages/components/src/markdown/styles.less (1)
20-22: 细节 👍：Popover 中 HR 去除下边框在气泡内移除
hr的底边，避免双线效果，符合设计一致性。packages/extension/src/browser/vscode/api/main.thread.editor.ts (1)
31-32: 同步 editor 模块重构的导入路径，LGTM与核心编辑器结构调整保持一致，无其他风险。
packages/components/src/icon/iconfont/iconMap.ts (1)
209-210: 新增映射符合预期新增
'diff': 'open-changes'与已有'open-changes'value 一致，可帮助调用方使用更直观的diffkey，无其他问题。packages/ai-native/src/browser/components/ChatEditor.tsx (1)
28-28: 确认新导入路径在所有包中可解析
insertSnippetWithMonacoEditor路径由editor-collection.service迁至base-editor-wrapper。请确保：
@opensumi/ide-editor/lib/browser/base-editor-wrapper在发布包中确实导出该函数；- 所有其他引用已同步更新，避免重复导出导致的编译冲突。
建议在 CI 中增加一次
tsc --noEmit静态编译验证。packages/design/src/browser/theme/light-theme.ts (1)
810-813: 新增配色需同步 Dark/Hc 主题已为 Light 主题添加
multiDiffEditor.*三个 token，但在深色与高对比度主题中未见同名条目，UI 将回退至默认值。
请确认是否需要在其它主题文件内补充对应颜色，保持跨主题一致性。packages/monaco/src/browser/contrib/multi-diff-editor/resource-label.module.less (2)
7-9: 命名与职责清晰，LGTM
.strikethrough的职责和实现都非常直观，没有发现问题。
17-23: RTL 方案需再次验证可读性为了在路径过长时保留末尾片段，这里使用了
direction: rtl; text-align: left;。
在英文或绝大部分 LTR 语言下没问题，但在真正的 RTL 文本环境（阿拉伯语、希伯来语）可能导致光标移动与文本渲染错位。请确认：
- 是否需要对
dir="rtl"的文档做不同处理；- 是否可接受复制文本时字符顺序被反转的副作用。
如果需要兼容 RTL，可考虑用
mask-image: linear-gradient()或<span dir="ltr">包装实现省略号，而不是依赖direction。packages/comments/src/browser/comments-zone.view.tsx (2)
209-213: 隐藏逻辑可能与组件内部状态不一致
onDidChangeModel时调用thread.hide()，但未同步CommentsZoneWidget的_isShow状态，也未触发hide()方法。若其他逻辑依赖widget.isShow，可能出现“线程已隐藏但组件状态仍为显示”的不一致。建议改为：
- const disposer = editor.monacoEditor.onDidChangeModel(() => { - thread.hide(); - }); + const disposer = editor.monacoEditor.onDidChangeModel(() => { + this.hide(); // 保证内部状态同步 + thread.hide(); // 如仍需通知线程自身 + });请确认是否必须双重调用，或者仅调用
this.hide()即可。
217-218: 👍 资源释放到位，避免内存泄漏在
dispose中正确销毁了disposer，实现规范。packages/editor/src/browser/editor.contribution.ts (1)
1248-1250:Details
❓ Verification inconclusive
editorOptions参数命名清晰，但请注意与label的覆盖顺序
label: options.title先于...options.editorOptions展开意味着当调用方在editorOptions中再次传入label时，会覆盖掉前面的标题。这种“后者优先”的行为是否与预期一致需要再确认。如果希望始终以
options.title为准，请将展开顺序对调：- await this.workbenchEditorService.open(multiDiffSourceUri, { - label: options.title, - ...options.editorOptions, - }); + await this.workbenchEditorService.open(multiDiffSourceUri, { + ...options.editorOptions, + label: options.title, + });
注意：
label属性展开顺序会导致后者覆盖前者当前在
packages/editor/src/browser/editor.contribution.ts第1248–1250 行，调用await this.workbenchEditorService.open(multiDiffSourceUri, { label: options.title, ...options.editorOptions, });由于 JavaScript 对象展开时后面同名属性会覆盖前面，若
options.editorOptions中含有label，就会覆盖掉options.title。请确认这种“后者优先”行为是否符合预期：
- 如希望调用方传入的
label能覆盖标题，可保留当前写法。- 如希望始终以
options.title为准，请调整顺序：- await this.workbenchEditorService.open(multiDiffSourceUri, { - label: options.title, - ...options.editorOptions, - }); + await this.workbenchEditorService.open(multiDiffSourceUri, { + ...options.editorOptions, + label: options.title, + });packages/theme/src/common/color-tokens/index.ts (1)
32-40:Details
✅ Verification successful
路径更名与新增导出需确认文件存在
已将
debugToolbar → debug-toolbar、chatColors → chat-colors并新增multi-diff-editor。
请确认三处目标文件均已提交，避免运行时Cannot find module。
🏁 Script executed:
#!/bin/bash # 检查目标 token 文件是否存在 fd 'debug-toolbar.ts' packages/theme/src/common/color-tokens fd 'chat-colors.ts' packages/theme/src/common/color-tokens fd 'multi-diff-editor.ts' packages/theme/src/common/color-tokensLength of output: 354
已验证：color-tokens 相关文件均已存在，无需额外处理
- packages/theme/src/common/color-tokens/debug-toolbar.ts
- packages/theme/src/common/color-tokens/chat-colors.ts
- packages/theme/src/common/color-tokens/multi-diff-editor.ts
导出语句（index.ts 第32–40行）无需变更。
packages/monaco-enhance/src/browser/zone-widget.ts (1)
298-302: 空值判断修复 👍
minimap && minimapWidth ? minimapWidth + 5 : 0有效避免undefined + 5，逻辑正确。
无其他问题。packages/editor/src/browser/decoration-applier.ts (1)
37-48: 订阅加入Disposable，泄漏风险已解决将
onDidChangeModel与onDidDispose包装进this.addDispose，确保编辑器关闭时及时清理，👍
无额外问题。packages/monaco/src/browser/monaco.service.ts (1)
58-60: 注入IWorkspaceService需确认所有运行时环境可用某些无工作区场景（如纯只读 diff 查看器）可能没有
IWorkspaceService，强制注入会导致 DI 解析失败。请确认或在构造函数加可选依赖处理。packages/editor/src/common/editor.ts (1)
242-246:decorationApplier设为必需字段会破坏旧实现现有所有
IEditor实现都必须立即提供decorationApplier，否则编译不过。请确认所有实现均已同步更新；若非强需求，可改为可选属性以保证向后兼容。packages/comments/src/browser/comments-thread.ts (1)
117-124: LGTM: 编辑器组件初始化逻辑正确在构造函数中添加的逻辑确保了在创建评论线程时，为所有匹配 URI 的编辑器正确初始化组件。
packages/editor/src/browser/multi-diff/multi-diff-editor.ts (1)
201-205: 🛠️ Refactor suggestion改进只读状态的判断逻辑
当前只读状态的判断使用了 OR 逻辑，这可能导致当只有一个编辑器实例存在时，整个 diff 视图都变为只读。建议根据实际的编辑器类型分别设置只读状态。
originalInstance: original?.instance, modifiedInstance: modified?.instance, contextKeys: r.contextKeys, options: { - readOnly: (modified || original)?.instance.readonly, + readOnly: modified?.instance.readonly ?? original?.instance.readonly ?? false, // TODO: codelens，wordWrap options ...this.convertedOptions.diffOptions, },Likely an incorrect or invalid review comment.
packages/editor/src/browser/base-editor-wrapper.ts (1)
1-279: LGTM: 代码提取合理，结构清晰将编辑器包装类从
editor-collection.service.ts提取到独立文件中，提高了代码的模块化和可维护性。DiffEditorPart类正确地扩展了BaseMonacoEditorWrapper，并实现了必要的文档模型更新逻辑。packages/comments/src/browser/comments.service.ts (1)
536-540: LGTM: 多 Diff 编辑器支持实现正确正确处理了多 Diff 编辑器的文档模型更新，确保评论能在平铺模式下正常工作。
packages/editor/src/browser/editor-collection.service.ts (2)
122-147: LGTM: 重复编辑器检查逻辑完善改进后的
addEditors方法正确处理了重复编辑器的情况：
- 通过 URI 查找现有编辑器
- 通过 Monaco 编辑器 ID 进行二次验证
- 对于已存在的编辑器，清理并重新应用装饰
176-182: LGTM: 多 Diff 编辑器创建实现正确
createMultiDiffEditor方法正确实现了多 Diff 编辑器的创建，并将当前服务实例传递给编辑器以便进行生命周期管理。
| if (tabBarMode && scrollerRef.current) { | ||
| scrollerRef.current.addEventListener('wheel', onMouseWheel); | ||
| scrollerRef.current.addEventListener('wheel', onMouseWheel, { passive: true }); | ||
| } | ||
| return () => { | ||
| scrollerRef.current?.removeEventListener('wheel', onMouseWheel); |
There was a problem hiding this comment.
被动监听器中调用 preventDefault 会抛错 – 需移除 passive: true
addEventListener('wheel', onMouseWheel, { passive: true }) 与 onMouseWheel 内的 e.preventDefault() 冲突，运行时浏览器会输出
“Unable to preventDefault inside passive event listener invocation” 并且滚动拦截失效。
建议改为非被动监听，或移除 preventDefault：
- scrollerRef.current.addEventListener('wheel', onMouseWheel, { passive: true });
+scrollerRef.current.addEventListener('wheel', onMouseWheel, { passive: false });或完全省略第三参以保持默认行为（非 passive）。
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (tabBarMode && scrollerRef.current) { | |
| scrollerRef.current.addEventListener('wheel', onMouseWheel); | |
| scrollerRef.current.addEventListener('wheel', onMouseWheel, { passive: true }); | |
| } | |
| return () => { | |
| scrollerRef.current?.removeEventListener('wheel', onMouseWheel); | |
| if (tabBarMode && scrollerRef.current) { | |
| - scrollerRef.current.addEventListener('wheel', onMouseWheel, { passive: true }); | |
| + scrollerRef.current.addEventListener('wheel', onMouseWheel, { passive: false }); | |
| } | |
| return () => { | |
| scrollerRef.current?.removeEventListener('wheel', onMouseWheel); | |
| }; |
🤖 Prompt for AI Agents
In packages/components/src/scrollbars/index.tsx around lines 134 to 138, the
'wheel' event listener is added with the option { passive: true }, but the
onMouseWheel handler calls e.preventDefault(), causing a runtime error and
preventing scroll interception. To fix this, remove the { passive: true } option
when adding the event listener so that preventDefault can be called without
error, or alternatively remove the preventDefault call from onMouseWheel if
passive listening is required.
| /** | ||
| * 移除即将显示的 widget，避免竞态条件 | ||
| */ | ||
| removePaddingShowedWidgets(): void; |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
接口命名疑似拼写错误，建议统一为 Pending
removePaddingShowedWidgets 中的 Padding 看起来应为 Pending（待处理）而非内边距 Padding。
为了避免误解，建议将方法名及对应实现统一改为 removePendingShowedWidgets，并同步更新相关调用处。
- removePaddingShowedWidgets(): void;
+ /** 移除即将展示但尚未渲染完成的 widget，避免竞态 */
+ removePendingShowedWidgets(): void;请修正方法命名：removePendingShowedWidgets
在 packages/comments/src/common/index.ts 中，方法 removePaddingShowedWidgets 中的 “Padding” 应为 “Pending”（待处理），建议统一改为 removePendingShowedWidgets，并同步更新 JSDoc 注释及所有调用处以避免歧义。
变更示例：
--- packages/comments/src/common/index.ts
@@ -543,7 +543,7 @@
- /**
- * 移除即将显示的 widget，避免竞态条件
- */
- removePaddingShowedWidgets(): void;
+ /**
+ * 移除待渲染的 widget，避免竞态条件
+ */
+ removePendingShowedWidgets(): void;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * 移除即将显示的 widget，避免竞态条件 | |
| */ | |
| removePaddingShowedWidgets(): void; | |
| /** | |
| * 移除待渲染的 widget，避免竞态条件 | |
| */ | |
| removePendingShowedWidgets(): void; |
🤖 Prompt for AI Agents
In packages/comments/src/common/index.ts around lines 543 to 546, the method
name removePaddingShowedWidgets contains a typo where "Padding" should be
"Pending". Rename the method to removePendingShowedWidgets, update the JSDoc
comment accordingly, and refactor all references and calls to this method
throughout the codebase to maintain consistency and avoid confusion.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/comments/__test__/browser/comment-thread.test.ts (1)
32-37: listEditors 建议使用 jest.fn 便于断言目前
listEditors仅返回空数组，若将来需要验证该方法被调用次数，可改成 jest mock 函数：- useValue: mockService({ - listEditors: () => [], - }), + useValue: mockService({ + listEditors: jest.fn(() => []), + }),这样既不影响现有行为，又方便后续断言。
packages/comments/__test__/browser/comments-feature.registry.test.ts (1)
33-38: 同样建议将 listEditors 设为 jest.fn与其他测试保持一致，便于后期验证调用情况：
- listEditors: () => [], + listEditors: jest.fn(() => []),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/comments/__test__/browser/comment-thread.test.ts(2 hunks)packages/comments/__test__/browser/comment.service.test.ts(3 hunks)packages/comments/__test__/browser/comments-feature.registry.test.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: unittest (ubuntu-latest, 18.x, node)
- GitHub Check: unittest (macos-latest, 18.x, jsdom)
- GitHub Check: unittest (ubuntu-latest, 18.x, jsdom)
- GitHub Check: unittest (macos-latest, 18.x, node)
- GitHub Check: ubuntu-latest, Node.js 20.x
- GitHub Check: build-windows
- GitHub Check: build (macos-latest, 20.x)
- GitHub Check: build (ubuntu-latest, 20.x)
🔇 Additional comments (5)
packages/comments/__test__/browser/comment-thread.test.ts (1)
5-7: 引入 mockService & EditorCollectionService 👍新增的
mockService与EditorCollectionService引入与后续依赖注入保持一致，没有问题。packages/comments/__test__/browser/comment.service.test.ts (3)
79-81: 补充 currentUri 属性 ✅为
currentEditor增加currentUri能避免空值访问，保持与业务实现一致。
98-101: listEditors 返回空数组可能导致覆盖面不足在真实环境中
listEditors会包含当前活动编辑器。测试时始终返回[]可能掩盖跨编辑器渲染逻辑的问题。建议：- listEditors: () => [], + listEditors: jest.fn(() => [currentEditor]),并根据需要调整断言，以提升测试覆盖率。
297-298: deltaDecorations 期望次数由 9→1 需要确认逻辑变化调用次数骤减意味着高亮装饰的内部实现已有调整，建议回顾
comments.service.ts中相关逻辑，确认只调用一次即可满足全部场景，避免遗漏其他装饰更新。packages/comments/__test__/browser/comments-feature.registry.test.ts (1)
4-9: 测试依赖注入重构良好统一使用
createBrowserInjector、MockInjector等工具后，代码更简洁一致，无问题。
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
packages/editor/src/browser/multi-diff/multi-diff-editor.ts (2)
160-165: 未使用的形参editor应删除
compareMultiple的第一个参数editor整个函数体内未被引用，保留会增加阅读噪音。若未来无需使用，直接移除并更新接口。
260-275:any类型掩盖真实问题，降低类型安全通过
(ref.object as any).modifiedInstance取值会绕过类型检查，出现属性变更时编译无法提示。建议为RefCounted<IDocumentDiffItem>定义扩展接口或使用类型守卫，彻底去掉any。packages/comments/src/browser/comments.service.ts (2)
373-398: 对多-diff 编辑器的判断粗糙且变量未使用const currentMultiDiffEditor = this.editorCollectionService.listMultiDiffEditors()[0]; if (currentMultiDiffEditor) { editor.updateDocumentModel?.(URI.from(newModelUrl)); }
- 当同时存在多个 multi-diff editor 时，仅取首个实例并未校验 uri 是否匹配，可能误判。
currentMultiDiffEditor之后未被再次使用，完全可以删除该临时变量并改写为更明确的条件。
708-715: AbortController Map 需限定生命周期
editorRenderAbortControllers以 editorId 为键，但仅在 editoronDidChangeModel与 disposer 回调里清理。若编辑器因为其它路径被销毁（例如工作区关闭），Map 可能遗留未清理条目。建议在editor.onDispose时同样调用清理逻辑。
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/comments/src/browser/comments.service.ts(10 hunks)packages/editor/src/browser/multi-diff/multi-diff-editor.ts(7 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/editor/src/browser/multi-diff/multi-diff-editor.ts (4)
packages/editor/src/browser/types.ts (1)
IConvertedMonacoOptions(474-478)packages/editor/src/common/multi-diff.ts (1)
IMultiDiffEditor(61-111)packages/editor/src/common/editor.ts (2)
ICodeEditor(297-311)IResourceOpenOptions(625-737)packages/utils/src/types.ts (1)
isString(38-44)
| private createAndRegisterEditorParts( | ||
| modifiedEditor: { editor: ICodeEditor } | undefined, | ||
| originalEditor: { editor: ICodeEditor } | undefined, | ||
| modifiedInstance: any, | ||
| originalInstance: any, | ||
| documents: ISettableObservable<readonly RefCounted<IDocumentDiffItem>[] | 'loading', void>, | ||
| ): void { | ||
| const editors: DiffEditorPart[] = []; | ||
| if (modifiedEditor) { | ||
| const modifiedDiffEditorPart = this.injector.get(DiffEditorPart, [ | ||
| modifiedEditor.editor, | ||
| () => modifiedInstance, | ||
| EditorType.MODIFIED_DIFF, | ||
| documents, | ||
| ]); | ||
| editors.push(modifiedDiffEditorPart); | ||
| } | ||
| if (originalEditor) { | ||
| const originalDiffEditorPart = this.injector.get(DiffEditorPart, [ | ||
| originalEditor.editor, | ||
| () => originalInstance, | ||
| EditorType.ORIGINAL_DIFF, | ||
| documents, | ||
| ]); | ||
| editors.push(originalDiffEditorPart); | ||
| } | ||
| this.editorCollectionService.addEditors(editors); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
DiffEditorPart 未集中管理，生命周期难以保证
createAndRegisterEditorParts 每次新建 DiffEditorPart 实例后立即交给 editorCollectionService，但当前类本身既不保存引用，也不在 dispose() 中统一销毁。这意味着若 editorCollectionService 没有完整托管其生命周期，会产生悬挂实例。
建议：
- 维护
private readonly registeredEditorParts: DiffEditorPart[] = []; - 在此方法里向数组 push，
dispose()时遍历并调用dispose()。
🤖 Prompt for AI Agents
In packages/editor/src/browser/multi-diff/multi-diff-editor.ts between lines 131
and 158, the DiffEditorPart instances created in createAndRegisterEditorParts
are passed to editorCollectionService but not stored or disposed by this class,
risking dangling instances. Fix this by adding a private array
registeredEditorParts to hold these instances, push each new DiffEditorPart into
this array inside createAndRegisterEditorParts, and then override the dispose()
method of this class to iterate over registeredEditorParts and call dispose() on
each to ensure proper lifecycle management.
| constructor( | ||
| public readonly multiDiffWidget: MultiDiffEditorWidget, | ||
| private convertedOptions: IConvertedMonacoOptions, | ||
| private editorCollectionService: EditorCollectionServiceImpl, | ||
| ) { | ||
| super(); | ||
| this.editorCollectionService.addMultiDiffEditors([this as IMultiDiffEditor]); | ||
| } | ||
|
|
There was a problem hiding this comment.
构造函数注册但未解除，可能导致内存泄漏
在构造函数中调用 editorCollectionService.addMultiDiffEditors 注册自身，但 dispose() 中没有对应的移除逻辑；当多-diff editor 被销毁后，集合里仍会残留引用。建议在 dispose() 里补充反向操作。
dispose(): void {
super.dispose();
+ // 避免泄漏
+ this.editorCollectionService.removeMultiDiffEditors?.([this as IMultiDiffEditor]);
this.multiDiffWidget.dispose();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| constructor( | |
| public readonly multiDiffWidget: MultiDiffEditorWidget, | |
| private convertedOptions: IConvertedMonacoOptions, | |
| private editorCollectionService: EditorCollectionServiceImpl, | |
| ) { | |
| super(); | |
| this.editorCollectionService.addMultiDiffEditors([this as IMultiDiffEditor]); | |
| } | |
| dispose(): void { | |
| super.dispose(); | |
| + // 避免泄漏 | |
| + this.editorCollectionService.removeMultiDiffEditors?.([this as IMultiDiffEditor]); | |
| this.multiDiffWidget.dispose(); | |
| } |
🤖 Prompt for AI Agents
In packages/editor/src/browser/multi-diff/multi-diff-editor.ts around lines 75
to 83, the constructor registers the instance with
editorCollectionService.addMultiDiffEditors but the dispose() method lacks the
corresponding removal logic, causing potential memory leaks. To fix this,
override the dispose() method and call
editorCollectionService.removeMultiDiffEditors with this instance to unregister
it, ensuring references are properly cleaned up when the editor is destroyed.
| this.onCommentRangeProviderChange(() => { | ||
| this.renderCommentRange(editor); | ||
| }); |
There was a problem hiding this comment.
重复注册事件且未纳入 disposer，存在泄漏风险
onCommentRangeProviderChange 的监听在 529-533 行已通过 disposer.addDispose 注册，这里再次注册且未加入 disposer，将导致同一 editor 创建多次监听且无法自动释放。请去除重复代码或使用 disposer.addDispose 统一管理。
- this.onCommentRangeProviderChange(() => {
- this.renderCommentRange(editor);
- });
+ // 已在上方通过 disposer 注册，无需再次绑定📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| this.onCommentRangeProviderChange(() => { | |
| this.renderCommentRange(editor); | |
| }); | |
| // 已在上方通过 disposer 注册，无需再次绑定 |
🤖 Prompt for AI Agents
In packages/comments/src/browser/comments.service.ts around lines 555 to 557,
the event listener for onCommentRangeProviderChange is registered again without
adding it to the disposer, causing potential memory leaks due to multiple
listeners on the same editor. Remove this duplicate registration or ensure it is
added to the disposer using disposer.addDispose to properly manage and dispose
of the listener.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
packages/editor/src/browser/multi-diff/multi-diff-editor.ts (3)
266-278: 仍然使用setTimeout(0)，时序问题未解决
上一轮评审已提出改为 Promise/事件机制。本段代码依旧保留setTimeout，问题未得到解决。请参考既有建议完成替换。
131-158: 🛠️ Refactor suggestionDiffEditorPart 生命周期未托管
createAndRegisterEditorParts创建的DiffEditorPart仅通过editorCollectionService.addEditors注册，没有在本类或服务中统一保存 / 释放引用。
若editorCollectionService没有负责销毁这些实例，将造成悬挂对象。建议：
- 增加
private readonly registeredParts: DiffEditorPart[] = [];editors.push(...)后同步this.registeredParts.push(...)- 在
dispose()中遍历registeredParts调用dispose()。[建议示例]
+private readonly registeredParts: DiffEditorPart[] = []; ... const editors: DiffEditorPart[] = []; ... editors.push(modifiedDiffEditorPart); +this.registeredParts.push(modifiedDiffEditorPart); ... this.editorCollectionService.addEditors(editors);dispose(): void { super.dispose(); + for (const part of this.registeredParts) { + part.dispose(); + } this.multiDiffWidget.dispose(); }
75-83:⚠️ Potential issue缺少反向注销，存在内存泄漏风险
构造函数中将当前
multiDiffEditor注册进editorCollectionService，但在dispose()中并未调用对应的removeMultiDiffEditors（或类似接口）进行反向操作。
对象被销毁后，集合里仍会残留引用，导致潜在泄漏。dispose(): void { super.dispose(); + this.editorCollectionService.removeMultiDiffEditors?.([this as IMultiDiffEditor]); this.multiDiffWidget.dispose(); }packages/comments/src/browser/comments.service.ts (1)
563-570: 重复注册onCommentRangeProviderChange，会导致监听器泄漏前面 533 行已通过
disposer.addDispose绑定监听，这里再次注册且未加入disposer，与先前评审指出的问题完全相同。请删除该段或加入
disposer.addDispose(...)，避免重复监听。
🧹 Nitpick comments (1)
packages/comments/src/browser/comments.service.ts (1)
721-734: AbortController 创建频繁但缺少超时/阈值策略
renderCommentRange每次调用都会新建AbortController，高频光标移动情况下可能造成大量实例快速创建与 GC。考虑：
- 复用同一
AbortController，仅在未完成的情况下abort()并重置。- 或设置最小调用间隔/防抖。
虽非致命问题，但可显著降低开销。
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/comments/src/browser/comments.service.ts(13 hunks)packages/editor/src/browser/multi-diff/multi-diff-editor.ts(7 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: unittest (macos-latest, 18.x, node)
- GitHub Check: unittest (macos-latest, 18.x, jsdom)
- GitHub Check: build (ubuntu-latest, 20.x)
- GitHub Check: build (macos-latest, 20.x)
- GitHub Check: unittest (ubuntu-latest, 18.x, node)
- GitHub Check: unittest (ubuntu-latest, 18.x, jsdom)
- GitHub Check: build-windows
- GitHub Check: ubuntu-latest, Node.js 20.x
| const documentRefs = documents.get(); | ||
| for (const ref of documentRefs) { | ||
| if (isString(ref)) { | ||
| continue; | ||
| } | ||
| const modified = ref.object.modified; | ||
| const original = ref.object.original; | ||
| if (!modified && !original) { | ||
| continue; | ||
| } | ||
| let modifiedEditor = modified ? this.multiDiffWidget.tryGetCodeEditor(modified.uri) : undefined; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
documents 可能为 'loading'，for-of 会误遍历字符串
当 documents.get() 仍为 'loading' 时，for (const ref of documentRefs) 将按字符遍历 'loading'。
虽然内部用 isString(ref) 过滤，但仍产生不必要循环，并可能掩盖真实的等待逻辑。
-const documentRefs = documents.get();
-for (const ref of documentRefs) {
+const documentRefs = documents.get();
+if (isString(documentRefs)) {
+ // 仍在加载，等待后续 onMultiDiffModelChange 处理
+ return;
+}
+for (const ref of documentRefs as typeof documentRefs & RefCounted<IDocumentDiffItem>[]) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const documentRefs = documents.get(); | |
| for (const ref of documentRefs) { | |
| if (isString(ref)) { | |
| continue; | |
| } | |
| const modified = ref.object.modified; | |
| const original = ref.object.original; | |
| if (!modified && !original) { | |
| continue; | |
| } | |
| let modifiedEditor = modified ? this.multiDiffWidget.tryGetCodeEditor(modified.uri) : undefined; | |
| const documentRefs = documents.get(); | |
| if (isString(documentRefs)) { | |
| // 仍在加载，等待后续 onMultiDiffModelChange 处理 | |
| return; | |
| } | |
| for (const ref of documentRefs as typeof documentRefs & RefCounted<IDocumentDiffItem>[]) { | |
| if (isString(ref)) { | |
| continue; | |
| } | |
| const modified = ref.object.modified; | |
| const original = ref.object.original; | |
| if (!modified && !original) { | |
| continue; | |
| } | |
| let modifiedEditor = modified | |
| ? this.multiDiffWidget.tryGetCodeEditor(modified.uri) | |
| : undefined; | |
| // … 后续逻辑保持不变 … | |
| } |
🤖 Prompt for AI Agents
In packages/editor/src/browser/multi-diff/multi-diff-editor.ts around lines 249
to 259, the documents.get() call may return the string 'loading', causing the
for-of loop to iterate over each character of the string. To fix this, add a
check before the loop to verify that documentRefs is not a string (i.e., not
'loading') and only proceed with the loop if it is an array or iterable of
document references. This prevents unnecessary looping and avoids masking the
actual loading state.
| private editorDisposerMap = new Map<string, Disposable>(); | ||
|
|
||
| public handleOnCreateEditor(editor: IEditor) { | ||
| const disposer = new Disposable(); | ||
| if (this.allEditors.some((e) => e.monacoEditor.getId() === editor.monacoEditor.getId())) { | ||
| return this.editorDisposerMap.get(editor.monacoEditor.getId()) || disposer; | ||
| } | ||
| this.editorDisposerMap.set(editor.monacoEditor.getId(), disposer); | ||
| this.allEditors.push(editor); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
editorDisposerMap 条目未在编辑器销毁后移除
虽然在 disposer 内部对 AbortController 做了清理，但并未将对应条目从 editorDisposerMap、allEditors 中删除，长时间使用可能导致集合无限增长。
建议在 disposer 的 dispose 回调中同步执行：
+this.editorDisposerMap.delete(editor.monacoEditor.getId());
+this.allEditors = this.allEditors.filter(e => e !== editor);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/comments/src/browser/comments.service.ts around lines 308 to 316,
the editorDisposerMap and allEditors collections are not cleaned up when an
editor is destroyed, causing potential memory leaks. To fix this, add a dispose
callback to the disposer that removes the editor's entry from editorDisposerMap
and also removes the editor from allEditors when dispose is called, ensuring
proper cleanup of resources.
3 participants
Types
Background or solution
支持平铺模式下的评论能力
Changelog
support comments on multi-diff view
Summary by CodeRabbit