fix(web-shell): polish embedded shell interactions - #7477
Conversation
|
Thanks for the PR! Template looks good ✓ Problem: these are observed UI inconsistencies — tooltip arrows not matching the bordered surface, slash-menu flipping below the composer on narrow screens, passive artifact refresh failures surfacing global error toasts, and the worktree action leaking into embedded consumers. No linked issue, but these are self-evident visual/interaction problems you can see by using the Web Shell. The before/after description is clear enough for a polish PR. Direction: aligned — Web Shell interaction polish is squarely within scope. No auth, sandbox, model-selection, or telemetry surfaces are touched. CHANGELOG has no direct reference to tooltip styling or artifact error suppression, but worktree and slash-command are active areas, so the neighborhood is relevant. Size: not applicable — no core infrastructure paths are touched. All changes are in Approach: five related polish items in one PR, but the total diff is modest (173+/148−) and each change is small. The scope feels right for a "polish" PR. One thing worth noting: the Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题：这些是已观察到的 UI 不一致——Tooltip 箭头与描边表面不匹配、斜杠菜单在窄屏翻转到输入框下方、被动 artifacts 刷新失败弹出全局错误 toast、Worktree 操作泄漏到嵌入方。没有关联 issue，但这些都是使用 Web Shell 就能直接看到的视觉/交互问题。Before/after 描述对 polish PR 来说足够清晰。 方向：对齐——Web Shell 交互优化完全在范围内。未触及 auth、sandbox、模型选择或遥测。CHANGELOG 没有直接提到 Tooltip 样式或 artifact 错误抑制，但 worktree 和斜杠命令是活跃领域。 规模：不适用——未触及核心基础设施路径。所有改动在 方案：一个 PR 包含五项相关优化，但总 diff 不大（173+/148−），每项改动都较小。范围对 "polish" PR 来说合理。值得注意的是 进入代码审查 🔍 — Qwen Code · qwen3.7-max Reviewed at |
🖼️ 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 |
Code ReviewIndependent proposal: For these five polish items I would have done roughly the same — CSS pseudo-element arrow instead of the SVG One blocking issue: 4 existing tests fail. The Everything else looks clean:
Build & typecheck: Real-Scenario TestingThis is a Web Shell (browser UI) PR — tooltip styling, slash-menu layout, and artifact panel behavior can't be driven via tmux. Verified the CLI still works with the PR changes bundled: Build + bundle succeeded. Web Shell production build ( 中文说明独立方案： 对这五项优化，我的方案基本一致——用 CSS 伪元素箭头替代 SVG 一个阻塞问题：4 个现有测试失败。 其余改动均无问题：Tooltip 箭头伪元素方案合理；WorkspaceSelector 的 tooltip 抑制逻辑正确；斜杠面板固定宽度更简洁；artifact 错误静默处理安全（唯一调用方已同步修改）；ArtifactPanel 类型修复和 ChatPane subagent 透传正确；Drawer 的 构建和类型检查通过。CLI 功能正常。4 个测试失败是唯一回归。 — Qwen Code · qwen3.7-max Reviewed at |
|
Confidence: 3/5 — clean approach that fixes real TS errors on Stepping back: the PR does what it sets out to do, and does it well. The tooltip arrow pseudo-element, the controlled tooltip/menu interaction, the simplified slash-panel layout, the passive artifact error handling, and the worktree toggle prop are all straightforward, well-scoped changes. My independent proposal matched the PR's approach almost exactly — I didn't find a simpler path it missed. What tips this from "approve" to "needs one more pass" is the test regression. The Worth noting: this PR fixes 3 TypeScript errors that currently exist on @ytahdn could you update the worktree toggle tests to pass 中文说明置信度：3/5 — 方案干净，修复了 总体来看：PR 完成了它设定的目标，而且做得很好。Tooltip 箭头伪元素、受控的 tooltip/菜单交互、简化的斜杠面板布局、被动 artifact 错误处理、以及 worktree toggle prop 都是范围明确、实现简洁的改动。我的独立方案与 PR 几乎完全一致——没有找到更简路径。 从"批准"变为"需要再过一轮"的原因是测试回归。 值得注意的是：此 PR 修复了 @ytahdn 能否更新 worktree toggle 测试，传入 — Qwen Code · qwen3.7-max Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
The code looks good and fixes 3 real TS errors on main, but 4 existing worktree-toggle tests in App.test.tsx fail because they don't pass the new showWorktreeToggle prop. Please add showWorktreeToggle: true to the test setup — once those pass, this is ready to ship. 🙏
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Review Summary
Solid UI polish PR overall. The showWorktreeToggle opt-in design is clean, and the tooltip/slash-menu interaction improvements are well-directed. Two concerns worth addressing before merge:
Issues
-
Double-silent artifact errors (
useSessionArtifacts.tsL76 +actions.tsL1210): Daemon side removesdispatchActionError, client side catches all errors withsetError(null). If the artifact endpoint fails persistently (permission change, protocol mismatch), neither users nor developers get any signal. Suggest keeping at least aconsole.warnor preserving error state for non-disconnect errors. -
Slash menu clipping in small viewports (
ChatEditor.tsxL909):avoidCollisions={false}means the menu won't flip when viewport height is insufficient. In a ~600px window with many commands, the top items become invisible and unreachable. Suggest keeping collision detection or addingmax-height + overflow-y: auto. -
Fixed 620px width removed narrow-screen fallback (
ChatEditor.module.cssL193): The@container (max-width: 699px)single-column fallback was removed. In sidebar embeds (~400px), the two-column layout gets very cramped (220px left + ~170px right). Suggest keeping a container query breakpoint for single-column degradation. -
Tooltip suppression not reset on keyboard Escape (
WorkspaceSelector.tsxL63-66):suppressTooltipRefonly resets on pointer enter/leave/blur. Keyboard users pressing Escape to close the menu may permanently suppress the tooltip until the next pointer event. Consider resetting inonOpenChange(false). -
Global
sideOffsetchange (tooltip.tsx): Changing from 0 to 8 affects all tooltip consumers — confirm no visual regression elsewhere.
Minor
loadArtifactsplain Error message could reuse an existing error code for client-side differentiation- Detail-side calculation logic (~20 lines in ChatEditor.tsx) could be extracted to a utility function
- Tests only cover
showWorktreeToggle: true; add an assertion for defaultfalse
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. 1 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here.
— qwen3.7-max via Qwen Code /review
| useEffect(() => { | ||
| if (showWorktreeToggle) return; | ||
| pendingWorktreeRef.current = undefined; | ||
| setWorktreePending(false); | ||
| }, [showWorktreeToggle]); |
There was a problem hiding this comment.
[Suggestion] The new useEffect that clears worktreePending state when showWorktreeToggle flips to false has no test.
Failure scenario: if a consumer initially renders with showWorktreeToggle={true}, the user enables the worktree intent, and then the consumer re-renders with showWorktreeToggle={false} (e.g., due to a config change), the effect should clear the pending intent. Without a test, a regression that leaves stale worktreePending=true — showing the worktree badge when the toggle feature is disabled — would go undetected.
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Thanks, this is a valid regression-test suggestion. The current implementation both gates the badge rendering on showWorktreeToggle and clears the pending intent when the prop becomes false. To keep this UI-polish PR scoped to the requested behavior, we are not adding the dynamic prop-transition test in this PR.
| } catch { | ||
| if (requestIdRef.current !== requestId) return; | ||
| if (isSessionDisconnectedError(err)) { | ||
| setError(null); | ||
| return; | ||
| } | ||
| setError(err instanceof Error ? err.message : String(err)); | ||
| setError(null); |
There was a problem hiding this comment.
[Suggestion] The catch block now unconditionally silences all errors from loadArtifacts, but the error path has no test coverage.
Failure scenario: previously, only "session is not connected" errors were silenced; now every error (malformed responses, network failures, server-side bugs) is silently swallowed. If the silencing logic is later changed or removed (e.g., a contributor re-adds error surfacing), there is no test to catch the regression, and the "artifact errors are invisible to the user" contract has no regression guard.
| } catch { | |
| if (requestIdRef.current !== requestId) return; | |
| if (isSessionDisconnectedError(err)) { | |
| setError(null); | |
| return; | |
| } | |
| setError(err instanceof Error ? err.message : String(err)); | |
| setError(null); | |
| } catch { | |
| if (requestIdRef.current !== requestId) return; | |
| // Intentionally silence: artifact discovery is passive | |
| setError(null); | |
| } |
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Thanks, this is a valid regression-test suggestion. Silencing passive artifact-discovery failures is an explicit behavior change in this PR, but we are keeping the current PR minimal and are not adding a dedicated error-path test here.
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
LGTM. Inline comments noted — the artifact error silencing and small-viewport clipping are worth a follow-up but non-blocking.
|
/triage |
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
Code Review Overview (AI Generated)
PR: #7477 fix(web-shell): polish embedded shell interactions
Type: Bug Fix / UX polish
Change size: +180/-155 across 11 files
HEAD: 30b990cc
Findings Summary
- Critical/Major: 0 items
- Minor: 2 items (1 unique, 1 overlapping)
- Nit: 2 items (both unique)
Key Observations
Well-scoped polish PR. I independently verified all six claims in the description against the implementation: the tooltip suppression state machine in WorkspaceSelector is correct across mouse/keyboard/touch flows, the showWorktreeToggle opt-in is properly gated end-to-end (eligibility, badge render, cleanup effect, memo deps), the drawer onOpenFilePreview wiring completes both ArtifactPanel variants, and the subagent workspaceActions passthrough is type-consistent with the TurnOutputOpenRequest union (the subagent variant intentionally carries no workspaceActions field).
Cross-Validation
| Finding | Other Reviewer | My Assessment |
|---|---|---|
| C1: Double-silent artifact errors | yiliang114 | Confirmed — valid. Author frames it as an explicit product decision (passive discovery); my complementary angle is the dead error plumbing this leaves behind (see inline) |
C2: Slash menu clipping with avoidCollisions={false} |
yiliang114 | Disagree (false positive) — verified against @radix-ui/react-popper source: the size() middleware runs unconditionally (only shift()/flip() are gated on avoidCollisions), so --radix-popper-content-available-height stays populated. The CSS chain already bounds and scrolls: [data-web-shell-slash-menu] { max-height: var(...); overflow: hidden } → .slashPanel { max-height: min(460px, available - 20px) } → .slashList { overflow-y: auto }. The suggested "add max-height + overflow-y: auto" already exists; items remain reachable via scroll. Author's rebuttal is correct |
| C3: Fixed 620px width, single-column fallback removed | yiliang114 | Confirmed as intentional — the PR description explicitly promises a "stable two-column layout" on narrow embeds; the detail column truncates with ellipsis and full text moves to the adaptive hover card. Product decision, not a defect |
| C4: Tooltip suppression not reset on Escape | yiliang114 | Disagree — onBlur also resets suppressTooltipRef when the menu is closed, so suppression is not permanent: Tab-away → blur clears it → Tab-back → tooltip opens. Combined with the pointerEnter/pointerLeave resets, all realistic flows recover. Nit at most |
C5: Global sideOffset 0→8 |
yiliang114 | Confirmed as a verify-item — intentional consistency change; the arrow pseudo-element and spacing change together for all tooltip consumers |
C6: Tests only cover showWorktreeToggle: true |
yiliang114 / ci-bot | Confirmed — matches my finding (see inline) |
| C7: Worktree tests failing without the new prop | qwen-code-ci-bot | Resolved — fixed in commit 30b990cc |
Unique-1: Dead error plumbing end-to-end |
— | New finding at useSessionArtifacts.ts:78 (inline) |
| Unique-2: Tooltip loses cwd disambiguation | — | New Nit at WorkspaceSelector.tsx |
Unique-3: Arrow offset asymmetry (-bottom-[6px] vs -5px elsewhere) |
— | New Nit at tooltip.tsx |
Additional Audit Coverage
Areas I independently checked beyond existing findings:
- Radix collision semantics: fetched
@radix-ui/react-popperdist source to confirmsize()middleware and its CSS-variable population are not gated onavoidCollisions— resolves C2 definitively. - Preflight dedup safety: the
font/color/paddingremoval from.gitBranchChipButtonis covered by the global button reset instyles/preflight.css(scoped to[data-web-shell-root]/[data-web-shell-portal-root]), so no style regression. - CSS variable existence:
--chat-editor-bg-tertiaryis defined (App.module.css);--subtle-bgremains used by other components, so no dead variable. - Tooltip state values: the old
data-open:animate-innever matched Tooltip'sdelayed-open/instant-openstates; the newdata-[state=instant-open]rule correctly adds the previously missing instant-open animation. - Detail-side computation: best-effort preference only — the detail popover keeps collision avoidance, so Radix still guarantees no overflow if the computed side doesn't fit.
- Blast radius of
loadArtifactschange: only production caller isuseSessionArtifacts(catches silently); no test asserts the removed notice strings;TurnOutputOpenRequest/ArtifactPanelTabsubagent variants are type-consistent with the new passthrough. 'workspaceActions' in activeTabnarrowing: tab creation spreads the key only when truthy, and the?? defaultWorkspaceActionsfallback covers the present-but-undefined case — behavior for non-subagent tabs is unchanged.
Final Verdict
Recommend Merge — no blocking issues. The silent-artifact-failure behavior is an explicit, documented product decision; the only cleanup I'd push for is removing (or repurposing) the now-unreachable error plumbing and adding one default-false worktree test to lock in the new embedded contract. C2 and C4 are verified false positives.
This review was generated by QoderWork AI
| return; | ||
| } | ||
| setError(err instanceof Error ? err.message : String(err)); | ||
| setError(null); |
There was a problem hiding this comment.
[Minor] error state is now dead plumbing end-to-end
With every path calling setError(null), the error field can never be non-null, yet it is still returned from the hook (SessionArtifactsState.error), consumed in App.tsx (error={artifactsError} on both ArtifactPanel instances), and rendered in ArtifactPanel (if (error) return <div className={styles.empty}>{error}</div>). That render branch is now unreachable, and the field misleads future readers into thinking failures surface somewhere.
Complementary to @yiliang114's silence concern: whatever the product decision on visibility, either (a) delete the error plumbing in the hook/App/panel, or (b) keep it and set it for non-transient failures so the panel can show an unobtrusive inline hint (and consider a console.debug here so persistent failures are at least observable in devtools).
This review was generated by QoderWork AI
|
|
||
| it('shows the toggle in the empty state for a trusted git workspace', async () => { | ||
| const { container } = renderApp(); | ||
| const { container } = renderApp({ showWorktreeToggle: true }); |
There was a problem hiding this comment.
[Minor] No test locks in the new default-hidden behavior
All seven updated tests now pass showWorktreeToggle: true, but the core contract this PR introduces — embedded consumers (prop absent/false) do not see the worktree action — has no assertion. A counterpart test would prevent the original leak from silently returning:
it('hides the toggle by default for embedded consumers', async () => {
const { container } = renderApp();
await flush();
await flush();
expect(container.querySelector(toggleSelector)).toBeNull();
});(Also flagged by @yiliang114 in the overview.)
This review was generated by QoderWork AI
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
LGTM — no Critical/Major issues found. Approving with the two Minor suggestions from my previous review (dead error plumbing cleanup, default-false worktree test) left at the author's discretion.
This review was generated by QoderWork AI
4 participants
What this PR does
This PR polishes several Web Shell interactions: tooltips use a consistent white bordered surface and no longer reopen after dismissing the workspace menu, the slash-command panel stays above the composer with a stable two-column layout and adaptive detail placement, and the Git control matches the surrounding toolbar. Artifact discovery failures are now passive instead of producing global error notifications. Embedded consumers can opt into the empty-state worktree action with
showWorktreeToggle, while the standalone shell keeps it enabled. It also completes the recently added rendered-file preview wiring for drawer and split-pane paths.Why it's needed
The affected controls had inconsistent hover styling, unstable overlay placement on narrow screens, repeated passive artifact errors, and standalone-only worktree behavior leaking into embedded integrations. The rendered-file preview follow-up prevents missing callbacks in the mobile drawer and keeps workspace-scoped actions attached only to compatible panel requests.
Reviewer Test Plan
How to verify
showWorktreeToggleand confirm the action is hidden.Evidence (Before & After)
Before: tooltip arrows and toolbar hover states were inconsistent, the slash menu could flip below the composer or squeeze its detail card, passive artifact refresh failures surfaced errors, and embedded shells exposed the worktree action by default. After: overlays have stable placement and styling, passive artifact loads fail silently, worktree isolation is opt-in for embedded consumers, and rendered-file preview paths are fully wired.
Tested on
Environment (optional)
Validated with package lint, TypeScript type checking, and production builds for WebUI and Web Shell. UI automation was not run.
Risk & Scope
showWorktreeToggleis optional and defaults to false for embedded consumers, while the standalone entry explicitly enables it.Linked Issues
N/A
中文说明
本 PR 做了什么
本 PR 优化了多处 Web Shell 交互：Tooltip 统一为白底描边样式，关闭 workspace 菜单后不会自动重新弹出；斜杠命令面板固定在输入框上方，使用稳定的左右两列布局，并根据可用空间放置详情；Git 控件的样式与周围工具栏保持一致。Artifacts 被动加载失败不再产生全局错误通知。嵌入方可以通过
showWorktreeToggle选择是否展示空状态 Worktree 入口，独立 Web Shell 则继续启用。PR 同时补齐了最近新增的文件渲染预览在 Drawer 和分屏路径中的接线。为什么需要
相关控件此前存在 hover 样式不一致、窄屏浮层位置不稳定、被动 artifacts 请求反复报错，以及独立模式的 Worktree 功能默认泄漏到嵌入接入等问题。文件预览的补充修复可以避免移动端 Drawer 缺少回调，并确保 workspace actions 只附加到兼容的面板请求。
Reviewer 测试计划
验证方式
showWorktreeToggle，确认该入口隐藏。前后对比证据
修改前：Tooltip 箭头和工具栏 hover 样式不一致，斜杠菜单可能翻转到输入框下方或挤压详情卡片，被动 artifacts 刷新失败会展示错误，嵌入组件默认出现 Worktree 入口。修改后：浮层定位与样式稳定，被动 artifacts 加载静默失败，嵌入方需显式开启 Worktree 隔离，文件渲染预览路径完整接通。
已测试平台
环境
已通过 WebUI 和 Web Shell 的包级 lint、TypeScript 类型检查及生产构建。未运行 UI 自动化测试。
风险与范围
showWorktreeToggle为可选参数，嵌入方默认 false，独立入口会显式开启。关联 Issue
N/A