fix(web-shell): suppress stale pending prompt refresh errors - #6352
Conversation
|
Thanks for the PR! Template looks good ✓ Problem: The Direction: Aligned. Web-shell queued-prompt UX is an active area (CHANGELOG #6025). Fixing error-reporting semantics so that benign races don't surface misleading messages is a reasonable incremental improvement. Approach: The discriminated union ( Two call-site changes with asymmetric conditions — Moving on to code review. 🔍 中文说明感谢贡献! 模板完整 ✓ 问题: 方向: 对齐。Web-shell 排队 prompt UX 是活跃开发领域(CHANGELOG #6025)。修复错误报告语义,避免正常竞态显示误导性信息,是合理的增量改进。 方案: 用可辨识联合类型( 两处调用点使用了不对称的条件——成功路径用 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Code ReviewIndependent proposal: I'd replace the boolean return with a discriminated union, map each early-exit to a semantically correct value, and update the two callers to check only for The PR matches this approach exactly. The four return sites in
Both call sites in No correctness bugs, security issues, or convention violations found. One observation: the PR body claims "insert, edit, and delete" but the diff only touches Build & TestsReal-Scenario TestingThis PR changes web-shell error-reporting semantics for a race condition that requires a browser client and specific async timing (rapid queue mutations during an active turn). The race condition cannot be driven from tmux — it needs a browser connected to a running daemon with concurrent turn activity. Web-shell dev server smoke test (PR code): Server starts cleanly and serves the app without errors. The behavior change (suppressing misleading error messages on superseded refreshes) is internal to the hook's error-reporting logic and only manifests under the specific race condition timing. 中文说明代码审查独立方案:将布尔返回值替换为可辨识联合类型,将每个提前退出映射到语义正确的值,成功路径只检查 PR 完全匹配此方案。
无正确性 bug、安全问题或规范违反。一个观察:PR 描述声称"插入、编辑、删除",但 diff 只改了 构建与测试真实场景测试此 PR 修改了 web-shell 的错误报告语义,针对的是需要浏览器客户端和特定时序(活跃 turn 期间快速队列变更)的竞态条件。该竞态无法通过 tmux 驱动——需要连接到运行中 daemon 的浏览器。 Web-shell 开发服务器冒烟测试显示服务器正常启动,无错误。行为变更仅在特定竞态时序下才会触发。 — Qwen Code · qwen3.7-max |
|
This is a small, focused fix that solves a real code-level problem: Build is green, all 1016 tests pass, web-shell serves without errors. The specific race condition (superseded refresh during rapid queue mutations) can't be driven from tmux, but the code change is narrow enough that the risk is low: only One honest reservation: the PR body mentions "insert, edit, and delete" but the diff only touches edit/delete via The lack of a reproduction is noted but not blocking. The boolean conflation is a verifiable code smell, and the fix makes the code strictly more correct regardless of whether a user has hit the specific timing window. Approving. ✅ 中文说明这是一个小而集中的修复,解决了一个真实的代码层面问题: 构建通过,1016 个测试全部通过,web-shell 无错误运行。特定的竞态条件(快速队列变更期间被覆盖的刷新)无法通过 tmux 驱动,但代码变更范围足够窄,风险很低:仅影响 一个诚实的保留意见:PR 描述提到"插入、编辑、删除",但 diff 只通过 缺乏复现已记录但不构成阻塞。布尔值混淆是可验证的代码异味,无论用户是否真正触发了特定的时序窗口,修复都使代码更加正确。 批准。✅ — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
wenshao
left a comment
There was a problem hiding this comment.
No blockers found. Suggestion-level recommendations are in the Suggestion summary comment below.
— qwen3.7-max via Qwen Code /review
Suggestions — commit
|
| File | Issue | Suggested fix |
|---|---|---|
useQueuedPrompts.ts:606,623 |
Asymmetric predicates (=== 'failed' vs !== 'refreshed') lack comments explaining why the two call sites use different tolerance levels. Future maintainer risk of "normalizing" them. |
Add 1-2 line comments at each call site explaining the optimistic vs conservative rationale. |
history-store.ts:143-196 |
No logging in the recovery path. If a corrupted history file loses entries during compaction, it would be invisible in logs. | Add log.debug when recovery produces 0 entries from a non-empty line, or when a glued line is split into multiple entries. |
history-store.ts:143 |
recoverHistoryObjectsFromLine silently drops top-level JSON arrays — the assumption "entries are always plain objects" is implicit and undocumented. |
Add a one-line comment at the top of the function: // Assumes entries are always plain JSON objects, not arrays or primitives. |
history-store.test.ts |
Missing test for unterminated string input (e.g., crash mid-serialization leaves {"id":"a1","data":"unterminated). The function handles this correctly but the edge case deserves coverage. |
Add a test case with a line containing an unterminated string, asserting the complete objects before it are preserved and the incomplete one is dropped. |
useQueuedPrompts.ts:55-60 |
RefreshPendingPromptsResult type has no JSDoc explaining when each variant is returned or how callers should react. |
Add a JSDoc block documenting each variant's semantics and the expected caller behavior. |
— qwen3.7-max via Qwen Code /review
What this PR does
This PR makes pending prompt refresh results explicit so the web shell can distinguish a real refresh failure from a refresh that was skipped or superseded by a newer request. Queue insert, edit, and delete flows now only show the pending prompt refresh error when the refresh request itself fails.
Why it's needed
When a queued prompt is inserted, edited, or removed, the web shell removes the server-side pending prompt and then refreshes the pending prompt list. The refresh helper previously returned
falsefor several different outcomes, including normal races where a later refresh superseded the current one. Those benign races were treated as user-visible failures, which could show misleading messages such as “Queue changed but pending prompts could not refresh” even though the queue mutation had already succeeded.Reviewer Test Plan
How to verify
Review the queue insert/edit/delete path and confirm that a superseded or skipped pending prompt refresh no longer reports a user-visible error after
removePendingPromptsucceeds. A reviewer can also trigger rapid queued prompt changes while a turn is active and confirm that stale refreshes do not surface the “pending prompts could not refresh” error unless the pending prompt fetch actually fails.Evidence (Before & After)
Before: a stale pending prompt refresh could return the same failure value as an actual request error, causing a misleading user-visible queue error after a successful queue mutation.
After: refresh results distinguish
refreshed,skipped,superseded, andfailed; onlyfailedreports the pending prompt refresh error after a successful queue mutation.Tested on
Environment (optional)
Local validation:
npx prettier --check packages/web-shell/client/hooks/useQueuedPrompts.ts;npm run build --workspace=packages/webui;npm run build --workspace=packages/sdk-typescript;npm run build --workspace=packages/web-shell.Risk & Scope
Linked Issues
N/A
中文说明
What this PR does
这个 PR 让 pending prompt 的刷新结果变得更明确,因此 web shell 可以区分真正的刷新失败,以及刷新被跳过或被更新请求覆盖的正常情况。队列插入、编辑、删除流程现在只会在刷新请求本身失败时显示 pending prompt 刷新错误。
Why it's needed
当用户插入、编辑或移除一个排队 prompt 时,web shell 会先移除服务端的 pending prompt,然后刷新 pending prompt 列表。之前刷新辅助函数对多种结果都返回
false,其中包括后续刷新覆盖当前刷新这种正常竞态。这类正常竞态会被当成用户可见失败,导致即使队列变更已经成功,也可能显示 “Queue changed but pending prompts could not refresh” 这类误导提示。Reviewer Test Plan
How to verify
检查队列插入、编辑、删除路径,确认
removePendingPrompt成功后,如果 pending prompt 刷新被覆盖或跳过,不再报告用户可见错误。审核者也可以在一个 turn 正在运行时快速变更多个队列 prompt,确认过期刷新不会弹出 “pending prompts could not refresh” 错误,除非 pending prompt 获取请求本身确实失败。Evidence (Before & After)
Before:过期的 pending prompt 刷新会返回和真实请求错误相同的失败值,导致成功变更队列后仍可能出现误导性的用户可见队列错误。
After:刷新结果区分为
refreshed、skipped、superseded和failed;成功变更队列后,只有failed会报告 pending prompt 刷新错误。Tested on
Environment (optional)
本地验证:
npx prettier --check packages/web-shell/client/hooks/useQueuedPrompts.ts;npm run build --workspace=packages/webui;npm run build --workspace=packages/sdk-typescript;npm run build --workspace=packages/web-shell。Risk & Scope
Linked Issues
N/A