Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughReplaced per-iteration Changes
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
ehayes2000
force-pushed
the
fix-ai-rendering
branch
from
704ba71 to
0f46260
Compare
![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)
js/app/packages/core/component/AI/component/message/AssistantMessage.tsx (1)
373-429: 🧹 Nitpick | 🔵 TrivialConsider using
matchfromts-patternfor exhaustive type handling.The if-else chain on the discriminated union
AssistantMessagePartcould usematchfor cleaner, exhaustive pattern matching. This would also surface the unhandledtoolCallErrvariant (see relevant snippets showing 4 variants exist).♻️ Proposed refactor using `match`
+import { match } from 'ts-pattern';Then replace the if-else chain:
- if (currentPart()!.type === 'toolCall') { - const toolCall = () => - currentPart()! as Extract<AssistantMessagePart, { type: 'toolCall' }>; - - return ( - <RenderTool - tool_id={toolCall().id} - chat_id={'todo'} - json={toolCall().json} - name={toolCall().name} - message_id={props.message.id} - part_index={i()} - type="call" - isComplete={isCompleteSelector(toolCall().id)} - renderContext={{ - renderContext: { - isStreaming: props.isStreaming, - }, - }} - /> - ); - } else if (currentPart()!.type === 'toolCallResponseJson') { - const toolResponse = () => - currentPart()! as Extract< - AssistantMessagePart, - { type: 'toolCallResponseJson' } - >; - - return ( - <RenderTool - isComplete={true} - tool_id={toolResponse().id} - chat_id={'todo'} - json={toolResponse().json} - name={toolResponse().name} - message_id={props.message.id} - part_index={i()} - type="response" - renderContext={{ - renderContext: { - isStreaming: props.isStreaming, - }, - }} - /> - ); - } else if (currentPart()!.type === 'text') { - const textPart = () => - currentPart()! as Extract<AssistantMessagePart, { type: 'text' }>; - - if (textPart().text.trim().length > 0) - return ( - <ChatMessageMarkdown - text={textPart().text} - generating={() => props.isStreaming} - /> - ); - } + return match(currentPart()!) + .with({ type: 'toolCall' }, (toolCall) => ( + <RenderTool + tool_id={toolCall.id} + chat_id={'todo'} + json={toolCall.json} + name={toolCall.name} + message_id={props.message.id} + part_index={i()} + type="call" + isComplete={isCompleteSelector(toolCall.id)} + renderContext={{ + renderContext: { + isStreaming: props.isStreaming, + }, + }} + /> + )) + .with({ type: 'toolCallResponseJson' }, (toolResponse) => ( + <RenderTool + isComplete={true} + tool_id={toolResponse.id} + chat_id={'todo'} + json={toolResponse.json} + name={toolResponse.name} + message_id={props.message.id} + part_index={i()} + type="response" + renderContext={{ + renderContext: { + isStreaming: props.isStreaming, + }, + }} + /> + )) + .with({ type: 'text' }, (textPart) => + textPart.text.trim().length > 0 ? ( + <ChatMessageMarkdown + text={textPart.text} + generating={() => props.isStreaming} + /> + ) : null + ) + .with({ type: 'toolCallErr' }, () => null) // Explicitly handle error variant + .exhaustive();As per coding guidelines: "For exhaustive switch statements in TypeScript, use
matchfromts-pattern".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/app/packages/core/component/AI/component/message/AssistantMessage.tsx` around lines 373 - 429, The current if/else chain in AssistantMessage.tsx using currentPart() to discriminate AssistantMessagePart is not exhaustive and misses the toolCallErr variant; replace the chain with ts-pattern's match (import { match } from 'ts-pattern') on currentPart() to perform exhaustive pattern matching over AssistantMessagePart (cases: toolCall, toolCallResponseJson, text, toolCallErr, etc.), map each case to the same JSX you currently return (use RenderTool, ChatMessageMarkdown, and isCompleteSelector where appropriate, preserving props.message.id, part_index i(), and props.isStreaming), and ensure the toolCallErr case is handled (render an error UI or RenderTool with error state) so the compiler/runtime know all variants are covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@js/app/packages/core/component/AI/component/message/AssistantMessage.tsx`:
- Around line 370-371: The current code defines a redundant wrapper const
currentPart = () => part() and then calls currentPart(); replace this
indirection by using the existing memo accessor directly (either call part()
where needed or assign const currentPart = part) to simplify the logic in
AssistantMessage.tsx; update any references that call currentPart() to call
part() (or use the direct accessor variable) and remove the extra arrow-function
wrapper introduced around the mapArray part accessor.
---
Outside diff comments:
In `@js/app/packages/core/component/AI/component/message/AssistantMessage.tsx`:
- Around line 373-429: The current if/else chain in AssistantMessage.tsx using
currentPart() to discriminate AssistantMessagePart is not exhaustive and misses
the toolCallErr variant; replace the chain with ts-pattern's match (import {
match } from 'ts-pattern') on currentPart() to perform exhaustive pattern
matching over AssistantMessagePart (cases: toolCall, toolCallResponseJson, text,
toolCallErr, etc.), map each case to the same JSX you currently return (use
RenderTool, ChatMessageMarkdown, and isCompleteSelector where appropriate,
preserving props.message.id, part_index i(), and props.isStreaming), and ensure
the toolCallErr case is handled (render an error UI or RenderTool with error
state) so the compiler/runtime know all variants are covered.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 763630e7-a445-4e95-a56d-4aaa60f0cf5f
📒 Files selected for processing (1)
js/app/packages/core/component/AI/component/message/AssistantMessage.tsx
Comment on lines
+370
to
+371
| const currentPart = () => part(); | ||
| if (!currentPart()) return null; |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Simplify the aliasing: currentPart just wraps part redundantly.
Since part from mapArray is already a memo accessor, const currentPart = () => part() adds an unnecessary layer of indirection. Consider using part directly or assigning const currentPart = part;.
🔧 Proposed simplification
- const currentPart = () => part();
- if (!currentPart()) return null;
+ const currentPart = part;
+ if (!currentPart()) return null;📝 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 currentPart = () => part(); | |
| if (!currentPart()) return null; | |
| const currentPart = part; | |
| if (!currentPart()) return null; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/app/packages/core/component/AI/component/message/AssistantMessage.tsx`
around lines 370 - 371, The current code defines a redundant wrapper const
currentPart = () => part() and then calls currentPart(); replace this
indirection by using the existing memo accessor directly (either call part()
where needed or assign const currentPart = part) to simplify the logic in
AssistantMessage.tsx; update any references that call currentPart() to call
part() (or use the direct accessor variable) and remove the extra arrow-function
wrapper introduced around the mapArray part accessor.
This was referenced Apr 6, 2026
Merged
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.
No description provided.