fix(vscode): preserve thinking state and recover missing edit snapshots - #4147
Merged
Conversation
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves VS Code companion edit-message behavior by preserving thinking-message ordering and making edit recovery more resilient when local conversation snapshots are missing.
Changes:
- Adjusts assistant placeholder timestamps so thinking segments sort after edited user messages.
- Adds conversation recovery/upsert and conversation-id rename support.
- Updates edit flow to continue with ACP rewind when local snapshots are unavailable.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
packages/vscode-ide-companion/src/webview/hooks/message/useMessageHandling.ts |
Updates stream placeholder timestamp assignment to preserve message ordering. |
packages/vscode-ide-companion/src/webview/hooks/message/useMessageHandling.test.tsx |
Adds regression coverage for thinking ordering with matching timestamps. |
packages/vscode-ide-companion/src/webview/handlers/SessionMessageHandler.ts |
Adds snapshot recovery, ACP-only edit fallback, and conversation-id alignment. |
packages/vscode-ide-companion/src/webview/handlers/SessionMessageHandler.test.ts |
Adds tests for edit fallback, snapshot recovery, and session-id alignment. |
packages/vscode-ide-companion/src/services/conversationStore.ts |
Adds conversation rename and upsert helpers. |
packages/vscode-ide-companion/src/services/conversationStore.test.ts |
Adds tests for rename and upsert behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
880
to
888
| if (!renamed) { | ||
| console.warn( | ||
| '[SessionMessageHandler] Failed to align conversation store with ACP session id:', | ||
| previousConversationId, | ||
| acpSessionId, | ||
| ); | ||
| } | ||
| } | ||
| this.currentConversationId = acpSessionId; |
LaZzyMan
requested changes
May 14, 2026
LaZzyMan
left a comment
LaZzyMan
left a comment
Collaborator
There was a problem hiding this comment.
Review
The targeted fixes — preserving thinking segments across edited messages and recovering from missing local snapshots — are sound, and the new tests cover the happy paths. However, the alignment branch that renames the local conversation to match the ACP session id bypasses the canonical setter that keeps the message router in sync with each handler. Both sub-reviewers independently flagged the same line; the fix is small but the failure mode is silent state corruption, so it should land before merge.
1. Conversation-id rename bypasses the router setter and leaves serialized state pointing at a deleted id (severity: high · confidence: high)
After the first message succeeds, the handler assigns the new ACP session id directly to its own currentConversationId field. The owning message router keeps its own copy, which is what gets serialized into the persisted webview state. After the rename, the router still holds the old (now-deleted) conv_* id, so on the next VS Code reload the restored state references a conversation that no longer exists. Going through the router's setCurrentConversationId fans the update out to every handler and keeps the persisted state in sync. Additionally, when renameConversationId returns false because the target id already exists (collision path), the code logs a warning and then still reassigns to the colliding id — the just-written user/assistant turn ends up stranded under the previous id while subsequent messages land in the pre-existing conversation, silently splicing two histories together. Bail out on false instead of continuing the reassignment.
Verdict
REQUEST_CHANGES — bug fixes are correct but the post-stream alignment branch leaks a stale conversation id into persisted webview state (and corrupts on rare id collisions); both sub-reviewers landed on the same line.
DragonnZhang
added a commit
that referenced
this pull request
May 14, 2026
PR #4147 review flagged that SessionMessageHandler directly assigns this.currentConversationId after the first ACP message, bypassing the MessageRouter's setCurrentConversationId. This leaves the router's copy (which gets serialized to webview state) pointing at the old (now-deleted) conversation id. Add an onConversationIdChanged callback to BaseMessageHandler that the MessageRouter wires up to its own setCurrentConversationId. SessionMessageHandler now invokes this callback instead of direct assignment, ensuring all handlers stay in sync and persisted state remains valid.
DragonnZhang
enabled auto-merge (squash)
May 15, 2026 05:48
LaZzyMan
approved these changes
May 15, 2026
LaZzyMan
left a comment
LaZzyMan
left a comment
Collaborator
There was a problem hiding this comment.
Review
Re-reviewed after c1bb52b9. The router-sync issue from the prior round is cleanly resolved: a syncCurrentConversationId callback now plumbs BaseMessageHandler writes back through the message router so every handler stays in sync, and the rename collision branch now bails out without reassigning. The new tests cover both the success and collision paths, and the drive-by conversion of all eight direct assignment sites in SessionMessageHandler removes a whole class of latent divergence bugs. The two pre-existing minor items from the prior round (silent ACP-only addMessage no-op, and the localized +2 timestamp fix not generalizing to back-to-back same-ms edits) are unchanged and remain non-blocking.
Verdict
APPROVE — prior critical resolved with appropriate test coverage; remaining minor items are non-blockers.
TaimoorSiddiquiOfficial
pushed a commit
to TaimoorSiddiquiOfficial/HopCode
that referenced
this pull request
May 15, 2026
…ts (QwenLM#4147) * fix(vscode): allow editing sessions without local snapshots * fix(vscode): keep thinking after edited user message * fix(vscode): sync conversation id alignment through router (cherry picked from commit a656930)
xaelistic
pushed a commit
to xaelistic/qwen-code
that referenced
this pull request
Jun 7, 2026
…4147) Co-authored-by: Scott Densmore <scottdensmore@mac.com>
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.
Summary
What changed:
renameConversationIdandupsertConversationmethods to ConversationStore for better session alignmentWhy it changed:
Reviewer focus:
Validation
ConversationStore.renameConversationId(success and conflict cases)ConversationStore.upsertConversation(insert and update cases)SessionMessageHandlersnapshot recovery and conversation alignmentuseMessageHandlingthinking preservation with matching timestampsScope / Risk
Main risk or tradeoff:
maxExistingTimestamp + 2to guarantee uniquenessNot covered / not validated:
Breaking changes / migration notes:
Testing Matrix
Testing matrix notes:
Linked Issues / Bugs
🤖 Generated with Qwen Code