Refactored boot time / on demand asset generation #21866
Conversation
mike182uk
force-pushed
the
mike-onc-662-fix-file-write-issues-in-ghost-application-related-to-asset-2
branch
6 times, most recently
from
6cf4c9d to
ceb9ad9
Compare
mike182uk
changed the title
mike182uk
force-pushed
the
mike-onc-662-fix-file-write-issues-in-ghost-application-related-to-asset-2
branch
3 times, most recently
from
e48ee21 to
7960d6b
Compare
mike182uk
changed the title
mike182uk
force-pushed
the
mike-onc-662-fix-file-write-issues-in-ghost-application-related-to-asset-2
branch
2 times, most recently
from
173a2e5 to
6ead434
Compare
| @@ -22,7 +22,9 @@ | |||
| "scripts": { | |||
| "archive": "npm pack", | |||
| "dev": "node --watch index.js", | |||
| "build:assets": "postcss core/frontend/public/ghost.css --no-map --use cssnano -o core/frontend/public/ghost.min.css", | |||
| "build:assets:css": "postcss core/frontend/public/ghost.css --no-map --use cssnano -o core/frontend/public/ghost.min.css", | |||
| "build:assets:js": "../minifier/bin/minify core/frontend/public/comment-counts.js core/frontend/public/comment-counts.min.js && ../minifier/bin/minify core/frontend/public/member-attribution.js core/frontend/public/member-attribution.min.js && ../minifier/bin/minify core/frontend/public/admin-auth/message-handler.js core/frontend/public/admin-auth/admin-auth.min.js", | |||
There was a problem hiding this comment.
@daniellockyer Any thoughts on this?
| }, serveStatic( | ||
| path.join(config.getContentPath('public'), 'admin-auth') | ||
| )); | ||
| }, createServeAuthFrameFileMw(config, urlUtils)); |
There was a problem hiding this comment.
Had to switch to using a custom middleware as express/serve-static does not allow us to hook into the response body to perform the necessary placeholder replacements
| @@ -56,29 +53,3 @@ describe('vhost utils', function () { | |||
| }); | |||
| }); | |||
| }); | |||
|
|
|||
| describe('getContentPath', function () { | |||
There was a problem hiding this comment.
No longer needed as we are reverting the changes added here
mike182uk
force-pushed
the
mike-onc-662-fix-file-write-issues-in-ghost-application-related-to-asset-2
branch
2 times, most recently
from
0d7846d to
d85ef0a
Compare
mike182uk
force-pushed
the
mike-onc-662-fix-file-write-issues-in-ghost-application-related-to-asset-2
branch
from
d85ef0a to
0056514
Compare
…application-related-to-asset-2
…application-related-to-asset-2
WalkthroughThe pull request involves a significant refactoring of asset management in the Ghost project. The changes focus on simplifying and streamlining the handling of frontend assets, particularly around admin authentication, comment counts, and member attribution. Multiple asset-related classes and middleware have been removed, with a new emphasis on minimizing and serving only card-related assets. The modifications include updates to configuration files, build scripts, and middleware to support this new approach to asset management. Changes
Sequence DiagramsequenceDiagram
participant Client
participant AdminApp
participant AuthFrameMiddleware
participant FileSystem
Client->>AdminApp: Request auth frame
AdminApp->>AuthFrameMiddleware: Serve auth frame
AuthFrameMiddleware->>FileSystem: Read file
FileSystem-->>AuthFrameMiddleware: Return file content
AuthFrameMiddleware->>AuthFrameMiddleware: Replace placeholders
AuthFrameMiddleware->>Client: Send modified file
This sequence diagram illustrates the new approach to serving the authentication frame, showing how the middleware now directly reads and serves files with placeholder replacements. Tip 🌐 Web search-backed reviews and chat
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
ghost/minifier/bin/minify (1)
22-26: Enhance error handling with specific error messages.The current error handling could be more informative by including specific error details.
} catch (error) { - console.error(error); + console.error('Minification failed:', error.message); + if (process.env.DEBUG) { + console.error(error.stack); + } process.exit(1); }ghost/core/core/bridge.js (1)
19-19: LGTM! Update file header documentation.The import changes align with moving asset generation to build time. Consider updating the file header documentation to reflect the simplified asset management approach.
* The bridge is responsible for handing communication from the server to the frontend. * Data should only be flowing server -> frontend. * As the architecture improves, the number of cross requires here should go down + * Asset management: Only card assets are handled on-demand, while other assets are pre-generated at build time. * Eventually, the aim is to make this a component that is initialized on boot and is either handed to or actively creates the frontend, if the frontend is desired.ghost/core/core/frontend/web/site.js (1)
76-96: LGTM! Card assets middleware refactoring looks good.The separation of CSS and JS middleware with conditional serving improves code organization. Consider extracting the path checks into a shared utility function to reduce duplication.
+function isCardAssetRequest(reqPath, assetPath) { + return reqPath === `/${assetPath}`; +} + function serveCardAssetsCssMiddleware(req, res, next) { - if (req.path === `/${cardAssetsCSSPath}`) { + if (isCardAssetRequest(req.path, cardAssetsCSSPath)) { return cardAssets.serveMiddleware()(req, res, next); } next(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
.gitignore(1 hunks)ghost/core/core/bridge.js(1 hunks)ghost/core/core/frontend/services/assets-minification/AdminAuthAssets.js(0 hunks)ghost/core/core/frontend/services/assets-minification/CommentCountsAssets.js(0 hunks)ghost/core/core/frontend/services/assets-minification/MemberAttributionAssets.js(0 hunks)ghost/core/core/frontend/services/assets-minification/index.js(1 hunks)ghost/core/core/frontend/src/member-attribution/.eslintrc(0 hunks)ghost/core/core/frontend/web/site.js(2 hunks)ghost/core/core/server/web/admin/app.js(3 hunks)ghost/core/core/server/web/admin/middleware/serve-auth-frame-file.js(1 hunks)ghost/core/core/shared/config/helpers.js(1 hunks)ghost/core/package.json(2 hunks)ghost/core/test/unit/server/web/admin/middleware.test.js(2 hunks)ghost/core/test/unit/shared/config/helpers.test.js(0 hunks)ghost/minifier/bin/minify(1 hunks)
💤 Files with no reviewable changes (5)
- ghost/core/core/frontend/services/assets-minification/MemberAttributionAssets.js
- ghost/core/core/frontend/src/member-attribution/.eslintrc
- ghost/core/core/frontend/services/assets-minification/CommentCountsAssets.js
- ghost/core/test/unit/shared/config/helpers.test.js
- ghost/core/core/frontend/services/assets-minification/AdminAuthAssets.js
🔇 Additional comments (12)
ghost/core/core/shared/config/helpers.js (2)
91-91: LGTM! Path construction simplified.The change to directly use content path aligns with the PR's goal of moving asset generation to build time and makes the path resolution more consistent with other cases.
91-91: Verify dependent code that might expect temporary paths.Since we're changing from temporary to content paths for public assets, let's verify that no existing code depends on the old behavior.
✅ Verification successful
No dependency issues found with public asset path changes
After examining the core file handling implementations, particularly in
serve-public-file.jsandasset-url.js, the codebase already properly handles both static files (from core) and built files (from content) through configuration. The change from temporary to content paths aligns with the existing architecture.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for code that might expect temporary paths for public assets echo "Searching for potential references to public asset paths..." # Look for file operations or path constructions involving 'public' rg -l 'public.*(?:path|dir|folder|temp|tmp)' --type js # Look for specific usage of getContentPath with 'public' ast-grep --pattern 'getContentPath("public")' --lang javascript ast-grep --pattern "getContentPath('public')" --lang javascriptLength of output: 857
Script:
#!/bin/bash # Examine the implementation of key files echo "=== serve-public-file.js ===" cat ghost/core/core/frontend/web/middleware/serve-public-file.js echo -e "\n=== asset-url.js ===" cat ghost/core/core/frontend/meta/asset-url.js echo -e "\n=== Searching for test assertions about public paths ===" rg "public.*path.*assert" --type js -C 2Length of output: 6845
ghost/core/core/frontend/services/assets-minification/index.js (1)
1-7: LGTM! Simplified asset management aligns with PR objectives.The removal of boot-time assets (
AdminAuthAssets,CommentCountsAssets,MemberAttributionAssets) and retaining onlycardAssetsaligns with moving asset generation to build time while keeping cards generated on demand.Let's verify that the removed assets are now handled at build time:
✅ Verification successful
Verified: Asset minification changes are safe and aligned with build-time approach
The removed asset classes (
AdminAuthAssets,CommentCountsAssets,MemberAttributionAssets) were specifically for runtime minification. The features themselves remain properly implemented in their respective areas (admin, comments-ui, member services), indicating these assets are now handled at build time rather than runtime.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that removed assets are now handled at build time # Expected: Find build scripts or tasks for these assets # Search for build-time asset generation rg -l "comment-counts|member-attribution|admin-auth" package.jsonLength of output: 67
Script:
#!/bin/bash # Search for removed assets across the codebase echo "=== Searching for asset references ===" rg -l "AdminAuthAssets|CommentCountsAssets|MemberAttributionAssets" echo -e "\n=== Checking build scripts ===" fd -e js -e ts build.js build-assets.js echo -e "\n=== Checking for asset files ===" fd -g "*admin-auth*" -g "*comment-counts*" -g "*member-attribution*"Length of output: 701
Script:
#!/bin/bash # Search for asset-related files and configurations echo "=== Searching for webpack configs ===" fd -t f webpack.config.js echo -e "\n=== Searching for asset source files ===" rg -l "admin-auth|comment-counts|member-attribution" --type js --type ts echo -e "\n=== Searching for client implementations ===" rg -l "loadAdminAuth|loadCommentCounts|loadMemberAttribution" --type js --type tsLength of output: 1946
ghost/minifier/bin/minify (1)
1-2: Ensure script has executable permissions.The shebang line is correct, but ensure the script has proper executable permissions set.
✅ Verification successful
Script permissions are correctly set
The script has appropriate executable permissions (-rwxr-xr-x) allowing execution by all users while maintaining security.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the script has executable permissions ls -l ghost/minifier/bin/minifyLength of output: 107
ghost/core/core/server/web/admin/app.js (1)
12-12: LGTM! Auth frame middleware changes look good.The switch to a custom middleware is well-justified as it allows for response body modifications that aren't possible with express/serve-static.
Also applies to: 42-55
ghost/core/test/unit/server/web/admin/middleware.test.js (2)
1-8: LGTM! Test setup is comprehensive.The test setup includes proper mocking of dependencies and file system operations, with good coverage of edge cases.
Also applies to: 67-107
113-203: LGTM! Test cases provide thorough coverage.Excellent test coverage including security aspects (path traversal), error handling, and content replacement functionality.
ghost/core/core/frontend/web/site.js (1)
99-102: LGTM! Asset serving simplified correctly.The change to use servePublicFile middleware aligns with the PR objective of moving asset generation to build time.
.gitignore (1)
125-127: LGTM! Build artifacts correctly ignored.The added entries properly exclude the build-time generated minified JS files from version control.
ghost/core/package.json (3)
25-27: Review the hardcoded paths in build scripts.The build scripts contain several hardcoded paths which could make the build process fragile:
- CSS build:
core/frontend/public/ghost.css- JS build: Multiple paths in
core/frontend/public/Let's verify these paths exist and are consistent:
Consider:
- Using path variables or configuration for better maintainability
- Adding error handling for missing files
- Using glob patterns for better flexibility
Example refactor:
- "build:assets:css": "postcss core/frontend/public/ghost.css --no-map --use cssnano -o core/frontend/public/ghost.min.css", + "build:assets:css": "postcss \"$(node -e 'console.log(require(\"./config\").paths.cssSource)')\" --no-map --use cssnano -o \"$(node -e 'console.log(require(\"./config\").paths.cssOutput)')\"",
26-26: 🛠️ Refactor suggestionVerify minifier dependency and its capabilities.
The script uses a relative path
../minifier/bin/minifywhich could be problematic:
- It assumes a specific directory structure
- It might not work in all environments (e.g., CI/CD)
Let's verify the minifier setup:
Consider:
- Using the package's binary directly through
node_modules- Adding error handling for missing minifier
- Using parallel processing for multiple files
Example refactor:
- "build:assets:js": "../minifier/bin/minify core/frontend/public/comment-counts.js core/frontend/public/comment-counts.min.js && ../minifier/bin/minify core/frontend/public/member-attribution.js core/frontend/public/member-attribution.min.js && ../minifier/bin/minify core/frontend/public/admin-auth/message-handler.js core/frontend/public/admin-auth/admin-auth.min.js", + "build:assets:js": "node scripts/build-js-assets.js",Create a new file
scripts/build-js-assets.jsto handle JS minification with proper error handling and parallel processing.
285-285: Verify build order in dev dependencies.The
devscript now depends onbuild:assets, which is correct given the PR's objective to move asset generation to build time. However, we should verify the build order is optimal.Let's analyze the dependency tree:
The dependency order looks correct:
build:assetsruns firstbuild:tsruns after
This ensures assets are available before the development server starts.✅ Verification successful
Build order is properly managed by nx dependency system
The nx configuration in nx.json shows that build dependencies are correctly managed through nx's target defaults. The system ensures that:
- Dependencies are built before dependent projects
- TypeScript builds follow proper dependency order
- Build caching is configured for optimal performance
The integration of
build:assetsinto this system is safe as nx will automatically handle the correct build sequence based on project dependencies.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Analyze build dependencies and their order # Check for circular dependencies in nx configuration echo "Checking for circular dependencies..." jq '.nx.targets | to_entries[] | select(.value.dependsOn != null) | .key + ": " + (.value.dependsOn | join(", "))' package.json # Verify build:ts is properly ordered echo "Checking build:ts ordering..." jq '.nx.targets.dev.dependsOn[]' package.jsonLength of output: 453
Script:
#!/bin/bash # Extract and analyze build-related scripts echo "Analyzing build scripts..." jq -r '.scripts | to_entries | .[] | select(.key | contains("build") or contains("dev"))' package.json # Check if there are any pre/post scripts echo -e "\nChecking for pre/post scripts..." jq -r '.scripts | keys[] | select(startswith("pre") or startswith("post"))' package.json # Show the actual dev script content echo -e "\nDev script content:" jq -r '.scripts.dev' package.jsonLength of output: 945
Script:
#!/bin/bash # Check the content of dev.js script echo "Examining dev.js script content..." cat .github/scripts/dev.js # Search for build-related patterns in dev.js echo -e "\nSearching for build patterns..." grep -A 5 -B 5 "build:assets\|build:" .github/scripts/dev.jsLength of output: 10083
Script:
#!/bin/bash # Check nx configuration echo "Checking nx configuration..." cat nx.json 2>/dev/null || echo "nx.json not found" echo -e "\nChecking ghost/core/project.json..." cat ghost/core/project.json 2>/dev/null || echo "project.json not found" # Find and check all project.json files echo -e "\nSearching for all project.json files..." find . -name "project.json" -type f -exec sh -c 'echo "=== {} ==="; cat "{}"' \;Length of output: 1542
| const src = path.parse(process.argv[2]); | ||
| const dest = path.parse(process.argv[3]); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add command-line argument validation.
The script should validate that both required arguments are provided.
+if (process.argv.length !== 4) {
+ console.error('Usage: bin/minify <source> <destination>');
+ process.exit(1);
+}
+
const src = path.parse(process.argv[2]);
const dest = path.parse(process.argv[3]);📝 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.
| const src = path.parse(process.argv[2]); | |
| const dest = path.parse(process.argv[3]); | |
| if (process.argv.length !== 4) { | |
| console.error('Usage: bin/minify <source> <destination>'); | |
| process.exit(1); | |
| } | |
| const src = path.parse(process.argv[2]); | |
| const dest = path.parse(process.argv[3]); |
| const filename = path.parse(req.url).base; | ||
|
|
||
| let basePath = path.join(config.get('paths').publicFilePath, 'admin-auth'); | ||
| let filePath; | ||
|
|
||
| if (filename === '') { | ||
| filePath = path.join(basePath, 'index.html'); | ||
| } else { | ||
| filePath = path.join(basePath, filename); | ||
| } |
There was a problem hiding this comment.
Prevent path traversal vulnerability.
The current implementation might be vulnerable to path traversal attacks through the filename in the URL.
- const filename = path.parse(req.url).base;
+ const filename = path.basename(req.url);
+
+ // Validate filename to prevent directory traversal
+ if (filename.includes('..') || !filename.match(/^[a-zA-Z0-9._-]+$/)) {
+ return next();
+ }
let basePath = path.join(config.get('paths').publicFilePath, 'admin-auth');
let filePath;📝 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.
| const filename = path.parse(req.url).base; | |
| let basePath = path.join(config.get('paths').publicFilePath, 'admin-auth'); | |
| let filePath; | |
| if (filename === '') { | |
| filePath = path.join(basePath, 'index.html'); | |
| } else { | |
| filePath = path.join(basePath, filename); | |
| } | |
| const filename = path.basename(req.url); | |
| // Validate filename to prevent directory traversal | |
| if (filename.includes('..') || !filename.match(/^[a-zA-Z0-9._-]+$/)) { | |
| return next(); | |
| } | |
| let basePath = path.join(config.get('paths').publicFilePath, 'admin-auth'); | |
| let filePath; | |
| if (filename === '') { | |
| filePath = path.join(basePath, 'index.html'); | |
| } else { | |
| filePath = path.join(basePath, filename); | |
| } |
| return fs.readFile(filePath).then((data) => { | ||
| let dataString = data.toString(); | ||
|
|
||
| for (const [key, value] of Object.entries(placeholders)) { | ||
| dataString = dataString.replace(key, value); | ||
| } | ||
|
|
||
| res.end(dataString); | ||
| }).catch(() => { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add caching and content type headers.
The current implementation lacks caching headers and proper content type detection.
return fs.readFile(filePath).then((data) => {
let dataString = data.toString();
for (const [key, value] of Object.entries(placeholders)) {
dataString = dataString.replace(key, value);
}
+ // Set content type based on file extension
+ const ext = path.extname(filePath);
+ const contentType = {
+ '.html': 'text/html',
+ '.js': 'application/javascript',
+ '.css': 'text/css'
+ }[ext] || 'text/plain';
+
+ // Set headers
+ res.setHeader('Content-Type', contentType);
+ res.setHeader('Cache-Control', 'public, max-age=3600');
+
res.end(dataString);Committable suggestion skipped: line range outside the PR's diff.
…application-related-to-asset-2
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ghost/core/package.json (1)
25-27: Asset Build Scripts Refactoring
The introduction of the new asset build scripts is clear and aligns well with the PR objectives. The"build:assets:css"script uses PostCSS with cssnano for CSS minification, and"build:assets:js"chains multiple minification commands for JS assets. Please ensure that the relative paths (e.g.core/frontend/public/ghost.cssand the path to the minifier../minifier/bin/minify) are correct with respect to the location of thispackage.json. Also, consider if a failure in one of the chained commands should halt the process (which is the default behavior with&&).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ghost/core/package.json(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Unit tests (Node 20.11.1)
- GitHub Check: Unit tests (Node 18.12.1)
- GitHub Check: Regression tests (Node 20.11.1, sqlite3)
- GitHub Check: Regression tests (Node 20.11.1, mysql8)
- GitHub Check: Database tests (Node 20.11.1, sqlite3)
- GitHub Check: Database tests (Node 20.11.1, mysql8)
- GitHub Check: Lint
- GitHub Check: i18n
- GitHub Check: Database tests (Node 18.12.1, mysql8)
- GitHub Check: Ghost-CLI tests
🔇 Additional comments (1)
ghost/core/package.json (1)
286-287: Integrating Asset Building into the Dev Target
Adding"build:assets"as a dependency in the"dev"target under thenxconfiguration ensures that assets are built before the development server starts. This integration helps prevent runtime issues related to on-demand asset generation. Verify that this sequencing does not adversely affect dev start-up time for developers.
refs ONC-662
Refactored the generation of the following assets:
comment-counts.jsmember-attribution.jsadmin-authto occur at build time instead of at boot time or on demand
cardsare still built on demand as these assets need to be re-generated when a theme changes - we utilise the existing asset minification system for this, except we only execute the middleware responsible for this when a request is for a card asset (instead of any path, which is a bug in the current implementation)This also reverts the changes made in #21857 as these changes are no longer needed (because the assets are now generated at build time)