amirho/react
1
0
Fork 0
mirror of https://github.com/facebook/react.git synced 2026-02-26 02:54:59 +00:00
Code Issues Packages Projects Releases Wiki Activity

[DevTools] Fix false-positive re-render reports for filtered nodes (#35723)

Browse Source 
Fixes https://github.com/facebook/react/issues/33423,
https://github.com/facebook/react/issues/35245,
https://github.com/facebook/react/issues/19732.

As demoed
[here](https://github.com/facebook/react/issues/33423#issuecomment-2970750588),
React DevTools incorrectly highlights re-renders for descendants of
filtered-out nodes that didn't actually render.

There were multiple fixes suggesting changes in `didFiberRender()`
function, but these doesn't seem right, because this function is used in
a context of whether the Fiber actually rendered something (updated),
not re-rendered compared to the previous Fiber.

Instead, this PR adds additional validation at callsites that either
used for highlighting re-renders or capturing tree base durations and
are relying on `didFiberRender`. I've also added a few tests that
reproduce the failure scenario. Without the changes, the tests are
failing.
This commit is contained in:
Ruslan Lesiutin
2026-02-09 20:39:33 +00:00
committed by GitHub
parent eab523e2a9
commit 24f215ce8b
2 changed files with 189 additions and 18 deletions
Download Patch File Download Diff File Expand all files Collapse all files

160
packages/react-devtools-shared/src/__tests__/profilingCharts-test.js vendored
View File

@@ -298,4 +298,164 @@ describe('profiling charts', () => {
`);
});
});
describe('components behind filtered fibers should not report false re-renders', () => {
it('should not report a component as re-rendered when its filtered parent bailed out', () => {
let triggerUpdate;
function Count() {
const [count, setCount] = React.useState(0);
triggerUpdate = () => setCount(c => c + 1);
Scheduler.unstable_advanceTime(5);
return count;
}
function Greeting() {
Scheduler.unstable_advanceTime(3);
return 'Hello';
}
function App() {
Scheduler.unstable_advanceTime(1);
return (
<React.Fragment>
<Count />
<div>
<Greeting />
</div>
</React.Fragment>
);
}
utils.act(() => store.profilerStore.startProfiling());
utils.act(() => render(<App />));
// Verify tree structure: div is filtered, so Greeting appears as child of App
expect(store).toMatchInlineSnapshot(`
[root]
▾ <App>
<Count>
<Greeting>
`);
// Trigger a state update in Count. Should not cause Greeting to re-render.
utils.act(() => triggerUpdate());
utils.act(() => store.profilerStore.stopProfiling());
const rootID = store.roots[0];
const {chartData} = getFlamegraphChartData(rootID, 1);
const allNodes = chartData.rows.flat();
expect(allNodes).toEqual([
expect.objectContaining({name: 'App', didRender: false}),
expect.objectContaining({name: 'Greeting', didRender: false}),
expect.objectContaining({name: 'Count', didRender: true}),
]);
});
it('should not report a component as re-rendered when behind a filtered fragment', () => {
let triggerUpdate;
function Count() {
const [count, setCount] = React.useState(0);
triggerUpdate = () => setCount(c => c + 1);
Scheduler.unstable_advanceTime(5);
return count;
}
function Greeting() {
Scheduler.unstable_advanceTime(3);
return 'Hello';
}
function App() {
Scheduler.unstable_advanceTime(1);
return (
<React.Fragment>
<Count />
<React.Fragment>
<Greeting />
</React.Fragment>
</React.Fragment>
);
}
utils.act(() => store.profilerStore.startProfiling());
utils.act(() => render(<App />));
// Fragment with null key is filtered, so Greeting appears as child of App
expect(store).toMatchInlineSnapshot(`
[root]
▾ <App>
<Count>
<Greeting>
`);
// Trigger a state update in Count
utils.act(() => triggerUpdate());
utils.act(() => store.profilerStore.stopProfiling());
const rootID = store.roots[0];
const {chartData} = getFlamegraphChartData(rootID, 1);
const allNodes = chartData.rows.flat();
expect(allNodes).toEqual([
expect.objectContaining({name: 'App', didRender: false}),
expect.objectContaining({name: 'Greeting', didRender: false}),
expect.objectContaining({name: 'Count', didRender: true}),
]);
});
it('should correctly report sibling components that did not re-render', () => {
let triggerUpdate;
function Count() {
const [count, setCount] = React.useState(0);
triggerUpdate = () => setCount(c => c + 1);
Scheduler.unstable_advanceTime(5);
return count;
}
function Greeting() {
Scheduler.unstable_advanceTime(3);
return 'Hello';
}
function App() {
Scheduler.unstable_advanceTime(1);
return (
<React.Fragment>
<Count />
<Greeting />
</React.Fragment>
);
}
utils.act(() => store.profilerStore.startProfiling());
utils.act(() => render(<App />));
expect(store).toMatchInlineSnapshot(`
[root]
▾ <App>
<Count>
<Greeting>
`);
utils.act(() => triggerUpdate());
utils.act(() => store.profilerStore.stopProfiling());
const rootID = store.roots[0];
const {chartData} = getFlamegraphChartData(rootID, 1);
const allNodes = chartData.rows.flat();
expect(allNodes).toEqual([
expect.objectContaining({name: 'App', didRender: false}),
expect.objectContaining({name: 'Greeting', didRender: false}),
expect.objectContaining({name: 'Count', didRender: true}),
]);
});
});
});

47
packages/react-devtools-shared/src/backend/fiber/renderer.js vendored
View File

@@ -2088,6 +2088,10 @@ export function attach(
return changedKeys;
}
/**
* Returns true iff nextFiber actually performed any work and produced an update.
* For generic components, like Function or Class components, prevFiber is not considered.
*/
function didFiberRender(prevFiber: Fiber, nextFiber: Fiber): boolean {
switch (nextFiber.tag) {
case ClassComponent:
@@ -4520,7 +4524,10 @@ export function attach(
pushOperation(convertedTreeBaseDuration);
}
if (prevFiber == null || didFiberRender(prevFiber, fiber)) {
if (
prevFiber == null ||
(prevFiber !== fiber && didFiberRender(prevFiber, fiber))
) {
if (actualDuration != null) {
// The actual duration reported by React includes time spent working on children.
// This is useful information, but it's also useful to be able to exclude child durations.
@@ -5150,11 +5157,13 @@ export function attach(
elementType === ElementTypeMemo ||
elementType === ElementTypeForwardRef
) {
// Otherwise if this is a traced ancestor, flag for the nearest host descendant(s).
traceNearestHostComponentUpdate = didFiberRender(
prevFiber,
nextFiber,
);
if (prevFiber !== nextFiber) {
// Otherwise if this is a traced ancestor, flag for the nearest host descendant(s).
traceNearestHostComponentUpdate = didFiberRender(
prevFiber,
nextFiber,
);
}
}
}
}
@@ -5174,18 +5183,20 @@ export function attach(
previousSuspendedBy = fiberInstance.suspendedBy;
// Update the Fiber so we that we always keep the current Fiber on the data.
fiberInstance.data = nextFiber;
if (
mostRecentlyInspectedElement !== null &&
(mostRecentlyInspectedElement.id === fiberInstance.id ||
// If we're inspecting a Root, we inspect the Screen.
// Invalidating any Root invalidates the Screen too.
(mostRecentlyInspectedElement.type === ElementTypeRoot &&
nextFiber.tag === HostRoot)) &&
didFiberRender(prevFiber, nextFiber)
) {
// If this Fiber has updated, clear cached inspected data.
// If it is inspected again, it may need to be re-run to obtain updated hooks values.
hasElementUpdatedSinceLastInspected = true;
if (prevFiber !== nextFiber) {
if (
mostRecentlyInspectedElement !== null &&
(mostRecentlyInspectedElement.id === fiberInstance.id ||
// If we're inspecting a Root, we inspect the Screen.
// Invalidating any Root invalidates the Screen too.
(mostRecentlyInspectedElement.type === ElementTypeRoot &&
nextFiber.tag === HostRoot)) &&
didFiberRender(prevFiber, nextFiber)
) {
// If this Fiber has updated, clear cached inspected data.
// If it is inspected again, it may need to be re-run to obtain updated hooks values.
hasElementUpdatedSinceLastInspected = true;
}
}
// Push a new DevTools instance parent while reconciling this subtree.
reconcilingParent = fiberInstance;