Enable Shakapacker early hints #687
Conversation
WalkthroughAdds Thruster HTTP/2 proxy integration and documentation, bumps Ruby and Shakapacker/react_on_rails, routes Rails startup through thrust in Procfiles/Dockerfile/bin scripts, adds Early Hints verification scripts, adjusts webpack/react_on_rails and test headless Chrome drivers, and introduces precompile steps in dev startup and a Thruster footer. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser
participant Thruster
participant Rails
participant CDN
Note over Browser,Thruster: TLS/HTTP2 client connection
Browser->>Thruster: GET / (HTTP/2)
Thruster->>Rails: Proxy GET / (HTTP/1.1)
Rails-->>Thruster: 103 Early Hints (Link headers)
Thruster-->>Browser: 103 Early Hints (HTTP/2)
Rails-->>Thruster: 200 OK + body
Thruster-->>Browser: 200 OK + body
Note over CDN,Thruster: CDN may strip 1xx responses before Browser (see docs)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
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 <[email protected]>
- 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 <[email protected]>
- 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 <[email protected]>
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 <[email protected]>
- 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 <[email protected]>
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 <[email protected]>
Update both gem and npm package to shakapacker 9.4.0 to fix CI failures caused by version mismatch between gem (9.3.4.beta.0) and npm (9.3.3). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Fix eslint react/prop-types validation errors for CheckmarkIcon, CheckmarkCircleIcon, and InfoIcon components. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Load the Rails environment at script startup rather than lazily inside the locale generation step. This ensures ReactOnRails configuration is available before any precompile tasks run. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
- bin/dev now runs rescript build and locale generation BEFORE starting Procfile processes, eliminating race conditions and sleep delays - Added res:watch npm script (watch without clean) for Procfile.dev - Removed sleep delays and inline locale generation from Procfile.dev - Updated build_production_command to include rescript build - Removed precompile_hook from shakapacker.yml (handled by bin/dev and build commands) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Add Chrome debug scripts to .gitignore - Improve bin/dev ReScript build error output with detailed messages - Disable early_hints debug mode in production (exposes internals) - Remove duplicate infer_spec_type_from_file_location! call in rails_helper 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Move Rails environment loading inside run_precompile_tasks to improve startup time for help/kill commands (saves 1-3 seconds) - Remove unused HMR=true from Procfile.dev wp-server process (webpack config only checks WEBPACK_DEV_SERVER, not HMR env var) Co-Authored-By: Claude Opus 4.5 <[email protected]>
justin808
force-pushed
the
justin808/shakapacker-early-hints
branch
from
4df5be0 to
154aa64
Compare
Pull Request Review: Enable Shakapacker Early HintsSummaryThis PR successfully integrates Thruster HTTP/2 proxy and enables Early Hints for improved performance. Changes include dependency updates (Ruby 3.4.6, shakapacker 9.5.0, react_on_rails 16.2.0.rc1), comprehensive Thruster integration, improved development workflows, and extensive documentation. Overall AssessmentLGTM with minor recommendations ✅ Code Quality & Best Practices✅ Strengths
|
|
/deploy-review-app |
🎉 ✨ Deploy Complete! 🚀🌐 ➡️ Open Review AppDeployment successful for PR #687, commit 154aa64 🎮 Control Plane Console |
|
/deploy-review-app |
🎉 ✨ Deploy Complete! 🚀🌐 ➡️ Open Review AppDeployment successful for PR #687, commit 154aa64 🎮 Control Plane Console |
| .context/ | ||
|
|
||
| # Chrome DevTools debug scripts (development-only) | ||
| check_early_hints.js |
There was a problem hiding this comment.
These two lines should not be ignored if we are going to be committing these to the project.
Gemfile
Outdated
|
|
||
| gem "react_on_rails", "16.2.0.beta.20" | ||
| gem "shakapacker", "9.3.4.beta.0" | ||
| gem "react_on_rails", "16.2.0.rc1" |
There was a problem hiding this comment.
Update to final version 16.2.1.
Upgrade from the release candidate (16.2.0.rc1) to the stable release (16.2.1) for both the Ruby gem and npm package. Co-Authored-By: Claude Opus 4.5 <[email protected]>
🎉 ✨ Deploy Complete! 🚀🌐 ➡️ Open Review AppDeployment successful for PR #687, commit 714d825 🎮 Control Plane Console |
PR Review: HTTP/2 Early Hints Support via ThrusterOverall Assessment: ✅ Approved with Minor RecommendationsThis is a well-executed PR that adds HTTP/2 Early Hints support through Thruster proxy integration. The implementation is sound, documentation is excellent, and the security considerations are properly handled. Grade: B+ (Low Risk) ✅ Strengths1. Excellent Documentation
2. Proper Security Handling
3. Clean Integration
🔍 Issues FoundPriority 1: Docker Best PracticesMissing HEALTHCHECK in Dockerfile Add a health check for proper container orchestration: # Add after EXPOSE instruction
HEALTHCHECK --interval=30s --timeout=5s --start-period=5s --retries=3 \
CMD curl -f http://localhost:3000/up || exit 1Missing EXPOSE instruction While port 3000 is implicit, add for clarity: # Add before CMD
EXPOSE 3000Location: Priority 2: Testing CoverageNo integration tests for Thruster Consider adding specs to verify:
Suggested test file: Priority 3: Minor Improvements1. Gemfile documentation gem "thruster", "~> 0.1" # HTTP/2 proxy for early hints and asset delivery2. Shakapacker config comments production:
early_hints:
enabled: true # Generates Link headers for Thruster HTTP/2 early hints
debug: false # MUST be false in production (exposes asset structure if true)3. Debug mode protection 🔒 Security Analysis✅ No Critical Issues Found
📊 Performance ConsiderationsExpected improvements are realistic and well-documented:
The PR properly documents that benefits depend on asset bundle size, network conditions, and browser support. 🚀 Deployment ChecklistBefore deploying to production:
🎯 Recommendations SummaryBefore Merge
Before Production Deploy
Future Improvements
📝 Code Quality NotesExcellent
Good
✅ Final VerdictApproved for merge with the recommendation to add the Docker best practices (HEALTHCHECK and EXPOSE) before deploying to production. The identified issues are quality improvements rather than blockers. The SECRET_KEY_BASE handling has been carefully reviewed and is correct and secure. The placeholder approach during builds is standard Rails practice. Great work on the comprehensive documentation and clean integration! 🎉 |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@check_early_hints.js`:
- Around line 1-46: The script violates lint rules by using inline
require('ws'), postfix increments (msgId++), and unparenthesized single-arg
arrow params; move const WebSocket = require('ws') to the top with other
requires, replace msgId++ with an explicit increment pattern (e.g., msgId += 1
or increment before use and reference msgId), and wrap single-argument arrow
params with parentheses (e.g., (res) =>, (chunk) =>) especially in callbacks
like ws.on('open') and res.on('data'); apply the same fixes to the later
occurrences around the 65-83 region (other ws.send/handlers and any additional
inline requires or ++ uses).
- Around line 14-17: When no Chrome tabs are found (the check on tabs.length ===
0), the script currently logs and returns which yields a zero exit code; change
this to emit a non-zero failure exit so CI can detect the error—replace the
plain return after console.log('No Chrome tabs found') with a failing exit
(e.g., call process.exit(1) or throw an Error) in the same block that references
tabs to ensure the process exits with failure when no tabs are available.
In `@check_early_hints.py`:
- Around line 1-5: Add a non-zero exit path when Early Hints verification fails:
import sys at the top (add to the existing imports) and, in the
verification/result-handling block (the logic around check_early_hints /
verify_early_hints / main where the script decides success — referenced in the
diff around lines 85-89), detect the failure case and call sys.exit(1)
(optionally after printing/logging an error message) so the process exits with a
non-zero status on failure.
In `@docs/thruster.md`:
- Around line 43-66: Several fenced code blocks in the docs/thruster.md examples
lack language identifiers (MD040); update each fenced block under the headings
like "Production (`Procfile`)", "Development with HMR (`Procfile.dev`)",
"Development with Production Assets (`Procfile.dev-prod-assets`)", "Development
with Static Webpack (`Procfile.dev-static`)" and "Development with Static Assets
(`Procfile.dev-static-assets`)" (and the other ranges noted: 104-113, 125-129,
137-139, 211-217, 276-283) by adding appropriate language tags (e.g., text or
bash for Procfile commands, yaml/dockerfile if any container or Procfile-like
content) immediately after the opening triple backticks so every fenced block
has a language identifier.
🧹 Nitpick comments (8)
config/initializers/react_on_rails.rb (1)
9-13: Consider aligning env vars forres:buildwith the shakapacker run.Lines 12–13 run
yarn res:buildwithout explicit env; if that script is env-sensitive, matchingRAILS_ENV/NODE_ENVprevents mixed-mode outputs.♻️ Suggested adjustment
- config.build_test_command = "yarn res:build && RAILS_ENV=test bin/shakapacker" - config.build_production_command = "yarn res:build && RAILS_ENV=production NODE_ENV=production bin/shakapacker" + config.build_test_command = "RAILS_ENV=test NODE_ENV=test yarn res:build && RAILS_ENV=test NODE_ENV=test bin/shakapacker" + config.build_production_command = "RAILS_ENV=production NODE_ENV=production yarn res:build && RAILS_ENV=production NODE_ENV=production bin/shakapacker".controlplane/readme.md (3)
167-169: Add language specifier to fenced code block.This Procfile example lacks a language identifier. Adding one improves syntax highlighting and satisfies markdown linting.
🔧 Suggested fix
-``` +```text web: bundle exec thrust bin/rails server</details> --- `185-189`: **Add language specifier for log output code block.** This code block showing Thruster startup messages should have a language identifier for consistency. <details> <summary>🔧 Suggested fix</summary> ```diff -``` +```text [thrust] Starting Thruster HTTP/2 proxy [thrust] Proxying to http://localhost:3000 [thrust] Serving from ./public</details> --- `260-269`: **Add language specifier to architecture diagrams.** These ASCII architecture diagrams lack language identifiers. Use `text` for plain text diagrams. <details> <summary>🔧 Suggested fix</summary> ```diff -``` +```text User → HTTPS/HTTP2 → Thruster → HTTP/1.1 → Rails (Thruster handles TLS + HTTP/2)Control Plane + Thruster:
-+text
User → HTTPS/HTTP2 → Control Plane LB → HTTP/1.1 → Thruster → HTTP/1.1 → Rails
(LB handles TLS) (protocol: http) (HTTP/2 features)docs/early-hints-investigation.md (3)
38-48: Wrap bare URLs in markdown link syntax.Bare URLs in headings should be wrapped in proper markdown link syntax for better rendering and linting compliance.
🔧 Suggested fix
-### Production Test (https://reactrails.com/) +### Production Test ([https://reactrails.com/](https://reactrails.com/)) ... -### Staging Test (https://staging.reactrails.com/) +### Staging Test ([https://staging.reactrails.com/](https://staging.reactrails.com/))
80-83: Add language specifier to architecture diagram.🔧 Suggested fix
-``` +```text User → HTTPS/HTTP2 → [Cloudflare CDN] → Control Plane LB → Thruster → Rails [STRIPS 103] (receives 103) (sends 103)</details> --- `93-103`: **Fix blockquote formatting for consecutive quotes.** Consecutive blockquotes with a blank line between them trigger MD028. Either merge them or separate with non-blockquote content. <details> <summary>🔧 Suggested fix</summary> ```diff > "103 Early Hints fail to reach end-users across multiple production environments: > - Heroku with custom domains > - Heroku behind Cloudfront > - DigitalOcean behind Cloudflare ✅ **← YOUR SETUP** > - AWS ALB (reportedly breaks functionality)" - +> > "Despite testing major websites (GitHub, Google, Shopify, Basecamp), none currently serve 103 Early Hints in production, suggesting minimal real-world adoption."check_early_hints.py (1)
48-81: Avoid broad exception swallowing and unused loop var. Ruff flags B007/BLE001. Using_and logging the exception makes failures visible.🔧 Example fix
-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) @@ - except Exception as e: - break + except Exception as exc: + print(f"⚠️ Error reading CDP message: {exc}") + break
| const http = require('http'); | ||
|
|
||
| // Fetch Chrome tabs | ||
| const req = http.get('http://localhost:9222/json', (res) => { | ||
| let data = ''; | ||
|
|
||
| res.on('data', (chunk) => { | ||
| data += chunk; | ||
| }); | ||
|
|
||
| res.on('end', () => { | ||
| const tabs = JSON.parse(data); | ||
|
|
||
| if (tabs.length === 0) { | ||
| console.log('No Chrome tabs found'); | ||
| return; | ||
| } | ||
|
|
||
| const tab = tabs[0]; | ||
| console.log(`📱 Tab: ${tab.title}`); | ||
| console.log(`🔗 URL: ${tab.url}\n`); | ||
|
|
||
| // Connect to WebSocket | ||
| const WebSocket = require('ws'); | ||
| const ws = new WebSocket(tab.webSocketDebuggerUrl); | ||
|
|
||
| let msgId = 1; | ||
|
|
||
| ws.on('open', () => { | ||
| console.log('✅ Connected to Chrome DevTools Protocol\n'); | ||
|
|
||
| // Enable Runtime | ||
| ws.send(JSON.stringify({ | ||
| id: msgId++, | ||
| method: 'Runtime.enable' | ||
| })); | ||
|
|
||
| // Get HTML content | ||
| setTimeout(() => { | ||
| ws.send(JSON.stringify({ | ||
| id: msgId++, | ||
| method: 'Runtime.evaluate', | ||
| params: { | ||
| expression: 'document.documentElement.outerHTML' | ||
| } | ||
| })); |
There was a problem hiding this comment.
Align this script with ESLint/Prettier rules. The inline require, msgId++, and arrow params trigger current lint rules. Moving the require to the top, avoiding ++, and wrapping single-arg arrows will clear the lint errors.
🔧 Example fix
-const http = require('http');
+const http = require('http');
+const WebSocket = require('ws');
@@
- const WebSocket = require('ws');
const ws = new WebSocket(tab.webSocketDebuggerUrl);
@@
- ws.send(JSON.stringify({
- id: msgId++,
- method: 'Runtime.enable'
- }));
+ ws.send(JSON.stringify({
+ id: msgId,
+ method: 'Runtime.enable',
+ }));
+ msgId += 1;
@@
- ws.send(JSON.stringify({
- id: msgId++,
- method: 'Runtime.evaluate',
- params: {
- expression: 'document.documentElement.outerHTML'
- }
- }));
+ ws.send(JSON.stringify({
+ id: msgId,
+ method: 'Runtime.evaluate',
+ params: {
+ expression: 'document.documentElement.outerHTML',
+ },
+ }));
+ msgId += 1;
@@
- earlyHintsMatch.forEach(match => {
+ earlyHintsMatch.forEach((match) => {
console.log(match);
console.log();
});
@@
- linkMatches.slice(0, 5).forEach(link => {
+ linkMatches.slice(0, 5).forEach((link) => {
console.log(` ${link}`);
});Also applies to: 65-83
🧰 Tools
🪛 ESLint
[error] 24-24: Unexpected require().
(global-require)
[error] 33-33: Insert ⏎········
(prettier/prettier)
[error] 34-34: Insert ··
(prettier/prettier)
[error] 34-34: Unary operator '++' used.
(no-plusplus)
[error] 35-35: Replace ········method:·'Runtime.enable' with ··········method:·'Runtime.enable',
(prettier/prettier)
[error] 36-36: Replace ······}) with ········}),⏎······
(prettier/prettier)
[error] 40-40: Insert ⏎··········
(prettier/prettier)
[error] 41-41: Replace ·········· with ············
(prettier/prettier)
[error] 41-41: Unary operator '++' used.
(no-plusplus)
[error] 42-42: Insert ··
(prettier/prettier)
[error] 43-43: Replace ·········· with ············
(prettier/prettier)
[error] 44-44: Replace ············expression:·'document.documentElement.outerHTML' with ··············expression:·'document.documentElement.outerHTML',
(prettier/prettier)
[error] 45-45: Replace ··········} with ············},
(prettier/prettier)
[error] 46-46: Replace ········}) with ··········}),⏎········
(prettier/prettier)
🤖 Prompt for AI Agents
In `@check_early_hints.js` around lines 1 - 46, The script violates lint rules by
using inline require('ws'), postfix increments (msgId++), and unparenthesized
single-arg arrow params; move const WebSocket = require('ws') to the top with
other requires, replace msgId++ with an explicit increment pattern (e.g., msgId
+= 1 or increment before use and reference msgId), and wrap single-argument
arrow params with parentheses (e.g., (res) =>, (chunk) =>) especially in
callbacks like ws.on('open') and res.on('data'); apply the same fixes to the
later occurrences around the 65-83 region (other ws.send/handlers and any
additional inline requires or ++ uses).
| if (tabs.length === 0) { | ||
| console.log('No Chrome tabs found'); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Return a failing exit code when no Chrome tabs are available. Right now Line 14 logs and returns, which yields a success exit code and can mask failures in CI.
🐛 Proposed fix
- if (tabs.length === 0) {
- console.log('No Chrome tabs found');
- return;
- }
+ if (tabs.length === 0) {
+ console.log('No Chrome tabs found');
+ process.exit(1);
+ }📝 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.
| if (tabs.length === 0) { | |
| console.log('No Chrome tabs found'); | |
| return; | |
| } | |
| if (tabs.length === 0) { | |
| console.log('No Chrome tabs found'); | |
| process.exit(1); | |
| } |
🤖 Prompt for AI Agents
In `@check_early_hints.js` around lines 14 - 17, When no Chrome tabs are found
(the check on tabs.length === 0), the script currently logs and returns which
yields a zero exit code; change this to emit a non-zero failure exit so CI can
detect the error—replace the plain return after console.log('No Chrome tabs
found') with a failing exit (e.g., call process.exit(1) or throw an Error) in
the same block that references tabs to ensure the process exits with failure
when no tabs are available.
| #!/usr/bin/env python3 | ||
| import json | ||
| import urllib.request | ||
| import websocket | ||
| import ssl |
There was a problem hiding this comment.
Exit non-zero when Early Hints are not verified. Otherwise automation treats failures as success.
🐛 Proposed fix
-import json
+import json
+import sys
@@
-if found_early_hints:
- print("\n✅ SUCCESS: Early Hints are working!")
-else:
- print("\n⚠️ Could not verify Early Hints in the current page state")
- print("The page may need to be reloaded to capture HTTP 103 responses")
+if found_early_hints:
+ print("\n✅ SUCCESS: Early Hints are working!")
+ sys.exit(0)
+else:
+ print("\n⚠️ Could not verify Early Hints in the current page state")
+ print("The page may need to be reloaded to capture HTTP 103 responses")
+ sys.exit(1)Also applies to: 85-89
🤖 Prompt for AI Agents
In `@check_early_hints.py` around lines 1 - 5, Add a non-zero exit path when Early
Hints verification fails: import sys at the top (add to the existing imports)
and, in the verification/result-handling block (the logic around
check_early_hints / verify_early_hints / main where the script decides success —
referenced in the diff around lines 85-89), detect the failure case and call
sys.exit(1) (optionally after printing/logging an error message) so the process
exits with a non-zero status on failure.
| #### Production (`Procfile`) | ||
| ``` | ||
| web: bundle exec thrust bin/rails server | ||
| ``` | ||
|
|
||
| #### Development with HMR (`Procfile.dev`) | ||
| ``` | ||
| rails: bundle exec thrust bin/rails server -p 3000 | ||
| ``` | ||
|
|
||
| #### Development with Production Assets (`Procfile.dev-prod-assets`) | ||
| ``` | ||
| web: bundle exec thrust bin/rails server -p 3001 | ||
| ``` | ||
|
|
||
| #### Development with Static Webpack (`Procfile.dev-static`) | ||
| ``` | ||
| web: bundle exec thrust bin/rails server -p 3000 | ||
| ``` | ||
|
|
||
| #### Development with Static Assets (`Procfile.dev-static-assets`) | ||
| ``` | ||
| web: bundle exec thrust bin/rails server -p 3000 | ||
| ``` |
There was a problem hiding this comment.
Add language identifiers to fenced blocks (MD040). Several fences omit a language tag; markdownlint will flag these. Please add bash, yaml, dockerfile, or text as appropriate.
🔧 Example fix (apply similarly to other fences)
-#### Production (`Procfile`)
-```
+#### Production (`Procfile`)
+```text
web: bundle exec thrust bin/rails server
-```
+```Also applies to: 104-113, 125-129, 137-139, 211-217, 276-283
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
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)
🤖 Prompt for AI Agents
In `@docs/thruster.md` around lines 43 - 66, Several fenced code blocks in the
docs/thruster.md examples lack language identifiers (MD040); update each fenced
block under the headings like "Production (`Procfile`)", "Development with HMR
(`Procfile.dev`)", "Development with Production Assets
(`Procfile.dev-prod-assets`)", "Development with Static Webpack
(`Procfile.dev-static`)" and "Development with Static Assets
(`Procfile.dev-static-assets`)" (and the other ranges noted: 104-113, 125-129,
137-139, 211-217, 276-283) by adding appropriate language tags (e.g., text or
bash for Procfile commands, yaml/dockerfile if any container or Procfile-like
content) immediately after the opening triple backticks so every fenced block
has a language identifier.
Summary
This PR adds HTTP/2 Early Hints support via Thruster proxy and Shakapacker, enabling browsers to preload critical CSS and JS assets before the main response is ready.
Key Changes
Core Features
thrustergem to serve Rails via HTTP/2 with early hints supportearly_hintsconfiguration inconfig/shakapacker.ymlVersion Updates
Development Experience
bin/devscript: Simplified development server management with precompile tasks (rescript + locale generation)bin/dev --helpandbin/dev killcommandsTesting & Verification
check_early_hints.js,check_early_hints.py) for verifying early hintsDocumentation
docs/thruster.md: Comprehensive guide on Thruster setup and configurationdocs/early-hints-investigation.md: Investigation notes on early hints behaviordocs/verify-early-hints-manual.md: Manual verification stepsdocs/why-curl-doesnt-show-103.md: Explanation of curl limitations with HTTP 103docs/chrome-mcp-server-setup.md: Chrome MCP server setup guideDeployment
SECRET_KEY_BASEto release scripts and GVC templatesTesting
Run with production-like assets to see early hints in action:
Verify early hints are being sent:
Expected output shows multiple
HTTP/1.1 103 Early Hintsresponses withLinkheaders for CSS and JS preloading before the finalHTTP/1.1 200 OK.Breaking Changes
None - this is an additive feature that improves performance without changing existing behavior.
Summary by CodeRabbit
Release Notes
New Features
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.