perf: process static assets in parallel #3911
Conversation
|
Someone is attempting to deploy a commit to the Nitro Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughReplaced sequential iteration in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/build/virtual/public-assets.ts
🧰 Additional context used
📓 Path-based instructions (4)
src/**/*.{ts,js,tsx,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.{ts,js,tsx,jsx}: Usepathefor cross-platform path operations instead of Node.jsnode:path
Use ESM and modern JavaScript
Do not add comments explaining what the line does unless prompted
Files:
src/build/virtual/public-assets.ts
src/{build,dev,runner,cli}/**/*.{ts,js}
📄 CodeRabbit inference engine (AGENTS.md)
Use
consolafor logging in build/dev code; usenitro.loggerwhen available
Files:
src/build/virtual/public-assets.ts
src/**/*.{ts,js}
📄 CodeRabbit inference engine (AGENTS.md)
Use
unstoragefor storage abstraction
Files:
src/build/virtual/public-assets.ts
src/build/**/*.{ts,js}
📄 CodeRabbit inference engine (AGENTS.md)
Virtual modules must be registered in
src/build/virtual.ts
Files:
src/build/virtual/public-assets.ts
🧬 Code graph analysis (1)
src/build/virtual/public-assets.ts (2)
src/utils/parallel.ts (1)
runParallel(1-40)src/runtime/virtual/server-assets.ts (1)
assets(6-6)
🔇 Additional comments (1)
src/build/virtual/public-assets.ts (1)
9-9: Import looks good.The
runParallelimport is correctly structured using ESM syntax and a proper relative path.
| await runParallel( | ||
| new Set(files), | ||
| async (id) => { | ||
| let mimeType = | ||
| mime.getType(id.replace(/\.(gz|br)$/, "")) || "text/plain"; | ||
| if (mimeType.startsWith("text")) { | ||
| mimeType += "; charset=utf-8"; | ||
| } | ||
| const fullPath = resolve(nitro.options.output.publicDir, id); | ||
| const assetData = await fsp.readFile(fullPath); | ||
| const etag = createEtag(assetData); | ||
| const stat = await fsp.stat(fullPath); | ||
|
|
||
| const assetId = joinURL( | ||
| nitro.options.baseURL, | ||
| decodeURIComponent(id) | ||
| ); | ||
|
|
||
| let encoding; | ||
| if (id.endsWith(".gz")) { | ||
| encoding = "gzip"; | ||
| } else if (id.endsWith(".br")) { | ||
| encoding = "br"; | ||
| } | ||
|
|
||
| assets[assetId] = { | ||
| type: nitro._prerenderMeta?.[assetId]?.contentType || mimeType, | ||
| encoding, | ||
| etag, | ||
| mtime: stat.mtime.toJSON(), | ||
| size: stat.size, | ||
| path: relative(nitro.options.output.serverDir, fullPath), | ||
| data: | ||
| nitro.options.serveStatic === "inline" | ||
| ? assetData.toString("base64") | ||
| : undefined, | ||
| }; | ||
| }, | ||
| { concurrency: 25 } | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and examine the runParallel utility
find . -name "parallel.ts" -o -name "parallel.js" | head -20Repository: nitrojs/nitro
Length of output: 80
🏁 Script executed:
# Also check the structure around the virtual modules directory
ls -la src/build/virtual/ 2>/dev/null | head -20Repository: nitrojs/nitro
Length of output: 1052
🏁 Script executed:
# Examine the public-assets.ts file to see the actual implementation
cat -n src/build/virtual/public-assets.ts | head -100Repository: nitrojs/nitro
Length of output: 3994
🏁 Script executed:
# Now let's examine the runParallel utility to check error handling
cat -n src/utils/parallel.tsRepository: nitrojs/nitro
Length of output: 1330
🏁 Script executed:
# Also check if there are any tests or other usages that show error handling behavior
rg -A 5 -B 5 "runParallel" src/build/ | head -80Repository: nitrojs/nitro
Length of output: 1576
🏁 Script executed:
# Check if consola is available and used elsewhere in build code
rg "consola|nitro\.logger" src/build/ | head -20Repository: nitrojs/nitro
Length of output: 1401
🏁 Script executed:
# Check if there's any handling of errors returned from runParallel
rg -A 10 "await runParallel" src/build/virtual/public-assets.tsRepository: nitrojs/nitro
Length of output: 524
🏁 Script executed:
# Let's check the file path pattern more carefully - verify if src/utils/parallel.ts is in scope for coding guidelines
grep -r "src/utils" <<< "src/{build,dev,runner,cli}/**/*.{ts,js}"Repository: nitrojs/nitro
Length of output: 436
🏁 Script executed:
# Check if there are any error handling patterns elsewhere in the codebase for similar parallel operations
rg -B 5 -A 10 "catch.*error" src/build/ | head -60Repository: nitrojs/nitro
Length of output: 3421
Critical: Error handling swallows failures silently.
The runParallel utility (src/utils/parallel.ts, lines 21-23) catches errors and logs them with console.error but does not rethrow. This means:
- Before: Any asset processing failure would throw and fail the build
- After: Errors are logged but execution continues, resulting in an incomplete
assetsobject with no build-time failure
This could lead to missing assets in production without detection.
Fix: The utility should either rethrow errors after logging or provide error propagation as an option so callers can choose whether failures should terminate the build.
The concurrency limit of 25 and the parallelization approach itself are appropriate and will improve build performance. The shared assets object mutation is safe since each task writes to a unique key.
🤖 Prompt for AI Agents
In src/build/virtual/public-assets.ts around lines 35 to 74, the
asset-processing tasks call runParallel which currently swallows errors, so
failures are only logged and the build continues with a potentially incomplete
assets map; update the call to runParallel (or runParallel itself) so that any
error thrown by a task is propagated: either pass an option to runParallel to
rethrow after logging or change the utility to rethrow the caught error (or
aggregate and throw after all tasks finish), and ensure the caller awaits
rejection so the build fails on asset-processing errors.
pi0
changed the title
commit: |
|
Just realized I forgot to unite 2 awaits in to single await Promise.all as I did in v2 PR |
Done |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/build/virtual/public-assets.ts (1)
35-77: The error handling concern from the previous review remains valid.The critical issue flagged in the previous review about
runParallelsilently swallowing errors (rather than failing the build on asset processing failures) still applies to this implementation and should be addressed before merging.The parallelization approach and concurrency limit of 25 are appropriate for this use case.
🧹 Nitpick comments (1)
src/build/virtual/public-assets.ts (1)
76-76: Optional: Concurrency value could be tunable.The hard-coded concurrency of 25 is reasonable for most cases, but consider making it configurable (e.g., via an environment variable or nitro option) to allow tuning based on system resources and asset count.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/build/virtual/public-assets.ts
🧰 Additional context used
📓 Path-based instructions (4)
src/**/*.{ts,js,tsx,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.{ts,js,tsx,jsx}: Usepathefor cross-platform path operations instead of Node.jsnode:path
Use ESM and modern JavaScript
Do not add comments explaining what the line does unless prompted
Files:
src/build/virtual/public-assets.ts
src/{build,dev,runner,cli}/**/*.{ts,js}
📄 CodeRabbit inference engine (AGENTS.md)
Use
consolafor logging in build/dev code; usenitro.loggerwhen available
Files:
src/build/virtual/public-assets.ts
src/**/*.{ts,js}
📄 CodeRabbit inference engine (AGENTS.md)
Use
unstoragefor storage abstraction
Files:
src/build/virtual/public-assets.ts
src/build/**/*.{ts,js}
📄 CodeRabbit inference engine (AGENTS.md)
Virtual modules must be registered in
src/build/virtual.ts
Files:
src/build/virtual/public-assets.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: nitrojs/nitro PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-24T11:45:17.435Z
Learning: Applies to src/build/**/*.{ts,js} : Virtual modules must be registered in `src/build/virtual.ts`
📚 Learning: 2025-12-03T19:09:13.905Z
Learnt from: medz
Repo: nitrojs/nitro PR: 3834
File: src/presets/cloudflare/server-entry.ts:63-63
Timestamp: 2025-12-03T19:09:13.905Z
Learning: In the Nitro Cloudflare preset (src/presets/cloudflare/server-entry.ts), native errors from oxc-parser and Node.js readFile operations should be allowed to bubble up naturally without wrapping, as their native error messages are more user-friendly and provide better diagnostic information.
Applied to files:
src/build/virtual/public-assets.ts
🧬 Code graph analysis (1)
src/build/virtual/public-assets.ts (2)
src/utils/parallel.ts (1)
runParallel(1-40)src/runtime/virtual/server-assets.ts (1)
assets(6-6)
🔇 Additional comments (2)
src/build/virtual/public-assets.ts (2)
9-9: LGTM: Import is correct.The import properly brings in the
runParallelutility using ESM syntax.
44-47: Excellent: Parallel read and stat per asset.The use of
Promise.allto read the file and get stat information concurrently is a good optimization that eliminates sequential I/O waits for each asset.
1 participant
🔗 Linked issue
#3909
❓ Type of change
Process static assets in parallel
📚 Description
📝 Checklist