fix(web-shell): dedupe restored images and harden the sidebar shortcut handler - #7169
Conversation
…t handler Post-merge review follow-ups from QwenLM#7134 and QwenLM#7135: - restoreImages now runs only alongside an actual text change: it appends to the pasted-image list, so a deduplicated restore of the same prompt (reconnect/retry) previously doubled the attachments while the text correctly stayed single. - The Cmd+B/Ctrl+B handler no longer toggles the sidebar while a non-composer editable element is focused (sidebar search, session rename, settings inputs) — matching the codebase's isEditableTarget convention; the composer keeps the editor-convention behavior and still toggles, verified in a real browser. - All state updates moved out of React updater functions (purity contract, mirroring the hamburger handler); the effect re-binds on the involved state so the listener closure stays fresh. - A drawer forced open on a wide viewport now takes the drawer path: the shortcut closes the overlay instead of collapsing the rail invisibly underneath it. Refs QwenLM#7102 QwenLM#7128 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
🖼️ web-shell visual previewRendered against a mock daemon (no real backend): the PR base vs this PR head Screenshots · before / after✅ No screenshot changes against the PR base. Full-resolution recordings (.webm) are attached to the workflow run. — Qwen Code · web-shell visuals |
|
Thanks for the PR! Template looks good ✓ — all required sections present, bilingual, with Playwright evidence in the "Before & After" slot. Problem: All four issues are observable directly from the code, not theoretical:
No live reproduction needed — the bugs are legible from the code structure. Direction: Aligned. These are follow-ups to already-merged PRs (#7134, #7135) addressing automated-review suggestions that were left as non-blocking but represent real correctness gaps. Closing the loop on merged-PR review debt is good hygiene. Size: 46 additions + 14 deletions across 2 web-shell files. No core modules touched — not applicable. Approach: Scope feels right — four small, focused fixes in two files, no unrelated churn. The dedup fix (moving Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ — 所有必填章节齐全，双语，"Before & After" 中有 Playwright 证据。 问题： 四个问题均可从代码直接观测，非理论性：
无需实时复现——bug 从代码结构即可看清。 方向： 对齐。这是对已合并 PR（#7134、#7135）自动评审遗留建议的跟进，属于真实正确性缺口。关闭已合并 PR 的评审债是良好习惯。 规模： 46 行新增 + 14 行删除，2 个 web-shell 文件，未触及核心模块——不适用。 方案： 范围合理——两个文件中四个小而聚焦的修复，无无关改动。去重修复（将 进入代码审查 🔍 — Qwen Code · qwen3.7-max Reviewed at |
Code reviewIndependent proposal before reading the diff: move The PR matches this proposal exactly. Two files, 60 lines — nothing extraneous. No critical issues found. Specifically:
Reuse check: No new utilities or abstractions added — the PR reuses TestingUnit tests (verified locally)Browser E2E (author-provided, Playwright Chromium)tmux noteThis is a web-shell (browser-rendered React) change, not a CLI TUI change — tmux 中文说明代码审查读 diff 前的独立方案：将 PR 与方案完全一致。两个文件，60 行——无多余内容。 未发现关键问题：
复用检查： 未新增工具函数或抽象——复用 测试单元测试（本地验证）浏览器 E2E（作者提供，Playwright Chromium）tmux 说明此为 web-shell（浏览器渲染 React）变更，非 CLI TUI 变更——tmux — Qwen Code · qwen3.7-max Reviewed at |
|
Confidence: 4/5 — Clean, focused follow-up PR that fixes four observable correctness gaps in two files; minor reservation is that the image-restore branch lacks direct unit test coverage (acknowledged by the author). My independent proposal before reading the diff was identical to the PR's approach — dedup inside the text-change branch, editable-target guard with a composer carve-out, sequential state dispatch outside updaters, and What I liked: the scope is tight — four fixes, two files, no drive-by refactors or scope creep. Each change reuses an existing utility or attribute ( Minor reservation: the image-restore branch inside Ship it. ✅ 中文说明信心度：4/5 — 简洁、聚焦的跟进 PR，在两个文件中修复了四个可观测的正确性缺口；小保留意见是图片恢复分支缺乏直接单测覆盖（作者已承认）。 读 diff 前的独立方案与 PR 方案一致——文本变更分支内去重、编辑目标守卫配合 composer 例外、updater 外顺序状态派发、视口宽度检查前先检查 优点：范围紧凑——四个修复，两个文件，无顺手重构或范围蔓延。每个改动复用既有工具或属性（ 小保留意见： 可以合并 ✅ — 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.
LGTM, looks ready to ship. ✅
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.
— qwen3.7-max via Qwen Code /review
| } else { | ||
| setMobileDrawerOpen(true); | ||
| } |
There was a problem hiding this comment.
[Suggestion] The keyboard shortcut's drawer-open branch does not clear forceMobileDrawer, while the hamburger handler (line 6434) always calls setForceMobileDrawer(false) before toggling. — Concrete cost: today the state forceMobileDrawer === true && mobileDrawerOpen === false is unreachable, but a future change that independently closes the drawer without clearing forceMobileDrawer would leave the keyboard shortcut stuck in forced-drawer mode on wide viewports. The hamburger would handle that state correctly; the keyboard shortcut would not.
| } else { | |
| setMobileDrawerOpen(true); | |
| } | |
| } else { | |
| setForceMobileDrawer(false); | |
| setMobileDrawerOpen(true); | |
| } |
— qwen3.7-max via Qwen Code /review
| if (images && images.length > 0) { | ||
| editorRef.current?.restoreImages(images); | ||
| } |
There was a problem hiding this comment.
[Suggestion] No test verifies that restoreTextToEditor skips restoreImages when the text was already restored (the deduplicated path). — Failure scenario: a future refactor moves restoreImages back outside the next !== current guard. Because restoreImages appends (setPastedImages((prev) => [...prev, ...images])), a prompt with images restored twice across a reconnect would double the image attachments while the text correctly stays deduplicated — reintroducing the #7134 bug for images.
Suggested fix: add a test that calls restoreTextToEditor twice with the same text+images and asserts the mock editor's restoreImages is called only once.
— qwen3.7-max via Qwen Code /review
| if ( | ||
| isEditableTarget(target) && | ||
| !target?.closest('[data-web-shell-composer-editor]') | ||
| ) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
[Suggestion] The Cmd+B keyboard shortcut handler has no test coverage for its three new branches — editable-target exemption, forced-drawer close on wide viewport, and direct state reads replacing updater functions. — Failure scenario: a future change to the effect's dependency array or closure structure could regress the editable-target guard (Cmd+B toggles the sidebar while the user is typing in the sidebar search field) or the forced-drawer path without any test catching it.
Suggested fix: add a test that fires a Cmd+B keydown on window and asserts: (a) the sidebar does not toggle when focus is in an <input>, (b) a forced drawer on a wide viewport closes, and (c) the drawer opens/closes on a narrow viewport.
— qwen3.7-max via Qwen Code /review
✅ Local verification — real end-to-end tests (maintainer)Built and exercised this PR locally at head 1) Restored images dedupe with the text —
|
| Scenario | restoreImages calls |
Editor text |
|---|---|---|
| Same prompt restored twice (reconnect/retry) | 1 ✓ | single copy ✓ |
| Two different prompts restored | 2 ✓ | prepended, both kept ✓ |
- Fix confirmed load-bearing: moving the
restoreImagescall back outside the text-change guard (the pre-fix(web-shell): dedupe restored images and harden the sidebar shortcut handler #7169 position) makes the same test fail with 2 calls on the duplicate restore — i.e. the doubled attachments the PR describes. Restoring the guard → green again. - The "two different prompts" case proves this is a dedup, not a blanket suppression: images still flow whenever the text genuinely changes.
2) Sidebar Cmd/Ctrl+B hardening — App.tsx
Playwright (Chromium) against the real App (dark, sidebar enabled, StrictMode) on a wide 1280×860 viewport with a live mock daemon. Sidebar rail = 260px expanded / 56px collapsed; forced drawer = [data-sidebar-shell][role="dialog"].
| Scenario | Expected | Result |
|---|---|---|
| Nothing focused → Ctrl+B | toggles 260 ↔ 56 | ✅ |
| Composer focused → Ctrl+B | still toggles 260 → 56 (editor convention kept), draft untouched | ✅ |
| Sidebar search focused → Ctrl+B | guarded — stays 260, query preserved | ✅ |
| Forced drawer (wide) → Ctrl+B | drawer closes, rail stays expanded (no silent collapse) | ✅ |
Load-bearing controlled experiments (each isolates one changed line):
- Neuter the
isEditableTargetguard → the search-input case collapses to 56 (regresses). ✅ guard is doing the work. - Drop
forceMobileDrawer ||from the drawer condition → Ctrl+B leaves the drawer open while collapsing the rail underneath — the pre-fix visual no-op. ✅ branch is doing the work.
3) Regression gates
- Cited suites:
mergeRestoredPromptText(6) +sidebarToggleShortcut(5) = 11/11 ✓ - Full web-shell unit suite: 117/117 files, 1889 tests green (built the lib bundle so
build-artifact.test.tsreads a real artifact; its only failures without adist/areENOENT, PR-independent). - Builds:
vite build(app) +vite build(lib) both clean. - ESLint clean on both changed files; Prettier clean on both changed files.
tsc --noEmit: 0 errors in the changed files. The 23 pre-existing errors are all in unrelated files (GitBranchIndicator,GitDiffDialog,GoalsDialog) — SDK/webuidistdrift in my symlinked verification worktree, untouched by this PR.
Verdict
LGTM — ready to merge. All four review follow-ups behave as described, each backed by a real-flow test and a load-bearing revert; no regressions in the package's unit suite, build, lint, format, or typecheck.
Env: macOS (Darwin 24.6), Node v22.23.1, Playwright Chromium. Screenshots are Playwright captures of the real app in these scenarios.
中文版本（点击展开）
✅ 本地验证 —— 真实端到端测试（维护者）
在 head e641e9c6（基线 af6a9b64，已含 #7134 + #7135）本地构建并验证。两条 follow-up 都通过真实的生产代码路径驱动（而非仅测提取出的辅助函数），且每处修复都用「只回退被改的那一行、观察测试变红」的对照实验确认其确实起作用。
1）恢复图片随文本一起去重 —— useQueuedPrompts.ts
真实 hook 级 E2E（jsdom + createRoot/act）驱动真正的 useQueuedPrompts，配一个有状态的 mock 编辑器；提交失败会走本 PR 加固的 restoreTextToEditor(text, images) 路径。
| 场景 | restoreImages 调用次数 |
编辑器文本 |
|---|---|---|
| 同一 prompt 恢复两次（重连/重试） | 1 ✓ | 单份 ✓ |
| 两个不同 prompt 恢复 | 2 ✓ | 前置追加，均保留 ✓ |
- 修复确属关键： 把
restoreImages移回文本变更守卫之外（fix(web-shell): dedupe restored images and harden the sidebar shortcut handler #7169 之前的位置），同一测试在重复恢复时变为 2 次调用 —— 即 PR 描述的附件翻倍。恢复守卫后重新变绿。 - 「两个不同 prompt」用例证明这是去重而非一刀切屏蔽：只要文本真的变化，图片仍会恢复。
2）侧栏 Cmd/Ctrl+B 加固 —— App.tsx
Playwright（Chromium）对真实 App（暗色、启用侧栏、StrictMode），宽视口 1280×860，配真实 mock daemon。侧栏轨道展开 260px / 折叠 56px；强制抽屉 = [data-sidebar-shell][role="dialog"]。
| 场景 | 期望 | 结果 |
|---|---|---|
| 无聚焦 → Ctrl+B | 260 ↔ 56 切换 | ✅ |
| composer 聚焦 → Ctrl+B | 仍切换 260 → 56（保留编辑器惯例），草稿不动 | ✅ |
| 侧栏搜索框聚焦 → Ctrl+B | 被守卫 —— 保持 260，查询词保留 | ✅ |
| 强制抽屉（宽视口）→ Ctrl+B | 抽屉关闭，轨道保持展开（不在其下悄悄折叠） | ✅ |
对照实验（每个只隔离一处改动）：
- 使
isEditableTarget守卫失效 → 搜索框场景折叠到 56（回归）。✅ 守卫确实生效。 - 从抽屉条件里去掉
forceMobileDrawer ||→ Ctrl+B 让抽屉保持打开、轨道在其下折叠 —— 即修复前的视觉无操作。✅ 该分支确实生效。
（截图见上方英文版。）
3）回归门禁
- PR 引用的套件：
mergeRestoredPromptText（6）+sidebarToggleShortcut（5）= 11/11 ✓ - web-shell 全量单测： 117/117 文件、1889 测试通过（先构建 lib bundle，使
build-artifact.test.ts读到真实产物；无dist/时它仅报ENOENT，与本 PR 无关）。 - 构建：
vite build（app）+vite build（lib）均通过。 - ESLint 对两个改动文件干净；Prettier 对两个改动文件干净。
tsc --noEmit： 改动文件 0 错误。那 23 个既有错误全在无关文件（GitBranchIndicator、GitDiffDialog、GoalsDialog）—— 我这套 symlink 验证工作树里的 SDK/webuidist漂移，本 PR 未触及。
结论
LGTM —— 可合并。 四条评审 follow-up 行为均符合描述，各有真实流程测试 + 载荷回退佐证；单测、构建、lint、格式化、类型检查均无回归。
环境：macOS（Darwin 24.6）、Node v22.23.1、Playwright Chromium。截图为真实应用在各场景下的 Playwright 抓图。
3 participants
What this PR does
Applies the four post-merge automated-review follow-ups left on #7134 and #7135. Restored prompt images are now deduplicated together with the text:
restoreImagesappends to the pasted-image list, so a second restore of the same prompt (reconnect/retry) previously doubled the attachments while the text correctly stayed single. The Cmd+B/Ctrl+B sidebar shortcut is hardened three ways: it no longer fires while a non-composer editable element is focused (sidebar search, session rename, settings inputs — the composer intentionally keeps the editor-convention behavior and still toggles); all state updates are dispatched sequentially outside React updater functions (purity contract, mirroring the existing hamburger handler), with the effect re-binding on the involved state so the listener closure stays fresh; and a drawer forced open on a wide viewport now takes the drawer path, closing the overlay instead of collapsing the rail invisibly underneath it.Why it's needed
Both PRs merged with non-blocking inline suggestions from the automated review that are real correctness gaps: doubled image attachments on duplicate restores produce an inconsistent editor state exactly in the failure-recovery flow #7128 was about; a shortcut firing inside the sidebar's own search or rename inputs yanks the sidebar away mid-typing; side effects inside updater functions violate React's purity contract and can misbehave under strict/concurrent double-invocation; and the forced-drawer branch made the shortcut look like a no-op on wide viewports.
Reviewer Test Plan
How to verify
Automated coverage: the guard branch semantics are pinned by the existing
mergeRestoredPromptTextsuite (6 tests) and matcher suite (5 tests) — 11/11 pass; the image call now lives inside the already-tested text-change branch.npm run typecheck, the web-shell package build, eslint and prettier are clean.Evidence (Before & After)
Real-browser E2E (Playwright Chromium against a live
qwen servedaemon built from this branch):Before this change the search-input case toggled the sidebar mid-typing; the composer behavior is unchanged from #7135's original verification.
Tested on
Environment (optional)
macOS (Darwin 24.6), Node v22.23.1; Playwright Chromium against a live
qwen servedaemon, plus vitest unit tests.Risk & Scope
mergeRestoredPromptTextsuite and the behavior was reasoned through the same dedup semantics. The forced-drawer scenario was implemented per the review finding and manually reasoned; automation covers the desktop paths.Linked Issues
Refs #7102, #7128 (follow-ups to the reviews on #7134 and #7135)
中文说明
本 PR 做了什么
落实 #7134 与 #7135 合并后自动评审留下的四条 follow-up。恢复失败 prompt 时图片随文本一起去重：
restoreImages是向粘贴图片列表追加，同一 prompt 的第二次恢复（重连/重试）此前会让附件翻倍而文本保持单份。Cmd+B/Ctrl+B 侧栏快捷键三处加固：非 composer 的可编辑元素聚焦时不再触发（侧栏搜索、会话重命名、设置输入框——composer 有意保留编辑器惯例仍可切换）；全部状态更新移出 React updater 函数（纯度契约，对齐既有汉堡按钮 handler），effect 依赖相关状态重绑定保证闭包新鲜；宽视口下被强制打开的抽屉走抽屉路径，快捷键关闭覆盖层而不是在其下面不可见地折叠侧栏。为什么需要
两个 PR 合并时带着自动评审的非阻塞行内建议，但都是真实的正确性缺口：重复恢复时图片翻倍恰好发生在 #7128 关注的失败恢复流程里；快捷键在侧栏自己的搜索/重命名输入框里触发会在输入途中拽走侧栏；updater 内副作用违反 React 纯度契约、strict/并发双调用下可能不一致；强制抽屉分支让快捷键在宽视口看起来像无操作。
审阅测试计划
如何验证
自动化覆盖：守卫分支语义由既有
mergeRestoredPromptText（6 例）与匹配器（5 例）套件固定——11/11 通过；图片调用现位于已被测试的文本变更分支内。typecheck / web-shell 构建 / eslint / prettier 全绿。证据（Before & After）
真实浏览器 E2E（Playwright Chromium 对本分支构建的真实
qwen serve）：composer 聚焦 Ctrl+B 仍切换（260→56）；搜索框聚焦 Ctrl+B 被守卫（保持 260）。改动前搜索框场景会在输入途中切换侧栏；composer 行为与 #7135 原验证一致。测试平台
macOS 已本地验证（✅）；Windows / Linux 依赖 CI（⚠️ ）。
环境
macOS（Darwin 24.6）、Node v22.23.1；Playwright Chromium + 真实 daemon + vitest 单测。
风险与范围
mergeRestoredPromptText套件覆盖。强制抽屉场景按 review 发现实现并人工推演；自动化覆盖桌面路径。关联 Issue
Refs #7102, #7128（#7134 与 #7135 评审的 follow-up）
🤖 Generated with Claude Code