fix: small stream message changes #2849
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
Caution Review failedThe pull request is closed. WalkthroughRelaxes rendering gating and adds an early-return guard in the chat stream message component. Introduces a new multi-edit search/replace tool, implements atomic two-phase validation-and-apply edits in handlers, wires the tool into the registry, and adds unit tests covering single and multi-edit behaviors and failure modes. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant ChatTab
participant StreamMessage
participant MessageContent
User->>ChatTab: open chat / receive stream update
ChatTab->>StreamMessage: render(props: isWaiting, streamMessage, isAssistantStreamMessage)
alt isWaiting == false
StreamMessage-->>ChatTab: return null (no render)
else isWaiting == true
StreamMessage->>MessageContent: render assistant content
Note right of StreamMessage: loading indicator shown
end
sequenceDiagram
participant Caller
participant ToolsLayer as tools.ts
participant Handler as handleSearchReplaceMultiEditFileTool
participant FS as FileSystem
Caller->>ToolsLayer: invoke tool (multi-edit)
ToolsLayer->>Handler: call handler with params
Handler->>FS: readFile(path)
FS-->>Handler: originalContent
Handler->>Handler: Validation pass (validate each non-replace_all using originalContent)
alt any validation fails
Handler-->>ToolsLayer: throw error (no writes performed)
else validations pass
Handler->>Handler: Apply replace_all edits first, then non-replace_all edits
Handler->>FS: writeFile(path, finalContent)
FS-->>Handler: write result
Handler-->>ToolsLayer: return success result
end
ToolsLayer->>ToolsLayer: capture output (success or error.message)
ToolsLayer->>Caller: addToolResult(output)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/chat-messages/stream-message.tsx (1)
1-7: Add 'use client' — this component uses hooks and must be a Client Component.
useChatContextanduseMemorequire a client boundary in App Router. Add the directive at the top to avoid RSC build/runtime errors.+'use client'; import { useChatContext } from '@/app/project/[id]/_hooks/use-chat'; import { ChatMessageRole } from '@onlook/models/chat'; -import { useMemo } from 'react'; +import { useMemo } from 'react';
🧹 Nitpick comments (8)
apps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/chat-messages/stream-message.tsx (3)
31-34: Localize hardcoded UI text ("Thinking ...").Per guidelines, avoid hardcoded user-facing text in
apps/web/client/src/app/**/*.{ts,tsx}. Use next-intl.- <div className="flex w/full h/full flex-row items-center gap-2 px-4 my-2 text-small content-start text-foreground-secondary"> + <div className="flex w-full h-full flex-row items-center gap-2 px-4 my-2 text-small content-start text-foreground-secondary"> <Icons.LoadingSpinner className="animate-spin" /> - <p>Thinking ...</p> + <p>{t('thinking')}</p> </div>Add the import and hook (outside the changed hunk):
+'use client'; +import { useTranslations } from 'next-intl'; ... export const StreamMessage = () => { + const t = useTranslations('chat'); // ensure key chat.thinking exists
10-13: Drop unnecessary useMemo.This boolean check is trivial and doesn’t benefit from memoization; removing it simplifies the component.
-import { useMemo } from 'react'; ... - const isAssistantStreamMessage = useMemo(() => - streamMessage?.role === ChatMessageRole.ASSISTANT, - [streamMessage?.role] - ); + const isAssistantStreamMessage = streamMessage?.role === ChatMessageRole.ASSISTANT;
21-21: Verify MessageContent tolerates missing/empty parts.Condition no longer checks
parts. Ifpartscan beundefinedduring streaming, ensureMessageContenthandles it gracefully (e.g., optional chaining or default empty array), or add a lightweight guard.apps/web/client/src/components/tools/handlers/edit.ts (5)
26-28: Good shift to indexOf-based detection.This avoids split-allocation and is linear. Note the comment “single pass” is slightly misleading (two scans), but still an improvement.
32-35: Include the offending string in the single-edit error for consistency.Multi-edit already includes it. Mirror that here to improve diagnostics.
- if (secondIndex !== -1) { - throw new Error(`Multiple occurrences found. Use replace_all=true or provide more context.`); - } + if (secondIndex !== -1) { + throw new Error(`Multiple occurrences found for "${args.old_string}". Use replace_all=true or provide more context.`); + }
69-71: Overlapping occurrences aren’t detected.
indexOf(old, firstIndex + old.length)misses overlaps (e.g., old="aba", content="ababa"). If you need strict uniqueness, count with a lookahead or step-by-one scan.- const firstIndex = content.indexOf(edit.old_string); + const firstIndex = content.indexOf(edit.old_string); if (firstIndex === -1) { throw new Error(`String not found in file: ${edit.old_string}`); } - - const secondIndex = content.indexOf(edit.old_string, firstIndex + edit.old_string.length); - if (secondIndex !== -1) { + // Detect overlaps by advancing one char at a time + let foundAgain = false; + for (let i = firstIndex + 1; i < content.length; i++) { + if (content.indexOf(edit.old_string, i) === i) { + foundAgain = true; + break; + } + } + if (foundAgain) { throw new Error(`Multiple occurrences found for "${edit.old_string}". Use replace_all=true or provide more context.`); }If you prefer regex, add an
escapeRegExphelper outside this hunk and use a lookahead to count all (including overlaps).Also applies to: 74-77
37-37: Micro-opt: avoid an extra scan by slicing with known index.You already computed
firstIndex; constructing the new string via slices avoids an additionalreplacepass.- newContent = file.content.replace(args.old_string, args.new_string); + newContent = + file.content.slice(0, firstIndex) + + args.new_string + + file.content.slice(firstIndex + args.old_string.length);
64-81: Potential user-facing errors: localize if surfaced to UI.If these error messages bubble up to the client UI, route them through your i18n layer per guidelines for
apps/web/client/src/**/*.{ts,tsx}.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/chat-messages/stream-message.tsx(2 hunks)apps/web/client/src/components/tools/handlers/edit.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
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/components/tools/handlers/edit.tsapps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/chat-messages/stream-message.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Do not use the any type unless necessary
Files:
apps/web/client/src/components/tools/handlers/edit.tsapps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/chat-messages/stream-message.tsx
{apps,packages}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Avoid using the any type unless absolutely necessary
Files:
apps/web/client/src/components/tools/handlers/edit.tsapps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/chat-messages/stream-message.tsx
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/stream-message.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/stream-message.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/stream-message.tsx
🔇 Additional comments (1)
apps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/chat-messages/stream-message.tsx (1)
15-17: Early return is fine; confirm UX.Guarding on
!isWaitingavoids transient flicker. Verify that the final committed assistant message renders elsewhere when streaming completes.
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
| return `File ${args.file_path} edited with ${args.edits.length} changes`; | ||
| } catch (error) { | ||
| throw new Error(`Cannot multi-edit file ${args.file_path}: ${error}`); | ||
| throw new Error(`Cannot multi-edit file ${args.file_path}: ${(error as Error).message}`); |
There was a problem hiding this comment.
Consider standardizing error messaging. The multi-edit catch block uses (error as Error).message while the single-edit version interpolates the error directly.
| }); | ||
| output = await clientTool.handler(toolCall.input, editorEngine); | ||
| } catch (error) { | ||
| output = 'error handling tool call ' + error; |
There was a problem hiding this comment.
In the handleToolCall catch block, consider extracting the error message (e.g. using (error as Error).message) for clearer error reporting.
| output = 'error handling tool call ' + error; | |
| output = 'error handling tool call ' + ((error instanceof Error) ? error.message : error); |
1 participant
Description
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Important
Fixes chat panel loading indicator behavior and improves search-and-replace tool functionality with enhanced validation and error handling.
StreamMessageinstream-message.tsxnow only shows loading indicator whenisWaitingis true, preventing unnecessary placeholder content.handleToolCallintools.tsnow correctly handles tool call results, ensuring output is set in both success and error cases.handleSearchReplaceEditFileToolandhandleSearchReplaceMultiEditFileToolinedit.tsnow validate occurrences before applying changes, throwing errors for multiple occurrences unlessreplace_allis true.SEARCH_REPLACE_MULTI_EDIT_FILE_TOOL_NAMEand parameters totools.ts.edit.test.tsforhandleSearchReplaceEditFileToolandhandleSearchReplaceMultiEditFileTool, covering single and multiple occurrence cases, validation, and error handling.This description was created by
for 54b59ed. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
Bug Fixes
New Features
Tests