Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
wenshao merged 1 commit into from
Sep 6, 2026
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion packages/web-shell/client/components/dialogs/DialogShell.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,13 @@ describe('DialogShell', () => {
// stays full-width and never tracks the content.
expect(panel.className).toContain('w-max');
expect(panel.className).not.toContain('w-full');
expect(panel.className).toContain('min-w-[min(100%,560px)]');
// The floor has to subtract the same gutter the surviving base ceiling
// (`max-w-[calc(100%-2rem)]`) reserves. twMerge keeps both classes, and
// 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)]');

@wenshao wenshao Sep 6, 2026

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 解析出内联的 __qwenWebShellCss（readInjectedCss()，现有的 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)

expect(panel.className).toContain('max-w-[calc(100%-2rem)]');

@wenshao wenshao Sep 6, 2026

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:85 与 DialogShell.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)

expect(panel.className).toContain(
'sm:max-w-[min(calc(100vw-2rem),1120px)]',
);
Expand Down
7 changes: 6 additions & 1 deletion packages/web-shell/client/components/dialogs/DialogShell.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,12 @@ const sizeClass: Record<DialogSize, string> = {
// room. `w-max` wins over DialogContent's base `w-full` through
// tailwind-merge. The floor keeps small graphs from collapsing to a narrow
// panel; the ceiling keeps large ones from spanning a wide monitor.
auto: 'w-max min-w-[min(100%,560px)] sm:max-w-[min(calc(100vw-2rem),1120px)]',
// The floor uses the same 2rem gutter the base ceiling
// (`max-w-[calc(100%-2rem)]`) reserves: twMerge keeps both classes, and
// below `sm:` a bare `min(100%,560px)` floor outranks that ceiling, so the
// panel rendered flush to both screen edges on a phone while every fixed
// size kept its gutter.
auto: 'w-max min-w-[min(calc(100%-2rem),560px)] sm:max-w-[min(calc(100vw-2rem),1120px)]',
};

const FOCUSABLE_SELECTOR = [
Expand Down
Loading