Fix filesystem path loading #30
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes in this pull request focus on the Changes
Assessment against linked issues
Possibly related PRs
Poem
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? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/beasties-webpack-plugin/src/index.js (2)
Line range hint
178-186: Consider enhancing path resolution logging.While the error handling is solid, adding debug logs for path resolution steps would help troubleshoot future path-related issues.
Consider adding debug logging:
+ this.logger.debug(`Resolving stylesheet path: ${filename}`) sheet = await this.readFile(filename) this.logger.warn( `Stylesheet "${relativePath}" not found in assets, but a file was located on disk.${ this.options.pruneSource ? ' This means pruneSource will not be applied.' : '' }`, )
Line range hint
133-186: Consider extracting path normalisation logic.The path normalisation logic in
getCssAssetis complex and could benefit from being extracted into a separate method for better maintainability and testing.Consider refactoring like this:
+ /** + * Normalises asset paths for cross-platform compatibility + * @private + */ + normaliseAssetPath(href, publicPath, outputPath) { + let normalizedPath = href.replace(/^\//, '') + const pathPrefix = `${(publicPath || '').replace(/(^\/|\/$)/g, '')}/` + if (normalizedPath.indexOf(pathPrefix) === 0) { + normalizedPath = normalizedPath + .substring(pathPrefix.length) + .replace(/^\//, '') + } + return path.resolve(outputPath, normalizedPath) + } async getCssAsset(href, style) { const outputPath = this.options.path const publicPath = this.options.publicPath - let normalizedPath = href.replace(/^\//, '') - const pathPrefix = `${(publicPath || '').replace(/(^\/|\/$)/g, '')}/` - if (normalizedPath.indexOf(pathPrefix) === 0) { - normalizedPath = normalizedPath - .substring(pathPrefix.length) - .replace(/^\//, '') - } - const filename = path.resolve(outputPath, normalizedPath) + const filename = this.normaliseAssetPath(href, publicPath, outputPath)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/beasties-webpack-plugin/src/index.js(1 hunks)
🔇 Additional comments (1)
packages/beasties-webpack-plugin/src/index.js (1)
178-178: LGTM! This change fixes the Windows path handling issue.
The simplified readFile call now correctly handles file paths across different operating systems. The removal of the unnecessary compilation argument aligns with the API design and resolves issue #29.
CodSpeed Performance ReportMerging #30 will degrade performances by 48.53%Comparing Summary
Benchmarks breakdown
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #30 +/- ##
==========================================
+ Coverage 83.70% 84.73% +1.02%
==========================================
Files 7 7
Lines 1166 1166
Branches 269 271 +2
==========================================
+ Hits 976 988 +12
+ Misses 190 178 -12 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
thank you - this looks good to me (seems to have been introduced in GoogleChromeLabs/critters#61)
one more thing though: could you describe how to create a minimal reproduction? 🙏 I'll then convert that into a regression test + get it merged
|
Here you go: https://github.com/xpdota/beasties-bug-29-repro
|
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
packages/beasties-webpack-plugin/test/fixtures/fs-access/dist/style.cssis excluded by!**/dist/**
📒 Files selected for processing (2)
packages/beasties-webpack-plugin/test/fixtures/fs-access/index.html(1 hunks)packages/beasties-webpack-plugin/test/index.test.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/beasties-webpack-plugin/test/fixtures/fs-access/index.html
| describe('accessing file system', () => { | ||
| it('works', async () => { | ||
| const output = await compileToHtml('fs-access', configure, { | ||
| path: './dist/', | ||
| publicPath: '', | ||
| }) | ||
| expect(output.html).toMatch(/\.foo/) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage for filesystem path handling
The current test suite could benefit from more comprehensive coverage, particularly given the Windows path handling issues mentioned in PR #29. Consider adding these test cases:
- Platform-specific path testing
- Error handling scenarios
- Various path formats (relative, absolute)
Here's a suggested enhancement:
describe('accessing file system', () => {
it('works', async () => {
const output = await compileToHtml('fs-access', configure, {
path: './dist/',
publicPath: '',
})
expect(output.html).toMatch(/\.foo/)
})
+
+ it('handles Windows-style paths', async () => {
+ const output = await compileToHtml('fs-access', configure, {
+ path: '.\\dist\\',
+ publicPath: '',
+ })
+ expect(output.html).toMatch(/\.foo/)
+ })
+
+ it('handles absolute paths', async () => {
+ const output = await compileToHtml('fs-access', configure, {
+ path: path.resolve(__dirname, 'dist'),
+ publicPath: '',
+ })
+ expect(output.html).toMatch(/\.foo/)
+ })
+
+ it('handles missing stylesheet gracefully', async () => {
+ await expect(compileToHtml('fs-access', configure, {
+ path: './non-existent/',
+ publicPath: '',
+ })).rejects.toThrow()
+ })
})Committable suggestion skipped: line range outside the PR's diff.
| const output = await compileToHtml('fs-access', configure, { | ||
| path: './dist/', | ||
| publicPath: '', | ||
| }) |
There was a problem hiding this comment.
💡 Codebase verification
Hard-coded Path Separators Detected
The path handling implementation includes hard-coded path separators which may not resolve correctly across different platforms.
- Affected files:
packages/beasties/test/beasties.test.tspackages/beasties/test/beasties.bench.tspackages/beasties/test/security.test.tspackages/beasties/src/util.tspackages/beasties-webpack-plugin/test/index.test.tspackages/beasties-webpack-plugin/test/standalone.test.tspackages/beasties-webpack-plugin/test/helpers.tspackages/beasties/src/types.tspackages/beasties/src/index.d.tspackages/beasties/src/dom.tspackages/beasties/src/index.tspackages/beasties/src/css.ts
🔗 Analysis chain
Verify path handling implementation
Let's verify that the path handling implementation correctly resolves paths across different platforms.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for platform-specific path handling in the codebase
# Look for path handling logic
echo "Searching for path handling code..."
rg -l "path\.(resolve|join|normalize)" --type ts
# Check for Windows-specific path handling
echo "Checking for Windows path handling..."
rg -l "(\\\\|win32|process\.platform)" --type ts
# Look for filesystem operations
echo "Checking filesystem operations..."
ast-grep --pattern 'fs.$_($$$)'
Length of output: 1666
Script:
#!/bin/bash
# Description: Inspect path construction and platform-specific logic in the codebase
# Find implementations using path.resolve, path.join, path.normalize
echo "Searching for path construction implementations..."
ast-grep --pattern 'path.$_(...)' --type ts
# Check for hard-coded path separators
echo "Checking for hard-coded path separators..."
rg -l '["\']/["\']' --type ts
# Verify conditional logic based on process.platform
echo "Searching for platform-specific conditional statements..."
rg 'if\s*\(\s*process\.platform\s*===' --type ts
Length of output: 526
Script:
#!/bin/bash
# Description: Inspect path construction and platform-specific logic in the codebase
# Find implementations using path.resolve, path.join, path.normalize
echo "Searching for path construction implementations..."
ast-grep --pattern 'path.$_(...)' -- --type ts
# Check for hard-coded path separators
echo "Checking for hard-coded path separators..."
rg -l '["/\\\']' --type ts
# Verify conditional logic based on process.platform
echo "Searching for platform-specific conditional statements..."
rg 'if\s*\(\s*process\.platform\s*===' --type ts
Length of output: 453
Script:
#!/bin/bash
# Description: Inspect path construction and platform-specific logic in the codebase
# Find implementations using path.resolve, path.join, path.normalize
echo "Searching for path construction implementations..."
ast-grep --pattern 'path.$_($_)' --lang typescript
# Check for hard-coded path separators
echo "Checking for hard-coded path separators..."
rg -l '["/\\\\\']' --type ts
# Verify conditional logic based on process.platform
echo "Searching for platform-specific conditional statements..."
rg 'if\s*\(\s*process\.platform\s*===' --type ts
Length of output: 549
Script:
#!/bin/bash
# Description: Inspect path construction and platform-specific logic in the codebase
# Find implementations using path.resolve, path.join, path.normalize
echo "Searching for path construction implementations..."
ast-grep --pattern 'path.$_($_)' --lang typescript
# Check for hard-coded path separators
echo "Checking for hard-coded path separators..."
rg -l "['\"\\\\/]" --type ts
# Verify conditional logic based on process.platform
echo "Searching for platform-specific conditional statements..."
rg 'if\s*\(\s*process\.platform\s*===' --type ts
Length of output: 1123
|
thank you! |
3 participants
Fixes #29
I can't fully test, since it seems that some of the tests are broken on Windows as they expect a unix-style path, but I can confirm that it resolves the immediate issue.
Beasties.readFile()only takes a single argument, which is the filename.Summary by CodeRabbit
New Features
Bug Fixes
Tests