fix(web-shell): stop toasting on failed background artifact refreshes - #7437
fix(web-shell): stop toasting on failed background artifact refreshes#7437zjunothing wants to merge 1 commit into
Conversation
The session artifact panel's refresh is purely automatic — mount, prompt->idle, and the artifactsVersion workspace signal — yet loadArtifacts() routed every failure through dispatchActionError, which always fires an error-severity notice. During transient connectivity hiccups the panel repeatedly toasted "Load artifacts failed: Failed to fetch" for background polls the user cannot act on (QwenLM#7427). loadArtifacts now accepts { silent?: boolean }: when set, the failure notice is suppressed and the raw error is re-thrown (the calling hook needs the throw for its local error state). useSessionArtifacts passes silent: true from its only — automatic — call site; its existing catch already preserves last-good artifacts. This mirrors enqueueMidturnMessage's best-effort-and-silent philosophy for background-refreshable actions, per the maintainer's endorsed direction on the issue. User-initiated actions keep their notices. Regression tests per the issue's suggestion: a silent failure dispatches no notice while still rejecting; the default path keeps the notice; the hook passes silent on every refresh, lands the failure in local error state, and keeps last-good artifacts visible. Both new tests fail on the unpatched source. Fixes QwenLM#7427 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
🖼️ web-shell visual previewRendered against a mock daemon (no real backend): the PR base vs this PR head Screenshots · before / after✅ No screenshot changes against the PR base. Full-resolution recordings (.webm) are attached to the workflow run. — Qwen Code · web-shell visuals |
|
Thanks for the PR! Template looks good ✓ Problem: observed bug with clear evidence — #7427 reports the artifact panel spamming Direction: aligned. Suppressing toasts for background-refreshable actions follows the established Size: not applicable — no core module paths touched ( Approach: the scope feels right — interface option + conditional in the action + call-site update in the hook is the minimum viable change. The transient-vs-hard-error distinction (only suppress network-class failures) is deliberately left as a fast-follow per the maintainer's suggestion, which keeps this PR focused. No unrelated changes or drive-by refactors. Moving on to code review. 🔍 中文说明感谢贡献! 模板完整 ✓ 问题:已观测到的 bug,有明确证据——#7427 报告 artifact 面板在瞬时连接抖动期间反复弹出 方向:对齐。抑制后台可刷新操作的 toast 遵循同文件中 规模:不适用——未触及核心模块路径。 方案:范围合理——接口选项 + action 条件 + hook 调用点更新是最小可行变更。瞬态/硬错误区分按维护者建议留作快跟,保持本 PR 聚焦。无无关改动。 进入代码审查 🔍 — Qwen Code · qwen3.7-max Reviewed at |
Code reviewIndependent proposal (before reading the diff): given the title and motivation, I'd add an optional Findings: no critical blockers, no AGENTS.md violations. The implementation is clean and minimal — three production files changed (interface in
Tests are well-structured: the action-level tests verify both the silent path (no notice, still rejects) and the default path (notice preserved), and the hook-level test verifies Real-scenario testingThis is a browser-based web-shell feature — the artifact panel toast fires in the React UI when a background refresh fails. It can't be driven through tmux (no browser available in this environment). I verified what I could: the daemon starts and serves the web shell correctly with the PR code, and the unit tests comprehensively cover the behavior change. Unit tests (the definitive verification for this client-side behavior change): Typecheck clean on both 中文说明代码审查独立方案(未看 diff 前):根据标题和动机,我会给 发现: 无关键阻塞,无 AGENTS.md 违规。 实现干净且最小化——三个生产文件变更(
测试结构良好:action 级测试验证了 silent 路径(无 notice,仍拒绝)和默认路径(保留 notice),hook 级测试验证了每次刷新都传 真实场景测试这是浏览器端的 web-shell 功能——artifact 面板 toast 在后台刷新失败时由 React UI 触发。无法通过 tmux 驱动(此环境无浏览器)。已验证:daemon 使用 PR 代码正常启动并提供 web shell,单元测试全面覆盖了行为变更。 单元测试 29/29 通过。两个包的 typecheck 均干净。构建成功。 — Qwen Code · qwen3.7-max Reviewed at |
|
Confidence: 5/5 — clean across every stage; the maintainer endorsed this exact fix shape on the issue, the implementation matches it precisely, and the tests are solid. This is a textbook small fix: the problem is real and well-documented (#7427, root cause confirmed from source, maintainer-verified), the solution is the minimum viable change (one interface option, one conditional, one call-site update), and it follows an established precedent in the same file ( My independent proposal before reading the diff was identical to what the PR does — I didn't find a simpler path it missed. The code is straightforward, every change is necessary, and the If I had to maintain this in six months, I'd thank the author — clear comments, a well-documented interface, and a focused scope that's easy to reason about. 中文说明置信度:5/5 —— 每个阶段都干净;维护者在 issue 上背书了正是这个修复形状,实现精确匹配,测试扎实。 这是一个教科书式的小修复:问题真实且有充分文档(#7427,根因从源码确认,维护者验证),方案是最小可行变更(一个接口选项、一个条件、一个调用点更新),遵循同文件中已有的先例( 我在看 diff 前的独立方案与 PR 完全一致——没找到更简路径。代码直白,每个改动都必要, — Qwen Code · qwen3.7-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.
No issues found. LGTM! ✅
— qwen3.7-max via Qwen Code /review
doudouOUC
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
— qwen3.7-max via Qwen Code /review
| const session = requireSessionForAction( | ||
| addNotice, | ||
| sessionRef.current, |
There was a problem hiding this comment.
[Suggestion] requireSessionForAction sits before the try/catch block and calls dispatchActionError unconditionally when the session is gone — the silent check inside catch is never consulted for that error path. The hook's isConnected guard makes this unreachable today, but the type-level contract in types.ts ("silent: true suppresses the error-severity notice on failure") is incomplete: a future caller relying on it would still see a toast on session disconnection. The design precedent this PR cites (enqueueMidTurnMessage, line ~1008) avoids this by checking sessionRef.current directly and returning silently — it never calls requireSessionForAction. — Failure scenario: background refresh fires at the moment the session is torn down but React state is stale → requireSessionForAction toasts "Load artifacts failed" — the exact noise silent was designed to suppress.
| const session = requireSessionForAction( | |
| addNotice, | |
| sessionRef.current, | |
| if (options?.silent && !sessionRef.current) { | |
| throw new Error('Daemon session is not connected'); | |
| } | |
| const session = requireSessionForAction( | |
| addNotice, | |
| sessionRef.current, |
— qwen3.7-max via Qwen Code /review
gwinthis
left a comment
There was a problem hiding this comment.
Architecture Review — PR #7437
Verdict: ✅ Approve — correct silent-refresh pattern.
Design pattern: Silent background refresh
Automatic refreshes (mount, prompt→idle, artifactsVersion bump) pass { silent: true } to suppress the error toast. A transient fetch failure on an automatic poll is noise the user cannot act on.
Key properties:
- Error still thrown:
silentdoesn't mean swallowed — the error propagates to the calling hook, which stores it in local state - Last-good data preserved: On refresh failure,
artifactsstate is NOT cleared — the UI shows stale-but-valid data instead of going blank - Explicit opt-out: The
silentflag is per-call, not global — user-initiated refreshes still toast on failure
This mirrors the enqueueMidturnMessage best-effort-and-silent philosophy already established in the codebase.
Local verification — merge referenceI built and ran the real tests locally (Linux, Node v22.22.2, vitest v3.2.4, fresh worktrees). The PR is internally sound, but the bug it fixes (#7427) has already been fixed on What holds up ✅
Why it's superseded
|
current main (after #7477) |
this PR | |
|---|---|---|
actions.ts loadArtifacts |
no try/catch, no dispatchActionError → rejects raw error, no toast |
re-adds try/catch + dispatchActionError, gated by { silent } |
useSessionArtifacts catch |
catch { setError(null) } — swallows the error entirely, keeps last-good |
catch { setError(err.message) } — keeps error in local state |
So on current main, the background-refresh toast is already gone — in fact suppressed more aggressively than this PR does. Running the PR's own tests against current main source proves it:
suppresses the failure notice for silent loadArtifacts→ PASSES onmain(main is already silent).keeps the failure notice for default loadArtifacts→ FAILS onmain(main deleted that notice on purpose).
Merge status
Real conflict in packages/webui/src/daemon/session/actions.ts at loadArtifacts (confirmed by a clean test-merge into origin/main@434137b71; GitHub's CONFLICTING is accurate). The PR's only caller — useSessionArtifacts, which is fully automatic — would pass silent: true, so the restored non-silent toast branch is dead code. Net user-visible behavior after a conflict resolution that keeps the PR side = identical to main today, just with more code.
Recommendation
- Close as superseded by fix(web-shell): polish embedded shell interactions #7477 is the clean call — web-shell: artifact panel spams 'Load artifacts failed: Failed to fetch' on automatic refresh #7427 is resolved on
main, no user-visible regression. - The one thing this PR keeps that
maindropped is preserving the failure in the hook's localerrorstate (setError(err.message)vsmain'ssetError(null)). If you'd ever want a subtle "artifacts stale / refresh failed" affordance in the UI, that local-error signal is worth keeping — but that's a new, separate enhancement on top of fix(web-shell): polish embedded shell interactions #7477, not web-shell: artifact panel spams 'Load artifacts failed: Failed to fetch' on automatic refresh #7427, and there are zero non-automatic callers ofloadArtifactstoday to justify the{ silent }interface split. If desired, a small follow-up on the fix(web-shell): polish embedded shell interactions #7477 baseline is the better vehicle.
Verified on: origin/main @ 434137b71 · PR head @ 4440615cb (base 22433b642). Screenshots are real vitest runs from symlinked-node_modules worktrees.
🇨🇳 中文版(点击展开)
本地验证 — 合并参考
我在本地真实构建并运行了测试(Linux,Node v22.22.2,vitest v3.2.4,独立 worktree)。PR 本身自洽,但它修复的 #7427 已经被今天合入 main 的 #7477(cd5e1973e)抢先修掉了,因此本 PR 已被取代(superseded)且当前冲突。
成立的部分 ✅
- PR head 自洽:
packages/webui的actions.test.ts26/26、packages/web-shell的useSessionArtifacts.test.tsx3/3 在 PR head 全绿。 - 新增回归测试有效:只把源码回退到 PR 基线(保留测试)后,
suppresses…silent(webui)与passes silent…(web-shell)失败——与 PR 描述里"未修复源上失败"一致。 - web-shell: artifact panel spams 'Load artifacts failed: Failed to fetch' on automatic refresh #7427 的根因诊断正确(针对 PR 的基线):在
22433b642上,后台刷新失败确实走dispatchActionError弹error级 toast。
为何被取代 ⚠️
#7477 今天合入 main,对这条路径的两端都做了改写:
当前 main(#7477 之后) |
本 PR | |
|---|---|---|
actions.ts loadArtifacts |
无 try/catch、无 dispatchActionError → 直接 reject 原始错误,不弹 toast |
重新加回 try/catch + dispatchActionError,由 { silent } 控制 |
useSessionArtifacts catch |
catch { setError(null) } — 彻底吞掉错误、保留 last-good |
catch { setError(err.message) } — 把错误记进本地状态 |
也就是说当前 main 上后台刷新的 toast 已经消失——而且比本 PR 抑制得更彻底。把 PR 自己的测试跑在当前 main 源码上即可证明:
suppresses the failure notice for silent loadArtifacts→ 在main上通过(main 本就静默)。keeps the failure notice for default loadArtifacts→ 在main上失败(main 有意删掉了该 notice)。
合并状态
packages/webui/src/daemon/session/actions.ts 的 loadArtifacts 处存在真实冲突(在干净的 origin/main@434137b71 上试合并已确认;GitHub 的 CONFLICTING 属实)。PR 唯一调用方 useSessionArtifacts 完全是自动触发、会传 silent: true,因此被恢复的非 silent toast 分支是死代码。若按保留 PR 一侧来解冲突,最终用户可见行为与今天的 main 完全相同,只是代码更多。
建议
- 按被 fix(web-shell): polish embedded shell interactions #7477 取代关闭是干净的选择——web-shell: artifact panel spams 'Load artifacts failed: Failed to fetch' on automatic refresh #7427 在
main上已解决,无用户可见回归。 - 本 PR 相较
main唯一多做的一点:把失败保留在 hook 的本地error状态(setError(err.message),而main是setError(null))。若将来想在 UI 上做一个轻量的"artifacts 已过期 / 刷新失败"提示,这个本地 error 信号值得保留——但那是叠加在 fix(web-shell): polish embedded shell interactions #7477 之上的新的、独立的增强,而非 web-shell: artifact panel spams 'Load artifacts failed: Failed to fetch' on automatic refresh #7427;且当前loadArtifacts没有任何非自动调用方,不足以支撑{ silent }这个接口拆分。如需要,在 fix(web-shell): polish embedded shell interactions #7477 基线上做一个小的 follow-up 是更合适的载体。
验证于:origin/main @ 434137b71 · PR head @ 4440615cb(base 22433b642)。截图为 symlink-node_modules worktree 中的真实 vitest 运行。


What this PR does
Adds a
{ silent?: boolean }option toDaemonSessionActions.loadArtifacts: when set, the failure path skipsdispatchActionError's error-severity notice and re-throws the raw error (the calling hook needs the throw for its local error state).useSessionArtifacts— the action's only caller, and a purely automatic one (panel mount,promptStatus→ idle, theartifactsVersionworkspace signal) — passessilent: true; its existingcatchalready preserves last-good artifacts. User-initiated actions keep their notices, and the change mirrorsenqueueMidturnMessage's established best-effort-and-silent philosophy for background-refreshable actions.Why it's needed
#7427: the artifact panel repeatedly toasts
Load artifacts failed: Failed to fetchduring transient connectivity hiccups, because every automatic background refresh routes failures throughdispatchActionError, which unconditionally fires an error toast. The user can't act on a failed background poll, and the panel already keeps its last-good data — the toast is pure noise, reappearing on every retriggered refresh. The triage confirmed the root cause from source and the maintainer endorsed exactly this fix shape (interface option + conditional in the action + call-site update in the hook), including the regression-test design this PR implements.Reviewer Test Plan
How to verify
qwen serve, open the Web Shell with an active session, then briefly interrupt connectivity to the daemon (or block the artifacts endpoint). Before this PR: each automatic refresh pops an error toast. After: no toast; the panel keeps showing the last loaded artifacts; artifacts refresh resumes silently when connectivity returns.npx vitest run src/daemon/session/actions.test.ts(packages/webui) 26/26 andnpx vitest run client/hooks/useSessionArtifacts.test.tsx(packages/web-shell) 3/3.Evidence (Before & After)
Regression tests follow the design suggested on the issue, and both fail on the unpatched source (verified via
git stash):daemon.load_artifacts.failednotice{ silent: true }on every refresh; failure lands in localerrorstate; last-good artifacts stay visible;loadingsettlesTested on
Environment (optional)
macOS (Darwin 24.6), Node v22.23.1; vitest (jsdom for the hook).
npm run typecheck, eslint--max-warnings 0, prettier all clean.Risk & Scope
getStatsshares thedispatchActionErrorpattern but is only invoked by the explicit/statscommand, so it keeps its notice (per the maintainer's note); no other action changes.Linked Issues
Fixes #7427
中文说明
本 PR 做了什么
给
DaemonSessionActions.loadArtifacts增加{ silent?: boolean }:置位时失败路径跳过dispatchActionError的 error 级 notice、重抛原始错误(调用方 hook 需要 throw 来设置本地 error 状态)。唯一调用方useSessionArtifacts的刷新完全是自动触发(面板挂载、prompt→idle、artifactsVersion 信号),现传silent: true;其既有 catch 已保留上次成功的 artifacts。用户主动操作保留 notice;与同文件enqueueMidturnMessage的 best-effort-and-silent 先例一致。为什么需要
#7427:artifact 面板在瞬时连接抖动期间反复弹
Load artifacts failed: Failed to fetch——每次自动后台刷新的失败都走dispatchActionError无条件弹 toast。用户对失败的后台轮询无从行动、面板本就保留旧数据——toast 纯属噪音且随每次重触发反复出现。triage 已源码确认根因,维护者背书了正是本 PR 的修复形状(接口选项 + action 条件 + hook 调用点),包括本 PR 实现的回归测试设计。审阅测试计划
如何验证
qwen serve开 Web Shell 活跃会话,短暂中断与 daemon 的连接:本 PR 之前每次自动刷新弹错误 toast;之后无 toast、面板保留上次 artifacts、恢复连接后静默恢复刷新;证据(Before & After)
回归测试按 issue 中建议的设计实现,且都在未修复源码上失败(git stash 验证):silent 失败不派发 notice 仍拒绝;默认路径保留 notice;hook 每次刷新都传 silent、失败落本地 error、旧 artifacts 可见、loading 归位。
测试平台
macOS 已本地验证(✅);Windows / Linux 依赖 CI(⚠️ )。
环境
macOS(Darwin 24.6)、Node v22.23.1;vitest(hook 用 jsdom);typecheck / eslint / prettier 全绿。
风险与范围
getStats共享同模式但仅由显式/stats命令调用,按维护者说明保留 notice;无其它 action 变更。关联 Issue
Fixes #7427
🤖 Generated with Claude Code