Add Shakapacker precompile hook with ReScript support to Pro dummy app #1977
Conversation
|
Warning Rate limit exceeded@justin808 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 22 minutes and 4 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (5)
WalkthroughReplaced Changes
Sequence Diagram(s)sequenceDiagram
participant Shakapacker as Shakapacker
participant Hook as bin/shakapacker-precompile-hook
participant FS as Filesystem
participant Node as Yarn/NPM
participant Rails as Rails env
participant ROR as react_on_rails:generate_packs
Shakapacker->>Hook: invoke precompile hook
Hook->>FS: find_rails_root (traverse up to config/environment.rb)
alt ReScript config present
Hook->>Node: run yarn or npm build:rescript
Node-->>Hook: success / failure
end
Hook->>Rails: load environment (if needed) and check initializer flags
alt auto_load_bundle or components_subdirectory enabled
Hook->>ROR: run react_on_rails:generate_packs
ROR-->>Hook: success / failure
end
alt error
Hook->>Shakapacker: exit 1 (log error & backtrace)
else
Hook->>Shakapacker: exit 0
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Code Review: Fix Pro dummy app Playwright test timeouts✅ Overall AssessmentThis is a well-structured fix that correctly addresses the Playwright test timeout issue by aligning the Pro dummy app with the open-source configuration from PR #1972. The changes are consistent, well-documented, and follow established patterns. 📋 Summary of Changes
✅ Strengths1. Root Cause Analysis
2. Code Quality
3. Documentation
4. Consistency
🔍 Detailed Reviewapplication.html.erb (lines 24-31)✅ Good: Updated comment accurately describes the defer strategy purpose shakapacker.yml (lines 22-25)✅ Excellent: Includes security warning about precompile hook bin/shakapacker-precompile-hook (entire file)✅ Excellent: Robust implementation with multiple safety checks Specific highlights:
🐛 Potential IssuesNone identified - The code is production-ready. 🔒 Security Review✅ Command injection prevention: Uses array form for all ⚡ Performance Considerations✅ Minimal overhead: Early returns prevent unnecessary work 🧪 Test CoverageTest Plan Status: Recommendations for testing:
💡 Suggestions1. Test Plan Completion 2. Verification Script (optional) # In CI or locally
RAILS_ENV=test bin/shakapacker-precompile-hook
echo "Hook exit code: $?"3. Documentation (optional, future work) 📝 Checklist
🎯 RecommendationLGTM after CI passes ✅ This is a solid fix that:
Once the Playwright tests pass in CI, this is ready to merge. 📚 References
Review generated with assistance from Claude Code |
![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)
react_on_rails_pro/spec/dummy/bin/shakapacker-precompile-hook (1)
72-78: Surface skip reason when Bundler is unavailable. Right now wereturnsilently ifbundle_availableis false (or later when the task is missing), so folks won’t know packs were skipped. Consider emitting awarnbefore those early returns to make the hook’s behavior explicit during CI troubleshooting.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
react_on_rails_pro/spec/dummy/app/views/layouts/application.html.erb(1 hunks)react_on_rails_pro/spec/dummy/bin/shakapacker-precompile-hook(1 hunks)react_on_rails_pro/spec/dummy/config/shakapacker.yml(1 hunks)
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1781
File: node_package/src/ClientSideRenderer.ts:82-95
Timestamp: 2025-09-15T21:24:48.207Z
Learning: In React on Rails, the force_load feature includes both explicit `data-force-load="true"` usage and the ability to hydrate components during the page loading state (`document.readyState === 'loading'`). Both capabilities require a Pro license, so the condition `!railsContext.rorPro && (isComponentForceLoaded || document.readyState === 'loading')` correctly gates both scenarios.
Learnt from: justin808
Repo: shakacode/react_on_rails PR: 1770
File: lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.jsx:2-2
Timestamp: 2025-09-16T08:01:11.146Z
Learning: React on Rails uses webpack CSS Modules configuration with namedExports: true, which requires the import syntax `import * as style from './file.module.css'` rather than the default export pattern. This configuration enables better tree shaking and bundle size optimization for CSS modules.
Learnt from: alexeyr-ci2
Repo: shakacode/react_on_rails PR: 1732
File: spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx:40-44
Timestamp: 2025-04-26T21:55:55.874Z
Learning: In the react_on_rails project, files under `app-react16` directories are copied/moved to corresponding `/app` directories during the conversion process (removing the `-react16` suffix), which affects their relative import paths at runtime.
Learnt from: Romex91
Repo: shakacode/react_on_rails PR: 1697
File: package-scripts.yml:28-28
Timestamp: 2025-02-12T16:38:06.537Z
Learning: The file `node_package/lib/ReactOnRails.full.js` is autogenerated during the build process and should not be present in the repository.
Learnt from: theforestvn88
Repo: shakacode/react_on_rails PR: 1620
File: spec/dummy/client/app/startup/HelloTurboStream.jsx:3-3
Timestamp: 2024-07-27T10:08:35.868Z
Learning: The `RailsContext` import in `spec/dummy/client/app/startup/HelloTurboStream.jsx` is used later in the project, as clarified by the user theforestvn88.
Learnt from: theforestvn88
Repo: shakacode/react_on_rails PR: 1620
File: spec/dummy/client/app/startup/HelloTurboStream.jsx:3-3
Timestamp: 2024-10-08T20:53:47.076Z
Learning: The `RailsContext` import in `spec/dummy/client/app/startup/HelloTurboStream.jsx` is used later in the project, as clarified by the user theforestvn88.
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1745
File: node_package/src/RSCRequestTracker.ts:8-14
Timestamp: 2025-07-08T05:57:29.630Z
Learning: The global `generateRSCPayload` function in React on Rails Pro (RORP) is provided by the framework during rendering requests, not implemented in application code. The `declare global` statements are used to document the expected interface that RORP will inject at runtime.
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: lib/react_on_rails/helper.rb:190-197
Timestamp: 2025-02-18T13:08:01.477Z
Learning: RSC support validation in React on Rails Pro is handled through a chain of validations:
1. Pro version check in `run_stream_inside_fiber`
2. RSC support check during pack generation via `ReactOnRailsPro.configuration.enable_rsc_support`
3. RSC support validation during component registration
This makes additional validation in the helper methods unnecessary.
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/clientStartup.ts:18-21
Timestamp: 2025-02-13T16:50:47.848Z
Learning: In the react_on_rails module, the `reactOnRailsPageUnloaded` function in clientStartup.ts is intentionally kept private as it's only used internally as a callback for `onPageUnloaded`.
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1696
File: node_package/src/RSCPayloadContainer.ts:0-0
Timestamp: 2025-04-09T12:56:10.756Z
Learning: In the react_on_rails codebase, RSC payloads are already stringified using `JSON.stringify()` before being processed by the `escapeScript` function, which handles escaping of special characters. The function only needs to handle specific HTML markers like comments and closing script tags.
📚 Learning: 2025-09-16T08:01:11.146Z
Learnt from: justin808
Repo: shakacode/react_on_rails PR: 1770
File: lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.jsx:2-2
Timestamp: 2025-09-16T08:01:11.146Z
Learning: React on Rails uses webpack CSS Modules configuration with namedExports: true, which requires the import syntax `import * as style from './file.module.css'` rather than the default export pattern. This configuration enables better tree shaking and bundle size optimization for CSS modules.
Applied to files:
react_on_rails_pro/spec/dummy/app/views/layouts/application.html.erbreact_on_rails_pro/spec/dummy/bin/shakapacker-precompile-hookreact_on_rails_pro/spec/dummy/config/shakapacker.yml
📚 Learning: 2025-04-26T21:55:55.874Z
Learnt from: alexeyr-ci2
Repo: shakacode/react_on_rails PR: 1732
File: spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx:40-44
Timestamp: 2025-04-26T21:55:55.874Z
Learning: In the react_on_rails project, files under `app-react16` directories are copied/moved to corresponding `/app` directories during the conversion process (removing the `-react16` suffix), which affects their relative import paths at runtime.
Applied to files:
react_on_rails_pro/spec/dummy/app/views/layouts/application.html.erbreact_on_rails_pro/spec/dummy/bin/shakapacker-precompile-hook
📚 Learning: 2025-02-12T16:38:06.537Z
Learnt from: Romex91
Repo: shakacode/react_on_rails PR: 1697
File: package-scripts.yml:28-28
Timestamp: 2025-02-12T16:38:06.537Z
Learning: The file `node_package/lib/ReactOnRails.full.js` is autogenerated during the build process and should not be present in the repository.
Applied to files:
react_on_rails_pro/spec/dummy/app/views/layouts/application.html.erbreact_on_rails_pro/spec/dummy/bin/shakapacker-precompile-hook
📚 Learning: 2024-07-27T10:08:35.868Z
Learnt from: theforestvn88
Repo: shakacode/react_on_rails PR: 1620
File: spec/dummy/client/app/startup/HelloTurboStream.jsx:3-3
Timestamp: 2024-07-27T10:08:35.868Z
Learning: The `RailsContext` import in `spec/dummy/client/app/startup/HelloTurboStream.jsx` is used later in the project, as clarified by the user theforestvn88.
Applied to files:
react_on_rails_pro/spec/dummy/app/views/layouts/application.html.erb
📚 Learning: 2025-09-15T21:24:48.207Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1781
File: node_package/src/ClientSideRenderer.ts:82-95
Timestamp: 2025-09-15T21:24:48.207Z
Learning: In React on Rails, the force_load feature includes both explicit `data-force-load="true"` usage and the ability to hydrate components during the page loading state (`document.readyState === 'loading'`). Both capabilities require a Pro license, so the condition `!railsContext.rorPro && (isComponentForceLoaded || document.readyState === 'loading')` correctly gates both scenarios.
Applied to files:
react_on_rails_pro/spec/dummy/app/views/layouts/application.html.erb
📚 Learning: 2025-07-08T05:57:29.630Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1745
File: node_package/src/RSCRequestTracker.ts:8-14
Timestamp: 2025-07-08T05:57:29.630Z
Learning: The global `generateRSCPayload` function in React on Rails Pro (RORP) is provided by the framework during rendering requests, not implemented in application code. The `declare global` statements are used to document the expected interface that RORP will inject at runtime.
Applied to files:
react_on_rails_pro/spec/dummy/app/views/layouts/application.html.erbreact_on_rails_pro/spec/dummy/bin/shakapacker-precompile-hook
📚 Learning: 2025-02-13T16:50:26.861Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/turbolinksUtils.ts:34-36
Timestamp: 2025-02-13T16:50:26.861Z
Learning: In React on Rails, when checking for Turbolinks version 5 using `turbolinksVersion5()`, always ensure `Turbolinks` exists first by checking `turbolinksInstalled()` to prevent TypeError when accessing properties.
Applied to files:
react_on_rails_pro/spec/dummy/app/views/layouts/application.html.erb
📚 Learning: 2025-02-13T16:50:47.848Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/clientStartup.ts:18-21
Timestamp: 2025-02-13T16:50:47.848Z
Learning: In the react_on_rails module, the `reactOnRailsPageUnloaded` function in clientStartup.ts is intentionally kept private as it's only used internally as a callback for `onPageUnloaded`.
Applied to files:
react_on_rails_pro/spec/dummy/app/views/layouts/application.html.erb
📚 Learning: 2025-04-09T12:56:10.756Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1696
File: node_package/src/RSCPayloadContainer.ts:0-0
Timestamp: 2025-04-09T12:56:10.756Z
Learning: In the react_on_rails codebase, RSC payloads are already stringified using `JSON.stringify()` before being processed by the `escapeScript` function, which handles escaping of special characters. The function only needs to handle specific HTML markers like comments and closing script tags.
Applied to files:
react_on_rails_pro/spec/dummy/app/views/layouts/application.html.erb
🔇 Additional comments (2)
react_on_rails_pro/spec/dummy/app/views/layouts/application.html.erb (1)
27-33: Defer attribute restores hydration order. Switching todefermirrors the OSS dummy fix and guarantees component packs register before hydration—looks solid.react_on_rails_pro/spec/dummy/config/shakapacker.yml (1)
22-25: Hook config lines up with the automation. Wiring the precompile hook here cleanly connects the new script into pack generation flow.
Code Review: PR #1977OverviewThis PR successfully addresses Playwright E2E test timeouts in the Pro dummy app by aligning with the defer loading strategy fix from PR #1972. The changes are well-documented and the root cause analysis is thorough. ✅ Strengths1. Excellent Problem Analysis
2. Consistent ImplementationThe 3. Robust Hook ScriptThe precompile hook demonstrates several best practices:
4. Security AwarenessThe 🔍 Code Quality ObservationsPrecompile Hook Script (
|
Code Review - PR #1977SummaryThis PR successfully addresses Playwright E2E test timeouts in the Pro dummy app by aligning it with the defer loading strategy from PR #1972. The changes are well-motivated, properly documented, and follow established patterns from the open-source codebase. Positive Aspects1. Excellent Problem Analysis ✅The PR description clearly explains:
2. Code Consistency ✅
3. Security Awareness ✅The # SECURITY: Only reference trusted scripts within your project. Ensure the hook path
# points to a file within the project root that you control.4. Robust Hook Implementation ✅The precompile hook demonstrates good practices:
Observations & Minor Suggestions1. Comment Update in Layout
|
justin808
force-pushed
the
justin808/playwright-streaming-timeout
branch
from
c8be1f5 to
36d768b
Compare
justin808
force-pushed
the
justin808/playwright-streaming-timeout
branch
from
36d768b to
cbb7df2
Compare
Code Review: PR #1977OverviewThis PR fixes Playwright E2E test timeouts in the Pro dummy app by applying the same ✅ Strengths
🔍 Issues & Recommendations1. Security: Command Injection Risk (Medium Priority)Location: success = if yarn_available
system("yarn", "build:rescript")
elsif npm_available
system("npm", "run", "build:rescript")Issue: The script uses the array form of Recommendation: Add a comment explaining the security consideration: # Use array form to prevent shell injection
success = if yarn_available
system("yarn", "build:rescript")2. Error Handling: Silent Failures (Low Priority)Location: else
warn "⚠️ Warning: Neither yarn nor npm found. Skipping ReScript build."
returnIssue: When neither package manager is found, the script warns but continues. If ReScript is required, this could lead to broken builds that fail later. Recommendation: Consider making this configurable or failing fast if ReScript config exists: else
warn "❌ Error: ReScript config found but no package manager available"
exit 1
end3. Code Quality: Complex Regex (Low Priority)Location: has_auto_load = config_file =~ /^\s*(?\!#).*config\.auto_load_bundle\s*=/
has_components_subdir = config_file =~ /^\s*(?\!#).*config\.components_subdirectory\s*=/Issue: The negative lookahead Recommendation: Use a more robust regex: # Match lines that don't start with # (after optional whitespace)
has_auto_load = config_file =~ /^\s*[^#]*config\.auto_load_bundle\s*=/
has_components_subdir = config_file =~ /^\s*[^#]*config\.components_subdirectory\s*=/Or use Ruby parsing: has_auto_load = config_file.lines.any? { |line| line.strip[0] \!= '#' && line.include?('config.auto_load_bundle =') }4. Missing Test Coverage (Medium Priority)Issue: No tests for the precompile hook script. Given its critical role in the build process, it should have unit tests. Recommendation: Add tests in
5. Documentation: Shakapacker Version (Low Priority)Location: The comment references Shakapacker docs, but doesn't specify the minimum version required for Recommendation: Add version requirement to the comment: # Hook to run before webpack compilation (requires Shakapacker >= X.X.X)🧪 Testing ConsiderationsThe PR description mentions a test plan but doesn't show completed checkboxes. Before merging:
🎯 Performance ConsiderationsPositive: Using Neutral: The precompile hook adds build time overhead, but this is necessary for auto-generated packs. 📋 SummaryApprove with minor suggestions. The core fix is sound and well-implemented. The issues identified are mostly improvements rather than blockers:
The PR successfully addresses the test timeout issue and aligns the Pro dummy app with the open-source configuration. Great work documenting the root cause and solution! 📚 References
|
Code Review for PR #1977Thank you for this PR! The changes effectively address the Playwright test timeout issue by aligning the Pro dummy app with the open-source configuration. Here's my detailed review: ✅ Strengths1. Root Cause Analysis is SolidThe PR correctly identifies the race condition caused by 2. Consistency Across CodebaseAligning the Pro dummy app with the open-source dummy app (from #1972) ensures consistent behavior and reduces maintenance burden. 3. Comprehensive SolutionThe PR includes all necessary pieces:
4. Good DocumentationThe inline comments explain why 🔍 Observations & Suggestions1. Code Duplication: Precompile HookThe Pro version (line 75): task_list = IO.popen(["bundle", "exec", "rails", "-T"], err: %i[child out], &:read)Open-source version (line 76): task_list = IO.popen(["bundle", "exec", "rails", "-T"], err: [:child, :out], &:read)The Pro version uses Suggestion: Consider one of these approaches to reduce duplication:
2. RuboCop Violation in Pro VersionThe open-source version has a RuboCop disable comment for Action Required: # Add these lines to react_on_rails_pro/spec/dummy/bin/shakapacker-precompile-hook
# rubocop:disable Metrics/CyclomaticComplexity
def generate_packs_if_needed
# ... existing code ...
end
# rubocop:enable Metrics/CyclomaticComplexity3. Security Note is Well-PlacedThe security comment in 4. Test CoverageThe PR description mentions test timeouts were the issue, and the fix ensures proper hydration. However:
5. Comment Accuracy in
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
react_on_rails_pro/spec/dummy/app/views/layouts/application.html.erb(1 hunks)react_on_rails_pro/spec/dummy/bin/shakapacker-precompile-hook(1 hunks)react_on_rails_pro/spec/dummy/config/initializers/react_on_rails.rb(1 hunks)react_on_rails_pro/spec/dummy/config/shakapacker.yml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- react_on_rails_pro/spec/dummy/config/initializers/react_on_rails.rb
🚧 Files skipped from review as they are similar to previous changes (2)
- react_on_rails_pro/spec/dummy/app/views/layouts/application.html.erb
- react_on_rails_pro/spec/dummy/config/shakapacker.yml
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: justin808
Repo: shakacode/react_on_rails PR: 1770
File: lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.jsx:2-2
Timestamp: 2025-09-16T08:01:11.146Z
Learning: React on Rails uses webpack CSS Modules configuration with namedExports: true, which requires the import syntax `import * as style from './file.module.css'` rather than the default export pattern. This configuration enables better tree shaking and bundle size optimization for CSS modules.
Learnt from: alexeyr-ci2
Repo: shakacode/react_on_rails PR: 1732
File: spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx:40-44
Timestamp: 2025-04-26T21:55:55.874Z
Learning: In the react_on_rails project, files under `app-react16` directories are copied/moved to corresponding `/app` directories during the conversion process (removing the `-react16` suffix), which affects their relative import paths at runtime.
Learnt from: theforestvn88
Repo: shakacode/react_on_rails PR: 1620
File: spec/dummy/client/app/startup/HelloTurboStream.jsx:3-3
Timestamp: 2024-10-08T20:53:47.076Z
Learning: The `RailsContext` import in `spec/dummy/client/app/startup/HelloTurboStream.jsx` is used later in the project, as clarified by the user theforestvn88.
Learnt from: theforestvn88
Repo: shakacode/react_on_rails PR: 1620
File: spec/dummy/client/app/startup/HelloTurboStream.jsx:3-3
Timestamp: 2024-07-27T10:08:35.868Z
Learning: The `RailsContext` import in `spec/dummy/client/app/startup/HelloTurboStream.jsx` is used later in the project, as clarified by the user theforestvn88.
Learnt from: Romex91
Repo: shakacode/react_on_rails PR: 1697
File: package-scripts.yml:28-28
Timestamp: 2025-02-12T16:38:06.537Z
Learning: The file `node_package/lib/ReactOnRails.full.js` is autogenerated during the build process and should not be present in the repository.
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1781
File: node_package/src/ClientSideRenderer.ts:82-95
Timestamp: 2025-09-15T21:24:48.207Z
Learning: In React on Rails, the force_load feature includes both explicit `data-force-load="true"` usage and the ability to hydrate components during the page loading state (`document.readyState === 'loading'`). Both capabilities require a Pro license, so the condition `!railsContext.rorPro && (isComponentForceLoaded || document.readyState === 'loading')` correctly gates both scenarios.
📚 Learning: 2025-02-12T16:38:06.537Z
Learnt from: Romex91
Repo: shakacode/react_on_rails PR: 1697
File: package-scripts.yml:28-28
Timestamp: 2025-02-12T16:38:06.537Z
Learning: The file `node_package/lib/ReactOnRails.full.js` is autogenerated during the build process and should not be present in the repository.
Applied to files:
react_on_rails_pro/spec/dummy/bin/shakapacker-precompile-hook
📚 Learning: 2025-04-26T21:55:55.874Z
Learnt from: alexeyr-ci2
Repo: shakacode/react_on_rails PR: 1732
File: spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx:40-44
Timestamp: 2025-04-26T21:55:55.874Z
Learning: In the react_on_rails project, files under `app-react16` directories are copied/moved to corresponding `/app` directories during the conversion process (removing the `-react16` suffix), which affects their relative import paths at runtime.
Applied to files:
react_on_rails_pro/spec/dummy/bin/shakapacker-precompile-hook
📚 Learning: 2025-07-08T05:57:29.630Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1745
File: node_package/src/RSCRequestTracker.ts:8-14
Timestamp: 2025-07-08T05:57:29.630Z
Learning: The global `generateRSCPayload` function in React on Rails Pro (RORP) is provided by the framework during rendering requests, not implemented in application code. The `declare global` statements are used to document the expected interface that RORP will inject at runtime.
Applied to files:
react_on_rails_pro/spec/dummy/bin/shakapacker-precompile-hook
📚 Learning: 2025-09-16T08:01:11.146Z
Learnt from: justin808
Repo: shakacode/react_on_rails PR: 1770
File: lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.jsx:2-2
Timestamp: 2025-09-16T08:01:11.146Z
Learning: React on Rails uses webpack CSS Modules configuration with namedExports: true, which requires the import syntax `import * as style from './file.module.css'` rather than the default export pattern. This configuration enables better tree shaking and bundle size optimization for CSS modules.
Applied to files:
react_on_rails_pro/spec/dummy/bin/shakapacker-precompile-hook
📚 Learning: 2025-10-23T17:22:01.074Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1875
File: lib/react_on_rails/utils.rb:112-124
Timestamp: 2025-10-23T17:22:01.074Z
Learning: In React on Rails, when Pro is installed but not licensed, the intended behavior is to raise an error on boot. The `react_on_rails_pro?` method validates licenses and should raise errors early (including during path resolution in methods like `server_bundle?`) to enforce licensing requirements rather than failing later with obscure errors.
Applied to files:
react_on_rails_pro/spec/dummy/bin/shakapacker-precompile-hook
⏰ 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). (4)
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: lint-js-and-ruby
- GitHub Check: claude-review
justin808
force-pushed
the
justin808/playwright-streaming-timeout
branch
from
9c9abd1 to
ec22681
Compare
justin808
changed the title
…ding strategy Apply the same defer loading strategy fix from commit d1a8a1a to the Pro dummy app to resolve race condition causing Playwright test timeouts. ## Problem Playwright E2E tests for streaming were timing out waiting for console message "ToggleContainer with title", indicating React components weren't hydrating. ## Root Cause The Pro dummy app was still using async: true for javascript_pack_tag while the open-source dummy app was updated to defer: true in commit d1a8a1a. This created a race condition where: - Generated component packs load asynchronously - Main client-bundle also loads asynchronously - If client-bundle executes before component registrations complete, React tries to hydrate unregistered components - ToggleContainer never hydrates, useEffect never runs, console.log never fires ## Solution 1. Changed javascript_pack_tag from async: true to defer: true in application.html.erb 2. Added precompile_hook to shakapacker.yml for pack generation 3. Added bin/shakapacker-precompile-hook script Using defer: true ensures script execution order - generated component packs load and register components before main bundle executes, preventing the race condition. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- Remove unnecessary rubocop disable/enable directives - Fix Style/SymbolArray violation in shakapacker-precompile-hook - Add explanatory comment about generated_component_packs_loading_strategy defaulting to :defer to match OSS dummy app configuration Note: The failing "React Router Sixth Page" RSpec test is a known flaky test that also fails intermittently on master. This is not a regression from the defer loading strategy changes. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- Revert Pro dummy app back to async: true (Pro supports Selective Hydration) - Fix ReScript build to run from Rails root instead of current directory - Use File.join for proper path resolution of config files - Wrap build commands in Dir.chdir(rails_root) for correct execution - Add early Rails root resolution with proper error handling - Remove unnecessary defer strategy comment from initializer - Add blank line before main execution section for style 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Convert precompile hooks from Ruby to bash and DRY them up: **Why Bash Over Ruby:** - Simpler: No Ruby stdlib dependencies - Faster: No Ruby interpreter startup overhead - Standard: Most build hooks are shell scripts - Cross-platform: Works everywhere (sh/bash universal) - Fewer dependencies: Doesn't require Ruby loaded **Changes:** - Create shared bash script in generator templates - Fix ReScript build to run from Rails root (cd into rails_root) - Fix pack generation to run from Rails root - Use proper path resolution with Rails root for all file checks - Replace Ruby scripts in both dummy apps with bash version - All three copies identical (generator template + 2 dummy apps) **Script Features:** - Finds Rails root by walking up directory tree - Detects ReScript config (bsconfig.json or rescript.json) - Runs ReScript builds from correct directory - Detects auto_load_bundle/components_subdirectory config - Generates packs when configured - Cross-platform package manager detection (yarn/npm) - Proper error handling and exit codes - Sets REACT_ON_RAILS_SKIP_VALIDATION for build context **Testing:** - Shellcheck passes with no warnings - Script executes successfully in non-pro dummy app - ReScript builds complete successfully - Pack generation runs successfully - All files have trailing newlines 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
justin808
force-pushed
the
justin808/playwright-streaming-timeout
branch
from
8ef957c to
0f91a20
Compare
Code ReviewSummaryThis PR converts the precompile hook from Ruby to POSIX shell script, which is a solid improvement for cross-platform compatibility and reduces dependencies. The implementation is clean and well-structured. However, there are several important issues that need to be addressed. Critical Issues1. Shell Script Execution Issue with
|
Consolidate duplicate precompile hook logic across OSS dummy app, Pro dummy app, and generator template into a single shared Ruby implementation. Changes: - Create lib/tasks/precompile/shakapacker_precompile_hook_shared.rb with common logic - Update spec/dummy/bin/shakapacker-precompile-hook to load shared implementation - Update react_on_rails_pro/spec/dummy/bin/shakapacker-precompile-hook to load shared implementation - Simplify generator template to use Ruby instead of shell script (removes ReScript build logic) Benefits: - Eliminates 342 lines of duplicate code across 3 files - Easier to maintain and update precompile hook logic in one place - Consistent behavior across all environments - Better error handling with Ruby exceptions 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Code Review - PR #1977SummaryThis PR refactors the Shakapacker precompile hook to add ReScript build support and consolidate duplicate code across dummy apps. The changes follow good DRY principles and improve the developer experience. ✅ Strengths1. Excellent Code ReuseThe creation of 2. Robust Error Handling
3. Security Improvements
4. Cross-Platform Compatibility
5. Good Documentation
🐛 Issues Found1. Critical: Regex Pattern Missing Comment ExclusionLocation: The regex patterns for detecting config need to exclude comments: # Current (INCORRECT - matches commented lines):
return unless initializer_content.match?(/^\s*config\.auto_load_bundle\s*=/) ||
initializer_content.match?(/^\s*config\.components_subdirectory\s*=/)
# Should be (matches only uncommented lines):
return unless initializer_content.match?(/^\s*(?\!#).*config\.auto_load_bundle\s*=/) ||
initializer_content.match?(/^\s*(?\!#).*config\.components_subdirectory\s*=/)This is inconsistent with the generator template version (lines 37-38), which correctly excludes comments. This could cause the hook to run even when auto-pack generation is commented out. 2. Platform Compatibility Issue:
|
…n available Update the generator template to intelligently use the shared precompile hook implementation when running in the gem's development/test context, while maintaining a self-contained fallback for generated user applications. This provides the best of both worlds: - Test dummy apps and generator template all use the same shared code - Generated user apps have a standalone copy that works independently - Easier to maintain and update the precompile hook logic The hook now tries to load the shared implementation from the gem first, and falls back to its inline implementation if the gem isn't available or accessible. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
|
/run-skipped-ci |
|
🚀 Full CI Mode Enabled ✅ All checks are already running - nothing to do! All CI checks are already running on this PR. Added the |
Summary
Adds a Shakapacker precompile hook to the Pro dummy app to support automatic ReScript builds and React on Rails pack generation before webpack compilation.
Changes
1. Added Precompile Hook Script (
bin/shakapacker-precompile-hook)bsconfig.json(legacy) andrescript.jsonconfig filesreact_on_rails:generate_packswhenauto_load_bundleorcomponents_subdirectoryare configured2. Configured Shakapacker (
config/shakapacker.yml)precompile_hook: 'bin/shakapacker-precompile-hook'settingTechnical Details
ReScript Build Logic
File.join(rails_root, ...)for proper path resolutionDir.chdir(rails_root)package.jsonPack Generation Logic
auto_load_bundleorcomponents_subdirectorysettings (ignores comments)react_on_rails:generate_packsrake task existsWhy This Matters
The precompile hook ensures that:
Test Plan
bin/shakapackercompilationRelated
🤖 Generated with Claude Code
This change is