refactor(ui): split message part rendering#620
Merged
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (44)
📝 WalkthroughWalkthroughModularizes the message-part UI: introduces a registry, grouping/filtering helpers, paced markdown/highlight rendering, new part and tool components, diagnostics/file UI helpers, and tests updated to assert aggregated source-level properties and agent-pill removal. ChangesMessage-part UI modularization and registry system
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 9
🧹 Nitpick comments (6)
packages/ui/src/components/message-part/registry.ts (2)
32-34: 💤 Low valueConsider guarding against double-registration in development.
registerPartComponentsilently overwrites if called twice with the sametype. For a structural refactor, adding a dev-mode warning would catch accidental duplicate registrations.🛡️ Optional guard to detect double-registration
export function registerPartComponent(type: string, component: PartComponent) { + if (import.meta.env.DEV && PART_MAPPING[type]) { + console.warn(`[registry] Part type "${type}" is already registered`) + } PART_MAPPING[type] = component }🤖 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 `@packages/ui/src/components/message-part/registry.ts` around lines 32 - 34, Update registerPartComponent to detect and warn on duplicate registrations in development: check PART_MAPPING[type] before assigning and, if a different component is already registered and process.env.NODE_ENV !== 'production' (or import a DEV flag), log a console.warn or use the project's logger that includes the conflicting type and the existing vs new component identifiers; then still allow overwrite (or optionally skip assignment) depending on desired behavior. Reference: registerPartComponent and PART_MAPPING.
58-65: 💤 Low valueOptional: Simplify tool state structure.
The tool
statestores{ name, render? }butgetToolonly returnsrender. The storednamefield is never read. You could simplifystatetoRecord<string, ToolComponent | undefined>to match the pattern inPART_MAPPING.♻️ Optional simplification
-const state: Record< - string, - { - name: string - render?: ToolComponent - } -> = {} +const state: Record<string, ToolComponent | undefined> = {} export function registerTool(input: { name: string; render?: ToolComponent }) { - state[input.name] = input + state[input.name] = input.render return input } export function getTool(name: string) { - return state[name]?.render + return state[name] }🤖 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 `@packages/ui/src/components/message-part/registry.ts` around lines 58 - 65, The state currently stores objects with { name, render? } but getTool only returns the render, so simplify state to Record<string, ToolComponent | undefined> and update registerTool to assign input.render directly to state[input.name]; adjust any references to state shape accordingly (notably registerTool and getTool) so they match the PART_MAPPING pattern and remove the unused name field.packages/ui/src/components/message-part/message-router.tsx (1)
11-21: ⚡ Quick winConsider using type predicates instead of
asassertions for better type narrowing.The
astype assertions on lines 12 and 16 work becauseMessageTypeis a discriminated union from the SDK, but type predicates would provide proper type narrowing without bypassing TypeScript checks. Other parts of the codebase (e.g.,timeline-playground.stories.tsx) use this pattern:(m): m is UserMessage => m.role === "user"This approach is more idiomatic and eliminates reliance on manual type assertions.
🤖 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 `@packages/ui/src/components/message-part/message-router.tsx` around lines 11 - 21, The code uses `as` assertions to cast props.message to UserMessage/AssistantMessage in message-router.tsx (used by UserMessageDisplay and AssistantMessageDisplay); replace those casts with proper type predicate functions (e.g., isUserMessage(m): m is UserMessage and isAssistantMessage(m): m is AssistantMessage) that check m.role === "user" / "assistant", use those predicates in the Show conditions to let TypeScript narrow the type, and remove the `as` assertions from the UserMessageDisplay and AssistantMessageDisplay props so the components receive correctly narrowed types without bypassing the compiler.packages/ui/src/components/message-part/tools/question.tsx (1)
11-12: ⚡ Quick winConsider adding runtime validation for type casts.
Lines 11-12 cast
props.input.questionsandprops.metadata.answerstoQuestionInfo[]andQuestionAnswer[]without runtime validation. If the data doesn't match the expected shape, this could cause runtime errors when accessing properties likeq.question(line 39).While TypeScript casts provide compile-time type checking, they don't guarantee runtime safety when dealing with external data. Consider adding validation or type guards if these values come from untrusted sources.
🛡️ Proposed defensive validation
- const questions = createMemo(() => (props.input.questions ?? []) as QuestionInfo[]) - const answers = createMemo(() => (props.metadata.answers ?? []) as QuestionAnswer[]) + const questions = createMemo(() => { + const value = props.input.questions ?? [] + if (!Array.isArray(value)) return [] + return value as QuestionInfo[] + }) + const answers = createMemo(() => { + const value = props.metadata?.answers ?? [] + if (!Array.isArray(value)) return [] + return value as QuestionAnswer[] + })This adds basic array validation before casting. For stricter validation, consider using a schema validator like Zod.
🤖 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 `@packages/ui/src/components/message-part/tools/question.tsx` around lines 11 - 12, The current casts in createMemo for questions and answers assume props.input.questions and props.metadata.answers match QuestionInfo[]/QuestionAnswer[] at runtime; add defensive runtime checks (e.g., Array.isArray and simple item shape/type guards or a utility isQuestionInfo/isQuestionAnswer) before casting and fall back to [] if validation fails so accesses like q.question in the component are safe; update the createMemo calls that initialize questions and answers to use these validators (or a small helper validateArray<T>(value, predicate)) to ensure only properly-shaped objects are returned.packages/ui/src/components/message-part/tools/edit.tsx (1)
40-43: 💤 Low valueUse consistent optional chaining for path handling.
Lines 40 and 42 show inconsistent null handling:
- Line 40 uses optional chaining:
props.input.filePath?.includes("/")- Line 42 uses non-null assertion:
props.input.filePath!While the conditional check ensures
filePathis truthy at line 42, using optional chaining is more defensive and consistent with the pattern established at line 40.♻️ Proposed consistency improvement
<Show when={!pending() && props.input.filePath?.includes("/")}> <div data-slot="message-part-path"> - <span data-slot="message-part-directory">{getDirectory(props.input.filePath!)}</span> + <span data-slot="message-part-directory">{getDirectory(props.input.filePath)}</span> </div> </Show>Note: This assumes
getDirectoryhandles string input safely. The Show condition already guarantees filePath is a non-empty string.🤖 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 `@packages/ui/src/components/message-part/tools/edit.tsx` around lines 40 - 43, The code uses a non-null assertion when calling getDirectory but optional chaining in the conditional; replace the non-null assertion with consistent optional handling by calling getDirectory(props.input.filePath) (or getDirectory(props.input.filePath ?? '') if you prefer a fallback) inside the Show block and keep the existing Show condition and pending() check; update occurrences around the Show component, pending(), props.input.filePath and getDirectory usage to remove the trailing "!" for consistency.packages/ui/src/components/tool-info.ts (1)
67-73: 💤 Low valueDefault parameters precede a required one — defaults are effectively unreachable.
inputandmetadatahave defaults, but the requiredi18nfollows them. Callers can never omitinput/metadatato fall back to{}(they'd have to passundefinedexplicitly), so the defaults add noise without value and the parameter list invites positional mistakes. Consider reordering required params first, or makingi18npart of an options object.♻️ Suggested signature
export function toolInfoForInput( tool: string, + i18n: UiI18n, input: Record<string, any> = {}, metadata: Record<string, any> = {}, - i18n: UiI18n, options: { unknownSubtitle?: string } = {}, ): ToolInfo {🤖 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 `@packages/ui/src/components/tool-info.ts` around lines 67 - 73, The function signature for toolInfoForInput places optional params (input, metadata) before the required i18n, making their defaults effectively unreachable; fix by reordering required params before defaults or by collapsing i18n into an options object. For example, change the signature of toolInfoForInput so i18n: UiI18n appears immediately after tool (or replace the last options param with an options object that includes i18n and unknownSubtitle), then update all call sites to pass i18n in the new position or as part of the options; adjust the function body to read i18n from its new location and keep input and metadata defaulting to {}.
🤖 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 `@packages/ui/src/components/message-part.tsx`:
- Around line 27-29: getToolInfo currently calls toolInfoForInput without the
options object, causing unknown tools to get subtitle: undefined whereas
buildToolInfo sets subtitle: ""; update getToolInfo to forward the same options
used by buildToolInfo by passing an options object including unknownSubtitle
(e.g., unknownSubtitle: "") when calling toolInfoForInput; reference the
getToolInfo function and ensure it calls toolInfoForInput(tool, input, metadata,
useI18n(), { unknownSubtitle: "" }) so behavior matches buildToolInfo.
In `@packages/ui/src/components/message-part/markdown-render.tsx`:
- Around line 145-148: The function relativizeProjectPath leaves a leading path
separator because it slices using directory.length instead of the full prefix
length; update the slice to remove the entire prefix by using prefix.length
(i.e., after computing separator and prefix and verifying
path.startsWith(prefix), return path.slice(prefix.length)) so the leading
separator is stripped correctly.
In `@packages/ui/src/components/message-part/message-router.tsx`:
- Around line 26-41: CompactionPartDisplay currently has a signature that
accepts no parameters while PART_MAPPING and the PartComponent type expect
Component<MessagePartProps>; update CompactionPartDisplay to accept the props
shape (e.g., (props: MessagePartProps) or function CompactionPartDisplay(props:
MessagePartProps)) and use props as needed (or ignore them) so its type matches
PartComponent, ensuring compatibility with PART_MAPPING and the Part function
that forwards part, message, hideDetails, defaultOpen, showAssistantCopyPartID,
and turnDurationMs.
In `@packages/ui/src/components/message-part/parts/text.tsx`:
- Around line 78-84: The handleCopy function currently calls
navigator.clipboard.writeText(content) without error handling; wrap that await
call in a try/catch so clipboard failures don't bubble up — on success keep the
existing setCopied(true) and timeout to reset, and on error handle gracefully
(e.g., log or show a fallback notification and ensure setCopied(false) or no
visual success state is set). Specifically update handleCopy to try { await
navigator.clipboard.writeText(content); setCopied(true); setTimeout(...); }
catch (err) { /* handle/log error and avoid showing copied state */ } to ensure
robust behavior when clipboard access is denied.
In `@packages/ui/src/components/message-part/session-link.ts`:
- Around line 22-24: The current logic uses path.indexOf("/session") which
matches substrings and can create malformed links; instead, treat the path as
segments by splitting on "/" (use the existing path and id variables), find the
first segment equal to "session" (not just containing it), and rebuild the URL
by joining segments up to that segment then appending "session" and the id;
update the code that computes idx and the returned string so it uses
segment-aware matching (e.g., operate on path.split("/"), check segments ===
"session", compute the slice accordingly) to ensure correct route matching.
In `@packages/ui/src/components/message-part/tools/bash.tsx`:
- Around line 57-63: The handleCopy function currently awaits
navigator.clipboard.writeText(content) without error handling; wrap that call in
try/catch inside handleCopy (and only call it if navigator.clipboard and
writeText exist) to catch permission or API errors, setCopied true on success,
and on failure fall back to a safe alternative (e.g., create a temporary
textarea and use document.execCommand('copy')) or surface an error
state/notification so unhandled promise rejections are avoided; reference
handleCopy, navigator.clipboard.writeText, setCopied, and the temporary timeout
reset when implementing the fix.
In `@packages/ui/src/components/message-part/tools/webfetch.tsx`:
- Around line 13-17: The code currently returns props.input.url directly via
createMemo (the url variable) which allows unsafe protocols; update the
createMemo that computes url to parse and validate the input: if value is not a
string return ""; otherwise try new URL(value) and only return the string if its
protocol is "http:" or "https:" (reject "javascript:", "data:", etc. by
returning ""); ensure the same protocol check is applied to the other createMemo
usage covering lines ~29-39 so any href rendered only uses the validated url
variable derived from props.input.url.
In `@packages/ui/src/components/message-part/tools/write.tsx`:
- Line 19: The current createMemo calls
getDiagnostics(props.metadata.diagnostics, props.input.filePath) but
dereferences props.metadata and will throw if metadata is null/undefined; update
the memo to guard metadata (e.g. use optional chaining or a default) before
accessing diagnostics—call getDiagnostics(props.metadata?.diagnostics ?? [],
props.input.filePath) or pass undefined when metadata is absent so createMemo
and getDiagnostics handle missing metadata safely; change the expression around
createMemo and getDiagnostics accordingly.
- Line 50: The Show condition hides the accordion when props.input.content is an
empty string because it checks truthiness; update the condition that currently
uses "props.input.content && path()" to explicitly test for null/undefined
instead (e.g., check props.input.content != null) so empty-string content still
renders while still gating on path() — locate the Show around the MessagePart
write UI where props.input.content and path() are used and replace the truthy
check with a nullish check.
---
Nitpick comments:
In `@packages/ui/src/components/message-part/message-router.tsx`:
- Around line 11-21: The code uses `as` assertions to cast props.message to
UserMessage/AssistantMessage in message-router.tsx (used by UserMessageDisplay
and AssistantMessageDisplay); replace those casts with proper type predicate
functions (e.g., isUserMessage(m): m is UserMessage and isAssistantMessage(m): m
is AssistantMessage) that check m.role === "user" / "assistant", use those
predicates in the Show conditions to let TypeScript narrow the type, and remove
the `as` assertions from the UserMessageDisplay and AssistantMessageDisplay
props so the components receive correctly narrowed types without bypassing the
compiler.
In `@packages/ui/src/components/message-part/registry.ts`:
- Around line 32-34: Update registerPartComponent to detect and warn on
duplicate registrations in development: check PART_MAPPING[type] before
assigning and, if a different component is already registered and
process.env.NODE_ENV !== 'production' (or import a DEV flag), log a console.warn
or use the project's logger that includes the conflicting type and the existing
vs new component identifiers; then still allow overwrite (or optionally skip
assignment) depending on desired behavior. Reference: registerPartComponent and
PART_MAPPING.
- Around line 58-65: The state currently stores objects with { name, render? }
but getTool only returns the render, so simplify state to Record<string,
ToolComponent | undefined> and update registerTool to assign input.render
directly to state[input.name]; adjust any references to state shape accordingly
(notably registerTool and getTool) so they match the PART_MAPPING pattern and
remove the unused name field.
In `@packages/ui/src/components/message-part/tools/edit.tsx`:
- Around line 40-43: The code uses a non-null assertion when calling
getDirectory but optional chaining in the conditional; replace the non-null
assertion with consistent optional handling by calling
getDirectory(props.input.filePath) (or getDirectory(props.input.filePath ?? '')
if you prefer a fallback) inside the Show block and keep the existing Show
condition and pending() check; update occurrences around the Show component,
pending(), props.input.filePath and getDirectory usage to remove the trailing
"!" for consistency.
In `@packages/ui/src/components/message-part/tools/question.tsx`:
- Around line 11-12: The current casts in createMemo for questions and answers
assume props.input.questions and props.metadata.answers match
QuestionInfo[]/QuestionAnswer[] at runtime; add defensive runtime checks (e.g.,
Array.isArray and simple item shape/type guards or a utility
isQuestionInfo/isQuestionAnswer) before casting and fall back to [] if
validation fails so accesses like q.question in the component are safe; update
the createMemo calls that initialize questions and answers to use these
validators (or a small helper validateArray<T>(value, predicate)) to ensure only
properly-shaped objects are returned.
In `@packages/ui/src/components/tool-info.ts`:
- Around line 67-73: The function signature for toolInfoForInput places optional
params (input, metadata) before the required i18n, making their defaults
effectively unreachable; fix by reordering required params before defaults or by
collapsing i18n into an options object. For example, change the signature of
toolInfoForInput so i18n: UiI18n appears immediately after tool (or replace the
last options param with an options object that includes i18n and
unknownSubtitle), then update all call sites to pass i18n in the new position or
as part of the options; adjust the function body to read i18n from its new
location and keep input and metadata defaulting to {}.
🪄 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: Pro Plus
Run ID: 771f8dd5-974d-4e84-a4aa-80a03f4e593d
📒 Files selected for processing (41)
packages/app/src/no-mode-picker.test.tspackages/ui/src/components/message-part-registry.test.tspackages/ui/src/components/message-part-stale.test.tspackages/ui/src/components/message-part.tsxpackages/ui/src/components/message-part/agent-tone.tspackages/ui/src/components/message-part/assistant-message-display.tsxpackages/ui/src/components/message-part/assistant-parts.tsxpackages/ui/src/components/message-part/context-tool-group.tsxpackages/ui/src/components/message-part/context-tool-helpers.tspackages/ui/src/components/message-part/grouping.tspackages/ui/src/components/message-part/highlighted-text.tsxpackages/ui/src/components/message-part/markdown-render.tsxpackages/ui/src/components/message-part/message-router.tsxpackages/ui/src/components/message-part/parts/compaction-and-divider.tsxpackages/ui/src/components/message-part/parts/index.tspackages/ui/src/components/message-part/parts/reasoning.tsxpackages/ui/src/components/message-part/parts/text.tsxpackages/ui/src/components/message-part/parts/tool.tsxpackages/ui/src/components/message-part/registry.tspackages/ui/src/components/message-part/session-link.tspackages/ui/src/components/message-part/shared-utils.tspackages/ui/src/components/message-part/tools/_diagnostics.tsxpackages/ui/src/components/message-part/tools/_file-accordion.tsxpackages/ui/src/components/message-part/tools/agent.tsxpackages/ui/src/components/message-part/tools/apply-patch.tsxpackages/ui/src/components/message-part/tools/bash.tsxpackages/ui/src/components/message-part/tools/edit.tsxpackages/ui/src/components/message-part/tools/glob.tsxpackages/ui/src/components/message-part/tools/grep.tsxpackages/ui/src/components/message-part/tools/index.tspackages/ui/src/components/message-part/tools/list.tsxpackages/ui/src/components/message-part/tools/question.tsxpackages/ui/src/components/message-part/tools/read.tsxpackages/ui/src/components/message-part/tools/skill.tsxpackages/ui/src/components/message-part/tools/todowrite.tsxpackages/ui/src/components/message-part/tools/webfetch.tsxpackages/ui/src/components/message-part/tools/websearch.tsxpackages/ui/src/components/message-part/tools/worktree.tsxpackages/ui/src/components/message-part/tools/write.tsxpackages/ui/src/components/message-part/user-message.tsxpackages/ui/src/components/tool-info.ts
Perf delta summaryComparator: pass
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the monolithic message-part.tsx component into a modular structure within a new message-part/ directory, separating concerns for different message parts and tools. The changes include extracting rendering logic into dedicated components and updating the test suite to accommodate the new file structure. Feedback suggests replacing window.location.assign with the application's router for navigation to prevent unnecessary full page reloads.
Owner
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
This was referenced May 14, 2026
This was referenced May 15, 2026
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
Split
packages/ui/src/components/message-part.tsxinto focused message-part modules while keepingmessage-part.tsxas the public facade.Moved the tool-info switch behind
toolInfoForInput()inpackages/ui/src/components/tool-info.ts, and added source guards for registration coverage, facade import boundaries, registry independence, hidden tools, default-open behavior, and deferred heavy tool bodies.Why
#601 Area A calls out
message-part.tsxas too large to audit safely. This PR is the structural slice only: make the renderer easier to inspect without changing message, tool, streaming, grouping, or public import behavior.Related Issue
Part of #601.
Human Review Status
Pending. A human should make the final merge decision after reviewing the final diff and verification evidence.
Review Focus
AssistantPartsstill preserves grouping, staging,Index each, keys, and default open behavior.deferat the sameBasicToollayer.message-part.tsxfacade.Risk Notes
Medium structural risk because missing a side-effect import would not necessarily fail typecheck. Mitigated with
message-part-registry.test.tscoverage for part/tool registrations, hidden tools,partDefaultOpen,defer, facade import boundaries, and registry independence.No dependency, schema, permission, generated-file, or platform behavior changes intended.
Review follow-up also tightened defensive guards around clipboard failure, safe WebFetch hrefs, write-tool missing metadata, empty write content, and session path matching. These are hardening changes with no intended visible UI change.
A later CI follow-up stabilized shared app test mocks so Linux CI does not leak partial module mocks across concurrent test files.
Known baseline issue: the local E2E command below still has two failures, and both reproduced unchanged on the current
devcheckout. They are recorded as baseline failures, not introduced by this branch.How To Verify
Screenshots or Recordings
Not included. This is a pure structural refactor with no intended visible UI change. Electron smoke was still completed for the desktop shell and composer.
Checklist
dev, and my PR title and commit messages use Conventional Commits in EnglishSummary by CodeRabbit