Polish TUI transcript and reasoning display#200
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (18)
WalkthroughAdds end-to-end provider reasoning support: the OpenAI provider gains a ChangesReasoning Stream: Provider → TUI
Sequence DiagramssequenceDiagram
participant Runtime as zeroruntime
participant Provider as OpenAI provider
participant ToolState as toolState
participant TUI as model
participant Transcript as transcript rows
Runtime->>Provider: stream SSE chunks
Provider->>ToolState: emitContent(delta.content)
ToolState->>ToolState: thinkTagSplitter.push()
ToolState->>TUI: StreamEventReasoning | StreamEventText
Note over Provider: Error/Idle/Tool boundary
Provider->>ToolState: flushContent()
ToolState->>TUI: emit buffered content
Provider->>ToolState: closeBufferedOpen()
Provider->>TUI: StreamEventError or StreamEventDone
Runtime->>Provider: OnReasoning(delta)
Provider->>TUI: sendAgentReasoning(delta)
TUI->>TUI: streamingReasoning += delta
Runtime->>Provider: OnText | OnToolCall
Provider->>Provider: flushReasoning()
Provider->>TUI: agentRowMsg(rowReasoning)
TUI->>Transcript: append reasoningTranscriptRow
TUI->>TUI: streamingReasoning = ""
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
🚥 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 docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@internal/providers/openai/provider.go`:
- Around line 188-190: The buffered content is flushed on line 188 when the
context is cancelled, but the sendEvent function suppresses non-error events
once ctx.Done() is closed, causing the flushed content to be discarded. To fix
this, ensure that the buffered content events from state.flushContent and
state.closeOpen are transmitted to the event stream immediately and
independently from the error event, either by sending them before the context
cancellation can suppress them or by using a mechanism that bypasses the
context-based suppression in sendEvent for these final buffered fragments. The
goal is to preserve the buffered text, reasoning, and tool fragments in the
events that reach the output despite the cancellation context.
In `@internal/tui/transcript_selection.go`:
- Around line 214-236: The expanded reasoning body lines in the selectable
transcript rendering are appended to the lines slice without width clamping,
which can cause text overflow on narrow terminals and impact selection hit
detection. In the loop that processes renderedLines (after the
renderReasoningBodyLines call), apply fitStyledLine(...) to clamp the rendered
line to the viewport width before appending it to the lines slice with the
leading spacing, similar to how the non-selectable reasoning renderer handles
this. Ensure the width parameter used in fitStyledLine matches the width
constraints already being applied elsewhere in this function.
🪄 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: 269fab02-43c5-4514-a99a-7c920acfb478
📒 Files selected for processing (14)
internal/providers/openai/provider.gointernal/providers/openai/provider_test.gointernal/providers/openai/tool_state.gointernal/tui/flush.gointernal/tui/model.gointernal/tui/model_test.gointernal/tui/mouse_test.gointernal/tui/render_cache.gointernal/tui/rendering.gointernal/tui/rendering_lime_test.gointernal/tui/theme.gointernal/tui/transcript.gointernal/tui/transcript_selection.gointernal/tui/transcript_view.go
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/providers/openai/provider_test.go (1)
452-457: ⚡ Quick winAssert the exact cancel sequence here.
Filtering by type is too loose for this regression. The test still passes if buffered text is emitted after the cancellation error, or if the stream emits
doneafter the error. Since this case is meant to lock flush-on-cancel behavior, assert the ordered sequence and explicitly forbidStreamEventDone.Suggested assertion shape
- if text := eventsOfType(events, zeroruntime.StreamEventText); len(text) != 2 || text[0].Content != "visible " || text[1].Content != "<thi" { - t.Fatalf("text events = %#v, want visible text plus buffered partial tag", text) - } - if len(eventsOfType(events, zeroruntime.StreamEventError)) != 1 { - t.Fatalf("events = %#v, want one context error after buffered content", events) - } + if len(events) != 3 || + events[0].Type != zeroruntime.StreamEventText || events[0].Content != "visible " || + events[1].Type != zeroruntime.StreamEventText || events[1].Content != "<thi" || + events[2].Type != zeroruntime.StreamEventError { + t.Fatalf("events = %#v, want text, buffered text, then context error", events) + } + if done := eventsOfType(events, zeroruntime.StreamEventDone); len(done) != 0 { + t.Fatalf("events = %#v, want no done after context cancel", events) + }🤖 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 `@internal/providers/openai/provider_test.go` around lines 452 - 457, The test assertions in the OpenAI provider test are too loose because they filter events by type independently rather than checking their exact ordered sequence. Replace the separate type-filtered checks for text and error events with a single assertion that validates the complete ordered sequence of events. The assertion should verify that events occur in the exact order: first two text events with the expected content ("visible " and "<thi"), followed by exactly one error event, and importantly, it should explicitly forbid any StreamEventDone event after the error to properly lock down the flush-on-cancel behavior being tested.
🤖 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 `@internal/providers/openai/tool_state.go`:
- Around line 65-68: The emitContentWith method in toolState unconditionally
passes all content deltas through state.think.push() which parses <think> tags,
causing literal <think>...</think> markup in normal responses (code examples,
documentation, etc.) to be incorrectly rewritten as reasoning rows. Add an
explicit compatibility gate or model/provider opt-in check before applying the
think parsing logic: only call state.think.push() when the gate is enabled,
otherwise emit the content directly without parsing to preserve literal markup
in the response.
---
Nitpick comments:
In `@internal/providers/openai/provider_test.go`:
- Around line 452-457: The test assertions in the OpenAI provider test are too
loose because they filter events by type independently rather than checking
their exact ordered sequence. Replace the separate type-filtered checks for text
and error events with a single assertion that validates the complete ordered
sequence of events. The assertion should verify that events occur in the exact
order: first two text events with the expected content ("visible " and "<thi"),
followed by exactly one error event, and importantly, it should explicitly
forbid any StreamEventDone event after the error to properly lock down the
flush-on-cancel behavior being tested.
🪄 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: cbb56d46-d4e2-4632-8381-33a923a3e07f
📒 Files selected for processing (5)
internal/providers/openai/provider.gointernal/providers/openai/provider_test.gointernal/providers/openai/tool_state.gointernal/tui/rendering_lime_test.gointernal/tui/transcript_selection.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/providers/openai/provider.go
- internal/tui/transcript_selection.go
- internal/tui/rendering_lime_test.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/config/resolver_test.go (1)
1083-1126: ⚡ Quick winAdd an explicit
parse_think_tags: falsecase to lock pointer-boolean semantics.Line 1123 currently validates only the
truebranch. Add a sibling case asserting explicitfalseresolves to a non-nil pointer with*profile.ParseThinkTags == false, so regressions between unset vs explicit false are caught.Suggested test addition
+func TestResolveProviderProfileParseThinkTagsFalseAlias(t *testing.T) { + path := writeConfig(t, `{ + "activeProvider": "custom", + "providers": [{ + "name": "custom", + "provider_kind": "openai-compatible", + "base_url": "https://custom.example/v1", + "model_id": "custom-model", + "parse_think_tags": false + }] + }`) + resolved, err := Resolve(ResolveOptions{UserConfigPath: path, Env: map[string]string{}}) + if err != nil { + t.Fatalf("Resolve() error = %v", err) + } + if resolved.Provider.ParseThinkTags == nil || *resolved.Provider.ParseThinkTags { + t.Fatalf("ParseThinkTags = %#v, want explicit false", resolved.Provider.ParseThinkTags) + } +}🤖 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 `@internal/config/resolver_test.go` around lines 1083 - 1126, The current TestResolveProviderProfileExtendedJSONAliases test only validates the case where parse_think_tags is set to true (line 1123). Add a sibling test case that explicitly sets parse_think_tags to false in the JSON configuration and verifies that profile.ParseThinkTags is a non-nil pointer with the value false (using assertions similar to the existing true case but asserting !*profile.ParseThinkTags). This ensures regressions between unset versus explicitly false parse_think_tags values are caught.
🤖 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 `@internal/config/resolver_test.go`:
- Around line 1083-1126: The current
TestResolveProviderProfileExtendedJSONAliases test only validates the case where
parse_think_tags is set to true (line 1123). Add a sibling test case that
explicitly sets parse_think_tags to false in the JSON configuration and verifies
that profile.ParseThinkTags is a non-nil pointer with the value false (using
assertions similar to the existing true case but asserting
!*profile.ParseThinkTags). This ensures regressions between unset versus
explicitly false parse_think_tags values are caught.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 685abd8f-bf0c-4f85-8332-00f3d8201110
📒 Files selected for processing (7)
internal/config/resolver_test.gointernal/config/types.gointernal/providers/factory.gointernal/providers/factory_test.gointernal/providers/openai/provider.gointernal/providers/openai/provider_test.gointernal/providers/openai/tool_state.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/providers/openai/provider_test.go
- internal/providers/openai/tool_state.go
Contributor
Zero automated PR reviewVerdict: No blockers found Blockers
Validation
ScopeHead: This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality. |
Vasanthdev2004
approved these changes
Jun 14, 2026
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
Collaborator
There was a problem hiding this comment.
Verdict: APPROVE.
I reviewed current head 06e7b96 across both sides of the PR:
- Provider path:
<think>parsing is now gated byParseThinkTags, so normal literal<think>...</think>content is preserved by default. The cancellation/error paths flush buffered content through the buffered sender before the terminal error, so split tags/content are not dropped. - Factory/config path: defaults are conservative for official OpenAI and generic OpenAI-compatible models, with explicit profile override support for reasoning models that emit inline tags.
- TUI path: reasoning rows are separated from assistant answers, collapse/expand state is modeled on the row, streaming reasoning is flushed before tool calls/cancel/done, and mouse selection/toggling has coverage.
I do not see merge-blocking issues. One old minor CodeRabbit note about selectable reasoning header clamping is not a blocker in the current implementation because the header is bounded short text; body lines are already clamped.
Validation run locally:
git diff --check origin/main...HEADgo test ./internal/providers ./internal/providers/openai ./internal/config ./internal/tui -count=1go test ./...go vet ./...go run ./cmd/zero-release buildgo run ./cmd/zero-release smoke
GitHub checks are green and the PR is mergeable. If GitHub still shows CHANGES_REQUESTED, that is from stale CodeRabbit reviews on older commits; latest CodeRabbit completed successfully.
Collaborator
Author
|
@coderabbitai The requested issues have been fixed in the latest commits. The latest CodeRabbit run says no actionable comments were generated and the CodeRabbit status check is green, but GitHub still shows CHANGES_REQUESTED from older stale reviews. Please re-review this PR or clear the stale requested-changes state. |
|
✅ Action performedFull review finished. |
Collaborator
Author
|
@coderabbitai approve |
✅ Action performedComments resolved and changes approved. |
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
2 participants
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:
Tests:
Summary by CodeRabbit
Release Notes
New Features
<think>...</think>parsing that can split streamed content into reasoning vs plain text based on provider profile/model defaults.Bug Fixes
Tests