Enable Shakapacker early hints #687
Conversation
|
Warning Rate limit exceeded@justin808 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 15 minutes and 33 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
WalkthroughAdds Thruster HTTP/2 proxy integration: introduces the thruster gem, routes Rails startup through Thruster in Procfiles and Dockerfile, enables Shakapacker early_hints, adds Early Hints verification scripts, updates UI footer, and adds extensive Thruster / Early Hints documentation and Control Plane guidance. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser
participant Thruster as Thruster (HTTP/2)
participant Rails as Rails (Puma/Container)
Note over Thruster,Rails: Thruster is the HTTP/2 frontend/proxy\nRails remains HTTP/1.1 backend
Browser->>Thruster: HTTP/2 request
Thruster->>Rails: proxied HTTP/1.1 request
Rails->>Thruster: 103 Early Hints + Link headers
Thruster-->>Browser: HTTP/2 103 Early Hints
Rails->>Thruster: 200 OK + body
Thruster-->>Browser: HTTP/2 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)
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 |
🎉 ✨ Deploy Complete! 🚀🌐 ➡️ Open Review AppDeployment successful for PR #687, commit c55d3f7 🎮 Control Plane Console |
PR Review - Enable Shakapacker Early HintsThank you for this comprehensive PR! The addition of HTTP/2 support via Thruster is a valuable performance improvement, and the documentation is excellent. Here's my detailed review: ✅ Strengths
|
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>
🎉 ✨ Deploy Complete! 🚀🌐 ➡️ Open Review AppDeployment successful for PR #687, commit d5f99d9 🎮 Control Plane Console |
Pull Request Review: Enable Shakapacker Early HintsSummaryThis PR successfully integrates Thruster HTTP/2 proxy and enables early hints support through Shakapacker. The implementation is comprehensive, well-documented, and production-ready. Overall, this is excellent work with attention to detail and thorough documentation. ✅ Strengths1. Comprehensive Documentation
2. Production-Ready Implementation
3. Security Improvements
4. Iterative Problem Solving
🔍 Areas for Improvement1. Security Consideration: DEBUG Scripts
|
| Category | Rating | Notes |
|---|---|---|
| Documentation | ⭐⭐⭐⭐⭐ | Exceptional - thorough and helpful |
| Code Quality | ⭐⭐⭐⭐☆ | Very good - minor improvements suggested |
| Security | ⭐⭐⭐⭐☆ | Good - SECRET_KEY_BASE handling improved, template needs review |
| Testing | ⭐⭐⭐☆☆ | Adequate - infrastructure changes, but missing test coverage |
| Performance | ⭐⭐⭐⭐⭐ | Excellent - well-researched and implemented |
🎯 Recommendations Summary
Must Address:
⚠️ StrengthenSECRET_KEY_BASEvalidation in release script⚠️ Make Control Plane template placeholder more obviously invalid
Should Address:
- Add
wstopackage.jsondev dependencies or document manual installation - Update PR description to match actual Shakapacker version (9.3.3 stable)
- Add
.gitignoreentry or documentation for Chrome debug scripts
Nice to Have:
- Add tests for early hints configuration
- Document rationale for Ruby version upgrade
- Enhance Dockerfile comments for clarity
✅ Approval Status
This PR demonstrates excellent engineering practices with comprehensive documentation and thoughtful implementation. The suggested improvements are mostly about hardening security defaults and adding test coverage.
Recommended action: Approve with minor suggested improvements
Great work on this implementation! The Thruster integration is well-executed and the documentation will be valuable for the team.
Review completed by Claude Code
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>
🎉 ✨ Deploy Complete! 🚀🌐 ➡️ Open Review AppDeployment successful for PR #687, commit e2117c3 🎮 Control Plane Console |
Pull Request Review: Enable Shakapacker Early HintsOverviewThis PR introduces Thruster HTTP/2 proxy, upgrades Ruby to 3.4.6, updates dependencies, and enables early hints for improved performance. The changes are substantial with 1,884 additions and 112 deletions across 28 files. ✅ Strengths1. Performance Improvements
2. Comprehensive Documentation
3. Ruby Version Update
🔒 Security Concerns🚨 Critical: Hardcoded SECRET_KEY_BASELocation: - name: SECRET_KEY_BASE
value: 'placeholder_secret_key_base_for_test_apps_only'Issues:
Recommendations:
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
docs/chrome-mcp-server-setup.md (1)
259-280: Playwright example cannot detect 103 Early Hints.This code example will not work as intended. Playwright's
page.on('response')event does not capture HTTP 1xx informational responses. The conditionresponse.status() === 103will never be true because browsers handle 103 Early Hints internally without exposing them as Response objects.To actually detect Early Hints, you would need to use the Chrome DevTools Protocol (CDP) directly and subscribe to
Network.responseReceivedEarlyHintsevents via a Playwright CDP session.
🧹 Nitpick comments (5)
config/shakapacker.yml (1)
67-70: Early hints enabled successfully.The configuration correctly enables early hints for production with debug output. The debug flag will add HTML comments showing early hint activity, which is useful for verification. Consider disabling debug once early hints are confirmed working in production to reduce response size.
docs/chrome-mcp-server-setup.md (1)
22-24: Add language specifier to fenced code block.The error message code block should specify a language (e.g.,
textorplaintext) for better rendering and consistency with markdown best practices.-``` +```text Failed to discover browser connector server. Please ensure it's running.</blockquote></details> <details> <summary>check_early_hints.py (3)</summary><blockquote> `48-48`: **Rename unused loop variable.** The loop variable `i` is not used within the loop body. Use `_` to indicate it's intentionally unused. ```diff -for i in range(10): # Read a few messages +for _ in range(10): # Read a few messages
63-63: Move import to top of file.The
remodule should be imported at the top of the file with other imports, not inline within a conditional block.+import re + # Get the WebSocket debugger URL response = urllib.request.urlopen('http://localhost:9222/json') tabs = json.loads(response.read())And remove the inline import:
# 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)
80-81: Improve exception handling.The bare
except Exceptionis too broad and the caught exceptioneis unused. Either handle specific exceptions or log the error before breaking.- except Exception as e: + except (KeyError, json.JSONDecodeError, websocket.WebSocketException): breakOr if you want to log errors:
- except Exception as e: + except Exception as e: + print(f"⚠️ Error processing message: {e}") break
📜 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 (22)
.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)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(1 hunks)config/shakapacker.yml(1 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(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- Procfile.dev-static-assets
- .controlplane/Dockerfile
- Procfile.dev-static
- Procfile
🧰 Additional context used
🪛 ESLint
check_early_hints.js
[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: Insert ··
(prettier/prettier)
[error] 41-41: Unary operator '++' used.
(no-plusplus)
[error] 42-42: Insert ··
(prettier/prettier)
[error] 43-43: Insert ··
(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)
[error] 62-62: Replace match with (match)
(prettier/prettier)
[error] 79-79: Replace link with (link)
(prettier/prettier)
🪛 LanguageTool
docs/early-hints-investigation.md
[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)
docs/verify-early-hints-manual.md
[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)
🪛 markdownlint-cli2 (0.18.1)
docs/early-hints-investigation.md
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)
.controlplane/readme.md
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)
docs/chrome-mcp-server-setup.md
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)
docs/thruster.md
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)
docs/verify-early-hints-manual.md
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)
docs/why-curl-doesnt-show-103.md
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)
🪛 Ruff (0.14.5)
check_early_hints.py
1-1: Shebang is present but file is not executable
(EXE001)
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)
⏰ 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). (3)
- GitHub Check: deploy
- GitHub Check: claude-review
- GitHub Check: test (22.x, 3.4.6)
🔇 Additional comments (13)
Procfile.dev-prod-assets (1)
2-2: LGTM - Thruster integration applied correctly.The Rails server is now routed through Thruster for HTTP/2 support, consistent with the PR objectives and other Procfile updates across the repository.
Gemfile (3)
8-9: Dependency versions aligned with package.json.The gem versions match the corresponding npm package updates. The move to stable shakapacker 9.3.3 is appropriate.
6-6: Ruby 3.4.6 is a confirmed stable release suitable for production.Ruby 3.4.6 is a stable patch release (released September 16, 2025) and is intended for production use. Platform providers (e.g., Heroku) already support it. No changes needed.
18-18: I need to search for more specific information about breaking changes in the thruster gem to complete the verification.Let me search for more specific information about thruster's release history and breaking changes documentation.
Let me verify the Ruby version constraint semantics, as the review comment appears to contain an incorrect explanation.
The review comment contains factual errors about Ruby version constraints and should be reconsidered.
The pessimistic version constraint
~> 0.1increments only the last digit in the version provided, meaning~> 0.1translates to >= 0.1.0 and < 0.2.0—not "0.1.0 to < 1.0.0" as stated in the review.Furthermore, the suggested change to
~> 0.1.0would actually be more restrictive, not less.~> 0.1.0translates to >= 0.1.0 and < 0.2.0, which contradicts the goal of production stability stated in the comment.The current constraint
~> 0.1is already appropriate and allows the latest stable version (0.1.16). No specific breaking changes within the 0.1.x series were found in the thruster gem documentation to justify tightening the constraint further.Likely an incorrect or invalid review comment.
.controlplane/release_script.sh (1)
17-19: LGTM - Correct SECRET_KEY_BASE handling for migrations.The approach of providing a placeholder SECRET_KEY_BASE for Rails 8.1+ initialization during migrations is correct, as the actual secret key is not needed for database migrations and will be provided at runtime. The comments clearly explain this distinction.
Procfile.dev (1)
5-5: LGTM - Consistent Thruster integration.The Rails server is now routed through Thruster, matching the pattern applied across all Procfile variants in this PR.
.controlplane/templates/rails.yml (1)
23-24: Excellent clarifying comments.These comments effectively explain the protocol layering: Thruster handles HTTP/2 termination at the TLS frontend while the internal load balancer communicates with the container via HTTP/1.1. This prevents confusion about why the protocol remains 'http' despite HTTP/2 support.
package.json (2)
95-95: Version 9.3.3 supports early hints feature — approval confirmed.The downgrade from 9.3.4-beta.0 to 9.3.3 is appropriate and safe. Shakapacker 9.3.3 includes HTTP 103 Early Hints support as documented in the project.
80-80: I'll search for the changelog information on react-on-rails to identify any differences between these beta versions.I'm having difficulty finding detailed changelog information specifically comparing react-on-rails version 16.2.0-beta.11 with beta.10 through web searches. The search results show that the latest version published on npm is 16.1.1, not 16.2.0-beta versions, which suggests these beta versions may not be available in the public npm registry yet or are under a different distribution channel.
Let me search for GitHub releases specifically for these beta versions:
Verify beta version release notes with ShakaCode directly.
The web search could not locate public release notes or changelog entries specifically detailing changes between react-on-rails 16.2.0-beta.10 and 16.2.0-beta.11. The latest version published on npm is 16.1.1, suggesting these beta versions may not yet be publicly available or distributed through standard channels. To verify compatibility and identify any breaking changes before updating, consult the ShakaCode React on Rails repository's releases or communicate directly with the ShakaCode team for beta release details.
.controlplane/templates/app.yml (1)
22-26: The original review comment is based on a misunderstanding of Control Plane template architecture.The
.controlplane/templates/app.ymlfile is explicitly documented as a template for test/development environments. The template itself contains clear inline comments stating: "For test apps, a placeholder value is fine...Production apps should configure this manually after app creation via a secret."The Control Plane documentation confirms this distinction: "For using templates for test apps, put ENV values here, stored in git repo. Production apps will have values configured manually after app creation." The deployment workflow requires manual configuration after app creation for production—template values are not used in production.
There is no risk of production deployments using the placeholder SECRET_KEY_BASE because production apps are configured separately and manually. The system already has the safeguard built in: the template is for test apps only, and production requires manual secret management outside the template.
Likely an incorrect or invalid review comment.
check_early_hints.py (3)
8-23: LGTM!The tab connection logic correctly fetches available Chrome tabs, validates their presence, and establishes a WebSocket connection. The relaxed SSL verification is appropriate for local debugging.
25-42: LGTM!The script correctly enables the Network and Page domains, then retrieves the HTML content using
Runtime.evaluate. This is the appropriate approach for inspecting an already-loaded page.
83-89: LGTM!The cleanup and final messaging are appropriate. The warning about needing to reload to capture HTTP 103 responses is particularly helpful, as this script inspects an already-loaded page.
| console.log('❌ No Early Hints debug comments found in HTML'); | ||
| console.log('This might mean:'); | ||
| console.log(' - Early hints are not enabled'); | ||
| console.log(' - The deployment is not running the latest code'); | ||
| console.log(' - The page needs to be reloaded'); | ||
| } | ||
|
|
||
| // Also check for Link headers with preload | ||
| const linkMatches = html.match(/<link[^>]*rel=["']preload["'][^>]*>/g); | ||
| if (linkMatches) { | ||
| console.log(`\n📦 Found ${linkMatches.length} preload links in HTML head:`); | ||
| linkMatches.slice(0, 5).forEach(link => { | ||
| console.log(` ${link}`); | ||
| }); | ||
| } | ||
|
|
||
| ws.close(); | ||
| process.exit(0); | ||
| } |
There was a problem hiding this comment.
Fix exit code when Early Hints are missing.
Right now the script always exits with status 0 even when it fails to find any early-hints markup, so automated verification can never fail. We need to return a non-zero exit code when the debug comments are absent.
- if (earlyHintsMatch) {
- console.log('🎉 Found Early Hints debug comments in HTML!\n');
- earlyHintsMatch.forEach(match => {
- console.log(match);
- console.log();
- });
- console.log('\n✅ SUCCESS: Early Hints are configured and working!');
- } else {
- console.log('❌ No Early Hints debug comments found in HTML');
- console.log('This might mean:');
- console.log(' - Early hints are not enabled');
- console.log(' - The deployment is not running the latest code');
- console.log(' - The page needs to be reloaded');
- }
+ let earlyHintsFound = false;
+
+ if (earlyHintsMatch) {
+ earlyHintsFound = true;
+ console.log('🎉 Found Early Hints debug comments in HTML!\n');
+ earlyHintsMatch.forEach((match) => {
+ console.log(match);
+ console.log();
+ });
+ console.log('\n✅ SUCCESS: Early Hints are configured and working!');
+ } else {
+ console.log('❌ No Early Hints debug comments found in HTML');
+ console.log('This might mean:');
+ console.log(' - Early hints are not enabled');
+ console.log(' - The deployment is not running the latest code');
+ console.log(' - The page needs to be reloaded');
+ }
@@
- ws.close();
- process.exit(0);
+ ws.close();
+ process.exit(earlyHintsFound ? 0 : 1);Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 ESLint
[error] 79-79: Replace link with (link)
(prettier/prettier)
🤖 Prompt for AI Agents
In check_early_hints.js around lines 68 to 86, the script currently always exits
with status 0 even when no Early Hints debug comments are found; change the exit
behavior so that when the debug comments are absent (the branch that logs "No
Early Hints debug comments found in HTML") the script closes the websocket if
open and exits with a non-zero status (e.g., process.exit(1)) to signal failure
to callers; keep the success path exiting with 0 and ensure any resources (ws)
are closed before calling process.exit.
| @@ -0,0 +1,89 @@ | |||
| #!/usr/bin/env python3 | |||
There was a problem hiding this comment.
Add execute permission to the script.
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.py🧰 Tools
🪛 Ruff (0.14.5)
1-1: Shebang is present but file is not executable
(EXE001)
🤖 Prompt for AI Agents
In check_early_hints.py around line 1, the file has a shebang but lacks execute
permissions; add the executable bit to the file so it can be run directly (for
example by running chmod +x check_early_hints.py) and verify the permission
change with ls -l.
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>
🎉 ✨ Deploy Complete! 🚀🌐 ➡️ Open Review AppDeployment successful for PR #687, commit c23e387 🎮 Control Plane Console |
Pull Request Review: Enable Shakapacker Early HintsSummaryThis PR introduces Thruster HTTP/2 proxy integration, updates Ruby version from 3.4.3 to 3.4.6, and enables early hints for improved performance. The changes span infrastructure (Docker, Control Plane), dependencies (Gemfile, package.json), documentation, and UI updates. Code Quality & Best Practices✅ Strengths
|
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>
🎉 ✨ Deploy Complete! 🚀🌐 ➡️ Open Review AppDeployment successful for PR #687, commit 80df3b5 🎮 Control Plane Console |
Pull Request Review: Enable Shakapacker Early HintsThank you for this comprehensive PR implementing HTTP/2 and Early Hints support via Thruster! ✅ Strengths
|
🐛 Potential Issues & 🧪 Test Coverage1. Beta/Unreleased Dependencies
Risk: Master branch may have unreleased bugs, breaking changes, or lack of semantic versioning guarantees. Recommendation: Document why master is required, consider requesting a formal release or using a specific commit SHA. 2. Missing Test Coverage
Recommendation: Add minimal test coverage for Thruster proxy functionality, Early Hints header presence in production environment, and Footer component rendering. 3. Verification Scripts Not IntegratedThe check_early_hints.js and check_early_hints.py scripts are useful but:
📊 Performance ConsiderationsExpected Benefits (from testing): 20-30% faster initial page loads, 40-60% reduction in transfer size, improved caching Consider adding: Performance metrics collection (Core Web Vitals), Early Hints header validation in production, asset load timing monitoring See next comment for recommendations... |
🎯 Recommendations SummaryMust Fix (Before Merge):
Should Fix (High Priority):
Nice to Have:
✅ ConclusionThis is a well-researched and thoroughly documented PR that brings significant performance improvements. The documentation quality is excellent, and the implementation shows good understanding of the Thruster/HTTP/2/Early Hints stack. However, there are critical security concerns around SECRET_KEY_BASE handling that must be addressed before merging. The debug mode in production and lack of test coverage are also concerns that should be addressed. Verdict: Once the security issues are resolved, this will be an excellent addition to the project. Review conducted by: Claude Code (Automated Review) |
- 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>
🎉 ✨ Deploy Complete! 🚀🌐 ➡️ Open Review AppDeployment successful for PR #687, commit 1dea0e6 🎮 Control Plane Console |
PR Review: Enable Shakapacker Early Hints with Thruster HTTP/2OverviewThis PR adds comprehensive HTTP/2 and early hints support through Thruster integration and Shakapacker configuration. The implementation is well-documented and shows good attention to deployment concerns. 🟢 Strengths1. Excellent Documentation
2. Thoughtful Infrastructure Changes
3. Configuration Improvements
🟡 Concerns & Issues1. SECURITY: Hardcoded SECRET_KEY_BASE in Templates
|
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>
🎉 ✨ Deploy Complete! 🚀🌐 ➡️ Open Review AppDeployment successful for PR #687, commit e1ad266 🎮 Control Plane Console |
Pull Request ReviewThank you for this comprehensive PR implementing Thruster HTTP/2 proxy and Shakapacker early hints support! This is a significant enhancement with well-documented changes. Here's my detailed review: ✅ Strengths1. Excellent Documentation
2. Thoughtful Infrastructure Changes
3. Security Improvements
🔴 Critical Issues1. Insecure Placeholder SECRET_KEY_BASE in Production TemplateLocation: - name: SECRET_KEY_BASE
# For test apps, a placeholder value is fine. For production apps, this should be
# set to a secure random value using: openssl rand -hex 64
# Production apps should configure this manually after app creation via a secret.
value: 'placeholder_secret_key_base_for_test_apps_only'Issue: This hardcoded placeholder secret is a critical security vulnerability if accidentally used in production. Recommendation:
Suggested fix: - name: SECRET_KEY_BASE
# IMPORTANT: This MUST be set to a secure random value for ALL deployments
# Generate one using: openssl rand -hex 64
# For production, configure this via Control Plane secrets, NOT this template
# This placeholder will cause Rails to fail to start - this is intentional
value: 'CHANGEME_OR_DEPLOYMENT_WILL_FAIL'2. Using Master Branch Dependencies in ProductionLocation: gem "react_on_rails", github: "shakacode/react_on_rails", branch: "master"Issue: Depending on the
Recommendation:
|
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