Fix generator validation by using environment variable

[justin808]

Summary

Fixes failing CI tests by restoring the environment variable-based approach for skipping version validation during generator runtime.

Problem

After commit a1c71ea, CI was failing with version validation errors during generator tests:

• examples (3.2, minimum) - failure
• examples (3.4, latest) - failure

The error occurred when running rails generate react_on_rails:install:

**ERROR** ReactOnRails: No React on Rails npm package is installed.

The ARGV-based detection (ARGV.first.in?(%w[generate g])) was unreliable because Rails can modify or clear ARGV during the initialization process.

Solution

Restored the environment variable approach (previously removed in commit c3c1ad4):

1. Generator sets ENV variable: ENV["REACT_ON_RAILS_SKIP_VALIDATION"] = "true" at the start of run_generators
2. Engine checks ENV first: Added ENV check as the first condition in skip_version_validation?
3. Generator clears ENV: Uses ensure block to clean up after execution

The ENV variable persists through Rails initialization, making it more reliable than ARGV.

Changes

• lib/generators/react_on_rails/install_generator.rb: Set/clear REACT_ON_RAILS_SKIP_VALIDATION
• lib/react_on_rails/engine.rb: Check ENV variable first in skip_version_validation?
• spec/react_on_rails/engine_spec.rb: Added test coverage for ENV variable check

Test Plan

• All engine specs pass locally
• RuboCop passes
• CI generator tests pass (will verify after PR)

Related

• Fixes issues introduced in PR Fix Node.js 22 V8 crash in GitHub Actions workflows #1922 (commit a1c71ea)
• Restores behavior from commit af48844 (before removal in c3c1ad4)

🤖 Generated with Claude Code

---

This change is 

Summary by CodeRabbit

• New Features
  • Installation process now supports skipping version validation checks during setup
• Tests
  • Added test coverage for validation skipping functionality and debug logging

[coderabbitai]

Walkthrough

This pull request adds an environment variable-based mechanism to skip version validation during generator execution. The install generator sets REACT_ON_RAILS_SKIP_VALIDATION before running generators, and the engine's validation check respects this flag, allowing validators to be bypassed during setup phases.

Changes

Cohort / File(s)	Summary
Generator skip flag lib/generators/react_on_rails/install_generator.rb	Sets ENV["REACT_ON_RAILS_SKIP_VALIDATION"] = "true" before generator execution and removes it in ensure block for cleanup
Validation check lib/react_on_rails/engine.rb	Adds early return in skip_version_validation? when environment variable is set to "true", with debug logging; check runs before existing package.json validation
Validation tests spec/react_on_rails/engine_spec.rb	Adds new test context with setup/teardown for environment variable; tests that skip_version_validation? returns true when set and verifies debug message logging

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

• Straightforward environment variable pattern applied consistently across three files
• Logic is simple and localized to validation checks
• Tests follow existing patterns
• Minimal risk of side effects due to proper cleanup in ensure block

Possibly related PRs

• Fix CI failure by skipping version validation during generator runtime #1918: Also modifies the skip_version_validation? method and its tests to extend the validation-skipping behavior during generator flows.

Suggested labels

bug

Suggested reviewers

• Judahmeek

Poem

🐰 When generators run and validations sleep,
An env var flag keeps secrets deep,
Set it before, then clean the slate,
Version checks can skip and wait! ✨

Pre-merge checks and finishing touches

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'Fix generator validation by using environment variable' accurately summarizes the main change: using an environment variable to fix generator validation issues that were causing CI failures.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch justin808/fix-generator-validation

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

spec/react_on_rails/engine_spec.rb (1)

17-23: Consider capturing the original environment variable value.

While the current implementation works, the established pattern in this codebase is to capture and restore the original value of environment variables in tests to ensure complete isolation.

Based on learnings, apply this diff:

       context "when REACT_ON_RAILS_SKIP_VALIDATION is set" do
+        let(:original_value) { ENV["REACT_ON_RAILS_SKIP_VALIDATION"] }
+
         before do
           ENV["REACT_ON_RAILS_SKIP_VALIDATION"] = "true"
         end
 
         after do
-          ENV.delete("REACT_ON_RAILS_SKIP_VALIDATION")
+          if original_value.nil?
+            ENV.delete("REACT_ON_RAILS_SKIP_VALIDATION")
+          else
+            ENV["REACT_ON_RAILS_SKIP_VALIDATION"] = original_value
+          end
         end

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a1c71ea and 889a38b.

📒 Files selected for processing (3)

• lib/generators/react_on_rails/install_generator.rb (2 hunks)
• lib/react_on_rails/engine.rb (1 hunks)
• spec/react_on_rails/engine_spec.rb (1 hunks)

🧰 Additional context used

🧠 Learnings (11)

📓 Common learnings

Learnt from: justin808
Repo: shakacode/react_on_rails PR: 1622
File: spec/dummy/spec/rake/assets_precompile_rake_spec.rb:12-12
Timestamp: 2024-10-08T20:53:47.076Z
Learning: When stubbing environment variables in RSpec tests, use `before` and `after` hooks to ensure that the original values are restored after the tests, preventing any side effects on other tests. Example provided by justin808:
```ruby
describe "My test" do
  before do
    original_value = ENV["VARIABLE_NAME"]
    allow(ENV).to receive(:[]).with("VARIABLE_NAME").and_return("stubbed_value")
  end

  after do
    allow(ENV).to receive(:[]).with("VARIABLE_NAME").and_call_original
    ENV["VARIABLE_NAME"] = original_value
  end

  it "tests something" do
    # Your test code here
  end
end
```
This practice ensures test isolation and reliability.

Learnt from: justin808
Repo: shakacode/react_on_rails PR: 1622
File: spec/dummy/spec/rake/assets_precompile_rake_spec.rb:12-12
Timestamp: 2024-06-27T05:22:46.156Z
Learning: When stubbing environment variables in RSpec tests, use `before` and `after` hooks to ensure that the original values are restored after the tests, preventing any side effects on other tests. Example provided by justin808:
```ruby
describe "My test" do
  before do
    original_value = ENV["VARIABLE_NAME"]
    allow(ENV).to receive(:[]).with("VARIABLE_NAME").and_return("stubbed_value")
  end

  after do
    allow(ENV).to receive(:[]).with("VARIABLE_NAME").and_call_original
    ENV["VARIABLE_NAME"] = original_value
  end

  it "tests something" do
    # Your test code here
  end
end
```
This practice ensures test isolation and reliability.

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/install_generator.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/generators/react_on_rails/install_generator.rb
• lib/react_on_rails/engine.rb
• spec/react_on_rails/engine_spec.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/install_generator.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/generators/react_on_rails/install_generator.rb
• lib/react_on_rails/engine.rb
• spec/react_on_rails/engine_spec.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.rb
• spec/react_on_rails/engine_spec.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
• spec/react_on_rails/engine_spec.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
• spec/react_on_rails/engine_spec.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
• spec/react_on_rails/engine_spec.rb

📚 Learning: 2024-10-08T20:53:47.076Z

Learnt from: justin808
Repo: shakacode/react_on_rails PR: 1622
File: spec/dummy/spec/rake/assets_precompile_rake_spec.rb:12-12
Timestamp: 2024-10-08T20:53:47.076Z
Learning: When stubbing environment variables in RSpec tests, use `before` and `after` hooks to ensure that the original values are restored after the tests, preventing any side effects on other tests. Example provided by justin808:
```ruby
describe "My test" do
  before do
    original_value = ENV["VARIABLE_NAME"]
    allow(ENV).to receive(:[]).with("VARIABLE_NAME").and_return("stubbed_value")
  end

  after do
    allow(ENV).to receive(:[]).with("VARIABLE_NAME").and_call_original
    ENV["VARIABLE_NAME"] = original_value
  end

  it "tests something" do
    # Your test code here
  end
end
```
This practice ensures test isolation and reliability.

Applied to files:

• spec/react_on_rails/engine_spec.rb

📚 Learning: 2025-02-13T14:29:49.267Z

Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: spec/react_on_rails/utils_spec.rb:218-218
Timestamp: 2025-02-13T14:29:49.267Z
Learning: In RSpec tests, prefer using local variables over constants within test blocks to avoid constant redefinition warnings and maintain better test isolation.

Applied to files:

• spec/react_on_rails/engine_spec.rb

🧬 Code graph analysis (1)

spec/react_on_rails/engine_spec.rb (1)

lib/react_on_rails/engine.rb (1)

• skip_version_validation? (22-42)

🔇 Additional comments (3)

lib/generators/react_on_rails/install_generator.rb (1)

39-63: LGTM! Clean implementation with proper cleanup.

The environment variable approach correctly addresses the ARGV-based detection issues. Setting the variable at the start of run_generators and clearing it in the ensure block guarantees cleanup even if exceptions occur during generator execution.

spec/react_on_rails/engine_spec.rb (1)

25-33: LGTM! Tests thoroughly verify the new behavior.

The tests correctly validate both the return value and the debug logging when the environment variable is set.

lib/react_on_rails/engine.rb (1)

23-27: LGTM! Correct implementation of the environment variable check.

The early-exit logic correctly prioritizes the explicit environment variable override before other validation skip conditions. The debug logging provides helpful feedback for troubleshooting.

[claude]

Code Review - PR #1923: Fix generator validation by using environment variable

Overview

This PR addresses failing CI tests by restoring an environment variable-based approach to skip version validation during generator runtime. The solution is well-motivated and properly implemented.

✅ Strengths

1. Clear Problem Identification: The PR correctly identifies that ARGV-based detection is unreliable because Rails can modify ARGV during initialization.

2. Proper Resource Management: Uses ensure block to guarantee ENV cleanup, preventing pollution of the environment state.

3. Good Test Coverage: Added comprehensive test coverage for the new ENV variable check, including verification of debug logging.

4. Defensive Ordering: Places the ENV check first in skip_version_validation?, ensuring explicit control takes precedence over heuristics.

5. Documentation: Inline comments explain why the ENV variable approach is used and how it persists through Rails initialization.

🔍 Code Quality Observations

Excellent Practices:

• Clean separation of concerns between generator and engine
• Proper use of ensure block for cleanup
• Debug logging for observability
• Test coverage matches implementation

Minor Suggestions:

1. Thread Safety Consideration (Low Priority):

   # Current code:
   ENV["REACT_ON_RAILS_SKIP_VALIDATION"] = "true"

   While ENV is generally thread-safe in Ruby, if generators could theoretically run concurrently (e.g., in tests), consider whether this global state could cause issues. However, Rails generators typically run sequentially, so this is likely not a concern in practice.

2. ENV Value Check:

   # Current: lib/react_on_rails/engine.rb:24
   if ENV["REACT_ON_RAILS_SKIP_VALIDATION"] == "true"

   This is good - explicitly checking for "true" string rather than just presence prevents accidental triggering from ENV["REACT_ON_RAILS_SKIP_VALIDATION"] = "false".

3. Test Isolation (Already Handled):
   The test properly cleans up the ENV variable in an after block - excellent!

🎯 Specific Code Review

lib/generators/react_on_rails/install_generator.rb:40-62

def run_generators
  ENV["REACT_ON_RAILS_SKIP_VALIDATION"] = "true"
  # ... generator logic ...
ensure
  ENV.delete("REACT_ON_RAILS_SKIP_VALIDATION")
  print_generator_messages
end

✅ Perfect implementation - Sets ENV at the start, guarantees cleanup with ensure, clear comments.

lib/react_on_rails/engine.rb:23-27

if ENV["REACT_ON_RAILS_SKIP_VALIDATION"] == "true"
  Rails.logger.debug "[React on Rails] Skipping validation - disabled via environment variable"
  return true
end

✅ Good practices - Early return, debug logging, explicit string comparison.

spec/react_on_rails/engine_spec.rb:16-34

context "when REACT_ON_RAILS_SKIP_VALIDATION is set" do
  before { ENV["REACT_ON_RAILS_SKIP_VALIDATION"] = "true" }
  after { ENV.delete("REACT_ON_RAILS_SKIP_VALIDATION") }
  # ... tests ...
end

✅ Proper test hygiene - Setup and teardown ensure no test pollution.

🚀 Performance Considerations

• ENV access is O(1) and very fast
• Placing the ENV check first in the validation chain is optimal
• No performance concerns

🔒 Security Considerations

• Environment variables are appropriate for this use case
• The variable name is descriptive and unlikely to collide
• No security concerns - this is internal configuration

🐛 Potential Issues

None identified. The implementation is solid and addresses the root cause effectively.

📝 Suggestions for Future Enhancement (Not Blocking)

1. Consider documenting the ENV variable in the main README or developer docs for future reference, since it's now part of the public API (even if internal).

2. Alternative approach (optional): If you wanted to avoid global state entirely, you could consider passing a configuration object through the generator invocation chain. However, the ENV approach is simpler and more pragmatic for this use case.

✅ Final Assessment

LGTM - Approved

This is a well-implemented fix that:

• Solves the immediate CI failure
• Uses appropriate Ruby idioms
• Has proper test coverage
• Includes good documentation
• Follows the project's coding standards

The ENV variable approach is more reliable than ARGV detection and is properly scoped with cleanup guarantees.

📋 Pre-merge Checklist (as per CLAUDE.md)

• ✅ Code follows Ruby best practices
• ✅ Proper resource cleanup with ensure block
• ✅ Test coverage added and comprehensive
• ✅ Debug logging for observability
• ⏳ CI tests should pass (main goal of this PR)
• ⏳ RuboCop should pass (recommend running locally before merge)
• ⏳ Files should end with newline (verify before merge)

Recommendation: Merge after CI confirms the fix works as expected.

---

🤖 Review generated with Claude Code

[claude]

Code Review: Fix generator validation by using environment variable

Summary

This PR successfully fixes failing CI tests by restoring a reliable environment variable-based approach for skipping version validation during generator runtime. The solution is well-implemented and addresses the root cause effectively.

---

✅ Strengths

1. Root Cause Analysis

• Excellent diagnosis: The ARGV-based detection is indeed unreliable because Rails can modify ARGV during initialization
• Clear understanding that ENV variables persist through Rails initialization, making them more reliable

2. Code Quality

• Clean implementation: The ENV variable is set at the start and cleaned up in an ensure block (lib/generators/react_on_rails/install_generator.rb:42, 62)
• Proper ordering: ENV check is placed first in skip_version_validation? for optimal short-circuiting (lib/react_on_rails/engine.rb:23-27)
• Good documentation: Inline comments explain why ENV is used and its lifetime
• Defensive cleanup: Using ensure block guarantees cleanup even if errors occur

3. Test Coverage

• Comprehensive test coverage added for the new ENV variable check (spec/react_on_rails/engine_spec.rb:16-34)
• Tests verify both the return value AND the debug logging
• Proper test hygiene with after blocks to clean up ENV

4. Workflow Improvements

• Adding workflow_dispatch to both workflow files enables manual triggering, which is helpful for debugging CI issues

---

🔍 Potential Issues & Suggestions

1. Thread Safety Concern (Medium Priority)

Location: lib/generators/react_on_rails/install_generator.rb:42, 62

Issue: Environment variables are process-global. If multiple generators run concurrently (e.g., in parallel tests), they could interfere with each other.

Potential scenario:

• Generator A sets ENV var → Generator B sets ENV var → Generator A finishes and deletes it → Generator B fails validation

Suggestion: Consider using a more unique identifier or document that concurrent generator execution is not supported. However, this is likely a non-issue in practice since Rails generators typically run in a single process.

2. Test Coverage Gap (Low Priority)

Location: spec/react_on_rails/engine_spec.rb

Missing test: There is no test verifying that the ENV variable takes precedence over other conditions (package.json missing, ARGV detection). While the current tests are good, an integration test would be valuable.

3. Documentation (Low Priority)

The PR description is excellent, but consider adding a comment in the code about why this approach was chosen over ARGV.

---

🛡️ Security Review

• ✅ No security concerns
• ENV variable is properly cleaned up
• No user input is used in ENV variable name or value
• Variable name is specific enough to avoid collisions

---

⚡ Performance Considerations

• ✅ Excellent: ENV check is placed FIRST in the validation chain, providing optimal short-circuit evaluation
• ENV variable access is O(1) and very fast
• No performance concerns

---

🧪 Testing

What is tested:

• ✅ ENV variable check returns true
• ✅ Debug logging occurs
• ✅ Other skip conditions still work

What could be tested (optional):

• ENV variable cleanup in error scenarios
• Precedence of ENV over other conditions

---

📝 Style & Conventions

According to CLAUDE.md:

• ✅ Files should end with newline - all modified files comply
• ⚠️ ACTION REQUIRED: Must run bundle exec rubocop before pushing (CLAUDE.md requirement)
• ✅ Code follows existing patterns in the codebase

---

🎯 Recommendations

Before Merge:

1. ✅ Run bundle exec rubocop and fix any violations (CRITICAL per CLAUDE.md)
2. ✅ Verify CI tests pass
3. Optional: Add precedence test mentioned above

Post-Merge:

1. Consider adding a brief note to CHANGELOG.md about the fix (though this might be considered internal/CI-only and not user-visible)

---

📊 Overall Assessment

Rating: ⭐⭐⭐⭐½ (4.5/5)

Verdict: APPROVE with minor suggestions

This is a well-crafted fix that:

• Solves the immediate problem effectively
• Includes proper test coverage
• Uses defensive programming (ensure block)
• Has clear documentation in the PR description

The concerns raised are minor and mostly about edge cases that are unlikely to occur. The core solution is sound and production-ready.

Great work! 🎉

---

Note: Remember to run bundle exec rubocop before final push as required by CLAUDE.md.