fix(web-shell): refine markdown table interactions - #6500
Conversation
|
Thanks for the PR! Re-running triage after the follow-up commit Template looks good ✓ — all required sections present with bilingual body. Problem: This is a follow-up to the already-merged #6444, addressing interaction and performance edge cases found during code review (resize jank, keyboard shortcut swallowing, stale move handles). The prior PR provides context and the test evidence (73 passing tests, up from 70) confirms the refinements work as intended. Direction: Aligned — targeted polish-level fixes to a recently shipped feature. No product-direction concerns. Size: Not applicable — changes are in Approach: Scope feels right — each change maps directly to a named edge case from the review. The Moving on to code review. 🔍 中文说明感谢贡献!在后续提交 模板完整 ✓ — 所有必要章节齐全,含双语正文。 问题:这是已合并的 #6444 的后续优化,解决代码评审中发现的交互和性能边界问题(拖拽卡顿、键盘快捷键被吞、残留移动手柄)。前序 PR 提供了上下文,测试证据(73 个测试通过,从 70 个增加)确认了优化效果。 方向:对齐 — 针对近期上线功能的精准打磨,无产品方向问题。 规模:不适用 — 改动在 方案:范围合理 — 每个改动直接对应评审中命名的边界问题。 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Code ReviewRe-reviewing after follow-up commit Changes from initial commit:
Open review suggestion (from bot): The "stops resizing when page visibility changes" test doesn't dispatch No blockers found. Test ResultsThe Real-Scenario TestingN/A — this is a browser-based web-shell React component. The changes (resize drag batching, keyboard shortcuts, focus management, Escape layering) require a browser environment. The author reports manual verification on 中文说明代码审查在后续提交 相对初始提交的变更:
未解决的评审建议(来自 bot): "页面可见性变化时停止拖拽"测试在 未发现问题。 测试结果stderr 中的 真实场景测试不适用 — 这是基于浏览器的 web-shell React 组件。改动(拖拽合并、键盘快捷键、焦点管理、Escape 分层)需要浏览器环境。作者报告在 Chrome 的 — Qwen Code · qwen3.7-max |
|
This PR does what it says — four targeted refinements to table column interactions, each traceable to a specific review finding. The follow-up commit The code is straightforward: a small helper to eliminate duplication, a standard rAF batching pattern, an early-return guard for keyboard modifiers, global listeners for focus cleanup, and layered Escape handling. Nothing tries too hard. The test suite (73 tests, all passing) covers every new behavior including the Escape layering edge case. The diff is minimal — no unrelated changes, no speculative additions. If I had to maintain this in six months, the code reads clearly enough that I wouldn't need to reach for the author. One minor test-robustness note from the bot review (visibility test doesn't exercise the pending-width flush path directly) remains unaddressed, but the same code path is covered by the window blur test. Not a blocker. Approving. ✅ 中文说明这个 PR 做到了它所承诺的——四项针对表格列交互的精准优化,每项都可追溯到具体的评审发现。后续提交 代码简洁明了:一个小 helper 消除重复、标准 rAF 合并模式、键盘修饰键的 early-return 守卫、全局监听器处理焦点清理、以及分层 Escape 处理。没有过度设计。 测试套件(73 个测试,全部通过)覆盖了每个新行为,包括 Escape 分层边界情况。diff 最小化——无无关改动,无投机性添加。如果六个月后要维护这段代码,清晰度足够高,不需要找原作者。 bot 评审中有一个小建议(visibility 测试未直接验证待提交宽度刷新路径)未解决,但同一代码路径已由窗口 blur 测试覆盖。非阻塞项。 批准合并。 ✅ — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
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
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestion-level recommendations are in the Suggestion summary comment below.
Suggestions — commit
|
| File | Issue | Suggested fix |
|---|---|---|
EnhancedMarkdownTable.test.tsx |
stopResize pending-width flush path untested — no test does mousemove → blur → asserts final width reflects the pending mousemove | Add: mousedown → mousemove(clientX=200) → blur → assert width matches the mousemove position |
EnhancedMarkdownTable.test.tsx |
visibilitychange with document.hidden=false negative case untested — if stopResizeWhenHidden is reverted to unconditional stopResize, existing test still passes |
Add: start resize → visibilitychange with hidden=false → mousemove → assert width changed |
— qwen3.7-max via Qwen Code /review
|
Follow-up for the review suggestions has been addressed in Changes made:
Verification:
The resolved review thread was also marked resolved on GitHub. |
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestion-level recommendations are in the Suggestion summary comment below.
Suggestions — commit
|
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
What this PR does
Refines enhanced markdown table column interactions after follow-up review: resize drag updates are coalesced, keyboard shortcuts with modifiers are left to the browser and assistive tech, active move handles clear when focus moves away, and frozen active headers keep their highlight.
Why it's needed
The already-merged table controls work, but review found interaction and performance edge cases that could cause resize jank, swallow keyboard shortcuts, or leave move handles visible longer than intended.
Reviewer Test Plan
How to verify
Evidence (Before & After)
PATH="/Users/jifeng/.nvm/versions/node/v24.14.0/bin:$PATH" npx vitest run client/components/messages/EnhancedMarkdownTable.test.tsxpassed with 70 tests.git diff --check -- client/components/messages/EnhancedMarkdownTable.tsx client/components/messages/EnhancedMarkdownTable.test.tsx client/components/messages/EnhancedMarkdownTable.module.csspassed.Tested on
Environment (optional)
Node v24.14.0 for local focused tests.
Risk & Scope
Linked Issues
Follow-up to #6444.
中文说明
What this PR does
根据后续评审继续优化增强版 Markdown 表格的列交互:列宽拖拽更新按动画帧合并,带修饰键的键盘快捷键交还给浏览器和辅助技术处理,激活的列移动手柄会在焦点移开时清除,冻结且激活的表头会保留高亮。
Why it's needed
已合并的表格控制功能整体可用,但评审发现了一些交互和性能边界问题,可能导致列宽拖拽卡顿、吞掉键盘快捷键,或让列移动手柄显示时间过长。
Reviewer Test Plan
How to verify
Evidence (Before & After)
PATH="/Users/jifeng/.nvm/versions/node/v24.14.0/bin:$PATH" npx vitest run client/components/messages/EnhancedMarkdownTable.test.tsx通过,70 个测试通过。git diff --check -- client/components/messages/EnhancedMarkdownTable.tsx client/components/messages/EnhancedMarkdownTable.test.tsx client/components/messages/EnhancedMarkdownTable.module.css通过。Tested on
Environment (optional)
本地聚焦测试使用 Node v24.14.0。
Risk & Scope
Linked Issues
#6444 的后续修复。