I am in a process of splitting down the renderer implementation into
smaller units of logic that can be reused. This change is about
extracting pure functions only.
After https://github.com/facebook/react/pull/34089, when updating
(possibly, mounting) inside disconnected subtree, we don't record this
as an operation. This only happens during reconnect. The issue is that
`recordProfilingDurations()` can be called, which diffs tree base
duration and reports it to the Frontend:
https://github.com/facebook/react/blob/cba2bc83d8ce7887fabb90775d25b67ade36e5c3/packages/react-devtools-shared/src/backend/fiber/renderer.js#L4506-L4521
This operation can be recorded before the "Add" operation, and it will
not be resolved properly on the Frontend side.
Before the fix:
```
commit tree › Suspense › should handle transitioning from fallback back to content during profiling
Could not clone the node: commit tree does not contain fiber "5". This is a bug in React DevTools.
162 | const existingNode = nodes.get(id);
163 | if (existingNode == null) {
> 164 | throw new Error(
| ^
165 | `Could not clone the node: commit tree does not contain fiber "${id}". This is a bug in React DevTools.`,
166 | );
167 | }
at getClonedNode (packages/react-devtools-shared/src/devtools/views/Profiler/CommitTreeBuilder.js:164:13)
at updateTree (packages/react-devtools-shared/src/devtools/views/Profiler/CommitTreeBuilder.js:348:24)
at getCommitTree (packages/react-devtools-shared/src/devtools/views/Profiler/CommitTreeBuilder.js:112:20)
at ProfilingCache.getCommitTree (packages/react-devtools-shared/src/devtools/ProfilingCache.js:40:46)
at Object.<anonymous> (packages/react-devtools-shared/src/__tests__/profilingCommitTreeBuilder-test.js:257:44)
```
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.
There is an existing issue with serialisation logic for the traces from
Profiler panel.
I've discovered that `TREE_OPERATION_UPDATE_TREE_BASE_DURATION`
operation for some reason appears earlier in a sequence of operations,
before the `TREE_OPERATION_ADD` that registers the new node. It ends up
cloning non-existent node, which just creates an empty object and adds
it to the map of nodes.
This change only adds additional layer of validation to cloning logic,
so we don't swallow the error, if we attempt to clone non-existent node.
Fixes https://github.com/facebook/react/issues/31463,
https://github.com/facebook/react/issues/30114.
When switching between roots in the profiler flamegraph, the commit
index was preserved from the previous root. This caused an error
"Invalid commit X. There are only Y commits." when the new root had
fewer commits than the selected index.
This fix resets the commit index to 0 (or null if no commits) when the
commitData changes, which happens when switching roots.
`Error.prepareStackTrace` is non-standard feature and not all JavaScript
runtimes implement the methods that we are using in React DevTools
backend.
This PR adds additional checks for the presence of the methods that we
are using.
Follow-up to https://github.com/facebook/react/pull/34653.
React Native doesn't implement `getClientRect`, since this is applicable
to CSS box, which is not a concept for Native (maybe yet).
I am loosening the condition that gates `showOverlay()` call to pass if
`getClientRect` is not implemented.
Conceptually, everything that is inside `react-devtools-shared/backend`
should be Host-agnostic, because both on Web and Native it is installed
inside the Host JavaScript runtime, be it main frame of the page, or RN
instance. Since overlay & highlighting logic also lives there, it should
also follow these principles.
## Summary
Add keyboard shortcuts (Cmd/Ctrl + Left/Right arrow keys) to navigate
between commits in the Profiler's snapshot view.
Moved `filteredCommitIndices` management and commit navigation logic
(`selectNextCommitIndex`, `selectPrevCommitIndex`) from
`SnapshotSelector` into `useCommitFilteringAndNavigation` used by
`ProfilerContext` to enable keyboard shortcuts from the top-level
Profiler component.
## How did you test this change?
- New tests in ProfilerContext-tests
- Built browser extension: `yarn build:<browser name>`
- tested in browser: `yarn run test:<browser name>`
- Manually verified Left/Right arrow navigation cycles through commits
- Verified navigation respects commit duration filter
- Verified reload-and-profile button unaffected
Chrome:
https://github.com/user-attachments/assets/01d2a749-13dc-4d08-8bcb-3d4d45a5f97c
Edge with duration filter:
https://github.com/user-attachments/assets/a7f76ff7-2a0b-4b9c-a0ce-d4449373308b
firefox mixing hotkey with clicking arrow buttons:
https://github.com/user-attachments/assets/48912d68-7c75-40f2-a203-5e6d7e6b2d99
Follow-up to https://github.com/facebook/react/pull/34641.
Similar to https://github.com/facebook/react/pull/35293,
https://github.com/facebook/react/pull/35294.
React DevTools backend can be used in non-DOM environments, so we have
to feature-check some DOM APIs.
For now I am just no-oping newly added commands for Native, we should
revisit this decision once we would roll out Suspense panel there, if
needed. I am not sure if scrolling will be required as much as it is
needed on Web.
`isReactNativeEnvironment()` check is kinda clowny, but we've been
relying on it for quite some time already.
