perf(site): split sidebar context to prevent cascade re-renders#23726
perf(site): split sidebar context to prevent cascade re-renders#23726DanielleMaywood wants to merge 1 commit intomainfrom
Conversation
github-actions
bot
added
the
community
DanielleMaywood
left a comment
DanielleMaywood
left a comment
There was a problem hiding this comment.
🤖 This review was written by Coder Agent on behalf of Danielle Maywood 🤖
Review by Pluto 🐕 and team — 6 specialist reviewers, 22 hypotheses tested, 4 fact-checked.
Overall this is a well-designed change. The context split is the right architecture and the bridge component pattern is sound. Three items to address, one of which undermines the stated perf goal.
| interface ChatTreeNodeProps { | ||
| readonly chat: Chat; | ||
| readonly isChildNode: boolean; | ||
| readonly childChats: readonly Chat[]; |
There was a problem hiding this comment.
[P1 Important] childChats array prop defeats memo() — the perf win this PR is going for doesn't work yet
childChats is only used at line 519 for const hasChildren = childChats.length > 0; — a boolean derivation. The actual child rendering is handled by <ChatTreeChildren parentChatId={chatID} /> (line 744), which subscribes to the data context independently.
All three call sites compute childChats via .map().filter(), which always returns a new array reference. memo() uses shallow equality, so the comparison fails on every render and ChatTreeNode never bails out.
Verified against the React Compiler output: the compiler emits _c(95) for ChatTreeNode — 95 internal cache slots that make unchanged renders cheap but not free. The memo() wrapper is preserved in compiled output but defeated by the new array reference. Skipping 95 cache checks + vDOM production + reconciliation of NavLink/DropdownMenu/Tooltip for ~99 unchanged nodes per WS event is meaningful.
Suggestion: replace childChats: readonly Chat[] with hasChildren: boolean. Compute the boolean at each call site. With all props as primitives or structurally-shared references, memo() can actually bail out.
Also: memo() on SortableChatTreeNode (line 751) is useless — it calls useChatTreeData() internally (line 768), and context subscriptions bypass memo(). It re-renders on every data context change regardless. The compiler's _c(34) internal cache handles the optimization there. Consider removing it.
🤖 This comment was written by Coder Agent on behalf of Danielle Maywood 🤖
| // Compute per-node data from the data context so the | ||
| // memo'd ChatTreeNode can skip re-renders when its | ||
| // specific slice of data hasn't changed. | ||
| const childIDs = (chatTree.childrenById.get(chat.id) ?? []).filter( |
There was a problem hiding this comment.
[P2 Improvement] Per-node prop derivation duplicated in three rendering sites
This same five-step pattern — filter visible child IDs → resolve to Chat objects → compute isExpanded → compute errorReason → compute archiving/regenerating flags — is repeated in ChatTreeChildren (lines 459–489), here in SortableChatTreeNode (lines 797–831), and in the time-grouped section (lines 1250–1289).
Adding a new prop to ChatTreeNodeProps requires updating all three sites in lockstep, and variable naming already diverges (childChatsForNode vs childChats vs nodeChildChats).
Suggestion: extract a shared helper, e.g. function computeNodeProps(chatId: string, chat: Chat, data: ChatTreeDataValue) that returns the per-node prop slice. All three sites call it and spread the result.
🤖 This comment was written by Coder Agent on behalf of Danielle Maywood 🤖
| } | ||
| /> | ||
| ); | ||
| })}{" "} |
There was a problem hiding this comment.
[P3 Nit] Stray {" "} text node
This {" "} wasn't in the original code — it was introduced during the refactor. It creates an anonymous flex item inside the flex flex-col gap-0.5 container. Visually invisible but accidental DOM content. Prettier doesn't produce or remove it.
| })}{" "} | |
| })} |
🤖 This comment was written by Coder Agent on behalf of Danielle Maywood 🤖
The ChatTreeContext bundled both stable callbacks and frequently- changing data into a single context. Every WebSocket event (status change, title update) created a new context value, forcing all ~100 ChatTreeNode components to re-render — 500-700 component re-renders per event. Split into ChatTreeCallbacksContext (stable callbacks + config) and ChatTreeDataContext (tree data that changes on WS events). ChatTreeNode is now memo()-wrapped and receives per-node data as props instead of reading from context. A thin ChatTreeChildren bridge component subscribes to the data context for recursive child rendering, computing per-child props so memo() can skip unchanged nodes. When a single chat status changes, only that node re-renders instead of the entire sidebar tree.
DanielleMaywood
force-pushed
the
daniellemaywood/sidebar-rerender-fix
branch
from
763bdd4 to
b8f62ee
Compare
DanielleMaywood
removed
the
community
1 participant
Every WebSocket event (status change, title update) was re-rendering all ~100
ChatTreeNodecomponents in the sidebar — 500–700 re-renders per event — because they all subscribed to a singleChatTreeContextwhose value changed on every update.Split into two contexts:
ChatTreeCallbacksContext— stable callbacks and config that do not change on WS eventsChatTreeDataContext— tree data that changes when chats updateChatTreeNodeis nowmemo()-wrapped and receives per-node data as props (error state, expanded, archiving, etc.) instead of reading from context. A thinChatTreeChildrenbridge component subscribes to the data context for recursive child rendering, computing per-child props somemo()can skip unchanged nodes.When a single chat changes status, only that node and its direct parent re-render instead of the entire sidebar tree.
Profiling context
Before (React DevTools Profiler)
ChatTreeNodeat 50ms total self-time, withNavLink,DropdownMenu,MenuPortalcascading from every node.Root cause
ChatTreeContextbundled 18+ fields into one object.chatTree,chatById,visibleChatIDswere recomputed from thechatsarray on every WS event (newpages.flat()reference from react-query). EveryChatTreeNodecalleduseChatTree()→ context change bypassesmemo()→ full cascade.Fix approach
The store already preserves
ChatMessageobject identity for unchanged chats. By moving per-node derived data into props,memo()onChatTreeNodesees stablechatreferences for unaffected nodes and skips re-render. Only the node whose chat object changed (new ref from cache updater) re-renders.