Feat/carbon dev #5
Conversation
WalkthroughIntroduces glob and globSync functionality with concurrent filesystem traversal and optional gitignore filtering. Refactors the Dirent type into a shared module with file-type predicate methods. Adds comprehensive tests and benchmarks for glob operations. Updates configuration and removes PR template in favor of issue template. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (11)
benchmark/glob.ts (1)
33-33: Use a neutral method to check directory existence.Using
fast-globto check fornode_modulesexistence creates a circular dependency in the benchmark, as you're using one of the libraries being benchmarked to control the benchmark flow. This could introduce bias or side effects.Apply this diff to use Node's built-in filesystem check:
+import { existsSync } from 'fs' + // ... - const hasNodeModules = fastGlob.sync('node_modules').length > 0 + const hasNodeModules = existsSync('node_modules')__test__/glob.spec.ts (1)
50-70: Improve git_ignore test pattern for more reliable verification.The test uses pattern
target/**/*.d, but Rust build artifacts (intarget/) typically don't include.dfiles. This may cause the test to always pass with empty results, reducing its effectiveness.Consider using a pattern that matches actual Rust build artifacts:
- const ignoredFiles = globSync('target/**/*.d', { cwd: CWD }) + const ignoredFiles = globSync('target/**/*.rlib', { cwd: CWD }) // Should be empty or very few if ignored (actually cargo ignores target/) // But wait, standard_filters includes .ignore and .gitignore. // If we force git_ignore: false, we should see files - const includedFiles = globSync('target/**/*.d', { cwd: CWD, git_ignore: false }) + const includedFiles = globSync('target/**/*.rlib', { cwd: CWD, git_ignore: false })Alternatively, use
target/**/*to match all files for a more comprehensive test.src/types.rs (3)
1-1: Reconsider the blanket dead_code suppression.The module-level
#![allow(dead_code)]directive suppresses warnings for all unused code, which can hide genuinely unused items.If specific items need this attribute, apply it at the item level. Otherwise, consider removing it to maintain code hygiene.
7-14: Define constants for file_type values.The
file_typefield uses magic numbers (1-7) with meanings documented only in a comment. Consider defining module-level constants to improve readability and maintainability.Add constants at the module level:
const FILE_TYPE_FILE: u8 = 1; const FILE_TYPE_DIR: u8 = 2; const FILE_TYPE_SYMLINK: u8 = 3; const FILE_TYPE_BLOCK: u8 = 4; const FILE_TYPE_CHAR: u8 = 5; const FILE_TYPE_FIFO: u8 = 6; const FILE_TYPE_SOCKET: u8 = 7; const FILE_TYPE_UNKNOWN: u8 = 0;Then update the methods:
#[napi(js_name = "isFile")] pub fn is_file(&self) -> bool { - self.file_type == 1 + self.file_type == FILE_TYPE_FILE }
53-57: Clarify deprecation status and optimize if retained.The comment marks this as a "Deprecated alias," but the public API in
index.d.ts(line 13) exposespathas a standard getter without deprecation warnings.Additionally, cloning
parent_pathon every call is inefficient. Sinceparent_pathis already exposed as a readonly field, consider:
- If deprecated: Document the replacement API and add deprecation annotations
- If retained: Return a reference instead of cloning
If retained, return a reference:
-#[napi(getter)] -pub fn path(&self) -> String { - self.parent_path.clone() -} +#[napi(getter)] +pub fn path(&self) -> &str { + &self.parent_path +}src/glob.rs (6)
24-30: Consider implementing Default for GlobOptions.The manual construction of default options is verbose. Implementing the
Defaulttrait would simplify this.Add to
GlobOptions:impl Default for GlobOptions { fn default() -> Self { Self { cwd: None, with_file_types: None, exclude: None, concurrency: None, git_ignore: None, } } }Then simplify line 24:
-let opts = options.unwrap_or(GlobOptions { - cwd: None, - with_file_types: None, - exclude: None, - concurrency: None, - git_ignore: None, -}); +let opts = options.unwrap_or_default();
32-34: Document the default concurrency value.The default concurrency of 4 threads is reasonable but arbitrary. Consider documenting why this value was chosen or making it configurable based on available CPU cores.
You could use
num_cpus::get()for a dynamic default, or simply document the rationale in a comment:+// Default to 4 threads for balanced performance across different systems let concurrency = opts.concurrency.unwrap_or(4) as usize;
57-61: Clarify git_ignore option semantics.The
git_ignoreoption name might be ambiguous. The default valuetrueenables respecting.gitignorefiles (viastandard_filters), but the name could be read as "ignore git files" rather than "use gitignore rules."Consider renaming to
respect_git_ignoreoruse_git_ignorefor clarity, or add a comment:builder .overrides(overrides) // Apply glob patterns - .standard_filters(opts.git_ignore.unwrap_or(true)) // Automatically handle .gitignore, .ignore etc + .standard_filters(opts.git_ignore.unwrap_or(true)) // When true, respect .gitignore, .ignore, etc. .threads(concurrency); // Core: Enable multithreading with one line!
107-116: Validate path component extraction edge cases.The
unwrap_orfallbacks forparent_pathandfile_namemight produce unexpected results for edge cases (root-level files, special paths). Consider validating that these components exist before proceeding.Add validation or explicit error handling:
let parent_path = relative_path .parent() .map(|p| p.to_string_lossy().to_string()) .unwrap_or_else(|| ".".to_string()); let name = relative_path .file_name() .map(|n| n.to_string_lossy().to_string()) .ok_or_else(|| /* handle missing filename */)?;
134-136: Handle or log walk errors.Silently ignoring errors during filesystem traversal can hide permission issues, broken symlinks, or other problems. Consider logging these errors or collecting them for the caller.
At minimum, add a comment explaining why errors are ignored:
Err(_) => { - // Handle errors or ignore permission errors + // Ignore permission errors and inaccessible paths to continue traversal }Or collect errors for optional reporting:
Err(e) => { // Log or collect error for debugging eprintln!("Warning: Failed to access path: {}", e); }
143-152: Improve error messages for lock failures.The generic "Lock error" and "Mutex error" messages don't provide useful debugging information. These errors typically indicate a logic bug rather than a runtime failure.
Enhance error messages:
let final_results = Arc::try_unwrap(result_dirents) - .map_err(|_| Error::from_reason("Lock error"))? + .map_err(|_| Error::from_reason("Failed to unwrap Arc: reference count > 1"))? .into_inner() - .map_err(|_| Error::from_reason("Mutex error"))?; + .map_err(|e| Error::from_reason(format!("Failed to acquire mutex lock: {}", e)))?;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (15)
.github/ISSUE_TEMPLATE.md(1 hunks).husky/pre-commit(0 hunks)Cargo.toml(1 hunks)PR_TEMPLATE.md(0 hunks)__test__/glob.spec.ts(1 hunks)__test__/readdir.spec.ts(2 hunks)benchmark/glob.ts(1 hunks)index.d.ts(1 hunks)index.js(1 hunks)package.json(2 hunks)src/glob.rs(1 hunks)src/lib.rs(1 hunks)src/readdir.rs(3 hunks)src/types.rs(1 hunks)src/utils.rs(1 hunks)
💤 Files with no reviewable changes (2)
- PR_TEMPLATE.md
- .husky/pre-commit
🧰 Additional context used
🧬 Code graph analysis (7)
src/lib.rs (3)
src/glob.rs (1)
glob(177-179)src/readdir.rs (1)
readdir(179-181)src/rm.rs (1)
rm(122-124)
benchmark/glob.ts (2)
benchmark/bench.ts (1)
runBenchmarks(7-39)benchmark/rm.ts (4)
runGroup(32-85)run(87-95)dir(88-88)dir(89-89)
index.d.ts (1)
src/glob.rs (1)
glob(177-179)
index.js (1)
benchmark/readdir.ts (3)
readdirSync(25-25)readdirSync(19-19)readdirSync(31-31)
src/glob.rs (4)
src/utils.rs (1)
get_file_type_id(4-22)src/types.rs (1)
path(55-57)index.d.ts (1)
GlobOptions(18-24)src/readdir.rs (2)
compute(170-172)resolve(173-175)
__test__/glob.spec.ts (1)
src/glob.rs (1)
glob(177-179)
src/readdir.rs (1)
src/utils.rs (1)
get_file_type_id(4-22)
🪛 LanguageTool
.github/ISSUE_TEMPLATE.md
[style] ~10-~10: Consider using a different verb for a more formal wording.
Context: ...] 🐛 Bug fix (non-breaking change which fixes an issue) - [ ]
(FIX_RESOLVE)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: stable - x86_64-pc-windows-msvc - node@22
🔇 Additional comments (11)
.github/ISSUE_TEMPLATE.md (1)
1-35: Template structure and guidance are well-designed.The issue template provides comprehensive sections with clear categorization of change types, helpful placeholder instructions, and an actionable checklist. The emoji indicators enhance visual clarity and user engagement. Users can easily identify which type of change they're submitting and provide necessary context.
This is a solid replacement for the PR template, though you may want to verify that the title suggestion
[FEAT/BUG]adequately covers all change types listed (e.g., performance improvements, refactoring, documentation updates).package.json (2)
110-110: Verify the packageManager constraint change.The packageManager field was changed from an exact version constraint to a range (
pnpm@^9 || pnpm@^10). Please confirm:
- Is this intentional to support both pnpm 9 and 10?
- If this was unintentional, what was the original constraint?
67-68: Bothfast-globandglobare correctly placed as devDependencies.The
globandglobSyncfunctions exported by this package are Rust native bindings (not JavaScript wrappers around npm packages). The npmfast-globandglobpackages are used exclusively in benchmarking (benchmark/glob.ts) to compare performance against native implementations. Consumers using the public API will not encounter runtime errors, as they receive the Rust-compiled bindings directly.Likely an incorrect or invalid review comment.
index.js (1)
575-577: LGTM!The new exports (Dirent, glob, globSync) follow the existing pattern and are properly exposed from the native binding.
src/lib.rs (1)
13-23: LGTM!The module additions (glob, types, utils) follow the existing pattern. The selective re-export of
glob::*andtypes::*while keepingutilsinternal is appropriate for encapsulation.__test__/readdir.spec.ts (1)
28-46: LGTM!The test updates correctly reflect the Dirent API change from properties to methods (
isDirectory(),isFile()), aligning with Node.js fs.Dirent conventions.src/readdir.rs (1)
1-2: LGTM!The refactor to use external
Direntfromcrate::typesandget_file_type_idutility is correctly implemented. The usage is consistent across both single-directory and recursive paths.Also applies to: 86-86, 125-125
Cargo.toml (1)
12-12: No action required; version 0.4.25 is the latest release of theignorecrate with no known security advisories.The dependency is appropriately specified and current as of October 2025.
index.d.ts (1)
3-14: LGTM: Clean Dirent API.The Dirent class API is well-designed with readonly properties and comprehensive file-type predicate methods that align with the Rust implementation.
src/glob.rs (2)
9-17: LGTM: GlobOptions structure is correct.The options structure properly mirrors the TypeScript interface definition and uses appropriate Option types for all fields.
43-51: LGTM: Exclude pattern handling is correct.The approach of prepending "!" to exclude patterns aligns with the
ignorecrate's override syntax.
| @@ -0,0 +1,35 @@ | |||
| --- | |||
| name: Feature Request / Bug Fix | |||
| about: Common template for pr and bugfix | |||
There was a problem hiding this comment.
Fix inconsistent capitalization and phrasing in the front matter.
The about field uses inconsistent capitalization and abbreviated terms. Standardize to match the title field format.
Apply this diff:
-about: Common template for pr and bugfix
+about: Common template for feature requests and bug fixes📝 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.
| about: Common template for pr and bugfix | |
| about: Common template for feature requests and bug fixes |
🤖 Prompt for AI Agents
.github/ISSUE_TEMPLATE.md around line 3: the front-matter key "about" uses
lowercase and an abbreviated term; update it to match title-style capitalization
and wording (e.g., "About: Common template for PR and Bugfix") so the front
matter is consistently capitalized and phrased like the title.
| get path(): string | ||
| } | ||
|
|
||
| export declare function glob(pattern: string, options?: GlobOptions | undefined | null): Promise<unknown> |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Strengthen the glob return type.
The return type Promise<unknown> is too loose. It should match the synchronous variant to provide proper type safety.
Apply this diff:
-export declare function glob(pattern: string, options?: GlobOptions | undefined | null): Promise<unknown>
+export declare function glob(pattern: string, options?: GlobOptions | undefined | null): Promise<Array<string> | Array<Dirent>>📝 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.
| export declare function glob(pattern: string, options?: GlobOptions | undefined | null): Promise<unknown> | |
| export declare function glob(pattern: string, options?: GlobOptions | undefined | null): Promise<Array<string> | Array<Dirent>> |
🤖 Prompt for AI Agents
In index.d.ts around line 16, the async glob declaration returns
Promise<unknown>, which is too loose; change its return type to match the
synchronous variant (use Promise<string[]>), i.e. replace Promise<unknown> with
Promise<string[]>, keep the existing parameter types unchanged, and run type
checks to ensure callers compile against the stronger type.
| "glob": "^13.0.0", | ||
| "husky": "^9.1.7", | ||
| "lint-staged": "^16.1.6", | ||
| "mitata": "^1.0.34", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find benchmark directory structure and files
find . -type d -name benchmark -o -name benchmarks 2>/dev/null | head -20Repository: CoderSerio/hyper-fs
Length of output: 74
🏁 Script executed:
# Search for tinybench and mitata imports across the codebase
echo "=== tinybench usage ==="
rg -i "tinybench" --type ts --type js --type json 2>/dev/null | head -20
echo ""
echo "=== mitata usage ==="
rg -i "mitata" --type ts --type js --type json 2>/dev/null | head -20Repository: CoderSerio/hyper-fs
Length of output: 358
🏁 Script executed:
# List all benchmark files
ls -la benchmark/
echo ""
echo "=== Check rm.ts imports ==="
head -20 benchmark/rm.ts
echo ""
echo "=== Confirm tinybench not imported anywhere ==="
rg "from ['\"]\s*tinybench" --type ts --type js 2>/dev/null || echo "No tinybench imports found"Repository: CoderSerio/hyper-fs
Length of output: 1240
Remove tinybench from devDependencies—only mitata is used.
The codebase uses only mitata for benchmarking (imported in benchmark/glob.ts and benchmark/readdir.ts). tinybench is not imported or referenced anywhere and should be removed from package.json.
🤖 Prompt for AI Agents
In package.json around line 71, devDependencies include an unused "tinybench"
entry; remove the "tinybench" dependency from the devDependencies section (leave
"mitata" intact) and run npm/yarn install or update lockfile so package.json and
package-lock.json/yarn.lock remain consistent.
| if entry.depth() == 0 { | ||
| return ignore::WalkState::Continue; | ||
| } | ||
|
|
||
| if entry.file_type().map(|ft| ft.is_dir()).unwrap_or(false) { | ||
| return ignore::WalkState::Continue; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Document the depth and directory filtering logic.
The code skips entries at depth 0 and all directories. This behavior may not match user expectations for glob patterns like "src/*" which could match directories. The lengthy comment (lines 81-88) suggests uncertainty about the intended behavior.
Either:
- Document this as the intended behavior (files-only by default)
- Make directory inclusion configurable via
GlobOptions - Match standard glob semantics more closely
Consider adding a follow_dirs or include_dirs option to GlobOptions.
🤖 Prompt for AI Agents
In src/glob.rs around lines 90-96, the code unconditionally skips entries at
depth 0 and all directories; document or make this behavior configurable. Add an
include_dirs (or follow_dirs) boolean to GlobOptions (default false to preserve
current behavior), wire it into the code that currently returns Continue for
depth 0 and directory entries so directories are only skipped when include_dirs
is false, update GlobOptions constructors/builders and any call sites, and
add/update documentation and tests to assert the new flag's behavior (and
mention the existing files-only default in the comments if you choose to keep
that default).
| } else if cfg!(unix) && ft.is_block_device() { | ||
| 4 | ||
| } else if cfg!(unix) && ft.is_char_device() { | ||
| 5 | ||
| } else if cfg!(unix) && ft.is_fifo() { | ||
| 6 | ||
| } else if cfg!(unix) && ft.is_socket() { | ||
| 7 |
There was a problem hiding this comment.
Fix Unix-specific code to compile on all platforms.
The runtime cfg!(unix) check won't prevent compilation errors on non-Unix platforms. When building on Windows, the compiler will fail because is_block_device(), is_char_device(), etc., don't exist (since FileTypeExt is not in scope).
Use compile-time #[cfg(unix)] attributes instead:
} else if ft.is_symlink() {
3
- } else if cfg!(unix) && ft.is_block_device() {
+ #[cfg(unix)]
+ } else if ft.is_block_device() {
4
- } else if cfg!(unix) && ft.is_char_device() {
+ #[cfg(unix)]
+ } else if ft.is_char_device() {
5
- } else if cfg!(unix) && ft.is_fifo() {
+ #[cfg(unix)]
+ } else if ft.is_fifo() {
6
- } else if cfg!(unix) && ft.is_socket() {
+ #[cfg(unix)]
+ } else if ft.is_socket() {
7
} else {
0
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/utils.rs around lines 11 to 18, the code uses runtime cfg!(unix) with
calls to unix-only methods (is_block_device, is_char_device, is_fifo, is_socket)
which causes compilation failures on non-Unix targets because FileTypeExt isn't
available; change to compile-time conditionalization by importing
std::os::unix::fs::FileTypeExt behind #[cfg(unix)] and moving the unix-specific
branches into #[cfg(unix)] blocks (or implement an #[cfg(not(unix))] fallback
branch) so the Unix-only methods are only referenced when compiling for Unix
targets.
Type of Change
Description
TODO
Environment
Related Issues:
[List the issue numbers related to this issue, e.g. #123]
Checklist
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.