Skip to content

fix(web-shell): keep the auto-sized dialog's gutter on a phone - #11077

Merged
wenshao merged 1 commit into
mainfrom
fix/auto-dialog-gutter
Sep 6, 2026
Merged

wenshao merged 1 commit into
mainfrom
fix/auto-dialog-gutter

Conversation

@yiliang114

Copy link
Copy Markdown
Collaborator

What this PR does

Floors the size="auto" dialog at the same 1rem gutter its ceiling already reserves, so it stops rendering flush to both screen edges on a narrow viewport.

Why it's needed

auto floors the panel at min-w-[min(100%,560px)] so a small plan graph does not collapse to a narrow column. tailwind-merge keeps that class alongside the base ceiling max-w-[calc(100%-2rem)] — they are different modifier groups, so neither wins the merge — and below the sm: breakpoint min-width takes precedence over max-width. At 390px the used width is therefore 390px against a 358px ceiling, and the Plan & tasks dialog draws its rounded, ringed panel edge-to-edge while every fixed size (sm/md/lg/xl) keeps its gutter at that same width.

Split out of #10938, where it was one small fix behind a much larger visual change. It is independent of everything else on that branch.

Reviewer Test Plan

How to verify

  • Open a size="auto" dialog (Plan & tasks is the one that motivated this) at a 390px viewport. Expect a 1rem gutter on both sides, matching what size="md" does at the same width.
  • Confirm a wide viewport is unchanged: the panel still tracks its content up to the sm:max-w-[min(calc(100vw-2rem),1120px)] ceiling and does not collapse to a narrow column.

Evidence (Before & After)

Before: at 390px the panel's used width is 390px — min-w-[min(100%,560px)] resolves to 390px and outranks the 358px ceiling, so the panel touches both screen edges.

After: the floor resolves to 358px, the ceiling holds, and the gutter survives.

Tested on

OS Status
🍏 macOS ⚠️ not tested
🪟 Windows ⚠️ not tested
🐧 Linux

Environment (optional)

client/components/dialogs/DialogShell.test.tsx — 7/7 locally.

Risk & Scope

  • Main risk or tradeoff: DialogShell is shared, so this reaches every size="auto" dialog. That is the intent — the gutter is what every other size already does — but it is the reason the test now pins the new floor, the absence of the old one, and the surviving base ceiling together rather than just the new class.
  • Not validated / out of scope: no visual capture; the change is a single Tailwind arbitrary value and is asserted on the merged class list.
  • Breaking changes / migration notes: none.

Linked Issues

Refs #10938.

中文说明

这个 PR 做了什么

size="auto" 对话框的宽度下限,改为减去它的上限本来就预留的那 1rem 边距,使它在窄视口下不再紧贴屏幕两边。

为什么需要

automin-w-[min(100%,560px)] 作为下限,以免小的 plan 图坍缩成窄条。tailwind-merge 会把这个类和基础上限 max-w-[calc(100%-2rem)] 同时保留——两者属于不同的修饰符组,谁也不会被合并掉——而在 sm: 断点以下,min-width 的优先级高于 max-width。于是在 390px 时实际宽度是 390px、上限是 358px,Plan & tasks 对话框会把它带圆角和描边的面板一直画到屏幕两边,而所有固定尺寸(sm/md/lg/xl)在同样宽度下都保住了边距。

#10938 拆出:它原本是一个小修复,却压在一个大得多的视觉改动后面,且与该分支上的其他内容互不依赖。

评审者验证计划

如何验证

  • 在 390px 视口下打开一个 size="auto" 的对话框(促成本次修复的是 Plan & tasks)。两侧应有 1rem 边距,与同宽度下的 size="md" 一致。
  • 确认宽视口行为不变:面板仍随内容变宽,直到 sm:max-w-[min(calc(100vw-2rem),1120px)] 上限,且不会坍缩成窄条。

证据(改前 / 改后)

改前:390px 下面板实际宽度为 390px——min-w-[min(100%,560px)] 解析为 390px 并压过 358px 的上限,面板贴到屏幕两边。

改后:下限解析为 358px,上限生效,边距得以保留。

测试环境

系统 状态
🍏 macOS ⚠️ 未测试
🪟 Windows ⚠️ 未测试
🐧 Linux

运行环境(可选)

client/components/dialogs/DialogShell.test.tsx —— 本地 7/7 通过。

风险与范围

  • 主要风险或权衡: DialogShell 是共享组件,因此改动会作用到每一个 size="auto" 对话框。这正是意图——边距本来就是其他所有尺寸的既有行为——也正因如此,测试现在同时钉住新的下限、旧下限的消失、以及幸存的基础上限,而不只是断言新类存在。
  • 未验证 / 不在范围: 没有视觉截图;本改动只是一个 Tailwind 任意值,验证方式是断言合并后的类名列表。
  • 破坏性变更 / 迁移说明: 无。

关联 Issue

Refs #10938.

The `auto` size floors the panel at `min-w-[min(100%,560px)]` so a small plan
graph does not collapse to a narrow column. tailwind-merge keeps that class
alongside the base ceiling `max-w-[calc(100%-2rem)]` — they are different
modifier groups — and below the `sm:` breakpoint min-width wins over
max-width. On a 390px viewport the Plan & tasks dialog therefore rendered its
rounded, ringed panel flush to both screen edges, while every fixed size
(sm/md/lg/xl) kept the 1rem gutter at that same width.

Floor the panel at the gutter the ceiling already reserves. The size test now
pins the new floor, that the old one is gone, and that the base ceiling
survives the merge — the three only hold together.
@qwen-code-ci-bot

qwen-code-ci-bot commented Sep 5, 2026

Copy link
Copy Markdown
Collaborator

Qwen Triage finished — CI landed green on e9dc485 and the deferred approval was posted. finalize run

Qwen Triage 已完成 —— e9dc485 的 CI 全绿,延迟审批已提交。查看 finalize 运行

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Thanks for the PR!

Template looks good ✓ — every required heading is filled in, plus the full Chinese translation.

Problem: real and observed, not theoretical hardening. I checked the mechanism against the base code instead of taking the description on faith: DialogContent is fixed and carries w-full max-w-[calc(100%-2rem)] sm:max-w-sm, auto adds w-max min-w-[min(100%,560px)], and cn() is twMerge(clsx(...))min-w-* and max-w-* are different tailwind-merge groups, so both survive the merge exactly as described. Because the panel is fixed-positioned, 100% resolves against the viewport, so at 390px the floor is 390px against a 358px ceiling and min-width wins the conflict. The fixed sizes escape this because they use w-full with no min-width, which matches the description. One honest caveat: the before/after here is analytical (computed widths) rather than a visual capture, and the author says so under Risk & Scope rather than dressing it up.

Direction: aligned. A dialog painting its rounded, ringed panel flush to both screen edges on a phone is a visible defect, and narrow-viewport web-shell support is already an established concern in this repo — there is a mobile composer spec and a 390×844 case in the visuals suite. The reference CHANGELOG carries the same class of fix repeatedly (e.g. a detail dialog "overflowing the terminal and losing its header off-screen"), so dialog sizing under a constrained width is not a side quest.

Size: not applicable — no core paths are touched (packages/web-shell/client/components/dialogs/** only). 7 production lines, 8 test lines.

Approach: the scope feels right and it is the minimal change. I did consider the other obvious route — sm:min-w-[min(100%,560px)], dropping the floor below the breakpoint instead of shrinking it — but that discards the reason the floor exists (keeping a small plan graph from collapsing into a narrow column) precisely where the screen is smallest. Mirroring the ceiling's own 2rem gutter keeps that intent and is a no-op above ~592px of viewport, where both forms resolve to the same 560px. Worth naming for whoever reviews next: DialogShell is shared, so this reaches every size="auto" dialog — today that is exactly one production consumer, the Plan & tasks dialog in App.tsx.

Risk: no elevated risk signals (no Stage 1e high-risk path match).

Moving on to code review. 🔍

中文说明

感谢贡献!

模板完整 ✓ —— 所有必填小节都写了,中文翻译也是逐段完整的。

问题: 是真实观测到的 bug,不是理论性加固。我没有直接采信描述,而是对着主干代码验证了机制:DialogContentfixed 定位,带 w-full max-w-[calc(100%-2rem)] sm:max-w-smauto 追加了 w-max min-w-[min(100%,560px)]cn() 就是 twMerge(clsx(...)) —— min-w-*max-w-* 属于 tailwind-merge 的不同分组,所以两个类确实都会保留下来,与描述一致。因为面板是 fixed 定位,100% 相对视口解析,于是 390px 视口下下限是 390px、上限是 358px,而 min-width 在冲突时胜出。固定尺寸之所以没有这个问题,是因为它们用 w-full 且没有 min-width —— 这一点也和描述吻合。需要如实说明的是:这里的 before/after 是推算出来的宽度,不是视觉截图,作者在 Risk & Scope 里也直说了,没有粉饰。

方向: 对齐。对话框把带圆角和描边的面板画到屏幕两边,在手机上是肉眼可见的缺陷;而窄视口下的 web-shell 支持在本仓库本来就是既有关注点 —— 已有 mobile composer 的 spec,visuals 套件里也有 390×844 的用例。参考 CHANGELOG 里同类修复反复出现(例如某个详情对话框"溢出终端、表头跑到屏幕外"),所以在受限宽度下把对话框尺寸做对,不是偏离主线的事。

规模: 不适用 —— 没有触及核心路径(只改了 packages/web-shell/client/components/dialogs/**)。生产代码 7 行,测试 8 行。

方案: 范围合理,而且是最小改动。我确实想过另一条明显路径 —— sm:min-w-[min(100%,560px)],即在断点以下直接去掉下限而不是缩小它 —— 但那会把下限存在的理由(避免小的 plan 图坍缩成窄条)在屏幕最小的地方丢掉。改为对齐上限自己预留的那 2rem 边距,既保住了这个意图,又在视口约 592px 以上完全无差异(两种写法都解析为同样的 560px)。给后续 reviewer 点明一下:DialogShell 是共享组件,改动会作用到每个 size="auto" 对话框 —— 目前生产代码里只有一个消费方,即 App.tsx 里的 Plan & tasks 对话框。

风险: 无升级风险信号(Stage 1e 高风险路径未命中)。

进入代码审查 🔍

Qwen Code · qwen3.8-max-2026-09-02

Reviewed at e9dc485af43c5c872e6280ebb139a53471b25d10 · re-run with @qwen-code /triage

@qwen-code-ci-bot

qwen-code-ci-bot commented Sep 5, 2026

Copy link
Copy Markdown
Collaborator

Code review

My independent proposal from the title and the "Why it's needed" section — before looking at the diff — was to make the floor subtract the same gutter the ceiling already reserves, i.e. exactly min-w-[min(calc(100%-2rem),560px)], with the one real alternative being to scope the floor to sm: and drop it below the breakpoint. The PR took the first route, which is the right one; the sm:-scoped variant would give up the floor's purpose (a small plan graph not collapsing into a narrow column) on the narrowest screens. So the approach matches what I'd have written.

No critical blockers and no AGENTS.md violations. Things I checked rather than assumed:

  • The arbitrary value isn't a new syntax bet. Tailwind is at 4.3 here and normalizes math functions, and the identical shape already ships in this very file (sm:max-w-[min(calc(100vw-2rem),1120px)]) and in dialog.tsx (max-w-[calc(100%-2rem)]). So calc(100%-2rem) inside min() is a pattern already proven in production CSS here.
  • The behavior delta is confined to narrow viewports, arithmetically. Both floors resolve to the same 560px once the containing block is ≥592px wide, so wide screens are unchanged — that substantiates the "wide viewport unchanged" claim in the description instead of taking it on faith. Below that, the old floor tracked the viewport width and outran the ceiling; the new one tracks the ceiling.
  • Fullscreen is not in the blast radius. The fullscreen ternary swaps the whole sizeClass[size] string out for its own width classes, so no min-width from auto reaches that path. Fixed sizes (sm/md/lg/xl) are untouched, and they never had the conflict because they use w-full with no floor.
  • Nothing left inconsistent elsewhere. min-w-[min(100% appears nowhere else in the repo, so there is no sibling component still carrying the old shape.
  • The test got stronger, not just updated. It now pins the new floor, the absence of the old one, and the survival of the base ceiling from dialog.tsx. That third assertion is the one doing real work: if someone later adds a max-w-* to the auto string, tailwind-merge would silently drop the base ceiling, and this test is what catches it.

One non-blocking observation: these assertions prove the class merge, not the layout. Nothing that runs today measures the panel's rendered width, so the pixel-level half of the claim — 390px flush-to-edge before, a 1rem gutter after — rests on CSS reasoning (which is sound, and which I re-derived above) rather than on an executed measurement. The visuals suite runs at 1280×800 and its single 390×844 case covers the right-panel drawer, not this dialog. A 390px capture case for the Plan & tasks dialog in screenshots.spec.ts, or a boundingBox() assertion in an e2e spec, would pin it permanently — the repo already has both patterns on hand. The author flags this gap themselves under "Not validated / out of scope", which is the honest way to ship it.

Test evidence — the PR's own CI

Fetched once from the check-runs API for the reviewed commit; no polling, and no PR-derived code was built or executed by this review. As of this fetch nothing has failed — zero red checks — but the checks that actually matter for this change are still running, so the suite result is not yet available and is not being claimed either way. Desktop Shell is green on both ubuntu and windows. The macOS and Windows unit-test legs and the CLI integration leg are skipped by the workflow's own path/condition filters, not by this PR. The Capture web-shell visuals job is running, but note it will not show this fix: it captures at 1280×800 and no spec opens an size="auto" dialog.

Final CI results for e9dc485 (auto-updated by the triage finalize job after CI completed):

Check Conclusion
Capture web-shell visuals (ubuntu-latest, Node 22.x) ✅ success
Classify PR ✅ success
Desktop Shell (ubuntu-22.04) ✅ success
Desktop Shell (windows-2022) ✅ success
Integration Tests (no-AK, No Sandbox) ✅ success
Lint & Static (ubuntu-latest, Node 22.x) ✅ success
Test (ubuntu-latest, Node 22.x) ✅ success
web-shell E2E Smoke (ubuntu-latest, Node 22.x) ✅ success

One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行(取最新一次运行),省略 skipped,失败项排在最前。

Not verified, and why: the rendered gutter at 390px — no lane in this PR's CI measures layout, and the unit test asserts class strings. The author's DialogShell.test.tsx 7/7 result is their own local claim, not something this review re-ran.

Sandboxed verification would settle this: @qwen-code /verify — an A/B harness that opens the Plan & tasks dialog at a 390px viewport on base and on head and measures the panel's bounding box (expect used width 390px → 358px, i.e. a 16px gutter appearing on both sides) is the only thing that turns the layout half of this claim from reasoning into measurement. The class-merge half is already pinned by the unit test. @qwen-code /tmux is the wrong lane here — it drives the terminal TUI, and this is a browser-rendered web-shell surface.

中文说明

代码审查

在看 diff 之前,我只根据标题和"为什么需要"这一节给出的独立方案,就是让下限减去上限本来就预留的那份边距,也就是 min-w-[min(calc(100%-2rem),560px)];唯一真正的替代方案是把下限收进 sm: 断点、在断点以下直接去掉。PR 选了前者,这也是正确的选择 —— 收进 sm: 的写法会在最窄的屏幕上放弃下限存在的意义(避免小的 plan 图坍缩成窄条)。所以方案与我自己的判断一致。

没有致命阻塞项,也没有违反 AGENTS.md。以下是我实际核对过、而不是想当然的点:

  • 这个任意值不是在赌语法。 本仓库 Tailwind 是 4.3,会对数学函数做归一化;而且完全相同的写法已经存在于同一个文件里(sm:max-w-[min(calc(100vw-2rem),1120px)])和 dialog.tsx 里(max-w-[calc(100%-2rem)])。所以 min() 里套 calc(100%-2rem) 是这里已经被生产 CSS 验证过的写法。
  • 行为差异在算术上被限定在窄视口。 一旦包含块宽度 ≥592px,新旧下限都解析为同样的 560px,因此宽屏完全不变 —— 这是把描述里"宽视口不变"的说法验算了一遍,而不是采信。低于该宽度时,旧下限跟随视口宽度并压过上限,新下限则跟随上限。
  • 全屏模式不在影响范围内。 fullscreen 的三元表达式会整体替换掉 sizeClass[size],所以 auto 的 min-width 到不了那条路径。固定尺寸(sm/md/lg/xl)没有被改动,而且它们本来就不冲突,因为用的是 w-full 且没有下限。
  • 别处没有遗留不一致。 整个仓库再没有第二处 min-w-[min(100%,所以不存在还带着旧写法的兄弟组件。
  • 测试是变强了,而不只是跟着改。 它现在同时钉住新下限、旧下限的消失、以及 dialog.tsx 里基础上限的存活。第三条才是真正起作用的那条:将来若有人往 auto 字符串里再加一个 max-w-*,tailwind-merge 会静默丢掉基础上限,而这个断言能拦住。

一个非阻塞的观察:这些断言证明的是类名合并,不是布局。目前没有任何在跑的东西测量面板的实际渲染宽度,所以"改前 390px 贴边、改后两侧各留 1rem"这半个论断依赖的是 CSS 推理(推理是成立的,上面我也重新推导过),而不是一次实际执行的测量。visuals 套件跑在 1280×800,其中唯一的 390×844 用例覆盖的是右侧面板抽屉,不是这个对话框。在 screenshots.spec.ts 里为 Plan & tasks 对话框加一个 390px 的截图用例,或在某个 e2e spec 里加一条 boundingBox() 断言,就能把它永久钉住 —— 这两种写法仓库里都已经有现成范例。作者自己在"未验证 / 不在范围"里点明了这个缺口,这是很诚实的交付方式。

测试证据 —— PR 自己的 CI

针对被审查的那个 commit,从 check-runs API 取了一次;没有轮询,本次审查也没有构建或执行任何 PR 代码。取数时没有任何检查失败(零红),但真正与本次改动相关的检查仍在运行,所以整套测试结果尚不可得,这里也不对结果做任何断言。Desktop Shell 在 ubuntu 和 windows 上都是绿的。macOS、Windows 的单测分支以及 CLI 集成分支是工作流自身的路径/条件过滤跳过的,与本 PR 无关。Capture web-shell visuals 正在跑,但要注意它不会体现本次修复:它在 1280×800 下截图,且没有任何 spec 会打开 size="auto" 的对话框。

未验证项及原因:390px 下实际渲染出的边距 —— 本 PR 的 CI 中没有任何环节测量布局,单测断言的是类名字符串。作者所说的 DialogShell.test.tsx 本地 7/7 通过是其本人的说法,本次审查没有重跑。

沙箱验证可以把这件事定下来:@qwen-code /verify —— 用一个 A/B 装置,分别在 base 和 head 上以 390px 视口打开 Plan & tasks 对话框并测量面板的 bounding box(预期实际宽度 390px → 358px,即两侧各出现 16px 边距),这是唯一能把布局这半个论断从推理变成测量的手段。类名合并那一半已经被单测钉住了。@qwen-code /tmux 在这里不是合适的通道 —— 它驱动的是终端 TUI,而本次改动是浏览器渲染的 web-shell 界面。

Qwen Code · qwen3.8-max-2026-09-02

Reviewed at e9dc485af43c5c872e6280ebb139a53471b25d10 · re-run with @qwen-code /triage

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Confidence: 4/5 — solid, mechanism verified independently, and the only open items are non-blocking nits; approval is deferred below because the suite is still running.

Stepping back: I did not take this PR's framing on trust. I went back to the base code and re-derived the defect myself — the fixed panel whose 100% therefore resolves against the viewport, the base ceiling living in a different file, tailwind-merge keeping min-w-* and max-w-* because they are separate groups, and min-width winning the conflict below the sm: breakpoint. It holds, and the fix is the same one I would have written before seeing the diff. That is the strongest signal a small PR like this can give: an independent proposal converging on the author's.

The blast radius is small and, more importantly, nameable. One live consumer today (the Plan & tasks dialog), fixed sizes untouched, fullscreen untouched because its ternary swaps the whole size string out, and a strict no-op above ~592px of viewport where the old and new floors both resolve to 560px. The test came out stronger than the one it replaced rather than merely adjusted — pinning the survival of the base ceiling is the assertion that will pay off later.

What I am not claiming: that anyone has watched the gutter appear. The rendered-width half of this rests on CSS reasoning plus the author's local test run, and I named the lane that would convert it into a measurement in the review above. For a one-class-string change whose mechanism is deterministic and re-derivable from the code, that is a follow-up rather than a blocker — but it is a real gap, and it is the reason this is a 4 and not a 5.

On volume: this author has around thirty open PRs and this one was deliberately split out of #10938, which is the right call and the reason it is reviewable in five minutes. I judged it on its own diff, not on the author's throughput — and the author is a committer with admin rights, so there is no fork-trust question here either.

Nits, both non-blocking: nothing pins the rendered layout, so a 390px capture case for this dialog in screenshots.spec.ts (or a boundingBox() assertion) would make the fix self-defending; and the explanatory comment is near-duplicated between the source and the test, where the test could simply point at the source.

Six months from now this reads fine — the comment in the source explains a cross-file constraint that would otherwise look like magic arithmetic. Verdict: approve.

CI is still running on this commit (2 pull-request workflow runs outstanding, no red checks so far), so I am not posting an approval in this run — approval is deferred until CI lands green on e9dc485af43c5c872e6280ebb139a53471b25d10, and it will be withheld if anything lands red or the head moves.

中文说明

Confidence: 4/5 —— 扎实,机制已独立验证,剩下的只是非阻塞的小问题;下面之所以延后批准,是因为测试套件还在跑。

退一步看整体:我没有采信这个 PR 的说法。我回到主干代码自己把缺陷重新推导了一遍 —— 面板是 fixed,所以它的 100% 相对视口解析;基础上限写在另一个文件里;tailwind-merge 因为 min-w-*max-w-* 属于不同分组而把两者都保留;在 sm: 断点以下 min-width 在冲突中胜出。结论成立,而且这个修法与我在看 diff 之前会写出的方案一致。对这种小 PR 来说,这就是它能给出的最强信号:独立方案与作者收敛到同一处。

影响范围小,更重要的是可以点名。今天只有一个消费方(Plan & tasks 对话框),固定尺寸没被动到,全屏也没被动到(它的三元表达式会整体换掉尺寸字符串),并且在视口约 592px 以上严格无差异 —— 那里新旧下限都解析为 560px。测试比它替换掉的那版更强,而不只是跟着改:钉住基础上限存活的那条断言,将来才会真正兑现价值。

没有主张的是:有人亲眼看到边距出现。渲染宽度这半个论断依赖 CSS 推理加上作者本地的测试运行,我在上面的审查里点名了能把它变成测量的通道。对一个只改一个类名字符串、机制确定且可从代码重新推导的改动来说,这属于后续跟进而不是阻塞项 —— 但它确实是缺口,也是这里给 4 分而不是 5 分的原因。

关于数量:这位作者有大约三十个开放 PR,本 PR 是特意从 #10938 拆出来的,这是正确的做法,也正是它能在五分钟内看完的原因。我是按它自己的 diff 评判的,没有按作者的产出量评判 —— 而且作者是有 admin 权限的 committer,这里也不存在 fork 信任问题。

小问题,均不阻塞:目前没有任何东西钉住渲染布局,所以在 screenshots.spec.ts 里为这个对话框加一个 390px 的截图用例(或一条 boundingBox() 断言)能让这个修复自带防护;另外源码和测试里的解释性注释近乎重复,测试那一处其实可以直接指向源码。

六个月后再读这段代码是没问题的 —— 源码里的注释解释了一个跨文件的约束,否则那串算术看起来会像魔法。结论:批准。

本 commit 的 CI 仍在运行(还有 2 个 pull-request 工作流未完成,目前无红色检查),所以本次不提交批准 —— 批准延后到 CI 在 e9dc485af43c5c872e6280ebb139a53471b25d10 上全绿时再给出;若有检查变红或 head 发生移动,则不予批准。

Qwen Code · qwen3.8-max-2026-09-02

Reviewed at e9dc485af43c5c872e6280ebb139a53471b25d10 · re-run with @qwen-code /triage

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

🖼️ web-shell visual preview

Rendered against a mock daemon (no real backend): the PR base vs this PR head e9dc485. Only screenshots that changed are shown (flows below, if any, are head-only) — refreshes on every push.

Screenshots · before / after

terminal-turn-error-copy-narrow-dark before/after

Full-resolution recordings (.webm) are attached to the workflow run.

Qwen Code · web-shell visuals

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Downgraded from Approve to Comment: CI still running. Partially reviewed — gaps disclosed.

Not explored to full depth (tool budget reached): "agent 6a": none — all planned checks completed within budget (no unfinished checks to disclose)..

Not reviewed: reverse audit — stopped before round 1 by the review time budget.

Test Plan (not a blocker): client/components/dialogs/DialogShell.test.tsxno such file or directory.

中文说明

⚠️ 已从批准降级为评论:CI still running。 仅完成部分审查,审查缺口已披露。

未探索到全部深度(达到工具调用预算):"agent 6a"none — all planned checks completed within budget (no unfinished checks to disclose).

未审查:反向审计——评审时间预算不足,未能开始第 1 轮。

Test Plan(非阻断):client/components/dialogs/DialogShell.test.tsxno such file or directory

— qwen3.8-max via Qwen Code /review (v0.23.0)

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, looks ready to ship — CI landed green after the review. ✅

@wenshao
wenshao added this pull request to the merge queue Sep 6, 2026
Merged via the queue into main with commit d247a0a Sep 6, 2026
102 of 106 checks passed

@wenshao wenshao left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partially reviewed — gaps disclosed. Suggestions are inline.

Not reviewed: build-and-test — the packages/cli suite timed out locally (exit 143) even on a full 540s deadline; CI's Test (ubuntu-latest, Node 22.x) leg is green at this commit and the diff touches only packages/web-shell, which ran green locally (265 files, 5995 tests).

Not explored to full depth (tool budget reached): "agent reverse-audit (round 2)": did not open client/styles/globals.css / preflight.css to check whether a transform , contain , or filter on the web-shell portal root makes the panel's….

Test Plan (not a blocker): client/components/dialogs/DialogShell.test.tsxno such file or directory.

中文说明

仅完成部分审查,审查缺口已披露。 建议见行内评论。

未审查(原文为英文):build-and-test — the packages/cli suite timed out locally (exit 143) even on a full 540s deadline; CI's Test (ubuntu-latest, Node 22.x) leg is green at this commit and the diff touches only packages/web-shell, which ran green locally (265 files, 5995 tests).

未探索到全部深度(达到工具调用预算):"agent reverse-audit (round 2)"did not open client/styles/globals.css / preflight.css to check whether a transform , contain , or filter on the web-shell portal root makes the panel's…

Test Plan(非阻断):client/components/dialogs/DialogShell.test.tsxno such file or directory

— qwen3.8-max via Qwen Code /review (v0.23.0)

// below `sm:` a bare `min(100%,560px)` floor outranks that ceiling, so the
// panel would render flush to both screen edges on a phone.
expect(panel.className).toContain('min-w-[min(calc(100%-2rem),560px)]');
expect(panel.className).not.toContain('min-w-[min(100%,560px)]');

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] This literal is a complete Tailwind candidate sitting in a file Tailwind scans, so the flush-to-edges floor this PR sets out to retire is still compiled into the published CSS even though no component uses it any more. There is no @source directive anywhere in client/styles/ and this file is not git-ignored, so Tailwind v4's automatic source detection lifts the candidate straight out of the assertion and emits a rule for it. README.md:51 states the compiled Tailwind output is inlined into the published npm package, so the retired expression ships in every consumer's injected CSS permanently, and a grep of the built artifact for the flush-to-edges floor still returns a live-looking scoped rule. A future author who copies min-w-[min(100%,560px)] from this test or from the built CSS reintroduces exactly the bug this PR fixes, and this assertion stays green because it only inspects one auto dialog's className.

It also buys no guard: it reddens on the same source revert that line 194 already catches, and the both-literals-present state it looks like it uniquely covers is unreachable through cn().

Witness:

Published lib artifact built from this commit (npx vite build --config vite.lib.config.ts),
__qwenWebShellCss extracted and parsed by Chromium 1.61.1 headless:
  {"ruleCount":4675,"oldFloor":true,"newFloor":true,"baseCeiling":true,"sm720":true}
  dead rule present and scoped:
  ...[data-web-shell-portal-root][data-web-shell-shadcn] *).min-w-\[min\(100\%\,560px\)\]{min-width:min(100%,560px)}

ARM INTACT (scratch tree == commit, built twice, identical):
  dist/index.js 6,977.53 kB   grep -c 'min-width:min(100%,560px)'        = 1
                              grep -c 'min-width:min(100% - 2rem,560px)' = 1
ARM FIX (only DialogShell.test.tsx:195 deleted):
  dist/index.js 6,977.26 kB   grep -c 'min-width:min(100%,560px)'        = 0   new floor still 1
scan-base canary min-w-[123.456px] parked in the probe dir: 0 hits in BOTH arms
  -> the probe dir is outside the scan base, so line 195 was the only variable

redundancy arms (one revert, caught by either line):
  source reverted, 195 present  -> FAIL at 194
  source reverted, 194 deleted  -> FAIL at 195
  195 deleted, source reverted  -> FAIL at 194
twMerge(OLD+' '+NEW) and twMerge(NEW+' '+OLD) each collapse to a single floor
  -> both-present is impossible
Suggested change
expect(panel.className).not.toContain('min-w-[min(100%,560px)]');

One fact the fix must not violate: line 194, expect(panel.className).toContain('min-w-[min(calc(100%-2rem),560px)]');, has to stay — removing line 195 leaves it as the only pin on the floor, and the probe confirms it still reddens on a source revert.

Acceptance criterion: client/build-artifact.test.ts already parses the inlined __qwenWebShellCss out of dist/index.js with postcss (readInjectedCss(), used by the existing scopes every component CSS rule to a WebShell root case), so an added case asserting no rule there declares min-width:min(100%,560px) is the guard that should replace this one — please confirm it goes red against today's artifact, and red again if line 195 is restored after the fix.

中文说明

这个字面量本身就是一个完整的 Tailwind 候选类,而它所在的文件会被 Tailwind 扫描,因此本 PR 想要退役的「贴边」宽度下限,仍然会被编译进发布的 CSS —— 尽管已经没有任何组件在用它。client/styles/ 里没有任何 @source 指令,这个文件也没有被 git-ignore,所以 Tailwind v4 的自动源探测会直接从这条断言里把候选类取走并为它生成规则。README.md:51 说明编译后的 Tailwind 产物会被内联进发布的 npm 包,于是这个已退役的表达式会永久地随每个使用方注入的 CSS 一起发布;在构建产物里 grep 这个贴边下限,仍然会返回一条看起来生效的、带作用域的规则。将来若有人从这个测试文件或构建产物里照抄 min-w-[min(100%,560px)],就会把本 PR 修掉的 bug 原样带回来 —— 而这条断言依然是绿的,因为它只检查一个 auto 对话框的 className。

它也没有换来任何防护:源码回退时它会变红,但同一次回退第 194 行本来就能拦住;而它看起来独有的那个「两个字面量同时出现」状态,经过 cn() 根本不可达。

( witness 与 suggestion 块见上方英文部分。)

修复不能违反的一个既有事实:第 194 行 expect(panel.className).toContain('min-w-[min(calc(100%-2rem),560px)]'); 必须保留 —— 删掉第 195 行之后,它是下限唯一的钉住点,探针已确认源码回退时它仍会变红。

验收标准:client/build-artifact.test.ts 已经用 postcss 从 dist/index.js 解析出内联的 __qwenWebShellCssreadInjectedCss(),现有的 scopes every component CSS rule to a WebShell root 用例就在用它),所以应当由一条新用例来替代这个防护 —— 断言其中没有任何规则声明 min-width:min(100%,560px)。请确认它在当前产物上是红的,并且在修复之后若把第 195 行加回去,它会再次变红。

— qwen3.8-max via Qwen Code /review (v0.23.0)

// panel would render flush to both screen edges on a phone.
expect(panel.className).toContain('min-w-[min(calc(100%-2rem),560px)]');
expect(panel.className).not.toContain('min-w-[min(100%,560px)]');
expect(panel.className).toContain('max-w-[calc(100%-2rem)]');

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] The cross-file invariant this fix rests on — the auto floor's gutter must equal the base ceiling's gutter declared at client/components/ui/dialog.tsx:85 — is pinned only by two independent hardcoded class literals, so the guard can be satisfied by editing the test string and the relationship itself stays unpinned. If someone retunes the shared dialog gutter in dialog.tsx:85 from max-w-[calc(100%-2rem)] to max-w-[calc(100%-3rem)], this assertion goes red in a file the change never touched, and its message names a class literal rather than a relationship. The cheapest way back to green is to bump that literal, and doing so ships a floor of 100%-2rem against a ceiling of 100%-3rem: min-width beats max-width, so at a 390px viewport the auto panel uses 358px against a 342px ceiling and renders 16px from each edge while every fixed size keeps 24px — the exact defect this PR removes, now behind a green suite, with the comment at DialogShell.tsx:46-50 still quoting a stale calc(100%-2rem). The under-guttered window is viewport < 608px (the floor caps at 560px from 592px, but a 3rem ceiling only reaches 560px at 608px).

Witness:

Five-arm probe in an isolated scratch tree, real vitest runs:

ARM A  pristine PR                                   -> 7 tests, Test Files 1 passed
ARM D  mutant: dialog.tsx:85 ceiling 2rem -> 3rem    -> FAIL sizes the auto dialog...
       expected 'fixed top-1/2 left-1/2 z-[var(--web-s...' to contain 'max-w-[calc(100%-2rem)]'
       (1 failed | 6 passed)
ARM B  = ARM D + only the test's ceiling literal bumped
                                                       -> 7 tests, 1 file passed  <-- the hole
ARM C  = ARM B + the suggested relationship assertion  -> FAIL: expected '2' to be '3'
       rendered className: ... max-w-[calc(100%-3rem)] ... w-max
       min-w-[min(calc(100%-2rem),560px)] sm:max-w-[min(calc(100vw-2rem),1120px)]
       -> a 2rem floor and a 3rem ceiling survive twMerge simultaneously
ARM E  pristine PR + relationship assertion only      -> 7 tests passed
Suggested change
expect(panel.className).toContain('max-w-[calc(100%-2rem)]');
expect(panel.className).toContain('max-w-[calc(100%-2rem)]');
const floorGutter = /min-w-\[min\(calc\(100%-(\d+(?:\.\d+)?)rem\)/.exec(
panel.className,
)?.[1];
const ceilingGutter =
/(?:^|\s)max-w-\[calc\(100%-(\d+(?:\.\d+)?)rem\)\]/.exec(
panel.className,
)?.[1];
expect(floorGutter).toBeDefined();
expect(floorGutter).toBe(ceilingGutter);

One fact the fix must not violate: neither class name may be composed from a shared constant or interpolation. Both sites ship complete literals that Tailwind extracts by scanning source text — dialog.tsx:85 and DialogShell.tsx:51 — so a max-w-[calc(100%-${gutter})] template would emit no CSS at all and silently drop the ceiling.

Acceptance criterion: the same sizes the auto dialog to its content instead of a fixed step case is what must go red when the guard is removed — change dialog.tsx:85 to max-w-[calc(100%-3rem)] and bump only the existing literal assertion, which is green today (probe ARM B) and red with the relationship assertion in place (probe ARM C, expected '2' to be '3').

Worth saying plainly: this one is a judgment call a maintainer may decline. The wrong path is not silent — it requires greening a red test by editing a literal whose four-line comment directly above spells out the floor/ceiling relationship — and AGENTS.md's Simplicity First rule is a defensible reason not to add a regex self-consistency assertion over Tailwind arbitrary-value strings.

中文说明

这个修复所依赖的跨文件不变量 —— auto 下限的边距必须等于 client/components/ui/dialog.tsx:85 里声明的基础上限的边距 —— 只由两个各自硬编码的类名字面量钉住,因此只要改一下测试里的字符串就能让防护通过,而两者之间的关系本身始终没有被钉住。如果有人把 dialog.tsx:85 的共享对话框边距从 max-w-[calc(100%-2rem)] 调成 max-w-[calc(100%-3rem)],这条断言会在一个该改动从未触碰的文件里变红,而它的报错信息指向的是一个类名字面量,不是一层关系。最省事的恢复绿灯方式就是把那个字面量跟着改一下,而这样交付出去的代码是:下限 100%-2rem、上限 100%-3rem。由于 min-width 优先于 max-width,在 390px 视口下 auto 面板的实际宽度是 358px、上限是 342px,两侧各留 16px,而所有固定尺寸都留 24px —— 正是本 PR 要消除的那个缺陷,如今藏在一片绿色的测试套件后面,而 DialogShell.tsx:46-50 的注释还在引用过期的 calc(100%-2rem)。边距不足的区间是视口 < 608px(下限从 592px 起就固定在 560px,而 3rem 的上限要到 608px 才达到 560px)。

(witness 与 suggestion 块见上方英文部分。)

修复不能违反的一个既有事实:两个类名都不能由共享常量或字符串插值拼出来。两处都是以完整字面量发布的,Tailwind 靠扫描源码文本提取它们 —— dialog.tsx:85DialogShell.tsx:51 —— 所以 max-w-[calc(100%-${gutter})] 这样的模板根本不会生成任何 CSS,会静默地丢掉上限。

验收标准:去掉这个防护时应当变红的,就是同一个 sizes the auto dialog to its content instead of a fixed step 用例 —— 把 dialog.tsx:85 改成 max-w-[calc(100%-3rem)] 并只更新现有的那条字面量断言:今天是绿的(探针 ARM B),加上关系断言后是红的(探针 ARM C,expected '2' to be '3')。

坦白说一句:这一条属于维护者完全可以不接受的取舍。走错的那条路并非无声无息 —— 它要求开发者通过修改一个字面量来把红灯变绿,而那个字面量正上方就有四行注释把下限/上限的关系讲清楚了 —— 而且按 AGENTS.md 的 Simplicity First 原则,不为 Tailwind 任意值字符串增加一条正则自洽断言,也是一个站得住脚的选择。

— qwen3.8-max via Qwen Code /review (v0.23.0)

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Released in v0.23.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants