Skip to content
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

fix: comments zone widget show hide #4361

Merged
merged 4 commits into from
Feb 17, 2025
Merged

fix: comments zone widget show hide #4361

merged 4 commits into from
Feb 17, 2025

Conversation

Ricbet
Copy link
Member

@Ricbet Ricbet commented Feb 12, 2025

Types

  • 🐛 Bug Fixes

Background or solution

Changelog

修复切换文件时评论组件不显示的问题

Summary by CodeRabbit

  • Refactor
    • 优化了评论区的显示与隐藏逻辑,提升界面响应速度和稳定性。
    • 改进了评论生命周期管理,使切换评论显示时更加流畅、一致,降低了潜在错误风险。

@Ricbet Ricbet self-assigned this Feb 12, 2025
@opensumi opensumi bot added the 🐞 bug Something isn't working label Feb 12, 2025
Copy link
Contributor

coderabbitai bot commented Feb 12, 2025

Walkthrough

本次变更主要调整了评论区域组件的生命周期管理和类型安全性。具体来说,在 CommentsZoneWidget 中,将 wrapperRoot 属性的类型更改为可选,并将构造函数中 thread 参数改为 public readonly。同时增加了 renderDOM 方法用于封装渲染逻辑,确保组件装载时安全地创建和检查 DOM 元素; showhide 方法相应修改以适应新的生命周期管理。另在 CommentsService 中,将调用 thread.hideWidgetsByDispose() 改为调用 thread.hide(),优化评论线程的显示控制。

Changes

File(s) Change Summary
packages/comments/.../comments-zone.view.tsx - 将 CommentsZoneWidget 中的 wrapperRoot 类型改为 `ReactDOM.Root
packages/comments/.../comments.service.ts - 在 registerDecorationProvider 方法中,将 thread.hideWidgetsByDispose() 替换为 thread.hide()

Sequence Diagram(s)

sequenceDiagram
    participant Widget as CommentsZoneWidget
    participant Editor as IEditor
    participant Thread as ICommentsThread
    participant DOM as ReactDOM.Root

    Editor->>Widget: 构造实例 (传入 thread、options)
    Widget->>Widget: 构造函数设定 thread 为 public readonly
    Widget->>Widget: 调用 show() 方法
    Widget->>Widget: 执行 renderDOM() 方法
    Widget->>DOM: 检查 wrapperRoot,创建 _wrapper 元素并挂载组件
    Widget->>Widget: 调用 hide() 方法 → 调用 super.hide() 并重置 wrapperRoot
Loading
sequenceDiagram
    participant Service as CommentsService
    participant Thread as ICommentsThread

    Service->>Thread: 在 registerDecorationProvider 中
    Thread->>Thread: 调用 hide() 方法(替代 hideWidgetsByDispose())
Loading

Suggested reviewers

  • erha19
  • hacke2

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

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

yarn install v1.22.22
[1/4] Resolving packages...
warning eslint@8.57.1: This version is no longer supported. Please see https://eslint.org/version-support for other options.
warning eslint > @humanwhocodes/config-array@0.13.0: Use @eslint/config-array instead
warning eslint > @humanwhocodes/config-array > @humanwhocodes/object-schema@2.0.3: Use @eslint/object-schema instead
warning eslint > file-entry-cache > flat-cache > rimraf@3.0.2: Rimraf versions prior to v4 are no longer supported
warning eslint > file-entry-cache > flat-cache > rimraf > glob@7.2.3: Glob versions prior to v9 are no longer supported
warning eslint > file-entry-cache > flat-cache > rimraf > glob > inflight@1.0.6: This module is not supported, and leaks memory. Do not use it. Check out lru-cache if you want a good and tested way to coalesce async requests by a key value, which is much more comprehensive and powerful.
warning eslint-import-resolver-typescript > glob@7.2.3: Glob versions prior to v9 are no longer supported
error Couldn't find any versions for "@opensumi/ide-dev-tool" that matches "workspace:*"
info Visit https://yarnpkg.com/en/docs/cli/install for documentation about this command.


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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
packages/comments/src/browser/comments-zone.view.tsx (1)

200-223: 建议添加错误处理机制

renderDOM 方法中的 DOM 操作应该添加错误处理,以防止在以下情况出现问题:

  1. DOM 元素创建失败
  2. React root 创建失败
  3. 渲染过程中发生异常

建议添加 try-catch 块:

 private renderDOM() {
+  try {
     if (this.wrapperRoot) {
       this.wrapperRoot.unmount();
     }

     const _wrapper = document.createElement('div');
     this._container.appendChild(_wrapper);
     this.observeContainer(_wrapper);
     const customRender = this.commentsFeatureRegistry.getZoneWidgetRender();

     this.wrapperRoot = ReactDOM.createRoot(_wrapper);
     this.wrapperRoot.render(
       <ConfigProvider value={this.appConfig}>
         {customRender ? customRender(this.thread, this) : <CommentsZone thread={this.thread} widget={this} />}
       </ConfigProvider>,
     );

     this.addDispose({
       dispose: () => {
         this.wrapperRoot?.unmount();
         this.wrapperRoot = undefined;
       },
     });
+  } catch (error) {
+    console.error('Failed to render comments widget:', error);
+    // 可以在这里添加错误恢复逻辑
+  }
 }
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 81c0f99 and 16edff3.

📒 Files selected for processing (2)
  • packages/comments/src/browser/comments-zone.view.tsx (3 hunks)
  • packages/comments/src/browser/comments.service.ts (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
  • GitHub Check: unittest (ubuntu-latest, 18.x, jsdom)
  • GitHub Check: build (ubuntu-latest, 20.x)
  • GitHub Check: unittest (ubuntu-latest, 18.x, node)
  • GitHub Check: unittest (macos-latest, 18.x, jsdom)
  • GitHub Check: build (macos-latest, 20.x)
  • GitHub Check: ubuntu-latest, Node.js 20.x
  • GitHub Check: unittest (macos-latest, 18.x, node)
  • GitHub Check: build-windows
🔇 Additional comments (4)
packages/comments/src/browser/comments-zone.view.tsx (3)

178-198: 改进了组件的生命周期管理和类型安全性

以下改进提高了代码的健壮性:

  1. wrapperRoot 类型现在可以为 undefined,这样更安全
  2. thread 参数设为 public readonly,防止意外修改
  3. 在构造函数中调用 renderDOM(),确保正确初始化

233-238: 正确的显示逻辑顺序

show 方法中先调用 super.show(),然后调用 renderDOM(),这个顺序确保了组件在显示时能够正确渲染。


240-244: 优化了隐藏逻辑,避免不必要的资源销毁

hide 方法中的 dispose 改为 super.hide(),这样可以:

  1. 保持组件状态,便于后续重用
  2. 避免不必要的资源销毁和重建
  3. 提高性能
packages/comments/src/browser/comments.service.ts (1)

1031-1031: 优化了线程隐藏逻辑

hideWidgetsByDispose() 改为 hide(),这个改动:

  1. CommentsZoneWidget 的更改保持一致
  2. 改进了生命周期管理
  3. 避免了不必要的资源销毁

@erha19
Copy link
Member

erha19 commented Feb 12, 2025

这个是啥表现?好像没遇到过

@Ricbet
Copy link
Member Author

Ricbet commented Feb 12, 2025

这个是啥表现?好像没遇到过

新建评论 -> 切换文件 -> 切换回来 评论组件消失了

@Ricbet
Copy link
Member Author

Ricbet commented Feb 13, 2025

/next

@opensumi
Copy link
Contributor

opensumi bot commented Feb 13, 2025

🎉 PR Next publish successful!

3.7.1-next-1739448958.0

@hacke2
Copy link
Member

hacke2 commented Feb 17, 2025

@Ricbet 这个发个 Hotfix ?

@Ricbet Ricbet changed the base branch from main to v3.7 February 17, 2025 07:46
@Ricbet Ricbet changed the base branch from v3.7 to main February 17, 2025 07:46
@Ricbet Ricbet force-pushed the fix/comments-widget branch from 6e99f89 to 96e065b Compare February 17, 2025 07:49
@Ricbet Ricbet changed the base branch from main to v3.7 February 17, 2025 07:49
hacke2
hacke2 previously approved these changes Feb 17, 2025
Copy link
Member

@hacke2 hacke2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
packages/comments/src/browser/comments-zone.view.tsx (2)

188-198: 考虑延迟初始化渲染

在构造函数中立即调用 renderDOM() 可能过早。建议考虑以下改进:

  1. 将渲染延迟到 show() 方法首次调用时
  2. 避免在隐藏状态下创建不必要的 DOM 元素
 constructor(editor: IEditor, public readonly thread: ICommentsThread, options?: IOptions) {
   super(editor.monacoEditor, thread.range, {
     ...options,
     showInHiddenAreas: true,
   });
   this._editor = editor;

   this._isShow = !thread.isCollapsed.get();
-  this.renderDOM();
 }

200-223: 建议增加错误处理机制

renderDOM 方法的实现基本合理,但建议增加以下改进:

  1. 添加 try-catch 块来处理可能的渲染错误
  2. 在错误发生时提供回退机制
  3. 添加日志记录以便于调试
 private renderDOM() {
+  try {
     if (this.wrapperRoot) {
       this.wrapperRoot.unmount();
     }

     const _wrapper = document.createElement('div');
     this._container.appendChild(_wrapper);
     this.observeContainer(_wrapper);
     const customRender = this.commentsFeatureRegistry.getZoneWidgetRender();

     this.wrapperRoot = ReactDOM.createRoot(_wrapper);
     this.wrapperRoot.render(
       <ConfigProvider value={this.appConfig}>
         {customRender ? customRender(this.thread, this) : <CommentsZone thread={this.thread} widget={this} />}
       </ConfigProvider>,
     );

     this.addDispose({
       dispose: () => {
         this.wrapperRoot?.unmount();
         this.wrapperRoot = undefined;
       },
     });
+  } catch (error) {
+    console.error('评论区渲染失败:', error);
+    // 实现回退机制
+  }
 }
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 16edff3 and 96e065b.

📒 Files selected for processing (2)
  • packages/comments/src/browser/comments-zone.view.tsx (3 hunks)
  • packages/comments/src/browser/comments.service.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/comments/src/browser/comments.service.ts
⏰ Context from checks skipped due to timeout of 90000ms (6)
  • GitHub Check: unittest (ubuntu-latest, 18.x, jsdom)
  • GitHub Check: unittest (ubuntu-latest, 18.x, node)
  • GitHub Check: build (ubuntu-latest, 20.x)
  • GitHub Check: build (macos-latest, 20.x)
  • GitHub Check: build-windows
  • GitHub Check: ubuntu-latest, Node.js 20.x
🔇 Additional comments (2)
packages/comments/src/browser/comments-zone.view.tsx (2)

178-178: 类型定义改进!

wrapperRoot 的类型从 ReactDOM.Root 改为 ReactDOM.Root | undefined 提高了类型安全性,有助于防止空指针异常。


233-244: 显示/隐藏逻辑优化合理!

showhide 方法的修改改进了组件的生命周期管理:

  1. show 中调用 renderDOM 确保了 DOM 的正确重建
  2. 使用 super.hide() 替代 dispose 更符合组件生命周期管理的最佳实践

Copy link

codecov bot commented Feb 17, 2025

Codecov Report

Attention: Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Project coverage is 54.20%. Comparing base (81708ca) to head (7593567).
Report is 1 commits behind head on v3.7.

Files with missing lines Patch % Lines
packages/comments/src/browser/comments.service.ts 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             v3.7    #4361      +/-   ##
==========================================
- Coverage   54.21%   54.20%   -0.01%     
==========================================
  Files        1634     1634              
  Lines       99917    99917              
  Branches    21691    21699       +8     
==========================================
- Hits        54169    54162       -7     
- Misses      38006    38011       +5     
- Partials     7742     7744       +2     
Flag Coverage Δ
jsdom 49.74% <0.00%> (-0.01%) ⬇️
node 12.24% <0.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@hacke2 hacke2 merged commit de91327 into v3.7 Feb 17, 2025
11 checks passed
@hacke2 hacke2 deleted the fix/comments-widget branch February 17, 2025 12:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐞 bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants