Templates engine#68
Conversation
This commit refines the hostname validation process in the middleware by introducing a more robust check for main domains. It modifies the redirection logic to allow normal routing for valid main domains while maintaining the existing subdomain detection functionality. This enhances the clarity and maintainability of the code.
…uction This commit updates the npm ci command to include development dependencies and adds an echo command to skip husky setup in production. Additionally, it modifies the prepare and postinstall scripts in package.json to conditionally run husky only if not in a production environment, enhancing the build process.
WalkthroughThe changes update build and install scripts to conditionally run Husky only when appropriate, avoiding its setup in production environments. The middleware logic is also revised to strictly permit only the main domain and its "www." variant, modifying how requests are validated and processed based on hostname. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Middleware
participant App
User->>Middleware: HTTP request with hostname
Middleware->>Middleware: Remove port from hostname
alt Hostname is main domain or "www." domain
Middleware->>App: Allow request to proceed
else
Middleware->>User: Block or handle as invalid
end
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 0
🧹 Nitpick comments (1)
package.json (1)
10-11: Good defensive programming for Husky setup.The conditional logic properly prevents Husky-related errors in production environments. However, consider these improvements:
- The shell syntax mixing could be more consistent
- Consider adding error handling for edge cases
For better consistency and robustness, consider this approach:
- "prepare": "if command -v husky >/dev/null 2>&1; then husky; fi", - "postinstall": "if [ \"$NODE_ENV\" != \"production\" ] && command -v husky >/dev/null 2>&1; then husky; fi", + "prepare": "command -v husky >/dev/null 2>&1 && husky || true", + "postinstall": "[ \"$NODE_ENV\" != \"production\" ] && command -v husky >/dev/null 2>&1 && husky || true",This approach uses consistent shell syntax and ensures the scripts never fail.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
amplify.yml(1 hunks)middleware.ts(1 hunks)package.json(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (4)
amplify.yml (1)
18-19:Details
✅ Verification successful
Verify the necessity of dev dependencies in production build.
The addition of
--include=devexplicitly installs dev dependencies, but this contradicts the stated goal of avoiding Husky in production environments.Consider whether dev dependencies are truly needed for the frontend build:
If dev dependencies aren't required for building, consider reverting to standard
npm cito align with production best practices.
🏁 Script executed:
#!/bin/bash # Description: Check if build process actually requires dev dependencies # Search for dev dependencies usage in build scripts and frontend code echo "Checking package.json build script dependencies:" cat package.json | jq '.scripts.build' echo "Checking if any build-time imports reference devDependencies:" rg -A 3 -B 3 'import.*from.*(' --type js --type ts --type jsx --type tsx | head -20 echo "Checking Next.js config for dev dependency usage:" fd "next.config" --exec cat {}Length of output: 1403
Let’s confirm which devDependencies exist and if the codebase uses TypeScript (and thus needs devDeps for builds):
🏁 Script executed:
#!/bin/bash # List all devDependencies echo "devDependencies in package.json:" jq '.devDependencies' package.json echo "" # Show a sample of TypeScript files (if any) echo "First 20 .ts/.tsx files:" find . -maxdepth 4 \( -name '*.ts' -o -name '*.tsx' \) | head -n 20 echo "" # Count total TS/TSX files echo "Total .ts/.tsx file count:" find . -maxdepth 4 \( -name '*.ts' -o -name '*.tsx' \) | wc -lLength of output: 2255
No changes required: devDependencies are needed for the build
The Next.js build (next build) and its tooling—TypeScript, TailwindCSS, PostCSS, etc.—all live indevDependencies, so usingnpm ci --include=devin the CI is intentional and necessary. Skipping Husky setup in production via theechois also correct and safe.No further action needed.
middleware.ts (3)
30-30: Minor improvement: consistent variable naming.The change to extract
cleanHostnameis good for consistency with the domain validation logic above.
23-26:Details
❓ Verification inconclusive
Verify the removal of redirect logic for main domains.
The change from redirecting to continuing with
NextResponse.next()for main domains could affect the user experience. Ensure this doesn't break any expected redirect behavior.Search for any tests or documentation that might expect redirect behavior:
🏁 Script executed:
#!/bin/bash # Description: Check for tests or docs mentioning domain redirects echo "Searching for redirect-related tests:" rg -i "redirect.*domain|domain.*redirect" --type js --type ts -A 3 -B 3 echo "Checking for any middleware tests:" fd "middleware" --type f --exec rg -l "test|spec" {} echo "Looking for any redirect expectations in comments:" rg "redirect|rewrite" --type ts --type js -A 2 -B 2 | grep -i "domain\|main\|www"Length of output: 1313
Confirm removal of main-domain redirect doesn’t impact user flows
I ran searches across tests, specs, and documentation—there are no references to redirect logic for main domains. Please verify that replacing the redirect with
NextResponse.next()in middleware.ts (lines 23–26) preserves all expected behaviors. If needed, update or add tests and documentation to cover this change.
18-21:Details
✅ Verification successful
Verify the impact of stricter domain validation.
The updated logic now only allows exact domain matches or www-prefixed variants, which is more secure but could break existing functionality if subdomains were previously supported.
Consider adding logging to track any blocked requests:
const isMainDomain = allowedDomains.some(domain => { const cleanHostname = hostname.split(':')[0] - return cleanHostname === domain || cleanHostname === `www.${domain}` + const isMatch = cleanHostname === domain || cleanHostname === `www.${domain}` + if (!isMatch && process.env.NODE_ENV === 'development') { + console.log(`Blocked hostname: ${cleanHostname}, expected: ${domain} or www.${domain}`) + } + return isMatch })Verify that this change doesn't break existing subdomain functionality:
🏁 Script executed:
#!/bin/bash # Description: Check for any subdomain references in the codebase echo "Searching for subdomain-related code:" rg -i "subdomain|\.fasttify\.com" --type ts --type js --type tsx --type jsx -A 2 -B 2 echo "Checking for any hardcoded domain references:" rg "fasttify\.com|localhost" --type ts --type js --type tsx --type jsx -A 1 -B 1Length of output: 412
🏁 Script executed:
#!/bin/bash # Re-run domain reference checks without file‐type filters to avoid “unrecognized file type” errors echo "Searching for subdomain-related code (no type filters):" rg -i 'subdomain|\.fasttify\.com' -A2 -B2 . echo "Checking for hardcoded domain references (no type filters):" rg 'fasttify\.com|localhost' -A1 -B1 .Length of output: 10867
Domain validation update confirmed
I’ve verified that the new
isMainDomaincheck only applies to the main domains (fasttify.com/www.fasttify.comin production,localhost/www.localhostin development) and does not interfere with the existingextractSubdomainlogic or subdomain‐based URL rewriting. Subdomains (e.g.store.fasttify.comorfoo.localhost:3000) are still correctly detected and routed. No further changes are needed.
1 participant
Summary by CodeRabbit