fix(utils,core,action): normalize path handling and test guards#2809
fix(utils,core,action): normalize path handling and test guards#2809
Conversation
🦋 Changeset detectedLatest commit: 1d5e3ee The changes in this PR will be included in the next version bump. This PR includes changesets to release 31 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces ad-hoc string path handling with Node path utilities across utils and core; adds getDirectoryPath and directory batching in compress; normalizes Windows backslashes to POSIX paths; extends tests for backslash/publicFolder cases; relaxes one fast-glob version assertion. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as CLI/Caller
participant Compress as packages/core/compress
participant FS as node:fs/promises
participant Compressor as compressSingleFile
CLI->>Compress: run compress(settings)
Compress->>Compress: for each outputPath → getDirectoryPath(outputPath)
Compress->>FS: mkdir(uniqueDirs..., { recursive: true })
Compress->>Compressor: compressSingleFile(input, outputPath)
Compressor-->>Compress: { code: "ok" }
Compress->>FS: writeFile(outputPath, code)
Compress-->>CLI: results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ✏️ Tip: You can disable in-progress messages and the fortune message in your review settings. Tip You can disable the changed files summary in the walkthrough.Disable the ✨ 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 |
2 similar comments
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ✏️ Tip: You can disable in-progress messages and the fortune message in your review settings. Tip You can disable the changed files summary in the walkthrough.Disable the ✨ 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 |
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ✏️ Tip: You can disable in-progress messages and the fortune message in your review settings. Tip You can disable the changed files summary in the walkthrough.Disable the ✨ 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Greptile OverviewGreptile SummaryThis PR standardizes path handling across Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Caller as User/Action/CLI
participant Core as @node-minify/core compress()
participant Dir as createDirectory()/getDirectoryPath()
participant FS as node:fs/promises.mkdir
participant Utils as @node-minify/utils
Caller->>Core: compress(settings)
alt settings.output provided
Core->>Dir: createDirectory(settings.output)
Dir->>Dir: getDirectoryPath(outputPath)
Dir->>FS: mkdir(dirPath, {recursive:true})
end
alt output is array
Core->>Utils: run({settings, content, index}) (per file)
else output is string/undefined
Core->>Utils: compressSingleFile(settings)
end
|
Greptile OverviewGreptile SummaryThis PR improves path handling across utils and core packages by replacing string manipulation with proper Key Changes
Testing
Notes
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant compress
participant utils
participant path_module as node:path
participant fs as node:fs/promises
User->>compress: compress(settings)
alt output specified
compress->>compress: createDirectory(output)
compress->>path_module: path.posix.dirname(outputPath)
path_module-->>compress: dirPath (POSIX)
compress->>path_module: path.win32.dirname(outputPath)
path_module-->>compress: dirPath (Windows)
compress->>compress: choose first non-trivial dirPath
compress->>fs: mkdir(dirPath, {recursive: true})
fs-->>compress: directory created
end
alt single file
compress->>utils: compressSingleFile(settings)
utils-->>compress: result
else multiple files
compress->>compress: compressArrayOfFiles(settings)
loop for each input file
compress->>utils: getContentFromFilesAsync(input)
utils-->>compress: content
compress->>utils: run({settings, content, index})
utils-->>compress: compressed result
end
end
User->>utils: setPublicFolder(input, publicFolder)
utils->>path_module: path.normalize(publicFolder)
path_module-->>utils: normalized path
utils->>path_module: path.relative(publicFolder, filePath)
path_module-->>utils: relativePath
alt already in publicFolder
utils->>utils: return normalized path
else not in publicFolder
utils->>path_module: path.join(publicFolder, filePath)
path_module-->>utils: joined path
end
utils-->>User: {input: processedPaths}
User->>utils: wildcards(pattern, publicFolder)
utils->>path_module: path.join(publicFolder, pattern)
path_module-->>utils: fullPath
utils->>fg: fg.globSync(fullPath, options)
fg-->>utils: matched files
utils-->>User: {input: files}
User->>utils: setFileNameMin(file, output, publicFolder)
utils->>path_module: path.parse(file)
path_module-->>utils: {dir, name, ext}
alt publicFolder specified
utils->>path_module: path.join(publicFolder, name)
path_module-->>utils: fileWithoutExtension
end
alt replaceInPlace
utils->>path_module: path.join(dir, fileWithoutExtension)
path_module-->>utils: fileWithoutExtension
end
utils->>utils: output.replace("$1", fileWithoutExtension)
utils-->>User: minified filename
|
3 similar comments
Greptile OverviewGreptile SummaryThis PR improves path handling across utils and core packages by replacing string manipulation with proper Key Changes
Testing
Notes
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant compress
participant utils
participant path_module as node:path
participant fs as node:fs/promises
User->>compress: compress(settings)
alt output specified
compress->>compress: createDirectory(output)
compress->>path_module: path.posix.dirname(outputPath)
path_module-->>compress: dirPath (POSIX)
compress->>path_module: path.win32.dirname(outputPath)
path_module-->>compress: dirPath (Windows)
compress->>compress: choose first non-trivial dirPath
compress->>fs: mkdir(dirPath, {recursive: true})
fs-->>compress: directory created
end
alt single file
compress->>utils: compressSingleFile(settings)
utils-->>compress: result
else multiple files
compress->>compress: compressArrayOfFiles(settings)
loop for each input file
compress->>utils: getContentFromFilesAsync(input)
utils-->>compress: content
compress->>utils: run({settings, content, index})
utils-->>compress: compressed result
end
end
User->>utils: setPublicFolder(input, publicFolder)
utils->>path_module: path.normalize(publicFolder)
path_module-->>utils: normalized path
utils->>path_module: path.relative(publicFolder, filePath)
path_module-->>utils: relativePath
alt already in publicFolder
utils->>utils: return normalized path
else not in publicFolder
utils->>path_module: path.join(publicFolder, filePath)
path_module-->>utils: joined path
end
utils-->>User: {input: processedPaths}
User->>utils: wildcards(pattern, publicFolder)
utils->>path_module: path.join(publicFolder, pattern)
path_module-->>utils: fullPath
utils->>fg: fg.globSync(fullPath, options)
fg-->>utils: matched files
utils-->>User: {input: files}
User->>utils: setFileNameMin(file, output, publicFolder)
utils->>path_module: path.parse(file)
path_module-->>utils: {dir, name, ext}
alt publicFolder specified
utils->>path_module: path.join(publicFolder, name)
path_module-->>utils: fileWithoutExtension
end
alt replaceInPlace
utils->>path_module: path.join(dir, fileWithoutExtension)
path_module-->>utils: fileWithoutExtension
end
utils->>utils: output.replace("$1", fileWithoutExtension)
utils-->>User: minified filename
|
Greptile OverviewGreptile SummaryThis PR improves path handling across utils and core packages by replacing string manipulation with proper Key Changes
Testing
Notes
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant compress
participant utils
participant path_module as node:path
participant fs as node:fs/promises
User->>compress: compress(settings)
alt output specified
compress->>compress: createDirectory(output)
compress->>path_module: path.posix.dirname(outputPath)
path_module-->>compress: dirPath (POSIX)
compress->>path_module: path.win32.dirname(outputPath)
path_module-->>compress: dirPath (Windows)
compress->>compress: choose first non-trivial dirPath
compress->>fs: mkdir(dirPath, {recursive: true})
fs-->>compress: directory created
end
alt single file
compress->>utils: compressSingleFile(settings)
utils-->>compress: result
else multiple files
compress->>compress: compressArrayOfFiles(settings)
loop for each input file
compress->>utils: getContentFromFilesAsync(input)
utils-->>compress: content
compress->>utils: run({settings, content, index})
utils-->>compress: compressed result
end
end
User->>utils: setPublicFolder(input, publicFolder)
utils->>path_module: path.normalize(publicFolder)
path_module-->>utils: normalized path
utils->>path_module: path.relative(publicFolder, filePath)
path_module-->>utils: relativePath
alt already in publicFolder
utils->>utils: return normalized path
else not in publicFolder
utils->>path_module: path.join(publicFolder, filePath)
path_module-->>utils: joined path
end
utils-->>User: {input: processedPaths}
User->>utils: wildcards(pattern, publicFolder)
utils->>path_module: path.join(publicFolder, pattern)
path_module-->>utils: fullPath
utils->>fg: fg.globSync(fullPath, options)
fg-->>utils: matched files
utils-->>User: {input: files}
User->>utils: setFileNameMin(file, output, publicFolder)
utils->>path_module: path.parse(file)
path_module-->>utils: {dir, name, ext}
alt publicFolder specified
utils->>path_module: path.join(publicFolder, name)
path_module-->>utils: fileWithoutExtension
end
alt replaceInPlace
utils->>path_module: path.join(dir, fileWithoutExtension)
path_module-->>utils: fileWithoutExtension
end
utils->>utils: output.replace("$1", fileWithoutExtension)
utils-->>User: minified filename
|
Greptile OverviewGreptile SummaryThis PR improves path handling across utils and core packages by replacing string manipulation with proper Key Changes
Testing
Notes
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant compress
participant utils
participant path_module as node:path
participant fs as node:fs/promises
User->>compress: compress(settings)
alt output specified
compress->>compress: createDirectory(output)
compress->>path_module: path.posix.dirname(outputPath)
path_module-->>compress: dirPath (POSIX)
compress->>path_module: path.win32.dirname(outputPath)
path_module-->>compress: dirPath (Windows)
compress->>compress: choose first non-trivial dirPath
compress->>fs: mkdir(dirPath, {recursive: true})
fs-->>compress: directory created
end
alt single file
compress->>utils: compressSingleFile(settings)
utils-->>compress: result
else multiple files
compress->>compress: compressArrayOfFiles(settings)
loop for each input file
compress->>utils: getContentFromFilesAsync(input)
utils-->>compress: content
compress->>utils: run({settings, content, index})
utils-->>compress: compressed result
end
end
User->>utils: setPublicFolder(input, publicFolder)
utils->>path_module: path.normalize(publicFolder)
path_module-->>utils: normalized path
utils->>path_module: path.relative(publicFolder, filePath)
path_module-->>utils: relativePath
alt already in publicFolder
utils->>utils: return normalized path
else not in publicFolder
utils->>path_module: path.join(publicFolder, filePath)
path_module-->>utils: joined path
end
utils-->>User: {input: processedPaths}
User->>utils: wildcards(pattern, publicFolder)
utils->>path_module: path.join(publicFolder, pattern)
path_module-->>utils: fullPath
utils->>fg: fg.globSync(fullPath, options)
fg-->>utils: matched files
utils-->>User: {input: files}
User->>utils: setFileNameMin(file, output, publicFolder)
utils->>path_module: path.parse(file)
path_module-->>utils: {dir, name, ext}
alt publicFolder specified
utils->>path_module: path.join(publicFolder, name)
path_module-->>utils: fileWithoutExtension
end
alt replaceInPlace
utils->>path_module: path.join(dir, fileWithoutExtension)
path_module-->>utils: fileWithoutExtension
end
utils->>utils: output.replace("$1", fileWithoutExtension)
utils-->>User: minified filename
|
📝 WalkthroughWalkthroughThe changes enhance cross-platform path handling throughout the codebase by replacing string concatenation with Node's Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
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 |
Greptile OverviewGreptile SummaryThis PR improves path handling across utils and core packages by replacing string manipulation with proper Key Changes
Testing
Notes
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant compress
participant utils
participant path_module as node:path
participant fs as node:fs/promises
User->>compress: compress(settings)
alt output specified
compress->>compress: createDirectory(output)
compress->>path_module: path.posix.dirname(outputPath)
path_module-->>compress: dirPath (POSIX)
compress->>path_module: path.win32.dirname(outputPath)
path_module-->>compress: dirPath (Windows)
compress->>compress: choose first non-trivial dirPath
compress->>fs: mkdir(dirPath, {recursive: true})
fs-->>compress: directory created
end
alt single file
compress->>utils: compressSingleFile(settings)
utils-->>compress: result
else multiple files
compress->>compress: compressArrayOfFiles(settings)
loop for each input file
compress->>utils: getContentFromFilesAsync(input)
utils-->>compress: content
compress->>utils: run({settings, content, index})
utils-->>compress: compressed result
end
end
User->>utils: setPublicFolder(input, publicFolder)
utils->>path_module: path.normalize(publicFolder)
path_module-->>utils: normalized path
utils->>path_module: path.relative(publicFolder, filePath)
path_module-->>utils: relativePath
alt already in publicFolder
utils->>utils: return normalized path
else not in publicFolder
utils->>path_module: path.join(publicFolder, filePath)
path_module-->>utils: joined path
end
utils-->>User: {input: processedPaths}
User->>utils: wildcards(pattern, publicFolder)
utils->>path_module: path.join(publicFolder, pattern)
path_module-->>utils: fullPath
utils->>fg: fg.globSync(fullPath, options)
fg-->>utils: matched files
utils-->>User: {input: files}
User->>utils: setFileNameMin(file, output, publicFolder)
utils->>path_module: path.parse(file)
path_module-->>utils: {dir, name, ext}
alt publicFolder specified
utils->>path_module: path.join(publicFolder, name)
path_module-->>utils: fileWithoutExtension
end
alt replaceInPlace
utils->>path_module: path.join(dir, fileWithoutExtension)
path_module-->>utils: fileWithoutExtension
end
utils->>utils: output.replace("$1", fileWithoutExtension)
utils-->>User: minified filename
|
3 similar comments
Greptile OverviewGreptile SummaryThis PR improves path handling across utils and core packages by replacing string manipulation with proper Key Changes
Testing
Notes
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant compress
participant utils
participant path_module as node:path
participant fs as node:fs/promises
User->>compress: compress(settings)
alt output specified
compress->>compress: createDirectory(output)
compress->>path_module: path.posix.dirname(outputPath)
path_module-->>compress: dirPath (POSIX)
compress->>path_module: path.win32.dirname(outputPath)
path_module-->>compress: dirPath (Windows)
compress->>compress: choose first non-trivial dirPath
compress->>fs: mkdir(dirPath, {recursive: true})
fs-->>compress: directory created
end
alt single file
compress->>utils: compressSingleFile(settings)
utils-->>compress: result
else multiple files
compress->>compress: compressArrayOfFiles(settings)
loop for each input file
compress->>utils: getContentFromFilesAsync(input)
utils-->>compress: content
compress->>utils: run({settings, content, index})
utils-->>compress: compressed result
end
end
User->>utils: setPublicFolder(input, publicFolder)
utils->>path_module: path.normalize(publicFolder)
path_module-->>utils: normalized path
utils->>path_module: path.relative(publicFolder, filePath)
path_module-->>utils: relativePath
alt already in publicFolder
utils->>utils: return normalized path
else not in publicFolder
utils->>path_module: path.join(publicFolder, filePath)
path_module-->>utils: joined path
end
utils-->>User: {input: processedPaths}
User->>utils: wildcards(pattern, publicFolder)
utils->>path_module: path.join(publicFolder, pattern)
path_module-->>utils: fullPath
utils->>fg: fg.globSync(fullPath, options)
fg-->>utils: matched files
utils-->>User: {input: files}
User->>utils: setFileNameMin(file, output, publicFolder)
utils->>path_module: path.parse(file)
path_module-->>utils: {dir, name, ext}
alt publicFolder specified
utils->>path_module: path.join(publicFolder, name)
path_module-->>utils: fileWithoutExtension
end
alt replaceInPlace
utils->>path_module: path.join(dir, fileWithoutExtension)
path_module-->>utils: fileWithoutExtension
end
utils->>utils: output.replace("$1", fileWithoutExtension)
utils-->>User: minified filename
|
Greptile OverviewGreptile SummaryThis PR improves path handling across utils and core packages by replacing string manipulation with proper Key Changes
Testing
Notes
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant compress
participant utils
participant path_module as node:path
participant fs as node:fs/promises
User->>compress: compress(settings)
alt output specified
compress->>compress: createDirectory(output)
compress->>path_module: path.posix.dirname(outputPath)
path_module-->>compress: dirPath (POSIX)
compress->>path_module: path.win32.dirname(outputPath)
path_module-->>compress: dirPath (Windows)
compress->>compress: choose first non-trivial dirPath
compress->>fs: mkdir(dirPath, {recursive: true})
fs-->>compress: directory created
end
alt single file
compress->>utils: compressSingleFile(settings)
utils-->>compress: result
else multiple files
compress->>compress: compressArrayOfFiles(settings)
loop for each input file
compress->>utils: getContentFromFilesAsync(input)
utils-->>compress: content
compress->>utils: run({settings, content, index})
utils-->>compress: compressed result
end
end
User->>utils: setPublicFolder(input, publicFolder)
utils->>path_module: path.normalize(publicFolder)
path_module-->>utils: normalized path
utils->>path_module: path.relative(publicFolder, filePath)
path_module-->>utils: relativePath
alt already in publicFolder
utils->>utils: return normalized path
else not in publicFolder
utils->>path_module: path.join(publicFolder, filePath)
path_module-->>utils: joined path
end
utils-->>User: {input: processedPaths}
User->>utils: wildcards(pattern, publicFolder)
utils->>path_module: path.join(publicFolder, pattern)
path_module-->>utils: fullPath
utils->>fg: fg.globSync(fullPath, options)
fg-->>utils: matched files
utils-->>User: {input: files}
User->>utils: setFileNameMin(file, output, publicFolder)
utils->>path_module: path.parse(file)
path_module-->>utils: {dir, name, ext}
alt publicFolder specified
utils->>path_module: path.join(publicFolder, name)
path_module-->>utils: fileWithoutExtension
end
alt replaceInPlace
utils->>path_module: path.join(dir, fileWithoutExtension)
path_module-->>utils: fileWithoutExtension
end
utils->>utils: output.replace("$1", fileWithoutExtension)
utils-->>User: minified filename
|
Greptile OverviewGreptile SummaryThis PR improves path handling across utils and core packages by replacing string manipulation with proper Key Changes
Testing
Notes
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant compress
participant utils
participant path_module as node:path
participant fs as node:fs/promises
User->>compress: compress(settings)
alt output specified
compress->>compress: createDirectory(output)
compress->>path_module: path.posix.dirname(outputPath)
path_module-->>compress: dirPath (POSIX)
compress->>path_module: path.win32.dirname(outputPath)
path_module-->>compress: dirPath (Windows)
compress->>compress: choose first non-trivial dirPath
compress->>fs: mkdir(dirPath, {recursive: true})
fs-->>compress: directory created
end
alt single file
compress->>utils: compressSingleFile(settings)
utils-->>compress: result
else multiple files
compress->>compress: compressArrayOfFiles(settings)
loop for each input file
compress->>utils: getContentFromFilesAsync(input)
utils-->>compress: content
compress->>utils: run({settings, content, index})
utils-->>compress: compressed result
end
end
User->>utils: setPublicFolder(input, publicFolder)
utils->>path_module: path.normalize(publicFolder)
path_module-->>utils: normalized path
utils->>path_module: path.relative(publicFolder, filePath)
path_module-->>utils: relativePath
alt already in publicFolder
utils->>utils: return normalized path
else not in publicFolder
utils->>path_module: path.join(publicFolder, filePath)
path_module-->>utils: joined path
end
utils-->>User: {input: processedPaths}
User->>utils: wildcards(pattern, publicFolder)
utils->>path_module: path.join(publicFolder, pattern)
path_module-->>utils: fullPath
utils->>fg: fg.globSync(fullPath, options)
fg-->>utils: matched files
utils-->>User: {input: files}
User->>utils: setFileNameMin(file, output, publicFolder)
utils->>path_module: path.parse(file)
path_module-->>utils: {dir, name, ext}
alt publicFolder specified
utils->>path_module: path.join(publicFolder, name)
path_module-->>utils: fileWithoutExtension
end
alt replaceInPlace
utils->>path_module: path.join(dir, fileWithoutExtension)
path_module-->>utils: fileWithoutExtension
end
utils->>utils: output.replace("$1", fileWithoutExtension)
utils-->>User: minified filename
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
packages/core/src/compress.ts
Outdated
| // Support POSIX and Windows path separators consistently. | ||
| const posixDirPath = path.posix.dirname(outputPath); | ||
| const windowsDirPath = path.win32.dirname(outputPath); | ||
| const dirPath = | ||
| posixDirPath !== "." && posixDirPath !== "" | ||
| ? posixDirPath | ||
| : windowsDirPath !== "." && windowsDirPath !== "" | ||
| ? windowsDirPath | ||
| : ""; |
There was a problem hiding this comment.
simplified approach using path.dirname() directly
The current logic tries both path.posix.dirname() and path.win32.dirname() and picks the first non-trivial result. This can be simplified to just use path.dirname() which automatically uses the correct separator for the current platform:
| // Support POSIX and Windows path separators consistently. | |
| const posixDirPath = path.posix.dirname(outputPath); | |
| const windowsDirPath = path.win32.dirname(outputPath); | |
| const dirPath = | |
| posixDirPath !== "." && posixDirPath !== "" | |
| ? posixDirPath | |
| : windowsDirPath !== "." && windowsDirPath !== "" | |
| ? windowsDirPath | |
| : ""; | |
| // Extract directory path using platform-appropriate separator handling | |
| const dirPath = path.dirname(outputPath); | |
| // Early return if no directory path (dirname returns "." for files in current dir) | |
| if (!dirPath || dirPath === ".") { |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/core/src/compress.ts
Line: 114:122
Comment:
simplified approach using `path.dirname()` directly
The current logic tries both `path.posix.dirname()` and `path.win32.dirname()` and picks the first non-trivial result. This can be simplified to just use `path.dirname()` which automatically uses the correct separator for the current platform:
```suggestion
// Extract directory path using platform-appropriate separator handling
const dirPath = path.dirname(outputPath);
// Early return if no directory path (dirname returns "." for files in current dir)
if (!dirPath || dirPath === ".") {
```
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.
📝 WalkthroughWalkthroughThe changes enhance cross-platform path handling throughout the codebase by replacing string concatenation with Node's Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
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 |
2 similar comments
📝 WalkthroughWalkthroughThe changes enhance cross-platform path handling throughout the codebase by replacing string concatenation with Node's Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
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 |
📝 WalkthroughWalkthroughThe changes enhance cross-platform path handling throughout the codebase by replacing string concatenation with Node's Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
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 |
Additional Comments (1)
The current logic tries both Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! |
8 similar comments
Additional Comments (1)
The current logic tries both Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! |
Additional Comments (1)
The current logic tries both Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! |
Additional Comments (1)
The current logic tries both Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! |
Additional Comments (1)
The current logic tries both Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! |
Additional Comments (1)
The current logic tries both Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! |
Additional Comments (1)
The current logic tries both Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! |
Additional Comments (1)
The current logic tries both Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! |
Additional Comments (1)
The current logic tries both Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! |
Additional Comments (1)
The current logic tries both Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@packages/core/__tests__/compress-paths.test.ts`:
- Around line 25-39: The test currently asserts mkdir was called with a
backslash path but if getDirectoryPath is changed to normalize separators,
update the assertion in compress-paths.test.ts to expect the normalized
directory string (e.g., "nested/dir") instead of "nested\\dir"; locate the test
referencing compress(settings) and the mocked mkdir and change the expected path
to match getDirectoryPath's normalized output so
vi.mocked(mkdir).toHaveBeenCalledWith("nested/dir", { recursive: true }) remains
correct.
In `@packages/core/src/compress.ts`:
- Around line 136-148: getDirectoryPath currently may return a Win32 path with
backslashes (from path.win32.dirname) which can create a single directory with a
literal backslash on POSIX; update getDirectoryPath to normalize the win32
result to POSIX separators before returning (e.g., replace backslashes with '/'
and optionally run path.posix.normalize on that string) so mkdir receives a
proper nested path; keep the existing checks for "." and empty string and only
apply the replacement/normalization to the windowsDirPath branch in
getDirectoryPath.
🧹 Nitpick comments (2)
packages/utils/src/wildcards.ts (1)
79-79: Pre-existing: filter callback parameterpathshadows the importedpathmodule.Not introduced by this PR, but the filter callbacks at lines 79 and 108 use
pathas the parameter name, which shadows the newly importedpathmodule. If the filter body ever needspath.xxx, it would silently use the string instead. Consider renaming tofilePathin a follow-up.Suggested rename
- const finalPaths = files.filter((path: string) => !path.includes("*")); + const finalPaths = files.filter((filePath: string) => !filePath.includes("*"));- const finalPaths = processedPaths.filter( - (path: string) => !path.includes("*") - ); + const finalPaths = processedPaths.filter( + (filePath: string) => !filePath.includes("*") + );Also applies to: 107-108
packages/utils/__tests__/setPublicFolder.test.ts (1)
3-3: Nit: import uses.jsextension instead of.ts.Pre-existing, but the coding guidelines require
.tsextensions in local import paths. Consider updating in a follow-up.As per coding guidelines: "Include
.tsfile extension in local import paths (e.g.,./setup.ts)"Suggested fix
-import { setPublicFolder } from "../src/setPublicFolder.js"; +import { setPublicFolder } from "../src/setPublicFolder.ts";
srod
force-pushed
the
codex/fix-review-findings
branch
from
d525471 to
67b8008
Compare
|
@greptile |
| function getDirectoryPath(outputPath: string): string { | ||
| const dirPath = path.dirname(outputPath); | ||
| if (dirPath && dirPath !== ".") { | ||
| return dirPath.replaceAll("\\", "/"); | ||
| } | ||
|
|
||
| const windowsDirPath = path.win32.dirname(outputPath); | ||
| if (windowsDirPath && windowsDirPath !== ".") { | ||
| return windowsDirPath.replaceAll("\\", "/"); | ||
| } |
There was a problem hiding this comment.
Windows absolute paths break
getDirectoryPath() normalizes by doing replaceAll("\\", "/") before calling mkdir(). On Windows, this turns e.g. C:\\out\\file.min.js into C:/out, which is not a valid Windows path for fs.mkdir (it can be interpreted as a relative path and fail). Since this function is specifically meant to support Windows-style output paths, the normalization should preserve Windows absolute-path semantics (or avoid rewriting separators entirely and let Node handle native separators).
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/core/src/compress.ts
Line: 136:145
Comment:
**Windows absolute paths break**
`getDirectoryPath()` normalizes by doing `replaceAll("\\", "/")` before calling `mkdir()`. On Windows, this turns e.g. `C:\\out\\file.min.js` into `C:/out`, which is not a valid Windows path for `fs.mkdir` (it can be interpreted as a relative path and fail). Since this function is specifically meant to support Windows-style output paths, the normalization should preserve Windows absolute-path semantics (or avoid rewriting separators entirely and let Node handle native separators).
How can I resolve this? If you propose a fix, please make it concise.
Unify path handling for wildcard/public-folder utilities and action manifest checks across POSIX and Windows separators. In core, preserve dirname separator semantics when creating output folders to avoid ENOENT for mixed-separator outputs (for example tmp\sub/out.js). Includes regression tests and a patch changeset entry.
srod
force-pushed
the
codex/fix-review-findings
branch
from
e6502df to
1d5e3ee
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #2809 +/- ##
===========================================
+ Coverage 95.18% 95.22% +0.03%
===========================================
Files 73 73
Lines 1725 1739 +14
Branches 523 527 +4
===========================================
+ Hits 1642 1656 +14
Misses 83 83 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1 participant
Summary:\nFixes path-handling issues found in review and hardens related tests.\n\nChanges:\n- normalize publicFolder handling in utils (setPublicFolder, wildcards, setFileNameMin) so trailing slashes are no longer required\n- avoid false positives when checking whether an input is already inside publicFolder\n- make core output directory creation support both POSIX and Windows separators\n- relax action manifest test to assert direct fast-glob dependency presence without pinning an exact version string\n- add regression tests, including a Windows-separator coverage test for core\n\nTesting:\n- bun run test packages/utils packages/core packages/action\n- bun run --filter @node-minify/utils typecheck\n- bun run --filter @node-minify/core typecheck\n- bun run --filter @node-minify/action typecheck\n- bun run --filter @node-minify/utils lint\n- bun run --filter @node-minify/core lint\n- bun run --filter @node-minify/action lint
Summary by cubic
Normalize path handling across utils and core for consistent cross-platform behavior. Trailing slashes are optional, globs use forward slashes, Windows-style paths are supported, and core preserves original separators when creating output directories (including mixed separators).
Written for commit 1d5e3ee. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests