Handle missing web-shell sessions without redirecting - #6357
Conversation
|
Thanks for the PR! (Re-run after extensive maintainer review cycles and multiple author pushes.) Template looks good ✓ Problem: Real UX issue — opening a web-shell URL with a stale or nonexistent session id silently redirected to the new-chat page instead of telling the user the session no longer exists. Parsing daemon error text was fragile. This is an observed problem with clear before/after described and a screenshot showing the expected behavior. Direction: Aligned. Explicit missing-session state is the right pattern. Propagating structured HTTP status codes via Size:
Title is not Approach: Scope is focused on the stated goal. The two new fields ( Moving on to code review and verification. 🔍 中文说明感谢贡献！（Re-run — 在 maintainer 多轮 review 和作者多次推送后重新审查。） 模板完整 ✓ 问题： 真实的 UX 问题——使用过期或不存在的 session id 打开 web-shell URL 时，会静默跳转到新建聊天页面，而不是告知用户会话已不存在。解析 daemon 错误文案也很脆弱。这是一个已观测到的问题，有清晰的 before/after 描述和截图。 方向： 对齐。明确的缺失会话状态是正确的模式。通过 规模： 方案： 范围聚焦。 进入代码审查和验证 🔍 — Qwen Code · qwen3.7-max |
|
Addressed the triage note by adding an inline comment explaining why the session-id-change effect intentionally preserves the missing-session route until the user clicks new chat. Verified with: |
Code Review (Re-run after maintainer review cycles)Independent proposal: I'd add an optional Comparison with the diff: The PR's approach matches this closely. Key implementation decisions:
Findings:
VerificationUnit tests (reported by author, confirmed by CI):
CI: Tmux real-scenario testing: Not applicable — this is a browser-based web shell feature that renders in a Chromium/Firefox tab. The behavior (rendering a missing-session empty state when the daemon returns 404/410) cannot be exercised via tmux in a terminal. Unit tests comprehensively cover the rendering logic and provider state transitions. Maintainer real-browser verification: @wenshao performed a full real-browser E2E run on Linux (Chromium + Vite dev server + real 中文说明代码审查（Re-run — maintainer 多轮 review 后）独立方案： 在 对比： PR 的实现与这个方案高度一致。关键设计决策：
发现： 无阻塞问题。所有状态转换正确处理新字段。App.tsx 渲染简洁明了。Status helper 通过 SDK barrel 正确共享。 验证单元测试： 180+ 个测试覆盖所有新行为。CI ✅。Build + typecheck ✅。 Tmux 真实场景测试： 不适用——这是浏览器 web shell 功能。 Maintainer 真实浏览器验证： @wenshao 在 Linux 上用真实 Chromium + Vite dev + 真实 daemon 做了完整 E2E 验证，所有 Reviewer Test Plan 断言通过。RED/GREEN 回退分析确认新测试有效。 — Qwen Code · qwen3.7-max |
|
Re-run after extensive maintainer review cycles (8+ rounds of feedback from @wenshao, all addressed by the author) and real-browser verification on Linux. This PR has been through an unusually thorough gauntlet for a community contribution. Looking at it fresh against the current state of the diff: Does the implementation match my independent proposal? Yes — and exceeds it in several areas. The Does this solve something users actually care about? Yes. Stale web-shell links silently redirecting is a real frustration — the screenshot in the PR body shows exactly what users see today. The fix gives a clear, actionable state. Is the code straightforward? Yes. The two-field design ( Is every change necessary? The PR grew through iterative review rather than being designed end-to-end upfront, which shows in a few places (e.g., the heartbeat catch block now handles auth, missing-session, and generic disconnect in one place with branching). But the resulting code is correct, well-tested, and the iterative approach is how most real-world features ship. No drive-by refactors or scope creep — every commit addresses specific review feedback. Does the evidence support the claims? Yes. CI passes on Linux. The maintainer's real-browser run on Linux with a genuine 404 daemon response confirms all assertions. RED/GREEN analysis with surgical reverts proves the tests have teeth. 180+ unit tests cover the new behavior comprehensively. Am I approving because it's genuinely good? Yes. This is a well-scoped fix for a real UX problem, with clean implementation, comprehensive tests, and real-world verification. The author has been responsive to every round of feedback. No concerns. Approving. ✅ 中文说明Re-run — 经过 maintainer 8+ 轮 review（@wenshao 的反馈，作者全部处理完毕）和 Linux 真实浏览器验证。 这个 PR 经过了异常严格的打磨。对照当前 diff 重新审视： 与独立方案对比： 一致，并在多个方面超出预期—— 是否解决真实用户问题： 是。过期 web-shell 链接静默跳转是真实的痛点。修复提供了清晰、可操作的状态。 代码是否简洁： 是。双字段设计让 UI 逻辑简单同时保留原始状态码。Heartbeat 分支可读性好。 每个改动都必要吗： PR 通过迭代 review 增长，而非预先端到端设计。但最终代码正确、测试全面。无顺手重构或范围蔓延。 证据是否支持声明： 是。CI 通过。Maintainer 真实浏览器验证通过。RED/GREEN 回退分析确认测试有效。180+ 单元测试全面覆盖。 是否真心认为好而批准： 是。这是一个范围合理的真实 UX 问题修复，实现干净，测试全面，验证充分。作者对每轮反馈都积极响应。 没有顾虑，批准。 ✅ — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
No blockers found. CI is currently failing (Post Coverage Comment, review-pr, delay-automatic-review, Test (ubuntu-latest, Node 22.x)) — recommend checking CI before merge. Suggestion-level recommendations are in the Suggestion summary comment below.
— qwen3.7-max via Qwen Code /review
Suggestions — commit
|
| File | Issue | Suggested fix |
|---|---|---|
packages/webui/src/daemon/session/DaemonSessionProvider.tsx:~1435 |
Heartbeat .catch sets errorStatus for 404/410 but preserves sessionId, so the missing-session UI in App.tsx (which requires !connection.sessionId) never triggers from heartbeat-detected terminal errors. The main SSE handler correctly clears sessionId, but there's a transient gap. |
When extractHttpStatus(error) returns a terminal status in the heartbeat handler, mirror the SSE handler: clear sessionId: undefined and preserve capabilities. |
packages/web-shell/client/App.tsx:~2284 |
handleMissingSessionNewSession uses .then()/.catch() but createNewSession has an internal try/catch that always returns boolean, so the .catch() is unreachable for createNewSession errors and can only fire if onSessionIdChange throws. |
Convert to async/await and drop the unreachable .catch(): const success = await createNewSession(); if (success) onSessionIdChange?.(undefined); |
— qwen3.7-max via Qwen Code /review
|
Addressed the latest review suggestions and conflict with main: updated the missing-session new-session path to avoid duplicate session-id notifications, mirrored the SSE missing-session behavior for heartbeat-detected 404/410 errors, added regression coverage, and merged current upstream main. Verified with: |
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
|
@qwen-code /resolve |
… inline panels Merge origin/main into the PR branch. The two conflicted areas were independent features touching the same JSX/CSS regions in App.tsx and App.module.css: - PR branch: added a missing-session empty state (inline message + new session button) that replaces the chat view when a session is not found. - Main branch: refactored Settings/Daemon Status from modal dialogs into inline panels with activePanel state, and added chatViewWrap wrapping. Resolution: both features coexist — the missing-session ternary gates the entire chat area; when absent, the main layout (inline panels, scheduled tasks page, chat view) renders as usual. Approval overlay and StatusBar handler take main's versions (ref-based overlay, openPanel for settings).
|
Qwen Code resolved the merge conflicts and pushed the branch update. Merge Conflict Resolution Summary — PR #6357Branches
Conflicted Files1.
|
| Side | What it added |
|---|---|
| PR | .missingSessionState, .missingSessionMessage, .missingSessionButton — styles for the empty-state UI shown when a session is not found |
| Main | .panelHost, .panelHeader, .panelBack, .panelTitle, .panelBody, .chatViewWrap, .chatViewHidden, .appChatEmpty .chatViewWrap — styles for the inline panel system that replaced modal dialogs |
Resolution: Combined both sets of CSS rules. They are independent — the missing-session styles style the empty state, the panel styles style the inline settings/daemon-status panels. Also removed a stray } left over from a conflict marker boundary.
2. packages/web-shell/client/App.tsx
8 conflict regions across ~4700 lines
Conflict 1 — createNewSession dependency array (line ~2416)
- PR: Added
handleMissingSessionNewSessioncallback aftercreateNewSession, which depends on[createNewSession, onSessionIdChange] - Main: Changed
createNewSessiondeps from[reportError, sessionActions]to[closeMobileDrawer, closePanel, reportError, sessionActions] - Resolution: Took main's expanded dep array for
createNewSessionAND kept the PR'shandleMissingSessionNewSessioncallback immediately after it. The callback callscreateNewSession()(which now also closes panels/mobile drawer) then resets the session ID.
Conflicts 2 + 3 + 8 — Main JSX rendering structure (lines ~4330, ~4466, ~4735)
- PR: Wrapped the chat content in a
{missingSession ? <emptyState> : <originalContent>}ternary - Main: Restructured the chat area: added
{activePanel && <section>...</section>}, wrapped chat i
🔴 Build Failure AnalysisFailing check: Root CauseThe PR wraps the main content area in a
The JSX parser (esbuild) gets confused by the mismatched nesting and misinterprets a later expression as an unterminated regular expression. FixRemove the two orphan
No extra wrapper div was added, so no extra 🇨🇳 中文分析🔴 构建失败分析失败的检查项： 根因PR 将主内容区域包裹在
JSX 解析器 (esbuild) 因嵌套结构不匹配而混乱，将后续的表达式误解析为未终止的正则表达式。 修复方法删除第 4459 行和第 4709 行的两个孤立
没有新增额外的包裹 div，因此不需要额外的 |
|
Addressed the latest review feedback:
Verification:
|
|
@qwen-code /triage |
|
Addressed the latest review threads in 9f022e3:
Verification:
|
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
— qwen3.7-max via Qwen Code /review
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
|
Follow-up fixes pushed in
Verified locally:
|
✅ Local verification (real browser + real daemon, Linux)I built and drove this branch end-to-end on Linux — the row your test matrix left as 1. Real end-to-end (Chromium + Vite dev of this branch + real daemon)Setup: the PR frontend runs through the web-shell Vite dev server built from this branch's source (so The browser console emitted this branch's own new breadcrumb, confirming the running code is this PR (neither the log line nor the copy exists on Missing-session route — the explicit empty state renders instead of a silent redirect:
After clicking “New session” — route clears to
Asserted directly in the driver against the live DOM/URL:
2. Unit suites (real
|
| Package | File(s) | Tests |
|---|---|---|
webui |
status.test.ts + actions.test.ts + DaemonSessionProvider.test.tsx |
157 ✅ |
web-shell |
client/App.test.tsx + client/index.test.tsx |
36 ✅ |
cli |
acp-integration/acpAgent.test.ts |
191 ✅ |
384/384 passed. (CI Test (ubuntu-latest) is also green on the full suite.)
3. RED/GREEN — the new tests actually have teeth
Three surgical one-expression reverts, each isolating a different layer; each flips exactly the expected tests, and restoring returns all to green:
| Revert | Suite | Effect |
|---|---|---|
resolveConnectionErrorStatus → return nextStatus |
status.test.ts |
2 fail (404/410 preservation across status-less retries) |
App.tsx missingSession = false |
App.test.tsx |
3 fail (404 & 410 empty-state render) |
provider missingLoadedSession = false |
DaemonSessionProvider.test.tsx |
2 fail (- missingSession: true / + false for 404 & 410) |
4. Scope / honest notes
- The real-browser run exercised the 404 path (the daemon returns 404 for unknown sessions). The 410 path is covered by the unit tests through the identical
isMissingSessionHttpStatusgate (MISSING_SESSION_HTTP_STATUSES = [404, 410]) — I couldn't easily make a real daemon emit a 410. - The daemon backend is unchanged by this PR; I used the built
maindaemon purely as the real 404 source. Only the frontend rendering of that 404 is what this PR changes, and that's what the browser run verifies.
Verdict: behavior matches the Reviewer Test Plan, and this fills the Linux gap. LGTM from a verification standpoint. 👍
中文说明
✅ 本地验证（真实浏览器 + 真实 daemon，Linux）
我在 Linux（你们测试矩阵里标 404 的 qwen serve daemon，做了完整的端到端验证。Reviewer Test Plan 里的每一条都成立。
1. 真实端到端（Chromium + 本分支 Vite dev + 真实 daemon）
环境：web-shell 的 Vite dev server 直接用本分支源码运行（也就是说真正执行的是本分支的 web-shell/client/App.tsx + webui/.../DaemonSessionProvider + 新增的 status.ts），并代理到真实的 qwen serve 后端。我打开了一个过期/不存在的会话路由：
GET /session/stale-session-2f9c1a-DOES-NOT-EXIST
→ daemon: 404 "POST /session/:id/load: No session with id "stale-session-2f9c1a-DOES-NOT-EXIST""
浏览器控制台打印出了本分支新增的日志，可以确认正在运行的就是本 PR 的代码（这行日志和这句文案在 main 上都不存在）：
[DaemonSessionProvider] terminal session error (sessionId=…, status=404, message=…)
缺失会话路由 —— 显示的是明确的空状态，而不是静默跳转（见上方第一张截图）。
点击「New session」后 —— 路由清空回 /，正常输入框恢复（见上方第二张截图）。
driver 直接对实时 DOM/URL 的断言：
| 检查项 | 结果 |
|---|---|
| 显示「Current session does not exist」 | ✅ |
输入框 + 发送按钮被隐藏（[data-testid="submit"] 计数） |
✅ 0 |
| 「New session」按钮存在 | ✅ 1 |
| 侧边栏保持可见（capabilities 被保留） | ✅ |
| 失败 load 后 URL 仍保留旧 session id | ✅ /session/stale-… |
「New session」→ 只调用 clearSession()，不 create/attach |
✅ |
「New session」→ 路由清空回 /，输入框计数 |
✅ 1 |
2. 单元测试（真实 vitest，在本分支运行）
| 包 | 文件 | 测试数 |
|---|---|---|
webui |
status.test.ts + actions.test.ts + DaemonSessionProvider.test.tsx |
157 ✅ |
web-shell |
client/App.test.tsx + client/index.test.tsx |
36 ✅ |
cli |
acp-integration/acpAgent.test.ts |
191 ✅ |
384/384 通过。（CI 的 Test (ubuntu-latest) 全量套件也是绿的。）
3. RED/GREEN —— 新增测试确实“抓得住”
三处针对不同层次、各改一行表达式的回退，每一处都精准让预期的测试变红，恢复后全部转绿：
| 回退 | 套件 | 效果 |
|---|---|---|
resolveConnectionErrorStatus → return nextStatus |
status.test.ts |
2 失败（404/410 在无状态重试时的保留） |
App.tsx missingSession = false |
App.test.tsx |
3 失败（404 & 410 空状态渲染） |
provider missingLoadedSession = false |
DaemonSessionProvider.test.tsx |
2 失败（404 & 410：- missingSession: true / + false） |
4. 范围 / 如实说明
- 真实浏览器这一路跑的是 404 路径（daemon 对未知会话返回 404）。410 路径由单元测试通过完全相同的
isMissingSessionHttpStatus判定（MISSING_SESSION_HTTP_STATUSES = [404, 410]）覆盖 —— 真实 daemon 不太好构造 410。 - daemon 后端本 PR 未改动；我只是用
main构建出来的 daemon 作为真实 404 的来源。本 PR 改的是对这个 404 的前端渲染，而这正是浏览器这一路所验证的。
结论：行为与 Reviewer Test Plan 一致，并补上了 Linux 这一环。从验证角度 LGTM。👍
🤖 Verified locally with Claude Code · model: Opus 4.8 (1M context)
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Qwen Code Review — PR #6357
Status: Review failed / timed out
The automated Qwen Code review was started with /review 6357 --comment but did not complete within the 60-minute timeout. The process produced partial stream-json output (~289 KB) while actively reading the diff and searching the codebase, but no final result or assistant summary was emitted before the timeout.
- PR title: Handle missing web-shell sessions without redirecting
- Head SHA:
9f7385f48fe4ed019fa7a12fc54ad88a2bae7304 - Outcome:
ERROR— qwen CLI review timed out - Inline comments: not posted (review did not finish)
Please re-run the review or inspect the PR manually.
| status: 'connected', | ||
| error: undefined, | ||
| errorStatus: undefined, | ||
| } |
There was a problem hiding this comment.
[Suggestion] The heartbeat .then() recovery path clears errorStatus: undefined and restores status: 'connected' after consecutive failures, but no test verifies this recovery transition. The test suite covers heartbeat failures setting errorStatus (410, 401, 403) but has zero tests for recovery from error back to connected. If this clearing logic were removed or broken, users would remain stuck in a disconnected state after transient heartbeat failures resolve.
Suggested test: heartbeat fails with 410 (threshold=1, state goes disconnected with errorStatus: 410), then heartbeat succeeds, and assert connection.status === 'connected' and connection.errorStatus === undefined.
— qwen3.7-max via Qwen Code /review
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
5 participants
What this PR does
This PR makes the web shell handle missing or expired session routes explicitly. When the controlled session load fails with a terminal missing-session status, the chat pane shows a minimal empty state with a message that the current session does not exist and a button to start a new session. The URL is not cleared automatically during the failed load; it is only cleared when the user clicks the new-session action.
The session provider now preserves structured HTTP status information on connection errors so the web shell can branch on status codes instead of parsing daemon error text. It also keeps workspace capabilities available after a terminal session load failure so the sidebar can continue rendering workspace session organization such as recent and archived sections.
Why it's needed
Opening a web-shell URL with a stale or nonexistent session id previously fell through to the normal no-session flow and could replace the route with the new-chat page. That made it look like the app silently redirected instead of telling the user that the requested session no longer exists. Parsing the daemon error message was also fragile because the SDK prefixes HTTP errors and daemon text can change.
This change gives users a stable, understandable state for stale links while keeping the existing new-session flow one click away.
Reviewer Test Plan
How to verify
Open the web shell with a session id that the daemon cannot load and confirm that the main chat pane shows only the missing-session message and a new-session button while the sidebar remains visible. Confirm the URL keeps the stale session id after the failed load. Click the new-session button and confirm the route is cleared and the normal create-page state appears. Reviewers can also simulate provider state by returning a 404 or 410 from session load and checking that the same missing-session empty state appears.
Evidence (Before & After)
Before: a missing session load could clear the controlled route and show the normal new-chat page, and the sidebar could lose organization metadata because capabilities were not preserved after the failed load. After: 404 and 410 session load failures render the explicit missing-session page, the route stays intact until the user clicks new session, and workspace capabilities remain available to the sidebar.
Tested on
Environment (optional)
Local verification from the repository root on macOS: npm run build, npm run typecheck, cd packages/web-shell && npx vitest run client/App.test.tsx client/index.test.tsx, cd packages/webui && npx vitest run src/daemon/session/DaemonSessionProvider.test.tsx.
Risk & Scope
Linked Issues
N/A
中文说明
What this PR does
这个 PR 让 web shell 显式处理缺失或已失效的会话路由。当受控 session load 返回终态的会话不存在状态时，聊天区域会显示一个最小空白状态，提示当前会话不存在，并提供新建会话按钮。失败 load 时不会自动清空 URL；只有用户点击新建会话按钮时才清空路由。
session provider 现在会在连接错误上保留结构化 HTTP 状态，因此 web shell 可以基于状态码判断，而不是解析 daemon 错误文案。同时，在终态 session load 失败后会保留 workspace capabilities，让侧边栏仍然可以正常渲染最近、归档等工作区会话组织信息。
Why it's needed
以前使用过期或不存在的 session id 打开 web-shell URL 时，可能会落入普通无会话流程并把路由替换成新建聊天页。这样用户看到的是页面静默跳走，而不是明确知道请求的会话已经不存在。解析 daemon 错误文案也很脆弱，因为 SDK 会给 HTTP 错误加前缀，daemon 文案也可能变化。
这个改动为失效链接提供稳定、可理解的状态，同时保留一键进入新建会话的现有流程。
Reviewer Test Plan
How to verify
使用 daemon 无法 load 的 session id 打开 web shell，确认主聊天区域只显示会话不存在提示和新建会话按钮，同时侧边栏仍然可见。确认失败 load 后 URL 仍保留旧 session id。点击新建会话按钮后，确认路由被清空并进入正常创建页面。Reviewer 也可以通过让 session load 返回 404 或 410 来模拟 provider 状态，并确认都会显示同一个缺失会话空白页。
Evidence (Before & After)
Before：缺失会话 load 可能清空受控路由并显示普通新建聊天页，侧边栏也可能因为失败 load 后没有保留 capabilities 而丢失组织信息。After：404 和 410 的 session load 失败会渲染明确的缺失会话页，路由会保留到用户点击新建会话为止，并且 workspace capabilities 会继续提供给侧边栏。
Tested on
Environment (optional)
在 macOS 的仓库根目录本地验证：npm run build、npm run typecheck、cd packages/web-shell && npx vitest run client/App.test.tsx client/index.test.tsx、cd packages/webui && npx vitest run src/daemon/session/DaemonSessionProvider.test.tsx。
Risk & Scope
Linked Issues
N/A