Fix ci-changes-detector to recognize Pro package test files

[justin808]

Summary

Fixes the ci-changes-detector script to correctly identify test file changes in both the open-source and Pro packages, preventing false-positive "docs-only" detection.

Root Cause

The ci-changes-detector script was missing patterns for test files in the Pro package. When PR #2074 changed only packages/react-on-rails-pro/tests/testUtils.ts, the detector incorrectly classified it as a docs-only change because the pattern only matched packages/react-on-rails-pro/src/**/*.

This triggered the ensure-master-docs-safety action, which correctly failed the workflow since the previous commit had test failures. The safety mechanism worked as intended, but the root cause was the missing pattern.

Changes

• ✅ Add patterns for packages/react-on-rails/tests/**/* to trigger JS tests
• ✅ Add patterns for packages/react-on-rails-pro/tests/**/* to trigger Pro tests
• ✅ Add patterns for scripts/, package.json, and tsconfig.json in both packages
• ✅ Update comments to clarify what's included in each pattern

Testing

Tested with multiple file patterns:

• ✅ packages/react-on-rails-pro/tests/testUtils.ts → correctly detected as PRO_JS_CHANGED
• ✅ packages/react-on-rails/tests/test.ts → correctly detected as JS_CHANGED
• ✅ packages/react-on-rails-pro/package.json → correctly detected as PRO_JS_CHANGED
• ✅ README.md → correctly detected as docs_only

Impact

• Pro and open-source package test file changes will now correctly trigger CI
• Package configuration changes will trigger appropriate test suites
• Prevents false-positive "docs-only" detection that was blocking the latest master commit
• The ensure-master-docs-safety mechanism will work properly with correct change detection

🤖 Generated with Claude Code

Summary by CodeRabbit

• Chores
  • Improved build system detection for JavaScript and TypeScript file changes to ensure accurate identification of code-related modifications.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

Modified the CI change detection script to include additional JavaScript/TypeScript source file patterns (tests, scripts, tsconfig.json, and package.json files) for both React on Rails core and Pro variants, expanding when JS_CHANGED and PRO_JS_CHANGED flags are triggered.

Changes

Cohort / File(s)	Summary
CI Detection Script Expansion script/ci-changes-detector	Expanded source file detection patterns to include tests, tests/**/* , scripts, tsconfig.json, and package.json files for React on Rails core and Pro variants; JS_CHANGED and PRO_JS_CHANGED flags now trigger for changes to these additional file categories

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

• Verify that added file patterns correctly capture intended test, script, and config file changes
• Ensure Pro variant detection patterns align with the core variant structure
• Confirm that DOCS_ONLY flag behavior remains appropriate for the new patterns

Possibly related PRs

• #2020: Related modification to CI change detection for React on Rails Pro and JavaScript file tracking, adding Pro-specific JS/TS detection with PRO_* flags.

Suggested labels

bug, review-needed

Suggested reviewers

• AbanoubGhadban
• alexeyr-ci2

Poem

🐰 A script that sees with broader sight,
Now tests and configs shine so bright,
Pro patterns join the detection dance,
CI gates get a better chance! ✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly identifies the main change: fixing the ci-changes-detector to recognize Pro package test files, which aligns with the PR's primary objective of correcting test file detection in the CI script.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ 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-master-ci-logic

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3eb3961 and 0dc5432.

📒 Files selected for processing (1)

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

🧰 Additional context used

🧠 Learnings (7)

📓 Common learnings

Learnt from: alexeyr-ci2
Repo: shakacode/react_on_rails PR: 1732
File: spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx:40-44
Timestamp: 2025-04-26T21:55:55.874Z
Learning: In the react_on_rails project, files under `app-react16` directories are copied/moved to corresponding `/app` directories during the conversion process (removing the `-react16` suffix), which affects their relative import paths at runtime.

Learnt from: Romex91
Repo: shakacode/react_on_rails PR: 1697
File: package-scripts.yml:28-28
Timestamp: 2025-02-12T16:38:06.537Z
Learning: The file `node_package/lib/ReactOnRails.full.js` is autogenerated during the build process and should not be present in the repository.

Learnt from: 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.

📚 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/ci-changes-detector

📚 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/ci-changes-detector

📚 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:

• script/ci-changes-detector

📚 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:

• script/ci-changes-detector

📚 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:

• script/ci-changes-detector

📚 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:

• script/ci-changes-detector

⏰ 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: claude-review

🔇 Additional comments (2)

script/ci-changes-detector (2)

94-95: LGTM! Fix correctly includes test and configuration files.

The expanded patterns now properly detect changes to test files, scripts, and package configurations, which resolves the false-positive "docs-only" classification issue described in the PR.

Minor note on pattern redundancy: Both tests/* and tests/**/* are included (same for scripts). In bash case patterns, * already matches path separators, so tests/* will match nested files like tests/unit/foo.test.ts. The **/* variant is functionally redundant but harmless and consistent with existing patterns elsewhere in the script (lines 77, 83, etc.).

---

112-113: LGTM! Pro package patterns mirror the open-source fix.

The expanded patterns now correctly detect changes to Pro package test files, scripts, and configurations. The PR testing confirms that packages/react-on-rails-pro/tests/testUtils.ts now triggers PRO_JS_CHANGED as intended, preventing the false "docs-only" classification.

---

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

Summary

This PR correctly fixes a critical gap in the ci-changes-detector script that was causing test file changes in Pro package to be incorrectly classified as docs-only changes. The fix is well-targeted and addresses the root cause identified in the PR description.

✅ Strengths

1. Correct Root Cause Analysis: The PR accurately identified that packages/react-on-rails-pro/tests/testUtils.ts was not being caught by existing patterns, leading to false-positive "docs-only" detection.

2. Symmetrical Fix: The changes maintain symmetry between open-source and Pro package patterns, making the script easier to maintain:

   • Both packages now include: tests/*, tests/**/*, scripts/*, scripts/**/*, package.json, tsconfig.json

3. Improved Documentation: Updated comments from generic "JavaScript/TypeScript source" to more specific "JavaScript/TypeScript source (including tests, scripts, and package files)" clearly indicate what's included.

4. Proper Testing: The PR description shows manual testing with multiple file patterns, which is good practice for a change detection script.

5. Impact Awareness: The PR correctly understands the downstream impact on the ensure-master-docs-safety mechanism.

🔍 Observations & Minor Suggestions

1. Missing scripts/ Directory in Pro Package:

   • The pattern includes packages/react-on-rails-pro/scripts/* but this directory doesn't currently exist (only packages/react-on-rails/scripts/ exists with one file).
   • This is not a blocker - it's actually good defensive programming to include it in case it's added later.
   • Consider: If the Pro package is unlikely to ever have a scripts directory, you could remove it for clarity, but keeping it maintains symmetry and future-proofs the script.

2. Bash Pattern Matching Caveat:

   • The patterns use * and **/* which in bash case statements don't actually work recursively like they do in glob patterns.
   • However, this works correctly because the script is matching against individual file paths from git diff --name-only, not globbing the filesystem.
   • Example: packages/react-on-rails-pro/tests/utils/foo.ts will match packages/react-on-rails-pro/tests/**/* in a case statement.

3. Potential Enhancement (not required for this PR):

   • Consider adding basic tests for this script, perhaps using bats (Bash Automated Testing System) or simple shell assertions.
   • This would prevent future regressions like the one this PR fixes.
   • Example test cases could verify that known file paths trigger the correct flags.

4. Pattern Consistency Check:

   • Line 95: packages/react-on-rails/package.json is listed explicitly
   • Line 95: Also has package.json at root (without path prefix)
   • This is correct - root-level package.json and package-level package.json are both needed.

🎯 Code Quality Assessment

• Correctness: ✅ The patterns will correctly match the intended files
• Best Practices: ✅ Follows existing pattern conventions in the script
• Performance: ✅ No performance concerns (linear scan of changed files)
• Security: ✅ No security concerns (read-only analysis script)
• Maintainability: ✅ Improved with better comments

🧪 Test Coverage

Manual Testing: Documented and adequate for this change
Automated Testing: None exists for this script (acceptable for current state, but could be improved)

🚀 Recommendation

APPROVE - This PR is ready to merge. It correctly addresses the identified issue, maintains code consistency, and includes appropriate documentation of the changes.

The fix is minimal, targeted, and well-tested manually. The pattern additions are defensive and future-proof the script against similar issues.

---

Optional Follow-up (separate PR, not blocking):

• Add basic automated tests for ci-changes-detector script to prevent future regressions
• Document the bash case pattern matching behavior in a comment for future maintainers

Great work identifying and fixing this! 🎉

[claude]

Code Review: Fix ci-changes-detector to recognize Pro package test files

✅ Overall Assessment

This is an excellent bug fix that addresses a real issue in the CI change detection logic. The PR is well-documented, thoroughly tested, and follows best practices.

🎯 What This Fixes

Root Cause: When PR #2074 modified packages/react-on-rails-pro/tests/testUtils.ts, the ci-changes-detector incorrectly classified it as "docs-only" because the pattern only matched packages/react-on-rails-pro/src/**/*. This triggered the ensure-master-docs-safety action, which correctly failed since the previous commit had test failures.

Solution: Add patterns to detect changes in tests/, scripts/, package.json, and tsconfig.json for both open-source and Pro packages.

---

💡 Strengths

1. Comprehensive Pattern Coverage: The fix correctly adds patterns for:

   • Test files (tests/* and tests/**/*)
   • Build scripts (scripts/* and scripts/**/*)
   • Package configuration (package.json, tsconfig.json)

2. Symmetric Treatment: Both open-source and Pro packages get the same pattern enhancements, maintaining consistency.

3. Improved Documentation: The comments now clearly state "including tests, scripts, and package files" which helps future maintainers understand the scope.

4. Thorough Testing: The PR description shows multiple test cases covering different file types and confirming the patterns work correctly.

5. Proper Context: The PR explains the interaction with the ensure-master-docs-safety mechanism, showing understanding of the broader CI system.

---

🔍 Code Quality Analysis

Pattern Correctness ✅
The bash case patterns are correct:

• packages/react-on-rails/tests/* matches files directly in tests/
• packages/react-on-rails/tests/**/* matches files in subdirectories
• Same logic for scripts/ directories

Existing Directory Verification ✅

• packages/react-on-rails/tests/ exists (13 test files)
• packages/react-on-rails-pro/tests/ exists (17 files including testUtils.ts)
• packages/react-on-rails/scripts/ exists (symlink-node-package script)
• packages/react-on-rails-pro/scripts/ does NOT exist currently

---

🤔 Minor Observations

1. Pro scripts/ directory doesn't exist yet: The pattern includes packages/react-on-rails-pro/scripts/* but this directory doesn't currently exist. This is fine (defensive programming for future additions), but worth noting.

2. Root-level tsconfig.json vs package-specific: The pattern adds packages/react-on-rails/tsconfig.json and packages/react-on-rails-pro/tsconfig.json. These are package-specific configs. The root-level tsconfig.json is already covered by the lint config pattern (line 131), so this is correct.

3. Pattern length: The case patterns are getting quite long (95 and 113 are now single lines with many pipe-separated patterns). This is fine for bash, but if this continues to grow, consider:

   • Refactoring to use functions or regex patterns
   • Or splitting into sub-patterns for readability

   However, this is not a blocker for this PR - the current approach is consistent with the existing pattern.

---

🔒 Security Considerations

No security concerns. This is a shell script used in CI for change detection only.

---

🎭 Performance Considerations

No performance impact. The script already iterates through all changed files; adding more patterns to the case statement is negligible overhead.

---

🧪 Test Coverage Assessment

The PR description shows manual testing with realistic file paths:

• ✅ Pro test file correctly detected as PRO_JS_CHANGED
• ✅ Open-source test file correctly detected as JS_CHANGED
• ✅ Package.json changes correctly detected
• ✅ Docs-only changes still work correctly

Recommendation: Consider adding integration tests for this script in the future, but manual testing is sufficient for this bug fix.

---

✨ Suggestions (Optional, Non-Blocking)

1. Future refactoring: If the pattern lists continue to grow, consider extracting patterns into variables:

   JS_PATTERNS="package.json|yarn.lock|packages/react-on-rails/src/**/*|packages/react-on-rails/tests/**/*|..."
   case "$file" in
     $JS_PATTERNS)
       ...
   esac

   But this is not necessary for this PR.

2. Consider adding .github/workflows/package-js-tests.yml pattern: I notice the open-source JS pattern includes .github/workflows/package-js-tests.yml but the Pro pattern doesn't include a corresponding workflow file. If there's a Pro-specific workflow, consider adding it for symmetry.

---

🎉 Recommendation: APPROVE ✅

This PR:

• ✅ Solves a real, documented problem
• ✅ Uses the correct approach
• ✅ Is well-tested
• ✅ Follows project conventions
• ✅ Has clear documentation
• ✅ No security, performance, or quality concerns

The fix is minimal, focused, and addresses exactly what needs to be fixed without over-engineering. Great work! 🚀

---

Generated with Claude Code