fix(web-shell): keep git mode popover open when picking branch/worktree - #7668
Conversation
Clicking the "New branch" or "Worktree" option dismissed the popover
instead of revealing the branch-name input / confirm button. The option
click bubbles as a React synthetic event through the portal up to the
composer surface onClick, which calls core.focus() and moves focus
outside the popover — tripping the Radix focus-outside dismissal (the
popover only guarded the pointer path via onInteractOutside).
Stop click propagation on the popover content, matching the composer
ToolbarPopover pattern in ChatEditor. Also fix the e2e selectors
(getByText('New branch') matched both the name and description spans)
and add a delayed still-open assertion so the flash-then-dismiss
regression cannot false-pass.
Review — fix(web-shell): keep git mode popover open when picking branch/worktreeVerdict: LGTM / merge-ready. Minimal, well-reasoned fix that mirrors an existing proven pattern in the same file. I traced every claim in the PR description against the code and it holds up. What it doesThe git-mode popover is portaled out of the composer, but a React synthetic click inside it still bubbles through the React tree (not the DOM tree) to the composer surface's Correctness — verified
Tests — good
Optional (non-blocking)
RiskLow. UI event-propagation only; no server, security, or perf implications. Windows/Linux left to CI (noted by author). 中文说明结论:可以合并。 改动最小、思路清晰,且复用了同文件内已验证过的既有模式。我把 PR 描述里的每条论断都对照代码核实过,均成立。 做了什么:git-mode 弹层通过 portal 渲染到 composer 之外,但弹层内的 React 合成点击仍会沿 React 树(而非 DOM 树)冒泡到 composer 表面容器的 正确性(已核实):
测试: 可选(非阻塞):这已是第 2+ 个 portaled 弹层靠 风险:低。仅 UI 事件传播,无服务端/安全/性能影响。Windows/Linux 交给 CI(作者已说明)。 |
ytahdn
left a comment
There was a problem hiding this comment.
LGTM. PopoverContent 添加 onClick stopPropagation 阻止 click 冒泡到 composer surface 抢焦点,与 ToolbarPopover 现有模式一致。E2E 回归守卫(300ms wait + visibility assert)+ getByRole 精确匹配。
— qwen3.7-plus via Qwen Code /review
Review 总结变更概述修复 git mode popover 点击选项(branch/worktree)后被意外关闭的问题。2 文件,+27/-8。 代码质量评估
结论无 Critical 缺陷,修复最小且与现有模式一致。可以合并。👍 |
🖼️ web-shell visual previewRendered against a mock daemon (no real backend): the PR base vs this PR head Screenshots · before / afterℹ️ No screenshot changed against the PR base — but this PR edits 1 render-shaping file:
Either the change has no visual effect (logic, plumbing, a state the scenarios never reach), or no scenario renders this UI — in which case the preview cannot see it, and an empty result is a coverage gap rather than a clean bill of health. To make it visible, add a scenario to Full-resolution recordings (.webm) are attached to the workflow run. — Qwen Code · web-shell visuals |
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. |
|
Thanks for the PR! Template looks good ✓ Problem: observed bug — clicking "New branch" or "Worktree" in the git mode popover dismisses the popover instead of revealing the follow-up UI. Reported directly by a maintainer, with a clear mechanism (React synthetic click bubbles through the portal to the composer surface, whose Direction: aligned — this is a straightforward bug fix for a broken interaction in the web shell's git mode popover. No auth, sandbox, model selection, telemetry, or public contract concerns. Size: not applicable — no core paths touched ( Approach: the scope feels exactly right. One production line ( Moving on to code review. 🔍 中文说明感谢贡献! 模板完整 ✓ 问题:已观测到的 bug——点击 git 模式弹层中的"新建分支"或"Worktree 隔离"选项时,弹层会直接关闭而不是展开后续 UI。由 maintainer 直接反馈,机制清晰(React 合成事件沿 portal 冒泡到 composer 表面容器,其 方向:对齐——这是 web shell git 模式弹层交互 bug 的直接修复,不涉及 auth、sandbox、模型选择、遥测或公共契约。 规模:不适用——未触及核心路径( 方案:范围恰好。一行生产代码(在 进入代码审查 🔍 — Qwen Code · qwen3.8-max-preview Reviewed at |
Code ReviewThe fix is one line: The test changes are solid: the No critical blockers. No convention violations. Nothing to flag. CI Test Evidence
The main unit/integration test suite passed. The web-shell E2E smoke test was still in progress at review time (~10 min poll); the PR author reports the git-mode Playwright spec passes locally (5/5, verified both directions). Not verified: Windows/Linux local runs (author noted 中文说明代码审查修复只有一行:在 测试改动扎实:原来的 无关键阻塞项,无规范违反,无需标记的问题。 CI 测试证据
主单元/集成测试套件通过。web-shell E2E 冒烟测试在审查时仍在运行(轮询约 10 分钟);PR 作者报告 git-mode Playwright 用例本地全部通过(5/5,双向验证)。 未验证:Windows/Linux 本地运行(作者标注 — Qwen Code · qwen3.8-max-preview Reviewed at |
|
Confidence: 5/5 — clean, minimal fix that mirrors an established pattern; would merge without hesitation. This is exactly the kind of PR that's easy to review and easy to maintain. One production line that applies the same 中文说明置信度:5/5 —— 干净、最小化的修复,与已有模式一致;毫不犹豫地合并。 这正是那种容易审查、容易维护的 PR。一行生产代码应用了其它工具栏弹层已在使用的 — Qwen Code · qwen3.8-max-preview Reviewed at |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
✅ Local verification — real build + Playwright run (merge reference)Checked out to an isolated worktree at the PR head ( 1 · PR head — all git-mode E2E pass
2 · Load-bearing check — revert only the one-line fixTo prove the fix (not just the tests) does the work, I reverted only the
The bug reproduces the instant the fix is removed, and the guards discriminate fix-from-bug → the fix is load-bearing and the regression guards are meaningful, not tautological. 3 · Before / after — the user-visible bugClicking New branch in the git-mode popover:
VerdictReproduced + verified — merge-ready. The fix is minimal, mirrors the existing Environment: macOS · Chromium · Playwright 1.61 · web-shell Vite dev server against the mock daemon. Two runs (fix present / fix reverted) differ only in the single 🇨🇳 中文版本✅ 本地验证 —— 真实构建 + Playwright 运行(合并参考)把 PR head( 1 · PR head —— git-mode E2E 全绿
2 · 承重性验证 —— 只回退那一行修复为证明起作用的是修复本身(而非测试本身),我只回退了
一旦移除修复,bug 立即复现,且守卫能区分"修复态/bug 态" → 修复是承重的,回归守卫是有意义的,不是自证式的空测试。 3 · 修复前 / 后 —— 用户可见的 bug点击 git-mode 弹层里的 New branch(见上方前后对比截图):
结论已复现 + 已验证 —— 可以合并。 修复最小、复用了同一 composer 内既有的 环境:macOS · Chromium · Playwright 1.61 · web-shell Vite dev server + mock daemon。两次运行(有修复 / 回退修复)仅差那一行 |
Scope the New branch option to the popover locator and settle past the ~100ms dismissal window before re-asserting the popover and input stay visible, so a regression of #7668 hard-fails here instead of only producing a wrong (visually reviewed) screenshot. Mirrors the proven guard in web-shell.git-mode.spec.ts.
…uals suite (QwenLM#7672) * test(web-shell): capture the git-mode new-branch sub-state in the visuals suite The `git mode selector` visuals scenario captured the composer chip and the opened three-mode popover, but stopped there: selecting an option used to dismiss the popover ~100ms later (the click bubbled through the React tree out of the portaled content to the composer surface's onClick → core.focus() → Radix focus-outside close), so the branch-name sub-state couldn't be shot stably. QwenLM#7668 fixed that dismissal, so it now can. Extend the scenario to click "New branch", fill a valid branch name, and capture the revealed input (validated) with its Create-branch affordance, in both themes. All six git-mode captures are byte-stable across runs (0% pixel diff). Match the option by role — its label is split across a name and a description span, so getByText('New branch') is ambiguous (also fixed in QwenLM#7668). Beyond covering a state the preview never showed, this doubles as a visual regression guard for QwenLM#7668: if the popover ever dismisses on option-click again, the input goes missing and the assertion fails here, not only in the screenshot. * test(web-shell): strengthen git-mode branch assertion and trim comment (QwenLM#7672) * test(web-shell): harden git-mode branch capture into a real QwenLM#7668 guard Scope the New branch option to the popover locator and settle past the ~100ms dismissal window before re-asserting the popover and input stay visible, so a regression of QwenLM#7668 hard-fails here instead of only producing a wrong (visually reviewed) screenshot. Mirrors the proven guard in web-shell.git-mode.spec.ts. --------- Co-authored-by: wenshao <wenshao@example.com> Co-authored-by: qwen-code-ci-bot <qwen-code-ci-bot@users.noreply.github.com>


What this PR does
On the new-conversation screen, clicking the "New branch" or "Worktree" option in the git mode popover dismissed the popover instead of revealing the branch-name input or the worktree confirm button, so the git mode could never be switched away from the current branch. This makes those option clicks keep the popover open so the follow-up UI shows and the selection can be confirmed.
Why it's needed
The git mode popover renders its content through a portal, but an option click still bubbles through the React tree up to the composer surface, whose click handler moves focus into the editor. That focus jump lands outside the popover and trips Radix's focus-outside dismissal, closing the popover about 100ms after the option's own UI flashes visible — so the branch input / confirm button appears for an instant and then vanishes, and the chip stays on the current branch. The popover already guarded the pointer-outside path (
onInteractOutside) but not this focus-outside path. The other composer toolbar popovers already avoid this by stopping click propagation; this applies the same treatment to the git mode popover.Reviewer Test Plan
How to verify
Run the web shell against a daemon bound to a trusted git workspace and open the new-conversation screen. Click the branch chip in the composer toolbar to open the git mode popover. Click "New branch": the branch-name input should appear and stay open (previously the popover closed); type a name and confirm, and the chip switches to the new branch. Click "Worktree": the "Create worktree" confirm button should appear and stay open; confirm, and the chip switches to worktree mode. "Current branch" and clicking outside still dismiss the popover as before.
E2E:
cd packages/web-shell && npx playwright test --config playwright.config.ts web-shell.git-mode.spec.ts --project=chromium— 5 pass. The branch/worktree tests now wait 300ms and assert the popover is still open; with the fix reverted they fail at that assertion (verified both directions).Evidence (Before & After)
Before: clicking "New branch" / "Worktree" flashes the input / confirm for ~100ms, then the popover closes and the chip stays on the current branch. After: the input / confirm stays visible and the selection can be confirmed, switching the chip to the new branch / worktree mode.
Tested on
Environment (optional)
npm run devandqwen serveagainst a local daemon with a trusted git workspace; Playwright e2e (chromium).Risk & Scope
Linked Issues
Reported directly by a maintainer; no tracking issue.
中文说明
这个 PR 做了什么
在新建对话页面,点击 git 模式弹层里的“新建分支”或“Worktree 隔离”选项时,弹层会直接关闭,而不是展开分支名输入框或 Worktree 确认按钮,导致 git 模式永远无法从当前分支切换出去。本 PR 让这些选项的点击保持弹层打开,从而显示后续 UI 并完成确认。
为什么需要
git 模式弹层的内容通过 portal 渲染,但选项的点击仍会沿 React 树冒泡到 composer 表面容器,而该容器的点击处理会把焦点移回编辑器。这次焦点跳转落到弹层之外,触发了 Radix 的 focus-outside 关闭逻辑——大约在选项自身 UI 闪现 100ms 后弹层就被关闭,于是分支输入框/确认按钮只出现一瞬间就消失,chip 仍停在当前分支。该弹层原本只防了 pointer-outside 路径(
onInteractOutside),没防这条 focus-outside 路径。composer 里其它工具栏弹层早已通过阻止点击冒泡规避了这个问题;本 PR 对 git 模式弹层做同样处理。评审测试计划
如何验证
把 web shell 连接到一个绑定了可信 git 仓库的 daemon,打开新建对话页面。点击 composer 工具栏里的分支 chip 打开 git 模式弹层。点“新建分支”:分支名输入框应出现并保持打开(此前弹层会关闭);输入名称并确认,chip 切换到新分支。点“Worktree 隔离”:“创建 Worktree”确认按钮应出现并保持打开;确认后 chip 切换到 worktree 模式。“当前分支”以及点击外部仍像以前一样关闭弹层。
E2E:
cd packages/web-shell && npx playwright test --config playwright.config.ts web-shell.git-mode.spec.ts --project=chromium—— 5 个通过。branch/worktree 用例现在会等待 300ms 并断言弹层仍然打开;撤掉修复后会在该断言处失败(已双向验证)。证据(修改前后)
修改前:点击“新建分支”/“Worktree 隔离”会让输入框/确认按钮闪现约 100ms,随后弹层关闭,chip 停在当前分支。修改后:输入框/确认按钮保持可见,可以完成确认,chip 切换到新分支 / worktree 模式。
测试环境
运行环境(可选)
npm run dev和qwen serve,连接带可信 git 仓库的本地 daemon;Playwright e2e(chromium)。风险与范围
关联 Issue
由 maintainer 直接反馈,无对应 issue。