Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Aug 14, 2025

This PR addresses review comments from PR #12988 by extracting the duplicated is_jsx_fragment function that was present in both jsx_fragments.rs and jsx_no_useless_fragment.rs rules.

Changes Made

Code Deduplication

  • Extracted is_jsx_fragment function from both React linting rules
  • Moved the function to crates/oxc_linter/src/utils/react.rs as a shared utility
  • Updated both jsx_fragments and jsx_no_useless_fragment rules to use the shared function
  • Removed duplicate implementations and cleaned up unused imports

Self-Closing Fragment Handling

The review also mentioned handling self-closing fragments (e.g., <Fragment />). Upon investigation, this was already correctly implemented in the original code:

let Some(closing_element) = &jsx_elem.closing_element else {
    return; // Skip self-closing elements
};

Self-closing fragments cannot be converted to shorthand syntax (<></>) and should not trigger the rule, which is the correct behavior already in place.

Function Details

The extracted is_jsx_fragment function recognizes both forms of React fragments:

  • <Fragment> (identifier reference)
  • <React.Fragment> (member expression)
pub fn is_jsx_fragment(elem: &JSXOpeningElement) -> bool {
    match &elem.name {
        JSXElementName::IdentifierReference(ident) => ident.name == "Fragment",
        JSXElementName::MemberExpression(mem_expr) => {
            if let JSXMemberExpressionObject::IdentifierReference(ident) = &mem_expr.object {
                ident.name == "React" && mem_expr.property.name == "Fragment"
            } else {
                false
            }
        }
        _ => false,
    }
}

Testing

  • ✅ All existing tests continue to pass (755/755)
  • ✅ Specific jsx-related tests pass (49/49)
  • ✅ Both jsx_fragments and jsx_no_useless_fragment rules maintain their behavior
  • ✅ Self-closing fragment test cases verify correct behavior (included in existing test suite)

This refactoring eliminates code duplication while maintaining full backward compatibility and correctness of both linting rules.


💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.

@graphite-app
Copy link
Contributor

graphite-app bot commented Aug 14, 2025

How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

  • 0-merge - adds this PR to the back of the merge queue
  • hotfix - for urgent hot fixes, skip the queue and merge this PR next

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

Copilot AI changed the title [WIP] @oxc-project/oxc/pull/12988 review the review comments in this PR, and action them refactor(linter): extract duplicated is_jsx_fragment function to shared utils Aug 14, 2025
Copilot AI requested a review from camc314 August 14, 2025 16:19
Copilot finished work on behalf of camc314 August 14, 2025 16:19
@github-actions github-actions bot added A-linter Area - Linter C-cleanup Category - technical debt or refactoring. Solution not expected to change behavior labels Aug 14, 2025
@camc314 camc314 marked this pull request as ready for review August 14, 2025 16:22
Copilot AI review requested due to automatic review settings August 14, 2025 16:22
Copilot AI and others added 2 commits August 14, 2025 17:22
- Extract duplicated `is_jsx_fragment` function from jsx_fragments and jsx_no_useless_fragment
- Move function to utils/react.rs to eliminate code duplication
- Update both rules to use shared utility function
- Clean up unused imports in both rule files
- All tests continue to pass

Co-authored-by: camc314 <18101008+camc314@users.noreply.github.com>
- Verified self-closing fragment handling is correct (already implemented)
- Extracted duplicated is_jsx_fragment function to shared utils
- All jsx-related tests passing (49/49)
- Code properly formatted
- Review comments from PR #12988 addressed

Co-authored-by: camc314 <18101008+camc314@users.noreply.github.com>
@camc314 camc314 force-pushed the copilot/fix-9d8549e3-f488-457f-b361-de903413e428 branch from d84177e to 039f986 Compare August 14, 2025 16:22
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR extracts a duplicated is_jsx_fragment function from two React linting rules to eliminate code duplication. The function identifies React fragments in both <Fragment> and <React.Fragment> forms.

  • Extracted the shared is_jsx_fragment function to utils/react.rs
  • Updated both jsx_fragments and jsx_no_useless_fragment rules to use the shared utility
  • Removed duplicate implementations and cleaned up unused imports

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
crates/oxc_linter/src/utils/react.rs Added the shared is_jsx_fragment function with documentation
crates/oxc_linter/src/rules/react/jsx_no_useless_fragment.rs Removed duplicate function and updated calls to use shared utility
crates/oxc_linter/src/rules/react/jsx_fragments.rs Removed duplicate function, cleaned up imports, and updated calls to use shared utility

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.

@codspeed-hq
Copy link

codspeed-hq bot commented Aug 14, 2025

CodSpeed Instrumentation Performance Report

Merging #13093 will not alter performance

Comparing copilot/fix-9d8549e3-f488-457f-b361-de903413e428 (3ffe0be) with main (0b61338)

Summary

✅ 34 untouched benchmarks

Copilot AI requested a review from camc314 August 14, 2025 16:31
Copilot finished work on behalf of camc314 August 14, 2025 16:31
Copilot finished work on behalf of camc314 August 14, 2025 16:36
@camc314
Copy link
Contributor

camc314 commented Aug 14, 2025

@copilot update the code such that is_jsx_fragment is imported directly, and isn't used like utils:: is_jsx_fragment at it's call sites

…s_jsx_fragment

Co-authored-by: camc314 <18101008+camc314@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Aug 14, 2025

@copilot update the code such that is_jsx_fragment is imported directly, and isn't used like utils:: is_jsx_fragment at it's call sites

Updated the code to import is_jsx_fragment directly instead of using utils::is_jsx_fragment at call sites. Changed imports to utils::is_jsx_fragment and updated all function calls accordingly. (3ffe0be)

Copilot finished work on behalf of camc314 August 14, 2025 16:51
@camc314 camc314 merged commit 1c15288 into main Aug 14, 2025
25 checks passed
@camc314 camc314 deleted the copilot/fix-9d8549e3-f488-457f-b361-de903413e428 branch August 14, 2025 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-linter Area - Linter C-cleanup Category - technical debt or refactoring. Solution not expected to change behavior

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants