new-log-viewer: Ensure all static assets are copied from /public to /dist during the build process (fixes #95).
#96
Conversation
WalkthroughThe changes involve updating the Changes
Assessment against linked issues
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 (
|
junhaoliao
changed the title
/public to /dist during the build process.
junhaoliao
changed the title
/public to /dist during the build process./public to /dist during the build process (fixes #95).
![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)
new-log-viewer/package.json (1)
49-49: LGTM! Consider pinning the dependency version.The addition of
copy-webpack-pluginas a dev dependency is appropriate for addressing the favicon issue mentioned in the PR objectives. This plugin will likely be used in the Webpack configuration to copy the favicon to the build output directory.Consider pinning the exact version of
copy-webpack-pluginto ensure consistent builds across different environments. You can do this by removing the caret:- "copy-webpack-plugin": "^12.0.2", + "copy-webpack-plugin": "12.0.2",This change would prevent potential issues arising from minor updates to the plugin.
new-log-viewer/webpack.common.js (1)
70-83: Well-configured CopyWebpackPlugin with a minor suggestion.The CopyWebpackPlugin configuration effectively manages static assets, correctly ignoring the index.html file. The Windows path compatibility handling is a nice touch.
Consider adding a comment explaining why index.html is ignored, to improve code maintainability:
ignore: [ // Ignore index.html as it's handled by HtmlWebpackPlugin indexHtmlPath.replaceAll("\\", "/"), ],
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
new-log-viewer/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
- new-log-viewer/package.json (1 hunks)
- new-log-viewer/webpack.common.js (2 hunks)
🧰 Additional context used
🔇 Additional comments (5)
new-log-viewer/webpack.common.js (5)
5-5: LGTM! CopyWebpackPlugin addition is appropriate.The addition of CopyWebpackPlugin aligns with the PR objectives and is a good practice for managing static assets in webpack configurations.
10-12: Excellent addition of path variables.The introduction of
distPath,publicPath, andindexHtmlPathvariables enhances code readability and maintainability. Centralizing these paths makes future updates easier and ensures cross-platform compatibility through the use ofpath.resolve.
64-64: Good use of the new distPath variable.Updating the output path to use the
distPathvariable maintains consistency with the newly defined variables and simplifies the configuration.
85-85: Good update to HtmlWebpackPlugin configuration.Using the
indexHtmlPathvariable for the template path in HtmlWebpackPlugin configuration maintains consistency and simplifies the overall webpack setup.
Line range hint
5-85: Overall excellent webpack configuration updates.These changes effectively address the PR objectives by:
- Centralizing path definitions for improved maintainability.
- Properly configuring CopyWebpackPlugin to manage static assets.
- Updating HtmlWebpackPlugin to use the new path variables.
These modifications should resolve the favicon issue and improve the overall build process for the YScope log viewer.
To verify the changes:
new-log-viewer/webpack.common.js
Outdated
| from: publicPath, | ||
| globOptions: { | ||
| ignore: [ | ||
| // The `replaceAll()` is needed because the plugin does not convert |
There was a problem hiding this comment.
| // The `replaceAll()` is needed because the plugin does not convert | |
| // Below `replaceAll()` is needed because the plugin does not convert |
junhaoliao
changed the title
/public to /dist during the build process (fixes #95)./public are copied to /dist during the build process (fixes #95).
junhaoliao
changed the title
/public are copied to /dist during the build process (fixes #95)./public to /dist during the build process (fixes #95).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
new-log-viewer/webpack.common.js (2)
10-12: LGTM! Path variables improve organization.The addition of
distPath,publicPath, andindexHtmlPathvariables enhances code organization and maintainability. These centralized path definitions will make future updates easier and ensure cross-platform compatibility.For consistency, consider using
__dirnamein all path definitions:-const distPath = path.resolve(__dirname, "dist"); -const publicPath = path.resolve(__dirname, "public"); +const distPath = path.resolve(__dirname, "dist"); +const publicPath = path.resolve(__dirname, "public"); const indexHtmlPath = path.resolve(publicPath, "index.html");
70-83: LGTM! CopyWebpackPlugin configuration is well-structured.The CopyWebpackPlugin configuration effectively copies static assets from
/publicto/dist, aligning with the PR objective. The ignore pattern forindex.htmlis appropriate, as it's handled separately by HtmlWebpackPlugin.The use of
replaceAll()to handle Windows paths is a good practice. For clarity, consider adding a comment explaining why this is necessary:// `replaceAll()` is needed because the plugin does not convert Windows // paths to POSIX paths for `ignore` patterns. -indexHtmlPath.replaceAll("\\", "/"), +indexHtmlPath.replaceAll("\\", "/"), // Convert Windows backslashes to forward slashes
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- new-log-viewer/webpack.common.js (2 hunks)
🧰 Additional context used
🔇 Additional comments (4)
new-log-viewer/webpack.common.js (4)
5-5: LGTM! CopyWebpackPlugin addition is appropriate.The addition of CopyWebpackPlugin aligns with the PR objective of copying static assets from
/publicto/distduring the build process. This is a suitable choice for managing static assets in webpack configurations.
64-64: LGTM! Output path update is consistent.The update to use
distPathfor the output path is consistent with the newly defined variables and improves code maintainability.
85-85: LGTM! HtmlWebpackPlugin template path update is consistent.The update to use
indexHtmlPathfor the HtmlWebpackPlugin template path is consistent with the newly defined variables and improves code maintainability.
Line range hint
1-161: Overall, excellent implementation of static asset management.The changes effectively address the PR objective of copying static assets from
/publicto/distduring the build process. The implementation is well-structured, maintainable, and consistent. The use of centralized path variables and appropriate webpack plugins demonstrates good coding practices.To verify the implementation, you can run the following script:
References
Fixes #95
new-log-viewer series: #45 #46 #48 #51 #52 #53 #54 #55 #56 #59 #60 #61 #62 #63 #66 #67 #68 #69 #70 #71 #72 #73 #74 #76 #77 #78 #79 #80 #81 #82 #83 #84 #89 #91 #93 #94
Description
Validation performed
/distand in the built log viewer the YSscope icon was displayed correctly.Summary by CodeRabbit