feat(web-shell): refine file cards and configure drop destination - #11298
Conversation
…act-icons-main # Conflicts: # packages/web-shell/client/App.test.tsx # packages/web-shell/client/App.tsx # packages/web-shell/client/components/WebShellTranscript.tsx # packages/web-shell/client/components/artifacts/ArtifactPanel.tsx # packages/web-shell/client/customization.tsx
…ll-attachment-style
E2E verification reportLocal macOS / headless Chromium, isolated browser harnesses using the actual Web Shell components with mocked daemon responses. No production session was modified. All seven drop-routing cases passed:
Visual checks: artifact SVGs appear in file cards/tags; sent tags align with adjacent text; hover uses a neutral border while keyboard focus stays visible. Final upload rows remain full-width (760px at an 800px viewport, 240px at a 280px viewport) without overflow, retaining the success text and close action. Component regression tests cover progress/success, collision rename results and existing upload interactions. Validation: 368 focused tests passed; Web Shell build and typecheck passed; two full diff reviews completed. Expected error-path logs and existing paste-test act warnings were emitted, with no test failures. Windows/Linux and the full repository suite were not run. |
🖼️ web-shell visual previewRendered against a mock daemon (no real backend): the PR base vs this PR head Screenshots · before / afterFull-resolution recordings (.webm) are attached to the workflow run. — Qwen Code · web-shell visuals |
|
Thanks for the PR — and for the follow-up commit. This is a re-run, so everything below is re-checked against Template looks good ✓ — unchanged from the gate pass: every required heading is present and actually filled in, and the two older design docs ( Problem. Feature PR, so there is no reproduction to ask for. The two goals stand: the attachment/ Direction. Same assessment as the gate pass, now better documented: Size. 861 changed lines across 22 files at this head: 453 production (TSX + CSS modules), 339 test, 69 docs. No core paths and no cross-package span, so the two-tier core gate does not apply, and it sits under both the 500- and 1000-line advisories. Approach. The follow-up commit is the minimal set the review asked for — regression fixes plus the tests that pin them, nothing new added on top. The Risk. No elevated risk signals — none of the changed files match the revert-correlated path list. One watch item that is not a risk finding: the visuals job at this head flags Moving on to code review. 🔍 中文说明感谢贡献，也感谢后续提交。这是一次 re-run，以下内容均针对 模板完整 ✓ —— 与 gate 阶段相同：所有必需标题都在且为实际填写；两份较早的设计文档（ 问题。 这是 feature PR，不需要复现。两个目标不变：附件与 方向。 与 gate 阶段判断相同，且文档更完善： 规模。 该 head 上 22 个文件共 861 行改动：生产代码 453 行（TSX + CSS modules）、测试 339 行、文档 69 行。未触及核心路径，也不跨包，两层核心 gate 不适用；同时低于 500 行与 1000 行两个提示阈值。 方案。 后续提交正是 review 所要求的最小集合 —— 回归修复加上钉住它们的测试，没有额外扩大范围。gate 阶段看起来与目标无关的 风险。 无升级风险信号 —— 改动文件均未命中与 revert 相关的路径列表。一个需要留意但不属于风险发现的点：visuals 作业在该 head 上把 进入代码审查 🔍 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
Code reviewRe-run over Every concrete regression from the last pass is fixed, and each fix carries the test that would have caught it. Long upload results now ellipsize in a The The hover question from the gate is resolved properly. Two things I had wrong last time, corrected against this commit. The Leftovers, all non-blocking. The shared What I checked on the routing half beyond the tests. Testing evidenceUnattended CI run, so I built and executed nothing myself — the evidence below is this PR's own CI on the reviewed commit, read through the API, plus the visuals job's own logs and artifact. No check is red, so there is no failing-job log to quote.
One watch item in the visual signal, stated plainly. At this head the The standing gap is unchanged: the visuals suite still has no scenario for any surface this PR restyles — I checked all five specs at this commit and none mentions attachments, uploads, file tags or dropped files, and the capture list has no card, tag or upload-row view. So the styling half's only visual evidence remains the author's local Chromium pass at 800px/280px on macOS, which I have not re-run. Not verified: the visual result of the restyle in either theme on any real surface — no CI check renders one, and this run cannot drive a browser. Not verified: Windows and Linux browser behaviour, which the author also lists as untested. Not verified: the author's 368 focused tests and seven mocked drop-routing cases as a local run — CI's Linux unit job and the component tests in the diff are the closest machine-checked proxy. The routing half is a behavioural claim and CI does substantiate it: the seven-case matrix (both-available, preference-unavailable, neither-available, cancel-on-change) exercises the real handlers against mocked drag events and is green on Linux. The visual half is the gap, and neither 中文说明代码审查针对 上次提出的每个具体回归都已修复，且每个修复都带着本应抓住它的测试。 过长的上传结果现在在
gate 阶段的 hover 问题已被妥善解决。 上次我说错的两点，已按本 commit 更正。 遗留项，均非阻塞。 共享的 路由那一半我在测试之外还核对了什么。 测试证据无人值守 CI 运行，因此我没有构建或执行任何代码——下面的证据是通过 API 读到的、本 PR 自己在被审查 commit 上的 CI 结果，以及 visuals 作业自身的日志与产物。没有红色检查，因此没有失败日志可引用。 视觉信号里有一个需要留意的点，直接说明。 在该 head 上， 既有缺口不变： visuals 套件仍然没有任何场景覆盖本 PR 改样式的界面——我核对了该 commit 上全部五个 spec，没有一个提到附件、上传、文件标签或拖入文件，截图清单里也没有卡片、标签或上传行视图。因此改样式那一半唯一的视觉证据仍是作者在 macOS 本地 Chromium 上做的 800px/280px 检查，我没有重跑。 未验证：任一真实界面上、任一主题下的改样式视觉结果——没有 CI 检查会渲染它们，本次运行也无法驱动浏览器。未验证：Windows 与 Linux 的浏览器行为，作者本人也列为未测试。未验证：作者自述的 368 项定向测试与七个模拟拖入场景作为本地运行——CI 的 Linux 单测作业和 diff 里的组件测试是最接近的机器可核验替代。 路由那一半是行为性主张，CI 确实佐证了它：七场景矩阵（两者都可用、偏好去向不可用、两者都不可用、状态变化时取消）针对模拟 drag 事件走了真实处理函数，在 Linux 上是绿的。缺口在视觉那一半，而 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
|
Confidence: 4/5 — the routing half is well tested and the styling half is now internally consistent; what remains is follow-up work, not doubt about this diff. Last pass I deferred at 3/5 on two judgement calls, and both have since been answered in a way I can check. The theme-colour question is no longer a silent side effect: the design doc now states that the fixed format colours are intentional across consumers and that Lucide stroke styling continues for formats without artwork, which is also why the The follow-up commit is what changed my mind on the rest. It is not a sweep of the review comments — it is the concrete regressions fixed one by one, each with the test that would have caught it: the ellipsized upload result with its full text in a Two honest reservations, neither blocking. The visuals suite still cannot see any surface this PR restyles — I checked all five specs at this commit — so the card and tag styling ships on the author's local Chromium pass plus component tests, and adding one scenario per restyled surface is still the highest-value follow-up on this PR. And the visuals job flags 中文说明Confidence: 4/5 —— 路由那一半测试充分，改样式那一半现在内部一致；剩下的是后续工作，而不是对这个 diff 的怀疑。 上一轮我以 3/5 暂缓，原因是两个判断性问题，此后两者都以我可以核对的方式得到了回答。主题颜色问题不再是悄悄发生的副作用：设计文档现在写明固定格式色在所有消费方是有意的、无素材格式继续用 Lucide 描边；这也解释了环境面板调用点上的 让我对其余部分改变看法的是后续提交。它不是对 review 意见的笼统清扫，而是把具体回归逐个修复，且每个都带着本应抓住它的测试：上传结果省略号截断并在 两点诚实的保留，均不阻塞。visuals 套件仍然看不到本 PR 改样式的任何界面——我核对了该 commit 上全部五个 spec——因此卡片与标签样式依赖作者本地 Chromium 检查加组件测试；为每个被改样式的界面补一个场景仍是本 PR 价值最高的后续动作。另外 visuals 作业把 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
LGTM. Tier: Standard. Drop routing is correct and well-tested; no correctness or security blockers.
What I checked:
- Full routing matrix (7 paths): both-available/no-pref → dialog; upload-pref → uploadFiles; attach-pref → ingestFiles; upload-disabled (any pref) → early return → imageDropCapture fallback; neither-available → prevent+stop. All correctly implemented and all 7 tests in ChatEditor.test.tsx are structurally sound (they’d fail on inversion).
- FileAttachmentContent extension regex — correct for dotfiles, compound extensions, extension-less names
- FileTypeIcon condition ‘kind !== “file” || Icon === FileIcon’: artifact SVG takes priority; Lucide used for recognised-but-no-SVG kinds (json→braces, py→code, etc.); file.svg as final fallback. Correct.
- getArtifactIconKind widened to Pick<> — only fields actually used, no callers broken
- availabilityKey updated alongside routing change; useLayoutEffect dep-array extended to clear pending files on availability change — both correct
- isPreviewableFileComposerTag + !tag.icon guard: custom tag icons and directory-kind tags correctly bypass the new SVG path
- fileUploadEnabled narrowed semantics: documented in prop JSDoc and design doc; flipped tests keep the behavior change visible in history
Non-blocking notes (same territory as ci-bot’s stage-2 comment):
-
Theme-colour at 4 pre-existing FileTypeIcon call sites. The artifact SVGs bake #3B82F6/#14B8A6/#EF4444 with no currentColor; rendering via
means they can never inherit the container colour. Four sites that were getting theme-adaptive Lucide strokes until this diff — EnvironmentPanel.tsx:497+583 (both pass strokeWidth={1.7}, now a no-op), ArtifactPanel.tsx:3120 (unsupported-attachment placeholder), ArtifactPanel.tsx:558 (.md file tab now full-colour next to grayscale artifact tabs), QueuedPromptDisplay.tsx:340 — now get fixed-colour icons silently. The PR says “sharing artifact SVG icons” so this looks intentional, but the four untouched sites are unmentioned. Two options: (a) confirm it’s intentional, drop the dead strokeWidth props, add a sentence to the design doc; (b) keep Lucide at those four sites and only apply SVGs at the two new surfaces.
-
fileUploadEnabled semantics change bundled with styling. The prop no longer blocks attachment drag-in — a behaviour change on a published documented prop, disclosed in the updated JSDoc but sharing a PR with restyle work. Hosts that used fileUploadEnabled={false} to suppress all file drag-in will see attachments remain available.
-
Dead fileChipRemove pointer-events rules (ChatEditor.module.css ~2214-2224): base rule now sets pointer-events: auto; opacity: 1 unconditionally, making the hover/focus-visible override and @media (hover: none) revert assert the opposite of actual behaviour. Safe to trim.
-
File-tag style written three times with two unintentional differences: vertical-align: middle in widget inline styles and UserMessage.module.css but absent in ChatEditor.module.css; padding-right: 8px in UserMessage only. Background token differences are correct; these two look accidental.
-
Design-doc branch reference: web-shell-attachment-cards.md mentions a private local branch codex/web-shell-artifact-icons-main. Assets are in this PR; the sentence can go.
No blockers. ✅
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查：反向审计——在 5 轮的反审轮数上限内未收敛。
— qwen3.8-max via Qwen Code /review (v0.23.0)
| box-sizing: border-box; | ||
| width: 272px; | ||
| max-width: 100%; | ||
| padding: 8px 32px 8px 8px; |
There was a problem hiding this comment.
[Suggestion] The card re-skin changes the geometry of the attachments strip but leaves its height budget at the old chip value. Each card is 66px against the old chip's 26px (8px+8px padding + 1px+1px border + FileAttachmentContent's flex: 0 0 48px icon box, which is taller than the .details stack), and the new .fileTag { min-height: 28px } grows a tag row from its natural 24px — but --chat-editor-attachments-max-height is still 136px (declared on .container at ChatEditor.module.css:28, consumed at :829). Two cards side by side need 550px, so any composer narrower than ~577px wraps them into 4 + 66 + 6 + 66 = 142px against a 136px cap: the second card's bottom 6px, and the bottom edge of a :focus-visible ring (outline-offset: 2px), fall below the clip. The common combined case is worse — .tags and .files are siblings inside the same capped [data-web-shell-composer-attachments] element, so one @file tag plus two cards needs 28 + 8 + 4 + 66 + 6 + 66 = 178px, leaving 42px below the fold. Before this change the same budget held four rows of chips (130px) and five files did not scroll at all. The token also cannot be recovered from outside: .container shadows any value a host sets on an ancestor.
Recompute the budget for combined occupancy rather than for .files alone — measured, 178px clears one tag row plus two cards at viewport heights of roughly 800px and above, whereas 148px (two card rows only) still leaves 30px clipped as soon as one @file tag shares the strip. Alternatively make the cap content-driven. Note the ceiling when choosing the number: at a 600px-tall viewport .content measures 240px and .attachments is squeezed to 138px, so below that height some clipping is unavoidable at any budget value — pick the number against --chat-editor-max-height: min(350px, 40vh) (:25) and --chat-editor-input-min-height: 44px (:27), not against the 136px being replaced.
Witness:
real Chromium against the real CSS module text at this commit, A/B'd against the merge-base CSS and chip markup.
files-only: PR 2 files @577px rows=1 attClient=70 attScroll=70 scrollable=false | @570px rows=2 attClient=136 attScroll=142 scrollable=true clipped=6 | @360px identical | 3 files @360px attScroll=214 clipped=78
base: 4 files @360px chipH=[26,26,26,26] attClient=30 attScroll=30 scrollable=false | 5 files @360px rows=2 attScroll=62 scrollable=false
combined (1 fileTag + 2 cards): cap 136 -> client=136 scroll=178, 42px below the fold (child _tags_ h=36 padding=0px 0px 8px rows=1, kid _tag_ _fileTag_ h=28; child _files_ h=142 rows=2 kids h=66 w=272 x2)
cap 148 -> client=148 scroll=178, 30px still clipped (8 clipped descendants)
cap 178 -> client=178 scroll=178 clipped=0 ; 500x1000 identical
cap 178 @600x600 -> client=138 scroll=178, 40px clipped (the composer's own min(350px,40vh) cap dominates)
host override: --chat-editor-attachments-max-height:400px on :root -> maxH=136px (unchanged, shadowed by .container)
tag-row growth arm (.fileTag with only min-height:28px deleted): file tag row h=24, .tags=32, combined scroll=174 -> the new rule contributes +4px per row
One existing fact the fix must not violate: --chat-editor-attachments-max-height: 136px is declared on .container at ChatEditor.module.css:28 and consumed as max-height: var(--chat-editor-attachments-max-height, 136px) at :829; the new value must still leave room inside .content's own cap --chat-editor-max-height: min(350px, 40vh) (:25) together with --chat-editor-input-min-height: 44px (:27) and .content's padding: 12px. .attachments is flex: 0 1 auto; min-height: 0 (:826-828), so it shrinks rather than pushing the input below its floor. One reported cost was measured FALSE and should not be acted on: the @smoke assertion at client/e2e/web-shell.smoke.spec.ts:1848-1850 (toBeLessThanOrEqual(136)) is insensitive to this variable at every value tested — the real spec passed 3/3 at cap 136, 148, 178 and 1000 with a byte-identical bounding box (76.7 / 66.6 / 39.7px at 1000/800/600px viewports), because .attachments is squeezed by the parent .content and its own scrollHeight=152 and never reaches the cap in that scenario. Do not de-duplicate that constant on the strength of this finding.
中文说明
卡片改版改变了附件区的几何尺寸，却把它的高度预算留在了旧的 chip 数值上。每张卡片是 66px，而旧 chip 是 26px（8px+8px padding + 1px+1px border + FileAttachmentContent 的 flex: 0 0 48px 图标框，它比 .details 那一叠更高），新增的 .fileTag { min-height: 28px } 又让 tag 行从自然的 24px 变高 —— 但 --chat-editor-attachments-max-height 仍是 136px（在 ChatEditor.module.css:28 的 .container 上声明，在 :829 被消费）。两张卡片并排需要 550px，因此任何窄于约 577px 的输入区都会换行，变成 4 + 66 + 6 + 66 = 142px 对上 136px 的上限：第二张卡片底部的 6px，以及 :focus-visible 外框（outline-offset: 2px）的下缘，都会落到裁剪线以下。更常见的是组合场景 —— .tags 与 .files 是同一个受限于 [data-web-shell-composer-attachments] 元素内的兄弟节点，所以一个 @file tag 加两张卡片需要 28 + 8 + 4 + 66 + 6 + 66 = 178px，有 42px 落在可视区之外。本次改动之前，同样的预算可以容纳四行 chip（130px），五个文件也完全不会滚动。而且这个 token 无法从外部覆盖：.container 会遮蔽宿主在祖先元素上设置的值。
建议按组合占用重新计算预算，而不是只按 .files 计算 —— 实测 178px 可以在视口高度约 800px 及以上时容纳一行 tag 加两张卡片，而 148px（只考虑两行卡片）在有一个 @file tag 同处该区域时仍会裁掉 30px。也可以让这个上限由内容决定。选择数值时请注意天花板：在 600px 高的视口下 .content 实测为 240px，.attachments 被压缩到 138px，因此低于该高度时无论预算取何值都不可避免会有裁剪 —— 数值应当参照 --chat-editor-max-height: min(350px, 40vh)（:25）和 --chat-editor-input-min-height: 44px（:27）来定，而不是参照被替换的 136px。
— qwen3.8-max via Qwen Code /review (v0.23.0)
There was a problem hiding this comment.
Keeping the existing scrollable attachment height budget in this PR. More card content requires scrolling, and the parent composer height still constrains short viewports even with a larger budget. The proposed combined-occupancy resize is deferred; no height change is claimed here.
| {safeIconUrl && ( | ||
| {isPreviewableFileComposerTag(tag) && | ||
| !tag.icon && | ||
| safeIconUrl === getComposerTagIconUrl('file') ? ( |
There was a problem hiding this comment.
[Suggestion] This three-clause predicate, and the styling that goes with it, are now hand-copied into the three renderers of one visual object: here, ChatEditor.tsx:2415-2417, and useComposerCore.ts:710-715 — the third also re-inlines the class as a string (icon.style.cssText = 'display:inline-flex;flex:0 0 auto;margin-left:8px;color:var(--muted-foreground);'), and .fileTagIcon now exists twice byte-identically (UserMessage.module.css:470-474, ChatEditor.module.css:2482-2486) beside a third .fileTag copy that has already drifted (padding-right: 8px exists only in the UserMessage copy, and the background token differs between --background and --chat-editor-bg-primary).
The three copies also compute safeIconUrl three different ways: ChatEditor.tsx:2403 uses tag.icon ?? getComposerTagIconUrl(tag.kind, composerTagIcons), UserMessage.tsx:549-552 adds viewModel?.iconUrl, and useComposerCore.ts:676 uses this.tag.iconUrl ?? getComposerTagIconUrl(this.tag.kind) with no host icon map at all. They agree today only because all five inline-tag insertion sites happen to pre-resolve through resolveComposerTagIcon. The cost is that the next change to this rule — honouring a composerTagIcons.file override, a new metadata.fileKind, dropping the !tag.icon clause, changing icon size or margin — has to be applied in three places, and a miss makes the same tag render as a file-type card on one surface and as a generic grey mask icon on another, with no test catching it because each surface's test asserts only its own copy and the cssText copy silently ignores any CSS-module edit.
Extract the predicate next to the helpers it already calls, in utils/composerTag.ts — e.g. shouldRenderFileTypeIcon(tag, safeIconUrl, composerTagIcons?) implementing isPreviewableFileComposerTag(tag) && !tag.icon && safeIconUrl === getComposerTagIconUrl('file', composerTagIcons) — and call it from all three sites, keeping size and class at each call site.
Witness:
grep enumeration over the real tree at the reviewed commit:
predicate `safeIconUrl === getComposerTagIconUrl('file')` (non-test): 3 sites
hooks/useComposerCore.ts:714 components/messages/UserMessage.tsx:587 components/ChatEditor.tsx:2417
.fileTagIcon definitions: UserMessage.module.css:470 ChatEditor.module.css:2482 (byte-identical)
same declarations re-inlined as a string: useComposerCore.ts:718
'display:inline-flex;flex:0 0 auto;margin-left:8px;color:var(--muted-foreground);'
three safeIconUrl resolutions quoted: ChatEditor.tsx:2403 / UserMessage.tsx:549-552 / useComposerCore.ts:676
One existing fact the fix must not violate: Two constraints, both verified. (1) packages/web-shell/client/utils/composerTag.ts:29-36 — "these live here rather than in hooks/useComposerCore.ts on purpose: that module imports the whole CodeMirror editor at top level … Importing them from useComposerCore dragged ~1 MB of CodeMirror into every exported HTML file (PR 11031)"; the shared helper must stay in a CodeMirror-free module, and packages/web-shell/client/build-artifact.test.ts:330 (expect(js).not.toContain('@codemirror/')) fails if it does not. (2) composerTag.ts:148-157 — getComposerTagIconUrl resolves getOwnIconUrl(customIconUrls, kind) ?? getOwnIconUrl(builtinTagIconUrls, kind), so a shared helper must keep taking the host composerTagIcons map; comparing against getComposerTagIconUrl('file') with the map omitted would flip hosts that override file from the mask icon to the file-type card.
Acceptance criterion: packages/web-shell/client/components/messages/UserMessage.test.tsx 'previews a sent workspace file tag by path' asserts container.querySelector('svg.lucide-file-text') is non-null for a kind: 'file' tag with no icon — it must stay green, as must the ChatEditor/useComposerCore chip tests that assert the composer-side icon; the extraction is behaviour-preserving, so a red there means the predicate moved wrongly. Please prove it by removing the fix afterwards, running that test, and confirming it goes red.
中文说明
这个三条件判断以及与之配套的样式，现在被手工复制到了同一个视觉对象的三个渲染器中：这里、ChatEditor.tsx:2415-2417，以及 useComposerCore.ts:710-715 —— 第三处还把类名以字符串形式重新内联了一遍（icon.style.cssText = 'display:inline-flex;flex:0 0 auto;margin-left:8px;color:var(--muted-foreground);'），而 .fileTagIcon 现在有两份逐字节相同的定义（UserMessage.module.css:470-474、ChatEditor.module.css:2482-2486），旁边还有第三份 .fileTag 副本且已经出现漂移（padding-right: 8px 只存在于 UserMessage 那份，背景 token 在 --background 与 --chat-editor-bg-primary 之间不一致）。
三份副本计算 safeIconUrl 的方式也各不相同：ChatEditor.tsx:2403 用 tag.icon ?? getComposerTagIconUrl(tag.kind, composerTagIcons)，UserMessage.tsx:549-552 还加了 viewModel?.iconUrl，而 useComposerCore.ts:676 用 this.tag.iconUrl ?? getComposerTagIconUrl(this.tag.kind)，完全没有传入宿主图标表。它们今天之所以一致，只是因为五个内联 tag 插入点恰好都先经过 resolveComposerTagIcon 预解析。代价是：下一次修改这条规则时 —— 支持 composerTagIcons.file 覆盖、新增 metadata.fileKind、去掉 !tag.icon 条件、调整图标尺寸或边距 —— 必须同时改三处，漏掉一处就会让同一个 tag 在一个界面渲染成文件类型卡片、在另一个界面渲染成灰色通用 mask 图标，而且没有测试能发现，因为每个界面的测试只断言自己那一份，cssText 那份还会静默忽略任何 CSS module 的修改。
建议把这个判断抽取到它已经在调用的那些 helper 旁边，即 utils/composerTag.ts —— 例如 shouldRenderFileTypeIcon(tag, safeIconUrl, composerTagIcons?)，实现 isPreviewableFileComposerTag(tag) && !tag.icon && safeIconUrl === getComposerTagIconUrl('file', composerTagIcons) —— 并在三处调用它，尺寸和类名仍留在各调用点。
— qwen3.8-max via Qwen Code /review (v0.23.0)
There was a problem hiding this comment.
Deferring the shared-predicate/style refactor. The renderers have different customization and layout contexts; no new abstraction is needed for the concrete fixes in this follow-up. Existing predicates remain unchanged.
| chip.style.verticalAlign = 'middle'; | ||
| chip.style.background = 'var(--chat-editor-bg-primary)'; | ||
| chip.style.borderRadius = '8px'; | ||
| chip.style.minHeight = '28px'; |
There was a problem hiding this comment.
[Suggestion] Neither half of the visual re-skin this change exists to ship is observed by any assertion, so both can be reverted wholesale without reddening a single test in the package.
On the widget side, deleting this restyle block (useComposerCore.ts:665-672 — verticalAlign, background: var(--chat-editor-bg-primary), borderRadius: 8px, minHeight: 28px, fontFamily, title) leaves 92 tests green across useComposerCore.dom.test.tsx, useComposerCore.test.ts and useComposerCore.mobile.dom.test.tsx, so @file reference chips can silently revert to the pre-change 20px --secondary monospace pill with vertical-align: baseline while the two React surfaces re-skinned in the same commit keep the card look — reintroducing exactly the three-surface divergence this change exists to remove.
On the upload-strip side, deleting the <FileTypeIcon … className={styles.uploadRowIcon} /> element (ChatEditor.tsx:2732-2738), moving the busy spinner back to the leading slot, or dropping title={upload.file.name} each leave ChatEditor.test.tsx at 118 green and the package green apart from an unbuilt-dist harness failure. uploadRowIcon, uploadRowSpinner and uploadRowName appear in zero test files. A positive control confirms the comparator is not simply blind to this family: dropping title={name} in FileAttachmentContent — a surface these tests do pin — reddens four. The upload strip is the one of the three re-skinned surfaces with a state-dependent element (the busy spinner) and it has no oracle at all.
Pin both. For the widget, extend the new uses file format icons while preserving explicit icons and directories test to read the chip that owns the [data-file-type-icon="html"] icon (icon.closest('span[style*="min-height"]')) and assert minHeight is 28px, background contains --chat-editor-bg-primary and verticalAlign is middle, plus a negative assertion on the directory chip. For the strip, add to the existing upload-strip tests (which already render a strip via chooseDropAction('upload'), e.g. ChatEditor.test.tsx:2800) an assertion that [class*="uploadRowIcon"] is present and, for a row held in uploading by a deferred uploadWorkspaceFile promise, that [class*="uploadRowStatus"] [class*="uploadRowSpinner"] is present.
(Site: the CodeMirror chip's new card styling — this finding covers 2 sites and each is commented separately.)
Witness:
mutation runs in a scratch tree.
(a) widget restyle: BASELINE useComposerCore.dom.test.tsx + useComposerCore.test.ts + useComposerCore.mobile.dom.test.tsx -> 3 files / 92 passed (92); MUTATION A (delete only the 5 style lines) -> 92 passed; MUTATION B (delete the whole block 665-672) -> 92 passed, and whole-package 17 failed | 6439 passed (6456) where all 17 are build-artifact.test.ts ENOENT on packages/web-shell/dist (scratch-tree harness gap) — no unit test anywhere reddens.
(b) upload strip: baseline ChatEditor.test.tsx -> 118 passed (118); Mut A delete <FileTypeIcon … className={styles.uploadRowIcon}/> -> 118 passed, full package 17 failed | 6439 passed (same ENOENT harness gap); Mut B restore the pre-PR leading slot {busy ? <LoaderCircleIcon className={styles.uploadRowSpinner}/> : <UploadIcon/>} -> same 17 harness failures only; Mut C drop title={upload.file.name} -> 118 passed.
POSITIVE CONTROL (drop title={name} in FileAttachmentContent, a surface the PR DOES pin): x 'renders attachments with a file icon, title, and type', x 'renders attachment metadata for report.HTML | README | LICENSE' -> 2 files failed, 4 tests failed | 157 passed (161) — the comparator can see markup changes in this exact family; the two re-skin halves simply have nothing watching them.
grep: uploadRowIcon, uploadRowSpinner, uploadRowName appear in zero test files; chat-editor-bg-primary / minHeight / 'min-height: 28px' across *.{ts,tsx} return only the two production sites at useComposerCore.ts:667,669.
One existing fact the fix must not violate: Two measured constraints. (1) useComposerCore.dom.test.tsx:1581 — expect(tooltip?.parentElement?.getAttribute('title')).toBeNull(); — any assertion or guard added to this block must not restore an unconditional chip.title (the defect confirmed separately at R1-7), or that tooltip contract test goes red. Also the directory chip's style.minHeight is '20px', NOT '' (the base cssText at useComposerCore.ts:631-632 sets it on every chip), so the negative assertion must expect '20px' or use verticalAlign === 'baseline' / background === 'var(--secondary)'; the positive half works as proposed (measured iconAncestorChipMinHeight '28px', verticalAlign 'middle'). (2) data-file-type-icon is emitted ONLY on FileTypeIcon's artifact branch (FileTypeIcon.tsx:94); the Lucide branch at :100 emits no such attribute — so an upload-strip probe filename must resolve to artwork (report.pdf, notes.md), not notes.txt, or the selector matches nothing and the new assertion passes vacuously.
Acceptance criterion: (a) useComposerCore.dom.test.tsx — after adding the style assertions, deleting the if (isPreviewableFileComposerTag(this.tag)) { … } block at useComposerCore.ts:665-672 must turn it red (today it does not). (b) ChatEditor.test.tsx — the new pair must go red when the <FileTypeIcon … className={styles.uploadRowIcon} /> element is deleted from ChatEditor.tsx:2732-2738, and the second must go red when the {busy && <LoaderCircleIcon …/>} block moves back out of .uploadRowStatus into the leading slot. Please prove it by removing the fix afterwards, running that test, and confirming it goes red.
中文说明
本次改动所要交付的视觉改版，两个部分都没有任何断言观察，因此两者都可以被整体回退而不会让包内任何一个测试变红。
widget 一侧：删除这个改版代码块（useComposerCore.ts:665-672 —— verticalAlign、background: var(--chat-editor-bg-primary)、borderRadius: 8px、minHeight: 28px、fontFamily、title），useComposerCore.dom.test.tsx、useComposerCore.test.ts 与 useComposerCore.mobile.dom.test.tsx 合计 92 个测试依然全绿，于是 @file 引用 chip 可能悄悄退回改动前 20px、--secondary、等宽字体、vertical-align: baseline 的药丸样式，而同一个 commit 里改版的两个 React 界面仍保持卡片外观 —— 正好重新引入了本次改动想要消除的三界面不一致。
上传行一侧：删除 <FileTypeIcon … className={styles.uploadRowIcon} />（ChatEditor.tsx:2732-2738）、把 busy spinner 移回前导位置，或去掉 title={upload.file.name}，每一种都让 ChatEditor.test.tsx 保持 118 全绿，整包除一个未构建 dist 导致的 harness 失败外也全绿。uploadRowIcon、uploadRowSpinner、uploadRowName 在测试文件中出现零次。一个正向对照证明比较器对这一族组件并非 simply 看不见：删掉 FileAttachmentContent 中的 title={name}（这个界面是有测试的）会让四个测试变红。上传行是三个改版界面中唯一带状态相关元素（busy spinner）的，却完全没有验证手段。
建议两侧都补上。widget 一侧，扩展新增的 uses file format icons while preserving explicit icons and directories 测试，读取持有 [data-file-type-icon="html"] 图标的那个 chip（icon.closest('span[style*="min-height"]')），断言 minHeight 为 28px、background 含 --chat-editor-bg-primary、verticalAlign 为 middle，并对目录 chip 补一个反向断言。上传行一侧，在既有的上传行测试中（它们已经通过 chooseDropAction('upload') 渲染出上传行，例如 ChatEditor.test.tsx:2800）断言 [class*="uploadRowIcon"] 存在，并对一个由延迟的 uploadWorkspaceFile promise 保持在 uploading 状态的行断言 [class*="uploadRowStatus"] [class*="uploadRowSpinner"] 存在。
— qwen3.8-max via Qwen Code /review (v0.23.0)
There was a problem hiding this comment.
Browser checks verified the current inline presentation and reserved 16x16 icon geometry. The requested committed visual/style assertion expansion was not added, so this coverage suggestion remains open.
| name={upload.file.name} | ||
| mimeType={upload.file.type} | ||
| size={20} | ||
| className={styles.uploadRowIcon} |
There was a problem hiding this comment.
[Suggestion] Neither half of the visual re-skin this change exists to ship is observed by any assertion, so both can be reverted wholesale without reddening a single test in the package.
On the widget side, deleting this restyle block (useComposerCore.ts:665-672 — verticalAlign, background: var(--chat-editor-bg-primary), borderRadius: 8px, minHeight: 28px, fontFamily, title) leaves 92 tests green across useComposerCore.dom.test.tsx, useComposerCore.test.ts and useComposerCore.mobile.dom.test.tsx, so @file reference chips can silently revert to the pre-change 20px --secondary monospace pill with vertical-align: baseline while the two React surfaces re-skinned in the same commit keep the card look — reintroducing exactly the three-surface divergence this change exists to remove.
On the upload-strip side, deleting the <FileTypeIcon … className={styles.uploadRowIcon} /> element (ChatEditor.tsx:2732-2738), moving the busy spinner back to the leading slot, or dropping title={upload.file.name} each leave ChatEditor.test.tsx at 118 green and the package green apart from an unbuilt-dist harness failure. uploadRowIcon, uploadRowSpinner and uploadRowName appear in zero test files. A positive control confirms the comparator is not simply blind to this family: dropping title={name} in FileAttachmentContent — a surface these tests do pin — reddens four. The upload strip is the one of the three re-skinned surfaces with a state-dependent element (the busy spinner) and it has no oracle at all.
Pin both. For the widget, extend the new uses file format icons while preserving explicit icons and directories test to read the chip that owns the [data-file-type-icon="html"] icon (icon.closest('span[style*="min-height"]')) and assert minHeight is 28px, background contains --chat-editor-bg-primary and verticalAlign is middle, plus a negative assertion on the directory chip. For the strip, add to the existing upload-strip tests (which already render a strip via chooseDropAction('upload'), e.g. ChatEditor.test.tsx:2800) an assertion that [class*="uploadRowIcon"] is present and, for a row held in uploading by a deferred uploadWorkspaceFile promise, that [class*="uploadRowStatus"] [class*="uploadRowSpinner"] is present.
(Site: the upload-strip row's new leading icon and relocated spinner — this finding covers 2 sites and each is commented separately.)
Witness:
mutation runs in a scratch tree.
(a) widget restyle: BASELINE useComposerCore.dom.test.tsx + useComposerCore.test.ts + useComposerCore.mobile.dom.test.tsx -> 3 files / 92 passed (92); MUTATION A (delete only the 5 style lines) -> 92 passed; MUTATION B (delete the whole block 665-672) -> 92 passed, and whole-package 17 failed | 6439 passed (6456) where all 17 are build-artifact.test.ts ENOENT on packages/web-shell/dist (scratch-tree harness gap) — no unit test anywhere reddens.
(b) upload strip: baseline ChatEditor.test.tsx -> 118 passed (118); Mut A delete <FileTypeIcon … className={styles.uploadRowIcon}/> -> 118 passed, full package 17 failed | 6439 passed (same ENOENT harness gap); Mut B restore the pre-PR leading slot {busy ? <LoaderCircleIcon className={styles.uploadRowSpinner}/> : <UploadIcon/>} -> same 17 harness failures only; Mut C drop title={upload.file.name} -> 118 passed.
POSITIVE CONTROL (drop title={name} in FileAttachmentContent, a surface the PR DOES pin): x 'renders attachments with a file icon, title, and type', x 'renders attachment metadata for report.HTML | README | LICENSE' -> 2 files failed, 4 tests failed | 157 passed (161) — the comparator can see markup changes in this exact family; the two re-skin halves simply have nothing watching them.
grep: uploadRowIcon, uploadRowSpinner, uploadRowName appear in zero test files; chat-editor-bg-primary / minHeight / 'min-height: 28px' across *.{ts,tsx} return only the two production sites at useComposerCore.ts:667,669.
One existing fact the fix must not violate: Two measured constraints. (1) useComposerCore.dom.test.tsx:1581 — expect(tooltip?.parentElement?.getAttribute('title')).toBeNull(); — any assertion or guard added to this block must not restore an unconditional chip.title (the defect confirmed separately at R1-7), or that tooltip contract test goes red. Also the directory chip's style.minHeight is '20px', NOT '' (the base cssText at useComposerCore.ts:631-632 sets it on every chip), so the negative assertion must expect '20px' or use verticalAlign === 'baseline' / background === 'var(--secondary)'; the positive half works as proposed (measured iconAncestorChipMinHeight '28px', verticalAlign 'middle'). (2) data-file-type-icon is emitted ONLY on FileTypeIcon's artifact branch (FileTypeIcon.tsx:94); the Lucide branch at :100 emits no such attribute — so an upload-strip probe filename must resolve to artwork (report.pdf, notes.md), not notes.txt, or the selector matches nothing and the new assertion passes vacuously.
Acceptance criterion: (a) useComposerCore.dom.test.tsx — after adding the style assertions, deleting the if (isPreviewableFileComposerTag(this.tag)) { … } block at useComposerCore.ts:665-672 must turn it red (today it does not). (b) ChatEditor.test.tsx — the new pair must go red when the <FileTypeIcon … className={styles.uploadRowIcon} /> element is deleted from ChatEditor.tsx:2732-2738, and the second must go red when the {busy && <LoaderCircleIcon …/>} block moves back out of .uploadRowStatus into the leading slot. Please prove it by removing the fix afterwards, running that test, and confirming it goes red.
中文说明
本次改动所要交付的视觉改版，两个部分都没有任何断言观察，因此两者都可以被整体回退而不会让包内任何一个测试变红。
widget 一侧：删除这个改版代码块（useComposerCore.ts:665-672 —— verticalAlign、background: var(--chat-editor-bg-primary)、borderRadius: 8px、minHeight: 28px、fontFamily、title），useComposerCore.dom.test.tsx、useComposerCore.test.ts 与 useComposerCore.mobile.dom.test.tsx 合计 92 个测试依然全绿，于是 @file 引用 chip 可能悄悄退回改动前 20px、--secondary、等宽字体、vertical-align: baseline 的药丸样式，而同一个 commit 里改版的两个 React 界面仍保持卡片外观 —— 正好重新引入了本次改动想要消除的三界面不一致。
上传行一侧：删除 <FileTypeIcon … className={styles.uploadRowIcon} />（ChatEditor.tsx:2732-2738）、把 busy spinner 移回前导位置，或去掉 title={upload.file.name}，每一种都让 ChatEditor.test.tsx 保持 118 全绿，整包除一个未构建 dist 导致的 harness 失败外也全绿。uploadRowIcon、uploadRowSpinner、uploadRowName 在测试文件中出现零次。一个正向对照证明比较器对这一族组件并非 simply 看不见：删掉 FileAttachmentContent 中的 title={name}（这个界面是有测试的）会让四个测试变红。上传行是三个改版界面中唯一带状态相关元素（busy spinner）的，却完全没有验证手段。
建议两侧都补上。widget 一侧，扩展新增的 uses file format icons while preserving explicit icons and directories 测试，读取持有 [data-file-type-icon="html"] 图标的那个 chip（icon.closest('span[style*="min-height"]')），断言 minHeight 为 28px、background 含 --chat-editor-bg-primary、verticalAlign 为 middle，并对目录 chip 补一个反向断言。上传行一侧，在既有的上传行测试中（它们已经通过 chooseDropAction('upload') 渲染出上传行，例如 ChatEditor.test.tsx:2800）断言 [class*="uploadRowIcon"] 存在，并对一个由延迟的 uploadWorkspaceFile promise 保持在 uploading 状态的行断言 [class*="uploadRowStatus"] [class*="uploadRowSpinner"] 存在。
— qwen3.8-max via Qwen Code /review (v0.23.0)
There was a problem hiding this comment.
The upload status regression now checks the full rename title, and browser checks verified the compact row. The additional committed leading-icon/spinner placement assertions were not added; leaving this broader coverage request open.
| icon.style.cssText = | ||
| 'display:inline-flex;flex:0 0 auto;margin-left:8px;color:var(--muted-foreground);'; | ||
| icon.setAttribute('aria-hidden', 'true'); | ||
| this.contentRoot = createRoot(icon); |
There was a problem hiding this comment.
[Suggestion] The default @file chip's icon was two synchronous inline-style writes and is now a React root create plus a scheduled render per widget build, so toDOM hands CodeMirror a chip whose icon span is still empty and the glyph appears only when React flushes. Measured in real Chromium with a MutationObserver at the insertion microtask, plus rAF and timeout checkpoints: at insertion the chip is 92.23px wide with iconSpanChildren 0 and no data-file-type-icon, and it is still in that state at raf1 (t=345.0) and timeout0 (t=346.0) — so the blank state is paintable, not merely a same-tick artifact — widening to 108.23px with the glyph at raf2 (t=360.4). The base arm of the same script measured 104.41px at every checkpoint, so this is a regression introduced here: every inline @file chip visibly pops 16px wider a frame after it appears, and CodeMirror measures the line at the narrow width first. (The delta is 16px, not 24px — the empty display:inline-flex span keeps its margin-left:8px, so only the glyph is added.)
On top of that, the re-resolution effect (useComposerCore.ts:1710-1734, deps [composerTagIcons, onComposerTagClick, renderComposerTag, renderComposerTagTooltip, resolveComposerTagIcon]) clears and re-adds every inline tag when those customization identities change, and resolveComposerTagIcon snapshots onComposerTagClickRef.current into onClick (:1673-1678), which eq() compares by identity — so an embedding host passing those public customizations as inline literals rebuilds every chip on every re-render, each rebuild allocating a root per chip and unmounting the old one. In-repo App.tsx passes stable props, so today's observable cost is the per-insertion blank frame; that half is traced rather than measured.
Build the icon as plain DOM inside the widget instead — resolve getArtifactIconKind({ kind: 'file', title: tagValue }) and append <img src={ARTIFACT_ICON_URLS[kind]} width="16" height="16" alt=""> (or reuse the existing mask span with the artifact data URL), keeping FileTypeIcon for the two React surfaces. That was run: the chip is complete at the insertion microtask (108.23px with iconSpanChildren 1 at every checkpoint) and all 65 dom tests stay green. destroy() already unmounts contentRoot (:862-868), so this is allocation churn rather than a leak — but the plain-DOM version has neither.
Witness:
real Chromium, MutationObserver recording the chip at the moment CodeMirror inserts it (microtask, before React's scheduler macrotask) plus rAF/timeout checkpoints.
PR (unmodified), tag 'notes.md':
insert-microtask t=338.2 chipWidth 92.23 iconSpanWidth 0 iconSpanChildren 0 hasFileTypeIcon false
raf1 t=345.0 chipWidth 92.23 iconSpanChildren 0 hasFileTypeIcon false <- blank state survives past the first frame
timeout0 t=346.0 chipWidth 92.23 iconSpanChildren 0 false
raf2 t=360.4 chipWidth 108.23 iconSpanWidth 16 iconSpanChildren 1 hasFileTypeIcon true
BASE (useComposerCore.ts @ 668df522c0, same script): chipWidth 104.41 at EVERY checkpoint (319.4 / 326.1 / 328.2 / 341.5 / 419.6)
CANDIDATE FIX (plain-DOM <img src={ARTIFACT_ICON_URLS[getArtifactIconKind({kind:'file',title:tagValue})]} width/height 16 data-file-type-icon={kind}> in the same span):
insert-microtask chipWidth 108.23 iconSpanChildren 1 hasFileTypeIcon true -> 108.23 at raf1/timeout0/raf2/timeout100
client/hooks/useComposerCore.dom.test.tsx -> 65 passed (65), keeping the pinned [data-file-type-icon="file"] row
One existing fact the fix must not violate: getArtifactIconKind returns 'file' for names with no artwork (artifacts/ArtifactIcon.tsx:105-131) and FileTypeIcon falls back to the Lucide result there (FileTypeIcon.tsx:91), so a plain-DOM replacement must keep a glyph for extensionless values — the 'file'-kind tag the edited test at useComposerCore.dom.test.tsx:1674-1676 pins via [data-file-type-icon="file"] — or those chips lose their icon entirely. Two corrections to the filed claim, both measured: the width delta is 16px, not 24px, and destroy() is not a leak (useComposerCore.ts:862-868 unmounts contentRoot), so the cost is allocation churn plus the blank frame.
Acceptance criterion: A useComposerCore.dom.test.tsx case asserting iconSpan.firstElementChild is non-null synchronously after the dispatch that adds the tag (no extra React flush), or equivalently spying on react-dom/client's createRoot and asserting it is NOT called for a plain kind:'file' tag with no renderContent/tooltip — today it is called once per such chip. Please prove it by removing the fix afterwards, running that test, and confirming it goes red.
中文说明
默认 @file chip 的图标原本是两次同步的内联样式写入，现在变成每次 widget 构建都要创建一个 React root 并调度一次渲染，因此 toDOM 交给 CodeMirror 的 chip 其图标 span 还是空的，字形要等 React flush 之后才出现。在真实 Chromium 中用 MutationObserver 在插入微任务处、并配合 rAF 与 timeout 检查点实测：插入时 chip 宽 92.23px，iconSpanChildren 0，没有 data-file-type-icon；在 raf1（t=345.0）和 timeout0（t=346.0）时仍是这个状态 —— 说明空白状态是会被绘制出来的，而不只是同一 tick 内的瞬时现象 —— 到 raf2（t=360.4）才带上字形并变宽到 108.23px。同一脚本的基准侧在每个检查点都是 104.41px，所以这是本次改动引入的回退：每个内联 @file chip 都会在出现后一帧明显地弹宽 16px，而 CodeMirror 会先按较窄的宽度测量该行。（差值是 16px 而不是 24px —— 空的 display:inline-flex span 保留了它的 margin-left:8px，所以只多出字形本身。）
此外，重新解析 effect（useComposerCore.ts:1710-1734，依赖 [composerTagIcons, onComposerTagClick, renderComposerTag, renderComposerTagTooltip, resolveComposerTagIcon]）会在这些定制项的标识变化时清除并重新添加所有内联 tag，而 resolveComposerTagIcon 会把 onComposerTagClickRef.current 快照进 onClick（:1673-1678），eq() 又按标识比较它 —— 所以宿主若把这些公开定制项写成内联字面量，每次重渲染都会重建所有 chip，每次重建都要为每个 chip 分配一个 root 并卸载旧的。仓库内的 App.tsx 传的是稳定 props，因此当前可观察到的代价是每次插入时的空白帧；后一半是推理而非实测。
建议改为在 widget 内用普通 DOM 构建图标 —— 解析 getArtifactIconKind({ kind: 'file', title: tagValue }) 后追加 <img src={ARTIFACT_ICON_URLS[kind]} width="16" height="16" alt="">（或者复用现有的 mask span 配 artifact data URL），把 FileTypeIcon 留给两个 React 界面。这个方案已经跑过：chip 在插入微任务时就是完整的（每个检查点都是 108.23px、iconSpanChildren 1），65 个 dom 测试全绿。destroy() 本来就会卸载 contentRoot（:862-868），所以这里是分配抖动而非泄漏 —— 但普通 DOM 方案两者都没有。
— qwen3.8-max via Qwen Code /review (v0.23.0)
There was a problem hiding this comment.
Partially addressed in 04028cd: reserving the icon wrapper at 16x16 removes the observed width shift, verified in the browser. The asynchronous React icon render is intentionally retained to share SVG/Lucide fallback behavior; synchronous first-paint glyph rendering and root-allocation changes are not claimed fixed, so this thread remains open.
|
Addressed the concrete regressions from the review:
Added regression assertions for direct-drop focus, disabled attachment feedback, attach-only drag feedback, inert drop propagation, full rename status and tooltip behavior for file tags, plus filename parsing cases. Decisions on the remaining suggestions: fixed-color artifact SVGs are intentional across shared consumers, per the requested visual design; Lucide color/stroke props remain useful for fallback formats. This is now documented. The card width, one-line filename treatment and scrollable attachment height budget are retained; increasing the budget still cannot avoid scrolling in a short composer. The preview's filename-plus-type accessible name is descriptive, so no additional ARIA override is added. Broader visual-fixture coverage, menu/custom-icon matrix expansion and refactoring the shared predicate/fallback tables are deferred to keep this follow-up focused. The React icon component is retained to share fallback behavior; reserving its geometry addresses the observed layout shift without introducing a second DOM renderer. Validation: 245 focused tests passed. Browser verification covered long status truncation/full title, custom tooltip behavior, immediate focus for both direct destinations and stable 16×16 icon geometry. The isolated verification server was stopped afterward. Full repository |
|
@qwen-code /triage |
|
Sandboxed verification: ❌ not passed — findings reported (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 186 passed · 1 failed · 187 total Flakiness gate: ✅ 6 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定：❌ 不通过 · 报告了发现（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：186 通过 · 1 失败 · 187 总计 抖动门：✅ 6 changed test file(s) x 5 identical rounds, no divergence Verification reportPR #11298 — deep verification reportVerdict: The central claim is proven load-bearing: all 21 cells of the drop-routing truth table match the documented behaviour at head, all 21 match the pre-PR behaviour at base, and 12 cells flip between the arms. The findings are one pre-existing defect the PR widens and makes internally inconsistent (F-1), one cosmetic defect in the PR's new card component (F-2), and one unpinned attribute (F-3). None blocks the merge; all are worth a reviewer's attention. 中文摘要
Central claim and A/BCentral claim. Harness
Reading the table:
Secondary claim 1 — destination-level count. H1 can only report which lane ChatEditor handed the drop to, so Secondary claim 2 — shared icons and the new cards. The two-line attachment card renders icon + filename + short format label and a per-file remove button ( Reviewer Test Plan, per step
FindingsF-1 — a dropped folder becomes a 0-byte phantom attachment in the submitted prompt (Suggestion; root cause pre-existing, PR widens and makes it inconsistent)
Measured with
Two distinct consequences at head:
Attribution, stated precisely: the root cause is pre-existing (S1 and S4 are byte-identical on base and head; base's dialog "reference" action used the filtered lane, so base had the same two-lane split in a less visible form). The PR's contribution is (a) consequence 1, because it added the direct Measured candidate fix (not applied to the PR)Teach let hasUnavailableFileItem = false;
+ // Dropped folders surface in `dataTransfer.files` as 0-byte Files; only the
+ // items API can tell them apart, and folder attachments are out of scope.
+ const isDirectoryItem = (item: DataTransferItem | undefined): boolean =>
+ item?.kind === 'file' && item.webkitGetAsEntry?.()?.isDirectory === true;
+
if (dataTransfer.files.length > 0) {
- for (const file of Array.from(dataTransfer.files)) {
- classifyFile(file, file.type, result);
- }
+ const files = Array.from(dataTransfer.files);
+ const items = Array.from(dataTransfer.items);
+ for (let index = 0; index < files.length; index += 1) {
+ if (isDirectoryItem(items[index])) continue;
+ classifyFile(files[index], files[index].type, result);
+ }
} else {
for (const item of Array.from(dataTransfer.items)) {
if (item.kind !== 'file') continue;
+ if (isDirectoryItem(item)) continue;
const file = item.getAsFile();Applied in a scratch copy and re-driven through the same harnesses:
That third result is the point: the suite is green on both sides, so it pins nothing along this axis — the fixture that would go red is a F-2 — the new card's type line shows a raw MIME string for extensionless files (Suggestion, PR-introduced, cosmetic)
F-3 — the upload row's new filename tooltip is pinned by no test (coverage gap; the round's single failed assertion)The PR adds Mutation matrix (completeness reporting, not merge conditions)Unmutated control:
Every kill failed the intended behavioural assertion with expected-vs-actual values (quoted in How the survivors enter Not covered
MethodologyEnvironment: the lane's own Flakiness gate logEvidence imagesHarness scripts and raw logs are in the workflow run artifacts (7-day retention). — Qwen Code · sandboxed verification |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
Re-review (Round 2) — addressing fix commit 04028cd
Prior round findings resolved:
| R1 finding | Resolved? |
|---|---|
| Design-doc private branch reference | ✅ Removed |
Dead fileChipRemove pointer-events CSS |
✅ Cleaned up |
focusComposer() missing from direct-route paths |
✅ Added to both upload and attach lanes |
chip.title unconditional before custom tooltip |
✅ Guarded by hasCustomTooltip |
| Icon span lacked explicit 16×16 dimensions | ✅ Fixed |
| Fixed artifact SVG colours treated as undisclosed change | ✅ Documented as intentional in design doc |
FileTypeIcon ? and # in filenames |
✅ replace(/[?#]/g, '_') added with tests |
Spurious stopPropagation() in neither-available path |
✅ Removed |
New round findings:
No blockers.
Minor — vertical-align: middle missing from ChatEditor.module.css .fileTag
UserMessage.module.css .fileTag and the CodeMirror widget inline styles both set vertical-align: middle, so inline file tags align correctly in sent messages and in the editor's CodeMirror widget layer. ChatEditor.module.css .fileTag (applied via TopComposerTag to the React-rendered tag in the above-editor attachment strip) does not have vertical-align. In practice the attachment strip is a flex row so the parent layout absorbs the difference, but if the tag ever appears inline in text context it will sit on the text baseline rather than centred. Non-blocking.
Checked (R2):
- Fix commit diff (11 files): all stated changes verified against original non-blocking notes — every item above lands as described.
hasCustomTooltipguard: set before thechip.titlewrite;appendTooltipstill the path whentooltipis non-null; the new test'uses a custom inline tooltip without a native title for kind file'covers both branches.- Drop routing at new head:
uploadDropEnabled = uploadEnabled && (!attachmentsEnabled || fileDropAction !== 'attach')correctly prevents upload drag feedback when preference is attach;handleUploadDroproutes toingestFiles+focusComposer()in that branch. event.stopPropagation()removed from neither-available path — outer handlers can now observe the cancelled drop event.- Focus call added to upload branch too (not just attach branch).
- CSS cleanup: dead hover/focus/media-hover rules for
fileChipRemoveremoved;uploadRowStatuscolour corrected;uploadRowStatusTextextracted. - UserMessage hover differentiation:
messageTagClickable:where(.fileTag):hoveruses softer border mix;messageTagClickable:not(.fileTag):hoverkeeps accent-colour border.
Not covered: Windows/Linux browser drop behaviour (no host available — consistent with prior round).
Cross-check: CI bot stage-2 suggestions — all items either addressed in this fix commit or documented as intentional (fixed-colour artwork, dead iconForFile paths). No new findings from the bot since the fix commit.
No blockers. Approval blockers: none.
Reviewed with AI assistance.
|
Released in v0.23.1. |
3 participants
What this PR does
Renders attachments as icon/filename/type cards and @file references as compact tags, sharing artifact SVG icons. Upload rows remain full-width with neutral styling and retain progress, success/rename results, errors and existing actions.
Adds
fileDropAction="upload" | "attach". Without a preference, ask only when both destinations are available; use the sole available destination directly.Why it's needed
Unifies file presentation and lets embedding hosts choose drop behavior without unnecessary dialogs.
Reviewer Test Plan
How to verify
Evidence (Before & After)
Before: simple attachment chips, generic reference icons and colored upload rows. After: two-line attachment cards, compact format-specific tags and neutral single-line upload rows with existing status information preserved. Local Chromium checks found no overflow at 800px and 280px viewport widths. Seven mocked browser drop-routing cases passed; a separate E2E report comment lists the cases.
368 focused tests passed, including shared icon consumers and public prop plumbing. Web Shell build, typecheck and diff whitespace checks passed. Two full diff review passes found no new actionable defects.
Tested on
Environment (optional)
Local worktree, Vite/dev daemon, headless Chromium and package-local Vitest. Browser routing checks used mocked daemon responses.
Risk & Scope
fileUploadEnabled={false}now disables workspace upload only, leaving attachments available. Attachment availability is separately controlled byattachmentsEnabled. Upload capability and workspace trust checks remain intact.Linked Issues
None.
中文说明
本 PR 的改动
附件展示为图标/文件名/格式卡片，@文件引用为紧凑标签，共用 artifact SVG 图标。上传行保持通栏，使用中性样式，并保留进度、成功/重命名结果、错误及原有操作。
新增
fileDropAction="upload" | "attach"。不传时仅在两种方式均可用时询问；只有一种可用时直接处理。原因
统一文件展示，让宿主能够控制拖入行为，避免不必要的选择弹窗。
Reviewer 验证计划
验证方式
前后对比与证据
之前是简单附件标签、通用引用图标和有色上传行；现在是两行附件卡片、按格式展示图标的紧凑标签和保留状态信息的中性单行上传展示。本地 Chromium 检查确认 800px 和 280px 视口下均无溢出。7 个使用模拟响应的浏览器拖入场景通过，具体场景见独立 E2E 报告评论。
368 项定向测试通过，覆盖共享图标调用方和公开 props 传递。Web Shell 构建、类型检查和 diff 空白检查通过。完整 diff 连续两轮 review 未发现新的可执行缺陷。
测试平台
macOS 已测试；Windows 和 Linux 未测试。
环境
本地 worktree、Vite/开发 daemon、无头 Chromium 和包内 Vitest。浏览器路由检查使用模拟 daemon 响应。
风险与范围
fileUploadEnabled={false}现在只禁用工作区上传，附件仍可用。附件可用性由attachmentsEnabled独立控制。上传能力和工作区信任检查保持不变。关联 Issue
无。