[DevTools] Fix false-positive re-render reports for filtered nodes

[everettbu]

Mirror of facebook/react#35723
Original author: hoxyq

---

Fixes react/react#33423, react/react#35245, react/react#19732.

As demoed here, 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.

[greptile-apps]

Greptile Overview

Greptile Summary

This PR adjusts React DevTools’ backend Fiber renderer to avoid false-positive “did render”/update signals for components that are descendants of filtered-out Fibers (e.g. host nodes or unkeyed fragments) when the filtered parent bailed out. It does this by adding identity guards (prevFiber !== nextFiber) at callsites that rely on didFiberRender, so reused Fiber objects aren’t treated as having rendered.

It also adds profiling flamegraph regression tests that reproduce the filtered-parent bailout scenario and assert that only the updating component is marked didRender: true for the relevant commit.

Confidence Score: 4/5

• This PR is likely safe to merge, with minor test robustness concerns.
• The renderer.js change is a narrow, well-scoped guard around didFiberRender that aligns with the reported filtered-fiber bailout scenario and shouldn’t affect cases where prev/next are distinct. The main remaining concern is that the new tests assert an ordering of flamegraph nodes that may be unstable across renderer/layout changes, which could cause avoidable CI flakiness.
• packages/react-devtools-shared/src/tests/profilingCharts-test.js

Important Files Changed

Filename	Overview
packages/react-devtools-shared/src/tests/profilingCharts-test.js	Adds three profiling regression tests for filtered fibers; assertions currently rely on flamegraph node ordering (brittle across renderer/layout changes).
packages/react-devtools-shared/src/backend/fiber/renderer.js	Guards didFiberRender callsites with prevFiber !== nextFiber to avoid treating filtered/bailout descendants as updated; change appears consistent with existing identity-based assumptions.

[greptile-apps]

_{2 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

Invalid matcher arguments

toContainEqual only takes a single expected value, but these new tests pass two (Greeting and Count). This means the second expectation isn’t actually being asserted (or the matcher may throw), so the test won’t reliably catch regressions. Consider splitting into two expect(allNodes).toContainEqual(...) calls, or using expect(allNodes).toEqual(expect.arrayContaining([ ... ])).

Also appears at packages/react-devtools-shared/src/tests/profilingCharts-test.js:403 and :452.

Prompt To Fix With AI

This is a comment left during a code review.
Path: packages/react-devtools-shared/src/__tests__/profilingCharts-test.js
Line: 350:353

Comment:
**Invalid matcher arguments**

`toContainEqual` only takes a single expected value, but these new tests pass two (`Greeting` and `Count`). This means the second expectation isn’t actually being asserted (or the matcher may throw), so the test won’t reliably catch regressions. Consider splitting into two `expect(allNodes).toContainEqual(...)` calls, or using `expect(allNodes).toEqual(expect.arrayContaining([ ... ]))`.

Also appears at packages/react-devtools-shared/src/__tests__/profilingCharts-test.js:403 and :452.

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

[greptile-apps]

_{2 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

Brittle toEqual ordering

These new assertions require chartData.rows.flat() to return nodes in a specific order (App, then Greeting, then Count). That ordering is an implementation detail of the flamegraph layout and may vary across React versions/renderer changes, causing flaky failures even if the underlying didRender behavior is correct. Prefer an order-insensitive assertion (e.g. expect(allNodes).toEqual(expect.arrayContaining([...])) or separate toContainEqual checks) so the test only enforces the intended didRender values.

Prompt To Fix With AI

This is a comment left during a code review.
Path: packages/react-devtools-shared/src/__tests__/profilingCharts-test.js
Line: 350:353

Comment:
**Brittle `toEqual` ordering**

These new assertions require `chartData.rows.flat()` to return nodes in a specific order (`App`, then `Greeting`, then `Count`). That ordering is an implementation detail of the flamegraph layout and may vary across React versions/renderer changes, causing flaky failures even if the underlying `didRender` behavior is correct. Prefer an order-insensitive assertion (e.g. `expect(allNodes).toEqual(expect.arrayContaining([...]))` or separate `toContainEqual` checks) so the test only enforces the intended `didRender` values.

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

[everettbu]

Upstream PR was closed or merged. Code is synced via branch mirror.