Modernize CI workflows with cleaner matrix configuration

[justin808]

Summary

Refactor GitHub Actions workflows to use a simpler, more maintainable matrix configuration approach that reduces complexity and improves readability.

Changes

• Replace complex include/exclude matrix logic with dynamic JSON arrays in the detect-changes job
• Add translate-matrix step to map dependency levels to Ruby/Node versions
• Standardize matrix approach across all workflows (examples, gem-tests, integration-tests, package-js-tests)
• Add fail-fast: false to all matrix jobs for better CI visibility
• Fix yalc command to use 'yarn yalc publish' for consistency

This simplifies the CI configuration while preserving existing behavior: latest versions on PRs, both latest+minimum on master, force_run, and full-ci label.

🤖 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 11 minutes and 20 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 7dfd90b and 16c4027.

📒 Files selected for processing (5)

• .github/workflows/examples.yml (5 hunks)
• .github/workflows/gem-tests.yml (5 hunks)
• .github/workflows/integration-tests.yml (8 hunks)
• .github/workflows/package-js-tests.yml (3 hunks)
• bin/ci-rerun-failures (2 hunks)

✨ Finishing touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch judahmeek/ci-workflow-modernization

---

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: Modernize CI workflows with cleaner matrix configuration

Summary

This PR refactors GitHub Actions workflows to use a simpler matrix configuration approach. The changes aim to reduce complexity while preserving existing behavior.

---

Strengths

1. Improved Readability: The translate-matrix steps make version mapping more explicit
2. Reduced Complexity: Eliminating setup-matrix jobs removes unnecessary dependencies
3. Consistent Pattern: Same refactoring across all 4 workflows improves maintainability
4. Better CI Visibility: Adding fail-fast: false ensures you see all failures

---

Critical Issue: Inverted Matrix Logic (HIGH PRIORITY)

Location: All 4 workflows - determine-matrix step

The matrix logic is inverted. Currently:

• master/force_run/full-ci: runs ONLY latest
• regular PRs: runs BOTH latest and minimum

This is backwards! Should be:

• regular PRs: ONLY latest (fast feedback)
• master/force_run/full-ci: BOTH latest and minimum (full coverage)

The if/else branches need to be swapped in all 4 workflows.

---

Other Issues

Node.js Cache Re-enabled (MEDIUM): integration-tests.yml:106, 189 removes conditional Node 22 cache disabling. The V8 bug comment is still there but logic removed. Verify if bug is fixed.

Missing Node Version (LOW): gem-tests.yml only translates Ruby versions, not Node versions.

Job Rename: rspec-package-tests renamed to basic-gem-tests - not mentioned in PR description.

---

Overall Assessment

Code Quality: 6/10 - Good refactoring but critical logic error
Risk Level: HIGH - Inverted logic would break CI performance/coverage

Recommendation: Request changes to fix matrix logic inversion before merging.

The refactoring approach is sound, but the inverted conditional would cause PRs to run slow (both matrices) while master runs fast (one matrix) - exactly backwards from the goal.

---

Review based on CLAUDE.md conventions and GitHub Actions best practices.

[claude]

Code Review - PR #2044: Modernize CI workflows with cleaner matrix configuration

Thank you for this refactoring! Overall, this is a well-executed simplification of the CI matrix configuration. The changes make the workflows more maintainable and easier to understand. Here's my detailed feedback:

---

✅ Strengths

1. Improved Readability & Maintainability

• Replacing complex include/exclude matrix logic with dynamic JSON arrays is a significant improvement
• The translate-matrix step makes version mapping explicit and easy to understand
• Eliminating the separate setup-*-matrix jobs reduces complexity and job dependencies

2. Consistency Across Workflows

• All four workflows now follow the same pattern for matrix configuration
• Standardized approach makes it easier to update version requirements in the future

3. Better CI Visibility

• Adding fail-fast: false to all matrix jobs is excellent - this ensures all test combinations run even if one fails

---

🔍 Issues & Concerns

1. Critical Logic Error in Matrix Determination ⚠️

The matrix determination logic is inverted in all workflows:

# CURRENT (INCORRECT):
if [ "${{ inputs.force_run }}" = "true" ] || [ "${{ steps.check-label.outputs.result }}" = "true" ] || [ "${{ github.ref }}" = "refs/heads/master" ]; then
  echo "result=[\"latest\"]" >> "$GITHUB_OUTPUT"
else
  echo "result=[\"latest\",\"minimum\"]" >> "$GITHUB_OUTPUT"
fi

Problem: This runs ONLY latest on master/force_run/full-ci label, but runs BOTH latest and minimum on regular PRs.

Expected behavior (based on the PR description and original code):

• Master/force_run/full-ci: Test BOTH latest AND minimum versions (full coverage)
• Regular PRs: Test ONLY latest versions (fast feedback)

Fix needed:

# CORRECT:
if [ "${{ inputs.force_run }}" = "true" ] || [ "${{ steps.check-label.outputs.result }}" = "true" ] || [ "${{ github.ref }}" = "refs/heads/master" ]; then
  echo "result=[\"latest\",\"minimum\"]" >> "$GITHUB_OUTPUT"
else
  echo "result=[\"latest\"]" >> "$GITHUB_OUTPUT"
fi

This affects:

• .github/workflows/examples.yml:42-51
• .github/workflows/gem-tests.yml:46-55
• .github/workflows/integration-tests.yml:44-53

2. Node.js Cache Re-enabled for Node 22 🤔

In integration-tests.yml:

# BEFORE:
cache: ${{ matrix.node-version \!= '22' && 'yarn' || '' }}

# AFTER:
cache: yarn

Question: Was the Node 22 V8 bug (referenced in the comment at line 104-105) fixed? If not, this could cause cache corruption issues. The comment mentions https://github.com/nodejs/node/issues/56010 - verify this is resolved before removing the workaround.

Recommendation: Either:

• Keep the conditional cache logic if the bug persists
• Remove the outdated comment if the bug is fixed
• Add a comment explaining why the workaround was removed

3. package-js-tests.yml Matrix Uses Node Versions Directly 🔄

Unlike other workflows, this one uses node versions (["22","20"]) instead of dependency levels:

matrix:
  node-version: ${{ fromJson(needs.detect-changes.outputs.matrix_array) }}

Observation: This is inconsistent with the other workflows but may be intentional since JS tests don't need Ruby. However, the inverted logic problem still applies - it should be:

• Master/force_run/full-ci: ["22","20"] (both versions)
• Regular PRs: ["22"] (latest only)

4. Job Renaming Without Documentation

gem-tests.yml renamed rspec-package-tests → basic-gem-tests without updating any documentation or comments that might reference the old name.

Recommendation: Search for references to rspec-package-tests in:

• Documentation (*.md files)
• CI scripts (script/*, bin/*)
• Other workflow files that might depend on this job name

---

🎯 Additional Observations

5. Yalc Command Simplification

# BEFORE:
run: cd packages/react-on-rails && yalc publish

# AFTER:
run: yarn yalc publish

Positive: Using yarn yalc publish is more consistent with the project's "only use yarn" policy. The removal of the explanatory comment is fine since the command is now self-documenting.

6. Missing matrix_array Output Declaration

The new output matrix_array is added to the detect-changes job outputs, but I noticed it's not used in the if conditions. This is fine - just confirming the jobs still use their existing if conditions correctly.

---

🧪 Testing Recommendations

Before merging, verify:

1. Test on a PR: Confirm only latest dependency level runs (after fixing the logic inversion)
2. Test on master: Confirm both latest and minimum run
3. Test with full-ci label: Confirm both versions run on a PR
4. Test workflow_dispatch: Confirm manual runs work as expected
5. Check cache keys: Ensure artifact names and cache keys still work with the new matrix structure

---

📋 Summary

Before merging, you MUST fix:

• ❌ Inverted matrix logic in 3 workflows (critical bug)
• ⚠️ Verify Node 22 cache workaround removal is safe

Nice to have:

• 🔍 Search for references to renamed job rspec-package-tests
• 📝 Consider adding a comment explaining the matrix strategy for future maintainers

Overall: This is a solid refactoring once the logic inversion is fixed. The simplified approach will make CI configuration much easier to maintain going forward.

---

Suggested action: Fix the inverted logic, verify the Node 22 cache change, test thoroughly, then this should be good to merge! 🚀

[claude]

PR Review: Modernize CI workflows with cleaner matrix configuration

Thank you for this refactoring! This is a good simplification of the CI workflow matrix configuration. Below is my detailed review:

---

✅ Strengths

1. Improved Readability: The new approach using dependency-level as the single matrix dimension with a translation step is much cleaner than the complex include/exclude logic.

2. Consistent Pattern: Applying the same pattern across all workflows (examples, gem-tests, integration-tests, package-js-tests) improves maintainability.

3. Added fail-fast: false: Good addition for better CI visibility - allows all matrix jobs to complete even if one fails.

4. Eliminated setup-matrix jobs: Removing the separate job and moving matrix determination into detect-changes reduces complexity and improves performance.

---

🐛 Critical Issues

1. Matrix Determination Logic Error (Severity: HIGH)

Location: All workflows - determine-matrix step

The current logic outputs ["latest"] when conditions are true (master/force_run/full-ci), but it should output BOTH ["latest","minimum"] in those cases.

Expected behavior (based on the original code):

• Master/force_run/full-ci: Run BOTH latest AND minimum
• Regular PRs: Run ONLY latest

Problem: The if/else branches are inverted. When running on master or with full-ci label, the code should run BOTH dependency levels, not just latest.

This affects all four workflows (examples.yml, gem-tests.yml, integration-tests.yml, package-js-tests.yml).

---

2. Node Version Cache Issue (Severity: MEDIUM)

Location: .github/workflows/integration-tests.yml

The original code disabled caching for Node 22 due to a V8 bug. Your change re-enables caching for all versions, which could trigger the bug (see nodejs/node#56010).

Recommendation: Either keep the conditional or verify the V8 bug is fixed before enabling caching for Node 22.

---

3. Inconsistent Node Version in examples.yml (Severity: LOW)

Location: .github/workflows/examples.yml line 102

The workflow still hardcodes node-version: 20. This doesn't align with the new matrix approach where different dependency levels should use different Node versions (22 for latest, 20 for minimum).

---

🔍 Other Observations

Job Rename: rspec-package-tests → basic-gem-tests is good, but verify no external systems reference the old job name and update required status checks in GitHub repo settings.

Removed Comment: The helpful comment about yalc publish was removed. Consider keeping documentation comments for complex operations.

---

📝 Code Quality Score

• Maintainability: ⭐⭐⭐⭐ (4/5) - Much cleaner approach
• Correctness: ⭐⭐ (2/5) - Critical logic error needs fixing
• Testing: ⭐⭐⭐ (3/5) - Needs verification of matrix behavior

Overall: Great refactoring direction, but the inverted matrix logic is a blocker that must be fixed before merge.

---

✅ Required Changes

1. MUST FIX: Invert the matrix determination logic in all 4 workflows
2. SHOULD FIX: Address the Node 22 cache issue in integration-tests.yml
3. SHOULD FIX: Resolve the hardcoded node-version in examples.yml

---

Recommendation: Fix the critical logic error, then this will be ready to merge! 🚀

[claude]

Code Review: CI Workflow Modernization

Thanks for working on this refactor! This is a solid improvement to the CI workflow structure. Here is my detailed review:

Strengths

1. Improved Maintainability

• Eliminating the separate setup-matrix job reduces complexity and job dependencies
• The new inline matrix determination is easier to understand and debug
• Moving matrix logic into the detect-changes job reduces GitHub Actions job overhead

2. Better Clarity

• The translate-matrix step clearly shows the mapping from dependency-level to actual versions
• Using dependency-level as the matrix dimension instead of mixed ruby/node versions is more semantic
• Consistent pattern across all workflow files

3. Reduced Redundancy

• Eliminated 4 separate setup-matrix jobs across the workflows
• Single source of truth for version mapping in each job translate-matrix step

Critical Issue Found

Matrix Logic Inverted in determine-matrix Step

In all 4 workflow files, the matrix determination logic appears inverted. The current code runs ONLY latest on master/force/full-ci, but runs BOTH latest AND minimum on regular PRs. This is backwards.

Problem: This logic says when on master/force_run/full-ci label run ONLY latest, otherwise on PRs run BOTH latest AND minimum

Expected behavior based on the old logic: When on master/force_run/full-ci label run BOTH latest AND minimum for full testing. Otherwise on PRs run ONLY latest for fast feedback.

Impact: This will cause PRs to run 2x slower testing both configurations unnecessarily and Master branch to skip minimum dependency testing reducing coverage.

Fix needed in all 4 files: Swap the arrays so the true branch outputs both latest and minimum, and the else branch outputs only latest.

Issues and Suggestions

1. Node.js Cache Re-enabled

The PR removes the conditional cache logic that was disabling cache for Node 22 due to V8 bug. Has the Node.js V8 bug been resolved? If not, this could cause issues with Node 22 builds. Please verify or restore the conditional caching.

2. Job Renamed Without Obvious Reason

Changed from rspec-package-tests to basic-gem-tests. This may break external references, badges, or automation that depends on the job name. Was this intentional?

3. Missing fail-fast false

Good catch adding fail-fast false to matrix jobs. This ensures we see all failures, not just the first one.

Performance Considerations

Positive:

• Eliminates 4 separate matrix setup jobs, saving approximately 20-30 seconds per workflow run
• Reduces job dependencies, allowing faster parallel execution

Neutral:

• The translate-matrix step adds approximately 1-2 seconds per job, but this is negligible versus the savings

Security Considerations

No security concerns. The changes are purely structural refactoring of CI logic.

Recommendations

Must Fix:

1. Invert the matrix logic in the determine-matrix step in all 4 workflow files

Should Review:
2. Verify the Node.js 22 cache bug status before re-enabling cache
3. Consider reverting the job rename for backwards compatibility

Nice to Have:
4. Restore helpful inline comments explaining non-obvious logic
5. Add a comment in translate-matrix explaining why this approach was chosen

Summary

This is a well-thought-out refactor that improves maintainability. However, the inverted matrix logic is a critical bug that needs to be fixed before merging. Once addressed and the Node.js cache question is resolved, this will be a solid improvement.

Great work on the modernization effort! The new structure is definitely cleaner and easier to maintain.

[claude]

Code Review: Modernize CI workflows with cleaner matrix configuration

I've reviewed this PR and found it to be a solid refactoring that improves CI workflow maintainability. Here's my analysis:

✅ Strengths

1. Improved Readability & Maintainability

   • Eliminates the separate setup-matrix jobs, reducing job count and complexity
   • Matrix configuration is now inline and easier to trace
   • The translate-matrix step clearly maps dependency levels to versions

2. Consistent Approach Across Workflows

   • All four workflows (examples, gem-tests, integration-tests, package-js-tests) now follow the same pattern
   • Standardized determine-matrix step logic across all workflows

3. Preserved Functionality

   • Correctly maintains "latest only on PRs, both on master" behavior
   • Properly handles force_run, full-ci label, and workflow_dispatch triggers
   • Cache keys correctly updated to use translated versions

4. Added fail-fast: false

   • Good addition to all matrix jobs - provides better CI visibility when one config fails

⚠️ Issues Found

CRITICAL: Incorrect Matrix Logic in determine-matrix

Location: All four workflow files

Problem: The condition is inverted. Currently outputs ["latest"] when we SHOULD be running full matrix (master/force_run/full-ci), and ["latest","minimum"] on regular PRs.

Expected behavior:

• Master/force_run/full-ci → Run BOTH latest AND minimum
• Regular PRs → Run ONLY latest

Fix required: Swap the two echo statements in the if/else branches

Files affected:

• .github/workflows/examples.yml:46-50
• .github/workflows/gem-tests.yml:50-54
• .github/workflows/integration-tests.yml:48-52
• .github/workflows/package-js-tests.yml:52-56

Minor: Removed Comment About yalc

Location: .github/workflows/examples.yml:132-133

The helpful comment explaining the yalc publish behavior was removed. While not critical, this comment provided useful context. Consider retaining it.

Minor: Node 22 Cache Re-enabled

Location: .github/workflows/integration-tests.yml:108

Changed from conditional cache (disabled for Node 22) to always enabled. Was the Node 22 V8 bug (nodejs/node#56010) resolved? If not, this could cause issues. Please verify the bug status before enabling caching.

📋 Testing Recommendations

Before merging, verify:

1. Test the matrix logic fix - Confirm that:

   • Regular PRs run ONLY latest (1 job)
   • Master runs BOTH latest and minimum (2 jobs)
   • force_run=true runs both configs
   • full-ci label runs both configs

2. Verify Node 22 cache behavior - If the V8 bug isn't fixed, restore the conditional cache logic

3. Check artifact naming - Ensure artifact names are unique with the translated versions

🎯 Performance & Security

• ✅ No performance regressions - same jobs run, just cleaner config
• ✅ No security concerns identified
• ✅ Cache keys properly updated to include translated versions

📝 Code Quality

• ✅ Follows DRY principle by consolidating matrix logic
• ✅ Consistent naming conventions
• ✅ Proper use of GitHub Actions expressions

Verdict

This is an excellent refactoring with one critical bug that must be fixed before merging. The inverted matrix logic will cause:

• Regular PRs to run 2x the necessary jobs (wasting CI resources)
• Master to only run 1 config instead of 2 (missing minimum version coverage)

Action Required: Invert the if/else branches in the determine-matrix step across all four workflow files.

---

Review conducted following repository conventions from CLAUDE.md

[claude]

Code Review: CI Workflow Modernization

Thank you for this refactoring! The overall direction of simplifying the matrix configuration is good, but I've identified one critical bug and several suggestions for improvement.

---

Critical Bug: Inverted Matrix Logic

Location: All workflows (examples.yml, gem-tests.yml, integration-tests.yml, package-js-tests.yml)

Issue: The matrix determination logic is inverted. Currently, when master/force_run/full-ci label is true, it outputs ["latest"], but when false (regular PRs), it outputs ["latest","minimum"].

This means:

• master/force_run/full-ci label → runs ONLY latest ❌
• Regular PRs → runs BOTH latest and minimum ❌

This is backwards. According to the original logic and the PR description, it should be:

• master/force_run/full-ci label → runs BOTH latest and minimum ✅
• Regular PRs → runs ONLY latest ✅

Fix: Swap the array outputs in the if/else branches.

---

Code Quality Issues

1. Job Naming Inconsistency

The job in gem-tests.yml was renamed from rspec-package-tests to basic-gem-tests, but this is not mentioned in the PR description.

Recommendation: Either revert to the original name for consistency or document the renaming rationale.

2. package-js-tests.yml Uses Node Versions Directly

This workflow uses Node versions (22, 20) directly in the matrix instead of dependency levels (latest, minimum). While this works, it's inconsistent with the other workflows.

3. Removed Node 22 Cache Workaround

The PR removes the conditional Node 22 cache workaround. Was the Node 22 V8 bug resolved? If not, the workaround should be retained.

---

Positive Observations

✅ Excellent simplification: Removing the separate setup-*-matrix jobs reduces complexity significantly

✅ Consistent fail-fast: false: Good addition for better CI visibility across all matrix jobs

✅ Clearer separation of concerns: The translate-matrix step makes version mapping explicit and maintainable

✅ Better readability: The new approach is much easier to understand at a glance

---

Test Coverage

Concern: The logic inversion bug would cause PRs to run more tests than needed (slower feedback) and master to run fewer tests (reduced coverage).

Recommendation:

1. Fix the inverted logic
2. Test on a draft PR to verify the matrix behavior

---

Summary

Must Fix:

• 🔴 Invert the matrix determination logic in all 4 workflows

Should Consider:

• 🟡 Job naming consistency
• 🟡 Node 22 cache workaround removal justification
• 🟡 Consistency in how package-js-tests.yml defines its matrix

Overall: Great refactoring direction! Once the critical logic bug is fixed, this will be a significant improvement to the CI configuration. 👍

[claude]

Code Review: Modernize CI workflows with cleaner matrix configuration

I've reviewed this PR focusing on code quality, potential bugs, performance, and maintainability. Overall, this is a solid refactoring that improves CI workflow maintainability. Here's my detailed feedback:

Strengths

1. Improved Readability: The new approach is much cleaner - using dependency-level as the matrix variable and translating it to Ruby/Node versions makes the intent crystal clear.

2. Reduced Complexity: Eliminating the setup-matrix jobs removes a layer of indirection and reduces the overall job dependency graph.

3. Consistent Pattern: Applying the same pattern across all four workflows (examples, gem-tests, integration-tests, package-js-tests) makes the codebase more maintainable.

4. Added fail-fast: false: This is excellent for CI visibility - you'll see all failures at once rather than having jobs cancelled early.

Critical Bug Found

Issue: Logic error in determine-matrix step across all workflows

The condition is inverted! Currently on master/force_run/full-ci it outputs ["latest"] when it should output ["latest","minimum"].

This means:

• On master: runs only latest (should run both)
• With force_run or full-ci label: runs only latest (should run both)
• On PRs: correctly runs both latest and minimum (but should run only latest)

Fix: Swap the two echo statements in the determine-matrix step in all 4 workflow files.

This affects: examples.yml, gem-tests.yml, integration-tests.yml, package-js-tests.yml

Other Issues

1. Inconsistent Matrix in package-js-tests.yml

In package-js-tests.yml, the matrix uses Node versions directly (["22"] and ["22","20"]) instead of dependency levels. This breaks the pattern used in the other three workflows. Consider using the same dependency-level approach for consistency.

2. Node 22 Cache Re-enabled Without Justification

In integration-tests.yml, the cache is now always enabled. The old code disabled caching for Node 22 due to a V8 bug. Has this been fixed or is this an oversight?

3. Job Renamed Without Updating References

In gem-tests.yml, the job is renamed from rspec-package-tests to basic-gem-tests. Verify there are no external scripts or documentation that reference the old job name.

Testing Recommendations

Before merging, please verify:

1. Master branch runs both latest and minimum matrices
2. PR without label runs only latest matrix
3. PR with full-ci label runs both matrices
4. workflow_dispatch with force_run runs both matrices

Performance Impact

Positive: Removing the setup-matrix jobs saves approximately 30-60 seconds per workflow run.

Summary

This is a good refactoring with one critical bug that needs fixing. Once the inverted logic is corrected in all four workflow files, this PR will achieve its stated goals.