feat(web-shell): improve markdown table readability - #6626
Merged
Conversation
Collaborator
|
Thanks for the PR! Template looks good ✓ Problem: Feature addition — improving Markdown table readability in the web shell. Not a bug fix, so no reproduction needed. The PR provides clear before/after evidence from browser MCP testing and a detailed 9-step test plan. The problem (dense tables hard to scan, long text clutter, freeze buried in toolbar) is well-described. Direction: Aligned. Web-shell is an actively developed area — CHANGELOG already has "refine markdown table interactions (#6500)" as a recent entry, so table UX iteration is clearly in scope. This is a natural next step. Size: Not applicable (web-shell, not core). Production code: 369 lines across 3 files (CSS module 123, component 284, i18n 14). Test code: 518 lines. Focused on one component area — no sprawl. Approach: Scope feels right for the stated goals. Density toggle (3 modes via CSS custom properties), global long-text expand/collapse replacing per-cell controls, freeze moved to context menu, raw text preserved for TSV copy fidelity. Each feature directly addresses a stated problem. Two follow-up commits addressed all review blockers (TSV newline flattening, formula-like clipboard prefix, non-first-column context menu, compact keyboard resize from rendered width, long-text toolbar visibility tied to visible columns, context-menu click not clearing active column). Maintainer @wenshao validated locally with 89/89 component tests and 1355/1355 full suite. Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题：这是一个功能增强——改善 web shell 中 Markdown 表格的可读性。不是 bug 修复，不需要复现。PR 提供了浏览器 MCP 测试的 before/after 证据和详细的 9 步测试计划。问题（密集表格难以扫描、长文本杂乱、冻结入口太显眼）描述清晰。 方向：对齐。Web-shell 是活跃开发的领域——CHANGELOG 中已有 "refine markdown table interactions (#6500)"，表格 UX 迭代显然在范围内。 规模：不适用（web-shell，非核心模块）。生产代码：369 行，跨 3 个文件（CSS module 123、组件 284、i18n 14）。测试代码：518 行。集中在一个组件区域。 方案：范围合理。密度切换（3 种模式，通过 CSS 自定义属性实现）、全局长文本展开/收起替代逐单元格控制、冻结移入右键菜单、TSV 复制保留原始换行。每个功能都直接对应已陈述的问题。两次 follow-up 提交解决了所有审查阻塞项（TSV 换行压平、公式类剪贴板前缀、非首列右键菜单、紧凑密度键盘调整从渲染宽度起始、长文本工具栏可见性绑定到可见列、右键菜单点击不清除 active column）。维护者 @wenshao 本地验证 89/89 组件测试和 1355/1355 全量测试通过。 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Collaborator
Code ReviewReviewed the full diff (4 files: CSS module, component, tests, i18n) including all follow-up commits. No critical blockers found. The implementation is clean and well-structured:
No AGENTS.md violations. No over-abstraction, no unnecessary duplication, code stays in the right place. TestingThis is a web-shell React component — CLI tmux testing is not applicable. Tested via unit tests and maintainer browser validation. Unit Test Results (maintainer validation by @wenshao)Behavior Verification (maintainer browser testing by @wenshao)All 9 Reviewer Test Plan items verified in Google Chrome via Playwright:
— Qwen Code · qwen3.7-max |
Collaborator
ReflectionRe-ran triage after the two follow-up commits that addressed all review blockers. The code is unchanged since the initial approval at This PR does what it says, and does it well. Four files in one component area, each change mapping to a stated goal. No drive-by refactors, no scope creep. The implementation choices are sound: CSS custom properties for density make future adjustments trivial, the global expand/collapse is cleaner than per-cell controls, and moving freeze to a context menu declutters the toolbar without hiding the feature. The The follow-up commits are disciplined — each one targets specific review feedback (TSV newline flattening, clipboard formula prefix, compact resize, context menu state) without expanding scope. The 89/89 component tests pass. 1355/1355 full suite. Typecheck, lint, format all clean. Maintainer @wenshao validated every Reviewer Test Plan item in a real browser. The prior markdown table PR (#6500) in the CHANGELOG shows this is continued iteration on an established area, not speculative feature work. Approving. ✅ 中文说明反思在两次 follow-up 提交解决了所有审查阻塞项后重新执行 triage。代码自初始批准（ 这个 PR 做到了它所描述的事情，而且做得很好。4 个文件集中在一个组件区域，每个改动都对应一个已陈述的目标。没有夹带重构，没有范围蔓延。 实现选择合理：密度使用 CSS 自定义属性，未来调整很简单；全局展开/收起比逐单元格控制更简洁；冻结移入右键菜单减少了工具栏干扰。 Follow-up 提交很克制——每个都针对特定的审查反馈（TSV 换行压平、剪贴板公式前缀、紧凑调整列宽、右键菜单状态），没有扩大范围。 89/89 组件测试通过。1355/1355 全量测试通过。类型检查、lint、格式化全部通过。维护者 @wenshao 在真实浏览器中验证了 Reviewer Test Plan 的每一项。CHANGELOG 中之前的 Markdown 表格 PR (#6500) 表明这是已建立领域的持续迭代。 批准合并 ✅ — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
approved these changes
Jul 9, 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. ✅
jifeng
requested review from
qwen-code-ci-bot,
wenshao and
ytahdn
and removed request for
wenshao and
ytahdn
July 9, 2026 17:54
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. |
qwen-code-ci-bot
requested changes
Jul 9, 2026
Collaborator
Suggestions — commit
|
qwen-code-ci-bot
requested changes
Jul 10, 2026
qwen-code-ci-bot
requested changes
Jul 10, 2026
wenshao
previously approved these changes
Jul 10, 2026
wenshao
left a comment
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
The four files in this PR (EnhancedMarkdownTable CSS/tests/tsx + i18n) are clean. The table readability features are well-implemented with comprehensive test coverage (87/87 table tests + 29/29 ChatPane tests pass).
main (including typo regressions in ide-client.ts and types.ts, and a CSS inconsistency in SplitView.module.css). These are not part of this PR's changes but appear in the full diff. Consider rebasing onto main to keep the PR focused.
— qwen3.7-max via Qwen Code /review
wenshao
added a commit
to wenshao/qwen-code
that referenced
this pull request
Jul 10, 2026
Ten more runs, three more defects, and a correction to the record. The record first. The runs that produced the evidence for the previous two commits, and for these, executed the review skill as it exists on main --- not this branch. main has no chunk plan, no territory agents, and no receipts, so any claim those commits made about which topology an agent ran under, or which chunk a symbol landed in, was reconstructed rather than observed. The defects they fix are real and were confirmed against main's own text, which this branch inherits unchanged: the severity taxonomy sits in Step 6 while Step 3 assigns severities, and cross-file impact analysis walks only the consumer direction. The causal stories about chunk agents were not observed and should not have been written as though they were. Now the new ones. The diff base. Agents were handed a diff command and left to choose a base. `main..HEAD` and `main...HEAD` differ by one character and by the entire meaning of the review: a two-dot diff against a main that has moved shows main's later commits reversed, so main's fixes read as the branch's regressions. A review of PR QwenLM#6626 approved the four files the PR actually changed, then warned the author publicly that their branch carried "typo regressions" in a file the PR never touched and should be rebased. main had corrected `compatability` to `compatibility` after the fork point. The branch had done nothing. Capture resolves the base once and hands agents a file; they never see a ref name, and a finding in a file outside the report's `files[]` is not a finding about this PR. The review body. "A Suggestion never goes in body" is stated twice and was violated anyway, because a model holding a finding it cannot anchor would rather say it somewhere than drop it. On PR QwenLM#6631 an unanchorable Suggestion about `session.ts:2048` --- a line in no hunk --- became a second paragraph of the public review body. So the rule stops being prose: for COMMENT the body is exactly one of three sentences plus the footer and nothing else, and you read what you are about to send and confirm it. A Suggestion that will not anchor is deleted; it is already in the terminal output and the Step 8 report. And the downgrade sentence. On PR QwenLM#6489 a review with three Suggestions and no Critical announced it had been "downgraded from Approve" --- telling the author the PR would otherwise have been approved, which was false: a Suggestion-only review is COMMENT on its own. Decide the event from the findings first, apply the downgrade flag second, and write the sentence only if it changed the answer.
wenshao
added a commit
to wenshao/qwen-code
that referenced
this pull request
Jul 10, 2026
Three defects, all found by reading what live reviews actually posted. The diff base. Agents were handed a diff command and left to choose a base. `main..HEAD` and `main...HEAD` differ by one character and by the entire meaning of the review: a two-dot diff against a main that has moved shows main's later commits reversed, so main's fixes read as the branch's regressions. A review of PR QwenLM#6626 approved the four files the PR actually changed, then warned the author publicly that their branch carried "typo regressions" in a file the PR never touched and should be rebased. main had corrected `compatability` to `compatibility` after the fork point. The branch had done nothing. Capture now resolves the base once and hands agents a file; they never see a ref name, and a finding in a file outside the report's `files[]` is not a finding about this PR. The review body. "A Suggestion never goes in body" is stated twice and was violated anyway, because a model holding a finding it cannot anchor would rather say it somewhere than drop it. On PR QwenLM#6631 an unanchorable Suggestion about `session.ts:2048` — a line in no hunk — became a second paragraph of the public review body. So the rule stops being prose: for COMMENT the body is exactly one of three sentences plus the footer and nothing else, and you read what you are about to send and confirm it. A Suggestion that will not anchor is deleted; it is already in the terminal output and the Step 8 report. The downgrade sentence. On PR QwenLM#6489 a review with three Suggestions and no Critical announced it had been "downgraded from Approve" — telling the author the PR would otherwise have been approved, which was false: a Suggestion-only review is COMMENT on its own. Decide the event from the findings first, apply the downgrade flag second, and write the sentence only if it changed the answer.
Collaborator
Author
|
Review follow-up summary for the latest table readability fixes:
Verification completed:
中文说明： 这两次 follow-up 分别修复了复制安全与 TSV 结构、非首列右键菜单、紧凑密度键盘调整列宽、隐藏长文本列后的工具栏状态，以及点击冻结菜单时 active column 高亮被误清除的问题。已通过组件测试和本地浏览器交互验证。 |
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.
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.
pull Bot
pushed a commit
to mcx/qwen-code
that referenced
this pull request
Jul 10, 2026
…QwenLM#6612) * feat(review): give every line of a large diff an accountable reviewer Review agents were handed the diff *command* and left to run it themselves. Shell tool output is capped at 30 000 characters and split head-1/5 / tail-4/5, so on a large changeset every agent received a few hundred lines off the top of the first file, the tail of the last file, and a truncation marker in place of everything between. Measured on a 211 000-character diff: 14.4% of the changeset, the same 14.4% for all ten agents. Nineteen of the twenty defects maintainers eventually confirmed on that PR lay in the hidden 85.6%. The ten-way dimension fan-out multiplied redundant reads of the visible sliver rather than adding coverage, and each review round sampled a different subset of the bugs depending on which files an agent happened to open on its own. The diff is now captured to a file and partitioned. `read_file` still caps a single read at ~25 000 characters, so writing the diff out is necessary but not sufficient — a whole-file read of that diff returns its first 611 lines. Chunks are therefore bounded by both a line budget (attention) and a character budget (what one un-truncated read returns), split on hunk boundaries, and never through the middle of a function. They tile the diff exactly, which is what makes the new coverage receipts checkable: past 500 diff lines each chunk gets one agent that owns it and must account for it, and a chunk with no receipt is re-reviewed before the run proceeds. "No blockers" can no longer be reported over code nobody read. Coverage alone did not close the gap. Chunk agents held every state-machine defect in that PR inside their assigned territory and reported none of them: the bugs were not inside any hunk but between new lines sitting two thousand lines apart, and what the agents lacked was not the lines but the question. A heavily rewritten file now also gets three whole-file agents that walk a fixed invariant checklist — mutable fields cleared on every exit path, timers cancelled on every close without discarding captured data, map inserts matched by deletes, retry counters incremented at every entry, status returns actually checked, error codes classified permanent versus transient, config honoured on every path, early returns that skip a required side effect. The checklist is split three ways deliberately: one agent asked to run all eight checks over a 2 400-line file runs one of them properly. Verification is sharded at eight findings per agent, because one verifier re-reading code for sixty findings degrades on the tail of its list. A verifier may now downgrade a Critical but never delete one — a rejected Critical is invisible to every later stage, a downgraded one still reaches a human. The reverse audit fans out per chunk instead of asking a single context-starved agent to re-read the whole diff, no longer skips verification, and stops after two consecutive dry rounds rather than one: on the PR that motivated this, the review reported "no blockers" twice and the next round surfaced five Criticals, three of them in code present since the first commit. * fix(review): keep small-diff reads inside the read_file cap Step 3A told every agent to read the whole diff in one call. `read_file` truncates a single call at ~25 000 characters, so a 500-line diff of long lines would come back short — the same blind spot the chunk plan removes, reintroduced at a smaller scale. Across the last 39 merged PRs that take the Step 3A path the largest diff is 23 570 characters, so this never fired in practice, but the margin is six percent. Step 3A now walks the chunk ranges, which are sized to fit one un-truncated read: one or two calls at this size. Derive a file's pre-change line count from the diff instead of measuring it with a second `git show` per file. `git show <base>:<newpath>` returns nothing for a renamed file, reporting zero pre-change lines and classifying a wholesale rewrite as light. The identity holds exactly for creations, deletions, renames and ordinary edits, and halves the process spawns. * fix(review): choose the topology from source lines, not diff lines Diff size is a bad proxy for review risk because test code dominates it. Across this repo's last 40 merged PRs the median diff is 41% test code and 14 of the 40 are more than half tests; PR QwenLM#6457, which motivated the territory fan-out, is itself 58% tests. Gating on raw diff lines therefore carved small production changes into territories: a change of 173 source lines shipping 489 lines of new tests went to the chunked topology, where its production code ended up owned by a single agent, when the dimension fan-out would have read it through eight lenses. Territory fan-out is worth it when there is a lot of risky code to divide, not a lot of lines. The gate is now `srcDiffLines > 500`, with `diffLines > 2400` as a second clause — a delivery bound rather than a risk one, since past that point chunking uses fewer agents than the ten-lens topology anyway and reading a diff that large dilutes all ten. On the 40-PR sample six PRs move back to the dimension fan-out, for about 5% more agents in total across the sample. Paths are classified as source, test, or generated, and the per-kind line counts ship in the fetch report. Chunking is unchanged: the plan still tiles every line, tests and generated files included. What the gate decides is how many reviewers there are and what each is asked to do. Heaviness is likewise restricted to source files — the invariant checklist asks about fields, timers, collections, and error taxonomies, and a rewritten test file has none of those. * fix(review): decode C-quoted diff paths as bytes `git diff` C-quotes any path with a control character or a non-ASCII byte, so a file named `sub/中文文件.ts` arrives as `"b/sub/\344\270\255..."`. The chunk planner stripped the backslashes, turning it into `sub/344270255...ts` — a name that exists nowhere. Every downstream use of the path then failed silently: the line count came back zero, the file could never be classified as heavy, and the chunk agent was told it was reviewing a file that does not exist. Reuse core's `unquoteCStylePath`, which reassembles the octal escapes as UTF-8 bytes, rather than keeping a second, wrong decoder here. Coverage was never affected — line ranges stayed correct — but this repo has non-ASCII paths, so the mislabelling was reachable. Also correct two places that claimed hunks are never split. They are: a hunk larger than the chunk target is split at a top-level declaration, because a brand-new file arrives as one enormous hunk and treating it as atomic would hand a single agent a 50 000-character territory. * fix(review): make diff capture and header parsing robust to git config Four defects, all found in review of this branch. Diff capture obeyed whatever the user's git config said. With `color.diff=always` every `diff --git` line arrives wrapped in ANSI escapes, the parser recognises none of them, and the plan comes back with zero files and zero chunks — the coverage guarantee silently evaluates to nothing. `diff.mnemonicPrefix` renames the `a/`/`b/` prefixes to `i/`/`w/` and every path is then wrong; `diff.external` and textconv filters emit output that is not a unified diff at all. Capture now pins `--no-ext-diff --no-textconv --no-color --unified=3` and the two prefixes. The `diff --git` header was split with a greedy regex. Git separates the two paths with a space and does not quote a path merely for containing one, so `a/img with space.png b/img with space.png` split into `space.png`. Usually the `---`/`+++` headers disambiguate, but a binary or mode-only section has neither. For a non-rename both paths are the same string, so the split point is arithmetic; a rename states its new path outright in `rename to`. A chunk boundary could land on a `-` line. Those exist only on the old side, so the "starts at a top-level declaration" guarantee did not hold for the post-change file an invariant agent later reads. Split points are now restricted to lines present on the new side. An `oversized` chunk — one hunk with no safe interior boundary — can exceed what a single `read_file` returns. Chunks now carry their character count, and a chunk agent is told to page when a read reports truncation. A `Covered:` receipt for a range the agent only half read is worse than no receipt at all. * fix(review): split past a distant boundary, and stop probing GitHub for anchors Both defects surfaced running the new review against PR QwenLM#6591. A 1431-line React component was emitted as a single 45 675-character chunk — nearly twice what one `read_file` returns — because the splitter looked for a safe boundary only inside the 400-line budget window, found none, and gave up on the entire remainder. Twenty-seven boundaries existed further along; the first sat 460 lines in. It now reaches past the window for the next one, so a single distant boundary can no longer collapse a whole file into one chunk. That PR goes from 15 chunks with one over the read cap to 18 with none. Step 7 validated comment anchors by trial. GitHub rejects an entire review with a 422 if any comment's line falls outside every hunk of its file, and the skill offered no cheap way to check, so a run against a real PR submitted five throwaway reviews carrying the bodies `Test`, `Test`, `t`, `t`, `t` to discover which anchors would stick. Those are permanent, public reviews on someone else's pull request. The fetch report now carries each file's hunks as new-side line ranges, which turns the check into a lookup, and the skill states plainly that a review is never submitted to test an anchor. * fix(review): stop reading hunk payload as metadata, and harden the plan Eleven defects from review of this branch. The worst two were silent. A unified diff emits a removed line whose content starts with `-- ` as `--- ...`, and an added line whose content starts with `++ ` as `+++ ...`. SQL, Lua and Haskell comments start with `-- `. The parser read those payload lines as file headers: the path was overwritten by the line's text, and the line vanished from the add/remove counts. A two-file diff — one SQL file losing a comment, one text file gaining a `++ ` line — came back with the second file named `plus line`. Metadata is now only recognised before a file's first hunk. The tiling invariant — every diff line belongs to exactly one chunk, which is what makes a missing coverage receipt mean something — was asserted only in tests. `buildDiffPlan` now checks it and refuses to return a plan with a hole. The rest: a split point could take a *deleted* blank line as evidence of the blank line before a declaration, though that blank exists only in the old file; whole-file invariant agents were pointed at `chunks[].files[]`, which merges hunks at lines 10 and 900 into one `10-902` span and would have had them report pre-existing defects as new; pure-deletion hunks were exported as the inclusive range `[N, N]`, so a right-side comment could be anchored where GitHub has no line and the 422 would sink the whole review; a deleted file could be marked heavy and send three agents to read a post-image that does not exist; a chunk holding a single line longer than one `read_file` can never be fully read by paging, and must now report itself uncoverable rather than receipt a lie; capture did not pin rename detection or `--no-relative`; `gitRaw` had no timeout, so a credential prompt on headless CI would hang forever; a failed base fetch was swallowed, leaving a stale merge-base and a structurally complete report describing the wrong diff; and local reviews still captured with a bare `git diff`, which `color.diff=always` alone renders unparseable. Adds an integration test that drives the real capture against a real repository under hostile git config, covering the paths synthetic fixtures cannot: renames and binaries and mode-only changes with spaces in their names, C-quoted non-ASCII names, and payload lines that impersonate headers. * fix(review): pin submodule output, and separate written lines from hunk spans Four defects from review of this branch. Diff capture left submodules to user config. `diff.ignoreSubmodules=all` hides a changed gitlink completely — a silent coverage hole in the file that is now the review's source of truth — and `diff.submodule=log` replaces the whole `diff --git` section with prose no parser can read. Both are pinned now, and the integration test asserts a bumped gitlink survives them. Whole-file invariant agents were handed `files[].hunks[]` as "the changed lines". A hunk spans the three context lines git prints either side of every change: on PR QwenLM#6457's `QQChannel.ts` those spans cover 1 962 new-side lines of which only 1 403 were written. The agent would have reported defects in 559 lines that predate the PR. The report now also carries `addedRanges[]` — the exact lines the change wrote — and the skill gates invariant agents on those, keeping `hunks[]` for the one thing it is right for, GitHub anchor validation. `Uncoverable:` was introduced as a chunk agent's answer for a chunk holding a line longer than one read, but the receipt accounting still demanded a `Covered:` line from every chunk and relaunched any chunk lacking one — so an uncoverable chunk would have been retried forever. It is now a first-class terminal status: accepted by the accounting, carried into Step 6 under "Not reviewed", and it blocks an Approve verdict. Step 3A, which also walks the chunk plan, is covered by the same rule. The integration test built its fixture repository inside the developer's git environment, so a global `core.hooksPath` or `commit.gpgsign` ran during the test and `~/.gitconfig` decided what the "clean" baseline was. It now disables system and global config, hooks and signing, and sets the executable bit through the index rather than shelling out to `chmod`, which does nothing on Windows. * feat(review): plan any captured diff, and stop the report outgrowing one read Seven items from review of this branch. None blocking; two of them were the skill promising a topology it could not deliver. Step 3B's chunk agents are "one per entry in `chunks[]`", and only `fetch-pr` produced a chunk plan. A local-diff review, and a cross-repo review in lightweight mode, therefore routed into the territory fan-out with no chunk list, no receipts and no tiling guarantee. `qwen review plan-diff <diff-file>` now emits the same plan from any captured diff; redirecting `git diff` or `gh pr diff` to a file already sidesteps the shell's character cap, so all four review paths share one mechanism. A bare diff has no tree to read a post-image from, so it gets chunk agents but no invariant agents, and says so by omission. The fetch report is read with the same `read_file` that truncates at 25 000 characters — and for a seven-file PR it was already 28 056. The tail of `chunks[]` was being silently lost: the coverage hole this design closes, reappearing one level up. `addedRanges[]` now ships only on `heavy` files, its only consumer, which brings that report to 24 992; the skill says to page the read; and the command prints a note when the report exceeds one read. It stays pretty-printed on purpose — a compact one-line JSON cannot be paged by line. The tiling assertion threw inside `fetch-pr` after the worktree existed and before any report was written, so an unforeseen diff shape killed the review outright. It now degrades to the documented diff-less report with a loud warning, keeping both the loudness and the review. `gitOpt` and `git` had no timeout, and `resolveMergeBase` uses `gitOpt` for a network fetch — the exact path whose credential prompt the `gitRaw` timeout was added to survive. All three wrappers now share a deadline and `GIT_TERMINAL_PROMPT=0`. Markdown under `docs/` or at the repository root classifies as `docs` and stays out of `srcDiffLines`, so a translation PR does not trip the territory gate. Markdown inside a source tree stays `source` — the bundled skill prompts are behaviour, not prose. Also: the user docs stated the gate without its `diffLines > 2400` clause, and `READ_FILE_CHAR_CAP` was exported but never used. It now backs the report-size warning. * test(review): unit-test the merge-base and plan-report seams The last open review thread asked for `resolveMergeBase`, `fileMetrics` and `gitRaw` to be testable with git mocked out. Three of the four functions it named have since moved: `classifyHeavy` is a pure function with unit tests, `fileMetrics` became `buildPlanReport`, which already takes an injected post-image resolver, and `gitRaw`'s output path is exercised by the real-git integration test. `resolveMergeBase` was still private and untested. It now lives behind a three-method `GitProbe` — fetch, refExists, mergeBase — that `fetch-pr` fills from the real wrappers. Seven tests cover the branches that matter and that no end-to-end run reaches: the tracking ref preferred over the local branch, the fall-through when the tracking ref shares no history, and above all the dangerous one — a failed fetch that still resolves a merge-base from a stale local ref, which produces a structurally complete report describing a diff nobody wrote. `buildPlanReport` gains seven of its own: the injected resolver is asked once per file and never for a binary, a null resolver means "no tree, decide nothing" rather than a guess, `addedRanges` ship only where an invariant agent will read them, and a pure-deletion hunk never reaches the anchorable ranges. * fix(review): see deletions, survive suppressBlankEmpty, and stop approving unread code Seven findings from review of the merged head. Three of them were the design contradicting itself. `diff.suppressBlankEmpty` prints a blank context line as a physically empty record rather than a lone space, and there is no command-line flag to override it — only `-c`. The parser advanced its new-side cursor for space-prefixed context alone, so every `addedRanges` entry after the first blank line shifted up by one, and the split-point heuristic stopped recognising blank lines. The capture now pins the config, and the parser treats an empty hunk-body record as context regardless, because a diff from `gh pr diff` or a hand-captured file never passes through that pin. A whole-file invariant agent was given the post-change file and the ranges the PR wrote. A deletion appears in neither. Removing a `clearTimeout()`, a `Map.delete()`, or a retry-counter increment is exactly what the checklist hunts, and the text it was handed cannot show a line that is no longer there — telling it to "cite the surrounding hunk" pointed at data it never received. Heavy files now carry a `diffRange` into the report, and the agent reads its own slice of the diff, where the `-` lines are. The receipt accounting demanded exactly one per chunk and said it applied to Step 3A, where nine dimension agents each walk every chunk: literal execution yields nine receipts or none. Territory ownership is a Step 3B idea. What both paths share is the uncoverable rule, and that needs no agent — a chunk is uncoverable iff its `maxLineChars` exceeds the read cap, which the orchestrator reads out of the plan before launching anything. That rule was also never threaded into Step 7, so a green PR with an unread chunk could receive a public LGTM. Any uncoverable chunk now downgrades APPROVE to COMMENT and must be named in the body. Also: the capture recipes redirected into `.qwen/tmp` before anything created it; a file-path review of an unchanged file produced an empty plan that no agent could read, and the skill now branches to a full-file read instead; and the docs classifier called `website/src/App.tsx` prose while calling `packages/cua-driver/docs/*.md` source — it now matches prose extensions under a documentation directory at any depth. * fix(review): tell agents what a severity means before asking for one The severity definitions lived once, in Step 6 — after every severity had already been assigned. Step 3's finding format asked each agent for `Severity: Critical | Suggestion | Nice to have` and never said what the words meant. The agents that fill that field are separate subagents with separate priors and no shared definition between them, so each fell back on its own, and the priors disagree. Observed on a live review of PR QwenLM#6635 — a run of the skill as it stands on main, whose Step 3 and Step 6 text this branch inherits unchanged. One review, CHANGES_REQUESTED, ten inline comments. Six were Critical, and four of those six were coverage gaps: "zero test coverage", "no references to `workers`", "no test exercises this". Two Suggestions in the same review were the identical class. The verdict is computed from Criticals alone, so that PR was blocked partly on the strength of findings its own reviewer had, elsewhere, called suggestions. The two genuine Criticals — a fail-fast that no longer fires before the daemon reports healthy, and a startup failure path that never closes the HTTP server — would have blocked it on their own. The definitions now sit in the finding format that every agent is handed, they are listed among the things every agent prompt must carry, and Step 6 points back at them rather than restating them. A missing test is a Suggestion: "this file has zero references to X" is a coverage statistic, not a defect. Two shapes stay Critical because something is genuinely wrong — a test asserting the opposite of the intended behaviour, and a test weakened or deleted in the diff so new behaviour passes. If a missing test would let a specific incorrect behaviour ship, report that behaviour and cite the gap as evidence. * fix(review): walk cross-file edges in both directions Cross-file impact analysis only ever asked "will the existing callers break?" Every bullet was about signature compatibility, and the budget rule told agents in so many words to "skip unchanged-signature modifications". A field added to an interface changes no signature and breaks no caller, so the analysis was blind to it by construction. The failure that exposed this, on PR QwenLM#6621: the diff added `deviceFlowRegistry?` to WorkspaceRuntime and passed it into the dispatcher for every secondary ACP mount, and nothing anywhere assigned it. The reviewing agent saw the declaration, found no writer, wrote "intentionally deferred to a later milestone", and filed a Suggestion to fix the JSDoc. The reader was AcpDispatcher — a file the diff never touched — where `if (!this.deviceFlowRegistry)` turned `auth/device_flow/start` into an INTERNAL_ERROR and `auth/status` into an empty list on every non-primary workspace. Workspace-qualified ACP shipped its authentication dead, and the review called it a documentation nit. A second reviewer filed the same observation as Critical; the author fixed it with code and dropped the field. Reading cannot find this. The declaration, the pass-through, and the read sit in three different places, and the read is outside the diff, so no agent reaches it by paging through hunks. Only a grep for the read sites does. So: for every field, option, or optional parameter the diff adds, grep its read sites, including outside the diff, and ask what happens when it arrives undefined. Severity is decided at the read site, not the declaration. And an agent must not explain an unpopulated field with author intent it cannot observe — "reserved for future use" is a claim about a person, not about code, and reaching for one means filling a hole in your own field of view. * fix(review): pin the diff base, and make the review body checkable Three defects, all found by reading what live reviews actually posted. The diff base. Agents were handed a diff command and left to choose a base. `main..HEAD` and `main...HEAD` differ by one character and by the entire meaning of the review: a two-dot diff against a main that has moved shows main's later commits reversed, so main's fixes read as the branch's regressions. A review of PR QwenLM#6626 approved the four files the PR actually changed, then warned the author publicly that their branch carried "typo regressions" in a file the PR never touched and should be rebased. main had corrected `compatability` to `compatibility` after the fork point. The branch had done nothing. Capture now resolves the base once and hands agents a file; they never see a ref name, and a finding in a file outside the report's `files[]` is not a finding about this PR. The review body. "A Suggestion never goes in body" is stated twice and was violated anyway, because a model holding a finding it cannot anchor would rather say it somewhere than drop it. On PR QwenLM#6631 an unanchorable Suggestion about `session.ts:2048` — a line in no hunk — became a second paragraph of the public review body. So the rule stops being prose: for COMMENT the body is exactly one of three sentences plus the footer and nothing else, and you read what you are about to send and confirm it. A Suggestion that will not anchor is deleted; it is already in the terminal output and the Step 8 report. The downgrade sentence. On PR QwenLM#6489 a review with three Suggestions and no Critical announced it had been "downgraded from Approve" — telling the author the PR would otherwise have been approved, which was false: a Suggestion-only review is COMMENT on its own. Decide the event from the findings first, apply the downgrade flag second, and write the sentence only if it changed the answer. * fix(review): decide the event by counting, not by weighing A review of PR QwenLM#6584 filed three inline Suggestions and submitted APPROVE with an empty body. GitHub recorded it as an approval. The rule it broke has been in Step 7 all along --- APPROVE means no Critical *and* no Suggestion --- and so has the one about the body, which is empty only for REQUEST_CHANGES. Both were stated twice. Both were ignored. They are ignored because at submit time the model is reasoning about what it wants to say, and "these are only suggestions, the PR is fine" is a sentence it can talk itself into. Nothing in that sentence is a count. So the event and the body become arithmetic. Count the Criticals, count the Suggestions, read the row off a three-row table, and only then apply the downgrade flags --- which can turn APPROVE or REQUEST_CHANGES into COMMENT and nothing else. Then read back what you are about to send and confirm it matches the row. A body holding text the table does not authorise is a finding that failed to anchor; if it is a Suggestion, it gets deleted, not relocated into public prose that no line of code answers to. This subsumes the body-only invariant added in the previous commit, which the same submit-time reasoning had already defeated once, on PR QwenLM#6631. * fix(review): stop the plan report outgrowing the read it must fit in The report tells an agent how to page everything else, so it has to be readable in one `read_file` — about 25 000 characters. Running the real `fetch-pr` against PR QwenLM#6457 produced 25 070. Two constraints pull against each other. Compact JSON is a single enormous line, and `read_file` pages at line boundaries, so a report too big for one call could never be read at all. Indented JSON pages fine but spends four lines on `{ "start": 812, "end": 815 }`, and a heavily rewritten file contributes hundreds of them: `QQChannel.ts` alone carries 140 added ranges and 49 hunks. So indent the structure and inline the leaves. Same JSON, same keys, one range per line, still pageable — and 28% smaller. The QwenLM#6457 report goes from 25 070 bytes to 18 042, and the "page it" warning that used to fire on a seven-file PR now stays quiet. The earlier attempt at this trimmed `addedRanges` to heavy files only and landed at 24 992 bytes on the same PR. Eight bytes of headroom was not a fix. Tests pin the three properties that matter: the collapsed text parses back to an identical object, no range spans two lines, and a path that literally spells a range is not mistaken for one — JSON escapes the quotes inside a string value, and the collapse patterns require unescaped ones. * fix(review): prune the worktree registration a deleted directory leaves behind `cleanStale` and `cleanup` both guarded `git worktree remove` behind `existsSync(path)`, and neither ever pruned. Delete the directory by hand — which is exactly what reclaiming disk with `rm -rf .qwen/tmp` does — and git keeps the worktree registered but missing. From then on `/review` on that PR cannot run: $ git worktree add .qwen/tmp/review-pr-6457 qwen-review/pr-6457 fatal: '...' is a missing but already registered worktree; use 'add -f' to override, or 'prune' or 'remove' to clear and the branch delete that `cleanStale` does next fails too, because the phantom worktree still has that branch checked out. Nothing in the review command surface ran `git worktree prune`, so nothing ever cleared it. This surfaced running the real skill: the orchestrator's first `fetch-pr` failed, it fell back to `qwen review cleanup`, and retried. The leak is not rare — three abandoned worktrees from May and June were still registered in this checkout, one per review that died before Step 9. `releaseWorktree` now does both halves in the order they depend on: remove the directory if it is there, prune the registration unconditionally (a no-op when nothing is stale), and only then let the caller delete the branch. Both callers share it. The tests drive real git. Deleting a worktree directory by hand and re-adding it throws "missing but already registered" without the prune, and `branch -D` throws "used by worktree" — both assertions fail if the prune is removed, which is the point of writing them. * fix(review): put the open comments where a truncated read will find them `read_file` returns the first `truncateToolOutputThreshold` characters — 25 000 by default — sets `isTruncated`, and pages by line. `pr-context` wrote "## Open inline comments (no replies yet — may still need attention)" last, so on a PR with a long history it was the first thing lost, and nothing read the flag that said so. On PR QwenLM#5738 that section began at character 27 125 of a 31 220-character file. The review submitted "Reviewed — no blockers." Five Critical threads were unresolved; four had in fact been addressed, but the fifth — `clearCiEnv()` clearing only `CI*` while `writeTerminalTitle` branches on `TMUX`/`STY`/ `ZELLIJ`/`DVTM` — was live, in the diff, and never seen. Regenerating the context for ten PRs: four lost part or all of the section, and all four were the PRs with the most review rounds. Small PRs never trip it. - Emit the open threads before the already-discussed ones. The findings a round must answer outrank the ones already settled. - `pr-context` warns when the file exceeds the threshold, naming any headings past the cut, and says so plainly when the loss is inside the last section's body instead. - Step 2 of SKILL.md now tells the agent to read `isTruncated` and page the remainder before Step 3. Reordering buys headroom; it does not create it. A 40 000-character context still loses its tail, which is what the warning is for. * fix(review): load this repo's review rules, and re-check open Criticals before approving Two gaps the dogfood on live PRs surfaced, both invisible from reading the skill. `load-rules` looks for a `## Code Review` heading in AGENTS.md and QWEN.md. Neither had one, so it wrote an empty file on every run: every `/review` in this repo reviewed with zero project rules. Add the section, distilled from the conventions already scattered through AGENTS.md (ESM, no cross-package relative imports, kebab-case/PascalCase naming, collocated tests, comments-only-when-why), plus the two hard lessons below. The section loads from the base branch by design — a PR cannot inject its own review rules — so it takes effect once merged. The skill treated a zero-Critical outcome as a fallback rather than a claim. On one PR it published two Criticals citing code not present at the reviewed commit (a fabricated blocker on an already-approved PR); on another it submitted C=0 while a live, twice-filed Critical still stood (a dropped blocker). Add a step before the verdict: for each unresolved Critical on the PR, read the code at the reviewed commit and record still-stands / fixed-by-this-diff / cannot-tell. The event follows from the code, not from the finding count or the thread flags — `isResolved`/`isOutdated` track the anchored line, not whether the bug was fixed. - AGENTS.md: new `## Code Review` section. - load-rules.ts: export `extractCodeReviewSection`; load-rules.test.ts covers the boundary scan and asserts AGENTS.md's own section extracts non-empty, so deleting the heading fails the build. - SKILL.md: re-verification step ahead of the Verdict.
Collaborator
✅ Local validation report (maintainer build & test)I checked out this branch locally, ran the full automated suite, and exercised every item in the Reviewer Test Plan in a real browser (Google Chrome, driven via Playwright). Everything passes — details and screenshots below. TL;DR — Typecheck ✅ · Lint ✅ · Format ✅ · Component tests 89/89 ✅ · Full web-shell suite 1355/1355 ✅ · All 9 behaviors verified visually. No blocking issues. Environment
Automated checks
This PR adds +17 focused test cases covering density cycling, global long-text expand/collapse, the first-column context menu (open / Escape / outside-click / scroll / resize dismissal, and mutual exclusivity with the native menu), row-detail alignment, blank-value rendering, and clipboard behavior. Behavior verification (real browser)1. Toolbar + collapsed long text (default / Standard density) — test plan #2 2. Compact density auto-sizes short columns — test plan #3 3. Global Expand / Collapse text — test plan #4, #8 4. Row-detail alignment fix — test plan #5 5. First-column freeze menu — test plan #6, #7 6. Frozen first column + shadow boundary — test plan #6 7. Light theme + zebra striping 8. i18n — Simplified Chinese (zh-CN) toolbar 9. i18n — zh-CN row details + blank value — test plan #5 Clipboard behavior — verified, worth a note for reviewersThe PR switches copy to use the full raw cell value. I confirmed two intentional, separately-tested behaviors:
So "preserves line breaks" and "flattens newlines" are both correct depending on copy mode, and both paths are covered by the new tests. No cell content is lost on copy. VerdictLGTM from a build / test / behavior standpoint — all gates are green and every Reviewer Test Plan item reproduces exactly as described. The one intended trade-off (compact density auto-sizes columns the user hasn't manually resized, changing their visual width; explicitly-resized widths are preserved) is documented in the PR and behaves correctly. Screenshots were captured against a self-contained render harness for 中文说明（点击展开）✅ 本地验证报告（维护者构建 & 测试）我在本地检出了该分支，跑通了完整的自动化测试，并在真实浏览器（Google Chrome，通过 Playwright 驱动）中逐条验证了 Reviewer Test Plan 的每一项。全部通过，详情与截图见下。 结论 —— 类型检查 ✅ · Lint ✅ · 格式化 ✅ · 组件测试 89/89 ✅ · web-shell 全量测试 1355/1355 ✅ · 9 项行为均已可视化验证。无阻塞问题。 环境
自动化检查
该 PR 新增了 +17 个针对性测试用例，覆盖密度切换、全局长文本展开/收起、首列右键菜单（打开 / Escape / 外部点击 / 滚动 / resize 关闭，以及与原生菜单互斥）、行详情对齐、空白值渲染，以及剪贴板行为。 行为验证（真实浏览器）截图对应上文英文部分的 1–9：
剪贴板行为 —— 已验证，供 reviewer 注意该 PR 改为使用完整的原始单元格值。我确认了两种有意为之、分别有测试覆盖的行为：
因此“保留换行”与“压平换行”视复制模式而定，二者都正确，且都有新测试覆盖。复制不会丢失单元格内容。 结论从构建 / 测试 / 行为角度 LGTM —— 所有关卡全绿，Reviewer Test Plan 每一项都能按描述复现。唯一的有意取舍（紧凑密度会对用户未手动调整过的列自动适配宽度，从而改变视觉宽度；手动调整过的宽度会被保留）已在 PR 中说明，且行为正确。 截图基于为 |
wenshao
approved these changes
Jul 10, 2026
Collaborator
|
@qwen-code /triage |
qwen-code-ci-bot
approved these changes
Jul 10, 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
3 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
This PR improves Markdown table readability in the web shell by adding display-focused controls for dense data tables. It adds a toolbar density toggle, a global long-text expand/collapse control, compact row details, zebra striping, long-text tooltips while collapsed, and a first-column freeze action from the first visible column's context menu with a visual shadow boundary. It also keeps row details visually consistent by left-aligning detail labels and values even when the source table column is right-aligned.
Why it's needed
Markdown tables in daemon output are usually data displays rather than editable spreadsheets, so the main need is scanning, comparison, and copying without losing source content. Before this change, wide columns, long text, and row details made tables harder to read; freezing was too prominent in the toolbar; and copying multiline values could normalize away useful line breaks. The updated behavior makes dense tables easier to scan while preserving full raw cell values for copy operations.
Reviewer Test Plan
How to verify
Evidence (Before & After)
Before: long text was handled per cell, the freeze control lived as a visible toolbar action, compact density still kept broad fixed-width columns, row details inherited numeric column alignment so values such as Score / 92 looked inconsistent, and TSV copy used normalized text that could flatten multiline cell values.
After: browser MCP verification on http://127.0.0.1:5179/ confirmed the Chinese demo renders the updated toolbar, long text defaults to collapsed with tooltips, Expand text / Collapse text controls all long text globally, compact density shrinks short columns from about 177px to about 85-96px while capping the long-note column, first-column freeze shows a shadow boundary, Escape closes the column context menu without clearing the active column highlight, non-first-column right-click does not show the freeze menu, row details show Score and 92 with start/left alignment, blank details show (空白), and filtering to a row without long text hides the Expand text action. Clipboard contents are covered by unit tests because the browser MCP clipboard is sandboxed in this environment.
Tested on
Environment (optional)
Local Vite demo at http://127.0.0.1:5179/ in the in-app browser, plus targeted Vitest coverage for EnhancedMarkdownTable.
Risk & Scope
Linked Issues
Demo
Uploading 录屏2026-07-10 01.52.542222.mov…
N/A
中文说明
What this PR does
这个 PR 优化了 web shell 中 Markdown 表格的可读性，新增了面向数据展示场景的表格控制能力。它增加了工具栏密度切换、全局长文本展开/收起、更加紧凑的行详情、斑马纹、折叠长文本时的 tooltip，以及首个可见数据列右键菜单中的冻结首列能力，并为冻结列增加右侧阴影边界。它还让行详情保持统一的左对齐，即使原始表格里的某一列是右对齐数字列，详情中的 label 和 value 也会和其他详情项保持一致。
Why it's needed
daemon 输出中的 Markdown 表格通常是用于展示数据，而不是像电子表格一样编辑数据，因此核心需求是更容易扫描、比较和复制，同时不丢失原始内容。改动前，宽列、长文本和行详情会降低可读性；冻结入口放在工具栏里过于显眼；复制多行内容时还可能因为使用规范化文本而丢失有意义的换行。更新后的行为让密集表格更易阅读，同时复制操作仍保留完整原始单元格值。
Reviewer Test Plan
How to verify
Evidence (Before & After)
Before：长文本按单元格逐个处理，冻结入口是显眼的工具栏按钮，紧凑密度仍然使用较宽的固定列宽，行详情会继承数字列对齐导致 Score / 92 这类值看起来不一致，TSV 复制使用规范化文本导致多行单元格可能被压平。
After：通过浏览器 MCP 在 http://127.0.0.1:5179/ 验证，中文 demo 可以正常展示更新后的工具栏；长文本默认折叠并带 tooltip；展开文本 / 收起文本会全局控制所有长文本；紧凑密度会把短列从约 177px 收缩到约 85-96px，同时限制长备注列宽；冻结首列后显示阴影边界；Escape 会关闭列右键菜单且不会清掉当前列高亮；非首列右键不会显示冻结菜单；行详情中的 Score 和 92 都是 start/左对齐；空白详情显示 (空白)；筛选到不含长文本的行后展开文本入口会隐藏。剪贴板内容由于当前浏览器 MCP 环境存在沙箱限制，已通过单元测试覆盖。
Tested on
Environment (optional)
本地 Vite demo：http://127.0.0.1:5179/，在内置浏览器中验证；同时针对 EnhancedMarkdownTable 运行了定向 Vitest 测试。
Risk & Scope
Linked Issues
N/A