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.
When dealing with optimistic state, a common problem is not knowing the
id of the thing we're waiting on. Items in lists need keys (and single
items should often have keys too to reset their state). As a result you
have to generate fake keys. It's a pain to manage those and when the
real item comes in, you often end up rendering that with a different
`key` which resets the state of the component tree. That in turns works
against the grain of React and a lot of negatives fall out of it.
This adds a special `optimisticKey` symbol that can be used in place of
a `string` key.
```js
import {optimisticKey} from 'react';
...
const [optimisticItems, setOptimisticItems] = useOptimistic([]);
const children = savedItems.concat(
optimisticItems.map(item =>
<Item key={optimisticKey} item={item} />
)
);
return <div>{children}</div>;
```
The semantics of this `optimisticKey` is that the assumption is that the
newly saved item will be rendered in the same slot as the previous
optimistic items. State is transferred into whatever real key ends up in
the same slot.
This might lead to some incorrect transferring of state in some cases
where things don't end up lining up - but it's worth it for simplicity
in many cases since dealing with true matching of optimistic state is
often very complex for something that only lasts a blink of an eye.
If a new item matches a `key` elsewhere in the set, then that's favored
over reconciling against the old slot.
One quirk with the current algorithm is if the `savedItems` has items
removed, then the slots won't line up by index anymore and will be
skewed. We might be able to add something where the optimistic set is
always reconciled against the end. However, it's probably better to just
assume that the set will line up perfectly and otherwise it's just best
effort that can lead to weird artifacts.
An `optimisticKey` will match itself for updates to the same slot, but
it will not match any existing slot that is not an `optimisticKey`. So
it's not an `any`, which I originally called it, because it doesn't
match existing real keys against new optimistic keys. Only one
direction.
