Skip to content

[DevTools] Only schedule a single update per Supense when changing timeline#633

Closed
everettbu wants to merge 2 commits into
mainfrom
sebbie/02-28-_devtools_only_schedule_a_single_update_per_supense_when_changing_timeline
Closed

[DevTools] Only schedule a single update per Supense when changing timeline#633
everettbu wants to merge 2 commits into
mainfrom
sebbie/02-28-_devtools_only_schedule_a_single_update_per_supense_when_changing_timeline

Conversation

@everettbu

Copy link
Copy Markdown

Mirror of facebook/react#35927
Original author: eps1lon


Changing the Suspense timeline milestone was brute force. We sent the Fiber to each renderer for update. Not only is that wasteful, it can also break React if the Fibers from one renderer were not compatible with another renderer.

E.g. scheduling an update on a Fiber from React canary with React experimental would crash. Instead of pendingIndicator being null | () => {}, React would see undefined since pendingIndicator simply doesn't exist in React canary.

Test plan

Before:

CleanShot.2026-02-28.at.17.02.27.mp4

After:

CleanShot.2026-02-28.at.16.59.45.mp4

@everettbu everettbu added CLA Signed React Core Team Opened by a member of the React Core Team labels Feb 28, 2026
@everettbu
everettbu marked this pull request as ready for review February 28, 2026 17:17
@greptile-apps

greptile-apps Bot commented Feb 28, 2026

Copy link
Copy Markdown

Greptile Summary

This PR fixes a crash when DevTools manages multiple React renderers (e.g., React canary + React experimental) by scoping Suspense milestone updates to the correct renderer instead of broadcasting to all renderers. Each SuspenseTimelineStep now carries a rendererID, allowing the frontend to group suspended sets by renderer and send targeted overrideSuspenseMilestone messages.

  • Added rendererID to SuspenseTimelineStep type and threaded it through all timeline construction methods in store.js
  • Refactored SuspenseTimeline.js to group suspended nodes by rendererID and send one bridge message per renderer
  • Changed agent.js to accept a rendererID parameter instead of iterating all renderers, though the null-safety guard present in sibling methods is missing
  • Updated tests to pass the new required rendererID parameter

Confidence Score: 4/5

  • This PR is safe to merge with one minor defensive coding concern in agent.js.
  • The core logic change is sound and well-motivated — it fixes a real cross-renderer crash. The implementation correctly threads rendererID through all relevant paths, and the SuspenseTimeline.js refactor properly initializes empty sets for all renderers before grouping. The one concern is the missing null check in agent.js for the renderer lookup, which is inconsistent with all sibling methods and could throw a TypeError if a stale rendererID is used.
  • packages/react-devtools-shared/src/backend/agent.js — missing null check for renderer lookup

Important Files Changed

Filename Overview
packages/react-devtools-shared/src/backend/agent.js Changed overrideSuspenseMilestone from iterating all renderers to targeting a specific rendererID, but missing null check for the renderer lookup unlike all sibling methods.
packages/react-devtools-shared/src/devtools/store.js Threads rendererID through timeline step construction methods with proper lookups and error handling for both initial paint and transition code paths.
packages/react-devtools-shared/src/devtools/views/SuspenseTab/SuspenseTimeline.js Refactored changeTimelineIndex to group suspended sets by rendererID and send per-renderer bridge messages, correctly initializing empty sets for all renderers first.
packages/react-devtools-shared/src/frontend/types.js Added rendererID: number field to SuspenseTimelineStep type.
packages/react-devtools-shared/src/bridge.js Added rendererID: number to OverrideSuspenseMilestone type definition, consistent with agent.js type.
packages/react-devtools-shared/src/tests/store-test.js Updated test calls to overrideSuspenseMilestone to include the new required rendererID parameter.

Fix All in Claude Code Fix All in Codex

Last reviewed commit: 2bf26ec

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

6 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

Comment on lines +794 to +797
const renderer = ((this._rendererInterfaces[
(rendererID: any)
]: any): RendererInterface);
if (renderer.supportsTogglingSuspense) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Missing null check for renderer lookup

Every other renderer lookup in this class (e.g., overrideSuspense at line 782, overrideError at line 770, logElementToConsole at line 757) guards against an invalid rendererID with a renderer == null check and a console.warn. This method skips that check and instead casts the result directly to RendererInterface, then immediately accesses renderer.supportsTogglingSuspense. If the rendererID is invalid or the renderer was unregistered, this will throw a TypeError instead of gracefully warning.

Suggested change
const renderer = ((this._rendererInterfaces[
(rendererID: any)
]: any): RendererInterface);
if (renderer.supportsTogglingSuspense) {
const renderer = this._rendererInterfaces[rendererID];
if (renderer == null) {
console.warn(`Invalid renderer id "${rendererID}"`);
} else if (renderer.supportsTogglingSuspense) {
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/react-devtools-shared/src/backend/agent.js
Line: 794-797

Comment:
**Missing null check for renderer lookup**

Every other renderer lookup in this class (e.g., `overrideSuspense` at line 782, `overrideError` at line 770, `logElementToConsole` at line 757) guards against an invalid `rendererID` with a `renderer == null` check and a `console.warn`. This method skips that check and instead casts the result directly to `RendererInterface`, then immediately accesses `renderer.supportsTogglingSuspense`. If the `rendererID` is invalid or the renderer was unregistered, this will throw a `TypeError` instead of gracefully warning.

```suggestion
    const renderer = this._rendererInterfaces[rendererID];
    if (renderer == null) {
      console.warn(`Invalid renderer id "${rendererID}"`);
    } else if (renderer.supportsTogglingSuspense) {
```

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

Fix in Claude Code Fix in Codex

@everettbu

Copy link
Copy Markdown
Author

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

@everettbu everettbu closed this Mar 3, 2026
@everettbu
everettbu deleted the sebbie/02-28-_devtools_only_schedule_a_single_update_per_supense_when_changing_timeline branch March 3, 2026 11:24
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