[tree view] Do not render items removed from the items prop - #23337
[tree view] Do not render items removed from the items prop#23337Anexus5919 wants to merge 2 commits into
items prop#23337Conversation
Deploy previewBundle size
Check out the code infra dashboard for more information about this PR. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d379decd6f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| >(parameters: MinimalTreeViewParameters<R, Multiple>): MinimalTreeViewState<R, Multiple> { | ||
| return { | ||
| treeId: undefined, | ||
| treeId: createTreeViewDefaultId(), |
There was a problem hiding this comment.
When a Tree View is rendered without an explicit id in SSR, this now increments the module-global globalTreeViewDefaultId during render and serializes that value into the HTML. In a long-lived server process the counter carries over between requests, while the browser starts from 0 during hydration, so later requests can server-render mui-tree-view-2 but hydrate as mui-tree-view-1, causing hydration mismatches and inconsistent item ids/ARIA relationships. The previous post-mount generation avoided putting these counter IDs in SSR output; keep render-time ids deterministic per request, e.g. via React useId, or defer default id generation until after hydration.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch, this was a real regression. globalTreeViewDefaultId is module scoped, so a long lived server process keeps counting across requests while the browser restarts from zero, and every request after the first would hydrate with a mismatched id.
Fixed in c24ac71: the default id now comes from useId (@mui/utils/useId, same as the Data Grid) and is passed to the store through the parameters, so it is stable across server and client. createTreeViewDefaultId is gone.
|
@noraleonte could you take a look at this one, or point me to the right person? The store now derives its state from the props during render instead of in a layout effect, so I would like a maintainer to confirm the timing change is acceptable before this goes further. Unrelated but worth flagging: so the Scheduler state is one render behind its parameters for the same reason. Out of scope here, but I can open a separate issue if that is useful. cc @JCQuintas |
noraleonte
left a comment
noraleonte
left a comment
There was a problem hiding this comment.
Thanks for digging into this. The diagnosis is correct and the repro output made it easy to follow.
I don't want to take the change to the store though. store.state is also read from outside React, by apiRef, event handlers and plugins, and today it always matches what is on screen. Deriving during render gives that up: a render that React starts and then drops would leave the store describing a tree that was never shown. It would also move us away from how the Scheduler, Charts and Data Grid all work.
There is a smaller bug in the current version too. store.parameters !== storeParameters is already true on the first render for SimpleTreeView, because its constructor normalises the parameters. So applyParametersDuringRender runs on mount and the items: EMPTY_ARRAY normalisation is lost.
The route I would rather explore is to keep the store synced in an effect and give React a separate view of the state to render from, so the committed state stays untouched for everything outside React. I still need to work out the details.
Whatever we end up with has to fit these constraints:
- no breaking change rn, and no change to existing public APIs
apiRefkeeps returning what is on screen- works with virtualization and with lazy loading (and other existent features)
- the Scheduler can use the same solution, since it has the same code
The useId part is separate and I like it. If you split it into its own PR I can review that one searately.
|
Thanks, this is fair and I agree with all of it. the I take the point on also I will leave this one open and hold off on a rewrite until you have settled the direction. One idea in case it is useful: rather than a second copy of the state, keep The detail I have not solved is clearing |
2 participants
Fixes #23331
Changes
The Tree View store was synced from the root component's props inside a layout effect, so on the render where
props.itemschanges the tree still renders the previous set of items. The store catches up after commit and schedules a second render with the correct set.Nothing flickers on screen because a layout effect runs before paint, but the item slot does render with ids that are no longer in
props.items. AslotProps.itemcallback that looks the item up in the user's own data, as shown in Using additional props, then receives an id it cannot resolve and throws. It also costs a wasted render pass of the subtree on everyitemschange.The state derived from the parameters is now computed during render, so the items rendered in that pass always match
props.items.MinimalTreeViewStore.applyParametersDuringRenderapplies the new state without notifying the subscribers, since React does not allow scheduling updates on other components while rendering.flushRenderUpdatenotifies in a layout effect, which only matters for the subscribers that bailed out of that render.treeIdnow comes fromuseIdand is passed to the store through the parameters. It used to be generated by the first sync, which no longer runs on mount. This also removes a full re-render of every item right after mount.@mui/x-internals/storeis untouched, so no other package is affected.Repro of the reported behavior, before the fix, logging the ids the item slot is rendered with after the child is removed:
['1', '1', '1.1', '1.1', '1', '1']The first pass is the stale one, doubled by StrictMode. After the fix,
1.1is gone.Testing
Added a regression test in
RichTreeView.test.tsx. It lives there rather than in thedescribeTreeViewsuites because that harness supplies its ownslotProps.item, and the stale pass is only observable when the callback identity changes on each render. When it does not,RichTreeViewItembails out of the re-render throughReact.memoand the test would pass without the fix.x-tree-viewandx-tree-view-prosuites pass, including the focus, keyboard navigation and memoization tests that depend on when the items state is applied.