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: support case sensitive #1714

Merged
merged 24 commits into from
Dec 17, 2024
Merged

Conversation

notcold
Copy link
Contributor

@notcold notcold commented Dec 3, 2024

Close #1710
Close #164

【支持大小写检测】

Summary by CodeRabbit

  • 新功能

    • 添加了条件性插件 CaseSensitivePlugin,用于处理文件名的大小写敏感性。
    • 新增配置项 caseSensitiveCheck,默认值为 true,用于控制大小写检查的启用。
    • 新增模块 case_sensitive,扩展了插件架构。
  • 文档

    • 更新了配置文档,添加了 caseSensitiveCheck 配置项的说明。
    • 更新了中文配置文档,包含 caseSensitiveCheck 配置项的说明。
  • 测试

    • 添加了新的测试用例,以验证大小写敏感性检查的功能。
  • 持续集成

    • CI工作流更新,支持在macOS和Ubuntu环境下运行构建和测试。

Copy link
Contributor

coderabbitai bot commented Dec 3, 2024

Warning

Rate limit exceeded

@notcold has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 21 minutes and 47 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between b0a2e66 and e12e460.

📒 Files selected for processing (1)
  • crates/mako/src/plugins/case_sensitive.rs (1 hunks)

概述

步骤说明

本次更改引入了一个新的大小写敏感性检查插件,专门针对 macOS 平台。该插件可以检查文件路径的大小写是否匹配,并在配置中通过 caseSensitiveCheck 选项进行控制。

变更

文件 变更摘要
crates/mako/src/compiler.rs 条件性地添加 CaseSensitivePlugin 到编译器插件列表
crates/mako/src/plugins.rs 添加新的 case_sensitive 模块
crates/mako/src/plugins/case_sensitive.rs 新增大小写敏感性检查插件实现
crates/mako/src/config.rs 添加 case_sensitive_check 配置字段
e2e/fixtures/error.case-sensitive/* 新增端到端测试相关文件
crates/binding/src/lib.rs 添加 caseSensitiveCheck 参数
docs/config.mddocs/config.zh-CN.md 更新文档,说明新的配置选项
.github/workflows/ci.yml 更新 CI 工作流以支持 macOS
crates/mako/src/config/mako.config.default.json 添加 caseSensitiveCheck 属性,默认为 false

可能相关的 PR

  • fix: 🐛 typing of stats #1712: 此 PR 涉及对 crates/binding/src/lib.rsBuildParams 结构的修改,与主 PR 中 Config 结构的 caseSensitiveCheck 字段添加相关。

诗歌

🐰 兔子编译器,大小写严谨
文件路径检查,丝毫不放松
macOS 平台上,每个字母都精准
插件闪耀光芒,代码质量添翼


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

Walkthrough

This PR introduces a new plugin, CaseSensitivePlugin, to the Mako compiler. The plugin checks for case sensitivity in file paths, ensuring that the file names match the case on disk. This enhancement aims to prevent issues related to case-insensitive file systems.

Changes

Files Summary
crates/mako/src/compiler.rs Added CaseSensitivePlugin to the list of plugins.
crates/mako/src/plugins.rs Declared the new case_sensitive module.
crates/mako/src/plugins/case_sensitive.rs Implemented the CaseSensitivePlugin with methods to check case sensitivity and integrated it into the plugin system. Includes unit tests.
crates/mako/test/build/case-sensitive/index.ts Added a test import statement to check case sensitivity.
crates/mako/test/build/case-sensitive/mako.config.json Configuration changes for testing case sensitivity.
crates/mako/test/build/case-sensitive/package.json Added package configuration for testing case sensitivity.

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

🧹 Outside diff range and nitpick comments (2)
crates/mako/src/plugins/case_sensitive.rs (1)

70-72: 建议使用日志库替换 println!

在生产环境中,直接使用 println! 进行输出可能不是最佳实践。建议使用诸如 log 等日志库,以便更好地控制日志级别和输出目的地。在第70-72行和第88行,您使用了 println!,可以考虑替换为适当的日志记录方法。

您可以参考以下修改:

- println!(
-     "File name is case-insensitive. Correct name is: {}",
-     correct_name
- );
+ log::warn!(
+     "文件名大小写不匹配,正确的名称是:{}",
+     correct_name
+ );

- println!("case_sensitive_plugin");
+ log::info!("case_sensitive_plugin");

Also applies to: 88-88

crates/mako/src/plugins.rs (1)

3-3: 代码格式一致性建议

在第3行,添加了新的模块声明 pub mod case_sensitive;。为了保持代码风格的一致性,建议在模块列表中按照字母顺序排列模块,或根据模块功能分类排序,以提高可读性和维护性。

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 569ef8d and 3d7b4de.

⛔ Files ignored due to path filters (1)
  • crates/mako/test/build/case-sensitive/assets/umi-logo.png is excluded by !**/*.png
📒 Files selected for processing (6)
  • crates/mako/src/compiler.rs (1 hunks)
  • crates/mako/src/plugins.rs (1 hunks)
  • crates/mako/src/plugins/case_sensitive.rs (1 hunks)
  • crates/mako/test/build/case-sensitive/index.ts (1 hunks)
  • crates/mako/test/build/case-sensitive/mako.config.json (1 hunks)
  • crates/mako/test/build/case-sensitive/package.json (1 hunks)
✅ Files skipped from review due to trivial changes (3)
  • crates/mako/test/build/case-sensitive/index.ts
  • crates/mako/test/build/case-sensitive/mako.config.json
  • crates/mako/test/build/case-sensitive/package.json
🔇 Additional comments (1)
crates/mako/src/compiler.rs (1)

247-247: 确认插件添加顺序是否合理

在第247行,将 CaseSensitivePlugin 添加到了内置插件列表中。插件的顺序可能影响其执行顺序,进而影响插件之间的交互。请确保 CaseSensitivePlugin 放置在合适的位置,以避免潜在的依赖问题。

Copy link

codecov bot commented Dec 4, 2024

Codecov Report

Attention: Patch coverage is 2.00000% with 49 lines in your changes missing coverage. Please review.

Project coverage is 54.78%. Comparing base (71f79db) to head (e12e460).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
crates/mako/src/plugins/case_sensitive.rs 0.00% 49 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1714      +/-   ##
==========================================
- Coverage   54.92%   54.78%   -0.15%     
==========================================
  Files         180      181       +1     
  Lines       17931    17981      +50     
==========================================
+ Hits         9848     9850       +2     
- Misses       8083     8131      +48     

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

@Jinbao1001 Jinbao1001 changed the title Feat/case sensitive feat: case sensitive Dec 4, 2024
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: 3

🧹 Outside diff range and nitpick comments (2)
crates/mako/src/plugins/case_sensitive.rs (2)

1-14: 结构设计合理,建议添加文档注释

结构体的设计使用 Arc<Mutex<HashMap>> 实现了线程安全的缓存,这是一个很好的选择。不过建议为 CaseSensitivePlugin 添加文档注释,说明其用途和工作原理。

+/// 用于检查文件名大小写敏感性的插件
+/// 
+/// 该插件维护了一个线程安全的缓存,用于存储目录下的文件名,
+/// 以避免重复的文件系统访问。
 pub struct CaseSensitivePlugin {
     cache_map: Arc<Mutex<HashMap<String, Vec<String>>>>,
 }

102-125: 建议增加更多测试用例

当前测试覆盖率不够完整,建议添加以下测试场景:

  1. 正常情况(大小写匹配)的测试
  2. node_modules 路径的跳过测试
  3. 不同层级目录的测试
  4. 缓存机制的测试
     #[test]
-    fn test_case_sensitive_checker() {
+    fn test_case_sensitive_mismatch() {
         let root = PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("test/build/case-sensitive");
         let compiler = setup_compiler("test/build/case-sensitive", false);
         let plugin = CaseSensitivePlugin::new();
         let file = &File::new(
             root.join("Assets/umi-logo.png")
                 .to_string_lossy()
                 .to_string(),
             compiler.context.clone(),
         );
         let result = plugin.load(&PluginLoadParam { file }, &compiler.context);
         assert!(result.is_err());
     }
+
+    #[test]
+    fn test_case_sensitive_match() {
+        let root = PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("test/build/case-sensitive");
+        let compiler = setup_compiler("test/build/case-sensitive", false);
+        let plugin = CaseSensitivePlugin::new();
+        let file = &File::new(
+            root.join("assets/umi-logo.png")
+                .to_string_lossy()
+                .to_string(),
+            compiler.context.clone(),
+        );
+        let result = plugin.load(&PluginLoadParam { file }, &compiler.context);
+        assert!(result.is_ok());
+    }
+
+    #[test]
+    fn test_node_modules_skip() {
+        let root = PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("test/build/case-sensitive");
+        let compiler = setup_compiler("test/build/case-sensitive", false);
+        let plugin = CaseSensitivePlugin::new();
+        let file = &File::new(
+            root.join("node_modules/package/Test.js")
+                .to_string_lossy()
+                .to_string(),
+            compiler.context.clone(),
+        );
+        let result = plugin.load(&PluginLoadParam { file }, &compiler.context);
+        assert!(result.is_ok());
+    }
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 3d7b4de and 6941fbf.

📒 Files selected for processing (1)
  • crates/mako/src/plugins/case_sensitive.rs (1 hunks)

@sorrycc
Copy link
Member

sorrycc commented Dec 12, 2024

!bench

Copy link
Member

@sorrycc sorrycc left a comment

Choose a reason for hiding this comment

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

1、用例组织请放到 e2e/fixtures 下,比如 error.case_sensitive,然后参考 https://github.com/umijs/mako/blob/master/e2e/fixtures/error.less.syntax-error/expect.js#L4 的方式对 error 做断言
2、这个功能配置开启吧,因为担心潜在的性能问题。增加一个 caseSensitiveCheck 的配置后才启用,同时应该只有 mac 下才需要,所以非 mac 平台也不需要启用这个功能

@notcold
Copy link
Contributor Author

notcold commented Dec 16, 2024

@sorrycc 调整好了,有空再review下

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

🧹 Nitpick comments (4)
crates/mako/src/plugins/case_sensitive.rs (1)

90-94: 增强错误消息的可读性

错误消息可以更清晰地描述问题。

建议修改为:

-return Err(anyhow!(
-    "{} does not match the corresponding path on disk [{}]",
-    load_param.file.path.to_string_lossy().to_string(),
-    dist_path
-));
+return Err(anyhow!(
+    "文件路径大小写不匹配\n期望: {}\n实际: {}",
+    dist_path,
+    load_param.file.path.display()
+));
crates/mako/src/compiler.rs (1)

272-275: 代码逻辑正确,建议添加注释说明

条件判断和插件初始化的逻辑正确。建议为这段代码添加注释,说明为什么只在 macOS 平台启用此插件。

建议添加如下注释:

 #[cfg(target_os = "macos")]
+// macOS 默认文件系统是大小写不敏感的,需要特殊处理
 if config.case_sensitive_check {
     plugins.push(Arc::new(plugins::case_sensitive::CaseSensitivePlugin::new()));
 }
crates/mako/src/config.rs (2)

222-225: 配置字段定义正确,建议改进文档

配置字段的定义和序列化注解使用正确。建议改进注释的表述,使其更加清晰和专业。

建议修改注释如下:

-    // 是否开启 case sensitive 检查,只有mac平台才需要开启
+    /// 是否开启文件系统大小写敏感检查
+    /// 
+    /// 由于 macOS 默认使用大小写不敏感的文件系统(Case-insensitive APFS),
+    /// 此选项仅在 macOS 平台下有效,用于检测可能由大小写引起的文件路径问题。
     #[serde(rename = "caseSensitiveCheck", default = "default_case_sensitive")]
     pub case_sensitive_check: bool,

227-229: 建议为默认值函数添加文档注释

函数实现正确,但建议添加文档注释说明其用途和行为。

建议添加如下文档:

+/// 返回是否默认开启大小写敏感检查
+/// 
+/// 仅在 macOS 平台返回 true,因为只有 macOS 的默认文件系统是大小写不敏感的。
 fn default_case_sensitive() -> bool {
     std::env::consts::OS == "macos"
 }
📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between 6941fbf and b3c8ee9.

⛔ Files ignored due to path filters (1)
  • e2e/fixtures/error.case-sensitive/assets/umi-logo.png is excluded by !**/*.png
📒 Files selected for processing (8)
  • crates/mako/src/compiler.rs (1 hunks)
  • crates/mako/src/config.rs (1 hunks)
  • crates/mako/src/plugins/case_sensitive.rs (1 hunks)
  • e2e/fixtures/error.case-sensitive/expect.js (1 hunks)
  • e2e/fixtures/error.case-sensitive/index.ts (1 hunks)
  • e2e/fixtures/error.case-sensitive/mako.config.json (1 hunks)
  • e2e/fixtures/error.case-sensitive/package.json (1 hunks)
  • packages/mako/binding.d.ts (2 hunks)
✅ Files skipped from review due to trivial changes (3)
  • e2e/fixtures/error.case-sensitive/mako.config.json
  • e2e/fixtures/error.case-sensitive/index.ts
  • e2e/fixtures/error.case-sensitive/package.json
🔇 Additional comments (4)
e2e/fixtures/error.case-sensitive/expect.js (1)

1-8: 测试用例实现正确!

测试用例正确验证了大小写敏感性检查的错误消息,确保了插件的预期行为。

crates/mako/src/plugins/case_sensitive.rs (2)

41-41: ⚠️ Potential issue

改进锁处理机制

当前的锁处理方式可能在并发场景下导致问题。建议使用更安全的错误处理方式。

建议修改为:

-let mut cache_map = self.cache_map.lock().unwrap_or_else(|e| e.into_inner());
+let mut cache_map = match self.cache_map.lock() {
+    Ok(guard) => guard,
+    Err(poisoned) => {
+        log::error!("缓存映射锁被污染: {}", poisoned);
+        return String::new();
+    }
+};

Likely invalid or redundant comment.


50-55: ⚠️ Potential issue

优化文件系统操作的错误处理

当前的文件系统操作错误处理不够健壮,可能导致运行时panic。

建议修改为:

-else if let Ok(files) = fs::read_dir(dir) {
-    files.for_each(|entry| {
-        entries.push(entry.unwrap().file_name().to_string_lossy().to_string());
-    });
-    cache_map.insert(dir.to_string(), entries.to_vec());
+else {
+    match fs::read_dir(dir) {
+        Ok(files) => {
+            for entry in files.flatten() {
+                entries.push(entry.file_name().to_string_lossy().to_string());
+            }
+            cache_map.insert(dir.to_string(), entries.clone());
+        }
+        Err(e) => {
+            log::warn!("读取目录失败: {}", e);
+        }
+    }
+}

Likely invalid or redundant comment.

packages/mako/binding.d.ts (1)

184-188: 类型定义更新正确!

新增的配置选项定义清晰,类型安全性良好。statsoptimization的扩展符合TypeScript最佳实践。

Also applies to: 239-239

Copy link
Member

@sorrycc sorrycc left a comment

Choose a reason for hiding this comment

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

1、test/build/case-sensitive 的作用是啥?没用的话可以去掉
2、config.md 和 config.zh-CN.md 里同步更新下文档的部分
3、crates/binding/src/lib.rs 里补下配置的类型

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

🧹 Nitpick comments (2)
crates/mako/src/plugins/case_sensitive.rs (2)

12-14: 建议添加结构体文档注释

为了提高代码的可维护性和可读性,建议为 CaseSensitivePlugin 结构体添加详细的文档注释,包括:

  • 结构体的主要功能和用途
  • cache_map 字段的作用和数据结构说明
  • 线程安全性相关说明
+/// 文件路径大小写敏感性检查插件
+/// 
+/// 用于验证文件系统中文件路径的大小写是否与实际匹配。
+/// 使用线程安全的缓存机制来存储目录条目,以提高性能。
 pub struct CaseSensitivePlugin {
+    /// 缓存目录内容的线程安全映射
+    /// key: 目录路径
+    /// value: 目录下的文件和文件夹名称列表
     cache_map: Arc<Mutex<HashMap<String, Vec<String>>>>,
 }

17-21: 建议添加构造函数文档注释

为构造函数添加文档注释,说明其用途和返回值。

+    /// 创建一个新的大小写敏感性检查插件实例
+    /// 
+    /// # Returns
+    /// 返回一个初始化的 `CaseSensitivePlugin` 实例,其中包含一个空的缓存映射
     pub fn new() -> Self {
         CaseSensitivePlugin {
             cache_map: Default::default(),
         }
     }
📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between 2ea746b and 8510c58.

📒 Files selected for processing (2)
  • crates/mako/src/compiler.rs (1 hunks)
  • crates/mako/src/plugins/case_sensitive.rs (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • crates/mako/src/compiler.rs
🔇 Additional comments (1)
crates/mako/src/plugins/case_sensitive.rs (1)

86-91: 改进错误消息的可读性和信息量

错误消息应该更加清晰和具有描述性,包含预期和实际的文件路径。

-                return Err(anyhow!(
-                    "{} does not match the corresponding path on disk [{}]",
-                    load_param.file.path.to_string_lossy().to_string(),
-                    dist_path
-                ));
+                return Err(anyhow!(
+                    "文件路径大小写不匹配\n预期: {}\n实际: {}",
+                    dist_path,
+                    load_param.file.path.display()
+                ));

@sorrycc
Copy link
Member

sorrycc commented Dec 17, 2024

1、mac only 的逻辑为啥删了?
2、注意下,文档里的配置是按字母排序的

@notcold
Copy link
Contributor Author

notcold commented Dec 17, 2024

ci环境不是mac单侧执行挂了,我就去掉试一下,顺序我改一下

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

🧹 Nitpick comments (4)
e2e/fixtures/error.case-sensitive/expect.js (1)

5-14: 建议改进错误处理的可维护性

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

  1. 错误消息直接硬编码在代码中
  2. 平台特定的逻辑可能需要更好的文档说明

建议进行如下改进:

+ // 定义常量提高可维护性
+ const ERROR_MESSAGES = {
+   MAC_CASE_MISMATCH: `/Assets/umi-logo.png does not match the corresponding path on disk [assets]`,
+   MISSING_INDEX: `should have file: index.js`
+ };

module.exports = (err) => {
  if (os.platform() === 'darwin') {
    assert(
-     err.stderr.includes(`/Assets/umi-logo.png does not match the corresponding path on disk [assets]`),
+     err.stderr.includes(ERROR_MESSAGES.MAC_CASE_MISMATCH),
      "should throw error"
    );
  } else {
-   assert("index.js" in files, "should have file: index.js");
+   assert("index.js" in files, ERROR_MESSAGES.MISSING_INDEX);
  }
};

另外,建议添加注释说明不同平台的处理逻辑。

crates/mako/src/plugins/case_sensitive.rs (3)

12-21: 建议添加结构体和方法的文档注释

为了提高代码的可维护性和可读性,建议为结构体和方法添加详细的文档注释。

+/// 用于检查文件路径大小写敏感性的插件
+/// 
+/// 该插件使用缓存机制来存储目录条目,以提高性能
 pub struct CaseSensitivePlugin {
+    /// 缓存映射,用于存储目录路径到其包含的文件名列表的映射
     cache_map: Arc<Mutex<HashMap<String, Vec<String>>>>,
 }

 impl CaseSensitivePlugin {
+    /// 创建一个新的 CaseSensitivePlugin 实例
+    /// 
+    /// # Returns
+    /// 返回初始化后的插件实例
     pub fn new() -> Self {
         CaseSensitivePlugin {
             cache_map: Default::default(),
         }
     }

23-34: 优化路径检查逻辑

当前实现可以通过以下几点来改进:

  1. 使用常量定义排除目录
  2. 改进路径组件的处理方式
+    /// 需要排除的目录列表
+    const EXCLUDED_DIRS: &[&str] = &["node_modules"];
+
     pub fn is_checkable(&self, load_param: &PluginLoadParam, root: &String) -> bool {
         let file_path = &load_param.file.path;
         if !file_path.starts_with(root) {
             return false;
         }
         for component in file_path.iter() {
-            if component.eq_ignore_ascii_case("node_modules") {
+            let component_str = component.to_string_lossy();
+            if Self::EXCLUDED_DIRS.iter().any(|&dir| component_str.eq_ignore_ascii_case(dir)) {
                 return false;
             }
         }
         true
     }

86-91: 改进错误消息的可读性

当前的错误消息可以更清晰地描述问题所在,帮助用户更好地理解和解决问题。

                 return Err(anyhow!(
-                    "{} does not match the corresponding path on disk [{}]",
-                    load_param.file.path.to_string_lossy().to_string(),
-                    dist_path
+                    "文件路径大小写不匹配\n期望的路径: {}\n实际的路径: {}",
+                    dist_path,
+                    load_param.file.path.display()
                 ));
📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between 8510c58 and 8b0829f.

📒 Files selected for processing (6)
  • .github/workflows/ci.yml (2 hunks)
  • crates/mako/src/compiler.rs (1 hunks)
  • crates/mako/src/plugins/case_sensitive.rs (1 hunks)
  • docs/config.md (1 hunks)
  • docs/config.zh-CN.md (2 hunks)
  • e2e/fixtures/error.case-sensitive/expect.js (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
  • docs/config.zh-CN.md
  • docs/config.md
  • crates/mako/src/compiler.rs
🔇 Additional comments (5)
e2e/fixtures/error.case-sensitive/expect.js (1)

6-11: 验证 macOS 特定逻辑的正确性

需要验证 macOS 平台下的错误处理逻辑是否与预期一致。

✅ Verification successful

Let me gather more information about the test case and its context.


让我再检查一下测试用例的目录结构,特别是大小写敏感的部分。


macOS 大小写敏感测试用例验证完成

该测试用例正确实现了对 macOS 平台特定的大小写敏感性检查:

  • 测试目录中存在小写的 assets 目录,而错误信息中检查的是大写的 Assets 路径
  • 这正确反映了 macOS 文件系统默认大小写不敏感的特性
  • 错误消息准确描述了路径不匹配的问题
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# 检查是否存在其他与大小写相关的测试用例
rg -l "case.*sensitive" --type js

# 检查是否有其他地方使用了类似的错误消息
rg "does not match the corresponding path on disk" --type js

Length of output: 248


Script:

#!/bin/bash
# 检查测试文件的完整内容
cat e2e/fixtures/error.case-sensitive/expect.js

# 查找相关的测试资源文件
fd -t f "umi-logo.png" -x ls -l {}

# 检查是否有其他平台相关的测试
rg "os\.platform\(\)" -A 2 --type js

Length of output: 1433


Script:

#!/bin/bash
# 检查测试目录结构,特别关注大小写情况
ls -la e2e/fixtures/error.case-sensitive/

# 检查是否存在大写的 Assets 目录
find e2e/fixtures/error.case-sensitive/ -type d -iname "assets"

Length of output: 574

crates/mako/src/plugins/case_sensitive.rs (2)

50-55: ⚠️ Potential issue

优化文件系统操作的错误处理和性能

当前实现存在以下问题:

  1. 直接使用 unwrap 处理文件系统操作可能导致程序崩溃
  2. 不必要的 to_vec 克隆操作影响性能
-                    } else if let Ok(files) = fs::read_dir(dir) {
-                        files.for_each(|entry| {
-                            entries.push(entry.unwrap().file_name().to_string_lossy().to_string());
-                        });
-                        cache_map.insert(dir.to_string(), entries.to_vec());
+                    } else {
+                        match fs::read_dir(dir) {
+                            Ok(files) => {
+                                for entry in files.flatten() {
+                                    entries.push(entry.file_name().to_string_lossy().to_string());
+                                }
+                                cache_map.insert(dir.to_string(), entries.clone());
+                            }
+                            Err(e) => {
+                                log::warn!("读取目录失败 '{}': {}", dir, e);
+                                continue;
+                            }
+                        }

Likely invalid or redundant comment.


41-41: ⚠️ Potential issue

改进锁的错误处理机制

直接使用 unwrap_or_else 处理锁的 PoisonError 可能会导致数据不一致。建议添加适当的错误处理。

-        let mut cache_map = self.cache_map.lock().unwrap_or_else(|e| e.into_inner());
+        let mut cache_map = match self.cache_map.lock() {
+            Ok(guard) => guard,
+            Err(poisoned) => {
+                log::error!("缓存映射锁被污染: {}", poisoned);
+                return String::new();
+            }
+        };

Likely invalid or redundant comment.

.github/workflows/ci.yml (2)

51-51: 支持 macOS 构建的更改已正确实现

添加 macos-latest 到构建矩阵是合适的,这样可以确保在 macOS 环境下测试大小写敏感性功能。


93-93: E2E 测试矩阵配置正确

在 E2E 测试中添加 macos-latest 与构建作业的更改保持一致,确保了完整的跨平台测试覆盖。

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

♻️ Duplicate comments (1)
crates/mako/src/plugins/case_sensitive.rs (1)

41-41: ⚠️ Potential issue

改进锁的错误处理

直接使用 unwrap_or_else 处理 PoisonError 可能会掩盖并发问题。

🧹 Nitpick comments (3)
crates/mako/src/plugins/case_sensitive.rs (3)

12-14: 结构体文档缺失

建议为 CaseSensitivePlugin 结构体添加文档注释,说明其用途和功能。

建议添加如下文档:

+/// 用于检查文件路径大小写敏感性的插件
+/// 
+/// 该插件主要用于在 macOS 系统上验证文件路径的大小写是否与磁盘上的实际路径匹配。
+/// 使用缓存机制来优化性能,减少文件系统访问。
pub struct CaseSensitivePlugin {
    cache_map: Arc<Mutex<HashMap<String, Vec<String>>>>,
}

86-91: 改进错误消息的可读性

错误消息可以更清晰地描述问题,并提供更多上下文信息。

建议修改为:

-                return Err(anyhow!(
-                    "{} does not match the corresponding path on disk [{}]",
-                    load_param.file.path.to_string_lossy().to_string(),
-                    dist_path
-                ));
+                return Err(anyhow!(
+                    "文件路径大小写不匹配\n期望: {}\n实际: {}\n请确保文件名大小写与磁盘上的实际文件名完全匹配",
+                    dist_path,
+                    load_param.file.path.display()
+                ));

107-119: 增强测试用例覆盖率

当前测试仅覆盖了错误情况,建议添加更多测试场景:

  1. 正常情况(大小写匹配)
  2. 不同的目录层级
  3. 特殊字符路径

建议添加如下测试用例:

#[test]
fn test_case_sensitive_checker_success() {
    let root = PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("test/build/case-sensitive");
    let compiler = setup_compiler("test/build/case-sensitive", false);
    let plugin = CaseSensitivePlugin::new();
    
    // 测试正确的大小写
    let file = &File::new(
        root.join("assets/umi-logo.png")
            .to_string_lossy()
            .to_string(),
        compiler.context.clone(),
    );
    let result = plugin.load(&PluginLoadParam { file }, &compiler.context);
    assert!(result.is_ok());
}
📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between 4ff3cf7 and b0a2e66.

📒 Files selected for processing (2)
  • crates/mako/src/config/mako.config.default.json (1 hunks)
  • crates/mako/src/plugins/case_sensitive.rs (1 hunks)
🔇 Additional comments (1)
crates/mako/src/plugins/case_sensitive.rs (1)

50-55: ⚠️ Potential issue

优化文件系统操作和错误处理

当前实现在处理文件系统操作时存在以下问题:

  1. 未正确处理 unwrap 可能导致的错误
  2. 重复克隆 entries 向量
  3. 缺少错误日志

建议修改为:

-    } else if let Ok(files) = fs::read_dir(dir) {
-        files.for_each(|entry| {
-            entries.push(entry.unwrap().file_name().to_string_lossy().to_string());
-        });
-        cache_map.insert(dir.to_string(), entries.to_vec());
+    } else {
+        match fs::read_dir(dir) {
+            Ok(files) => {
+                for entry in files {
+                    match entry {
+                        Ok(entry) => entries.push(entry.file_name().to_string_lossy().to_string()),
+                        Err(e) => log::warn!("读取目录条目失败: {}", e),
+                    }
+                }
+                cache_map.insert(dir.to_string(), entries.clone());
+            }
+            Err(e) => {
+                log::error!("读取目录 '{}' 失败: {}", dir, e);
+            }
+        }

Likely invalid or redundant comment.

@notcold
Copy link
Contributor Author

notcold commented Dec 17, 2024

@sorrycc 覆盖率怎么办,ubuntu下执行,不会触发代码逻辑,所以覆盖率就算不进去

@sorrycc
Copy link
Member

sorrycc commented Dec 17, 2024

@sorrycc 覆盖率怎么办,ubuntu下执行,不会触发代码逻辑,所以覆盖率就算不进去

覆盖率不用管。

@sorrycc sorrycc changed the title feat: case sensitive feat: support case sensitive Dec 17, 2024
@sorrycc sorrycc merged commit 9a00db2 into umijs:master Dec 17, 2024
23 of 25 checks passed
This was referenced Dec 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants