⚡ Bolt: Use async file writing in compression workflow#2727
Conversation
Replaces synchronous `writeFile` with asynchronous `writeFileAsync` in the compression pipeline to prevent event loop blocking during file I/O operations. - Added `writeFileAsync` to `@node-minify/utils`. - Updated `run` in `@node-minify/utils` to use `writeFileAsync`. - Updated tests in `@node-minify/utils`.
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds an async file-write API and converts internal write flows to use it: introduces Changes
Sequence Diagram(s)sequenceDiagram
participant Run as run()
participant WriteOutput as writeOutput()
participant MultiWriter as writeMultipleOutputs()
participant FileWriter as writeFileAsync()
participant FS as fs/promises
Run->>WriteOutput: await writeOutput(result, settings, index)
activate WriteOutput
alt multiple outputs
WriteOutput->>MultiWriter: await writeMultipleOutputs(outputs, settings, index)
activate MultiWriter
MultiWriter->>FileWriter: await writeFileAsync({file, content, index})
activate FileWriter
FileWriter->>FS: writeFile(path, content)
FS-->>FileWriter: Promise<void>
deactivate FileWriter
FileWriter-->>MultiWriter: content
deactivate MultiWriter
else single output
WriteOutput->>FileWriter: await writeFileAsync({file, content, index})
activate FileWriter
FileWriter->>FS: writeFile(path, content)
FS-->>FileWriter: Promise<void>
deactivate FileWriter
FileWriter-->>WriteOutput: content
end
deactivate WriteOutput
WriteOutput-->>Run: Promise<void>
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
Comment |
Greptile SummaryReplaced synchronous file writing with asynchronous file operations in the compression workflow. The core Key Changes:
Implementation Quality: Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Core as Core/Compressor
participant Run as run()
participant WriteOutput as writeOutput()
participant WriteFile as writeFileAsync()
participant FS as Node.js fs/promises
Core->>Run: run({ settings, content, index })
Run->>Run: Validate settings & compressor
Run->>Core: settings.compressor({ settings, content, index })
Core-->>Run: CompressorResult { code, map?, buffer?, outputs? }
Run->>WriteOutput: await writeOutput(result, settings, index)
alt In-Memory Mode
WriteOutput-->>Run: return (skip disk write)
else Multiple Outputs
WriteOutput->>WriteFile: await writeFileAsync() [for each output]
WriteFile->>FS: await writeFile()
FS-->>WriteFile: success
WriteFile-->>WriteOutput: content
else Buffer Output
WriteOutput->>WriteFile: await writeFileAsync(buffer)
WriteFile->>FS: await writeFile()
FS-->>WriteFile: success
WriteFile-->>WriteOutput: buffer
else Code Output
WriteOutput->>WriteFile: await writeFileAsync(code)
WriteFile->>FS: await writeFile()
FS-->>WriteFile: success
WriteFile-->>WriteOutput: code
opt Source Map Present
WriteOutput->>WriteFile: await writeFileAsync(map)
WriteFile->>FS: await writeFile()
FS-->>WriteFile: success
WriteFile-->>WriteOutput: map
end
end
WriteOutput-->>Run: void
Run-->>Core: result.code
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional Comments (1)
-
foo.js, line 1 (link)style: Empty file added accidentally - should be removed
5 files reviewed, 2 comments
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @srod. * #2727 (comment) The following files were modified: * `packages/utils/src/run.ts` * `packages/utils/src/writeFile.ts`
📝 Add docstrings to `bolt/optimize-file-writing-8256924729521665993`
|
@greptile |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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
🧹 Nitpick comments (1)
packages/utils/src/run.ts (1)
143-189: Consider parallelizing file writes for improved throughput.The current sequential approach using
awaitinside the loop is correct and safe, but writes files one at a time. If the number of outputs is typically small, you could usePromise.allfor parallel writes:🔎 Optional parallel write approach
- for (let i = 0; i < outputs.length; i++) { - const outputResult = outputs[i]; - if (!outputResult) { - continue; - } - - const format = outputResult.format || "out"; - let targetFile: string; - - const arrayItem = outputsArray[i]; - - if ( - isArrayOutput && - arrayItem !== undefined && - arrayItem !== "$1" && - arrayItem.trim() !== "" - ) { - // Explicit output path provided in array - use as-is - targetFile = arrayItem; - } else if (typeof output === "string" && output === "$1") { - // $1 only: auto-generate from input filename in input directory - const baseName = inputBase || "output"; - targetFile = inputDir - ? join(inputDir, `${baseName}.${format}`) - : `${baseName}.${format}`; - } else if (typeof output === "string" && output.includes("$1")) { - // $1 pattern in path: replace and append format - const extensionlessName = inputBase || "output"; - const outputFile = output.replace(/\$1/g, extensionlessName); - targetFile = `${outputFile}.${format}`; - } else if (typeof output === "string") { - // Single string output: append format extension - targetFile = `${output}.${format}`; - } else { - // Fallback - const baseName = inputBase || "output"; - targetFile = inputDir - ? join(inputDir, `${baseName}.${format}`) - : `${baseName}.${format}`; - } - - await writeFileAsync({ - file: targetFile, - content: outputResult.content, - index, - }); - } + const writePromises = outputs + .map((outputResult, i) => { + if (!outputResult) { + return null; + } + + const format = outputResult.format || "out"; + let targetFile: string; + + const arrayItem = outputsArray[i]; + + if ( + isArrayOutput && + arrayItem !== undefined && + arrayItem !== "$1" && + arrayItem.trim() !== "" + ) { + targetFile = arrayItem; + } else if (typeof output === "string" && output === "$1") { + const baseName = inputBase || "output"; + targetFile = inputDir + ? join(inputDir, `${baseName}.${format}`) + : `${baseName}.${format}`; + } else if (typeof output === "string" && output.includes("$1")) { + const extensionlessName = inputBase || "output"; + const outputFile = output.replace(/\$1/g, extensionlessName); + targetFile = `${outputFile}.${format}`; + } else if (typeof output === "string") { + targetFile = `${output}.${format}`; + } else { + const baseName = inputBase || "output"; + targetFile = inputDir + ? join(inputDir, `${baseName}.${format}`) + : `${baseName}.${format}`; + } + + return writeFileAsync({ + file: targetFile, + content: outputResult.content, + index, + }); + }) + .filter(Boolean); + + await Promise.all(writePromises);That said, the sequential approach is simpler and avoids potential filesystem contention with many outputs—feel free to keep it as-is if parallelism isn't a priority.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/utils/src/run.tspackages/utils/src/writeFile.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/utils/src/writeFile.ts
🔇 Additional comments (3)
packages/utils/src/run.ts (3)
15-15: LGTM!Import correctly updated to use the new async variant.
26-48: LGTM!The
runfunction correctly awaitswriteOutput, ensuring file writes complete before returning the result code. The public API surface remains unchanged for consumers.
59-102: LGTM!All
writeFileAsynccalls are properly awaited, and the sequential write order (main file before source map) is correct. Early returns prevent unnecessary operations.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #2727 +/- ##
===========================================
+ Coverage 99.47% 99.48% +0.01%
===========================================
Files 47 47
Lines 758 781 +23
Branches 247 245 -2
===========================================
+ Hits 754 777 +23
Misses 4 4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1 participant
💡 What: Replaced synchronous file writing with asynchronous file writing in the core
runfunction.🎯 Why: Synchronous I/O blocks the Node.js event loop, which can degrade performance when processing multiple files or in a server environment.
📊 Impact: Prevents blocking of the event loop during file write operations, improving concurrency handling.
🔬 Measurement: Verified by running the test suite, including new tests for
writeFileAsync.PR created automatically by Jules for task 8256924729521665993 started by @srod
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.