feat - Persist thinking blocks to conversation history and restore on session switch#210
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Persist thinking-block content/title/state into the per-conversation persistence model so they can be restored when switching sessions, and harden ThinkingBlock.parseSections against a crash that occurs when two **Title** markers appear back-to-back.
Changes:
- Add
ThinkingBlockData/ThinkingBlockStateand wire them intoReplyData(transient pending block) andEditAgentRoundData(attached block); addfinalizeLastThinkingBlock/updateThinkingBlockTitletoConversationPersistenceManagerand accumulate thinking text inConversationDataFactory. - Route conversation/turn IDs from
ChatView→ChatContentViewer→ThinkingTurnWidget(setConversationContext), chain finalize → title generation → title persistence insealThinking, persistCANCELLEDstate inonChatMessageCancelled, and restore thinking blocks during history replay for both main and subagent turns. - Fix
ThinkingBlock.parseSectionsStringIndexOutOfBoundsExceptionwhen adjacent titles causecursor > matcher.start()by emitting an empty body in that case.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
.../core/persistence/CopilotTurnData.java |
Adds ThinkingBlockData/ThinkingBlockState, transient pendingThinkingBlock on ReplyData, and thinkingBlock on EditAgentRoundData. |
.../core/persistence/ConversationDataFactory.java |
Accumulates streamed thinking text into the reply's pending block. |
.../core/persistence/ConversationPersistenceManager.java |
Adds cache-only finalizeLastThinkingBlock and updateThinkingBlockTitle operations. |
.../ui/chat/ThinkingTurnWidget.java |
Adds setConversationContext, persistence chaining in sealThinking, cancellation persistence, and restoreThinkingBlock. |
.../ui/chat/ThinkingBlock.java |
Guards parseSections substring against cursor > matcher.start() from adjacent titles. |
.../ui/chat/ChatContentViewer.java |
Stores conversationId and propagates context to ThinkingTurnWidget on each report event. |
.../ui/chat/ChatView.java |
Syncs conversationId to viewer on begin; restores persisted thinking blocks per round on history replay. |
.../ui/chat/BaseTurnWidget.java |
Restores thinking blocks in subagent rounds; renames a local to disambiguate from the new subagentWidget. |
Comments suppressed due to low confidence (2)
com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/ThinkingTurnWidget.java:96
- The new early-return when
active.persistTurnId == nullchanges the visible behavior ofsealThinking: previously the block was always sealed (and, if no LSP, completed) so the spinner stopped. Now, if the conversation context has not been propagated (e.g. anendevent arrives without any priorreporthaving set the context for the currently-active widget), the thinking block silently stays in STREAMING state forever — the spinner never stops and no title is fetched. Consider sealing/completing the block visually regardless and only skipping the persistence calls whenpersistTurnIdis null.
if (active.persistTurnId == null) {
return;
}
com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/ThinkingTurnWidget.java:163
- When
persistTurnIdis null, this falls back toactive.turnId, but the class-level Javadoc onpersistTurnIdexplicitly states thatturnIdmay not match the persistence key (especially for subagent turns whereturnId = toolCallId + suffix). Usingactive.turnIdas the persistence key in that case will silently look up the wrong (or no) turn in the cache and the cancellation state will never be persisted. It would be clearer to skip the persistence call entirely whenpersistTurnId == null, mirroring the guard added insealThinking.
String cancelTurnId = active.persistTurnId != null ? active.persistTurnId : active.turnId;
active.finalizeLastThinkingBlock(ThinkingBlockState.CANCELLED, cancelTurnId);
ethanyhou
reviewed
May 15, 2026
d5a18f0 to
ba03246
Compare
jdneo
reviewed
May 16, 2026
jdneo
reviewed
May 16, 2026
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
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.
resolve #201
Change 1: Persist thinking blocks to conversation history
Persist thinking block content, state, and title so thinking blocks can be restored when switching sessions or loading conversation history.
Data Model
ThinkingBlockDatawithid,content,title, andstatefields.ThinkingBlockStateenum forCOMPLETEDandCANCELLED.thinkingBlocktoEditAgentRoundDataso thinking content is stored with the agent round it belongs to.Persistence
AgentRound.AgentRoundarrives, merge it into the placeholder by matching the UI-generated thinking block ID.thinkingBlockId.thinkingBlockId.-1) until the real server round arrives.UI
ThinkingBlock.setConversationContext(conversationId, persistTurnId)to route persistence context to the active widget, including subagent widgets.sealThinking()uses stored context and persists generated titles by thinking block ID.onChatMessageCancelled()routes throughgetActiveTurnWidget()so subagent thinking cancellation is handled correctly.ChatContentViewercaches the active thinking block ID per turn while processing progress events, soChatViewcan pass the correct ID to persistence without querying widget state again.conversationIdtoChatContentVieweron begin events.Restore
EditAgentRoundData.thinkingBlock.Validation
Change 2: Fix ThinkingBlock.parseSections crash on adjacent titles
When two bold title patterns (e.g. Title1 immediately followed by Title2) appear with no body text between them, the newline-skipping logic advances the cursor past the next match's start position, causing a StringIndexOutOfBoundsException. This crashes the entire conversation restore and leaves the chat panel broken.
Fix: guard the substring call with a cursor <= matcher.start() check, producing an empty body when titles are adjacent.
Stack trace:
Change 3 : Fix Thinking Stream Whitespace Handling
StringUtils.isBlank.**Title**.StringUtils.isEmptyfor thinking fragments sonull/ empty strings are ignored, while meaningful markdown whitespace such as"\n"is preserved.Before:
