feat(web-shell): add cell value dialog on double-click in markdown tables - #6530
Merged
Conversation
…bles Double-clicking a table cell opens a modal dialog showing the cell's full content with copy and close actions. The dialog clears any active selection or row details on open, supports Escape to dismiss, and click-outside to close. Adds EN/ZH i18n keys and four unit tests covering open, copy, dismiss, and state-clearing behaviour.
qwen-code-ci-bot
requested changes
Jul 8, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Thanks for the PR @jifeng! 👋
The PR body doesn't follow the pull request template. The template requires these sections:
- What this PR does — describe the change in prose
- Why it's needed — motivation and user-facing benefit
- Reviewer Test Plan — how a reviewer can confirm the change works (with Before/After evidence for UI changes)
- Risk & Scope — tradeoffs and out-of-scope items
- Linked Issues — related issue references
- 中文说明 — Chinese translation in a
<details>block
Could you update the PR description to match the template? The Reviewer Test Plan is especially important — without it, review may be delayed. Happy to re-run triage once updated.
中文说明
感谢贡献 @jifeng！👋
PR 正文没有按照 PR 模板 填写。模板要求以下章节：
- What this PR does — 用自然语言描述改动
- Why it's needed — 动机和用户价值
- Reviewer Test Plan — 审查者如何确认改动有效（UI 改动需要 Before/After 证据）
- Risk & Scope — 权衡和不在范围内的部分
- Linked Issues — 关联的 issue
- 中文说明 —
<details>中的中文翻译
请按照模板更新 PR 描述。Reviewer Test Plan 尤其重要——没有它审查可能会被延迟。更新后可以重新触发审查。
— Qwen Code · qwen3.7-max
Contributor
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. |
wenshao
requested changes
Jul 8, 2026
Collaborator
Suggestions — commit
|
| File | Issue | Suggested fix |
|---|---|---|
EnhancedMarkdownTable.tsx:2430 |
Cell dialog reimplements ~60 lines of focus trapping, Escape, portal, and backdrop close that DialogShell.tsx already provides. The simplified backdrop (mousedown-only) is less robust than DialogShell's drag-safe pattern — selecting text whose cursor drifts to the backdrop closes the dialog prematurely. CSS theme variables and i18n keys are also duplicated. |
Consider refactoring to use DialogShell with a compact variant, or at minimum adopting its drag-safe backdrop pattern (mousedown+mouseup+click). |
EnhancedMarkdownTable.test.tsx |
Missing test for auto-close effect when the dialog's row disappears from visibleRows (e.g., filtered out). A similar test exists for row detail auto-close but the cell dialog variant is uncovered. |
Add a test that opens the dialog, applies a filter excluding that row, and asserts the dialog closes. |
EnhancedMarkdownTable.test.tsx |
Missing test for clipboard unavailable path. copyCellDialogValue silently returns when navigator.clipboard is unavailable. All copy tests mock clipboard first. |
Add a test without mockClipboard() that asserts the Copy button label stays as "Copy". |
— qwen3.7-max via Qwen Code /review
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed — no blockers. Suggestion-level recommendations are in the Suggestion summary comment below.
Collaborator
Suggestions — commit
|
| File | Issue | Suggested fix |
|---|---|---|
EnhancedMarkdownTable.tsx:1649-1658 |
Auto-close effect has incomplete cleanup: (a) calls setCellDialog(null) without resetCopiedCellDialog() — every other close path calls both; (b) doesn't check hiddenColumns.has(cellDialog.columnIndex), so hiding the dialog's column leaves it open. |
Add resetCopiedCellDialog() after setCellDialog(null) and add hiddenColumns.has(cellDialog.columnIndex) to the dismiss condition with hiddenColumns in the dependency array. |
EnhancedMarkdownTable.tsx:2538-2543 |
onDoubleClick doesn't call event.preventDefault(). Browser's native dblclick default creates a word-level text selection that persists behind the dialog backdrop. |
Add event.preventDefault() before openCellDialog(row.key, columnIndex). |
EnhancedMarkdownTable.tsx:1528-1536 |
clearActiveColumnOnOutsideMouseDown doesn't check cellDialogRef. Clicks inside the portaled dialog (Copy/Close, text selection) propagate to document as outside clicks, clearing activeColumn. |
Add !cellDialogRef.current?.contains(target) to the containment check. |
EnhancedMarkdownTable.tsx:1880-1883 |
copyCellDialogValue passes cell.text directly to sanitizeForClipboard, but other copy paths use cellClipboardText() first (preserves rawText, normalizes \r\n\t). Two copy paths for the same cell produce different clipboard values. |
Route through sanitizeForClipboard(cellClipboardText(currentCellDialogCell)). |
EnhancedMarkdownTable.test.tsx, App.test.tsx |
Missing test coverage: (a) auto-close when row disappears from visibleRows; (b) toggleRowDetail dismissing an open cell dialog; (c) interaction blocker release path. |
Add focused tests for each path. |
EnhancedMarkdownTable.tsx:2538,~2672 |
Keyboard/touch accessibility: (a) cell dialog only accessible via mouse double-click — no tabIndex/role/onKeyDown on <td> (WCAG 2.1 SC 2.1.1); (b) scrollable value area has no tabindex. |
Add keyboard activation path and tabIndex={0} to the value div. |
— qwen3.7-max via Qwen Code /review
Collaborator
Author
|
本次根据评审意见做了以下修复：
验证：
|
wenshao
requested changes
Jul 8, 2026
qwen-code-ci-bot
requested changes
Jul 8, 2026
ytahdn
previously approved these changes
Jul 8, 2026
ytahdn
left a comment
ytahdn
left a comment
Collaborator
There was a problem hiding this comment.
LGTM. 代码审查无缺陷，逐项确认：
- 焦点陷阱 Tab/Shift+Tab 循环正确，Escape 关闭并 stopPropagation 不穿透到表格
- 所有关闭路径（Escape / backdrop / Close 按钮 / 行数据变更）均正确清理 timer 和 copied 状态
- 双击交互元素（链接等）正确过滤，不会误触发对话框
- 打开对话框时完整清除 selection / detailRow / filterMenu 状态
- 复制使用 generation counter 防止过期回调，关闭后正确回焦
- 测试覆盖 10 个用例，涵盖打开/复制/关闭/焦点陷阱/状态清除/交互元素过滤等场景
- 移动端响应式适配到位
实现质量很高，可以合并。
Collaborator
|
@qwen-code /triage |
Collaborator
|
Thanks for the PR @jifeng! 👋 Template looks good now ✓ — the previous round flagged missing sections, and they're all filled in properly this time, including the video evidence. Problem: Real UX gap. When enhanced Markdown tables wrap or truncate long cell values, users can't easily grab a substring — a region, request ID, or error fragment — without copying the entire cell. The video demo clearly shows the awkward workflow before this change. Direction: Well-aligned. Enhanced Markdown tables are a core web-shell feature, and an Excel-like cell inspection dialog is a natural addition. Doesn't change any existing behavior (selection, TSV copy, sorting, filtering all preserved). Size: 358 production lines (148 CSS + 204 TSX + 6 i18n), 173 test lines. Not applicable for core module gates — all changes are in Approach: Scope feels right. Four files, one feature — dialog state, focus trapping, clipboard copy, dismiss paths, and tests. No drive-by refactors or scope creep. The risk section honestly calls out the double-click behavior change, which is the right tradeoff to flag. Moving on to code review. 🔍 中文说明感谢贡献 @jifeng！👋 模板现在完整 ✓ — 上一轮指出缺少的章节已全部补齐，包括视频证据。 问题： 真实的 UX 痛点。增强 Markdown 表格对长单元格值换行或截断时，用户很难只复制一部分内容（如 region、request ID、错误片段），只能复制整个单元格。视频演示清楚展示了改动前的不便。 方向： 对齐。增强 Markdown 表格是 web-shell 的核心功能，类 Excel 的单元格查看弹窗是自然的扩展。不改变现有行为（选择、TSV 复制、排序、筛选均保留）。 规模： 358 行生产代码（148 CSS + 204 TSX + 6 i18n），173 行测试代码。不触及核心模块——所有改动在 方案： 范围合理。4 个文件，1 个功能——弹窗状态、焦点陷阱、剪贴板复制、关闭路径和测试。没有夹带无关重构或范围蔓延。Risk 部分诚实地指出了双击行为变化，这是正确的权衡。 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Collaborator
Code ReviewRead through the diff and the full component file. The implementation is clean and follows existing patterns in
No correctness bugs, security concerns, or AGENTS.md violations found. TestingUnit tests (vitest, local)Ran the full test file with the PR's changes applied: All 82 tests pass — 73 existing + 9 new. The 9 new tests cover: open, copy, empty copy, Escape dismiss, focus trap, Escape non-interference with table, selection/detail cleanup, filter menu cleanup, and interactive target guard. The stderr Real-scenario testing (tmux)Not applicable for this PR — the cell value dialog is a browser-rendered web component inside the web-shell. tmux-based CLI testing doesn't reach it. The author provided a video recording demonstrating the before/after behavior on macOS, which serves as the real-user evidence. 中文说明代码审查阅读了 diff 和完整组件文件。实现干净，遵循
未发现正确性缺陷、安全问题或 AGENTS.md 违规。 测试单元测试（vitest，本地）应用 PR 改动后运行完整测试文件： 全部 82 个测试通过——73 个已有 + 9 个新增。stderr 中的 真实场景测试（tmux）不适用——单元格值弹窗是 web-shell 中的浏览器渲染 web 组件，tmux CLI 测试无法覆盖。作者提供了 macOS 上的视频录制作为真实用户证据。 — Qwen Code · qwen3.7-max |
Collaborator
|
This is a solid, well-scoped feature PR. The problem is real (I've felt the pain of trying to grab a substring from a long table cell), the solution is the minimum viable approach (a read-only dialog with selectable text and copy), and the implementation is careful without being overwrought. Going back to what I would have done independently: I'd have added a modal dialog on double-click with text selection and copy — which is exactly what this PR does. The focus trapping, Escape propagation blocking, and state cleanup on open are details I might have gotten wrong on the first pass, but the author nailed them. The test suite is thorough — 9 new tests covering every dismiss path, focus behavior, and state interaction. All 82 tests pass locally. The video evidence shows it working on macOS. The risk section honestly identifies the behavior change (double-click now opens the dialog instead of being part of a text selection gesture). One minor note: the No blockers. Shipping this. ✅ 中文说明这是一个范围合理、实现扎实的功能 PR。问题是真实存在的（从长表格单元格中选取子串确实不方便），方案是最小可行路径（只读弹窗 + 文本选择 + 复制），实现细致但不冗余。 独立思考的话，我也会选择在双击时打开一个带文本选择和复制功能的模态弹窗——和本 PR 完全一致。焦点陷阱、Escape 阻止冒泡、打开时状态清理这些细节我第一版可能会遗漏，但作者处理得很到位。 测试全面——9 个新测试覆盖所有关闭路径、焦点行为和状态交互。本地全部 82 个测试通过。视频证据展示了 macOS 上的效果。Risk 部分诚实指出了行为变化。 一个小备注： 无阻塞问题。✅ — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
previously approved these changes
Jul 8, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
Collaborator
Maintainer verification report — real-browser runtime testI built this PR locally (head Verdict: the feature does exactly what the description says in a normal transcript. There is one real defect that appears once the transcript is virtualized, plus a test-coverage gap on three of the four dismiss paths. Details and reproductions below. 1. 🔴 The
|
| observable | PR as-is | expected |
|---|---|---|
scrim rect [x,y,w,h] |
[140, 260, 1000, 352] |
[0, 0, 1280, 800] |
| dialog centre Y | 436 |
400 |
| viewport probes intercepted by the scrim | 0 / 4 | 4 / 4 |
real mouse click at (640, 40) |
lands on <p> of another message |
lands on the scrim |
| …does that click dismiss the dialog? | no | yes |
| dialog Y while the transcript scrolls | 314 → 686 → 1086 (drifts off screen, then the virtualizer unmounts the row and the dialog silently vanishes) |
constant |
The containing block was confirmed programmatically: _virtualRow_… { transform: matrix(1,0,0,1,0,1320) }.
User-visible consequences:
- "Clicking outside it" — one of the four dismiss paths in the description — does not work. Only the ~1000×352 box around that message dismisses.
aria-modal="true"is not true: real clicks reach the transcript behind the dialog.- The dialog is off-centre, scrolls away with the content, and disappears without warning when its row leaves the virtual window.
Screenshot of the scrim confined to one message row (note the un-dimmed filler message … lines above and below):
filler message 57 ← not dimmed (outside the scrim)
filler message 58
filler message 59
┌──────────────────────────────────────────────┐ ← scrim starts here
│ Here is a table: │
│ ┌───────── Current field value ─────┐ │
│ │ Alpha [Copy] [Close] │ │
│ └───────────────────────────────────┘ │
└──────────────────────────────────────────────┘ ← scrim ends here
filler message 100 ← not dimmed
filler message 101
This is not a pattern the PR invented. The pre-existing .filterMenu (…module.css:243, also position: fixed, same parent) has the identical defect on main: with virtualization on, its inline top: 8px renders at y = 268 (drift 260px); with virtualization off, drift is 0. So the file already has one instance — this PR adds a second, more visible one (a full-screen modal instead of a dropdown).
Suggested fix — verified A/B in the browser
Portal the dialog, as every other overlay in this codebase already does (dialogs/DialogShell.tsx:298, AtMentionPanel.tsx:187, ChatEditor.tsx:706).
I applied createPortal(<backdrop/>, document.body) and re-ran the same probes:
| observable | PR as-is | + createPortal |
|---|---|---|
| scrim rect | [140, 260, 1000, 352] |
✅ [0, 0, 1280, 800] |
| dialog centre Y | 436 |
✅ 400 |
| probes intercepted by scrim | 0 / 4 | ✅ 4 / 4 |
real click at (640,40) reaches transcript |
yes | ✅ no |
| click-outside dismisses | no | ✅ yes |
dialog Y across scrollTop = 700 → 300 |
686 → 1086 |
✅ 281 → 281 |
Two things a naive portal breaks — please handle both:
- Theme variables are lost.
App.tsx:4194applies.themeDark/.themeLightto its own root div, so custom properties do not reachdocument.body. I measured the naive portal:--cardunset,background-image: none, scrimrgba(0,0,0,0),color: rgb(0,0,0)— a black-on-white dialog with an invisible Close button.DialogShellsolves this by re-declaring the vars inside its own.module.cssand putting the theme class on the portal root viauseTheme(); the same is needed here. - 7 of the 9 new unit tests assert with
container.querySelector('[role="dialog"]'). Under a portal they must becomedocument.querySelector(...)(I confirmed:7 failed | 75 passedafter portaling, purely from the query scope).
Severity context: markdownTableMode defaults to 'basic' (App.tsx:810) and nothing in this repo sets 'advanced', so this only bites embedders who opt into enhanced tables — i.e. exactly this feature's audience — once a session exceeds 200 transcript items.
2. 🟡 Three of the four dismiss paths (and focus restore) are untested
I mutation-tested the PR's own test file (82 tests) to check the new tests actually pin behaviour:
mutation applied to EnhancedMarkdownTable.tsx |
tests that fail |
|---|---|
openCellDialog() never called |
8 ✅ |
drop the isInteractiveSelectionTarget guard |
1 ✅ |
value: cell.text → value: 'X' |
2 ✅ |
drop the || cellDialog guard in clearActiveColumnOnEscape |
1 ✅ |
drop setSelection/setOpenFilterMenu/setDetailRowKey(null) |
2 ✅ |
| never focus the first element on open | 1 ✅ |
backdrop onMouseDown → no-op (click-outside) |
0 ❌ |
× icon + footer Close onClick → no-op |
0 ❌ |
| remove focus-restore on close | 0 ❌ |
All three unpinned behaviours do work — I verified each in the browser. They just aren't guarded against regression. Three short assertions would close this.
3. 🟡 The motivating case ("endpoint") is the one that doesn't work
remark-gfm autolinks bare URLs, so an endpoint cell renders as <a href=…>, and isInteractiveSelectionTarget (correctly) suppresses the dialog there. Result: double-clicking the URL text is a silent no-op; double-clicking the parts of the cell that aren't link glyphs opens the dialog. Measured on a wrapped URL cell: the link's line boxes cover only 23.3% of the cell area (3 line boxes), so the behaviour looks random to a user.
The description lists "endpoint" and "request id" as the motivating examples. Request ids (plain text) work perfectly; endpoints mostly don't. Worth a follow-up — e.g. allow the dialog when the double-click target is a link and the cell has no other interactive affordance, or add a hover "expand" affordance.
4. ✅ Everything else checks out (verified, not assumed)
- Full untruncated value shown (114-char id, exact match);
Copywrites exactly that string to the real system clipboard; label flips toCopied!. - Native partial text selection inside the value box works (
user-select: text) — a mid-string range round-trips exactly. - Empty cell copies
''(the guard isvalue == null, not!value) and the whole copy path mirrors the existingcopySelectiongeneration/timer/!navigator.clipboardstructure exactly. - Using
onMouseDownon the scrim rather thanonClickis the right call: a drag-select that overshoots onto the scrim does not dismiss the dialog. Nice detail. - Escape gating is load-bearing: with the dialog open, Escape closes it and does not run
clearActiveColumnOnEscape(sort/reorder handles stay visible). Mutation confirms the guard is what protects this. - Focus: lands on
×on open; DOM order keeps×first even when the cell value contains a link; focus is restored to the table scroller (not<body>) on close. - Opening the dialog clears the cell selection (
4 → 0selected cells), row details and any open filter menu. - No stale native word-selection leaks under the modal.
- i18n: EN 1478 keys / ZH 1597 keys, zero duplicates, all three new keys present in both;
当前字段值 / 复制 / 关闭render correctly. - 0 console errors, 0 page errors, in both non-virtualized and virtualized runs.
eslintclean on all touched files;tsc --noEmitreports nothing in the touched files.- Full
packages/web-shellsuite: 1269 / 1269 pass (build-artifact.test.tsneedsvite build --config vite.lib.config.tsrun first — a pre-existing ordering dependency, unrelated to this PR).
Non-virtualized behaviour is exactly as described — scrim [0,0,1280,800], all four dismiss paths work, dialog centred.
Recommendation
- Before merge: fix (1) — portal the dialog and carry the theme vars, and update the 7 tests. Without it, "click outside to dismiss" and
aria-modalare broken for any long session with advanced tables on. - Nice to have in this PR: the three assertions from (2).
- Follow-up issue: (3), plus the same portal fix for the pre-existing
.filterMenu.
Everything else is solid work — the copy/selection semantics, Escape gating and state clearing are all correct and I could not break them.
中文版报告
维护者验证报告 —— 真实浏览器运行时测试
我在本地构建了这个 PR（head 4bc7e12），并在真实的 headless Chromium 中驱动了完整的真实组件链路 MessageList → MessageItem → Markdown → EnhancedMarkdownTable，使用真实的 @tanstack/react-virtual、真实的 remark/rehype 管线和真实的系统剪贴板 —— 而不是 jsdom。
结论： 在普通会话中，功能完全符合描述。但存在一个真实缺陷，会在会话被虚拟滚动接管后出现；另外四条关闭路径中有三条没有测试覆盖。详情如下。
1. 🔴 虚拟滚动下 position: fixed 遮罩层会塌缩
.cellDialogBackdrop { position: fixed; inset: 0 }（EnhancedMarkdownTable.module.css:540）是直接渲染在表格容器内部的（EnhancedMarkdownTable.tsx:2414）。当会话消息数超过 VIRTUAL_SCROLL_THRESHOLD = 200（MessageList.tsx:1431）后，每条消息都会被包进一个带 transform: translateY(...) 的行里（MessageList.tsx:2894）。带 transform 的祖先元素会成为 position: fixed 的包含块，于是遮罩层塌缩到那一条消息行内部。
在 1280×800 视口下实测（在虚拟列表中的表格上打开弹窗）：
| 观测项 | PR 当前实现 | 期望 |
|---|---|---|
遮罩层矩形 [x,y,w,h] |
[140, 260, 1000, 352] |
[0, 0, 1280, 800] |
| 弹窗垂直中心 | 436 |
400 |
| 视口采样点被遮罩层拦截 | 0 / 4 | 4 / 4 |
在 (640, 40) 真实鼠标点击 |
命中另一条消息的 <p> |
命中遮罩层 |
| …该点击能关闭弹窗吗？ | 不能 | 能 |
| 滚动会话时弹窗的 Y 坐标 | 314 → 686 → 1086（漂出屏幕，随后虚拟化卸载该行，弹窗无声消失） |
保持不变 |
包含块已通过程序确认：_virtualRow_… { transform: matrix(1,0,0,1,0,1320) }。
用户可见的后果：
- 描述中列出的四条关闭路径之一 —— "点击弹窗外部" —— 失效。 只有那条消息周围约 1000×352 的区域才能关闭弹窗。
aria-modal="true"名不副实：真实点击可以穿透到弹窗背后的会话内容。- 弹窗不居中、会随内容滚动、并在其所在行离开虚拟窗口时毫无提示地消失。
这个模式不是本 PR 发明的。 已有的 .filterMenu（…module.css:243，同样 position: fixed，同一父节点）在 main 上就有完全相同的缺陷：开启虚拟滚动时，它的内联 top: 8px 实际渲染在 y = 268（偏移 260px）；关闭虚拟滚动时偏移为 0。也就是说这个文件里已经有一处，本 PR 又加了一处、而且更显眼（全屏模态 vs 下拉菜单）。
建议的修复 —— 我已在浏览器中做了 A/B 验证：
像本仓库其它所有浮层一样使用 portal（dialogs/DialogShell.tsx:298、AtMentionPanel.tsx:187、ChatEditor.tsx:706）。
我加上 createPortal(<backdrop/>, document.body) 后重跑了同样的探测：
| 观测项 | PR 当前实现 | 加 createPortal 后 |
|---|---|---|
| 遮罩层矩形 | [140, 260, 1000, 352] |
✅ [0, 0, 1280, 800] |
| 弹窗垂直中心 | 436 |
✅ 400 |
| 采样点被遮罩层拦截 | 0 / 4 | ✅ 4 / 4 |
(640,40) 真实点击穿透到会话 |
是 | ✅ 否 |
| 点击外部可关闭 | 否 | ✅ 是 |
scrollTop = 700 → 300 时弹窗 Y |
686 → 1086 |
✅ 281 → 281 |
但直接 portal 会带来两个问题，请一并处理：
- 主题变量会丢失。
App.tsx:4194把.themeDark/.themeLight加在它自己的根 div 上，因此这些自定义属性不会传递到document.body。我实测了直接 portal 的结果：--card未定义、background-image: none、遮罩层rgba(0,0,0,0)、color: rgb(0,0,0)—— 变成白底黑字、关闭按钮不可见的弹窗。DialogShell的做法是在自己的.module.css里重新声明这些变量，并通过useTheme()把主题 class 加在 portal 根节点上；这里需要同样处理。 - 9 个新增单测里有 7 个用
container.querySelector('[role="dialog"]')断言。portal 之后必须改成document.querySelector(...)（我验证过：portal 后7 failed | 75 passed，纯粹是查询作用域问题）。
严重程度补充： markdownTableMode 默认是 'basic'（App.tsx:810），且仓库内没有任何地方设置成 'advanced'。所以这个问题只影响主动开启增强表格的接入方 —— 也就是这个功能的目标用户 —— 且会话超过 200 条消息之后。
2. 🟡 四条关闭路径中的三条（以及焦点归还）没有测试
我对 PR 自带的测试文件（82 个用例）做了变异测试，检查新增测试是否真的锁住了行为：
对 EnhancedMarkdownTable.tsx 施加的变异 |
失败的用例数 |
|---|---|
openCellDialog() 永不调用 |
8 ✅ |
去掉 isInteractiveSelectionTarget 守卫 |
1 ✅ |
value: cell.text → value: 'X' |
2 ✅ |
去掉 clearActiveColumnOnEscape 中的 || cellDialog 守卫 |
1 ✅ |
去掉 setSelection/setOpenFilterMenu/setDetailRowKey(null) |
2 ✅ |
| 打开时不聚焦第一个可聚焦元素 | 1 ✅ |
遮罩层 onMouseDown 改为空函数（点击外部关闭） |
0 ❌ |
× 图标与底部 Close 的 onClick 改为空函数 |
0 ❌ |
| 移除关闭时的焦点归还 | 0 ❌ |
这三条未被锁住的行为实际都是正常工作的 —— 我在浏览器里逐条验证过。只是没有防回归保护。补三条断言即可。
3. 🟡 最典型的动机场景（"endpoint"）恰恰不生效
remark-gfm 会自动把裸 URL 转成链接，所以 endpoint 单元格渲染为 <a href=…>，而 isInteractiveSelectionTarget（正确地）在那里阻止了弹窗。结果是：双击 URL 文字本身没有任何反应；双击单元格中非链接文字的区域才会打开弹窗。在一个折行的 URL 单元格上实测：链接的行盒只覆盖单元格面积的 23.3%（3 个行盒），从用户视角看行为很随机。
描述里把 "endpoint" 和 "request id" 列为动机示例。request id（纯文本）工作得很好；endpoint 大多数情况下不行。建议后续跟进 —— 例如：当双击目标是链接、且该单元格没有其它交互元素时仍允许打开弹窗；或者在 hover 时提供一个显式的"展开"入口。
4. ✅ 其余部分都没问题（均为实测，非推断）
- 弹窗展示完整未截断的值（114 字符的 id，精确匹配）；
Copy把这个字符串原样写入真实系统剪贴板；按钮文案切换为Copied!。 - 值区域内的原生部分文本选择可用（
user-select: text）—— 中间一段范围可以精确取出。 - 空单元格复制出
''（守卫是value == null而非!value），整条复制路径与已有的copySelection的 generation / timer /!navigator.clipboard结构完全一致。 - 遮罩层用
onMouseDown而不是onClick是正确选择：在值区域内起手、拖选时越界到遮罩层上松开，不会误关弹窗。这个细节做得很好。 - Escape 的门控是有效的：弹窗打开时按 Escape 只关弹窗，不会同时触发
clearActiveColumnOnEscape（排序/列拖拽把手保持可见）。变异测试证明这个守卫确实在起作用。 - 焦点：打开时落在
×上；即使单元格值里含链接，DOM 顺序也保证×仍是第一个；关闭后焦点归还给表格滚动容器（而不是<body>）。 - 打开弹窗会清除单元格选区（
4 → 0个选中单元格）、行详情、以及已打开的筛选菜单。 - 弹窗背后不会残留原生的双击选词。
- i18n：EN 1478 个 key / ZH 1597 个 key，零重复，三个新 key 在两边都存在；
当前字段值 / 复制 / 关闭渲染正确。 - 虚拟化与非虚拟化两种模式下均为 0 console 错误、0 page 错误。
- 所有改动文件
eslint通过；tsc --noEmit在改动文件上无报错。 packages/web-shell全量测试：1269 / 1269 通过（build-artifact.test.ts需要先跑vite build --config vite.lib.config.ts—— 这是已有的顺序依赖，与本 PR 无关）。
非虚拟化场景下的行为与描述完全一致 —— 遮罩层 [0,0,1280,800]，四条关闭路径全部可用，弹窗居中。
建议
- 合并前： 修复 (1) —— portal 弹窗并且带上主题变量，并且更新那 7 个测试。否则对任何开启了增强表格的长会话，"点击外部关闭"和
aria-modal都是坏的。 - 本 PR 内最好补上： (2) 中的三条断言。
- 后续 issue： (3)，以及对已有
.filterMenu施加同样的 portal 修复。
其余部分都是扎实的工作 —— 复制/选区语义、Escape 门控、状态清理都正确，我没能把它们弄坏。
Collaborator
Author
|
已根据最新评审补充修复并推送：
验证：
|
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed — no blockers. Suggestion-level recommendations are in the Suggestion summary comment below.
Collaborator
|
@qwen-code /resolve |
Merge origin/main into feat/web-shell-cell-dialog. Conflicts in EnhancedMarkdownTable (css, tsx, test) resolved by keeping both sides: PR's cell dialog (theme variables, backdrop, focus trap, copy) and main's readability improvements (emptyValue style, density/longTextExpanded state, columnContextMenu guard). Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Collaborator
|
Qwen Code resolved the merge conflicts and pushed the branch update. Merge Conflict Resolution Summary — PR #6530Base branch: main Conflicting PRs on main
Files that conflicted (3)1.
|
| Region | PR side | main side | Resolution |
|---|---|---|---|
| State declarations | cellDialog state |
longTextExpanded + density state |
Kept all three state hooks |
| Reset handler | setCellDialog(null) |
setLongTextExpanded(false) + setDensity('standard') |
Kept all three resets |
| Escape-key guard | … || cellDialog) return |
… || columnContextMenu) return |
Combined: … || cellDialog || columnContextMenu) return |
| Effect dependency array | [cellDialog, openFilterMenu] |
[columnContextMenu, openFilterMenu] |
Combined: [cellDialog, columnContextMenu, openFilterMenu] |
3. EnhancedMarkdownTable.test.tsx
- PR side: Added
doubleClick()test helper. - main side: Added
rightClick(),openColumnMenu(), andfreezeFirstColumn()test helpers. - Resolution: Kept all four helpers. They serve independent interaction patterns (double-click for cell dialog, right-click for column context menu).
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed — no blockers. Suggestion-level recommendations are in the Suggestion summary comment below.
Collaborator
✅ Local validation report — PR #6530 (web-shell cell value dialog)I checked this PR out locally (worktree at PR head Results
End‑to‑end behavior verified (real app, not just unit tests)
Screenshots1) Enhanced table rendered · drag 2) Dialog on double‑click (dark) · drag 3) “Copied!” confirmation (dark) · drag 4) Dialog (light theme) · drag Findings🔴 1. Blocking but trivial — Fix is a one‑liner: 🟡 2. Bare‑URL cells don't open the dialog (worth noting). 🟡 3. Minor — copy uses normalized text. VerdictFeature works as described and is well covered by tests; the implementation is solid. The one blocking item before merge is the Prettier fix (finding #1). Findings #2/#3 are informational. 中文版本（点击展开）✅ 本地验证报告 — PR #6530（web-shell 单元格字段值弹窗）我在本地检出了该 PR（worktree 指向 PR head 结果
已验证的端到端行为（真实应用，而非仅单测）
截图见上方英文部分的截图。 发现🔴 1. 阻塞但很容易修 —— 修复只需一行： 🟡 2. 纯 URL 单元格不会打开弹窗（值得留意）。 🟡 3. 次要 —— 复制使用的是归一化文本。 结论功能与描述一致，测试覆盖充分，实现可靠。合并前唯一的阻塞项是 Prettier 格式修复（发现 #1）。发现 #2/#3 属于信息性提示。 |
wenshao
previously approved these changes
Jul 10, 2026
Collaborator
|
@qwen-code /triage |
Collaborator
|
@qwen-code /resolve |
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Collaborator
|
Qwen Code resolved the merge conflicts and pushed the branch update. Merge Conflict Resolution Summary — PR #6530Conflicted file
What conflictedThe PR branch (
ResolutionKept both sides:
No other files were modified — all other changes auto-merged cleanly. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
| document.activeElement instanceof HTMLElement | ||
| ? document.activeElement | ||
| : null; | ||
| setSelection(null); |
Collaborator
There was a problem hiding this comment.
[Suggestion] openCellDialog closes openFilterMenu but does not close columnContextMenu. Other overlay openers (toggleFilterMenu, openColumnContextMenu) consistently close competing overlays. If the column context menu happens to be open when the user double-clicks a cell, both overlays would briefly coexist.
Suggested change
| setSelection(null); | |
| setSelection(null); | |
| setOpenFilterMenu(null); | |
| setColumnContextMenu(null); | |
| setDetailRowKey(null); |
— qwen3.7-max via Qwen Code /review
| }; | ||
|
|
||
| const copyCellDialogValue = () => { | ||
| if (currentCellDialogText == null || !navigator.clipboard) return; |
Collaborator
There was a problem hiding this comment.
[Suggestion] When navigator.clipboard is unavailable (e.g., embedded iframes without clipboard-write permission), this early return gives the user no feedback — they click "Copy" and nothing happens. The same silent-no-op pattern exists in the other two copy functions (lines 2132, 2159), so this is a pre-existing gap. Consider disabling the Copy button or showing a brief toast when navigator.clipboard is falsy.
— qwen3.7-max via Qwen Code /review
| .cellDialogBackdrop { | ||
| position: fixed; | ||
| inset: 0; | ||
| z-index: 1000; |
Collaborator
There was a problem hiding this comment.
[Suggestion] .cellDialogBackdrop shares z-index: 1000 with .filterMenu (line 271) and .columnContextMenu (line 287). While openCellDialog closes the filter menu, the column context menu is not closed (see separate finding), and relying on DOM render order for stacking is fragile. A defensive bump to z-index: 1010 would prevent visual overlap if a race condition or future change leaves a sibling overlay open.
Suggested change
| z-index: 1000; | |
| z-index: 1010; |
— qwen3.7-max via Qwen Code /review
|
|
||
| vi.mock('@qwen-code/sdk/daemon', async (importOriginal) => ({ | ||
| ...(await importOriginal<typeof import('@qwen-code/sdk/daemon')>()), | ||
| vi.mock('@qwen-code/sdk/daemon', () => ({ |
Collaborator
There was a problem hiding this comment.
[Suggestion] The mock was changed from importOriginal-based spread to a hardcoded partial object. The previous pattern guaranteed all real exports remained available to transitively-imported modules. If DAEMON_GOAL_STATUS_SENTINEL_PREFIX is renamed or a new runtime export is added, this mock silently diverges and tests fail with opaque undefined is not a function errors.
Suggested change
| vi.mock('@qwen-code/sdk/daemon', () => ({ | |
| vi.mock('@qwen-code/sdk/daemon', async (importOriginal) => ({ | |
| ...(await importOriginal<typeof import('@qwen-code/sdk/daemon')>()), | |
| isDaemonTurnError: () => false, | |
| })); |
— qwen3.7-max via Qwen Code /review
| expect(testState.latestChatEditorProps?.dialogOpen).toBe(true); | ||
| }); | ||
|
|
||
| it('blocks app-level shortcuts while an external modal is registered', async () => { |
Collaborator
There was a problem hiding this comment.
[Suggestion] This test registers an interaction blocker and verifies shortcuts are suppressed, but never releases the blocker and verifies shortcuts are restored. The Math.max(0, count - 1) decrement path and dialogOpen returning to false are untested. A bug in the release function would permanently lock out Ctrl+L/O/Y shortcuts with no recovery path.
Consider extending the test to click the probe a second time (releasing the blocker), then dispatch Ctrl+L and assert mockStore.reset IS called.
— qwen3.7-max via Qwen Code /review
| @@ -1727,9 +1851,53 @@ export function EnhancedTable({ | |||
|
|
|||
| const toggleRowDetail = (rowKey: string) => { | |||
| setSelection(null); | |||
Collaborator
There was a problem hiding this comment.
[Suggestion] toggleRowDetail now closes the cell dialog (forward direction), but only the reverse direction is tested ("clears table selection and row details when opening a cell dialog"). If setCellDialog(null) were accidentally removed, both the cell dialog and row details panel could coexist with overlapping UI and conflicting Escape handling.
Consider adding a test: open a cell dialog via double-click, then toggle row details, and assert the cell dialog is gone.
— qwen3.7-max via Qwen Code /review
wenshao
previously approved these changes
Jul 11, 2026
Collaborator
|
@qwen-code /resolve |
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Collaborator
|
Qwen Code resolved the merge conflicts and pushed the branch update. Merge Conflict Resolution SummaryPR: #6530 — feat(web-shell): add cell value dialog on double-click in markdown tables Conflicted file
Conflict analysisThe PR branch wrapped the chat content area in an
ResolutionCombined both changes:
No other files were modified. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
| vi.mock('@qwen-code/sdk/daemon', async (importOriginal) => ({ | ||
| ...(await importOriginal<typeof import('@qwen-code/sdk/daemon')>()), | ||
| vi.mock('@qwen-code/sdk/daemon', () => ({ | ||
| DAEMON_GOAL_STATUS_SENTINEL_PREFIX: 'qwen-goal-status:', |
Collaborator
There was a problem hiding this comment.
[Suggestion] The mock hardcodes DAEMON_GOAL_STATUS_SENTINEL_PREFIX: 'qwen-goal-status:' but the real value in packages/sdk-typescript/src/daemon/ui/sentinels.ts is 'web-shell:goal-status:v1:'. The previous mock used importOriginal which kept this value in sync automatically. If the real sentinel prefix changes, this mock continues passing with a stale value, potentially masking integration issues.
Suggested change
| DAEMON_GOAL_STATUS_SENTINEL_PREFIX: 'qwen-goal-status:', | |
| DAEMON_GOAL_STATUS_SENTINEL_PREFIX: 'web-shell:goal-status:v1:', |
— qwen3.7-max via Qwen Code /review
| setDetailRowKey(null); | ||
| } | ||
| }, [detailRowKey, visibleRows]); | ||
| if ( |
Collaborator
There was a problem hiding this comment.
[Suggestion] The useEffect that closes the cell dialog when its referenced row disappears from visibleRows has no corresponding test. The existing "keeps the cell value dialog in sync with table updates" test only changes cell values but never removes the row entirely. A test should open a cell dialog, re-render with the row filtered/sorted/removed, and verify the dialog auto-closes.
— qwen3.7-max via Qwen Code /review
wenshao
approved these changes
Jul 11, 2026
Collaborator
|
@qwen-code /triage |
1 similar comment
Collaborator
|
@qwen-code /triage |
qwen-code-ci-bot
approved these changes
Jul 11, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
Adds a read-only cell value dialog to enhanced Markdown tables in the web shell. Double-clicking a table cell opens a compact modal that shows the full field value, allows selecting part of the text, and provides a copy action for the full value. The dialog can be dismissed with Escape, the close icon, the close button, or by clicking outside it.
Why it's needed
Enhanced Markdown tables already support copying selected cells, but users sometimes need only part of a long cell value, such as a region, request id, endpoint, path, or error fragment. When the table view wraps or truncates dense content, copying a precise substring is awkward. This gives users an Excel-like inspection path without changing the existing table selection and TSV copy behavior.
Reviewer Test Plan
How to verify
Open a web-shell message that renders an enhanced Markdown table. Double-click any data cell and confirm a compact “Current field value” dialog appears with the full cell value. Drag-select part of the text inside the value box and copy it to confirm native text selection works. Click the dialog “Copy” button and confirm it copies the full cell value. Press Escape, click the close icon, click the “Close” button, and click outside the dialog to confirm each path dismisses it. Select one or more table cells, then double-click a cell and confirm the previous cell selection is cleared so TSV copy state does not conflict with the dialog.
Run the focused unit test:
Expected result: the enhanced table test file passes.
Evidence (Before & After)
Before: enhanced tables supported copying selected cells or the visible table, but there was no focused read-only view for selecting and copying only part of a single cell value.
After: double-clicking a cell opens a compact current-field-value dialog with selectable text and copy/close actions.
2026-07-08.16.50.09.mov
Tested on
Environment (optional)
Verified locally with a Vite web-shell demo page and the focused enhanced table Vitest file. The focused test run passed with 77 tests.
Risk & Scope
Linked Issues
None.
中文说明
What this PR does
为 web shell 的增强 Markdown 表格增加只读单元格值弹窗。双击表格单元格后，会打开一个紧凑弹窗展示完整字段值，用户可以选择其中一部分文本，也可以通过复制按钮复制完整字段值。弹窗支持通过 Escape、右上角关闭图标、关闭按钮或点击弹窗外部关闭。
Why it's needed
增强 Markdown 表格已经支持复制选中的单元格，但用户有时只需要长单元格值中的一部分，例如 region、request id、endpoint、路径或错误片段。当表格视图中的密集内容换行或显示受限时，精确复制子串比较麻烦。这个改动提供了类似 Excel 的查看路径，同时不改变现有表格选择和 TSV 复制行为。
Reviewer Test Plan
How to verify
打开一条会渲染增强 Markdown 表格的 web-shell 消息。双击任意数据单元格，确认出现紧凑的“Current field value”弹窗并展示完整单元格值。在值区域内拖选部分文本并复制，确认原生文本选择可用。点击弹窗中的“Copy”按钮，确认会复制完整单元格值。分别按 Escape、点击关闭图标、点击“Close”按钮、点击弹窗外部，确认这些路径都能关闭弹窗。先选择一个或多个表格单元格，再双击某个单元格，确认之前的单元格选择会被清除，避免 TSV 复制状态与弹窗冲突。
运行聚焦单测：
预期结果：增强表格测试文件通过。
Evidence (Before & After)
Before：增强表格支持复制选中的单元格或可见表格，但没有一个聚焦的只读视图来选择并复制单个单元格值中的一部分内容。
After：双击单元格会打开紧凑的当前字段值弹窗，弹窗内文本可选择，并提供复制和关闭操作。
2026-07-08.16.50.09.mov
Tested on
Environment (optional)
已通过本地 Vite web-shell demo 页面和增强表格聚焦 Vitest 文件验证。聚焦测试运行通过，共 77 个测试。
Risk & Scope
Linked Issues
无。