Trigger generator tests when TypeScript files change

[justin808]

Summary

• When TypeScript files in packages/react-on-rails/ change, CI now also triggers generator tests
• Generator tests perform a full build which catches TypeScript compilation errors

Problem (Fixes #2205)

PR #2125 changed TypeScript types in packages/react-on-rails/src/types/index.ts, which exposed a latent type error in serverRenderReactComponent.ts. This wasn't caught until the release script ran pnpm publish which performs a fresh TypeScript build.

The generator/example tests would have caught this because they run a full build, but they weren't triggered because the CI change detection didn't consider packages/react-on-rails/** changes as requiring generator tests.

Solution

Updated script/ci-changes-detector to trigger generator tests when JS_CHANGED=true (which includes TypeScript file changes in packages/react-on-rails/src/).

Test plan

• Verify the script correctly detects TypeScript changes and sets run_generators=true
• CI passes

🤖 Generated with Claude Code

[coderabbitai]

Warning

Rate limit exceeded

@justin808 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 12 minutes and 27 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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.

📥 Commits

Reviewing files that changed from the base of the PR and between d67c1aa and 9836ea4.

📒 Files selected for processing (1)

• script/ci-changes-detector (1 hunks)

✨ Finishing touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch jg/fix-2205

---

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.}

[claude]

Code Review - PR #2208

Summary

This PR fixes a critical gap in CI coverage by ensuring generator tests run when TypeScript files change in packages/react-on-rails/. This is a focused, well-justified fix for issue #2205.

---

✅ What's Good

1. Excellent Problem/Solution Match: The fix directly addresses the root cause identified in Run generator tests when packages/react-on-rails TypeScript files change #2205 - TypeScript compilation errors that only surface during full builds (which generator tests perform).

2. Minimal, Surgical Change: Only 2 lines changed - the condition to trigger RUN_GENERATORS now includes JS_CHANGED. This is the right level of granularity.

3. Clear Documentation: The inline comment explaining why JS changes trigger generator tests references the issue number, making future maintenance easier.

4. Consistent with Existing Patterns: The change follows the same conditional logic pattern used throughout the script (e.g., line 266 for RUN_DUMMY_TESTS).

5. Proper Scope: JS_CHANGED already includes TypeScript files from packages/react-on-rails/src/** (line 97), so no additional patterns needed.

---

🔍 Observations & Questions

1. Verification Strategy

The test plan mentions verifying the script correctly detects changes, but I don't see automated tests for ci-changes-detector.

Suggestion: Consider adding a test suite for this critical script, especially since it controls CI behavior. Example structure:

# test/ci-changes-detector.bats or similar
test_typescript_changes_trigger_generators() {
  # Mock git diff output
  # Run script
  # Assert run_generators=true
}

This would prevent future regressions and make verification repeatable.

2. Performance Impact

Generator tests are among the heaviest CI jobs (full builds, multiple examples). With this change, every TypeScript change in packages/react-on-rails/ will trigger them.

Questions:

• Is this the intended tradeoff? (Safety vs CI time)
• Could we narrow the trigger to specific TS files (e.g., type definitions, core files)?
• What's the average runtime impact? (Baseline: TS-only PR without this change vs. with)

Current logic: JS changes already trigger dummy tests (line 266), so you're adding generators on top. This seems appropriate given the issue - generator tests caught the type error that other checks missed.

3. Pro Package Consistency

I notice there's a PRO_JS_CHANGED flag (line 65, 117) but the generator trigger logic only checks GENERATORS_CHANGED || JS_CHANGED.

Question: Should Pro JS changes also trigger Pro generator tests? Or are Pro generators tested separately via PRO_DUMMY_CHANGED?

If Pro has separate generator tests, consider documenting that in a comment to explain the asymmetry.

---

🐛 Potential Issues

Minor: Comment Precision

Line 270-271 says "generators do a full build which catches TypeScript compilation errors."

Clarification: The generators themselves don't build - they generate example apps that then get built as part of the test. Consider rewording:

# JS changes trigger generator tests because generator tests perform full builds
# of generated examples, which catches TypeScript compilation errors (see issue #2205)

This makes the mechanism clearer (generates → builds examples → catches TS errors).

---

🔒 Security & Performance

• Security: No concerns - this only affects CI job scheduling.
• Performance: Adds generator tests to all PRs touching TS files. Given the severity of the issue this fixes (broken releases), the tradeoff seems justified.

---

📋 Test Coverage

Current state: Manual verification only ("Verify the script correctly detects TypeScript changes").

Recommendations:

1. Add automated tests for the script's logic (see Observation TODO for first version #1)
2. Document the expected CI job matrix for common change scenarios
3. Consider adding a CI workflow that validates the detector's output on PRs

---

✨ Additional Suggestions

1. Document the decision: Consider adding to .claude/docs/ci-architecture.md (or similar) explaining the CI trigger matrix and why certain changes trigger heavy jobs.

2. Monitor impact: After merging, track average PR CI time for TS-only changes to validate the performance tradeoff is acceptable.

3. Future optimization: If generator tests become a bottleneck, consider:

   • Splitting into "quick" generators (basic setup) vs "comprehensive" (all examples)
   • Caching build artifacts more aggressively
   • Running generators only for type-definition changes (packages/react-on-rails/src/types/**)

---

🎯 Verdict

LGTM with minor suggestions ✅

This is a solid fix that addresses a real gap in CI coverage. The change is minimal, well-documented, and follows existing patterns. The suggestions above are mostly about future-proofing and documentation, not blockers.

Recommended next steps:

1. ✅ Merge as-is if CI passes
2. Consider adding automated tests for the detector script (follow-up PR)
3. Monitor CI performance impact over next few PRs

---

Great work tracking down the root cause and implementing a targeted fix! 🚀

[claude]

Code Review - PR #2208

Summary

This PR fixes issue #2205 by ensuring TypeScript changes trigger generator tests, which perform full builds and catch compilation errors. The fix is simple, targeted, and effective.

---

✅ Strengths

1. Root Cause Analysis: Excellent problem identification. The issue was real - TypeScript compilation errors were only caught during pnpm publish because generator tests weren't running for TS changes.

2. Minimal, Surgical Fix: The change is exactly 2 lines (plus a comment), modifying only what's necessary.

3. Clear Documentation: The inline comment explains why this change exists and references the issue number for future maintainers.

4. Leverages Existing Infrastructure: JS_CHANGED is already properly set (line 97) for all TypeScript files in packages/react-on-rails/src/**, so no changes to pattern matching needed.

5. Consistent with Project Patterns: Matches the existing logic style (e.g., lines 254, 266) where multiple conditions trigger the same test suite.

---

🔍 Considerations & Questions

1. Performance Impact (Low Priority)

Generator tests are now triggered for ANY JS/TS change in packages/react-on-rails/, not just TypeScript compilation-breaking changes. This means:

• More frequent generator test runs on PRs
• Longer CI times when JS/TS files change

Analysis: This is likely acceptable because:

• Generator tests validate the full build pipeline, which is valuable
• The alternative (running only on specific file patterns) would be more complex and error-prone
• Better to over-test than under-test for build safety

Recommendation: ✅ Accept as-is. The trade-off favors safety.

---

2. Potential False Positives

Changes to packages/react-on-rails/tests/** or packages/react-on-rails/scripts/** will also trigger generator tests since they set JS_CHANGED=true (line 97).

Example: A change to only packages/react-on-rails/tests/ComponentRegistry.test.ts would now run generator tests even though it can't break the build.

Recommendation: If you want to optimize, you could create a separate JS_SRC_CHANGED flag for just src/ files and keep JS_CHANGED for tests/scripts. However, this adds complexity. Current approach is fine unless CI costs become prohibitive.

---

3. TypeScript Config Changes

The pattern at line 97 includes packages/react-on-rails/tsconfig.json, which will correctly trigger generator tests. This is good - tsconfig.json changes can absolutely break builds.

Recommendation: ✅ Working as intended.

---

🐛 Potential Issues

Logic Clarity (Minor): The comment says "JS changes trigger generator tests" but could be even clearer about the two distinct reasons generator tests run. Very low severity - existing comment is adequate.

---

🔒 Security Concerns

None. This is a CI configuration change with no security implications.

---

📊 Test Coverage

Current Coverage: The change is implicitly tested by CI workflows (e.g., .github/workflows/examples.yml at line 98 checks run_generators).

Additional Testing: The ci-changes-detector script itself has no unit tests. Consider adding a test suite in a future PR to verify:

• TypeScript changes → run_generators=true
• Generator changes → run_generators=true
• Docs-only changes → run_generators=false

Recommendation: Not required for this PR, but would improve maintainability long-term.

---

✅ Final Verdict

APPROVED ✓

This is a well-reasoned, minimal fix that directly addresses the root cause from #2205. The code is clean, well-commented, and follows existing patterns.

Minor Suggestions (Optional):

1. Verify the script behavior with test scenarios
2. Consider the JS_SRC_CHANGED optimization if CI costs become an issue
3. Add unit tests for ci-changes-detector in a future PR

Great work on the root cause analysis and keeping the fix surgical! 🎯

[claude]

Code Review for PR #2208

Summary

This PR addresses issue #2205 by ensuring that TypeScript changes in packages/react-on-rails/ trigger generator tests, which perform a full build and would catch TypeScript compilation errors that might otherwise be missed until release.

---

✅ Positive Aspects

1. Root Cause Analysis: The PR correctly identifies that TypeScript type changes weren't being caught early because generator tests weren't triggered for JS/TS changes.

2. Minimal, Targeted Fix: The change is small (3 additions, 1 deletion) and surgical - only modifying the specific condition that needs to change.

3. Clear Documentation: The added comment explains why JS changes trigger generator tests, referencing issue Run generator tests when packages/react-on-rails TypeScript files change #2205 for future maintainers.

4. Logical Solution: Generator tests already do a full build, so leveraging them to catch TypeScript errors is efficient and doesn't require new infrastructure.

---

🔍 Code Quality Analysis

Script Logic (script/ci-changes-detector:270-272)

Before:

if [ "$GENERATORS_CHANGED" = true ]; then
  RUN_GENERATORS=true
fi

After:

# JS changes trigger generator tests because generators do a full build
# which catches TypeScript compilation errors (see issue #2205)
if [ "$GENERATORS_CHANGED" = true ] || [ "$JS_CHANGED" = true ]; then
  RUN_GENERATORS=true
fi

Analysis:

• ✅ The OR logic is correct: generators should run if either generators changed OR JS/TS changed
• ✅ Comment provides context for future maintainers
• ✅ Consistent with similar patterns in the file (e.g., lines 266-268)

---

⚠️ Potential Concerns & Questions

1. Performance Impact (CI Runtime)

Issue: Every JS/TS change will now trigger generator tests, which are slower than unit tests.

Looking at the file patterns (line 97):

package.json|yarn.lock|packages/react-on-rails/src/*|packages/react-on-rails/src/**/*|
packages/react-on-rails/tests/*|packages/react-on-rails/tests/**/*|
packages/react-on-rails/scripts/*|packages/react-on-rails/scripts/**/*|
packages/react-on-rails/package.json|packages/react-on-rails/tsconfig.json

This means generator tests will run for:

• Any TypeScript source change (src/**/*)
• Any test change (tests/**/*)
• Build script changes
• Package dependency changes

Question: Is this the desired granularity? Should test-only changes also trigger full generator tests, or should the condition be more specific?

Possible refinement:

# Only trigger generators for source/config changes, not test changes
if [ "$GENERATORS_CHANGED" = true ] || 
   ([ "$JS_CHANGED" = true ] && echo "$CHANGED_FILES" | grep -q "packages/react-on-rails/src/"); then
  RUN_GENERATORS=true
fi

2. Missing Test Coverage

Observation: The PR description includes a test plan checkbox:

• Verify the script correctly detects TypeScript changes and sets run_generators=true

Recommendation: Before merging, please verify this locally:

# Test 1: Verify JS changes trigger generators
echo "test" > packages/react-on-rails/src/test.ts
script/ci-changes-detector origin/master | grep "Generator tests"
git checkout packages/react-on-rails/src/test.ts

# Test 2: Verify generator changes still trigger (regression test)
echo "test" > react_on_rails/lib/generators/test.rb
script/ci-changes-detector origin/master | grep "Generator tests"
git checkout react_on_rails/lib/generators/test.rb

3. Symmetry with Pro Package

Observation: Looking at lines 274-288, I notice that PRO_JS_CHANGED already triggers Pro lint, Pro tests, Pro dummy tests, and Pro node renderer tests.

Question: Should PRO_JS_CHANGED also trigger Pro generator tests? If the same issue could occur in the Pro package, this change might need to be symmetric.

Check if there are Pro generators that should also run on Pro JS changes.

---

🔐 Security Considerations

✅ No security concerns - this is a CI infrastructure change with no user-facing impact.

---

📊 Performance Considerations

Impact: Medium - Generator tests will run more frequently

Mitigation: The change detection is already sophisticated enough to skip CI for docs-only changes. The extra CI time is justified to catch type errors early.

Metrics to monitor:

• Average CI duration for JS-only changes (before vs after)
• Whether generator tests catch issues that would otherwise reach production

---

🧪 Testing Recommendations

1. Manual verification (as noted above) before merging
2. Monitor first few PRs after merge to ensure:
   • Generator tests run when expected
   • No excessive CI runs for trivial changes
3. Consider adding integration test to verify the script behavior:
   • Mock git diff output
   • Verify correct flags are set

---

📝 Suggestions

Optional: More Granular Control

If CI runtime becomes an issue, consider splitting into two flags:

• RUN_GENERATORS_FULL - Full generator tests (when generators change)
• RUN_GENERATORS_BUILD_ONLY - Just verify build succeeds (when JS/TS changes)

This would catch TypeScript errors faster without running the full generator suite.

Documentation Update

Consider updating .claude/docs/avoiding-ci-failure-cycles.md or similar docs to mention that TypeScript changes now trigger generator tests.

---

✅ Final Recommendation

APPROVE with minor suggestions

The core change is sound and addresses the issue correctly. The concerns raised are optimization opportunities, not blockers.

Before merging:

• ✅ Run the manual verification tests suggested above
• ✅ Check if Pro package needs similar changes
• ✅ Verify CI passes
• 🤔 Consider whether test-only changes should skip generator tests (optional optimization)

Great work on identifying and fixing the root cause! This will prevent similar issues from reaching production in the future.

---

References:

• Issue: Run generator tests when packages/react-on-rails TypeScript files change #2205
• Related PRs: Move React/Shakapacker version compatibility to generator smoke tests #2125 (type change), Fix TypeScript error in processPromise return type #2204 (type error fix)