Merged
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughBuffers assistant "text" events and batches compatible text deltas with a 100ms flush; flushes on lifecycle or certain incoming events. Adds batching helpers and tests. Also updates composer (advanced settings), message list (grouping/navigation/coalescing), subagent strip UI, mission-thread test, and a permission label tweak. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (5)
apps/desktop/src/renderer/components/missions/MissionThreadMessageList.test.ts (1)
140-172: Strengthen this test so it truly enforces turn-based merging.At Line 140 and Line 162, both fragments use
itemId: "text-1", so this can still pass if merge behavior regresses toward item-based grouping. Also, themerged.some(...)check at Line 168-172 is permissive and won’t fail if duplicate text envelopes are emitted.Suggested test tightening
@@ { sessionId: "session-1", timestamp: "2026-03-10T12:00:02.000Z", event: { type: "text", turnId: "turn-7", - itemId: "text-1", + itemId: "text-2", text: " with quieter scrolling", }, }, ]); - expect( - merged.some((envelope) => - envelope.event.type === "text" && envelope.event.text === "Grouped command cards with quieter scrolling", - ), - ).toBe(true); + const textEvents = merged.filter((envelope) => envelope.event.type === "text"); + expect(textEvents).toHaveLength(1); + expect(textEvents[0]?.event).toMatchObject({ + type: "text", + turnId: "turn-7", + text: "Grouped command cards with quieter scrolling", + });Based on learnings: Mission chat uses structured message records and runtime IDs; worker and orchestrator channels reuse shared chat message renderer patterns.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/missions/MissionThreadMessageList.test.ts` around lines 140 - 172, The test currently can pass via item-based grouping because both fragments reuse itemId "text-1" and the assertion is permissive; update the fixture fragments so each text fragment uses a unique itemId (e.g., "text-a" and "text-b") but keep the same turnId ("turn-7") for fragments that should merge, then tighten assertions on the merged array: assert there is exactly one envelope where envelope.event.type === "text" and envelope.event.text === "Grouped command cards with quieter scrolling" (e.g., check merged.filter(e => e.event.type === "text" && e.event.text === ...).length === 1) and optionally assert that this envelope.event.turnId === "turn-7" and no other text envelopes contain the combined string; adjust the test data and the checks around the merged variable to enforce turn-based merging rather than itemId-based grouping.apps/desktop/src/renderer/components/chat/ChatSubagentStrip.tsx (1)
289-294: Consider hiding "+X more" when expanded.The "+X more" indicator remains visible even when expanded and all items are shown. This could be slightly confusing since the "more" items are already visible.
♻️ Optional: Hide indicator when expanded
- {snapshots.length > 6 ? ( + {!expanded && snapshots.length > 6 ? ( <span className="font-mono text-[9px] text-fg/25"> +{snapshots.length - 6} more </span> ) : null}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/chat/ChatSubagentStrip.tsx` around lines 289 - 294, The "+X more" indicator in ChatSubagentStrip remains visible even when the strip is expanded; update the conditional rendering that uses snapshots.length and expanded so the span only renders when snapshots.length > 6 AND expanded is false (i.e., !expanded). Locate the JSX around the snapshots check in ChatSubagentStrip (the span showing +{snapshots.length - 6}) and add the expanded guard so it hides whenever the expanded state is true; keep ArrowsOutSimple and its expanded-based rotation logic unchanged.apps/desktop/src/renderer/components/chat/AgentChatMessageList.test.tsx (3)
231-259: Add a Windows absolute-path case here.This only exercises a POSIX workspace root. A
C:\\...fixture would catch separator/prefix regressions in the file-link mapping before they reach Windows users.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/chat/AgentChatMessageList.test.tsx` around lines 231 - 259, Add a second assertion in the same test to cover a Windows absolute-path: when calling renderMessageList (the test within AgentChatMessageList.test.tsx that maps absolute workspace file references) include a message event whose text contains a Windows-style path (e.g. starting with "C:\\") so the code path in the file-link mapping that normalizes separators/prefixes is exercised; then mirror the existing checks (waitFor the globalThis.window.ade.files.listWorkspaces call and fireEvent.click on the button whose accessible name is the Windows path) to ensure the Windows case resolves to a Files navigation target.
226-228: Avoid exactJSON.stringify(location.state)comparisons.These checks are order-sensitive. A harmless refactor that builds the same state object with a different key insertion order will fail the suite even though navigation behavior is unchanged.
Also applies to: 260-262, 451-452, 600-602
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/chat/AgentChatMessageList.test.tsx` around lines 226 - 228, The test is comparing JSON string output literally which is order-sensitive; instead, extract the JSON part from screen.getByTestId("location").textContent (strip the "/files::" prefix), parse it via JSON.parse, and assert against the expected object using deep equality (e.g., expect(parsed).toEqual({...}) or expect(parsed).toMatchObject({...})), and apply the same change to the other literal JSON-string assertions in this test file that use getByTestId("location") (the similar checks around the other occurrences flagged).
347-395: Replace Tailwind CSS class assertions with accessible queries.The tests at both locations check for specific Tailwind classes using
innerHTMLandquerySelector, which are fragile to harmless style updates. Testing Library guidelines recommend asserting user-visible behavior instead of implementation details.Lines 347–395 searches
innerHTMLformin-h-[5.5rem]; lines 454–476 usesquerySelector(".bg-amber-400\\/85"). Instead, usedata-testidattributes on the reserved live-bubble space and needs-input indicator, or leverage existing accessible markers likescreen.getByRole()orscreen.getByText()(which the test already uses elsewhere for "Needs Input").🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/chat/AgentChatMessageList.test.tsx` around lines 347 - 395, The test "keeps the live assistant bubble stable until the turn finishes" currently asserts Tailwind classes via live.container.innerHTML (and elsewhere via querySelector for the needs-input indicator); replace these fragile implementation checks by querying for user-facing/accessibility hooks instead: update the rendered component (used by renderMessageList) to add data-testid attributes (e.g. data-testid="live-bubble-placeholder" for the reserved live-bubble space and data-testid="needs-input-indicator" or ensure an accessible label for the "Needs Input" element), then change the assertions to use screen.getByTestId('live-bubble-placeholder') / expect(...).toBeInTheDocument() when streaming and expect(queryByTestId('live-bubble-placeholder')).toBeNull() after the done event (and similarly assert the needs-input indicator via getByTestId or getByRole/getByText). Ensure you update references to renderMessageList and the test block named exactly as above.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/main/services/chat/agentChatService.ts`:
- Around line 1955-1974: The new queueBufferedTextEvent postpones committing
assistant text into the transcript, so snapshot readers like getChatTranscript
and the conversation-tail builders (which read committed entries /
recentConversationEntries) can miss the in-flight assistant chunk; fix by
ensuring snapshot reads include managed.bufferedText or by flushing buffered
text before producing snapshots—specifically, update getChatTranscript and any
conversation-tail builder code to merge managed.bufferedText (from the
ManagedChatSession) into the returned transcript/recentConversationEntries when
present, or call flushBufferedText(managed) at the start of those read paths so
the pending chunk is committed; reference queueBufferedTextEvent,
managed.bufferedText, flushBufferedText, scheduleBufferedTextFlush,
getChatTranscript, and recentConversationEntries when making the change.
In `@apps/desktop/src/main/services/chat/chatTextBatching.ts`:
- Around line 13-15: The equality check in the chat batching logic (comparing
buffered and event) incorrectly treats two anonymous chunks as the same; update
the logic in chatTextBatching.ts (the block using buffered and event, comparing
turnId and itemId) to return false when both identities are absent. Concretely,
ensure you first detect if (buffered.turnId ?? buffered.itemId) is nullish or
(event.turnId ?? event.itemId) is nullish and return false in that case,
otherwise perform the existing equality checks on turnId and itemId.
- Around line 42-49: The listed event types should not be in the non-flush
allowlist; remove `todo_update`, `subagent_started`, `subagent_progress`,
`subagent_result`, `tool_use_summary`, `web_search`, and `auto_approval_review`
from the switch branch that returns false in chatTextBatching (the function that
decides flush behavior), so they no longer short-circuit buffered assistant
prose; either delete those case entries or change their behavior to return true
(so they flush like `tool_call`, `command`, and `approval_request`) ensuring
transcript structural events flush pending prose before emitting.
In `@apps/desktop/src/renderer/components/chat/AgentChatComposer.tsx`:
- Around line 270-299: The Computer use control is a toggle but lacks an
accessibility state; update the button that uses computerUseAllowed and
onToggleComputerUse to include aria-pressed={computerUseAllowed} so assistive
tech can announce On/Off, and apply the same change to the corresponding
buttons/components that control Proof (use the boolean backing prop, e.g.,
computerUsePolicy.retainArtifacts) and Project Context (the boolean state used
in that block) so each toggle's element includes aria-pressed bound to its
boolean state and still calls its existing handler (e.g., onToggleComputerUse)
when activated.
- Around line 704-707: The Escape handler currently only closes the advanced
menu (onKeyDown -> setAdvancedMenuOpen(false)) but doesn't restore keyboard
focus; update the onKeyDown handler so that when event.key === "Escape" you both
call setAdvancedMenuOpen(false) and move focus back to the trigger by calling
advancedButtonRef.current?.focus() (guarding that the ref exists). Ensure the
focus call runs after closing (e.g., directly after setAdvancedMenuOpen or
scheduled with a microtask/requestAnimationFrame if needed) so keyboard users
regain a stable focus target.
In `@apps/desktop/src/renderer/components/chat/AgentChatMessageList.test.tsx`:
- Around line 40-64: The test leaks an own property when originalAde is
undefined; update the afterEach cleanup for globalThis.window.ade (referencing
originalAde and globalThis.window.ade in the afterEach block) to remove the
property when originalAde is undefined (delete globalThis.window.ade) and
otherwise restore the original object (globalThis.window.ade = originalAde) so
the window shape is fully restored for other suites.
- Around line 11-17: The LocationProbe (used in AgentChatMessageList.test.tsx)
currently renders only location.pathname and location.state, causing assertions
that expect query strings (e.g., /work?sessionId=chat-1) to fail; update the
LocationProbe rendering to include location.search (e.g., render pathname +
location.search) alongside location.state so tests that assert full URLs
including query parameters pass; locate the JSX in the LocationProbe component
where {location.pathname} is rendered and add {location.search} in the output
string.
In `@apps/desktop/src/renderer/components/chat/AgentChatMessageList.tsx`:
- Around line 773-780: The code currently allows "../" and "~/" in
looksLikeWorkspacePath but resolveFilesNavigationTarget only strips "./",
enabling path-traversal into openFilePath; update the logic so that
looksLikeWorkspacePath and resolveFilesNavigationTarget explicitly reject any
candidate starting with "../" or "~/" (and any other parent-traversal patterns)
and only allow "./", absolute workspace-root-relative paths, or safe relative
paths without upward traversal; change checks in looksLikeWorkspacePath and the
corresponding normalization in resolveFilesNavigationTarget (also adjust the
similar logic around the other occurrence at lines referenced 876-881) so
navigation to /files is only performed for safe, non-traversing paths before
calling openFilePath.
In `@apps/desktop/src/renderer/components/chat/AgentChatPane.tsx`:
- Around line 540-551: The assistantLabel derivation uses
selectedSession.provider and raw model family (selectedModelDesc) which can
drift from the effective provider; update assistantLabel to derive from the
effective provider variable (sessionProvider) and the normalized model
descriptor (e.g., selectedModelDesc.cliCommand or family only as a fallback) so
labels match the actual runtime model, then recompute messagePlaceholder from
that assistantLabel; change references in the assistantLabel computation
(notably the branches that check selectedSession?.provider and
selectedModelDesc?.family) to use sessionProvider and normalized model info so
the placeholder and transcript label remain aligned with the active provider.
In `@apps/desktop/src/renderer/components/chat/ChatSubagentStrip.tsx`:
- Around line 314-328: The task row button in ChatSubagentStrip.tsx uses
onMouseEnter/onMouseLeave to toggle previews but lacks keyboard equivalents; add
onFocus and onBlur handlers that call setHoveredTaskId(snapshot.taskId) and
clear it (matching the onMouseLeave behavior using setHoveredTaskId((cur) =>
(cur === snapshot.taskId ? null : cur))) so Tab/keyboard users trigger the same
preview behavior as mouse users; ensure these handlers are added alongside the
existing onMouseEnter/onMouseLeave on the button that uses snapshot.taskId and
setPinnedTaskId.
---
Nitpick comments:
In `@apps/desktop/src/renderer/components/chat/AgentChatMessageList.test.tsx`:
- Around line 231-259: Add a second assertion in the same test to cover a
Windows absolute-path: when calling renderMessageList (the test within
AgentChatMessageList.test.tsx that maps absolute workspace file references)
include a message event whose text contains a Windows-style path (e.g. starting
with "C:\\") so the code path in the file-link mapping that normalizes
separators/prefixes is exercised; then mirror the existing checks (waitFor the
globalThis.window.ade.files.listWorkspaces call and fireEvent.click on the
button whose accessible name is the Windows path) to ensure the Windows case
resolves to a Files navigation target.
- Around line 226-228: The test is comparing JSON string output literally which
is order-sensitive; instead, extract the JSON part from
screen.getByTestId("location").textContent (strip the "/files::" prefix), parse
it via JSON.parse, and assert against the expected object using deep equality
(e.g., expect(parsed).toEqual({...}) or expect(parsed).toMatchObject({...})),
and apply the same change to the other literal JSON-string assertions in this
test file that use getByTestId("location") (the similar checks around the other
occurrences flagged).
- Around line 347-395: The test "keeps the live assistant bubble stable until
the turn finishes" currently asserts Tailwind classes via
live.container.innerHTML (and elsewhere via querySelector for the needs-input
indicator); replace these fragile implementation checks by querying for
user-facing/accessibility hooks instead: update the rendered component (used by
renderMessageList) to add data-testid attributes (e.g.
data-testid="live-bubble-placeholder" for the reserved live-bubble space and
data-testid="needs-input-indicator" or ensure an accessible label for the "Needs
Input" element), then change the assertions to use
screen.getByTestId('live-bubble-placeholder') / expect(...).toBeInTheDocument()
when streaming and expect(queryByTestId('live-bubble-placeholder')).toBeNull()
after the done event (and similarly assert the needs-input indicator via
getByTestId or getByRole/getByText). Ensure you update references to
renderMessageList and the test block named exactly as above.
In `@apps/desktop/src/renderer/components/chat/ChatSubagentStrip.tsx`:
- Around line 289-294: The "+X more" indicator in ChatSubagentStrip remains
visible even when the strip is expanded; update the conditional rendering that
uses snapshots.length and expanded so the span only renders when
snapshots.length > 6 AND expanded is false (i.e., !expanded). Locate the JSX
around the snapshots check in ChatSubagentStrip (the span showing
+{snapshots.length - 6}) and add the expanded guard so it hides whenever the
expanded state is true; keep ArrowsOutSimple and its expanded-based rotation
logic unchanged.
In
`@apps/desktop/src/renderer/components/missions/MissionThreadMessageList.test.ts`:
- Around line 140-172: The test currently can pass via item-based grouping
because both fragments reuse itemId "text-1" and the assertion is permissive;
update the fixture fragments so each text fragment uses a unique itemId (e.g.,
"text-a" and "text-b") but keep the same turnId ("turn-7") for fragments that
should merge, then tighten assertions on the merged array: assert there is
exactly one envelope where envelope.event.type === "text" and
envelope.event.text === "Grouped command cards with quieter scrolling" (e.g.,
check merged.filter(e => e.event.type === "text" && e.event.text === ...).length
=== 1) and optionally assert that this envelope.event.turnId === "turn-7" and no
other text envelopes contain the combined string; adjust the test data and the
checks around the merged variable to enforce turn-based merging rather than
itemId-based grouping.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 99aeb6b4-9218-4e2e-96a4-f2201b141ec3
📒 Files selected for processing (12)
apps/desktop/src/main/services/chat/agentChatService.tsapps/desktop/src/main/services/chat/chatTextBatching.test.tsapps/desktop/src/main/services/chat/chatTextBatching.tsapps/desktop/src/renderer/components/chat/AgentChatComposer.test.tsxapps/desktop/src/renderer/components/chat/AgentChatComposer.tsxapps/desktop/src/renderer/components/chat/AgentChatMessageList.test.tsxapps/desktop/src/renderer/components/chat/AgentChatMessageList.tsxapps/desktop/src/renderer/components/chat/AgentChatPane.tsxapps/desktop/src/renderer/components/chat/ChatSubagentStrip.test.tsxapps/desktop/src/renderer/components/chat/ChatSubagentStrip.tsxapps/desktop/src/renderer/components/missions/MissionThreadMessageList.test.tsapps/desktop/src/renderer/components/shared/permissionOptions.ts
apps/desktop/src/renderer/components/chat/AgentChatMessageList.tsx
Outdated
Show resolved
Hide resolved
Comment on lines
+540
to
+551
| const assistantLabel = presentation?.assistantLabel?.trim() | ||
| || (selectedSession?.provider === "claude" | ||
| ? "Claude" | ||
| : selectedSession?.provider === "codex" | ||
| ? "Codex" | ||
| : selectedModelDesc?.family === "anthropic" || selectedModelDesc?.cliCommand === "claude" | ||
| ? "Claude" | ||
| : selectedModelDesc?.family === "openai" || selectedModelDesc?.cliCommand === "codex" | ||
| ? "Codex" | ||
| : "Assistant"); | ||
| const messagePlaceholder = presentation?.messagePlaceholder?.trim() | ||
| || (assistantLabel === "Agent" ? "Message the agent..." : `Message ${assistantLabel}...`); | ||
| || (assistantLabel === "Assistant" ? "Message the assistant..." : `Message ${assistantLabel}...`); |
There was a problem hiding this comment.
Derive assistantLabel from the effective provider, not selectedSession.provider / raw family.
This can drift from the model that will actually run: selectedSession.provider still wins while an empty session is being switched to another model, and selectedModelDesc?.family === "openai" also labels unified OpenAI API chats as "Codex". Reusing sessionProvider here keeps the placeholder and transcript label aligned.
🎯 Suggested simplification
- const assistantLabel = presentation?.assistantLabel?.trim()
- || (selectedSession?.provider === "claude"
- ? "Claude"
- : selectedSession?.provider === "codex"
- ? "Codex"
- : selectedModelDesc?.family === "anthropic" || selectedModelDesc?.cliCommand === "claude"
- ? "Claude"
- : selectedModelDesc?.family === "openai" || selectedModelDesc?.cliCommand === "codex"
- ? "Codex"
- : "Assistant");
+ const assistantLabel = presentation?.assistantLabel?.trim()
+ || (sessionProvider === "claude"
+ ? "Claude"
+ : sessionProvider === "codex"
+ ? "Codex"
+ : "Assistant");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const assistantLabel = presentation?.assistantLabel?.trim() | |
| || (selectedSession?.provider === "claude" | |
| ? "Claude" | |
| : selectedSession?.provider === "codex" | |
| ? "Codex" | |
| : selectedModelDesc?.family === "anthropic" || selectedModelDesc?.cliCommand === "claude" | |
| ? "Claude" | |
| : selectedModelDesc?.family === "openai" || selectedModelDesc?.cliCommand === "codex" | |
| ? "Codex" | |
| : "Assistant"); | |
| const messagePlaceholder = presentation?.messagePlaceholder?.trim() | |
| || (assistantLabel === "Agent" ? "Message the agent..." : `Message ${assistantLabel}...`); | |
| || (assistantLabel === "Assistant" ? "Message the assistant..." : `Message ${assistantLabel}...`); | |
| const assistantLabel = presentation?.assistantLabel?.trim() | |
| || (sessionProvider === "claude" | |
| ? "Claude" | |
| : sessionProvider === "codex" | |
| ? "Codex" | |
| : "Assistant"); | |
| const messagePlaceholder = presentation?.messagePlaceholder?.trim() | |
| || (assistantLabel === "Assistant" ? "Message the assistant..." : `Message ${assistantLabel}...`); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/renderer/components/chat/AgentChatPane.tsx` around lines 540
- 551, The assistantLabel derivation uses selectedSession.provider and raw model
family (selectedModelDesc) which can drift from the effective provider; update
assistantLabel to derive from the effective provider variable (sessionProvider)
and the normalized model descriptor (e.g., selectedModelDesc.cliCommand or
family only as a fallback) so labels match the actual runtime model, then
recompute messagePlaceholder from that assistantLabel; change references in the
assistantLabel computation (notably the branches that check
selectedSession?.provider and selectedModelDesc?.family) to use sessionProvider
and normalized model info so the placeholder and transcript label remain aligned
with the active provider.
- Fix typecheck errors: waitFor import, toBeInTheDocument matcher, executionMode/currentLaneId nullability - Fix test failures: add location.search to LocationProbe, add afterEach(cleanup) to SubagentStrip and Composer tests, fix Computer use button accessible name matcher - Reject directory-traversal (../) and home-relative (~/) paths in workspace path resolution - Flush buffered text before transcript reads so snapshots include pending assistant content - Don't collapse anonymous text chunks lacking turnId/itemId identity - Remove discrete UI card events (todo_update, subagent_started, etc.) from the non-flush allowlist so they properly flush buffered prose - Add aria-pressed to Computer use, Proof, and Project Context toggles - Return focus to Advanced button trigger on Escape dismiss - Add onFocus/onBlur handlers to SubagentStrip task rows for keyboard accessibility - Derive assistantLabel from model descriptor before session provider to avoid stale provider drift Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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 (1)
apps/desktop/src/renderer/components/chat/AgentChatMessageList.tsx (1)
2134-2165:⚠️ Potential issue | 🟠 MajorKeep ask-user tool normalization shared with
AgentChatPane.This branch now treats
askuser/ask_useras modal-backed questions, butextractAskUserQuestion()inapps/desktop/src/renderer/components/chat/AgentChatPane.tsxstill only acceptsaskUser. For those variants this card hides the approval buttons and tells the user to answer in the modal, yet the modal never appears.
♻️ Duplicate comments (3)
apps/desktop/src/main/services/chat/agentChatService.ts (1)
1247-1257:⚠️ Potential issue | 🟠 MajorThis snapshot can still miss the newest assistant chunk.
flushBufferedText()only commits the event in memory; the transcript append still happens asynchronously viafs.promises.appendFile. Because this path then prefers any non-empty on-disk transcript overrecentConversationEntries, an active session that already has its earlieruser_messagerow can still return without the just-flushed text. Please merge the live in-memory entries into this read path, or make the flush await durable transcript writes, and add a regression test for the “existing transcript + pending buffered text” case.Please verify with a regression test that seeds one transcript row, buffers assistant text, calls
getChatTranscript()immediately, and asserts the returned entries already contain that text before the timer fires.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/chat/agentChatService.ts` around lines 1247 - 1257, The current getChatTranscript path can miss the most-recent assistant chunk because flushBufferedText(managed) only commits in-memory and appends to disk asynchronously; update getChatTranscript (the code using flushBufferedText, readTranscriptEntries, and managed.recentConversationEntries) to include the live in-memory buffered assistant chunks into the transcript merge (or alternatively make flushBufferedText await durable fs.promises.appendFile completion) so the returned transcript always contains the just-flushed text; also add a regression test that seeds one transcript row, buffers assistant text, calls getChatTranscript() immediately, and asserts the returned entries include the buffered text before any timer-based write completes.apps/desktop/src/renderer/components/chat/AgentChatMessageList.tsx (1)
770-783:⚠️ Potential issue | 🟠 MajorReject parent-traversal segments anywhere in the candidate path.
The new prefix check blocks
../foo, butfoo/../../secret.envand/workspace/app/../secret.envstill survive both helpers and get forwarded intoopenFilePath. That leaves the same cross-workspace traversal vector open; reject any normalized segment equal to..before navigating to/files.As per coding guidelines,
apps/desktop/src/**: Electron desktop app — check for IPC security, proper main/renderer process separation, and React best practices.Also applies to: 845-885
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/chat/AgentChatMessageList.tsx` around lines 770 - 783, The current looksLikeWorkspacePath allows paths with parent-traversal segments elsewhere; after calling normalizeWorkspacePathCandidate(value) in looksLikeWorkspacePath, split the normalized candidate on '/' and immediately return false if any segment === ".." (and optionally reject empty segments or "." if desired), so any path containing parent-traversal is rejected before continuing to the existing checks; apply the same defensive check wherever paths are validated before calling openFilePath (e.g., the caller that forwards to openFilePath) to ensure ../ segments are blocked end-to-end.apps/desktop/src/renderer/components/chat/AgentChatPane.tsx (1)
540-551:⚠️ Potential issue | 🟡 Minor
assistantLabelstill maps unified OpenAI models toCodex.
selectedModelDesc?.family === "openai"wins before the effectivesessionProvider, so unified/OpenAI chats and some pre-launch model switches can still renderCodexin the placeholder/transcript even when the runtime is not Codex. Derive this fromsessionProviderinstead, or at least gate theCodexlabel behindisCliWrapped.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/chat/AgentChatPane.tsx` around lines 540 - 551, The assistantLabel logic in AgentChatPane (the assistantLabel const and therefore messagePlaceholder) incorrectly maps unified OpenAI models to "Codex" because it checks selectedModelDesc?.family === "openai" before considering the runtime/session provider; update the assistantLabel computation to prefer selectedSession?.provider (or check isCliWrapped) when deciding "Codex" vs "Codex" runtime labels so that unified/OpenAI models only render "Codex" when the session/provider indicates Codex (or when isCliWrapped is true), and ensure messagePlaceholder continues to use the corrected assistantLabel.
🧹 Nitpick comments (1)
apps/desktop/src/renderer/components/chat/AgentChatComposer.test.tsx (1)
85-87: Make the permission-label assertion order-independent.The current check is correct, but indexing
[0]is brittle if option order changes later.♻️ Suggested refactor
- expect(getPermissionOptions({ family: "openai", isCliWrapped: true })[0]?.label).toBe("Plan"); + const options = getPermissionOptions({ family: "openai", isCliWrapped: true }); + const planOption = options.find((opt) => opt.value === "plan"); + expect(planOption?.label).toBe("Plan");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/chat/AgentChatComposer.test.tsx` around lines 85 - 87, The test is brittle by assuming the "Plan" label is at index 0 in getPermissionOptions({ family: "openai", isCliWrapped: true }) — make the assertion order-independent by retrieving the options from getPermissionOptions and asserting that at least one option has label "Plan" (e.g., check labels include "Plan" or that some(option => option.label === "Plan")). Update the assertion in AgentChatComposer.test.tsx to use that membership check instead of indexing [0].
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/renderer/components/chat/AgentChatMessageList.tsx`:
- Around line 759-767: normalizeWorkspacePathCandidate fails to treat Windows
absolute paths as absolute (e.g. C:\repo or file:///C:/repo) so workspace-root
matching breaks; update normalizeWorkspacePathCandidate to detect and normalize
Windows drive-letter paths before returning: after computing withoutQuery,
replace backslashes with forward slashes, if withoutQuery matches
/^\/?[A-Za-z]:[\\/]/ then strip a leading slash if present (turn "/C:/..." into
"C:/..."), and return that normalized path so FilesWorkspace.rootPath
comparisons (and normalizeFileSystemPath behavior) will match on Windows; keep
existing URL/mailto/# checks and null returns.
- Around line 794-833: The component InlineDisclosureRow currently initializes
its local state with useState(defaultOpen) so subsequent changes to the
defaultOpen prop (e.g., when a task fails) are ignored; update the component to
respond to prop changes by adding a useEffect that calls setOpen(defaultOpen)
whenever defaultOpen changes (optionally gated by expandable) so the row
auto-expands/collapses when the incoming defaultOpen prop updates; refer to
InlineDisclosureRow, the open state and setOpen setter to implement this sync.
---
Duplicate comments:
In `@apps/desktop/src/main/services/chat/agentChatService.ts`:
- Around line 1247-1257: The current getChatTranscript path can miss the
most-recent assistant chunk because flushBufferedText(managed) only commits
in-memory and appends to disk asynchronously; update getChatTranscript (the code
using flushBufferedText, readTranscriptEntries, and
managed.recentConversationEntries) to include the live in-memory buffered
assistant chunks into the transcript merge (or alternatively make
flushBufferedText await durable fs.promises.appendFile completion) so the
returned transcript always contains the just-flushed text; also add a regression
test that seeds one transcript row, buffers assistant text, calls
getChatTranscript() immediately, and asserts the returned entries include the
buffered text before any timer-based write completes.
In `@apps/desktop/src/renderer/components/chat/AgentChatMessageList.tsx`:
- Around line 770-783: The current looksLikeWorkspacePath allows paths with
parent-traversal segments elsewhere; after calling
normalizeWorkspacePathCandidate(value) in looksLikeWorkspacePath, split the
normalized candidate on '/' and immediately return false if any segment === ".."
(and optionally reject empty segments or "." if desired), so any path containing
parent-traversal is rejected before continuing to the existing checks; apply the
same defensive check wherever paths are validated before calling openFilePath
(e.g., the caller that forwards to openFilePath) to ensure ../ segments are
blocked end-to-end.
In `@apps/desktop/src/renderer/components/chat/AgentChatPane.tsx`:
- Around line 540-551: The assistantLabel logic in AgentChatPane (the
assistantLabel const and therefore messagePlaceholder) incorrectly maps unified
OpenAI models to "Codex" because it checks selectedModelDesc?.family ===
"openai" before considering the runtime/session provider; update the
assistantLabel computation to prefer selectedSession?.provider (or check
isCliWrapped) when deciding "Codex" vs "Codex" runtime labels so that
unified/OpenAI models only render "Codex" when the session/provider indicates
Codex (or when isCliWrapped is true), and ensure messagePlaceholder continues to
use the corrected assistantLabel.
---
Nitpick comments:
In `@apps/desktop/src/renderer/components/chat/AgentChatComposer.test.tsx`:
- Around line 85-87: The test is brittle by assuming the "Plan" label is at
index 0 in getPermissionOptions({ family: "openai", isCliWrapped: true }) — make
the assertion order-independent by retrieving the options from
getPermissionOptions and asserting that at least one option has label "Plan"
(e.g., check labels include "Plan" or that some(option => option.label ===
"Plan")). Update the assertion in AgentChatComposer.test.tsx to use that
membership check instead of indexing [0].
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6304037e-b1ea-4e1f-9c5f-5d0456f94cf0
📒 Files selected for processing (10)
apps/desktop/src/main/services/chat/agentChatService.tsapps/desktop/src/main/services/chat/chatTextBatching.test.tsapps/desktop/src/main/services/chat/chatTextBatching.tsapps/desktop/src/renderer/components/chat/AgentChatComposer.test.tsxapps/desktop/src/renderer/components/chat/AgentChatComposer.tsxapps/desktop/src/renderer/components/chat/AgentChatMessageList.test.tsxapps/desktop/src/renderer/components/chat/AgentChatMessageList.tsxapps/desktop/src/renderer/components/chat/AgentChatPane.tsxapps/desktop/src/renderer/components/chat/ChatSubagentStrip.test.tsxapps/desktop/src/renderer/components/chat/ChatSubagentStrip.tsx
✅ Files skipped from review due to trivial changes (2)
- apps/desktop/src/main/services/chat/chatTextBatching.test.ts
- apps/desktop/src/renderer/components/chat/ChatSubagentStrip.test.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/desktop/src/main/services/chat/chatTextBatching.ts
- apps/desktop/src/renderer/components/chat/AgentChatMessageList.test.tsx
- apps/desktop/src/renderer/components/chat/AgentChatComposer.tsx
- apps/desktop/src/renderer/components/chat/ChatSubagentStrip.tsx
- Fix 5 transcript rendering tests: use getAllByText for elements that appear in both summary and detail views, add text event to activity test so streaming indicator renders, use body textContent for diff assertions - Handle Windows drive-letter paths in workspace path resolution by normalizing backslashes and stripping leading slash from /C:/... - Make InlineDisclosureRow react to defaultOpen prop changes so rows auto-expand when status changes (e.g. task failure) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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 (1)
apps/desktop/src/renderer/components/chat/AgentChatMessageList.tsx (1)
3044-3057:⚠️ Potential issue | 🟠 MajorThe empty-state masks live activity-only turns.
appendCollapsedEvent()now dropsactivityrows, so a turn that currently has only activity events still makesrows.length === 0on Line 3065. This branch renders the empty-state and suppresses the already computedstreamingIndicator, which makes an active turn look idle until some other visible row arrives.Suggested fix
- {rows.length === 0 ? ( + {rows.length === 0 && !streamingIndicator && !turnSummaryCard ? (Also applies to: 3065-3077
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/chat/AgentChatMessageList.tsx` around lines 3044 - 3057, The empty-state branch is hiding active turns because appendCollapsedEvent drops activity rows so rows.length === 0 even when there is a live activity; update the rendering logic in AgentChatMessageList so the empty-state is not chosen when there is an active streaming indicator or latestActivity. Concretely, change the condition that decides to render the empty-state (where rows.length === 0 is currently used) to also check showStreamingIndicator || latestActivity (or otherwise prefer rendering streamingIndicator/turnSummaryCard), so streamingIndicator (and TurnSummaryCard when present) are rendered for activity-only turns instead of the idle empty-state; use the existing symbols streamingIndicator, latestActivity, showStreamingIndicator, turnSummary, and appendCollapsedEvent to locate and adjust the logic.
♻️ Duplicate comments (2)
apps/desktop/src/renderer/components/chat/AgentChatMessageList.tsx (2)
775-780:⚠️ Potential issue | 🟠 MajorReject parent-traversal segments before building
openFilePath.Line 775 currently accepts any
./prefix, so values like./../secret.envare rendered as safe file links. The resolver then strips only the leading"./"and pushes"../secret.env"into/filesstate; embedded segments likefoo/../../bar.tssurvive for the same reason. Please reject..segments after normalization in both the detector and the resolver.As per coding guidelines,
apps/desktop/src/**: Electron desktop app — check for IPC security, proper main/renderer process separation, and React best practices.Also applies to: 863-895
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/chat/AgentChatMessageList.tsx` around lines 775 - 780, The current detector accepts any candidate starting with "./" and the resolver strips only the leading "./", allowing embedded ".." segments through; update both the detector (where candidate is checked) and the resolver (where openFilePath or the file path is constructed and pushed into files state) to normalize the path and reject any path containing parent-traversal segments. Concretely: for the candidate check, run a POSIX-normalize (or split/join) on candidate after removing a single leading "./", then if the normalized path contains any ".." path segments or begins with "../" reject it (return false); likewise, in the resolver before calling setFiles/openFilePath push, normalize the constructed path and if any ".." segments remain, abort and do not add the path. Use the existing variable names candidate, openFilePath and the files state update to locate the places to change.
767-769:⚠️ Potential issue | 🟠 MajorDrive-letter paths still miss workspace-root matching.
After Line 768,
file:///C:/repo/src/foo.tsbecomesC:/repo/src/foo.ts, but Line 864 only treats leading/as absolute. Those clicks fall through to the relative-path branch and navigate withopenFilePath: "C:/..."instead of matchingFilesWorkspace.rootPath, so the new workspace-link flow still breaks on Windows.As per coding guidelines,
apps/desktop/src/**: Electron desktop app — check for IPC security, proper main/renderer process separation, and React best practices.Also applies to: 863-890
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/chat/AgentChatMessageList.tsx` around lines 767 - 769, The Windows drive-letter normalization (in AgentChatMessageList where withoutQuery is sliced for /^\/[A-Za-z]:\//) causes paths like C:/repo/... to miss the absolute-path check later and be treated as relative; update the absolute-path detection (the branch around the code that checks for a leading '/' and sets openFilePath vs. workspace match) to also recognize Windows drive-letter prefixes (e.g. /^[A-Za-z]:[\\/]/) by normalizing both the candidate path and FilesWorkspace.rootPath (or comparing path.win32.normalize equivalents) before matching, so drive-letter paths correctly match FilesWorkspace.rootPath and route to the workspace-link flow rather than the relative-path branch.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/renderer/components/chat/AgentChatMessageList.tsx`:
- Around line 820-840: The expand/collapse button in AgentChatMessageList does
not expose its state to assistive tech; add aria-expanded and aria-controls to
the button and an id on the collapsible panel so screen readers can detect
open/closed state. Generate a stable unique id for the panel (e.g., via React's
useId or a component-level id like panelId), set aria-controls={panelId} on the
button and aria-expanded={open} (or aria-expanded={expandable ? open :
undefined}) on the button, and set id={panelId} on the div that wraps
{children}; keep aria-controls/aria-expanded only when expandable to avoid
misleading semantics.
---
Outside diff comments:
In `@apps/desktop/src/renderer/components/chat/AgentChatMessageList.tsx`:
- Around line 3044-3057: The empty-state branch is hiding active turns because
appendCollapsedEvent drops activity rows so rows.length === 0 even when there is
a live activity; update the rendering logic in AgentChatMessageList so the
empty-state is not chosen when there is an active streaming indicator or
latestActivity. Concretely, change the condition that decides to render the
empty-state (where rows.length === 0 is currently used) to also check
showStreamingIndicator || latestActivity (or otherwise prefer rendering
streamingIndicator/turnSummaryCard), so streamingIndicator (and TurnSummaryCard
when present) are rendered for activity-only turns instead of the idle
empty-state; use the existing symbols streamingIndicator, latestActivity,
showStreamingIndicator, turnSummary, and appendCollapsedEvent to locate and
adjust the logic.
---
Duplicate comments:
In `@apps/desktop/src/renderer/components/chat/AgentChatMessageList.tsx`:
- Around line 775-780: The current detector accepts any candidate starting with
"./" and the resolver strips only the leading "./", allowing embedded ".."
segments through; update both the detector (where candidate is checked) and the
resolver (where openFilePath or the file path is constructed and pushed into
files state) to normalize the path and reject any path containing
parent-traversal segments. Concretely: for the candidate check, run a
POSIX-normalize (or split/join) on candidate after removing a single leading
"./", then if the normalized path contains any ".." path segments or begins with
"../" reject it (return false); likewise, in the resolver before calling
setFiles/openFilePath push, normalize the constructed path and if any ".."
segments remain, abort and do not add the path. Use the existing variable names
candidate, openFilePath and the files state update to locate the places to
change.
- Around line 767-769: The Windows drive-letter normalization (in
AgentChatMessageList where withoutQuery is sliced for /^\/[A-Za-z]:\//) causes
paths like C:/repo/... to miss the absolute-path check later and be treated as
relative; update the absolute-path detection (the branch around the code that
checks for a leading '/' and sets openFilePath vs. workspace match) to also
recognize Windows drive-letter prefixes (e.g. /^[A-Za-z]:[\\/]/) by normalizing
both the candidate path and FilesWorkspace.rootPath (or comparing
path.win32.normalize equivalents) before matching, so drive-letter paths
correctly match FilesWorkspace.rootPath and route to the workspace-link flow
rather than the relative-path branch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 84d0bf87-8313-483d-85a1-a5bdcc1c6e76
📒 Files selected for processing (2)
apps/desktop/src/renderer/components/chat/AgentChatMessageList.test.tsxapps/desktop/src/renderer/components/chat/AgentChatMessageList.tsx
Comment on lines
+820
to
+840
| <button | ||
| type="button" | ||
| className={cn( | ||
| "flex w-full items-center gap-2 rounded-lg px-1.5 py-1 text-left transition-colors hover:bg-white/[0.03]", | ||
| !expandable && "cursor-default hover:bg-transparent", | ||
| )} | ||
| onClick={() => { | ||
| if (expandable) setOpen((value) => !value); | ||
| }} | ||
| > | ||
| {expandable ? ( | ||
| open ? <CaretDown size={10} weight="bold" className="text-fg/28" /> : <CaretRight size={10} weight="bold" className="text-fg/28" /> | ||
| ) : ( | ||
| <span className="ml-[2px] inline-flex h-1.5 w-1.5 rounded-full bg-white/12" aria-hidden="true" /> | ||
| )} | ||
| <div className="min-w-0 flex-1">{summary}</div> | ||
| </button> | ||
| {expandable && open ? ( | ||
| <div className="ml-5 mt-1 space-y-2 border-l border-white/[0.05] pl-3"> | ||
| {children} | ||
| </div> |
There was a problem hiding this comment.
Expose the disclosure state to assistive tech.
These rows expand and collapse content, but the new button never reports its state. Adding aria-expanded here (and ideally aria-controls) is important so screen readers can tell whether command, file, and task details are open.
As per coding guidelines, apps/desktop/src/**: Electron desktop app — check for IPC security, proper main/renderer process separation, and React best practices.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/renderer/components/chat/AgentChatMessageList.tsx` around
lines 820 - 840, The expand/collapse button in AgentChatMessageList does not
expose its state to assistive tech; add aria-expanded and aria-controls to the
button and an id on the collapsible panel so screen readers can detect
open/closed state. Generate a stable unique id for the panel (e.g., via React's
useId or a component-level id like panelId), set aria-controls={panelId} on the
button and aria-expanded={open} (or aria-expanded={expandable ? open :
undefined}) on the button, and set id={panelId} on the div that wraps
{children}; keep aria-controls/aria-expanded only when expandable to avoid
misleading semantics.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
apps/desktop/src/renderer/components/chat/AgentChatMessageList.tsx (2)
772-786:⚠️ Potential issue | 🟠 MajorParent-traversal inputs are still allowed into
/files.
looksLikeWorkspacePath()returnstruebefore any traversal check for inputs like./../secret.ts, andresolveFilesNavigationTarget()still forwards embedded..segments likesrc/../../secret.tsor/repo/src/../../secret.tsintoopenFilePath. Please reject parent traversal before the./fast path and again before building the navigation target. As per coding guidelines,apps/desktop/src/**: Electron desktop app — check for IPC security, proper main/renderer process separation, and React best practices.Also applies to: 856-896
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/chat/AgentChatMessageList.tsx` around lines 772 - 786, looksLikeWorkspacePath currently accepts inputs like "./../secret.ts" because the "./" fast-path occurs before checking for parent-traversal; update looksLikeWorkspacePath to reject any candidate that contains ".." path segments or "/../" or "../" anywhere (and also "~/") before returning true for "./" or other patterns, and ensure resolveFilesNavigationTarget (and any code that builds openFilePath) also normalizes and rejects paths containing parent-traversal segments (e.g., by checking normalizedPath.includes("..") or using path.normalize and verifying it does not escape the repo root) before forwarding to openFilePath so no embedded ".." can reach the file-open logic.
759-769:⚠️ Potential issue | 🟠 MajorDrive-letter absolute paths still fall through as relative paths.
After normalization,
C:\repo\src\foo.tsbecomesC:/repo/src/foo.ts, but the absolute-path branches here still only key off a leading/. That makes Windows workspace links miss theFilesWorkspace.rootPathmatch and even skips the refetch path inopenWorkspacePath(), so these links still break on Windows.Also applies to: 865-889, 2817-2839
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/chat/AgentChatMessageList.tsx` around lines 759 - 769, normalizeWorkspacePathCandidate currently returns Windows drive-letter paths as "C:/..." but downstream code only treats paths starting with "/" as absolute, causing Windows absolute paths to be misclassified as relative; update normalization and any absolute-path checks to recognize drive-letter absolute paths (regex like ^[A-Za-z]:/) and treat them as absolute. Specifically, modify normalizeWorkspacePathCandidate to detect and preserve leading drive letters as absolute, and update openWorkspacePath and any comparisons against FilesWorkspace.rootPath so they also consider paths that start with a drive-letter (e.g., C:/) or UNC forms as absolute when deciding to match rootPath or trigger refetch logic. Ensure regexes and path-slicing logic account for both "/C:/..." → "C:/..." normalization and already-normalized "C:/" forms so Windows links resolve correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/renderer/components/chat/AgentChatMessageList.tsx`:
- Around line 265-269: The empty-state logic is erroneously using rows that have
had event.type === "activity" removed, causing live sessions to appear empty and
drop streamingIndicator; update AgentChatMessageList so you don't mutate the
primary rows used for empty-state checks—either keep a separate transcriptRows
(filtering out activity events) for rendering the transcript while preserving
the original rows array for the empty-state/streamingIndicator checks, or
compute streamingIndicator directly from the raw events list; adjust references
to rows, event.type === "activity", and streamingIndicator/empty-state branch so
activity-only sessions no longer trigger the onboarding empty state.
- Around line 2854-2871: The current logic in the useMemo that builds
turnModelState (variables: turnModelState, map, lastModel, resolveModelLabel,
events) incorrectly lets the active turn inherit lastModel when its done event
hasn't arrived; change the logic so that when computing a model for a message
with a turnId you only return a model if that turnId exists in map (i.e., the
turn emitted a "done" and has an entry); for messages belonging to the
active/unresolved turn return null instead of falling back to lastModel, and
keep lastModel only for messages that truly have no turnId (rows with no turnId
should still use lastModel). Ensure the lookup site that currently falls back to
turnModelState.lastModel (the code referenced around where turnModelState is
consumed) is updated to respect this: use map.get(turnId) or null for
turn-scoped rows, and only use lastModel for rows with no turnId.
- Around line 334-357: The current reverse scan can merge a new text event into
a non-adjacent previous text row; instead, only merge into the immediately
previous visible text row and do not batch anonymous chunks. Replace the
reverse-find logic with a single check of the last element (const last =
out[out.length - 1]) and only merge if last?.event.type === "text",
(event.turnId ?? null) === (last.event.turnId ?? null) and (event.itemId ??
null) === (last.event.itemId ?? null), and at least one of event.turnId or
event.itemId is present (i.e. refuse when both are null); when those conditions
hold update last's timestamp and append event.text as you do now (using
existing, envelope.timestamp, and event.text).
---
Duplicate comments:
In `@apps/desktop/src/renderer/components/chat/AgentChatMessageList.tsx`:
- Around line 772-786: looksLikeWorkspacePath currently accepts inputs like
"./../secret.ts" because the "./" fast-path occurs before checking for
parent-traversal; update looksLikeWorkspacePath to reject any candidate that
contains ".." path segments or "/../" or "../" anywhere (and also "~/") before
returning true for "./" or other patterns, and ensure
resolveFilesNavigationTarget (and any code that builds openFilePath) also
normalizes and rejects paths containing parent-traversal segments (e.g., by
checking normalizedPath.includes("..") or using path.normalize and verifying it
does not escape the repo root) before forwarding to openFilePath so no embedded
".." can reach the file-open logic.
- Around line 759-769: normalizeWorkspacePathCandidate currently returns Windows
drive-letter paths as "C:/..." but downstream code only treats paths starting
with "/" as absolute, causing Windows absolute paths to be misclassified as
relative; update normalization and any absolute-path checks to recognize
drive-letter absolute paths (regex like ^[A-Za-z]:/) and treat them as absolute.
Specifically, modify normalizeWorkspacePathCandidate to detect and preserve
leading drive letters as absolute, and update openWorkspacePath and any
comparisons against FilesWorkspace.rootPath so they also consider paths that
start with a drive-letter (e.g., C:/) or UNC forms as absolute when deciding to
match rootPath or trigger refetch logic. Ensure regexes and path-slicing logic
account for both "/C:/..." → "C:/..." normalization and already-normalized "C:/"
forms so Windows links resolve correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d583994a-0173-4414-ac6c-b9cd650a15ea
📒 Files selected for processing (1)
apps/desktop/src/renderer/components/chat/AgentChatMessageList.tsx
Comment on lines
+334
to
+357
| if (event.type === "text") { | ||
| const nextTurn = event.turnId ?? null; | ||
| const prevItem = prev.event.itemId ?? null; | ||
| const nextItem = event.itemId ?? null; | ||
| if (prevTurn === nextTurn && prevItem === nextItem) { | ||
| out[out.length - 1] = { | ||
| ...prev, | ||
| timestamp: envelope.timestamp, | ||
| event: { | ||
| ...prev.event, | ||
| text: `${prev.event.text}${event.text}` | ||
| } | ||
| }; | ||
| return; | ||
| const matchIndex = [...out] | ||
| .reverse() | ||
| .findIndex((candidate) => | ||
| candidate.event.type === "text" | ||
| && (candidate.event.turnId ?? null) === nextTurn | ||
| && (candidate.event.itemId ?? null) === nextItem, | ||
| ); | ||
| if (matchIndex >= 0) { | ||
| const actualIndex = out.length - 1 - matchIndex; | ||
| const existing = out[actualIndex]; | ||
| if (existing?.event.type === "text") { | ||
| out[actualIndex] = { | ||
| ...existing, | ||
| timestamp: envelope.timestamp, | ||
| event: { | ||
| ...existing.event, | ||
| text: `${existing.event.text}${event.text}`, | ||
| }, | ||
| }; | ||
| return; | ||
| } |
There was a problem hiding this comment.
Don't back-merge text across transcript boundaries.
This reverse scan rewrites the last matching text row anywhere in out, so anonymous chunks (turnId/itemId both absent) can collapse into unrelated assistant messages, and identified chunks can also jump ahead of intervening visible rows. apps/desktop/src/main/services/chat/chatTextBatching.ts already refuses to batch anonymous text for this reason; the renderer should keep that rule and only merge the immediately previous visible text row. Based on learnings, Mission chat uses structured message records and runtime IDs; worker and orchestrator channels reuse shared chat message renderer patterns.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/renderer/components/chat/AgentChatMessageList.tsx` around
lines 334 - 357, The current reverse scan can merge a new text event into a
non-adjacent previous text row; instead, only merge into the immediately
previous visible text row and do not batch anonymous chunks. Replace the
reverse-find logic with a single check of the last element (const last =
out[out.length - 1]) and only merge if last?.event.type === "text",
(event.turnId ?? null) === (last.event.turnId ?? null) and (event.itemId ??
null) === (last.event.itemId ?? null), and at least one of event.turnId or
event.itemId is present (i.e. refuse when both are null); when those conditions
hold update last's timestamp and append event.text as you do now (using
existing, envelope.timestamp, and event.text).
- Show streaming indicator even when rows is empty (activity-only sessions no longer display the onboarding empty state) - Require turnId or itemId identity before merging text chunks in the renderer, preventing anonymous text from collapsing into unrelated assistant messages - Only resolve turn model from the done-event map; return null for active turns instead of inheriting the previous turn's model Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/desktop/src/renderer/components/chat/AgentChatMessageList.tsx (1)
2820-2849: Good async retry pattern for workspace resolution.The
openWorkspacePathcallback appropriately:
- Attempts resolution with cached workspaces first
- Falls back to fresh workspace enumeration for absolute paths that fail initial resolution
- Uses proper async error handling with cancellation awareness
- Updates state with the refreshed workspace list for future calls
Consider logging or providing user feedback when path resolution silently fails, as clicking a non-functional link with no visible response can be confusing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/chat/AgentChatMessageList.tsx` around lines 2820 - 2849, When openWorkspacePath fails to resolve a target (after trying resolveFilesNavigationTarget with filesWorkspaces and after attempting listWorkspaces), surface that failure instead of silently returning: update the openWorkspacePath callback to log the failed path and reason (use console.error or processLogger) and/or trigger user feedback (e.g., call a provided onError/onToast handler) when target is null; reference resolveFilesNavigationTarget, normalizeWorkspacePathCandidate, listWorkspaces, setFilesWorkspaces, navigate, and onOpenWorkspacePath to locate where to insert the log/feedback so clicking a non-functional link provides visible diagnostics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/desktop/src/renderer/components/chat/AgentChatMessageList.tsx`:
- Around line 2820-2849: When openWorkspacePath fails to resolve a target (after
trying resolveFilesNavigationTarget with filesWorkspaces and after attempting
listWorkspaces), surface that failure instead of silently returning: update the
openWorkspacePath callback to log the failed path and reason (use console.error
or processLogger) and/or trigger user feedback (e.g., call a provided
onError/onToast handler) when target is null; reference
resolveFilesNavigationTarget, normalizeWorkspacePathCandidate, listWorkspaces,
setFilesWorkspaces, navigate, and onOpenWorkspacePath to locate where to insert
the log/feedback so clicking a non-functional link provides visible diagnostics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e76fb2df-9a54-4f50-b13b-9fcb6fe4f7c7
📒 Files selected for processing (1)
apps/desktop/src/renderer/components/chat/AgentChatMessageList.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements
Other