fix(web-shell): use backend-authoritative queue state - #9407
Merged
Conversation
Collaborator
|
✅ Qwen Triage finished — CI landed green on ✅ Qwen Triage 已完成 —— |
Collaborator
|
Thanks for the PR — this cleans up a genuinely confusing state machine and the scope is tight.
Moving on to code review. 🔍 中文说明感谢贡献！这个 PR 把一个确实令人困惑的状态机清理得很干净，范围也很收敛。
进入代码审查。🔍 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
Code reviewNo critical blockers found. The central mechanism is exactly the minimal primitive this needed: The race handling reads correctly too: a stale reconciliation whose snapshot is unavailable returns without dropping when a newer authoritative row already owns the state, every settle path is guarded by owner/session/workspace tokens, and the promoted-media retention keeps hidden payloads recoverable when the pending-prompt refresh fails so a later start event can still salvage images. Every Two non-blocking notes worth knowing: (1) the residual tradeoff — when a post-dispatch transport failure coincides with an unavailable reconciliation, the row is dropped without restoring the draft; that is the documented fail-over-duplicate choice, and the item resurfaces on reconnect if the daemon actually accepted it; (2) a rare edge where clearing the queue while an admission is still in flight can surface a "queue failed" toast for the item the user just cleared. Neither blocks merge. sequenceDiagram
participant P1 as Composer
participant P2 as useQueuedPrompts
participant P3 as daemon session actions
participant P4 as Daemon
P1->>P2: enqueue prompt with media
P2->>P3: upload media, then dispatch admission (fires onAdmissionStarted)
alt failure before dispatch
P3-->>P2: error, admission never started
P2-->>P1: drop local row, restore full draft, refresh from backend
else dispatched, then settles
P3->>P4: admission request
P4-->>P3: accepted, rejected, or lost response
P3-->>P2: settle outcome
P2-->>P1: drop local row, render backend snapshot, restore nothing
end
Files changed (16 of 16 shown)
Testing evidence — PR's own CI (unattended run, PR code not executed)At review time the main unit suite is still running on the reviewed commit; the Desktop Shell builds and the safety scans already passed. Windows/macOS unit and integration checks are skipped on this PR's pipeline lane (gated), so the Linux suite is the load-bearing CI signal here. The finalize workflow will update the table below once CI settles. Not verified: real-daemon end-to-end behavior (see the sandboxed-lane note below). Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 Sandboxed verification would settle the remaining gap: 中文说明代码审查未发现阻塞性问题。核心机制恰好是这个改动所需的最小原语： 竞态处理读起来也是正确的：当更新的权威队列项已拥有该状态时，拿不到快照的过期刷新会直接返回而不删除；每条接纳结束路径都有 owner/session/workspace 令牌保护；promoted 媒体的保留逻辑确保 pending 刷新失败时隐藏载荷仍可被后续 start 事件救回。全树所有 两个非阻塞的注意点：(1) 残留的权衡——发送后传输失败且刷新也不可用时，本地项会被丢弃且不恢复草稿；这是文档中明确选择的"宁可丢失、不可重复"策略，若守护进程实际已接收，重连后该项会重新出现；(2) 罕见边界——接纳仍在进行中时清空队列，仍可能为刚被清掉的项弹出"队列失败"提示。两者均不阻塞合并。 时序图展示了新的接纳结束流程：发送前失败→丢弃本地项、恢复完整草稿、刷新后端；已发送→丢弃本地项、渲染后端快照、不恢复任何内容。文件表见英文部分。 测试证据无人值守 CI 运行，未执行 PR 代码。审查时主单元测试仍在运行；Desktop Shell 构建与安全扫描已通过。Windows/macOS 单测与集成测试在此 PR 的流水线通道中被跳过（受门控），因此 Linux 套件是关键的 CI 信号。CI 稳定后 finalize 工作流会更新表格。未验证：真实守护进程的端到端行为（见上方沙箱验证说明）——作者的手动测试仅覆盖 macOS；如需补上这一缺口，维护者可触发 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
|
Confidence: 4/5 — solid, coherent change that removes a confusing state machine instead of patching it; only non-blocking reservations remain (CI still running, manual verification was macOS-only). Stepping back: this PR matches what I would have proposed independently — converge the queue display to the backend once every admission attempt settles, and restore drafts only when non-delivery is provable — and it goes further in two places I'd have gotten wrong. First, it reuses the existing Is it needed? Yes — duplicate queue rows and a "local copy discarded" state that could appear next to a live backend row are the kind of defect users notice immediately, and the before/after semantics are concrete rather than theoretical. Every edit in the diff serves the stated goal; the direct-submit banner intentionally keeps its old behavior and its i18n keys, which is a clean scope line, not leftover. Reservations, named plainly: the fail-over-duplicate tradeoff means a genuinely lost message (post-dispatch transport failure plus unavailable reconciliation) is not restored to the editor — documented and defensible, but it is a behavior users should know about; and the author's manual evidence covers macOS only. The Stage 2 comment names Verdict: approve once CI lands. The main unit suite is still in flight on the reviewed commit, so approval is deferred until CI lands green on 中文说明回顾整体：这个 PR 与我独立设想的方案一致——每次接纳尝试结束后让队列展示收敛到后端、仅在可证明未送达时恢复草稿——并且在两处比我的设想更好。其一，复用了既有的 是否必要？是——重复的队列项、与后端活跃队列项并存却提示"本地副本已丢弃"的状态，都是用户立刻能察觉的缺陷，前后语义具体而非理论假设。diff 中每一处改动都服务于既定目标；直接提交的横幅有意保留旧行为及其 i18n key，这是清晰的范围边界，不是遗漏。 保留意见（直说）：宁可丢失不可重复的权衡意味着真正丢失的消息（发送后传输失败且刷新不可用）不会恢复到编辑器——文档已说明且合理，但这是用户应当知晓的行为变化；作者的手动验证仅覆盖 macOS。Stage 2 评论已指明可用 结论：CI 通过后批准。主单元测试在受审提交上仍在运行，因此批准延迟到 CI 在该提交上全部变绿后执行。 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
🖼️ web-shell visual previewRendered against a mock daemon (no real backend): the PR base vs this PR head Screenshots · before / afterFull-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
approved these changes
Aug 18, 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 — CI landed green after the review. ✅
chiga0
approved these changes
Aug 18, 2026
chiga0
left a comment
chiga0
left a comment
Collaborator
There was a problem hiding this comment.
Code Review Overview (AI Generated)
PR: #9407 fix(web-shell): use backend-authoritative queue state
Type: Bug Fix + Refactor
Change size: +594/-569 across 16 files
HEAD SHA: f9747c0
Findings Summary
- Critical/Major: 0
- Minor: 0
- Nit: 3
Key Observations
This is a well-engineered simplification. Removing admissionOutcome: unknown and payloadAvailable eliminates a footgun that created duplicate queue rows and misleading restore/discard UI. The new design is cleaner: every admission attempt ends with either a backend refresh or a deletion, never with a persistent local-only row. The onAdmissionStarted callback correctly places the dispatch boundary - fires synchronously after media upload but before the daemon call - so the pre/post-dispatch distinction is accurate at every call site.
Test coverage is thorough: workspace-switch races, stale reconcile ordering, no-session edge cases, and the keeps a row restored by a newer reconcile test all exercise the subtlest correctness properties.
Verified: ChatPane.tsx independent unknownPromptAdmission system (for direct idle-path submits) is intentionally out of scope and untouched. Its i18n keys (queue.admissionUnknown, queue.continueEditing, queue.discardUnknown, queue.continueEditingConfirm) are still live.
Nits (no action required)
-
Dead i18n key queue.restoreUnknown - the Restore local copy queue-row button was removed; the ChatPane direct-submit banner uses queue.continueEditing instead. Safe to delete in a follow-up.
-
void removeUploadedMedia() in mid-turn rejected/catch paths - changed from await. Intentional (dont block reconcile), but a rejection from removeMedia will become a silent unhandled promise rejection rather than surfacing in error tracking. Low operational risk but worth noting.
-
enqueueStarted = false; throw in the .then(accepted: false) handler - resetting enqueueStarted before throwing converts the no-session accepted: false case into the catch-paths !enqueueStarted branch. It works correctly, but a one-liner comment (e.g. treat no-session rejection as pre-dispatch so the catch path restores the draft) would save future readers the trace.
Cross-Validation
| Finding | CI Bot | My Assessment |
|---|---|---|
| CI green, no issues | LGTM | Confirmed - no correctness bugs found |
| Unique-N1: dead queue.restoreUnknown key | - | Nit only |
| Unique-N2: void removeUploadedMedia | - | Nit only |
| Unique-N3: enqueueStarted reset comment | - | Nit only |
Additional Audit Coverage
- onAdmissionStarted timing: verified it fires synchronously after media upload, before session.submitPrompt / session.enqueueMidTurnMessage - the dispatch boundary is correct at both call sites in actions.ts.
- ChatPane.unknownPromptAdmission: confirmed this separate state-machine is unaffected; its live i18n keys are not dead.
- State machine correctness for midTurnState submitting: the new row state participates in applyMidTurnSnapshot filter (previously admissionOutcome unknown did the same work via the OR branch) - behaviour is equivalent.
- clearQueuedPrompts blast radius: submitting prompts are now simply dropped; completionCallbacks and submitAbortControllers cleanup is still correct in the existing code paths.
- isDefinitelyRejectedPromptAdmission removal from useQueuedPrompts: import is cleanly unused post-PR; ChatPane.tsx still imports it independently for the direct-submit path.
Final Verdict
Approve - no correctness issues found. Three nits are all cosmetic/cleanup and can be addressed in follow-ups.
This review was generated by QoderWork AI
ytahdn
commented
Aug 18, 2026
ytahdn
left a comment
ytahdn
left a comment
Collaborator
Author
There was a problem hiding this comment.
审查总结 / Review Summary
变更概览 / Overview: 16 files, ~2100-line diff. Removes the entire client-side "admission outcome unknown" state machine in favor of a daemon-authoritative queue display: pre-dispatch failure → draft restored to the editor; post-dispatch → reconciled against daemon state. Introduces onAdmissionStarted to define the dispatch boundary, reconcile sequence numbers to discard stale overlaps, and snapshot media salvage.
🎉 做得好 / Highlights
- 派发边界精准 / Precise dispatch boundary: Verified in
actions.ts—onAdmissionStartedfires after media upload / local validation and immediately before daemon dispatch in bothsubmitPromptandenqueueMidTurnMessage; the no-session short-circuit ({ accepted: false }) correctly does not fire it. The "undispatched ⇒ safe to restore draft" invariant holds on both paths. - 删除彻底自洽 / Complete, self-consistent removal:
admissionOutcome/payloadAvailableremoved from types, hooks,QueuedPromptDisplay,midTurnDedup, i18n and CSS in lockstep. A full-tree grep at head (f9747c0) confirmspromptAdmission.tsand the retained i18n keys are still used by the direct-submit path inApp.tsx/ChatPane.tsx— not dead code; queue vs direct-submit responsibilities stay cleanly separated. - 竞态处理考究 / Careful race handling:
applyMidTurnSnapshotsalvages images before deleting pending admissions (salvagedImages/promotedImages), and retains the hidden payload whenapplyPromotedis false so a later start event can recover media.midTurnReconcileSeqRefcorrectly discards stale overlapping reconciles; resettingenqueueStarted = falsebefore throwing on!result.accepted && !enqueueDispatchedroutes the catch through the "never started" branch so the draft is restored. - 文档与测试 / Docs & tests: Design doc included; three new/extended test files (192 targeted tests per the PR description) match the risk surface of the change.
🟢 小问题 / Nits (non-blocking)
useQueuedPrompts.ts— theknowncomputation chains three=== truecomparisons (snapshot?.messages.some(...) === true || ...). Understandable for collapsingboolean | undefined, but an earlyif (snapshot)narrowing would read lighter.- Several error paths now use fire-and-forget
void removeUploadedMedia()instead ofawait. The right call — a cleanup failure shouldn't mask the original error — but a failed delete leaves the uploaded media reference on the daemon; worth confirming the daemon GCs unreferenced media. - The normal-queue
.finallyrefresh guard checks onlyownerToken + sessionId, while the mid-turntargetIsCurrent()also checksworkspaceCwd. Almost certainly harmless (workspace switches usually change the session too), but the asymmetric guards could use a one-line comment if intentional.
结论 / Verdict
✅ 批准 / Approve — a clean, well-executed refactor: replacing the ambiguity state machine with "daemon-authoritative + dispatch boundary" significantly reduces complexity; all five failure scenarios (no session / upload failure / post-dispatch rejection / transport failure / session switch) hold up to scrutiny, and the cleanup leaves no loose ends. The three nits above are polish, not blockers.
qqqys
added a commit
to qqqys/qwen-code
that referenced
this pull request
Aug 19, 2026
Resolve the queue conflicts against QwenLM#9407 (backend-authoritative queue state). The unknown-admission row state that Goal v3 built on is gone upstream, so the explicit Insert path now lets the daemon's mid-turn snapshot decide: a confirmed message becomes a daemon-owned mid-turn row, a settled or promoted one leaves the local queue, and an insert the daemon cannot confirm returns to the local Goal hold instead of lingering as an "unknown" row. Local Goal-held rows still survive an owner change; daemon-owned rows are re-rendered from the next snapshot. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Collaborator
|
Released in v0.21.14. |
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
Why it's needed
The client previously retained optimistic queue rows when delivery was uncertain, so one message could appear both as a local copy and as a server queue item, and users could see misleading “local copy discarded” states. At the same time, failures that happened before any request reached the daemon could discard recoverable draft content. The queue should converge to the backend after every settled admission attempt while preserving drafts only when non-delivery is known.
Reviewer Test Plan
How to verify
@attachment through the ordinary queue path and force media upload to fail before prompt dispatch. Expect the local queue row to disappear, a backend refresh to run, and the complete text, media, and annotations to return to the editor.Evidence (Before & After)
Tested on
Environment (optional)
Node.js 22+, local workspace, no sandbox.
Risk & Scope
Linked Issues
N/A
中文说明
本 PR 做了什么
为什么需要
此前客户端会在消息是否送达不确定时保留乐观队列项，导致同一条消息可能同时显示为本地副本和服务端队列项，并出现误导性的“本地副本已丢弃”状态。另一方面，在请求尚未到达守护进程前发生的失败，也可能丢失本可恢复的草稿内容。新的行为是在每次接纳流程结束后统一以后端状态收敛，同时仅在确认消息未送达时恢复草稿。
Reviewer 测试计划
如何验证
@附件的消息，并让媒体上传在请求发送前失败。预期本地队列项消失、重新拉取后端状态，并将完整文本、媒体和标注恢复到编辑器。前后对比证据
测试平台
环境（可选）
Node.js 22+，本地工作区，无沙箱。
风险与范围
关联 Issue
无。