Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactored boot time / on demand asset generation #21866

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

mike182uk
Copy link
Member

@mike182uk mike182uk commented Dec 11, 2024

refs ONC-662

Refactored the generation of the following assets:

  • comment-counts.js
  • member-attribution.js
  • admin-auth

to occur at build time instead of at boot time or on demand

cards are 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)

@mike182uk 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 December 12, 2024 14:56
@mike182uk mike182uk changed the title Refactored built asset generation for static assets Refactored boot time / on demand asset generation to occur at build time Dec 12, 2024
@mike182uk 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 December 13, 2024 14:01
@mike182uk mike182uk changed the title Refactored boot time / on demand asset generation to occur at build time Refactored boot time / on demand asset generation Dec 13, 2024
@mike182uk 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 December 13, 2024 14:12
@mike182uk mike182uk marked this pull request as ready for review December 13, 2024 14:21
@@ -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",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@daniellockyer Any thoughts on this?

}, serveStatic(
path.join(config.getContentPath('public'), 'admin-auth')
));
}, createServeAuthFrameFileMw(config, urlUtils));
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 () {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No longer needed as we are reverting the changes added here

@mike182uk 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 December 20, 2024 10:09
@mike182uk mike182uk force-pushed the mike-onc-662-fix-file-write-issues-in-ghost-application-related-to-asset-2 branch from d85ef0a to 0056514 Compare January 6, 2025 08:14
Copy link
Contributor

coderabbitai bot commented Jan 23, 2025

Walkthrough

The 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

File Change Summary
.gitignore Added entries to ignore minified JavaScript files for comment counts, member attribution, and admin authentication
ghost/core/core/bridge.js Removed ensureAdminAuthAssetsMiddleware and simplified asset imports
ghost/core/core/frontend/services/assets-minification/ Removed AdminAuthAssets.js, CommentCountsAssets.js, and MemberAttributionAssets.js classes
ghost/core/core/frontend/services/assets-minification/index.js Updated exports to only include cardAssets
ghost/core/core/frontend/web/site.js Refactored asset middleware handling
ghost/core/core/server/web/admin/app.js Removed ensureAdminAuthAssetsMiddleware and added new auth frame file middleware
ghost/core/package.json Added new build scripts for CSS and JS asset minification
ghost/minifier/bin/minify New script for file minification

Sequence Diagram

sequenceDiagram
    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
Loading

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
  • We have enabled web search-based reviews and chat for all users. This feature allows CodeRabbit to access the latest documentation and information on the web.
  • You can disable this feature by setting web_search: false in the knowledge_base settings.
  • Please share any feedback in the Discord discussion.

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 8c2e62d and 4903245.

📒 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.js and asset-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 javascript

Length 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 2

Length 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 only cardAssets aligns 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.json

Length 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 ts

Length 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/minify

Length 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:

  1. CSS build: core/frontend/public/ghost.css
  2. JS build: Multiple paths in core/frontend/public/

Let's verify these paths exist and are consistent:

Consider:

  1. Using path variables or configuration for better maintainability
  2. Adding error handling for missing files
  3. 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 suggestion

Verify minifier dependency and its capabilities.

The script uses a relative path ../minifier/bin/minify which could be problematic:

  1. It assumes a specific directory structure
  2. It might not work in all environments (e.g., CI/CD)

Let's verify the minifier setup:

Consider:

  1. Using the package's binary directly through node_modules
  2. Adding error handling for missing minifier
  3. 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.js to handle JS minification with proper error handling and parallel processing.


285-285: Verify build order in dev dependencies.

The dev script now depends on build: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:

  1. build:assets runs first
  2. build:ts runs 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:assets into 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.json

Length 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.json

Length 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.js

Length 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

Comment on lines +12 to +14
const src = path.parse(process.argv[2]);
const dest = path.parse(process.argv[3]);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
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]);

Comment on lines +10 to +19
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);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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);
}

Comment on lines +21 to +29
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(() => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.css and the path to the minifier ../minifier/bin/minify) are correct with respect to the location of this package.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

📥 Commits

Reviewing files that changed from the base of the PR and between 4903245 and de468b2.

📒 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 the nx configuration 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant