Upgrade Shakapacker to 9.3.0 #1896
Conversation
WalkthroughThis PR upgrades Shakapacker from 8.2.0 to 9.3.0, integrates SWC compiler support with conditional dependency detection via configuration parsing, implements React 19 compatibility using named imports in TypeScript/React files, updates generators with package-manager-driven installation logic, adds Changes
Sequence Diagram(s)sequenceDiagram
participant Gen as Generator
participant DetectPM as detect_package_manager_and_exact_flag
participant FileSystem as Project Root
participant PkgMgr as Package Manager
Gen->>DetectPM: Check for lock files
DetectPM->>FileSystem: Scan for yarn.lock
DetectPM->>FileSystem: Scan for pnpm-lock.yaml
DetectPM->>FileSystem: Scan for bun.lockb
DetectPM->>FileSystem: Scan for package-lock.json
alt Multiple lock files found
DetectPM->>Gen: Warn about precedence
end
alt At least one lock file found
DetectPM->>Gen: Return [package_manager, exact_flag, add_command]
else No lock files found
DetectPM->>Gen: Return ["npm", "--save-exact", "install"]
end
Gen->>PkgMgr: Install dependencies using detected manager
sequenceDiagram
participant Gen as BaseGenerator
participant Config as Config File (shakapacker.yml)
participant YAML as YAML Parser
participant Helper as Helper Method
Gen->>Helper: Call using_swc?
Helper->>Config: Read default.javascript_compiler
alt File exists and parses
YAML->>Helper: Parse YAML
Helper->>Helper: Check if == "swc"
else File missing or parse error
Helper->>Gen: Log warning, default to false (Babel)
end
Gen->>Gen: Conditionally add SWC deps (`@swc/core`, swc-loader)
Gen->>Gen: Or add Babel deps (`@babel/preset-react`, etc.)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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: Update shakapacker to 9.3.0SummaryThis PR updates shakapacker from version 8.2.0 to 9.3.0 across the codebase. Overall, this is a well-executed dependency update with comprehensive test coverage updates. The changes are consistent and follow the project's conventions. ✅ Strengths
|
Code Review: Update shakapacker to 9.3.0Overall Assessment✅ APPROVED - This is a well-executed dependency update with proper version management and good backward compatibility considerations. Strengths1. Excellent Version Strategy 🎯The second commit demonstrates excellent understanding of semantic versioning:
Code reference: 2. Comprehensive Update Coverage ✅All necessary files were updated:
3. Good Testing Hygiene 🧪
Observations & Recommendations1. Missing CHANGELOG Entry 📝Issue: No update to CHANGELOG.md for this version bump Recommendation: Add an entry documenting this upgrade, especially noting:
2. Dependency Updates in yarn.lock 🔍Observation: The update introduces new dependencies:
Question: Have these been tested for compatibility? The new dependencies appear to be internal Shakapacker requirements, which should be fine, but worth verifying CI passes. 3. Generated Examples Version Constraint
|
Code Review: Update shakapacker to 9.3.0SummaryThis PR updates the shakapacker dependency from version 8.2.0 to 9.3.0 across the codebase. The changes are systematic and well-documented. ✅ Strengths
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
rakelib/shakapacker_examples.rake (1)
37-37: Consider using pessimistic version constraint~> 9.3for better compatibility.The
>= 9.3.0constraint allows any future version including potentially incompatible major versions (10.x, 11.x). Using~> 9.3would allow patch and minor updates within the 9.x line while preventing breaking changes from future major versions.Apply this diff to use a pessimistic version constraint:
- sh_in_dir(example_type.dir, "echo \"gem 'shakapacker', '>= 9.3.0'\" >> #{example_type.gemfile}") + sh_in_dir(example_type.dir, "echo \"gem 'shakapacker', '~> 9.3'\" >> #{example_type.gemfile}")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
Gemfile.lockis excluded by!**/*.lockspec/dummy/Gemfile.lockis excluded by!**/*.lockspec/dummy/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (5)
Gemfile.development_dependencies(1 hunks)lib/generators/react_on_rails/templates/base/base/config/shakapacker.yml(1 hunks)rakelib/shakapacker_examples.rake(1 hunks)script/convert(1 hunks)spec/dummy/package.json(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{js,jsx,ts,tsx,css,scss,json,yml,yaml,md}
📄 CodeRabbit inference engine (CLAUDE.md)
Prettier is the sole authority for formatting all non-Ruby files; never manually format them
Files:
lib/generators/react_on_rails/templates/base/base/config/shakapacker.ymlspec/dummy/package.json
{Gemfile,Rakefile,config.ru,**/*.{rb,rake,gemspec,ru}}
📄 CodeRabbit inference engine (CLAUDE.md)
{Gemfile,Rakefile,config.ru,**/*.{rb,rake,gemspec,ru}}: All Ruby code must pass RuboCop with zero offenses before commit/push
RuboCop is the sole authority for Ruby file formatting; never manually format Ruby files
Files:
rakelib/shakapacker_examples.rake
🧠 Learnings (6)
📓 Common learnings
Learnt from: alexeyr-ci
Repo: shakacode/react_on_rails PR: 1687
File: spec/dummy/package.json:0-0
Timestamp: 2025-01-23T18:20:45.824Z
Learning: When adding or updating dependencies in spec/dummy/package.json, maintain version consistency with other package.json files in the codebase to avoid potential version conflicts.
📚 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:
lib/generators/react_on_rails/templates/base/base/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:
lib/generators/react_on_rails/templates/base/base/config/shakapacker.ymlscript/convert
📚 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:
lib/generators/react_on_rails/templates/base/base/config/shakapacker.ymlscript/convert
📚 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:
lib/generators/react_on_rails/templates/base/base/config/shakapacker.yml
📚 Learning: 2025-01-23T18:20:45.824Z
Learnt from: alexeyr-ci
Repo: shakacode/react_on_rails PR: 1687
File: spec/dummy/package.json:0-0
Timestamp: 2025-01-23T18:20:45.824Z
Learning: When adding or updating dependencies in spec/dummy/package.json, maintain version consistency with other package.json files in the codebase to avoid potential version conflicts.
Applied to files:
spec/dummy/package.jsonscript/convert
🔇 Additional comments (4)
Gemfile.development_dependencies (1)
3-3: LGTM!The shakapacker version update is consistent with other changes in this PR. Exact version pinning ensures reproducible builds across development environments.
spec/dummy/package.json (2)
57-57: No issues found —swc-loader0.2.6 is compatible with webpack 5.72.0 and shakapacker.[email protected] is compatible with Webpack 5 (including 5.72.0), and Shakapacker v6.1.1+ includes opt-in SWC support. Since shakapacker 9.3.0 is newer than 6.1.1+, the added dependency is appropriate for this configuration.
32-32: No compatibility issues detected.Shakapacker 9.3.0 recommends @swc/core ^1.3.0, and the specified version ^1.7.0 falls within this compatible range. The configuration is correct.
script/convert (1)
20-22: Shakapacker 9.3.0 is still in beta—verify if intentional and test API compatibility.The codebase extensively uses shakapacker APIs that could break with major version changes:
- Ruby:
Shakapacker::Instance,Shakapacker::Configuration,Shakapacker::DevServer,Shakapacker::Manifest, exception handling- JavaScript:
generateWebpackConfig,webpackConfig,inliningCss,config,devServer,merge, Babel preset importsShakapacker 9.3.0 is still in beta—no stable release notes or formal breaking changes documentation has been published yet. Before upgrading:
- Confirm 9.3.0 beta is acceptable for your use case (consider using the latest stable version instead)
- Review the 9.3.0 beta releases and PRs on the Shakapacker repository to identify any API-breaking changes affecting the patterns found in the codebase
- Run full integration tests after updating, including webpack config generation and bundle loading
lib/generators/react_on_rails/templates/base/base/config/shakapacker.yml
Outdated
Show resolved
Hide resolved
Pull Request Review: Update shakapacker to 9.3.0OverviewThis PR updates Shakapacker from version 8.2.0 to 9.3.0 across the codebase. Overall, the changes are well-structured and comprehensive. The PR description indicates that all linting checks pass and git hooks succeeded. ✅ Strengths1. Comprehensive Version UpdatesAll version references have been properly updated across:
2. Configuration Changes Are CorrectThe # OLD (v8.2.0):
webpack_loader: 'babel'
# NEW (v9.3.0):
javascript_transpiler: "swc" # Default, 20x fasterThis change aligns with Shakapacker 9.x's improved configuration naming and defaults to SWC for better performance. 3. Proper Dependency AdditionsThe PR correctly adds the required SWC dependencies to
These are necessary for the new default transpiler configuration. 4. Lock Files Updated CorrectlyBoth
|
PR Review: Update shakapacker to 9.3.0Overall AssessmentThis is a well-executed dependency upgrade that properly handles the breaking changes introduced in Shakapacker 9.x. The PR demonstrates good attention to compatibility and includes all necessary configuration updates. ✅ Code Quality & Best Practices✅ Strengths
Potential Issues & Concerns
|
Code Review: Shakapacker 9.3.0 UpgradeSummaryThis PR successfully upgrades Shakapacker from 8.2.0 to 9.3.0 with proper handling of breaking changes. The changes are well-structured across 4 commits that logically progress through the upgrade process. ✅ Strengths1. Excellent Commit StructureThe PR follows a logical progression through initial version bump, backward compatibility correction, SWC support, and CSS Modules fix. This incremental approach makes the changes easy to review and understand. 2. Backward Compatibility PreservationThe second commit correctly reverts the async loading version check to 8.2.0 in lib/react_on_rails/packer_utils.rb. This is the right decision - users with shakapacker >= 8.2.0 should be able to use async loading. 3. Proper Handling of Breaking ChangesSWC Transpiler Support: Added required dependencies (@swc/core, swc-loader) and updated template to use new javascript_transpiler setting with good documentation about performance benefits. CSS Modules Compatibility: The webpack config fix properly addresses Shakapacker 9.0 breaking change by setting namedExport: false to preserve compatibility with existing default imports. 🔍 Issues & Recommendations1. Missing CSS Modules Fix in Generator TemplateThe CSS Modules compatibility fix is applied to spec/dummy/config/webpack/commonWebpackConfig.js but needs verification in generator templates. New projects should have this fix. 2. Documentation GapsMissing: CHANGELOG entry, migration guide for users upgrading from 8.x to 9.x, documentation of breaking changes. Recommendation: Add CHANGELOG entry documenting the upgrade, breaking changes (SWC as default transpiler, CSS Modules configuration), and migration steps. 3. Test CoverageMissing: Tests for SWC transpiler configuration, CSS Modules with default imports, integration tests for 9.3.0 upgrade. 4. Version ConflictsGenerated examples require >= 9.3.0 but main gem uses = 9.3.0 (exact version). Consider using ~> 9.3 for flexibility or document why exact version is required. 5. SecuritySWC dependencies use caret ranges which is good for security patches. Current approach is reasonable. 🎯 PerformanceSWC is 20x faster than Babel - significant improvement. Consider adding before/after build time metrics to quantify benefits. 📋 Action Items Before Merge
🎓 Code Quality Assessment
✅ Final RecommendationApprove with Minor Improvements Requested The upgrade is well-executed, especially the handling of breaking changes. Main improvements needed are documentation (CHANGELOG) and ensuring generator templates include the CSS Modules fix. Great work on backward compatibility preservation and logical commit progression! 🎉 |
Code Review - PR #1896: Update shakapacker to 9.3.0I've completed a comprehensive review of this PR. Overall, this is a well-executed dependency upgrade with appropriate configuration updates. Here's my detailed feedback: ✅ Positive Findings1. Comprehensive Version Updates
2. SWC Integration (Performance Improvement)
3. Breaking Change Mitigation
4. Configuration Updates
|
Pull Request Review: Update Shakapacker to 9.3.0SummaryThis PR updates Shakapacker from version 8.2.0 to 9.3.0 across the codebase. Overall, the update is well-structured and comprehensive. The changes are properly tested according to the PR description, and the automated git hooks have passed. ✅ Code Quality & Best PracticesStrengths:
Areas for Improvement:1. Missing Test Update
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
lib/react_on_rails/engine.rb (1)
12-14: Good defensive guard to handle missing package.json.The validation skip when
package.jsondoesn't exist prevents startup errors during initial setup, which aligns well with the Shakapacker installation workflow.Minor suggestion: Consider renaming the variable to
package_json_pathfor clarity, since it holds a path rather than JSON content.Apply this diff to improve variable naming:
# Skip validation if package.json doesn't exist yet (during initial setup) -package_json = VersionChecker::NodePackageVersion.package_json_path -next unless File.exist?(package_json) +package_json_path = VersionChecker::NodePackageVersion.package_json_path +next unless File.exist?(package_json_path)spec/dummy/config/webpack/commonWebpackConfig.js (1)
32-43: Consider more robust loader detection and handle additional webpack rule formats.The current implementation has a few limitations:
Incomplete rule.use handling: Only processes
rule.usewhen it's an array. Webpack rules can also have:
rule.useas a single loader objectrule.useas a functionrule.loaderinstead ofrule.useFragile loader matching: Using
loader.loader.includes('css-loader')could match unintended paths (e.g.,/custom-css-loader/). Consider a more precise check likeloader.loader.endsWith('css-loader')or using a regex pattern.Consider this more comprehensive approach:
-baseClientWebpackConfig.module.rules.forEach((rule) => { - if (Array.isArray(rule.use)) { - rule.use.forEach((loader) => { - if (loader.loader && loader.loader.includes('css-loader') && loader.options?.modules) { - // eslint-disable-next-line no-param-reassign - loader.options.modules.namedExport = false; - // eslint-disable-next-line no-param-reassign - loader.options.modules.exportLocalsConvention = 'camelCase'; - } - }); - } -}); +const configureCssLoader = (loader) => { + if (loader.loader?.match(/css-loader/) && loader.options?.modules) { + // eslint-disable-next-line no-param-reassign + loader.options.modules.namedExport = false; + // eslint-disable-next-line no-param-reassign + loader.options.modules.exportLocalsConvention = 'camelCase'; + } +}; + +baseClientWebpackConfig.module.rules.forEach((rule) => { + // Handle rule.use as array + if (Array.isArray(rule.use)) { + rule.use.forEach(configureCssLoader); + } + // Handle rule.use as single loader object + else if (rule.use && typeof rule.use === 'object') { + configureCssLoader(rule.use); + } + // Handle rule.loader (alternative to rule.use) + else if (rule.loader) { + configureCssLoader({ loader: rule.loader, options: rule.options }); + } +});
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
lib/generators/react_on_rails/base_generator.rb(1 hunks)lib/generators/react_on_rails/install_generator.rb(1 hunks)lib/react_on_rails/engine.rb(1 hunks)spec/dummy/config/webpack/commonWebpackConfig.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
{Gemfile,Rakefile,config.ru,**/*.{rb,rake,gemspec,ru}}
📄 CodeRabbit inference engine (CLAUDE.md)
{Gemfile,Rakefile,config.ru,**/*.{rb,rake,gemspec,ru}}: All Ruby code must pass RuboCop with zero offenses before commit/push
RuboCop is the sole authority for Ruby file formatting; never manually format Ruby files
Files:
lib/react_on_rails/engine.rblib/generators/react_on_rails/install_generator.rblib/generators/react_on_rails/base_generator.rb
**/*.{js,jsx,ts,tsx,css,scss,json,yml,yaml,md}
📄 CodeRabbit inference engine (CLAUDE.md)
Prettier is the sole authority for formatting all non-Ruby files; never manually format them
Files:
spec/dummy/config/webpack/commonWebpackConfig.js
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use ESLint for JS/TS code (lint via rake lint or yarn lint)
Files:
spec/dummy/config/webpack/commonWebpackConfig.js
🧠 Learnings (11)
📚 Learning: 2025-02-18T13:08:01.477Z
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.
Applied to files:
lib/react_on_rails/engine.rb
📚 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:
lib/react_on_rails/engine.rb
📚 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:
lib/react_on_rails/engine.rb
📚 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:
lib/react_on_rails/engine.rblib/generators/react_on_rails/install_generator.rblib/generators/react_on_rails/base_generator.rb
📚 Learning: 2025-01-23T18:20:45.824Z
Learnt from: alexeyr-ci
Repo: shakacode/react_on_rails PR: 1687
File: spec/dummy/package.json:0-0
Timestamp: 2025-01-23T18:20:45.824Z
Learning: When adding or updating dependencies in spec/dummy/package.json, maintain version consistency with other package.json files in the codebase to avoid potential version conflicts.
Applied to files:
lib/react_on_rails/engine.rblib/generators/react_on_rails/base_generator.rbspec/dummy/config/webpack/commonWebpackConfig.js
📚 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:
lib/react_on_rails/engine.rb
📚 Learning: 2025-02-18T13:08:01.477Z
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 is handled in deeper level calls of the React on Rails Pro codebase, so it doesn't need to be validated again in the `rsc_payload_react_component` helper method.
Applied to files:
lib/react_on_rails/engine.rb
📚 Learning: 2024-12-12T13:07:09.929Z
Learnt from: alexeyr-ci
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/ReactOnRailsRSC.ts:87-87
Timestamp: 2024-12-12T13:07:09.929Z
Learning: When handling errors in 'node_package/src/ReactOnRailsRSC.ts', include the error stack in error messages in development and test environments to aid debugging.
Applied to files:
lib/react_on_rails/engine.rblib/generators/react_on_rails/base_generator.rb
📚 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:
lib/react_on_rails/engine.rblib/generators/react_on_rails/install_generator.rblib/generators/react_on_rails/base_generator.rb
📚 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:
lib/generators/react_on_rails/install_generator.rbspec/dummy/config/webpack/commonWebpackConfig.js
📚 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:
lib/generators/react_on_rails/install_generator.rb
🧬 Code graph analysis (1)
lib/react_on_rails/engine.rb (2)
lib/generators/react_on_rails/generator_helper.rb (1)
package_json(7-20)lib/react_on_rails/version_checker.rb (1)
package_json_path(222-224)
⏰ 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). (11)
- GitHub Check: build
- GitHub Check: build-dummy-app-webpack-test-bundles (3.2, 20)
- GitHub Check: build-dummy-app-webpack-test-bundles (3.4, 22)
- GitHub Check: lint-js-and-ruby
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: rspec-package-tests (3.4, minimum)
- GitHub Check: claude-review
- GitHub Check: rspec-package-tests (3.2, latest)
- GitHub Check: rspec-package-tests (3.2, minimum)
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: build-dummy-app-webpack-test-bundles
🔇 Additional comments (3)
lib/generators/react_on_rails/base_generator.rb (1)
204-205: LGTM! SWC dependencies added for Shakapacker 9.x support.The addition of
@swc/coreandswc-loaderaligns with the Shakapacker 9.3.0 upgrade. These packages enable SWC-based transpilation as an alternative to Babel.lib/generators/react_on_rails/install_generator.rb (1)
497-498: LGTM! SWC dependencies added for Shakapacker 9.x support.These additions mirror the changes in
base_generator.rband correctly enable SWC transpilation support for the Shakapacker 9.3.0 upgrade.spec/dummy/config/webpack/commonWebpackConfig.js (1)
29-31: ****The review comment misidentifies the scope and impact of this change. The webpack configuration modification only affects the test app (
spec/dummy), not the generator templates.Analysis shows:
- Generator templates use
import * as style(named imports) and will continue using Shakapacker 9.0 defaults (namedExport: true)- The dummy test app uses
import css/styles(default imports) and is now configured withnamedExport: falseto align with those imports- These are separate concerns with separate configurations
The change reconciles the dummy app's webpack configuration with its actual CSS Module import patterns—it's not breaking existing imports. The retrieved learning about
namedExports: trueremains valid for generated projects using template defaults.Likely an incorrect or invalid review comment.
PR Review: Update shakapacker to 9.3.0Overall, this is a well-executed upgrade with thoughtful handling of breaking changes. The PR addresses multiple compatibility issues systematically. Here's my detailed review: ✅ Strengths
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
lib/react_on_rails/engine.rb (1)
16-17: Consider more robust generator detection.The current ARGV string matching could produce false positives if any argument contains "generate" or "g" (e.g., file paths, script names). A more precise approach would be to check the Rails command context directly.
Consider using a more targeted check:
- # Skip validation when running generators (packages may not be installed yet) - next if defined?(Rails::Generators) && ARGV.any? { |arg| arg.include?("generate") || arg.include?("g") } + # Skip validation when running generators (packages may not be installed yet) + next if defined?(Rails::Command::GenerateCommand) || (defined?(Rails::Generators) && Rails.const_defined?(:Generator))Alternatively, check the command name more precisely:
- next if defined?(Rails::Generators) && ARGV.any? { |arg| arg.include?("generate") || arg.include?("g") } + next if defined?(Rails::Generators) && ARGV.first&.match?(/\A(g|generate)\z/)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
lib/generators/react_on_rails/install_generator.rb(2 hunks)lib/react_on_rails/engine.rb(1 hunks)package_json_issue_comment.md(1 hunks)spec/dummy/config/webpack/commonWebpackConfig.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/generators/react_on_rails/install_generator.rb
🧰 Additional context used
📓 Path-based instructions (3)
{Gemfile,Rakefile,config.ru,**/*.{rb,rake,gemspec,ru}}
📄 CodeRabbit inference engine (CLAUDE.md)
{Gemfile,Rakefile,config.ru,**/*.{rb,rake,gemspec,ru}}: All Ruby code must pass RuboCop with zero offenses before commit/push
RuboCop is the sole authority for Ruby file formatting; never manually format Ruby files
Files:
lib/react_on_rails/engine.rb
**/*.{js,jsx,ts,tsx,css,scss,json,yml,yaml,md}
📄 CodeRabbit inference engine (CLAUDE.md)
Prettier is the sole authority for formatting all non-Ruby files; never manually format them
Files:
spec/dummy/config/webpack/commonWebpackConfig.jspackage_json_issue_comment.md
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use ESLint for JS/TS code (lint via rake lint or yarn lint)
Files:
spec/dummy/config/webpack/commonWebpackConfig.js
🧠 Learnings (10)
📚 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:
lib/react_on_rails/engine.rb
📚 Learning: 2025-02-18T13:08:01.477Z
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.
Applied to files:
lib/react_on_rails/engine.rb
📚 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:
lib/react_on_rails/engine.rb
📚 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:
lib/react_on_rails/engine.rb
📚 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:
lib/react_on_rails/engine.rb
📚 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:
lib/react_on_rails/engine.rb
📚 Learning: 2025-02-18T13:08:01.477Z
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 is handled in deeper level calls of the React on Rails Pro codebase, so it doesn't need to be validated again in the `rsc_payload_react_component` helper method.
Applied to files:
lib/react_on_rails/engine.rb
📚 Learning: 2024-12-12T13:07:09.929Z
Learnt from: alexeyr-ci
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/ReactOnRailsRSC.ts:87-87
Timestamp: 2024-12-12T13:07:09.929Z
Learning: When handling errors in 'node_package/src/ReactOnRailsRSC.ts', include the error stack in error messages in development and test environments to aid debugging.
Applied to files:
lib/react_on_rails/engine.rb
📚 Learning: 2025-01-23T18:20:45.824Z
Learnt from: alexeyr-ci
Repo: shakacode/react_on_rails PR: 1687
File: spec/dummy/package.json:0-0
Timestamp: 2025-01-23T18:20:45.824Z
Learning: When adding or updating dependencies in spec/dummy/package.json, maintain version consistency with other package.json files in the codebase to avoid potential version conflicts.
Applied to files:
lib/react_on_rails/engine.rbpackage_json_issue_comment.md
📚 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:
spec/dummy/config/webpack/commonWebpackConfig.js
🧬 Code graph analysis (1)
lib/react_on_rails/engine.rb (2)
lib/generators/react_on_rails/generator_helper.rb (1)
package_json(7-20)lib/react_on_rails/version_checker.rb (1)
package_json_path(222-224)
🔇 Additional comments (4)
lib/react_on_rails/engine.rb (1)
12-14: LGTM! Safe guard for initial setup.The package.json existence check is appropriate for initial setup scenarios where the file hasn't been created yet.
spec/dummy/config/webpack/commonWebpackConfig.js (2)
27-29: LGTM! Appropriate guard for SCSS loader.The defensive check prevents errors if the SCSS rule doesn't exist or lacks a
usearray, which is good practice for plugin configurations.
31-45: LGTM! Appropriate backward compatibility fix for Shakapacker 9.0.This correctly overrides Shakapacker 9.0's new default (
namedExport: true) to preserve existing import patterns (import styles from './file.module.css'). The mutation is intentional and properly guarded.package_json_issue_comment.md (1)
1-58: Clarify the purpose and placement of this documentation.This file appears to document a feature request or issue comment for the
package_jsongem. The filename and content suggest it might be:
- A draft issue comment for an external repository
- Temporary documentation that shouldn't be committed
- Internal documentation that should be in a
docs/directoryCould you clarify:
- Is this file intended to be committed to the repository?
- If yes, should it be in a more appropriate location (e.g.,
docs/, or as a GitHub issue)?- If it's a draft for an external issue, consider moving it outside the repository.
PR Review: Update Shakapacker to 9.3.0I've reviewed the changes in this PR. Overall, this is a well-structured upgrade with good attention to compatibility concerns. Here's my detailed feedback: ✅ Strengths
🔍 Potential Issues & Concerns1. Rails Engine Validation Logic (lib/react_on_rails/engine.rb:17)next if defined?(Rails::Generators) && ARGV.any? { |arg| arg.include?("generate") || arg.include?("g") }Issues:
Recommendation: # Check if we're in a generator context more reliably
generator_running = defined?(Rails::Generators) &&
(caller.any? { |line| line.include?('generators/') } ||
Rails.const_defined?(:Command) && Rails::Command.current == 'generate')
next if generator_runningOr use an environment variable approach that generators can set explicitly. 2. CSS Modules Configuration (spec/dummy/config/webpack/commonWebpackConfig.js:27-28)if (scssConfigIndex !== -1 && baseClientWebpackConfig.module.rules[scssConfigIndex]?.use) {
baseClientWebpackConfig.module.rules[scssConfigIndex].use.push(sassLoaderConfig);
}Good: The guard check was added to prevent errors if the SCSS rule isn't found. Concern: The CSS Modules configuration (lines 34-45) doesn't have similar safety checks. It directly mutates loader configs without checking if they exist. Recommendation: baseClientWebpackConfig.module.rules.forEach((rule) => {
if (Array.isArray(rule.use)) {
rule.use.forEach((loader) => {
if (loader?.loader && loader.loader.includes('css-loader') && loader.options?.modules) {
// ... existing code
}
});
}
});3. Package Installation Strategy (lib/generators/react_on_rails/install_generator.rb:430-447)The change to always use direct Current code: success = system("npm", "install", "--save-exact", react_on_rails_pkg)Issues:
Recommendation:
🎯 Performance & Security Considerations
🧪 Test CoverageQuestion: Were the following scenarios tested?
The PR description mentions RuboCop passing and bundle/yarn install succeeding, but explicit generator testing would be valuable. 📝 Minor Suggestions
✅ Best Practices Adherence
🎬 RecommendationConditional APPROVE pending:
The core upgrade work is solid, but the package manager detection regression needs attention before merging. Great work on this upgrade! The attention to backward compatibility and the SWC migration is well-executed. 🚀 |
Code Review: Shakapacker 9.3.0 UpdateOverviewThis PR updates Shakapacker from 8.2.0 to 9.3.0. Overall, the update is well-structured and follows the project's coding standards. However, I've identified several issues that should be addressed before merging. Critical Issues1. Package Manager Hard-Coding in
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
lib/generators/react_on_rails/generator_helper.rb (1)
98-128: Good package manager detection logic with minor clarity suggestions.The implementation correctly detects package managers and returns appropriate commands. The fallback to npm is sensible.
Consider these optional improvements for clarity:
- Document the precedence order - The order matters when multiple lock files exist, but it's implicit in the hash order:
# Detects the package manager based on lock files and returns the appropriate command and exact flag # Precedence order: yarn > pnpm > bun > npm (first detected is used) # Returns: [package_manager, exact_flag, add_command]
- Note the command difference - It's subtle that npm uses "install" while others use "add". Consider adding a comment:
lock_files = { "yarn.lock" => ["yarn", "--exact", "add"], "pnpm-lock.yaml" => ["pnpm", "--save-exact", "add"], "bun.lockb" => ["bun", "--exact", "add"], "package-lock.json" => ["npm", "--save-exact", "install"] # npm uses 'install' not 'add' }ISSUE_ANALYSIS.md (1)
1-485: Valuable design documentation capturing implementation rationale.This document provides excellent context for the changes in this PR by:
- Analyzing the problems with existing approaches
- Evaluating multiple solution options
- Documenting the recommended approach (which was implemented)
The document shows that Issue #1 (engine validation) and Issue #3 (package manager detection) solutions were implemented in
lib/react_on_rails/engine.rbandlib/generators/react_on_rails/generator_helper.rbrespectively.Optional improvements:
- Add implementation status - Consider adding a section linking to the actual implementation:
## Implementation Status - ✅ Issue #1: Implemented in `lib/react_on_rails/engine.rb` (lines 12-17) - ⏳ Issue #2: Pending - ✅ Issue #3: Implemented in `lib/generators/react_on_rails/generator_helper.rb` (lines 98-128)
Consider renaming - If this is meant to be permanent documentation, consider moving to
docs/directory or renaming to something likeSHAKAPACKER_9_UPGRADE_ANALYSIS.mdto make the context clearer.Formatting - The markdownlint warnings about emphasis-as-heading can be fixed by converting bold text to proper headings where appropriate (lines 42, 89, 108, 158, 189, 215, 282, 321, 372, 440).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.github/workflows/lint-js-and-ruby.yml(1 hunks)ISSUE_ANALYSIS.md(1 hunks)knip.ts(3 hunks)lib/generators/react_on_rails/generator_helper.rb(1 hunks)lib/generators/react_on_rails/install_generator.rb(6 hunks)lib/react_on_rails/engine.rb(1 hunks)react_on_rails_pro/spec/dummy/client/app/components/HelloWorld.module.scss(1 hunks)spec/dummy/client/app/components/HelloWorld.module.scss(1 hunks)spec/dummy/config/webpack/commonWebpackConfig.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- spec/dummy/config/webpack/commonWebpackConfig.js
- lib/generators/react_on_rails/install_generator.rb
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{js,jsx,ts,tsx,css,scss,json,yml,yaml,md}
📄 CodeRabbit inference engine (CLAUDE.md)
Prettier is the sole authority for formatting all non-Ruby files; never manually format them
Files:
knip.tsISSUE_ANALYSIS.mdspec/dummy/client/app/components/HelloWorld.module.scssreact_on_rails_pro/spec/dummy/client/app/components/HelloWorld.module.scss
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use ESLint for JS/TS code (lint via rake lint or yarn lint)
Files:
knip.ts
{Gemfile,Rakefile,config.ru,**/*.{rb,rake,gemspec,ru}}
📄 CodeRabbit inference engine (CLAUDE.md)
{Gemfile,Rakefile,config.ru,**/*.{rb,rake,gemspec,ru}}: All Ruby code must pass RuboCop with zero offenses before commit/push
RuboCop is the sole authority for Ruby file formatting; never manually format Ruby files
Files:
lib/generators/react_on_rails/generator_helper.rblib/react_on_rails/engine.rb
🧠 Learnings (14)
📚 Learning: 2025-09-29T12:23:13.615Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-29T12:23:13.615Z
Learning: Applies to **/*.{js,jsx,ts,tsx} : Use ESLint for JS/TS code (lint via rake lint or yarn lint)
Applied to files:
.github/workflows/lint-js-and-ruby.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:
.github/workflows/lint-js-and-ruby.ymlISSUE_ANALYSIS.mdlib/react_on_rails/engine.rbreact_on_rails_pro/spec/dummy/client/app/components/HelloWorld.module.scss
📚 Learning: 2025-09-29T12:23:13.615Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-29T12:23:13.615Z
Learning: Applies to {Gemfile,Rakefile,config.ru,**/*.{rb,rake,gemspec,ru}} : All Ruby code must pass RuboCop with zero offenses before commit/push
Applied to files:
.github/workflows/lint-js-and-ruby.yml
📚 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:
.github/workflows/lint-js-and-ruby.ymlISSUE_ANALYSIS.mdlib/generators/react_on_rails/generator_helper.rblib/react_on_rails/engine.rb
📚 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:
.github/workflows/lint-js-and-ruby.ymlknip.tsISSUE_ANALYSIS.mdspec/dummy/client/app/components/HelloWorld.module.scssreact_on_rails_pro/spec/dummy/client/app/components/HelloWorld.module.scss
📚 Learning: 2025-09-29T12:23:13.615Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-29T12:23:13.615Z
Learning: Never run npm; use Yarn Classic equivalents only
Applied to files:
knip.ts
📚 Learning: 2025-02-13T19:09:15.991Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/RSCWebpackLoader.ts:0-0
Timestamp: 2025-02-13T19:09:15.991Z
Learning: In React Server Components webpack loader, using `new Function('return import("react-server-dom-webpack/node-loader")')()` is necessary as a workaround to bypass TypeScript compilation issues with direct dynamic imports.
Applied to files:
knip.ts
📚 Learning: 2025-02-18T13:08:01.477Z
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.
Applied to files:
ISSUE_ANALYSIS.mdlib/react_on_rails/engine.rb
📚 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:
ISSUE_ANALYSIS.mdlib/react_on_rails/engine.rb
📚 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:
lib/react_on_rails/engine.rb
📚 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:
lib/react_on_rails/engine.rb
📚 Learning: 2025-02-18T13:08:01.477Z
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 is handled in deeper level calls of the React on Rails Pro codebase, so it doesn't need to be validated again in the `rsc_payload_react_component` helper method.
Applied to files:
lib/react_on_rails/engine.rb
📚 Learning: 2024-12-12T13:07:09.929Z
Learnt from: alexeyr-ci
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/ReactOnRailsRSC.ts:87-87
Timestamp: 2024-12-12T13:07:09.929Z
Learning: When handling errors in 'node_package/src/ReactOnRailsRSC.ts', include the error stack in error messages in development and test environments to aid debugging.
Applied to files:
lib/react_on_rails/engine.rb
📚 Learning: 2024-10-08T20:53:47.076Z
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.
Applied to files:
react_on_rails_pro/spec/dummy/client/app/components/HelloWorld.module.scss
🧬 Code graph analysis (2)
lib/generators/react_on_rails/generator_helper.rb (1)
lib/generators/react_on_rails/generator_messages.rb (1)
add_warning(13-15)
lib/react_on_rails/engine.rb (2)
lib/generators/react_on_rails/generator_helper.rb (1)
package_json(7-20)lib/react_on_rails/version_checker.rb (1)
package_json_path(222-224)
🪛 LanguageTool
ISSUE_ANALYSIS.md
[uncategorized] ~463-~463: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...ase failures 3. Fix Issue #2 last - Low priority defensive programming improvement ## T...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
🪛 markdownlint-cli2 (0.18.1)
ISSUE_ANALYSIS.md
42-42: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
89-89: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
108-108: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
158-158: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
189-189: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
215-215: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
282-282: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
321-321: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
372-372: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
440-440: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🔇 Additional comments (6)
spec/dummy/client/app/components/HelloWorld.module.scss (1)
1-2: Verify that the SCSS import path and variable are available.The import statement is being added to enable usage of
$bright-colorfrom the shared variables file. Ensure thatapp-variables.scssexists at the specified path and that the$bright-colorvariable is properly defined.react_on_rails_pro/spec/dummy/client/app/components/HelloWorld.module.scss (1)
1-5: Verify that the SCSS import path and variable are available in the react_on_rails_pro environment.The import statement mirrors the changes in
spec/dummy/client/app/components/HelloWorld.module.scss, enabling usage of$bright-colorfrom the shared variables file. Ensure thatapp-variables.scssexists at the specified path in this parallel environment and that the$bright-colorvariable is properly defined..github/workflows/lint-js-and-ruby.yml (1)
73-76: Clarify the rationale for suppressing knip config hints.The addition of
--no-config-hintsto the knip invocation (Line 75) is not mentioned in the PR objectives, which focus on shakapacker version updates. This flag suppresses knip's configuration diagnostics, which could mask legitimate issues.What is the underlying knip configuration problem triggered by the shakapacker/SWC upgrade? Consider documenting this in a comment or commit message if this is a known workaround.
lib/react_on_rails/engine.rb (1)
9-17: Excellent validation guards implementation.The two-tier skip logic is well-designed:
- Skips when
package.jsondoesn't exist (initial setup scenario)- Skips when explicitly requested via environment variable (generator scenario)
This approach eliminates the fragile ARGV inspection and provides explicit control. The guards are ordered efficiently with the cheaper file existence check first.
The implementation aligns with the recommended solution in ISSUE_ANALYSIS.md and provides a robust way to handle validation timing without false positives.
knip.ts (2)
10-18: Clarified binary ignores for dev-only tooling.The additions of
npsande2e-testwith clear comments improve the configuration documentation.
112-134: Updated ignore rules align with Shakapacker 9 tooling changes.The dependency ignore adjustments reflect the updated build configuration:
- Moving
mini-css-extract-pluginto root workspace ignores- Adding
webpack-mergeas a direct dependency (transitive from shakapacker)- Removing previously ignored loaders that may now be properly detected
The changes are consistent with the broader webpack/CSS loader configuration updates in this PR.
…ssing require Addresses 3 critical issues identified in PR #1896 review: 1. Remove invalid javascript_transpiler configuration - Shakapacker 9.3.0 does not support a top-level javascript_transpiler key - Removed lines 39-42 from shakapacker.yml template - Transpiler selection is configured via webpack config and installed loaders 2. Update Pro package.json files to Shakapacker 9.3.0 - Updated react_on_rails_pro/spec/dummy/package.json (8.0.0 → 9.3.0) - Updated react_on_rails_pro/spec/execjs-compatible-dummy/package.json (8.0.0 → 9.3.0) - Added swc-loader and @swc/core dependencies to both files - Ensures version consistency across the codebase 3. Fix missing GeneratorMessages require in generator_helper.rb - Added require_relative "generator_messages" to prevent NameError - Issue occurred when detect_package_manager_and_exact_flag found multiple lock files - Now consistent with other generator files (base_generator.rb, install_generator.rb) All RuboCop checks pass. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
lib/generators/react_on_rails/generator_helper.rb (1)
100-129: Consider clarifying the package manager precedence order.The method correctly detects package managers and handles multiple lock files, but the warning message doesn't explain the precedence order (yarn → pnpm → bun → npm). Users might benefit from knowing why a particular package manager was chosen when multiple lock files exist.
Consider updating the warning message to be more explicit:
if detected.size > 1 GeneratorMessages.add_warning(<<~MSG.strip) ⚠️ Multiple package manager lock files detected: #{detected.map { |lf, _| " • #{lf}" }.join("\n")} This can cause dependency conflicts. Consider using only one package manager. - Using #{detected.first[0]} based on file precedence. + Using #{detected.first[0]} based on precedence order: yarn → pnpm → bun → npm. MSG end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
lib/generators/react_on_rails/generator_helper.rb(2 hunks)lib/generators/react_on_rails/templates/base/base/config/shakapacker.yml(0 hunks)react_on_rails_pro/spec/dummy/client/app/components/HelloWorld.module.scss(1 hunks)react_on_rails_pro/spec/dummy/package.json(1 hunks)react_on_rails_pro/spec/execjs-compatible-dummy/package.json(1 hunks)spec/dummy/client/app/components/HelloWorld.module.scss(1 hunks)
💤 Files with no reviewable changes (1)
- lib/generators/react_on_rails/templates/base/base/config/shakapacker.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- react_on_rails_pro/spec/dummy/client/app/components/HelloWorld.module.scss
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-01-23T18:20:45.824Z
Learnt from: alexeyr-ci
Repo: shakacode/react_on_rails PR: 1687
File: spec/dummy/package.json:0-0
Timestamp: 2025-01-23T18:20:45.824Z
Learning: When adding or updating dependencies in spec/dummy/package.json, maintain version consistency with other package.json files in the codebase to avoid potential version conflicts.
Applied to files:
react_on_rails_pro/spec/dummy/package.jsonreact_on_rails_pro/spec/execjs-compatible-dummy/package.json
📚 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/package.jsonreact_on_rails_pro/spec/execjs-compatible-dummy/package.jsonlib/generators/react_on_rails/generator_helper.rb
📚 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/package.jsonreact_on_rails_pro/spec/execjs-compatible-dummy/package.json
📚 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/package.jsonreact_on_rails_pro/spec/execjs-compatible-dummy/package.jsonspec/dummy/client/app/components/HelloWorld.module.scss
📚 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:
lib/generators/react_on_rails/generator_helper.rb
🧬 Code graph analysis (1)
lib/generators/react_on_rails/generator_helper.rb (1)
lib/generators/react_on_rails/generator_messages.rb (1)
add_warning(13-15)
⏰ 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). (1)
- GitHub Check: examples (3.2, minimum)
🔇 Additional comments (4)
spec/dummy/client/app/components/HelloWorld.module.scss (2)
1-1: File import path verified and is accessible.The imported
app-variables.scssfile exists at the expected location. The relative import path is correct and the module will load successfully.
1-4: CSS Modules configuration is correctly aligned with Shakapacker 9.3.0.The webpack configuration explicitly sets
namedExport: falseandexportLocalsConvention: 'camelCase', which perfectly matches the camelCase class name.brightColorin the SCSS file. The configuration is intentionally set for backward compatibility with default imports (import styles from './file.module.css'), and the code follows this pattern correctly. No changes are needed.react_on_rails_pro/spec/execjs-compatible-dummy/package.json (1)
33-35: Verify dependency version consistency across dummy/spec package.json files.The PR updates shakapacker and adds SWC tooling in multiple
package.jsonfiles. Ensure that versions forshakapacker,swc-loader, and@swc/coreare consistent acrossspec/dummy/package.jsonandreact_on_rails_pro/spec/execjs-compatible-dummy/package.jsonto prevent version conflicts during builds and tests.Based on learnings from prior PRs, inconsistent versions across dummy setups can cause integration issues. Can you verify that these versions match in both locations?
lib/generators/react_on_rails/generator_helper.rb (1)
5-5: LGTM! GeneratorMessages is now accessible.The
require_relativestatement properly addresses the previous concern aboutGeneratorMessagesavailability.
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)
package-scripts.yml(1 hunks)react_on_rails_pro/spec/dummy/package.json(2 hunks)react_on_rails_pro/spec/execjs-compatible-dummy/package.json(2 hunks)spec/dummy/babel.config.js(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 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:
package-scripts.ymlreact_on_rails_pro/spec/execjs-compatible-dummy/package.jsonreact_on_rails_pro/spec/dummy/package.json
📚 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:
package-scripts.ymlreact_on_rails_pro/spec/execjs-compatible-dummy/package.jsonreact_on_rails_pro/spec/dummy/package.json
📚 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:
package-scripts.ymlreact_on_rails_pro/spec/execjs-compatible-dummy/package.jsonreact_on_rails_pro/spec/dummy/package.json
📚 Learning: 2025-01-23T18:20:45.824Z
Learnt from: alexeyr-ci
Repo: shakacode/react_on_rails PR: 1687
File: spec/dummy/package.json:0-0
Timestamp: 2025-01-23T18:20:45.824Z
Learning: When adding or updating dependencies in spec/dummy/package.json, maintain version consistency with other package.json files in the codebase to avoid potential version conflicts.
Applied to files:
react_on_rails_pro/spec/execjs-compatible-dummy/package.jsonspec/dummy/babel.config.jsreact_on_rails_pro/spec/dummy/package.json
⏰ 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). (11)
- GitHub Check: build-dummy-app-webpack-test-bundles (3.4, 22)
- GitHub Check: examples (3.2, minimum)
- GitHub Check: examples (3.4, latest)
- GitHub Check: build-dummy-app-webpack-test-bundles (3.2, 20)
- GitHub Check: lint-js-and-ruby
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: claude-review
- GitHub Check: markdown-link-check
- GitHub Check: rspec-package-tests (3.4, minimum)
- GitHub Check: rspec-package-tests (3.2, minimum)
- GitHub Check: rspec-package-tests (3.2, latest)
🔇 Additional comments (3)
spec/dummy/babel.config.js (1)
1-2: LGTM! Necessary update for shakapacker 9.3.0 compatibility.The explicit
.jsextension and ESLint directive are appropriate for the updated shakapacker module resolution requirements. The configuration logic remains unchanged.react_on_rails_pro/spec/dummy/package.json (1)
81-81: Correct placement of build-time dependencies.All build-time tools (shakapacker, @swc/core, swc-loader) are correctly placed in devDependencies, which aligns with npm conventions for spec fixtures and is consistent with the shakapacker 9.3.0 upgrade.
Also applies to: 91-92
package-scripts.yml (1)
29-29: LGTM! Package manager consistency.The switch from
npm run buildtoyarn run buildaligns with the project's use of Yarn as the primary package manager and is consistent across the codebase updates.
…ssing require Addresses 3 critical issues identified in PR #1896 review: 1. Remove invalid javascript_transpiler configuration - Shakapacker 9.3.0 does not support a top-level javascript_transpiler key - Removed lines 39-42 from shakapacker.yml template - Transpiler selection is configured via webpack config and installed loaders 2. Update Pro package.json files to Shakapacker 9.3.0 - Updated react_on_rails_pro/spec/dummy/package.json (8.0.0 → 9.3.0) - Updated react_on_rails_pro/spec/execjs-compatible-dummy/package.json (8.0.0 → 9.3.0) - Added swc-loader and @swc/core dependencies to both files - Ensures version consistency across the codebase 3. Fix missing GeneratorMessages require in generator_helper.rb - Added require_relative "generator_messages" to prevent NameError - Issue occurred when detect_package_manager_and_exact_flag found multiple lock files - Now consistent with other generator files (base_generator.rb, install_generator.rb) All RuboCop checks pass. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
justin808
force-pushed
the
justin808/shakapacker-9.3.0
branch
from
ce33ba8 to
9f254fb
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/generators/react_on_rails/install_generator.rb (1)
545-555: Update manual install instructions to respect the detected package manager.This warning always tells users to run
npm install, even when we just attempted Yarn/Pnpm/Bun. On Yarn and Pnpm projects that guidance leads to duplicate lockfiles and conflicting dependency state. Please build the fallback command fromdetect_package_manager_and_exact_flagbefore emitting the message so we point developers to the right tool.- install_command = dev ? "npm install --save-dev" : "npm install" + package_manager, _exact_flag, add_command = detect_package_manager_and_exact_flag + install_command = + case package_manager + when "npm" + dev ? "npm #{add_command} --save-dev" : "npm #{add_command}" + when "yarn" + dev ? "yarn #{add_command} -D" : "yarn #{add_command}" + when "pnpm" + dev ? "pnpm #{add_command} -D" : "pnpm #{add_command}" + when "bun" + dev ? "bun #{add_command} --dev" : "bun #{add_command}" + else + dev ? "npm install --save-dev" : "npm install" + end
🧹 Nitpick comments (1)
package_json_issue_comment.md (1)
1-57: Clarify the file's purpose and intended location.The filename
package_json_issue_comment.mdsuggests this documents an issue or discussion, but it's unclear whether this is meant as permanent documentation in the codebase. Based on the enriched summary, this file appears to explain the motivation behind the package manager detection helper and exact-install flow changes.If this is intended as permanent documentation, consider:
Relocate to a dedicated directory — Move to
docs/decisions/(or similar ADR/decision-log directory) with a clearer name likepackage-manager-exact-install-support.mdto indicate it's an architectural decision or design record.Add integration points — Reference this from relevant code files (e.g.,
lib/generators/react_on_rails/generator_helper.rbandlib/generators/react_on_rails/install_generator.rb) via inline comments explaining the rationale for the approach.Remove if temporary — If this is a temporary artifact (issue comment saved for reference), consider removing it or storing it in a separate issue/discussion tracker rather than committing it to the main codebase.
Add cross-references — Link to the specific generator methods and explain how the problem described here is solved by the new package manager detection flow.
Based on learnings, consistent documentation across the codebase helps maintain clarity on version management decisions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
Gemfile.lockis excluded by!**/*.lockspec/dummy/Gemfile.lockis excluded by!**/*.lockspec/dummy/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (20)
.github/workflows/lint-js-and-ruby.yml(1 hunks)Gemfile.development_dependencies(1 hunks)ISSUE_ANALYSIS.md(1 hunks)knip.ts(3 hunks)lib/generators/react_on_rails/base_generator.rb(1 hunks)lib/generators/react_on_rails/generator_helper.rb(2 hunks)lib/generators/react_on_rails/install_generator.rb(6 hunks)lib/generators/react_on_rails/templates/base/base/config/shakapacker.yml(0 hunks)lib/react_on_rails/engine.rb(1 hunks)package-scripts.yml(1 hunks)package_json_issue_comment.md(1 hunks)rakelib/shakapacker_examples.rake(1 hunks)react_on_rails_pro/spec/dummy/client/app/components/HelloWorld.module.scss(1 hunks)react_on_rails_pro/spec/dummy/package.json(2 hunks)react_on_rails_pro/spec/execjs-compatible-dummy/package.json(2 hunks)script/convert(1 hunks)spec/dummy/babel.config.js(1 hunks)spec/dummy/client/app/components/HelloWorld.module.scss(1 hunks)spec/dummy/config/webpack/commonWebpackConfig.js(1 hunks)spec/dummy/package.json(2 hunks)
💤 Files with no reviewable changes (1)
- lib/generators/react_on_rails/templates/base/base/config/shakapacker.yml
🚧 Files skipped from review as they are similar to previous changes (10)
- package-scripts.yml
- react_on_rails_pro/spec/dummy/client/app/components/HelloWorld.module.scss
- Gemfile.development_dependencies
- react_on_rails_pro/spec/execjs-compatible-dummy/package.json
- spec/dummy/config/webpack/commonWebpackConfig.js
- react_on_rails_pro/spec/dummy/package.json
- rakelib/shakapacker_examples.rake
- knip.ts
- spec/dummy/client/app/components/HelloWorld.module.scss
- lib/generators/react_on_rails/generator_helper.rb
🧰 Additional context used
🧠 Learnings (11)
📚 Learning: 2025-01-23T18:20:45.824Z
Learnt from: alexeyr-ci
Repo: shakacode/react_on_rails PR: 1687
File: spec/dummy/package.json:0-0
Timestamp: 2025-01-23T18:20:45.824Z
Learning: When adding or updating dependencies in spec/dummy/package.json, maintain version consistency with other package.json files in the codebase to avoid potential version conflicts.
Applied to files:
spec/dummy/babel.config.jsscript/convertspec/dummy/package.jsonISSUE_ANALYSIS.mdlib/generators/react_on_rails/base_generator.rbpackage_json_issue_comment.md
📚 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:
script/convertlib/generators/react_on_rails/install_generator.rb.github/workflows/lint-js-and-ruby.ymlISSUE_ANALYSIS.mdlib/generators/react_on_rails/base_generator.rb
📚 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:
script/convertlib/generators/react_on_rails/install_generator.rblib/react_on_rails/engine.rb.github/workflows/lint-js-and-ruby.ymlISSUE_ANALYSIS.mdlib/generators/react_on_rails/base_generator.rb
📚 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:
lib/generators/react_on_rails/install_generator.rb.github/workflows/lint-js-and-ruby.ymlISSUE_ANALYSIS.md
📚 Learning: 2025-02-18T13:08:01.477Z
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.
Applied to files:
lib/generators/react_on_rails/install_generator.rblib/react_on_rails/engine.rbISSUE_ANALYSIS.md
📚 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:
lib/generators/react_on_rails/install_generator.rblib/react_on_rails/engine.rbISSUE_ANALYSIS.md
📚 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:
lib/generators/react_on_rails/install_generator.rb
📚 Learning: 2024-12-12T13:07:09.929Z
Learnt from: alexeyr-ci
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/ReactOnRailsRSC.ts:87-87
Timestamp: 2024-12-12T13:07:09.929Z
Learning: When handling errors in 'node_package/src/ReactOnRailsRSC.ts', include the error stack in error messages in development and test environments to aid debugging.
Applied to files:
lib/generators/react_on_rails/install_generator.rblib/react_on_rails/engine.rb
📚 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:
lib/generators/react_on_rails/install_generator.rblib/react_on_rails/engine.rb
📚 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:
lib/react_on_rails/engine.rb
📚 Learning: 2025-02-18T13:08:01.477Z
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 is handled in deeper level calls of the React on Rails Pro codebase, so it doesn't need to be validated again in the `rsc_payload_react_component` helper method.
Applied to files:
lib/react_on_rails/engine.rb
🧬 Code graph analysis (2)
lib/generators/react_on_rails/install_generator.rb (2)
lib/generators/react_on_rails/generator_helper.rb (2)
detect_package_manager_and_exact_flag(103-129)add_npm_dependencies(24-40)lib/generators/react_on_rails/base_generator.rb (1)
handle_npm_failure(242-254)
lib/react_on_rails/engine.rb (2)
lib/generators/react_on_rails/generator_helper.rb (1)
package_json(8-21)lib/react_on_rails/version_checker.rb (5)
package_json_path(222-224)build(12-14)build(218-220)react_on_rails_package?(254-256)react_on_rails_pro_package?(258-260)
🪛 LanguageTool
ISSUE_ANALYSIS.md
[uncategorized] ~463-~463: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...ase failures 3. Fix Issue #2 last - Low priority defensive programming improvement ## T...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
🪛 markdownlint-cli2 (0.18.1)
ISSUE_ANALYSIS.md
42-42: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
89-89: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
108-108: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
158-158: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
189-189: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
215-215: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
282-282: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
321-321: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
372-372: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
440-440: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
⏰ 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). (12)
- GitHub Check: dummy-app-integration-tests (3.2, 20)
- GitHub Check: dummy-app-integration-tests (3.4, 22)
- GitHub Check: lint-js-and-ruby
- GitHub Check: rspec-package-tests (3.4, minimum)
- GitHub Check: rspec-package-tests (3.2, minimum)
- GitHub Check: rspec-package-tests (3.2, latest)
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: examples (3.4, latest)
- GitHub Check: examples (3.2, minimum)
- GitHub Check: build
- GitHub Check: claude-review
- GitHub Check: markdown-link-check
🔇 Additional comments (1)
package_json_issue_comment.md (1)
24-35: Verify this workaround is no longer needed after PR changes.The code snippet shows a direct
npm install --save-exactworkaround. Confirm that the new package manager detection helper and exact-install flow (mentioned in the enriched summary) fully addresses this limitation, and that this workaround code is either replaced or deprecated in the current PR.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
lib/react_on_rails/dev/pack_generator.rb (1)
46-64: IO redirection is not thread-safe.Directly assigning to
$stdoutand$stderraffects all threads in the process and can cause issues if multiple threads are writing simultaneously. While this may be acceptable for a single-threaded context, consider documenting this limitation or using a thread-safe alternative like capturing output within the Rake task itself.lib/generators/react_on_rails/base_generator.rb (1)
221-229: Refactor to use proper YAML parsing instead of string matching.The current implementation uses simple string matching (
include?), which is fragile and could lead to false positives or negatives:
- ❌ Matches commented lines:
# javascript_compiler: swc- ❌ Misses whitespace variations:
javascript_compiler: swc(extra spaces)- ❌ Doesn't respect YAML structure or default sections
Apply this diff to parse YAML properly:
def using_swc? # Check shakapacker.yml for javascript_compiler setting # Default to babel if not specified shakapacker_config_path = File.join(destination_root, "config", "shakapacker.yml") return false unless File.exist?(shakapacker_config_path) - config_content = File.read(shakapacker_config_path) - config_content.include?("javascript_compiler: swc") + require 'yaml' + config = YAML.load_file(shakapacker_config_path) + # Check the default section for javascript_compiler setting + config.dig("default", "javascript_compiler") == "swc" +rescue StandardError + false end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
lib/generators/react_on_rails/base_generator.rb(2 hunks)lib/generators/react_on_rails/templates/base/base/config/shakapacker.yml(1 hunks)lib/react_on_rails/dev/pack_generator.rb(2 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 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:
lib/generators/react_on_rails/templates/base/base/config/shakapacker.ymllib/generators/react_on_rails/base_generator.rb
📚 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:
lib/generators/react_on_rails/templates/base/base/config/shakapacker.ymllib/react_on_rails/dev/pack_generator.rblib/generators/react_on_rails/base_generator.rb
📚 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:
lib/generators/react_on_rails/templates/base/base/config/shakapacker.ymllib/generators/react_on_rails/base_generator.rb
📚 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:
lib/generators/react_on_rails/templates/base/base/config/shakapacker.ymllib/react_on_rails/dev/pack_generator.rblib/generators/react_on_rails/base_generator.rb
📚 Learning: 2025-09-29T14:45:42.687Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1833
File: lib/react_on_rails/dev/process_manager.rb:72-83
Timestamp: 2025-09-29T14:45:42.687Z
Learning: In Ruby bundler contexts, when bundler intercepts system commands for executables not in the Gemfile, both version checks (like `system("foreman", "--version")`) and execution commands (like `system("foreman", "start", ...)`) fail equally, both returning false. This means availability checks using version flags accurately reflect whether execution commands will work in the current bundler context.
Applied to files:
lib/react_on_rails/dev/pack_generator.rb
⏰ 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). (12)
- GitHub Check: dummy-app-integration-tests (3.2, 20)
- GitHub Check: dummy-app-integration-tests (3.4, 22)
- GitHub Check: rspec-package-tests (3.2, latest)
- GitHub Check: markdown-link-check
- GitHub Check: examples (3.2, minimum)
- GitHub Check: rspec-package-tests (3.4, minimum)
- GitHub Check: rspec-package-tests (3.2, minimum)
- GitHub Check: build
- GitHub Check: lint-js-and-ruby
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: examples (3.4, latest)
- GitHub Check: claude-review
🔇 Additional comments (2)
lib/react_on_rails/dev/pack_generator.rb (1)
27-35: No actionable changes required—this is an established and idiomatic pattern in the codebase.The
defined?(Bundler)check is the appropriate way to verify whether the Bundler constant is available before calling Bundler methods. This pattern is already established in the same codebase (process_manager.rblines 91, 103) and uses a consistent two-stage approach: first checking if Bundler is defined, then usingrespond_to?checks for specific methods (as seen inwith_unbundled_context). Prior verification of this pattern in bundler contexts confirmed it correctly reflects whether execution paths will work in the current environment. The check serves its intended purpose in routing to the appropriate execution strategy.Likely an incorrect or invalid review comment.
lib/generators/react_on_rails/base_generator.rb (1)
176-187: LGTM! Clear conditional dependency management.The conditional logic cleanly separates SWC and Babel dependencies based on the compiler configuration, with helpful inline comments.
lib/generators/react_on_rails/templates/base/base/config/shakapacker.yml
Outdated
Show resolved
Hide resolved
| require "rake" | ||
|
|
||
| # Load Rails environment if not already loaded | ||
| require File.expand_path("config/environment", Dir.pwd) unless defined?(Rails) |
There was a problem hiding this comment.
Rails root path assumption may fail.
Using Dir.pwd assumes the current working directory is the Rails root, which may not hold true in all contexts (e.g., when called from subdirectories or during certain deployment scenarios). Consider using Rails.root if Rails is already loaded, or searching upward for config/environment.rb.
Apply this diff to use a more robust path resolution:
- require File.expand_path("config/environment", Dir.pwd) unless defined?(Rails)
+ unless defined?(Rails)
+ # Search upward for Rails root
+ rails_env = Dir.pwd
+ while rails_env != "/" && !File.exist?(File.join(rails_env, "config", "environment.rb"))
+ rails_env = File.dirname(rails_env)
+ end
+ raise "Could not find Rails root from #{Dir.pwd}" if rails_env == "/"
+ require File.expand_path("config/environment", rails_env)
+ end🤖 Prompt for AI Agents
In lib/react_on_rails/dev/pack_generator.rb around line 41, replace the brittle
use of Dir.pwd when requiring config/environment with a robust path resolution:
if Rails is already defined and Rails.root is available use Rails.root,
otherwise walk upward from Dir.pwd (or __dir__) searching parent directories for
config/environment.rb and use that directory as the root; finally call require
File.expand_path("config/environment", resolved_root) unless defined?(Rails).
Ensure the search stops at filesystem root and raises a clear error if
config/environment.rb is not found.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
TURBO_NAVIGATION_TEST_ISSUE.md (1)
144-157: Consider file organization: Move to dedicated directory.This comprehensive test plan document would be better served in a dedicated documentation directory (e.g.,
docs/contributor-info/ordocs/testing/) rather than at the project root, aligning with the structure suggested in lines 155-157 and improving discoverability.lib/react_on_rails/dev/server_manager.rb (1)
433-445: Consider adding verbose parameter for consistency with PackGenerator.The method doesn't accept a
verboseparameter likePackGenerator.generate(verbose: verbose)does, creating an inconsistent user experience. When users pass--verbose, they would expect to see detailed output for all build steps, including ReScript compilation.Apply this diff to align with the PackGenerator pattern:
def run_static_development(procfile, verbose: false, route: nil) print_procfile_info(procfile, route: route) print_server_info( "⚡ Starting development server with static assets...", [ "Generating React on Rails packs", "Using shakapacker --watch (no HMR)", "CSS extracted to separate files (no FOUC)", "Development environment (source maps, faster builds)", "Auto-recompiles on file changes" ], route: route ) - build_rescript_if_needed + build_rescript_if_needed(verbose: verbose) PackGenerator.generate(verbose: verbose) ProcessManager.ensure_procfile(procfile) ProcessManager.run_with_process_manager(procfile) end def run_development(procfile, verbose: false, route: nil) print_procfile_info(procfile, route: route) - build_rescript_if_needed + build_rescript_if_needed(verbose: verbose) PackGenerator.generate(verbose: verbose) ProcessManager.ensure_procfile(procfile) ProcessManager.run_with_process_manager(procfile) end -def build_rescript_if_needed +def build_rescript_if_needed(verbose: false) # Check for both old (bsconfig.json) and new (rescript.json) config files return unless File.exist?("bsconfig.json") || File.exist?("rescript.json") - print "🔧 Building ReScript... " - success = system("yarn build:rescript > /dev/null 2>&1") - puts success ? "✅" : "❌" + if verbose + puts "🔧 Building ReScript..." + success = system("yarn build:rescript") + else + print "🔧 Building ReScript... " + success = system("yarn build:rescript > /dev/null 2>&1") + puts success ? "✅" : "❌" + end return if success puts "❌ ReScript build failed" exit 1 end.github/workflows/pro-integration-tests.yml (1)
70-75: Deduplicate identical yalc publish steps across jobs.The "Build and publish packages with yalc" steps are identical across all three jobs (lines 70–75, 190–195, 382–387). This violates the DRY principle and increases maintenance burden. Consider using a reusable workflow or composite action to eliminate duplication.
Example using a composite action:
# .github/actions/build-publish-yalc/action.yml name: Build and Publish Packages with yalc runs: using: composite steps: - name: Build and publish packages shell: bash run: | cd .. yarn run yalc:publish cd react_on_rails_pro yarn run yalc:publishThen replace each occurrence with:
- uses: ./.github/actions/build-publish-yalcAlso applies to: 190-195, 382-387
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/pro-integration-tests.yml(3 hunks)TURBO_NAVIGATION_TEST_ISSUE.md(1 hunks)lib/react_on_rails/dev/server_manager.rb(1 hunks)spec/dummy/app/views/layouts/application.html.erb(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 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:
lib/react_on_rails/dev/server_manager.rb
📚 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:
lib/react_on_rails/dev/server_manager.rb
📚 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:
lib/react_on_rails/dev/server_manager.rb
📚 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:
TURBO_NAVIGATION_TEST_ISSUE.mdspec/dummy/app/views/layouts/application.html.erb
📚 Learning: 2024-10-08T20:53:47.076Z
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.
Applied to files:
spec/dummy/app/views/layouts/application.html.erb
🧬 Code graph analysis (1)
lib/react_on_rails/dev/server_manager.rb (1)
lib/react_on_rails/dev/pack_generator.rb (2)
generate(7-75)generate(9-23)
🪛 LanguageTool
TURBO_NAVIGATION_TEST_ISSUE.md
[style] ~30-~30: This sentence contains multiple usages of the word “again”. Consider removing or replacing it.
Context: ...d refreshing again → ✅ JavaScript works again This severely degraded the user experi...
(REPETITION_OF_AGAIN)
🪛 markdownlint-cli2 (0.18.1)
TURBO_NAVIGATION_TEST_ISSUE.md
146-146: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: build-dummy-app-webpack-test-bundles (3.2, 20)
- GitHub Check: examples (3.4, latest)
- GitHub Check: examples (3.2, minimum)
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: rspec-package-tests (3.2, latest)
- GitHub Check: rspec-package-tests (3.4, minimum)
- GitHub Check: rspec-package-tests (3.2, minimum)
- GitHub Check: claude-review
- GitHub Check: build
- GitHub Check: markdown-link-check
- GitHub Check: lint-js-and-ruby
🔇 Additional comments (5)
TURBO_NAVIGATION_TEST_ISSUE.md (2)
1-10: Verify alignment with PR objectives.This file documents a Turbo navigation regression test plan, but the PR objectives describe updating shakapacker from 8.2.0 to 9.3.0. There's no evident connection between these changes. Clarify whether this file should be included in this PR or if it belongs to a separate issue/PR.
42-84: Comprehensive test plan with strong verification approach.The proposed Playwright test scenario is well-structured and effectively captures the regression scenario. The test matrix covering initial load, post-navigation, and back-navigation provides good coverage. The verification steps with explicit fail/pass instructions make this implementation checklist actionable.
spec/dummy/app/views/layouts/application.html.erb (2)
13-13: LGTM! Migration to Turbo and defer attribute aligns with modern Rails.The change correctly migrates from Turbolinks to Turbo tracking (
data-turbo-track: 'reload') and addsdefer: true, which aligns perfectly with the comment on line 12 about using defer for proper script loading order.
13-13: Multiple ERB templates and other files still contain Turbolinks references that need updating alongside this change.The search reveals that beyond the single change shown in
application.html.erb, the following files in the main codebase (excluding fixtures/test bundles) should be reviewed and updated as part of the Turbolinks-to-Turbo migration:
- ERB templates with
data-turbolinks-trackattributes: These need to be updated todata-turbo-trackto match the change in the layout file.- ERB meta tags:
<meta name="turbolinks-cache-control">should be updated to<meta name="turbo-cache-control">.- Test descriptions: The integration test file contains "Turbolinks across pages" which could be updated to reflect Turbo.
- Route names/helpers:
turbolinks_cache_disabled_pathis semantic but may need renaming for consistency.The compiled JavaScript bundle files in fixtures show legacy Turbolinks event handling code (e.g.,
turbolinks:before-render,turbolinks:render), which appears to be intentionally preserved for fixture/snapshot purposes and likely don't require changes.lib/react_on_rails/dev/server_manager.rb (1)
419-419: LGTM! Appropriate placement for ReScript build step.The ReScript build is correctly positioned before pack generation in both development modes, ensuring compiled ReScript code is available when packs are generated.
Also applies to: 427-427
| success = system("yarn build:rescript > /dev/null 2>&1") | ||
| puts success ? "✅" : "❌" | ||
|
|
||
| return if success | ||
|
|
||
| puts "❌ ReScript build failed" | ||
| exit 1 |
There was a problem hiding this comment.
Suppressed error output makes debugging ReScript build failures difficult.
When the ReScript build fails, all compilation errors are discarded (> /dev/null 2>&1), leaving developers with only a generic "ReScript build failed" message. Compiler errors, type mismatches, and syntax issues won't be visible.
Consider showing the actual error output when the build fails:
-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")
-
- print "🔧 Building ReScript... "
- success = system("yarn build:rescript > /dev/null 2>&1")
- puts success ? "✅" : "❌"
-
- return if success
-
- puts "❌ ReScript build failed"
- exit 1
-end
+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")
+
+ print "🔧 Building ReScript... "
+ stdout, stderr, status = Open3.capture3("yarn", "build:rescript")
+ puts status.success? ? "✅" : "❌"
+
+ return if status.success?
+
+ puts "❌ ReScript build failed"
+ puts ""
+ puts Rainbow("📋 Build Output:").red.bold
+ puts Rainbow("─" * 60).red
+ puts stdout unless stdout.empty?
+ puts stderr unless stderr.empty?
+ puts Rainbow("─" * 60).red
+ exit 1
+endCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In lib/react_on_rails/dev/server_manager.rb around lines 438 to 444, the
ReScript build call suppresses all output ("> /dev/null 2>&1") so errors are
hidden; change the invocation so stdout/stderr are captured and only suppressed
on success — run the build without redirecting or capture the output (e.g., use
Open3.capture3 or backticks) and if the command fails print the captured
stderr/stdout before exiting so developers see the compiler errors and
diagnostics.
…ISSUE.md Replaced "PR #XXXX" with "PR #1896" since commit f03b935 that fixed the Turbo navigation bug is part of this PR. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
This comment explains React on Rails' use case for exact version support and provides context for why we had to implement a workaround. To be posted at: shakacode/package_json#25 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
The HelloWorld.module.scss files were using $bright-color variable without importing the app-variables.scss file where it's defined. This was causing webpack build failures. Changes: - spec/dummy/client/app/components/HelloWorld.module.scss: Add @import - react_on_rails_pro/spec/dummy/client/app/components/HelloWorld.module.scss: Add @import - knip.ts: Auto-cleaned unused dependency ignores 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
…n skip, and webpack safety This commit addresses three code review issues: 1. Fix package manager hard-coding (High Priority) - Add detect_package_manager_and_exact_flag helper to GeneratorHelper - Detect package manager from lock files (yarn, pnpm, bun, npm) - Update all package installation methods to use detected PM - Warn users when multiple lock files are detected - Fixes issue where npm was hard-coded, causing conflicts for yarn/pnpm/bun users 2. Fix fragile ARGV-based generator detection (Medium Priority) - Replace ARGV.any? check with explicit ENV variable - Install generator sets REACT_ON_RAILS_SKIP_VALIDATION=true - More robust than string matching on ARGV - Works correctly in all contexts (rake, console, tests) - Prevents false positives from paths containing "generate" or "g" 3. Add defensive checks to webpack CSS Modules config (Low Priority) - Add comprehensive type and existence checks before accessing loader properties - Prevents potential runtime errors with malformed webpack configs - Consistent with defensive pattern used for SCSS configuration All changes verified with: - bundle exec rubocop: 0 offenses - Install generator specs: 53 examples, 0 failures Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
The version checker was failing during `rails generate react_on_rails:install` because it ran after package.json was created by shakapacker:install but before the generator had a chance to install the react-on-rails package. Error was: **ERROR** ReactOnRails: No React on Rails npm package is installed. This commit adds an additional check to skip validation if neither react-on-rails nor react-on-rails-pro package is found in package.json, allowing the generator to complete its installation process. The validation will still run normally once the packages are installed, ensuring version compatibility in production apps. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
The sass-resources-loader webpack plugin automatically injects app-variables.scss into all SCSS files, making manual @import statements redundant and potentially problematic. The previous fix attempted to import app-variables.scss to resolve undefined $bright-color variables, but this caused issues with React Server Components builds. The variables are already available through the webpack sass-resources-loader configuration. Changes: - spec/dummy/client/app/components/HelloWorld.module.scss: Remove @import - react_on_rails_pro/spec/dummy/client/app/components/HelloWorld.module.scss: Remove @import Fixes React Router Sixth Page RSC test failure. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
…ssing require Addresses 3 critical issues identified in PR #1896 review: 1. Remove invalid javascript_transpiler configuration - Shakapacker 9.3.0 does not support a top-level javascript_transpiler key - Removed lines 39-42 from shakapacker.yml template - Transpiler selection is configured via webpack config and installed loaders 2. Update Pro package.json files to Shakapacker 9.3.0 - Updated react_on_rails_pro/spec/dummy/package.json (8.0.0 → 9.3.0) - Updated react_on_rails_pro/spec/execjs-compatible-dummy/package.json (8.0.0 → 9.3.0) - Added swc-loader and @swc/core dependencies to both files - Ensures version consistency across the codebase 3. Fix missing GeneratorMessages require in generator_helper.rb - Added require_relative "generator_messages" to prevent NameError - Issue occurred when detect_package_manager_and_exact_flag found multiple lock files - Now consistent with other generator files (base_generator.rb, install_generator.rb) All RuboCop checks pass. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Add @import statement to HelloWorld.module.scss files to import app-variables.scss which defines $bright-color variable. This fixes the "Undefined variable" SCSS compilation error during webpack builds. Fixed in both: - spec/dummy/client/app/components/HelloWorld.module.scss - react_on_rails_pro/spec/dummy/client/app/components/HelloWorld.module.scss 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
…files These are build-time tools and should be in devDependencies, not dependencies. Changes: - react_on_rails_pro/spec/dummy/package.json: Moved @swc/core and swc-loader to devDependencies - react_on_rails_pro/spec/execjs-compatible-dummy/package.json: Moved @swc/core and swc-loader to devDependencies Both entries maintain the same version strings (^1.7.0 and ^0.2.6). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Shakapacker is a build-time tool and should be in devDependencies, not dependencies. Changed in react_on_rails_pro/spec/dummy/package.json: - Removed "shakapacker": "9.3.0" from dependencies - Added "shakapacker": "9.3.0" to devDependencies This completes the correct categorization of all build-time tools (@swc/core, swc-loader, and shakapacker) as devDependencies. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Change the build.prepack script to use 'yarn run build' instead of 'npm run build'. In a yarn workspace, using npm to run scripts that depend on workspace features (like 'yarn workspace react-on-rails run build') causes build failures. This fixes the yalc package publishing failures in CI where the prepack script would fail silently, resulting in packages being published without the built files, leading to downstream errors. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
1. Revert package-scripts.yml change (npm->yarn) - This file doesn't exist on master, so the change shouldn't be included 2. Add javascript_compiler setting to shakapacker.yml template - Adds configuration option to choose between 'babel' or 'swc' - Defaults to 'babel' for wider ecosystem support - Documents the tradeoffs between the two compilers 3. Fix swc/babel dependency management - Move swc packages (@swc/core, swc-loader) from devDependencies to dependencies - Make swc and babel mutually exclusive based on shakapacker.yml setting - Add using_swc? helper method to check javascript_compiler configuration - When using swc, install swc packages instead of babel packages This ensures proper dependency installation based on the chosen JavaScript compiler. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
When bin/dev starts, webpack immediately tries to build bundles that import ReScript components (HelloWorldReScript.res.js). However, these .res.js files are generated by the ReScript compiler from .res source files, and webpack fails if they don't exist yet. Solution: Build ReScript files before starting the Procfile processes. This ensures all .res.js files are generated before webpack starts. Changes: - lib/react_on_rails/dev/server_manager.rb: - Add build_rescript_if_needed method to compile ReScript before dev server - Check for both bsconfig.json (old) and rescript.json (new) config files - Run yarn build:rescript if ReScript is configured - Exit with error if ReScript build fails Fixes webpack Module not found errors for HelloWorldReScript.res.js 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Implement explicit yalc publishing before yarn install to avoid frozen-lockfile conflicts. This resolves the issue where preinstall scripts triggered yalc operations that modified yarn.lock, causing CI to fail with "--frozen-lockfile" checks. Changes: 1. Add explicit "Build and publish packages with yalc" step that runs yarn run yalc:publish for both root and Pro packages before install 2. Add --ignore-scripts flag to Pro dummy app yarn install to skip preinstall hook (since yalc already published in previous step) 3. Apply to all 3 jobs: build, rspec, and playwright tests This approach: - Separates build/publish from install for better CI reproducibility - Prevents yarn.lock modifications during install - Maintains frozen-lockfile enforcement for dependency stability - Faster CI (no redundant builds during preinstall) The preinstall script remains useful for local development to auto-update yalc packages, but in CI we use explicit steps for better control. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
… regression Documents a regression that existed for over 1 year where JavaScript failed to work after Turbo navigation (only worked on hard refresh). This issue provides a comprehensive plan for adding Playwright E2E tests to prevent this type of regression in the future. The issue includes: - Background on what happened and when the bug was introduced (PR #1620) - Impact on users (broken JS after navigation) - Proposed Playwright test implementation - Verification steps to prove the test catches the regression - Additional test cases to consider - Success criteria and implementation checklist To verify Playwright is working correctly, developers can: 1. Run the test with the current fix (should pass) 2. Revert the fix in application.html.erb (should fail) 3. Restore the fix (should pass again) This proves the test effectively catches Turbo navigation issues. Related commits: - Bug introduced: 56ee2bd (PR #1620, ~1 year ago) - Bug fixed: f03b935 (this PR) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Change 'yarn run yalc:publish' to 'yarn && yalc publish' for the react_on_rails_pro package, since it doesn't have a yalc:publish script defined (only the workspace packages do). The workflow now: 1. Runs yarn run yalc:publish at root (publishes workspace packages) 2. Runs yarn && yalc publish in react_on_rails_pro (installs + publishes Pro) This matches the command structure used in the preinstall script. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
The build.prepack script in package-scripts.yml was using `npm run build` which causes failures in CI. This is inconsistent with the rest of the codebase which uses yarn. Changed line 29 from: (npm run build >/dev/null 2>&1 || true) To: (yarn run build >/dev/null 2>&1 || true) This matches the fix previously made in packages/react-on-rails/package.json (commit 9f254fb) but was missed in the root package-scripts.yml file. Fixes CI error: Building react-on-rails seems to have failed! 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Added 'tree' as the language identifier for the fenced code block showing the file structure, changing from generic ``` to ```tree for better syntax highlighting and clarity. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
…ISSUE.md Replaced "PR #XXXX" with "PR #1896" since commit f03b935 that fixed the Turbo navigation bug is part of this PR. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
The yalc workflow modifies yarn.lock when adding packages, so we cannot use --frozen-lockfile after yalc has run. Keep --ignore-scripts to skip the preinstall hook (since yalc already ran in the previous step). This allows the install to succeed after yalc has modified the lockfile. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Updated shakapacker gem version from 8.0.0 to 9.3.0 in Pro Gemfiles to match
the package.json changes, fixing the version mismatch error:
"Shakapacker gem and node package versions do not match
Detected: 9.3.0
gem: 8.0.0"
Changed in:
- react_on_rails_pro/Gemfile.development_dependencies (8.0.0 → 9.3.0)
- react_on_rails_pro/spec/execjs-compatible-dummy/Gemfile (8.0 → 9.3.0)
This ensures gem and npm package versions match across all Pro dummy apps.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <[email protected]>
Updated yarn.lock to match package.json changes for @swc/core and swc-loader dependencies moved to devDependencies. Fixes CI lint-js-and-ruby failure: "Your lockfile needs to be updated, but yarn was run with --frozen-lockfile" 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Updated yarn.lock to match package.json changes for @swc/core and swc-loader dependencies moved to devDependencies in the execjs-compatible-dummy app. Fixes CI lint-js-and-ruby failure for ExecJS dummy app: "Your lockfile needs to be updated, but yarn was run with --frozen-lockfile" 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Shakapacker 9.3.0 defaults to SWC compiler if javascript_compiler is not explicitly set in shakapacker.yml. Update the using_swc? method to reflect this new default behavior. Changes: - using_swc? now returns true (SWC) by default when config doesn't exist - using_swc? returns true unless javascript_compiler is explicitly set to babel - Updated template shakapacker.yml to comment out javascript_compiler setting - Added clear documentation that Shakapacker 9.3.0+ defaults to SWC This ensures the generator correctly detects and installs SWC dependencies by default, matching Shakapacker 9.3.0's new default compiler. Addresses PR review comment on lib/generators/react_on_rails/base_generator.rb:225 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Change using_swc? helper to parse shakapacker.yml with YAML.safe_load_file instead of raw string matching, which incorrectly interpreted commented lines as active configuration. Changes: - Add require "yaml" to base_generator.rb - Parse YAML to read javascript_compiler from default section - Return true only when javascript_compiler is explicitly set to "swc" - Default to false (babel) when file is missing or key is absent - Add error handling for YAML parsing failures - Update template comment to reflect babel as default This ensures commented configuration lines don't affect compiler detection and provides more predictable behavior. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Critical fixes: 1. using_swc? already properly parses YAML (no changes needed) 2. Make precompile hook cross-platform: - Replace Unix-specific `command -v` with Ruby's system() array form - Use File::NULL for cross-platform output redirection - Use array form of system() calls for better reliability 3. Fix fragile config detection in precompile hook: - Use regex to match only actual config assignments - Ignore comments and strings containing config names 4. Remove development documentation files: - ISSUE_ANALYSIS.md - PR_REVIEW_RESPONSE.md - package_json_issue_comment.md 5. Fix Rake task management: - Remove Rake::Task.clear (was clearing ALL tasks, not just ours) - Add task.reenable to support multiple invocations These changes improve cross-platform compatibility (Windows support), prevent false positives in config detection, and fix rake task issues. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
justin808
force-pushed
the
justin808/shakapacker-9.3.0
branch
from
a229abc to
7a9f539
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
react_on_rails_pro/spec/dummy/package.json (1)
82-94: Fix conflicting shakapacker versions.Dependencies still pin 9.2.0 while devDependencies now pin 9.3.0. Yarn will refuse/dual-install because both exact ranges can’t be satisfied, and the dummy app will keep using 9.2.0 instead of the upgraded version. Please align on a single 9.3.0 entry in
dependenciesand drop the duplicate indevDependencies.- "shakapacker": "9.2.0", + "shakapacker": "9.3.0", @@ - "shakapacker": "9.3.0",spec/dummy/package.json (1)
32-57: Bump shakapacker here to 9.3.0.The spec dummy app still declares
shakapacker9.2.0, so running its build will stay on the old release. Please update this entry to9.3.0to match the rest of the upgrade.- "shakapacker": "9.2.0", + "shakapacker": "9.3.0",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
Gemfile.lockis excluded by!**/*.lockreact_on_rails_pro/spec/dummy/yarn.lockis excluded by!**/yarn.lock,!**/*.lockreact_on_rails_pro/spec/execjs-compatible-dummy/yarn.lockis excluded by!**/yarn.lock,!**/*.lockspec/dummy/Gemfile.lockis excluded by!**/*.lockspec/dummy/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (30)
.github/workflows/lint-js-and-ruby.yml(1 hunks).github/workflows/pro-integration-tests.yml(3 hunks)Gemfile.development_dependencies(1 hunks)PR_SUMMARY_REACT_19_FIX.md(1 hunks)SHAKAPACKER_UPGRADE_PR_PLAN.md(1 hunks)TURBO_NAVIGATION_TEST_ISSUE.md(1 hunks)docs/upgrading/react-19-quick-reference.md(1 hunks)docs/upgrading/react-19-upgrade-guide.md(1 hunks)docs/upgrading/upgrading-react-on-rails.md(1 hunks)eslint-rules/README.md(1 hunks)eslint.config.ts(1 hunks)lib/generators/react_on_rails/base_generator.rb(3 hunks)lib/generators/react_on_rails/generator_helper.rb(2 hunks)lib/generators/react_on_rails/install_generator.rb(6 hunks)lib/generators/react_on_rails/templates/base/base/config/shakapacker.yml(2 hunks)lib/react_on_rails/dev/pack_generator.rb(1 hunks)package-scripts.yml(1 hunks)packages/react-on-rails-pro/src/RSCProvider.tsx(4 hunks)packages/react-on-rails-pro/src/RSCRoute.tsx(3 hunks)rakelib/shakapacker_examples.rake(1 hunks)react_on_rails_pro/spec/dummy/client/app/components/HelloWorld.module.scss(1 hunks)react_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/AsyncOnServerSyncOnClient.server.tsx(0 hunks)react_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/LazyApolloGraphQLApp.server.tsx(0 hunks)react_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/ServerComponentRouter.server.tsx(0 hunks)react_on_rails_pro/spec/dummy/package.json(2 hunks)react_on_rails_pro/spec/execjs-compatible-dummy/Gemfile(2 hunks)react_on_rails_pro/spec/execjs-compatible-dummy/package.json(1 hunks)script/convert(1 hunks)spec/dummy/client/app/components/HelloWorld.module.scss(1 hunks)spec/dummy/package.json(2 hunks)
💤 Files with no reviewable changes (3)
- react_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/ServerComponentRouter.server.tsx
- react_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/AsyncOnServerSyncOnClient.server.tsx
- react_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/LazyApolloGraphQLApp.server.tsx
🚧 Files skipped from review as they are similar to previous changes (11)
- rakelib/shakapacker_examples.rake
- .github/workflows/lint-js-and-ruby.yml
- lib/generators/react_on_rails/generator_helper.rb
- spec/dummy/client/app/components/HelloWorld.module.scss
- lib/generators/react_on_rails/install_generator.rb
- script/convert
- lib/react_on_rails/dev/pack_generator.rb
- eslint.config.ts
- react_on_rails_pro/spec/execjs-compatible-dummy/Gemfile
- react_on_rails_pro/spec/dummy/client/app/components/HelloWorld.module.scss
- Gemfile.development_dependencies
🧰 Additional context used
🧠 Learnings (16)
📚 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:
lib/generators/react_on_rails/templates/base/base/config/shakapacker.ymllib/generators/react_on_rails/base_generator.rbdocs/upgrading/react-19-quick-reference.mddocs/upgrading/upgrading-react-on-rails.mdpackage-scripts.ymlreact_on_rails_pro/spec/dummy/package.jsondocs/upgrading/react-19-upgrade-guide.mdreact_on_rails_pro/spec/execjs-compatible-dummy/package.jsonpackages/react-on-rails-pro/src/RSCRoute.tsxeslint-rules/README.md
📚 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:
lib/generators/react_on_rails/templates/base/base/config/shakapacker.ymllib/generators/react_on_rails/base_generator.rbdocs/upgrading/react-19-quick-reference.mddocs/upgrading/upgrading-react-on-rails.mdreact_on_rails_pro/spec/dummy/package.jsonPR_SUMMARY_REACT_19_FIX.mddocs/upgrading/react-19-upgrade-guide.mdpackages/react-on-rails-pro/src/RSCProvider.tsxreact_on_rails_pro/spec/execjs-compatible-dummy/package.jsonpackages/react-on-rails-pro/src/RSCRoute.tsxeslint-rules/README.md
📚 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:
lib/generators/react_on_rails/templates/base/base/config/shakapacker.ymllib/generators/react_on_rails/base_generator.rbdocs/upgrading/react-19-quick-reference.mddocs/upgrading/upgrading-react-on-rails.mdreact_on_rails_pro/spec/dummy/package.jsonPR_SUMMARY_REACT_19_FIX.mddocs/upgrading/react-19-upgrade-guide.mdpackages/react-on-rails-pro/src/RSCProvider.tsxreact_on_rails_pro/spec/execjs-compatible-dummy/package.jsonpackages/react-on-rails-pro/src/RSCRoute.tsx
📚 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:
lib/generators/react_on_rails/templates/base/base/config/shakapacker.yml.github/workflows/pro-integration-tests.ymlpackages/react-on-rails-pro/src/RSCRoute.tsx
📚 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:
lib/generators/react_on_rails/base_generator.rbpackages/react-on-rails-pro/src/RSCProvider.tsxpackages/react-on-rails-pro/src/RSCRoute.tsx
📚 Learning: 2025-01-23T18:20:45.824Z
Learnt from: alexeyr-ci
Repo: shakacode/react_on_rails PR: 1687
File: spec/dummy/package.json:0-0
Timestamp: 2025-01-23T18:20:45.824Z
Learning: When adding or updating dependencies in spec/dummy/package.json, maintain version consistency with other package.json files in the codebase to avoid potential version conflicts.
Applied to files:
spec/dummy/package.json.github/workflows/pro-integration-tests.ymlreact_on_rails_pro/spec/dummy/package.jsonreact_on_rails_pro/spec/execjs-compatible-dummy/package.json
📚 Learning: 2025-02-18T13:08:01.477Z
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.
Applied to files:
docs/upgrading/react-19-quick-reference.mdPR_SUMMARY_REACT_19_FIX.mddocs/upgrading/react-19-upgrade-guide.mdpackages/react-on-rails-pro/src/RSCProvider.tsxpackages/react-on-rails-pro/src/RSCRoute.tsx
📚 Learning: 2025-02-13T19:09:15.991Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/RSCWebpackLoader.ts:0-0
Timestamp: 2025-02-13T19:09:15.991Z
Learning: In React Server Components webpack loader, using `new Function('return import("react-server-dom-webpack/node-loader")')()` is necessary as a workaround to bypass TypeScript compilation issues with direct dynamic imports.
Applied to files:
PR_SUMMARY_REACT_19_FIX.mdpackages/react-on-rails-pro/src/RSCProvider.tsxpackages/react-on-rails-pro/src/RSCRoute.tsxeslint-rules/README.md
📚 Learning: 2025-06-09T07:58:02.646Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1696
File: node_package/src/RSCPayloadGenerator.ts:124-168
Timestamp: 2025-06-09T07:58:02.646Z
Learning: In React Server Components (RSC) implementations, explicit error handling in RSC payload generation streams (like in RSCPayloadGenerator.ts) is not needed because errors will propagate to client components that fetch the RSC payloads (such as RSCRoute components). React's built-in error handling mechanisms and ErrorBoundary components are sufficient for handling these errors at the component level, which allows for better user experience with appropriate error UI fallbacks.
Applied to files:
packages/react-on-rails-pro/src/RSCProvider.tsxpackages/react-on-rails-pro/src/RSCRoute.tsx
📚 Learning: 2025-02-18T13:08:01.477Z
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 is handled in deeper level calls of the React on Rails Pro codebase, so it doesn't need to be validated again in the `rsc_payload_react_component` helper method.
Applied to files:
packages/react-on-rails-pro/src/RSCProvider.tsxpackages/react-on-rails-pro/src/RSCRoute.tsx
📚 Learning: 2025-06-11T12:34:58.182Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1696
File: node_package/src/RSCProvider.tsx:0-0
Timestamp: 2025-06-11T12:34:58.182Z
Learning: `RSCProvider` intentionally keeps failed `getServerComponent` promises cached (even if rejected) to avoid repeated fetch attempts; callers must use `refetchComponent` to retry.
Applied to files:
packages/react-on-rails-pro/src/RSCProvider.tsxpackages/react-on-rails-pro/src/RSCRoute.tsx
📚 Learning: 2024-12-12T13:07:09.929Z
Learnt from: alexeyr-ci
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/ReactOnRailsRSC.ts:87-87
Timestamp: 2024-12-12T13:07:09.929Z
Learning: When handling errors in 'node_package/src/ReactOnRailsRSC.ts', include the error stack in error messages in development and test environments to aid debugging.
Applied to files:
packages/react-on-rails-pro/src/RSCProvider.tsxpackages/react-on-rails-pro/src/RSCRoute.tsx
📚 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:
packages/react-on-rails-pro/src/RSCProvider.tsx
📚 Learning: 2024-10-08T20:53:47.076Z
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.
Applied to files:
react_on_rails_pro/spec/execjs-compatible-dummy/package.json
📚 Learning: 2025-01-23T18:19:25.261Z
Learnt from: alexeyr-ci
Repo: shakacode/react_on_rails PR: 1687
File: node_package/src/buildConsoleReplay.ts:12-12
Timestamp: 2025-01-23T18:19:25.261Z
Learning: When reviewing imports, carefully distinguish between default exports and named exports, as they represent different usage patterns. The presence of an import from a file doesn't necessarily mean all exported members are being used.
Applied to files:
packages/react-on-rails-pro/src/RSCRoute.tsx
📚 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:
TURBO_NAVIGATION_TEST_ISSUE.md
🧬 Code graph analysis (1)
packages/react-on-rails-pro/src/RSCProvider.tsx (1)
packages/react-on-rails-pro/src/getReactServerComponent.client.ts (1)
ClientGetReactServerComponentProps(27-31)
🪛 LanguageTool
SHAKAPACKER_UPGRADE_PR_PLAN.md
[uncategorized] ~188-~188: Possible missing comma found.
Context: ...eInterop: falsewas generating invalid imports likeimport ReactClient from 'react/in...
(AI_HYDRA_LEO_MISSING_COMMA)
[style] ~309-~309: ‘Based on the fact that’ might be wordy. Consider a shorter alternative.
Context: ... testing --- ## Recommended Approach Based on the fact that you mentioned 9.2.0 (though git shows 8...
(EN_WORDINESS_PREMIUM_BASED_ON_THE_FACT_THAT)
[grammar] ~319-~319: The word ‘complete’ is a noun or an adjective. A verb is missing or misspelled, or maybe a comma is missing.
Context: ...erns - ✅ Reasonable risk per PR - ✅ Can complete in 2-3 weeks - ✅ Each PR provides value...
(MD_JJ)
[typographical] ~319-~319: If specifying a range, consider using an en dash instead of a hyphen.
Context: ...sonable risk per PR - ✅ Can complete in 2-3 weeks - ✅ Each PR provides value indepe...
(HYPHEN_TO_EN)
[typographical] ~325-~325: If specifying a range, consider using an en dash instead of a hyphen.
Context: ...pgrade) - thorough testing, merge after 4-5 days - Week 3: PR C (React 19 + polish)...
(HYPHEN_TO_EN)
[typographical] ~326-~326: If specifying a range, consider using an en dash instead of a hyphen.
Context: ... PR C (React 19 + polish) - merge after 3-4 days --- ## Next Steps 1. **Confirm ...
(HYPHEN_TO_EN)
[uncategorized] ~334-~334: Possible missing preposition found.
Context: ... appropriate commits cherry-picked 3. Test locally before pushing 4. **Create PR...
(AI_HYDRA_LEO_MISSING_TO)
docs/upgrading/react-19-upgrade-guide.md
[uncategorized] ~47-~47: “React” is a proper noun and needs to be capitalized.
Context: ...uration. ### 2. No Default Export from react/index.js React 19 removed the default ...
(A_GOOGLE)
[uncategorized] ~86-~86: Possible missing preposition found.
Context: ...ct.createFactory() - Use JSX instead - Legacy Context (contextTypes, getChildConte...
(AI_HYDRA_LEO_MISSING_OF)
[grammar] ~387-~387: The word “rollback” is a noun. The verb is spelled with a space.
Context: ...an If you encounter issues and need to rollback: ```bash # Downgrade React yarn add re...
(NOUN_VERB_CONFUSION)
TURBO_NAVIGATION_TEST_ISSUE.md
[style] ~30-~30: This sentence contains multiple usages of the word “again”. Consider removing or replacing it.
Context: ...d refreshing again → ✅ JavaScript works again This severely degraded the user experi...
(REPETITION_OF_AGAIN)
[typographical] ~138-~138: If specifying a range, consider using an en dash instead of a hyphen.
Context: ...ultiple navigations**: Navigate through 3-4 pages to ensure no memory leaks 2. **Ba...
(HYPHEN_TO_EN)
[duplication] ~139-~139: Possible typo: you repeated a word.
Context: ...ttons**: Test browser back/forward with Turbo 3. Turbo Frames: Test components inside turbo ...
(ENGLISH_WORD_REPEAT_RULE)
[typographical] ~186-~186: If specifying a range, consider using an en dash instead of a hyphen.
Context: ...bo functionality Estimated effort: 2-4 hours (including documentation and veri...
(HYPHEN_TO_EN)
eslint-rules/README.md
[uncategorized] ~17-~17: Loose punctuation mark.
Context: ...lient-only APIs like: - createContext, useContext - useState, useEffect,...
(UNLIKELY_OPENING_PUNCTUATION)
[typographical] ~17-~17: Two consecutive commas
Context: ...ent-only APIs like: - createContext, useContext - useState, useEffect, useLayoutEffect, `useRed...
(DOUBLE_PUNCTUATION)
[typographical] ~18-~18: Two consecutive commas
Context: ...State, useEffect, useLayoutEffect, useReducer-Component, PureComponent - Other hooks (use*` f...
(DOUBLE_PUNCTUATION)
⏰ 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). (12)
- GitHub Check: rspec-package-tests (3.2, minimum)
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: build-dummy-app-webpack-test-bundles (3.4, 22, latest)
- GitHub Check: build-dummy-app-webpack-test-bundles (3.2, 20, minimum)
- GitHub Check: lint-js-and-ruby
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: examples (3.4, latest)
- GitHub Check: examples (3.2, minimum)
- GitHub Check: build
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: markdown-link-check
- GitHub Check: claude-review
🔇 Additional comments (5)
SHAKAPACKER_UPGRADE_PR_PLAN.md (1)
1-338: Comprehensive upgrade strategy documented.This planning document provides a clear breakdown of the Shakapacker upgrade approach with well-structured PR sequencing, risk assessment, and testing checklists. The document effectively communicates the migration strategy and serves as a valuable reference for the upgrade process.
lib/generators/react_on_rails/base_generator.rb (2)
5-5: LGTM: YAML library imported for configuration parsing.The addition of
require "yaml"supports the new YAML-based configuration detection in theusing_swc?method.
189-200: LGTM: Conditional dependency installation logic.The conditional branching correctly installs SWC packages (
@swc/core,swc-loader) whenusing_swc?returns true, and Babel packages otherwise. This provides a clean separation between the two compiler ecosystems.packages/react-on-rails-pro/src/RSCProvider.tsx (2)
15-17: LGTM: 'use client' directive and React 19-compatible imports.The addition of the
'use client'directive correctly marks this component as a client component since it usescreateContextanduseContext. The switch from default import to named imports (createContext,useContext,type ReactNode) aligns with React 19's conditional exports and resolves the TypeScript import issues mentioned in the PR objectives.
22-24: LGTM: Consistent type updates and React API usage.All references have been correctly updated:
React.ReactNode→ReactNodein type annotationsReact.useContext→useContext(now directly imported)React.createContext→createContext(now directly imported)These changes ensure consistency with the new import pattern and maintain type safety throughout the component.
Also applies to: 27-27, 51-51, 53-53, 79-79, 104-104
| def using_swc? | ||
| # Check shakapacker.yml for javascript_compiler setting | ||
| # Parse YAML to properly handle comments and get actual configured value | ||
| shakapacker_config_path = File.join(destination_root, "config", "shakapacker.yml") | ||
| return false unless File.exist?(shakapacker_config_path) | ||
|
|
||
| begin | ||
| config = YAML.safe_load_file(shakapacker_config_path) | ||
| # Check default section for javascript_compiler setting | ||
| javascript_compiler = config.dig("default", "javascript_compiler") | ||
| # Return true only if explicitly set to "swc" | ||
| javascript_compiler == "swc" | ||
| rescue StandardError => e | ||
| # If YAML parsing fails, default to babel (false) | ||
| puts "Warning: Could not parse shakapacker.yml: #{e.message}" | ||
| false | ||
| end | ||
| end |
There was a problem hiding this comment.
Critical discrepancy: default behavior contradicts PR objectives and Shakapacker 9.3.0 expectations.
The current implementation returns false (Babel) when shakapacker.yml is missing (line 238) or when javascript_compiler is not explicitly set to "swc" (line 245). However, the PR objectives state: "using_swc? now defaults to true for Shakapacker 9.3.0 — returns true when config is absent and only false if javascript_compiler: babel is explicitly set."
Additionally, a past review comment at line 238 specifically questioned whether this should return true for Shakapacker 9.3.0's default behavior.
If Shakapacker 9.3.0 truly defaults to SWC, apply this diff to match the stated behavior:
def using_swc?
# Check shakapacker.yml for javascript_compiler setting
# Parse YAML to properly handle comments and get actual configured value
shakapacker_config_path = File.join(destination_root, "config", "shakapacker.yml")
- return false unless File.exist?(shakapacker_config_path)
+ # Default to SWC (true) for Shakapacker 9.3.0 when config file doesn't exist
+ return true unless File.exist?(shakapacker_config_path)
begin
config = YAML.safe_load_file(shakapacker_config_path)
# Check default section for javascript_compiler setting
javascript_compiler = config.dig("default", "javascript_compiler")
- # Return true only if explicitly set to "swc"
- javascript_compiler == "swc"
+ # Default to true (SWC) for Shakapacker 9.3.0, return false only if explicitly set to "babel"
+ javascript_compiler != "babel"
rescue StandardError => e
- # If YAML parsing fails, default to babel (false)
+ # If YAML parsing fails, default to SWC (true) for Shakapacker 9.3.0
puts "Warning: Could not parse shakapacker.yml: #{e.message}"
- false
+ true
end
endAlternatively, if Shakapacker 9.3.0 still defaults to Babel, please update the PR objectives to reflect the correct default behavior.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def using_swc? | |
| # Check shakapacker.yml for javascript_compiler setting | |
| # Parse YAML to properly handle comments and get actual configured value | |
| shakapacker_config_path = File.join(destination_root, "config", "shakapacker.yml") | |
| return false unless File.exist?(shakapacker_config_path) | |
| begin | |
| config = YAML.safe_load_file(shakapacker_config_path) | |
| # Check default section for javascript_compiler setting | |
| javascript_compiler = config.dig("default", "javascript_compiler") | |
| # Return true only if explicitly set to "swc" | |
| javascript_compiler == "swc" | |
| rescue StandardError => e | |
| # If YAML parsing fails, default to babel (false) | |
| puts "Warning: Could not parse shakapacker.yml: #{e.message}" | |
| false | |
| end | |
| end | |
| def using_swc? | |
| # Check shakapacker.yml for javascript_compiler setting | |
| # Parse YAML to properly handle comments and get actual configured value | |
| shakapacker_config_path = File.join(destination_root, "config", "shakapacker.yml") | |
| # Default to SWC (true) for Shakapacker 9.3.0 when config file doesn't exist | |
| return true unless File.exist?(shakapacker_config_path) | |
| begin | |
| config = YAML.safe_load_file(shakapacker_config_path) | |
| # Check default section for javascript_compiler setting | |
| javascript_compiler = config.dig("default", "javascript_compiler") | |
| # Default to true (SWC) for Shakapacker 9.3.0, return false only if explicitly set to "babel" | |
| javascript_compiler != "babel" | |
| rescue StandardError => e | |
| # If YAML parsing fails, default to SWC (true) for Shakapacker 9.3.0 | |
| puts "Warning: Could not parse shakapacker.yml: #{e.message}" | |
| true | |
| end | |
| end |
| # Hook to run before compilation (e.g., for ReScript builds, pack generation) | ||
| # See: https://github.com/shakacode/shakapacker/blob/main/docs/precompile_hook.md | ||
| precompile_hook: bin/shakapacker-precompile-hook | ||
|
|
There was a problem hiding this comment.
Remove the duplicate precompile_hook entry.
This new block redefines precompile_hook, but the template already sets the same key later (Line 49). Duplicate YAML keys are confusing and some parsers treat them as errors; at minimum the latter assignment silently overrides the former. Please drop this new stanza and keep a single, well-documented definition.
- # Hook to run before compilation (e.g., for ReScript builds, pack generation)
- # See: https://github.com/shakacode/shakapacker/blob/main/docs/precompile_hook.md
- precompile_hook: bin/shakapacker-precompile-hook
-📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Hook to run before compilation (e.g., for ReScript builds, pack generation) | |
| # See: https://github.com/shakacode/shakapacker/blob/main/docs/precompile_hook.md | |
| precompile_hook: bin/shakapacker-precompile-hook | |
| # Hook to run before compilation (e.g., for ReScript builds, pack generation) | |
| # See: https://github.com/shakacode/shakapacker/blob/main/docs/precompile_hook.md | |
| precompile_hook: bin/shakapacker-precompile-hook | |
| # Hook to run before compilation (e.g., for ReScript builds, pack generation) | |
| # See: https://github.com/shakacode/shakapacker/blob/main/docs/precompile_hook.md | |
| precompile_hook: bin/shakapacker-precompile-hook |
🤖 Prompt for AI Agents
In lib/generators/react_on_rails/templates/base/base/config/shakapacker.yml
around lines 7 to 10, there is a duplicate precompile_hook entry; remove this
earlier stanza entirely so only the single precompile_hook definition (the one
later at ~line 49) remains, and ensure any necessary documentation/comments are
kept with the remaining definition.
f86c684 to
843f1f7
Compare
justin808
changed the title
Code Review: Shakapacker 9.3.0 UpgradeOverall AssessmentThis is a well-structured and comprehensive upgrade PR that successfully migrates from Shakapacker 8.2.0 to 9.3.0. The changes demonstrate careful consideration of backward compatibility, robust error handling, and thoughtful documentation. Great work! Strengths1. Excellent Code Quality
2. Smart Compiler Detection
3. CI/CD Improvements
4. Comprehensive Documentation
Issues & RecommendationsCritical Issues1. Documentation Files Should Not Be Committed to MasterFiles: SHAKAPACKER_UPGRADE_PR_PLAN.md, TURBO_NAVIGATION_TEST_ISSUE.md Issue: These are planning/design documents that don't belong in the production codebase. Recommendation: Remove before merging or move to docs/contributor-info/ directory. High Priority Issues2. Inconsistent Package Manager Handling Between GeneratorsFile: lib/generators/react_on_rails/install_generator.rb:531 Issue: add_rspack_dependencies still hardcodes npm install instead of using detect_package_manager_and_exact_flag Recommendation: Use detect_package_manager_and_exact_flag consistently across all dependency installation methods. 3. Missing --exact Flag UsageFile: lib/generators/react_on_rails/install_generator.rb:496, 515, 558 Issue: The method retrieves the exact_flag but doesn't apply it for most dependency installations. Current Code:
Recommendation: Apply exact_flag to ALL dependency installations for reproducible builds:
Why this matters: Without --exact, package managers may install range versions, leading to version drift between installations. 4. YAML Security HardeningFile: lib/generators/react_on_rails/base_generator.rb:241 Issue: YAML.safe_load_file lacks explicit permitted_classes argument. Recommendation: Add explicit restrictions for defense-in-depth:
Medium Priority Issues5. Code Duplication: using_swc? MethodFiles: lib/generators/react_on_rails/base_generator.rb:234-251 Recommendation: Consider extracting to ReactOnRails::PackerUtils.using_swc?(config_path) for reuse across codebase. 6. Inconsistent Babel Package InstallationFiles:
Issue: install_generator always installs Babel packages regardless of SWC setting, while base_generator conditionally excludes them. Recommendation: Make install_generator.rb also conditional on using_swc? to avoid installing unused Babel packages. 7. Hardcoded Version StringFile: rakelib/shakapacker_examples.rake:37 Issue: gem 'shakapacker', '>= 9.3.0' is hardcoded Recommendation: Extract to constant for easier maintenance:
Low Priority / Nice-to-Have8. Verify RuboCop CompliancePer CLAUDE.md: Must run bundle exec rubocop before every commit and fix ALL violations. Recommendation: Verify this was done before merge. Security AssessmentNo security concerns identified
Performance ConsiderationsPerformance improvements observed:
Potential concern:
Test CoverageStrengths:
Gaps Identified:
Recommendation: Add specs for detect_package_manager_and_exact_flag and using_swc? methods in follow-up PR. Best Practices Compliance (per CLAUDE.md)
Recommendations SummaryBefore Merge:
Follow-up PRs:
ConclusionThis is strong work on a complex upgrade. The code is well-structured, handles edge cases, and includes helpful user-facing warnings. The main issues are:
With the recommended fixes, this PR will be in excellent shape for merge. Estimated fix time: 1-2 hours for critical issues + test additions Great job on the thorough upgrade! |
Summary
This PR upgrades Shakapacker from 9.2.0 to 9.3.0 and includes all necessary compatibility fixes and configuration updates.
Key Changes
Shakapacker Version Update
SWC Compiler Support
using_swc?helper to properly parse shakapacker.ymlCSS Modules Compatibility
Generator Improvements
Build & CI Fixes
Documentation & Testing
Testing
Breaking Changes
None - this upgrade maintains backward compatibility while adding new features.
Migration Notes
For projects upgrading to Shakapacker 9.3.0:
shakapackergem to9.3.0in Gemfilebundle installyarn upgrade [email protected]Related PRs
🤖 Generated with Claude Code
This change is