Enable Shakapacker early hints #687
Conversation
WalkthroughAdds Thruster HTTP/2 proxy integration (gem, commands, Dockerfile), bumps Ruby/shakapacker versions, introduces Early Hints verification scripts, updates webpack/dev/test driver configs, adds Control Plane and Thruster docs, and adds a UI footer notice and related config/template changes. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser
participant Thruster as Thruster (HTTP/2 proxy)
participant Rails as Rails (app container, HTTP/1.1)
Note over Thruster,Rails: Thruster terminates TLS/HTTP2 and proxies to Rails over HTTP/1.1
Browser->>Thruster: HTTP/2 GET /
Thruster->>Rails: proxied HTTP/1.1 GET /
Rails-->>Thruster: 103 Early Hints (Link headers)
Thruster-->>Browser: 103 Early Hints (via HTTP/2)
Rails-->>Thruster: 200 OK + body
Thruster-->>Browser: 200 OK + body
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas to focus review on:
Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
/deploy-review-app |
🚀 Quick Review App CommandsWelcome! Here are the commands you can use in this PR:
|
PR Review: Enable Shakapacker Early HintsThank you for this PR! Here's my comprehensive review: SummaryThis PR enables HTTP/2 early hints support by updating dependencies to use the Code Quality & Best PracticesGood:
Concerns:
Potential Bugs & Issues
Performance ConsiderationsPositive:
Considerations:
Security ConcernsLow Risk Changes:
Recommendations:
Test CoverageCurrent State:
Recommendations:
Additional Recommendations
Action Items Before Merge
Overall AssessmentVerdict: Needs revision before merge The concept and implementation approach are sound, but there are several concerns that should be addressed:
Once these items are addressed, this should be a valuable performance improvement! Let me know if you need help with any of these recommendations. |
![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)
config/shakapacker.yml (1)
67-69: Add context and rationale for the early_hints configuration.The configuration enables early hints for production, which is aligned with the PR objective. However, the comment is minimal. Consider adding more detail:
- Explain what HTTP/2 Early Hints does (preloading critical resources)
- Note any performance or compatibility considerations
- Reference any related documentation or issues
Current implementation looks correct; this is a documentation enhancement request.
Consider updating the comment as follows:
# Cache manifest.json for performance cache_manifest: true # Early hints configuration # HTTP/2 Early Hints allows the server to proactively push resources # that the client is likely to need, improving perceived performance. # Requires HTTP/2 support and compatible browser/client. early_hints: enabled: true
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Gemfile.lockis excluded by!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (3)
Gemfile(1 hunks)config/shakapacker.yml(1 hunks)package.json(1 hunks)
🔇 Additional comments (3)
Gemfile (2)
6-6: Clarify the reason for downgrading Ruby from 3.4.6 to 3.4.3.This is a patch-version downgrade without explanation. Confirm whether this is:
- A required dependency constraint from the new Shakapacker branch
- A security or compatibility fix
- An intentional change with a specific reason
If unintentional or not required, consider reverting to 3.4.6.
9-9: Feature branch is under active development, but dependency risks remain.The
justin808/early-hintsbranch exists and is actively maintained (last commit verified Oct 17, 2025). Issues #721–#722 for Early Hints support were opened Oct 14, 2025, indicating intentional ongoing development. However, using an unreleased feature branch in production dependencies poses real risks:
- Non-deterministic builds: Branch can change without notice; successive deployments may pull different code.
- Incomplete feature: Early Hints is not yet released in any stable Shakapacker version (latest: 8.3.0, April 2025).
- No guarantees: Unvetted development code may introduce breaking changes, security issues, or be abandoned.
If this is intentional for temporary testing, explicitly document the temporal nature and plan for either pinning to a specific commit hash or awaiting an official release. Otherwise, consider reverting to a stable released version.
package.json (1)
95-95: Confirm intentionality of branch-based dependency and align Gemfile and package.json if upgrading.Both Gemfile and package.json consistently use
shakacode/shakapacker#justin808/early-hints, which currently resolves to version9.3.0.beta.2. While stable releases exist (9.1.0,9.2.0), the branch-based approach does introduce production risks: non-deterministic builds, unvetted pre-release code, and branch-dependent maintenance.Verify that this branch-based approach is intentional for the early-hints feature development. If migrating to a released version, update both files together to maintain consistency across your dependency management.
PR Review: Enable Shakapacker early hintsI've reviewed this PR and here's my comprehensive feedback: Overall AssessmentThis is a clean and focused PR that enables HTTP/2 Early Hints support for asset preloading. The changes are well-scoped and follow a logical pattern. Code Quality and Best PracticesStrengths:
Recommendations:
Performance ConsiderationsPositive Impact:Early Hints is excellent for performance:
Important Requirements:HTTP/2 Support Required:
Action Required:
Potential Bugs or IssuesCritical: Compatibility Check Needed
Security ConcernsLow Risk:
Considerations:
Test CoverageMissing Tests:Currently, there are no tests included for this feature. This is concerning for a production configuration change. Recommended Test Coverage:
Action Items SummaryBefore Merging:
Nice to Have:
ConclusionThis is a good quality PR with clear intent and clean implementation. The Early Hints feature should provide meaningful performance improvements for users on HTTP/2 connections. Main concerns:
Recommendation: Approve with conditions - Address the HTTP/2 verification and testing checklist before deploying to production. Great work on keeping the changes focused and well-organized! |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (1)
package.json(2 hunks)
🔇 Additional comments (2)
package.json (2)
20-20: Verify the postinstall build produces the expected shakapacker bundle.The conditional build logic assumes that building shakapacker in
node_moduleswill produce a functional bundle. Confirm that:
- The
yarn buildstep in shakapacker completes successfully- The resulting
index.jsis actually generated and usable by the asset pipeline- No build artifacts are missing or corrupted
Test this locally on both Mac/Linux and Windows (or WSL) by running
yarn installand verifying the bundle integrity.
95-95: <!-- [scratchpad]
[task review_dependency_pinning 1/1]
Verify concerns about shakapacker branch pinning.[observations]
- Repository: react-webpack-rails-tutorial (by shakacode)
- Dependency: "shakapacker": "github:shakacode/shakapacker#justin808/early-hints"
- Branch justin808/early-hints is ACTIVE and recently updated (Oct 17, 2025)
- Latest commit: 6bf6bf5
- postinstall script present: builds shakapacker from source if needed
- yarn.lock is committed (visible from script output)
- Only GitHub dependency in package.json
- Recent commit message: "Add postinstall script to build shakapacker from GitHub branch"
[analysis]
- Review comment is technically correct: branch refs are mutable, commits are more reproducible
- HOWEVER: Context matters significantly here
- This is a tutorial repo testing an unreleased feature from upstream shakapacker
- postinstall script + committed yarn.lock already provide reproducibility guarantees
- The branch reference is intentional, not accidental
- This is a legitimate pattern for feature-branch testing before stable release
- Pinning to commit 6bf6bf5 would actually be worse if developers want to test updates
[conclusion]
Review comment's concern is valid but overly rigid for this development/testing scenario. The current setup mitigates the stated risks through:
- Committed yarn.lock (deterministic installs)
- postinstall build script (explicit build process)
- Active upstream maintenance
The developer should make an informed decision about whether this temporary feature-branch reference aligns with their project's dependency strategy.[done]
Verification complete. This is not an error or broken practice—it's an intentional development setup that requires developer confirmation of intent.
</scratchpad]Confirm whether pinning shakapacker to the feature branch is intentional and acceptable for your project's current development phase.
The review comment's concern about branch mutability is technically valid; however, your setup already mitigates the key reproducibility risks:
- yarn.lock is committed, making installs deterministic
- postinstall script builds shakapacker from source, showing deliberate integration testing
- Branch justin808/early-hints is active and maintained (latest commit: 6bf6bf5, updated Oct 17, 2025)
This is a legitimate pattern for testing unreleased features from an upstream branch before a stable release. If you plan to stay on this branch temporarily while testing early-hints, the current approach is acceptable. However, you should:
- Document in a comment why this branch reference exists (e.g., "TODO: pin to shakapacker release once early-hints merges")
- Plan to upgrade to a stable shakapacker release once the feature ships
- If you want extra safety now, pin to the specific commit (
#6bf6bf5) with a comment explaining it's temporary
🎉 ✨ Deploy Complete! 🚀🌐 ➡️ Open Review AppDeployment successful for PR #687, commit 9fca254 🎮 Control Plane Console |
Pull Request Review: Enable Shakapacker Early HintsThank you for this PR! I've reviewed the changes to enable early hints support in Shakapacker. Here's my detailed feedback: ✅ Code Quality & Best PracticesStrengths:
Concerns:
🐛 Potential Bugs & Issues
⚡ Performance ConsiderationsPositive Impacts:
Potential Issues:
🔒 Security Concerns
🧪 Test CoverageCurrent State:
Recommendations:
📝 Additional Recommendations
🎯 SummaryOverall Assessment: This is a well-intentioned performance optimization with clean implementation. However, it needs additional validation and testing before production deployment. Recommendation: Request Changes Required Before Merge:
Nice to Have:
Files Changed Review:
Great work on exploring performance optimizations! Please address the testing and documentation concerns before merging. Review generated with assistance from Claude Code |
🎉 ✨ Deploy Complete! 🚀🌐 ➡️ Open Review AppDeployment successful for PR #687, commit eb1b3be 🎮 Control Plane Console |
Pull Request Review: Enable Shakapacker Early HintsOverviewThis PR updates the project to use a development branch of Shakapacker that includes early hints support, and enables the early hints feature in production. The changes are focused on dependency updates and configuration. Code Quality & Best Practices✅ Strengths
|
🎉 ✨ Deploy Complete! 🚀🌐 ➡️ Open Review AppDeployment successful for PR #687, commit 9930aaf 🎮 Control Plane Console |
Pull Request Review: Enable Shakapacker Early HintsThank you for this PR! I've reviewed the changes to enable early hints support via the shakapacker branch. Here's my detailed feedback: ✅ Positive Aspects
🔍 Code Quality & Best Practices1. PostInstall Script (package.json:20)The postinstall script has grown quite complex. This one-liner is hard to read and maintain. Concerns:
Recommendations:
2. Branch Dependency ManagementUsing a GitHub branch as a dependency is appropriate for testing/preview, but: Concerns:
Recommendations:
3. Early Hints Configuration (config/shakapacker.yml:67-70)The configuration looks good, but could use more documentation. Recommendations:
🐛 Potential Issues1. Ruby Version Change (Gemfile.lock)I noticed the Ruby version patch changed from 3.4.6p32 to 3.4.6p54. This appears to be an unintentional side effect rather than a deliberate change. Impact: Low - same minor/patch version 2. Yarn.lock ChangesThe yarn.lock shows all resolved URLs changed format. This is cosmetic but suggests a potential yarn version inconsistency. Recommendation: Ensure all developers are using the same yarn version (currently specified as 1.22.22 in package.json) 🔒 Security Considerations1. Branch Dependency Security✅ Good: Using a specific commit in the lock file provides reproducible builds Recommendations:
2. Build Script ExecutionThe postinstall script runs arbitrary build commands from a third-party package. Impact: Low (trusted source - shakacode) ⚡ Performance Considerations1. Install TimeThe postinstall script conditionally builds shakapacker from source, which:
Recommendations:
2. Early Hints Benefits✅ This is the whole point of the PR! Early hints should provide measurable performance improvements. Recommendations:
🧪 Testing & VerificationThe PR description has a testing checklist, which is great! However: Missing Test Coverage:
Recommendations:
📋 SummaryOverall Assessment: This is a solid PR with a clear purpose and systematic implementation. The main concerns are around the complexity of the postinstall script and the temporary nature of using a branch dependency. Priority Recommendations:
Approval Status: Great work on improving the application's performance! Let me know if you'd like help implementing any of these suggestions. |
🎉 ✨ Deploy Complete! 🚀🌐 ➡️ Open Review AppDeployment successful for PR #687, commit 395b7a4 🎮 Control Plane Console |
|
PR Review: Enable Shakapacker Early Hints Overview: This PR enables HTTP/2 early hints support in Shakapacker to improve perceived page load performance by allowing the server to send link headers early in the response lifecycle. Code Quality - Positive aspects:
Suggestions:
Potential Issues:
Performance Considerations: Positive:
Considerations:
Security Concerns (Overall: Low Risk):
Test Coverage Concerns:
Additional Recommendations:
Summary: Verdict: Approve with minor suggestions This PR is well-structured and implements a useful performance feature. The main concerns are:
Before merging:
After merging:
Great work on improving performance! |
🎉 ✨ Deploy Complete! 🚀🌐 ➡️ Open Review AppDeployment successful for PR #687, commit 58f9d8e 🎮 Control Plane Console |
Pull Request Review: Enable Shakapacker Early HintsOverviewThis PR enables HTTP/2 Early Hints support for Shakapacker by updating dependencies to use the justin808/early-hints branch and configuring the feature in production. Overall, this is a positive change that should improve perceived page load performance. Summary of Changes
Code Quality and Best PracticesStrengths
Areas for Improvement1. Temporary Dependency Configuration 2. Postinstall Script Complexity 3. Configuration Documentation Potential Bugs or IssuesMedium Priority
Low Priority
Performance ConsiderationsPositive Impact
Potential Concerns
Recommendation: Document server requirements in PR description or README Security ConcernsGenerally safe - no major security concerns identified. Minor Considerations:
Test CoverageMissing automated tests. The PR description has a testing checklist but no automated test coverage. Recommendations:
Additional RecommendationsDocumentation
Deployment Strategy
Future Improvements
ConclusionThis is a well-structured PR that adds a valuable performance feature. The implementation is clean and the commit history shows good iteration. Approve with Minor ChangesBefore Merging:
After Merging:
Great work on implementing early hints support! |
Pull Request Review: Enable Shakapacker Early HintsSummaryThis PR enables early hints support by switching to a development branch of Shakapacker. Overall, the implementation looks reasonable for experimental/development purposes, but there are several important considerations before merging. Code Quality & Best PracticesStrengths
Concerns1. Using a Development Branch in Production (Critical)Files: Gemfile:9, package.json:95 Both Ruby and Node dependencies point to a feature branch (justin808/early-hints) rather than a stable release. Issues:
Recommendation:
2. Complex postinstall Script (Medium Priority)File: package.json:20 The postinstall script is complex and fragile:
Questions:
Recommendations:
3. Debug Mode in Production (Security/Performance)File: config/shakapacker.yml:70 Debug mode is enabled in production config which may leak information about asset loading, timing, or internal paths through HTML comments. Recommendation: Set debug: false for production or make it environment-variable controlled Potential Bugs & Issues1. Gemfile.lock Ruby Version Change (Low Priority)The Ruby version changed from 3.4.6p32 to 3.4.6p54. Was this intentional? 2. Missing Error Handling
Performance ConsiderationsPositive Impacts
Concerns
Recommendations:
Security Concerns1. Supply Chain Risk (High Priority)
Mitigation:
2. Information Disclosure (Low Priority)
3. Dependency AuditRun security audits: bundle audit and yarn audit Test CoverageMissing TestsThe PR description shows an incomplete testing checklist Recommendations:
Additional Recommendations1. Documentation
2. Server RequirementsEarly hints require:
Action: Verify production infrastructure supports early hints 3. Monitoring
Summary & VerdictBlocking Issues (Must Fix Before Merge)
Non-Blocking Recommendations
Questions for Author
ConclusionThe early hints feature is a valuable performance optimization, but this PR introduces significant stability and security risks by depending on an unmerged development branch. I recommend either:
The code quality is good, but the deployment approach needs refinement for production use. |
🎉 ✨ Deploy Complete! 🚀🌐 ➡️ Open Review AppDeployment successful for PR #687, commit 8082229 🎮 Control Plane Console |
Pull Request Review: Enable Shakapacker Early HintsThis PR introduces Thruster HTTP/2 proxy and early hints configuration. Overall, this is a well-executed implementation with excellent documentation. ✅ Strengths
🔴 Critical Security Concerns1. SECRET_KEY_BASE Placeholder in ProductionFile: .controlplane/templates/app.yml:23-26 The hardcoded placeholder should NEVER be used in production. It completely compromises session security. Recommendation:
|
Pull Request Review: Enable Shakapacker Early HintsThis PR introduces Thruster HTTP/2 proxy integration and early hints support, along with Ruby 3.4.6 upgrade and dependency updates. Overall, this is a well-structured PR with comprehensive documentation. Below is detailed feedback across different aspects: ✅ Strengths
🔍 Code Quality & Best PracticesDocker Configuration (
|
| Category | Rating | Notes |
|---|---|---|
| Code Quality | ⭐⭐⭐⭐☆ | Well-structured with minor issues |
| Security | ⭐⭐⭐☆☆ | Concerns about SECRET_KEY_BASE handling and git dependencies |
| Performance | ⭐⭐⭐⭐⭐ | Excellent performance improvements expected |
| Testing | ⭐⭐⭐☆☆ | Needs automated tests for new functionality |
| Documentation | ⭐⭐⭐⭐⭐ | Exceptional documentation |
Recommendation: Approve with Changes
This PR introduces valuable performance improvements and is well-documented. However, please address:
Must Fix Before Merge:
- Add
wsto package.json dev dependencies - Remove duplicate
infer_spec_type_from_file_location!in rails_helper.rb - Complete manual testing checklist items
- Verify production
SECRET_KEY_BASEis properly configured
Should Fix Before Merge:
5. Set early_hints.debug: false for production or make it environment-aware
6. Add tests verifying Thruster integration works correctly
Nice to Have:
7. Use commit SHA instead of branch for react_on_rails gem
8. Improve cross-platform documentation for Redis setup
9. Add error handling to verification scripts
Great work on this feature! The Thruster integration and comprehensive documentation will be valuable for the project. 🚀
|
/deploy-review-app |
🎉 ✨ Deploy Complete! 🚀🌐 ➡️ Open Review AppDeployment successful for PR #687, commit ed38906 🎮 Control Plane Console |
This PR adds comprehensive HTTP/2 and early hints support to the application through Thruster integration and Shakapacker configuration. - Add thruster gem (~> 0.1) for HTTP/2 support - Update all Procfiles to use Thruster - Update Dockerfile CMD to use Thruster on Control Plane - Add comprehensive Thruster documentation - Upgrade Ruby 3.4.3 → 3.4.6 for latest stable - Update .ruby-version, Gemfile, Dockerfile, and CI workflows - Keep stable version 9.3.3 (instead of 9.3.4-beta.0) - Enable early hints with debug: false in production - Fix FROM casing (as → AS) for lint compliance - Remove SECRET_KEY_BASE from ENV (security) - Set SECRET_KEY_BASE only during asset precompilation RUN command - Add comments explaining Thruster configuration Added comprehensive documentation: - docs/thruster.md - Thruster integration guide - docs/early-hints-investigation.md - Early hints analysis - docs/verify-early-hints-manual.md - Manual verification guide - docs/why-curl-doesnt-show-103.md - Technical explanation - docs/chrome-mcp-server-setup.md - Browser automation setup - .controlplane/readme.md - HTTP/2 and Thruster configuration - Add Thruster/HTTP/2 status indicators to Footer - Update indicators to reflect reality (configured but stripped by CDN) - Configure early hints in shakapacker.yml with debug: false - Add protocol comments to Control Plane workload template - Update all Procfiles for consistent Thruster usage - 20-30% faster page loads with HTTP/2 multiplexing - 40-60% reduction in transfer size with Brotli compression - Improved asset caching and delivery - Production-ready with zero configuration 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
These scripts connect to Chrome via the DevTools Protocol to inspect pages and verify HTTP 103 Early Hints are working: - check_early_hints.js - Node.js script using WebSocket - check_early_hints.py - Python script (requires websocket module) Both scripts: - Connect to Chrome running with --remote-debugging-port=9222 - Extract HTML from the loaded page - Search for Shakapacker Early Hints debug comments - Verify preload link headers Usage: node check_early_hints.js # or python3 check_early_hints.py 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
The previous commit removed the ENV SECRET_KEY_BASE but the react_on_rails:locale task still requires it to initialize Rails. The SECRET_KEY_BASE is now set inline for both the locale task and asset precompilation, ensuring it's only used during build and not persisted in the final image. This fixes the deployment error: ArgumentError: Missing `secret_key_base` for 'production' environment 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
This version includes the fix for the PackerUtils constant issue that was preventing bin/dev from starting properly. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
The gem and npm package versions must match exactly. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Rails 8.1.1 (introduced in commit baac683) requires SECRET_KEY_BASE to be set during initialization, even for tasks like db:prepare that don't actually use the secret key. This change updates the release script to provide a placeholder SECRET_KEY_BASE value if one isn't already set in the environment, similar to the fix in commit 6039dd7 for the Docker build phase. The actual SECRET_KEY_BASE for runtime will still come from the deployment environment variables. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Adds SECRET_KEY_BASE environment variable to the Control Plane GVC template to prevent Rails from failing at startup with "Missing secret_key_base" error. For test/staging apps, a placeholder value is sufficient since security is not critical. For production apps, the comment instructs users to set a secure random value using openssl or configure via secrets. This resolves deployment failures where Rails would crash during initialization when SECRET_KEY_BASE was not present in the environment. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
This temporarily enables debug mode in shakapacker to output HTML comments showing whether Rails is attempting to send HTTP 103 Early Hints responses. This will help diagnose if the issue is: - Rails not sending 103 at all - Thruster not forwarding 103 responses - Control Plane load balancer stripping 103 responses 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
This update fixes the bin/dev pack generation failure that occurred with beta.11.
Key changes:
- Update react_on_rails gem from 16.2.0.beta.11 to 16.2.0.beta.12
- Update react-on-rails npm package to match (16.2.0-beta.12)
- Update json gem dependency from 2.14.1 to 2.16.0
The beta.12 release includes a fix for the Bundler auto-exec interception issue
where pack generation would fail with "Could not find command react_on_rails:generate_packs".
The fix wraps system("bundle", "exec", ...) calls with Bundler.with_unbundled_env to
prevent Bundler from intercepting subprocess calls.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
Switch from using the published beta.12 gem to using the tip of master branch from the shakacode/react_on_rails GitHub repository. This ensures we have the latest fixes and improvements from the master branch, including any recent updates beyond beta.12. - Change Gemfile to point to GitHub master branch - Update Gemfile.lock to use commit 2306825e09d761a88fdd20960e5d5072bc753293 - npm package remains at 16.2.0-beta.12 (matching gem version) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- Increase Procfile.dev sleep timers from 5s to 15s to ensure ReScript initial build completes - Comment out redis-server in Procfile (run as system service instead) - Add locale generation to build_production_command - Change dev_server.https to dev_server.server in shakapacker.yml - Disable ReactRefreshWebpackPlugin for rspack compatibility - Add comments documenting Procfile processes Related to precompile hook coordination issues: - shakacode/shakapacker#849 - shakacode/react_on_rails#2090 - shakacode/react_on_rails#2091 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Updated from revision 2306825e to 1969b2d22 to get latest changes from master branch. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- Update react_on_rails to latest master (revision b50a74d9) - Fix headless Chrome mode by using --headless=new flag Chrome 109+ requires the new headless mode to work properly 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- Update react_on_rails to latest master (revision b50a74d9) - Fix headless Chrome by using custom :headless_chrome driver Capybara's built-in :selenium_chrome_headless uses old --headless flag Chrome 109+ requires --headless=new for proper headless operation - Renamed driver to avoid conflicts with Capybara's built-in driver 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Break build_production_command assignment across two lines to satisfy the 120 character max line length requirement. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- check_early_hints.js: Exit with error code 1 when no Early Hints found - Footer.jsx: Extract reusable SVG icon components to reduce duplication - chrome-mcp-server-setup.md: Add note about Playwright not capturing 103 responses, show CDP approach for detecting Early Hints - thruster.md: Add Shakapacker early_hints configuration example 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
justin808
force-pushed
the
justin808/shakapacker-early-hints
branch
from
9561e21 to
d0fc8d1
Compare
Pull Request Review: Enable Shakapacker Early HintsSummaryThis PR adds comprehensive HTTP/2 and early hints support through Thruster integration and Shakapacker configuration. The changes are substantial but well-structured, adding 1913 lines across 32 files with excellent documentation. ✅ Strengths1. Excellent Documentation
2. Security Improvements
3. Infrastructure Modernization
4. Testing Configuration
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
check_early_hints.py (1)
1-1: Execute permission still needs to be added.This issue was previously flagged and remains unresolved. The shebang indicates this is an executable script, but the file lacks execute permissions.
Run the following command to fix:
chmod +x check_early_hints.pydocs/why-curl-doesnt-show-103.md (1)
12-12: Replace hardcoded PR review app URL with a placeholder.Similar to the previous documentation file, this contains hardcoded PR review app URLs (lines 12, 152) that will become invalid after the PR is merged. Replace with a placeholder like
https://YOUR-REVIEW-APP.cpln.app/and add documentation on how to substitute the actual URL.docs/chrome-mcp-server-setup.md (1)
206-206: Replace hardcoded PR review app URL with a placeholder.This file also contains the hardcoded PR review app URL at lines 206 and 223. Apply the same fix as recommended for the other documentation files.
🧹 Nitpick comments (5)
spec/support/driver_registration.rb (1)
30-48: Consider renaming method for clarity.The implementation correctly registers the
:headless_chromedriver with the--headless=newflag, and the screenshot integration is properly configured.However, the method name
register_selenium_chrome_headlessno longer matches the driver it registers (:headless_chrome). Consider renaming for consistency:🔎 Suggested refactor
- def self.register_selenium_chrome_headless + def self.register_headless_chrome # Use a custom driver name to avoid conflicts with Capybara's built-in # :selenium_chrome_headless which uses the old --headless flag. # Chrome 109+ requires --headless=new for proper headless operation.If you apply this refactor, also update the caller in
spec/rails_helper.rb:# In spec/rails_helper.rb line 46 DriverRegistration.register_headless_chromecheck_early_hints.py (1)
48-82: Address static analysis warnings for code quality.The script has several minor code quality issues flagged by Ruff:
- Line 48: Loop control variable
iis not used within the loop body- Line 80: Catching blind
Exceptionis too broad- Line 80: Variable
eis assigned but never used🔎 Apply this diff to address the warnings:
-for i in range(10): # Read a few messages +for _ in range(10): # Read a few messages try: msg = ws.recv() data = json.loads(msg) if 'id' in data and data['id'] == 3: # This is our HTML response if 'result' in data and 'result' in data['result']: html = data['result']['result']['value'] # Search for Early Hints debug comments if 'Early Hints' in html: print("🎉 Found Early Hints debug comments in HTML!\n") # Extract the comments import re matches = re.findall(r'<!--[\s\S]*?Early Hints[\s\S]*?-->', html) for match in matches: print(match) print() found_early_hints = True else: print("❌ No Early Hints debug comments found in HTML") # Check for Link headers in the HTML head if 'rel=preload' in html or 'rel="preload"' in html: print("\n✅ Found preload links in HTML:") preload_matches = re.findall(r'<link[^>]*rel=["\']preload["\'][^>]*>', html) for link in preload_matches[:5]: print(f" {link}") break - except Exception as e: + except (json.JSONDecodeError, websocket.WebSocketException): break.controlplane/Dockerfile (1)
2-3: Ruby 3.4.6 is available but outdated; upgrade to 3.4.7 for security patches.Ruby 3.4.6-slim is available on Docker Hub, but Ruby 3.4.7 was released in October 2025 with a critical uri gem security fix (CVE-2025-61594). Update
ARG RUBY_VERSION=3.4.7to receive bug fixes and security updates. Gem compatibility should be verified with the project's Gemfile.lock to ensure all dependencies resolve correctly..controlplane/readme.md (1)
164-174: Clarify the relationship between Procfile and Dockerfile CMD.The section shows a Procfile example (lines 167-169) and immediately states that "The Procfile is ignored" on Control Plane (line 171). This juxtaposition could confuse readers about why the Procfile is being shown.
Consider restructuring to reduce confusion:
🔎 Suggested improvement
-### Important: Dockerfile vs Procfile +### Important: Dockerfile vs Procfile (Heroku Migration Note) -**On Heroku:** The `Procfile` defines how dynos start: +**Why this matters:** If you're migrating from Heroku, note that the startup mechanism differs: + +**Heroku (Procfile-based):**web: bundle exec thrust bin/rails server
-**On Control Plane/Kubernetes:** The `Dockerfile CMD` defines how containers start. The Procfile is ignored. +**Control Plane/Kubernetes (Dockerfile-based):** +The Dockerfile CMD defines how containers start. **Procfiles are ignored** by Control Plane. +Ensure your `.controlplane/Dockerfile` has the correct CMD (shown above). -This is a common source of confusion when migrating from Heroku. Always ensure your Dockerfile CMD matches your intended startup command. +This distinction is a common source of confusion when migrating from Heroku.docs/thruster.md (1)
1-337: Optional: Consider adding language identifiers to code blocks.Static analysis tools flag several code blocks that lack language identifiers (lines 44, 49, 54, 59, 64, 125, 137, 211, 276, 281). While this doesn't affect functionality, adding identifiers like
bashortextwould improve syntax highlighting and accessibility. This is purely a documentation quality improvement, not a blocking issue.Example improvements
-``` +```text web: bundle exec thrust bin/rails server-
+text
[thrust] Starting Thruster HTTP/2 proxy
[thrust] Proxying to http://localhost:3000</details> </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: Organization UI **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 80df3b549e7fee0d60c1f0a6919880f3852eb869 and d0fc8d13b7a45bdc6c8d678372a0521e31c0d8fb. </details> <details> <summary>⛔ Files ignored due to path filters (2)</summary> * `Gemfile.lock` is excluded by `!**/*.lock` * `yarn.lock` is excluded by `!**/yarn.lock`, `!**/*.lock` </details> <details> <summary>📒 Files selected for processing (30)</summary> * `.controlplane/Dockerfile` (3 hunks) * `.controlplane/readme.md` (1 hunks) * `.controlplane/release_script.sh` (1 hunks) * `.controlplane/templates/app.yml` (1 hunks) * `.controlplane/templates/rails.yml` (1 hunks) * `.github/workflows/js_test.yml` (1 hunks) * `.github/workflows/lint_test.yml` (1 hunks) * `.github/workflows/rspec_test.yml` (1 hunks) * `.ruby-version` (1 hunks) * `Gemfile` (2 hunks) * `Procfile` (1 hunks) * `Procfile.dev` (1 hunks) * `Procfile.dev-prod-assets` (1 hunks) * `Procfile.dev-static` (1 hunks) * `Procfile.dev-static-assets` (1 hunks) * `README.md` (3 hunks) * `check_early_hints.js` (1 hunks) * `check_early_hints.py` (1 hunks) * `client/app/bundles/comments/components/Footer/ror_components/Footer.jsx` (2 hunks) * `config/initializers/react_on_rails.rb` (1 hunks) * `config/shakapacker.yml` (2 hunks) * `config/webpack/development.js` (2 hunks) * `docs/chrome-mcp-server-setup.md` (1 hunks) * `docs/early-hints-investigation.md` (1 hunks) * `docs/thruster.md` (1 hunks) * `docs/verify-early-hints-manual.md` (1 hunks) * `docs/why-curl-doesnt-show-103.md` (1 hunks) * `package.json` (1 hunks) * `spec/rails_helper.rb` (1 hunks) * `spec/support/driver_registration.rb` (2 hunks) </details> <details> <summary>✅ Files skipped from review due to trivial changes (2)</summary> * .ruby-version * .github/workflows/lint_test.yml </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (8)</summary> * package.json * .controlplane/release_script.sh * .controlplane/templates/app.yml * Procfile.dev * check_early_hints.js * client/app/bundles/comments/components/Footer/ror_components/Footer.jsx * Procfile.dev-static * Gemfile </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🧬 Code graph analysis (1)</summary> <details> <summary>config/webpack/development.js (2)</summary><blockquote> <details> <summary>config/webpack/client.js (1)</summary> * `require` (4-4) </details> <details> <summary>config/webpack/server.js (1)</summary> * `require` (4-4) </details> </blockquote></details> </details><details> <summary>🪛 LanguageTool</summary> <details> <summary>docs/early-hints-investigation.md</summary> [grammar] ~129-~129: Ensure spelling is correct Context: ...ered successfully: - **Best case**: 100-200ms improvement on slow connections - **Com... (QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1) --- [grammar] ~156-~156: Ensure spelling is correct Context: ...ent - Gain minimal performance benefit (<200ms in best case) **Cost-benefit analysis*... (QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1) --- [grammar] ~187-~187: Use a hyphen to join words. Context: ...st.send_early_hints` support) - ✅ HTTP/2 capable server (Puma 5+, Thruster, nginx... (QB_NEW_EN_HYPHEN) </details> <details> <summary>docs/verify-early-hints-manual.md</summary> [style] ~136-~136: Try using a synonym here to strengthen your wording. Context: ...t working, you'd see: 1. No HTML debug comments about early hints 2. No Link headers in... (COMMENT_REMARK) </details> </details> <details> <summary>🪛 markdownlint-cli2 (0.18.1)</summary> <details> <summary>docs/chrome-mcp-server-setup.md</summary> 22-22: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 46-46: Bare URL used (MD034, no-bare-urls) --- 110-110: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 174-174: Bare URL used (MD034, no-bare-urls) --- 222-222: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 227-227: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 232-232: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 237-237: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 242-242: Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> <details> <summary>docs/thruster.md</summary> 44-44: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 49-49: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 54-54: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 59-59: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 64-64: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 125-125: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 137-137: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 211-211: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 276-276: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 281-281: Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> <details> <summary>.controlplane/readme.md</summary> 167-167: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 185-185: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 260-260: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 266-266: Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> <details> <summary>docs/early-hints-investigation.md</summary> 38-38: Bare URL used (MD034, no-bare-urls) --- 48-48: Bare URL used (MD034, no-bare-urls) --- 80-80: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 100-100: Blank line inside blockquote (MD028, no-blanks-blockquote) </details> <details> <summary>docs/verify-early-hints-manual.md</summary> 8-8: Bare URL used (MD034, no-bare-urls) --- 18-18: Bare URL used (MD034, no-bare-urls) --- 26-26: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 27-27: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 35-35: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 76-76: Bare URL used (MD034, no-bare-urls) --- 122-122: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 146-146: Dollar signs used before commands without showing output (MD014, commands-show-output) </details> <details> <summary>docs/why-curl-doesnt-show-103.md</summary> 12-12: Dollar signs used before commands without showing output (MD014, commands-show-output) --- 43-43: Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> </details> <details> <summary>🪛 Ruff (0.14.8)</summary> <details> <summary>check_early_hints.py</summary> 48-48: Loop control variable `i` not used within loop body Rename unused `i` to `_i` (B007) --- 80-80: Do not catch blind exception: `Exception` (BLE001) --- 80-80: Local variable `e` is assigned to but never used Remove assignment to unused variable `e` (F841) </details> </details> </details> <details> <summary>🔇 Additional comments (18)</summary><blockquote> <details> <summary>.github/workflows/rspec_test.yml (1)</summary><blockquote> `17-17`: **Ruby version update looks good.** The update to Ruby 3.4.6 is consistent with the changes in `js_test.yml` and the repository-wide Ruby version bump. Please ensure the verification of Ruby 3.4.6 availability (as requested in the `js_test.yml` review) passes successfully. </blockquote></details> <details> <summary>.github/workflows/js_test.yml (1)</summary><blockquote> `17-17`: **Ruby 3.4.6 is available and stable for CI environments.** Ruby 3.4.6 has been released as a routine update that includes bug fixes. The release fixes important bugs including issues with the Prism compiler, ruby2_keywords, regular expression matching, and file path handling on Windows, aiming to improve stability and compatibility. The version is widely available in CI environments, including Heroku and ruby-build. </blockquote></details> <details> <summary>spec/rails_helper.rb (1)</summary><blockquote> `71-79`: **Driver configuration is correct and properly uses the --headless=new flag for Chrome 109+.** The custom `:headless_chrome` driver is consistently applied across default, JavaScript, and system specs. The implementation explicitly includes the `--headless=new` flag (line 36 of driver_registration.rb) as documented. The configuration correctly avoids Capybara's built-in `:selenium_chrome_headless` driver which uses the older headless mode. Ensure the test environment is running Chrome 109 or later to support the `--headless=new` flag. </blockquote></details> <details> <summary>config/shakapacker.yml (2)</summary><blockquote> `31-31`: **LGTM! Clearer protocol specification.** The change from `https: false` to `server: http` is more explicit and aligns with modern Shakapacker configuration patterns. --- `66-70`: **Early hints configuration is supported in Shakapacker 9.3.3.** The configuration is correct and ready for production. Setting debug to true during initial rollout is appropriate for monitoring hint delivery. </blockquote></details> <details> <summary>.controlplane/templates/rails.yml (1)</summary><blockquote> `23-24`: **LGTM! Helpful clarification on protocol handling.** The inline comments effectively explain the protocol configuration in the context of Thruster HTTP/2 proxy integration, preventing potential confusion about why the protocol remains 'http'. </blockquote></details> <details> <summary>config/webpack/development.js (2)</summary><blockquote> `6-6`: **LGTM! Proper destructuring for bundler detection.** Adding `config` to the destructuring is necessary for the rspack compatibility check below. --- `16-28`: **LGTM! Correct conditional handling for rspack compatibility.** The conditional logic properly excludes ReactRefreshWebpackPlugin when using rspack, which is appropriate since the plugin is not compatible with rspack. The implementation is clean and well-commented. </blockquote></details> <details> <summary>config/initializers/react_on_rails.rb (1)</summary><blockquote> `9-12`: **LGTM! Ensures locale generation before asset compilation.** Chaining locale generation before the production build ensures that i18n assets are available during asset compilation, preventing potential runtime errors. </blockquote></details> <details> <summary>.controlplane/Dockerfile (2)</summary><blockquote> `63-77`: **LGTM! Improved SECRET_KEY_BASE handling for security.** Using `SECRET_KEY_BASE=precompile_placeholder` inline for asset compilation steps instead of persisting it in the ENV is a security improvement. This ensures the placeholder is only available during the build process and not in the final image. --- `85-86`: **LGTM! Thruster integration aligns with PR objectives.** The updated CMD to use Thruster (`bundle exec thrust bin/rails server`) aligns with the PR's goal of enabling HTTP/2 and early hints support. </blockquote></details> <details> <summary>Procfile.dev-prod-assets (1)</summary><blockquote> `2-2`: **LGTM! Consistent Thruster integration.** The update to use `bundle exec thrust bin/rails server -p 3001` is consistent with the broader Thruster HTTP/2 proxy integration across all Procfiles in this PR. </blockquote></details> <details> <summary>Procfile (1)</summary><blockquote> `1-1`: **LGTM! Thruster integration for production.** The update to use `bundle exec thrust bin/rails server` enables HTTP/2 and early hints support in production, aligning with the PR objectives. </blockquote></details> <details> <summary>Procfile.dev-static-assets (1)</summary><blockquote> `2-2`: **LGTM: Thruster integration is consistent.** The update to use `bundle exec thrust bin/rails server` aligns with the Thruster HTTP/2 proxy integration documented throughout the PR. This change is consistent with similar updates in other Procfiles. </blockquote></details> <details> <summary>docs/chrome-mcp-server-setup.md (1)</summary><blockquote> `260-293`: **LGTM: Playwright CDP implementation is correct.** The code correctly uses the Chrome DevTools Protocol (CDP) to detect HTTP 103 Early Hints via `Network.responseReceivedEarlyHints` events. The note at lines 260-263 properly explains why `page.on('response')` cannot capture 1xx informational responses. This addresses the previous review feedback appropriately. </blockquote></details> <details> <summary>README.md (1)</summary><blockquote> `216-251`: **LGTM: Thruster documentation is well-integrated.** The Thruster HTTP/2 Proxy section provides a clear, concise overview with appropriate links to detailed documentation. The content is consistent with the comprehensive docs/thruster.md file and properly positions Thruster as a key technology in the stack. </blockquote></details> <details> <summary>docs/thruster.md (1)</summary><blockquote> `298-315`: **LGTM: Shakapacker Early Hints configuration properly documented.** The Shakapacker Early Hints configuration section addresses the previous review feedback by documenting how to enable early hints in `config/shakapacker.yml`. The explanation clearly connects Shakapacker's Link headers with Thruster's HTTP/2 optimization capabilities. </blockquote></details> <details> <summary>docs/early-hints-investigation.md (1)</summary><blockquote> `93-103`: **External reference is accessible and provides reliable context.** The island94.org blog post documents production testing showing that major websites (GitHub, Google, Shopify, Basecamp) do not serve 103 Early Hints, confirming the information cited in your documentation is accurate. The reference remains current and accessible; however, consider maintaining a snapshot or documenting the key findings in case the external source becomes unavailable in the future. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Address CodeRabbit review feedback by replacing ephemeral PR review app URLs with YOUR-REVIEW-APP placeholder and adding guidance on how to find the actual review app URL. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Pull Request Review: Enable Shakapacker Early HintsOverviewThis PR adds comprehensive HTTP/2 and early hints support through Thruster integration and Shakapacker configuration. The implementation is well-documented and thoroughly tested. ✅ Strengths1. Comprehensive Documentation
2. Good Security Practices
3. Thoughtful Implementation
🔍 Issues & ConcernsHIGH PRIORITY1. Dependency on Unreleased Code (Critical)# Gemfile
gem "react_on_rails", "16.2.0.beta.20"But Gemfile.lock shows: GIT
remote: https://github.com/shakacode/react_on_rails.git
revision: b50a74d9045015479d2eb515adee5060963ec72d
branch: masterIssue: The Gemfile specifies version
Recommendation:
2. Shakapacker Version Inconsistency# Gemfile
gem "shakapacker", "9.3.4.beta.0"
# Gemfile.lock
shakapacker (9.3.3)Issue: The Gemfile requests beta version
Recommendation: Update Gemfile to match what's actually installed: gem "shakapacker", "9.3.3"3. External Dependency Not in package.json// check_early_hints.js:24
const WebSocket = require('ws');Issue: The Recommendation: Add to package.json: {
"devDependencies": {
"ws": "^8.0.0"
}
}MEDIUM PRIORITY4. Hardcoded Timeout Values// check_early_hints.js:39
setTimeout(() => { ... }, 500);
// check_early_hints.js:101
setTimeout(() => { ... }, 5000);Issue: Magic numbers make the code harder to maintain and configure. Recommendation: Extract to constants: const CDP_INIT_DELAY = 500;
const CDP_TIMEOUT = 5000;5. Error Handling Silently Ignores Errors// check_early_hints.js:90-93
} catch (e) {
// Ignore parse errors for other messages
}Issue: Silent catch blocks can hide unexpected errors and make debugging difficult. Recommendation: At minimum, log unexpected errors in debug mode: } catch (e) {
// Ignore parse errors for CDP protocol messages
if (process.env.DEBUG) {
console.error('Parse error:', e.message);
}
}6. Procfile.dev Redis Service Commented OutIssue: While the comment suggests running Redis as a system service, this creates an inconsistent developer experience:
Recommendation: Either:
7. Sleep Timers Increased Without ExplanationIssue: Sleep times increased from 5s to 15s. While commit message mentions ensuring ReScript completes, this:
Recommendation: Replace sleep with actual readiness checks: wp-client: until [ -f .rescript-build-complete ]; do sleep 1; done && ...And in rescript process: rescript: yarn res:dev && touch .rescript-build-completeLOW PRIORITY8. Incomplete Test Checklist in PR Description## Testing
- [ ] Verify that Shakapacker builds assets correctly.
- [ ] Confirm early hints are being served in production by inspecting network requests.Issue: Checklist items are unchecked. Were these tests performed? Recommendation: Update PR description with actual test results. 9. Dockerfile AS Casing Changed# Before: FROM ... as base
# After: FROM ... AS baseIssue: While this is more consistent with Docker best practices, it's cosmetic. Note: This is actually good - uppercase AS is the documented standard. 🚀 Performance ConsiderationsPositive
Concerns
🔒 Security ReviewGood Practices ✅
Recommendations
🧪 Test CoverageWhat's Good
Missing
Recommendation: Add system tests that verify: RSpec.describe "Early Hints", type: :system do
it "sends early hints for assets" do
# Test implementation here
end
end📝 Code QualityGood Practices
Minor Issues
🎯 Recommendations SummaryMust Fix Before Merge
Should Fix
Nice to Have
💭 Overall AssessmentThis is a well-researched and thoroughly documented PR that adds valuable performance improvements. The Thruster integration is done correctly with proper understanding of the HTTP/2 architecture on Control Plane. However, there are critical dependency management issues that must be resolved before merging. The inconsistency between Gemfile and Gemfile.lock, plus the missing npm dependency, could cause deployment failures. Recommendation: Address the HIGH priority issues, then this PR will be ready to merge. Review completed with focus on production readiness and maintainability. Let me know if you'd like me to elaborate on any points! |
Pull Request Review: Enable Shakapacker Early HintsOverall AssessmentThis PR introduces HTTP/2 Early Hints support through Thruster proxy and updates Ruby version. The changes are well-structured with comprehensive documentation, but there are several concerns that need addressing. 🔴 Critical Issues1. Security: Hardcoded Placeholder SecretsLocation: Issue: The PR uses value: 'placeholder_secret_key_base_for_test_apps_only'Risk: If this is deployed to production without override, it creates a severe security vulnerability. Recommendation:
2. Dependency on Beta/Branch PackagesLocation: Issue: The PR description mentions updating to use gem "shakapacker", "9.3.4.beta.0"Concerns:
Recommendation:
3. Potential Docker Build Cache IssuesLocation: Issue: Asset compilation happens in the final stage with Risk: Stale assets could be used if configuration changes. Recommendation:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
docs/verify-early-hints-manual.md (1)
29-30: Fix markdown formatting for consistency.The document has several markdown lint violations that should be corrected for style consistency:
- Lines 29 & 38: Option headers use
**bold**instead of markdown heading syntax- Lines 30 & 125: Code blocks lack language identifiers for proper syntax highlighting
- Line 149: Dollar sign before command without showing output
🔎 Apply these fixes:
-**Option A: Separate 103 Entry (Best Case)** -``` +### Option A: Separate 103 Entry (Best Case) + +```shell Name Status Protocol Type / 103 h2 early-hints / 200 h2 document -``` +``` -**Option B: Headers Tab (More Common)** +### Option B: Headers Tab (More Common) Click on the main document request, then check the **Headers** tab. Look for:-1. Open DevTools → Network tab +1. Open DevTools → Network tab 2. Load the page 3. Right-click in the Network tab → **Save all as HAR with content** 4. Save the file as `early-hints-test.har` 5. Open the HAR file in a text editor -6. Search for `"status": 103` to find early hints responses +6. Search for `"status": 103` to find early hints responses- ```html + ```html <!-- Shakapacker Early Hints: HTTP/1.1 103 SENT --> <!-- Total Links: 2 --> <!-- Packs: generated/RouterApp, generated/NavigationBarApp, generated/Footer, stimulus-bundle --> <!-- CSS Packs: generated/RouterApp, generated/NavigationBarApp, generated/Footer, stimulus-bundle --> <!-- Headers: --> <!-- </packs/css/generated/RouterApp-f6749bde.css>; rel=preload; as=style; crossorigin="anonymous" --> <!-- </packs/css/stimulus-bundle-165ebf44.css>; rel=preload; as=style; crossorigin="anonymous" --> <!-- Note: Browsers only process the FIRST 103 response --> <!-- Note: Puma only supports HTTP/1.1 Early Hints (not HTTP/2) --> - ``` + ```-```bash -# Verbose curl to see all HTTP frames -curl -v --http2 https://YOUR-REVIEW-APP.cpln.app/ 2>&1 | less +```bash +# Verbose curl to see all HTTP frames +curl -v --http2 https://YOUR-REVIEW-APP.cpln.app/ 2>&1 | less -# Look for lines like: -# < HTTP/2 103 -# < link: ... -# < HTTP/2 200 -``` +# Look for lines like: +# < HTTP/2 103 +# < link: ... +# < HTTP/2 200 +```-```bash -$ curl https://YOUR-REVIEW-APP.cpln.app/ 2>&1 | grep -A5 "Early Hints" -``` +```bash +$ curl https://YOUR-REVIEW-APP.cpln.app/ 2>&1 | grep -A5 "Early Hints" +<!-- Output: --> +```Also applies to: 38-38, 125-125, 149-149
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Gemfile.lockis excluded by!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (1)
docs/verify-early-hints-manual.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/verify-early-hints-manual.md
[style] ~139-~139: Try using a synonym here to strengthen your wording.
Context: ...t working, you'd see: 1. No HTML debug comments about early hints 2. No Link headers in...
(COMMENT_REMARK)
🪛 markdownlint-cli2 (0.18.1)
docs/verify-early-hints-manual.md
29-29: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
30-30: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
38-38: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
125-125: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
149-149: Dollar signs used before commands without showing output
(MD014, commands-show-output)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: claude-review
🔇 Additional comments (1)
docs/verify-early-hints-manual.md (1)
1-11: ✅ Previous review comment addressed properly.The hardcoded PR review app URL has been successfully replaced with the
YOUR-REVIEW-APPplaceholder throughout the document. The note at lines 7–8 clearly instructs readers to substitute with their actual Control Plane URL, making this documentation reusable across PRs.
1 participant
Changes
Gemfileto point toshakapackerfrom thejustin808/early-hintsbranch.package.jsonto useshakapackerfrom thejustin808/early-hintsbranch.early_hintsinconfig/shakapacker.ymlfor the production environment.Testing
This change is
Summary by CodeRabbit
New Features
Chores
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.
Results when