Skip to content

feat(web-shell): show Settings and Daemon Status as an in-place panel#6341

Merged
wenshao merged 16 commits into
QwenLM:mainfrom
wenshao:feat/web-shell-inline-settings-status-panel
Jul 6, 2026
Merged

feat(web-shell): show Settings and Daemon Status as an in-place panel#6341
wenshao merged 16 commits into
QwenLM:mainfrom
wenshao:feat/web-shell-inline-settings-status-panel

Conversation

@wenshao

@wenshao wenshao commented Jul 5, 2026

Copy link
Copy Markdown
Collaborator

What this PR does

In the Web Shell, the Settings and Daemon Status buttons opened centered modal dialogs that dimmed the whole app. This renders them as a full-height panel that takes over the chat area (message list + composer) in place instead — the sidebar stays visible and a ← Back button (or Escape) returns to the chat. The panel content is left-aligned and fills the chat pane width rather than being centered in a narrow column. On the Daemon Status page the time-series charts get a taller plot and the overview cards a wider track, so a wide window is actually used; the overview / metrics / diagnostics tab grouping is kept.

Why it's needed

The modal overlays wasted horizontal space (centered, narrow) and dimmed the app for what are really full "views", not transient dialogs. Rendering them in place uses the full width, keeps the sidebar usable, and — on the Status dashboard — makes the trend charts legible instead of squeezed into a 52px sliver that only widened in a fullscreen mode the panel layout can't reach.

Reviewer Test Plan

How to verify

  1. cd packages/web-shell && npx vite build, then run qwen serve --web and open the printed loopback URL.
  2. Click Settings (gear) or Daemon Status (pulse) in the sidebar footer. Expect the chat area to be replaced in place by the panel — no dimmed modal backdrop, sidebar stays, a ← Back button in the header.
  3. Press Back or Escape → returns to the chat. Open a settings sub-choice (model / approval mode) → it still opens as a modal on top of the panel and returns to the panel on close. Switching / creating a session from the sidebar also returns to the chat.
  4. On Daemon Status → Metrics the charts are taller; Overview shows ~3 wide columns on a wide window.

Verified locally with qwen serve --web + Playwright (real Chrome). DOM assertions: with a panel open it renders inside the chat pane (not a document.body portal), there is no [role=dialog][aria-modal] backdrop, and the composer is replaced; Back and Escape both restore the composer; 0 console errors. tsc -p tsconfig.lib.json and eslint pass; the 3 affected component test files (67 tests) pass.

Evidence (Before & After)

Before: Settings / Daemon Status opened as a centered modal dialog with a dimmed backdrop over the chat.

After:

Settings — in-place, full-width, left-aligned:

settings panel

Daemon Status → Metrics — taller charts (52px → 140px plot):

status metrics

Daemon Status → Overview — ~3 wide columns instead of 4 cramped ones:

status overview

Tested on

OS Status
🍏 macOS
🪟 Windows ⚠️
🐧 Linux ⚠️

Environment (optional)

qwen serve --web (loopback) + Playwright real-Chrome; packages/web-shell vite build.

Risk & Scope

  • Main risk or tradeoff: Web Shell only, pure UI / layout. Panels are now views (no focus-trap / aria-modal like the old dialogs); Escape returns to the chat and is intercepted before the chat's own Esc handling. Sub-dialogs (model / approval mode) remain modals on top of the panel.
  • Not validated / out of scope: Windows / Linux (left to CI); non–Web-Shell surfaces are unaffected.
  • Breaking changes / migration notes: none.

Linked Issues

N/A

中文说明

这个 PR 做了什么

在 Web Shell 里,设置Daemon 状态按钮原本打开居中的模态弹窗、遮暗整个界面。本 PR 改为在聊天区就地渲染一个占满高度的面板(替换消息列表 + 输入框)——侧栏保持可见,通过 ← 返回按钮(或 Escape)回到聊天。面板内容左对齐、占满聊天区宽度,而不是居中在窄栏里。Daemon 状态页的时序图表加高、概览卡片加宽,让宽窗口真正被利用;概览 / 指标 / 诊断的 tab 分组保留。

为什么需要

模态弹窗浪费横向空间(居中、窄),而且为本质是完整"视图"的内容遮暗界面。就地渲染用满宽度、保持侧栏可用,并在状态面板让趋势图清晰,而不是挤在 52px 的细条里(那个只有面板布局触发不到的全屏模式才会加宽)。

复核测试计划

如何验证

  1. cd packages/web-shell && npx vite build,再 qwen serve --web,打开输出的 loopback URL。
  2. 点侧栏底部的设置(齿轮)或 Daemon 状态(脉冲)。预期聊天区被面板就地替换——无遮暗蒙层,侧栏保留,顶部有 ← 返回按钮。
  3. 返回Escape → 回到聊天。在设置里打开二级选择(模型 / 审批模式)→ 仍作为模态浮在面板之上,关闭后回到面板。从侧栏切换 / 新建会话也会回到聊天。
  4. Daemon 状态 → 指标图表更高;概览在宽窗口显示约 3 列宽卡片。

已在本地用 qwen serve --web + Playwright(真实 Chrome)验证。DOM 断言:面板打开时渲染在聊天区内部(非 body portal)、模态蒙层、输入框被替换;返回和 Escape 都恢复输入框;0 console 错误tsc -p tsconfig.lib.jsoneslint 通过;3 个相关组件测试文件(67 个用例)通过。

证据(前后对比)

改动前: 设置 / Daemon 状态是居中模态弹窗 + 遮暗蒙层。

改动后: 见上方英文部分的三张截图——设置面板就地全宽左对齐、状态指标图表加高(52px → 140px)、状态概览约 3 列宽卡片。

测试平台

macOS 已测;Windows / Linux 交给 CI。

风险与范围

  • 主要风险 / 取舍:仅 Web Shell,纯 UI / 布局。面板现在是视图(不再有旧弹窗的焦点陷阱 / aria-modal);Escape 返回聊天,且在聊天自身 Esc 逻辑之前拦截。二级弹窗(模型 / 审批模式)仍是浮在面板上的模态。
  • 未验证 / 范围外:Windows / Linux(交 CI);非 Web Shell 的界面不受影响。
  • 破坏性变更:无。

The Settings and Daemon Status buttons opened centered modal overlays that
dimmed the whole app. Render them as a full-height panel that replaces the
chat surface instead — a Back button or Escape returns to the chat — with the
content left-aligned and filling the chat pane width rather than centered in a
narrow column. On the Daemon Status page, give the time-series charts a taller
plot and the overview cards a wider track so a wide window is actually used;
the tab grouping (overview / metrics / diagnostics) is kept.
@qwen-code-ci-bot

qwen-code-ci-bot commented Jul 5, 2026

Copy link
Copy Markdown
Collaborator

Re-run after 95affdf (Scheduled Tasks mutual exclusivity fix). Template still complete ✓

Problem: Real UX issue — Settings and Daemon Status opened as centered modal dialogs that dimmed the whole app and wasted horizontal space. The before/after screenshots demonstrate the improvement clearly.

Direction: Aligned. Converting modal dialogs to in-place views is a well-established pattern. Web Shell only, no core/auth/sandbox/model/telemetry implications. Scope is focused on two components + CSS adjustments.

Approach: The scope is right for the stated goal. The chat view stays mounted (display:none) while a panel is shown, preserving the composer draft and attachments across open→Back — a deliberate trade-off (JSX duplication for state preservation) that's the right call. The latest commit (95affdf) adds mutual exclusivity between the panel and the Scheduled Tasks full-page overlay, fixing a real z-index collision where Daemon Status rendered behind Scheduled Tasks. Centralized into openPanel() / openScheduledTasks() helpers — clean, minimal, routed through all six open sites.

Moving on to code review. 🔍

中文说明

95affdf(Scheduled Tasks 互斥修复)后的重新评估。模板仍然完整 ✓

问题: 真实的 UX 问题——设置和 Daemon 状态以居中模态弹窗打开,遮暗整个界面并浪费横向空间。前后对比截图清楚展示了改善效果。

方向: 对齐。将模态弹窗转换为就地视图是成熟的设计模式。仅 Web Shell,无核心/认证/沙箱/模型/遥测影响。

方案: 范围与目标匹配。最新提交 95affdf 新增了面板与 Scheduled Tasks 全页视图的互斥逻辑,修复了 Daemon Status 渲染在 Scheduled Tasks 覆盖层后面的真实 z-index 冲突。集中到 openPanel() / openScheduledTasks() 辅助函数——干净、最小化,路由所有六个打开入口。

进入代码审查 🔍

Qwen Code · qwen3.7-max

@qwen-code-ci-bot

qwen-code-ci-bot commented Jul 5, 2026

Copy link
Copy Markdown
Collaborator

Code Review

Re-run after fix rounds (0c09836, 31fac68, 44a36cd, a122011, 6dd9790, e4c5a4b, 98aea1f, 95affdf). No critical issues found.

State management: Replacing two boolean states (showSettingsDialog, showDaemonStatusDialog) with a single activePanel union type is a good simplification. The closePanel callback is correctly wired into createNewSession, loadSidebarSession, the Escape key handler, and the /resume command path.

Mutual exclusivity (new in 95affdf): The openPanel() and openScheduledTasks() helpers ensure that the panel and the Scheduled Tasks full-page overlay are mutually exclusive — opening one closes the other. All six open sites (the /settings and /schedule commands, the three sidebar handlers, and the StatusBar) are routed through them. The commit message explains the z-index collision clearly: the Scheduled Tasks page is a position:absolute overlay that would otherwise cover the Daemon Status panel. Two mutation-checked tests verify both directions.

Draft preservation: The chat view stays mounted (display:none) while a panel is shown, so unsent drafts and attachments survive open→Back. The chatViewHidden CSS class toggles display: none, and aria-hidden is set explicitly on the hidden chat view for AT safety.

Approval auto-close: When an actionable approval arrives while a panel is open, an effect closes the panel AND dismisses stacked sub-dialogs (model/approval pickers), so the approval overlay surfaces correctly. Focus is routed to the approval overlay wrapper (tabIndex={-1}) rather than the composer, since ToolApproval's keyboard handler ignores editable targets.

Focus management: The panel focus effect correctly handles: Back button focus on panel open, composer focus restore on panel close (no pending approval), composer focus restore after approval resolves post-auto-close, and the approvalOverlayActive consolidation prevents drift between the auto-close effect and the panel focus guard.

Escape key layering: DialogShell uses a capture-phase listener with stopPropagation(), so when a sub-dialog is open over the panel, Escape closes the sub-dialog first. The sidebar's own Escape handling (data-sidebar-shell marker) is isolated from the panel's bubble-phase handler.

CSS changes: Chart height var(--chart-height, 140px) replaces the old 52px baseline with a caller-overridable custom property. Grid minmax(min(100%, 340px), 1fr) prevents overflow. Metrics charts get a dedicated .chartsGrid at 460px min. The standalone.css user-select selector was updated from [data-mobile-drawer] to [data-sidebar-shell] to match the current sidebar marker.

Sub-dialog behavior: Settings panel stays open under model/approval sub-dialogs — closing the sub-dialog returns to settings (not the chat). Intentional and better UX for a panel-based layout.

Build & Test Results

# CI — Ubuntu (Node 22.x)
✅ Pass — 26m24s (all packages including web-shell suite)
   https://github.com/QwenLM/qwen-code/actions/runs/28767730734/job/85295204152

# Previous local runs (from prior triage, pre-95affdf):
# TypeScript check (tsc -p tsconfig.lib.json --noEmit): ✅ zero errors
# Vite build: ✅ 8.73s — zero errors
# Full web-shell suite: ✅ 68 test files, 1029 tests — zero failures
# Server serve verification: ✅ HTTP 200

# The 95affdf commit adds 2 tests (46 lines) covering the
# panel↔Scheduled Tasks mutual exclusivity. Both test directions:
# - Opening Daemon Status closes Scheduled Tasks
# - Opening Scheduled Tasks closes Settings panel
# Both are mutation-checked per the commit message.

Real-Scenario Testing

This is a browser-only UI change (Web Shell). tmux cannot capture visual panel layout, modal absence, or CSS grid adjustments — the meaningful evidence is in-browser DOM assertions and screenshots.

Author's verification (from PR body, Playwright + real Chrome):

  • Panel renders inside chat pane (not a body portal), no [role=dialog][aria-modal] backdrop, composer replaced
  • Back and Escape both restore the composer
  • Settings↔Status scroll resets to top (key={activePanel})
  • Draft preserved across panel open→Back (DRAFT PRESERVED: true)
  • Focus lands on Back button when panel opens, restores to composer on close
  • Approval overlay surfaces correctly when approval arrives during panel open
  • 0 console errors

What a reviewer should verify manually:

  1. cd packages/web-shell && npx vite build && qwen serve --web
  2. Click Settings (gear) → chat area replaced by panel, no dimmed backdrop, sidebar stays visible
  3. Type in composer first → open panel → Back → draft still there
  4. Press Escape or ← Back → returns to chat, focus on composer
  5. Open model sub-dialog from Settings → opens as modal on top of panel, closes back to panel
  6. Click Daemon Status (pulse) → same in-place behavior, Metrics tab shows taller charts
  7. Switch/create session from sidebar → returns to chat (panel closes)
  8. Open Scheduled Tasks → open Daemon Status → Scheduled Tasks closes, Daemon Status shows (and vice versa)
中文说明

代码审查

修复多轮后的重新评估(含 95affdf 互斥修复)。未发现关键问题。

互斥逻辑(95affdf 新增): openPanel()openScheduledTasks() 辅助函数确保面板和 Scheduled Tasks 全页视图互斥——打开一个会关闭另一个。所有六个打开入口都已路由。两个变异校验的测试覆盖双向验证。

状态管理: 用单个 activePanel 联合类型替代两个布尔状态是好的简化。closePanel 回调正确接入了所有路径。

草稿保留: 面板显示时聊天视图保持挂载(display:none),未发送的草稿和附件在 打开→返回 后仍在。

审批自动关闭: 当可操作审批到来时,effect 关闭面板并关闭堆叠的子弹窗,使审批浮层正确显示。

焦点管理: 面板焦点 effect 正确处理所有场景。approvalOverlayActive 合并防止漂移。

CSS 变更: 图表高度改为可覆盖的自定义属性。防溢出网格。专用的宽指标图表网格。

构建与测试结果

  • CI Ubuntu 测试:✅ 通过(26m24s)
  • 95affdf 新增 2 个测试(46 行)覆盖面板↔Scheduled Tasks 互斥

真实场景测试

纯浏览器 UI 变更。作者已用 Playwright + 真实 Chrome 验证。建议复核者手动验证上述 8 个步骤。

Qwen Code · qwen3.7-max

@qwen-code-ci-bot

qwen-code-ci-bot commented Jul 5, 2026

Copy link
Copy Markdown
Collaborator

This PR has matured through nine fix rounds and a final mutual-exclusivity patch (95affdf) that addresses a real z-index collision between the panel and the Scheduled Tasks overlay. The problem is real, the solution is straightforward, and the implementation is solid.

What stands out is the iterative depth. Each review round surfaced real issues — composer draft loss, approval overlay hidden behind the panel, stacked sub-dialog dismissal, focus management edge cases, and now panel↔Scheduled Tasks z-index collision — and the author addressed each one thoroughly with mutation-checked tests. The latest commit adds openPanel() / openScheduledTasks() helpers that route all six open sites through a last-opened-wins pattern, with two tests covering both directions.

The approval auto-close path remains the most complex part: when an actionable approval arrives while a panel (possibly with a stacked sub-dialog) is open, the effect closes the panel, dismisses sub-dialogs, routes focus to the approval overlay, and the dialogOpen prop keeps the composer dormant. The approvalOverlayActive consolidation prevents drift between the three consumers. Well-reasoned and well-tested.

The chat view JSX duplication in App.tsx (~200 lines) is a known trade-off — keeping the chat view mounted preserves composer state (draft, attachments). Extracting a <ChatView /> component would reduce duplication but require threading ~30 props. Pragmatism over purity, and can be refactored later if App.tsx grows further.

CI green across all platforms. Screenshots show a clear before/after improvement. All changes are web-shell only — no core module impact.

Ships the feature cleanly. ✅

中文说明

这个 PR 经过九轮修复和最终的互斥补丁(95affdf),解决了面板和 Scheduled Tasks 覆盖层之间的真实 z-index 冲突。问题真实,方案直接,实现扎实。

最突出的是迭代深度。每轮审查都发现了真实问题——输入框草稿丢失、审批浮层被面板遮挡、堆叠子弹窗关闭、焦点管理的边界情况,以及现在的面板↔Scheduled Tasks z-index 冲突——作者用变异检验的测试彻底解决了每一个。最新提交添加了 openPanel() / openScheduledTasks() 辅助函数,将所有六个打开入口路由到"最后打开者胜出"的模式,两个测试覆盖双向验证。

审批自动关闭路径仍是最复杂的部分:设计合理、测试充分。聊天视图 JSX 重复是已知的权衡——以后可以重构。

CI 全平台通过。截图展示了清晰的前后对比改进。所有变更仅 web-shell——无核心模块影响。

可以合入。✅

Qwen Code · qwen3.7-max

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, looks ready to ship. ✅

@wenshao wenshao left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Downgraded from Request changes to Comment: self-PR; CI still running. Inline comments contain the review findings.

— GPT-5 via Qwen Code /review

Comment thread packages/web-shell/client/App.tsx Outdated
Comment thread packages/web-shell/client/App.tsx
Comment thread packages/web-shell/client/App.tsx Outdated
Comment thread packages/web-shell/client/App.tsx
Comment thread packages/web-shell/client/components/dialogs/DaemonStatusDialog.module.css Outdated

@wenshao wenshao left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed — no blockers. Suggestion-level recommendations are in the Suggestion summary comment below.

@wenshao

wenshao commented Jul 5, 2026

Copy link
Copy Markdown
Collaborator Author

Suggestions — commit b6c2d18

File Issue Suggested fix
packages/web-shell/client/App.tsx:3745 handleFastModelSelect has entirely new logic (closePanel(), sendPrompt, reloadWorkspaceSettings) with zero test coverage. If closePanel() is accidentally removed, the model-change response piles up behind the hidden chat view. Add a test that opens Settings, triggers fast-model selection, asserts the panel closed and reloadWorkspaceSettings was called.
packages/web-shell/client/App.tsx:4241 Touch-device text selection regression: the panel uses role="region", but the user-select: text rule in standalone.css only matches [role='dialog'] and [data-user-selectable]. Users on touch devices cannot select/copy text in Settings or Daemon Status panels. Add data-user-selectable="" to the panel <section>.
packages/web-shell/client/App.tsx:2355 createNewSession calls closePanel() but no test verifies the panel closes when a new session is created. Add a test: open panel → trigger onNewSession → assert [data-testid="inline-panel"] is gone.
packages/web-shell/client/App.tsx:2375 loadSidebarSession calls closePanel() but no test covers the sidebar-click path (only /resume is tested). Expose onLoadSession from the sidebar mock, open a panel, invoke it, and assert the panel closes.

— qwen3.7-max via Qwen Code /review

Comment thread packages/web-shell/client/App.tsx
Comment thread packages/web-shell/client/App.tsx Outdated
Comment thread packages/web-shell/client/App.tsx
wenshao added 2 commits July 5, 2026 21:53
…-settings-status-panel

# Conflicts:
#	packages/web-shell/client/App.tsx
Address review feedback on the in-place Settings / Daemon Status panel:

- Keep the chat view (message list + composer) mounted and just hidden while a
  panel is shown, so typing a prompt, opening Settings/Status, then going Back
  no longer discards the unsent draft and attachments (the composer subtree was
  being unmounted and remounted empty).
- Focus the Back button when a panel opens and restore focus to the composer
  when it closes, replacing the focus management DialogShell used to provide.
- Reload workspace settings after the fast-model command resolves so the still-
  mounted Settings panel doesn't keep showing the previous value.
- Don't close the panel when Escape is handled inside the sidebar (its search
  input clears on Escape without stopping the event).
- Guard the overview card grid with min(100%, 340px) so a 340px track can't
  overflow a narrow panel.
Add key={activePanel} on the panel body so switching directly between Settings
and Daemon Status (activePanel goes 'settings' -> 'status' without passing
through null) remounts the scroll container and opens at the top instead of
inheriting the previous panel's scrollTop.
@wenshao

wenshao commented Jul 5, 2026

Copy link
Copy Markdown
Collaborator Author

Thanks for the detailed review — pushed fixes for all 8 threads and rebased onto latest main (merge 0279d37). Per-thread replies are inline; summary below.

Fixes (0c09836, 31fac68):

  • [Critical] Composer draft loss — the chat view (message list + composer) now stays mounted and is only hidden (display:none) while a panel is shown, so an unsent draft and attachments survive open → Back.
  • Focus — focus the Back button when a panel opens, restore the composer when it closes.
  • Fast-model stale value — reload workspace settings after the /model --fast command resolves.
  • Sidebar Escape — the panel no longer closes when Escape is handled inside the sidebar.
  • Overview grid overflowminmax(min(100%, 340px), 1fr).
  • Panel-switch scroll inheritancekey={activePanel} remounts the scroll container on Settings↔Status.

All verified with qwen serve --web + Playwright: draft preserved (DRAFT PRESERVED: true), focus lands on Back, Escape returns to chat, Settings↔Status scroll resets to top, 0 console errors. tsc -p tsconfig.lib.json + eslint green.

中文

感谢细致的 review——已针对全部 8 条线程推送修复,并 rebase 到最新 main(merge 0279d37)。逐条回复在各线程内联,下面是汇总。

修复(0c0983631fac68):

  • [Critical] 输入框草稿丢失 — 聊天视图(消息列表 + 输入框)现在保持挂载,面板显示时仅隐藏(display:none),因此未发送的草稿和附件在 打开 → 返回 后仍在。
  • 焦点 — 打开面板聚焦返回按钮,关闭时恢复输入框。
  • fast-model 显示旧值/model --fast 命令 resolve 后 reload 工作区设置。
  • 侧栏 Escape — Escape 在侧栏内被处理时不再关闭面板。
  • 概览网格溢出minmax(min(100%, 340px), 1fr)
  • 切换面板继承滚动位置key={activePanel} 在设置↔状态切换时 remount 滚动容器。

全部用 qwen serve --web + Playwright 验证:草稿保留(DRAFT PRESERVED: true)、焦点落在返回键、Escape 返回聊天、设置↔状态滚动重置到顶部、0 console 错误。tsc -p tsconfig.lib.json + eslint 通过。

The chatViewWrap added to keep the composer mounted while a panel is shown was
filling the pane, which cancelled the empty (new-chat) state's vertical
centering (welcome header + composer stuck to the top instead of centered).
Shrink the wrapper to its content in the empty state so
`.appChatEmpty .chatPane { justify-content: center }` centers it again, matching
how `.content` behaves there.

@wenshao wenshao left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Downgraded from Approve to Comment: self-PR; CI failing: Post Coverage Comment, review-pr, delay-automatic-review, Test (ubuntu-latest, Node 22.x).

No issues found. LGTM! ✅

— qwen3.7-max via Qwen Code /review

Comment thread packages/web-shell/client/App.tsx
Comment thread packages/web-shell/client/App.tsx
Comment thread packages/web-shell/client/App.tsx Outdated
wenshao added 2 commits July 5, 2026 23:37
My earlier merge of origin/main 3-way-combined WebShellSidebar.test.tsx
incorrectly, dropping the `{ }` destructuring from the 7 session-organization
tests. renderSidebar returns `{ container, rerender }` (not the element), so
`const container = renderSidebar(...)` made `container.querySelector` throw.
Restore the file to origin/main so the tests pass again.

@doudouOUC doudouOUC left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

Overall the approach is sound — converting modal dialogs to in-place panels for Settings and Daemon Status is the right UX direction. The implementation is clean with good focus management and keyboard handling. A few concerns below.

Positive

  • Good focus management: auto-focus Back button on open, return to composer on close
  • Correct key={activePanel} on panelBody to force remount between views
  • Thoughtful Escape handling that scopes to outside the sidebar drawer
  • Proper deps array updates for closePanel in createNewSession / loadSidebarSession
  • reloadWorkspaceSettings() after fast model switch to keep panel in sync

Comment thread packages/web-shell/client/App.module.css
Comment thread packages/web-shell/client/App.tsx
Comment thread packages/web-shell/client/App.tsx Outdated
The in-place Settings/Daemon Status panel hides the chat footer with
display:none, and the ToolApproval / AskUserQuestion overlays live in
that footer. A gated tool call that arrived while a panel was open
rendered the approval into a hidden container, so the turn hung with no
visible prompt (reported [Critical]).

Close the panel when an actionable approval is pending so it surfaces.
Only actionable approvals count (pendingToolApproval / pendingAskUserApproval
already gate on canActOnPendingApproval), so a non-owner in a shared
session isn't yanked out of Settings by someone else's prompt. Leave
focus for the overlay rather than the composer on this path: ToolApproval
uses a window-level key handler that ignores editable targets, so
focusing the composer would swallow its shortcuts.

Also refocus the Back button on panel->panel switches (not just on open),
so focus no longer relies on the Back button being the same DOM node
across the keyed panel body, and expose SvgLineChart's plot height as
--chart-height (fallback 140px) so a constrained caller can shrink it.

Adds App-level tests: the panel auto-closes on a pending tool approval,
and stays open when the only block is a resolved (non-actionable) one.
@wenshao

wenshao commented Jul 5, 2026

Copy link
Copy Markdown
Collaborator Author

Thanks both — addressed this round in 44a36cd (pushed). Per-thread replies inline; rollup below.

Fixed

  • [Critical] Approval hidden behind the panel — a gated tool call arriving while Settings/Status was open rendered ToolApproval / AskUserQuestion into the display:none chat footer, hanging the turn. An effect now closes the panel when an actionable approval is pending, so it surfaces. On that close path focus is left for the overlay (not the composer), since ToolApproval's window key-handler ignores editable targets.
  • Focus on panel→panel switch — the focus effect now refocuses Back on any truthy activePanel, not just on open, so it no longer relies on Back being the same DOM node across the keyed body.
  • SvgLineChart height — now var(--chart-height, 140px) so a constrained caller can override it.

Explained / confirmed (no change needed)

  • key={activePanel} vs scroll-ref — the key isn't what remounts DaemonStatusDialog; the type-swapping ternary does that regardless. The ref approach would drop state preservation and reintroduce the stale-scrollTop bug the key fixed, so I kept it.
  • Escape priority while streaming — intended; interactionBlocked already suppresses cancel while a panel is open, so nothing is shadowed.
  • onSubDialog focus / double-suppressionDialogShell restores focus to the opening settings control; interactionBlocked is a single OR, so no double-suppression.

Open for your call

  • Virtual scroll + display:none (left unresolved) — reasoned from the code that retained scrollTop + the virtualizer's ResizeObserver re-measure keep it correct, and explained why an unconditional scrollToBottom-on-close would regress a scrolled-up reader. Offered to add a targeted long-transcript E2E if you'd like the extra proof.

Verification: tsc -p tsconfig.lib.json clean; full build chain exit 0; eslint clean on touched files; web-shell suite 1016/1016. The new App.test auto-close test is mutation-checked (disabling the effect fails only that test; the resolved-permission control still passes).

中文

已在本轮 44a36cd 处理(已推送),逐线程回复见内联,汇总如下。

已修

  • [Critical] 审批被面板遮挡——打开设置/状态时到来的受限工具调用会把 ToolApproval/AskUserQuestion 渲染进 display:none 的聊天页脚,导致回合挂起。现新增 effect:存在可操作审批时关闭面板使其显示。该关闭路径把焦点留给浮层(非输入框),因 ToolApproval 的 window 按键处理会忽略可编辑目标。
  • 面板→面板切换的焦点——焦点 effect 现对任意 truthy 的 activePanel 都重聚焦返回按钮,不再依赖返回按钮跨 keyed body 是同一 DOM 节点。
  • SvgLineChart 高度——改为 var(--chart-height, 140px),受限调用方可覆盖。

已解释/确认(无需改动)

  • key={activePanel} vs 滚动 ref——key 不是 DaemonStatusDialog 重挂载的原因,切换类型的三元表达式才是;ref 方案既保不住状态,又会重新引入 key 修好的 scrollTop 陈旧 bug,故保留。
  • 流式时的 Escape 优先级——预期;面板打开时 interactionBlocked 已抑制取消,无遮蔽。
  • onSubDialog 焦点/双重抑制——DialogShell 把焦点恢复到打开它的设置控件;interactionBlocked 是单个 OR,无双重抑制。

留待你定

  • 虚拟滚动 + display:none(未解决)——基于代码推理:保留的 scrollTop + 虚拟化器 ResizeObserver 重测量保证正确,并说明无条件的关闭时 scrollToBottom 会让向上翻看的读者退化。如需额外佐证,可加一个长记录定向 E2E。

验证: tsc -p tsconfig.lib.json 干净;完整构建链 exit 0;改动文件 eslint 干净;web-shell 套件 1016/1016。新增 App.test 自动关闭用例已做变异校验(禁用 effect 仅该用例失败,已解决权限的对照仍通过)。

@wenshao wenshao left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Downgraded from Request changes to Comment: self-PR. Inline comments contain the review findings.

— qwen3.7-max via Qwen Code /review

Comment thread packages/web-shell/client/App.tsx
Comment thread packages/web-shell/client/App.tsx
Comment thread packages/web-shell/client/App.test.tsx Outdated
Comment thread packages/web-shell/client/App.tsx Outdated
Comment thread packages/web-shell/client/App.tsx Outdated
wenshao added 2 commits July 6, 2026 01:58
Follow-up to the approval auto-close fix, addressing review suggestions:

- Add data-testid="inline-panel" to the panel <section> and query by it in
  App.test instead of querySelector('section'), which would false-positive if
  any future component renders a <section>.
- Add a companion test for the pendingAskUserApproval branch of the auto-close
  effect. The ask-user block carries toolCall.input.questions so
  isAskUserPermission() classifies it as an AskUserQuestion; mutation-checked
  (restricting the effect to pendingToolApproval fails only this test).
- Document why handleFastModelSelect's post-sendPrompt reload is not racy:
  sendPrompt awaits waitForAcceptedPromptCompletion, so the /model --fast turn
  has already applied when reloadWorkspaceSettings() runs. Also note why the
  command path needs the explicit reload that the setWorkspaceSetting pickers
  (vision/voice) get for free via the settingsVersion signal.
The /resume <id> command and the ResumeDialog onSelect both call
loadSession() without closePanel(), unlike createNewSession and
loadSidebarSession. Loading a session means the user wants that chat, so
leaving a Settings/Daemon Status panel open would hide it. Add closePanel()
to both paths for consistency (a no-op when no panel is open).

Currently reachable only in theory — the composer that submits /resume is
display:none while a panel is shown — but the guard keeps the invariant if
a non-composer entry point (sidebar/shortcut) is ever added.

Adds a mutation-checked test (removing closePanel from the /resume handler
fails it) and gives the ChatEditor test mock a focus() method, since the
panel-close focus effect now runs editorRef.focus() on this no-approval path.

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found. LGTM! ✅

— qwen3.7-max via Qwen Code /review

@wenshao wenshao left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Downgraded from Request changes to Comment: self-PR. Inline comments contain the review findings.

— GPT-5 via Qwen Code /review

Comment thread packages/web-shell/client/App.tsx Outdated
Follow-up to the approval auto-close fix. When an approval arrives while a
Settings/Status panel is open, the auto-close clears activePanel, so
interactionBlocked flips false and useComposerCore's dialogOpen effect
refocuses the still-mounted composer. ToolApproval ignores approval
shortcuts from editable targets, so the now-visible approval stops
responding to Enter/Escape/number keys until focus moves away.

Key the ChatEditor dialogOpen prop off the pending approval too, so the
composer stays blurred while an approval owns the keyboard. Consolidate the
"an approval overlay is active" condition — previously duplicated in the
auto-close effect and the panel focus guard — into a single
approvalOverlayActive value so the three consumers can't drift.

Uses the actionable-approval condition (pendingToolApproval /
pendingAskUserApproval) rather than raw pendingApproval, matching the
auto-close gating and avoiding a blurred composer when a non-actionable
approval renders no overlay.

Adds a mutation-checked test asserting dialogOpen is true while an approval
is pending with no panel open.

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No review findings — the implementation is clean and well-tested. Downgraded from Approve to Comment: CI still running.

Low-confidence note (terminal only): The activePanel === 'status' branch (rendering DaemonStatusDialog) has no unit test coverage — all 5 new tests open the panel via /settings. The gap is narrow (simple ternary, lifecycle tested regardless of panel type) but a regression in the 'status' literal or import would go undetected.

— qwen3.7-max via Qwen Code /review

A review note observed that all five panel tests open via /settings, so the
activePanel === 'status' branch (DaemonStatusDialog) had no coverage — a
regression in the 'status' literal would go undetected. Add a test that
opens Daemon Status through the sidebar and asserts the panel opens and
auto-closes on a pending approval, exercising that branch and confirming the
auto-close is panel-type-agnostic.

Gives the WebShellSidebar test mock an onOpenDaemonStatus button, since
there is no slash command to open the status panel. Mutation-checked:
neutering setActivePanel('status') fails only this test.
@wenshao

wenshao commented Jul 5, 2026

Copy link
Copy Markdown
Collaborator Author

Re: the low-confidence note on the activePanel === 'status' branch having no test coverage — good call, closed it in e4c5a4b.

Added opens the Daemon Status panel and auto-closes it on a pending approval, which opens Daemon Status through the sidebar (there's no slash command for it, so the WebShellSidebar test mock now exposes an onOpenDaemonStatus button) and asserts the panel opens and auto-closes — exercising the 'status' literal and confirming the auto-close is panel-type-agnostic. Mutation-checked: neutering setActivePanel('status') fails only this test. (The DaemonStatusDialog import itself is a mock here, but it's covered by DaemonStatusDialog.test.tsx and would fail tsc if it regressed.)

中文

关于 activePanel === 'status' 分支无测试覆盖的低置信度提示——说得对,已在 e4c5a4b 补齐。

新增 opens the Daemon Status panel and auto-closes it on a pending approval:经侧栏打开守护状态(无对应斜杠命令,故 WebShellSidebar mock 现暴露一个 onOpenDaemonStatus 按钮),断言面板打开并在审批到来时自动关闭——覆盖 'status' 字面量并确认自动关闭与面板类型无关。已做变异校验:置空 setActivePanel('status') 只有此测试失败。(此处 DaemonStatusDialog 本身是 mock,但它由 DaemonStatusDialog.test.tsx 覆盖,若 import 回归会在 tsc 阶段失败。)

Comment thread packages/web-shell/client/App.tsx Outdated
Comment thread packages/web-shell/client/App.tsx Outdated
Comment thread packages/web-shell/client/App.tsx
Comment thread packages/web-shell/client/App.tsx
Comment thread packages/web-shell/client/App.tsx Outdated
Comment thread packages/web-shell/client/App.tsx Outdated
Comment thread packages/web-shell/client/App.tsx Outdated
Comment thread packages/web-shell/client/App.tsx
Comment thread packages/web-shell/client/App.tsx
Comment thread packages/web-shell/client/App.tsx
Addresses a review round on the inline Settings/Status panel.

[Critical] When an approval arrives while a DialogShell sub-dialog (model
picker / approval-mode picker) is open over the panel, the auto-close
removed the panel but left the sub-dialog backdrop covering the footer
approval overlay, so the turn hung. Worse, the approval-mode picker stayed
usable — selecting "yolo" auto-approved (handleSetMode) a tool call the
user never saw. Dismiss the panel AND both sub-dialogs when an actionable
approval is pending.

[Critical] After the panel closes for an approval, focus fell to <body>
(the Back button unmounted; ToolApproval has no autofocus). Move focus onto
the ToolApproval overlay wrapper (tabindex=-1, so its window-listener
shortcuts keep working and Enter doesn't confirm early) once it is visible.
AskUserQuestion keeps managing its own focus.

Suggestions:
- Guard reloadWorkspaceSettings() rejection (was unhandled).
- aria-hidden the display:none chat view so AT can't wander into it.
- Close the panel before /model --fast so its response shows in the chat
  in context instead of piling up behind the hidden panel.
- Remove the dead .panelBodyInner wrapper (redundant width:100%).
- Rename data-mobile-drawer -> data-sidebar-shell (it wraps the desktop
  sidebar for all viewports) across App.tsx + standalone.css.

Tests (+5, mutation-checked): sub-dialog dismissal on approval, overlay
focus, Escape closes panel / sidebar-Escape does not, dialogOpen while a
panel replaces the chat. Adds an observable DialogShell test mock.

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Downgraded from Approve to Comment: CI still running.

No blocking issues. Three suggestion-level findings on test coverage and a dead CSS rule below.

— qwen3.7-max via Qwen Code /review

Comment thread packages/web-shell/client/App.test.tsx
Comment thread packages/web-shell/client/App.tsx Outdated

@wenshao wenshao left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Downgraded from Request changes to Comment: self-PR; CI still running. Inline comments contain the review findings.

Comment thread packages/web-shell/client/App.tsx
[Critical] The panel focus effect consumed prevActivePanelRef to null on the
approval auto-close (correctly skipping editor focus then). When the approval
later resolved with no panel to return to, neither branch fired and focus was
left on <body> — the visible composer took no keyboard input. Track the
approvalOverlayActive transition too and restore composer focus on
approval-resolve-after-panel-close. (useComposerCore's dialogOpen effect also
covers this; the extra branch makes the panel effect self-contained.)

Suggestions:
- Log the reloadWorkspaceSettings() rejection instead of swallowing it.
- Remove the dead :global([data-dialog-fullscreen]) .grid rule (the
  allowFullscreen source was removed when Daemon Status became a panel).

Tests (+4, focus-restore mutation-checked): focus restored after an approval
resolves post-auto-close; Back-button closes the panel and restores focus;
fast-model pick closes the panel, sends /model --fast, and reloads settings;
chat view is aria-hidden while a panel is shown. Adds interactive
SettingsMessage / ModelDialog mocks and stable editor-focus / settings-reload
spies.

@wenshao wenshao left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Downgraded from Approve to Comment: self-PR; CI still running. No blocking issues. One suggestion-level finding on approval overlay accessibility below.

— qwen3.7-max via Qwen Code /review

@wenshao

wenshao commented Jul 6, 2026

Copy link
Copy Markdown
Collaborator Author

Suggestions — commit 98aea1f

  • packages/web-shell/client/App.tsx:4423,4437 — Approval overlay wrapper <div ref={approvalOverlayRef} tabIndex={-1}> receives programmatic focus but has no role or aria-label. Screen readers announce nothing meaningful when focus lands here. Consider role="alertdialog" + aria-label.

— qwen3.7-max via Qwen Code /review

doudouOUC
doudouOUC previously approved these changes Jul 6, 2026

@doudouOUC doudouOUC left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review (v2 — post 98aea1f)

Significant improvement since the first pass. All critical issues from prior rounds have been addressed:

Previously raised — now resolved

Issue Resolution
display:none + virtual scroll concern Author confirmed: browsers retain scrollTop on display:none nodes; ResizeObserver re-measures on un-hide. No action needed.
Escape priority during streaming Correctly scoped: interactionBlocked via activePanel !== null already suppresses cancel; the panel Escape handler is consistent.
onSubDialog not closing panel handleFastModelSelect now calls closePanel() + reloadWorkspaceSettings() — the fast-model turn is visible to the user.
Hardcoded 140px chart height Now uses var(--chart-height, 140px) — callers can override.
Approval overlay hidden behind panel useEffect auto-closes panel + sub-dialogs on approvalOverlayActive, with focus management and tests.

New observations (non-blocking)

  1. Focus effect complexity is high but correct. The three-branch useEffect (panel open/close, approval-resolve) handles all transitions correctly. Considered suggesting extraction into a custom hook for readability, but the inline comments document each branch well enough.

  2. approvalOverlayRef shared between two conditional elementsToolApproval and AskUserQuestion both use the same ref. This is safe because they're mutually exclusive (classified by isAskUserPermission), but a future refactor adding a third overlay type would need to account for this.

  3. Test coverage is thorough. 15+ new test cases covering: panel open/close via /settings and sidebar button, Escape from panel vs sidebar, approval auto-close for both tool and ask-user variants, focus transitions, sub-dialog dismissal, /resume closing panel, aria-hidden on chatViewWrap, fast-model settings reload, and composer dormancy.

  4. data-mobile-drawerdata-sidebar-shell rename — Good semantic improvement; consistently updated in standalone.css, the Escape handler, and the touch scroll prevention.

Verdict

The implementation is solid. Approval interactions are safe, keyboard/AT focus is well-managed, tests verify the critical paths. No blocking issues remaining. LGTM.

@wenshao

wenshao commented Jul 6, 2026

Copy link
Copy Markdown
Collaborator Author

Thanks for the thorough re-review @doudouOUC — much appreciated. Resolved the virtual-scroll thread now that it's confirmed no-action.

On observation #2 (approvalOverlayRef shared between the ToolApproval and AskUserQuestion wrappers): agreed it's safe today only because they're mutually exclusive via isAskUserPermission. If a third overlay type is ever added, the ref-per-wrapper pattern (or a keyed container) would need revisiting — noted in the code's intent so it isn't a silent trap.

中文

感谢细致的复审 @doudouOUC。已 resolve 虚拟滚动那条线程(既已确认无需改动)。关于观察 #2(approvalOverlayRefToolApprovalAskUserQuestion 两个 wrapper 间共享):同意,今天安全仅因二者经 isAskUserPermission 互斥。若未来新增第三种浮层类型,这个每-wrapper-一个-ref 的写法(或改用 keyed 容器)需重新考量——已在代码意图中记下,避免成为隐蔽陷阱。

@wenshao

wenshao commented Jul 6, 2026

Copy link
Copy Markdown
Collaborator Author

@qwen-code /triage

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No review findings. Downgraded from Approve to Comment: CI still running.

— qwen3.7-max via Qwen Code /review

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, looks ready to ship. ✅

Resolved App.tsx conflicts between this branch's inline Settings/Daemon
Status panel and main's new inline Scheduled Tasks page — both are
full-pane views that replace/cover the chat:

- Kept `activePanel` (Settings/Status; it replaced the old
  `showDaemonStatusDialog` boolean) and adopted main's `mainView`
  ('chat' | 'scheduledTasks').
- `interactionBlocked = dialogOpen || mainView !== 'chat'` — `dialogOpen`
  keeps `activePanel !== null`, plus main's full-pane guard.
- Render both full-pane blocks before the (still-mounted) chat view.

Also extended the approval auto-close: the Scheduled Tasks `fullPage`
overlay covers the chat footer where the approval renders, so an approval
now dismisses it too (like the panel/sub-dialogs) and the overlay-focus is
gated on `mainView === 'chat'`. Added a mutation-checked test.

Rebuilt @qwen-code/sdk and @qwen-code/webui so web-shell typechecks against
main's new SDK exports (DaemonScheduledTask, listScheduledTasks, session
color). Verified: web-shell 1040 tests pass, tsc + vite build clean.
@wenshao wenshao dismissed stale reviews from qwen-code-ci-bot and doudouOUC via 83ee328 July 6, 2026 04:10
…ly exclusive

Opening Scheduled Tasks (mainView — a position:absolute z-index overlay) and
then Daemon Status (activePanel) rendered the Daemon Status panel *behind* the
Scheduled Tasks overlay, so the button looked like it did nothing. These are
mutually-exclusive full-pane views, so opening one must close the other.

Centralize into openPanel() / openScheduledTasks() helpers (last-opened wins)
and route all six open sites (the /settings and /schedule commands, the three
sidebar handlers, and the StatusBar) through them.

Mutation-checked tests: opening Daemon Status closes the Scheduled Tasks page
(the reported repro), and opening Scheduled Tasks closes an open panel.

@doudouOUC doudouOUC left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. All prior concerns addressed — approval overlay surfacing, focus management, CSS variable for chart height, comprehensive tests. No blocking issues.

@wenshao

wenshao commented Jul 6, 2026

Copy link
Copy Markdown
Collaborator Author

@qwen-code /triage

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, looks ready to ship. ✅

@wenshao wenshao added this pull request to the merge queue Jul 6, 2026
Merged via the queue into QwenLM:main with commit 170ce79 Jul 6, 2026
83 of 84 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants