feat: gpt5.2, conversation branch #79
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request implements a comprehensive conversation branching system that enables users to edit messages and create alternative conversation paths, along with adding support for new GPT-5 models and reasoning display features. The implementation spans both frontend (React/TypeScript) and backend (Go) with protocol buffer schema updates.
Key changes:
- Implements conversation branching architecture with Branch struct for tracking edit history
- Adds GPT-5.1, GPT-5.2, and GPT-5.2 Pro model support with pricing information
- Introduces reasoning token display for AI responses with streaming support
Reviewed changes
Copilot reviewed 32 out of 32 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
internal/models/conversation.go |
Adds Branch struct and methods for branch management, migration from legacy format |
internal/api/chat/create_conversation_message_stream_v2.go |
Updates message streaming to use branches, handles parent message IDs for edits |
internal/api/mapper/conversation_v2.go |
Maps conversation model to proto with branch selection support |
internal/api/chat/get_conversation_v2.go |
Supports optional branch_id parameter for retrieving specific branches |
internal/api/chat/list_supported_models_v2.go |
Adds GPT-5.x model definitions with context sizes and pricing |
proto/chat/v2/chat.proto |
Adds BranchInfo, reasoning field, ReasoningChunk, and parent_message_id |
pkg/gen/api/chat/v2/*.go |
Generated Go code from proto changes |
webapp/_webapp/src/components/branch-switcher.tsx |
New UI component for navigating between conversation branches |
webapp/_webapp/src/components/message-entry-container/user.tsx |
Adds message editing interface with edit button and textarea |
webapp/_webapp/src/components/message-entry-container/assistant.tsx |
Displays reasoning content in expandable card format |
webapp/_webapp/src/hooks/useSendMessageStream.ts |
Handles parentMessageId for branching, truncates messages client-side |
internal/services/toolkit/handler/stream_v2.go |
Adds HandleReasoningDelta for streaming reasoning tokens |
internal/services/toolkit/client/completion_v2.go |
Parses reasoning_content and reasoning fields from streaming responses |
webapp/_webapp/src/stores/conversation/handlers/handleReasoningChunk.ts |
New handler for processing reasoning chunks in streaming messages |
| Multiple handler files | Removes "openai_" prefix from message IDs for cleaner identifier format |
webapp/_webapp/src/views/settings/*.tsx |
Reduces label text size from text-sm to text-xs |
webapp/_webapp/src/main.tsx |
Adds URL patterns to Faro monitoring ignore list |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const handlePrevious = () => { | ||
| if (currentIndex <= 1) return; | ||
| const prevBranchId = branches[currentIndex - 2]?.id; | ||
| if (prevBranchId) { | ||
| switchToBranch(prevBranchId); | ||
| } | ||
| }; | ||
|
|
||
| const handleNext = () => { | ||
| if (currentIndex >= totalBranches) return; | ||
| const nextBranchId = branches[currentIndex]?.id; | ||
| if (nextBranchId) { | ||
| switchToBranch(nextBranchId); | ||
| } | ||
| }; |
There was a problem hiding this comment.
The component should handle the case where conversation.branches array length doesn't match conversation.totalBranches. If the arrays are out of sync, accessing branches[currentIndex - 2] or branches[currentIndex] could return undefined even when the navigation should be valid, or conversely allow navigation when it shouldn't be possible.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR is being reviewed by Cursor Bugbot
Details
You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
| } | ||
| // For new conversations, ensure branches and get the active one | ||
| conversation.EnsureBranches() | ||
| activeBranch = conversation.GetActiveBranch() |
There was a problem hiding this comment.
Missing nil check for activeBranch in new conversations
High Severity
For new conversations, GetActiveBranch() is called without a nil check after EnsureBranches(). This is inconsistent with the existing conversation path in appendConversationMessage, which properly checks for nil at lines 175-179. If GetActiveBranch() returns nil (e.g., if EnsureBranches() doesn't create a branch), accessing activeBranch.OpenaiChatHistoryCompletion at line 319 will cause a nil pointer dereference and crash the server.
Additional Locations (1)
| const updatedParts = prevMessage.parts.map((part: MessageEntry) => { | ||
| const isTargetPart = part.messageId === chunk.messageId && part.assistant; | ||
|
|
||
| if (!isTargetPart) return part; |
There was a problem hiding this comment.
Reasoning chunks silently dropped due to race condition
High Severity
The backend sends ReasoningChunk messages before sending StreamPartBegin for the assistant message. The frontend handler checks part.messageId === chunk.messageId && part.assistant, but the assistant part doesn't exist yet since handleStreamPartBegin hasn't been called. This causes all reasoning content to be silently dropped because isTargetPart evaluates to false, making the new reasoning feature non-functional for streamed responses.
Additional Locations (1)
| currentBranchIndex: branchInfo.currentBranchIndex, | ||
| totalBranches: branchInfo.totalBranches, | ||
| })); | ||
| } |
There was a problem hiding this comment.
Async branch update can corrupt unrelated conversation state
Medium Severity
The updateBranchInfoAsync function fetches branch info for a specific conversationId but applies the update to whatever currentConversation exists when the async call completes. If the user navigates to a different conversation before the fetch completes, the branch info from the old conversation (with currentBranchId, branches, etc.) will overwrite the new conversation's branch metadata. The function needs to verify that currentConversation.id still matches the fetched conversationId before applying the update.
|
related to #71 |
…ches (#80) Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: Junyi-99 <14367694+Junyi-99@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 32 out of 32 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| bsonMessages[i] = bsonMsg |
There was a problem hiding this comment.
The branch's UpdatedAt timestamp is not being updated when AI response messages are appended to it. This means GetActiveBranch() will not correctly identify the most recently modified branch. You should update activeBranch.UpdatedAt to the current time before saving the conversation.
| // Find parent message and truncate after it | ||
| foundIndex := -1 | ||
| for i, msg := range sourceInappHistory { | ||
| if id, ok := msg["messageId"].(string); ok && strings.Contains(id, truncateAfterMsgID) { |
There was a problem hiding this comment.
Using strings.Contains for message ID matching could lead to false positives if one message ID is a substring of another. Consider using exact equality comparison instead: id == truncateAfterMsgID.
| if field, ok := delta.JSON.ExtraFields["reasoning_content"]; ok && field.Raw() != "null" { | ||
| var s string | ||
| err := json.Unmarshal([]byte(field.Raw()), &s) | ||
| if err != nil { | ||
| // fmt.Println(err) | ||
| if err == nil { | ||
| reasoning_content += s | ||
| streamHandler.HandleReasoningDelta(chunk.ID, s) | ||
| } | ||
| } else if field, ok := delta.JSON.ExtraFields["reasoning"]; ok && field.Raw() != "null" { | ||
| var s string | ||
| err := json.Unmarshal([]byte(field.Raw()), &s) | ||
| if err == nil { | ||
| reasoning_content += s | ||
| streamHandler.HandleReasoningDelta(chunk.ID, s) | ||
| } |
There was a problem hiding this comment.
The reasoning field handling in the OpenAI response parsing checks for both "reasoning_content" and "reasoning" fields. This dual check suggests uncertainty about the field name. Ensure you're checking the correct field name that the OpenAI API actually uses for reasoning output, as incorrect field names will silently fail to capture reasoning data.
| if (prompts.length === 0) { | ||
| return ( | ||
| <div className="transition-all duration-100 absolute bottom-full left-0 right-0 mb-1 z-50 bg-white shadow-lg rounded-lg border border-gray-200 p-4"> | ||
| <div className="text-gray-500 text-sm text-center">No prompts found</div> | ||
| </div> | ||
| ); | ||
| } |
There was a problem hiding this comment.
The "No prompts found" message is shown when prompts.length === 0, but the parent condition requires prompt.startsWith("/"). This means the "No prompts found" state will only show when the user has typed "/" but no prompts match, not when the prompts list is genuinely empty. Consider whether this is the intended behavior.
| // Use specified branch_id if provided, otherwise use active branch | ||
| branchID := req.GetBranchId() | ||
|
|
||
| return &chatv2.GetConversationResponse{ | ||
| Conversation: mapper.MapModelConversationToProtoV2(conversation), | ||
| Conversation: mapper.MapModelConversationToProtoV2WithBranch(conversation, branchID), |
There was a problem hiding this comment.
Missing input validation for branchId parameter. The code should validate that the provided branchId exists in the conversation's branches before attempting to use it. Currently, if an invalid branchId is provided, the code will silently fall back to the active branch without indicating an error to the client.
| messages: [], | ||
| })); | ||
| } | ||
| } |
There was a problem hiding this comment.
Client truncates messages before API call without rollback
Medium Severity
When editing a message, the client-side conversation is truncated immediately before the streaming API call. If the API call fails or encounters an error, the truncated messages are not restored. The error handlers (handleError, handleStreamError) only mark streaming messages as stale but do not restore the previously truncated conversation messages. This leaves the client in an inconsistent state where it shows fewer messages than what the server has stored.
|
|
||
| *inappChatHistory = append(*inappChatHistory, chatv2.Message{ | ||
| MessageId: fmt.Sprintf("openai_%s", contentId), | ||
| MessageId: contentId, |
There was a problem hiding this comment.
Inconsistent message ID prefix removal across handlers
Medium Severity
The PR removes the "openai_" prefix from message IDs in several handlers (utils.go, utils_v2.go, stream.go, toolcall.go) but toolcall_v2.go (not modified in this PR) still uses "openai_toolCall[%d]_%s" format. This creates inconsistent message ID formats within the same conversation. The strings.Contains matching in CreateNewBranch may behave unexpectedly when trying to match messages across these different formats during branch truncation operations.
Additional Locations (1)
| <div className="pd-app-tab-content-footer chat-prompt-input noselect rnd-cancel relative"> | ||
| {/* Only show one popup at a time - priority: prompts > actions > model selection */} | ||
| {prompts.length > 0 && <PromptSelection prompts={prompts} />} | ||
| {prompt.startsWith("/") && <PromptSelection prompts={prompts} />} |
There was a problem hiding this comment.
Overlapping popups when typing slash with no prompts
Medium Severity
The comment states "Only show one popup at a time - priority: prompts > actions > model selection" but the logic doesn't enforce this. When a user types "/" and prompts.length === 0 but actions.length > 0, both PromptSelection (showing "No prompts found") and ActionSelection will render simultaneously. The condition on line 104 was changed from prompts.length > 0 to prompt.startsWith("/") but line 105's condition wasn't updated to account for this, causing both components to render when the user types "/" with no matching prompts but available actions.
This pull request introduces significant improvements to the conversation branching system, adds support for new GPT-5 models, and refines how conversations and their branches are managed and returned via the API. The main changes include implementing a robust branching mechanism for chat conversations, updating the API to support branch selection and history, and expanding the list of supported language models.
Conversation Branching and API Enhancements:
Branchstruct toConversationinmodels/conversation.go, enabling multiple branches per conversation for edit history, and migrated legacy fields for backward compatibility.create_conversation_message_stream_v2.goto append messages to the correct branch, support branch creation on message edits, and ensure all API methods use the active or specified branch for chat history and LLM calls. [1] [2] [3] [4] [5] [6] [7]mapper/conversation_v2.goto allow explicit branch selection, include branch metadata in API responses, and ensure backward compatibility with legacy conversations. [1] [2]GetConversationAPI to allow branch selection via abranch_idparameter, returning the correct branch's history and metadata.Supported Models Expansion:
Other Changes:
Note
Introduces branch-aware conversations and reasoning streaming, plus new GPT-5.x models.
BranchtoConversationwith migration helpers (EnsureBranches,CreateNewBranch,GetActiveBranch); histories now per-branchparent_message_idcreates a branched edit; LLM calls use branch history;GetConversationacceptsbranch_id; mapper returns branch metadata (branches,current_branch_id, indices)chat.protowithBranchInfo, branch fields onConversation, optionalbranch_idandparent_message_id; addReasoningChunk; regenerate pb and gwreasoningdeltas;HandleTextDoneItemincludes reasoning; in-app assistant messages support reasoningparentMessageIdand truncates UI to parent; reasoning shown above answers; updates to converters/handlers; minor UI tweaks and safer prompt selectionWritten by Cursor Bugbot for commit 98580ce. This will update automatically on new commits. Configure here.