Skip to content

[compiler][e2e] Add rule of react violation fixture#60

Open
everettbu wants to merge 2 commits into
gh/mofeiZ/25/basefrom
gh/mofeiZ/25/head
Open

[compiler][e2e] Add rule of react violation fixture#60
everettbu wants to merge 2 commits into
gh/mofeiZ/25/basefrom
gh/mofeiZ/25/head

Conversation

@everettbu

@everettbu everettbu commented Dec 12, 2025

Copy link
Copy Markdown

Mirror of facebook/react#31114
Original author: mofeiZ


Stack from ghstack (oldest at bottom):

  • -> #31114
  • #31103

mofeiZ added 2 commits October 3, 2024 11:06
[ghstack-poisoned]
[ghstack-poisoned]
@everettbu everettbu added CLA Signed React Core Team Opened by a member of the React Core Team labels Dec 12, 2025
@greptile-apps

greptile-apps Bot commented Dec 12, 2025

Copy link
Copy Markdown

Greptile Overview

Greptile Summary

Added new e2e test fixture inner-mutation-state.e2e.tsx to demonstrate Rule of React violations in the React Compiler

  • Test demonstrates a reactivity bug caused by shallow copying state and mutating nested objects
  • The test expects different behavior when __FORGET__ is enabled vs disabled
  • Found critical bugs in implementation: function signature mismatch (missing newValue parameter), incorrect conditional logic (both branches set errorMessage instead of value), and stale closure capturing count value

Confidence Score: 2/5

  • This PR contains multiple critical bugs that will cause runtime errors and test failures
  • Score reflects three critical issues: (1) function signature mismatch where type declares newValue: string parameter but implementation omits it, causing TypeScript errors, (2) logic bug where both conditional branches incorrectly set data.errorMessage instead of data.value, and (3) stale closure bug with count variable that won't reflect current state
  • The single file inner-mutation-state.e2e.tsx requires immediate attention to fix critical bugs before merge

Important Files Changed

File Analysis

Filename Score Overview
compiler/packages/babel-plugin-react-compiler/src/tests/e2e/rule-of-react-violations/inner-mutation-state.e2e.tsx 2/5 New e2e test fixture for Rule of React violations with critical bugs: function signature mismatch, incorrect conditional logic setting wrong property, and stale closure issue with state

@greptile-apps greptile-apps Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

1 file reviewed, 3 comments

Edit Code Review Agent Settings | Greptile

Comment on lines +48 to +51
const update = (
field: 'username' | 'address',
kind: 'errorMessage' | 'value',
) => {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

syntax: function signature mismatch - the type definition on line 30 declares a third parameter newValue: string, but the implementation doesn't accept it. Either remove newValue from the type or add it to the function parameters.

Suggested change
const update = (
field: 'username' | 'address',
kind: 'errorMessage' | 'value',
) => {
const update = (
field: 'username' | 'address',
kind: 'errorMessage' | 'value',
newValue: string,
) => {
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/__tests__/e2e/rule-of-react-violations/inner-mutation-state.e2e.tsx
Line: 48:51

Comment:
**syntax:** function signature mismatch - the type definition on line 30 declares a third parameter `newValue: string`, but the implementation doesn't accept it. Either remove `newValue` from the type or add it to the function parameters.

```suggestion
  const update = (
    field: 'username' | 'address',
    kind: 'errorMessage' | 'value',
    newValue: string,
  ) => {
```

How can I resolve this? If you propose a fix, please make it concise.

Comment on lines +58 to +62
if (kind === 'errorMessage') {
data.errorMessage = `value${count}`;
} else {
data.errorMessage = `value${count}`;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

logic: both branches set data.errorMessage - when kind === 'value', should set data.value instead

Suggested change
if (kind === 'errorMessage') {
data.errorMessage = `value${count}`;
} else {
data.errorMessage = `value${count}`;
}
if (kind === 'errorMessage') {
data.errorMessage = `value${count}`;
} else {
data.value = `value${count}`;
}
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/__tests__/e2e/rule-of-react-violations/inner-mutation-state.e2e.tsx
Line: 58:62

Comment:
**logic:** both branches set `data.errorMessage` - when `kind === 'value'`, should set `data.value` instead

```suggestion
      if (kind === 'errorMessage') {
        data.errorMessage = `value${count}`;
      } else {
        data.value = `value${count}`;
      }
```

How can I resolve this? If you propose a fix, please make it concise.

}
return newState;
});
setCount(count + 1);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

logic: stale closure issue - count value used on line 59/61 will always be the value from when update was created, not the current state. Should use the functional form: setCount(prevCount => prevCount + 1) and access count from the updater, or use count from within the setState callback

Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/__tests__/e2e/rule-of-react-violations/inner-mutation-state.e2e.tsx
Line: 65:65

Comment:
**logic:** stale closure issue - `count` value used on line 59/61 will always be the value from when `update` was created, not the current state. Should use the functional form: `setCount(prevCount => prevCount + 1)` and access count from the updater, or use `count` from within the setState callback

How can I resolve this? If you propose a fix, please make it concise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed React Core Team Opened by a member of the React Core Team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants