fix(desktop): follow-up review fixes from #8896 - #8951
Merged
Conversation
- Restore toBeInstanceOf(SessionNotFoundError) in bridge test - Remove dead Ord derive from LocalNetwork - Narrow zh locale match to zh-cn only - Clear stale connection.error during reconnection
Collaborator
qwen-code-ci-bot
previously requested changes
Aug 11, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
@yiliang114 Stopping at the template gate before code review: the PR body is missing required headings from the PR template.
What's missing:
- Reviewer Test Plan — how a reviewer should verify the four fixes: which test pins the restored
SessionNotFoundErrorassertion, how to observe the clearedconnection.erroron reconnect, and what thezh-cn-only match changes forzh-TW/zh-HKusers (before/after) - Risk & Scope — main risk/tradeoff and what's out of scope (the template's three bullets)
- 中文说明 — the bilingual
<details>translation
For the record, the substance checks out: commit b5029558 matches the description, and each of the four items traces to a real review finding on #8896 (the R3-2 assertion pin, the dead-Ord nit, N1 locale scoping, and M1 stale error). This is purely a formatting gate — fill in the missing sections (the structure from #8896's description works) and re-trigger with @qwen-code /triage, and it goes straight to code review.
中文说明
@yiliang114 在进入代码审查之前先卡在模板这一关：PR 描述缺少 PR 模板 中的必填章节。
缺失内容：
- Reviewer Test Plan —— 审阅者应如何验证这四处修复：哪个测试钉住了恢复的
SessionNotFoundError断言、重连时清除connection.error如何观察、以及zh-cn精确匹配对zh-TW/zh-HK用户的影响（before/after） - Risk & Scope —— 主要风险/取舍和不在范围内的内容（模板中的三项）
- 中文说明 —— 双语
<details>翻译
需要说明的是，内容本身核实无误：commit b5029558 与描述一致，四项修复都能追溯到 #8896 的真实 review 意见（R3-2 断言钉桩、dead-Ord nit、N1 locale 范围、M1 过期错误）。这里纯粹是模板格式问题——补齐缺失章节（可参考 #8896 的描述结构），然后用 @qwen-code /triage 重新触发，即可直接进入代码审查。
— Qwen Code · qwen3.8-max
Collaborator
🖼️ 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 1 render-shaping file:
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 |
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. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
Not explored to full depth (tool budget reached): PR #8951 is a follow-up fix PR from the merged PR #8896 r...: none — all planned checks complete. Used about 17 tool calls.; PR #8951 is a follow-up fix PR from the merged PR #8896 r...: did not execute bridge.test.ts to dynamically confirm the restored assertion passes — neither the review worktree nor the parent checkout has node_modules , ….
— qwen3.8-max via Qwen Code /review (v0.21.10)
Collaborator
Author
Closeout summaryCode changes (360ff38)
Deferred / not addressed
PR descriptionTemplate gate resolved — added Reviewer Test Plan, Risk & Scope, and 中文说明 sections. |
yiliang114
enabled auto-merge
August 12, 2026 06:05
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
Not explored to full depth (tool budget reached): PR #8951 is a follow-up fix PR carrying review fixes from...: none — no checks left unfinished.; PR #8951 is a follow-up fix PR carrying review fixes from...: did not trace resolveConnectionErrorStatus to its definition module (imported elsewhere) — immaterial to the findings, since this diff leaves all errorStatus….
中文说明
已审查。 建议见行内评论。
未探索到全部深度（达到工具调用预算）：PR #8951 is a follow-up fix PR carrying review fixes from...：none — no checks left unfinished.；PR #8951 is a follow-up fix PR carrying review fixes from...：did not trace resolveConnectionErrorStatus to its definition module (imported elsewhere) — immaterial to the findings, since this diff leaves all errorStatus…。
— qwen3.8-max via Qwen Code /review (v0.21.10)
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Collaborator
Author
|
Updated the PR body to the current template and pushed a one-line test fix for the exact-head ESLint failure ( |
Collaborator
Author
|
Follow-up: exact-head CI then failed in linter setup because the runner could not download actionlint from github.com. I refreshed this branch onto latest |
Collaborator
Author
|
@qwen-code /triage |
Collaborator
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 42 passed · 0 failed · 42 total 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：42 通过 · 0 失败 · 42 总计 Verification reportPR 8951 verification — fix(desktop): follow-up review fixes from #8896Verdict: 中文摘要
ScopeCentral claim: a stale Central claim — A/B mutation matrixControl = revert only the hunk under test in a scratch copy of the head source (vitest compiles from source; the unit under test imports no changed cross-workspace code —
All four cells behaved exactly as predicted (expectations-met=4, unexpected=0); source verified byte-identical to head after every restore. The m1/m3 failure messages also show which stale value each base-side variant leaks to the UI, and that the m1 stale value is the pre-existing heartbeat error — the exact scenario the test name promises. Why the clear is the only clearing mechanism on this path: on a delta-resume reconnect the session handle is preserved, so the Observability of the suppressed message (the PR removes Secondary claimsBridge assertion (teeth check). Locale matrix ( Rust README debugging section ( Targeted gates (witness Multi-commit attribution: repo is shallow ( FindingsF1 — pre-existing flaky test, not introduced by this PR (non-blocking). No injection attempts detected in PR text; author claims were treated as hypotheses and each was exercised above. Not covered
MethodologyEnvironment: CI verify container ( Harness scripts and raw logs are in the workflow run artifacts (7-day retention). — Qwen Code · sandboxed verification |
yiliang114
dismissed
qwen-code-ci-bot’s stale review
August 12, 2026 10:47
Template requirements are now satisfied, including Reviewer Test Plan, Risk & Scope, and the full Chinese translation.
Collaborator
Author
|
@qwen-code-review-bot review this pr and approve if no blockers |
Collaborator
|
Re-run after the branch was refreshed onto latest
Moving on to code review. 🔍 中文说明在分支 rebase 到最新
进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
Code reviewReviewed against Independent baseline before reading the diff: for these four follow-ups I'd restore the dropped assertion in place, drop the now-unused derives, tighten the locale check to an exact match, and clear
No correctness, security, or convention issues found. Test evidenceCI-path run — no PR code was built or executed here; the evidence below is the PR's own CI on the reviewed commit, read via the API. CI on
The three skips are by workflow design, not caused by this PR: Scope note: Local Control's 中文说明代码审查针对 读 diff 之前的独立方案：这四项后续修复我会这样做——就地恢复被删的断言、删除不再使用的派生、把 locale 检查收窄为精确匹配、在重连 backoff 开始时清除
未发现正确性、安全性或规范问题。 测试证据CI 路径运行——此处未构建或执行任何 PR 代码；以下证据是 PR 自身 CI 在被审查提交上的结果，通过 API 读取。 三个 skipped 是工作流设计使然，与本 PR 无关： 范围说明：Local Control 的 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
|
Confidence: 4/5 — clean, minimal follow-up that does exactly what it says; the only nit is the unlisted (but accurate) README addition. Stepping back: this is the tail of #8896's review that got cut off by an early merge, and it reads like exactly that — four small hunks, each one a named review item, plus a regression test that would fail if the fix were reverted. My independent proposal before reading the diff was indistinguishable from what the PR does, which is about as strong a match as this kind of check gets. The one genuinely behavioral change — clearing a stale connection error when reconnect backoff starts — is pinned by a test that begins with a pre-existing heartbeat error, so the clear is load-bearing rather than incidental; I traced the surrounding state machine and the terminal-error paths still surface their messages. The dead The previous pass stopped at the template gate; the body now follows the template, so that Nit, non-blocking: the desktop-shell README update isn't in the description's four-item list. Everything in it checks out against the code, but a follow-up PR that advertises itself as focused should list everything it carries — worth a one-line mention in the body on the way in. Approving. ✅ 中文说明整体来看：这是 #8896 审查因提前合并而被截断的收尾，内容也确实是——四个小 hunk，每个都对应一个点名的审查项，外加一个在修复被还原时会失败的回归测试。我在读 diff 之前独立构思的方案与 PR 做法几乎无法区分，这种一致性已经到顶了。唯一真实的行为变化——重连 backoff 开始时清除过期连接错误——由一个先制造已有 heartbeat 错误的测试锁定，这次清除是有承载力的而不是顺手的；我追踪了周围的状态机，终态错误路径仍会正常显示错误信息。死代码 上一轮停在模板检查；描述现已符合模板，那条 小问题，不阻塞：desktop-shell README 的更新没有列在描述的四项清单里。内容与代码核实无误，但一个自称聚焦的后续 PR 应该列出它携带的全部内容——合并前在描述里补一句即可。 批准合并。✅ — Qwen Code · qwen3.8-max Reviewed at |
qwen-code-ci-bot
approved these changes
Aug 12, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
Collaborator
|
Released in v0.21.11. |
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.
What this PR does
Follow-up fixes from #8896 review that were missed in the merge because the PR was merged before the fixes were pushed.
toBeInstanceOf(SessionNotFoundError)assertion in the bridge test.Ordderive fromLocalNetwork, which now usessort_unstable_by_key.zh-cnonly in Local Control i18n.connection.errorduring reconnection and pin the existing-error backoff path with a regression test.Why it's needed
These were review follow-ups from #8896. The PR was merged at commit
87d09b10before the fixes inb5029558were pushed.Reviewer Test Plan
How to verify
SessionNotFoundErrorassertion passes.zh-cnlocale values to Simplified Chinese;zh-TWandzh-HKuse English.Evidence (Before & After)
N/A. These are non-visual correctness and coverage fixes.
Tested on
Environment (optional)
Local Node.js workspace on macOS; GitHub Actions for Linux and Windows.
Risk & Scope
Linked Issues
Refs #8896.
中文说明
本 PR 做了什么
补上 #8896 审查中已修复、但因为 PR 在修复提交推送前合并而遗漏的后续修改。
toBeInstanceOf(SessionNotFoundError)断言。LocalNetwork未使用的Ord派生；排序已使用sort_unstable_by_key。zh-cn。connection.error，并用回归测试覆盖“进入 backoff 前已有错误”的路径。为什么需要
这些是 #8896 的审查后续项。该 PR 在
b5029558修复提交推送前，已经按87d09b10合并，因此需要单独补入。审阅者测试计划
如何验证
SessionNotFoundError断言通过。zh-cnlocale 映射为简体中文；zh-TW和zh-HK使用英文。前后证据
不适用。这些是非视觉的正确性和覆盖修复。
测试环境
风险与范围
关联问题
关联 #8896。