Conversation
PR SummaryMedium Risk Overview The web generator now aggregates these asset maps and copies the referenced files into Reviewed by Cursor Bugbot for commit 536ed59. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #753 +/- ##
==========================================
+ Coverage 77.08% 77.42% +0.33%
==========================================
Files 153 153
Lines 13749 13865 +116
Branches 1111 1122 +11
==========================================
+ Hits 10599 10735 +136
+ Misses 3146 3126 -20
Partials 4 4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 4 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 536ed59. Configure here.
| } | ||
|
|
||
| return assetsMap; | ||
| } |
There was a problem hiding this comment.
Asset filename collisions silently overwrite different images
Medium Severity
extractAssetsFromAST uses basename(originalUrl) as the destination filename, so two different images with the same name from different directories (e.g., ./diagrams/arch.png in fs.md and ./diagrams/arch.png in net.md) both map to arch.png in /assets/. One copy silently overwrites the other, and both markdown files end up referencing the wrong image. The AST URLs are also both rewritten to /assets/arch.png, making the collision unrecoverable.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 536ed59. Configure here.
|
|
||
| for (const [source, dest] of allAssets.entries()) { | ||
| try { | ||
| await cp(source, dest, { force: true }); |
There was a problem hiding this comment.
Use copyFile with COPYFILE_FICLONE for files
Low Severity
cp is used to copy individual asset files, but the project convention requires copyFile from node:fs/promises with the constants.COPYFILE_FICLONE flag for single-file copies. copyFile with COPYFILE_FICLONE attempts a zero-copy clone and gracefully falls back, which is more efficient for single files than cp.
Triggered by learned rule: Use copyFile with COPYFILE_FICLONE instead of manual readFile+writeFile
Reviewed by Cursor Bugbot for commit 536ed59. Configure here.
| return false; | ||
| } catch { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Use URL.parse() instead of new URL()
Low Severity
isLocalAsset uses new URL(url) inside a try/catch to detect local paths. The project convention prefers URL.parse(), which returns null for invalid URLs instead of throwing. This can be simplified to return URL.parse(url) === null.
Triggered by learned rule: Prefer URL.parse() and destructuring over new URL().property
Reviewed by Cursor Bugbot for commit 536ed59. Configure here.
| assetsMap.set(sourcePath, fileName); | ||
|
|
||
| // Rewrite AST URL | ||
| imageNode.url = `/assets/${fileName}`; |
There was a problem hiding this comment.
Hardcoded 'assets' path duplicated across two generators
Low Severity
The string 'assets' is hardcoded as an output directory name in both buildContent.mjs (URL rewrite to `/assets/${fileName}`) and generate.mjs (filesystem path via join(outputDir, 'assets', name)). Since this is a cross-generator constant controlling output path structure, it belongs as a named constant. Changing it in one generator without the other would silently break image resolution.
Additional Locations (1)
Triggered by learned rule: Regex patterns and external URLs belong in constants files
Reviewed by Cursor Bugbot for commit 536ed59. Configure here.
|
Why can't the user specify "all my assets are in |
If you mean as what do you think? |
Say wdyt @nodejs/web-infra I want opinions |
|
I'll hold off on fixing the bot's comments, Waiting for the final decision. |
2 participants
Description
Local images referenced in Markdown files were not resolved correctly or copied to the final build, this PR will solve this by copying the images in the
output/assets/.I solved this from root from the jsx-ast phase as they are markdown AST because we can iterate on them easier than JSX elements.
Steps:
Updated
src/generators/ast/generate.mjsto include the absolutefullPathof each Markdown file.Updated
src/generators/metadata/utils/parse.mjsto include thefullPathinto the metadata entries.Implemented
extractAssetsFromASTinsrc/generators/jsx-ast/utils/buildContent.mjs, that identifies local images, rewrites their URLs to a centralized /assets/ directory, and collects the source paths.Implemented
copyProjectAssetsto the web generatorsrc/generators/web/generate.mjsto aggregate all unique assets and copy them to the output directory.Validation
Added tests in
src/generators/jsx-ast/utils/__tests__/buildContent.test.mjsto verify that theassetsMapis populated with absolute paths.Tested against Node.js API documentation; local images (like
logo.png) are now correctly copied to the out/assets folder and displayed in the web generated.Related Issues
#748
Check List
node --run testand all tests passed.node --run format&node --run lint.