Skip to content

feat(web-shell): add context mention customization - #6578

Merged
dreamWB merged 11 commits into
QwenLM:mainfrom
dreamWB:codex/webshell-context-capabilities
Jul 10, 2026
Merged

feat(web-shell): add context mention customization#6578
dreamWB merged 11 commits into
QwenLM:mainfrom
dreamWB:codex/webshell-context-capabilities

Conversation

@dreamWB

@dreamWB dreamWB commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator

What this PR does

This PR adds host customization surfaces for Web Shell context mentions and composer/message tags. Hosts can now configure built-in @ providers, provide custom provider tabs and item rendering, attach custom icons/metadata/serialization to accepted composer tags, parse stored user-message text into inline tag parts during transcript rendering, and customize tag rendering, tooltips, and click handling across both the composer and user bubbles.

Why it's needed

Some Web Shell embedders need structured context references rather than plain @file text. The serialized prompt must remain the source of truth for agent input and history replay, while the UI should be able to show compact chips with host-specific hover and click behavior. These changes keep that host-specific protocol outside Web Shell and expose generic extension points instead.

Reviewer Test Plan

How to verify

Review the new Web Shell props and types around builtinAtProviders, WebShellAtProvider.tabs, WebShellAtProvider.renderItem, WebShellComposerTag.icon, WebShellComposerTag.metadata, renderComposerTag, renderComposerTagTooltip, onComposerTagClick, and parseUserMessageContent. Then run the targeted Web Shell tests and build listed below.

Evidence (Before & After)

N/A — this is an embeddable Web Shell API/UI extension and does not change the default Qwen Code CLI flow. The default built-in @ providers remain enabled unless a host opts into the new configuration.

Tested on

OS Status
🍏 macOS ✅ tested
🪟 Windows ⚠️ not tested
🐧 Linux ⚠️ not tested

Environment (optional)

Tested locally on macOS with Node/npm from the repository setup. Commands run: npm ci (which also completed repository prepare/build), targeted Web Shell Prettier check, targeted Web Shell ESLint, npm run test --workspace=@qwen-code/web-shell -- --run client/components/AtMentionPanel.test.tsx client/hooks/useAtMentionMenu.test.tsx client/hooks/useComposerCore.test.ts client/components/messages/UserMessage.test.tsx, npm run build --workspace=@qwen-code/web-shell, git diff --check, and npm run check:serve-fast-path-bundle.

Full npm run preflight was attempted locally on 2026-07-09. It completed clean, npm ci, format, lint:ci, build, typecheck, and reached test:ci, but exited non-zero in the CLI workspace. Follow-up targeted reruns showed the originally observed scheduled-tasks, usage-stats, AppContainer, and server anti-clickjacking cases passing individually; extensions/list passed with QWEN_CODE_LANG=en. The remaining failures appear local-environment / full-parallel CLI test instability rather than Web Shell regressions.

Risk & Scope

  • Main risk or tradeoff: The Web Shell composer and user-message rendering paths now accept more host-provided renderers and metadata, so the main risk is regressions in default mention/tag behavior. The added tests cover built-in provider filtering, provider tabs, custom item rendering, inline tags, serialization, user-message parsing, and fallback behavior.
  • Not validated / out of scope: Full npm run preflight was attempted but did not pass locally because of unrelated CLI workspace test failures. Cross-platform manual validation was not run locally. This PR also does not include a concrete host integration or host-specific context schema.
  • Breaking changes / migration notes: No intended breaking changes. Built-in @ providers remain enabled by default, default item rendering is preserved, and transcript content remains a plain string.

Linked Issues

No linked issue yet; opening as a draft PR for maintainer discussion on the Web Shell host customization API.

中文说明

What this PR does

这个 PR 为 Web Shell 的上下文 mention 和 composer/message tag 增加宿主侧扩展能力。宿主现在可以配置内置 @ provider,提供自定义 provider tab 和 item 渲染,为选中的 composer tag 附加自定义图标、metadata 和序列化文本,在用户消息历史渲染时把存储的纯文本解析成内联 tag parts,并在输入框和用户气泡中统一自定义 tag 渲染、tooltip 和点击行为。

Why it's needed

一些 Web Shell 嵌入方需要结构化上下文引用,而不仅是普通的 @file 文本。真正发送给 agent 和历史回放的内容仍然应该以序列化 prompt 为唯一事实来源,同时 UI 需要能展示紧凑 chip,并支持宿主特定的 hover 和点击行为。本次改动把宿主特定协议留在 Web Shell 外部,只在 Web Shell 中提供通用扩展点。

Reviewer Test Plan

How to verify

请重点检查 builtinAtProvidersWebShellAtProvider.tabsWebShellAtProvider.renderItemWebShellComposerTag.iconWebShellComposerTag.metadatarenderComposerTagrenderComposerTagTooltiponComposerTagClickparseUserMessageContent 相关的新 Web Shell props/types。然后运行上面列出的 Web Shell 定向测试和 build 命令。

Evidence (Before & After)

N/A — 这是 Web Shell 可嵌入 API/UI 扩展,不改变默认 Qwen Code CLI 流程。默认内置 @ provider 仍然启用,除非宿主主动使用新的配置关闭或筛选。

Tested on

OS Status
🍏 macOS ✅ tested
🪟 Windows ⚠️ not tested
🐧 Linux ⚠️ not tested

Environment (optional)

在 macOS 本地使用仓库 Node/npm 环境验证。已运行命令:npm ci(同时完成仓库 prepare/build)、Web Shell 定向 Prettier check、Web Shell 定向 ESLint、npm run test --workspace=@qwen-code/web-shell -- --run client/components/AtMentionPanel.test.tsx client/hooks/useAtMentionMenu.test.tsx client/hooks/useComposerCore.test.ts client/components/messages/UserMessage.test.tsxnpm run build --workspace=@qwen-code/web-shellgit diff --check,以及 npm run check:serve-fast-path-bundle

2026-07-09 本地尝试运行了完整 npm run preflight。它已完成 cleannpm ciformatlint:cibuildtypecheck,并执行到 test:ci,但在 CLI workspace 测试阶段非零退出。后续定向复跑显示最初失败的 scheduled-tasksusage-statsAppContainer、server anti-clickjacking 用例均可单独通过;extensions/listQWEN_CODE_LANG=en 下通过。剩余失败更像本机环境 / 全量并行 CLI 测试不稳定,而不是 Web Shell 回归。

Risk & Scope

  • Main risk or tradeoff: Web Shell composer 和用户消息渲染路径现在接受更多宿主提供的 renderer 和 metadata,主要风险是默认 mention/tag 行为回归。新增测试覆盖了内置 provider 过滤、provider tabs、自定义 item 渲染、内联 tag、序列化、用户消息解析和 fallback 行为。
  • Not validated / out of scope: 已尝试完整 npm run preflight,但因为无关 CLI workspace 测试失败,本地未能通过。没有做跨平台手工验证。这个 PR 也不包含具体宿主集成或宿主特定上下文 schema。
  • Breaking changes / migration notes: 预期没有破坏性变更。内置 @ provider 默认仍然启用,默认 item 渲染保持不变,transcript content 仍然是纯字符串。

Linked Issues

暂无关联 issue;先以 draft PR 形式提交,方便维护者讨论 Web Shell 宿主扩展 API。

@BZ-D

BZ-D commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator

#6537
这两个 PR 做的事情是否差不多?

@dreamWB
dreamWB marked this pull request as ready for review July 9, 2026 07:50
@qwen-code-ci-bot

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

Copy link
Copy Markdown
Collaborator

Re-run update (2026-07-10): Fourth triage pass. No new commits since last pass (65fd7b6ee). Author agreed to fix the label type issue but hasn't pushed the change yet.

Template looks good ✓

Problem: Feature addition — host customization surfaces for Web Shell @ mentions and composer/message tags. Driven by a concrete Web Shell embedder integration (confirmed by author), not ahead-of-time API design.

Direction: Web Shell host customization is aligned with the product's embeddable architecture. Complementary to #6537 (which improves default transcript rendering). No direction concerns.

Size: ~1,553 production lines (including ~12 lines in an unrelated design doc reformat), ~847 test lines. Primary changes contained in packages/web-shell/client/ — not core infrastructure. Two minor cross-package touch-ups: packages/cli/src/ui/components/TranscriptView.test.tsx (formatting, -3 lines) and packages/mobile-mcp/src/index.ts (formatting, -6 lines).

Approach: All critical findings from the first three review cycles resolved across four fix commits. The one outstanding item is WebShellAtProvider.label: string → ReactNode — a breaking public API type change. The author has agreed to fix this by either keeping label: string + adding renderLabel, or documenting the widening explicitly. Awaiting the commit.

Moving on to code review. 🔍

中文说明

第四次重新运行更新(2026-07-10): 自上次评审以来没有新提交(65fd7b6ee)。作者同意修复 label 类型问题但尚未推送变更。

模板完整 ✓

问题: 功能新增——为 Web Shell 的 @ mention 和 composer/message tag 增加宿主侧扩展能力。由具体的 Web Shell 嵌入方集成驱动(作者确认),非提前 API 设计。

方向: Web Shell 宿主自定义与产品的可嵌入架构一致。与 #6537 互补。无方向问题。

规模: ~1,553 行生产代码(含约 12 行无关设计文档格式化),~847 行测试代码。主要变更在 packages/web-shell/client/ 内——不属于核心基础设施。两个小型跨包改动:TranscriptView.test.tsx(格式化)和 mobile-mcp/index.ts(格式化)。

方案: 前三轮评审的所有关键发现均已通过四个修复提交解决。唯一的待解决项是 WebShellAtProvider.label: string → ReactNode——破坏性公共 API 类型变更。作者已同意通过保持 label: string 并添加 renderLabel,或明确记录类型扩展来修复。等待提交。

进入代码审查 🔍

Qwen Code · qwen3.7-max

@dreamWB

dreamWB commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator Author

Replying to @BZ-D’s question about #6537:

They are related, but not the same scope. #6537 improves the default transcript rendering for built-in composer references (file / extension / MCP) by reconstructing chips from serialized prompt text. This PR adds host-facing extension points for custom @ providers, custom item rendering, tag metadata/serialization, and custom composer/user-message tag rendering.

In other words, #6537 can become part of the default built-in behavior, while this PR is for embedders that need to own custom reference semantics and UI behavior.

@qwen-code-ci-bot

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

Copy link
Copy Markdown
Collaborator

Re-run update (2026-07-10): Fourth pass. Rebuilt and retested on a fresh worktree checkout. No code changes since last pass.

Code Review

All previously raised critical issues remain resolved:

  • Mask-mode icon isSafeImageSrc() validation — confirmed in AtMentionPanel.tsx, ChatEditor.tsx, useComposerCore.ts, UserMessage.tsx. ✓
  • Tooltip appendTooltip try/catch — confirmed with proper cleanup and console.warn fallback. ✓
  • Custom renderer error handlingComposerTagWidget.toDOM() and UserMessageTag both wrap host renderers in try/catch. ✓
  • Inline tag position clamping, provider id dedup, safe image src, keyboard handling — all confirmed. ✓

Remaining outstanding item: WebShellAtProvider.label: ReactNode is a breaking public API type change from string. Author has agreed to fix (keep label: string + add renderLabel, or document the widening) but hasn't pushed the commit yet. This needs resolution before merge.

Minor observations (not blockers):

  • createRoot in CodeMirror widgets — fragile React/CM lifecycle coupling. destroy() properly unmounts, but a missed call leaks a React root.
  • CSS tooltip styles duplicated between ChatEditor.module.css and UserMessage.module.css.

No correctness bugs, security issues, or regressions found.

Build & Test

Build (vite + vite lib + tsc): ✅ all three stages pass.

Full web-shell suite on clean worktree:

 Test Files  82 passed (82)
      Tests  1339 passed (1339)
   Duration  4.61s

Typecheck: ✅ clean across all packages.

Zero regressions.

Tmux Real-Scenario Testing

N/A — this PR is an embeddable Web Shell API/UI extension that does not change the default Qwen Code CLI flow. There is no CLI-level before/after to capture.

中文说明

第四次重新运行更新(2026-07-10): 在干净 worktree 上重新构建和测试。自上次评审以来无代码变更。

代码审查

所有之前提出的关键问题均已解决:

  • Mask 模式图标 isSafeImageSrc() 验证 — 已在四个渲染路径中确认。✓
  • Tooltip appendTooltip try/catch — 已确认有正确的清理和 console.warn 回退。✓
  • 自定义渲染器错误处理ComposerTagWidget.toDOM()UserMessageTag 均将宿主渲染器包裹在 try/catch 中。✓

待解决项: WebShellAtProvider.label: ReactNode 是从 string 的破坏性 API 类型变更。作者已同意修复但尚未推送提交。需在合入前解决。

构建与测试

构建(vite + vite lib + tsc):✅ 全部通过。完整 web-shell 套件:82 个测试文件,1339 个测试全部通过。Typecheck:✅ 所有包均通过。零回归。

Tmux 真实场景测试

N/A — Web Shell API/UI 扩展,不改变默认 CLI 流程。

Qwen Code · qwen3.7-max

@qwen-code-ci-bot

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

Copy link
Copy Markdown
Collaborator

Re-run verdict (2026-07-10) — fourth pass

No code changes since the last triage. The situation is unchanged: all critical findings from three prior review cycles are resolved, build and 1339 tests pass, typecheck is clean.

The PR is well-structured and the author has been responsive across four fix commits. The implementation quality is high — clean separation between types, state, and rendering, thorough error handling around host-provided callbacks, and good test coverage for the new extension points.

The one item still blocking merge is WebShellAtProvider.label: string → ReactNode. The author has explicitly agreed to fix this (either by keeping label: string + adding renderLabel, or by explicitly documenting the widening), but the commit hasn't been pushed yet.

Still deferring to maintainer review rather than auto-approving, given the prior approval was dismissed by @yiliang114. Once the author pushes the label type fix, this PR should be ready for a final maintainer sign-off and merge.

中文说明

重新运行结论(2026-07-10)——第四次

自上次评审以来无代码变更。情况不变:前三轮评审的所有关键发现均已解决,构建和 1339 个测试通过,typecheck 干净。

PR 结构良好,作者在四个修复提交中一直积极响应。实现质量很高——类型、状态和渲染之间分离清晰,对宿主提供的回调有充分的错误处理,新扩展点的测试覆盖良好。

仍然阻塞合入的一项是 WebShellAtProvider.label: string → ReactNode。作者已明确同意修复(保持 label: string 并添加 renderLabel,或明确记录类型扩展),但尚未推送提交。

仍交由维护者审查而非自动批准,鉴于之前的批准被 @yiliang114 撤销。一旦作者推送 label 类型修复,此 PR 即可进行最终维护者签字并合入。

Qwen Code · qwen3.7-max

@dreamWB

dreamWB commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator Author

Thanks for the triage. Let me clarify the intent and scope.

This is driven by a concrete Web Shell embedder integration, not an ahead-of-time API design. The embedder needs custom structured references to work consistently across the full flow: selecting an @ context, rendering the accepted composer tag, serializing the prompt sent to the daemon, and replaying the same reference as an inline chip in user-message history.

On #6537: I see it as complementary rather than duplicate. #6537 improves the default behavior for built-in references by reconstructing built-in file / extension / MCP chips from serialized prompt text. This PR is about host-owned reference semantics: custom providers, custom item layout, custom tag metadata/serialization, and custom rendering/click behavior. If #6537 lands first, I’m happy to rebase and reuse its built-in composer tag utility as the default path, while keeping the host extension points here.

On the API surface:

  • parseUserMessageContent, renderComposerTag, renderComposerTagTooltip, and onComposerTagClick are meant to keep composer tags and replayed user-message tags consistent without Web Shell knowing the host’s reference protocol.
  • provider tabs and item rendering are needed for hosts whose @ source is not a flat file-like list.
  • metadata and serialized are needed because the UI label is not always the prompt text sent to the daemon.

I agree the built-in provider config may be more flexible than necessary. If maintainers prefer, I can simplify WebShellBuiltinAtProvidersConfig to boolean | readonly WebShellBuiltinAtProviderId[] in this PR.

For WebShellAtProvider.label, the intent was to allow icon/rich tab labels, but I agree this changes the public type. I can avoid widening the existing field by keeping label: string and adding a separate optional renderLabel or labelNode if that is preferred.

Comment thread packages/web-shell/client/hooks/useComposerCore.ts
Comment thread packages/web-shell/client/hooks/useComposerCore.ts Outdated
Comment thread packages/web-shell/client/hooks/useComposerCore.ts
@qwen-code-ci-bot

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

Copy link
Copy Markdown
Collaborator

Suggestions — commit 94ec3fa8

File Issue Suggested fix
useComposerCore.ts:693 When createRoot succeeds but .render() throws, the catch sets contentRoot = null without calling .unmount(), leaking React fiber nodes. Same issue in appendTooltip. Call this.contentRoot?.unmount() before setting to null in the catch block.
useComposerCore.ts:622 eq() compares tooltip by ReactNode reference. resolveComposerTagIcon produces a fresh ReactNode each call, so eq() always returns false when tooltips are used — causing widget teardown/rebuild on every decoration pass. Store the tooltip renderer function (not its output) on the tag; call it lazily in appendTooltip. Compare renderer function identity in eq().
Tests (multiple files) builtinAtProviders object form (include/exclude/enabled: false) never tested. ComposerTagWidget.destroy() React root cleanup untested. No ChatEditor.test.tsx for top-tag click/tooltip. Inline CodeMirror widget click path untested. Add tests for object-form config, destroy lifecycle, and ChatEditor top-tag rendering.
useComposerCore.ts (toDOM), ChatEditor.tsx:1287-1315, UserMessage.tsx:211-263 Icon lookup, label/value extraction, and conditional rendering duplicated across 3 independent implementations (imperative DOM + 2 React). Extract a shared getComposerTagParts(tag) helper returning { iconUrl, label, value, fallback }.
UserMessage.tsx:60-80 Parts from parseUserMessageContent are mapped directly into the React tree without running through sanitizeComposerTag. Stored-message content can carry unsanitized tag.metadata/tag.icon. Run parser output through sanitizeComposerTag before rendering, matching the composer's sanitization path.
useComposerCore.ts:678,693 Two catch blocks log identical '[WebShell] inline tag renderContent failed' for distinct root causes (host function threw vs React render threw). Differentiate: '… renderContent function threw' vs '… renderContent React render failed'.
useComposerCore.ts (toDOM), UserMessage.tsx (UserMessageTag) When tooltip is a string, both native title attribute and custom <span role="tooltip"> element activate on hover — producing duplicate tooltips. Suppress native title when a custom tooltip is present.

— qwen3.7-max via Qwen Code /review

@dreamWB

dreamWB commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator Author

Addressed the review feedback in 3ebbc1a26.

Changes made:

  • Fixed inline composer tag remove-key handling: Backspace/Delete now invoke the same removal path instead of being swallowed.
  • Cleared inline React roots more defensively and dispatch clearInlineTagsEffect before editor teardown.
  • Documented the inline composer tag renderer limitation: inline tags are mounted from CodeMirror-managed React roots and must be context-free.
  • Passed composerTagIcons through customization so user-message tags use the same icon overrides as composer tags.
  • Sanitized custom provider iconTooltip text.
  • Renamed the internal inline placement interface to avoid colliding with the public placement union.
  • Added missing coverage for tab switching, invalid/overlapping inline placements, and user-message tag click/icon behavior.
  • Tightened AtProvider registration: custom providers may not reuse built-in ids or duplicate another custom provider id. Duplicate ids are reported with console.error and ignored, avoiding ambiguous display/search behavior.

Verification:

  • cd packages/web-shell && npx vitest run client/hooks/useAtMentionMenu.test.tsx client/hooks/useComposerCore.test.ts client/components/messages/UserMessage.test.tsx
  • npm run typecheck
  • npm run build
  • git diff --check

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

dreamWB commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator Author

Pushed 1c7c570a9 to address the latest review feedback.

Changes:

  • Guard inline composer tag renderComposerTag execution so a host renderer exception logs a warning and falls back to default tag content instead of breaking CodeMirror widget creation.
  • Keep icon fields as URL/source data instead of running display-text sanitization over them, preserving valid data image sources.
  • Guard AtMentionPanel image icons with the existing isSafeImageSrc check before rendering <img src>.

Verification:

  • npx vitest run client/hooks/useAtMentionMenu.test.tsx client/components/AtMentionPanel.test.tsx client/hooks/useComposerCore.dom.test.tsx client/hooks/useComposerCore.test.ts
  • npm run typecheck
  • npm run build

@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. Suggestion-level recommendations are in the Suggestion summary comment below.

@dreamWB

dreamWB commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator Author

Pushed ab3f2ec71 for the latest suggestion-level review.

Accepted and fixed:

  • Wrapped inline tag React root creation/rendering in the same fallback path as renderComposerTag, so a custom renderer failure falls back to default tag content instead of breaking widget creation.
  • Clamped inline tag positions after leading-whitespace trimming so submitted prompts keep tags that land at the start of the trimmed text.
  • Added try/catch around user-message tag content and tooltip renderers, with default chip fallback when host rendering throws.

Intentionally not changing:

  • I am not moving inline tag renderer/tooltip evaluation to a lazy function-storage model in this PR. That is an optimization/lifecycle refactor for prop-change churn, not a correctness blocker, and it would expand the API/lifecycle shape more than this review round needs.

Verification:

  • npx vitest run client/hooks/useComposerCore.dom.test.tsx client/components/messages/UserMessage.test.tsx client/hooks/useComposerCore.test.ts
  • npm run typecheck
  • npm run build

@dreamWB

dreamWB commented Jul 10, 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. ✅ One note: the WebShellAtProvider.label: string → ReactNode change is a breaking API type change — worth calling out in release notes or resolving before merge (e.g. keep label: string + add renderLabel).

yiliang114
yiliang114 previously approved these changes Jul 10, 2026
Comment thread packages/web-shell/client/components/AtMentionPanel.tsx Outdated
Comment thread packages/web-shell/client/hooks/useComposerCore.ts
@dreamWB
dreamWB enabled auto-merge July 10, 2026 03:37
dreamWB added 2 commits July 10, 2026 11:46
…t-capabilities

# Conflicts:
#	packages/web-shell/client/App.tsx
#	packages/web-shell/client/customization.tsx
#	packages/web-shell/client/index.ts
…ies' into codex/webshell-context-capabilities
@dreamWB

dreamWB commented Jul 10, 2026

Copy link
Copy Markdown
Collaborator Author

@qwen-code /triage

@yiliang114

Copy link
Copy Markdown
Collaborator

@qwen-code /triage

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

@dreamWB
dreamWB added this pull request to the merge queue Jul 10, 2026
Merged via the queue into QwenLM:main with commit 24bca9a Jul 10, 2026
32 of 35 checks passed
chip.style.cssText =
'display:inline-flex;align-items:center;max-width:min(44ch,100%);min-height:20px;margin:0 0.25ch;border:1px solid var(--border);border-radius:4px;background:var(--secondary);color:var(--foreground);font-family:var(--font-mono,monospace);font-size:12px;line-height:1.2;vertical-align:baseline;';
'position:relative;display:inline-flex;align-items:center;max-width:min(44ch,100%);min-height:20px;margin:0 0.25ch;border:1px solid var(--border);border-radius:4px;background:var(--secondary);color:var(--foreground);font-family:var(--font-mono,monospace);font-size:12px;line-height:1.2;vertical-align:baseline;';
if (this.tag.tooltipText) chip.title = this.tag.tooltipText;

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.

[Critical] Dual native + custom tooltip on hover. When the tooltip is a string, chip.title = this.tag.tooltipText sets the native browser tooltip, and this.appendTooltip(chip, this.tag.tooltip) (line 716) adds a custom React-rendered tooltip. Both appear simultaneously on hover, producing two overlapping tooltips.

Suggested change
if (this.tag.tooltipText) chip.title = this.tag.tooltipText;
if (this.tag.tooltipText && (this.tag.tooltip === undefined || this.tag.tooltip === null)) chip.title = this.tag.tooltipText;

— qwen3.7-max via Qwen Code /review

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.

Confirmed against current main: the duplicate-tooltip report is valid, although I would classify the impact as a UI regression rather than Critical. Follow-up PR #6729 makes the native title a fallback when a custom tooltip exists, adds an aria-describedby association for the custom tooltip, and includes focused DOM regression coverage. The WebShell suite passes 1,384/1,384 tests on macOS.

const renderComposerTagContent = (tag: WebShellComposerTag) => {
const custom = renderComposerTag?.({
tag,
placement: 'composer',

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.

[Suggestion] renderComposerTag is called without try/catch here (and renderComposerTagTooltip at line 1411). If a host-provided renderer throws, the entire composer crashes. By contrast, UserMessage.tsx wraps both in try/catch with console.warn fallback, and useComposerCore.ts (inline widget) does the same. The composer is the most dangerous place to crash since it's the interactive input area.

Wrap both calls in try/catch matching the pattern used elsewhere:

let custom: ReactNode | null | undefined;
try {
  custom = renderComposerTag?.({ tag, placement: 'composer', readonly: false });
} catch (error) {
  console.warn('[WebShell] composer tag render failed', error);
}

— qwen3.7-max via Qwen Code /review

return (
<span
key={tag.id}
className={styles.tag}

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.

[Suggestion] Nested interactive elements: when onComposerTagClick is provided, this <span> gets role="button" + tabIndex={0}, while the inner remove <button> remains a separately focusable element. This violates WCAG nested-interactive guidelines — screen readers announce two separate widgets, and keyboard navigation gets confusing.

Consider restructuring: when the tag is both clickable and removable, make the whole tag a <button> and use aria-describedby for the remove action, or use a split-button pattern.

— qwen3.7-max via Qwen Code /review

<button
type="button"
className={styles.atTabScrollButton}
aria-label="Previous tab"

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.

[Suggestion] Hardcoded English aria-label on tab scroll buttons. Every other user-facing string in this component uses t() for i18n (e.g., t('common.back'), t('common.search')), but these labels are hardcoded in English. Screen-reader users in non-English locales will hear English labels.

Suggested change
aria-label="Previous tab"
aria-label={t('common.previous')}

— qwen3.7-max via Qwen Code /review

tabIndex={clickable ? 0 : undefined}
title={getTagText(tag)}
onClick={(event) => {
if (!clickable) return;

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.

[Suggestion] Dual tooltip: title={getTagText(tag)} always sets the native browser tooltip to the tag's display text. When renderComposerTagTooltip is configured, the custom CSS tooltip (lines 259-262) also renders on hover. Both tooltips appear simultaneously — one showing the tag label, the other showing the tooltip content.

Omit title when a custom tooltip renderer is configured:

Suggested change
if (!clickable) return;
title={tooltip === undefined || tooltip === null ? getTagText(tag) : undefined}

— qwen3.7-max via Qwen Code /review

};
chip.addEventListener('mouseenter', show);
chip.addEventListener('mouseleave', hide);
chip.addEventListener('focusin', show);

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.

[Suggestion] Tooltip element has role="tooltip" but no id, and the chip has no aria-describedby pointing to it. Screen readers won't announce the tooltip content when the chip is focused. This matters especially for clickable chips (role="button" + tabIndex=0).

Assign a unique id to the tooltip element and set aria-describedby on the chip:

tooltipElement.id = `composer-tag-tooltip-${++tooltipIdCounter}`;
chip.setAttribute('aria-describedby', tooltipElement.id);

— qwen3.7-max via Qwen Code /review

chip.addEventListener('pointerdown', (event) => {
event.preventDefault();
event.stopPropagation();
});

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.

[Suggestion] focusin/focusout bubble from descendants, causing tooltip flicker. When focus moves between the chip and its descendant remove <button>, the sequence is: focusout (chip) → hide → focusin (remove button, bubbling) → show → focusout (remove button, bubbling) → hide. This produces visible flicker.

Guard with relatedTarget to detect focus staying within the chip subtree:

chip.addEventListener('focusout', (event) => {
  if (!chip.contains(event.relatedTarget as Node)) hide();
});

— qwen3.7-max via Qwen Code /review

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.

6 participants