fix(cli): render assistant <options> blocks as a numbered list in the remote transcript view#197
fix(cli): render assistant <options> blocks as a numbered list in the remote transcript view#197romerjon wants to merge 3 commits into
Conversation
… remote transcript view The Happier base system prompt instructs agents to append quick-reply options as an <options>...</options> XML block. The mobile/web app parses this at markdown render time and shows tappable buttons, and the Gemini backend extracts it at turn end, but the Claude remote-mode terminal transcript (formatClaudeMessageForInk / formatClaudeMessage) printed the raw XML to the user. Move the options parser from backends/gemini/utils to utils (it is no longer Gemini-specific), add formatTextWithOptionsForTerminal, and route assistant text and result summaries through it so terminal surfaces show a readable numbered list instead of raw XML.
WalkthroughChangesCLI behavior updates
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant AssistantOutput
participant segmentTrailingOptions
participant formatTextWithOptionsForTerminal
participant InkTerminal
AssistantOutput->>segmentTrailingOptions: provide accumulated text
segmentTrailingOptions->>formatTextWithOptionsForTerminal: return prose and trailing options
formatTextWithOptionsForTerminal->>InkTerminal: render numbered Options list
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
Greptile SummaryThis PR makes Claude terminal transcripts render assistant options as readable text. The main changes are:
Confidence Score: 5/5The changed flow looks mergeable after small cleanup to option-block edge cases.
apps/cli/src/utils/optionsParser.ts and apps/cli/src/ui/messageFormatterInk.ts
|
| Filename | Overview |
|---|---|
| apps/cli/src/utils/optionsParser.ts | Moved the parser into shared utilities and added terminal list rendering, with a remaining edge case for multiple option blocks. |
| apps/cli/src/ui/messageFormatterInk.ts | Applies terminal options formatting to Ink transcript messages, with a remaining edge case for split text blocks. |
| apps/cli/src/ui/messageFormatter.ts | Applies terminal options formatting to the non-Ink Claude message formatter. |
| apps/cli/src/backends/gemini/runGemini.ts | Updates the Gemini backend import to the moved parser location. |
| apps/cli/src/utils/optionsParser.test.ts | Adds focused tests for parser extraction and terminal formatting. |
| apps/cli/src/ui/messageFormatterInk.test.ts | Adds Ink formatter tests for assistant text and result summaries. |
Reviews (1): Last reviewed commit: "fix(cli): render assistant <options> blo..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| * @returns The text with the options block rendered as a numbered list | ||
| */ | ||
| export function formatTextWithOptionsForTerminal(text: string): string { | ||
| const { text: textWithoutOptions, options } = parseOptionsFromText(text); |
There was a problem hiding this comment.
When assistant or result text contains two complete <options> blocks, parseOptionsFromText() only matches and removes the first one. The terminal formatter then renders the first block as a list but leaves the later block as raw XML, so the transcript still shows the markup this change is meant to hide.
| for (const block of assistantMsg.message.content) { | ||
| if (block.type === 'text') { | ||
| messageBuffer.addMessage(block.text || '', 'assistant') | ||
| messageBuffer.addMessage(formatTextWithOptionsForTerminal(block.text || ''), 'assistant') |
There was a problem hiding this comment.
This formats each assistant text block on its own, so a complete options block is only recognized when <options> and </options> are in the same block.text. If the SDK message carries adjacent text blocks where the options XML is split across them, both fragments pass through unchanged and the remote transcript still displays raw XML.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
apps/cli/src/ui/messageFormatterInk.test.ts (1)
6-13: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider adding
message.roleto the assistant fixture to avoid theunknowncast.
buildAssistantMessagecasts throughunknownbecause the fixture is missingmessage.role: 'assistant'required bySDKAssistantMessage. Adding the field would let the cast be simplified toas SDKAssistantMessageor removed entirely with a typed builder.♻️ Optional typed fixture improvement
function buildAssistantMessage(text: string): SDKMessage { return { type: 'assistant', message: { + role: 'assistant', content: [{ type: 'text', text }], }, - } as unknown as SDKAssistantMessage + } as SDKAssistantMessage }🤖 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 `@apps/cli/src/ui/messageFormatterInk.test.ts` around lines 6 - 13, Update buildAssistantMessage to include message.role: 'assistant' in the fixture, then remove the unnecessary unknown cast and use a direct SDKAssistantMessage type assertion or a properly typed return value.apps/cli/src/ui/messageFormatterInk.ts (1)
76-76: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign null-handling style with
messageFormatter.tsfor consistency.Line 76 uses
block.text || ''while the equivalent inmessageFormatter.tsline 80 usesblock.text ?? ''. Forstring | undefinedboth are equivalent, but??is more precise (preserves empty strings). Line 99'sresultMsg.result || ''is redundant since the surroundingif ('result' in resultMsg && resultMsg.result)already narrows to truthystring—messageFormatter.tsline 103 correctly omits the fallback.♻️ Optional consistency fix
- messageBuffer.addMessage(formatTextWithOptionsForTerminal(block.text || ''), 'assistant') + messageBuffer.addMessage(formatTextWithOptionsForTerminal(block.text ?? ''), 'assistant')- messageBuffer.addMessage(formatTextWithOptionsForTerminal(resultMsg.result || ''), 'result') + messageBuffer.addMessage(formatTextWithOptionsForTerminal(resultMsg.result), 'result')Also applies to: 99-99
🤖 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 `@apps/cli/src/ui/messageFormatterInk.ts` at line 76, Update message formatting in the relevant handlers to match messageFormatter.ts: replace block.text || '' with block.text ?? '', and remove the redundant || '' fallback from resultMsg.result after the existing truthiness check. Use the surrounding message-buffer calls to locate both changes.apps/cli/src/utils/optionsParser.test.ts (1)
59-81: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a test for text after the options block.
The tests cover text before options, options-only, no options, and incomplete blocks. A message with text both before and after the
<options>block (e.g.,"Before\n<options>...</options>\nAfter") isn't tested forformatTextWithOptionsForTerminal.parseOptionsFromTextshould preserve trailing text viatext.replace(optionsRegex, '').trim(), but an explicit test would guard against regressions in the text-assembly logic.🧪 Suggested additional test
describe('formatTextWithOptionsForTerminal', () => { + it('preserves text after the options block', () => { + const input = 'Before text\n<options>\n<option>A</option>\n<option>B</option>\n</options>\nAfter text'; + const result = formatTextWithOptionsForTerminal(input); + expect(result).toBe('Before text\n\nAfter text\n\nOptions:\n 1. A\n 2. B'); + expect(result).not.toContain('<options>'); + }); + it('returns text unchanged for an incomplete options block', () => {🤖 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 `@apps/cli/src/utils/optionsParser.test.ts` around lines 59 - 81, Add a test in the formatTextWithOptionsForTerminal suite covering text both before and after a complete <options> block, such as “Before\n<options>...</options>\nAfter”. Assert the formatted output preserves both surrounding text, inserts the numbered Options list between them, and removes the original options markup.
🤖 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.
Nitpick comments:
In `@apps/cli/src/ui/messageFormatterInk.test.ts`:
- Around line 6-13: Update buildAssistantMessage to include message.role:
'assistant' in the fixture, then remove the unnecessary unknown cast and use a
direct SDKAssistantMessage type assertion or a properly typed return value.
In `@apps/cli/src/ui/messageFormatterInk.ts`:
- Line 76: Update message formatting in the relevant handlers to match
messageFormatter.ts: replace block.text || '' with block.text ?? '', and remove
the redundant || '' fallback from resultMsg.result after the existing truthiness
check. Use the surrounding message-buffer calls to locate both changes.
In `@apps/cli/src/utils/optionsParser.test.ts`:
- Around line 59-81: Add a test in the formatTextWithOptionsForTerminal suite
covering text both before and after a complete <options> block, such as
“Before\n<options>...</options>\nAfter”. Assert the formatted output preserves
both surrounding text, inserts the numbered Options list between them, and
removes the original options markup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 73c4fa6f-e09e-49f3-9bbe-878305bfe7bb
📒 Files selected for processing (6)
apps/cli/src/backends/gemini/runGemini.tsapps/cli/src/ui/messageFormatter.tsapps/cli/src/ui/messageFormatterInk.test.tsapps/cli/src/ui/messageFormatterInk.tsapps/cli/src/utils/optionsParser.test.tsapps/cli/src/utils/optionsParser.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/cli/src/backends/claude/runClaude.ts (1)
846-857: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winUse the resolved bridge state before deciding to rebuild
nextLocalPermissionBridgeEnabledis still raw meta here, soHAPPIER_LOCAL_PERMISSION_BRIDGE=0/1can retriggerrebuildLocalPermissionBridge()on unrelated meta churn even when the effective state is unchanged.- That disposes/recreates the bridge unnecessarily and can interrupt in-flight permission requests.
- The same fix is needed in the fast-start path at
apps/cli/src/backends/claude/runClaude.ts:1589-1604.🤖 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 `@apps/cli/src/backends/claude/runClaude.ts` around lines 846 - 857, Resolve nextLocalPermissionBridgeEnabled through resolveLocalPermissionBridgeEnabledOverride before comparing it with localPermissionBridgeEnabled in the shown meta-update path, and use that effective value for subsequent assignment and rebuild decisions. Apply the same change in the fast-start path near the existing bridge state initialization so raw metadata changes masked by HAPPIER_LOCAL_PERMISSION_BRIDGE do not trigger unnecessary rebuildLocalPermissionBridge calls.
🤖 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.
Outside diff comments:
In `@apps/cli/src/backends/claude/runClaude.ts`:
- Around line 846-857: Resolve nextLocalPermissionBridgeEnabled through
resolveLocalPermissionBridgeEnabledOverride before comparing it with
localPermissionBridgeEnabled in the shown meta-update path, and use that
effective value for subsequent assignment and rebuild decisions. Apply the same
change in the fast-start path near the existing bridge state initialization so
raw metadata changes masked by HAPPIER_LOCAL_PERMISSION_BRIDGE do not trigger
unnecessary rebuildLocalPermissionBridge calls.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 889aace1-6645-4dcb-903d-dfd0cbc29c56
📒 Files selected for processing (3)
apps/cli/src/backends/claude/remote/claudeRemoteMetaState.tsapps/cli/src/backends/claude/runClaude.tspackages/protocol/src/prompts/systemPromptBaseV1.ts
… first (in-place replacement; empty blocks dropped)
5183076 to
a8e6df2
Compare
|
Addressed the multi-block edge case from Greptile's review: Also force-pushed to remove an unrelated commit that briefly leaked into this branch (prompt/tooling changes preferring the native question tool — those now live on a separate branch and may become a follow-up PR so this one stays a pure rendering fix). The XML-split-across-streamed-text-blocks case remains out of scope here since it needs turn-end buffering — same category as related leak #1 in the PR body. |
|
@romerjon Thank you very much for this contribution — this is a genuinely useful fix, and we really appreciate the care you put into identifying the different surfaces, documenting the repro, adding tests, and following up on the initial review feedback. The core user-facing direction is exactly right: Happier-owned terminal views should show readable numbered choices rather than leaking the raw We did a deeper review against the current local worktree and found a few cases we would like to address before merging:
The happy path and your multi-block follow-up both work, and we did not identify a security, auth, database, or public API concern. This is mainly about making the presentation boundary robust and preventing the options grammar from becoming split across several owners. Would you be happy either to apply these changes on this PR, or to enable Allow edits from maintainers so we can help apply them directly before merging? sent by Codex |
|
Thanks for the thorough review — the direction you're describing is better than what I shipped, and I'm happy to go with it. I've just enabled Allow edits from maintainers on this branch, so please feel free to push directly; I'm also glad to do the revisions myself if you'd rather I take a pass first — whichever is less work on your side. Agreeing point by point:
One heads-up for context, not scope-creep: I have a separate local branch that reworks the injected options prompt to prefer a native question tool (AskUserQuestion) with XML only as fallback. I'm deliberately not opening that as a competing PR — it intersects your canonical-parser plan and the prompt contract, so I'd rather raise it as a discussion once this lands and defer to whatever shape you settle on here. Let me know if you'd like me to apply the six revisions on this PR or leave it to you. |
…-block segmenter Addresses maintainer review on happier-dev#197: - single segmentTrailingOptions() owns the block/item grammar; terminal formatter and Gemini adapter both consume it (drop parseOptionsFromText / hasIncompleteOptions divergent duplicates) - match only the turn-final trailing block (end-anchored, last-opener) so literal/fenced <options> mid-text is never rewritten - preserve surrounding whitespace verbatim; only the trailing block span changes - assemble contiguous SDK text blocks before formatting so an options block split across adjacent text fragments is recognised - revert the dead-code parallel messageFormatter.ts to original - typed SDKAssistantMessage fixtures; cover fenced markup, exact whitespace, empty/unclosed blocks, last-block-only, and split-across-blocks Gemini app message stays trimmed (byte-identical); terminal path preserves whitespace. 209 tests green, typecheck + build + import-cycles clean.
|
Applied all six revisions in
One deliberate call worth flagging: Gemini's app-facing message keeps its previous |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/cli/src/utils/optionsParser.ts (1)
79-95: 📐 Maintainability & Code Quality | 🔵 TrivialExtract a named
interfacefor the segmentation result.
segmentTrailingOptionsreturns an anonymous inline object type that gets destructured identically in this file (formatTextWithOptionsForTerminal) and inrunGemini.ts. A named interface would satisfy the "prefer interface for object shapes"/"prefer explicit exported types" guidelines and give both consumers a shared, documented contract instead of re-deriving the shape at each call site.As per coding guidelines,
apps/cli/**/*.{ts,tsx}: "Preferinterfaceovertypefor defining object shapes in TypeScript" and "Prefer explicit exported types and named exports in TypeScript modules."♻️ Proposed refactor
+export interface TrailingOptionsSegment { + before: string; + options: string[]; + hasIncompleteTrailingOptions: boolean; +} + -export function segmentTrailingOptions(text: string): { - before: string; - options: string[]; - hasIncompleteTrailingOptions: boolean; -} { +export function segmentTrailingOptions(text: string): TrailingOptionsSegment {🤖 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 `@apps/cli/src/utils/optionsParser.ts` around lines 79 - 95, Define and export a named interface for the result returned by segmentTrailingOptions, including before, options, and hasIncompleteTrailingOptions, then use it as the function’s return type. Preserve the existing return values and update consumers such as formatTextWithOptionsForTerminal and runGemini.ts to rely on this shared exported contract.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@apps/cli/src/utils/optionsParser.ts`:
- Around line 79-95: Define and export a named interface for the result returned
by segmentTrailingOptions, including before, options, and
hasIncompleteTrailingOptions, then use it as the function’s return type.
Preserve the existing return values and update consumers such as
formatTextWithOptionsForTerminal and runGemini.ts to rely on this shared
exported contract.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 775b2d4d-71c4-4d1c-b12a-4fce32afa3d5
📒 Files selected for processing (5)
apps/cli/src/backends/gemini/runGemini.tsapps/cli/src/ui/messageFormatterInk.test.tsapps/cli/src/ui/messageFormatterInk.tsapps/cli/src/utils/optionsParser.test.tsapps/cli/src/utils/optionsParser.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/cli/src/ui/messageFormatterInk.test.ts
2 participants
Problem
The Happier base system prompt (
HAPPIER_BASE_SYSTEM_PROMPT_OPTIONS_V1,packages/protocol/src/prompts/systemPromptBaseV1.ts) instructs agents to append quick-reply options as an<options><option>…</option></options>XML block. The mobile/web app extracts this at markdown render time (apps/ui/sources/components/markdown/parseMarkdownBlock.ts) and shows tappable buttons, and the Gemini backend extracts it at turn end (optionsParser.ts), but the Claude remote-mode terminal transcript view prints the raw XML to the user:formatClaudeMessageForInk(apps/cli/src/ui/messageFormatterInk.ts) passes assistant text blocks and the result summary straight to the Ink message buffer.Repro
Run
happier claude, switch the session to remote mode, and from the app send a prompt that makes the agent ask a clarifying question. The terminal transcript shows the literal<options>…</options>block; the app shows buttons.Fix
optionsParser.tsfrombackends/gemini/utilstoutils(it is no longer Gemini-specific; import inrunGemini.tsupdated).formatTextWithOptionsForTerminal(), which replaces a complete options block with a readableOptions: 1./2./…list. Incomplete or absent blocks pass through unchanged.messageFormatterInk.tsand the parallelmessageFormatter.ts.Behavior on every other surface is unchanged.
How tested
yarn workspace @happier-dev/cli typecheck,test:import-cycles, and vitest are all green; added unit tests for the parser/formatter (src/utils/optionsParser.test.ts) and forformatClaudeMessageForInkvia a realMessageBuffer(src/ui/messageFormatterInk.test.ts).Related leaks noticed while investigating (out of scope, happy to file separately)
acpCommonHandlers.ts), so the XML also shows there — fixing needs a turn-end rewrite of the last buffer message.normalizeTurnAssistantText) does not strip the block, so push previews can contain the collapsed XML.codingPromptBehaviorV1.responseOptions, a behavior change since the app mirrors local sessions.AI disclosure (per CONTRIBUTING)
Investigated and implemented with Claude under my direction; I defined the bug, the surface audit, and the fix shape, and verified typecheck + tests on my machine.
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
Render assistant
<options>blocks as numbered lists in the CLI terminal viewformatTextWithOptionsForTerminalto optionsParser.ts, which replaces a trailing<options>...</options>block with a numbered list while leaving all preceding text intact.segmentTrailingOptionsinstead of the oldparseOptionsFromText, so only trailing options blocks are parsed and removed from the displayed message.<options>XML is no longer shown in assistant or result messages; only trailing blocks are converted — mid-text or incomplete (still-streaming) blocks are left as-is.Macroscope summarized 9782746.
Summary by CodeRabbit
New Features
Bug Fixes