Repository navigation
[web] optimize UI rerender boundaries - #404
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThis PR comprehensively refactors state management patterns across the application, replacing object destructuring with granular Zustand selectors, applies React.memo memoization to optimize component re-renders, prevents duplicate AI streaming start chunks, enhances drag-and-drop logic defensiveness, and refactors hooks for better stability and performance. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…ptimizations # Conflicts: # apps/web/src/components/layout/middle-content/page-views/dashboard/GlobalAssistantView.tsx # apps/web/src/lib/ai/shared/hooks/useMessageActions.ts
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/web/src/components/dialogs/MovePageDialog.tsx (1)
103-115:⚠️ Potential issue | 🟠 Major
availableDrivescreates a new reference every render, risking an infinite effect loop.
availableDrives(line 69) is recomputed via.filter()on every render, producing a new array reference each time. Because it's listed as a dependency at line 115, this effect re-fires every render. Inside,setExpandedNodes(new Set())always creates a new object, which triggers another re-render — forming an infinite loop.This is pre-existing and not introduced by this PR, but the tighter selector boundaries make it more likely to surface in practice. Consider wrapping
availableDrivesinuseMemoor removing it from the dependency array.Suggested fix
+ const availableDrives = useMemo( + () => drives.filter( + (d: Drive) => d.isOwned || d.role === "OWNER" || d.role === "ADMIN" + ), + [drives] + ); - const availableDrives = drives.filter( - (d: Drive) => d.isOwned || d.role === "OWNER" || d.role === "ADMIN" - );(Also add
useMemoto the React import at line 3.)apps/web/src/lib/ai/shared/hooks/useMessageActions.ts (1)
220-224:⚠️ Potential issue | 🟠 Major
setMessages(filteredMessages)uses a stale snapshot — inconsistent with the functional-updater pattern used elsewhere.
filteredMessagesis derived from themessagesclosure captured at callback creation time. The sequentialdelcalls above (lines 206–218) can take significant time, during which new messages may arrive. Setting messages to the stale snapshot will clobber them.Use a functional updater for consistency with
handleEdit/handleDelete:Proposed fix
- // Remove them from state - const filteredMessages = messages.filter( - (m) => !assistantMessagesToDelete.some((toDelete) => toDelete.id === m.id) - ); - setMessages(filteredMessages); + // Remove them from state + setMessages((previousMessages) => + previousMessages.filter( + (m) => !assistantMessagesToDelete.some((toDelete) => toDelete.id === m.id) + ) + );
🤖 Fix all issues with AI agents
In
`@apps/web/src/components/layout/left-sidebar/page-tree/MultiSelectToolbar.tsx`:
- Line 35: The selector creates a new array on every store update which forces
re-renders; fix by passing Zustand's shallow equality function to the hook so
the array is compared shallowly instead of by reference. Import useShallow from
'zustand/shallow' (or 'zustand/react/shallow' depending on your setup) and call
useMultiSelectStore with the same selector
(Array.from(state.selectedPages.values())) as the first arg and useShallow as
the second argument; this stabilizes selectedPages so handleBulkDelete and the
props passed into MovePageDialog and CopyPageDialog won't be recreated
unnecessarily.
In
`@apps/web/src/components/layout/middle-content/page-views/dashboard/GlobalAssistantView.tsx`:
- Around line 311-320: The inline setMessages updater functions are recreated
every render causing handleEdit, handleDelete, and handleRetry in
useMessageActions to be unstable; wrap those updater callbacks in useCallback
(referencing latestAgentMessagesRef and latestGlobalMessagesRef) so they are
stable across renders and only update when the refs change, then use the
memoized callbacks as the setMessages dependency for handleEdit, handleDelete,
and handleRetry to prevent unnecessary handler recreation.
🧹 Nitpick comments (12)
apps/web/src/components/dialogs/CopyPageDialog.tsx (1)
105-115:availableDrivesin the dependency array triggers the effect on every render.
availableDrivesis computed via.filter()on line 71, which creates a new array reference each render. As auseEffectdependency, this causes the effect to fire repeatedly — potentially resettingselectedDriveIdandselectedParentIdduring user interaction. This is pre-existing but undermines the re-render optimizations introduced here.Consider memoizing
availableDriveswithuseMemo:♻️ Proposed fix
+import { useState, useEffect, useCallback, useMemo } from "react"; -import { useState, useEffect, useCallback } from "react";- const availableDrives = drives.filter( - (d: Drive) => d.isOwned || d.role === "OWNER" || d.role === "ADMIN" - ); + const availableDrives = useMemo( + () => drives.filter( + (d: Drive) => d.isOwned || d.role === "OWNER" || d.role === "ADMIN" + ), + [drives] + );apps/web/src/hooks/useTabSync.ts (1)
38-38: Consider using the store'sselectActiveTabselector for consistency.The store already exports
selectActiveTab(state)which does the same lookup. Using it here would keep the logic DRY and automatically benefit from any future changes to how the active tab is resolved.+import { useTabsStore, selectActiveTab } from '@/stores/useTabsStore'; ... - const activeTab = state.tabs.find((t) => t.id === state.activeTabId); + const activeTab = selectActiveTab(state);This is purely a consistency nit — the current code is functionally equivalent.
apps/web/src/components/layout/middle-content/page-views/channel/ChannelView.tsx (2)
438-443: Custom memo comparator is correct but may drift.The comparator only checks
page.idandpage.driveId, which matches allpageproperties currently consumed in this component. This is a valid optimization — the heavy internal state (messages, socket listeners, effects) makes skipping parent-driven rerenders worthwhile.Consider adding a brief comment above the comparator noting that it must be updated if additional
pagefields are read in the future, to guard against staleness:export default memo( ChannelView, + // Only re-render when the channel identity changes. + // Update this comparator if additional page fields are consumed. (prevProps, nextProps) => prevProps.page.id === nextProps.page.id && prevProps.page.driveId === nextProps.page.driveId );
234-271:messagesin the dependency array defeats memoization forhandleRemoveReaction.Since
messagesis a dependency, this callback is recreated on every message arrival, which propagates a newonRemoveReactionprop to everyMessageReactionsinstance — exactly the kind of rerender cascade this PR aims to eliminate.You can remove the
messagesdependency by capturing the removed reaction inside the state updater:♻️ Suggested refactor
const handleRemoveReaction = useCallback(async (messageId: string, emoji: string) => { if (!user) return; - // Optimistic update - const removedReaction = messages - .find((m) => m.id === messageId) - ?.reactions?.find((r) => r.emoji === emoji && r.userId === user.id); - - setMessages((prev) => - prev.map((m) => { - if (m.id !== messageId) return m; - return { - ...m, - reactions: (m.reactions || []).filter( - (r) => !(r.emoji === emoji && r.userId === user.id) - ), - }; - }) - ); + // Capture the removed reaction inside the updater so `messages` + // doesn't need to be a dependency. + let removedReaction: Reaction | undefined; + setMessages((prev) => + prev.map((m) => { + if (m.id !== messageId) return m; + removedReaction ??= m.reactions?.find( + (r) => r.emoji === emoji && r.userId === user.id + ); + return { + ...m, + reactions: (m.reactions || []).filter( + (r) => !(r.emoji === emoji && r.userId === user.id) + ), + }; + }) + ); try { await del(`/api/channels/${page.id}/messages/${messageId}/reactions`, { emoji }); } catch { // Revert optimistic update on error if (removedReaction) { setMessages((prev) => prev.map((m) => { if (m.id !== messageId) return m; return { ...m, reactions: [...(m.reactions || []), removedReaction], }; }) ); } toast.error('Failed to remove reaction'); } - }, [page.id, user, messages]); + }, [page.id, user]);apps/web/src/components/layout/left-sidebar/page-tree/MultiSelectToolbar.tsx (1)
54-92:handleBulkDeletereads stale snapshot ofselectedPageson invocation.
selectedPagesandselectedCountare captured in the closure at callback creation time (Lines 56–57). BecauseselectedPagesis derived from the selector on Line 35 (and will be a new array each render as noted above), the values used inside the callback are from the render that last recreated it — which may not match the store's current state if an update races in between.A more resilient pattern is to read the store imperatively at call time:
Suggested fix
const handleBulkDelete = useCallback(async () => { setIsDeleting(true); - const count = selectedCount; - const pageIds = selectedPages.map((p) => p.id); + const { selectedPages: currentPages } = useMultiSelectStore.getState(); + const count = currentPages.size; + const pageIds = Array.from(currentPages.keys()); const toastId = toast.loading( `Moving ${count} ${count === 1 ? "page" : "pages"} to trash...` ); ... - }, [selectedCount, selectedPages, exitMultiSelectMode, onMutate]); + }, [exitMultiSelectMode, onMutate]);apps/web/src/lib/ai/shared/hooks/useMessageActions.ts (2)
69-136:messagesin the dependency array causeshandleEditidentity to change on every message update.Since
messagesis in the deps (line 136), andhandleEdit/handleDelete/handleRetryall depend on it, their identities change on every incoming message. If any of these callbacks are passed as props to memoized children, it defeatsReact.memo.The
messagesdependency is only needed to captureoriginalMessagefor rollback (line 73). Consider using a ref to hold the latest messages so the callback identity remains stable:const messagesRef = useRef(messages); messagesRef.current = messages;Then read
messagesRef.currentinside the callback instead ofmessages, and removemessagesfrom the dep array.This is optional given the PR's scope, but it would align with the stated goal of tightening rerender boundaries.
193-236: Sequentialdelcalls inhandleRetrycould be parallelized.Lines 206–218
awaiteach delete one at a time. Since these are independent network calls,Promise.allSettledwould reduce total latency for retry, especially when multiple assistant messages need cleanup.Suggested change
- // Delete them from the database - for (const msg of assistantMessagesToDelete) { - try { - if (isAgentMode) { - await del( - `/api/ai/page-agents/${agentId}/conversations/${conversationId}/messages/${msg.id}` - ); - } else { - await del(`/api/ai/global/${conversationId}/messages/${msg.id}`); - } - } catch (error) { - console.error('Failed to delete old assistant message:', error); - } - } + // Delete them from the database (in parallel — calls are independent) + await Promise.allSettled( + assistantMessagesToDelete.map((msg) => { + const url = isAgentMode + ? `/api/ai/page-agents/${agentId}/conversations/${conversationId}/messages/${msg.id}` + : `/api/ai/global/${conversationId}/messages/${msg.id}`; + return del(url).catch((error) => { + console.error('Failed to delete old assistant message:', error); + }); + }) + );apps/web/src/components/layout/right-sidebar/ai-assistant/SidebarHistoryTab.tsx (1)
392-392: Consider whether default shallow compare is sufficient forselectedAgentprop.
React.memo(SidebarHistoryTab)uses the default shallow comparison. TheselectedAgentprop is anAgentInfoobject — if the parent creates a new object reference on each render (even with same data), memo won't help. Verify that parents pass a stable reference (e.g., from a Zustand selector, which returns the same reference if the value hasn't changed).apps/web/src/contexts/GlobalChatContext.tsx (1)
272-295:initialMessagesin deps may cause unnecessary re-renders for history-only consumers.The
conversationContextValuememo includesinitialMessagesin its dependency array. When a conversation is loaded,initialMessagesgets a new array reference, causing the conversation context to update. Consumers likeSidebarHistoryTabthat only usecurrentConversationIdand the action functions will still re-render despite not usinginitialMessages.This is a minor gap — the big win (avoiding streaming re-renders) is preserved. If you want to further reduce re-renders, you could split into an even more granular context or have consumers use
useMemo/selectors on the context value. But this is likely fine as-is.apps/web/src/components/layout/middle-content/page-views/ai-page/AiChatView.tsx (1)
302-348:buildFreshPageContext— type cast is incomplete for SWR 2.x cache shape.Line 305 casts the SWR cache entry as
{ data?: TreePage[] } | undefined, but SWR 2.x'scache.get(key)returns{ data, error, isValidating } | undefined. The cast only capturesdata, missingerrorandisValidatingproperties.Since the code only accesses
.data, the cast works functionally. However, add a brief inline comment noting the complete SWR cache shape for clarity:// SWR 2.x cache shape: { data?, error?, isValidating? }apps/web/src/hooks/usePageTreeSocket.ts (1)
69-78: Double-revalidation path for events missing eitherpageIdorsocketId.Lines 69-72 trigger
debouncedRevalidate()whenpageIdis absent, and lines 76-78 trigger it again whensocketIdis absent. If an event arrives with both fields missing, only the first branch fires (early return), which is fine. But for events that do have apageIdbut nosocketId(e.g., server-originated updates that include the target page), you fall back to a full tree revalidation even though thepageIdis known and present in the tree. This is intentional per the comment (strict correctness for server-side updates), but it means tool-originated or server-side page renames won't get the granularupdateNodepath — they'll always do a full refetch.If that's acceptable, a brief inline comment clarifying this trade-off for future maintainers would help.
apps/web/src/components/layout/right-sidebar/index.tsx (1)
85-92:handleTabChangeis recreated every render — consideruseCallbackfor consistency.Given that this PR is specifically about tightening memoization boundaries and
RightPanelis now wrapped inmemo, stabilizinghandleTabChangewithuseCallbackwould be consistent with the optimization goals. It's passed toTabs.onValueChangeand, while unlikely to cause measurable perf issues, it would prevent Radix'sTabsfrom seeing a new function prop each render.♻️ Proposed stabilization
- const handleTabChange = (tab: string) => { - const validTab = tab as SidebarTab; - if (isDashboardContext) { - setDashboardActiveTab(validTab); - } else { - setLocalActiveTab(validTab); - } - }; + const handleTabChange = useCallback((tab: string) => { + const validTab = tab as SidebarTab; + if (isDashboardContext) { + setDashboardActiveTab(validTab); + } else { + setLocalActiveTab(validTab); + } + }, [isDashboardContext, setDashboardActiveTab]);
- Wrap availableDrives in useMemo in MovePageDialog and CopyPageDialog - Use messagesRef pattern in useMessageActions to stabilize callback identities - Switch handleRetry to functional updater and parallelize delete calls - Add useShallow for MultiSelectToolbar selector, read store imperatively in handleBulkDelete - Stabilize GlobalAssistantView setMessages callbacks with useCallback - Capture removedReaction inside state updater in ChannelView handleRemoveReaction - Use selectActiveTab selector in useTabSync for consistency - Wrap handleTabChange in useCallback in right-sidebar Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 participant
Summary
Validation
Summary by CodeRabbit
New Features
Bug Fixes
Performance