Skip to content

fix(web-shell): avoid duplicate inline tag tooltips - #6729

Merged
wenshao merged 5 commits into
QwenLM:mainfrom
dreamWB:codex/webshell-tooltip-dedup
Jul 12, 2026
Merged

fix(web-shell): avoid duplicate inline tag tooltips#6729
wenshao merged 5 commits into
QwenLM:mainfrom
dreamWB:codex/webshell-tooltip-dedup

Conversation

@dreamWB

@dreamWB dreamWB commented Jul 11, 2026

Copy link
Copy Markdown
Collaborator

What this PR does

Makes the native title tooltip a fallback for inline composer tags instead of rendering it alongside a host-provided custom tooltip. Custom tooltips now receive a unique id and are associated with the tag through aria-describedby.

Adds DOM regression coverage for the string-tooltip path introduced in #6578.

Why it's needed

When a WebShell host returns a string from renderComposerTagTooltip, the value is used for both the native browser tooltip and the custom React tooltip. Hovering an inline tag can therefore display two overlapping tooltip surfaces with the same content. The follow-up also preserves an accessible description after the duplicate native title is removed.

Reviewer Test Plan

How to verify

  1. Configure an inline composer tag and return a string such as Details from renderComposerTagTooltip.
  2. Hover or focus the rendered tag and confirm that only the custom tooltip is present; the tag should not have a native title attribute.
  3. Confirm that the tooltip has a unique id and that the tag's aria-describedby points to it.
  4. Run QWEN_CODE_LANG=en npm run test:ci --workspace @qwen-code/web-shell; all 87 test files and 1,384 tests should pass.

Evidence (Before & After)

Before: a string tooltip rendered both the browser-native title tooltip and the custom WebShell tooltip, which could overlap. After: only the custom tooltip renders, and it is referenced by aria-describedby. This is covered by the focused DOM regression test; no screenshot was captured because plugin/browser integration validation is intentionally outside this follow-up's scope.

Tested on

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

Environment (optional)

Node.js v22.17.1, npm 10.9.2. WebShell package tests, formatting, lint, build, typecheck, and the serve fast-path bundle closure check were run locally. Root test:ci also exposed unrelated failures in untouched CLI tests on current main: one locale-sensitive extension-list assertion (passes with QWEN_CODE_LANG=en) and 12 AuthDialog navigation assertions.

Risk & Scope

  • Main risk or tradeoff: Tooltip ids use a module-local monotonic counter; the id is only used to associate each live tooltip with its tag.
  • Not validated / out of scope: Plugin/browser integration, non-inline message tooltip behavior, and the other suggestion-level comments on feat(web-shell): add context mention customization #6578.
  • Breaking changes / migration notes: None. The existing host customization API and native fallback behavior are preserved.

Linked Issues

Fixes #6728

中文说明

本 PR 做了什么

将行内输入标签的原生 title 提示改为兜底逻辑,不再与宿主提供的自定义提示同时渲染。自定义提示现在会获得唯一 id,并通过 aria-describedby 与标签建立关联。

同时为 #6578 引入的字符串提示路径补充了 DOM 回归测试。

为什么需要这个改动

当 WebShell 宿主从 renderComposerTagTooltip 返回字符串时,该值此前会同时用于浏览器原生提示和自定义 React 提示。因此,悬停行内标签时可能出现两个内容相同且相互重叠的提示。本次修复在移除重复原生 title 的同时,也保留了无障碍描述关系。

Reviewer Test Plan

如何验证

  1. 配置一个行内输入标签,并让 renderComposerTagTooltip 返回类似 Details 的字符串。
  2. 悬停或聚焦该标签,确认只存在自定义提示,且标签没有原生 title 属性。
  3. 确认提示拥有唯一 id,标签的 aria-describedby 指向该 id。
  4. 运行 QWEN_CODE_LANG=en npm run test:ci --workspace @qwen-code/web-shell,应通过全部 87 个测试文件、1,384 个测试。

证据(修复前后)

修复前:字符串提示会同时渲染浏览器原生 title 提示和 WebShell 自定义提示,两者可能重叠。修复后:只渲染自定义提示,并由 aria-describedby 引用。本行为已有聚焦的 DOM 回归测试覆盖;由于本次跟进明确不包含插件和浏览器集成验证,因此未录制截图。

已测试平台

OS 状态
🍏 macOS
🪟 Windows ⚠️
🐧 Linux ⚠️

环境(可选)

Node.js v22.17.1,npm 10.9.2。本地已运行 WebShell 包测试、格式化、lint、构建、类型检查以及 serve fast-path bundle 闭包检查。根目录 test:ci 还暴露了当前 main 未改动 CLI 测试中的无关失败:一个受语言环境影响的 extension-list 断言(设置 QWEN_CODE_LANG=en 后通过)以及 12 个 AuthDialog 导航断言。

风险与范围

  • 主要风险或权衡:提示 id 使用模块内单调递增计数器;该 id 仅用于关联当前存活的提示与标签。
  • 未验证或不在范围内:插件/浏览器集成、非行内消息提示行为,以及 feat(web-shell): add context mention customization #6578 上其他 Suggestion 级别评论。
  • 破坏性变更或迁移说明:无。现有宿主定制 API 和原生兜底行为保持不变。

关联 Issue

Fixes #6728

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Thanks for the PR!

Template looks good ✓

Problem: Observed bug with linked issue (#6728). When renderComposerTagTooltip returns a string, the value is used for both the native browser title tooltip and the custom React tooltip, resulting in two overlapping tooltips with the same content. Clear and well-described.

Direction: Aligned — this is a focused UI bug fix that removes duplicate tooltip surfaces and improves accessibility via aria-describedby. No concerns.

Size: 11 production lines (10 additions + 1 deletion in useComposerCore.ts), 18 test lines. Not a core module. No size concerns.

Approach: Minimal and well-scoped. Three surgical changes: (1) module-local counter for unique tooltip IDs, (2) suppress native title when a custom tooltip is rendered (keeping it as fallback otherwise), (3) set id on tooltip element and aria-describedby on chip. The regression test covers the exact scenario. Clean.

Moving on to code review. 🔍

中文说明

感谢贡献!

模板完整 ✓

问题:已观测到的 bug,已关联 issue (#6728)。当 renderComposerTagTooltip 返回字符串时,该值同时用于浏览器原生 title 提示和自定义 React 提示,导致两个重叠的提示框。描述清晰。

方向:对齐——这是一个聚焦的 UI bug 修复,移除了重复提示框并通过 aria-describedby 改善了无障碍性。无顾虑。

规模:11 行生产代码(useComposerCore.ts 中 10 行新增 + 1 行删除),18 行测试代码。非核心模块。无规模顾虑。

方案:最小且范围清晰。三处精确改动:(1) 模块级计数器生成唯一提示 ID,(2) 渲染自定义提示时抑制原生 title(否则保留作为兜底),(3) 为提示元素设置 id 并为标签设置 aria-describedby。回归测试覆盖了确切场景。干净利落。

进入代码审查 🔍

Qwen Code · qwen3.7-max

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Code Review

Clean, minimal fix. The approach — suppress native title when a custom tooltip renders, add unique IDs, wire up aria-describedby — is exactly what I'd do. No correctness issues, no security concerns, no regressions. The module-local counter is safe (browser-only, no SSR). The aria-describedby addition is a nice accessibility improvement.

One minor observation: the stderr warning about synchronous React unmount appears in the new test, but it's a pre-existing teardown pattern, not introduced by this change.

Test Results

Full web-shell suite: 87 files, 1,384 tests — all passing

 ✓ hooks/useComposerCore.dom.test.tsx (5 tests) 178ms
 ✓ App.test.tsx (65 tests) 1514ms
 ...
 Test Files  87 passed (87)
      Tests  1384 passed (1384)
   Duration  4.85s

The new test uses a custom inline tooltip without a native title validates all four assertions:

  • Tooltip text content is correct
  • No native title attribute on the parent
  • Tooltip element has a unique id
  • aria-describedby on the chip points to the tooltip id

Real-Scenario Testing

This is a web-shell DOM change (React tooltip in jsdom). No CLI command renders web-shell tooltips — tmux real-scenario testing is not applicable. The DOM regression test is the correct verification path and covers the behavior thoroughly.

中文说明

代码审查

干净、最小化的修复。方案——渲染自定义提示时抑制原生 title,添加唯一 ID,关联 aria-describedby——正是我会采取的做法。无正确性问题,无安全隐患,无回归。模块级计数器是安全的(仅浏览器端,无 SSR)。aria-describedby 的添加是一个不错的无障碍改进。

一个小观察:新测试中出现了同步 React 卸载的 stderr 警告,但这是已有的拆卸模式,非本次变更引入。

测试结果

web-shell 完整套件:87 个文件,1,384 个测试——全部通过

 ✓ hooks/useComposerCore.dom.test.tsx (5 tests) 178ms
 ✓ App.test.tsx (65 tests) 1514ms
 ...
 Test Files  87 passed (87)
      Tests  1384 passed (1384)
   Duration  4.85s

新测试 uses a custom inline tooltip without a native title 验证了全部四个断言:

  • 提示文本内容正确
  • 父元素无原生 title 属性
  • 提示元素有唯一 id
  • 标签的 aria-describedby 指向提示 id

真实场景测试

这是 web-shell DOM 变更(jsdom 中的 React 提示)。没有 CLI 命令能渲染 web-shell 提示——tmux 真实场景测试不适用。DOM 回归测试是正确的验证路径,已全面覆盖该行为。

Qwen Code · qwen3.7-max

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

This is exactly the kind of PR the gate should let through cleanly. Real bug (linked issue, clear reproduction path), minimal fix (11 production lines), good test coverage (DOM regression test validates all assertions), and an accessibility improvement as a bonus.

The approach is the same one I'd take independently: suppress native title when a custom tooltip is present, keep it as a fallback otherwise, and wire up aria-describedby for screen readers. Nothing in the diff is unnecessary — every line serves the stated goal.

1,384 tests pass with no regressions. Ready to ship. ✅

中文说明

这正是审查门应该放行的 PR。真实的 bug(已关联 issue,有清晰的复现路径),最小化的修复(11 行生产代码),良好的测试覆盖(DOM 回归测试验证了所有断言),以及额外的无障碍改进。

方案与我独立设想的完全一致:渲染自定义提示时抑制原生 title,否则保留作为兜底,并为屏幕阅读器关联 aria-describedby。diff 中没有不必要的代码——每一行都服务于既定目标。

1,384 个测试全部通过,无回归。可以合并。✅

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. ✅

@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.

Reviewed — no blockers. Suggestions are inline.

Comment thread packages/web-shell/client/hooks/useComposerCore.ts Outdated
doudouOUC
doudouOUC previously approved these changes Jul 11, 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.

No issues found. LGTM! ✅

— qwen3.7-max via Qwen Code /review

Comment thread packages/web-shell/client/hooks/useComposerCore.ts
Comment thread packages/web-shell/client/hooks/useComposerCore.ts
Comment thread packages/web-shell/client/hooks/useComposerCore.dom.test.tsx
Comment thread packages/web-shell/client/hooks/useComposerCore.dom.test.tsx

@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.

Reviewed — no blockers. Suggestions are inline.

Comment thread packages/web-shell/client/hooks/useComposerCore.ts

@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.

Reviewed — no blockers. Suggestions are inline.

Comment thread packages/web-shell/client/hooks/useComposerCore.dom.test.tsx
Comment thread packages/web-shell/client/hooks/useComposerCore.ts

@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 commented Jul 12, 2026

Copy link
Copy Markdown
Collaborator

✅ Local verification — real build & tests (maintainer merge reference)

Verified this PR end-to-end in an isolated worktree at the PR head (aaee8612d), freshly npm ci-installed, on macOS. Bottom line: the fix does exactly what it claims, the new regression tests genuinely fail on pre-fix code and pass on the fix, and all quality gates are green. 👍

Environment: macOS (darwin 24.6.0) · Node v22.23.1 · npm 10.9.8 · scope confirmed as exactly 2 files, both in packages/web-shell.

1. What the fix does (confirmed by reading the code)

When a WebShell host returns a string (e.g. 'Details') from renderComposerTagTooltip, the inline tag receives both tag.tooltip (ReactNode) and tag.tooltipText (string). On main, toDOM set chip.title = tooltipText (a native browser tooltip) and appendTooltip rendered the custom React tooltip — so hovering showed two overlapping surfaces with identical text. This PR removes the unconditional native title, keeps it only as a catch-block fallback when the custom tooltip fails to render, and links the tag to its custom tooltip via a unique id + aria-describedby (an accessibility win).

2. Test results (on PR head)

Run Result
Targeted useComposerCore.dom.test.tsx 6 passed (2 new + 4 pre-existing)
Full @qwen-code/web-shell suite 89 files · 1413 tests — all passed
tsc --noEmit (typecheck) ✅ no errors
eslint (both changed files) ✅ clean
prettier --check (both changed files) ✅ formatted

Note: the PR description cites 87 files / 1384 tests — the counts have grown to 89 / 1413 simply because main gained tests since; nothing is regressed.

3. Before / after proof (the tests really guard the fix)

To prove the two new tests aren't vacuous, I reverted only the fix's source hunks in place (keeping the two new tests) to reproduce main's pre-fix behaviour, then ran the file. (I reverted hunks in place rather than overlaying origin/main's whole file because current main refactored a neighbouring utils/composerTag module that doesn't exist at this PR's base.)

  • Pre-fix → 2 FAIL, exactly on the behaviour this PR changes:
    • uses a custom inline tooltip without a native titleexpected 'Details' to be null (the duplicate native title is present).
    • falls back to a native title when attaching an inline tooltip failsexpected 'block' to be 'none' (on main the hover listeners are attached before the render try, so an orphaned tooltip still shows on hover).
  • Post-fix (PR head) → all 6 PASS.

before/after test evidence

4. Behaviour / DOM evidence

Rendered with the verbatim cssText from ComposerTagWidget and the real web-shell dark-theme tokens. Before: native title tooltip and the custom tooltip overlap (title="Details", no aria-describedby). After: a single custom tooltip, referenced by aria-describedby="composer-tag-tooltip-1", no native title.

before/after DOM evidence

5. One observation (non-blocking)

The two new tests emit a React dev warning — "Attempted to synchronously unmount a root while React was already rendering." This is pre-existing and not introduced by this PR: ComposerTagWidget.destroy() calls this.tooltipRoot?.unmount() synchronously during CodeMirror widget teardown, and that code is identical on main. It only surfaces now because these are the first DOM tests to exercise the successful string-tooltip render path (the older tooltip tests throw, so no root is ever created). Harmless — every test passes — but a future cleanup could defer the unmount if the noise is undesirable.

Verdict

LGTM — recommend merge. ✅ Focused, correct fix with genuine regression coverage, a real accessibility improvement (aria-describedby), and no lint/type/format/test regressions.

🇨🇳 中文版本(点击展开)

✅ 本地验证 —— 真实构建与测试(供维护者合并参考)

我在 PR head(aaee8612d)对应的独立 worktree 中完成了端到端验证,全新执行 npm ci 安装,运行环境为 macOS。结论:本 PR 的修复完全符合其描述,新增的回归测试在修复前确实失败、在修复后通过,所有质量门禁均为绿色。 👍

环境: macOS(darwin 24.6.0)· Node v22.23.1 · npm 10.9.8 · 确认改动范围恰好为 2 个文件,且都位于 packages/web-shell

1. 修复内容(已通过阅读源码确认)

当 WebShell 宿主从 renderComposerTagTooltip 返回字符串(如 'Details')时,行内标签会同时得到 tag.tooltip(ReactNode) tag.tooltipText(字符串)。在 main 上,toDOM 会设置 chip.title = tooltipText(浏览器原生提示),appendTooltip 又渲染了自定义 React 提示 —— 因此悬停时会出现两个内容相同、相互重叠的提示层。本 PR 移除了无条件的原生 title,仅在自定义提示渲染失败时把它作为 catch 兜底保留,并通过唯一 id + aria-describedby 把标签与自定义提示关联起来(无障碍性提升)。

2. 测试结果(PR head 上)

运行项 结果
定向 useComposerCore.dom.test.tsx 6 通过(2 个新增 + 4 个既有)
@qwen-code/web-shell 完整测试套件 89 个文件 · 1413 个测试 —— 全部通过
tsc --noEmit(类型检查) ✅ 无错误
eslint(两个改动文件) ✅ 无告警
prettier --check(两个改动文件) ✅ 格式合规

说明:PR 描述写的是 87 文件 / 1384 测试,现已增至 89 / 1413,仅因 main 期间新增了测试;无任何回归。

3. 修复前 / 后对比证明(测试确实守护了本次修复)

为证明这两个新测试并非“永真”测试,我仅就地回退了修复涉及的源码 hunk(同时保留两个新测试),以还原 main 的修复前行为,再运行该测试文件。(之所以就地回退 hunk 而非直接覆盖 origin/main 整个文件,是因为当前 main 重构了相邻的 utils/composerTag 模块,而该模块在本 PR 的基线上并不存在。)

  • 修复前 → 2 个失败,恰好落在本 PR 改动的行为上:
    • uses a custom inline tooltip without a native titleexpected 'Details' to be null(存在重复的原生 title)。
    • falls back to a native title when attaching an inline tooltip failsexpected 'block' to be 'none'(在 main 上,悬停监听器在渲染 try 之前绑定,因此渲染失败后残留提示仍会在悬停时显示)。
  • 修复后(PR head)→ 6 个全部通过。

(截图见上方英文部分)

4. 行为 / DOM 证据

使用 ComposerTagWidget 中原样的 cssText 与真实的 web-shell 深色主题变量渲染。修复前:原生 title 提示自定义提示重叠(title="Details",无 aria-describedby)。修复后:仅有一个自定义提示,通过 aria-describedby="composer-tag-tooltip-1" 关联,且无原生 title

(截图见上方英文部分)

5. 一点观察(不阻塞合并)

两个新测试会输出一条 React 开发告警 —— “Attempted to synchronously unmount a root while React was already rendering.” 这是既有问题,并非本 PR 引入ComposerTagWidget.destroy() 在 CodeMirror widget 拆卸时会同步调用 this.tooltipRoot?.unmount(),而这段代码在 main完全一致。它之所以现在才出现,是因为这两个是首个走到成功渲染字符串提示路径的 DOM 测试(既有的提示测试会抛异常,因此从未创建过 root)。无害 —— 所有测试均通过 —— 但如果不希望有这类噪音,未来可考虑延迟 unmount。

结论

LGTM —— 建议合并。 ✅ 修复聚焦、正确,带有真实的回归测试覆盖,并带来实实在在的无障碍性提升(aria-describedby),且未引入任何 lint / 类型 / 格式 / 测试回归。

Verified locally by the maintainer in an isolated worktree; screenshots reproduce the widget DOM with verbatim source styling. Not a substitute for CI.

@wenshao
wenshao added this pull request to the merge queue Jul 12, 2026
Merged via the queue into QwenLM:main with commit 8a2be47 Jul 12, 2026
184 of 188 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.

fix(web-shell): avoid duplicate inline composer tag tooltips

4 participants