fix(vscode-ide-companion): unlock the session-switch overlay when the daemon never confirms - #10431
Conversation
… daemon never confirms Selecting a past session from the history dropdown arms switchingSessionId and renders the "switching session" overlay. The only clearing paths are callbacks from the embedded web-shell App (onSessionIdChange/onError); when the daemon is unreachable the App is never mounted (WorkspaceSessionProvider returns its unavailable state first), so the overlay and the disabled composer/history controls stay locked until the webview is reloaded (#10405). Arm a host-side timeout when the switch starts: if the embedded shell does not confirm the new session within the bound, clear switchingSessionId and surface a failure notice so the user can retry. The timer is cancelled when the switch is confirmed or errors, and on unmount. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Thanks for the PR! Template looks good ✓ Problem: observed bug with solid evidence. #10405 documents the full trigger path with line references against the #9811 head, the issue triage confirmed the root cause statically, and there is a component-level reproduction (the overlay text persists indefinitely; the failing pre-fix assertion is quoted in the issue). The defect exists only on the unmerged #9811 branch, which is why this stacks on it — the direction the issue triage pointed at ("fix lands in PR #9811 itself, or an immediate follow-up"). Direction: aligned. This restores an exit path from a permanently locked panel in the VS Code companion — a pure defect fix, not theoretical hardening. No CHANGELOG signal to check; the feature is not on Size: not core infrastructure — everything lives in Approach: the scope feels right. Of the three options the issue sketches (clear on a host-side unreachable transition, add a timeout, make the overlay cancellable), the timeout is the only one implementable without touching the web-shell package: when the daemon is unreachable, Risk: no high-risk paths matched. One structural caveat for reviewers: the base is an unmerged PR branch, so repo CI (whose Moving on to code review. 🔍 中文说明感谢贡献! 模板完整 ✓ 问题:已观测到的 bug,证据扎实。#10405 给出了基于 #9811 头部的完整触发路径与行号引用,issue triage 静态确认了根因,并有组件级复现(遮罩文本无限期停留;issue 中引用了修复前失败的断言)。该缺陷只存在于未合并的 #9811 分支上,因此本 PR 以该分支为 base——这正是 issue triage 指明的方向("修复应落在 PR #9811 本身,或紧随的 follow-up")。 方向:对齐。这是为 VS Code 伴侣中被永久锁死的面板恢复退出路径——纯缺陷修复,不是理论性加固。没有可查的 CHANGELOG 信号,该功能本身还未落到 规模:非核心基础设施——改动全部位于 方案:范围合理。issue 给出三个选项(宿主侧不可达状态转换时清除、加超时、让遮罩可取消),超时是唯一不需要改动 web-shell 包的方案:daemon 不可达时 风险:未命中高风险路径。一个给审阅者的结构性提醒:由于 base 是未合并 PR 的分支,仓库 CI(其 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewIndependent baseline first: with Went through the whole diff and the surrounding component at the PR head. The wiring is complete and I found no correctness issues:
The two new tests hit the right shape: the mock web-shell never calls back (the daemon-unreachable shape), fake timers advance 4× the bound, and the second test pins both the immediate clear and that no bogus timeout notice appears afterwards. Nothing to flag — no blockers, no convention violations. Testing evidence
Sandboxed verification would settle this: 中文说明代码审查先给独立基线:daemon 不可达时 逐行看过 diff 与 PR 头部上的周边组件,接线完整,未发现正确性问题:
两个新测试打点准确:mock 的 web-shell 永不回调(即 daemon 不可达形态),假定时器推进 4 倍时限,第二个测试同时钉住"确认即清除"与"事后不出现伪造超时提示"。无可指摘——没有阻塞项,没有规范违例。 测试证据
沙箱验证可以补齐这一点: — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — minimal fix that matches the approach I would have chosen independently, wiring verified complete at the reviewed commit; the one reservation is evidentiary, not in the code. Going back to my independent proposal: when the embedded shell never mounts, elapsed time is the host's only signal, and a self-healing watchdog is the right shape — this PR is exactly that, and the simpler-looking alternatives either need user action (cancel button) or web-shell changes (error propagation). Every line in the diff earns its place; there are no drive-bys. The timer's lifecycle is airtight under the interleavings I could construct (re-selection, mismatched confirmation, late confirmation after expiry, unmount mid-switch). Six months from now this reads as an obvious fix, which is the compliment. The honest caveat is the evidence, not the code: repo CI structurally never runs on this base branch, so the two new component tests — which are the pinning evidence, reported red before the fix — have only run on the author's machine. I'm approving anyway because the author is a maintainer, the reproduction chain is fully documented and statically confirmed, the blast radius of being wrong is the status quo (overlay stays locked as before — no regression path), and this branch only reaches users through #9811, where the combined tree meets full CI. A maintainer who wants independent proof before merging #9811 can settle it with Reminder for the merge path: this is a stacked PR — it should land after #9811 (or be retargeted to 中文说明回到我的独立方案:内嵌 shell 永不挂载时,经过的时间是宿主唯一的信号,自愈型看门狗是正确的形态——本 PR 正是如此,而看起来更简单的替代方案要么需要用户动手(取消按钮),要么需要改 web-shell(错误外传)。diff 中每一行都有存在理由,没有顺手改动。在我能构造的交错场景下(重复选择、不匹配的确认、超时后迟到的确认、切换中途卸载),定时器生命周期都无懈可击。半年后读起来像一个显而易见的修复——这是夸奖。 诚实的保留意见在证据而非代码:仓库 CI 结构性地不会在此 base 分支上运行,因此两个新组件测试——真正的钉住证据,据称修复前为红——只在作者机器上跑过。我仍然批准,理由是:作者是维护者,复现链条有完整记录并经静态确认,出错的影响半径就是现状(遮罩照旧锁死——不存在回归路径),且该分支只能经由 #9811 到达用户,届时合并后的树会接受完整 CI。想在合并 #9811 前获得独立证据的维护者,可以用 合并路径提醒:这是一个堆叠 PR——应在 #9811 之后合入(若该分支有变动则改 target 到 — Qwen Code · qwen3.8-max Reviewed at |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not reviewed: build-and-test — CI ran no test job covering this change on this base branch (only routing jobs ran; verify/build-cli/tmux-testing among the skipped checks), and the local workspace build chain fails identically at the merge base at packages/web-shell (pre-existing); only the changed suite (EmbeddedApp.test.tsx) was run locally and passed.
Test Plan (not a blocker): src/webview/EmbeddedApp.test.tsx — no such file or directory; src/webview/strings.test.ts — no such file or directory; src/webview/EmbeddedApp.tsx — no such file or directory; src/webview/strings.ts — no such file or directory.
中文说明
仅完成部分审查,审查缺口已披露。 建议见行内评论。
未审查:build-and-test — CI ran no test job covering this change on this base branch (only routing jobs ran; verify/build-cli/tmux-testing among the skipped checks), and the local workspace build chain fails identically at the merge base at packages/web-shell (pre-existing); only the changed suite (EmbeddedApp.test.tsx) was run locally and passed。
Test Plan(非阻断):src/webview/EmbeddedApp.test.tsx — no such file or directory; src/webview/strings.test.ts — no such file or directory; src/webview/EmbeddedApp.tsx — no such file or directory; src/webview/strings.ts — no such file or directory。
— qwen3.8-max via Qwen Code /review (v0.22.2)
…lock-daemon-unreachable Resolve the session-switch overlay timeout conflicts by keeping the cutover branch's effect-based timeout and stable handleShellError callback, and dropping this branch's redundant ref-based timer. Both sides independently bounded the switch overlay; the effect-based form covers switching and creating alike, so the ref mechanism was removed and its tests re-pointed at the shared implementation. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline. 2 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here.
2 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R2-2(round-2 numbering of the confirm-at-time-0 lower-bound finding) — same defect as R1-6 'advances straight to 60s / boundary unpinned', already reported in round 1 (comment 3883692612); folded into the R1-6 re-post
- R2-6 (error-path clear untested) — same defect as R1-5, already reported in round 1 (comment 3883692595); folded into the R1-5 re-post
Not explored to full depth (tool budget reached): "agent 5": run EmbeddedApp.test.tsx to confirm the suite is green — no node_modules in the review worktree and a full monorepo npm ci exceeds the tool budget; all te…; "agent 3c": live run of packages/vscode-ide-companion/src/webview/EmbeddedApp.test.tsx to confirm the new tests pass — the review worktree and the main checkout both lack…; "agent 6b": negative run — removing the timeout effect from EmbeddedApp.tsx to confirm the new tests go RED — skipped to avoid mutating shared-worktree source at the tool c….
Not reviewed: reverse audit — stopped before round 6 by the review time budget.
Test Plan (not a blocker): src/webview/EmbeddedApp.test.tsx — no such file or directory; src/webview/strings.test.ts — no such file or directory; src/webview/EmbeddedApp.tsx — no such file or directory; src/webview/strings.ts — no such file or directory; 13 tests pass — this review observed 468 passed; and 1 more.
Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:
packages/vscode-ide-companion/src/webview/EmbeddedApp.test.tsx:608 — [probe] D2-1 locked (disabled) state during a pending switch is never asserted — deleting the disabled binding ships greenpackages/vscode-ide-companion/src/webview/EmbeddedApp.test.tsx:615 — [probe] D2-2 stale-confirmation guard (EmbeddedApp.tsx:1269) is exercised by no test — deleting it ships greenpackages/vscode-ide-companion/src/webview/EmbeddedApp.test.tsx:618 — [probe] D2-4 onSessionInfoChange stale-info guard (EmbeddedApp.tsx:1286) is untested with a switch pendingpackages/vscode-ide-companion/src/webview/EmbeddedApp.test.tsx:621 — [probe] D2-3 confirm-time webShellSessionChanged post during a pending switch is never assertedpackages/vscode-ide-companion/src/webview/EmbeddedApp.test.tsx:586 — [probe] D2-5 switch actuation is unpinned — deleting onSelect's setRuntime (EmbeddedApp.tsx:848-851) ships green
中文说明
仅完成部分审查,审查缺口已披露。 建议见行内评论。 2 条建议级发现无法锚定到改动行,已丢弃;此处无需进一步处理。
本轮确认的 2 条建议级发现已在 PR 上报告过,不再重复发布(列表见上方英文部分)。
未探索到全部深度(达到工具调用预算):"agent 5":run EmbeddedApp.test.tsx to confirm the suite is green — no node_modules in the review worktree and a full monorepo npm ci exceeds the tool budget; all te…;"agent 3c":live run of packages/vscode-ide-companion/src/webview/EmbeddedApp.test.tsx to confirm the new tests pass — the review worktree and the main checkout both lack…;"agent 6b":negative run — removing the timeout effect from EmbeddedApp.tsx to confirm the new tests go RED — skipped to avoid mutating shared-worktree source at the tool c…。
未审查:反向审计——评审时间预算不足,未能开始第 6 轮。
Test Plan(非阻断):src/webview/EmbeddedApp.test.tsx — no such file or directory; src/webview/strings.test.ts — no such file or directory; src/webview/EmbeddedApp.tsx — no such file or directory; src/webview/strings.ts — no such file or directory; 13 tests pass — this review observed 468 passed; and 1 more。
收敛姿态下延后(第 2 轮,非阻断)——已记录,本轮不要求修改:共 5 条(原文未翻译,列表见上方英文部分)。
— qwen3.8-max via Qwen Code /review (v0.22.3)
|
Closeout: resolved five outdated review threads left behind after later commits. No code changes; current scan found no active review threads or red exact-head checks. |
|
Superseded — closing. The same fail-safe is already on |
What this PR does
Adds a host-side timeout for the VS Code companion's session-switch overlay. When the user picks a past session from the history dropdown,
EmbeddedAppnow arms a timer; if the embedded web-shell has not confirmed the new session within the bound (15s), it clearsswitchingSessionIdand shows a localized failure notice so the user can retry. The timer is cancelled when the switch is confirmed, when the shell reports an error, and on unmount.Why it's needed
Fixes #10405. While the daemon is unreachable, selecting a session from history arms
switchingSessionId, and the only clearing paths are callbacks from the embedded web-shellApp(onSessionIdChangeon a matching id,onError). With the daemon unreachable,WorkspaceSessionProviderrenders its unavailable state before mountingApp, so neither callback can ever fire — the "switching session" overlay plus the disabled composer/history controls lock the panel permanently until the webview is reloaded. This was flagged in the PR #9811 review threadPRRT_kwDOPB-92c6dFJHQand acknowledged as an unresolved follow-up.Reviewer Test Plan
How to verify
Component-level tests against the real
EmbeddedApp(red before the fix, green after):unlocks the panel when the daemon never confirms a session switch— opens history, selects a past session while the embedded shell never calls back (the daemon-unreachable shape:Appnever mounts). Before the fix the overlay stayed forever:expect(container.textContent).not.toContain('Loading conversation…')failed with"…Loading conversation…Old conversation"still rendered. After the fix the overlay clears once the timeout elapses and the history button is re-enabled.still clears the overlay at once when the shell confirms the switch— regression guard: a confirmed switch clears the overlay immediately and cancels the failure timer, so no bogus "timed out" notice appears afterwards.Manual equivalent: run the companion against a live daemon, stop the daemon, pick a past session — the overlay appears, then lifts after ~15s with the notice "Switching conversations timed out. Try again." / "切换会话超时,请重试。"
Evidence (Before & After)
Verified at component-test level (no live daemon in this environment). Before: the repro test fails — overlay text stays after arbitrary time elapses. After: 13/13 tests pass, overlay clears at the bound and controls re-enable.
Tested on
Environment (optional)
Unit/component tests only (vitest, jsdom) against
codex/vscode-web-shell-cutoverheadb97ca86297; no live daemon involved.Risk & Scope
runtime.sessionIdwas already updated on selection, so a late confirmation still settles through the normalonSessionIdChangepath.creatingSessionflag shares the overlay but already clears via.finally()(per issue triage its exposure is limited to a never-settling create promise) — untouched. No web-shell package changes.session.switchFailed(EN + zh-CN).Linked Issues
Fixes #10405
Targets the unmerged PR #9811 branch (
codex/vscode-web-shell-cutoveras base, same approach as #10419), since the defect exists only on that branch, not onmain.中文说明
这个 PR 做了什么
为 VS Code 伴侣的会话切换遮罩增加宿主侧超时。用户从历史下拉选择过往会话后,
EmbeddedApp会启动一个定时器;如果内嵌 web-shell 在时限(15 秒)内没有确认新会话,就清除switchingSessionId并显示本地化的失败提示,让用户可以重试。定时器在切换被确认、shell 报错或组件卸载时取消。为什么需要
修复 #10405。daemon 不可达时,从历史列表选择会话会置位
switchingSessionId,而仅有的两条清除路径都来自内嵌 web-shellApp的回调(匹配 id 的onSessionIdChange、onError)。daemon 不可达时WorkspaceSessionProvider会在挂载App之前就渲染不可用状态,两个回调都不可能触发——"切换会话"遮罩加上被禁用的输入框/历史控件会把面板永久锁死,只能重载 webview。该问题在 PR #9811 的 review 线程PRRT_kwDOPB-92c6dFJHQ中已被指出,作者承认为未解决的 follow-up。审阅者测试方案
如何验证
针对真实
EmbeddedApp的组件级测试(修复前红、修复后绿):unlocks the panel when the daemon never confirms a session switch——打开历史、选择一个过往会话,内嵌 shell 永不回调(即 daemon 不可达形态:App不会挂载)。修复前遮罩永久停留:expect(container.textContent).not.toContain('Loading conversation…')失败,页面仍渲染"…Loading conversation…Old conversation"。修复后超时即清除遮罩,历史按钮恢复可用。still clears the overlay at once when the shell confirms the switch——回归保护:确认成功的切换仍立即清除遮罩并取消失败定时器,事后不会冒出伪造的"超时"提示。命令与输出见英文部分(13/13 通过,
tsc --noEmit与 eslint 均干净)。手动等价验证:用在线 daemon 启动伴侣,停掉 daemon,选择一个过往会话——遮罩出现,约 15 秒后解除并提示 "切换会话超时,请重试。"
前后对比证据
以组件级测试验证(本环境无真实 daemon)。修复前:复现测试失败——任意长时间后遮罩文本仍在;修复后:13/13 通过,遮罩在时限处解除、控件恢复可用。
测试环境
环境(可选)
仅单元/组件测试(vitest、jsdom),基于
codex/vscode-web-shell-cutover头部b97ca86297;未涉及真实 daemon。风险与范围
runtime.sessionId已更新,迟到的确认仍会走正常onSessionIdChange路径落定。creatingSession共用同一遮罩,但它已有.finally()清除路径(按 issue triage 其暴露面仅限 promise 永不落定)——未改动。不改动 web-shell 包。session.switchFailed(英文 + 中文)。关联 Issue
Fixes #10405
目标为未合并的 PR #9811 分支(以
codex/vscode-web-shell-cutover为 base,与 #10419 相同做法),因为该缺陷只存在于该分支,main上没有。