Conversation
|
@buaoyezz Bro, you can look at my branch changes a few days ago, #2188 there are several main problems are:
|
Sorry, I just saw your PR. Got it, thanks! |
…s from PR #2195 - Add extractSessionListItems() utility for robust ACP response parsing - Refactor getSessionList() and getSessionListPaged() to use the new utility - Add openNewChatTabCommand to create new session when opening chat tab - Add comprehensive test coverage for session list extraction Co-authored-by: ZZAoYe <zzbuaoye@gmail.com>
|
Issues & Suggestions
SidebarWebviewProvider.ts (766 lines) is largely a copy-paste of WebViewProvider.ts (1450 lines). The constructor, agent event wiring, permission handling, auth flow, forceReLogin(), respondToPendingPermission(), handleWebviewReady(), active editor listeners, and more are Recommendation: Extract shared logic into a base class or composition helper. The sidebar and panel providers differ only in how they host the webview (WebviewView vs WebviewPanel) — the agent lifecycle, permission handling, and message routing are identical and should be
updateAuthStateFromMessage() is called in sendMessageToWebView(), which means the sidebar tracks its own auth state based on outgoing messages. This works but is fragile — if a new message type is added that affects auth state and someone forgets to update this switch
The openNewChatTab command now calls provider.createNewSession(), but the PR doesn't show where createNewSession is defined on SidebarWebviewProvider. Looking at the code, there's no createNewSession method on the sidebar provider — only in QwenAgentManager. If Recommendation: Either add createNewSession() to SidebarWebviewProvider or ensure the createWebViewProvider callback in registerNewCommands always returns a WebViewProvider.
WebViewProvider handles AskUserQuestionRequest with pendingAskUserQuestionRequest/pendingAskUserQuestionResolve. The sidebar provider does not register an onAskUserQuestion handler at all. This means interactive prompts from the agent will hang or fail when initiated from
In the permission request callback (~line 629), this.messageHandler.setPermissionHandler(handler) is called inside a new Promise constructor. But handler is a closure that calls this.pendingPermissionResolve?.() — which is the same resolve that was just set. If a second WebViewProvider has the same issue, but duplicating it isn't great.
In the permission handler (~lines 637-646), when isCancel is true, qwen.diff.closeAll is executed. But then the void (async () => { ... })() block also runs. Meanwhile, in the pendingPermissionResolve callback (~line 621), the !isCancel branch also runs closeAll and
The PR adds tests for registerNewCommands, extractSessionListItems, and sidebar registration in extension.test.ts, but there are no unit tests for SidebarWebviewProvider — the largest new file (766 lines). Permission handling, auth state tracking, and webview lifecycle are
SidebarWebviewProvider creates a QwenAgentManager and ConversationStore in its constructor (called at activation time), even if the user never opens the sidebar. This was noted in prior review comments. Consider lazy initialization. |
Thanks for the detailed feedback. I understand this PR won’t be merged as-is, and I appreciate that some parts were cherry-picked into #2188. Since that PR has already landed and this branch now conflicts with main, I’ll close this PR. |
TLDR
Adds a focused VS Code sidebar integration for Qwen Code.
What changed
Validation
pm --prefix packages/vscode-ide-companion test
pm exec eslint src/extension.ts src/extension.test.ts src/webview/WebViewContent.ts src/webview/SidebarWebviewProvider.ts
This supersedes #1954 with a smaller, latest-main-based change set focused only on sidebar integration.