feat(studio): coding agent chat polish - #592
Merged
Merged
Conversation
Signed-off-by: Danielle Ali <44468613+dmariali@users.noreply.github.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds session titles end-to-end, updates assistant message surface rendering, removes running-state tool-call styling, and splits Studio summary output into detail and final parts. ChangesSession Title Feature
Assistant Message Surface Rendering
Running-State Styling Removal
Studio Summary Partitioning
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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)
web/packages/studio/src/routes/agents/ClaudeCodeChatRoute/toolParts.ts (1)
199-218: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winWire
titlethrough or drop it from the parser
titleis accepted byStudioSummaryFieldand matched by the regex, butgetStudioSummaryFieldsnever returns it. That silently discards the parsed value; either expose it from this helper or remove it from the matcher/type.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/packages/studio/src/routes/agents/ClaudeCodeChatRoute/toolParts.ts` around lines 199 - 218, The `getStudioSummaryFields` parser currently accepts and matches `title` via `StudioSummaryField` and the regex, but then drops it by never returning it. Update the helper in `toolParts.ts` to either include `title` in the returned object using the existing parsed `fields.title`, or remove `title` from both `StudioSummaryField` and the matcher if it is not meant to be supported, so the parser behavior stays consistent.
♻️ Duplicate comments (1)
web/packages/common/src/components/AssistantChat/AssistantMessage.tsx (1)
32-34: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMessage-level gate doesn't prevent per-part empty surfaces.
hasRenderableContentonly decides whether to renderAssistantChatMessageContentat all; it doesn't stop individual emptytextparts inside a multi-part message from being wrapped inASSISTANT_MESSAGE_SURFACE_CLASSby theTextrenderer (see companion file). Root-cause fix belongs inAssistantChatMessageContent.tsx'sTextclosure.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/packages/common/src/components/AssistantChat/AssistantMessage.tsx` around lines 32 - 34, The current message-level check in AssistantMessage only gates the whole AssistantChatMessageContent, but empty text parts can still be wrapped by the Text renderer inside AssistantChatMessageContent.tsx. Move the fix into the Text closure there by skipping rendering for text parts whose trimmed content is empty, so only non-empty text parts get ASSISTANT_MESSAGE_SURFACE_CLASS while non-text parts continue to render normally.
🧹 Nitpick comments (4)
web/packages/common/src/components/AssistantChat/AssistantChatMessageContent.tsx (1)
19-46: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate wrap logic between
TextandImage.Both renderers repeat the same
contentSurfaceClassName ? <div>...</div> : contentpattern. Extract a small helper (e.g.,wrapInSurface(content, className)) to avoid drift.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/packages/common/src/components/AssistantChat/AssistantChatMessageContent.tsx` around lines 19 - 46, The Text and Image renderers in AssistantChatMessageContent both duplicate the same contentSurfaceClassName wrapping logic, so extract a shared helper to keep behavior consistent and avoid drift. Create a small reusable wrapper function near MessagePrimitive.Parts (for example, one that takes the rendered content and contentSurfaceClassName) and use it in both the Text and Image branches so they both delegate to the same surface-wrapping path.web/packages/common/src/components/AssistantChat/index.test.tsx (1)
300-341: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd coverage for mixed empty/non-empty text parts.
No test exercises a message with an empty text part alongside a non-empty part or tool-call — the scenario where the empty-surface bug (flagged in
AssistantChatMessageContent.tsx) would manifest.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/packages/common/src/components/AssistantChat/index.test.tsx` around lines 300 - 341, Add a test case in AssistantChat index tests that covers a mixed assistant message with an empty text part plus a non-empty text part and/or a tool-call, since the current coverage only checks fully populated text segments. Update the AssistantChatMessageContent rendering expectations to verify that empty text parts do not produce an empty message surface, while adjacent non-empty parts and tool-call content still render correctly; use the existing StaticAssistantChatThread, assistant-chat-message-surface, and assistant-chat-tool-pill assertions as the locating points.web/packages/studio/src/routes/agents/ClaudeCodeChatRoute/historyPanel/helpers.ts (1)
139-171: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated finalize-title logic between
getSmartPromptHistoryTitleandgetPromptHistoryTitle.Both apply
capitalizeTitle(stripRequestPrefix(...).replace(/[,:;\s-]+$/g, '').trim())thenlimitTitleLength. Consider extracting a sharedfinalizeTitle(text)helper.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/packages/studio/src/routes/agents/ClaudeCodeChatRoute/historyPanel/helpers.ts` around lines 139 - 171, There is duplicated title-finalization logic in getSmartPromptHistoryTitle and getPromptHistoryTitle. Extract the shared stripRequestPrefix/trim/trailing-punctuation/capitalizeTitle/limitTitleLength flow into a reusable helper such as finalizeTitle, then have both functions call it so the formatting behavior stays consistent in helpers.ts.web/packages/studio/src/routes/agents/ClaudeCodeChatRoute/toolParts.ts (1)
391-434: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTable extraction ignores fenced code blocks.
extractMarkdownTablePartsscans raw lines for table-shaped pipe/dash patterns without tracking```fences. Pipe/dash content inside a code fence (e.g. an ASCII table or CSV sample shown as code) would be misdetected as a real table and yanked out intofinalParts, breaking the code block's formatting.♻️ Proposed fix: skip lines inside code fences
const extractMarkdownTableParts = ( text: string ): { readonly remainingText: string; readonly tableParts: readonly ThreadAssistantMessagePart[]; } => { const lines = text.split('\n'); const tableLines = new Set<number>(); const tableParts: ThreadAssistantMessagePart[] = []; + let inCodeFence = false; for (let index = 1; index < lines.length; index += 1) { + if (/^\s*```/.test(lines[index - 1]!)) inCodeFence = !inCodeFence; + if (inCodeFence) continue; if (!MARKDOWN_TABLE_DELIMITER_ROW.test(lines[index]!) || !lines[index - 1]!.includes('|')) { continue; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/packages/studio/src/routes/agents/ClaudeCodeChatRoute/toolParts.ts` around lines 391 - 434, extractMarkdownTableParts currently detects markdown tables from raw lines without accounting for fenced code blocks, so pipe/dash patterns inside code samples can be pulled out incorrectly. Update this helper to track whether the scan is inside a ``` fence while iterating lines, and skip table detection and table line collection whenever inCodeFence is true. Keep the existing table parsing behavior for real markdown tables outside fences, and preserve the current remainingText/tableParts handling in ClaudeCodeChatRoute/toolParts.ts.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@web/packages/common/src/components/AssistantChat/AssistantChatMessageContent.tsx`:
- Around line 21-30: The Text renderer in AssistantChatMessageContent should
skip whitespace-only parts before adding the content surface wrapper. Update the
Text branch in AssistantChatMessageContent so it checks text.trim() and returns
the raw content (or nothing) when the text is blank, while still using
MessageContent and the contentSurfaceClassName wrapper for real text.
---
Outside diff comments:
In `@web/packages/studio/src/routes/agents/ClaudeCodeChatRoute/toolParts.ts`:
- Around line 199-218: The `getStudioSummaryFields` parser currently accepts and
matches `title` via `StudioSummaryField` and the regex, but then drops it by
never returning it. Update the helper in `toolParts.ts` to either include
`title` in the returned object using the existing parsed `fields.title`, or
remove `title` from both `StudioSummaryField` and the matcher if it is not meant
to be supported, so the parser behavior stays consistent.
---
Duplicate comments:
In `@web/packages/common/src/components/AssistantChat/AssistantMessage.tsx`:
- Around line 32-34: The current message-level check in AssistantMessage only
gates the whole AssistantChatMessageContent, but empty text parts can still be
wrapped by the Text renderer inside AssistantChatMessageContent.tsx. Move the
fix into the Text closure there by skipping rendering for text parts whose
trimmed content is empty, so only non-empty text parts get
ASSISTANT_MESSAGE_SURFACE_CLASS while non-text parts continue to render
normally.
---
Nitpick comments:
In
`@web/packages/common/src/components/AssistantChat/AssistantChatMessageContent.tsx`:
- Around line 19-46: The Text and Image renderers in AssistantChatMessageContent
both duplicate the same contentSurfaceClassName wrapping logic, so extract a
shared helper to keep behavior consistent and avoid drift. Create a small
reusable wrapper function near MessagePrimitive.Parts (for example, one that
takes the rendered content and contentSurfaceClassName) and use it in both the
Text and Image branches so they both delegate to the same surface-wrapping path.
In `@web/packages/common/src/components/AssistantChat/index.test.tsx`:
- Around line 300-341: Add a test case in AssistantChat index tests that covers
a mixed assistant message with an empty text part plus a non-empty text part
and/or a tool-call, since the current coverage only checks fully populated text
segments. Update the AssistantChatMessageContent rendering expectations to
verify that empty text parts do not produce an empty message surface, while
adjacent non-empty parts and tool-call content still render correctly; use the
existing StaticAssistantChatThread, assistant-chat-message-surface, and
assistant-chat-tool-pill assertions as the locating points.
In
`@web/packages/studio/src/routes/agents/ClaudeCodeChatRoute/historyPanel/helpers.ts`:
- Around line 139-171: There is duplicated title-finalization logic in
getSmartPromptHistoryTitle and getPromptHistoryTitle. Extract the shared
stripRequestPrefix/trim/trailing-punctuation/capitalizeTitle/limitTitleLength
flow into a reusable helper such as finalizeTitle, then have both functions call
it so the formatting behavior stays consistent in helpers.ts.
In `@web/packages/studio/src/routes/agents/ClaudeCodeChatRoute/toolParts.ts`:
- Around line 391-434: extractMarkdownTableParts currently detects markdown
tables from raw lines without accounting for fenced code blocks, so pipe/dash
patterns inside code samples can be pulled out incorrectly. Update this helper
to track whether the scan is inside a ``` fence while iterating lines, and skip
table detection and table line collection whenever inCodeFence is true. Keep the
existing table parsing behavior for real markdown tables outside fences, and
preserve the current remainingText/tableParts handling in
ClaudeCodeChatRoute/toolParts.ts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e3cf1146-1fb3-4ca7-b209-bf9b48064c8e
📒 Files selected for processing (20)
services/studio/src/nmp/studio/coding_agents.pyservices/studio/tests/unit/test_coding_agents.pyweb/packages/common/src/components/AssistantChat/AssistantChatMessageContent.tsxweb/packages/common/src/components/AssistantChat/AssistantMessage.tsxweb/packages/common/src/components/AssistantChat/index.test.tsxweb/packages/studio/src/index.cssweb/packages/studio/src/routes/agents/ClaudeCodeChatRoute/ClaudeCodeHistoryPanel.test.tsxweb/packages/studio/src/routes/agents/ClaudeCodeChatRoute/ClaudeCodeToolCallPart.test.tsxweb/packages/studio/src/routes/agents/ClaudeCodeChatRoute/ClaudeCodeToolCallPart.tsxweb/packages/studio/src/routes/agents/ClaudeCodeChatRoute/api.test.tsweb/packages/studio/src/routes/agents/ClaudeCodeChatRoute/api.tsweb/packages/studio/src/routes/agents/ClaudeCodeChatRoute/historyPanel/HistorySessionButton.tsxweb/packages/studio/src/routes/agents/ClaudeCodeChatRoute/historyPanel/helpers.test.tsweb/packages/studio/src/routes/agents/ClaudeCodeChatRoute/historyPanel/helpers.tsweb/packages/studio/src/routes/agents/ClaudeCodeChatRoute/toolCall/FileChangeToolCallCard.tsxweb/packages/studio/src/routes/agents/ClaudeCodeChatRoute/toolCall/SubtleToolCallRow.tsxweb/packages/studio/src/routes/agents/ClaudeCodeChatRoute/toolCall/constants.tsweb/packages/studio/src/routes/agents/ClaudeCodeChatRoute/toolParts.test.tsweb/packages/studio/src/routes/agents/ClaudeCodeChatRoute/toolParts.tsweb/packages/studio/src/routes/agents/ClaudeCodeChatRoute/types.ts
💤 Files with no reviewable changes (2)
- web/packages/studio/src/index.css
- web/packages/studio/src/routes/agents/ClaudeCodeChatRoute/toolCall/constants.ts
Contributor
|
Signed-off-by: Danielle Ali <44468613+dmariali@users.noreply.github.com>
arpitsardhana
pushed a commit
that referenced
this pull request
Jul 9, 2026
* feat(studio): coding agent chat polish Signed-off-by: Danielle Ali <44468613+dmariali@users.noreply.github.com> * hide empty text box Signed-off-by: Danielle Ali <44468613+dmariali@users.noreply.github.com> --------- Signed-off-by: Danielle Ali <44468613+dmariali@users.noreply.github.com>
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
2 participants
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