Fix JS tests workflow in CI #2155
Conversation
WalkthroughUpdates CI job matrix and test steps; centralizes Jest ts-jest settings with a shared tsconfig and adds .cts/.mts handling; moves and adjusts several package dependency entries across root, package-level, and dummy spec package.json files; updates script/convert to include react-on-rails-pro adjustments. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (11)
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 |
Code Review ✅SummaryThis PR fixes a critical bug in the JS tests workflow where the matrix exclusion logic was not working correctly. The issue stemmed from a misunderstanding of how GitHub Actions matrix strategies work. What Was WrongThe original configuration used: matrix:
include:
- node-version: '22'
- node-version: '20'
exclude:
- node-version: ${{ ... && '20' || '' }}The problem: In GitHub Actions, The FixThe corrected configuration uses: matrix:
node-version:
- '22'
- '20'
exclude:
- node-version: ${{ ... && '20' || '' }}Now the matrix is defined properly with a base dimension ( Review Findings✅ Correct Fix: The change properly addresses the root cause by moving from ✅ Consistent Logic: The exclude condition remains unchanged and will now work as intended:
✅ No Breaking Changes: The fix only corrects the matrix generation; all other workflow logic remains unchanged. ✅ Performance Impact: This will actually improve PR performance by properly skipping Node 20 tests when not needed, as originally intended. RecommendationsNone required - this is a straightforward bug fix with no additional changes needed. Additional NotesI verified that:
Great catch on identifying this issue! 🎯 Review conducted by Claude Code |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Code ReviewSummaryThis PR fixes a critical bug in the GitHub Actions matrix configuration for the JS tests workflow. The changes correctly address how matrix exclusions work in GitHub Actions and add necessary Jest configuration to handle CommonJS TypeScript files. Positive Findings1. Workflow Matrix Fix - Correct Solution The change from 2. Jest Configuration Enhancement The Jest config changes properly support Areas for Consideration1. Test Coverage The PR description says this is a "CI-only change" but the Jest changes enable testing .cts files. Have you verified these files are being tested? Run 2. CI Verification Current CI shows 3. Module Kind Configuration The Suggestions1. Update PR Description Add a section about the Jest configuration enhancements, not just the workflow fix. 2. Add Preventive Comment Consider adding a comment in the workflow file: # IMPORTANT: Use array format (not 'include') so that 'exclude' works correctly3. Consider Cleaner Matrix Logic The empty string exclusion ( Before Merging
Risk Assessment: Low-MediumChanges are isolated and additive. The fix addresses a real bug. Main concern is the current CI failure needs resolution. Great catch on the |
alexeyr-ci2
force-pushed
the
alexeyr/fix-js-tests-ci
branch
from
137df57 to
adf7945
Compare
Code Review for PR #2155SummaryThis PR fixes two issues:
✅ Strengths1. Correct Fix for GitHub Actions MatrixThe change from Before (incorrect): matrix:
include:
- node-version: '22'
- node-version: '20'
exclude:
- node-version: ${{ ... && '20' || '' }}After (correct): matrix:
node-version:
- '22'
- '20'
exclude:
- node-version: ${{ ... && '20' || '' }}Why this matters: In GitHub Actions, 2. Well-Documented Jest ChangesThe Jest configuration changes are well-commented and explain:
3. Future-ProofingAdding
🔍 Observations & Questions1. Jest Transform PatternThe regex pattern
Question: Does this override the default ts-jest transform that was already handling Looking at the code, it appears you're spreading
2. Testing CoverageConcern: I don't see test files for the While adding Jest support is good, consider:
3. moduleFileExtensions OrderThe new order is: This is fine, but Jest tries extensions in order when resolving imports. The typical recommendation is to put the most common extensions first for performance. Current order seems reasonable. 🎯 Recommendations1. Consider Testing the CI FixLow Priority: Since this is a CI-only change and the previous config was broken, it's hard to verify the fix without waiting for actual CI runs. The change looks correct based on GitHub Actions documentation. Verification step: After merge, verify that on the next PR (without full-ci label):
On master or PRs with full-ci label:
2. Document the CI Matrix StrategyOptional: Consider adding a comment in the workflow file explaining the matrix strategy: strategy:
matrix:
node-version:
# Always run: Latest Node version (fast feedback on PRs)
- '22'
# Master and full-ci label: Minimum supported Node version (full coverage)
- '20'
exclude:
# Skip Node 20 on regular PRs (run only on master or with full-ci label)
- node-version: ${{ github.event_name == 'pull_request' && github.ref != 'refs/heads/master' && needs.detect-changes.outputs.has_full_ci_label != 'true' && '20' || '' }}The comments already exist and are clear - good job! ✅ 3. Validate Jest Config LocallyBefore merge, run: pnpm testTo ensure the Jest config changes don't break existing tests. 📊 Code Quality Assessment
✅ Final VerdictLGTM (Looks Good To Me) - This PR correctly fixes the GitHub Actions matrix issue and appropriately extends Jest configuration for CommonJS/ES Module TypeScript files. Pre-merge Checklist
Great work identifying and fixing the matrix configuration bug! 🎉 |
Code ReviewSummaryThis PR fixes a critical GitHub Actions workflow configuration bug and enhances Jest configuration to support CommonJS TypeScript files. The changes are well-structured and address real issues in the codebase. ✅ Positive Observations1. Correct Fix for GitHub Actions Matrix SyntaxThe workflow fix is spot-on. The original code misunderstood how GitHub Actions matrix syntax works:
Reference: GitHub Actions Matrix Documentation This is exactly the right approach and should make the conditional Node 20 testing work as intended. 2. Jest Config Enhancement for .cts/.mts FilesThe Jest configuration changes are valuable:
The 🔍 Observations & Questions1. Testing RecommendationWhile the changes look correct, I'd recommend: # Verify the workflow syntax is valid
actionlint .github/workflows/package-js-tests.yml
# Test Jest config can handle .cts files
pnpm test # Should now properly transform .cts files if there are tests for them2. Documentation NoteThe workflow fix addresses a silent failure that's been happening since the workflow was created. Consider:
3. Jest Config: Minor Style NoteThe Jest config changes are solid, but one small observation:
🎯 Final AssessmentCode Quality: ⭐⭐⭐⭐⭐
Bug Fixes: ⭐⭐⭐⭐⭐
Best Practices: ⭐⭐⭐⭐⭐
Security: ✅ No concerns
Performance: ✅ Neutral/Positive
✅ Recommendation: APPROVE & MERGEThis PR is ready to merge. It fixes a genuine bug in the CI configuration and properly extends Jest to handle all TypeScript file variants in the codebase. Post-merge verification:
Review conducted following guidelines in |
PR Review: Fix JS tests workflow in CIThank you for this PR! I've reviewed the changes and have the following feedback: ✅ Positive Findings1. Correct GitHub Actions Matrix Fix (.github/workflows/package-js-tests.yml)
2. Comprehensive Jest Config Update (jest.config.base.js)
|
Comprehensive Code Review - PR #2155SummaryThis PR fixes two issues:
Strengths1. Correct Fix for GitHub Actions Matrix SyntaxThe workflow change is architecturally correct. The original configuration misunderstood how GitHub Actions matrix strategies work. Why this matters: In GitHub Actions, exclude only applies to the base matrix dimensions, not to entries added via include. This was causing the conditional Node 20 exclusion to fail silently, resulting in unnecessary test runs on every PR. 2. Well-Structured Jest ConfigurationThe Jest config changes demonstrate good engineering practices:
The module: ESNext override is necessary to avoid ts-jest warnings about isolatedModules. Issues That Need ResolutionCRITICAL: Failing CI CheckThe build (22) job is failing. According to CLAUDE.md guidelines, you should never claim a test is fixed without running it locally first. Required actions before merge:
Recommendations
AssessmentCode Quality: ⭐⭐⭐⭐⭐ Excellent - Clean, focused changes with proper understanding Bug Fix Accuracy: ⭐⭐⭐⭐⭐ Excellent - Addresses the root cause correctly Project Standards: ⭐⭐⭐⭐⭐ Excellent - Follows CLAUDE.md conventions Current Status: Pre-Merge Checklist
This is a high-quality fix for a real CI configuration bug. The only blocker is the failing CI check. Great work identifying this subtle matrix exclusion issue! Review conducted following CLAUDE.md and .claude/docs/ guidelines |
Code Review - PR #2155SummaryThis PR fixes a CI workflow configuration issue and extends Jest configuration to handle CommonJS TypeScript files. Overall, the changes are solid improvements with one area requiring attention. Strengths1. Correct GitHub Actions Matrix Fix
2. Proper Jest Configuration for .cts Files
Issues Requiring AttentionCRITICAL: CI Failure on build job for Node 22 The PR shows a FAILURE status for the build (22) job. According to CLAUDE.md guidelines, you must never claim a test is fixed without running it locally first. Required actions before merge:
From CLAUDE.md: Do not push hopeful fixes and wait for CI feedback. Test locally first whenever technically possible. Additional Recommendations
Security and Performance
Adherence to CLAUDE.md GuidelinesGood:
Needs attention:
VerdictDo NOT merge until:
The code quality is good, but the CI failure must be resolved per project guidelines. Suggested Next Steps
Overall Assessment: Strong fixes for real issues, but blocked on CI failure resolution. |
alexeyr-ci2
force-pushed
the
alexeyr/fix-js-tests-ci
branch
from
adf7945 to
7f5a05d
Compare
Code Review for PR #2155SummaryThis PR fixes two distinct issues:
✅ Positive Findings1. GitHub Actions Matrix Fix - Correct and Well-UnderstoodThe fix correctly addresses a GitHub Actions matrix configuration bug: Problem Identified:
Solution:
2. Jest Configuration Enhancement - Needed and Well-ImplementedThe Jest config changes add proper support for .cts and .mts files: Why This Matters:
Code Quality:
🔍 Suggestions & Considerations1. Test the GitHub Actions Matrix FixImportant: This fix changes CI behavior. Please verify the matrix now correctly excludes Node 20 on regular PRs. 2. Consider Adding Tests for .cts/.mts FilesSince you're adding support for .cts/.mts testing, consider adding a test file that imports/tests ReactDOMServer.cts to verify the transform actually works and prevent regression. 3. DocumentationThe PR description explains the matrix issue well, but consider adding a comment in the workflow file explaining why node-version: is used instead of include: to prevent future developers from refactoring it back. 🛡️ Security & PerformanceSecurity: ✅ No security concerns - configuration changes only Performance: ✅ No performance impact expected
Final VerdictRecommendation: ✅ APPROVE with minor suggestions Both fixes are technically correct, well-implemented, and address real issues. The main action item is to verify the matrix fix works as intended by observing this PR's CI runs. Great work identifying and fixing the matrix configuration issue! 🎉 |
Code Review - PR #2155: Fix JS tests workflow in CISummaryThis PR fixes two distinct issues:
✅ Code Quality & Best PracticesGitHub Actions Fix (.github/workflows/package-js-tests.yml)
Jest Configuration Enhancement (jest.config.base.js)
🐛 Potential Issues1. Jest Configuration Scope Concern
Question: Are you planning to add tests that directly import 2. Module Resolution Override // Override hybrid module kind (Node16/NodeNext) to avoid ts-jest warning
// about requiring isolatedModules: true
module: 'ESNext',However, the base 🚀 Performance ConsiderationsNo performance concerns. The changes are configuration-only. 🔒 Security ConcernsNo security issues identified. 🧪 Test CoverageCritical Gap: The PR description states it's a "CI-only change" but there are actually two separate changes:
Recommendations:
📋 Checklist ItemsFrom CLAUDE.md requirements:
💡 SuggestionsOption 1: Keep as-is but verify
Option 2: Split the PR (recommended)
Option 3: Minimal version
🎯 VerdictThe GitHub Actions fix is good and should be merged - it fixes a real bug. The Jest configuration changes need clarification:
Great catch on the matrix configuration bug! The linked screenshot clearly shows the issue. |
AbanoubGhadban
force-pushed
the
alexeyr/fix-js-tests-ci
branch
from
7f5a05d to
6fc616b
Compare
…n-rails-pro tests
size-limit report 📦
|
Code Review - PR #2155: Fix JS tests workflow in CIGreat work fixing the CI matrix configuration! I've reviewed the changes and have the following feedback: ✅ Strengths1. Correct Matrix Configuration FixThe primary fix is excellent - you correctly identified that matrix:
include:
- node-version: '22'
- node-version: '20'to: matrix:
node-version:
- '22'
- '20'This is the proper way to define a matrix axis, making the exclude work as intended. 2. Test Splitting for ClaritySplitting the single test command into two separate test runs is a good improvement: - name: Run JS unit tests for react-on-rails package
run: pnpm --filter react-on-rails test
- name: Run JS unit tests for react-on-rails-pro package
run: pnpm --filter react-on-rails-pro testThis provides better visibility in CI logs and makes it easier to identify which package failed. 3. Jest Config EnhancementsThe 🔍 Observations & Minor Concerns1. Dependency Reorganization RationaleThe changes to
Question: Is this related to fixing the JS tests, or is this a separate cleanup? The PR description focuses on the matrix fix but doesn't mention dependency restructuring. Consider:
2. Lockfile ChangesThe
3. Script/Convert ChangesThe gsub_file_content("../packages/react-on-rails-pro/package.json", /"react": "[^"]*",/, '"react": "18.0.0",')
gsub_file_content("../packages/react-on-rails-pro/package.json", /"react-dom": "[^"]*",/, '"react-dom": "18.0.0",')This makes sense given the dependency reorganization, but ensure:
🧪 Testing RecommendationsBased on CLAUDE.md guidelines, before merging: Required Local Testing:# 1. Clean install test (CRITICAL per CLAUDE.md)
rm -rf node_modules
pnpm install --frozen-lockfile
# 2. Build test
pnpm run build
# 3. Run JS tests locally
pnpm --filter react-on-rails test
pnpm --filter react-on-rails-pro test
# 4. Test conversion script (if Node 20 matrix is relevant)
script/convert
# Verify package.json changes are as expected
git diff packages/react-on-rails-pro/package.json
git restore . # Reset after verification
# 5. Lint check (MANDATORY)
bundle exec rubocop
pnpm run format.listDifferentCI Verification:✅ I see the CI is running now - wait for both Node 20 and Node 22 jobs to pass 📝 Documentation & ChangelogPer CLAUDE.md:
🎯 SummaryPrimary Fix: ✅ Excellent - the matrix configuration fix is correct and addresses the root issue Recommendation: ✅ Approve after CI passes, assuming:
Great debugging on the GitHub Actions matrix behavior! 🎉 |
Summary
The matrix in
.github/workflows/package-js-tests.ymldidn't work:excludeapplies to the matrix itself and not to combinations added by include. The result is https://github.com/shakacode/react_on_rails/actions/runs/19803605936on all workflow jobs since it was set up.
Pull Request checklist
CI-only change, none of the below are needed.
[ ] Add/update test to cover these changes[ ] Update documentation[ ] Update CHANGELOG fileSummary by CodeRabbit
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.