fix: making pruneSource work#138
Conversation
## Walkthrough
The update introduces logic to write the pruned (inverse) CSS stylesheet to disk within the `processStyle` method of the `Beasties` class. After pruning and serialising the CSS, if pruning is enabled, the code generates a file path using the configured output directory and stylesheet name, then writes the pruned CSS content to this file asynchronously. It also logs either an error or a success message based on the outcome of the file write operation. Additionally, a new asynchronous `writeFile` method was added to the `Beasties` class to handle file writing using a provided filesystem interface or the native Node.js method. No changes were made to exported or public entity signatures besides the addition of this method.
## Changes
| File(s) | Change Summary |
|------------------------------------------------|-----------------------------------------------------------------------------------------------------------------|
| packages/beasties/src/index.ts | Added asynchronous `writeFile` method; enhanced `processStyle` to write pruned CSS to disk asynchronously with logging after pruning. |
| packages/beasties/test/beasties.test.ts | Added new test verifying CSS file update when `pruneSource` is enabled, including assertions on pruned CSS content and logging. |
| packages/beasties/test/src/prune-source.css | Added new CSS file containing various style rules used for pruning tests. |
| packages/beasties/test/src/prune-source.html | Added new HTML test fixture linking to the new CSS file, used for testing pruning functionality. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant User
participant Beasties
participant FileSystem
User->>Beasties: processStyle()
alt Pruning enabled
Beasties->>Beasties: Prune and serialise CSS
Beasties->>Beasties: Resolve full stylesheet path
Beasties->>FileSystem: writeFile(pruned CSS)
FileSystem-->>Beasties: Success/Error callback
Beasties->>Beasties: Log success or error
else Pruning not enabled
Beasties->>Beasties: Skip writing pruned CSS
end
Beasties-->>User: Complete processStyleSuggested reviewers
|
![callstackai[bot]](https://avatars.githubusercontent.com/in/1040204?s=60&v=4){kind=small}
![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
♻️ Duplicate comments (2)
packages/beasties/src/index.ts (2)
673-676:⚠️ Potential issueSecurity vulnerability: potential path traversal attack
The code directly joins
this.options.pathwithnamewithout sanitizing or validating the paths. Sincenamecomes fromstyle.$$name, which could contain '../' sequences, this could lead to directory traversal vulnerabilities allowing writes outside the intended directory.Apply this change to fix the security vulnerability:
- const cssFilePath = path.resolve(this.options.path, name) + const cssFilePath = path.resolve(this.options.path, path.basename(name))
673-676: 🛠️ Refactor suggestionConsider handling file writing through a plugin system
The current implementation writes directly to the filesystem, which might not be appropriate for all use cases, particularly for webpack/vite plugins that work with virtual or generated files.
Based on the previous comments by danielroe and alan-agius4, consider implementing a plugin system for file writing:
- const cssFilePath = path.resolve(this.options.path, path.basename(name)) - this.writeFile(cssFilePath, sheetInverse) - .then(() => this.logger.info?.(`${name} was successfully updated`)) - .catch(err => this.logger.error?.(err)) + // Allow plugins to intercept file writing + const cssFilePath = path.resolve(this.options.path, path.basename(name)) + const writePromise = typeof this.options.onWritePrunedCss === 'function' + ? this.options.onWritePrunedCss(cssFilePath, sheetInverse) + : this.writeFile(cssFilePath, sheetInverse) + + writePromise + .then(() => this.logger.info?.(`${name} was successfully updated`)) + .catch(err => this.logger.error?.(err))This would require adding a new option to the
Optionsinterface:interface Options { // ... onWritePrunedCss?: (path: string, content: string) => Promise<void>; // ... }
🧹 Nitpick comments (1)
packages/beasties/src/index.ts (1)
75-94: New writeFile method looks good but consider using optional chainingThe implementation mirrors the existing
readFilemethod's pattern and correctly handles both custom filesystem interfaces and Node.js native methods.Consider using optional chaining on line 87 as suggested by the static analysis tool:
- if (fs && fs.writeFile) { + if (fs?.writeFile) {🧰 Tools
🪛 Biome (1.9.4)
[error] 87-87: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/beasties/src/index.ts(3 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/beasties/src/index.ts
[error] 87-87: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (1)
packages/beasties/src/index.ts (1)
22-22: Import statement added correctlyThe addition of
writeFileimport to accompany the existingreadFileimport is necessary for the new functionality.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/beasties/src/index.ts (1)
673-676:⚠️ Potential issuePotential directory traversal vulnerability.
The code joins
this.options.pathwithnamewithout extracting just the filename part. Sincenamecomes fromstyle.$$name, which could potentially contain '../' sequences, this might allow writing files outside the intended directory.Apply this fix to mitigate the vulnerability:
- const cssFilePath = path.resolve(this.options.path, name) + const cssFilePath = path.resolve(this.options.path, path.basename(name))This matches the approach suggested in previous review comments.
🧹 Nitpick comments (1)
packages/beasties/src/index.ts (1)
75-94: Well-implementedwriteFilemethod.This method mirrors the existing
readFilemethod pattern which is good for consistency, and it correctly supports custom filesystem implementations - important for plugin support as noted in previous reviews.Consider using optional chaining for a cleaner check:
- if (fs && fs.writeFile) { + if (fs?.writeFile) {🧰 Tools
🪛 Biome (1.9.4)
[error] 87-87: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/beasties/src/index.ts(3 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/beasties/src/index.ts
[error] 87-87: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (1)
packages/beasties/src/index.ts (1)
674-676: Good error handling with appropriate logging.The promise chain correctly handles both success and error cases with appropriate logging messages.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/beasties/src/index.ts (1)
75-93: Good implementation of the writeFile method.The new
writeFilemethod follows the same pattern as the existingreadFilemethod, which is good for consistency. It properly handles both the provided filesystem interface and the native Node.js method.Consider using optional chaining for a more concise check:
- if (fs && fs.writeFile) { + if (fs?.writeFile) {🧰 Tools
🪛 Biome (1.9.4)
[error] 86-86: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/beasties/src/index.ts(3 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/beasties/src/index.ts
[error] 86-86: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (2)
packages/beasties/src/index.ts (2)
22-22: Import looks good.The addition of the
writeFileimport from 'node:fs' is consistent with the existingreadFileimport pattern.
672-675: Implementation successfully writes pruned CSS back to the source file.The code correctly:
- Resolves the full file path using
path.resolve- Writes the pruned CSS to the source file asynchronously
- Logs appropriate success or error messages
This implementation properly addresses the main objective of the PR to ensure pruned CSS is written back to the source file.
CodSpeed Performance ReportMerging #138 will not alter performanceComparing Summary
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #138 +/- ##
==========================================
+ Coverage 85.93% 86.14% +0.21%
==========================================
Files 8 8
Lines 1294 1314 +20
Branches 311 319 +8
==========================================
+ Hits 1112 1132 +20
Misses 182 182 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
When pruneSource started to work, a bug is revealed: For example if I have a rule: sometimes it converts to (in pruned file): Without pruneSource it is not noticeable, because the original file remains untouched. |
|
So what do we do next @danielroe? |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/beasties/test/__snapshots__/beasties.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (3)
packages/beasties/test/beasties.test.ts(1 hunks)packages/beasties/test/src/prune-source.css(1 hunks)packages/beasties/test/src/prune-source.html(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/beasties/test/src/prune-source.css
🧰 Additional context used
🪛 HTMLHint (1.5.0)
packages/beasties/test/src/prune-source.html
[error] 1-1: Doctype must be declared before any non-comment content.
(doctype-first)
🔇 Additional comments (3)
packages/beasties/test/src/prune-source.html (1)
1-21: HTML test fixture structure looks appropriate for testing CSS pruning functionality.The HTML structure effectively provides a variety of elements and classes to test the pruning logic. The missing doctype flagged by HTMLHint is likely intentional for this test fixture, as it focuses on testing the CSS pruning behaviour rather than HTML validation.
packages/beasties/test/beasties.test.ts (2)
349-385: Comprehensive test case for pruneSource functionality.The test implementation is well-structured and covers the key aspects of the pruneSource feature:
- Proper setup with logger and pruneSource configuration
- Appropriate writeFile method override using Promise wrapper
- Thorough verification of both HTML output and CSS file content
- Good error handling in the writeFile mock
The test expectations look reasonable and the structure follows the existing patterns in the test suite.
366-374: Well-implemented writeFile override for testing.The Promise wrapper around
fs.writeFileSyncis correctly implemented with proper error handling. This approach ensures the test can verify the file writing behaviour whilst maintaining the asynchronous interface expected by the Beasties class.
| expect(result).toContain('<style>h1{color:blue}p{color:purple}.contents{padding:50px;text-align:center}.input-field{padding:10px}div:is(:hover,.active){color:#000}div:is(.selected,:hover){color:#fff}</style>') | ||
|
|
||
| const css = fs.readFileSync(path.join(fixtureDir, 'prune-source.css'), 'utf-8') | ||
| expect(css).toEqual('h2.unused{color:red}p.unused{color:orange}header{padding:0 50px}.banner{font-family:sans-serif}footer{margin-top:10px}.container{border:1px solid}.custom-element::part(tab){color:#0c0dcc;border-bottom:transparent solid 2px}.other-element::part(tab){color:#0c0dcc;border-bottom:transparent solid 2px}.custom-element::part(tab):hover{background-color:#0c0d19;color:#ffffff;border-color:#0c0d33}.custom-element::part(tab):hover:active{background-color:#0c0d33;color:#ffffff}.custom-element::part(tab):focus{box-shadow:0 0 0 1px #0a84ff inset, 0 0 0 1px #0a84ff, 0 0 0 4px rgba(10, 132, 255, 0.3)}.custom-element::part(active){color:#0060df;border-color:#0a84ff !important}') |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the expected CSS content matches the actual pruning logic.
The test expects specific CSS content after pruning. Ensure that the expected content in the assertion accurately reflects what the pruning algorithm should produce for the given HTML fixture.
🏁 Script executed:
#!/bin/bash
# Verify that the expected CSS content matches the actual pruning behaviour
# by examining the corresponding CSS fixture file
# Find and display the CSS fixture file content
fd -e css prune-source.css packages/beasties/test/src/ --exec cat {}
# Also check if there are any related CSS files that might be used
fd -e css . packages/beasties/test/src/ --exec ls -la {}Length of output: 1233
Update the expected CSS snapshot in the prune‐source assertion
The test’s hard-coded CSS doesn’t match the contents of prune-source.css (which now includes the h1, .contents, .input-field and the div:is() rules). Please update the assertion in packages/beasties/test/beasties.test.ts to reflect the actual fixture content:
• File: packages/beasties/test/beasties.test.ts
• Location: around the expect(css).toEqual(…) on lines 380–381
Proposed diff:
- expect(css).toEqual(
- 'h2.unused{color:red}p.unused{color:orange}header{padding:0 50px}.banner{font-family:sans-serif}footer{margin-top:10px}.container{border:1px solid}.custom-element::part(tab){color:#0c0dcc;border-bottom:transparent solid 2px}.other-element::part(tab){color:#0c0dcc;border-bottom:transparent solid 2px}.custom-element::part(tab):hover{background-color:#0c0d19;color:#ffffff;border-color:#0c0d33}.custom-element::part(tab):hover:active{background-color:#0c0d33;color:#ffffff}.custom-element::part(tab):focus{box-shadow:0 0 0 1px #0a84ff inset, 0 0 0 1px #0a84ff, 0 0 0 4px rgba(10, 132, 255, 0.3)}.custom-element::part(active){color:#0060df;border-color:#0a84ff !important}'
- )
+ expect(css).toEqual(
+ 'h1{color:blue}h2.unused{color:red}p{color:purple}p.unused{color:orange}header{padding:0 50px}.banner{font-family:sans-serif}.contents{padding:50px;text-align:center}.input-field{padding:10px}footer{margin-top:10px}.container{border:1px solid}.custom-element::part(tab){color:#0c0dcc;border-bottom:transparent solid 2px}.other-element::part(tab){color:#0c0dcc;border-bottom:transparent solid 2px}.custom-element::part(tab):hover{background-color:#0c0d19;color:#ffffff;border-color:#0c0d33}.custom-element::part(tab):hover:active{background-color:#0c0d33;color:#ffffff}.custom-element::part(tab):focus{box-shadow:0 0 0 1px #0a84ff inset, 0 0 0 1px #0a84ff, 0 0 0 4px rgba(10, 132, 255, 0.3)}.custom-element::part(active){color:#0060df;border-color:#0a84ff !important}div:is(:hover,.active){color:#000}div:is(.selected,:hover){color:#fff}'
+ )📝 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.
| expect(result).toContain('<style>h1{color:blue}p{color:purple}.contents{padding:50px;text-align:center}.input-field{padding:10px}div:is(:hover,.active){color:#000}div:is(.selected,:hover){color:#fff}</style>') | |
| const css = fs.readFileSync(path.join(fixtureDir, 'prune-source.css'), 'utf-8') | |
| expect(css).toEqual('h2.unused{color:red}p.unused{color:orange}header{padding:0 50px}.banner{font-family:sans-serif}footer{margin-top:10px}.container{border:1px solid}.custom-element::part(tab){color:#0c0dcc;border-bottom:transparent solid 2px}.other-element::part(tab){color:#0c0dcc;border-bottom:transparent solid 2px}.custom-element::part(tab):hover{background-color:#0c0d19;color:#ffffff;border-color:#0c0d33}.custom-element::part(tab):hover:active{background-color:#0c0d33;color:#ffffff}.custom-element::part(tab):focus{box-shadow:0 0 0 1px #0a84ff inset, 0 0 0 1px #0a84ff, 0 0 0 4px rgba(10, 132, 255, 0.3)}.custom-element::part(active){color:#0060df;border-color:#0a84ff !important}') | |
| const css = fs.readFileSync(path.join(fixtureDir, 'prune-source.css'), 'utf-8') | |
| expect(css).toEqual( | |
| 'h1{color:blue}h2.unused{color:red}p{color:purple}p.unused{color:orange}header{padding:0 50px}.banner{font-family:sans-serif}.contents{padding:50px;text-align:center}.input-field{padding:10px}footer{margin-top:10px}.container{border:1px solid}.custom-element::part(tab){color:#0c0dcc;border-bottom:transparent solid 2px}.other-element::part(tab){color:#0c0dcc;border-bottom:transparent solid 2px}.custom-element::part(tab):hover{background-color:#0c0d19;color:#ffffff;border-color:#0c0d33}.custom-element::part(tab):hover:active{background-color:#0c0d33;color:#ffffff}.custom-element::part(tab):focus{box-shadow:0 0 0 1px #0a84ff inset, 0 0 0 1px #0a84ff, 0 0 0 4px rgba(10, 132, 255, 0.3)}.custom-element::part(active){color:#0060df;border-color:#0a84ff !important}div:is(:hover,.active){color:#000}div:is(.selected,:hover){color:#fff}' | |
| ) |
🤖 Prompt for AI Agents
In packages/beasties/test/beasties.test.ts around lines 380 to 381, the expected
CSS string in the assertion does not match the actual contents of
prune-source.css, which now includes additional rules like h1, .contents,
.input-field, and div:is() selectors. Update the expected string in the
expect(css).toEqual(...) call to exactly match the current contents of
prune-source.css, including all new CSS rules and formatting.
|
Hi @danielroe, I finally found time to write the test) |
4 participants
It turns out they already calculate pruned styles, they just don't write them to the source css file.
✨
Description by Callstackai
This PR fixes the functionality of
pruneSourceby ensuring that pruned styles are written to the source CSS file.Diagrams of code changes
sequenceDiagram participant Client participant Beasties participant FileSystem Client->>Beasties: process(html) Note over Beasties: Process HTML and extract critical CSS alt pruneSource enabled Beasties->>FileSystem: writeFile(cssFilePath, nonCriticalCSS) alt Success FileSystem-->>Beasties: resolve() Beasties->>Beasties: logger.info("file updated") else Error FileSystem-->>Beasties: reject(error) Beasties->>Beasties: logger.error(error) end end Beasties-->>Client: return processed HTMLFiles Changed
pruneSourceis enabled.pruneSourceis enabled.pruneSourcefeature.pruneSourcefeature.This PR includes files in programming languages that we currently do not support. We have not reviewed files with the extensions
.snap,.css,.html. See list of supported languages.