perf(cache-utils): use native fs traversal#7104
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe cache-utils package removes Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 1
🧹 Nitpick comments (1)
packages/cache-utils/src/list.ts (1)
54-54: 🩺 Stability & Availability | 🔵 TrivialUse
currentPathinstead ofchild.parentPath
currentPathis the explicit parameter passed toreaddir, making it the canonical and safer source for constructing the full path. Relying onchild.parentPathcouples the code to specificDirentinternals which can vary across TypeScript/node type definitions and simplifies the logic.♻️ Proposed fix
- const childPath = join(child.parentPath, child.name) + const childPath = join(currentPath, child.name)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cache-utils/src/list.ts` at line 54, The path construction in readdir should use the explicit currentPath parameter instead of child.parentPath. Update the logic in readdir to build childPath from currentPath and child.name, keeping the code independent of Dirent internals and more portable across Node/TypeScript definitions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/cache-utils/src/fs.ts`:
- Around line 18-25: The cross-device fallback in fs.ts loses no-overwrite
behavior: the fs.cp path in the move helper can allow an existing destination to
remain while still deleting the source. Update the move logic in the same helper
that wraps fs.rename so the EXDEV fallback preserves overwrite: false semantics
before calling fs.rm, matching the same-device behavior and preventing source
deletion on destination collisions.
---
Nitpick comments:
In `@packages/cache-utils/src/list.ts`:
- Line 54: The path construction in readdir should use the explicit currentPath
parameter instead of child.parentPath. Update the logic in readdir to build
childPath from currentPath and child.name, keeping the code independent of
Dirent internals and more portable across Node/TypeScript definitions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 126fde29-4529-4038-a35e-57e8fb08e5e7
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
packages/cache-utils/package.jsonpackages/cache-utils/src/fs.tspackages/cache-utils/src/hash.tspackages/cache-utils/src/list.ts
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
netlify/blueprints(manual)
Switches the following: - `locate-path` -> use a regular `for...of` around `fs.access` - `move-file` -> use `fs.rename` and `fs.cp` for cross-device - `readdirp` -> use `fs.readdir`
efe1389 to
203e3cb
Compare
e18e dependency analysisNo dependency warnings found. |
| return | ||
| } | ||
|
|
||
| const digestPath = await locatePath(digests) |
There was a problem hiding this comment.
It looks like there are two behavioural changes here (see locate-path vs. fsPromises.access:
- By default, locate-path only matches on files, not directories.
- By default, paths are resolved from
process.cwd.
Maybe we can just steal the logic from its source here.
Not sure if these are important (e.g. maybe directories aren't possible? maybe the paths are always absolute? no idea).
Maybe add some test coverage for this too?
There was a problem hiding this comment.
i feel like im missing something because this is same as my other comment:
By default, paths are resolved from process.cwd
just like all fs functions, no?
There was a problem hiding this comment.
Could we add some types here while you're at it?
(For... reasons, our tsconfig is not very strict 😭.)
| export const moveCacheFile = async function (src: string, dest: string, move = false) { | ||
| // Moving is faster but removes the source files locally | ||
| if (move) { | ||
| return moveFile(src, dest, { overwrite: false }) |
There was a problem hiding this comment.
move-file resolves both src and dest from process.cwd() by default: https://npmx.dev/package/move-file#user-content-cwd
Could we add some coverage for relative paths here?
There was a problem hiding this comment.
this does too, doesn't it? as in all fs functions are relative to the cwd by default
agreed some tests will be a good idea but this behaviour is the same as before afaict
| let queue = [{ path: `${cacheDir}/${name}`, depth: 0 }] | ||
| const results: string[] = [] | ||
| let queueEntry: { path: string; depth: number } | undefined | ||
| while ((queueEntry = queue.pop())) { | ||
| const { path: currentPath, depth: currentDepth } = queueEntry | ||
| let children: Dirent[] | ||
| try { | ||
| children = await readdir(currentPath, { withFileTypes: true }) | ||
| } catch (error) { | ||
| if ((error as NodeJS.ErrnoException).code === 'ENOENT') { | ||
| continue | ||
| } | ||
| throw error | ||
| } | ||
| for (const child of children) { | ||
| const childPath = join(child.parentPath, child.name) | ||
| if ((child.isDirectory() || child.isFile()) && fileFilter(child.name)) { | ||
| results.push(join(base, relative(`${cacheDir}/${name}`, childPath))) | ||
| } | ||
| if ((depth === undefined || currentDepth < depth) && child.isDirectory()) { | ||
| queue.push({ path: childPath, depth: currentDepth + 1 }) | ||
| } | ||
| } | ||
| } | ||
| return results |
There was a problem hiding this comment.
If I'm reading all this right, I think this complexity isn't worthwhile, since the only reason we aren't using a simple readdirp('...', { recursive: true }) is because it doesn't support filtering on the fly, but since the fileFilter is actually just excluding basically one or zero files (no directories), it should be fine to filter after the fact, performance wise. What do you think?
| let queue = [{ path: `${cacheDir}/${name}`, depth: 0 }] | |
| const results: string[] = [] | |
| let queueEntry: { path: string; depth: number } | undefined | |
| while ((queueEntry = queue.pop())) { | |
| const { path: currentPath, depth: currentDepth } = queueEntry | |
| let children: Dirent[] | |
| try { | |
| children = await readdir(currentPath, { withFileTypes: true }) | |
| } catch (error) { | |
| if ((error as NodeJS.ErrnoException).code === 'ENOENT') { | |
| continue | |
| } | |
| throw error | |
| } | |
| for (const child of children) { | |
| const childPath = join(child.parentPath, child.name) | |
| if ((child.isDirectory() || child.isFile()) && fileFilter(child.name)) { | |
| results.push(join(base, relative(`${cacheDir}/${name}`, childPath))) | |
| } | |
| if ((depth === undefined || currentDepth < depth) && child.isDirectory()) { | |
| queue.push({ path: childPath, depth: currentDepth + 1 }) | |
| } | |
| } | |
| } | |
| return results | |
| const results = await readdir(`${cacheDir}/${name}`, { recursive: true }) | |
| return results.filter(({ name }) => fileFilter(name)) |
There was a problem hiding this comment.
i agree. will update 👍
2 participants
Switches the following:
locate-path-> use a regularfor...ofaroundfs.accessmove-file-> usefs.renameandfs.cpfor cross-devicereaddirp-> usefs.readdirFor us to review and ship your PR efficiently, please perform the following steps:
we can discuss the changes and get feedback from everyone that should be involved. If you`re fixing a typo or
something that`s on fire 🔥 (e.g. incident related), you can skip this step.
your code follows our style guide and passes our tests.
A picture of a cute animal (not mandatory, but encouraged)