feat(chat): implement parser-driven message block rendering#478
Open
pswamis wants to merge 3 commits intositeboon:mainfrom
Open
feat(chat): implement parser-driven message block rendering#478pswamis wants to merge 3 commits intositeboon:mainfrom
pswamis wants to merge 3 commits intositeboon:mainfrom
Conversation
Contributor
📝 WalkthroughWalkthroughAdds a UI-focused chat message parsing pipeline (parseChatMessageForUi + parseUiMessageContent) and updates message conversion and rendering to consume structured ParsedUiMessage variants for thinking, tools, file ops, search, summaries, images, streaming prose, and errors. Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant ChatSvc as ChatMessage
participant UiParser as parseUiMessageContent
participant MsgParser as parseChatMessageForUi
participant Renderer as MessageComponent
participant BrowserUI as UI
Client->>ChatSvc: send/receive ChatMessage
ChatSvc->>UiParser: raw content string
activate UiParser
UiParser->>UiParser: detect skip / taskNotification / text
UiParser-->>MsgParser: UiParsedMessage
deactivate UiParser
ChatSvc->>MsgParser: full ChatMessage
activate MsgParser
MsgParser->>MsgParser: inspect type/tool/toolResult
MsgParser-->>Renderer: ParsedUiMessage
deactivate MsgParser
Renderer->>Renderer: choose rendering branch by kind
Renderer->>BrowserUI: render components (icons, content, actions)
BrowserUI-->>Client: interactive rendered message
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/chat/utils/chatMessageParser.ts`:
- Around line 167-171: The fallback for building list uses toArray(obj.results)
which is always truthy (even if empty), preventing later sources from being
considered; change the logic around the list variable so you pick the first
non-empty array among obj.results, obj.items, obj.data, obj.content (e.g.,
transform each source with toArray then find the first with length > 0, or check
source truthiness/length before calling toArray) and default to [] if none are
non-empty; update references where list is defined and used (the list variable
and the toArray calls for obj.results/obj.items/obj.data/obj.content) so empty
arrays do not short-circuit valid fallbacks.
- Around line 216-218: The folder-detection logic in chatMessageParser.ts (the
mapping that sets the `type` field) is too narrow and only checks for `'dir'`
and `'folder'` via `row.type` and `row.kind`; update it to normalize the
type/kind strings and treat any of `'dir'`, `'directory'`, or `'folder'`
(case-insensitive) as a folder. Locate the code that assigns `type: ... ?
'folder' : 'file'` (the mapping that reads `row.type` and `row.kind`) and
replace the conditional with a normalized-check against the allowed folder names
(e.g., lowercased value in a set/array) so entries with `type: 'folder'` or
`type: 'directory'` are correctly labeled.
In `@src/components/chat/utils/uiParser.ts`:
- Around line 35-37: Normalize the parsed task status before returning the
notification: take the raw value from taskMatch[1], normalize by trimming and
lowercasing, map equivalents like 'done', 'success', 'ok' to 'completed' and map
failure equivalents (e.g., 'failed', 'error') to a canonical failure state, then
set the returned object's status field (the status property in the
taskNotification object) to that canonical value instead of forwarding the raw
string from taskMatch[1]; update the code around the taskMatch usage that builds
the { kind: 'taskNotification', status: ..., summary: ... } object to perform
this normalization.
In `@src/components/chat/view/subcomponents/MessageComponent.tsx`:
- Around line 463-474: The error-warning block removed the recovery action but
onGrantToolPermission is still passed in; restore the permission-grant UI when
parsedUiMessage indicates a tool permission error by using parsedUiMessage
(check properties like parsedUiMessage.permissionRequest or
parsedUiMessage.toolId) and render a button/link that calls
onGrantToolPermission with the appropriate identifier; update the conditional
inside the parsedUiMessage.kind === 'error-warning' branch to show that
interactive control (styled with the same error UI) only when
onGrantToolPermission is defined and the parsedUiMessage contains the permission
metadata so users can trigger the recovery flow from the message.
- Around line 476-489: The details block for parsedUiMessage.kind ===
'tool-invocation' only shows parsedUiMessage.toolInputRaw and omits
parsedUiMessage.content (the tool result); update the rendering in
MessageComponent.tsx inside that details block (the JSX that uses
parsedUiMessage.kind, parsedUiMessage.status, parsedUiMessage.toolName,
parsedUiMessage.toolInputRaw) to also display parsedUiMessage.content when
present (e.g., render a separate pre/div below the input with appropriate
styling and overflow handling), making sure the generic/unknown tool output is
visible and safely rendered as plain text.
- Around line 505-509: The current MessageComponent block that checks
parsedUiMessage.status === 'done' renders a static "Preview" placeholder; update
it to render the actual generated output from parsedUiMessage (e.g.,
parsedUiMessage.output, parsedUiMessage.outputs or parsedUiMessage.generated)
instead. Inside the MessageComponent's done branch, inspect the output shape and
render accordingly: if it's an image URL render an <img> tag with that src, if
it's text/markdown render the text (or pass through your Markdown renderer), and
if it's structured data map it to an appropriate visual component; ensure you
handle missing/empty output gracefully (fallback UI) and reuse existing
helper/rendering utilities in MessageComponent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c57746b2-20f6-45d2-a589-968c7301a18e
📒 Files selected for processing (4)
src/components/chat/utils/chatMessageParser.tssrc/components/chat/utils/messageTransforms.tssrc/components/chat/utils/uiParser.tssrc/components/chat/view/subcomponents/MessageComponent.tsx
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
src/components/chat/view/subcomponents/MessageComponent.tsx (1)
105-107: Permission grant UI state may reset during streaming updates.The
useEffectresetspermissionGrantStateto'idle'wheneverparsedUiMessage.toolIdorpermissionSuggestion?.entrychanges. During streaming,parsedUiMessageis recomputed on everymessageupdate, potentially resetting the state even when the tool identity hasn't meaningfully changed.This could cause the "Permission added" success state to briefly flicker back to the "Allow" button if a streaming update triggers a re-parse.
Consider using a ref to track whether the permission was granted for the current tool, or compare against the previous values before resetting:
Suggested improvement
+ const prevToolIdRef = React.useRef<string | undefined>(); + const prevEntryRef = React.useRef<string | undefined>(); React.useEffect(() => { + const currentToolId = parsedUiMessage.toolId; + const currentEntry = permissionSuggestion?.entry; + if (prevToolIdRef.current !== currentToolId || prevEntryRef.current !== currentEntry) { setPermissionGrantState('idle'); - }, [parsedUiMessage.toolId, permissionSuggestion?.entry]); + prevToolIdRef.current = currentToolId; + prevEntryRef.current = currentEntry; + } + }, [parsedUiMessage.toolId, permissionSuggestion?.entry]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/chat/view/subcomponents/MessageComponent.tsx` around lines 105 - 107, The useEffect that calls setPermissionGrantState('idle') runs on every recompute of parsedUiMessage (and permissionSuggestion?.entry) and can reset the UI during streaming; update it to only reset when the actual tool identity or permission entry meaningfully changes by tracking previous values (e.g., with a useRef like lastToolId/lastPermissionEntry) and comparing parsedUiMessage.toolId and permissionSuggestion?.entry against those previous values before calling setPermissionGrantState; reference the existing parsedUiMessage, permissionSuggestion?.entry, setPermissionGrantState, and permissionGrantState so the effect only resets when the tool/entry changes, not on every streaming-derived reparse.src/components/chat/utils/chatMessageParser.ts (3)
268-284: Summary detection heuristic is broad - may have false positives.The
looksLikeSummarycheck combines multiple heuristics:
- Explicit
isSummaryflag (via type assertion)- Presence of bullet-point items
- First line containing words like "complete", "done", "fixed", etc.
This could trigger for messages that aren't intended as summaries. Consider adding a stricter check or documenting that false positives are acceptable for this UI treatment:
const looksLikeSummary = Boolean((message as any).isSummary) || - summaryItems.length > 0 || + summaryItems.length >= 2 || // Require at least 2 bullet items /(complete|completed|done|fixed|refactoring complete|summary)/i.test(content.split('\n')[0] || '');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/chat/utils/chatMessageParser.ts` around lines 268 - 284, The current summary-detection in chatMessageParser.ts (inside the block guarded by !message.isToolUse) uses the variable looksLikeSummary combining (message as any).isSummary, extractSummaryItems(content), and a loose regex on the first line; tighten this logic to reduce false positives by requiring either an explicit isSummary flag OR one of these stricter conditions: extractSummaryItems returns >1 item, OR the first line matches a more specific regex (e.g., anchors or whole-word phrases like "summary:" or "summary -"/"summary —" or ends-with "complete" with punctuation) AND content length < 2500, or add a feature flag / comment documenting the heuristic; update the looksLikeSummary expression and related title/listItems construction (reference symbols: looksLikeSummary, extractSummaryItems, title, listItems) accordingly.
337-350: Note:applypatchcheck may be defensive/dead code.Per the server transformation in
server/projects.js:1725,apply_patchis converted to'Edit'before reaching the client. ThetoolNameLower === 'applypatch'check at line 337 may never match in practice, though it doesn't cause harm and provides resilience if the server behavior changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/chat/utils/chatMessageParser.ts` around lines 337 - 350, The condition in chatMessageParser.ts checking toolNameLower === 'applypatch' is likely dead because the server normalizes apply_patch to 'Edit'; update the if condition in the block that constructs the 'code-diff' object (the if using toolNameLower and returning { kind: 'code-diff', ... }) to only check for 'edit' (i.e., remove 'applypatch'), and optionally add a short comment above the check noting that the server normalizes apply_patch to 'Edit' for future readers; ensure references to toolNameLower, toolInputObj, and the returned structure remain unchanged.
440-452: Unreachable fallback path at line 454-460.At line 268, if
!message.isToolUse, the function returns early. Therefore, if execution reaches line 440,message.isToolUseis alwaystrue. The condition at line 441isExternalTool || message.isToolUsewill always be true (due tomessage.isToolUse), making the final fallback at lines 454-460 unreachable.This isn't a bug but creates dead code. Consider removing the unreachable fallback or restructuring:
Suggested cleanup
- const isExternalTool = toolNameLower.startsWith('mcp_') || toolNameLower.includes(':') || toolNameLower.includes('fetch'); - if (isExternalTool || message.isToolUse) { - return { + // Fallback for any unrecognized tool + return { kind: 'tool-invocation', collapsible: true, defaultOpen: false, toolName, toolId: toText(message.toolId || message.toolCallId).trim() || undefined, status: message.toolResult ? 'done' : 'running', toolInputRaw: toText(message.toolInput), content: toolResultText, }; - } - - return { - kind: 'streaming-prose', - collapsible: false, - defaultOpen: true, - isStreaming: Boolean(message.isStreaming), - content, - };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/chat/utils/chatMessageParser.ts` around lines 440 - 452, The conditional always evaluates to true because earlier logic guarantees message.isToolUse is true, so remove the dead fallback path: delete the unreachable block that returns the alternative tool-invocation object (the final fallback object after the isExternalTool check) and simplify/clean up the surrounding logic; reference and keep the isExternalTool computation and message.isToolUse usage (symbols: isExternalTool, message.isToolUse) but remove the unreachable fallback return object so only the intended tool-invocation branch remains in chatMessageParser.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/components/chat/utils/chatMessageParser.ts`:
- Around line 268-284: The current summary-detection in chatMessageParser.ts
(inside the block guarded by !message.isToolUse) uses the variable
looksLikeSummary combining (message as any).isSummary,
extractSummaryItems(content), and a loose regex on the first line; tighten this
logic to reduce false positives by requiring either an explicit isSummary flag
OR one of these stricter conditions: extractSummaryItems returns >1 item, OR the
first line matches a more specific regex (e.g., anchors or whole-word phrases
like "summary:" or "summary -"/"summary —" or ends-with "complete" with
punctuation) AND content length < 2500, or add a feature flag / comment
documenting the heuristic; update the looksLikeSummary expression and related
title/listItems construction (reference symbols: looksLikeSummary,
extractSummaryItems, title, listItems) accordingly.
- Around line 337-350: The condition in chatMessageParser.ts checking
toolNameLower === 'applypatch' is likely dead because the server normalizes
apply_patch to 'Edit'; update the if condition in the block that constructs the
'code-diff' object (the if using toolNameLower and returning { kind:
'code-diff', ... }) to only check for 'edit' (i.e., remove 'applypatch'), and
optionally add a short comment above the check noting that the server normalizes
apply_patch to 'Edit' for future readers; ensure references to toolNameLower,
toolInputObj, and the returned structure remain unchanged.
- Around line 440-452: The conditional always evaluates to true because earlier
logic guarantees message.isToolUse is true, so remove the dead fallback path:
delete the unreachable block that returns the alternative tool-invocation object
(the final fallback object after the isExternalTool check) and simplify/clean up
the surrounding logic; reference and keep the isExternalTool computation and
message.isToolUse usage (symbols: isExternalTool, message.isToolUse) but remove
the unreachable fallback return object so only the intended tool-invocation
branch remains in chatMessageParser.
In `@src/components/chat/view/subcomponents/MessageComponent.tsx`:
- Around line 105-107: The useEffect that calls setPermissionGrantState('idle')
runs on every recompute of parsedUiMessage (and permissionSuggestion?.entry) and
can reset the UI during streaming; update it to only reset when the actual tool
identity or permission entry meaningfully changes by tracking previous values
(e.g., with a useRef like lastToolId/lastPermissionEntry) and comparing
parsedUiMessage.toolId and permissionSuggestion?.entry against those previous
values before calling setPermissionGrantState; reference the existing
parsedUiMessage, permissionSuggestion?.entry, setPermissionGrantState, and
permissionGrantState so the effect only resets when the tool/entry changes, not
on every streaming-derived reparse.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c0cc36d1-2a8c-429e-8cba-18f7ec42a26f
📒 Files selected for processing (3)
src/components/chat/utils/chatMessageParser.tssrc/components/chat/utils/uiParser.tssrc/components/chat/view/subcomponents/MessageComponent.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/chat/utils/uiParser.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/components/chat/utils/chatMessageParser.ts (3)
264-264: Consider narrowing thepermissionRequestflag condition.Setting
permissionRequest: Boolean(message.toolResult?.isError && message.toolName)flags all tool errors as potential permission requests. However, the actual permission grant flow ingetClaudePermissionSuggestion(chatPermissions.ts) performs additional validation to determine if a permission entry can be built. This may cause the UI to evaluate permission logic for non-permission errors unnecessarily.This is minor since
permissionSuggestionis computed separately and will benullfor non-permission errors, so the UI handles it correctly. The current implementation is acceptable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/chat/utils/chatMessageParser.ts` at line 264, The current permissionRequest flag (permissionRequest: Boolean(message.toolResult?.isError && message.toolName)) is too broad; narrow it so only errors that could actually be permission prompts set the flag. Update the assignment in chatMessageParser.ts to check a permission-specific indicator on message.toolResult (e.g., a permission-related error code, type, or hint) or validate message.toolName against the subset of tools that can request permissions, rather than any tool error, and keep getClaudePermissionSuggestion in chatPermissions.ts as the final validator for building permission entries.
271-271: Type cast bypasses TypeScript safety for undocumented field.The cast
(message as any).isSummaryaccesses a field not defined in theChatMessageinterface. While the[key: string]: unknownindex signature allows arbitrary fields at runtime, this cast hides the access from TypeScript's type checker.Consider either adding
isSummary?: booleanto theChatMessageinterface if it's a valid field, or removing this check if the heuristics in lines 272-278 provide sufficient coverage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/chat/utils/chatMessageParser.ts` at line 271, The code unsafely type-casts message to any to read isSummary; update the ChatMessage type to include an optional isSummary?: boolean and then read message.isSummary (or, alternatively, remove the explicitSummary check entirely and rely on the existing heuristics). Specifically, add isSummary?: boolean to the ChatMessage interface declaration and replace the unsafe cast usage in chatMessageParser.ts (the explicitSummary assignment) so the field is accessed through the typed ChatMessage instead of (message as any).isSummary.
357-370: Questionable status determination for file-write operations.The logic
status = fileContent ? 'created' : 'saved'on Line 360 determines status based on whethertoolInput.contentis present. However, the presence of content in the input doesn't reliably indicate whether the file was newly created or an existing file was updated. Both creation and update operations typically include content.This may result in all file writes with content being labeled "created" regardless of actual operation type. Consider deriving the status from the tool result or a more explicit field if available.
Potential improvement if toolResult provides operation type
if (toolNameLower === 'write' || toolNameLower.includes('create_file') || toolNameLower.includes('createfile')) { const path = toText(toolInputObj?.file_path ?? toolInputObj?.path).trim(); - const fileContent = toText(toolInputObj?.content || ''); - const status = fileContent ? 'created' : 'saved'; + const resultObj = toObject(message.toolResult); + const isCreated = toolNameLower.includes('create') || resultObj?.created === true; + const status = isCreated ? 'created' : 'saved'; return {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/chat/utils/chatMessageParser.ts` around lines 357 - 370, The status for file-write items is being inferred from presence of toolInputObj.content (fileContent) in the toolNameLower === 'write' branch, which mislabels updates as "created"; update the logic in the block that returns kind: 'file-write' to derive status from an explicit operation indicator in the tool result (or a dedicated field on toolInputObj) instead of fileContent—e.g., check for a toolResult.operation / toolInputObj.operation / toolResult.type (or fall back to a conservative default like 'saved') and set status accordingly while keeping existing fields (path, filename, toolName) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/components/chat/utils/chatMessageParser.ts`:
- Line 264: The current permissionRequest flag (permissionRequest:
Boolean(message.toolResult?.isError && message.toolName)) is too broad; narrow
it so only errors that could actually be permission prompts set the flag. Update
the assignment in chatMessageParser.ts to check a permission-specific indicator
on message.toolResult (e.g., a permission-related error code, type, or hint) or
validate message.toolName against the subset of tools that can request
permissions, rather than any tool error, and keep getClaudePermissionSuggestion
in chatPermissions.ts as the final validator for building permission entries.
- Line 271: The code unsafely type-casts message to any to read isSummary;
update the ChatMessage type to include an optional isSummary?: boolean and then
read message.isSummary (or, alternatively, remove the explicitSummary check
entirely and rely on the existing heuristics). Specifically, add isSummary?:
boolean to the ChatMessage interface declaration and replace the unsafe cast
usage in chatMessageParser.ts (the explicitSummary assignment) so the field is
accessed through the typed ChatMessage instead of (message as any).isSummary.
- Around line 357-370: The status for file-write items is being inferred from
presence of toolInputObj.content (fileContent) in the toolNameLower === 'write'
branch, which mislabels updates as "created"; update the logic in the block that
returns kind: 'file-write' to derive status from an explicit operation indicator
in the tool result (or a dedicated field on toolInputObj) instead of
fileContent—e.g., check for a toolResult.operation / toolInputObj.operation /
toolResult.type (or fall back to a conservative default like 'saved') and set
status accordingly while keeping existing fields (path, filename, toolName)
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3fe70f86-b712-4d2a-aefe-e4f6356a2afc
📒 Files selected for processing (2)
src/components/chat/utils/chatMessageParser.tssrc/components/chat/view/subcomponents/MessageComponent.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.
This PR introduces a parser-first rendering pipeline for chat messages, replacing ad-hoc message branching with structured UI message types and dedicated rendering blocks.
What changed
Follow-up review fixes included
Result
Summary by CodeRabbit
New Features
UI/Visual Improvements