handle reasoning deltas and display them in the UI #2443
Conversation
WalkthroughAdds backend support for streaming multi-part reasoning summaries from OpenAI by introducing new SSE event types and data structures, extending the streaming item model and block state (summaryCount), and handling reasoning part/text-delta/done events. Updates request construction to set Reasoning.Summary = "auto" for gpt-5 when a thinking level is provided. Frontend AI message components were changed to pass and render reasoningText with an auto-scrolling reasoning panel and updated thinking-data shapes. Telemetry API signatures were simplified (removing context parameters) and call sites were updated. Documentation adjusted for Go compilation guidance. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: unknown linters: 'unusedfunc,unusedparams', run 'golangci-lint help linters' to see the list of supported linters 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
.roo/rules/rules.md (1)
183-184: Add the missing comma for readability.Insert a comma after “VSCode” to improve the sentence flow.
-If there are no Go errors in VSCode you can assume the code compiles fine. +If there are no Go errors in VSCode, you can assume the code compiles fine.frontend/app/aipanel/aimessage.tsx (3)
23-26: Clarify the displayText extraction logic.The logic extracts only the text after the last double newline (
\n\n). This pattern is unusual and lacks explanation. IfreasoningTextis "step1\n\nstep2\n\nstep3", only "step3" displays. While this might be intentional for showing the latest streaming chunk, it's not self-evident.Consider adding a comment explaining the rationale:
+ // Extract the latest reasoning chunk after the last double newline for streaming display const displayText = reasoningText ? (() => { const lastDoubleNewline = reasoningText.lastIndexOf("\n\n"); return lastDoubleNewline !== -1 ? reasoningText.substring(lastDoubleNewline + 2) : reasoningText; })() : "";Alternatively, if you want to show all reasoning text instead of just the latest chunk, remove the extraction logic:
- const displayText = reasoningText ? (() => { - const lastDoubleNewline = reasoningText.lastIndexOf("\n\n"); - return lastDoubleNewline !== -1 ? reasoningText.substring(lastDoubleNewline + 2) : reasoningText; - })() : ""; + const displayText = reasoningText || "";
41-41: Consider increasing the reasoning text max-height.
max-h-[2lh](2 line heights) is quite restrictive and may force users to scroll frequently for multi-line reasoning. This could degrade readability, especially during longer thinking sequences.Consider increasing the height to improve readability:
- className="text-sm text-gray-500 overflow-y-auto max-h-[2lh] max-w-[600px] pl-9" + className="text-sm text-gray-500 overflow-y-auto max-h-[6lh] max-w-[600px] pl-9"
17-21: Auto-scroll may interfere with user reading.The effect scrolls to the bottom whenever
reasoningTextchanges. During active streaming, this prevents users from scrolling up to review earlier reasoning steps, as new content continuously forces the view down.Consider implementing scroll behavior that respects user intent, such as only auto-scrolling if the user is already near the bottom:
useEffect(() => { - if (scrollRef.current && reasoningText) { - scrollRef.current.scrollTop = scrollRef.current.scrollHeight; + if (scrollRef.current && reasoningText) { + const { scrollTop, scrollHeight, clientHeight } = scrollRef.current; + const isNearBottom = scrollTop + clientHeight >= scrollHeight - 20; + if (isNearBottom) { + scrollRef.current.scrollTop = scrollRef.current.scrollHeight; + } } }, [reasoningText]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
.roo/rules/rules.md(1 hunks)frontend/app/aipanel/aimessage.tsx(5 hunks)pkg/aiusechat/openai/openai-backend.go(5 hunks)pkg/aiusechat/openai/openai-convertmessage.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
frontend/app/aipanel/aimessage.tsx (1)
frontend/app/aipanel/aitypes.ts (1)
WaveUIMessagePart(25-25)
pkg/aiusechat/openai/openai-backend.go (2)
pkg/web/sse/ssehandler.go (2)
AiMsgError(53-53)AiMsgReasoningDelta(44-44)pkg/aiusechat/uctypes/usechat-types.go (2)
WaveStopReason(171-185)StopKindError(158-158)
🪛 LanguageTool
.roo/rules/rules.md
[grammar] ~184-~184: There might be a mistake here.
Context: ...e you can assume the code compiles fine.
(QB_NEW_EN)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: Build for TestDriver.ai
🔇 Additional comments (3)
frontend/app/aipanel/aimessage.tsx (2)
456-481: LGTM! Clean refactor to support reasoning text.The function signature change from
string | nullto{ message: string; reasoningText?: string } | nullproperly supports streaming reasoning display. All return paths are consistent and handle the different streaming states correctly:
- Pending approvals → message only
- Reasoning part → message + reasoningText
- Default streaming → empty message (shows pulsing dots)
The logic correctly distinguishes between these states and provides appropriate context to the UI.
Optional: Line 472's
reasoningContentcould be inlined since it's used only once:if (lastPart?.type === "reasoning") { - const reasoningContent = lastPart.text || ""; - return { message: "AI is thinking...", reasoningText: reasoningContent }; + return { message: "AI is thinking...", reasoningText: lastPart.text || "" }; }
483-526: LGTM! Proper integration of reasoning text support.The component correctly integrates the new
thinkingDataobject structure:
- Line 490: Captures the thinking state with both message and reasoning text
- Line 501: Condition correctly uses
!thinkingDatato show "(no text content)" when not streaming- Lines 514-518: Properly passes both
messageandreasoningTexttoAIThinkingThe refactor from
thinkingMessage(string) tothinkingData(object) maintains correct behavior across all scenarios while enabling the new reasoning display feature.pkg/aiusechat/openai/openai-backend.go (1)
882-911: Reasoning stream wiring looks solid.Nice job threading the new reasoning summary events through the existing block state and SSE hooks; the separator handling keeps the UI readable without disturbing the deltas.
| if opts.Model == "gpt-5" { | ||
| reqBody.Reasoning.Summary = "auto" | ||
| } |
There was a problem hiding this comment.
Broaden the gpt‑5 model match before enabling auto summary.
OpenAI’s released model IDs almost always ship with suffixed variants (e.g., gpt-5.1, gpt-5.1-mini). With the strict equality check, those common IDs won’t request the reasoning summary, so the UI never receives the new reasoning deltas. Please loosen the check (e.g., strings.HasPrefix(opts.Model, "gpt-5")) or otherwise cover the known variants so the feature is exercised across the gpt-5 family.
- if opts.Model == "gpt-5" {
+ if strings.HasPrefix(opts.Model, "gpt-5") {
reqBody.Reasoning.Summary = "auto"
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if opts.Model == "gpt-5" { | |
| reqBody.Reasoning.Summary = "auto" | |
| } | |
| if strings.HasPrefix(opts.Model, "gpt-5") { | |
| reqBody.Reasoning.Summary = "auto" | |
| } |
🤖 Prompt for AI Agents
In pkg/aiusechat/openai/openai-convertmessage.go around lines 238 to 240, the
code currently enables auto reasoning summary only when opts.Model == "gpt-5",
which misses common suffixed variants; change the check to detect model name
prefixes (e.g., use strings.HasPrefix(opts.Model, "gpt-5")) so any gpt-5.*
variant will enable reqBody.Reasoning.Summary = "auto", and ensure the strings
package is imported if not already.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/wcloud/wcloud.go (1)
214-233: Consider retaining the context parameter for caller control and tracing.The removal of the
ctx context.Contextparameter fromSendAllTelemetryeliminates the caller's ability to control cancellation, custom timeouts, or propagate context values (e.g., trace IDs, request IDs). This deviates from Go best practices where contexts should flow from caller to callee.While the current call sites create their own contexts anyway, retaining the parameter would:
- Preserve flexibility for future use cases requiring custom timeouts or cancellation
- Enable context-based tracing and observability across the telemetry pipeline
- Align with idiomatic Go API design
Consider restoring the signature to:
-func SendAllTelemetry(clientId string) error { +func SendAllTelemetry(ctx context.Context, clientId string) error { defer func() { - ctx, cancelFn := context.WithTimeout(context.Background(), 2*time.Second) + cleanupCtx, cancelFn := context.WithTimeout(context.Background(), 2*time.Second) defer cancelFn() - telemetry.CleanOldTEvents(ctx) + telemetry.CleanOldTEvents(cleanupCtx) }() if !telemetry.IsTelemetryEnabled() { log.Printf("telemetry disabled, not sending\n") return nil } _, err := sendTEvents(clientId) if err != nil { return err } - err = sendTelemetry(clientId) + err = sendTelemetry(ctx, clientId) if err != nil { return err } return nil }And update
sendTelemetry:-func sendTelemetry(clientId string) error { - ctx, cancelFn := context.WithTimeout(context.Background(), WCloudDefaultTimeout) - defer cancelFn() +func sendTelemetry(ctx context.Context, clientId string) error { + ctx, cancelFn := context.WithTimeout(ctx, WCloudDefaultTimeout) + defer cancelFn() activity, err := telemetry.GetNonUploadedActivity(ctx)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cmd/server/main-server.go(1 hunks)pkg/wcloud/wcloud.go(2 hunks)pkg/wshrpc/wshserver/wshserver.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
cmd/server/main-server.go (3)
pkg/wstore/wstore_dbops.go (1)
DBGetSingleton(102-105)pkg/waveobj/wtype.go (2)
Client(125-133)Client(135-137)pkg/wcloud/wcloud.go (1)
SendAllTelemetry(214-233)
pkg/wshrpc/wshserver/wshserver.go (1)
pkg/wcloud/wcloud.go (1)
SendAllTelemetry(214-233)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build for TestDriver.ai
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (4)
cmd/server/main-server.go (2)
132-132: Timeout increase appears appropriate for the operation.The timeout was increased from 10 seconds to 15 seconds, which aligns with the internal operations in
SendAllTelemetry:
sendTEventswith internal context creation (up to ~10 batches × 5s timeout)sendTelemetrywith 5s timeout- Cleanup with 2s timeout
While 15 seconds accommodates the sequential operations, consider whether this timeout is appropriate for a shutdown wrapper. If telemetry is non-critical, a shorter timeout might be preferable to avoid blocking shutdown.
140-140: Call site correctly updated for new signature.The call to
SendAllTelemetryhas been appropriately updated to match the new signature, passing onlyclient.OIDwithout the context parameter.pkg/wshrpc/wshserver/wshserver.go (2)
902-902: Call site correctly updated for new signature.The call to
SendAllTelemetryhas been appropriately updated to match the new signature, passing onlyclient.OIDwithout the context parameter.
939-939: Call site correctly updated for new signature.The call to
SendAllTelemetryhas been appropriately updated to match the new signature, passing onlyclient.OIDwithout the context parameter. This is consistent with the other call site at line 902.
Merges 59 commits from wavetermdev/waveterm v0.12.0 release into a5af fork. Resolves 49 merge conflicts across frontend, backend, and configuration files. ## Major Upstream Features Integrated ### AI Enhancements (v0.12.0) - AI Response Feedback + Copy Buttons (wavetermdev#2457) - Reasoning Deltas Display (wavetermdev#2443) - Google AI File Summarization (wavetermdev#2455) - `wsh ai` Command Reimplementation (wavetermdev#2435) - Terminal Context Improvements (wavetermdev#2444) - Batch Tool Approval System - Enhanced AI Panel with welcome message - Context menu support for AI messages ### Infrastructure Updates - Mobile User Agent Emulation for web widgets (wavetermdev#2454) - OSC 7 Support for Fish & PowerShell shells (wavetermdev#2456) - Log Rotation System (wavetermdev#2432) - Onboarding improvements - React 19 compatibility updates - Tailwind v4 migration progress - Dependency updates (50+ commits) ## Fork Features Preserved ✅ **Horizontal Widget Bar** (tabbar.tsx) - Widgets remain in horizontal tab bar (not reverted to sidebar) - Fork-specific layout maintained ✅ **Optional Pane Title Labels** (blockframe.tsx) - Auto-generated pane titles preserved - Custom block rendering logic intact ✅ **Layout Model Modifications** (layoutModel.ts) - Fork's widget positioning logic maintained - Horizontal layout integration preserved ## Conflict Resolution Summary **Configuration (8 files):** - Accepted upstream: .golangci.yml, Taskfile.yml, package.json, go.mod, etc. - All dependencies updated to v0.12.0 levels **Backend AI (13 files):** - Accepted upstream: All pkg/aiusechat/ files - New AI tools: read_dir, screenshot, terminal context - Enhanced OpenAI backend with reasoning support **Frontend AI Panel (12 files):** - Accepted upstream: All frontend/app/aipanel/ files - New features: reasoning display, feedback buttons, welcome message - Enhanced message handling and UX **Backend Infrastructure (7 files):** - Accepted upstream: emain, pkg/telemetry, pkg/wcore, pkg/wshrpc - Updated RPC types and telemetry data structures **Frontend Fork Features (8 files):** - Preserved fork: blockframe.tsx, tabbar.tsx, layoutModel.ts - Accepted upstream: keymodel.ts, wshclientapi.ts, termwrap.ts, etc. **Deleted Files (1 file):** - Removed: frontend/app/modals/tos.tsx (deleted upstream) ## Files Changed - Configuration: 8 files - Backend: 20+ files - Frontend: 25+ files - Total staged: 135 files ## Testing Required 1. Verify AI panel functionality (reasoning, feedback, tools) 2. Test horizontal widget bar (fork feature) 3. Test pane title labels (fork feature) 4. Build verification: `npm install && npm run build:dev` 5. Backend build: `go build ./...` 6. Full test suite: `npm test` ## Known Issues⚠️ Widget bar integration may need review - upstream removed widget sidebar⚠️ Layout changes may conflict with horizontal widget positioning⚠️ React 19 compatibility should be tested thoroughly ## Rollback If issues arise, rollback available at: - Tag: fork-v0.11.6-pre-v0.12-merge - Branch: backup-pre-v0.12-merge ## Next Steps 1. Test all functionality thoroughly 2. Fix any runtime errors from merge 3. Review fork feature integration points 4. Update documentation if needed 5. Create PR to main after validation --- 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
1 participant
No description provided.