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

feat: Supports setting certain folders not to listen recursively #4196

Closed
wants to merge 25 commits into from

Conversation

Aaaaash
Copy link
Member

@Aaaaash Aaaaash commented Dec 5, 2024

Types

  • 🎉 New Features
  • 🐛 Bug Fixes
  • 📚 Documentation Changes
  • 💄 Code Style Changes
  • 💄 Style Changes
  • 🪚 Refactors
  • 🚀 Performance Improvements
  • 🏗️ Build System
  • ⏱ Tests
  • 🧹 Chores
  • Other Changes

Background or solution

Changelog

Summary by CodeRabbit

  • 新功能

    • AppConfig 接口中添加了可选属性 unRecursiveDirectories,允许用户指定不进行递归处理的目录路径。
    • watchFileChanges 方法现在支持递归选项,用户可选择是否监控子文件夹。
  • 改进

    • 文件监控逻辑得到了增强,以便更灵活地管理文件系统事件。
    • 客户端管理逻辑得到了简化,减少了不必要的检查。

@opensumi opensumi bot added the 🎨 feature feature required label Dec 5, 2024
Copy link
Contributor

coderabbitai bot commented Dec 5, 2024

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.

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:*"

Walkthrough

此拉取请求引入了一些修改,主要包括在AppConfig接口中添加一个可选属性unRecursiveDirectories,用于指定不应递归处理的目录路径数组。此外,FileSystemProvider接口的watch方法签名进行了更新,增加了一个可选的布尔参数recursive,以支持递归监视的配置。相关的FileServiceClientDiskFileSystemProvider类中的文件监视逻辑也进行了相应调整,以利用这些新选项。

Changes

文件路径 变更摘要
packages/core-browser/src/react-providers/config-provider.tsx 添加属性:unRecursiveDirectories?: string[]AppConfig接口
packages/core-common/src/types/file.ts 更新watch方法签名:watch(uri: Uri, options: { excludes?: string[] })watch(uri: Uri, options: { excludes?: string[]; recursive?: boolean })
packages/file-service/src/browser/file-service-client.ts 更新watchFileChanges方法,增加逻辑以根据unRecursiveDirectories配置决定是否递归监视,更新调用watch方法时的参数
packages/file-service/src/node/disk-file-system.provider.ts 更新watch方法签名,添加recursive选项,重构文件监视逻辑,新增getWatcherServer方法,移除initWatchServer方法
packages/file-service/src/node/file-service.ts 更新watchFileChanges方法签名,增加recursive属性,调整实现以利用此新选项
packages/file-service/src/node/recursive/file-service-watcher.ts 更新setClient方法逻辑,修改条件检查以确保客户端管理更清晰
packages/file-service/src/node/un-recursive/file-service-watcher.ts 更新doWatchsetClient方法逻辑,简化文件变化处理和客户端管理

Possibly related PRs

  • feat: use async in watch file changes #4026: 此PR通过实现异步操作增强了文件监视功能,与主PR中添加的unRecursiveDirectories属性直接相关,因为它允许根据指定目录更细粒度地控制文件监视行为。

Suggested reviewers

  • Ricbet

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 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: 1

🧹 Outside diff range and nitpick comments (2)
packages/file-service/src/node/disk-file-system.provider.ts (1)

Line range hint 395-446: 优化 getOrCreateWatcherServer 方法的资源管理

getOrCreateWatcherServer 方法中,每次调用都会重新初始化 watcherServerDisposeCollection。在重新赋值前,应确保之前的 DisposableCollection 已正确释放其资源,以避免潜在的资源泄漏。

packages/file-service/src/node/file-service.ts (1)

106-106: 使用可选链和空值合并运算符简化代码

在设置 recursive 参数时,可以使用可选链 ?. 和空值合并运算符 ?? 来简化代码,提高可读性。建议将:

recursive: options && options.recursive !== undefined ? options.recursive : this.options.recursive,

修改为:

recursive: options?.recursive ?? this.options.recursive,
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 8031f46 and 1afdad7.

📒 Files selected for processing (5)
  • packages/core-browser/src/react-providers/config-provider.tsx (1 hunks)
  • packages/core-common/src/types/file.ts (1 hunks)
  • packages/file-service/src/browser/file-service-client.ts (2 hunks)
  • packages/file-service/src/node/disk-file-system.provider.ts (6 hunks)
  • packages/file-service/src/node/file-service.ts (1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/file-service/src/node/file-service.ts

[error] 105-105: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

🔇 Additional comments (6)
packages/file-service/src/node/disk-file-system.provider.ts (2)

156-156: 检查可选链的使用

在调用 watcherServer.unwatchFileChanges(id) 时,使用了可选链操作符 ?.。请确认 watcherServer 是否可能为 undefined,并评估在这种情况下是否需要处理未初始化的 watcherServer,以防止潜在的错误被忽略。


174-174: 确认更改 doGetStat 的深度参数是否符合预期

stat 方法中调用 doGetStat 的深度参数从 1 改为 0,这将影响文件统计信息的获取深度。请验证此更改是否符合预期,且不会对依赖该方法的功能产生负面影响。

packages/core-common/src/types/file.ts (1)

160-160: watch 方法添加可选的 recursive 参数

FileSystemProvider 接口的 watch 方法中新增了可选参数 recursive,允许调用者指定是否递归监听子目录。此更改增强了接口的灵活性,向后兼容性也得到了维护。

packages/core-browser/src/react-providers/config-provider.tsx (1)

316-319: AppConfig 接口中添加 unRecursiveDirectories 属性

新增了可选属性 unRecursiveDirectories?: string[],用于指定不需要递归监听的目录列表。这为应用程序配置提供了更精细的控制。

packages/file-service/src/browser/file-service-client.ts (2)

342-344: 根据配置确定是否递归监听

watchFileChanges 方法中,添加了对 unRecursiveDirectories 配置的检查,通过判断当前路径是否以指定目录开头,来决定是否递归监听。这增强了文件监视的灵活性。


362-362: 验证 recursive 参数的逻辑

在调用 provider.watch 时,设置了 recursive: !isUnRecursive。请确认 isUnRecursive 的逻辑正确性,确保当目录在 unRecursiveDirectories 列表中时,递归监听被正确地禁用。

@Aaaaash
Copy link
Member Author

Aaaaash commented Dec 5, 2024

/next

@opensumi
Copy link
Contributor

opensumi bot commented Dec 5, 2024

🎉 PR Next publish successful!

3.5.1-next-1733392574.0

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

🧹 Outside diff range and nitpick comments (1)
packages/file-service/src/node/disk-file-system.provider.ts (1)

Line range hint 394-439: 建议优化 watcher 创建逻辑以提高健壮性

  1. 建议在 watcherServer 创建失败时清理资源:
 if (useRecursiveServer) {
-  watcherServer = this.recursiveFileSystemWatcher || this.injector.get(FileSystemWatcherServer, [excludes]);
-  this.recursiveFileSystemWatcher = watcherServer;
+  try {
+    watcherServer = this.recursiveFileSystemWatcher || this.injector.get(FileSystemWatcherServer, [excludes]);
+    this.recursiveFileSystemWatcher = watcherServer;
+  } catch (error) {
+    this.watcherServerDisposeCollection.dispose();
+    throw error;
+  }
  1. 建议使用临时变量避免竞态条件:
-watcherServer = this.recursiveFileSystemWatcher || this.injector.get(FileSystemWatcherServer, [excludes]);
-this.recursiveFileSystemWatcher = watcherServer;
+const existingWatcher = this.recursiveFileSystemWatcher;
+watcherServer = existingWatcher || this.injector.get(FileSystemWatcherServer, [excludes]);
+if (!existingWatcher) {
+  this.recursiveFileSystemWatcher = watcherServer;
+}
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 1afdad7 and 2bb6c3c.

📒 Files selected for processing (3)
  • packages/file-service/src/node/disk-file-system.provider.ts (6 hunks)
  • packages/file-service/src/node/recursive/file-service-watcher.ts (1 hunks)
  • packages/file-service/src/node/un-recursive/file-service-watcher.ts (2 hunks)
🔇 Additional comments (6)
packages/file-service/src/node/disk-file-system.provider.ts (3)

71-79: 代码结构优化:清晰分离递归和非递归文件监视器

通过将递归和非递归文件监视器分开存储,提高了代码的组织性和可维护性。


140-150: 确保 watcherServer 在使用前已正确初始化

watch 方法中,虽然有对 watcherServer 为 undefined 的检查,但在后续的 watchFileChanges 调用中仍然可能使用未初始化的 watcherServer。建议在整个方法中增加类型保护。


173-173: 验证深度参数变更的影响

doGetStat 的深度参数从 1 改为 0 可能会影响返回的文件信息量。请确认此更改是否符合预期,特别是在需要获取目录内容的场景下。

✅ Verification successful

Let me gather more information about the doGetStat method to understand its implementation and the significance of the depth parameter.


Let me try a different pattern to find the implementation.


Let me try one more search pattern to understand the usage context.


深度参数变更是安全的

根据代码分析,将 doGetStat 的深度参数从 1 改为 0 是安全的,原因如下:

  • stat 方法中,深度参数为 0 是合适的,因为它只需要获取单个文件/目录的状态信息,不需要递归获取子项信息。
  • 需要获取子项信息的其他方法(如 doMovecopy 等)都保持了深度参数为 1,以确保正确处理目录内容。
  • doGetStat 方法中的深度参数用于控制是否通过 doGetChildren 获取目录的子项,0 表示不获取子项,1 表示获取一级子项。
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# 搜索可能依赖目录内容的调用
rg -A 3 "doGetStat.*,\s*1\s*[,)]" --type ts

Length of output: 3585


Script:

#!/bin/bash
# Search for doGetStat implementation
ast-grep --pattern 'async doGetStat($_uri, $_depth, $_options) {
  $$$
}'

Length of output: 75


Script:

#!/bin/bash
# Search for doGetStat implementation with a more flexible pattern
ast-grep --pattern 'doGetStat($_) {
  $$$
}'

# Also search for references to depth parameter usage
rg -B 3 -A 10 "depth.*=.*[01]" packages/file-service/src/node/disk-file-system.provider.ts

Length of output: 139


Script:

#!/bin/bash
# Search for the file content to understand the context
cat packages/file-service/src/node/disk-file-system.provider.ts

# Also search for any stat-related method implementations
ast-grep --pattern 'stat($$$) {
  $$$
}'

Length of output: 22430

packages/file-service/src/node/un-recursive/file-service-watcher.ts (2)

247-250: 客户端设置逻辑优化合理

改进后的条件检查更加严谨,可以防止在以下情况下设置客户端:

  1. 已经存在客户端时
  2. 观察器已被销毁时

139-146: 需要验证移除文件名检查的影响

移除了 changeFileName === signalDoc 的检查可能会导致处理一些不必要的文件变更事件。建议验证此更改在以下场景下的影响:

  1. 性能影响:是否会增加不必要的文件系统操作
  2. 行为一致性:是否会触发多余的更新事件
✅ Verification successful

移除文件名检查是安全的改动

根据代码分析,移除 changeFileName === signalDoc 的检查是合理的,原因如下:

  1. 文件变更的判断主要依赖于 fs.pathExistsSync(basePath) 检查,这是一个可靠的文件系统状态验证
  2. 原有的 signalDoc 变量仅用于存储文件名,移除其检查不会影响文件监听的核心功能
  3. 文件更新和删除事件的触发逻辑保持不变,仍然基于文件系统的实际状态
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# 检查其他文件观察器实现中是否有类似的文件名匹配逻辑
ast-grep --pattern 'if ($_ === $_) {
  $$$
}'

Length of output: 65721


Script:

#!/bin/bash
# 检查文件观察器相关的实现
rg -A 5 -B 5 "file-service-watcher" --type ts

# 检查文件变更事件的处理逻辑
ast-grep --pattern 'pushUpdated($$$)' 
ast-grep --pattern 'pushDeleted($$$)'

Length of output: 6133


Script:

#!/bin/bash
# 检查文件变更事件的处理逻辑
rg -A 10 -B 10 "pushUpdated|pushDeleted" --type ts

# 检查文件观察器的具体实现
cat packages/file-service/src/node/un-recursive/file-service-watcher.ts

Length of output: 22653

packages/file-service/src/node/recursive/file-service-watcher.ts (1)

298-301: 客户端设置逻辑保持一致性

此更改与 UnRecursiveFileSystemWatcher 中的修改保持一致,通过检查 this.clientdisposed 状态来确保:

  1. 避免重复设置客户端
  2. 防止在观察器销毁后设置客户端

@Aaaaash
Copy link
Member Author

Aaaaash commented Dec 6, 2024

/next

@opensumi
Copy link
Contributor

opensumi bot commented Dec 6, 2024

🎉 PR Next publish successful!

3.5.1-next-1733450080.0

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

🧹 Outside diff range and nitpick comments (1)
packages/file-service/src/node/disk-file-system.provider.ts (1)

Line range hint 394-439: 优化 getOrCreateWatcherServer 方法的资源管理

该方法在创建新的 watcher 之前正确地处理了资源清理,但有以下几点建议:

  1. 建议添加对现有 watcher 实例的复用逻辑,避免不必要的重新创建
  2. 考虑添加错误处理机制,确保在创建失败时能够优雅降级

建议重构为:

 protected getOrCreateWatcherServer(excludes?: string[], recursive?: boolean) {
   if (!this.injector) {
     return undefined;
   }

+  const useRecursiveServer = recursive ?? this.recursive;
+  
+  // 尝试复用现有的 watcher 实例
+  if (useRecursiveServer && this.recursiveFileSystemWatcher) {
+    return this.recursiveFileSystemWatcher;
+  } else if (!useRecursiveServer && this.unrecursiveFileSystemWatcher) {
+    return this.unrecursiveFileSystemWatcher;
+  }

   if (this.watcherServerDisposeCollection) {
     this.watcherServerDisposeCollection.dispose();
   }

   this.watcherServerDisposeCollection = new DisposableCollection();
-  const useRecursiveServer = recursive ?? this.recursive;
   let watcherServer: FileSystemWatcherServer | UnRecursiveFileSystemWatcher;

   try {
     if (useRecursiveServer) {
-      watcherServer = this.recursiveFileSystemWatcher || this.injector.get(FileSystemWatcherServer, [excludes]);
+      watcherServer = this.injector.get(FileSystemWatcherServer, [excludes]);
       this.recursiveFileSystemWatcher = watcherServer;
     } else {
-      watcherServer = this.unrecursiveFileSystemWatcher || this.injector.get(UnRecursiveFileSystemWatcher, [excludes]);
+      watcherServer = this.injector.get(UnRecursiveFileSystemWatcher, [excludes]);
       this.unrecursiveFileSystemWatcher = watcherServer;
     }
+  } catch (error) {
+    this.logger.error('Failed to create watcher server:', error);
+    return undefined;
+  }
   // ... rest of the method
 }
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 2bb6c3c and c9a10f9.

📒 Files selected for processing (1)
  • packages/file-service/src/node/disk-file-system.provider.ts (5 hunks)
🔇 Additional comments (3)
packages/file-service/src/node/disk-file-system.provider.ts (3)

71-79: 文件监视器属性定义清晰分明

属性命名和注释清楚地表明了两种不同类型的文件监视器的用途,这有助于代码的可读性和维护性。


140-150: ⚠️ Potential issue

验证 watcherServer 的错误处理逻辑

当 watcherServer 为 undefined 时,虽然返回了 -1,但后续代码仍可能使用 watcherServer,这可能导致运行时错误。

建议修改为:

 async watch(uri: UriComponents, options?: { excludes?: string[]; recursive?: boolean }): Promise<number> {
   await this.whenReady;
   const _uri = Uri.revive(uri);

   const watcherServer = this.getOrCreateWatcherServer(options?.excludes, options?.recursive);

   if (!watcherServer) {
-    return -1;
+    throw new Error('Failed to initialize watcher server');
   }

   const id = await watcherServer.watchFileChanges(_uri.toString(), {
     excludes: options?.excludes ?? [],
   });
   // ...
 }

383-383: 验证 setWatchFileExcludes 中的 watcher 更新

在更新 watchFileExcludes 后调用 getOrCreateWatcherServer,但没有处理返回值为 undefined 的情况。

@Aaaaash
Copy link
Member Author

Aaaaash commented Dec 6, 2024

/next

1 similar comment
@Aaaaash
Copy link
Member Author

Aaaaash commented Dec 6, 2024

/next

@opensumi
Copy link
Contributor

opensumi bot commented Dec 6, 2024

🎉 PR Next publish successful!

3.5.1-next-1733458435.0

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: 1

🧹 Outside diff range and nitpick comments (3)
packages/file-service/src/node/disk-file-system.provider.ts (3)

71-79: 属性命名和注释规范

属性命名和注释清晰地表达了递归和非递归文件系统观察器的用途,这是一个很好的实践。建议在注释中补充一些使用场景的说明,以帮助其他开发者更好地理解这两种观察器的应用场景。

 /**
- * recursive file system watcher
+ * recursive file system watcher
+ * 用于递归监听文件夹变化
+ * @example
+ * - 监听整个项目目录
+ * - 监听 src 目录及其所有子目录
  */

141-151: 验证 watcherServer 的初始化状态

当前实现在 watcherServer 为空时直接返回 -1,但没有记录错误日志。建议添加日志记录以便于问题排查。

   if (!watcherServer) {
+    this.logger.warn('Failed to initialize watcher server');
     return -1;
   }

Line range hint 397-424: 优化 initWatcherServer 方法的错误处理

当前实现在初始化失败时没有明确的错误处理机制。建议添加错误处理并更新 _whenReadyDeferred 的状态。

   private initWatcherServer(excludes?: string[], force = false) {
     if (!this.injector || (this.recursiveFileSystemWatcher && this.unrecursiveFileSystemWatcher && !force)) {
+      this._whenReadyDeferred.reject(new Error('Failed to initialize watcher server: injector not available'));
       return;
     }

-    this.recursiveFileSystemWatcher = this.injector.get(FileSystemWatcherServer, [excludes]);
-    this.unrecursiveFileSystemWatcher = this.injector.get(UnRecursiveFileSystemWatcher, [excludes]);
+    try {
+      this.recursiveFileSystemWatcher = this.injector.get(FileSystemWatcherServer, [excludes]);
+      this.unrecursiveFileSystemWatcher = this.injector.get(UnRecursiveFileSystemWatcher, [excludes]);
+      this._whenReadyDeferred.resolve();
+    } catch (error) {
+      this.logger.error('Failed to initialize watcher servers:', error);
+      this._whenReadyDeferred.reject(error);
+    }
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between d84eb98 and 3461a3e.

📒 Files selected for processing (1)
  • packages/file-service/src/node/disk-file-system.provider.ts (6 hunks)
🔇 Additional comments (1)
packages/file-service/src/node/disk-file-system.provider.ts (1)

384-386: 验证 setWatchFileExcludes 的调用时机

需要验证此方法在文件服务初始化完成后调用是否安全,建议添加状态检查。

✅ Verification successful

Let me gather more context about the initialization and state management of the file service.


根据代码分析结果,我可以生成最终响应了:

setWatchFileExcludes 的调用是安全的,无需额外状态检查

原因如下:

  • initWatcherServerDiskFileSystemProvider 构造函数中已经初始化
  • setWatchFileExcludes 方法内部调用 initWatcherServer 时使用了 force = true 参数,确保了即使已经初始化过也会重新初始化
  • initWatcherServer 方法本身也有防御性检查:当 injector 不存在时会直接返回

因此当前实现已经足够安全,不需要添加额外的状态检查。

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# 搜索 setWatchFileExcludes 的调用位置,确认调用时机
rg -A 3 "setWatchFileExcludes" 

Length of output: 6603


Script:

#!/bin/bash
# Search for initWatcherServer implementation and its usage
ast-grep --pattern 'initWatcherServer($_) {
  $$$
}'

# Search for DiskFileSystemProvider class initialization
ast-grep --pattern 'class DiskFileSystemProvider {
  $$$
}'

Length of output: 116


Script:

#!/bin/bash
# 搜索 initWatcherServer 的实现和调用
rg -A 10 "initWatcherServer" 

# 搜索 DiskFileSystemProvider 的构造函数和初始化相关代码
rg -A 10 "class DiskFileSystemProvider" 

Length of output: 5022

@Aaaaash
Copy link
Member Author

Aaaaash commented Dec 6, 2024

/next

@opensumi
Copy link
Contributor

opensumi bot commented Dec 6, 2024

🎉 PR Next publish successful!

3.5.1-next-1733469232.0

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: 1

🧹 Outside diff range and nitpick comments (1)
packages/file-service/src/node/disk-file-system.provider.ts (1)

Line range hint 397-430: 建议加强错误处理和类型安全性

当前实现存在以下潜在问题:

  1. 使用了非空断言操作符,可能导致运行时错误
  2. 缺少对监视器初始化失败的错误处理

建议按如下方式修改:

 private initWatcherServer(excludes?: string[], force = false) {
   if (!this.injector || (this.recursiveFileSystemWatcher && this.unrecursiveFileSystemWatcher && !force)) {
     return;
   }

   if (force) {
     this.recursiveFileSystemWatcher?.dispose();
     this.unrecursiveFileSystemWatcher?.dispose();
   }

   try {
     this.recursiveFileSystemWatcher = this.injector.get(FileSystemWatcherServer, [excludes]);
     this.unrecursiveFileSystemWatcher = this.injector.get(UnRecursiveFileSystemWatcher, [excludes]);

     const watcherClient = {
       onDidFilesChanged: (events: DidFilesChangedParams) => {
         this.logger.log(events.changes, 'events.change');
         if (events.changes.length > 0) {
           const changes = events.changes.filter((c) => !this.ignoreNextChangesEvent.has(c.uri));
           this.fileChangeEmitter.fire(changes);
           if (Array.isArray(this.rpcClient)) {
             this.rpcClient.forEach((client) => {
               client.onDidFilesChanged({
                 changes,
               });
             });
           }
         }
       },
     };

-    this.recursiveFileSystemWatcher.setClient(watcherClient);
-    this.unrecursiveFileSystemWatcher.setClient(watcherClient);
+    this.recursiveFileSystemWatcher?.setClient(watcherClient);
+    this.unrecursiveFileSystemWatcher?.setClient(watcherClient);
     this._whenReadyDeferred.resolve();
+  } catch (error) {
+    this.logger.error('Failed to initialize watcher servers:', error);
+    this._whenReadyDeferred.reject(error);
+  }
 }
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 984bdc6 and e111fc4.

📒 Files selected for processing (1)
  • packages/file-service/src/node/disk-file-system.provider.ts (6 hunks)
🔇 Additional comments (2)
packages/file-service/src/node/disk-file-system.provider.ts (2)

71-79: 代码结构清晰,文档完善!

文件监视器的属性定义合理,并且使用JSDoc注释进行了清晰的文档说明。


432-443: 🛠️ Refactor suggestion

建议改进类型安全性和错误处理

当前实现存在以下问题:

  1. 使用了非空断言操作符
  2. 缺少对未初始化监视器的处理

建议按如下方式修改:

 private getWatcherServer(recursive?: boolean) {
   const useRecursiveServer = recursive ?? this.recursive;
-  let watcherServer: FileSystemWatcherServer | UnRecursiveFileSystemWatcher;
   this.initWatcherServer();

   if (useRecursiveServer) {
-    watcherServer = this.recursiveFileSystemWatcher!;
+    if (!this.recursiveFileSystemWatcher) {
+      this.logger.error('Recursive watcher server not initialized');
+      return undefined;
+    }
+    return this.recursiveFileSystemWatcher;
   } else {
-    watcherServer = this.unrecursiveFileSystemWatcher!;
+    if (!this.unrecursiveFileSystemWatcher) {
+      this.logger.error('Unrecursive watcher server not initialized');
+      return undefined;
+    }
+    return this.unrecursiveFileSystemWatcher;
   }
-
-  return watcherServer;
 }

Likely invalid or redundant comment.

Copy link

codecov bot commented Dec 6, 2024

Codecov Report

Attention: Patch coverage is 49.84424% with 161 lines in your changes missing coverage. Please review.

Project coverage is 54.03%. Comparing base (8cdf4ec) to head (d6afb78).
Report is 23 commits behind head on main.

Files with missing lines Patch % Lines
packages/core-common/src/collections.ts 38.59% 30 Missing and 5 partials ⚠️
...tension/src/hosted/api/vscode/ext.host.treeview.ts 21.42% 11 Missing ⚠️
...notebook/src/browser/variables/inspector-script.ts 0.00% 9 Missing and 1 partial ⚠️
packages/task/src/browser/task-executor.ts 23.07% 7 Missing and 3 partials ⚠️
...ages/core-browser/src/menu/next/menubar-service.ts 11.11% 8 Missing ⚠️
.../src/browser/vscode/api/main.thread.api.webview.ts 0.00% 4 Missing and 4 partials ⚠️
...sion/src/hosted/api/vscode/ext.host.api.webview.ts 11.11% 7 Missing and 1 partial ⚠️
...es/terminal-next/src/browser/links/link-manager.ts 20.00% 5 Missing and 3 partials ⚠️
...ion/src/browser/vscode/contributes/localization.ts 75.86% 7 Missing ⚠️
...ree-next/src/browser/dialog/file-dialog.service.ts 25.00% 5 Missing and 1 partial ⚠️
... and 20 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4196      +/-   ##
==========================================
- Coverage   54.07%   54.03%   -0.05%     
==========================================
  Files        1610     1616       +6     
  Lines       97796    98005     +209     
  Branches    20019    20062      +43     
==========================================
+ Hits        52885    52956      +71     
- Misses      37310    37427     +117     
- Partials     7601     7622      +21     
Flag Coverage Δ
jsdom 49.62% <49.22%> (-0.03%) ⬇️
node 15.55% <15.88%> (-0.03%) ⬇️

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.

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

🧹 Outside diff range and nitpick comments (1)
packages/file-service/src/node/disk-file-system.provider.ts (1)

Line range hint 397-429: 优化 initWatcherServer 方法的初始化逻辑

当前实现在强制重新初始化时存在以下问题:

  1. 在调用 dispose 之前没有检查 watcher 是否存在
  2. 没有清理 watcherCollection 中的记录

建议按如下方式修改:

 private initWatcherServer(excludes?: string[], force = false) {
   if (!this.injector || (this.recursiveFileSystemWatcher && this.unrecursiveFileSystemWatcher && !force)) {
     return;
   }

   if (force) {
+    if (this.recursiveFileSystemWatcher) {
       this.recursiveFileSystemWatcher.dispose();
+    }
+    if (this.unrecursiveFileSystemWatcher) {
       this.unrecursiveFileSystemWatcher.dispose();
+    }
+    // 清理所有监视器记录
+    this.watcherCollection.clear();
   }

   this.recursiveFileSystemWatcher = this.injector.get(FileSystemWatcherServer, [excludes]);
   this.unrecursiveFileSystemWatcher = this.injector.get(UnRecursiveFileSystemWatcher, [excludes]);

   const watcherClient = {
     // ... existing code ...
   };

   this.recursiveFileSystemWatcher.setClient(watcherClient);
   this.unrecursiveFileSystemWatcher.setClient(watcherClient);
   this._whenReadyDeferred.resolve();
 }
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between e111fc4 and c3a9ab3.

📒 Files selected for processing (1)
  • packages/file-service/src/node/disk-file-system.provider.ts (6 hunks)
🔇 Additional comments (4)
packages/file-service/src/node/disk-file-system.provider.ts (4)

71-79: 属性声明清晰地区分了两种监视器类型

属性声明和注释清晰地表明了递归和非递归文件监视器的用途,这有助于代码的可维护性。


141-156: 改进监视器的错误处理和资源释放

当前实现在以下方面需要改进:

  1. 监视器服务器为空时直接返回 -1,建议添加错误日志
  2. disposable 对象在释放资源时应该移除 watcherCollection 中的记录

建议按如下方式修改:

 async watch(uri: UriComponents, options?: { excludes?: string[]; recursive?: boolean }): Promise<number> {
   await this.whenReady;
   const _uri = Uri.revive(uri);

   const watcherServer = this.getWatcherServer(options?.recursive);

   if (!watcherServer) {
+    this.logger.error('Failed to initialize watcher server');
     return -1;
   }

   const id = await watcherServer.watchFileChanges(_uri.toString(), {
     excludes: options?.excludes ?? [],
   });
   const disposable = {
     dispose: () => {
       watcherServer?.unwatchFileChanges(id);
+      this.watcherCollection.delete(_uri.toString());
     },
   };
   this.watcherCollection.set(_uri.toString(), { id, options, disposable });
   return id;
 }

431-442: 优化 getWatcherServer 方法的空值处理

getWatcherServer 方法中使用了非空断言操作符,这可能在运行时导致错误。建议添加适当的空值检查。

   private getWatcherServer(recursive?: boolean) {
     const useRecursiveServer = recursive ?? this.recursive;
-    let watcherServer: FileSystemWatcherServer | UnRecursiveFileSystemWatcher;
     this.initWatcherServer();

     if (useRecursiveServer) {
-      watcherServer = this.recursiveFileSystemWatcher!;
+      if (!this.recursiveFileSystemWatcher) {
+        this.logger.error('Recursive watcher server not initialized');
+        return undefined;
+      }
+      return this.recursiveFileSystemWatcher;
     } else {
-      watcherServer = this.unrecursiveFileSystemWatcher!;
+      if (!this.unrecursiveFileSystemWatcher) {
+        this.logger.error('Unrecursive watcher server not initialized');
+        return undefined;
+      }
+      return this.unrecursiveFileSystemWatcher;
     }
-
-    return watcherServer;
   }

384-386: 验证重新初始化监视器的影响

在重新初始化监视器时,需要确保现有的监视任务能够正确迁移到新的监视器实例。

✅ Verification successful

Let me gather more information about the watcher initialization and cleanup process.


从执行结果中我可以看到完整的监视器初始化和清理逻辑。现在我可以生成最终响应了。

重新初始化监视器的实现是安全的

代码中已经实现了完整的清理机制:

  • initWatcherServer 中会先调用 dispose() 销毁现有的监视器实例
  • 新的监视器实例会通过依赖注入重新创建
  • 所有活跃的监视任务都通过 watcherCollection 进行管理,可以正确迁移
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# 描述:验证在重新初始化监视器时是否有活跃的监视任务

# 搜索可能的监视器使用位置
rg -l "watchFileChanges|unwatchFileChanges" --type ts

# 搜索可能的文件监视相关的测试用例
rg -l "watch.*test" --type ts

Length of output: 1639


Script:

#!/bin/bash
# 检查 initWatcherServer 方法的实现和相关清理逻辑
ast-grep --pattern 'initWatcherServer($_) {
  $$$
}'

# 检查 WatcherServer 相关的实现
rg -A 10 "class.*WatcherServer" --type ts

# 检查是否有相关的清理或销毁方法
rg -A 5 "dispose|cleanup|clear" packages/file-service/src/node/disk-file-system.provider.ts

Length of output: 3199

Aaaaash and others added 10 commits December 9, 2024 10:28
* chore: add typings

* feat: support close terminal whe task finish

---------

Co-authored-by: liuqian <xubing.bxb@alibaba-inc.com>
* feat: add typings

* feat: support environmentVariableMutator.option

* fix: fix test
* fix: support ensure save file dialog by enter keyboard event

* chore: fix test

* chore: add imports and handle null dom in file dialog service

* fix: correct width calculation in resize component

* chore: remove unused context and parameter in event handler

---------

Co-authored-by: John <qingyi.xjh@antgroup.com>
* feat: add typings

* chore: fix typo

* feat: api support
dependabot bot and others added 14 commits December 9, 2024 10:28
…4176)

Bumps [webpack-bundle-analyzer](https://github.com/webpack-contrib/webpack-bundle-analyzer) from 4.10.1 to 4.10.2.
- [Changelog](https://github.com/webpack-contrib/webpack-bundle-analyzer/blob/master/CHANGELOG.md)
- [Commits](webpack-contrib/webpack-bundle-analyzer@v4.10.1...v4.10.2)

---
updated-dependencies:
- dependency-name: webpack-bundle-analyzer
  dependency-type: direct:development
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: hacke2 <xinglong.wangwxl@antgroup.com>
Bumps [@types/react-mentions](https://github.com/DefinitelyTyped/DefinitelyTyped/tree/HEAD/types/react-mentions) from 4.1.13 to 4.4.0.
- [Release notes](https://github.com/DefinitelyTyped/DefinitelyTyped/releases)
- [Commits](https://github.com/DefinitelyTyped/DefinitelyTyped/commits/HEAD/types/react-mentions)

---
updated-dependencies:
- dependency-name: "@types/react-mentions"
  dependency-type: direct:development
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: hacke2 <xinglong.wangwxl@antgroup.com>
…4179)

Bumps [cross-spawn](https://github.com/moxystudio/node-cross-spawn) from 6.0.5 to 6.0.6.
- [Changelog](https://github.com/moxystudio/node-cross-spawn/blob/v6.0.6/CHANGELOG.md)
- [Commits](moxystudio/node-cross-spawn@v6.0.5...v6.0.6)

---
updated-dependencies:
- dependency-name: cross-spawn
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: hacke2 <xinglong.wangwxl@antgroup.com>
… not load correctly (#4181)

* fix: when using language pack extensions, extensions that use i10n do not load correctly

* fix: when no language pack extension used.ConfigurationContributionPoint failed to   contribute

* fix: when no language pack extension used.ConfigurationContributionPoint failed to contribute

* fix: improve logic of LocalizationsContributionPoint.contribute

---------

Co-authored-by: hacke2 <xinglong.wangwxl@antgroup.com>
Co-authored-by: hacke2 <xinglong.wangwxl@antgroup.com>
* feat: add notebook variable inspector panel

* fix: dispose event

* chore: simplify create logic

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>

* fix: lint

* fix: missing deps

---------

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Bumps [codecov/codecov-action](https://github.com/codecov/codecov-action) from 4 to 5.
- [Release notes](https://github.com/codecov/codecov-action/releases)
- [Changelog](https://github.com/codecov/codecov-action/blob/main/CHANGELOG.md)
- [Commits](codecov/codecov-action@v4...v5)

---
updated-dependencies:
- dependency-name: codecov/codecov-action
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: hacke2 <xinglong.wangwxl@antgroup.com>
* feat: treeview support badge

* feat: webview support badge

* fix: badge effect repair

* fix: delete useless methods

* fix: handle viewbadge type

* fix: optimize syntax

* fix: change implement

* feat: compatible with the original implementation

* feat: compatible with the original implementation

* fix: format optimization

* fix: format optimization

* fix: change test case
@Aaaaash Aaaaash force-pushed the refactor/support-watch-by-unrecursive branch from c3a9ab3 to d6afb78 Compare December 9, 2024 02:28
@Aaaaash Aaaaash closed this Dec 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🎨 feature feature required
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants