fix(webui): revert #8882's transactional session switching to the loading-skeleton model - #9129
Merged
Merged
Conversation
…ding-skeleton model The transactional cross-session switching from #8882 staged a handoff and kept the old attachment live until the target load committed. It added a large transition state machine (intent staging, same-session capture, watchdog deadlines, controlled rebind) across the daemon session layer and the web-shell provider, and left the UI pinned to the previous session while a switch prepared. Restore the loading-skeleton model: switching a session clears the transcript, shows the loading skeleton, and waits for the load result. - Remove sessionTransition state, onSessionTransitionCommit and the transactional target logic from WorkspaceSessionProvider. - Strip the transition state machine from DaemonSessionProvider and restore single-session restores: restore_in_progress retries stay bounded by the existing watchdog, and the skeleton UI keys on loadingTranscript. - Move useDaemonSessionOwnerGuard back under the daemon index export. - Delete the transactional design docs and both daemon integration tests; the restored behavior is covered by unit tests. - Drop the dead desiredSessionTargetPending prop (write gating now keys on loadingTranscript alone) and stop a failed switch's target workspace from leaking into the next workspace-less load. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Collaborator
ytahdn
added
the
autofix/takeover
Collaborator
|
🤝 Takeover engaged: the autofix loop now manages this PR — it will address new review feedback and resolve base conflicts until the label is removed or the round cap is reached. Remove the 中文说明🤝 已接管：autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突，直到移除标签或达到轮次上限。移除 |
Collaborator
🖼️ web-shell visual previewRendered against a mock daemon (no real backend): the PR base vs this PR head Screenshots · before / after
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. |
Collaborator
|
Confidence: 3/5 — a clean, faithful revert capped by policy: cross-package scale needs maintainer awareness, and whether to give up the transactional guarantees this soon is a product call, not a gate call. Stepping back: my independent proposal for this problem was exactly what this PR does — restore clear-and-wait, keep the prompt-admission owner guard, delete the design docs and integration tests that described the old model — and the implementation matches it carefully. I verified the result against the pre-#8882 state: nothing transactional survives, nothing dangles, and the keepers (owner guard, bounded restore_in_progress retry, client_identity compat) are each deliberate and tested. If this repo had no history attached to the code being removed, I'd approve. What gives me pause is not the code but the context. The machinery being deleted was merged three days ago, was built against real issues, and one of those issues — #8678, preserving the session when a large restore times out — is still open and becomes a destructive failure mode again under the restored model (failed switch → empty/error target, previous session detached). The PR is honest about that tradeoff, which is the right way to propose it, but honesty in the description doesn't settle whether the tradeoff is accepted. That belongs to a maintainer, and the original series' author should be in the loop on the revert of their merged work. ⏸️ Deferring to @doudouOUC (author of #8882/#8939/#8990 and a core code owner) — needs a human call on two things: (1) whether the simplicity win justifies dropping the #8678-preserving behavior while that issue is still open, and (2) the scope naming — this reverts parts of three PRs but only cites #8882. No blocking findings on the code itself; if a maintainer accepts the tradeoff, this is ready once CI lands green. 中文说明置信度：3/5——一次干净且忠实的 revert，但受策略上限约束：跨包规模需要维护者知会，而是否这么早就放弃事务式保证是产品决策，不是门禁能替项目做的决定。 退一步看：我对这个问题的独立方案与本 PR 完全一致——恢复"清空并等待"、保留提示准入 owner guard、删除描述旧模型的设计文档与集成测试——实现也细致地符合该方案。我把结果与 #8882 之前的状态核对过：事务机制无残留、无悬空引用，保留项（owner guard、有界的 restore_in_progress 重试、client_identity 兼容）都是有意为之且有测试。如果这段代码没有附带历史，我会直接批准。 让我犹豫的不是代码而是背景：被删除的机制三天前才合入，是针对真实 issue 构建的，而其中 #8678（大会话恢复超时时保留当前会话）仍然 open，在恢复后的模型下会重新变成破坏性失败场景（切换失败 → 空白/出错的目标页，原会话被断开）。PR 对这个取舍是坦诚的，这是提出它的正确方式，但描述坦诚不等于取舍已被接受。这应由维护者决定，被回退系列的作者也应知情。 ⏸️ 转交 @doudouOUC（#8882/#8939/#8990 的作者、核心 code owner）——需要人工判断两点：(1) 在 #8678 仍然 open 的情况下，简化收益是否足以放弃对它的保护行为；(2) 范围命名——本 PR 回退了三个 PR 的部分内容但只引用了 #8882。代码本身无阻塞性发现；若维护者接受该取舍，CI 全绿后即可合入。 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
|
Thanks for the unusually thorough revert write-up.
Code review is clean — details in the next comment. Holding approval for a maintainer call on the tradeoff. 中文说明感谢这份异常详尽的 revert 说明。
代码审查是干净的——细节见下一条评论。批准与否留给维护者对这个取舍的判断。 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
Code reviewIndependent baseline first: for "restore the loading-skeleton model" I would have reverted the #8882/#8939/#8990 merges while keeping the prompt-admission owner guard that closed #8923, then grepped the repo for dangling references to the removed machinery. This PR lands on the same end state — a clean To verify fidelity I diffed the resulting
Files changed (18)
CI evidence (this commit's own checks, fetched via API — PR code is never executed here)No red checks on this commit. The Linux unit suite — the one that runs the webui and web-shell unit tests the author cites — passed, which corroborates (though does not independently recount) the reported 271 webui session tests and 443 web-shell tests. The web-shell E2E smoke was still running at post time; the macOS/Windows unit jobs were skipped by this PR's CI matrix, so the author's macOS-only manual testing (their claim, not independently re-run) is the only signal for those platforms. The finalize job updates the table below once CI settles. 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: 中文说明代码审查：先写独立方案——我会回退三个事务式 PR 但保留关闭 #8923 的 owner guard，然后全仓搜索残留引用。本 PR 的终态与此一致（#8955 之后重写过相同文件，干净的 git revert 不可行，手工恢复是正确选择）。我把结果文件与 #8882 之前的版本做了 diff：骨架模型之外保留的恰好是有意保留的集合——owner guard、有界的 restore_in_progress 重试、client_identity 兼容路径，以及 8 月 11–14 日间合入的无关特性。事务机制无残留。 要点：删除的符号（sessionTransition、onSessionTransitionCommit、desiredSessionTargetPending 等）在仓库中已无任何悬空引用，desktop 等其他包也不消费它们；owner guard 仍从相同公共入口导出。两个附带修复（失败目标保持可见、失败切换的工作区不再泄漏给下一次无工作区切换）均有新单测钉住，包括被取代加载的保护分支。写闸门改为仅由 loadingTranscript 驱动。覆盖取舍（非阻塞，留给维护者）：约 1,049 行真实 daemon 集成测试被删除，替代的是 mock SDK 的单测。 CI 证据：该提交无红色检查。Linux 单测（运行作者提到的 webui 与 web-shell 单测的作业）已通过，佐证了其报告的 271 个 webui 会话测试与 443 个 web-shell 测试（本运行不独立复核具体数字）。发布时 web-shell E2E smoke 仍在运行；macOS/Windows 单测作业被本 PR 的 CI 矩阵跳过，作者仅在 macOS 手动验证（为其自述，未在此独立复核）。表格由 finalize 作业在 CI 落定后就地更新。 沙箱验证可补齐剩余缺口： — Qwen Code · qwen3.8-max Reviewed at |
ytahdn
commented
Aug 14, 2026
ytahdn
left a comment
ytahdn
left a comment
Collaborator
Author
There was a problem hiding this comment.
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) and Integration Tests (CLI, No Sandbox) were skipped in CI and their suites did not run locally (review ran without build/test per reviewer request).
Not explored to full depth (tool budget reached): "You are review agent 1c — Cross-file tracer for PR…": none material — the full diff was read, and every brief item was grepped against the post-change worktree..
Not reviewed: the entire diff, the linked-issue fidelity pass, the whole-diff test-coverage check, the build-and-test check, the invariant check (state, timers, collections) on packages/web-shell/client/components/WorkspaceSessionProvider.tsx, the invariant check (counters, return values, error taxonomies) on packages/web-shell/client/components/WorkspaceSessionProvider.tsx, the invariant check (config fields, early returns) on packages/web-shell/client/components/WorkspaceSessionProvider.tsx, the invariant check (state, timers, collections) on packages/webui/src/daemon/session/DaemonSessionProvider.tsx, the invariant check (counters, return values, error taxonomies) on packages/webui/src/daemon/session/DaemonSessionProvider.tsx, the invariant check (config fields, early returns) on packages/webui/src/daemon/session/DaemonSessionProvider.tsx — its prompt was built, but no agent on record was launched with it.
Not reviewed: reverse audit — no auditor was launched with a prompt this skill builds — the pass that hunts what the rest of the review missed ran, if at all, without the method its brief carries.
Not reviewed: verification — a verifier ran and opened its brief, but no agent was launched with the prompt the CLI built — the launch was written by hand, and the posted findings cannot be counted as verified against it.
中文说明
未审查：build-and-test — Test (macos-latest, Node 22.x) and Integration Tests (CLI, No Sandbox) were skipped in CI and their suites did not run locally (review ran without build/test per reviewer request)。
未探索到全部深度（达到工具调用预算）："You are review agent 1c — Cross-file tracer for PR…"：none material — the full diff was read, and every brief item was grepped against the post-change worktree.。
未审查：整个 diff、关联 issue 一致性检查、全 diff 测试覆盖检查、构建与测试验证、不变量检查（状态、定时器、集合）（packages/web-shell/client/components/WorkspaceSessionProvider.tsx）、不变量检查（计数器、返回值、错误分类）（packages/web-shell/client/components/WorkspaceSessionProvider.tsx）、不变量检查（配置字段、提前返回）（packages/web-shell/client/components/WorkspaceSessionProvider.tsx）、不变量检查（状态、定时器、集合）（packages/webui/src/daemon/session/DaemonSessionProvider.tsx）、不变量检查（计数器、返回值、错误分类）（packages/webui/src/daemon/session/DaemonSessionProvider.tsx）、不变量检查（配置字段、提前返回）（packages/webui/src/daemon/session/DaemonSessionProvider.tsx）——它的 prompt 已构建，但没有任何 agent 有记录用它启动过。
未审查：反向审计——没有审计 agent 是用本 skill 构建的 prompt 启动的——负责搜寻评审其余部分遗漏问题的这道工序，即便运行过，也缺失了 brief 承载的方法。
未审查：验证——有验证 agent 运行并打开了自己的 brief，但没有 agent 是用 CLI 构建的 prompt 启动的——启动 prompt 是手写的，发布的发现不能算作经它验证。
— DeepSeek/deepseek-v4-flash via Qwen Code /review (v0.21.11)
| @@ -3089,6 +2586,20 @@ export function DaemonSessionProvider(props: DaemonSessionProviderProps) { | |||
| reconnectAttempt = 0; | |||
| skipMetadataRefresh = true; | |||
| continue; | |||
| } else if (isRestoreInProgressLoadError(error)) { | |||
Collaborator
Author
There was a problem hiding this comment.
[Suggestion] The terminal restore_in_progress branch is untested — Failure scenario: a non-retryable 409 (or reason: 'awaiting_abandoned_cleanup', or autoReconnect off) lands on a switch and the terminal error rendering regresses undetected; grep shows only the two retryable cases are covered, and the 5000 ms default fallback in getRestoreInProgressRetryDelayMs plus the awaiting_abandoned_cleanup skip have no test.
中文说明
`restore_in_progress` 的终局分支（不可重试的 409 / `awaiting_abandoned_cleanup` / 缺少 `retryAfterSeconds` 时的 5000ms 默认值）没有测试覆盖：套件中仅有的两条 `restore_in_progress` 用例都是可重试且有延迟的。建议补充一条不可重试 409 的用例，验证终局错误态（`status: 'error'` 且不再重试）。— DeepSeek/deepseek-v4-flash via Qwen Code /review (v0.21.11)
Comment on lines
+11770
to
11773
| mockConnection.loadingTranscript = true; | ||
| rerender({ onSubmitBefore }); | ||
| }); | ||
| const allowBPrompt = vi.fn().mockResolvedValue(undefined); |
Collaborator
Author
There was a problem hiding this comment.
[Suggestion] Converted tests no longer verify write-blocking during a session load — Concrete cost: after the desiredSessionTargetPending prop was deleted, several tests toggle mockConnection.loadingTranscript with no assertion inside the loading window, so the toggles are inert scaffolding. Probe-verified: removing the toggle pair from this test leaves it green, meaning a regression in the write gate (Boolean(connection.loadingTranscript) in App) would not be caught here. Either assert inside the loading window (e.g. a submit is rejected) or drop the inert toggles.
中文说明
删除 `desiredSessionTargetPending` 后，多个转换后的测试只在相邻的 `act` 中切换 `mockConnection.loadingTranscript`，加载窗口内没有任何断言，属于无效脚手架。Probe 验证：删除本用例的 toggle 对后测试仍然通过，写闸门（App 中 `Boolean(connection.loadingTranscript)`）的回归不会被这里捕获。建议在加载窗口内补一条断言（如提交被拒绝），或删除无效的 toggle。— DeepSeek/deepseek-v4-flash via Qwen Code /review (v0.21.11)
Collaborator
Author
|
@qwen-code /takeover |
Collaborator
|
🔄 Takeover re-armed: the round counter starts a fresh window (previous rounds no longer count toward the cap); management continues. 中文说明🔄 已重新武装：轮次计数开启新窗口（此前轮次不再计入上限），托管继续。 |
wenshao
approved these changes
Aug 14, 2026
This was referenced Aug 14, 2026
Collaborator
|
Released in v0.21.12. |
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
5 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
This PR reverts the transactional cross-session switching introduced in #8882 and restores the loading-skeleton model for session switches: switching a session clears the current transcript, shows the loading skeleton, and waits for the load result. The transactional machinery — held transitions, staged handoffs, same-session event capture, watchdog deadlines and the controlled rebind path — is removed across the daemon session layer and the web-shell provider, along with the design docs and daemon integration tests that described it. Two follow-up fixes ride along: the dead write-gating prop left by the removal is dropped, and a failed switch no longer leaks its target workspace into the next workspace-less load.
Why it's needed
The transactional model kept the old attachment live and the UI pinned to the previous session while a switch prepared, trading a large state machine for behavior that was hard to reason about and to verify end to end. The loading-skeleton model is the simpler contract: clear the transcript, show the skeleton, wait for the load. Session restores keep their existing timeout and retry guarantees (restore_in_progress retries remain bounded by the daemon-advertised watchdog), and the restored behavior is covered by unit tests in place of the deleted integration tests.
Reviewer Test Plan
How to verify
Evidence (Before & After)
N/A — behavior-level revert; verified through unit suites: webui session tests (271) and web-shell tests (443) pass, and typecheck plus ESLint are clean on the changed packages.
Tested on
Environment (optional)
N/A — unit tests only.
Risk & Scope
Linked Issues
Reverts the transactional session switching from #8882.
中文说明
本 PR 回退 #8882 引入的事务式跨会话切换，恢复「切换会话 = 清空内容 → 展示骨架屏 → 等待加载结果」的模型。移除的内容包括跨会话切换状态机（暂存交接、同会话事件捕获、看门狗超时、受控重绑定）及其设计文档与 daemon 集成测试；同时附带两个修复：删除移除后遗留的死属性（写闸门改为仅由加载状态驱动），并阻止失败切换的目标工作区泄漏到下一次未指定工作区的加载。会话恢复的既有超时与重试保证保持不变（restore_in_progress 重试仍受 daemon 广告的看门狗约束），恢复后的行为由单元测试覆盖。
验证方式：webui 会话测试 271 个、web-shell 测试 443 个全部通过，改动包 typecheck 与 ESLint 干净（macOS 本地验证）。