Apply PR 2039 changes: modernize generator config and workflow #2040
Conversation
|
Warning Rate limit exceeded@justin808 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 2 minutes and 0 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughThree files updated to enhance React on Rails build configuration: a GitHub Actions workflow now runs yalc publish via yarn, a generator template enables test build command and adds component directory configuration, and a Rake task wraps generator commands with validation skip flags while integrating pack generation. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Code Review for PR #2040I've reviewed this PR which applies key modernization changes from PR #2039. Here's my feedback: ✅ Positive Aspects
🔍 Potential Issues & Concerns1. Breaking Change Without Migration Path (react_on_rails.rb.tt:31-32)Issue: Enabling According to
Risk: Users generating new apps with these defaults will:
Recommendation: Consider:
2. generate_packs Added Without Context (shakapacker_examples.rake:45)Issue: Adding
Recommendation: Ensure the generator creates the necessary directory structure BEFORE this rake task runs, or handle the case where the directory doesn't exist yet. 3. Potential Test Coverage GapQuestion: Are there integration tests that verify:
Per the diff, these are structural changes to generated apps - integration testing would be valuable. 📝 Minor Observations
🔒 Security & Performance
🎯 Testing RecommendationsBefore merging, I recommend verifying:
📊 Overall AssessmentQuality: Good - Changes are logical and align with modernization goals The PR effectively applies the key changes from #2039, but would benefit from:
Verdict: ✅ Approve with suggestions - The changes are sound but would benefit from the improvements noted above to prevent user confusion and runtime errors. |
Code Review - PR #2040SummaryThis PR applies configuration and workflow changes from PR #2039 to modernize generator defaults and improve CI consistency. Overall, the changes are sound and well-intentioned, but there are some concerns that should be addressed. Findings1. Workflow Change:
|
- Change yalc publish to yarn yalc publish in examples.yml - Enable build_test_command and add auto_load_bundle config - Add REACT_ON_RAILS_SKIP_VALIDATION wrapper for generator commands - Add generate_packs step after npm install in examples 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
justin808
force-pushed
the
justin808/apply-yalc-publish
branch
from
b713ce5 to
33e033c
Compare
|
/run-skipped-ci |
|
🚀 Full CI Mode Enabled ✅ All checks are already running - nothing to do! All CI checks are already running on this PR. Added the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
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/templates/base/base/config/initializers/react_on_rails.rb.tt (1)
18-29: Make generated apps use one test‑asset workflow (prefer Shakapacker auto‑compile).
- Problem: the generator currently enables both approaches — initializer template sets config.build_test_command (lib/generators/react_on_rails/templates/base/base/config/initializers/react_on_rails.rb.tt) while the generated shakapacker.yml sets test: compile: true (lib/generators/react_on_rails/templates/base/base/config/shakapacker.yml).
- Action (preferred): Comment out/remove config.build_test_command in the initializer template and update the surrounding comment to state that the generator defaults to Shakapacker auto‑compilation.
- Alternative (if TestHelper should be the default): set compile: false in the shakapacker.yml template and ensure the generator includes the TestHelper setup (spec helper) and clear documentation.
- Note: lib/react_on_rails/doctor.rb already warns when both are present and when TestHelper/build_test_command are misaligned — keep that behavior.
🧹 Nitpick comments (1)
rakelib/shakapacker_examples.rake (1)
39-43: Track the TODO for removing validation skip.The
REACT_ON_RAILS_SKIP_VALIDATIONwrapper is appropriately documented as temporary. Ensure this TODO is tracked so the validation skip is removed once the generators are updated to use the next release.Do you want me to create a tracking issue for this TODO?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/examples.yml(1 hunks)lib/generators/react_on_rails/templates/base/base/config/initializers/react_on_rails.rb.tt(1 hunks)rakelib/shakapacker_examples.rake(1 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: Romex91
Repo: shakacode/react_on_rails PR: 1697
File: package-scripts.yml:28-28
Timestamp: 2025-02-12T16:38:06.537Z
Learning: The file `node_package/lib/ReactOnRails.full.js` is autogenerated during the build process and should not be present in the repository.
Learnt from: alexeyr-ci2
Repo: shakacode/react_on_rails PR: 1732
File: spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx:40-44
Timestamp: 2025-04-26T21:55:55.874Z
Learning: In the react_on_rails project, files under `app-react16` directories are copied/moved to corresponding `/app` directories during the conversion process (removing the `-react16` suffix), which affects their relative import paths at runtime.
Learnt from: justin808
Repo: shakacode/react_on_rails PR: 1770
File: lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.jsx:2-2
Timestamp: 2025-09-16T08:01:11.146Z
Learning: React on Rails uses webpack CSS Modules configuration with namedExports: true, which requires the import syntax `import * as style from './file.module.css'` rather than the default export pattern. This configuration enables better tree shaking and bundle size optimization for CSS modules.
Learnt from: 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.
📚 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/initializers/react_on_rails.rb.ttrakelib/shakapacker_examples.rake
📚 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/initializers/react_on_rails.rb.ttrakelib/shakapacker_examples.rake
📚 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/initializers/react_on_rails.rb.ttrakelib/shakapacker_examples.rake
📚 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/initializers/react_on_rails.rb.ttrakelib/shakapacker_examples.rake
📚 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/generators/react_on_rails/templates/base/base/config/initializers/react_on_rails.rb.tt
📚 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/templates/base/base/config/initializers/react_on_rails.rb.ttrakelib/shakapacker_examples.rake
⏰ 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.4, latest)
🔇 Additional comments (3)
.github/workflows/examples.yml (1)
133-133: LGTM! Consistent with yarn-based tooling.The change from
yalc publishtoyarn yalc publishaligns with the yarn-based tooling approach used throughout the workflow.lib/generators/react_on_rails/templates/base/base/config/initializers/react_on_rails.rb.tt (1)
31-32: Configuration options verified as properly supported.The script output confirms both
auto_load_bundleandcomponents_subdirectoryare well-established, fully supported configuration options. The value"ror_components"is the documented default directory name, andauto_load_bundle = truecorrectly enables automatic bundle loading. These options appear throughout the codebase in tests, examples, and extensive documentation. No issues were found.rakelib/shakapacker_examples.rake (1)
45-45: I need to search more specifically in the codebase for the task definition:Verify the generate_packs task integration.
The
react_on_rails:generate_packsrake task is a documented part of React on Rails' file-system-based automated bundle generation feature, which generates import statements for bundles and is controlled by theauto_load_bundleoption. The task works in conjunction with thecomponents_subdirectoryconfiguration to automatically register React components.The sequence in the code is correct: the generator sets up the configuration with
auto_load_bundle: trueand specifiescomponents_subdirectory, then npm installs dependencies, and finallygenerate_packsgenerates the required bundle files based on those settings.
Code ReviewI've reviewed PR #2040 and found it to be well-structured with targeted improvements. Here's my detailed feedback: ✅ Strengths
🔍 Detailed AnalysisFile:
|
- Add explanatory comments for yarn yalc publish workflow command - Document REACT_ON_RAILS_SKIP_VALIDATION usage in example generation - Add CHANGELOG entry for new generator configuration defaults - Clarify that changes only affect newly generated applications 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Pull Request ReviewI've reviewed PR #2040 which applies generator configuration and workflow modernization changes from PR #2039. Here's my detailed feedback: ✅ Positive Aspects1. Well-Documented Changes
2. Good Default ConfigurationEnabling these options by default for new apps makes sense:
3. Backward Compatibility
🔍 Code Quality Observations1. Workflow Update (.github/workflows/examples.yml:133)- run: yalc publish
+ run: yarn yalc publishGood: This change aligns with the project's mandate to only use Minor suggestion: Consider adding a validation step in CI to catch any direct 2. Generator Template Changes (react_on_rails.rb.tt:29-32)- # config.build_test_command = "RAILS_ENV=test bin/shakapacker"
+ config.build_test_command = "RAILS_ENV=test bin/shakapacker"
+ config.auto_load_bundle = true
+ config.components_subdirectory = "ror_components"Concern: The template doesn't include comments explaining these new options. New users might not understand what they do. Recommendation: Add brief inline comments: # Enable file-system based component loading (see: docs/core-concepts/auto-bundling...)
config.auto_load_bundle = true
# Components in this subdirectory will be auto-registered (default: "ror_components")
config.components_subdirectory = "ror_components"3. Rake Task Updates (shakapacker_examples.rake:39-52)Issue: The change from Current code ( def generator_shell_commands
shell_commands = []
shell_commands << "rails generate react_on_rails:install ..."
shell_commands << "rails generate react_on_rails:dev_tests ..."
endThe rake task now does: generator_commands = example_type.generator_shell_commands.map do |cmd|
"REACT_ON_RAILS_SKIP_VALIDATION=true #{cmd}"
endGood: This correctly wraps each command with the environment variable. Potential issue: The method name is still plural ( 4. Missing Test CoverageConcern: I don't see test updates for:
Recommendation: Add tests to verify:
🐛 Potential Issues1. Error Handling in generate_packsLine 52 adds: sh_in_dir(example_type.dir, "bundle exec rake react_on_rails:generate_packs")Question: What happens if
Looking at Suggestion: Consider adding a comment explaining the failure mode, or catch and provide context: # Generate packs for auto-bundled components. Will fail if component structure is invalid.
sh_in_dir(example_type.dir, "bundle exec rake react_on_rails:generate_packs")2. Validation Skip ScopeThe comment in
Question: Which config options are validated? Looking at Finding: The install generator ( Discrepancy: The comment suggests it's for config validation, but the actual code uses it for version/package validation. The comment should be updated for accuracy: # Skip version validation when running generators on example apps during development.
# The generator validates that the npm package version matches the gem version,
# but during example generation, we're testing against the current gem codebase
# which may not have a published npm package yet.🔒 Security ConsiderationsNo security concerns identified. The ENV variable usage is appropriate for this context. ⚡ Performance ConsiderationsMinor: Adding
📋 Testing Recommendations
📝 DocumentationThe CHANGELOG entry is excellent. Consider also:
SummaryOverall Assessment: ✅ Approve with minor suggestions This PR makes sensible improvements to the generator defaults. The code changes are straightforward and well-intentioned. My main recommendations:
These are minor improvements that don't block merging. The core changes align well with the project's modernization goals and maintain backward compatibility. Great work on the modernization effort! 🎉 |
Summary
Applies key changes from PR 2039 to modernize generator configuration and workflow updates:
yalc publishtoyarn yalc publishfor consistencybuild_test_command,auto_load_bundle, andcomponents_subdirectoryconfig optionsREACT_ON_RAILS_SKIP_VALIDATIONwrapper for generator commands andgenerate_packsstepThese changes align with the modernization efforts to improve generator reliability and configuration defaults.
🤖 Generated with Claude Code
This change is
Summary by CodeRabbit
New Features
Chores