tsk-xocdcd [OPEN] taOStalk s1: content_blocks types + renderContent - #2153
tsk-xocdcd [OPEN] taOStalk s1: content_blocks types + renderContent#2153jaylfc wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughMessages now support typed content blocks, render each block kind with an unsupported fallback, and preserve legacy markdown rendering when blocks are absent or empty. Chat rows pass structured content to the renderer, with tests covering both paths. ChangesStructured content rendering
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant MessageList
participant renderContent
participant renderContentBlock
MessageList->>renderContent: pass message text and content_blocks
renderContent->>renderContentBlock: render each content block
renderContentBlock-->>renderContent: block output or unsupported fallback
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
PR Summary by QodoAdd message content_blocks typing and renderContent support
AI Description
Diagram
High-Level Assessment
Files changed (3)
|
|
nemotron-super review VERDICT: No blocking issues found
Automated first-pass review by the nemotron-super lane. The lead still reviews before merge. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@desktop/src/apps/MessagesApp.tsx`:
- Around line 260-269: Update the content-block rendering switch in
MessagesApp.tsx around the supported block-kind cases to render each known
payload, at minimum displaying TextContentBlock.text, while reserving the
unsupported fallback for unknown or unexpected runtime kinds. Update
desktop/src/apps/chat/__tests__/render-helpers.test.tsx lines 64-98 to assert
rendered content for supported text, thinking, tool_call, and status blocks and
retain fallback coverage only for unknown kinds.
🪄 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: 0f84c0c3-2c82-4be8-9804-a97a5f1da986
📒 Files selected for processing (3)
desktop/src/apps/MessagesApp.tsxdesktop/src/apps/chat/MessageList.tsxdesktop/src/apps/chat/__tests__/render-helpers.test.tsx
| case "text": | ||
| case "thinking": | ||
| case "tool_call": | ||
| case "status": | ||
| case "unknown": | ||
| default: | ||
| return ( | ||
| <div key={`block-${index}`} className="text-shell-text-tertiary text-xs italic"> | ||
| unsupported block: {block.kind} | ||
| </div> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Render known block payloads instead of marking them unsupported.
Once content_blocks is non-empty, legacy content is discarded. Lines 260-269 therefore hide every text, thinking, tool_call, and status payload and show only fallback labels.
desktop/src/apps/MessagesApp.tsx#L260-L269: render each supported kind (at minimumTextContentBlock.text); reserve the fallback forunknownand unexpected runtime kinds.desktop/src/apps/chat/__tests__/render-helpers.test.tsx#L64-L98: assert rendered block content for supported kinds and retain fallback coverage only for unknown kinds.
📍 Affects 2 files
desktop/src/apps/MessagesApp.tsx#L260-L269(this comment)desktop/src/apps/chat/__tests__/render-helpers.test.tsx#L64-L98
🤖 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 `@desktop/src/apps/MessagesApp.tsx` around lines 260 - 269, Update the
content-block rendering switch in MessagesApp.tsx around the supported
block-kind cases to render each known payload, at minimum displaying
TextContentBlock.text, while reserving the unsupported fallback for unknown or
unexpected runtime kinds. Update
desktop/src/apps/chat/__tests__/render-helpers.test.tsx lines 64-98 to assert
rendered content for supported text, thinking, tool_call, and status blocks and
retain fallback coverage only for unknown kinds.
|
nemotron-ultra-orB review VERDICT: Major correctness bug - all content block types render as "unsupported block" fallback
Automated first-pass review by the nemotron-ultra-orB lane. The lead still reviews before merge. |
Code Review by Qodo
1. Non-equality assertions in tests
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| it("renders one fallback line per block", () => { | ||
| const blocks: ContentBlock[] = [ | ||
| { kind: "text", text: "a" }, | ||
| { kind: "status", text: "b" }, | ||
| ]; | ||
| const { container } = render(<div>{renderContent("", blocks)}</div>); | ||
| expect(container.querySelectorAll("div").length).toBeGreaterThanOrEqual(2); | ||
| }); |
There was a problem hiding this comment.
1. Non-equality assertions in tests 📜 Skill insight ≡ Correctness
The new tests use range-based assertions (e.g., toBeGreaterThanOrEqual) for deterministic expectations, which can mask regressions by still passing when the output is wrong. Assertions here should check exact equality/length for deterministic results.
Agent Prompt
## Issue description
New tests use tolerance/range assertions (e.g., `toBeGreaterThanOrEqual`) where results should be deterministic, which can allow regressions to pass.
## Issue Context
Compliance requires tests to assert exact equality for deterministic values rather than tolerance/range checks.
## Fix Focus Areas
- desktop/src/apps/chat/__tests__/render-helpers.test.tsx[52-55]
- desktop/src/apps/chat/__tests__/render-helpers.test.tsx[91-98]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| function renderContentBlock(block: ContentBlock, index: number): React.ReactElement { | ||
| switch (block.kind) { | ||
| case "text": | ||
| case "thinking": | ||
| case "tool_call": | ||
| case "status": | ||
| case "unknown": | ||
| default: | ||
| return ( | ||
| <div key={`block-${index}`} className="text-shell-text-tertiary text-xs italic"> | ||
| unsupported block: {block.kind} | ||
| </div> | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| export function renderContent(text: string, content_blocks?: ContentBlock[]) { | ||
| if (content_blocks && content_blocks.length > 0) { | ||
| return content_blocks.map((block, i) => renderContentBlock(block, i)); | ||
| } |
There was a problem hiding this comment.
2. Blocks always render unsupported 🐞 Bug ≡ Correctness
renderContent() switches to content_blocks when present, but renderContentBlock() returns the “unsupported block” fallback for every kind, so any message with non-empty content_blocks becomes unreadable (the legacy markdown path is skipped).
Agent Prompt
## Issue description
`renderContentBlock()` currently falls through to the same fallback UI for *all* block kinds, but `renderContent()` short-circuits to blocks when `content_blocks` is non-empty. This causes real structured messages to render only “unsupported block: …” lines and bypasses the existing markdown renderer.
## Issue Context
`MessageList` now passes `msg.content_blocks` into `renderContent`, and the backend can broadcast messages containing `content_blocks`.
## Fix Focus Areas
- desktop/src/apps/MessagesApp.tsx[258-297]
- desktop/src/apps/chat/MessageList.tsx[536-545]
## Suggested fix approach
- Split the existing markdown logic into a helper (e.g. `renderMarkdown(text): ReactNode[]`).
- Implement per-kind rendering:
- `text`: render via the markdown helper using `block.text`.
- `thinking`: render markdown with collapsed disclosure support.
- `tool_call`: render a compact card using `name`, `status`, and previews.
- `status`: render a muted single-line block.
- unknown/unsupported: keep the fallback.
- Ensure unknown kinds do not prevent showing other blocks.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| export interface TextContentBlock { | ||
| kind: "text"; | ||
| text: string; | ||
| } | ||
|
|
||
| export interface ThinkingContentBlock { | ||
| kind: "thinking"; | ||
| text: string; | ||
| collapsed?: boolean; | ||
| } | ||
|
|
||
| export interface ToolCallContentBlock { | ||
| kind: "tool_call"; | ||
| call_id: string; | ||
| name: string; | ||
| input_preview?: string; | ||
| status: "running" | "done" | "error"; | ||
| result_preview?: string; | ||
| } | ||
|
|
||
| export interface StatusContentBlock { | ||
| kind: "status"; | ||
| text: string; | ||
| } | ||
|
|
||
| export interface UnknownContentBlock { | ||
| kind: "unknown"; | ||
| [key: string]: unknown; | ||
| } | ||
|
|
||
| export type ContentBlock = | ||
| | TextContentBlock | ||
| | ThinkingContentBlock | ||
| | ToolCallContentBlock | ||
| | StatusContentBlock | ||
| | UnknownContentBlock; |
There was a problem hiding this comment.
3. Content_blocks kind/type mismatch 🐞 Bug ≡ Correctness
Frontend ContentBlock definitions and the renderer depend on a kind discriminator, but backend docs/tests for content_blocks use a type field, so existing stored/emitted blocks won’t match and will render as unsupported (and currently also hide the message markdown due to the early block preference).
Agent Prompt
## Issue description
The frontend `ContentBlock` schema uses `kind`, but the backend’s documented/exampled `content_blocks` shape uses `type`. With the new `renderContent()` behavior, any message carrying `{type: ...}` blocks will not match the discriminant logic and will render incorrectly.
## Issue Context
- Backend design docs and backend unit tests show `content_blocks` entries using `type`.
- The chat API accepts and broadcasts `content_blocks` verbatim.
## Fix Focus Areas
- desktop/src/apps/MessagesApp.tsx[161-277]
- desktop/src/apps/chat/__tests__/render-helpers.test.tsx[63-99]
- docs/design/message-hub-core.md[89-97]
- tests/test_chat_messages.py[211-218]
## Suggested fix approach
- Decide on the canonical discriminator (`type` vs `kind`) and align the frontend with the backend contract.
- If compatibility is required, support both:
- In the renderer, derive `const kind = (block as any).kind ?? (block as any).type;`
- Update the TS types to model the backend shape (e.g., `type: string`) or a union that accepts both fields.
- Update tests to use the real payload key (`type`) and verify actual rendering of the corresponding blocks.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| describe("renderContent with content_blocks", () => { | ||
| it("renders unknown-kind fallback for every block kind", () => { | ||
| const blocks: ContentBlock[] = [ | ||
| { kind: "text", text: "hello" }, | ||
| { kind: "thinking", text: "thinking...", collapsed: true }, | ||
| { kind: "tool_call", call_id: "c1", name: "bash", status: "running" }, | ||
| { kind: "status", text: "done" }, | ||
| { kind: "unknown" }, | ||
| ]; | ||
| const { container } = render(<div>{renderContent("", blocks)}</div>); | ||
| const text = container.textContent || ""; | ||
| expect(text).toContain("unsupported block: text"); | ||
| expect(text).toContain("unsupported block: thinking"); | ||
| expect(text).toContain("unsupported block: tool_call"); | ||
| expect(text).toContain("unsupported block: status"); | ||
| expect(text).toContain("unsupported block: unknown"); | ||
| }); |
There was a problem hiding this comment.
4. Tests assert placeholder behavior 🐞 Bug ⚙ Maintainability
The new unit test asserts that known block kinds render as “unsupported block…”, locking in placeholder behavior and preventing the test suite from catching the missing rendering for text/thinking/tool_call/status.
Agent Prompt
## Issue description
The added tests currently validate that every supported block kind renders the unsupported fallback. Once block rendering is implemented (and per the slice design), these tests will be wrong and currently they don’t verify any real block output.
## Issue Context
These tests should instead assert that `text` shows its text, `status` shows its status line, etc., and only unknown/unhandled kinds produce the fallback.
## Fix Focus Areas
- desktop/src/apps/chat/__tests__/render-helpers.test.tsx[63-99]
- desktop/src/apps/MessagesApp.tsx[258-297]
## Suggested fix approach
- Replace the “unsupported block: text/thinking/…” expectations with assertions on the rendered content for each block kind.
- Keep a single test asserting the fallback behavior for truly unknown kinds (e.g., `{ type: "some_future_kind", ... }`).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
nemotron-ultra-kilo review VERDICT: Blocking issues found - renderContentBlock returns "unsupported block" for ALL known content block kinds (text, thinking, tool_call, status), making the feature non-functional.
Automated first-pass review by the nemotron-ultra-kilo lane. The lead still reviews before merge. |
|
Superseded by #2154, which is the same card (tsk-xocdcd) rebuilt with better test coverage (13 tests vs 12, and clearer test naming). Closing this one. This should never have existed - it is a straight violation of the one-PR-per-task rule I published as rulebook v1.2 point 4 this evening, and it wasted a throttle slot exactly as that rule predicts. The card was dispatched four times; when the lane found its branch already under review it renamed to That branch-rename path is correct for an ORPHANED branch with no PR attached, and wrong when a PR for the card is already open. Fixing the executor to check for an existing open PR by card id before creating one, so the rule is enforced mechanically rather than by my noticing at midnight. |
1 participant
Autonomous build of board card tsk-xocdcd.
Files:
desktop/src/apps/MessagesApp.tsx | 59 +++++++++++++++++++++-
desktop/src/apps/chat/MessageList.tsx | 4 +-
.../apps/chat/tests/render-helpers.test.tsx | 39 ++++++++++++++
3 files changed, 100 insertions(+), 2 deletions(-)
Summary by Gitar
ContentBlocktypes andrenderContentsupport inMessagesApp.tsxrenderContentwithcontent_blocksinrender-helpers.test.tsxThis will update automatically on new commits.
Summary by CodeRabbit
New Features
Bug Fixes