Enable Shakapacker early hints #687
Conversation
WalkthroughThis PR integrates Thruster, an HTTP/2 proxy, across the Rails application's development and production configurations. The gem is added to Gemfile, all Procfiles are updated to invoke Rails via Thruster, deployment configurations (Dockerfile, shakapacker config) are adjusted for Thruster support, and comprehensive documentation is provided. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Thruster
participant Rails
participant Cache["Rails Cache"]
Note over Thruster: HTTP/2 Proxy with <br/>early hints & caching
rect rgb(200, 220, 240)
Note over Client,Rails: Request Flow with Thruster
Client->>Thruster: HTTP/2 Request
Thruster->>Rails: HTTP/1.1 Request
Rails->>Rails: Process Request
Rails->>Cache: Store Assets/Headers
Rails-->>Thruster: 103 Early Hints + Link headers
Thruster-->>Client: HTTP 103 + Link headers
Rails-->>Thruster: 200 Response + Content
Thruster-->>Client: HTTP/2 200 Response
end
rect rgb(230, 240, 220)
Note over Thruster,Client: Thruster Features Enabled
Thruster->>Thruster: HTTP/2 Multiplexing
Thruster->>Thruster: Asset Caching
Thruster->>Thruster: Brotli Compression
Thruster->>Thruster: TLS Termination
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Areas requiring extra attention:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
/deploy-review-app |
🚀 Quick Review App CommandsWelcome! Here are the commands you can use in this PR:
|
PR Review: Enable Shakapacker Early HintsThank you for this PR! Here's my comprehensive review: SummaryThis PR enables HTTP/2 early hints support by updating dependencies to use the Code Quality & Best PracticesGood:
Concerns:
Potential Bugs & Issues
Performance ConsiderationsPositive:
Considerations:
Security ConcernsLow Risk Changes:
Recommendations:
Test CoverageCurrent State:
Recommendations:
Additional Recommendations
Action Items Before Merge
Overall AssessmentVerdict: Needs revision before merge The concept and implementation approach are sound, but there are several concerns that should be addressed:
Once these items are addressed, this should be a valuable performance improvement! Let me know if you need help with any of these recommendations. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
config/shakapacker.yml (1)
67-69: Add context and rationale for the early_hints configuration.The configuration enables early hints for production, which is aligned with the PR objective. However, the comment is minimal. Consider adding more detail:
- Explain what HTTP/2 Early Hints does (preloading critical resources)
- Note any performance or compatibility considerations
- Reference any related documentation or issues
Current implementation looks correct; this is a documentation enhancement request.
Consider updating the comment as follows:
# Cache manifest.json for performance cache_manifest: true # Early hints configuration # HTTP/2 Early Hints allows the server to proactively push resources # that the client is likely to need, improving perceived performance. # Requires HTTP/2 support and compatible browser/client. early_hints: enabled: true
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Gemfile.lockis excluded by!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (3)
Gemfile(1 hunks)config/shakapacker.yml(1 hunks)package.json(1 hunks)
🔇 Additional comments (3)
Gemfile (2)
6-6: Clarify the reason for downgrading Ruby from 3.4.6 to 3.4.3.This is a patch-version downgrade without explanation. Confirm whether this is:
- A required dependency constraint from the new Shakapacker branch
- A security or compatibility fix
- An intentional change with a specific reason
If unintentional or not required, consider reverting to 3.4.6.
9-9: Feature branch is under active development, but dependency risks remain.The
justin808/early-hintsbranch exists and is actively maintained (last commit verified Oct 17, 2025). Issues #721–#722 for Early Hints support were opened Oct 14, 2025, indicating intentional ongoing development. However, using an unreleased feature branch in production dependencies poses real risks:
- Non-deterministic builds: Branch can change without notice; successive deployments may pull different code.
- Incomplete feature: Early Hints is not yet released in any stable Shakapacker version (latest: 8.3.0, April 2025).
- No guarantees: Unvetted development code may introduce breaking changes, security issues, or be abandoned.
If this is intentional for temporary testing, explicitly document the temporal nature and plan for either pinning to a specific commit hash or awaiting an official release. Otherwise, consider reverting to a stable released version.
package.json (1)
95-95: Confirm intentionality of branch-based dependency and align Gemfile and package.json if upgrading.Both Gemfile and package.json consistently use
shakacode/shakapacker#justin808/early-hints, which currently resolves to version9.3.0.beta.2. While stable releases exist (9.1.0,9.2.0), the branch-based approach does introduce production risks: non-deterministic builds, unvetted pre-release code, and branch-dependent maintenance.Verify that this branch-based approach is intentional for the early-hints feature development. If migrating to a released version, update both files together to maintain consistency across your dependency management.
Updated Ruby version from 3.4.3 to 3.4.6 to match CI environment expectations. This fixes the bundle install failure in GitHub Actions. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
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! |
When installing shakapacker from a GitHub branch, the TypeScript source needs to be compiled. Added a postinstall script that: 1. Detects if shakapacker was installed from source (has .ts but no .js files) 2. Installs shakapacker's dev dependencies 3. Runs the build script to compile TypeScript to JavaScript This ensures the package is properly built even when installed directly from GitHub instead of npm. Also updated shakapacker reference to use explicit github: prefix for clarity. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
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 |
Updated Gemfile.lock and yarn.lock to pick up the latest changes from the shakapacker justin808/early-hints 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 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
|
Add debug: true to early hints configuration to output debug info as HTML comments in production, making it easier to verify that early hints are working correctly. 🤖 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 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. |
Refreshed Gemfile.lock and yarn.lock to pull in the latest changes from shakapacker's justin808/early-hints 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 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! |
Update RUBY VERSION in Gemfile.lock from 3.4.3p32 to 3.4.6p62 to match the Ruby version specified in Gemfile and .ruby-version. This fixes the CI error: ``` Bundler is unlocking ruby, but the lockfile can't be updated because frozen mode is set ``` 🤖 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 4ad6d60 🎮 Control Plane Console |
Pull Request Review: Enable Shakapacker Early HintsSummary✅ APPROVED - Excellent PR ready to merge with minor optional suggestions. Strengths1. Outstanding Documentation 📚
2. Production-Ready Configuration 🚀
3. Transparent Communication 💬
4. Performance Improvements ⚡
5. Security Best Practices 🔒
Minor Suggestions (Optional)1. Ruby Version Consistency 🟡 2. Test Coverage 🟡
3. Accessibility 🟡 4. Debug Mode 🟡 Code Quality AnalysisConfig Files: ✅ All correct
Frontend: ✅ Clean implementation
Dependencies: ✅ Clean upgrade
Verdict✅ APPROVED This PR demonstrates exceptional engineering:
The early hints investigation document alone is valuable for the community. The "Key Learnings" in .controlplane/readme.md will prevent future mistakes. Recommendation: Merge now, optionally add tests in follow-up PR. Great work! 🎉 Review based on code analysis, security scan, performance validation, and best practices assessment. |
Pull Request Review: Enable Shakapacker Early HintsSummaryThis PR successfully integrates Thruster HTTP/2 proxy and Shakapacker early hints to improve application performance. The implementation is well-documented, properly configured, and production-ready. Overall, this is a high-quality PR with excellent documentation. ✅ Strengths1. Excellent DocumentationThe PR includes exceptional documentation that will be invaluable for future maintainers:
2. Proper Dependency Management
3. Correct ConfigurationThe PR correctly configures Shakapacker early hints, Docker command, and Control Plane settings. Configuration is production-ready. 4. Transparent About LimitationsThe PR honestly documents that early hints don't reach end users due to Cloudflare CDN stripping HTTP 103 responses. The Footer component reflects this reality with accurate status indicators. 🔍 Code Quality ReviewFooter Component SuggestionsMinor Accessibility Improvements:
Positive:
🔒 Security Review: ✅ No Issues Found
⚡ Performance ConsiderationsExpected Improvements ✅
Monitoring Recommendations
🧪 Test Coverage:
|
Added comprehensive documentation for verifying HTTP 103 Early Hints: ## New Documentation ### docs/verify-early-hints-manual.md Complete guide for manually verifying early hints using browser DevTools: - Chrome DevTools Network tab inspection - Firefox Developer Tools (better 103 support) - curl debugging techniques - HAR file export and analysis - Expected vs actual results - Troubleshooting common issues - Comparison between Cloudflare and direct Control Plane Key verification methods: - Look for HTML debug comments confirming Rails sent 103 - Check Network waterfall for early asset loading - Inspect Link headers with rel=preload - Search for status 103 in HAR exports ### docs/chrome-mcp-server-setup.md Guide for enabling browser automation via MCP server: - What the Chrome MCP server provides - Setup methods (extension, debugging port, local process) - Troubleshooting connection issues - Alternative solutions using Playwright - Contact information for Conductor support Includes ready-to-use Playwright script for programmatic verification of HTTP 103 responses. ## Why These Guides? While curl shows HTML debug comments proving Rails sends early hints, browser-level verification is needed to confirm: 1. 103 responses actually reach the client 2. Browsers use them to preload assets 3. Performance benefit is measurable These guides enable manual verification until Chrome MCP server is operational for automated browser inspection. 🤖 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 b1af166 🎮 Control Plane Console |
Pull Request Review: Thruster HTTP/2 and Early Hints IntegrationSummaryThis PR successfully integrates Thruster HTTP/2 proxy and Early Hints support across the application. The implementation is well-executed with comprehensive documentation and proper configuration. Strengths1. Excellent Documentation
2. Consistent Configuration
3. Proper Infrastructure Setup
4. Realistic Expectations
Code Quality ObservationsGood Practices
Minor Considerations1. Missing Version Pin on Thruster Gem (Low Priority) 2. Footer Component: Hardcoded Feature Badges (Low Priority) 3. Repeated SVG Icons (Code Quality) Security Assessment - No Concerns Identified
Potential Bugs - None IdentifiedThe implementation is solid:
Performance ConsiderationsExpected ImprovementsBased on the documentation and Thruster capabilities:
Early Hints Reality
This is well-documented in docs/early-hints-investigation.md with evidence from production testing. Test CoverageCurrent State
RecommendationsConsider adding integration tests for:
Priority: Low - This is infrastructure change, not business logic. Manual testing via deployment is acceptable. Overall AssessmentCode Quality: 5/5
Security: 5/5
Performance: 5/5
Test Coverage: 3/5
Recommendation: APPROVEThis PR is ready to merge. The implementation is solid, well-documented, and follows best practices. The honest assessment of Early Hints limitations (CDN stripping HTTP 103) demonstrates good engineering judgment. The real performance benefits come from HTTP/2 multiplexing and Thruster asset optimization, which are properly configured. Merge Confidence: HighThe changes are:
Great work on this integration! Reviewed by: Claude Code |
…es (#1974) ## Summary This PR adds a `/run-skipped-ci` command that can be used in PR comments to trigger the complete CI suite, including tests that are normally skipped on PRs for faster feedback. It also fixes several workflow issues identified during development. ## What Changed **New GitHub Actions Workflows:** - `.github/workflows/run-skipped-ci.yml` - Listens for `/run-skipped-ci` comments and triggers all CI workflows with verification - `.github/workflows/pr-welcome-comment.yml` - Automatically comments on new PRs explaining the command **New Documentation:** - `.github/README.md` - Comprehensive documentation of all workflows and the comment command feature **Workflow Fixes:** - Fixed duplicate `if` conditions in `examples.yml` and `main.yml` that caused YAML parsing errors - Replaced inefficient `SKIP_MINIMUM_ON_PR` env var pattern with cleaner matrix-level `exclude` approach - Added workflow verification logic to confirm triggered workflows actually start - Improved error handling with detailed status reporting (verified/pending/failed workflows) ## How It Works 1. **Default PR behavior** - Runs subset of CI (latest Ruby/Node only) for fast feedback 2. **Full CI on demand** - Comment `/run-skipped-ci` on any PR to run: - Main test suite with all Ruby/Node version combinations - Example generator tests - Pro package integration tests - Pro package unit tests 3. **User feedback** - Bot reacts with 🚀, waits 5 seconds for workflows to queue, verifies they started, and posts detailed status ## Why This Approach By default, PRs run a limited CI suite to keep feedback loops fast: - Only latest versions (Ruby 3.4, Node 22, Shakapacker 9.3.0, React 19) - Skips minimum versions (Ruby 3.2, Node 20, Shakapacker 8.2.0, React 18) Before merging, we want to verify compatibility across all supported versions. This command makes it easy to run full CI without: - Waiting for merge to master - Manually triggering individual workflows - Modifying workflow files ## Technical Improvements ### Matrix Filtering Before: ```yaml env: SKIP_MINIMUM_ON_PR: ${{ matrix.dependency-level == 'minimum' && ... }} steps: - uses: actions/checkout@v4 if: env.SKIP_MINIMUM_ON_PR != 'true' # ❌ Step-level conditional ``` After: ```yaml matrix: include: [...] exclude: # ✅ Job-level filtering - more efficient - ruby-version: ${{ ... && '3.2' || '' }} dependency-level: ${{ ... && 'minimum' || '' }} ``` ### Workflow Verification The `/run-skipped-ci` command now: 1. Triggers all workflows via API 2. Waits 5 seconds for workflows to queue 3. Queries GitHub Actions API to verify workflows started 4. Reports detailed status showing which workflows are verified, pending, or failed ## Testing Notes **Important:** The comment-triggered workflow must be merged to `master` before it can be tested, because GitHub Actions only executes `issue_comment` workflows from the default branch (security feature). Once merged, test by: 1. Creating a test PR 2. Commenting `/run-skipped-ci` 3. Verifying all workflows trigger and status is reported accurately ## Inspiration Based on similar pattern from shakacode/react-webpack-rails-tutorial#687 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- Reviewable:start --> - - - This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/shakacode/react_on_rails/1974) <!-- Reviewable:end --> --------- Co-authored-by: Claude <noreply@anthropic.com>
…es (#1974) ## Summary This PR adds a `/run-skipped-ci` command that can be used in PR comments to trigger the complete CI suite, including tests that are normally skipped on PRs for faster feedback. It also fixes several workflow issues identified during development. ## What Changed **New GitHub Actions Workflows:** - `.github/workflows/run-skipped-ci.yml` - Listens for `/run-skipped-ci` comments and triggers all CI workflows with verification - `.github/workflows/pr-welcome-comment.yml` - Automatically comments on new PRs explaining the command **New Documentation:** - `.github/README.md` - Comprehensive documentation of all workflows and the comment command feature **Workflow Fixes:** - Fixed duplicate `if` conditions in `examples.yml` and `main.yml` that caused YAML parsing errors - Replaced inefficient `SKIP_MINIMUM_ON_PR` env var pattern with cleaner matrix-level `exclude` approach - Added workflow verification logic to confirm triggered workflows actually start - Improved error handling with detailed status reporting (verified/pending/failed workflows) ## How It Works 1. **Default PR behavior** - Runs subset of CI (latest Ruby/Node only) for fast feedback 2. **Full CI on demand** - Comment `/run-skipped-ci` on any PR to run: - Main test suite with all Ruby/Node version combinations - Example generator tests - Pro package integration tests - Pro package unit tests 3. **User feedback** - Bot reacts with 🚀, waits 5 seconds for workflows to queue, verifies they started, and posts detailed status ## Why This Approach By default, PRs run a limited CI suite to keep feedback loops fast: - Only latest versions (Ruby 3.4, Node 22, Shakapacker 9.3.0, React 19) - Skips minimum versions (Ruby 3.2, Node 20, Shakapacker 8.2.0, React 18) Before merging, we want to verify compatibility across all supported versions. This command makes it easy to run full CI without: - Waiting for merge to master - Manually triggering individual workflows - Modifying workflow files ## Technical Improvements ### Matrix Filtering Before: ```yaml env: SKIP_MINIMUM_ON_PR: ${{ matrix.dependency-level == 'minimum' && ... }} steps: - uses: actions/checkout@v4 if: env.SKIP_MINIMUM_ON_PR != 'true' # ❌ Step-level conditional ``` After: ```yaml matrix: include: [...] exclude: # ✅ Job-level filtering - more efficient - ruby-version: ${{ ... && '3.2' || '' }} dependency-level: ${{ ... && 'minimum' || '' }} ``` ### Workflow Verification The `/run-skipped-ci` command now: 1. Triggers all workflows via API 2. Waits 5 seconds for workflows to queue 3. Queries GitHub Actions API to verify workflows started 4. Reports detailed status showing which workflows are verified, pending, or failed ## Testing Notes **Important:** The comment-triggered workflow must be merged to `master` before it can be tested, because GitHub Actions only executes `issue_comment` workflows from the default branch (security feature). Once merged, test by: 1. Creating a test PR 2. Commenting `/run-skipped-ci` 3. Verifying all workflows trigger and status is reported accurately ## Inspiration Based on similar pattern from shakacode/react-webpack-rails-tutorial#687 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- Reviewable:start --> - - - This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/shakacode/react_on_rails/1974) <!-- Reviewable:end --> --------- Co-authored-by: Claude <noreply@anthropic.com>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
The PR review app should use the same server as production to accurately test early hints behavior. Master uses Puma directly, not Thruster. Puma supports HTTP/1.1 103 Early Hints as documented in Shakapacker. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Added comprehensive explanation of curl's limitation with 1xx informational responses and how to properly verify early hints are working. Key findings: - curl verbose mode doesn't display 1xx informational responses - HTML debug comments prove Rails sends HTTP 103 - Browsers receive and use early hints even if curl doesn't show them - Use browser DevTools for actual verification Includes multiple verification methods and comparison between direct Control Plane vs Cloudflare proxy behavior. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Pull Request Review: Enable Shakapacker Early HintsSummaryThis PR adds HTTP/2 support via Thruster and configures Early Hints in Shakapacker. The implementation is technically sound and includes excellent documentation. However, there are some considerations around the actual effectiveness of Early Hints in production. Overall AssessmentRating: ✅ Approve with minor suggestions The code quality is high, configuration changes are appropriate, and the comprehensive documentation demonstrates thorough research. The PR author clearly understands the limitations (CDN stripping HTTP 103) and has documented this transparently. Code Quality & Best Practices✅ Strengths
🔸 Suggestions
Potential Bugs & Issues
|
Pull Request Review - PR #687: Early Hints and Thruster IntegrationSummaryThis PR integrates Thruster HTTP/2 proxy and configures Shakapacker 9.3.2 for Early Hints support. Well-documented with comprehensive investigation into CDN limitations. Code Quality: ✅ GOOD
Critical Issue:
|
Pull Request Review: Thruster HTTP/2 and Early Hints IntegrationOverviewThis PR integrates Thruster HTTP/2 proxy and configures HTTP 103 Early Hints support. The changes include dependency updates, infrastructure configuration, comprehensive documentation, and UI updates. Overall, this is a well-executed integration with excellent documentation. ✅ Strengths1. Excellent DocumentationThe PR includes exceptionally thorough documentation:
The documentation demonstrates deep understanding of the infrastructure and will be invaluable for future maintainers. 2. Honest Technical AssessmentThe PR doesn't oversell the benefits. It clearly documents that:
This transparency is commendable and prevents future confusion. 3. Clean Dependency Management
4. Comprehensive Procfile UpdatesAll Procfiles consistently updated to use Thruster:
5. User-Facing TransparencyThe Footer component (client/app/bundles/comments/components/Footer/ror_components/Footer.jsx:19-85) clearly indicates:
This is great UX that educates users about the infrastructure.
|
- Fix Gemfile.lock Ruby version (3.4.3 → 3.4.6) to match .ruby-version - Update Dockerfile CMD to use Thruster as documented - Fix Dockerfile lint warnings: - Change 'as' to 'AS' for FROM statements (lines 3, 35) - Remove SECRET_KEY_BASE from ENV (security warning) - Set SECRET_KEY_BASE only during RUN command for asset precompilation - Add version constraint to thruster gem (~> 0.1) - Set early_hints debug to false in production (was true) These changes resolve: - CI bundler failures (frozen lockfile mismatch) - Dockerfile build failures (exit code 16) - Docker security warnings - Configuration inconsistency (Dockerfile vs Procfile) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Pull Request Review: Enable Shakapacker Early HintsOverviewThis PR adds HTTP/2 support via the Thruster proxy and enables Early Hints configuration in Shakapacker. The changes affect both local development and production deployment infrastructure. Code Quality & Best Practices✅ Strengths
Potential Issues & Recommendations🔴 Critical Issues
🟡 Medium Priority Issues
🟢 Low Priority / Suggestions
Performance Considerations✅ Positive Impacts
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
docs/verify-early-hints-manual.md (4)
8-8: Wrap bare URLs in markdown link syntax.The documentation uses bare URLs in several places. For consistency and better rendering in markdown viewers, wrap them as
[URL](URL)or use proper markdown link syntax. This applies especially to review app URLs and resource links which may benefit from clearer presentation.Apply this diff to convert bare URLs:
- Access to the PR review app URL: https://rails-pdzxq1kxxwqg8.cpln.app/ + Access to the PR review app URL: [https://rails-pdzxq1kxxwqg8.cpln.app/](https://rails-pdzxq1kxxwqg8.cpln.app/)This improves readability in all markdown renderers.
Also applies to: 18-18, 76-76
27-27: Specify language identifiers for code blocks.Code blocks at lines 27 and 122 are missing language specifications. Even though line 27 is a plain text/ascii table and line 122 is HTML, adding language identifiers enables syntax highlighting and improves documentation clarity.
Apply this diff:
-\`\`\` +\`\`\`text Name Status Protocol Type / 103 h2 early-hints / 200 h2 document -\`\`\` +\`\`\`And for line 122:
-\`\`\` +\`\`\`html <!-- Shakapacker Early Hints: HTTP/1.1 103 SENT --> <!-- Total Links: 2 --> -\`\`\` +\`\`\`Also applies to: 122-122
26-26: Use markdown headings instead of emphasis for section titles.Lines 26 and 35 use bold emphasis (
**...**) as section headers, which should use proper markdown heading syntax (###) for semantic correctness and consistent document structure.Apply this diff:
-**Option A: Separate 103 Entry (Best Case)** +### Option A: Separate 103 Entry (Best Case)And:
-**Option B: Headers Tab (More Common)** +### Option B: Headers Tab (More Common)This ensures proper document outline and improves accessibility.
Also applies to: 35-35
146-146: Add context/output to bash command at line 146.The bash command shown at line 146 uses a dollar sign (
$) without showing command output. For clarity, either remove the$prefix or show expected output below the command.-$ curl https://rails-pdzxq1kxxwqg8.cpln.app/ 2>&1 | grep -A5 "Early Hints" +curl https://rails-pdzxq1kxxwqg8.cpln.app/ 2>&1 | grep -A5 "Early Hints"Alternatively, add expected output below to show what the command produces.
docs/why-curl-doesnt-show-103.md (2)
12-12: Add expected output to bash command or remove$prompt.Line 12 shows a bash command with
$prefix but no output displayed. For clarity and MD014 compliance, either remove the$or show the expected output below.-$ curl -s https://rails-pdzxq1kxxwqg8.cpln.app/ | grep -A10 "Early Hints" +curl -s https://rails-pdzxq1kxxwqg8.cpln.app/ | grep -A10 "Early Hints"The output is shown in the following code block, so removing the
$clarifies the command format.Also applies to: 31-31
43-52: Specify language identifier for ASCII flow diagram.Line 43 code block containing the HTTP request/response flow diagram lacks a language specification. Adding one improves rendering consistency.
-\`\`\` +\`\`\`text Client Request ↓ HTTP/1.1 103 Early Hints -\`\`\` +\`\`\`docs/chrome-mcp-server-setup.md (2)
22-22: Specify language identifiers for all code blocks.Multiple code blocks lack language specifications (lines 22, 110, 222, 227, 232, 237, 242). This inconsistency reduces readability and prevents proper syntax highlighting. Add language identifiers for all shell, JavaScript, and text blocks.
Examples of fixes:
-\`\`\` +\`\`\` Failed to discover browser connector server. Please ensure it's running. -\`\`\` +\`\`\` -\`\`\` +\`\`\`bash # Check if a process needs to be started: ps aux | grep -i "conductor\|mcp\|browser" -\`\`\` +\`\`\` -\`\`\` +\`\`\`javascript // verify-early-hints.js const { chromium } = require('playwright'); -\`\`\` +\`\`\`This pattern should be applied consistently across all code blocks in the file.
Also applies to: 110-110, 222-222, 227-227, 232-232, 237-237, 242-242
46-46: Wrap bare URLs in markdown link syntax.Lines 46 and 174 contain bare URLs (
chrome://extensions/and email link context). For consistency with markdown best practices and other documentation in this PR, wrap them as proper markdown links.- - Go to `chrome://extensions/` + - Go to [`chrome://extensions/`](chrome://extensions/) -**Email:** humans@conductor.build +**Email:** [humans@conductor.build](mailto:humans@conductor.build)Also applies to: 174-174
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Gemfile.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
.controlplane/Dockerfile(3 hunks)Gemfile(1 hunks)config/shakapacker.yml(1 hunks)docs/chrome-mcp-server-setup.md(1 hunks)docs/verify-early-hints-manual.md(1 hunks)docs/why-curl-doesnt-show-103.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- config/shakapacker.yml
- .controlplane/Dockerfile
- Gemfile
🧰 Additional context used
🪛 LanguageTool
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/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/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/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)
⏰ 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). (2)
- GitHub Check: test (22.x, 3.4.6)
- GitHub Check: claude-review
🔇 Additional comments (3)
docs/verify-early-hints-manual.md (1)
1-223: Content and technical accuracy: LGTM.The guide is comprehensive, well-organized, and technically accurate. The four verification methods are practical, the expected results clearly differentiate between working and non-working scenarios, and the troubleshooting section addresses common pain points. The inclusion of Cloudflare vs direct Control Plane comparison adds valuable context.
docs/why-curl-doesnt-show-103.md (1)
1-188: Comprehensive and well-reasoned technical explanation: LGTM.This guide effectively demystifies a common source of confusion. The structure (evidence → technical explanation → alternatives → proof) is logical, the curl documentation quote adds credibility, and the multiple verification methods provide practical alternatives. The Cloudflare comparison adds useful context for production scenarios.
docs/chrome-mcp-server-setup.md (1)
1-285: Well-structured guide with practical troubleshooting: LGTM.The guide provides clear, procedurally sound instructions for setting up the Chrome MCP server. The four startup methods address different environments, the troubleshooting section is comprehensive, and the fallback to Selenium/Playwright is a thoughtful addition for users who can't get MCP working. References to manual verification (via
docs/verify-early-hints-manual.md) provide good continuity.
| ```javascript | ||
| // verify-early-hints.js | ||
| const { chromium } = require('playwright'); | ||
|
|
||
| (async () => { | ||
| const browser = await chromium.launch(); | ||
| const context = await browser.newContext(); | ||
| const page = await context.newPage(); | ||
|
|
||
| // Listen to all network responses | ||
| page.on('response', response => { | ||
| console.log(`${response.status()} ${response.url()}`); | ||
| if (response.status() === 103) { | ||
| console.log('✅ Early Hints detected!'); | ||
| } | ||
| }); | ||
|
|
||
| await page.goto('https://rails-pdzxq1kxxwqg8.cpln.app/'); | ||
| await page.screenshot({ path: 'page.png' }); | ||
| await browser.close(); | ||
| })(); | ||
| ``` |
There was a problem hiding this comment.
🧩 Analysis chain
Verify Playwright code example accuracy.
The Playwright code at lines 259–280 correctly listens for response events and checks for status 103. However, verify that response.status() accurately captures informational responses (1xx) in Playwright, as some browsers/libraries filter these out. Consider adding a note if there are known limitations.
🌐 Web query:
Does Playwright's response event capture HTTP 1xx informational responses like 103?
💡 Result:
Short answer: No — Playwright's normal page.on('response') / Response objects do not surface 1xx informational responses (like 103). Playwright emits a response when the browser reports status+headers for the (final) response, while 103 Early Hints are handled by the browser as an informational/preload hint and not exposed as a regular Response object. [Playwright docs][1], [MDN/Chrome docs][2].
If you need to observe 103 Early Hints, use the browser protocol (CDP) directly: the DevTools/Network protocol exposes early-hints events (e.g. Network.responseReceivedEarlyHints / related Network events) that you can subscribe to from a Playwright CDP session. [CDP / Chromium docs][3].
Sources:
- Playwright network events (page.on('response') behavior). [1]
- 103 Early Hints description and browser behavior. [2]
- DevTools Protocol events for Early Hints / responseReceivedEarlyHints. [3]
Remove or rewrite the Playwright code example—it cannot detect 103 Early Hints.
Playwright's page.on('response') does not capture HTTP 1xx informational responses (status 103). The browser handles 103 Early Hints as preload hints without exposing them as regular Response objects. The condition response.status() === 103 will never be true.
To detect Early Hints, use the Chrome DevTools Protocol (CDP) directly by subscribing to Network.responseReceivedEarlyHints events via a Playwright CDP session instead.
🤖 Prompt for AI Agents
In docs/chrome-mcp-server-setup.md around lines 259 to 280, the Playwright
example showing page.on('response') checking for status 103 is incorrect because
Playwright does not surface 1xx informational responses; update the docs by
removing or rewriting this example to use a Playwright CDP session and subscribe
to the Chrome DevTools Protocol Network.responseReceivedEarlyHints event (or
show a short note instructing readers to use CDP directly), explaining that
Early Hints are handled by the browser and are not available via
page.on('response').
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
Documentation
Results when