Fix performance of chat panel during streaming #3020
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
Warning Rate limit exceeded@saddlepaddle has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 18 minutes and 6 seconds before requesting another review. ⌛ 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. 📒 Files selected for processing (1)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughRefactors multiple chat and UI components to export memoized and/or MobX-observer-wrapped versions; introduces isStreaming/isStream and applied props; adds debounced rendering and theme-aware syntax highlighting for code blocks; optimizes conditional rendering in CollapsibleCodeBlock. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Chat UI
participant Msg as Message Component<br/>(memo/observer)
participant Store as MobX Store
participant Code as CodeBlock<br/>(memo)
UI->>Msg: Render message (props, messageId, isStream)
Msg->>Store: Subscribe (observer)
Note right of Msg: Memoized — skips re-render if props unchanged
alt message contains code (isStream=true)
UI->>Code: Render with isStreaming=true
Code->>Code: Debounce updates (300ms)
Code->>Code: Theme-aware syntax highlight
else static code (isStream=false)
UI->>Code: Render with isStreaming=false
Code->>Code: Update immediately
end
Store-->>Msg: Observable change
Msg->>Msg: Re-render only if needed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
| }, [code, isStreaming]); | ||
|
|
||
| return ( | ||
| <CodeBlockContext.Provider value={{ code }}> |
There was a problem hiding this comment.
Consider using the debounced code value (debouncedCode) in the context provider instead of the raw code. This ensures that when users copy the code via the copy button, it matches the debounced (displayed) version during streaming.
| <CodeBlockContext.Provider value={{ code }}> | |
| <CodeBlockContext.Provider value={{ code: debouncedCode }}> |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/ui/src/components/ai-elements/code-block.tsx (1)
134-143: Icon-only button lacks an accessible name; add aria-label (and clear timeout).Screen readers won’t announce purpose without an accessible name. Also clear the “copied” timeout on unmount to avoid setState after unmount.
Apply this diff:
- <Button + <Button className={cn('shrink-0', className)} onClick={copyToClipboard} size="icon" variant="ghost" + aria-label={isCopied ? 'Copied' : 'Copy code'} + title={isCopied ? 'Copied' : 'Copy code'} {...props} > - {children ?? <Icon size={14} />} + {children ?? <Icon size={14} aria-hidden="true" />} </Button>Optional cleanup (outside this hunk):
import { useEffect, useRef } from 'react'; const timerRef = useRef<number | null>(null); // after successful copy: timerRef.current = window.setTimeout(() => setIsCopied(false), timeout); // cleanup: useEffect(() => () => { if (timerRef.current) clearTimeout(timerRef.current); }, []);apps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/code-display/collapsible-code-block.tsx (2)
59-61: Use a button element for the trigger (keyboard a11y).Using a div as the CollapsibleTrigger child harms focus/keyboard support. Prefer a semantic button.
- <CollapsibleTrigger asChild> - <div className="flex-1 flex items-center gap-2 cursor-pointer pl-3 py-2"> + <CollapsibleTrigger asChild> + <button type="button" className="flex-1 flex items-center gap-2 cursor-pointer pl-3 py-2"> ... - </div> + </button> </CollapsibleTrigger>
1-1: Add 'use client' directive—observer and state require client boundary.This component uses
observer(MobX),useState, anduseEditorEnginehook, all of which require client execution. Per coding guidelines, apps/web/client/src/app/**/*.tsx defaults to Server Components; add 'use client' when using state/browser APIs. Per the learning guideline, observer components must be client components, and no upstream layout in the inheritance chain (app/layout.tsx,project/layout.tsx,project/[id]/layout.tsx) has a client boundary.Add the directive at the top of the file:
+ 'use client'; + import { useEditorEngine } from '@/components/store/editor';
🧹 Nitpick comments (14)
packages/ui/src/components/ai-elements/tool.tsx (2)
96-110: Well-structured memoization pattern.The refactoring correctly implements React.memo by creating an internal
ToolInputComponentand exporting the memoized version. TheisStreamingprop is properly typed and threaded through toCodeBlock.However, since
inputis typed asToolUIPart['input'](likely an object),memo's default shallow comparison may not prevent re-renders if the parent component creates new object references on each render. The memoization will still help when other parent props change, which may be sufficient for streaming scenarios.Consider adding a display name for better debugging experience:
export const ToolInput = memo(ToolInputComponent); +ToolInput.displayName = 'ToolInput';
112-151: Memoization pattern correctly applied with consistent prop threading.The
ToolOutputrefactoring mirrors theToolInputpattern and properly passesisStreamingto all threeCodeBlockinstances (lines 126, 128). The conditional rendering logic remains intact while adding the performance optimization.Similar to
ToolInput, theoutputprop may be a complex object, somemo's default shallow comparison might not catch all changes. However, this is acceptable for the streaming use case where the optimization targets unrelated prop changes.Optionally add a display name for consistency with debugging tools:
export const ToolOutput = memo(ToolOutputComponent); +ToolOutput.displayName = 'ToolOutput';apps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/chat-messages/assistant-message.tsx (2)
6-14: Consider consistent prop naming between components.The component receives
isStreamingbut passes it asisStreamto MessageContent. While functionally correct, this inconsistency makes the data flow harder to trace.Apply this diff to align prop names:
-const AssistantMessageComponent = ({ message, isStreaming }: { message: ChatMessage, isStreaming: boolean }) => { +const AssistantMessageComponent = ({ message, isStream }: { message: ChatMessage, isStream: boolean }) => { return ( <div className="px-4 py-2 text-small content-start flex flex-col text-wrap gap-2"> <MessageContent messageId={message.id} parts={message.parts} applied={false} - isStream={isStreaming} + isStream={isStream} /> </div> );Or rename the MessageContent prop to match the incoming prop name.
19-19: Reconsider the memo + observer wrapper combination.The pattern
memo(observer(...))may be redundant. MobX'sobserveralready performs shallow prop comparison and optimizes re-renders. Wrapping withmemoadds an additional equality check that typically provides minimal benefit and can complicate debugging.Unless you have a specific performance issue that requires both wrappers, consider using only
observer:-export const AssistantMessage = memo(observer(AssistantMessageComponent)); +export const AssistantMessage = observer(AssistantMessageComponent);If you need both, the recommended order from MobX documentation is
observer(memo(...))rather thanmemo(observer(...)).apps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/chat-messages/user-message.tsx (1)
333-333: Reconsider the memo + observer wrapper combination.Similar to AssistantMessage, the
memo(observer(...))pattern may be redundant. MobX'sobserveralready optimizes re-renders through shallow prop comparison.Consider using only
observerfor simplicity:-export const UserMessage = memo(observer(UserMessageComponent)); +export const UserMessage = observer(UserMessageComponent);If both wrappers are needed for specific performance reasons, the recommended order is
observer(memo(...)).packages/ui/src/components/ai-elements/code-block.tsx (2)
41-53: Debounce looks good; avoid redundant state sets.Guard setDebouncedCode to skip no-op updates.
Apply this diff:
- const timer = setTimeout(() => { - setDebouncedCode(code); - }, 300); + const timer = setTimeout(() => { + setDebouncedCode((prev) => (prev === code ? prev : code)); + }, 300); return () => clearTimeout(timer); } else { // When not streaming, update immediately - setDebouncedCode(code); + setDebouncedCode((prev) => (prev === code ? prev : code)); }
63-87: Hot path optimization: reduce Highlighter cost and prop churn.Two incremental wins:
- Load-time: switch to light build + dynamic import (code-split) or at least dynamic-import Prism.
- Render-time: memoize static prop objects to avoid new identities each render.
Option A (preferred): Light build + register needed languages
// imports (outside this hunk) import dynamic from 'next/dynamic'; const SyntaxHighlighter = dynamic( () => import('react-syntax-highlighter').then(m => m.PrismLight), { ssr: false } ); // Register only what you use, e.g.: import tsx from 'react-syntax-highlighter/dist/esm/languages/prism/tsx'; SyntaxHighlighter.registerLanguage('tsx', tsx);Option B: Keep Prism, but code-split it
import dynamic from 'next/dynamic'; const SyntaxHighlighter = dynamic( () => import('react-syntax-highlighter').then(m => m.Prism), { ssr: false } );Also memoize static props:
- codeTagProps={{ - className: 'font-mono text-xs', - }} + codeTagProps={codeTagProps} ... - customStyle={{ - margin: 0, - padding: '1rem', - fontSize: '0.875rem', - background: 'hsl(var(--background-secondary))', - color: 'hsl(var(--foreground))', - }} + customStyle={customStyle} ... - lineNumberStyle={{ - color: 'hsl(var(--muted-foreground))', - paddingRight: '1rem', - minWidth: '2.5rem', - }} + lineNumberStyle={lineNumberStyle}Add (outside this hunk):
import { useMemo } from 'react'; const codeTagProps = useMemo(() => ({ className: 'font-mono text-xs' }), []); const customStyle = useMemo(() => ({ margin: 0, padding: '1rem', fontSize: '0.875rem', background: 'hsl(var(--background-secondary))', color: 'hsl(var(--foreground))', }), []); const lineNumberStyle = useMemo(() => ({ color: 'hsl(var(--muted-foreground))', paddingRight: '1rem', minWidth: '2.5rem', }), []);apps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/code-display/collapsible-code-block.tsx (7)
20-25: Unused props (applied, messageId) and naming consistency.applied and messageId are declared but unused; consider removing or rendering an "Applied" chip/indicator. Also consider aligning isStream → isStreaming for consistency with downstream APIs (verify impact first).
Example (if you intend to show status):
const CollapsibleCodeBlockComponent = ({ path, content, - isStream, - branchId, + isStream, + branchId, + applied, }: CollapsibleCodeBlockProps) => { ...And render an "Applied" badge near the filename if true.
40-43: Guard branch lookup to avoid crashes if store is unavailable.Make branch resolution null‑safe.
- const branch = branchId - ? editorEngine.branches.allBranches.find(b => b.id === branchId) - : editorEngine.branches.activeBranch; + const branch = + branchId + ? editorEngine?.branches?.allBranches?.find(b => b.id === branchId) + : editorEngine?.branches?.activeBranch;
78-82: Improve long-branch-name UX.Add a title for hover tooltip so truncated names are discoverable.
- <span className="text-foreground-tertiary group-hover:text-foreground-secondary text-mini ml-0.5 flex-shrink-0 truncate max-w-24"> + <span + title={branch.name} + className="text-foreground-tertiary group-hover:text-foreground-secondary text-mini ml-0.5 flex-shrink-0 truncate max-w-24" + >
98-100: Infer language from path instead of hardcoding 'jsx'.Improves highlighting for ts/tsx/js/json/etc.
+ const inferLanguageFromPath = (p: string) => { + const ext = p.split('.').pop()?.toLowerCase(); + switch (ext) { + case 'ts': return 'typescript'; + case 'tsx': return 'tsx'; + case 'js': return 'javascript'; + case 'jsx': return 'jsx'; + case 'json': return 'json'; + case 'css': return 'css'; + case 'html': return 'html'; + case 'sh': return 'bash'; + default: return 'plaintext'; + } + }; ... - <CodeBlock code={content} language="jsx" isStreaming={isStream} className="text-xs overflow-x-auto" /> + <CodeBlock code={content} language={inferLanguageFromPath(path)} isStreaming={isStream} className="text-xs overflow-x-auto" />
101-117: i18n the labels and harden clipboard copy (fallback + cleanup).
- Replace hardcoded "Copy/Copied" with next‑intl. As per coding guidelines.
- Handle clipboard API errors/unsupported contexts and clear timeout on unmount.
- import { memo, useState } from 'react'; + import { memo, useEffect, useRef, useState } from 'react'; + import { useTranslations } from 'next-intl'; ... const [isOpen, setIsOpen] = useState(false); const [copied, setCopied] = useState(false); + const copyTimeoutRef = useRef<number | null>(null); + const t = useTranslations('chat.codeBlock'); ... - const copyToClipboard = () => { - navigator.clipboard.writeText(content); - setCopied(true); - setTimeout(() => setCopied(false), 2000); - }; + const copyToClipboard = async () => { + try { + if (navigator.clipboard?.writeText) { + await navigator.clipboard.writeText(content); + } else { + throw new Error('Clipboard API unavailable'); + } + } catch { + const ta = document.createElement('textarea'); + ta.value = content; + ta.style.position = 'fixed'; + ta.style.opacity = '0'; + document.body.appendChild(ta); + ta.select(); + try { document.execCommand('copy'); } finally { document.body.removeChild(ta); } + } + setCopied(true); + if (copyTimeoutRef.current) clearTimeout(copyTimeoutRef.current); + copyTimeoutRef.current = window.setTimeout(() => setCopied(false), 2000); + }; + + useEffect(() => () => { if (copyTimeoutRef.current) clearTimeout(copyTimeoutRef.current); }, []); ... - Copied + {t('copied')} ... - Copy + {t('copy')}Define messages:
- chat.codeBlock.copy
- chat.codeBlock.copied
89-95: Optional: add exit animation or drop AnimatePresence.You use AnimatePresence mode="wait" but no exit prop; add exit for a smoother collapse.
<motion.div key="content" initial={getAnimation()} animate={getAnimation()} + exit={{ height: 0, opacity: 0 }} transition={{ duration: 0.2, ease: 'easeInOut' }} style={{ overflow: 'hidden' }} >
8-9: observer + memo layering is redundant.observer already optimizes renders; wrapping with memo adds little and can confuse contributors. Prefer a single observer.
- import { memo, useState } from 'react'; + import { useState } from 'react'; ... - export const CollapsibleCodeBlock = memo(observer(CollapsibleCodeBlockComponent)); + export const CollapsibleCodeBlock = observer(CollapsibleCodeBlockComponent);Also applies to: 131-131
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
apps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/chat-messages/assistant-message.tsx(2 hunks)apps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/chat-messages/message-content/index.tsx(2 hunks)apps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/chat-messages/message-content/tool-call-display.tsx(2 hunks)apps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/chat-messages/message-content/tool-call-simple.tsx(2 hunks)apps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/chat-messages/user-message.tsx(4 hunks)apps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/code-display/collapsible-code-block.tsx(6 hunks)packages/ui/src/components/ai-elements/code-block.tsx(2 hunks)packages/ui/src/components/ai-elements/tool.tsx(3 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
apps/web/client/src/app/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/client/src/app/**/*.tsx: Default to Server Components; add 'use client' when using events, state/effects, browser APIs, or client‑only libraries
Do not use process.env in client code; import env from @/env insteadAvoid hardcoded user-facing text; use next-intl messages/hooks
Files:
apps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/chat-messages/assistant-message.tsxapps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/chat-messages/message-content/index.tsxapps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/chat-messages/user-message.tsxapps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/code-display/collapsible-code-block.tsxapps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/chat-messages/message-content/tool-call-display.tsxapps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/chat-messages/message-content/tool-call-simple.tsx
apps/web/client/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/client/src/**/*.{ts,tsx}: Use path aliases @/* and ~/* for imports that map to apps/web/client/src/*
Avoid hardcoded user-facing text; use next-intl messages/hooks insteadUse path aliases @/* and ~/* for imports mapping to src/*
Files:
apps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/chat-messages/assistant-message.tsxapps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/chat-messages/message-content/index.tsxapps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/chat-messages/user-message.tsxapps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/code-display/collapsible-code-block.tsxapps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/chat-messages/message-content/tool-call-display.tsxapps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/chat-messages/message-content/tool-call-simple.tsx
apps/web/client/src/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/client/src/**/*.tsx: Create MobX store instances with useState(() => new Store()) for stable references across renders
Keep the active MobX store in a useRef and perform async cleanup with setTimeout(() => storeRef.current?.clear(), 0) to avoid route-change races
Avoid useMemo for creating MobX store instances
Avoid putting the MobX store instance in effect dependency arrays if it causes loops; split concerns by domain
apps/web/client/src/**/*.tsx: Create MobX store instances with useState(() => new Store()) for stable identities across renders
Keep the active MobX store in a useRef and clean up asynchronously with setTimeout(() => storeRef.current?.clear(), 0)
Do not use useMemo to create MobX stores
Avoid placing MobX store instances in effect dependency arrays if it causes loops; split concerns instead
observer components must be client components; place a single client boundary at the feature entry; child observers need not repeat 'use client'
Files:
apps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/chat-messages/assistant-message.tsxapps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/chat-messages/message-content/index.tsxapps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/chat-messages/user-message.tsxapps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/code-display/collapsible-code-block.tsxapps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/chat-messages/message-content/tool-call-display.tsxapps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/chat-messages/message-content/tool-call-simple.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Do not use the any type unless necessary
Files:
apps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/chat-messages/assistant-message.tsxapps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/chat-messages/message-content/index.tsxapps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/chat-messages/user-message.tsxapps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/code-display/collapsible-code-block.tsxpackages/ui/src/components/ai-elements/code-block.tsxapps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/chat-messages/message-content/tool-call-display.tsxapps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/chat-messages/message-content/tool-call-simple.tsxpackages/ui/src/components/ai-elements/tool.tsx
apps/web/client/src/app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Default to Server Components; add 'use client' only when using events, state/effects, browser APIs, or client-only libs
Files:
apps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/chat-messages/assistant-message.tsxapps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/chat-messages/message-content/index.tsxapps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/chat-messages/user-message.tsxapps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/code-display/collapsible-code-block.tsxapps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/chat-messages/message-content/tool-call-display.tsxapps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/chat-messages/message-content/tool-call-simple.tsx
{apps,packages}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Avoid using the any type unless absolutely necessary
Files:
apps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/chat-messages/assistant-message.tsxapps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/chat-messages/message-content/index.tsxapps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/chat-messages/user-message.tsxapps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/code-display/collapsible-code-block.tsxpackages/ui/src/components/ai-elements/code-block.tsxapps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/chat-messages/message-content/tool-call-display.tsxapps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/chat-messages/message-content/tool-call-simple.tsxpackages/ui/src/components/ai-elements/tool.tsx
🧬 Code graph analysis (4)
apps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/chat-messages/assistant-message.tsx (1)
packages/models/src/chat/message/message.ts (1)
ChatMessage(19-19)
apps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/code-display/collapsible-code-block.tsx (2)
packages/ui/src/components/ai-elements/code-block.tsx (1)
CodeBlock(96-96)apps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/code-display/code-block.tsx (1)
CodeBlock(7-39)
apps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/chat-messages/message-content/tool-call-simple.tsx (1)
packages/ui/src/components/ai-elements/tool.tsx (4)
ToolInput(110-110)ToolOutput(151-151)ToolContent(86-94)Tool(14-19)
packages/ui/src/components/ai-elements/tool.tsx (1)
packages/ui/src/components/ai-elements/code-block.tsx (1)
CodeBlock(96-96)
🔇 Additional comments (13)
packages/ui/src/components/ai-elements/tool.tsx (1)
6-6: LGTM! Import aligns with performance optimization goals.The
memoimport is correctly added to support the memoization pattern applied toToolInputandToolOutputcomponents.apps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/chat-messages/message-content/tool-call-simple.tsx (3)
32-32: LGTM! Proper memo export pattern.The export wrapping is correctly implemented and will help reduce unnecessary re-renders during streaming operations.
5-15: Memoization pattern approved; 'use client' boundary verified in parent hierarchy.The restructuring to use an internal
ToolCallSimpleComponentwrapped withmemofollows React best practices and aligns with the PR's performance optimization goals. The 'use client' directive is properly established in parent components (chat-messages/index.tsxand others), so this child component correctly inherits the client boundary without repeating the directive.The
isStreamingprop propagation toToolInputandToolOutputis appropriate for streaming optimization.
25-26: Excellent propagation of streaming state to child components.Passing
isStreaming={loading}to bothToolInputandToolOutputenables these components to optimize their rendering during streaming, which directly addresses the PR's performance objectives. Both components are properly typed to accept the optionalisStreamingprop and pass it to their CodeBlock children for streaming optimization.apps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/chat-messages/message-content/index.tsx (2)
74-74: Good use of observer-only wrapper.Using only
observerwithoutmemois the appropriate pattern here. The observer wrapper provides the necessary reactivity without the redundant memoization layer seen in some other components in this PR.
1-74: No action required—architecture is correct per guidelines.The codebase correctly implements the client boundary pattern. The single
'use client'directive is properly placed atchat-messages/index.tsx(the feature entry point), which importsUserMessage,AssistantMessage, andErrorMessageas children. These child components in turn useMessageContent—an observer component that correctly inherits the client boundary without repeating the directive. Per coding guidelines, child observer components should not repeat'use client'.Likely an incorrect or invalid review comment.
apps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/chat-messages/user-message.tsx (2)
46-46: Observer wrapper is appropriate for MobX store usage.The use of
observeris justified since the component callsuseEditorEngine()(line 46), which likely returns a MobX store with observable properties that need reactivity tracking.
1-333: No action required—client boundary is correctly placed at feature entry per coding guidelines.The verified component hierarchy shows:
index.tsx(feature entry) has'use client'and directly importsUserMessageUserMessageis anobservercomponent that does not repeat'use client'- This follows the coding guideline: "observer components must be client components; place a single client boundary at the feature entry; child observers need not repeat 'use client'"
Since
index.tsxhas the client boundary and directly importsUserMessage, the observer component is correctly part of the client bundle without needing its own directive.apps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/chat-messages/assistant-message.tsx (1)
1-19: Now let me check the index.tsx file to verify the import/export chain:Now let me inspect the index.tsx file and check the parent hierarchy for the proper 'use client' boundary:Based on my verification, I have sufficient information to generate the final rewritten review comment. Let me analyze what I've learned:Verify 'use client' directive is already present at the parent boundary.
The 'use client' directive in index.tsx (confirmed at line 1) marks all its imports and child components as part of the client bundle. Since
AssistantMessageis imported and wrapped withobserverin the component tree, it already operates within a client context. You don't need to add the directive to every component intended for the client.No changes required—the existing parent boundary satisfies the observer component requirement.
apps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/chat-messages/message-content/tool-call-display.tsx (1)
16-16: LGTM! Theappliedprop is correctly implemented.The new
appliedboolean prop is properly added to the component signature and consistently passed to allCollapsibleCodeBlockinstances across the different tool branches (WriteFileTool, FuzzyEditFileTool, SearchReplaceEditTool, SearchReplaceMultiEditFileTool). This supports the streaming performance optimization mentioned in the PR objectives.Also applies to: 21-21, 91-91, 116-116, 141-141, 166-166
packages/ui/src/components/ai-elements/code-block.tsx (2)
55-56: Confirm copy behavior uses raw code vs. displayed (debounced) code.Context provides the live code, while the UI renders debouncedCode. Is this intentional for “copy latest even if render lags”? If you want copy to match what’s visible, provide both or switch to debouncedCode.
96-96: Memo wrapper LGTM.Good call to export a memoized wrapper; avoid adding a custom comparator so the effect can still observe code changes.
apps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/code-display/collapsible-code-block.tsx (1)
96-99: Nice perf win: gate CodeMirror render behind isOpen.Avoiding the heavy editor when collapsed is the right call for streaming performance.
| import { FuzzyEditFileTool, SearchReplaceEditTool, SearchReplaceMultiEditFileTool, TerminalCommandTool, TypecheckTool, WebSearchTool, WriteFileTool } from '@onlook/ai'; | ||
| import type { WebSearchResult } from '@onlook/models'; | ||
| import type { ToolUIPart } from 'ai'; | ||
| import { observer } from 'mobx-react-lite'; |
There was a problem hiding this comment.
🧩 Analysis chain
Verify the necessity of the observer wrapper and consider adding memoization.
The component is now wrapped with observer(), but scanning the implementation reveals no obvious access to MobX observable properties. All props appear to be plain values (strings, booleans, ToolUIPart), and the component body only accesses standard properties like toolPart.type, toolPart.state, etc.
Additionally, other components in this PR (mentioned in the AI summary) follow a memo(observer(...)) pattern for consistency and performance, but this component only uses observer().
Run the following script to verify if ToolUIPart or its accessed properties contain MobX observables:
If ToolUIPart does not contain observables, consider removing the observer() wrapper. Otherwise, for consistency with other components in this PR, apply this pattern:
-import { observer } from 'mobx-react-lite';
+import { memo } from 'react';
+import { observer } from 'mobx-react-lite';
-export const ToolCallDisplay = observer(ToolCallDisplayComponent);
+export const ToolCallDisplay = memo(observer(ToolCallDisplayComponent));Also applies to: 12-12, 230-230
🏁 Script executed:
#!/bin/bash
# Check if ToolUIPart type contains MobX observable definitions
# Search for ToolUIPart type definition
ast-grep --pattern 'type ToolUIPart = $$$'
# Also search for interface definition
ast-grep --pattern 'interface ToolUIPart {
$$$
}'
# Check for MobX observable usage in types
rg -n --type=ts --type=tsx 'ToolUIPart' -A 10 -B 2Length of output: 27
🏁 Script executed:
#!/bin/bash
# Correct search for ToolUIPart type definition using proper ripgrep syntax
# Search for ToolUIPart type definition in TypeScript files
echo "=== Searching for ToolUIPart type/interface definition ==="
rg -n 'type ToolUIPart|interface ToolUIPart' -A 15
# Search for where ToolUIPart is imported/used in the tool-call-display file
echo -e "\n=== Checking imports in tool-call-display.tsx ==="
rg -n 'ToolUIPart' apps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/chat-messages/message-content/tool-call-display.tsx -B 5 -A 3
# Look for MobX observable patterns in ToolUIPart related files
echo -e "\n=== Checking for MobX observable patterns ==="
rg -n '@observable|makeObservable|observable\(' -A 3 -B 1Length of output: 2019
🏁 Script executed:
#!/bin/bash
# View the actual component implementation to check for MobX store/observable access
echo "=== Full tool-call-display.tsx component ==="
cat apps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/chat-messages/message-content/tool-call-display.tsx
echo -e "\n=== Check for store hooks or observable access patterns ==="
rg -n 'useStore|useObserver|observer\(|\.get\(\)|@observable' apps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/chat-messages/message-content/tool-call-display.tsxLength of output: 8589
Remove the unnecessary observer() wrapper—this component does not access any MobX observables.
The component only receives plain props (strings, boolean, ToolUIPart type from the ai package) and accesses standard property values without any MobX store hooks or observable patterns. Wrapping a component with observer() when it doesn't access observables adds unnecessary overhead without providing any benefit. The applied prop is passed to child components as a plain prop, not indicating MobX reactivity tracking in this component.
Remove the observer wrapper and import:
-import { observer } from 'mobx-react-lite';
import stripAnsi from 'strip-ansi';
-export const ToolCallDisplay = observer(ToolCallDisplayComponent);
+export const ToolCallDisplay = ToolCallDisplayComponent;Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
apps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/chat-messages/message-content/tool-call-display.tsx
around line 4, remove the unnecessary MobX observer usage: delete the import of
observer from 'mobx-react-lite' and remove the observer(...) wrapper around the
component export so the component is a plain React component; ensure any imports
or references to observer are removed and update the export/default export
accordingly to return the component directly.
| - uses: oven-sh/setup-bun@v1 | ||
| with: | ||
| bun-version: latest | ||
| bun-version: 1.2.21 |
There was a problem hiding this comment.
Bun version inconsistency: test job now pins version to 1.2.21 while typecheck job still uses latest. Consider aligning them for consistent CI builds.
saddlepaddle
force-pushed
the
fix-performance
branch
from
ef82d8c to
3930079
Compare
Description
Tried to remove a lot of excessive renders from the chat. Basically, when /any/ of the messages were changing all of the content was rerendering before, which is especially bad for the code blocks / etc. that are pricey. This is almost enough to be fully stable, but there's still a few dropped frames here and there while the code edit tool streams.
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Summary by CodeRabbit
Performance Improvements
Bug Fixes
Important
Optimize chat panel performance by reducing re-renders and improving state management using
memoandobserverpatterns.memoandobserverto optimize rendering inAssistantMessage,MessageContent,ToolCallDisplay,ToolCallSimple,UserMessage, andCollapsibleCodeBlock.CodeBlockComponentto reduce re-renders during streaming.bun-versionto1.2.23inci.ymlandpackage.json.@onlook/admindependency frombun.lockand update@types/nodeversion.This description was created by
for ef82d8c. You can customize this summary. It will automatically update as commits are pushed.