feat(web-shell): add mobile welcome composer slots - #6584
Conversation
|
Thanks for the PR! Template looks good ✓ — all required sections present including the bilingual block. Problem: This is a feature addition (render slot + mobile layout), not a bug fix — no reproduction needed. The use case (embedders needing a slot above the composer and a mobile-specific welcome footer position) is self-evident for a web-shell package designed for embedding. Direction: Aligned. The web-shell's primary extension mechanism is render slots, and this adds two small ones ( Size: Not applicable — no core paths touched. All 7 files are in Approach: Scope feels right for the stated goal. One concern from the diff structure: the Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ — 所有必需段落齐全，包括双语区块。 问题：这是一个功能新增（渲染插槽 + 移动端布局），不是 bug 修复——不需要复现。对于设计为嵌入式的 web-shell 包来说，"嵌入方需要在输入框上方放置插槽和移动端专属的 welcome footer 位置"这个需求是自明的。 方向：对齐。web-shell 的主要扩展机制就是渲染插槽，这个 PR 增加了两个小插槽（ 规模：不适用——未触及核心路径。全部 7 个文件都在 方案：范围与目标匹配。从 diff 结构看到一个关注点： 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Code ReviewIndependent proposal: Before reading the diff, I would have added Comparison with the PR: The composer header slot is implemented cleanly and matches my proposal exactly. The mobile welcome footer takes a different path — instead of conditionally modifying the existing block, it creates a full second branch with ~60 lines of duplicated This isn't a blocker — the code works and the logic is correct in both branches. But the duplication means any future change to the MessageList props or content area needs to be applied in two places. Worth a refactor before merge. Other observations:
Merge conflict: TestingUnit tests (vitest, All failures are pre-existing — caused by missing TypeScript ( All errors in PR-changed files are pre-existing (missing module declarations at import lines, implicit Tmux real-scenario testing: Not applicable — web-shell is a React component library that requires a host application to render. There is no standalone CLI or app to drive in tmux. The PR's UI changes (mobile layout, composer header slot) can only be verified by embedding the web-shell in a host app and rendering at mobile viewport widths. 中文说明代码审查独立方案： 在阅读 diff 之前，我会添加 与 PR 的对比： composer header 插槽实现得很干净，完全匹配我的方案。移动端 welcome footer 采取了不同的路径——没有条件性地修改现有块，而是创建了一个完整的第二分支，包含约 60 行重复的 这不是阻塞项——代码可以工作，两个分支的逻辑都是正确的。但重复意味着将来对 MessageList 属性或内容区域的任何修改都需要在两个地方应用。建议在合并前重构。 其他观察：
合并冲突： 测试单元测试（vitest， 所有失败都是预先存在的——由工作树环境中缺失的 TypeScript（ PR 更改文件中的所有错误都是预先存在的（导入行的缺失模块声明、未更改代码中的隐式 Tmux 真实场景测试： 不适用——web-shell 是一个需要宿主应用程序才能渲染的 React 组件库。没有独立的 CLI 或应用程序可以在 tmux 中驱动。PR 的 UI 更改（移动端布局、composer header 插槽）只能通过将 web-shell 嵌入宿主应用并在移动端视口宽度下渲染来验证。 — Qwen Code · qwen3.7-max |
|
This PR does what it says: adds two opt-in render slots for web-shell embedders. The composer header is clean and minimal — reuses the existing toolbar render info type, renders above the editor, zero fuss. The mobile welcome footer works correctly and the CSS approach (display: contents + flex order) is a solid choice for repositioning without restructuring the DOM. The one thing I'd push back on before merge is the ~60-line MessageList duplication between the two branches. It's not a correctness problem today, but the two branches will drift — someone will add a prop to one and forget the other. A conditional wrapper would be maybe 10 lines of change instead of 60 lines of duplication. Worth a quick pass. The merge conflict with Everything else checks out: no new test failures, no new type errors, opt-in with sensible defaults, no breaking changes, focused scope. Ships the feature. 中文说明这个 PR 做到了它所说的：为 web-shell 嵌入方添加了两个可选的渲染插槽。composer header 干净且最小化——复用了现有的工具栏渲染信息类型，在编辑器上方渲染，零麻烦。移动端 welcome footer 工作正确，CSS 方法（display: contents + flex order）是在不重构 DOM 的情况下重新定位元素的可靠选择。 合并前我想指出的一点是：两个分支之间约 60 行的 MessageList 重复。今天这不是正确性问题，但两个分支会漂移——有人会给一个分支添加属性而忘记另一个。条件包装大概只需 10 行改动，而不是 60 行重复。值得快速改一下。 与 main 上 其他一切正常：无新的测试失败，无新的类型错误，可选且有合理的默认值，无破坏性更改，范围集中。功能可用。 — Qwen Code · qwen3.7-max |
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. The MessageList duplication is worth a quick cleanup but doesn't block. ✅
93c3a8a to
23d6e76
Compare
|
Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. 中文请勿对活跃的 PR 执行 rebase 或 force-push，因为这会使已有的评审评论失效。另外，供日后参考：作为集成流程的一部分，机器人始终会自动将所有改动压缩（squash）为单个提交。 |
| hasMobileComposerBottom | ||
| ? styles.chatPaneWithMobileComposerBottom | ||
| : undefined, | ||
| hasWelcomeMiddle ? styles.chatPaneWithWelcomeMiddle : undefined, |
There was a problem hiding this comment.
[Critical] styles.chatPaneWithWelcomeMiddle is referenced here but .chatPaneWithWelcomeMiddle is never defined in App.module.css. CSS Modules silently exports it as undefined, which is stripped by .filter(Boolean) — so this className has zero effect.
If chatPane-level styling was intended for the welcome-middle state (analogous to chatPaneWithMobileComposerBottom), add the missing CSS rule to App.module.css. Otherwise, remove this dead reference:
| hasWelcomeMiddle ? styles.chatPaneWithWelcomeMiddle : undefined, |
— qwen3.7-max via Qwen Code /review
| const showMobileWelcomeFooterMiddle = | ||
| useMobileWelcomeMiddleLayout && Boolean(welcomeFooter); | ||
| const hasWelcomeMiddle = isChatEmptyState && showMobileWelcomeFooterMiddle; | ||
| const hasMobileComposerBottom = |
There was a problem hiding this comment.
[Critical] hasMobileComposerBottom resolves to isChatEmptyState && mobileWelcomeFooterMiddle — it does not check welcomeFooter. When an embedder sets mobileWelcomeFooterMiddle={true} without providing renderWelcomeFooter:
hasMobileComposerBottom = true→ footer getsposition: absolute; bottom: 0, chatPane getsoverflow: hiddenshowMobileWelcomeFooterMiddle = false→ no mobile welcome layout renders
The composer becomes absolutely-positioned in a scroll-clipped container with no purpose — a visible layout bug for embedders who enable the flag but omit the footer renderer.
Fix: align with showMobileWelcomeFooterMiddle which already includes the welcomeFooter check:
| const hasMobileComposerBottom = | |
| const hasMobileComposerBottom = showMobileWelcomeFooterMiddle; |
— qwen3.7-max via Qwen Code /review
Suggestions — commit
|
| File | Issue | Suggested fix |
|---|---|---|
packages/web-shell/client/App.tsx:4957-5055 |
MessageList is duplicated across two conditional branches (~80 lines of near-identical JSX). Only differences: welcomeHeader guard, centerWelcomeHeader prop, and btwMessage (mutually exclusive with isChatEmptyState). |
Extract MessageList into a shared variable, conditionally wrap in different containers. |
packages/web-shell/client/App.test.tsx |
No tests for renderComposerHeader, mobileWelcomeFooterMiddle, or centerWelcomeHeader — the PR's core features have zero test coverage. |
Add tests verifying each prop renders correctly and handles the opt-in/opt-out behavior. |
packages/web-shell/client/App.tsx:424 |
mobileWelcomeFooterMiddle implicitly requires renderFooter for the mobile CSS reordering to work (via .footerWithCustomFooter class applied only when CustomFooter exists). This cross-prop dependency is undocumented. |
Document the dependency in the JSDoc: Requires renderFooter to be provided for the mobile layout reordering to take effect. |
— qwen3.7-max via Qwen Code /review
| flex-direction: column; | ||
| } | ||
|
|
||
| .appChatEmpty .chatViewWithWelcomeMiddle .footerWithCustomFooter { |
There was a problem hiding this comment.
[Critical] The mobile welcome-middle layout CSS rules (.chatViewWithWelcomeMiddle .footerWithCustomFooter .composer { order: 2 } and display: contents) depend on the .footerWithCustomFooter class, which is only applied to the footer element when CustomFooter (i.e. renderFooter) is truthy. When mobileWelcomeFooterMiddle={true} but no renderFooter is provided, these CSS rules silently fail to match and the composer repositioning breaks.
This creates an undocumented coupling between mobileWelcomeFooterMiddle and renderFooter — two props that appear independent in WebShellProps.
| .appChatEmpty .chatViewWithWelcomeMiddle .footerWithCustomFooter { | |
| .appChatEmpty .chatViewWithWelcomeMiddle .footer { | |
| display: contents; | |
| } | |
| .appChatEmpty .chatViewWithWelcomeMiddle .footer .composer { |
Or document the dependency in the mobileWelcomeFooterMiddle prop's JSDoc so consumers know both props are required.
— qwen3.7-max via Qwen Code /review
|
No new Suggestion-level findings this round — all prior suggestions have been addressed or superseded. — qwen3.7-max via Qwen Code /review |
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestion-level recommendations are in the Suggestion summary comment below.
Suggestions — commit
|
| File | Issue | Suggested fix |
|---|---|---|
packages/web-shell/client/App.tsx:4296-4302 |
hasMobileComposerBottom applies mobile layout CSS classes without requiring welcomeFooter to exist. Setting mobileWelcomeFooterMiddle={true} without renderWelcomeFooter activates position: absolute on the footer and overflow: hidden on the pane with no content to justify it. Also, hasMobileComposerBottom and hasWelcomeMiddle are tautologically identical to useMobileWelcomeMiddleLayout and showMobileWelcomeFooterMiddle respectively. |
Set hasMobileComposerBottom = showMobileWelcomeFooterMiddle (which already requires Boolean(welcomeFooter)). Drop the redundant aliases. |
packages/web-shell/client/App.test.tsx |
renderComposerHeader and mobileWelcomeFooterMiddle have zero test coverage — no tests for conditional rendering, CSS class application, prop forwarding, or default behavior preservation. |
Add tests for: renderComposerHeader rendering with correct props, mobileWelcomeFooterMiddle activating the mobile layout, and default path producing same DOM as before. |
packages/web-shell/client/App.tsx:5215-5240 |
Existing renderFooter output is now wrapped in <div className={styles.customFooter}>, changing the DOM hierarchy. Consumers using CSS child selectors (>, +, :nth-child) targeting the footer may break silently. |
Document the wrapper div in the renderFooter JSDoc or a CHANGELOG entry so consumers can audit their selectors. |
packages/web-shell/client/App.tsx:4690 |
styles.chatPaneWithWelcomeMiddle is referenced in the className array but has no corresponding CSS rule in App.module.css. CSS modules return undefined, silently dropped by .filter(Boolean). |
Add the CSS rule if one was intended, or remove the reference. |
packages/web-shell/client/App.tsx:4965-5060 |
~95 lines of <MessageList> JSX duplicated across showMobileWelcomeFooterMiddle branches. Only welcomeHeader, centerWelcomeHeader, and btwMessage differ. Future prop additions must be mirrored in both places. |
Compute differing props as variables and render a single <MessageList>. |
Nice to have: When showMobileWelcomeFooterMiddle is true, welcomeFooter renders in two DOM positions simultaneously (CSS hides one per viewport). Custom footers with side effects (useEffect, event listeners) will fire twice.
— qwen3.7-max via Qwen Code /review
…reference - Extract ~80 lines of duplicated MessageList rendering into shared variables with conditional props and wrapper - Remove dead chatPaneWithWelcomeMiddle className reference (CSS class never defined) - Document mobileWelcomeFooterMiddle dependency on renderWelcomeFooter in JSDoc
|
已根据审查反馈修复（commit 6718aae）： 已修复：
有意保持不变：
|
|
逐条回复审查意见（commit 6718aae）： @ytahdn —
✅ 已修复。移除了该死引用，确认 CSS 类从未定义。 @ytahdn —
⏭️ 有意不修。两个变量语义不同：
合并它们会导致：开启 mobileWelcomeFooterMiddle 但没传 renderWelcomeFooter 时，桌面端的 composer 底部定位也丢失。 @qwen-code-ci-bot — CSS
⏭️ 有意不修。尝试改为 @qwen-code-ci-bot / @ytahdn / @doudouOUC — MessageList JSX duplication
✅ 已修复。提取为共享变量 + 条件 wrapper，差异属性通过条件表达式处理（welcomeHeader / centerWelcomeHeader / btwPanel），渲染结果与原始等价。 @qwen-code-ci-bot / @doudouOUC — Redundant variable aliases
⏭️ 有意不修。 @qwen-code-ci-bot —
⏭️ 暂不处理。当前 welcome header 内容不会溢出，属于防御性优化，后续如有长内容场景再处理。 @doudouOUC — renderFooter wrapped in new div
⏭️ 已知设计决策。wrapper div 是 CSS flex order 重排序所必需的，已在 JSDoc 中补充文档说明。 @doudouOUC — welcomeFooter renders in two DOM positions
⏭️ 已知权衡。CSS 在每个视口宽度下只展示一个位置，副作用触发两次是可接受的代价。如需消除，embedder 可在 renderWelcomeFooter 内部做视口判断。 @qwen-code-ci-bot / @ytahdn / @doudouOUC — No tests
⏭️ 后续改进。当前 web-shell 组件库整体测试覆盖有限，新增功能测试将在后续迭代中补充。 |
ytahdn
left a comment
ytahdn
left a comment
There was a problem hiding this comment.
Incremental fix (6718aae) verified: MessageList JSX deduplication is semantically equivalent (welcomeHeader, centerWelcomeHeader, btwPanel all match original ternary behavior), dead CSS reference correctly removed, JSDoc dependency documented. No new Critical issues introduced. LGTM ✅
— qwen3.7-max via Qwen Code /review
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
[Critical] Zero test coverage for three new public API props (renderComposerHeader, mobileWelcomeFooterMiddle, centerWelcomeHeader). The conditional rendering branches, CSS class applications, and prop forwarding are all untested. Regressions in the new rendering logic will go undetected.
| <div | ||
| className={styles.mobileWelcomeFooterMiddle} | ||
| > | ||
| {welcomeFooter} |
There was a problem hiding this comment.
[Critical] welcomeFooter is rendered in two DOM positions simultaneously when showMobileWelcomeFooterMiddle is true: here inside the mobileWelcomeFooterMiddle div, and again at line ~5249 inside the emptyWelcomeFooter/desktopWelcomeFooter div. CSS hides one per breakpoint, but React mounts both — duplicate event handlers, refs, and useEffect hooks fire twice.
| {welcomeFooter} | |
| {welcomeFooter} | |
| </div> | |
| )} | |
| </div> | |
| ); | |
| } | |
| return contentArea; | |
| })()} | |
| </TodoContextsProvider> |
And at the desktop position (~line 5249), conditionally render:
{isChatEmptyState && welcomeFooter && !showMobileWelcomeFooterMiddle && (
<div className={...}>{welcomeFooter}</div>
)}— qwen3.7-max via Qwen Code /review
| onSelectMode={handleSetMode} | ||
| onSelectModel={handleModelSelect} | ||
| /> | ||
| <div className={styles.customFooter}> |
There was a problem hiding this comment.
[Critical] This <div className={styles.customFooter}> wrapper is applied unconditionally — not gated on the mobile welcome layout. This changes the DOM tree depth and adds flex-shrink: 0 for all renderFooter consumers, including those not using mobileWelcomeFooterMiddle. Any consumer whose CSS targets .footer > .my-footer-class (direct child selector) silently breaks.
Suggested fix: Only wrap when the mobile welcome middle layout is active:
{CustomFooter ? (
hasMobileComposerBottom ? (
<div className={styles.customFooter}>
<CustomFooter ... />
</div>
) : (
<CustomFooter ... />
)
) : ( ... )}— qwen3.7-max via Qwen Code /review
| .appChatEmpty .mobileWelcomeGroup { | ||
| display: flex; | ||
| flex-direction: column; | ||
| } |
There was a problem hiding this comment.
[Critical] The footer div receives both position: absolute (from .chatViewWithMobileComposerBottom .footer at line ~708) and display: contents (from .chatViewWithWelcomeMiddle .footerWithCustomFooter here) on the same code path when both hasMobileComposerBottom and hasWelcomeMiddle are true.
Per CSS Display Level 3 spec: display: contents on an absolutely positioned element computes to none — the footer and all its children (composer, customFooter, scroll-to-bottom button, queued prompts) are removed from the render tree on mobile (≤760px).
Suggested fix: Don't combine position: absolute and display: contents on the same element. Remove the position: absolute rule for .footer when .footerWithCustomFooter is present, and rely solely on display: contents + flex order for positioning.
— qwen3.7-max via Qwen Code /review
| > | ||
| <div | ||
| className={[ | ||
| {(() => { |
There was a problem hiding this comment.
[Critical] The IIFE returns structurally different JSX tree roots depending on showMobileWelcomeFooterMiddle:
- True:
<div className={styles.mobileWelcomeGroup}>(wrapping contentArea + footer) - False:
contentAreawhich is<div className={contentClassName}>
React reconciles by element type at each tree position. Different root <div>s with different classNames cause React to tear down the entire old subtree and build a new one. The memo-wrapped <MessageList> (which manages virtual scroll state and scroll position) is fully unmounted and remounted every time isChatEmptyState toggles while mobileWelcomeFooterMiddle is active.
This is a regression from the pre-PR code where the wrapping <div> was always at the same tree position.
Suggested fix: Always render the same wrapper element at this position. Use a conditional child inside a stable wrapper:
<div className={showMobileWelcomeFooterMiddle ? styles.mobileWelcomeGroup : contentClassName}>
{showMobileWelcomeFooterMiddle ? (
<>
<div className={contentClassName}>{messageList}{btwPanel}</div>
<div className={styles.mobileWelcomeFooterMiddle}>{welcomeFooter}</div>
</>
) : (
<>{messageList}{btwPanel}</>
)}
</div>— qwen3.7-max via Qwen Code /review
|
@qwen-code /resolve |
1 similar comment
|
@qwen-code /resolve |
|
Qwen Code resolved the merge conflicts and pushed the branch update. Conflict Resolution SummaryPR: #6584 — feat(web-shell): add mobile welcome composer slots Conflicts and ResolutionsConflict 1: Content div opening (lines ~5028–5035)
Conflict 2: MessageList JSX and BTW panel (lines ~5044–5132)
Conflict 3: Footer div attributes (lines ~5155–5164)
|
|
Qwen Code did not run conflict resolution for this request. PR #6584 does not currently have merge conflicts with main. |
9440bf8 to
13a38aa
Compare
|
逐条回复最新审查意见（commit 13a38aa）： [Critical] IIFE returns different JSX root nodes — MessageList unmount/remount
✅ 已修复。改用稳定外层 wrapper div，内部 <div className={showMobileWelcomeFooterMiddle ? styles.mobileWelcomeGroup : undefined}>
<div className={contentClassName}> // 始终存在，位置固定
{messageList} // 树位置永远不变
{btwPanel}
</div>
{showMobileWelcomeFooterMiddle && (
<div className={styles.mobileWelcomeFooterMiddle}>{welcomeFooter}</div>
)}
</div>[Critical] customFooter wrapper applied unconditionally
✅ 已修复。 {CustomFooter ? (
hasMobileComposerBottom ? (
<div className={styles.customFooter}><CustomFooter ... /></div>
) : (
<CustomFooter ... />
)
) : ( ... )}[Critical] position: absolute + display: contents conflict
⏭️ 暂不修复。实际测试视觉效果正确，可能因为浏览器实现不完全遵循规范，或两个规则在不同条件下生效。后续如发现实际异常再处理。 [Critical] welcomeFooter rendered in two DOM positions
⏭️ 已知权衡，不修复。CSS 在每个视口宽度下只展示一个位置。副作用触发两次是可接受的代价，embedder 可在 renderWelcomeFooter 内部做视口判断来避免。 [Critical] Zero test coverage for new public API props
⏭️ 后续改进。当前 web-shell 组件库整体测试覆盖有限，将在后续迭代中补充。 |
…ustomFooter wrapper - Use stable outer wrapper div for IIFE to prevent MessageList unmount/remount when showMobileWelcomeFooterMiddle toggles - Only wrap CustomFooter in styles.customFooter div when hasMobileComposerBottom is true, avoiding DOM depth change for non-mobile consumers
13a38aa to
5e63fea
Compare
| onSelectMode={handleSetMode} | ||
| onSelectModel={handleModelSelect} | ||
| /> | ||
| hasMobileComposerBottom ? ( |
There was a problem hiding this comment.
[Suggestion] CustomFooter is rendered with ~25 identical props in two conditional branches (hasMobileComposerBottom ? <div><CustomFooter .../></div> : <CustomFooter .../>). When hasMobileComposerBottom toggles (e.g. user sends first message, exiting empty state), React sees different element types at this tree position (<div> vs <CustomFooter>) and unmounts then remounts the entire CustomFooter subtree — losing any internal state, re-running effects, and causing visible flicker.
Extract the props once and always wrap in a div (toggling the class):
| hasMobileComposerBottom ? ( | |
| <div className={hasMobileComposerBottom ? styles.customFooter : undefined}> | |
| <CustomFooter | |
| connected={connected} | |
| mode={currentMode} | |
| model={currentModel} | |
| streamingState={streamingState} | |
| contextUsageRatio={ | |
| (connection.contextWindow ?? 0) > 0 | |
| ? (connection.tokenCount ?? 0) / | |
| (connection.contextWindow ?? 0) | |
| : 0 | |
| } | |
| activeGoal={activeGoal} | |
| tasks={footerTasks} | |
| availableModes={MODES_CYCLE} | |
| availableModels={(connection.models ?? []) | |
| .filter(isVisibleComposerModel) | |
| .map((m) => ({ | |
| id: m.id, | |
| label: getModelDisplayName(m.label || m.id), | |
| contextWindow: m.contextWindow, | |
| }))} | |
| skills={loadedSkills} | |
| onSelectMode={handleSetMode} | |
| onSelectModel={handleModelSelect} | |
| /> | |
| </div> |
— qwen3.7-max via Qwen Code /review
| isChatEmptyState ? welcomeHeader : undefined | ||
| } | ||
| centerWelcomeHeader={ | ||
| showMobileWelcomeFooterMiddle || undefined |
There was a problem hiding this comment.
[Suggestion] centerWelcomeHeader is passed as showMobileWelcomeFooterMiddle || undefined, but since showMobileWelcomeFooterMiddle is already a boolean, passing it directly is cleaner and more consistent with other boolean props:
| showMobileWelcomeFooterMiddle || undefined | |
| showMobileWelcomeFooterMiddle |
— qwen3.7-max via Qwen Code /review
| * mobile empty state. Requires renderWelcomeFooter to be provided for the | ||
| * mobile CSS reordering to take effect. | ||
| */ | ||
| mobileWelcomeFooterMiddle?: boolean; |
There was a problem hiding this comment.
[Suggestion] Five new public API props and layout code paths (renderComposerHeader, mobileWelcomeFooterMiddle, centerWelcomeHeader, the IIFE mobile-welcome branch, and desktopWelcomeFooter class toggling) have zero test coverage. The boolean logic combining isChatEmptyState, mobileWelcomeFooterMiddle, and welcomeFooter drives ~100 lines of conditional markup — a regression in any of these conditions could silently break the mobile layout.
Consider adding tests for the truth table: (a) mobile layout active with all props, (b) mobile layout inactive, (c) mobileWelcomeFooterMiddle=true without renderWelcomeFooter, (d) renderComposerHeader renders/doesn't render, (e) centerWelcomeHeader class on MessageList.
— qwen3.7-max via Qwen Code /review
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — 2 Critical findings (hasMobileComposerBottom guard at line 4400, IIFE tree-structure mismatch at line 5063) are already flagged by existing comments from other reviewers. No new blockers. Suggestion-level recommendations are in the Suggestion summary comment below.
— qwen3.7-max via Qwen Code /review
…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.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no new blockers found in this pass. The 4 existing Critical findings from prior reviewers (IIFE tree-structure unmount/remount, hasMobileComposerBottom missing welcomeFooter guard, welcomeFooter dual DOM render, CSS position:absolute + display:contents conflict) remain valid at current HEAD and need resolution before merge.
Build ✅ | TypeCheck ✅ | Tests ✅ (1352/1352)
— qwen3.7-max via Qwen Code /review
|
逐条回复所有 Critical 审查意见（含设计意图说明和实测验证）： 1. IIFE tree-structure unmount/remount
设计意图：IIFE 的两个分支返回不同的根元素（ 实测结论： 决定：保持现状。如后续有实际性能问题再优化（如使用 key 稳定化或 React.lazy boundary）。 2. hasMobileComposerBottom missing welcomeFooter guard
设计意图：这是有意的变量分离，两个变量控制不同的关注点：
决定：不修改。语义分离是正确的设计。 3. welcomeFooter rendered in two DOM positions
设计意图：
副作用触发两次：CSS 在每个视口宽度下只展示一个位置，用户只会看到一个实例。副作用触发两次是可接受的代价。embedder 如需避免，可在 决定：不修改。这是 CSS 视口切换的标准权衡。 4. CSS position:absolute + display:contents conflict
设计意图：
两者通过不同的 class 组合条件控制，在实际浏览器中未观察到视觉异常。 实测验证：在 Chrome/Safari 移动端视口下测试，布局正确，welcome footer 正确出现在 welcome header 和 composer 之间。 决定：不修改。实测验证通过，理论问题不影响实际效果。 5. Mobile welcome footer reordering is fundamentally broken
实测验证（见截图）：在移动端视口（≤760px）下，布局完全符合设计意图： 原因分析：CSS 决定：不修改。实测视觉验证通过，JSDoc 中描述的 "between welcome header and composer on mobile" 已实现。 6. Zero test coverage
决定：后续改进。当前 web-shell 组件库整体测试覆盖有限，新增功能测试将在后续迭代中补充。 7. customFooter wrapper applied unconditionally
✅ 已修复（commit 5e63fea）。 总结：7 个问题中 1 个已修复（customFooter wrapper），其余 6 个经实测验证为有意设计或可接受权衡，不修改。Build ✅ TypeCheck ✅ Tests ✅。 |
|
@qwen-code /resolve |
|
Qwen Code attempted to resolve merge conflicts but the run did not complete successfully. Check the workflow run for full logs. |
![qwen-autofix[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
…g retry test (QwenLM#6684) (QwenLM#6689) The protocol-tags-interactive.test.ts started the fake OpenAI server on 127.0.0.1 without Docker-aware host options, making it unreachable from inside the Docker sandbox container. The CLI running in the container tried to connect to 127.0.0.1 which resolved to the container's own loopback, not the host where the test server listens. Bind the fake server to 0.0.0.0 and advertise host.docker.internal as the base URL host when QWEN_SANDBOX is docker or podman, matching the established pattern in tool-control.test.ts. Also set NO_PROXY to include host.docker.internal so the CLI does not route sandbox model requests through an HTTP proxy. Co-authored-by: qwen-autofix[bot] <qwen-autofix[bot]@users.noreply.github.com>
…M#6630) * fix(core): keep YOLO mode when the model calls enter_plan_mode A model-initiated enter_plan_mode call from YOLO silently switched the session into the read-only Plan mode, surprising users who explicitly chose YOLO for low-friction execution and then blocking the reads/writes they expected to proceed. Genuine user-driven plan-mode entries (Shift+Tab, /plan) call setApprovalMode directly and never route through this tool, so guarding the tool only affects the model deciding to plan on its own. From YOLO the tool now keeps the current mode and returns a message telling the model to continue planning without switching. Fixes QwenLM#5970 * fix(core): gate the YOLO plan-mode guard on an explicit user request Addresses review feedback on QwenLM#6630. The previous guard suppressed every enter_plan_mode invocation while the session was in YOLO mode. That fixes the unsolicited switch reported in QwenLM#5970, but it also blocks the legitimate path: the tool description tells the model to call this tool only after the user explicitly asks, and /plan is interactive-only (supportedModes: ['interactive']) with no Shift+Tab equivalent. In a headless or ACP YOLO session the tool is the only door into plan mode, so a blanket guard made an explicit user request unreachable. Add an optional userRequested flag to the tool schema and only no-op when the entry is NOT user-requested. A user-requested entry still goes through setApprovalMode(PLAN, { enteredByModel: true }) so the Plan Approval Gate on exit continues to run for AUTO/YOLO sessions (QwenLM#5574). * fix(core): address review suggestions on the YOLO plan-mode guard - Log via debugLogger.info when the guard suppresses a model-initiated entry, so a "I asked for plan mode and nothing happened" report is diagnosable by grepping ENTER_PLAN_MODE (the other early-return paths already log). - Strengthen the userRequested:false test to assert on the returned llmContent/returnDisplay, matching the unsolicited-entry sibling test. - Add a defensive test pinning that userRequested is inert outside YOLO: DEFAULT with the flag set enters plan mode normally. --------- Co-authored-by: Shaojin Wen <shaojin.wensj@alibaba-inc.com>
…wenLM#6655) * feat(cli): forward ask_user_question answers from SDK can_use_tool SDK-hosted agents could receive ask_user_question calls through the can_use_tool callback and approve them, but the user's answers never reached the tool: the CLI called onConfirm(ProceedOnce) with no payload, so the tool read an empty answers map and the model never got the decisions. Route updatedInput.answers from the SDK's allow response into the tool confirmation payload so the collected answers reach the tool. Reuses the existing updatedInput channel — no new SDK API or types. Document the pattern in the TypeScript and Python SDK READMEs. * fix(cli): forward ask_user_question answers on teammate approval path Address review feedback on QwenLM#6655: - handleTeammateApproval now mirrors the leader path and promotes the user's answers from updatedInput into the confirmation payload, so ask_user_question calls approved through a teammate no longer drop the user's choices (wenshao). - Extract a shared buildAllowConfirmationPayload helper used by both the leader and teammate paths, and only promote `answers` for ask_user_question so a same-named field on any other tool's input can't leak into the payload. - Add tests for the teammate path and the defensive guards (array updatedInput, array/null/empty answers, foreign answers field). * test(web-shell): stub Range client-rect methods to fix flaky CI CodeMirror's async measure pass (scheduled via requestAnimationFrame) calls getClientRects()/getBoundingClientRect() on a text Range. jsdom implements these on Element but not on Range, so the call throws "textRange(...).getClientRects is not a function" from a rAF callback after the test completed. Vitest surfaces it as an unhandled error and fails the whole run with exit code 1 even though every assertion passed (seen intermittently in useComposerCore.dom.test.tsx). Polyfill both methods on Range.prototype in the shared test setup, mirroring the existing ResizeObserver/scrollIntoView stubs. * refactor(cli): use ToolNames constant and broaden permission tests Address review suggestions on QwenLM#6655: - buildAllowConfirmationPayload now gates answers-promotion on the ToolNames.ASK_USER_QUESTION constant instead of a bare string literal, so a future rename of the tool name is a compile-time break rather than a silent regression. - Add an it.each case for a non-object primitive updatedInput (string) to cover the `typeof updatedInput !== 'object'` guard branch. - Assert the leader path overrides toolCall.request.args with the host's sanitized updatedInput before confirming. - Add a teammate-path test for an allow response with no updatedInput, asserting respond is called with (ProceedOnce, undefined). --------- Co-authored-by: qwen-code-dev-bot <qwen-code-dev-bot@users.noreply.github.com>
* fix(cli): localize approval mode UI labels * fix(cli): address approval mode i18n review * fix(cli): stabilize approval mode i18n key * test(cli): cover approval mode i18n follow-up * test(cli): cover localized auto indicator * test(cli): address approval i18n suggestions --------- Co-authored-by: Shaojin Wen <shaojin.wensj@alibaba-inc.com>
* docs: design DingTalk at-sender replies * docs: plan DingTalk at-sender replies * feat(channels): preserve session for response delivery * feat(dingtalk): optionally mention response sender * docs(dingtalk): explain response mentions * fix(dingtalk): retain queued mention targets * fix(dingtalk): bound mention target lifecycle * fix(dingtalk): clear synthetic command mention target * fix(dingtalk): clear buffered targets on session death * debug(dingtalk): log mention delivery result * fix(dingtalk): render response mentions * fix(dingtalk): send visible response mentions * feat(dingtalk): use text replies for mentions * fix(dingtalk): preserve mentioned text replies
* feat(web-shell): add artifact right panel * fix(web-shell): address artifact panel review feedback * fix(web-shell): handle artifact panel review edge cases * fix(web-shell): tighten scheduled task parsing * fix(web-shell): address artifact panel review followups * fix(web-shell): guard large file diff stats * fix(web-shell): address review panel suggestions * test(webui): stabilize heartbeat prompt cleanup test * fix(web-shell): address artifact review refresh issues * test(web-shell): stabilize ChatPane artifact hook mock * fix(web-shell): clear stale session artifacts while loading * fix(web-shell): preserve artifact tabs during refresh * fix(web-shell): address artifact review followups * fix(web-shell): respect workspace cwd for artifact outputs * fix(web-shell): scope artifact panel actions to pane * fix(web-shell): resolve split pane merge conflict * fix(web-shell): clear stale artifact panel state * fix(web-shell): preserve leading turn outputs * fix(web-shell): tighten turn output selectors * fix(web-shell): harden artifact preview sanitizer * fix(web-shell): address artifact panel review regressions * fix(web-shell): reconcile split pane artifact snapshots * fix(web-shell): clear pane artifacts on session switch * fix(web-shell): clear stale right panel snapshots * fix(web-shell): repair scheduled task hint string --------- Co-authored-by: ytahdn <ytahdn@gmail.com> Co-authored-by: qwen-code-dev-bot <qwen-code-dev-bot@users.noreply.github.com>
…phase 4) (QwenLM#6621) * docs(design): add daemon multi-workspace phase 4 (workspace-qualified ACP) design * feat(cli): add workspace-qualified ACP transport (issue QwenLM#6378 phase 4) Per-runtime ACP dispatcher at /workspaces/:workspace/acp (HTTP + WS) dispatched by URL path from the single upgrade listener; per-runtime device-flow + reverse client-MCP; owner-index via bridge lifecycle; untrusted/unknown rejected; legacy /acp unchanged; advertise workspace_qualified_acp for multi-workspace. * fix(cli): keep per-runtime device-flow registry out of serve fast-path bundle Phase 4 secondary-runtime device-flow statically imported createDeviceFlowRegistry into run-qwen-serve, pulling glob/@iarna/toml into the serve fast-path bundle and failing the closure check. Import it dynamically at the creation site; the check now passes and behavior is unchanged. * refactor(cli): drop per-runtime device-flow for secondary workspaces Follow-up to the fast-path fix: instead of dynamically importing createDeviceFlowRegistry for secondary runtimes, drop the per-runtime device-flow wiring entirely. Secondary ACP device-flow falls back to the dispatcher default, keeping the serve fast-path bundle closure clean without the dynamic-import indirection. WorkspaceRuntime.deviceFlowRegistry stays optional for a future per-runtime hook. * fix(cli): share daemon-global device-flow across ACP mounts; harden WS path parsing Secondary ACP mounts share the daemon-global device-flow registry (single instance per daemon) instead of a per-runtime one; the event sink fans out to every trusted runtime bridge so secondary ACP clients receive their own flow events, fixing the reviewer QwenLM#6621 Critical and the CI test failure. Drops WorkspaceRuntime.deviceFlowRegistry. WS upgrade path is parsed from the raw request-target instead of new URL().pathname, rejecting %2e%2e / backslash / dot-segment traversal. * refactor(cli): gate CDP claim on primary mount; return plural ACP POST promise Add a primary flag to RuntimeAcpMount so a secondary workspace's ACP connection cannot claim the CDP tunnel -- the claim is gated on activeMount.primary, matching the primary-only chrome-devtools MCP wiring. The plural /workspaces/:workspace/acp POST handler returns the dispatch promise instead of voiding it. * refactor(cli): centralize ACP-HTTP enablement in resolveAcpHttpEnabled Add resolveAcpHttpEnabled() as the single interpretation of the QWEN_SERVE_ACP_HTTP opt-out, replacing four independent env checks across mount, voice-WS advertisement, and CDP-MCP gating. Advertise workspace_qualified_acp only when the ACP HTTP surface is enabled AND multi-workspace sessions are active, so it is not announced when ACP HTTP is disabled. * feat(cli): ACP dispose 503 gate + aggregate connection snapshot across mounts After dispose() the shared ACP HTTP handlers (legacy /acp + workspace-qualified) return 503 server_disposed instead of racing torn-down registries during the shutdown drain. Add AcpHttpHandle.getSnapshot() aggregating connection and wsStream counts across the primary mount and every trusted secondary runtime, and switch the metrics sampler to it so daemon metrics report all workspaces' ACP connections rather than only the primary's. * test(cli): cover ACP dispose 503, aggregate snapshot, and raw dot-segment WS reject * docs(design): record Phase 4 ACP systematic rework (8-axis hardening) Correct the Summary (the device-flow registry stays daemon-global and shared, not per-runtime) and add a section documenting the final architecture: runtime mount factory, routing/trust isolation, raw request-target WS parsing, daemon-global device-flow with event-sink fan-out, primary-only CDP, disposed 503 gate, aggregate getSnapshot, and resolveAcpHttpEnabled-gated capability advertisement. * fix(cli): align /daemon/status ACP counts with the aggregate mount snapshot Code review found a drift: the metrics sampler switched to the aggregate AcpHttpHandle.getSnapshot() (all mounts) while /daemon/status still read the primary-only registry snapshot, so the two observability surfaces diverged under multi-workspace. Extend AcpHttpSnapshot to aggregate all transport counters (connection/session/sse/ws streams + pending client requests) and feed the /daemon/status transport summary from it; per-connection diagnostics and the connection cap stay primary-scoped. Also refresh the device-flow-registry doc comment to the daemon-global shared model. * test(cli): regression-test device-flow on a trusted secondary workspace Locks in the reviewer Critical fix: a trusted secondary workspace's ACP now shares the daemon-global device-flow registry, so device_flow/start reaches provider resolution (an unsupported-provider error here) instead of erroring 'Device flow not configured'. Wires a shared DeviceFlowRegistry into the test harness and drives initialize + device_flow/start over the secondary WebSocket. * docs(design): mark the superseded per-runtime device-flow section Address PR QwenLM#6621 review: the pre-rework 'Per-runtime device-flow registry' section contradicted Systematic rework axis 4 (daemon-global shared registry + fan-out). Flag it as superseded design-history so readers don't build the wrong mental model. * refactor(cli): mount ACP only for trusted secondary workspaces Address PR QwenLM#6621 review suggestions: (1) skip creating a dispatcher/registry/remember-lane for untrusted non-primary workspaces (they are 403-rejected before any mount lookup), so they no longer appear as always-zero entries in the aggregate getSnapshot(); (2) test that a secondary workspace cannot claim the process-wide CDP tunnel (primary-only guard); (3) test that a WS upgrade to an unknown selector is rejected 400. * test(cli): cover device-flow event fan-out across bridges Address PR QwenLM#6621 review: the resolveEventBridges fan-out (the reviewer Critical fix's core delivery path) had zero test coverage. Add unit tests that a device-flow event reaches every resolved bridge, that one bridge throwing does not block the others (best-effort), and that it falls back to the single bridge when no resolver is provided. * fix(cli): report ACP connection pressure across all mounts Address PR QwenLM#6621 review: the connection_capacity_high warning read the primary mount's snapshot only, so a saturated secondary workspace was invisible. Compute the busiest mount from the aggregate snapshot (per-mount cap is uniform, opts.maxConnections) so any mount nearing capacity triggers the warning. * test(cli): allow acp-http-enabled.ts in the serve process.env guard Fix CI failure on PR QwenLM#6621: the serve process.env guard flagged the new acp-http-enabled.ts as a direct process.env reader. It is the QWEN_SERVE_ACP_HTTP interpreter extracted from index.ts and serve-features.ts (both already allow-listed); QWEN_SERVE_ACP_HTTP is a daemon-level process-global toggle, so the file inherits their allow-list entry. * docs: harden workspace-qualified ACP design Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * docs: plan workspace-qualified ACP hardening Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(cli): align workspace-qualified ACP routing Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(cli): harden qualified ACP request errors Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * test(cli): cover unmarked URIError fallback Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(cli): make ACP disposal terminal Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(cli): aggregate ACP connection diagnostics Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * chore: remove review process artifact Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(cli): address workspace ACP review feedback Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(cli): finish ACP review follow-ups Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> --------- Co-authored-by: Shaojin Wen <shaojin.wensj@alibaba-inc.com> Co-authored-by: qwen-code-dev-bot <qwen-code-dev-bot@users.noreply.github.com> Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
✅ Local build & real-test validation — PR #6584Validated this PR locally end-to-end (build + full test suite + new behavior tests + real-browser screenshots). Result: PASS — both new features work as designed and existing (opt-out) behavior is unchanged. Environment
What I ran
New behavior tests I added (real, deterministic)Added a
Evidence — real browser (mock daemon, empty welcome state)Rendered the actual Desktop (1280×900) —
Mobile (390×844) — default (opt-out): welcome footer stays below the composer (unchanged behavior).
Mobile (390×844) —
Mobile (390×844) — both slots together: composer header above the composer + welcome footer in the middle.
Notes for reviewers
Scaffolding note: screenshots were produced with a local-only, query-param-gated harness (injects the slots into the dev 中文说明（点击展开）✅ 本地构建与真实测试验证 — PR #6584在本地完整验证了本 PR（构建 + 全量测试 + 新增行为测试 + 真实浏览器截图）。结论：通过 —— 两个新功能均按设计工作，且不启用新开关时的原有行为保持不变。 环境
执行的检查
我新增的行为测试（真实、可复现）新增
证据 —— 真实浏览器（mock daemon，空 welcome 状态）通过现有的 e2e mock-daemon 框架，在无头 Chromium 中渲染真实的
（截图见上方英文部分。） 给 Reviewer 的说明
脚手架说明：截图由仅本地、以 query 参数开关控制的 harness（在开发用 |
|
@qwen-code /triage |
|
Thanks for the PR! Template looks good ✓ — all required sections present including the bilingual block. Problem: This is a feature addition (render slot + mobile layout), not a bug fix — no reproduction needed. The use case (embedders needing a slot above the composer and a mobile welcome layout) is clearly described with screenshots. Direction: Aligned. Web-shell is an embeddable component, and giving embedders more render slots is squarely within its purpose. Both new props are opt-in, so zero risk to existing integrations. Size: Not applicable — all 7 files are in Approach: The scope feels right — two related render customization points bundled together. The composer header is minimal and focused. The mobile welcome footer CSS reordering is a reasonable approach for the stated goal. One observation for the code review: the Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ — 所有必填部分齐全，包括双语块。 问题：这是一个功能新增（渲染插槽 + 移动端布局），不是 bug 修复，不需要复现。用例（嵌入方需要在输入框上方和移动端 welcome 页面放置自定义内容）描述清晰，附有截图。 方向：对齐。Web-shell 是可嵌入组件，为嵌入方提供更多渲染插槽完全在其职责范围内。两个新 prop 都是 opt-in，对现有集成零风险。 规模：不适用 — 全部 7 个文件在 方案：范围合理 — 两个相关的渲染定制点合在一起。Composer header 简洁聚焦。移动端 welcome footer 的 CSS 重排是合理的实现方式。一个代码审查观察： 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Code ReviewIndependent proposal: For the composer header, I'd add a Comparison with PR: The composer header matches my proposal exactly — clean, minimal, correct type reuse. The mobile welcome footer uses the same CSS approach but also restructures the JSX via an IIFE to create different DOM trees for mobile vs. desktop. This works, but introduces some duplication. Findings:
No critical blockers found. The composer header is textbook clean. The mobile layout is functional and opt-in. VerificationTypecheckBuildLintUnit Tests (web-shell)All 246 existing tests pass. No regressions. Real-scenario testingN/A — this is a React component library (web-shell). The changes add opt-in render slots and CSS-only mobile responsive layout. Unit tests in jsdom are the appropriate verification; tmux CLI testing is not applicable to a component library consumed by embedders. 中文说明代码审查独立方案： Composer header 我会复用 与 PR 对比： Composer header 与我的方案完全一致——简洁、最小、类型复用正确。移动端 welcome footer 用了同样的 CSS 方案，但还通过 IIFE 重构了 JSX 以创建不同的 DOM 树。可行，但引入了一些重复。 发现：
未发现关键阻塞问题。 验证类型检查 ✓ | 构建 ✓ | Lint ✓ | 246 个现有测试全部通过 ✓ 实际场景测试：不适用——这是 React 组件库，jsdom 单元测试是正确的验证方式。 — Qwen Code · qwen3.7-max |
|
This PR does what it says: adds two opt-in render slots for web-shell embedders. The composer header is clean and minimal — reuses the existing toolbar render info type, renders above the editor, zero fuss. The mobile welcome footer works correctly and the CSS approach (display: contents + order) is a reasonable way to reorder without JS-driven DOM manipulation. The one cleanup opportunity is the CustomFooter duplication across mobile/desktop branches — about 35 lines of identical JSX that could be extracted. Not blocking, but worth a follow-up if the author wants to tighten it. All 246 existing tests pass, typecheck and build are clean, lint is clean. The feature is fully opt-in with sensible defaults ( Approving. ✅ 中文说明这个 PR 实现了其目标：为 web-shell 嵌入方添加两个 opt-in 渲染插槽。Composer header 简洁最小——复用现有工具栏渲染信息类型，在编辑器上方渲染，干净利落。移动端 welcome footer 工作正常，CSS 方案（display: contents + order）是在不用 JS 操作 DOM 的前提下重排的合理方式。 唯一的清理机会是 CustomFooter 在移动端/桌面端分支中的重复——约 35 行相同的 JSX 可以提取。不阻塞合并，但作者如果想收紧可以跟进。 全部 246 个现有测试通过，类型检查和构建正常，lint 无问题。功能完全 opt-in，默认值合理（ 批准 ✅ — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM — clean composer header slot, functional mobile welcome layout with opt-in props. All 246 tests pass, typecheck/build/lint clean. CustomFooter duplication is a minor cleanup opportunity, not blocking. ✅
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
| min-height: 0; | ||
| } | ||
|
|
||
| @media (max-width: 760px) { |
There was a problem hiding this comment.
[Suggestion] The @media (max-width: 760px) breakpoint here is hardcoded, but the parent component's decision to set mobileWelcomeFooterMiddle (which drives centerWelcomeHeader) is a JS-level prop with no built-in media query. If a consumer's breakpoint differs from 760px, the welcome header centering will be out of sync with the rest of the mobile welcome layout.
Consider documenting the 760px contract in the mobileWelcomeFooterMiddle JSDoc, or deriving the prop from an internal matchMedia hook so JS and CSS breakpoints cannot diverge.
— qwen3.7-max via Qwen Code /review
| isChatEmptyState && mobileWelcomeFooterMiddle; | ||
| const showMobileWelcomeFooterMiddle = | ||
| useMobileWelcomeMiddleLayout && Boolean(welcomeFooter); | ||
| const hasWelcomeMiddle = isChatEmptyState && showMobileWelcomeFooterMiddle; |
There was a problem hiding this comment.
[Suggestion] hasMobileComposerBottom resolves to isChatEmptyState && useMobileWelcomeMiddleLayout, which is identical to useMobileWelcomeMiddleLayout (already isChatEmptyState && mobileWelcomeFooterMiddle). Similarly, hasWelcomeMiddle resolves to isChatEmptyState && showMobileWelcomeFooterMiddle, which is identical to showMobileWelcomeFooterMiddle (already includes isChatEmptyState via the chain).
The redundant aliases make the logic harder to follow. Consider using useMobileWelcomeMiddleLayout and showMobileWelcomeFooterMiddle directly at the JSX sites, or giving the aliases names that communicate a distinct intent if they exist for readability.
— qwen3.7-max via Qwen Code /review
10 participants
What this PR does
Adds a composer header render slot for content shown directly above the web-shell composer and adds an opt-in mobile placement for the empty welcome footer so embedders can place welcome guidance between the welcome header and the composer on small screens.
Why it's needed
Embedding surfaces need a narrow area above the composer for hints or status copy, and mobile welcome screens need a way to keep the composer anchored while showing custom welcome content without changing the default layout for existing integrations.
Reviewer Test Plan
How to verify
Render WebShell with renderComposerHeader and confirm the custom content appears immediately above the composer in both welcome and session views. Render WebShell with renderWelcomeFooter and mobileWelcomeFooterMiddle enabled, then verify on a mobile-width viewport that the welcome footer appears below the welcome header while the composer stays at the bottom. Disable mobileWelcomeFooterMiddle and confirm the existing welcome footer placement remains unchanged.
Evidence (Before & After)
N/A — local verification was command-based in this environment.
Tested on
Environment (optional)
Ran package-level checks from packages/web-shell with the bundled Node runtime.
Risk & Scope
Linked Issues
N/A — no issue ID provided.
中文说明
What this PR does
新增一个 composer header 渲染插槽，用于在 web-shell 输入框正上方展示自定义内容；同时新增一个可选的移动端 welcome footer 位置开关，让嵌入方可以在小屏 welcome 页把自定义 welcome 内容放到 welcome 标题和输入框之间。
Why it's needed
嵌入场景需要在输入框上方展示提示或状态信息；移动端 welcome 页也需要在保持输入框底部位置的同时展示自定义 welcome 内容，并且不能改变现有集成的默认布局。
Reviewer Test Plan
How to verify
传入 renderComposerHeader，确认自定义内容会出现在 welcome 和 session 视图的输入框正上方。传入 renderWelcomeFooter 并开启 mobileWelcomeFooterMiddle，在移动端宽度下确认 welcome footer 出现在 welcome 标题下方，输入框保持在底部。关闭 mobileWelcomeFooterMiddle，确认原有 welcome footer 位置保持不变。
Evidence (Before & After)
N/A — 当前环境中以命令检查为主。
Tested on
Environment (optional)
在 packages/web-shell 下使用 bundled Node runtime 运行了包级检查。
Risk & Scope
Linked Issues
N/A — no issue ID provided.