tsk-6ylbbx [OPEN] taOStalk s1: TextBlock + ThinkingBlock renderers ( - #2164
tsk-6ylbbx [OPEN] taOStalk s1: TextBlock + ThinkingBlock renderers (#2164jaylfc wants to merge 1 commit into
Conversation
|
Warning Review limit reached
Next review available in: 9 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
✨ 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 |
|
nemotron-super review VERDICT: No blocking issues found. Automated first-pass review by the nemotron-super lane. The lead still reviews before merge. |
PR Summary by QodoAdd ContentBlock renderers for text + thinking session-turn blocks
AI Description
Diagram
High-Level Assessment
Files changed (1)
|
|
nemotron-ultra-orB review VERDICT: Minor accessibility issue and missing tests; otherwise clean implementation.
Automated first-pass review by the nemotron-ultra-orB lane. The lead still reviews before merge. |
Code Review by Qodo
1. Index keys break state
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| }; | ||
|
|
||
| /* ------------------------------------------------------------------ */ | ||
| /* TextBlock — reuses the existing markdown renderer */ |
There was a problem hiding this comment.
1. Em dashes in comments 📜 Skill insight ✧ Quality
The new ContentBlocks.tsx file contains em dashes (—) in comment headers, which the checklist prohibits in public-facing text (including code comments). This can cause inconsistent typography and violates the no-em-dash policy.
Agent Prompt
## Issue description
The file introduces em dash characters (`—`) in comment headers, which are disallowed.
## Issue Context
The compliance rule bans em dashes in public-facing text, explicitly including code comments/docstrings.
## Fix Focus Areas
- desktop/src/apps/chat/ContentBlocks.tsx[31-32]
- desktop/src/apps/chat/ContentBlocks.tsx[45-46]
- desktop/src/apps/chat/ContentBlocks.tsx[91-92]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| export function renderContentBlock( | ||
| block: ContentBlock, | ||
| index: number, | ||
| ): React.ReactElement { | ||
| switch (block.kind) { | ||
| case "text": | ||
| return <TextBlock key={`block-${index}`} text={block.text ?? ""} />; | ||
| case "thinking": | ||
| return ( | ||
| <ThinkingBlock | ||
| key={`block-${index}`} | ||
| text={block.text ?? ""} | ||
| collapsed={block.collapsed} | ||
| /> | ||
| ); | ||
| default: | ||
| return <UnknownBlock key={`block-${index}`} kind={block.kind} />; | ||
| } | ||
| } |
There was a problem hiding this comment.
2. Blocks not rendered 🐞 Bug ≡ Correctness
ContentBlocks.tsx introduces renderContentBlock/TextBlock/ThinkingBlock, but the existing chat message body still renders msg.content via renderContent(), so structured blocks won’t appear anywhere in the current MessagesApp message view.
Agent Prompt
### Issue description
`ContentBlocks.tsx` defines block renderers and a dispatcher (`renderContentBlock`), but nothing in the current chat message view calls it. The UI still renders message bodies via `renderContent(msg.content)`.
### Issue Context
If structured/session-turn blocks are expected to render in the main chat timeline, the message model and/or rendering path needs to detect and map blocks to `renderContentBlock` instead of always treating the body as a single markdown string.
### Fix Focus Areas
- desktop/src/apps/chat/ContentBlocks.tsx[118-136]
- desktop/src/apps/chat/MessageList.tsx[524-542]
### Suggested approach
1. Decide how blocks are represented on `MessageRow` (e.g., `content_blocks: ContentBlock[]` or embedded in `metadata`).
2. In `MessageList`, render either:
- `content_blocks.map(renderContentBlock)` when blocks exist, or
- fallback to `renderContent(msg.content)` when they don’t.
3. Ensure the new field is typed and populated from the server payload.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| export function ThinkingBlock({ | ||
| text, | ||
| collapsed = true, | ||
| }: { | ||
| text: string; | ||
| collapsed?: boolean; | ||
| }) { | ||
| const id = useId(); | ||
| const [open, setOpen] = useState(() => !collapsed); | ||
| const contentId = `thinking-content-${id}`; | ||
|
|
||
| return ( | ||
| <div className="mt-2 first:mt-0"> | ||
| <button | ||
| type="button" | ||
| onClick={() => setOpen(!open)} | ||
| className="flex items-center gap-1.5 text-[12px] font-medium text-shell-text-tertiary hover:text-shell-text transition-colors" | ||
| aria-expanded={open} | ||
| aria-controls={contentId} | ||
| > | ||
| <span>Thinking</span> | ||
| {open ? <ChevronUp size={12} /> : <ChevronDown size={12} />} | ||
| </button> | ||
| {open && ( | ||
| <div | ||
| id={contentId} | ||
| className="mt-1.5 text-[13px] text-shell-text-tertiary whitespace-pre-wrap opacity-70" | ||
| > | ||
| {text} | ||
| </div> | ||
| )} | ||
| </div> | ||
| ); | ||
| } | ||
|
|
||
| /* ------------------------------------------------------------------ */ | ||
| /* UnknownBlock — dim fallback for unrecognised kinds */ | ||
| /* ------------------------------------------------------------------ */ | ||
|
|
||
| /** | ||
| * Renders any block whose `kind` is not yet handled by a dedicated | ||
| * component. Shows a dim "unsupported block" line with the kind name | ||
| * so the user can see something was emitted even if the UI doesn't | ||
| * know how to render it yet. This is the slice-2 seam. | ||
| */ | ||
| export function UnknownBlock({ kind }: { kind: string }) { | ||
| return ( | ||
| <div className="text-[12px] text-shell-text-tertiary opacity-70"> | ||
| Unsupported block: {kind} | ||
| </div> | ||
| ); | ||
| } | ||
|
|
||
| /* ------------------------------------------------------------------ */ | ||
| /* Dispatcher */ | ||
| /* ------------------------------------------------------------------ */ | ||
|
|
||
| /** | ||
| * Maps a single content block to its renderer via a `kind` switch. | ||
| * Unknown kinds fall through to `UnknownBlock`. Each returned element | ||
| * gets a stable key based on its position so React can reconcile | ||
| * block arrays without duplicate-key warnings. | ||
| */ | ||
| export function renderContentBlock( | ||
| block: ContentBlock, | ||
| index: number, | ||
| ): React.ReactElement { | ||
| switch (block.kind) { | ||
| case "text": | ||
| return <TextBlock key={`block-${index}`} text={block.text ?? ""} />; | ||
| case "thinking": | ||
| return ( | ||
| <ThinkingBlock | ||
| key={`block-${index}`} | ||
| text={block.text ?? ""} | ||
| collapsed={block.collapsed} | ||
| /> | ||
| ); | ||
| default: | ||
| return <UnknownBlock key={`block-${index}`} kind={block.kind} />; | ||
| } |
There was a problem hiding this comment.
3. Index keys break state 🐞 Bug ≡ Correctness
renderContentBlock uses index-based keys (block-${index}), but ThinkingBlock has local open/closed
state, so inserting/removing/reordering blocks can cause React to reuse a previous block’s state for
a different block.
Agent Prompt
### Issue description
`renderContentBlock()` keys each block by its array index. Because `ThinkingBlock` stores UI state (`open`) via `useState`, React can incorrectly carry open/closed state across blocks when the block list changes (insertions/removals/reordering), causing the wrong block to appear expanded/collapsed.
### Issue Context
This is a common React reconciliation pitfall: index keys are only safe for strictly static lists that never change membership/order.
### Fix Focus Areas
- desktop/src/apps/chat/ContentBlocks.tsx[55-88]
- desktop/src/apps/chat/ContentBlocks.tsx[118-135]
### Suggested approach
1. Introduce/require a stable identity on `ContentBlock` (e.g., `id`, or reuse an existing stable field like `call_id` if it is guaranteed unique per block).
2. Compute keys from that stable identity (e.g., `key={block.id}` or `key={`${block.kind}:${block.call_id}`}`).
3. Only fall back to index if you can prove the list is immutable in order and membership (otherwise keep index as last-resort fallback only).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
nemotron-ultra-kilo review VERDICT: Acceptable with minor concerns
Automated first-pass review by the nemotron-ultra-kilo lane. The lead still reviews before merge. |
|
Reviewed. The design is right; three things to fix before this merges. All green checks, which is exactly why none of these were caught by CI. Right, and worth saying: 1. Nothing imports this file. Grep for 2. No tests. 136 lines with three renderers and a kind switch, and not one test file. 3. Three em dashes in the comment banners, lines 31, 45 and 91. Public repo. The executor prompt you were given explicitly says no em dashes in code, comments or commit messages, so this is a compliance miss rather than a style preference. None of this is hard to fix and the shape of the work is correct. Push the three fixes to the same branch, one PR per task. @jaylfc noting the pattern rather than just the instance: a PR can be fully green, correctly designed, and still be dead code with no tests. That is the third variant tonight of green meaning less than it appears. |
|
Closing as incomplete against its card's acceptance criteria, so the slot frees and the card can be redone cleanly. Two failures verified on this head:
The wiring is right this time (renderContentBlock switch dispatches to TextBlock/ThinkingBlock rather than leaving them dead), so that part of the previous hold is resolved. Redo with tests and no em dashes. |
1 participant
Autonomous build of board card tsk-6ylbbx.
Files:
desktop/src/apps/chat/ContentBlocks.tsx | 136 ++++++++++++++++++++++++++++++++
1 file changed, 136 insertions(+)
Summary by Gitar
ContentBlocks.tsximplementingTextBlock,ThinkingBlock, andUnknownBlockrenderersrenderContentBlockdispatcher function for mapping content blocks bykindThis will update automatically on new commits.