feat(web-shell): refine advanced table controls - #7999
Merged
Conversation
Collaborator
Author
E2E test reportTested the standalone Web Shell locally in the in-app browser.
Automated verification:
|
Collaborator
Collaborator
|
Thanks for the PR! Template looks good ✓ Problem: this is a design refinement, not a bug fix — the old advanced table mixed sorting, filtering, and column visibility inside each header's popover, making the interaction model hard to discover. The design doc ( Direction: aligned. Web Shell table usability is squarely within scope, and the PR follows the AGENTS.md Web Shell conventions — shared UI primitives ( Size: 1 173 production-logic lines (811 TSX + 332 CSS + 27 i18n + 2 App CSS + 1 main.tsx) vs. 359 test lines. All changes are inside Approach: the scope feels right for the stated goal. Separating sort → dedicated icon, filter → filter-only popover, visibility + detail fields → a single "Custom columns" popover is a clean decomposition. Replacing the density cycle-button with a Risk: no elevated risk signals — none of the changed files match the high-revert-path patterns. Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题：这是一个设计优化，不是 bug 修复——旧版高级表格在每个表头弹窗中混合了排序、筛选和列可见性操作，交互模型不易发现。设计文档（ 方向：对齐。Web Shell 表格可用性完全在范围内，PR 遵循了 AGENTS.md 的 Web Shell 规范——使用共享 UI 原语（ 规模：1 173 行生产逻辑（811 TSX + 332 CSS + 27 i18n + 2 App CSS + 1 main.tsx），359 行测试。所有改动在 方案：范围与目标匹配。将排序→专用图标、筛选→纯筛选弹窗、可见性+详情字段→单个"自定义列"弹窗，是清晰的职责拆分。密度循环按钮换成 风险：无升级风险信号——改动文件均未命中高回滚路径模式。 进入代码审查 🔍 — Qwen Code · qwen3.8-max-preview Reviewed at |
Collaborator
Code reviewIndependent proposal: given the goal of separating sort/filter/visibility and adding independent detail-field config, I would (1) pull sort out of the filter popover into a dedicated header icon, (2) move column visibility into a standalone popover, (3) add a separate Comparison with the diff: the implementation matches the proposal. No correctness bugs, security holes, or regressions surfaced. A few observations:
No AGENTS.md violations. Shared UI primitives used throughout; portal root handled at the primitive level; CSS Modules for component styles; tests collocated. Files changed (9 of 9)
CI test evidenceAll PR CI checks completed on the reviewed commit. The unit suite and the Web Shell E2E smoke both passed. The
The author tested on macOS only; Windows and Linux visual rendering is not verified. The responsive container-query layout at narrow viewports is also not covered by CI. Sandboxed verification would settle the behavioral claims: 中文说明代码审查独立方案： 给定分离排序/筛选/可见性并添加独立详情字段配置的目标，我会 (1) 将排序从筛选弹窗中抽出到专用表头图标，(2) 将列可见性移入独立弹窗，(3) 添加单独的 与 diff 的比较： 实现与方案匹配。未发现正确性 bug、安全漏洞或回归。几个观察：
无 AGENTS.md 违规。全程使用共享 UI 原语；portal root 在原语层处理；组件样式使用 CSS Modules；测试与源码同目录。 CI 测试证据所有 PR CI 检查在审查提交上已完成。单元测试套件和 Web Shell E2E 冒烟测试均通过。 作者仅在 macOS 上测试；Windows 和 Linux 视觉渲染未验证。窄视口下的响应式容器查询布局也未被 CI 覆盖。沙箱验证可以确认行为声明： — Qwen Code · qwen3.8-max-preview Reviewed at |
Collaborator
|
Confidence: 4/5 — solid, coherent redesign with thorough tests; only non-blocking nits (zero-visible-columns edge case, macOS-only visual verification). Stepping back: this PR does one thing — restructure the advanced table's interaction model — and does it completely. The design doc up front, the clean separation of sort/filter/visibility/detail concerns, the consistent icon language, and the test rewrite all point in the same direction. My independent proposal landed on essentially the same decomposition, which is a good sign that the approach is the natural one for this problem. The code is straightforward. The two reservations are minor: hiding all columns is a legal but slightly odd state (the old guard prevented it), and the visual/rendering claims rest on macOS-only testing. Neither blocks merge — the first is a design choice the author can confirm, and the second is covered by the CI visual-capture job plus the unit suite. If I had to maintain this in six months, I would thank the author: the interaction model is easier to reason about than the old one, the design doc explains why, and the tests match the new behavior. Approving. ✅ 中文说明置信度：4/5 — 扎实、内聚的重设计，测试充分；仅有非阻塞的小问题（全隐藏列边界情况、仅 macOS 视觉验证）。 退一步看：这个 PR 做了一件事——重构高级表格的交互模型——而且做得完整。前置的设计文档、排序/筛选/可见性/详情职责的清晰拆分、一致的图标语言、以及测试重写，都指向同一方向。我的独立方案得出了基本相同的分解方式，这说明方案是该问题的自然解。 代码直截了当。 两个保留意见都是次要的：隐藏所有列是合法但略奇怪的状态（旧代码有保护），视觉/渲染声明仅基于 macOS 测试。两者都不阻塞合并——第一个是作者可以确认的设计选择，第二个由 CI 视觉捕获作业和单元测试套件覆盖。 如果六个月后我要维护这段代码，我会感谢作者：交互模型比旧版更容易推理，设计文档解释了原因，测试与新行为匹配。 批准。✅ — Qwen Code · qwen3.8-max-preview Reviewed at |
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. |
Collaborator
🖼️ web-shell visual previewRendered against a mock daemon (no real backend): the PR base vs this PR head Screenshots · before / afterℹ️ No screenshot changed against the PR base — but this PR edits 5 render-shaping files:
Either the change has no visual effect (logic, plumbing, a state the scenarios never reach), or no scenario renders this UI — in which case the preview cannot see it, and an empty result is a coverage gap rather than a clean bill of health. To make it visible, add a scenario to Full-resolution recordings (.webm) are attached to the workflow run. — Qwen Code · web-shell visuals |
qwen-code-ci-bot
previously approved these changes
Jul 29, 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 — CI landed green after the review. ✅
Collaborator
Author
|
@qwen-code /takeover |
qwen-code-dev-bot
added
the
autofix/takeover
chiga0
previously approved these changes
Jul 29, 2026
chiga0
left a comment
chiga0
left a comment
Collaborator
There was a problem hiding this comment.
Review — PR #7999 feat(web-shell): refine advanced table controls
Verdict: APPROVE — no Critical or Major issues at HEAD 88aa712.
Summary
Restructures the advanced Markdown table interaction model per the bundled design doc: sort moves to a dedicated header icon (header text is no longer a button), the filter popover becomes filter-only, column visibility/ordering moves into a shadcn-based CustomColumnsPopover with drag-to-reorder, expanded-row detail fields become independently configurable (source order, not draggable), density becomes a Select instead of a cycle button, and the toolbar gains Lucide icons + tooltips. main.tsx enables markdownTableMode: 'advanced' by default in standalone.
Verification highlights
- State integrity:
detailColumnsinitialized to all columns, reset both on table-data change (existing reset path) and via the popover's Reset (which also restorescolumnOrderandhiddenColumns).detailColumnIndexesmemo filters throughinitialColumnOrderso detail fields always render in source order regardless of table column reordering. - Hide-all-columns edge: the old last-visible-column guard is intentionally removed — recovery is always available via the always-accessible popover (toggle-all checkbox / Reset).
toggleAllTableColumns(false)defensively clears filters and sort. Empty detail selection shows a proper empty-state message. - Cross-table drag protection preserved:
dropColumngates on the component-localdraggingColumnRef+ MIME check, so drags from another table instance's popover are still rejected even though the header drag handles are gone. - New outside-click selection clearing: effect guarded by
selection != null, uses existingshellRef(attached to root shell div),stopDraggingis a stableuseCallback— deps are correct. - A11y: all icon-only buttons carry
aria-label+sr-onlytext; checkboxes have associatedLabels viauseId-scoped ids; toggle-all checkboxes express indeterminate state; actions header issr-only. - Tests: comprehensive migration — new popover helpers, drag-through-popover paths, density via
selectValue, detail-field independence, reset behavior, filter-menu exclusion assertions, outside-click clearing, and zh-CN locale updates. Removed tests correspond exactly to removed UI (header drag handles, moveVisibleColumn slot logic, last-column guard).
Minor observations (non-blocking)
- The removed "ignores column drags from another table" test relied on header handles; the protection mechanism (per-instance ref) is unchanged but no longer has a direct regression test for the popover path.
- The outside-mousedown effect re-subscribes on every
selectionchange (each drag update) — correct but slightly chatty; negligible at this scale.
Independent blind review (7-round + structural audit + adversarial pass) followed by cross-validation against existing findings.
ytahdn
removed
the
autofix/takeover
…ced-table-preview # Conflicts: # packages/web-shell/client/components/messages/EchartsFullDataBlock.test.tsx
Collaborator
Author
|
@qwen-code /triage |
qwen-code-ci-bot
approved these changes
Jul 29, 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. ✅
chiga0
approved these changes
Jul 29, 2026
chiga0
left a comment
chiga0
left a comment
Collaborator
There was a problem hiding this comment.
Re-review after merge commit 7175e0b (re-posting dismissed approval)
Verdict: APPROVE — no new issues introduced by the merge from main.
Merge verification
- PR-owned files unchanged:
EnhancedMarkdownTable.tsx/.module.css/.test.tsx,AssistantMessage.test.tsx,App.module.css, and the design doc are byte-identical to the88aa712state I reviewed (confirmed via compare). - Conflict resolution correct:
EchartsFullDataBlock.test.tsxwas deleted on main (replaced byMarkdownChartRenderer); the PR's one-line rename in that file is correctly dropped, and the newMarkdownChartRendererhas zero coupling toEnhancedMarkdownTable. - i18n.tsx: all PR-added keys (
customColumns.*,copyTsvHint,densityLabel,densityCurrent, updatedcopyVisible/rows) intact in both EN and ZH after merge. - main.tsx: both this PR's
markdownTableMode: 'advanced'(line 171) and the merged #7926 session-path changes coexist correctly. - Markdown.test.ts: consistently asserts 'Copy table' (4 occurrences), no stale 'Quick copy' references.
Original review findings (state integrity, hide-all-columns recoverability, cross-table drag protection, a11y, test coverage) remain valid — see previous review body.
Independent blind review (7-round + structural audit + adversarial pass) followed by cross-validation against existing findings.
Collaborator
|
@qwen-code /takeover |
qwen-code-dev-bot
added
the
autofix/takeover
Collaborator
|
🤝 Takeover engaged: the autofix loop now manages this PR — it will address new review feedback and resolve base conflicts until the label is removed or the round cap is reached. Remove the 中文说明🤝 已接管：autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突，直到移除标签或达到轮次上限。移除 |
Collaborator
|
🤖 AutoFix could not start — a setup step failed (or the run was cancelled) before the agent ran, so no fix was attempted. This is normally a transient infra issue, a broken base build, or a cancelled run — not this PR. It will retry on the next scan. AutoFix failed before producing a verified commit (the run crashed or timed out before it could explain why). Run log: https://github.com/QwenLM/qwen-code/actions/runs/30432317505 🧠 Handled by Qwen Code · model/模型 |
wenshao
pushed a commit
that referenced
this pull request
Jul 29, 2026
The engage ack rode a pull_request:labeled round-trip: takeover-command applies the label, the labeled event routes, and the takeover-ack job posts the confirmation. That event has now been observed to simply not fire twice in one day (#7999 — the author read the silence as failure and removed the label; #8002 — an engaged fork PR with no ack for hours), and fork label events can never ack at all since they carry no secrets: a fork /takeover stayed silent until the next scan picked the PR up (2h41m on #7993). takeover-command now posts the engage ack directly after applying the label — every admission gate has already passed at that point, so 'engaged' is truthful for in-repo and fork PRs alike; the fork variant adds the expectation that the first round comes from the next scheduled scan. The route side suppresses the label-path ack when the label sender is the bot (only the ack: the immediate scan still routes), and the review-scan's existing first-pickup ack dedups against the command's comment and heals it if the post failed. Two more silent paths become audible while here: a /takeover on a stacked (non-main-base) PR now refuses out loud instead of dropping with only a log line, and a /takeover stop on a non-main PR now proceeds to remove the label instead of leaving it stuck.
pull Bot
pushed a commit
to Little-Star888/qwen-code
that referenced
this pull request
Jul 30, 2026
…wenLM#8043) * fix(autofix): post the takeover engage ack from the command itself The engage ack rode a pull_request:labeled round-trip: takeover-command applies the label, the labeled event routes, and the takeover-ack job posts the confirmation. That event has now been observed to simply not fire twice in one day (QwenLM#7999 — the author read the silence as failure and removed the label; QwenLM#8002 — an engaged fork PR with no ack for hours), and fork label events can never ack at all since they carry no secrets: a fork /takeover stayed silent until the next scan picked the PR up (2h41m on QwenLM#7993). takeover-command now posts the engage ack directly after applying the label — every admission gate has already passed at that point, so 'engaged' is truthful for in-repo and fork PRs alike; the fork variant adds the expectation that the first round comes from the next scheduled scan. The route side suppresses the label-path ack when the label sender is the bot (only the ack: the immediate scan still routes), and the review-scan's existing first-pickup ack dedups against the command's comment and heals it if the post failed. Two more silent paths become audible while here: a /takeover on a stacked (non-main-base) PR now refuses out loud instead of dropping with only a log line, and a /takeover stop on a non-main PR now proceeds to remove the label instead of leaving it stuck. * fix(autofix): ack command-driven releases directly and key the scan grace on the label actor Review follow-up: the engage-side fix left the release side on the fragile round-trip — a loud add next to a mute stop re-creates the exact 'did it work or did the event get lost?' ambiguity this PR set out to remove, now on release. And a fork or non-main release could never ack at all (fork unlabeled events carry no secrets; the route ignores non-main releases). - takeover-command now posts the release ack directly after removing the label, mirroring the engage side, with the same three variants as the ack job (plain release / bot-authored / bot-authored+skip) chosen from the same PR_INFO the gates used. The route suppresses the unlabeled-path ack when the label sender is the bot. - The scan's first-pickup grace is keyed on the label event's actor: a bot-applied label defers only 45s (the command's own write is seconds behind the label — fork or in-repo alike), so the immediate scan still heals a failed command ack instead of slipping to the next scheduled tick, and an ic.json snapshot taken between the label write and the command ack cannot double-post. A human in-repo label keeps the 3-minute ack-job grace; a human fork still posts right away. - The fork note now says 'usually within minutes', matching the */10 cron instead of contradicting it by 6x. - Tests kill the two surviving mutations from review: the EN/ZH fork-note swap (per-language-half assertions) and the warning fallback downgraded to '|| true' (both fallback strings pinned). * test(autofix): behaviorally pin the LAST_LABELED_BY jq extraction (QwenLM#8043) * fix(autofix): correct stale comments about scan heal timing and command blast radius (QwenLM#8043) * test(autofix): pin release-ack body identity across the command and ack jobs (QwenLM#8043) --------- Co-authored-by: verify <verify@local> Co-authored-by: Qwen Code Autofix <qwen-code-autofix@users.noreply.github.com>
Collaborator
|
Released in v0.21.2. |
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
Refreshes the Web Shell advanced Markdown table with a compact toolbar, dedicated filter and sort controls, a shadcn-based custom-columns popover, independently configurable expanded-row fields, and Lucide icons. It keeps selection statistics, TSV and full-table copy, column resizing and ordering, first-column freezing, long-text expansion, full-value dialogs, and single-row expansion while tightening table density, alignment, responsive behavior, and light-theme detail styling.
Why it's needed
The previous advanced table mixed sorting, filtering, and visibility actions in each header and made column and expanded-row configuration difficult to understand. The updated interaction model separates those responsibilities, makes expanded details readable without internal scrolling, and aligns the component with the latest Web Shell visual design.
Reviewer Test Plan
How to verify
Render a completed Markdown table in standalone Web Shell. Confirm the toolbar is 38px high, headers are 40px high, standard rows are 48px high, and selecting numeric cells shows primary statistics and a visible Copy TSV button without covering the header. Use the custom-columns popover to hide and reorder table columns, independently select expanded-row fields, and reset both groups. Expand multiple rows and confirm only one remains open, all selected detail fields are visible within the table viewport, and the detail content aligns with the first data column. Sort only from the sort icon, filter only from the filter icon, resize columns, freeze the first visible column, copy the visible table, copy a selected TSV range, expand long text, and open a full cell value.
Evidence (Before & After)
Before: header menus combined sorting, filtering, and hiding; row details followed visible columns; toolbar and detail spacing did not match the current design.
After: sorting and filtering have dedicated Lucide controls, column and detail-field configuration are independent, selected details expand responsively without internal vertical scrolling, and compact toolbar/table dimensions remain aligned across hover, selection, and expanded states.
Tested on
Environment (optional)
Local standalone Web Shell at the development server, verified in the Codex in-app browser.
Risk & Scope
Linked Issues
N/A
中文说明
本 PR 的改动
更新 Web Shell 高级 Markdown 表格，提供紧凑工具栏、独立的筛选与排序控件、基于 shadcn 的自定义列弹窗、可独立配置的展开行字段以及 Lucide 图标。继续保留框选统计、TSV 与整表复制、列宽调整和排序、冻结首列、长文本展开、完整内容弹窗及单行展开能力，同时优化表格密度、对齐、响应式行为和浅色主题详情样式。
为什么需要
旧版高级表格在每个表头中混合了排序、筛选和隐藏操作，列配置与展开行配置也不够清晰。新的交互模型拆分了这些职责，让展开详情无需表格内部滚动即可阅读，并使组件符合最新的 Web Shell 视觉设计。
Reviewer Test Plan
验证方式
在独立 Web Shell 中渲染一张已完成的 Markdown 表格。确认工具栏高度为 38px、表头为 40px、标准内容行为 48px，并确认框选数值单元格后会显示主要统计信息和明确的“复制 TSV”按钮，且不会覆盖表头。使用自定义列弹窗隐藏和移动表格列、独立选择展开行字段，并重置两组配置。依次展开多行，确认始终只有一行保持展开、所有选中的详情字段都在表格可视区域内展示，并且详情内容与第一数据列对齐。确认排序仅由排序图标触发、筛选仅由筛选图标触发，同时验证列宽调整、冻结第一可见列、复制可见表格、复制选中 TSV、展开长文本和查看单元格完整内容。
前后对比证据
改动前：表头菜单混合排序、筛选和隐藏操作；展开详情跟随可见列；工具栏和详情间距不符合当前设计。
改动后：排序与筛选使用独立 Lucide 控件；表格列与详情字段独立配置；选中的详情可响应式展开且没有表格内部纵向滚动；紧凑工具栏和表格尺寸在悬停、框选及展开状态下保持对齐。
测试平台
环境（可选）
本地开发服务器上的独立 Web Shell，并通过 Codex 应用内浏览器验证。
风险与范围
关联 Issue
N/A