fix(vscode): force fresh ACP session on new-session action#2874
Merged
tanzhenxin merged 12 commits intomainfrom Apr 11, 2026
Merged
fix(vscode): force fresh ACP session on new-session action#2874tanzhenxin merged 12 commits intomainfrom
tanzhenxin merged 12 commits intomainfrom
Conversation
Ensure explicit new-session actions bypass active ACP session reuse so the VS Code sidebar clears context correctly. Add regression coverage for the agent manager and webview new-session entry points.
Replace the runtime import of `isSupportedImageMimeType` from `@qwen-code/qwen-code-core` with a local `SUPPORTED_PASTED_IMAGE_MIME_TYPES` set in the vscode-ide-companion package. The webview is bundled for a browser environment where Node.js-only core modules are unavailable, so keeping the MIME list local avoids esbuild failures during development. Added tests to verify the local list stays aligned with core and that the webview bundle does not contain core runtime imports.
The webview context-usage bar did not clear when the user started a new session because the old code always fell back to DEFAULT_TOKEN_LIMIT, producing a stale percentage even after usageStats and modelInfo were both cleared. Key changes: - Extract `knownTokenLimit()` in core/tokenLimits.ts that returns `undefined` for unrecognized models instead of a default, keeping `tokenLimit()` behavior unchanged. - In acpModelInfo.ts, derive `_meta.contextLimit` from the known-model table when the ACP payload omits a numeric limit. - Extract `computeContextUsage()` into its own module, which returns `null` when no trusted numeric limit is available — the UI then correctly hides the context bar. - Remove the `@qwen-code/qwen-code-core` runtime import from App.tsx so the webview bundle stays free of Node-only dependencies. Closes #2847
yiliang114
requested review from
DennisYu07,
DragonnZhang,
LaZzyMan,
Mingholy,
gwinthis,
pomelo-nwu and
tanzhenxin
as code owners
4 tasks
Contributor
📋 Review SummaryThis PR addresses issue #2847 by fixing a bug where clicking the "+" (new session) button in the VSCode sidebar silently reused the existing ACP session instead of creating a fresh one. The fix introduces a 🔍 General Feedback
🎯 Specific Feedback🟡 High
🟢 Medium
🔵 Low
✅ Highlights
|
yiliang114
changed the title
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. |
tanzhenxin
requested changes
Apr 5, 2026
Collaborator
tanzhenxin
left a comment
tanzhenxin
left a comment
There was a problem hiding this comment.
Review: fix(vscode): force fresh ACP session on new-session action
Overview
Fixes a bug where clicking "+" (new session) had no effect because createNewSession() silently reused the existing ACP session. Adds a forceNew option to AgentSessionOptions and passes it from all explicit new-session call sites. Also removes a Node.js-only runtime import from the webview-bundled imageSupport.ts.
Issues
1. Fresh ACP session but stale currentConversationId — data mixing (High)
Both new-session paths only emit conversationCleared (SessionMessageHandler.ts:590, WebViewProvider.ts:1617) but neither clears currentConversationId. The next prompt skips createConversation() because the old id is still present, appends the "new session" transcript to the prior local chat, and the first-message title logic never reruns. In practice, "+" appends to the prior conversation and can leave the header stuck.
2. forceNew doesn't guarantee fresh session during bootstrap (Medium)
createNewSession() bypasses currentSessionId reuse but still unconditionally returns sessionCreateInFlight (qwenAgentManager.ts:1177). Clicking "+" during startup can collapse onto the original bootstrap session instead of issuing a second session/new.
3. Incomplete UI reset for "fresh session" (Medium)
conversationCleared only clears messages/tool calls/title (useWebViewMessages.ts:916); it does not clear usageStats or request-state flags (isStreaming, isWaitingForResponse). The context bar still shows old session's token usage, and the composer may remain blocked if "+" is clicked mid-request.
Positives
- Clean API extension with
forceNewopt-in flag and safefalsedefault. - All three correct call sites covered.
- Thorough test coverage at manager, handler, and provider levels.
- Bundle safety test and MIME alignment test are clever safeguards.
Verdict
REQUEST_CHANGES — The forceNew mechanism correctly creates a fresh ACP session, but the local conversation state (#1), dedup guard (#2), and UI reset (#3) are incomplete. The "+" action will append to the prior conversation and show stale UI state.
wenshao
reviewed
Apr 6, 2026
wenshao
reviewed
Apr 6, 2026
wenshao
reviewed
Apr 6, 2026
Collaborator
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Review Summary
This PR fixes the "+" new-session button in the VSCode sidebar by adding a forceNew option to AgentSessionOptions, and replaces a Node.js-only core import in the webview bundle with a local MIME type set. The core logic change is clean and backward-compatible, with good test coverage.
2 suggestions found after multi-agent review (5 agents) + verification + reverse audit:
SessionMessageHandler.ts:596—currentConversationIdnot reset afterforceNewsession creation, causing local conversation history contamination.qwenAgentManager.ts:1197—forceNewdoesn't bypass the in-flight session dedup check, which can reproduce the original bug in a race with the initial bootstrap.
Both are edge-case improvements rather than blockers. Verdict: Comment — no critical issues, suggestions for improved state hygiene.
Reviewed by glm-5.1 via Qwen Code /review
…de-new-session-reset-context
…t-context refactor(vscode): harden context-usage display with trusted token limits
…ession-minimal # Conflicts: # packages/vscode-ide-companion/src/webview/App.tsx
tanzhenxin
reviewed
Apr 9, 2026
Collaborator
tanzhenxin
left a comment
tanzhenxin
left a comment
There was a problem hiding this comment.
Review
Nice rework — the resetConversationState helper is a big improvement, and the bundle safety tests are a clever touch. The forceNew mechanism works well for the common case.
A few minor things to consider (non-blocking):
knownTokenLimit vs explicit ACP null — In acpModelInfo.ts, the derived limit takes priority over an explicit null from the server. Unlikely to matter in practice since pattern table limits are correct for known models, but worth keeping in mind for future-proofing.
forceNew + in-flight dedup — If "+" is clicked during bootstrap, it still collapses onto the in-flight session. The race window is narrow and retrying works, so not a real issue — could be a nice follow-up hardening though.
Orphaned utils/tokenLimits.ts — Looks like it's no longer imported after the refactor. Can be deleted for cleanup.
Verdict
COMMENT — Looks good overall, nice work!
Collaborator
Author
|
@tanzhenxin I moved the non-blocking follow-up polish from your latest review into #3098 so this branch can stay focused on the original new-session fix. I won't keep stacking those changes into #2874. The earlier blocking items on this PR are already addressed here, so if the current diff looks good now, could you take another look and update the review state? |
tanzhenxin
approved these changes
Apr 11, 2026
Collaborator
tanzhenxin
left a comment
tanzhenxin
left a comment
There was a problem hiding this comment.
Review
All blocking issues from the first review are addressed. The forceNew mechanism works correctly, resetConversationState is thorough, and the bundle safety tests are a nice touch. Non-blocking polish moved to #3098 — good call.
Verdict
APPROVE
This was referenced Apr 12, 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
3 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
createNewSession()silently reused the existing ACP session when one was already active, so clicking the "+" (new session) button in the sidebar had no effect — context, model info, and usage stats were never reset.forceNewoption toAgentSessionOptionsand pass{ forceNew: true }from all explicit new-session call sites (SessionMessageHandler,WebViewProvider). The implicit bootstrap path continues to reuse existing sessions as before.isSupportedImageMimeTypefrom@qwen-code/qwen-code-coreinimageSupport.ts— the webview is bundled for a browser environment where Node.js-only core modules cause esbuild failures during development. Replaced with a localSUPPORTED_PASTED_IMAGE_MIME_TYPESset, with a test that pins it to the core-supported list.Files changed
qwenAgentManager.tsforceNewoption, bypass session reuse when setSessionMessageHandler.ts{ forceNew: true }for explicit new-session actionsWebViewProvider.ts{ forceNew: true }for sidebar new-session buttonimageSupport.tsforceNewbehavior and bundle safetyTest plan
qwenAgentManager.test.ts— verifiesforceNew: truecreates a fresh session even when one existsSessionMessageHandler.test.ts— verifies new-session webview message passesforceNewWebViewProvider.test.ts— verifies sidebar action passesforceNewimageSupport.test.ts— verifies local MIME set stays aligned with coreimageSupport.bundle.test.ts— verifies webview bundle has no core runtime importsCloses #2847