Fix JS tests workflow in CI

[alexeyr-ci2]

Summary

The matrix in .github/workflows/package-js-tests.yml didn't work: exclude applies to the matrix itself and not to combinations added by include. The result is https://github.com/shakacode/react_on_rails/actions/runs/19803605936

on 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 file

Summary by CodeRabbit

• Chores
  • Updated CI/CD workflow configuration. No user-visible changes.

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

[coderabbitai]

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

The GitHub Actions workflow matrix syntax is refactored from an include-list format to a direct axis-based approach for Node.js versions. The versions ('22' and '20') remain unchanged; only the matrix declaration structure is simplified.

Changes

Cohort / File(s)	Summary
Workflow Matrix Refactoring \.github/workflows/package-js-tests\.yml	Restructures matrix specification from strategy.matrix.include list to direct strategy.matrix.node-version axis declaration. Node.js versions ('22', '20') preserved; exclusion logic unchanged.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

• Straightforward configuration syntax migration with no functional changes
• Single file modification with negligible risk

Possibly related PRs

• HOTFIX: Add CI infrastructure pattern to ci-changes-detector #2080 — Adds .github/workflows/* to CI changes detector to recognize workflow file modifications as infrastructure changes.

Suggested labels

bug, review-needed

Suggested reviewers

• Judahmeek
• AbanoubGhadban

Poem

🐰 A matrix once tangled, now crystalline clear,
Axes aligned, the workflow runs dear,
From includes to directives, we streamline the way,
Simpler workflows for a brighter CI day! ✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'Fix JS tests workflow in CI' directly reflects the main change: fixing the matrix configuration in the CI workflow file to resolve incorrect job combinations.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[claude]

Code Review ✅

Summary

This PR 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 Wrong

The original configuration used:

matrix:
  include:
    - node-version: '22'
    - node-version: '20'
  exclude:
    - node-version: ${{ ... && '20' || '' }}

The problem: In GitHub Actions, exclude only applies to combinations generated from the base matrix, not from items added via include. This meant the exclusion logic never worked, causing Node 20 tests to run on every PR regardless of the conditional logic.

The Fix

The corrected configuration uses:

matrix:
  node-version:
    - '22'
    - '20'
  exclude:
    - node-version: ${{ ... && '20' || '' }}

Now the matrix is defined properly with a base dimension (node-version), and exclude correctly filters out Node 20 on PRs without the full-ci label.

Review Findings

✅ Correct Fix: The change properly addresses the root cause by moving from include-based matrix to a proper base matrix.

✅ Consistent Logic: The exclude condition remains unchanged and will now work as intended:

• On PRs: Run only Node 22 (latest) for fast feedback
• On master/with full-ci label: Run both Node 22 and Node 20 (full coverage)

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

Recommendations

None required - this is a straightforward bug fix with no additional changes needed.

Additional Notes

I verified that:

• This is the only workflow using exclude in the codebase
• The integration-tests.yml workflow uses a different approach (dynamic matrix via setup-integration-matrix job) which works correctly
• The fix aligns with GitHub Actions documentation on matrix strategies

Great catch on identifying this issue! 🎯

---

Review conducted by Claude Code

[claude]

Code Review

Summary

This 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 Findings

1. Workflow Matrix Fix - Correct Solution

The change from include to explicit node-version array is the correct fix. GitHub Actions exclude only works on base matrix dimensions, not on entries added via include. This explains why both Node 20 and 22 were running on all PRs.

2. Jest Configuration Enhancement

The Jest config changes properly support .cts files found in the codebase (reactApis.cts, ReactDOMServer.cts). Good practices: DRY principle with extracted tsconfig, proper preset extension, clear comments, and future-proofing with .mts support.

---

Areas for Consideration

1. 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 pnpm test locally to confirm.

2. CI Verification

Current CI shows build (22): FAILURE. This suggests the matrix fix is working (only Node 22 running), but there's a test failure. Please investigate whether it's related to the Jest config changes.

3. Module Kind Configuration

The module: 'ESNext' addition - is this necessary for .cts support or fixing a separate ts-jest warning? Consider documenting this in the PR description.

---

Suggestions

1. 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 correctly

3. Consider Cleaner Matrix Logic

The empty string exclusion (node-version: '') works but is subtle. Consider using a setup-matrix job like other workflows (.github/workflows/examples.yml lines 67-85).

---

Before Merging

• Run bundle exec rubocop
• Verify CI passes with the matrix change
• Confirm Node 20 is skipped on regular PRs
• Confirm both Node 20 and 22 run on master or with full-ci label
• Investigate and resolve the Node 22 build failure

---

Risk Assessment: Low-Medium

Changes are isolated and additive. The fix addresses a real bug. Main concern is the current CI failure needs resolution.

---

Great catch on the include vs array issue - that's a subtle GitHub Actions behavior! The code quality is good with proper structure and comments. Main action item: resolve the CI failure and verify the matrix exclusion works correctly.

[claude]

Code Review for PR #2155

Summary

This PR fixes two issues:

1. GitHub Actions matrix configuration bug - The workflow was incorrectly using include instead of direct matrix values, causing exclude not to work
2. Jest configuration enhancement - Adds support for .cts and .mts TypeScript file extensions

---

✅ Strengths

1. Correct Fix for GitHub Actions Matrix

The change from include: to direct node-version: values is correct and aligns with GitHub Actions matrix syntax:

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, exclude only works on matrix dimensions, not on entries added via include. This was causing the conditional exclusion of Node 20 on PRs to fail silently.

2. Well-Documented Jest Changes

The Jest configuration changes are well-commented and explain:

• Why rewriteRelativeImportExtensions is disabled
• Why module type is overridden to ESNext
• What the .cts/.mts file extensions are for

3. Future-Proofing

Adding .cts and .mts support makes sense given that:

• The codebase already has 2 .cts files (packages/react-on-rails/src/reactApis.cts, ReactDOMServer.cts)
• These file extensions are standard TypeScript for CommonJS/ES Module disambiguation

---

🔍 Observations & Questions

1. Jest Transform Pattern

The regex pattern '^.+\\.[cm]?ts$' will match:

• .ts files
• .cts files
• .mts files

Question: Does this override the default ts-jest transform that was already handling .ts files? If so, is there any difference in behavior?

Looking at the code, it appears you're spreading ...tsJestPreset.transform first, then overriding with the new pattern. This should work correctly, but it's worth verifying that:

• Regular .ts files still work as expected
• The tsconfig options are correctly passed to both transforms

2. Testing Coverage

Concern: I don't see test files for the .cts files yet.

While adding Jest support is good, consider:

• Are there (or will there be) test files for reactApis.cts and ReactDOMServer.cts?
• If not, the Jest config changes are future-proofing but not immediately exercised

3. moduleFileExtensions Order

The new order is: ['js', 'jsx', 'ts', 'tsx', 'cts', 'mts', 'json']

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.

---

🎯 Recommendations

1. Consider Testing the CI Fix

Low 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):

• Only Node 22 build runs
• Node 20 build is skipped

On master or PRs with full-ci label:

• Both Node 20 and 22 builds run

2. Document the CI Matrix Strategy

Optional: 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 Locally

Before merge, run:

pnpm test

To ensure the Jest config changes don't break existing tests.

---

📊 Code Quality Assessment

Category	Rating	Notes
Correctness	✅ Excellent	Fixes a real bug with proper understanding of GitHub Actions
Documentation	✅ Excellent	Clear comments explaining the changes
Testing	⚠️ Adequate	CI will validate, but hard to unit test workflow changes
Impact	🎯 High	Fixes broken CI matrix that was running unnecessary jobs
Risk	✅ Low	Changes are isolated and well-understood

---

✅ Final Verdict

LGTM (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

• Verify pnpm test passes locally
• Confirm actionlint CI check passes (already passing ✅)
• After merge: Monitor next PR to confirm Node 20 is correctly excluded on regular PRs

Great work identifying and fixing the matrix configuration bug! 🎉

[claude]

Code Review

Summary

This 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 Observations

1. Correct Fix for GitHub Actions Matrix Syntax

The workflow fix is spot-on. The original code misunderstood how GitHub Actions matrix syntax works:

• ❌ Before: Used include to define matrix values, then tried to exclude them (which doesn't work - exclude only applies to base matrix combinations, not include entries)
• ✅ After: Properly defines node-version as a matrix dimension with values ['22', '20'], then uses exclude to conditionally remove the '20' entry on PRs

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 Files

The Jest configuration changes are valuable:

• Good refactoring: Extracting tsconfig to a variable improves readability
• Necessary addition: The codebase has .cts files (reactApis.cts, ReactDOMServer.cts) that need proper Jest transformation
• Comprehensive: Added [cm]ts to both transform and moduleFileExtensions
• Well-documented: Added clear inline comments explaining the purpose

The module: 'ESNext' override is also correct - it avoids the ts-jest warning about isolatedModules when using Node16/NodeNext module resolution.

---

🔍 Observations & Questions

1. Testing Recommendation

While 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 them

2. Documentation Note

The workflow fix addresses a silent failure that's been happening since the workflow was created. Consider:

• Adding this pattern to your CI workflow testing guide
• The fix ensures Node 20 only runs on master or with full-ci label (as intended)

3. Jest Config: Minor Style Note

The Jest config changes are solid, but one small observation:

• The spread of tsJestPreset.transform followed by override is correct
• However, if ts-jest's default transform already handles .ts files, the explicit '^.+\\.[cm]?ts$' pattern will override it
• This appears intentional (to add the custom tsconfig to ALL TypeScript variants), which is correct

---

🎯 Final Assessment

Code Quality: ⭐⭐⭐⭐⭐

• Clean, focused changes
• Proper understanding of GitHub Actions matrix mechanics
• Good code organization and documentation

Bug Fixes: ⭐⭐⭐⭐⭐

• Fixes a real, verified workflow issue
• Addresses missing Jest configuration for .cts/.mts files

Best Practices: ⭐⭐⭐⭐⭐

• Follows repository conventions (per CLAUDE.md)
• CI-only change appropriately skips changelog/tests
• Clear commit message and PR description

Security: ✅ No concerns

• No security implications

Performance: ✅ Neutral/Positive

• Workflow fix may actually reduce CI usage by properly skipping Node 20 on PRs
• Jest config has negligible performance impact

---

✅ Recommendation: APPROVE & MERGE

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

1. Monitor the next PR to ensure Node 20 jobs are properly skipped (shouldn't appear in the job list)
2. On master branch, verify both Node 20 and 22 run
3. With full-ci label, verify both Node versions run

---

Review conducted following guidelines in CLAUDE.md and .claude/docs/

[claude]

PR Review: Fix JS tests workflow in CI

Thank you for this PR! I've reviewed the changes and have the following feedback:

---

✅ Positive Findings

1. Correct GitHub Actions Matrix Fix (.github/workflows/package-js-tests.yml)

• Good: The change from include to plain array format correctly fixes the matrix exclusion logic
• Correct: As noted in the PR description, exclude only applies to the matrix itself, not to combinations added via include
• Impact: This ensures that Node 20 is properly skipped on regular PRs (only running Node 22), while both versions run on master or with the full-ci label

2. Comprehensive Jest Config Update (jest.config.base.js)

• Good: Added support for .cts and .mts file extensions, which is forward-looking for CommonJS/ES Module TypeScript files
• Good: Set module: 'ESNext' to avoid ts-jest warnings about isolatedModules
• Good: Properly extended the transform configuration to handle the new file types
• Documentation: Excellent inline comments explaining the rewriteRelativeImportExtensions: false rationale

---

⚠️ Issues & Concerns

1. CI Failure on build (22)

The PR currently has a failing check: build (22): FAILURE. This needs to be investigated and resolved before merging.

Action required:

• Check the failure at: https://github.com/shakacode/react_on_rails/actions/runs/19836088294/job/56833622832
• Verify that the Jest config changes don't introduce test failures
• Ensure local tests pass: pnpm test

2. No Test Evidence

Per CLAUDE.md guidelines:

CRITICAL - LOCAL TESTING REQUIREMENTS:

1. NEVER claim a test is "fixed" without running it locally first

Recommendation: Add a comment showing:

• Local test results confirming Jest tests pass with the new config
• Confirmation that the workflow now correctly skips Node 20 on PRs (can be simulated or shown via actual CI runs)

---

📋 Suggestions for Improvement

1. Consider Testing the Matrix Logic

While the matrix fix looks correct, it would be helpful to verify:

• Does a regular PR now only run Node 22? (Expected: yes)
• Does master or full-ci label run both Node 20 and 22? (Expected: yes)

2. Jest Config: Minor Refinement

The tsconfig object is extracted well, but consider adding a comment about why it's shared:

// Shared tsconfig for both createJsWithTsPreset and explicit transform
const tsconfig = {
  // [existing comments]
};

3. Module File Extensions Order

Consider listing extensions in priority/frequency order:

moduleFileExtensions: ['ts', 'tsx', 'js', 'jsx', 'cts', 'mts', 'json'],

(TS files before JS, since this is primarily a TS project)

---

🔒 Security & Performance

• Security: No concerns. CI configuration changes don't introduce security risks.
• Performance: The matrix fix will improve performance by reducing unnecessary Node 20 runs on PRs.

---

✅ Pre-Merge Checklist

Per CLAUDE.md, before merging:

• Fix the build (22) CI failure
• Run bundle exec rubocop (if any Ruby files touched) - N/A for this PR
• Run pnpm run format.listDifferent to verify formatting
• Confirm all CI checks pass
• Verify the matrix logic works as expected (Node 20 only on master/full-ci)

---

📝 Changelog

This is a CI/build tooling change. Per CLAUDE.md:

Do NOT add entries for: linting, formatting, refactoring, tests, or documentation fixes

Recommendation: No changelog entry needed (correctly noted in PR description).

---

Overall Assessment

The changes are well-structured and address a real CI configuration bug. The Jest config improvements are forward-looking and well-documented. However, the failing CI check must be resolved before merging.

Status: ⚠️ Needs work (resolve CI failure)
Quality: ✅ Good (once CI passes)

[claude]

Comprehensive Code Review - PR #2155

Summary

This PR fixes two issues:

1. GitHub Actions workflow matrix bug - Corrects the matrix configuration so that exclude works properly
2. Jest configuration enhancement - Adds support for .cts and .mts TypeScript file extensions

---

Strengths

1. Correct Fix for GitHub Actions Matrix Syntax

The 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 Configuration

The Jest config changes demonstrate good engineering practices:

• DRY principle: Extracted tsconfig to avoid duplication
• Proper extension: Extends tsJestPreset correctly before overriding
• Clear documentation: Inline comments explain the settings
• Future-proofing: Adds .cts and .mts support (needed for existing files like reactApis.cts and ReactDOMServer.cts)

The module: ESNext override is necessary to avoid ts-jest warnings about isolatedModules.

---

Issues That Need Resolution

CRITICAL: Failing CI Check

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

1. Run pnpm test locally to reproduce
2. Run pnpm run build to check if Jest config changes broke anything
3. Document your local testing results

---

Recommendations

1. Verify Matrix Logic Works - After merge, monitor CI to confirm proper behavior
2. Consider Adding Documentation - Add a comment explaining the matrix syntax to prevent future confusion
3. Jest Config - Consider reordering moduleFileExtensions by frequency (minor optimization, not required)

---

Assessment

Code 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: ⚠️ NEEDS WORK - The build (22) CI check must pass before merge

---

Pre-Merge Checklist

• build (22) CI check passes
• Local tests pass: pnpm test
• Formatting check passes: pnpm run format.listDifferent
• Document what you tested locally and the results

---

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

[claude]

Code Review - PR #2155

Summary

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

---

Strengths

1. Correct GitHub Actions Matrix Fix
The change from include to direct matrix axis is exactly right:

• Problem identified correctly: exclude does not work with include - it only applies to Cartesian products of matrix axes
• Solution is optimal: Using node-version array creates a proper matrix axis that exclude can filter
• No behavioral change: Logic remains identical, just properly structured

2. Proper Jest Configuration for .cts Files

• Necessary for TypeScript CommonJS modules: The project uses .cts files (reactApis.cts, ReactDOMServer.cts)
• Good code organization: Extracted tsconfig to avoid duplication
• Comprehensive coverage: Added .cts and .mts to both transform and moduleFileExtensions

---

Issues Requiring Attention

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

1. Reproduce the failure locally with clean install and pnpm install --frozen-lockfile
2. Verify the Jest config changes work with pnpm test -- --verbose
3. Address any Node 22 specific issues

From CLAUDE.md: Do not push hopeful fixes and wait for CI feedback. Test locally first whenever technically possible.

---

Additional Recommendations

1. Document why .cts/.mts support was added - explain these are required for reactApis.cts and ReactDOMServer.cts

2. Clarify the TypeScript config override explanation for module: ESNext

3. Consider adding a test to verify Jest can actually import and test .cts files

---

Security and Performance

• No security concerns
• No performance impact (config changes only)
• No new dependencies added

---

Adherence to CLAUDE.md Guidelines

Good:

• CI-only change, correctly noted no CHANGELOG needed
• Workflow matrix fix follows GitHub Actions best practices

Needs attention:

• CRITICAL: Must investigate and fix the build (22) CI failure before merge
• LOCAL TESTING: Per CLAUDE.md, must verify locally that Jest config changes work

---

Verdict

Do NOT merge until:

1. build (22) CI job passes
2. Local testing confirms Jest config works with .cts files
3. All CI checks are green

The code quality is good, but the CI failure must be resolved per project guidelines.

---

Suggested Next Steps

1. Investigate the build (22) failure logs
2. Run full local test suite on Node 22
3. If the Jest config is causing issues, consider whether the transform pattern needs adjustment
4. Re-run CI and confirm all checks pass

Overall Assessment: Strong fixes for real issues, but blocked on CI failure resolution.

[claude]

Code Review for PR #2155

Summary

This PR fixes two distinct issues:

1. Critical GitHub Actions matrix bug - The workflow wasn't working as intended since setup
2. Jest configuration enhancement - Adds support for .cts and .mts TypeScript file extensions

---

✅ Positive Findings

1. GitHub Actions Matrix Fix - Correct and Well-Understood

The fix correctly addresses a GitHub Actions matrix configuration bug:

Problem Identified:

• The workflow used include: to define matrix values, then exclude: to conditionally remove them
• This doesn't work: exclude only applies to base matrix dimensions, NOT to values added via include
• Result: All workflows ran both Node 20 and 22 regardless of conditions

Solution:

• Changed from include: to direct matrix dimension: node-version:
• Now exclude can properly filter the base matrix
• This follows GitHub Actions best practices

2. Jest Configuration Enhancement - Needed and Well-Implemented

The Jest config changes add proper support for .cts and .mts files:

Why This Matters:

• The codebase already has .cts files (ReactDOMServer.cts, reactApis.cts)
• Without this config, Jest can't properly test these files
• The solution correctly extracts tsconfig for reuse, extends the transform pattern, and adds cts/mts to moduleFileExtensions

Code Quality:

• Good refactoring: extracted tsconfig to avoid duplication
• Clear comments explaining the changes
• Consistent with existing patterns

---

🔍 Suggestions & Considerations

1. Test the GitHub Actions Matrix Fix

Important: This fix changes CI behavior. Please verify the matrix now correctly excludes Node 20 on regular PRs.

2. Consider Adding Tests for .cts/.mts Files

Since 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. Documentation

The 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 & Performance

Security: ✅ No security concerns - configuration changes only

Performance: ✅ No performance impact expected

• Jest config changes are additive
• Matrix fix actually improves CI performance (fewer unnecessary runs on PRs)

---

Final Verdict

Recommendation: ✅ 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! 🎉

[claude]

Code Review - PR #2155: Fix JS tests workflow in CI

Summary

This PR fixes two distinct issues:

1. GitHub Actions matrix configuration bug - The exclude syntax wasn't working correctly
2. Jest configuration enhancement - Adding support for .cts/.mts file extensions

---

✅ Code Quality & Best Practices

GitHub Actions Fix (.github/workflows/package-js-tests.yml)

• ✅ Correct fix: Changed from include to direct node-version array syntax
• ✅ Well-documented issue: The PR description clearly explains that exclude doesn't work with include
• ✅ Behavior preserved: The conditional exclusion logic for Node 20 on PRs remains intact

Jest Configuration Enhancement (jest.config.base.js)

• ✅ Clean refactoring: Extracted tsconfig and tsJestPreset as named constants for reuse
• ✅ Good documentation: Comments explain why rewriteRelativeImportExtensions: false and module: 'ESNext' are needed
• ✅ Proper extension: Extends the transform config rather than replacing it
• ⚠️ Minor: The new module: 'ESNext' override may not be strictly necessary based on the code context

---

🐛 Potential Issues

1. Jest Configuration Scope Concern
The .cts/.mts support addition seems broader than necessary:

• Only 2 .cts files exist in the codebase (reactApis.cts, ReactDOMServer.cts)
• No .mts files exist
• No test files use .cts or .mts extensions
• These files compile to .cjs/.mjs and are consumed as CommonJS/ESM

Question: Are you planning to add tests that directly import .cts/.mts files, or is this preemptive for future use? If the latter, consider deferring until actually needed (YAGNI principle from CLAUDE.md: "Don't design for hypothetical future requirements").

2. Module Resolution Override
The new module: 'ESNext' override is added to silence a ts-jest warning:

// Override hybrid module kind (Node16/NodeNext) to avoid ts-jest warning
// about requiring isolatedModules: true
module: 'ESNext',

However, the base tsconfig.json extends @tsconfig/node14 which likely doesn't use Node16/NodeNext module resolution. Can you verify this warning actually exists in CI? If it's hypothetical, consider removing this override.

---

🚀 Performance Considerations

No performance concerns. The changes are configuration-only.

---

🔒 Security Concerns

No security issues identified.

---

🧪 Test Coverage

Critical Gap: The PR description states it's a "CI-only change" but there are actually two separate changes:

1. CI matrix fix - Can only be tested via GitHub Actions ✅
2. Jest config enhancement - Should have verification ⚠️

Recommendations:

1. Verify the jest changes locally:

   # Test that jest can still run tests
   pnpm test
   
   # Verify transform patterns match expected files
   pnpm exec jest --showConfig | grep -A10 transform

2. Confirm the ts-jest warning exists (if you haven't already):

   • Run tests with the old config
   • Capture the warning about isolatedModules
   • Verify the new config silences it

3. Document why .cts/.mts support is needed - Add a comment explaining the use case, or consider deferring this change if it's not immediately needed.

---

📋 Checklist Items

From CLAUDE.md requirements:

• ✅ No CHANGELOG needed (CI-only change)
• ✅ No tests needed for GH Actions matrix (tested via CI)
• ⚠️ Jest changes could use local verification
• 🔄 Consider: Should this be split into two PRs? (GH Actions fix vs Jest enhancement)

---

💡 Suggestions

Option 1: Keep as-is but verify

• Run pnpm test locally to confirm jest config works
• Confirm the ts-jest warning exists and is fixed
• Document in PR description why .cts/.mts support was added

Option 2: Split the PR (recommended)

• PR 1: Just the GH Actions matrix fix (urgent, clearly needed)
• PR 2: Jest config enhancements (can be reviewed more thoroughly with context)

Option 3: Minimal version

• Keep only the GH Actions fix
• Defer jest changes until there's a clear need (e.g., adding tests that import .cts files)

---

🎯 Verdict

The GitHub Actions fix is good and should be merged - it fixes a real bug.

The Jest configuration changes need clarification:

• Are they fixing an actual warning you're seeing?
• Is .cts/.mts support needed now or is it preemptive?
• Consider separating concerns for clearer change history.

Great catch on the matrix configuration bug! The linked screenshot clearly shows the issue.