Skip to content

Conversation

@justin808
Copy link
Member

@justin808 justin808 commented Oct 17, 2025

Summary

This PR adds HTTP/2 Early Hints support via Thruster proxy and Shakapacker, enabling browsers to preload critical CSS and JS assets before the main response is ready.

Key Changes

Core Features

  • Thruster HTTP/2 Proxy: Added thruster gem to serve Rails via HTTP/2 with early hints support
  • Shakapacker Early Hints: Enabled early_hints configuration in config/shakapacker.yml
  • Footer Branding: Added "Powered by Thruster HTTP/2" indicator

Version Updates

  • Ruby: 3.4.4 → 3.4.6
  • react_on_rails: 14.0.5 → 16.2.0.rc1
  • shakapacker: 8.2.0 → 9.5.0

Development Experience

  • New bin/dev script: Simplified development server management with precompile tasks (rescript + locale generation)
  • Procfile updates: All dev/prod Procfiles now use Thruster for HTTP/2 support
  • Lazy Rails loading: Moved Rails environment require inside precompile tasks for faster bin/dev --help and bin/dev kill commands

Testing & Verification

  • Added Chrome DevTools Protocol scripts (check_early_hints.js, check_early_hints.py) for verifying early hints
  • Updated system tests to use custom headless Chrome driver
  • Fixed Chrome headless configuration for Rails 8.1+

Documentation

  • docs/thruster.md: Comprehensive guide on Thruster setup and configuration
  • docs/early-hints-investigation.md: Investigation notes on early hints behavior
  • docs/verify-early-hints-manual.md: Manual verification steps
  • docs/why-curl-doesnt-show-103.md: Explanation of curl limitations with HTTP 103
  • docs/chrome-mcp-server-setup.md: Chrome MCP server setup guide

Deployment

  • Updated Control Plane Dockerfile and templates for Thruster
  • Added SECRET_KEY_BASE to release scripts and GVC templates

Testing

Run with production-like assets to see early hints in action:

bin/dev prod

Verify early hints are being sent:

curl -v --http2 http://localhost:3001/ 2>&1 | grep -A5 "< HTTP"

Expected output shows multiple HTTP/1.1 103 Early Hints responses with Link headers for CSS and JS preloading before the final HTTP/1.1 200 OK.

Breaking Changes

None - this is an additive feature that improves performance without changing existing behavior.

@coderabbitai
Copy link

coderabbitai bot commented Oct 17, 2025

Walkthrough

Adds Thruster HTTP/2 proxy integration and docs, bumps Ruby and Shakapacker, routes Rails through thrust in runfiles/Dockerfile/Procfiles, adds Early Hints verification scripts, adjusts webpack and test driver configs, updates dev startup to precompile, and adds a footer notice.

Changes

Cohort / File(s) Summary
Dependencies & Ruby
Gemfile, package.json, .ruby-version, .github/workflows/*
Ruby bumped 3.4.3 → 3.4.6; thruster gem added; shakapacker bumped to 9.4.0; res:watch script added; CI matrices updated.
Run / Procfile variants
Procfile, Procfile.dev, Procfile.dev-prod-assets, Procfile.dev-static, Procfile.dev-static-assets
Rails startup commands switch to bundle exec thrust bin/rails server variants; dev entries adjusted (rescript watch, removed sleeps, env tweaks, Redis commented).
Control Plane / Docker / templates
.controlplane/Dockerfile, .controlplane/release_script.sh, .controlplane/readme.md, .controlplane/templates/app.yml, .controlplane/templates/rails.yml
Dockerfile Ruby base updated; SECRET_KEY_BASE handling for precompile introduced; final CMD routes via Thruster; app template adds SECRET_KEY_BASE env placeholder; control-plane docs and inline comments added.
Shakapacker / webpack / react_on_rails
config/shakapacker.yml, config/webpack/development.js, config/initializers/react_on_rails.rb
shakapacker dev_server shape adjusted and early_hints flags added; webpack conditionally loads ReactRefreshWebpackPlugin only when assets_bundler ≠ rspack; react_on_rails build commands now run yarn res:build before shakapacker.
Dev verification scripts
check_early_hints.js, check_early_hints.py
New CDP scripts to query Chrome DevTools, fetch page HTML, detect Early Hints debug comments and preload links, and exit with success/failure codes.
UI footer
client/app/bundles/comments/components/Footer/ror_components/Footer.jsx
Adds "Powered by Thruster HTTP/2" info block with SVG icons and indicators (HTTP/2, Early Hints, Control Plane link).
Docs — Thruster & Early Hints
README.md, docs/thruster.md, docs/early-hints-investigation.md, docs/verify-early-hints-manual.md, docs/why-curl-doesnt-show-103.md, docs/chrome-mcp-server-setup.md, .controlplane/readme.md
New and expanded docs covering Thruster integration, Early Hints investigation/verification, Chrome MCP usage, troubleshooting, and control-plane guidance.
Tests / Capybara drivers
spec/rails_helper.rb, spec/support/driver_registration.rb
Replace :selenium_chrome_headless usage with custom :headless_chrome driver using --headless=new and --window-size=1920,1080; add system-spec hook and update screenshot registration.
Dev startup
bin/dev
Adds run_precompile_tasks to run yarn res:build and locale compilation before starting the dev server.
Misc / CI & config tweaks
config/shakapacker.yml, .gitignore, other small files
Minor config/comment updates, .context/ and early-hints scripts added to .gitignore, and CI workflow bumps.

Sequence Diagram(s)

sequenceDiagram
    participant Browser
    participant Thruster as Thruster (HTTP/2 proxy)
    participant Rails as Rails (Puma/App)
    participant CDN as CDN (optional)

    Note over Browser,Thruster: Browser connects via TLS/HTTP/2
    Browser->>Thruster: GET / (HTTP/2)
    Thruster->>Rails: proxied GET / (HTTP/1.1)
    Rails-->>Thruster: 103 Early Hints (Link headers)
    Thruster-->>Browser: 103 Early Hints (HTTP/2)
    Rails-->>Thruster: 200 OK + body
    Thruster-->>Browser: 200 OK + body
    Note over CDN,Thruster: If CDN present, it may intercept 1xx responses (see investigation docs)
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related issues

Possibly related PRs

Suggested reviewers

  • AbanoubGhadban

Poem

🐰 Thruster hums where bytes now flow,
HTTP/2 whispers, preload seeds go,
Early hints flutter, browsers prepare,
Rails hops along — lighter as air,
A rabbit cheers: faster webs we sow.

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Enable Shakapacker early hints' directly matches the primary change in the PR: introducing early hints configuration via Thruster and Shakapacker integration across development and production environments.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch justin808/shakapacker-early-hints

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@justin808
Copy link
Member Author

/deploy-review-app

@github-actions
Copy link

🚀 Quick Review App Commands

Welcome! Here are the commands you can use in this PR:

/deploy-review-app

Deploy your PR branch for testing

/delete-review-app

Remove the review app when done

/help

Show detailed instructions, environment setup, and configuration options.


@github-actions
Copy link

github-actions bot commented Oct 17, 2025

🏗️ Building Docker image for PR #687, commit 8731438

📝 View Build Logs

🎮 Control Plane Console

@github-actions github-actions bot requested a deployment to review-app October 17, 2025 02:30 In progress
@claude
Copy link

claude bot commented Oct 17, 2025

PR Review: Enable Shakapacker Early Hints

Thank you for this PR! Here's my comprehensive review:

Summary

This PR enables HTTP/2 early hints support by updating dependencies to use the justin808/early-hints branch and configuring the feature in production.


Code Quality & Best Practices

Good:

  • Clean, focused changes with clear intent
  • Configuration change is properly scoped to production environment only
  • Uses standard YAML configuration format

Concerns:

  1. Ruby Version Downgrade (Gemfile:6)

    • Changed from ruby "3.4.6" to ruby "3.4.3"
    • This appears unintentional and could cause deployment issues
    • Recommendation: Revert to 3.4.6 unless there's a specific compatibility reason for the downgrade
  2. Branch Dependencies in Production

    • Both Gemfile and package.json now point to a git branch rather than a stable release
    • Risk: Branch-based dependencies can change unexpectedly, breaking production builds
    • Recommendation:
      • Use this approach for testing only
      • Before merging, either:
        • Point to a tagged release/commit SHA for stability
        • Document the plan to update to an official release version
        • Add a TODO/issue to track when this will be changed to a stable version

Potential Bugs & Issues

  1. Missing Server Configuration

    • Early hints require server-level support (HTTP/2 or HTTP/3)
    • No changes to Puma/Nginx/server configuration are included
    • Action Required: Verify your production server supports HTTP/2 and early hints
    • Rails alone won't send early hints without proper server configuration
  2. Configuration Documentation

    • The early_hints configuration added to shakapacker.yml (lines 67-69) may not be the correct syntax
    • This configuration structure should be verified against the actual implementation in the justin808/early-hints branch
    • Recommendation: Check the shakapacker branch documentation for the correct config format
  3. Gemfile.lock Inconsistency

    • The lock file shows the dependency is now from Git, which is correct
    • However, ensure the revision SHA 6bf6bf5c21388a402362c516370606529d6cfecd represents a stable, tested commit

Performance Considerations

Positive:

  • Early hints can significantly improve page load performance by allowing browsers to preload critical assets
  • This is especially beneficial for apps with many JavaScript/CSS bundles
  • Expected improvement: Reduced time-to-interactive, faster asset loading

Considerations:

  • Early hints effectiveness depends on:
    • Browser support (modern browsers only)
    • Proper HTTP/2 configuration
    • CDN support (if using one)
  • Monitor performance metrics after deployment to validate improvements

Security Concerns

Low Risk Changes:

  • No security-sensitive code modifications
  • Configuration changes are isolated to production asset serving

Recommendations:

  1. Ensure the git branch source is from a trusted repository (✓ shakacode/shakapacker is trusted)
  2. Review the specific commits in the justin808/early-hints branch before deploying to production
  3. Consider pinning to a specific commit SHA instead of a branch for better supply chain security

Test Coverage

Current State:

  • PR includes a testing checklist but no automated tests
  • No tests verify early hints functionality

Recommendations:

  1. Add Integration Tests:

    # spec/requests/early_hints_spec.rb
    RSpec.describe "Early Hints", type: :request do
      it "sends early hints for asset preloading" do
        get root_path
        expect(response.headers["Link"]).to be_present
        # Verify Link headers contain preload directives
      end
    end
  2. Manual Testing Checklist (from PR description):

    • ✓ Verify Shakapacker builds assets correctly
    • ✓ Confirm early hints in production via network inspector
    • Add: Test in browsers that support early hints (Chrome, Edge)
    • Add: Verify Link headers are present in response
    • Add: Measure performance impact (before/after comparison)
  3. Testing Commands:

    # Check Link headers locally (if using Puma with HTTP/2)
    curl -I https://localhost:3000 --http2
    
    # Verify in production
    curl -I https://www.reactrails.com --http2 | grep -i link

Additional Recommendations

  1. Documentation:

    • Add a comment or documentation explaining why early hints are enabled
    • Document any server configuration requirements
    • Update deployment docs if server setup is needed
  2. Rollback Plan:

    • Document how to disable early hints if issues arise
    • Simply removing the config or setting enabled: false should work
  3. Monitoring:

    • Add metrics to track:
      • Early hints sent successfully
      • Performance improvements (TTFB, TTI)
      • Any errors related to early hints

Action Items Before Merge

  • Revert Ruby version to 3.4.6 or explain the downgrade
  • Confirm server configuration supports HTTP/2 early hints
  • Verify the shakapacker.yml configuration syntax is correct
  • Consider pinning to a commit SHA instead of branch
  • Add automated tests or document manual testing results
  • Plan for migration to stable release version

Overall Assessment

Verdict: Needs revision before merge

The concept and implementation approach are sound, but there are several concerns that should be addressed:

  1. Unintentional Ruby version downgrade
  2. Production stability risk from branch dependencies
  3. Missing server configuration verification
  4. Lack of test coverage

Once these items are addressed, this should be a valuable performance improvement!

Let me know if you need help with any of these recommendations.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between d054855 and 8731438.

⛔ Files ignored due to path filters (2)
  • Gemfile.lock is excluded by !**/*.lock
  • yarn.lock is 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-hints branch 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 version 9.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.

@github-actions
Copy link

github-actions bot commented Oct 17, 2025

🏗️ Building Docker image for PR #687, commit a810cca

📝 View Build Logs

🎮 Control Plane Console

@github-actions github-actions bot requested a deployment to review-app October 17, 2025 02:51 In progress
@claude
Copy link

claude bot commented Oct 17, 2025

PR Review: Enable Shakapacker early hints

I've reviewed this PR and here's my comprehensive feedback:

Overall Assessment

This 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 Practices

Strengths:

  • Minimal, targeted changes: Only modifies what's necessary (Gemfile, package.json, shakapacker.yml)
  • Clean dependency management: Uses branch references from the same organization (justin808/early-hints)
  • Proper configuration scope: Early hints enabled only in production environment (config/shakapacker.yml:67-69)
  • Consistent versioning: Gemfile.lock shows proper Git revision tracking

Recommendations:

  1. Document the branch strategy

    • This PR depends on an unreleased feature from a branch (justin808/early-hints)
    • Add a comment explaining why using a branch instead of a released version
    • Include migration plan to switch to official release
  2. Ruby version update anomaly

    • Ruby version changed from 3.4.6p32 to 3.4.6p54
    • This patch level change appears unrelated to early hints
    • Consider documenting this in the PR description

Performance Considerations

Positive Impact:

Early Hints is excellent for performance:

  • Sends 103 Early Hints status with Link headers before the full response
  • Allows browsers to preload critical assets (CSS, JS) while Rails processes the request
  • Can improve Time to First Byte (TTFB) perception and reduce render-blocking delays
  • Particularly beneficial for apps with significant server-side rendering time

Important Requirements:

HTTP/2 Support Required:
Early hints ONLY work with HTTP/2 or HTTP/3. I checked your Puma configuration and noticed:

  • No explicit HTTP/2 configuration visible
  • Default Puma setup uses HTTP/1.1

Action Required:

  1. Verify your production server setup supports HTTP/2
  2. If using a reverse proxy (nginx, CloudFlare, etc.), ensure it supports HTTP/2
  3. Check your deployment platform (Heroku/Control Plane) supports Early Hints
  4. Consider adding documentation about HTTP/2 requirements

Potential Bugs or Issues

Critical: Compatibility Check Needed

  1. Server compatibility:

    • Early Hints requires Rack >= 3.0 for proper support
    • Verify Gemfile.lock shows Rack 3.x (Rails 8.0 should include this)
  2. Browser compatibility:

    • Chrome/Edge: Full support
    • Firefox: Full support
    • Safari: Limited support (16.4+)
    • Gracefully degrades in unsupported browsers
  3. Middleware compatibility:

    • Some Rack middleware may not properly handle 103 responses
    • Common culprits: CDNs, WAFs, reverse proxies
    • Recommendation: Test thoroughly in staging with your full middleware stack

Security Concerns

Low Risk:

  • Early Hints only affects the timing of when asset URLs are sent to browsers
  • No new attack surface introduced
  • Uses the same asset URLs already in your manifest

Considerations:

  1. Content Security Policy (CSP): If using CSP, ensure Early Hints headers comply

  2. Branch dependency from GitHub:

    • Using branch refs means updates happen automatically on bundle update
    • Could introduce unexpected changes
    • Recommendation: Pin to specific commit SHA once feature stabilizes

Test Coverage

Missing Tests:

Currently, there are no tests included for this feature. This is concerning for a production configuration change.

Recommended Test Coverage:

  1. Configuration test - Verify early hints enabled in production config
  2. Integration test - Test early hints headers in production environment
  3. Manual testing checklist:
    • Verify early hints in production staging environment
    • Check browser DevTools Network tab for 103 status
    • Measure page load performance before/after
    • Test with different browsers (Chrome, Firefox, Safari)
    • Verify CDN/proxy compatibility

Action Items Summary

Before Merging:

  1. Document why using a branch dependency and migration plan to stable release
  2. Verify HTTP/2 is enabled in production environment
  3. Confirm Rack 3.x compatibility
  4. Add testing checklist to PR description
  5. Test in staging environment with actual HTTP/2 setup

Nice to Have:

  1. Add performance metrics/benchmarks from staging
  2. Pin to specific commit SHA instead of branch
  3. Document HTTP/2 requirement in deployment docs
  4. Add automated tests for configuration

Conclusion

This 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:

  1. HTTP/2 server compatibility (needs verification)
  2. Missing tests (adds risk for production feature)
  3. Branch dependency instead of release (sustainability concern)

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!

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a810cca and 9fca254.

⛔ Files ignored due to path filters (1)
  • yarn.lock is 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_modules will produce a functional bundle. Confirm that:

  • The yarn build step in shakapacker completes successfully
  • The resulting index.js is 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 install and 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:

  1. Committed yarn.lock (deterministic installs)
  2. postinstall build script (explicit build process)
  3. 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:

  1. Document in a comment why this branch reference exists (e.g., "TODO: pin to shakapacker release once early-hints merges")
  2. Plan to upgrade to a stable shakapacker release once the feature ships
  3. If you want extra safety now, pin to the specific commit (#6bf6bf5) with a comment explaining it's temporary

@github-actions
Copy link

github-actions bot commented Oct 17, 2025

🎉 ✨ Deploy Complete! 🚀

🌐 ➡️ Open Review App

Deployment successful for PR #687, commit 9fca254

🎮 Control Plane Console
📋 View Completed Action Build and Deploy Logs

@claude
Copy link

claude bot commented Oct 17, 2025

Pull Request Review: Enable Shakapacker Early Hints

Thank you for this PR! I've reviewed the changes to enable early hints support in Shakapacker. Here's my detailed feedback:

Code Quality & Best Practices

Strengths:

  • Clean, focused changes that modify only the necessary files
  • Proper use of git-based dependency management for the feature branch
  • Configuration changes are isolated to production environment (appropriate for new feature testing)
  • YAML configuration follows existing patterns in shakapacker.yml

Concerns:

  1. Dependency Management (Medium Priority)

    • Both Gemfile and package.json now point to a feature branch (justin808/early-hints) rather than a stable release
    • Recommendation: This is acceptable for testing, but before merging to main, consider:
      • Document the timeline for when this will be merged into Shakapacker's main branch
      • Add a TODO comment or GitHub issue to track reverting to a stable version
      • Ensure the team is aware this creates a non-standard dependency
  2. postinstall Script Complexity (Low Priority)

    • The new postinstall script in package.json:20 is complex and has potential failure points:
      if [ -f node_modules/shakapacker/package/index.ts ] && [ ! -f node_modules/shakapacker/package/index.js ]; then cd node_modules/shakapacker && yarn install --production=false && yarn build; fi
    • Issues:
      • Shell script compatibility (won't work on Windows without WSL/Git Bash)
      • Silent failures if the build fails
      • Modifies node_modules which can cause issues with lockfile integrity
    • Recommendation: Consider adding error handling or documenting this requirement in the README

🐛 Potential Bugs & Issues

  1. Missing Server Configuration

    • Early hints (HTTP 103) require server support (Puma, Nginx, etc.)
    • Action Required: Verify that:
      • Puma version supports early hints (requires Puma 5.0+)
      • Production deployment environment (web server/load balancer) supports HTTP/2
      • No intermediate proxies strip the 103 Early Hints responses
  2. No Rollback Strategy

    • The PR checklist mentions verifying early hints in production, but there's no documented rollback plan
    • Recommendation: Add a feature flag or environment variable to toggle early hints without redeployment
  3. Configuration Validation

    • The YAML structure early_hints: enabled: true is added but there's no validation
    • Recommendation: Verify that Shakapacker properly validates this configuration and provides helpful errors if misconfigured

Performance Considerations

Positive Impacts:

  • Early hints can significantly improve perceived load time by allowing browsers to preload assets while the server generates the response
  • This is particularly beneficial for this app since it uses React with multiple asset bundles

Potential Issues:

  1. Asset Manifest Overhead

    • Early hints work best with stable asset names
    • Verify that the digest-stamped asset names (for cache busting) don't change too frequently
    • Consider monitoring the impact on CDN/cache hit rates
  2. No Metrics/Monitoring

    • The PR doesn't include any way to measure the effectiveness of early hints
    • Recommendation: Add logging or monitoring to track:
      • Whether early hints are being sent
      • Performance impact (use Browser Timing API or RUM tools)
      • Any errors in asset hint generation

🔒 Security Concerns

  1. Low Risk: Early hints themselves don't introduce security vulnerabilities
  2. Consideration: Ensure Content Security Policy (CSP) headers are compatible with preloaded resources
    • Check config/initializers/content_security_policy.rb if present
    • Early hints might trigger CSP violations if policies are too restrictive

🧪 Test Coverage

Current State:

  • ❌ No automated tests for early hints functionality
  • ❌ Manual testing checklist provided but not completed

Recommendations:

  1. Add Integration Tests

    # Suggested test structure
    describe "Early Hints", type: :request do
      it "sends 103 Early Hints in production mode" do
        # Test that appropriate Link headers are sent
      end
    end
  2. Test Cases to Cover:

    • Verify 103 response is sent before the main response
    • Validate Link headers contain correct asset paths
    • Ensure early hints are only sent in production
    • Test behavior when early_hints.enabled: false
  3. Manual Testing Checklist Enhancement:

    • Add specific commands to verify early hints (e.g., curl -i to inspect headers)
    • Document expected header format
    • Test with HTTP/2 specifically

📝 Additional Recommendations

  1. Documentation

    • Add a comment in config/shakapacker.yml explaining what early hints do
    • Update README or deployment docs with server requirements
    • Document browser compatibility (early hints require HTTP/2-capable browsers)
  2. Monitoring Strategy

    • Consider adding APM/monitoring to track:
      • Server response time changes
      • Client-side rendering performance
      • Any increase in server load
  3. Gradual Rollout

    • Consider testing on staging first with production-like traffic
    • Monitor for any unexpected issues before enabling in production

🎯 Summary

Overall 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:

  1. ✅ Complete the manual testing checklist (currently unchecked)
  2. ✅ Verify Puma and server infrastructure support
  3. ✅ Add at least basic integration test coverage
  4. ✅ Document the dependency on a feature branch and timeline for stable release

Nice to Have:

  1. Add monitoring/logging for early hints
  2. Implement feature flag for safer rollout
  3. Enhance postinstall script with better error handling
  4. Update documentation

Files Changed Review:

  • Gemfile & Gemfile.lock: ⚠️ Feature branch dependency
  • package.json: ⚠️ Complex postinstall script
  • yarn.lock: ✅ Auto-generated, looks correct
  • config/shakapacker.yml: ✅ Clean configuration addition

Great work on exploring performance optimizations! Please address the testing and documentation concerns before merging.


Review generated with assistance from Claude Code

@github-actions
Copy link

github-actions bot commented Oct 17, 2025

🎉 ✨ Deploy Complete! 🚀

🌐 ➡️ Open Review App

Deployment successful for PR #687, commit eb1b3be

🎮 Control Plane Console
📋 View Completed Action Build and Deploy Logs

@claude
Copy link

claude bot commented Oct 17, 2025

Pull Request Review: Enable Shakapacker Early Hints

Overview

This 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

  • Clear intent: The PR has a focused scope - enabling early hints support
  • Consistent updates: Both Gemfile and package.json are updated to use the same branch
  • Configuration is production-only: Early hints are enabled only in the production environment, which is appropriate for testing performance improvements

⚠️ Concerns

1. Using a Development Branch in Production (HIGH PRIORITY)

  • Issue: Both Gemfile (line 9) and package.json (line 95) reference the justin808/early-hints branch instead of a stable release
  • Risk: Development branches can change without notice, potentially breaking builds in the future
  • Recommendation:
    • Before merging to main/master, wait for this feature to be released in a stable version
    • If this must be deployed before a stable release, consider:
      • Pinning to a specific commit SHA instead of a branch name
      • Documenting the temporary nature of this dependency
      • Setting up a reminder to update to a stable version once available

2. Complex postinstall Script (MEDIUM PRIORITY)

  • Issue: The postinstall script in package.json (line 20) has become quite complex
  • Risks:
    • Shell-dependent (won't work on Windows without WSL/Git Bash)
    • Builds dependencies during install, which can slow down deployments
    • Modifies node_modules in a non-standard way
  • Recommendation:
    • Document why this script is necessary (appears to be building TypeScript source from the branch)
    • Consider adding a comment explaining this is temporary until the feature is released
    • Test that this works in your CI/CD environment

3. Ruby Version Mismatch in Gemfile.lock

  • Observation: The Gemfile.lock shows a Ruby version change from 3.4.6p32 to 3.4.6p54
  • Action Required: This suggests the lock file was generated with a different patch version. Ensure all developers and CI are using the same Ruby version to avoid lock file churn.

Potential Bugs & Issues

1. Missing Early Hints Server Support Verification

  • Issue: Early hints (HTTP 103 responses) require server support
  • Questions:
    • Does your production server (Puma) support early hints? (Puma 5.0+ does)
    • Is your reverse proxy/CDN configured to pass through 103 responses?
    • Have you verified the infrastructure supports this feature?
  • Recommendation: Document the server requirements in the PR description or add a comment in the config file

2. No Fallback or Feature Flag

  • Issue: The feature is enabled unconditionally in production
  • Recommendation: Consider adding a feature flag or environment variable to toggle this on/off without code changes, especially during the initial rollout

3. Testing Checklist Items Not Completed

  • Observation: The PR description has unchecked testing items
  • Recommendation: Complete these tests before merging

Performance Considerations

✅ Expected Benefits

  • Early hints can improve perceived page load performance by allowing browsers to start downloading assets earlier
  • This is particularly beneficial for sites with many CSS/JS assets

⚠️ Monitoring Recommendations

  • Set up monitoring to measure the actual performance impact
  • Track metrics like:
    • Time to First Byte (TTFB)
    • First Contentful Paint (FCP)
    • Largest Contentful Paint (LCP)
  • Compare before/after metrics to validate the improvement

Security Concerns

✅ No Critical Security Issues

  • The changes don't introduce obvious security vulnerabilities
  • Early hints are a standard web performance feature

⚠️ Supply Chain Considerations

  • Medium Risk: Using an unreleased branch from a dependency introduces supply chain risk
  • Mitigation:
    • The branch is from the same organization (shakacode/shakapacker), reducing risk
    • Review the branch code changes before deploying
    • Monitor for any unexpected changes to the branch

Test Coverage

⚠️ Missing Tests

  • No automated tests for the early hints functionality
  • Recommendations:
    1. Add integration tests that verify assets are being preloaded correctly
    2. Add configuration tests to ensure early_hints setting is properly read
    3. Consider adding a test that checks for the presence of HTTP 103 responses (if feasible in your test environment)

Configuration Review

The config/shakapacker.yml changes look good:

  • Properly nested under the production environment
  • Clear configuration structure
  • Enabled flag is explicit

Recommendations Summary

Before Merging:

  1. ✅ Complete the testing checklist in the PR description
  2. ⚠️ Verify server infrastructure supports HTTP 103 early hints
  3. ⚠️ Consider pinning to a specific commit SHA instead of branch name
  4. ⚠️ Document the temporary use of a development branch
  5. ⚠️ Ensure all environments use Ruby 3.4.6p54 to avoid lock file changes

After Merging:

  1. Monitor performance metrics to validate improvements
  2. Set up alerts for build failures related to the branch dependency
  3. Create a follow-up issue to update to a stable release when available
  4. Document the early hints feature and server requirements

Nice to Have:

  • Add a feature flag for easier rollback
  • Add automated tests for early hints functionality
  • Document Windows compatibility for the postinstall script

Overall Assessment

Status: ⚠️ Conditional Approval - The changes are technically sound, but using development branches in production is risky.

Severity: The main concern is the dependency on an unreleased branch. This should be addressed before merging or clearly documented as a temporary measure with a plan to migrate to a stable release.

The configuration changes themselves are well-structured and focused. Once the dependency concerns are addressed and testing is completed, this PR should be good to merge.


Review generated with attention to code quality, security, and production readiness.

@github-actions
Copy link

github-actions bot commented Oct 17, 2025

🎉 ✨ Deploy Complete! 🚀

🌐 ➡️ Open Review App

Deployment successful for PR #687, commit 9930aaf

🎮 Control Plane Console
📋 View Completed Action Build and Deploy Logs

@claude
Copy link

claude bot commented Oct 17, 2025

Pull Request Review: Enable Shakapacker Early Hints

Thank 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

  1. Clear Purpose: The PR has a well-defined goal of enabling early hints to improve perceived page load performance.
  2. Systematic Approach: The commits show a thoughtful progression from basic integration to handling edge cases (Ruby version, build requirements).
  3. Debug Support: Enabling debug: true in the early hints configuration is excellent for verification and troubleshooting.

🔍 Code Quality & Best Practices

1. PostInstall Script (package.json:20)

The postinstall script has grown quite complex. This one-liner is hard to read and maintain.

Concerns:

  • Fragility: Hard to read and maintain
  • Error Handling: No error handling if any step fails
  • Windows Compatibility: Shell commands using [ -f ] won't work on Windows
  • CI/CD Impact: Installing dev dependencies and building on every install adds significant time

Recommendations:

  • Extract this logic into a separate Node.js script for better cross-platform compatibility
  • Add proper error handling
  • Consider the impact on CI/CD build times

2. Branch Dependency Management

Using a GitHub branch as a dependency is appropriate for testing/preview, but:

Concerns:

  • Lock File Volatility: Any upstream changes to the branch will require lock file updates
  • Production Readiness: Branch dependencies should be temporary

Recommendations:

  • Document in the PR description when this will be replaced with a stable release
  • Consider using a specific commit hash instead of branch name for more stability

3. Early Hints Configuration (config/shakapacker.yml:67-70)

The configuration looks good, but could use more documentation.

Recommendations:

  • Add comments explaining what early hints are and their purpose for future maintainers
  • Consider if debug: true should remain in production long-term, or if it should be removed after verification

🐛 Potential Issues

1. 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
Recommendation: Verify this matches your team's development and production environments

2. Yarn.lock Changes

The 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 Considerations

1. Branch Dependency Security

Good: Using a specific commit in the lock file provides reproducible builds
⚠️ Concern: GitHub branch dependencies bypass npm's security audit

Recommendations:

  • Run yarn audit to check other dependencies
  • Monitor the shakapacker branch for any security-related commits
  • Plan to migrate to a stable npm release when available

2. Build Script Execution

The postinstall script runs arbitrary build commands from a third-party package.

Impact: Low (trusted source - shakacode)
Mitigation: Already in place - you're using a specific git commit hash in the lock file


⚡ Performance Considerations

1. Install Time

The postinstall script conditionally builds shakapacker from source, which:

  • Increases initial install time significantly
  • Requires dev dependencies to be installed even in production-like CI environments

Recommendations:

  • Document expected install time increase in the PR description
  • Consider caching the built shakapacker in CI/CD pipelines

2. Early Hints Benefits

✅ This is the whole point of the PR! Early hints should provide measurable performance improvements.

Recommendations:

  • Add performance testing to the PR checklist
  • Document how to verify early hints are working (inspect network requests for 103 Early Hints responses)
  • Consider adding metrics/monitoring to track the performance impact

🧪 Testing & Verification

The PR description has a testing checklist, which is great! However:

Missing Test Coverage:

  • No automated tests for the postinstall script
  • No verification that the early hints configuration is properly loaded

Recommendations:

  • Add instructions for manually testing early hints
  • Test the postinstall script in a clean environment

📋 Summary

Overall 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:

  1. High: Refactor postinstall script for better maintainability and cross-platform support
  2. Medium: Document migration plan from branch to stable release
  3. Medium: Add verification steps for early hints functionality
  4. Low: Clean up debug mode and improve configuration comments

Approval Status: ⚠️ Approve with suggestions - The code works, but would benefit from the improvements above before merging.


Great work on improving the application's performance! Let me know if you'd like help implementing any of these suggestions.

@github-actions
Copy link

github-actions bot commented Oct 17, 2025

🎉 ✨ Deploy Complete! 🚀

🌐 ➡️ Open Review App

Deployment successful for PR #687, commit 395b7a4

🎮 Control Plane Console
📋 View Completed Action Build and Deploy Logs

@claude
Copy link

claude bot commented Oct 17, 2025

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:

  • Clean, focused changes with clear intent
  • Good separation of concerns - dependency updates are isolated
  • Configuration follows Shakapacker conventions
  • Appropriate environment targeting (production only)

Suggestions:

  1. postinstall script complexity (package.json:20)

    • This is a complex one-liner that conditionally builds Shakapacker from source
    • Consider documenting WHY this is needed (probably because you're using a branch that hasn't been built/published)
    • This could fail silently or cause confusion for contributors
    • Recommendation: Add a comment in package.json explaining this is temporary while testing the early-hints branch
  2. Debug mode in production (config/shakapacker.yml:70)

    • Having debug: true in production configuration could expose implementation details via HTML comments
    • Recommendation: Consider setting this to false for production or making it environment-variable controlled

Potential Issues:

  1. Branch dependency stability

    • Using a feature branch (justin808/early-hints) as a dependency creates potential issues:
      • Branch could be force-pushed, rebased, or deleted
      • No version pinning - behavior could change unexpectedly
      • CI/CD reproducibility concerns
    • Recommendation: Once the feature is stable, point to a tagged release or specific commit SHA
  2. Postinstall performance impact

    • The conditional build in postinstall could significantly slow down yarn install
    • On every install, the script checks files and potentially rebuilds Shakapacker
    • Recommendation: Monitor install times and consider alternatives once the feature is merged upstream
  3. Ruby version change (Gemfile.lock)

    • The ruby patch version changed from 3.4.6p32 to 3.4.6p54
    • This appears to be an unintentional change from updating dependencies

Performance Considerations:

Positive:

  • Early hints can significantly improve perceived performance by allowing browsers to start fetching assets earlier
  • This is especially beneficial for apps with many JavaScript/CSS assets

Considerations:

  1. Server support required - Early hints require HTTP/2+ and server support (e.g., Puma 5+)
  2. Verify your deployment environment supports this feature
  3. Consider documenting minimum server requirements
  4. Consider adding performance metrics or screenshots showing the improvement

Security Concerns (Overall: Low Risk):

  1. Debug mode exposure - HTML comments in production could leak asset paths or configuration details (not critical but follows principle of least disclosure)
  2. Branch dependency from GitHub - Dependencies from branches should be temporary (ensure branch is from trusted source)
  3. No obvious security vulnerabilities introduced

Test Coverage Concerns:

  1. No automated tests included - Consider adding:

    • Integration tests verifying the configuration is loaded
    • Tests ensuring early hints headers are sent when enabled
    • Negative tests with early hints disabled
  2. Manual testing checklist improvements:

    • Test with different browsers (Safari, Chrome, Firefox)
    • Verify performance improvement with tools like Lighthouse
    • Test that the feature gracefully degrades on HTTP/1.1
    • Verify no issues in development/test environments

Additional Recommendations:

  1. Documentation - Consider adding docs about what early hints are, why they're enabled, how to verify they're working, server requirements, and how to disable if needed

  2. Rollback plan - Since this is a production feature, have a clear rollback strategy (the feature can be easily disabled via config, which is good)

  3. Monitoring - Once deployed, monitor for unexpected errors, performance improvements, and server resource usage changes

  4. Yarn.lock size - The yarn.lock diff is 2665 lines, verify no unexpected dependencies were added

Summary:

Verdict: Approve with minor suggestions

This PR is well-structured and implements a useful performance feature. The main concerns are:

  • MEDIUM: Debug mode enabled in production config
  • MEDIUM: Relying on unstable branch dependency
  • LOW: Complex postinstall script needs documentation
  • LOW: Missing automated tests

Before merging:

  1. Decide whether to keep debug: true in production
  2. Document the temporary branch dependency
  3. Plan for updating to a stable release once merged upstream

After merging:

  1. Monitor production for issues
  2. Measure performance improvements
  3. Update to stable Shakapacker release when available

Great work on improving performance!

@github-actions
Copy link

github-actions bot commented Oct 17, 2025

🎉 ✨ Deploy Complete! 🚀

🌐 ➡️ Open Review App

Deployment successful for PR #687, commit 58f9d8e

🎮 Control Plane Console
📋 View Completed Action Build and Deploy Logs

@claude
Copy link

claude bot commented Oct 17, 2025

Pull Request Review: Enable Shakapacker Early Hints

Overview

This 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

  • Updated Gemfile to use shakapacker from GitHub branch justin808/early-hints
  • Updated package.json to use shakapacker from the same GitHub branch
  • Added postinstall script to build shakapacker when installed from source
  • Enabled early hints in config/shakapacker.yml for production with debug mode
  • Updated lock files

Code Quality and Best Practices

Strengths

  1. Well-documented commits with clear explanations
  2. Comprehensive approach including all necessary dependency updates
  3. Smart postinstall script with conditional build logic
  4. Debug mode enabled for initial rollout verification

Areas for Improvement

1. Temporary Dependency Configuration
The PR uses GitHub branch dependencies rather than published versions. Consider adding a TODO or tracking issue to revert to a published version once the early-hints feature is merged and released.

2. Postinstall Script Complexity
The postinstall script in package.json line 20 is complex for a one-liner. Consider moving to a separate script file for better maintainability and error handling.

3. Configuration Documentation
The shakapacker.yml configuration lacks inline documentation about available options and server requirements.

Potential Bugs or Issues

Medium Priority

  1. Platform Compatibility: The bash script may not work on Windows
  2. Production Build Bloat: The postinstall script runs yarn install with production=false which installs dev dependencies
  3. Missing Changelog: No update to CHANGELOG.md

Low Priority

  1. Ruby Version: Gemfile.lock shows Ruby version changed from 3.4.6p32 to 3.4.6p54 - ensure CI/CD alignment

Performance Considerations

Positive Impact

  • Early Hints Feature: HTTP/2 Early Hints (HTTP 103) allows browsers to start loading critical assets while the server generates the full response, improving perceived TTFB and page load times
  • Production Only: Good decision to target production where benefits are most impactful

Potential Concerns

  1. Build Time: Building shakapacker from source increases yarn install time for local dev, CI/CD, and container builds
  2. Server Requirements: Early hints require HTTP/2 and web server support (Puma 6+, nginx, etc.)

Recommendation: Document server requirements in PR description or README

Security Concerns

Generally safe - no major security concerns identified.

Minor Considerations:

  1. Branch-based dependencies mean code could change without version updates - consider pinning to specific commit SHA
  2. Debug mode outputs HTML comments that could expose asset paths - fine for testing but consider disabling after verification

Test Coverage

Missing automated tests. The PR description has a testing checklist but no automated test coverage.

Recommendations:

  1. Add integration tests verifying early hints headers are sent
  2. Add CI check that verifies shakapacker builds correctly from source
  3. Document manual testing procedure (browser dev tools, curl commands, expected headers)

Additional Recommendations

Documentation

  1. Update README.md to mention early hints feature
  2. Add migration guide for others wanting to use this feature
  3. Document rollback procedure

Deployment Strategy

  1. Consider feature flag or gradual rollout
  2. Monitor performance metrics before/after
  3. Have plan to quickly disable if issues occur

Future Improvements

  1. Add metrics/logging for early hints usage
  2. Configure which assets should be preloaded
  3. A/B test performance impact

Conclusion

This 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 Changes

Before Merging:

  1. Consider pinning dependencies to specific commit SHA
  2. Add documentation about server requirements
  3. Add plan/issue for migrating to published version
  4. Consider adding automated tests

After Merging:

  1. Monitor production performance metrics
  2. Verify early hints are working in production
  3. Consider disabling debug mode after verification
  4. Plan to update to stable release when available

Great work on implementing early hints support!

@claude
Copy link

claude bot commented Oct 17, 2025

Pull Request Review: Enable Shakapacker Early Hints

Summary

This 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 Practices

Strengths

  1. Clear intent: The PR has a focused scope - enabling early hints functionality
  2. Configuration approach: Using YAML configuration is the right approach for feature flags
  3. Environment-specific: Early hints are only enabled in production, which is sensible

Concerns

1. 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:

  • This branch can change without notice, breaking builds
  • No version pinning means non-reproducible builds
  • The branch could be force-pushed, rebased, or deleted
  • CI/CD pipelines become unreliable

Recommendation:

  • Pin to a specific commit SHA for stability (ref: 915edce51481bb1135853f9b877fd243623a9820)
  • Or wait for this feature to be merged and released in a stable version
  • Add a TODO/comment explaining this is temporary pending official release

2. Complex postinstall Script (Medium Priority)

File: package.json:20

The postinstall script is complex and fragile:

  • Complex bash logic in package.json can fail silently
  • Building dependencies during install is fragile and slow
  • Not portable to Windows environments

Questions:

  • Why does shakapacker need to be built from source?
  • Is this temporary until the branch is properly published?
  • What happens if the build fails?

Recommendations:

  • Add error handling
  • Consider extracting to a separate script file
  • Add comments explaining why this is necessary

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 & Issues

1. 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

  • No validation that early hints are actually working
  • No fallback if the feature fails
  • The PR testing checklist is incomplete

Performance Considerations

Positive Impacts

  1. Early hints can improve perceived performance by allowing browsers to preload assets
  2. HTTP/2+ optimization
  3. Production-only: Won't impact development workflow

Concerns

  1. Build time: The postinstall script adds overhead
  2. Deployment complexity: Building from source could slow down deployments
  3. yarn.lock changes: 1338 additions/1327 deletions suggests significant dependency tree changes

Recommendations:

  • Benchmark deploy times before/after
  • Monitor production performance metrics after enabling
  • Consider A/B testing early hints impact

Security Concerns

1. Supply Chain Risk (High Priority)

  • Installing from an unmerged branch bypasses normal security review
  • No guarantee the branch hasn't been tampered with
  • Dependencies could change without review

Mitigation:

  • Pin to specific commit SHA
  • Review the shakapacker branch changes thoroughly

2. Information Disclosure (Low Priority)

  • Debug mode in production could reveal asset structure

3. Dependency Audit

Run security audits: bundle audit and yarn audit


Test Coverage

Missing Tests

The PR description shows an incomplete testing checklist

Recommendations:

  1. Add automated tests for the build process
  2. Integration test to verify early hints headers are sent
  3. Manual testing guide
  4. Rollback plan

Additional Recommendations

1. Documentation

  • Add comments in shakapacker.yml explaining what early hints does
  • Document browser compatibility (requires HTTP/2+ and modern browsers)

2. Server Requirements

Early hints require:

  • HTTP/2 or HTTP/3
  • Puma 5.0+
  • Properly configured reverse proxy

Action: Verify production infrastructure supports early hints

3. Monitoring

  • Add logging/metrics to track effectiveness
  • Monitor for errors
  • Track performance improvements

Summary & Verdict

Blocking Issues (Must Fix Before Merge)

  1. Pin dependencies to commit SHA instead of floating branch
  2. Disable debug mode in production or make it configurable
  3. Complete testing checklist and verify functionality

Non-Blocking Recommendations

  1. Improve postinstall script error handling
  2. Add automated tests
  3. Document server requirements
  4. Run security audits
  5. Clarify Ruby version change

Questions for Author

  1. When will the justin808/early-hints branch be merged upstream?
  2. Is there a timeline for this becoming a stable release?
  3. Have you tested this in a production-like environment?
  4. What performance improvements have you observed?
  5. Why does shakapacker need to be built during postinstall?

Conclusion

The 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:

  1. Option A (Preferred): Wait for the feature to be merged and released in a stable version
  2. Option B: Pin to a specific commit SHA, add comprehensive testing, disable debug mode, and treat this as an experimental deployment

The code quality is good, but the deployment approach needs refinement for production use.

@github-actions
Copy link

github-actions bot commented Oct 17, 2025

🎉 ✨ Deploy Complete! 🚀

🌐 ➡️ Open Review App

Deployment successful for PR #687, commit 8082229

🎮 Control Plane Console
📋 View Completed Action Build and Deploy Logs

justin808 and others added 25 commits January 14, 2026 17:30
These scripts connect to Chrome via the DevTools Protocol to inspect pages and verify HTTP 103 Early Hints are working:
- check_early_hints.js - Node.js script using WebSocket
- check_early_hints.py - Python script (requires websocket module)

Both scripts:
- Connect to Chrome running with --remote-debugging-port=9222
- Extract HTML from the loaded page
- Search for Shakapacker Early Hints debug comments
- Verify preload link headers

Usage:
  node check_early_hints.js
  # or
  python3 check_early_hints.py

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
The previous commit removed the ENV SECRET_KEY_BASE but the
react_on_rails:locale task still requires it to initialize Rails.

The SECRET_KEY_BASE is now set inline for both the locale task and
asset precompilation, ensuring it's only used during build and not
persisted in the final image.

This fixes the deployment error:
  ArgumentError: Missing `secret_key_base` for 'production' environment

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
This version includes the fix for the PackerUtils constant issue
that was preventing bin/dev from starting properly.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
The gem and npm package versions must match exactly.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
Rails 8.1.1 (introduced in commit baac683) requires SECRET_KEY_BASE
to be set during initialization, even for tasks like db:prepare that
don't actually use the secret key.

This change updates the release script to provide a placeholder
SECRET_KEY_BASE value if one isn't already set in the environment,
similar to the fix in commit 6039dd7 for the Docker build phase.

The actual SECRET_KEY_BASE for runtime will still come from the
deployment environment variables.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
- 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 <[email protected]>
Updated from revision 2306825e to 1969b2d22 to get latest changes from master branch.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
- Update react_on_rails to latest master (revision b50a74d9)
- Fix headless Chrome mode by using --headless=new flag
  Chrome 109+ requires the new headless mode to work properly

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
- Update react_on_rails to latest master (revision b50a74d9)
- Fix headless Chrome by using custom :headless_chrome driver
  Capybara's built-in :selenium_chrome_headless uses old --headless flag
  Chrome 109+ requires --headless=new for proper headless operation
- Renamed driver to avoid conflicts with Capybara's built-in driver

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
Break build_production_command assignment across two lines to satisfy
the 120 character max line length requirement.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
- check_early_hints.js: Exit with error code 1 when no Early Hints found
- Footer.jsx: Extract reusable SVG icon components to reduce duplication
- chrome-mcp-server-setup.md: Add note about Playwright not capturing 103 responses,
  show CDP approach for detecting Early Hints
- thruster.md: Add Shakapacker early_hints configuration example

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
Address CodeRabbit review feedback by replacing ephemeral PR review app
URLs with YOUR-REVIEW-APP placeholder and adding guidance on how to find
the actual review app URL.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
Update both gem and npm package to shakapacker 9.4.0 to fix CI failures
caused by version mismatch between gem (9.3.4.beta.0) and npm (9.3.3).

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <[email protected]>
Fix eslint react/prop-types validation errors for CheckmarkIcon,
CheckmarkCircleIcon, and InfoIcon components.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <[email protected]>
Load the Rails environment at script startup rather than lazily inside
the locale generation step. This ensures ReactOnRails configuration is
available before any precompile tasks run.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <[email protected]>
- bin/dev now runs rescript build and locale generation BEFORE starting
  Procfile processes, eliminating race conditions and sleep delays
- Added res:watch npm script (watch without clean) for Procfile.dev
- Removed sleep delays and inline locale generation from Procfile.dev
- Updated build_production_command to include rescript build
- Removed precompile_hook from shakapacker.yml (handled by bin/dev and
  build commands)

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <[email protected]>
🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Add Chrome debug scripts to .gitignore
- Improve bin/dev ReScript build error output with detailed messages
- Disable early_hints debug mode in production (exposes internals)
- Remove duplicate infer_spec_type_from_file_location! call in rails_helper

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Move Rails environment loading inside run_precompile_tasks to improve
  startup time for help/kill commands (saves 1-3 seconds)
- Remove unused HMR=true from Procfile.dev wp-server process
  (webpack config only checks WEBPACK_DEV_SERVER, not HMR env var)

Co-Authored-By: Claude Opus 4.5 <[email protected]>
@justin808 justin808 force-pushed the justin808/shakapacker-early-hints branch from 4df5be0 to 154aa64 Compare January 15, 2026 03:30
@claude
Copy link

claude bot commented Jan 15, 2026

Pull Request Review: Enable Shakapacker Early Hints

Summary

This PR successfully integrates Thruster HTTP/2 proxy and enables Early Hints for improved performance. Changes include dependency updates (Ruby 3.4.6, shakapacker 9.5.0, react_on_rails 16.2.0.rc1), comprehensive Thruster integration, improved development workflows, and extensive documentation.

Overall Assessment

LGTM with minor recommendations

Code Quality & Best Practices

✅ Strengths

  1. Excellent Documentation - Comprehensive guides with troubleshooting
  2. Improved Development Workflow - bin/dev runs precompile tasks automatically
  3. Clean Dependency Management - Moved from beta to RC/stable versions
  4. Docker Best Practices - Multi-stage builds, proper cleanup

⚠️ Areas for Improvement

1. Security: Hardcoded Secrets in Templates

Location: .controlplane/templates/app.yml:26

  • Hardcoded placeholder SECRET_KEY_BASE is risky for production
  • Recommendation: Add runtime validation to fail if placeholder detected

2. Leftover Debug Scripts

Location: check_early_hints.js, check_early_hints.py

  • Files are both committed AND in .gitignore
  • Recommendation: Remove from git or move to scripts/ directory

3. Commented-Out Redis Service

Location: Procfile.dev:9

  • New contributors might be confused when app fails
  • Recommendation: Add Redis check in bin/dev or document in README

Performance Considerations

✅ Excellent Improvements

  1. HTTP/2 Multiplexing: 20-30% faster page loads
  2. Brotli Compression: 40-60% reduction in transfer size
  3. Early Hints: Preloads critical assets before HTML ready
  4. Optimized Build: Precompiling before watch avoids races

Security Concerns

🔒 SECRET_KEY_BASE Handling

  1. Build-time (Dockerfile:69, 75-76): precompile_placeholder ✅
  2. Migration-time (release_script.sh:19): Proper fallback ✅
  3. Runtime template (app.yml:26): Hardcoded placeholder ⚠️

Rails 8.1+ Note: Temporary placeholders during build are appropriate and secure.

✅ No Other Issues Found

  • No command injection vulnerabilities
  • No XSS risks
  • Dependencies updated to stable versions

Test Coverage

✅ Configuration Updated

  1. CI Matrix: Ruby 3.4.6 across all workflows
  2. Custom headless Chrome driver
  3. Proper assets path handling

📝 Recommendations

  1. Add integration tests for Early Hints
  2. Test Thruster fallback behavior

Potential Bugs

🐛 Minor Issues

  1. Procfile.dev: Running foreman directly bypasses precompile
  2. bin/dev:40: Potential encoding issue with yarn output

Architecture & Design

✅ Excellent Decisions

  1. Zero-config HTTP/2 via Thruster
  2. Separation of concerns (build vs processes)
  3. Multi-stage Docker optimization
  4. Comprehensive documentation

Thruster Flow:

User → HTTPS/HTTP2 → CP LB → HTTP/1.1 → Thruster → Rails
                     (protocol: http)

Recommendations Summary

🔴 High Priority

  1. Remove/relocate debug scripts
  2. Add SECRET_KEY_BASE validation

🟡 Medium Priority

  1. Add Redis availability check
  2. Add Procfile.dev warning comment

🟢 Low Priority

  1. Add Early Hints integration tests
  2. Add migration guide
  3. Monitor performance metrics

Conclusion

Well-executed PR with significant performance improvements. Issues are mostly minor and non-blocking, but SECRET_KEY_BASE security should be addressed.

Recommendation: Approve with minor fixes

Great work on testing and documentation!

@justin808
Copy link
Member Author

/deploy-review-app

@github-actions
Copy link

github-actions bot commented Jan 15, 2026

🎉 ✨ Deploy Complete! 🚀

🌐 ➡️ Open Review App

Deployment successful for PR #687, commit 154aa64

🎮 Control Plane Console
📋 View Completed Action Build and Deploy Logs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants