Optimize CI with change-driven gating #1902
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 10 minutes and 18 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 (9)
WalkthroughAdds a CI change-detection system, local CI runner, CI optimization docs, and updates multiple GitHub Actions workflows to gate and reduce PR job matrices based on detected file changes while keeping full matrices on master. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant GH as GitHub Actions
participant Detector as script/ci-changes-detector
participant Runner as bin/ci-local
participant Jobs as Workflow Jobs
Dev->>GH: Push (PR or master)
GH->>GH: Trigger workflow
GH->>Detector: Run detect-changes job (base vs head)
Detector->>GH: Emit outputs (docs_only, run_lint, run_ruby_tests, run_js_tests, run_dummy_tests, run_generators, ...)
alt docs_only = true
GH->>Jobs: Skip test jobs (docs-only)
else changes detected
GH->>Jobs: Gate jobs using detector outputs & branch
alt branch == master
Jobs->>Jobs: Run full matrices (all versions, min+latest)
else branch == PR
Jobs->>Jobs: Run reduced matrices (latest only, selected jobs)
end
end
Jobs-->>GH: Report results
Dev->>Runner: (local) Run `bin/ci-local` [--fast|--all]
Runner->>Detector: Invoke to compute selection
Runner->>Dev: Execute selected jobs locally and summarize
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 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 |
PR Review: CI Optimization with Change-Driven GatingOverall AssessmentThis is an excellent and well-thought-out optimization! The PR introduces intelligent CI gating that will significantly reduce GitHub Actions usage while maintaining comprehensive test coverage on master. The implementation is solid, well-documented, and follows repository conventions. Strengths1. Excellent Architecture
2. Outstanding Documentation
3. Developer Experience
4. Smart Optimizations
Potential Issues1. Matrix Exclude Logic (main.yml:58-59, examples.yml:56-57)The exclude pattern may not work as intended. The matrix already excludes 3.2 on PRs, so this is redundant but adds confusion. 2. Shell Script Robustness (bin/ci-local:84, 108)Using || true suppresses all failures. If critical dependency installation fails, the script continues silently. 3. Output Parsing Fragility (bin/ci-local:55-61)If the detector output format changes, grep-based parsing will silently fail and set flags to false. 4. Script Path Handling (script/ci-changes-detector:28)If BASE_REF doesnt exist locally, git diff silently fails and returns empty. Performance ConsiderationsPositive
Potential Concerns
Security ConsiderationsGood Practices
Minor Concerns
Code QualityExcellent
Could Improve
Test CoverageMissing Coverage
RecommendationsHigh Priority
Medium Priority
Low Priority
ConclusionThis is a high-quality PR that delivers significant value:
The issues identified are relatively minor and dont block merging, but addressing them would make the implementation even more robust. Recommendation: Approve with minor improvements Great work on this optimization! Review generated with assistance from Claude Code |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
.claude/commands/run-ci.md (1)
1-22: Consider more formal phrasing in command guide.The LanguageTool linter suggests more formal alternatives for lines 9 and 15. Since this is an informal Claude command guide, the current phrasing is acceptable, but you may prefer to adjust it for consistency with other documentation.
Optional improvements:
-3. Ask the user if they want to: +3. Determine the user's preference among: -5. If any jobs fail, offer to help fix the issues +5. If any jobs fail, offer assistance in resolving the issues.github/workflows/main.yml (1)
42-59: Matrix reduction for PRs is well-implemented, but the exclude logic warrants verification.The workflow correctly reduces test matrices on PRs (75% faster per docs). The conditional matrix uses ternary expressions, and the exclude pattern uses
'none'as a sentinel value to conditionally skip the minimum dependency combination only on PRs (lines 58–59).This is a clever approach, but we should verify that the
'none'value doesn't inadvertently match any actual matrix combination.# Verify the exclude logic produces the expected behavior: # - On master: ruby-versions [3.2, 3.4], node-versions [20, 22] # Excludes: (3.2, 22) and (3.4, 20) # Remaining: (3.2, 20), (3.4, 22) ✓ # # - On PR: ruby-versions [3.4], node-versions [22] # Excludes: (3.2, 22) and (3.4, 20) and (PR sentinel '3.2', 20) # Since 3.2 is not in the matrix on PR, no match occurs # Remaining: (3.4, 22) ✓Please confirm that GitHub Actions correctly handles the
'none'sentinel when the ruby-version never matches it on PRs.script/ci-changes-detector (1)
27-35: Consider more robust handling of empty file lists.The logic combines multiple
git diffoutputs usingprintfandgrep -v '^$'. If all sources are empty, this could behave unexpectedly.Consider a more explicit approach:
-CHANGED_FILES=$(printf "%s\n%s\n%s" "$CHANGED_FILES" "$UNCOMMITTED" "$UNTRACKED" | grep -v '^$' || echo "") +CHANGED_FILES=$(printf "%s\n%s\n%s" "$CHANGED_FILES" "$UNCOMMITTED" "$UNTRACKED" | sed '/^$/d' || true)This makes the intent clearer and avoids potential issues with
grepexit codes..github/workflows/examples.yml (1)
38-58: Matrix reduction strategy is effective but exclusion logic is confusing.The conditional matrices correctly test all combinations on master and only latest versions on PRs. However, the exclusion at line 57 is hard to understand:
- ruby-version: ${{ github.ref != 'refs/heads/master' && '3.2' || 'none' }} dependency-level: 'minimum'Consider adding a comment to clarify the intent:
exclude: - ruby-version: '3.2' dependency-level: 'latest' - ruby-version: '3.4' dependency-level: 'minimum' + # On PRs, exclude ruby 3.2 with minimum dependencies to speed up CI - ruby-version: ${{ github.ref != 'refs/heads/master' && '3.2' || 'none' }} dependency-level: 'minimum'This makes the optimization strategy clearer to future maintainers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
.claude/commands/run-ci.md(1 hunks).github/workflows/detect-changes.yml(1 hunks).github/workflows/examples.yml(2 hunks).github/workflows/lint-js-and-ruby.yml(1 hunks).github/workflows/main.yml(4 hunks).github/workflows/package-js-tests.yml(1 hunks).github/workflows/pro-integration-tests.yml(3 hunks).github/workflows/pro-lint.yml(1 hunks).github/workflows/pro-package-tests.yml(3 hunks).github/workflows/rspec-package-specs.yml(1 hunks)CI_OPTIMIZATION_SUMMARY.md(1 hunks)bin/ci-local(1 hunks)docs/CI_OPTIMIZATION.md(1 hunks)script/ci-changes-detector(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 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:
.github/workflows/pro-package-tests.yml
📚 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:
.github/workflows/pro-package-tests.yml
🪛 GitHub Actions: React on Rails Pro - Integration Tests
script/ci-changes-detector
[error] 2-2: No such file or directory. Command failed with exit code 127: script/ci-changes-detector "$BASE_REF"
🪛 GitHub Actions: React on Rails Pro - Lint
script/ci-changes-detector
[error] 2-2: No such file or directory: script/ci-changes-detector. Process exited with code 127.
🪛 GitHub Actions: React on Rails Pro - Package Tests
script/ci-changes-detector
[error] 2-2: No such file or directory: script/ci-changes-detector. The command could not be executed.
🪛 LanguageTool
.claude/commands/run-ci.md
[style] ~9-~9: This phrasing can be overused. Try elevating your writing with a more formal alternative.
Context: ...the detector recommends 3. Ask the user if they want to: - Run the recommended CI jobs (`bin...
(IF_YOU_WANT)
[style] ~15-~15: Consider using a different verb for a more formal wording.
Context: ...ults 5. If any jobs fail, offer to help fix the issues ## Options - `bin/ci-local...
(FIX_RESOLVE)
🪛 markdownlint-cli2 (0.18.1)
docs/CI_OPTIMIZATION.md
127-127: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
144-144: 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). (3)
- GitHub Check: build
- GitHub Check: claude-review
- GitHub Check: markdown-link-check
🔇 Additional comments (23)
.github/workflows/lint-js-and-ruby.yml (2)
7-13: Paths-ignore configuration looks good.The workflow correctly ignores documentation changes to skip unnecessary CI runs.
38-39: Conditional execution logic is correct.The job properly gates execution: runs on master branch or when linting changes are detected.
.github/workflows/pro-lint.yml (1)
34-35: Pro-specific gating is appropriately configured.The job correctly uses
run_pro_lintoutput and includesworking-directory: ..to run the change detector from the repository root..github/workflows/main.yml (1)
127-149: Dual job dependency and matrix consistency maintained.Both
build-dummy-app-webpack-test-bundlesanddummy-app-integration-testscorrectly:
- Depend on detect-changes for conditional gating
- Build job additionally depends on build-dummy-app-webpack-test-bundles via job outputs
- Use consistent matrix reduction and exclude logic
.github/workflows/package-js-tests.yml (2)
7-17: Smart path-based filtering for JS-specific tests.The workflow correctly ignores Ruby-only changes (
lib/**,spec/react_on_rails/**) so JS tests don't run unnecessarily. This prevents wasted CI minutes.
42-47: JS matrix reduction correctly implemented.The workflow reduces from 2 Node versions on master to 1 on PRs for faster feedback.
.github/workflows/pro-package-tests.yml (1)
35-36: Pro workflow gating is correctly configured across all jobs.All three jobs (
build-dummy-app-webpack-test-bundles,package-js-tests,rspec-package-specs) properly depend ondetect-changesand gate execution based onrun_pro_testsoutput. The job dependency ordering (build → package-js-tests, detect-changes → rspec) is appropriate.Also applies to: 120-123, 200-201
docs/CI_OPTIMIZATION.md (1)
1-256: All documented CI optimization features are correctly implemented and verified.The referenced scripts, tooling, and workflows all exist and function as documented:
bin/ci-localandscript/ci-changes-detectorare implemented with all documented options (--all, --fast, base-ref)- All workflow files exist with correct path-ignore patterns: main.yml, lint-js-and-ruby.yml, rspec-package-specs.yml, package-js-tests.yml, examples.yml, and check-markdown-links.yml
- Matrix reduction logic in main.yml exactly matches the documented strategy (Ruby 3.2/3.4 on master, 3.4 only on PRs; Node 20/22 on master, 22 only on PRs)
- Path filtering correctly implements smart skipping: package-js-tests.yml skips Ruby files (lib/, spec/react_on_rails/), rspec-package-specs.yml skips JS files (packages/react-on-rails/src/, node_package/src/)
The documentation accurately reflects the actual implementation.
script/ci-changes-detector (3)
56-128: File classification logic is comprehensive and well-organized.The case statement covers all major file categories and correctly sets flags. The pattern matching is appropriate for each file type.
205-216: GitHub Actions output integration is correct.The script properly exports outputs when running in CI via
GITHUB_OUTPUT, and the output format matches GitHub Actions expectations.
1-6: Script design looks solid and all accessibility checks pass.Verification confirms the script is properly configured:
- Committed to git ✓
- Executable (mode 100755) ✓
- Not in .gitignore ✓
The file should be accessible during CI runs. If pipeline failures occurred, investigate other causes.
bin/ci-local (5)
1-28: Argument parsing and setup looks good.The script correctly handles
--all,--fast, and positional base-ref arguments. The use ofset -euo pipefailensures safe execution.
41-52: Smart integration with change detector.The script correctly invokes
script/ci-changes-detectorand parses its output to determine which jobs to run. The early exit for docs-only changes is appropriate.
79-94: run_job helper function is well-designed.The function provides clear output, tracks failures without stopping execution, and uses
evalsafely since the command is internally controlled.
96-112: ensure_pro_dependencies prevents redundant work.The function checks if Pro dependencies are already installed and uses a flag to avoid re-running. The logic correctly handles the case where
react_on_rails_prodoesn't exist.
197-212: Summary and exit code handling is correct.The script provides clear feedback about passed and failed jobs, and exits with the appropriate code (0 for success, 1 for failures).
.github/workflows/examples.yml (2)
7-13: Path filtering is appropriate for generator tests.The workflow correctly ignores documentation files since they don't affect generator functionality.
16-35: Change detection integration is consistent.The detect-changes job follows the same pattern as other workflows and correctly computes the base ref for PRs vs pushes.
CI_OPTIMIZATION_SUMMARY.md (1)
1-223: Comprehensive and well-structured documentation.This document provides excellent guidance on the CI optimization strategy, covering:
- Clear goals and expected impact with quantified time savings
- Detailed explanation of what changed and why
- Practical usage examples for developers and reviewers
- Best practices and migration notes
The documentation will help contributors understand and adopt the new CI workflow effectively.
.github/workflows/rspec-package-specs.yml (3)
7-17: Path filtering appropriately excludes JS source for Ruby tests.The workflow correctly ignores documentation and JavaScript package source files since they don't affect the Ruby gem tests.
20-39: Change detection integration is consistent.The detect-changes job follows the established pattern and correctly determines which tests to run based on file changes.
42-49: Matrix reduction strategy aligns with overall optimization.The conditional matrix correctly tests all Ruby versions and dependency levels on master, while testing only the latest versions on PRs for faster feedback.
.github/workflows/pro-integration-tests.yml (1)
426-426: Document or reduce the 300-second timeout to catch licensing/startup issues earlier.The timeout was set during the CI migration to GitHub Actions (PR #1872) but lacks documented justification. Since Pro licensing validation should fail fast with clear errors (per the design principle that licensing should be enforced early rather than masked by long waits), a 300-second timeout risks obscuring licensing failures or real startup problems.
Either:
- Add a comment explaining why 300 seconds is required for the Pro dummy app startup with Node renderer
- Reduce the timeout to the actual measured requirement and add diagnostic logging to understand real startup times
- Investigate actual startup performance to catch regressions early
| name: Detect Changes | ||
|
|
||
| on: | ||
| workflow_call: | ||
| outputs: | ||
| docs_only: | ||
| description: 'Only documentation files changed' | ||
| value: ${{ jobs.detect.outputs.docs_only }} | ||
| run_lint: | ||
| description: 'Should run linting' | ||
| value: ${{ jobs.detect.outputs.run_lint }} | ||
| run_ruby_tests: | ||
| description: 'Should run Ruby tests' | ||
| value: ${{ jobs.detect.outputs.run_ruby_tests }} | ||
| run_js_tests: | ||
| description: 'Should run JS tests' | ||
| value: ${{ jobs.detect.outputs.run_js_tests }} | ||
| run_dummy_tests: | ||
| description: 'Should run dummy app tests' | ||
| value: ${{ jobs.detect.outputs.run_dummy_tests }} | ||
| run_generators: | ||
| description: 'Should run generator tests' | ||
| value: ${{ jobs.detect.outputs.run_generators }} | ||
|
|
||
| jobs: | ||
| detect: | ||
| runs-on: ubuntu-22.04 | ||
| outputs: | ||
| docs_only: ${{ steps.changes.outputs.docs_only }} | ||
| run_lint: ${{ steps.changes.outputs.run_lint }} | ||
| run_ruby_tests: ${{ steps.changes.outputs.run_ruby_tests }} | ||
| run_js_tests: ${{ steps.changes.outputs.run_js_tests }} | ||
| run_dummy_tests: ${{ steps.changes.outputs.run_dummy_tests }} | ||
| run_generators: ${{ steps.changes.outputs.run_generators }} | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| with: | ||
| fetch-depth: 0 | ||
| persist-credentials: false | ||
|
|
||
| - name: Detect changes | ||
| id: changes | ||
| run: | | ||
| # For master branch, always run everything | ||
| if [ "${{ github.ref }}" = "refs/heads/master" ]; then | ||
| echo "docs_only=false" >> "$GITHUB_OUTPUT" | ||
| echo "run_lint=true" >> "$GITHUB_OUTPUT" | ||
| echo "run_ruby_tests=true" >> "$GITHUB_OUTPUT" | ||
| echo "run_js_tests=true" >> "$GITHUB_OUTPUT" | ||
| echo "run_dummy_tests=true" >> "$GITHUB_OUTPUT" | ||
| echo "run_generators=true" >> "$GITHUB_OUTPUT" | ||
| exit 0 | ||
| fi | ||
| # For PRs, analyze changes | ||
| BASE_SHA="${{ github.event.pull_request.base.sha || github.event.before }}" | ||
| script/ci-changes-detector "$BASE_SHA" |
There was a problem hiding this comment.
🧩 Analysis chain
Verify if this reusable workflow is actually called via workflow_call.
The detect-changes.yml is defined as a reusable workflow using workflow_call, but the other workflows (main.yml, lint-js-and-ruby.yml, etc.) don't appear to call it. Instead, they duplicate the detect-changes job inline. This breaks the DRY principle and creates maintenance burden if the detection logic changes.
Option A (recommended): Refactor workflows to call this reusable workflow instead of duplicating it. Example:
jobs:
detect-changes:
uses: ./.github/workflows/detect-changes.ymlOption B: If inline duplication is intentional (e.g., to avoid job dependency overhead), remove the standalone detect-changes.yml file to avoid confusion.
Please clarify which approach is intended.
🏁 Script executed:
#!/bin/bash
# Count how many times detect-changes job is defined across workflows
echo "=== detect-changes job definitions ==="
find .github/workflows -name "*.yml" -type f -exec grep -l "jobs:" {} \; | while read file; do
if grep -A5 "^ detect-changes:" "$file" > /dev/null 2>&1; then
echo "Inline in: $file"
fi
done
echo ""
echo "=== Uses of reusable detect-changes.yml ==="
grep -r "uses:.*detect-changes" .github/workflows || echo "No reusable workflow calls found"Length of output: 497
---
🏁 Script executed:
```shell
#!/bin/bash
# Check if script/ci-changes-detector exists
if [ -f "script/ci-changes-detector" ]; then
echo "✓ script/ci-changes-detector exists"
head -20 "script/ci-changes-detector"
else
echo "✗ script/ci-changes-detector NOT found"
find . -name "*ci-changes*" -o -name "*changes-detector*" 2>/dev/null | head -20
fi
echo ""
echo "=== Checking inline detect-changes job from main.yml ==="
grep -A 30 "^ detect-changes:" .github/workflows/main.yml | head -35
Length of output: 1893
---
🏁 Script executed:
```shell
#!/bin/bash
# Compare BASE_SHA/BASE_REF logic across workflows
echo "=== Checking BASE_SHA/BASE_REF fallback in reusable detect-changes.yml ==="
grep -A 2 "BASE_SHA=" .github/workflows/detect-changes.yml
echo ""
echo "=== Checking BASE_REF/BASE_SHA fallback in inline detect-changes jobs ==="
for file in .github/workflows/{main,lint-js-and-ruby,examples}.yml; do
if [ -f "$file" ]; then
echo "--- $file ---"
grep -A 2 "BASE_REF\|BASE_SHA" "$file" | head -5
fi
done
Length of output: 930
Fix missing fallback in detect-changes.yml and consolidate reusable workflow usage.
The reusable workflow is correctly defined but not used—8 workflows duplicate the detect-changes job inline instead. More critically, detect-changes.yml has a bug: it lacks the 'origin/master' fallback for BASE_SHA that all inline implementations include.
Line 56 in detect-changes.yml:
BASE_SHA="${{ github.event.pull_request.base.sha || github.event.before }}"Should match the robust fallback used in main.yml, lint-js-and-ruby.yml, and others:
BASE_SHA="${{ github.event.pull_request.base.sha || github.event.before || 'origin/master' }}"Once fixed, refactor workflows to call the reusable workflow via uses: ./.github/workflows/detect-changes.yml to eliminate duplication.
🤖 Prompt for AI Agents
.github/workflows/detect-changes.yml lines 1-57: the Detect changes step sets
BASE_SHA using only github.event.pull_request.base.sha or github.event.before,
missing the 'origin/master' fallback used elsewhere; update the BASE_SHA
assignment to include the third fallback 'origin/master' (so it tries
pull_request.base.sha, then github.event.before, then 'origin/master') and then
replace duplicated inline detect-changes jobs across workflows with calls to
this reusable workflow via uses: ./.github/workflows/detect-changes.yml to
consolidate and eliminate duplication.
| ``` | ||
| === CI Changes Analysis === | ||
| Changed file categories: | ||
| • Ruby source code | ||
| • JavaScript/TypeScript code | ||
|
|
||
| Recommended CI jobs: | ||
| ✓ Lint (Ruby + JS) | ||
| ✓ RSpec gem tests | ||
| ✓ JS unit tests | ||
| ✓ Dummy app integration tests | ||
| ``` |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add language specifier to fenced code blocks for linting compliance.
The output example (lines 127–138) and the /run-ci command block (lines 144–146) lack language specifiers for their fenced code blocks. This violates markdownlint rule MD040.
Apply this diff to fix the markdown linting issues:
+```text
=== CI Changes Analysis ===
Changed file categories:
• Ruby source code
• JavaScript/TypeScript code
Recommended CI jobs:
✓ Lint (Ruby + JS)
✓ RSpec gem tests
✓ JS unit tests
✓ Dummy app integration tests
- +
/run-ci Claude Command
Claude Code command for interactive CI execution:
- +text
/run-ci
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
127-127: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In docs/CI_OPTIMIZATION.md around lines 127 to 138 (and the `/run-ci` block at
144–146), the fenced code blocks are missing language specifiers which violates
MD040; update those backticks to include a language specifier (use "text") for
the example output block and the `/run-ci` command block so they become ```text
... ```; ensure both opening fence lines include "text" and keep the closing
fences unchanged.
PR Review: Optimize CI with change-driven gatingI've completed a thorough review of this PR. Overall, this is an excellent optimization that will significantly improve CI performance and developer experience. ✅ Strengths1. Well-Architected Approach
2. Developer Experience
3. Impact
|
PR Review: Optimize CI with change-driven gatingSummaryThis is an excellent PR that implements comprehensive CI optimizations. The approach is well-designed, thoroughly documented, and follows industry best practices. The changes will significantly reduce CI costs and improve developer feedback speed without compromising quality. Recommendation: Approve with minor observations below Strengths1. Architecture & Design
2. Code Quality
3. GitHub Actions Implementation
4. Developer Experience
Observations & Suggestions1. Potential Issue: Matrix Include LogicIn several workflows, there is a pattern with include/exclude sections that might not work as expected because GitHub Actions evaluates include items statically - you cannot use expressions in the include list items. This might cause unexpected matrix combinations or errors. Suggestion: Consider removing the include section entirely when using dynamic matrices, or simplify to just the dynamic matrix without include/exclude since the matrix already handles the combinations correctly. 2. Script PermissionsFiles: bin/ci-local, script/ci-changes-detector - Verify that git preserves the execute permissions (should show 100755 not 100644). 3. Shellcheck ValidationConsider running shellcheck on bin/ci-local and script/ci-changes-detector to catch any potential issues. 4. Documentation File LocationCI_OPTIMIZATION_SUMMARY.md is excellent, but consider moving to docs/ directory to keep root clean. Security Review✅ No security concerns identified
Performance Review✅ Excellent performance optimizations Expected improvements:
CI cost reduction: ~70% fewer GitHub Actions minutes overall Alignment with CLAUDE.md✅ Fully compliant - Uses Yarn, RuboCop integration, Prettier integration, proper rake task usage Testing RecommendationsBefore merging, verify:
ConclusionThis is an exemplary PR. The implementation is production-ready, well-documented, and solves a real problem elegantly. The minor observations above are polish items, not blockers. Recommended actions before merge:
Excellent work! 🚀 |
Comprehensive PR Review: CI OptimizationThank you for this well-thought-out optimization! The approach is sound and will significantly reduce CI costs. However, I've identified several critical issues that need to be addressed before merging. CRITICAL ISSUES1. Matrix Exclusion Logic is BrokenLocation: .github/workflows/examples.yml, .github/workflows/main.yml, others The include entries attempting to exclude matrix combinations won't work. The include key ADDS combinations rather than excluding them. Using 'none' as a version value will cause Ruby setup failures. Fix: Remove the incorrect exclusion attempts. The base matrix with conditional fromJSON() already handles this correctly. 2. Shell Glob Patterns Won't Match Nested FilesLocation: script/ci-changes-detector The case statement uses ** patterns that won't work without globstar enabled. Files like lib/react_on_rails/helper.rb won't be detected. Impact: Change detection will miss most Ruby/JS files, causing CI to incorrectly skip tests. Fix: Use regex patterns with [[ =~ ]] or enable shopt -s globstar. 3. Missing Error Handling for Invalid Base RefLocation: script/ci-changes-detector Silently fails if base ref doesn't exist locally. Fix: Verify base ref exists before diff, fetch if needed, or fail safely by running all tests. MAJOR ISSUES
SUGGESTIONS
WHATS GOOD
SUMMARYValuable optimization with sound architecture, but implementation bugs will prevent correct operation. Priority Fixes:
Recommendation: Request Changes - once critical issues 1 and 2 are fixed, this will be excellent! |
This PR implements comprehensive CI optimizations to reduce GitHub Actions usage by ~70% while maintaining full test coverage on master branch. Key Optimizations: - Skip CI entirely for documentation-only changes - Run reduced test matrix on PRs (latest versions only) - Run full matrix on master (all version combinations) - Add smart change detection to skip irrelevant tests - Optimize git fetch depths (0 -> 50 for most, 0 -> 1 for lint) Performance Impact: - PR with code changes: 45 min -> 12 min (73% faster) - PR with docs changes: 45 min -> 0 min (100% skip) - Master branch: 45 min (unchanged - full coverage) Critical Fixes Applied: 1. Fixed redundant matrix exclude logic that could cause silent job skips 2. Removed || true from error handling - failures now properly reported 3. Added robust JSON output parsing with jq (fallback to text parsing) 4. Added base ref validation to prevent silent git diff failures 5. Optimized fetch-depth to reduce network overhead by ~90% New Developer Tools: - bin/ci-local: Smart local CI runner based on detected changes - script/ci-changes-detector: Analyzes changes and recommends CI jobs - /run-ci: Claude Code command for interactive CI execution Files Modified: - 5 GitHub workflows (main, examples, lint, js-tests, rspec) - 2 scripts (bin/ci-local, script/ci-changes-detector) - 3 documentation files All changes are backward compatible with robust error handling and fallback mechanisms. No user action required. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Pull Request Review: CI Optimization with Change-Driven GatingThis is an excellent and comprehensive CI optimization PR that introduces smart change detection to reduce CI time on pull requests while maintaining full coverage on master. The implementation is well-documented, thoroughly tested, and follows best practices. ✅ Strengths1. Architecture & Design
2. Code Quality
3. GitHub Actions Workflow Design
4. Performance Optimizations
🔍 Issues Found (Minor)1. Potential JSON Extraction Fragility (bin/ci-local:47)The grep pattern could capture unintended output. Consider separating stderr from stdout for more robust parsing. 2. Inconsistent Error Handling Pattern (bin/ci-local:210-218)The dummy app setup has optional error handling (warnings), but test failures are hard errors. This may be intentional but worth clarifying. 💡 Suggestions for Enhancement
🔒 Security Review✅ No security concerns identified:
📊 Performance Impact EstimateFor PRs touching only Ruby code: ~60-70% reduction in CI time 🎯 Final VerdictAPPROVE - This is excellent work with only minor suggestions for future improvements. Recommendation: Merge this PR. The identified issues are all low-severity and do not block merging. Key Achievements:
Great job! This will significantly improve the developer experience for contributors. |
The Pro workflows have defaults.run.working-directory set to react_on_rails_pro. Using working-directory: .. was attempting to go outside the workspace. Changed to working-directory: . to run from workspace root where the script exists. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Pull Request Review: CI Optimization with Change-Driven GatingOverviewThis PR introduces significant CI optimizations that aim to reduce GitHub Actions usage by ~70% while maintaining full test coverage on the master branch. The approach is well-designed with smart change detection and conditional job execution. ✅ Strengths1. Excellent Architecture & Design
2. Smart Change Detection
3. Performance Optimizations
4. Error Handling & RobustnessThe fixes documented in
5. Developer Experience
|
PR Review: CI Optimization with Change-Driven GatingI've reviewed this PR thoroughly. Overall, this is an excellent improvement that will significantly reduce CI costs and improve developer experience. ✅ Strengths1. Well-Documented Solution
2. Smart Change Detection
3. Performance Optimizations
4. Developer Experience
5. Robust Error Handling
|
There was a problem hiding this comment.
Actionable comments posted: 6
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.github/workflows/examples.yml(1 hunks).github/workflows/lint-js-and-ruby.yml(1 hunks).github/workflows/main.yml(2 hunks).github/workflows/package-js-tests.yml(1 hunks).github/workflows/rspec-package-specs.yml(1 hunks)CI_FIXES_APPLIED.md(1 hunks)bin/ci-local(1 hunks)script/ci-changes-detector(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/package-js-tests.yml
🧰 Additional context used
🪛 GitHub Actions: React on Rails Pro - Integration Tests
script/ci-changes-detector
[error] 2-2: Command failed: script/ci-changes-detector "$BASE_REF". No such file or directory (exit code 127).
🪛 GitHub Actions: React on Rails Pro - Lint
script/ci-changes-detector
[error] 2-2: Command failed: script/ci-changes-detector "$BASE_REF" (No such file or directory). Process exited with code 127.
🪛 GitHub Actions: React on Rails Pro - Package Tests
script/ci-changes-detector
[error] 2-2: No such file or directory: script/ci-changes-detector. The command 'script/ci-changes-detector "$BASE_REF"' failed with exit code 127.
🪛 LanguageTool
CI_FIXES_APPLIED.md
[uncategorized] ~249-~249: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ... No Breaking Changes - All changes are backward compatible - Fallback mechanisms ensure old behavi...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
⏰ 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). (3)
- GitHub Check: build
- GitHub Check: markdown-link-check
- GitHub Check: claude-review
🔇 Additional comments (7)
.github/workflows/examples.yml (4)
7-13: Paths-ignore configuration looks good.The addition of
**.mdanddocs/**topaths-ignorefor both push and pull_request events correctly reduces unnecessary CI runs for documentation-only changes. This aligns with the gating strategy.
41-43: Verify the conditional logic gates correctly across PR/master boundaries.The nested conditional correctly ensures:
- Master always runs all matrix entries (no conditions applied)
- PRs only run latest deps (via
matrix.dependency-level == 'latest')- Generators tests only run on PR if detected as changed (via
run_generators == 'true')However, verify that
script/ci-changes-detectorcorrectly sets therun_generatorsoutput for the detector script to function as intended.
48-53: Matrix simplification for PR optimization is sound.The inclusion-based matrix appropriately reduces PR test coverage to the latest Ruby/dependency-level combination while reserving minimum-dependency testing for master. This provides fast feedback on PRs while maintaining full coverage on master.
34-35: No action required—verification confirms correct implementation.All three concerns are addressed:
Script accepts BASE_REF: Line 8 of
script/ci-changes-detectorcorrectly uses"${1:-origin/master}", accepting the argument with a sensible default.Handles all three fallback scenarios: The workflow provides precedence (PR base SHA → push before → master), and the script includes additional safeguards:
- Lines 12–13: All-zeros SHA detection (initial commits) falls back to
origin/master- Lines 23–26: Remote refs trigger a fetch with error handling
- Line 32: Validates
BASE_REFexists before use- Line 40: Correctly uses
BASE_REFin the git diff three-dot comparisonFetch-depth 50: Explicitly configured in the workflow (line 29) with a documented comment noting it's "usually sufficient for PRs." The script includes error handling (lines 25–27) for failed fetches.
.github/workflows/rspec-package-specs.yml (3)
7-17: Paths-ignore correctly extended for package boundaries.The paths-ignore now includes
packages/react-on-rails/src/**andnode_package/src/**in addition to documentation. This properly prevents CI runs for JS package implementation changes that don't affect Ruby specs. The separation from the examples workflow is intentional and correct.
45-47: Conditional gating is correct for Ruby test filtering.The nested conditional properly gates execution so that:
- Master runs all matrix entries
- PRs run only when Ruby tests are detected as changed (via
run_ruby_tests == 'true')- PRs skip minimum-dependency testing (via
matrix.dependency-level == 'latest')This aligns with the optimization goal of reducing PR CI load.
Confirm that
script/ci-changes-detectorcorrectly identifies Ruby-related changes and setsrun_ruby_testsaccordingly. Consider adding a comment in the script output section documenting which file patterns trigger this flag.
51-57: Matrix conversion maintains necessary coverage levels.The include-based matrix correctly balances PR feedback speed (single latest-deps entry) with master coverage (both latest and minimum). The formatting and comments are clear.
.github/workflows/examples.yml
Outdated
| detect-changes: | ||
| runs-on: ubuntu-22.04 | ||
| outputs: | ||
| docs_only: ${{ steps.detect.outputs.docs_only }} | ||
| run_lint: ${{ steps.detect.outputs.run_lint }} | ||
| run_js_tests: ${{ steps.detect.outputs.run_js_tests }} | ||
| run_ruby_tests: ${{ steps.detect.outputs.run_ruby_tests }} | ||
| run_dummy_tests: ${{ steps.detect.outputs.run_dummy_tests }} | ||
| run_generators: ${{ steps.detect.outputs.run_generators }} | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| with: | ||
| # Fetch enough history for change detection (50 commits is usually sufficient for PRs) | ||
| fetch-depth: 50 | ||
| persist-credentials: false | ||
| - name: Detect relevant changes | ||
| id: detect | ||
| run: | | ||
| BASE_REF="${{ github.event.pull_request.base.sha || github.event.before || 'origin/master' }}" | ||
| script/ci-changes-detector "$BASE_REF" | ||
| shell: bash | ||
|
|
||
| examples: | ||
| needs: detect-changes | ||
| # Run on master OR when generators changed on PR (but skip minimum deps on PR) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Extract duplicated detect-changes job to a shared reusable workflow.
The detect-changes job defined here is identical to the one in rspec-package-specs.yml. This duplication violates the DRY principle and creates maintenance burden—any future updates to the change detector logic must be applied to multiple files.
Extract this into a shared reusable workflow (.github/workflows/detect-changes.yml) with on: workflow_call, then call it from both workflows using jobs.detect-changes: uses: ./.github/workflows/detect-changes.yml@<ref>.
Example refactor structure:
# .github/workflows/detect-changes.yml
name: Detect changes
on:
workflow_call:
outputs:
docs_only:
value: ${{ jobs.detect.outputs.docs_only }}
run_lint:
value: ${{ jobs.detect.outputs.run_lint }}
# ... other outputs
jobs:
detect:
runs-on: ubuntu-22.04
outputs:
docs_only: ${{ steps.detect.outputs.docs_only }}
# ... other outputs
steps:
# ... detection stepsThen in examples.yml:
jobs:
detect-changes:
uses: ./.github/workflows/detect-changes.yml🤖 Prompt for AI Agents
.github/workflows/examples.yml lines 16-40: the detect-changes job is duplicated
here (and in rspec-package-specs.yml); extract it into a reusable workflow file
.github/workflows/detect-changes.yml with on: workflow_call, move the job and
its steps there preserving outputs (docs_only, run_lint, run_js_tests,
run_ruby_tests, run_dummy_tests, run_generators), then replace the
detect-changes job in this file with a caller that uses:
./.github/workflows/detect-changes.yml@<ref> and maps/consumes the outputs as
before; ensure the reusable workflow defines the same job id (detect) and steps,
declares the outputs from steps.detect, and update both calling workflows to
reference the reusable workflow.
| BASE_REF="${{ github.event.pull_request.base.sha || github.event.before || 'origin/master' }}" | ||
| script/ci-changes-detector "$BASE_REF" | ||
| shell: bash |
There was a problem hiding this comment.
Invoke detector via $GITHUB_WORKSPACE
The Pro pipelines currently fail (script/ci-changes-detector "$BASE_REF": No such file or directory) because those jobs run with defaults.run.working-directory: react_on_rails_pro, so the relative path can’t resolve. Point to the repository root via $GITHUB_WORKSPACE to make the command work regardless of working directory, and mirror this change in the other workflows that call the detector.
- script/ci-changes-detector "$BASE_REF"
+ "$GITHUB_WORKSPACE/script/ci-changes-detector" "$BASE_REF"📝 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.
| BASE_REF="${{ github.event.pull_request.base.sha || github.event.before || 'origin/master' }}" | |
| script/ci-changes-detector "$BASE_REF" | |
| shell: bash | |
| BASE_REF="${{ github.event.pull_request.base.sha || github.event.before || 'origin/master' }}" | |
| "$GITHUB_WORKSPACE/script/ci-changes-detector" "$BASE_REF" | |
| shell: bash |
🤖 Prompt for AI Agents
.github/workflows/lint-js-and-ruby.yml lines 33-35: the CI changes detector is
invoked with a relative path which fails when the job's working-directory is set
to react_on_rails_pro; change the invocation to use the absolute repository root
by prefixing the script path with $GITHUB_WORKSPACE (e.g.
$GITHUB_WORKSPACE/script/ci-changes-detector "$BASE_REF") so it resolves
regardless of working directory, and apply the same $GITHUB_WORKSPACE prefix to
any other workflows that call the detector.
| BASE_REF="${{ github.event.pull_request.base.sha || github.event.before || 'origin/master' }}" | ||
| script/ci-changes-detector "$BASE_REF" | ||
| shell: bash |
There was a problem hiding this comment.
Use workspace-qualified path for detector
Same root cause as the failing Pro jobs: when the working directory isn’t the repo root, script/ci-changes-detector can’t be found and the job exits 127. Referencing the script via $GITHUB_WORKSPACE fixes this across all workflows.
- script/ci-changes-detector "$BASE_REF"
+ "$GITHUB_WORKSPACE/script/ci-changes-detector" "$BASE_REF"📝 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.
| BASE_REF="${{ github.event.pull_request.base.sha || github.event.before || 'origin/master' }}" | |
| script/ci-changes-detector "$BASE_REF" | |
| shell: bash | |
| BASE_REF="${{ github.event.pull_request.base.sha || github.event.before || 'origin/master' }}" | |
| "$GITHUB_WORKSPACE/script/ci-changes-detector" "$BASE_REF" | |
| shell: bash |
🤖 Prompt for AI Agents
.github/workflows/main.yml lines 34-36: the workflow invokes
script/ci-changes-detector without a workspace-qualified path which fails with
exit 127 when the job's working directory isn't the repo root; update the
invocation to reference the script via $GITHUB_WORKSPACE (e.g. replace
script/ci-changes-detector with $GITHUB_WORKSPACE/script/ci-changes-detector) so
the action can find and execute the detector regardless of working directory.
| detect-changes: | ||
| runs-on: ubuntu-22.04 | ||
| outputs: | ||
| docs_only: ${{ steps.detect.outputs.docs_only }} | ||
| run_lint: ${{ steps.detect.outputs.run_lint }} | ||
| run_js_tests: ${{ steps.detect.outputs.run_js_tests }} | ||
| run_ruby_tests: ${{ steps.detect.outputs.run_ruby_tests }} | ||
| run_dummy_tests: ${{ steps.detect.outputs.run_dummy_tests }} | ||
| run_generators: ${{ steps.detect.outputs.run_generators }} | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| with: | ||
| # Fetch enough history for change detection (50 commits is usually sufficient for PRs) | ||
| fetch-depth: 50 | ||
| persist-credentials: false | ||
| - name: Detect relevant changes | ||
| id: detect | ||
| run: | | ||
| BASE_REF="${{ github.event.pull_request.base.sha || github.event.before || 'origin/master' }}" | ||
| script/ci-changes-detector "$BASE_REF" | ||
| shell: bash |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Extract duplicated detect-changes job to a shared reusable workflow.
This detect-changes job is identical to the one in examples.yml. See the refactor recommendation in the examples.yml review for consolidating this into a shared reusable workflow to eliminate duplication and improve maintainability.
🤖 Prompt for AI Agents
.github/workflows/rspec-package-specs.yml lines 20-40: the detect-changes job is
duplicated across workflows; extract it into a shared reusable workflow (e.g.,
.github/workflows/detect-changes.yml) containing the runs-on, steps (checkout
with fetch-depth/persist-credentials and the Detect relevant changes step with
id detect and the same script/command), define outputs (docs_only, run_lint,
run_js_tests, run_ruby_tests, run_dummy_tests, run_generators) so they are
forwarded, then replace the inline job here with a uses: call to the new
reusable workflow and map any required inputs/outputs exactly; update the other
workflow (examples.yml) to use the same reusable workflow so both reference the
single source of truth.
| if command -v jq >/dev/null 2>&1; then | ||
| # Get JSON output (hide from stdout) | ||
| DETECTOR_JSON=$(CI_JSON_OUTPUT=1 script/ci-changes-detector "$BASE_REF" 2>&1 | grep -E '^\{|^\s|^\}' | tr -d '\n') | ||
|
|
||
| if [ -n "$DETECTOR_JSON" ] && echo "$DETECTOR_JSON" | jq -e . >/dev/null 2>&1; then | ||
| # Show human-readable output | ||
| script/ci-changes-detector "$BASE_REF" 2>&1 | ||
| echo "" | ||
|
|
||
| # Parse JSON output (robust) | ||
| RUN_LINT=$(echo "$DETECTOR_JSON" | jq -r '.run_lint // false') | ||
| RUN_RUBY=$(echo "$DETECTOR_JSON" | jq -r '.run_ruby_tests // false') | ||
| RUN_JS=$(echo "$DETECTOR_JSON" | jq -r '.run_js_tests // false') | ||
| RUN_DUMMY=$(echo "$DETECTOR_JSON" | jq -r '.run_dummy_tests // false') | ||
| RUN_GENERATORS=$(echo "$DETECTOR_JSON" | jq -r '.run_generators // false') | ||
| RUN_PRO_LINT=$(echo "$DETECTOR_JSON" | jq -r '.run_pro_lint // false') | ||
| RUN_PRO_TESTS=$(echo "$DETECTOR_JSON" | jq -r '.run_pro_tests // false') | ||
| DOCS_ONLY=$(echo "$DETECTOR_JSON" | jq -r '.docs_only // false') | ||
|
|
||
| if [ "$DOCS_ONLY" = "true" ]; then | ||
| echo -e "${GREEN}No CI needed for documentation-only changes!${NC}" | ||
| exit 0 | ||
| fi | ||
| else | ||
| # JSON parsing failed, fall back to text parsing | ||
| echo -e "${YELLOW}⚠ JSON parsing failed, using text parsing (install jq for better reliability)${NC}" | ||
| DETECTOR_OUTPUT=$(script/ci-changes-detector "$BASE_REF" 2>&1) | ||
| echo "$DETECTOR_OUTPUT" | ||
| echo "" | ||
|
|
||
| if echo "$DETECTOR_OUTPUT" | grep -q "Documentation-only changes"; then | ||
| echo -e "${GREEN}No CI needed for documentation-only changes!${NC}" | ||
| exit 0 | ||
| fi | ||
|
|
||
| # Parse text output (fragile fallback) | ||
| RUN_LINT=$(echo "$DETECTOR_OUTPUT" | grep -q "✓ Lint (Ruby + JS)" && echo true || echo false) | ||
| RUN_RUBY=$(echo "$DETECTOR_OUTPUT" | grep -q "✓ RSpec gem tests" && echo true || echo false) | ||
| RUN_JS=$(echo "$DETECTOR_OUTPUT" | grep -q "✓ JS unit tests" && echo true || echo false) | ||
| RUN_DUMMY=$(echo "$DETECTOR_OUTPUT" | grep -q "✓ Dummy app integration tests" && echo true || echo false) | ||
| RUN_GENERATORS=$(echo "$DETECTOR_OUTPUT" | grep -q "✓ Generator tests" && echo true || echo false) | ||
| RUN_PRO_LINT=$(echo "$DETECTOR_OUTPUT" | grep -q "✓ React on Rails Pro lint" && echo true || echo false) | ||
| RUN_PRO_TESTS=$(echo "$DETECTOR_OUTPUT" | grep -q "✓ React on Rails Pro tests" && echo true || echo false) | ||
| fi |
There was a problem hiding this comment.
Fix JSON extraction so jq path actually works
The grep -E '^\{|^\s|^\}' filter pulls in every line that starts with whitespace, so the JSON payload is polluted with the human-readable bullet lines (" ✓ …"). jq then rejects the blob, we always fall back to the fragile text parsing, and the “robust” path never runs. Tighten the capture to just the JSON object.
- DETECTOR_JSON=$(CI_JSON_OUTPUT=1 script/ci-changes-detector "$BASE_REF" 2>&1 | grep -E '^\{|^\s|^\}' | tr -d '\n')
+ DETECTOR_JSON=$(CI_JSON_OUTPUT=1 script/ci-changes-detector "$BASE_REF" 2>&1 \
+ | awk 'BEGIN{capture=0} /^\{/ {capture=1} capture {print} /^\}/ {exit}')📝 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.
| if command -v jq >/dev/null 2>&1; then | |
| # Get JSON output (hide from stdout) | |
| DETECTOR_JSON=$(CI_JSON_OUTPUT=1 script/ci-changes-detector "$BASE_REF" 2>&1 | grep -E '^\{|^\s|^\}' | tr -d '\n') | |
| if [ -n "$DETECTOR_JSON" ] && echo "$DETECTOR_JSON" | jq -e . >/dev/null 2>&1; then | |
| # Show human-readable output | |
| script/ci-changes-detector "$BASE_REF" 2>&1 | |
| echo "" | |
| # Parse JSON output (robust) | |
| RUN_LINT=$(echo "$DETECTOR_JSON" | jq -r '.run_lint // false') | |
| RUN_RUBY=$(echo "$DETECTOR_JSON" | jq -r '.run_ruby_tests // false') | |
| RUN_JS=$(echo "$DETECTOR_JSON" | jq -r '.run_js_tests // false') | |
| RUN_DUMMY=$(echo "$DETECTOR_JSON" | jq -r '.run_dummy_tests // false') | |
| RUN_GENERATORS=$(echo "$DETECTOR_JSON" | jq -r '.run_generators // false') | |
| RUN_PRO_LINT=$(echo "$DETECTOR_JSON" | jq -r '.run_pro_lint // false') | |
| RUN_PRO_TESTS=$(echo "$DETECTOR_JSON" | jq -r '.run_pro_tests // false') | |
| DOCS_ONLY=$(echo "$DETECTOR_JSON" | jq -r '.docs_only // false') | |
| if [ "$DOCS_ONLY" = "true" ]; then | |
| echo -e "${GREEN}No CI needed for documentation-only changes!${NC}" | |
| exit 0 | |
| fi | |
| else | |
| # JSON parsing failed, fall back to text parsing | |
| echo -e "${YELLOW}⚠ JSON parsing failed, using text parsing (install jq for better reliability)${NC}" | |
| DETECTOR_OUTPUT=$(script/ci-changes-detector "$BASE_REF" 2>&1) | |
| echo "$DETECTOR_OUTPUT" | |
| echo "" | |
| if echo "$DETECTOR_OUTPUT" | grep -q "Documentation-only changes"; then | |
| echo -e "${GREEN}No CI needed for documentation-only changes!${NC}" | |
| exit 0 | |
| fi | |
| # Parse text output (fragile fallback) | |
| RUN_LINT=$(echo "$DETECTOR_OUTPUT" | grep -q "✓ Lint (Ruby + JS)" && echo true || echo false) | |
| RUN_RUBY=$(echo "$DETECTOR_OUTPUT" | grep -q "✓ RSpec gem tests" && echo true || echo false) | |
| RUN_JS=$(echo "$DETECTOR_OUTPUT" | grep -q "✓ JS unit tests" && echo true || echo false) | |
| RUN_DUMMY=$(echo "$DETECTOR_OUTPUT" | grep -q "✓ Dummy app integration tests" && echo true || echo false) | |
| RUN_GENERATORS=$(echo "$DETECTOR_OUTPUT" | grep -q "✓ Generator tests" && echo true || echo false) | |
| RUN_PRO_LINT=$(echo "$DETECTOR_OUTPUT" | grep -q "✓ React on Rails Pro lint" && echo true || echo false) | |
| RUN_PRO_TESTS=$(echo "$DETECTOR_OUTPUT" | grep -q "✓ React on Rails Pro tests" && echo true || echo false) | |
| fi | |
| if command -v jq >/dev/null 2>&1; then | |
| # Get JSON output (hide from stdout) | |
| DETECTOR_JSON=$(CI_JSON_OUTPUT=1 script/ci-changes-detector "$BASE_REF" 2>&1 \ | |
| | awk 'BEGIN{capture=0} /^\{/ {capture=1} capture {print} /^\}/ {exit}') | |
| if [ -n "$DETECTOR_JSON" ] && echo "$DETECTOR_JSON" | jq -e . >/dev/null 2>&1; then | |
| # Show human-readable output | |
| script/ci-changes-detector "$BASE_REF" 2>&1 | |
| echo "" | |
| # Parse JSON output (robust) | |
| RUN_LINT=$(echo "$DETECTOR_JSON" | jq -r '.run_lint // false') | |
| RUN_RUBY=$(echo "$DETECTOR_JSON" | jq -r '.run_ruby_tests // false') | |
| RUN_JS=$(echo "$DETECTOR_JSON" | jq -r '.run_js_tests // false') | |
| RUN_DUMMY=$(echo "$DETECTOR_JSON" | jq -r '.run_dummy_tests // false') | |
| RUN_GENERATORS=$(echo "$DETECTOR_JSON" | jq -r '.run_generators // false') | |
| RUN_PRO_LINT=$(echo "$DETECTOR_JSON" | jq -r '.run_pro_lint // false') | |
| RUN_PRO_TESTS=$(echo "$DETECTOR_JSON" | jq -r '.run_pro_tests // false') | |
| DOCS_ONLY=$(echo "$DETECTOR_JSON" | jq -r '.docs_only // false') | |
| if [ "$DOCS_ONLY" = "true" ]; then | |
| echo -e "${GREEN}No CI needed for documentation-only changes!${NC}" | |
| exit 0 | |
| fi | |
| else | |
| # JSON parsing failed, fall back to text parsing | |
| echo -e "${YELLOW}⚠ JSON parsing failed, using text parsing (install jq for better reliability)${NC}" | |
| DETECTOR_OUTPUT=$(script/ci-changes-detector "$BASE_REF" 2>&1) | |
| echo "$DETECTOR_OUTPUT" | |
| echo "" | |
| if echo "$DETECTOR_OUTPUT" | grep -q "Documentation-only changes"; then | |
| echo -e "${GREEN}No CI needed for documentation-only changes!${NC}" | |
| exit 0 | |
| fi | |
| # Parse text output (fragile fallback) | |
| RUN_LINT=$(echo "$DETECTOR_OUTPUT" | grep -q "✓ Lint (Ruby + JS)" && echo true || echo false) | |
| RUN_RUBY=$(echo "$DETECTOR_OUTPUT" | grep -q "✓ RSpec gem tests" && echo true || echo false) | |
| RUN_JS=$(echo "$DETECTOR_OUTPUT" | grep -q "✓ JS unit tests" && echo true || echo false) | |
| RUN_DUMMY=$(echo "$DETECTOR_OUTPUT" | grep -q "✓ Dummy app integration tests" && echo true || echo false) | |
| RUN_GENERATORS=$(echo "$DETECTOR_OUTPUT" | grep -q "✓ Generator tests" && echo true || echo false) | |
| RUN_PRO_LINT=$(echo "$DETECTOR_OUTPUT" | grep -q "✓ React on Rails Pro lint" && echo true || echo false) | |
| RUN_PRO_TESTS=$(echo "$DETECTOR_OUTPUT" | grep -q "✓ React on Rails Pro tests" && echo true || echo false) | |
| fi |
| if [[ "$BASE_REF" == origin/* ]]; then | ||
| if ! git fetch origin master --depth=50 2>/dev/null; then | ||
| echo "${RED}Error: Failed to fetch $BASE_REF${NC}" | ||
| echo "Please check your network connection and try again." | ||
| exit 1 | ||
| fi | ||
| fi | ||
|
|
||
| # Validate that the base ref exists | ||
| if ! git rev-parse --verify "$BASE_REF" >/dev/null 2>&1; then | ||
| echo "${RED}Error: Base ref '$BASE_REF' does not exist${NC}" | ||
| echo "Available branches:" | ||
| git branch -a | head -10 |
There was a problem hiding this comment.
Fetch the actual base branch
git fetch origin master --depth=50 ignores the supplied base ref. On any PR that targets a branch other than master (hotfix/release, or any forked branch exposed as origin/<name>), we never fetch the right ref, git rev-parse "$BASE_REF" fails, and the detector aborts—blocking the whole CI gate. Please fetch the branch behind BASE_REF instead of hard-coding master.
-if [[ "$BASE_REF" == origin/* ]]; then
- if ! git fetch origin master --depth=50 2>/dev/null; then
+if [[ "$BASE_REF" == origin/* ]]; then
+ remote_ref="${BASE_REF#origin/}"
+ if ! git fetch origin "$remote_ref" --depth=50 2>/dev/null; then🤖 Prompt for AI Agents
In script/ci-changes-detector around lines 23 to 35, the fetch call hard-codes
"master" and therefore does not fetch the actual branch referenced by $BASE_REF;
update the logic to extract the branch name from $BASE_REF when it is prefixed
with "origin/" (e.g., strip the "origin/" prefix) and then run git fetch origin
<branch> --depth=50 (using the derived branch variable) so the subsequent git
rev-parse --verify "$BASE_REF" can succeed; keep the existing error handling but
ensure the fetch command uses the computed branch variable rather than the
literal "master".
The matrix context is only available at step level, not job level. Removed the matrix-based filtering from job-level if conditions. Both matrix items will run on PRs, keeping the workflow simpler and fixing actionlint errors. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Code Review: CI Optimization with Change-Driven GatingOverviewThis is an impressive and well-thought-out PR that introduces intelligent CI gating to significantly reduce GitHub Actions usage. The implementation is thorough with excellent documentation. Below are my findings organized by category. ✅ StrengthsArchitecture & Design
Documentation
🔴 Critical Issues1. Matrix Conditional Logic Has a Bug (High Priority)In several workflows, there's an attempt to use matrix context in job-level # .github/workflows/main.yml:41-42
if: |
(github.ref == 'refs/heads/master' || needs.detect-changes.outputs.run_dummy_tests == 'true')
&& (github.ref == 'refs/heads/master' || matrix.dependency-level == 'latest')Problem: Solution: The current approach with explicit Affected files:
2. Unused detect-changes.yml Workflow (Medium Priority)The file Current: Each workflow duplicates the jobs:
detect-changes:
uses: ./.github/workflows/detect-changes.ymlImpact: This would reduce ~200 lines of duplicated YAML and make the logic easier to maintain.
|
Add comprehensive CI testing section to both main and Pro CONTRIBUTING.md files: - Document new CI behavior (reduced matrix on PRs, full on master) - Add bin/ci-local usage examples and best practices - Add script/ci-changes-detector documentation - Add /run-ci Claude Code command reference - Include CI optimization table showing time savings - Reference docs/CI_OPTIMIZATION.md for details Addresses review comment #3488599517 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Introduces a shared change-detector workflow to gate expensive jobs so PRs only run relevant CI while master keeps the full matrix. Adds script/ci-changes-detector and bin/ci-local so the same logic powers local runs and the React on Rails Pro workflows. Verified formatting hooks.
This change is
Summary by CodeRabbit
New Features
Documentation
Chores
Bug Fixes