fix(web-shell): hide planned SSE reconnect as Connection lost - #9041
fix(web-shell): hide planned SSE reconnect as Connection lost#9041samuelhsin wants to merge 6 commits into
Conversation
Idle SSE recycle was painted as an outage banner and leaked to host onError. Co-authored-by: Cursor <cursoragent@cursor.com>
github-actions
Bot
added
the
review/self-reported
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
@samuelhsin Thanks for the PR — but the root cause here was already fixed on main two days ago, so this filter no longer has anything to match against.
The problem was real. When #8887 was reported, DaemonSessionProvider set connection.error to the backoff countdown ("Reconnecting in ${delayMs}ms") during auto-reconnect — verified at 82dd2e17, and a maintainer confirmed the chain in the issue thread.
But it was fixed at the source. #8896 (fix(desktop): consolidate 0.1.1 regressions, merged Aug 11 as 28ab8bae) removed exactly that assignment — its description calls out "normal SSE endings do not surface a synthetic reconnect error". The backoff now sets error: undefined. I checked every connection.error assignment in the provider at abfd4436 — which is this PR's own merge base: the remaining ones are all real failures (auth, terminal session errors, missing baseUrl, session-close exposure), and none produces Reconnecting in \d+ms.
So on this branch:
- The
/^Reconnecting in \d+ms$/filter cannot match — the "Connection lost" banner this PR targets can no longer occur upstream. - The PR still changes behavior beyond that dead premise:
shouldSurfaceDaemonConnectionErroralso suppresses all connection errors whilestatus === 'connecting'(both the pane banner and the hostonError). That could hide genuine connect-time failures, and the PR description doesn't motivate it.
Requested change: please re-verify against current main. If you can still reproduce the banner somewhere (e.g. a hosted build carrying an older provider), share that reproduction — otherwise #8896 resolved #8887 and this can be closed. If the connecting-state suppression or the onError change has its own motivation, propose it separately with that justification.
中文说明
@samuelhsin 感谢贡献——但这里的根因两天前已经在 main 上修复了,这个过滤器已经没有可匹配的内容。
问题曾经是真实存在的。 #8887 报告时,DaemonSessionProvider 在自动重连退避期间会把 connection.error 设为倒计时文案("Reconnecting in ${delayMs}ms")——已在 82dd2e17 核实,maintainer 也在 issue 线程里确认了这条链路。
但根因已被源头修复。 #8896(fix(desktop): consolidate 0.1.1 regressions,8 月 11 日合并,28ab8bae)恰好删掉了这个赋值——它的描述明确写了 "normal SSE endings do not surface a synthetic reconnect error"。退避现在设置 error: undefined。我检查了 abfd4436(即本 PR 的 merge base)上 provider 里所有 connection.error 赋值:剩余的都是真实故障(auth、终态 session 错误、缺少 baseUrl、session 关闭暴露),没有任何路径会产生 Reconnecting in \d+ms。
因此在这个分支上:
/^Reconnecting in \d+ms$/过滤不可能命中——本 PR 针对的「Connection lost」横幅在上游已不会再出现。- 在这个已失效的前提之外,PR 还改变了行为:
shouldSurfaceDaemonConnectionError同时会抑制status === 'connecting'时的所有连接错误(窗格横幅和宿主onError都不再触发)。这可能掩盖真实的连接期故障,而 PR 描述没有为此提供动机。
请求修改:请在当前 main 上重新验证。如果你仍能在某处复现横幅(例如使用旧版 provider 的托管构建),请提供该复现;否则 #8896 已经解决了 #8887,本 PR 可以关闭。如果 connecting 状态的抑制或 onError 的变更有独立动机,请单独提出并给出各自的理由。
— Qwen Code · qwen3.8-max
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. |
🖼️ web-shell visual previewRendered against a mock daemon (no real backend): the PR base vs this PR head Screenshots · before / afterℹ️ No screenshot changed against the PR base — but this PR edits 2 render-shaping files:
Either the change has no visual effect (logic, plumbing, a state the scenarios never reach), or no scenario renders this UI — in which case the preview cannot see it, and an empty result is a coverage gap rather than a clean bill of health. To make it visible, add a scenario to Full-resolution recordings (.webm) are attached to the workflow run. — Qwen Code · web-shell visuals |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
Unresolved, please confirm:
- [Critical] Existing review-level blocker (qwen-code-ci-bot triage, CHANGES_REQUESTED, 2026-08-13T04:06Z): the root cause of issue 8887 was already fixed by PR 8896 (commit 28ab8ba) before this PR's base, so the countdown filter has nothing to match — independently re-verified at 897b297 and still standing; already posted by this account as an open CHANGES_REQUESTED review, not re-posted here.
Not explored to full depth (tool budget reached): Context: PR #9041 fixes a Web Shell bug where a planned S...: could not inspect the installed @qwen-code/sdk package source (node_modules not present in the worktree) to confirm the exact producer and whether it ever emi…; Context: PR #9041 fixes a Web Shell bug where a planned S...: none — all checks I started were completed; I did not attempt to run the web-shell test suite (review-only task; CI covers it).; Context: PR #9041 fixes a Web Shell bug where a planned S...: nothing left incomplete — all planned checks were executed within the budget (about 34 calls).## Review result — Agent 1a (line-by-line correctness): no finding….
Test Plan (not a blocker): client/utils/sessionErrors.test.ts — no such file or directory; client/components/ChatPane.test.tsx — no such file or directory.
中文说明
已审查。 建议见行内评论。
未决，请确认：共 1 条（原文未翻译，列表见上方英文部分）。
未探索到全部深度（达到工具调用预算）：Context: PR #9041 fixes a Web Shell bug where a planned S...：could not inspect the installed @qwen-code/sdk package source (node_modules not present in the worktree) to confirm the exact producer and whether it ever emi…；Context: PR #9041 fixes a Web Shell bug where a planned S...：none — all checks I started were completed; I did not attempt to run the web-shell test suite (review-only task; CI covers it).；Context: PR #9041 fixes a Web Shell bug where a planned S...：nothing left incomplete — all planned checks were executed within the budget (about 34 calls).## Review result — Agent 1a (line-by-line correctness): no finding…。
Test Plan（非阻断）：client/utils/sessionErrors.test.ts — no such file or directory; client/components/ChatPane.test.tsx — no such file or directory。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| onError?.(new Error(connectionError)); | ||
| } | ||
| }, [connection.error, onError]); | ||
| }, [connectionError, connectionStatus, onError]); |
There was a problem hiding this comment.
[Suggestion] The App-level onError forwarding effect — the host-facing half of this fix — has no test coverage: App.test.tsx never renders App with an onError prop (the only onError captured there is the unrelated AskUserQuestion callback). The new ChatPane tests cover the banner in a different component, and sessionErrors.test.ts covers the predicate in isolation, but nothing exercises this effect. — Failure scenario: reverting this effect to the pre-PR unconditional if (connection.error) onError?.(...) keeps the entire suite green (execution probe: 3 files, 518/518 tests pass on the revert arm), so the planned-reconnect countdown would silently resume being forwarded to the host's onError — the original bug on the host-facing path — with no failing test.
Suggested tests (the renderApp harness already mutates mockConnection.error/status, e.g. around line 9723):
it('does not forward a planned SSE reconnect countdown to the host onError', () => {
const onError = vi.fn();
mockConnection.status = 'disconnected';
mockConnection.error = 'Reconnecting in 1000ms';
renderApp({ onError }); // adapt to the harness shape
expect(onError).not.toHaveBeenCalled();
});
it('forwards a real connection error to the host onError', () => {
const onError = vi.fn();
mockConnection.status = 'error';
mockConnection.error = 'Unauthorized';
renderApp({ onError });
expect(onError).toHaveBeenCalledWith(
expect.objectContaining({ message: 'Unauthorized' }),
);
});中文说明
App 层的 onError 转发 effect——本修复面向宿主的一半——没有任何测试覆盖：App.test.tsx 从未给 App 传过 onError prop（那里唯一捕获的 onError 是无关的 AskUserQuestion 回调）。新增的 ChatPane 测试覆盖的是另一个组件里的横幅，sessionErrors.test.ts 只单独覆盖了谓词本身，没有任何测试走过这个 effect。失败场景：把这个 effect 还原为 PR 前的无条件 if (connection.error) onError?.(...)，整个测试套件仍然全绿（执行探针：3 个文件在还原分支上 518/518 通过），计划内重连倒计时会悄悄恢复转发给宿主 onError——即宿主侧路径上的原始 bug——且没有任何测试失败。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| export function shouldSurfaceDaemonConnectionError(connection: { | ||
| status: string; | ||
| error?: string; | ||
| }): connection is { status: string; error: string } { |
There was a problem hiding this comment.
[Suggestion] The predicate types status as plain string, decoupling the !== 'connecting' comparison from the DaemonConnectionStatus union at compile time. Both call sites (App.tsx, ChatPane.tsx) already hold union-typed status from useConnection(); only this signature widens it. — Failure scenario: if the provider's status union is ever renamed (e.g. backoff moves to a 'reconnecting' state) while this predicate keeps the stale 'connecting' literal, tsc accepts it (plain string comparison) and the hardcoded test strings still match the stale literal, so surfacing behavior silently flips at runtime with no compile or test signal. (A mistyped comparison literal is likewise accepted by tsc, though the existing connecting test case would catch that variant.) Execution probe (5-arm tsc): the stale and typo'd literals compile under status: string but fail with TS2367 under union typing; the union-typed version compiles cleanly at both real call sites.
import type { DaemonConnectionStatus } from '@qwen-code/webui/daemon-react-sdk';
export function shouldSurfaceDaemonConnectionError(connection: {
status: DaemonConnectionStatus;
error?: string;
}): connection is { status: DaemonConnectionStatus; error: string } {(DaemonConnectionStatus is re-exported at packages/webui/src/daemon-react-sdk.ts:188; both caller files already import from that package.)
中文说明
谓词把 status 标为普通 string，使 !== 'connecting' 比较在编译期与 DaemonConnectionStatus 联合类型脱钩。两个调用点（App.tsx、ChatPane.tsx）从 useConnection() 拿到的本来就是联合类型的 status，只有这个签名把它放宽成了 string。失败场景：如果 provider 的 status 联合类型将来改名（例如退避阶段改用 'reconnecting' 状态），而该谓词仍保留过时的 'connecting' 字面量，tsc 会接受（普通字符串比较），且硬编码的测试字符串仍会匹配过时的字面量，于是展示行为在运行时悄悄翻转，既无编译信号也无测试信号。（比较字面量打错同样会被 tsc 接受，不过现有的 connecting 测试用例能捕获这种变体。）执行探针（5 组 tsc 对照）：过时/打错的字面量在 status: string 下可以编译，在联合类型下报 TS2367；联合类型版本在两个真实调用点都能干净编译。
— qwen3.8-max via Qwen Code /review (v0.21.10)
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not explored to full depth (tool budget reached): "Context: PR #9041 fixes Web Shell treating a planned SSE…": exact producer of the Reconnecting in <N>ms countdown string could not be located — it is not in the repo sources of this worktree and node_modules is not ins…; "Context: PR #9041 fixes Web Shell treating a planned SSE…": could not verify the external SDK's actual countdown message format against the regex — @qwen-code/webui/daemon-react-sdk (source of the 'Reconnecting in Nms….
Test Plan (not a blocker): client/utils/sessionErrors.test.ts — no such file or directory; client/components/ChatPane.test.tsx — no such file or directory.
中文说明
未探索到全部深度（达到工具调用预算）："Context: PR #9041 fixes Web Shell treating a planned SSE…"：exact producer of the Reconnecting in <N>ms countdown string could not be located — it is not in the repo sources of this worktree and node_modules is not ins…；"Context: PR #9041 fixes Web Shell treating a planned SSE…"：could not verify the external SDK's actual countdown message format against the regex — @qwen-code/webui/daemon-react-sdk (source of the 'Reconnecting in Nms…。
Test Plan（非阻断）：client/utils/sessionErrors.test.ts — no such file or directory; client/components/ChatPane.test.tsx — no such file or directory。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| ); | ||
| } | ||
|
|
||
| const PLANNED_RECONNECT_ERROR = /^Reconnecting in \d+ms$/u; |
There was a problem hiding this comment.
[Critical] R2-1: This PR claims to fix issue 8887, but issue 8887's root cause was already fixed at the source by PR 8896 (28ab8bae56, merged 2026-08-11 — an ancestor of this branch's merge base), so at the reviewed commit the /^Reconnecting in \d+ms$/ filter has nothing to match. git show 28ab8bae56 deletes exactly error: \Reconnecting in ${delayMs}ms`from theDaemonSessionProviderbackoff (the backoff now setserror: undefined), and every remaining error:` assignment in the provider is a real failure (auth, terminal session errors, non-retriable without autoReconnect, heartbeat exhaustion). The new tests pass only by injecting the obsolete pre-PR-8896 literal directly.
Beyond that dead premise, shouldSurfaceDaemonConnectionError also suppresses all connection errors while status === 'connecting' (pane banner and host onError) — not requested by issue 8887, not motivated in the PR description, and unreachable against the in-tree provider (every transition into connecting sets error: undefined). Since web-shell accepts any @qwen-code/webui >= 0.0.1 peer, a host provider that reports an error while connecting would have a genuine connect-time failure silently swallowed.
Failure scenario: merging ships dead code plus an unargued semantic change → hosts on a provider build that pairs connecting with a set error lose both the banner and onError for real failures (silent connect-time outage), while the banner this PR targets can no longer occur upstream.
This independently re-verifies the open triage CHANGES_REQUESTED blocker against the reviewed commit (git merge-base --is-ancestor 28ab8bae56 HEAD → true; no Reconnecting in producer remains in-tree). Suggested path: close this PR (PR 8896 already resolved issue 8887), or reduce it to a separately motivated proposal — e.g. an explicit defense for hosts pinned to pre-PR-8896 webui builds, which needs a maintainer decision.
中文说明
[Critical] R2-1：本 PR 声称修复 issue 8887，但 issue 8887 的根因已经由 PR 8896（28ab8bae56，2026-08-11 合并——是本分支 merge base 的祖先）在源头修复，因此在被审查的提交上 /^Reconnecting in \d+ms$/ 过滤器没有任何可匹配的内容。git show 28ab8bae56 恰好删除了 DaemonSessionProvider 退避路径中的 error: \Reconnecting in ${delayMs}ms`（退避现在设置 error: undefined），provider 中剩余的所有 error:` 赋值都是真实故障（鉴权、终态 session 错误、未开 autoReconnect 的不可重试错误、heartbeat 耗尽）。新增测试之所以通过，只是因为直接注入了 PR 8896 之前的过时文案。
在这个已失效的前提之外，shouldSurfaceDaemonConnectionError 还会抑制 status === 'connecting' 时的所有连接错误（窗格横幅和宿主 onError）——issue 8887 没有提出这个需求，PR 描述也没有为此提供动机，而且在 tree 内的 provider 上不可达（每次进入 connecting 都同时设置 error: undefined）。由于 web-shell 接受任意 @qwen-code/webui >= 0.0.1 peer 版本，宿主 provider 如果在 connecting 时携带错误，真实的连接期故障会被静默吞掉。
失败场景：合并后会引入死代码加一个没有理由说明的语义变更 → 使用「connecting 携带错误」的 provider 构建的宿主，将同时失去横幅和 onError（连接期故障静默）；而本 PR 针对的横幅在上游已不会再出现。
以上是对未决 triage CHANGES_REQUESTED 阻断项在被审查提交上的独立复核（git merge-base --is-ancestor 28ab8bae56 HEAD → true；tree 内已无任何 Reconnecting in 生产者）。建议路径：关闭本 PR（PR 8896 已解决 issue 8887），或将其缩减为单独提出、单独给出动机的提案——例如明确为仍固定在 PR 8896 之前 webui 构建的宿主做防御，这需要 maintainer 决策。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| ); | ||
| } | ||
|
|
||
| const PLANNED_RECONNECT_ERROR = /^Reconnecting in \d+ms$/u; |
There was a problem hiding this comment.
[Suggestion] R2-3: The suppression is a cross-package string contract pinned nowhere: no shared constant, no producer-side test, and the tests hardcode the identical bare literal 'Reconnecting in 1000ms' on both sides — they prove the matcher agrees with itself, not with any producer. The only in-tree producer of that string was removed by PR 8896, and nothing in the code says whether this filter is dead code or load-bearing (its only live targets are hosts pinned to older @qwen-code/webui builds, per the peer range >=0.0.1). Independently reported by five review dimensions this round.
Failure scenario: if webui (or a host-pinned webui) ever re-surfaces a reconnect countdown with different wording (Reconnecting in 1.5s, a prefix/suffix, an attempt counter, localization), the anchored regex silently stops matching, the false "Connection lost" banner and host onError return for every planned SSE recycle, and every test added by this PR still passes — no CI signal warns anyone.
Suggested fix: pin the contract at the producer — export the sentinel from @qwen-code/webui (or better, a structured signal such as a reconnecting status / errorKind field) and consume it here; or add a contract test in packages/webui asserting the reconnect backoff never populates connection.error. Minimal version as a one-click suggestion:
| const PLANNED_RECONNECT_ERROR = /^Reconnecting in \d+ms$/u; | |
| // Matches the reconnect-backoff countdown DaemonSessionProvider used to surface via | |
| // connection.error; that assignment was removed by PR 8896. Kept for hosts pinned | |
| // to older @qwen-code/webui builds (peer range >=0.0.1) that still emit it. | |
| const PLANNED_RECONNECT_ERROR = /^Reconnecting in \d+ms$/u; |
中文说明
[Suggestion] R2-3：这个抑制逻辑是一个没有在任何地方固定的跨包字符串契约：没有共享常量，没有生产者侧测试，且测试两侧都硬编码了同一个裸字面量 'Reconnecting in 1000ms'——它们证明的只是匹配器与它自己一致，而不是与任何生产者一致。该字符串在 tree 内唯一的生产者已被 PR 8896 删除，代码中也没有任何说明能区分这个过滤器是死代码还是仍然有效（它目前唯一可能的生效对象，是固定在旧版 @qwen-code/webui 构建上的宿主，peer 范围为 >=0.0.1）。本轮有五个独立的审查维度各自报告了此问题。
失败场景：如果 webui（或宿主固定的 webui）将来以不同文案重新出现重连倒计时（Reconnecting in 1.5s、前后缀、尝试次数、本地化），带锚定的正则将静默失配，每次计划内 SSE 回收都会重新出现误报的 "Connection lost" 横幅和宿主 onError，而本 PR 新增的所有测试仍会通过——CI 不会给出任何警告。
建议修复：在生产者侧固定契约——从 @qwen-code/webui 导出该哨兵（更好的做法是结构化信号，例如 reconnecting 状态 / errorKind 字段）并在此消费；或在 packages/webui 中加一个契约测试，断言重连退避永远不会写入 connection.error。最简版本见下方一键 suggestion：
— qwen3.8-max via Qwen Code /review (v0.21.11)
| typeof connection.error === 'string' && | ||
| connection.error.length > 0 && |
There was a problem hiding this comment.
[Suggestion] R2-5: The empty-string/undefined error branches added in this diff are untested. A verification probe removed connection.error.length > 0 && and re-ran the suite: 86/86 still passed — the mutant survives. Adding the two cases below fails against the mutant and passes with the guard restored (flip-verified), so the guard is load-bearing but unpinned.
Failure scenario: deleting .length > 0 leaves every test green, after which a connection object carrying error: '' renders a role="alert" banner reading "Connection lost: " with no message text and fires onError with an empty Error — behavior the pre-diff connection.error && check suppressed.
Suggested fix: add two unit cases to sessionErrors.test.ts:
expect(
shouldSurfaceDaemonConnectionError({ status: 'disconnected', error: '' }),
).toBe(false);
expect(
shouldSurfaceDaemonConnectionError({ status: 'disconnected' }),
).toBe(false);中文说明
[Suggestion] R2-5：本 diff 新增的空字符串 / undefined 错误分支没有测试覆盖。验证探针删除了 connection.error.length > 0 && 后重跑测试套件：86/86 依然全部通过——变异体存活。补上下述两个用例后，针对变异体会失败、恢复守卫后通过（已做翻转验证），说明该守卫确实起作用，只是没有被测试钉住。
失败场景：删除 .length > 0 后所有测试仍为绿色，此后携带 error: '' 的 connection 对象会渲染一个没有消息内容的 role="alert" 横幅（"Connection lost: "），并以空 Error 触发 onError——而 diff 之前的 connection.error && 检查会抑制这种行为。
建议修复：在 sessionErrors.test.ts 中补两个用例（代码见英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.11)
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not explored to full depth (tool budget reached): "Context: PR #9041 (QwenLM/qwen-code) changes the Web Shell…": none — no checks left unfinished.; "Context: PR #9041 (QwenLM/qwen-code) changes the Web Shell…": none — all planned checks are complete. One more thing: should I quickly run lint on the changed files? Not needed. Formatting is prettier-standard. Skip.; "Context: PR #9041 (QwenLM/qwen-code) changes the Web Shell…": could not inspect @qwen-code/sdk (external dependency, node_modules absent in the worktree) to confirm the real countdown message format against PLANNED_RECO….
Test Plan (not a blocker): client/utils/sessionErrors.test.ts — no such file or directory; client/components/ChatPane.test.tsx — no such file or directory.
[Critical] R2-1 (re-checked at 58e31dd this round — still stands; originally posted as the inline thread at sessionErrors.ts:8): This PR claims to fix issue 8887, but issue 8887's root cause was already fixed at the source by PR 8896 (28ab8ba, merged 2026-08-11 — an ancestor of this branch's merge base), so at the reviewed commit the /^Reconnecting in \d+ms$/ filter has nothing to match; the new tests pass only by injecting the obsolete pre-8896 literal. Beyond that dead premise, shouldSurfaceDaemonConnectionError suppresses ALL connection errors while status === 'connecting' (pane banner and host onError) — not requested by issue 8887, not motivated in the PR description, and able to swallow genuine connect-time failures for host providers allowed by the @qwen-code/webui >= 0.0.1 peer range. Suggested path: close this PR (PR 8896 already resolved issue 8887), or reduce it to a separately motivated proposal with a maintainer decision.
中文说明
未探索到全部深度（达到工具调用预算）："Context: PR #9041 (QwenLM/qwen-code) changes the Web Shell…"：none — no checks left unfinished.；"Context: PR #9041 (QwenLM/qwen-code) changes the Web Shell…"：none — all planned checks are complete. One more thing: should I quickly run lint on the changed files? Not needed. Formatting is prettier-standard. Skip.；"Context: PR #9041 (QwenLM/qwen-code) changes the Web Shell…"：could not inspect @qwen-code/sdk (external dependency, node_modules absent in the worktree) to confirm the real countdown message format against PLANNED_RECO…。
Test Plan（非阻断）：client/utils/sessionErrors.test.ts — no such file or directory; client/components/ChatPane.test.tsx — no such file or directory。
[Critical] R2-1 (re-checked at 58e31dd this round — still stands; originally posted as the inline thread at sessionErrors.ts:8): This PR claims to fix issue 8887, but issue 8887's root cause was already fixed at the source by PR 8896 (28ab8ba, merged 2026-08-11 — an ancestor of this branch's merge base), so at the reviewed commit the /^Reconnecting in \d+ms$/ filter has nothing to match; the new tests pass only by injecting the obsolete pre-8896 literal. Beyond that dead premise, shouldSurfaceDaemonConnectionError suppresses ALL connection errors while status === 'connecting' (pane banner and host onError) — not requested by issue 8887, not motivated in the PR description, and able to swallow genuine connect-time failures for host providers allowed by the @qwen-code/webui >= 0.0.1 peer range. Suggested path: close this PR (PR 8896 already resolved issue 8887), or reduce it to a separately motivated proposal with a maintainer decision.
— qwen3.8-max via Qwen Code /review (v0.21.11)
| return ( | ||
| typeof connection.error === 'string' && | ||
| connection.error.length > 0 && | ||
| connection.status !== 'connecting' && | ||
| !isPlannedReconnectError(connection.error) | ||
| ); |
There was a problem hiding this comment.
[Suggestion] R3-1: Suppressed connection errors leave zero trace — when shouldSurfaceDaemonConnectionError swallows a non-empty error (a planned-reconnect countdown, or ANY error while status === 'connecting' — the new unit test explicitly blesses hiding Unauthorized there), nothing is written anywhere: no banner, no host onError, no log line. Before this change the raw text surfaced in the banner and reached the host onError (a hook hosts can log); the only residual host signal is onConnectionChange with the bare status. — Concrete cost: oncall investigating "the web shell silently stopped updating mid-session" cannot distinguish "the connection never errored" from "it errored continuously but every signal was suppressed".
| return ( | |
| typeof connection.error === 'string' && | |
| connection.error.length > 0 && | |
| connection.status !== 'connecting' && | |
| !isPlannedReconnectError(connection.error) | |
| ); | |
| const surfaced = | |
| typeof connection.error === 'string' && | |
| connection.error.length > 0 && | |
| connection.status !== 'connecting' && | |
| !isPlannedReconnectError(connection.error); | |
| if (!surfaced && connection.error) { | |
| console.debug('[web-shell] suppressed connection error:', connection.error); | |
| } | |
| return surfaced; |
中文说明
[Suggestion] R3-1：被抑制的连接错误不留任何痕迹——当 shouldSurfaceDaemonConnectionError 吞掉一个非空错误时（计划内重连倒计时，或 status === 'connecting' 时的任何错误——新增单元测试明确认可了在该状态下隐藏 Unauthorized），任何地方都没有记录：没有横幅、没有宿主 onError、没有日志。改动之前，原始文案会显示在横幅里并送达宿主 onError（宿主可以据此打日志）；改动之后宿主仅剩 onConnectionChange 的空状态信号。——具体代价：值班排查「web shell 会话中途静默停止更新」时，无法区分「连接从未出错」和「连接一直出错但所有信号都被抑制」。
建议：在返回 false 之前，对被抑制的非空错误打一条 console.debug（如可行，按不同消息去重），使被抑制的状态仍然可以排查。
— qwen3.8-max via Qwen Code /review (v0.21.11)
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not explored to full depth (tool budget reached): "Context: PR #9041 (QwenLM/qwen-code) changes the Web Shell…": none — all checks I started were completed within budget.; "You are review agent reverse-audit — Reverse audit agent…": none — all checks above completed within budget.; "Context: PR #9041 (QwenLM/qwen-code) changes the Web Shell…": could not locate the producer of the literal Reconnecting in Nms string — it appears nowhere in this repo's source (searched web-shell, webui incl. the reconn…; "Context: PR #9041 (QwenLM/qwen-code) changes the Web Shell…": could not execute the web-shell vitest suite (dependencies not installed in this worktree); all checks were static reads of the exact reviewed commit..
Test Plan (not a blocker): client/utils/sessionErrors.test.ts — no such file or directory; client/components/ChatPane.test.tsx — no such file or directory.
[Critical] R2-1: This PR claims to fix issue 8887, but issue 8887's root cause was already fixed at the source by PR 8896 (28ab8ba, merged 2026-08-11 — an ancestor of this branch's merge base, re-verified via git merge-base --is-ancestor at the reviewed commit fe63b1b), so the /^Reconnecting in \d+ms$/ filter has nothing to match: the backoff in DaemonSessionProvider now sets error: undefined, and no other in-tree path produces the countdown string. The new tests pass only by injecting the obsolete pre-8896 literal. Beyond that dead premise, shouldSurfaceDaemonConnectionError suppresses ALL connection errors while status === 'connecting' (pane banner and host onError) — not requested by issue 8887, not motivated in the PR description, unreachable against the in-tree provider, and able to swallow genuine connect-time failures for host providers allowed by the @qwen-code/webui >= 0.0.1 peer range. The maintainer-requested reproduction on an older provider build was never posted. Suggested path: close this PR (PR 8896 already resolved issue 8887), or reduce it to a separately motivated proposal — e.g. an explicit defense for hosts pinned to pre-PR-8896 webui builds — which needs a maintainer decision. (Re-checked at fe63b1b this round — still stands; originally posted as the inline thread at sessionErrors.ts:8.)
中文说明
未探索到全部深度（达到工具调用预算）："Context: PR #9041 (QwenLM/qwen-code) changes the Web Shell…"：none — all checks I started were completed within budget.；"You are review agent reverse-audit — Reverse audit agent…"：none — all checks above completed within budget.；"Context: PR #9041 (QwenLM/qwen-code) changes the Web Shell…"：could not locate the producer of the literal Reconnecting in Nms string — it appears nowhere in this repo's source (searched web-shell, webui incl. the reconn…；"Context: PR #9041 (QwenLM/qwen-code) changes the Web Shell…"：could not execute the web-shell vitest suite (dependencies not installed in this worktree); all checks were static reads of the exact reviewed commit.。
Test Plan（非阻断）：client/utils/sessionErrors.test.ts — no such file or directory; client/components/ChatPane.test.tsx — no such file or directory。
[Critical] R2-1: This PR claims to fix issue 8887, but issue 8887's root cause was already fixed at the source by PR 8896 (28ab8ba, merged 2026-08-11 — an ancestor of this branch's merge base, re-verified via git merge-base --is-ancestor at the reviewed commit fe63b1b), so the /^Reconnecting in \d+ms$/ filter has nothing to match: the backoff in DaemonSessionProvider now sets error: undefined, and no other in-tree path produces the countdown string. The new tests pass only by injecting the obsolete pre-8896 literal. Beyond that dead premise, shouldSurfaceDaemonConnectionError suppresses ALL connection errors while status === 'connecting' (pane banner and host onError) — not requested by issue 8887, not motivated in the PR description, unreachable against the in-tree provider, and able to swallow genuine connect-time failures for host providers allowed by the @qwen-code/webui >= 0.0.1 peer range. The maintainer-requested reproduction on an older provider build was never posted. Suggested path: close this PR (PR 8896 already resolved issue 8887), or reduce it to a separately motivated proposal — e.g. an explicit defense for hosts pinned to pre-PR-8896 webui builds — which needs a maintainer decision. (Re-checked at fe63b1b this round — still stands; originally posted as the inline thread at sessionErrors.ts:8.)
— qwen3.8-max via Qwen Code /review (v0.21.11)
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not explored to full depth (tool budget reached): "Context: PR #9041 (QwenLM/qwen-code) changes the Web Shell…": could not execute the new tests** — the worktree has no node_modules (no root or package-level vitest binary), and I did not spend a full monorepo install; th…; "You are review agent reverse-audit — Reverse audit agent…": none — all checks above completed within budget.; "Context: PR #9041 (QwenLM/qwen-code) changes the Web Shell…": none — all checks above completed within budget..
Test Plan (not a blocker): client/utils/sessionErrors.test.ts — no such file or directory; client/components/ChatPane.test.tsx — no such file or directory.
[Critical] R2-1: This PR claims to fix issue 8887, but issue 8887's root cause was already fixed at the source by PR 8896 (28ab8ba, merged 2026-08-11 — an ancestor of the reviewed commit b6b2b6c), so at the reviewed commit the /^Reconnecting in \d+ms$/ filter has nothing to match: git show 28ab8bae56 deletes exactly error: \Reconnecting in ${delayMs}ms`from the DaemonSessionProvider backoff (the backoff now setserror: undefined), and every remaining error:assignment in the provider is a real failure (auth, terminal session errors, non-retriable without autoReconnect, heartbeat exhaustion). The new tests pass only by injecting the obsolete pre-PR-8896 literal directly. Beyond that dead premise,shouldSurfaceDaemonConnectionErrorsuppresses ALL connection errors whilestatus === 'connecting'(pane banner and hostonError) — not requested by issue 8887, not motivated in the PR description, and unreachable against the in-tree provider (every transition into 'connecting' sets error: undefined). Since web-shell accepts any @qwen-code/webui >= 0.0.1peer, a host provider that reports an error while 'connecting' would have a genuine connect-time failure silently swallowed. Failure scenario: merging ships dead code plus an unargued semantic change → hosts on a provider build that pairs 'connecting' with a set error lose both the banner andonError` for real failures (silent connect-time outage), while the banner this PR targets can no longer occur upstream. Re-verified at b6b2b6c this round (ancestry check, producer grep, provider state machine, executed new tests) — still stands. Suggested path: close this PR (PR 8896 already resolved issue 8887), or reduce it to a separately motivated proposal — e.g. an explicit defense for hosts pinned to pre-PR-8896 webui builds, with the reproduction evidence — which needs a maintainer decision.
中文说明
未探索到全部深度（达到工具调用预算）："Context: PR #9041 (QwenLM/qwen-code) changes the Web Shell…"：could not execute the new tests** — the worktree has no node_modules (no root or package-level vitest binary), and I did not spend a full monorepo install; th…；"You are review agent reverse-audit — Reverse audit agent…"：none — all checks above completed within budget.；"Context: PR #9041 (QwenLM/qwen-code) changes the Web Shell…"：none — all checks above completed within budget.。
Test Plan（非阻断）：client/utils/sessionErrors.test.ts — no such file or directory; client/components/ChatPane.test.tsx — no such file or directory。
[Critical] R2-1: This PR claims to fix issue 8887, but issue 8887's root cause was already fixed at the source by PR 8896 (28ab8ba, merged 2026-08-11 — an ancestor of the reviewed commit b6b2b6c), so at the reviewed commit the /^Reconnecting in \d+ms$/ filter has nothing to match: git show 28ab8bae56 deletes exactly error: \Reconnecting in ${delayMs}ms`from the DaemonSessionProvider backoff (the backoff now setserror: undefined), and every remaining error:assignment in the provider is a real failure (auth, terminal session errors, non-retriable without autoReconnect, heartbeat exhaustion). The new tests pass only by injecting the obsolete pre-PR-8896 literal directly. Beyond that dead premise,shouldSurfaceDaemonConnectionErrorsuppresses ALL connection errors whilestatus === 'connecting'(pane banner and hostonError) — not requested by issue 8887, not motivated in the PR description, and unreachable against the in-tree provider (every transition into 'connecting' sets error: undefined). Since web-shell accepts any @qwen-code/webui >= 0.0.1peer, a host provider that reports an error while 'connecting' would have a genuine connect-time failure silently swallowed. Failure scenario: merging ships dead code plus an unargued semantic change → hosts on a provider build that pairs 'connecting' with a set error lose both the banner andonError` for real failures (silent connect-time outage), while the banner this PR targets can no longer occur upstream. Re-verified at b6b2b6c this round (ancestry check, producer grep, provider state machine, executed new tests) — still stands. Suggested path: close this PR (PR 8896 already resolved issue 8887), or reduce it to a separately motivated proposal — e.g. an explicit defense for hosts pinned to pre-PR-8896 webui builds, with the reproduction evidence — which needs a maintainer decision.
— qwen3.8-max via Qwen Code /review (v0.21.11)
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not explored to full depth (tool budget reached): "PR #9041 (QwenLM/qwen-code) adds a shouldSurfaceDaemonConne…": none — all checks I started completed (~17 calls). I did not execute the vitest suite (node_modules not installed in the worktree); CI covers execution.; "PR #9041 (QwenLM/qwen-code) adds a shouldSurfaceDaemonConne…": could not execute sessionErrors.test.ts / ChatPane.test.tsx — worktree has no node_modules ( vitest unresolvable) and a full install outweighed the value…; "PR #9041 (QwenLM/qwen-code) adds a shouldSurfaceDaemonConne…": no npm run typecheck run; type-level claims above are from manual analysis of the involved types ( DaemonConnectionStatus , error?: string , Error construc…; "PR #9041 (QwenLM/qwen-code) adds a shouldSurfaceDaemonConne…": none — all checks above completed within budget..
Test Plan (not a blocker): client/utils/sessionErrors.test.ts — no such file or directory; client/components/ChatPane.test.tsx — no such file or directory.
[Critical] R2-1: This PR claims to fix issue 8887, but issue 8887's root cause was already fixed at the source by PR 8896 (28ab8ba, merged 2026-08-11 — an ancestor of this branch's merge base, re-verified via git merge-base --is-ancestor at the reviewed commit bd9be75), so the /^Reconnecting in \d+ms$/ filter has nothing to match: git show 28ab8bae56 deletes exactly error: \Reconnecting in ${delayMs}ms`from the DaemonSessionProvider backoff (the backoff now setserror: undefined), and every remaining error:assignment in the provider is a real failure (auth, terminal session errors, non-retriable without autoReconnect, heartbeat exhaustion). The new tests pass only by injecting the obsolete pre-PR-8896 literal directly. Beyond that dead premise,shouldSurfaceDaemonConnectionErrorsuppresses ALL connection errors whilestatus === 'connecting'(pane banner and hostonError) — not requested by issue 8887, not motivated in the PR description, unreachable against the in-tree provider (every transition into 'connecting' sets error: undefined), and able to swallow genuine connect-time failures for host providers allowed by the @qwen-code/webui >= 0.0.1peer range. The maintainer-requested reproduction on an older provider build was never posted; the only change since the last reviewed commit (b6b2b6c7) is a merge of main. Failure scenario: merging ships dead code plus an unargued semantic change → the banner this PR targets can no longer occur upstream, and "Fixes #8887" would auto-close an issue already resolved by PR 8896, while hosts on a provider build that pairs 'connecting' with a set error lose both the banner andonError` for real failures (silent connect-time outage). Suggested path: close this PR (PR 8896 already resolved issue 8887), or reduce it to a separately motivated proposal — e.g. an explicit defense for hosts pinned to pre-PR-8896 webui builds, with the reproduction evidence — which needs a maintainer decision. (Re-checked at bd9be75 this round — still stands; originally posted as the inline thread at sessionErrors.ts:8, not re-posted inline because it overlaps that existing thread.)
中文说明
未探索到全部深度（达到工具调用预算）："PR #9041 (QwenLM/qwen-code) adds a shouldSurfaceDaemonConne…"：none — all checks I started completed (~17 calls). I did not execute the vitest suite (node_modules not installed in the worktree); CI covers execution.；"PR #9041 (QwenLM/qwen-code) adds a shouldSurfaceDaemonConne…"：could not execute sessionErrors.test.ts / ChatPane.test.tsx — worktree has no node_modules ( vitest unresolvable) and a full install outweighed the value…；"PR #9041 (QwenLM/qwen-code) adds a shouldSurfaceDaemonConne…"：no npm run typecheck run; type-level claims above are from manual analysis of the involved types ( DaemonConnectionStatus , error?: string , Error construc…；"PR #9041 (QwenLM/qwen-code) adds a shouldSurfaceDaemonConne…"：none — all checks above completed within budget.。
Test Plan（非阻断）：client/utils/sessionErrors.test.ts — no such file or directory; client/components/ChatPane.test.tsx — no such file or directory。
[Critical] R2-1: This PR claims to fix issue 8887, but issue 8887's root cause was already fixed at the source by PR 8896 (28ab8ba, merged 2026-08-11 — an ancestor of this branch's merge base, re-verified via git merge-base --is-ancestor at the reviewed commit bd9be75), so the /^Reconnecting in \d+ms$/ filter has nothing to match: git show 28ab8bae56 deletes exactly error: \Reconnecting in ${delayMs}ms`from the DaemonSessionProvider backoff (the backoff now setserror: undefined), and every remaining error:assignment in the provider is a real failure (auth, terminal session errors, non-retriable without autoReconnect, heartbeat exhaustion). The new tests pass only by injecting the obsolete pre-PR-8896 literal directly. Beyond that dead premise,shouldSurfaceDaemonConnectionErrorsuppresses ALL connection errors whilestatus === 'connecting'(pane banner and hostonError) — not requested by issue 8887, not motivated in the PR description, unreachable against the in-tree provider (every transition into 'connecting' sets error: undefined), and able to swallow genuine connect-time failures for host providers allowed by the @qwen-code/webui >= 0.0.1peer range. The maintainer-requested reproduction on an older provider build was never posted; the only change since the last reviewed commit (b6b2b6c7) is a merge of main. Failure scenario: merging ships dead code plus an unargued semantic change → the banner this PR targets can no longer occur upstream, and "Fixes #8887" would auto-close an issue already resolved by PR 8896, while hosts on a provider build that pairs 'connecting' with a set error lose both the banner andonError` for real failures (silent connect-time outage). Suggested path: close this PR (PR 8896 already resolved issue 8887), or reduce it to a separately motivated proposal — e.g. an explicit defense for hosts pinned to pre-PR-8896 webui builds, with the reproduction evidence — which needs a maintainer decision. (Re-checked at bd9be75 this round — still stands; originally posted as the inline thread at sessionErrors.ts:8, not re-posted inline because it overlaps that existing thread.)
— qwen3.8-max via Qwen Code /review (v0.21.11)
Pull request was closed
2 participants
What this PR does
Stops WebShell from treating a planned SSE recycle as an outage. Idle chat panes (including Split View) no longer flash an orange "Connection lost / Reconnecting in ~1000ms" banner, and the embedding host no longer receives that countdown as an
onError. Real failures still surface: auth errors, exhausted heartbeat, and other genuine disconnect messages.Why it's needed
Hosts in front of the daemon intentionally recycle long-lived event streams so half-open subscribers cannot pile up. The session already resumes via
Last-Event-IDwithout wiping the transcript, so users can keep chatting. Painting that recycle as "Connection lost" is a false alarm, and Split View repeats it once per pane. Fixes #8887.Reviewer Test Plan
How to verify
packages/web-shell):npx vitest run client/utils/sessionErrors.test.ts client/components/ChatPane.test.tsxstatus: 'disconnected'anderror: 'Reconnecting in 1000ms'shows no "Connection lost" alert.status: 'error'with a real message (for exampleUnauthorized) still shows the orange alert.Evidence (Before & After)
Before: idle Split View panes flashed
Connection lost: Reconnecting in ~1000ms(Chinese UI:连接已断开: Reconnecting in ~1000ms) on each planned SSE recycle, even though prompts and streaming still worked.After: planned reconnect countdown is not rendered as an alert and is not forwarded to host
onError. Auth and heartbeat-exhausted failures still show the banner.Tested on
Environment (optional)
N/A — unit tests only. Hosted proxy recycle was not re-run in this change.
Risk & Scope
error.reconnectingconnection status; changing host-side SSE recycle timers; live hosted DataWorks idle-recycle verification.onErrorfor the reconnect countdown will stop seeing that false alarm.Linked Issues
Fixes #8887
中文说明
这个 PR 做了什么
避免 WebShell 把计划内的 SSE 回收当成故障。空闲聊天窗格（包括分栏）不再闪橙色「连接已断开 / Reconnecting in ~1000ms」横幅，嵌入宿主也不会再把这段倒计时当成
onError。真实故障仍会展示：鉴权失败、heartbeat 耗尽，以及其他真正的断开信息。为什么需要
daemon 前面的代理会有意回收长连接，避免半开订阅堆积。会话本身已经通过
Last-Event-ID续上，transcript 不会被清掉，用户也还能继续发消息。把这次回收画成「连接已断开」是误报，分栏还会每个窗格闪一次。修复 #8887。评审测试计划
如何验证
packages/web-shell下跑：npx vitest run client/utils/sessionErrors.test.ts client/components/ChatPane.test.tsxstatus: 'disconnected'且error: 'Reconnecting in 1000ms'的窗格不出现「Connection lost」告警。status: 'error'配上真实错误（例如Unauthorized）仍会显示橙色告警。证据（Before & After）
Before：空闲分栏在每次计划内 SSE 回收时闪
Connection lost: Reconnecting in ~1000ms（中文 UI：连接已断开: Reconnecting in ~1000ms），但 prompt 和流式回复其实还能用。After：计划内重连倒计时不再渲染为告警，也不会转发给宿主
onError。鉴权和 heartbeat 耗尽失败仍会显示横幅。测试环境
运行环境（可选）
N/A — 仅单元测试。本次没有在托管环境重跑代理回收。
风险与范围
error，横幅可能再次出现。reconnecting连接状态；没有改宿主侧 SSE 回收时间；没有做 DataWorks 托管空闲回收的实机验证。onError的宿主，将不再收到这次误报。关联 Issue
Fixes #8887
Made with Cursor