Fix Pro dummy app Playwright test timeouts by aligning with defer loading strategy #1977
Conversation
WalkthroughReplaced async with defer for the client bundle in the dummy layout; added a Shakapacker precompile hook script that optionally builds ReScript and conditionally runs react_on_rails pack generation; added 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 (bsconfig.json or rescript.json)
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✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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
…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>
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
|
- 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>
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
| def build_rescript_if_needed | ||
| # Check for both old (bsconfig.json) and new (rescript.json) config files | ||
| return unless File.exist?("bsconfig.json") || File.exist?("rescript.json") | ||
|
|
||
| puts "🔧 Building ReScript..." | ||
|
|
||
| # Cross-platform package manager detection | ||
| yarn_available = system("yarn", "--version", out: File::NULL, err: File::NULL) | ||
| npm_available = system("npm", "--version", out: File::NULL, err: File::NULL) | ||
|
|
||
| success = if yarn_available | ||
| system("yarn", "build:rescript") | ||
| elsif npm_available | ||
| system("npm", "run", "build:rescript") | ||
| else | ||
| warn "⚠️ Warning: Neither yarn nor npm found. Skipping ReScript build." | ||
| return | ||
| end | ||
|
|
||
| if success | ||
| puts "✅ ReScript build completed successfully" | ||
| else | ||
| warn "❌ ReScript build failed" | ||
| exit 1 | ||
| end | ||
| end |
There was a problem hiding this comment.
Use the Rails root when detecting and building ReScript assets.
build_rescript_if_needed works off the current working directory. When the precompile hook is invoked from outside the Rails root (e.g., when Shakapacker spawns the hook from the packs output directory), the File.exist? checks will miss bsconfig.json/rescript.json, and the subsequent yarn build:rescript will run without the right package.json. The build is then skipped or fails silently, so generated ReScript packs never reach the Playwright run.
Please resolve this by anchoring both the existence checks and the build command to rails_root (and aborting early if it can’t be found). For example:
def build_rescript_if_needed
- return unless File.exist?("bsconfig.json") || File.exist?("rescript.json")
+ rails_root = find_rails_root
+ return unless rails_root
+
+ config_files = %w[bsconfig.json rescript.json].map { |name| File.join(rails_root, name) }
+ return unless config_files.any? { |path| File.exist?(path) }
puts "🔧 Building ReScript..."
- yarn_available = system("yarn", "--version", out: File::NULL, err: File::NULL)
- npm_available = system("npm", "--version", out: File::NULL, err: File::NULL)
+ yarn_available = system("yarn", "--version", out: File::NULL, err: File::NULL)
+ npm_available = system("npm", "--version", out: File::NULL, err: File::NULL)
- success = if yarn_available
- system("yarn", "build:rescript")
- elsif npm_available
- system("npm", "run", "build:rescript")
- else
- warn "⚠️ Warning: Neither yarn nor npm found. Skipping ReScript build."
- return
- end
+ success = Dir.chdir(rails_root) do
+ if yarn_available
+ system("yarn", "build:rescript")
+ elsif npm_available
+ system("npm", "run", "build:rescript")
+ else
+ warn "⚠️ Warning: Neither yarn nor npm found. Skipping ReScript build."
+ return
+ end
+ endThis guarantees we always evaluate the right config files and run the package-manager script where package.json lives.
🤖 Prompt for AI Agents
In react_on_rails_pro/spec/dummy/bin/shakapacker-precompile-hook around lines 24
to 49, the ReScript detection and build run against the current working
directory which can be outside the Rails root; change the logic to first resolve
rails_root (abort early if missing), then use File.join(rails_root,
"bsconfig.json") and File.join(rails_root, "rescript.json") for existence
checks, and run the package manager commands from rails_root (e.g., invoke
system within that directory or chdir temporarily) so yarn/npm sees the correct
package.json; ensure the function returns/errs appropriately if rails_root is
not found or the build command fails.
Summary
Fixes Playwright E2E test timeouts in Pro dummy app by applying the same defer loading strategy fix from #1972 (commit d1a8a1a).
Problem
The Playwright E2E tests for streaming were timing out after 30 seconds, waiting for a console message
"ToggleContainer with title"that never appeared. This indicated that React components weren't hydrating properly.Root Cause
The Pro dummy app was still using
async: trueforjavascript_pack_tagwhile the open-source dummy app was updated todefer: truein commit d1a8a1a (#1972). This created a race condition where:client-bundlealso loads asynchronouslyclient-bundleexecutes before component registrations complete, React tries to hydrate unregistered componentsToggleContainernever hydrates →useEffectnever runs → console.log never fires → test times outChanges
javascript_pack_tagfromasync: truetodefer: trueinapplication.html.erbprecompile_hooktoshakapacker.ymlfor automatic pack generationbin/shakapacker-precompile-hookscript for ReScript builds and pack generationWhy This Fixes It
Using
defer: trueensures proper script execution order:Test Plan
ToggleContainerhydrates and logs console messageRelated
fixture.ts:83)🤖 Generated with Claude Code
This change is
Summary by CodeRabbit
Bug Fixes
New Features
Documentation