fix(desktop): hug the right edge and ease the workbar column open and shut - #4890
Conversation
0d21671 to
1e6ac50
Compare
… shut The right workbar's collapse toggle sat 24px in from the plate's edge on macOS, where the platform draws nothing on the right: the titlebar strip and the workbar's own bar each spelled out `--space-6 + overlay-right-width`. Fold that into one `--maka-titlebar-gutter-right` token with the bar's own `--space-2` inset, so the toggle keeps the same x in both bands and only Windows caption buttons push it inward. The column also snapped between `display: none` and its width, unlike the sidebar. On a wide window the frame and the overlay panel that share its grid area now animate width and margin, with `visibility` gated by the same duration. Their children hold the open width and hang off the right edge (`justify-content: end` on the lone grid track; an auto margin would resolve to 0 against negative free space), so the face and the toggle keep their resting x and the box's left edge sweeps over them, the way the sidebar reveals its own. Below 991px the column stacks under the conversation at full width, so there collapsing still removes the row, as it does for the bottom placement at every width. A modal obscuring the shell sets `hidden` on the frame instead of collapsing it, so a dialog does not play the ease. The panels stay mounted while hidden; a CDP trace of the 280ms ease costs ~95ms of main thread over 33 frames with no drops at 120Hz. `min-width`/`max-width` on the column go: `workbar-layout.ts` already clamps the value, and a floor would stop the collapse short of 0. Dead rules go with them: the narrow block's `max-width: none`, the panel's `[hidden]` override that `base.css` already covers with `!important`, and the side-chat panel's flex column, which the panel's own grid track now stretches. The shell story that holds the toggle to one x carries the motion contract too, since it is the one story that mounts both bands: it asserts the 8px gutter, then samples frames around collapse and restore for a running width transition, the panel tracking the frame, and the face and toggle never moving. It writes the simulated caption width on the document root, where the gutter token reads it, and passes `motionEnabled` so the shell frame does not pin the fixture attribute that turns transitions off. A narrow-viewport story pins the stacked layout. Generated-by: Claude Code
1e6ac50 to
8556eed
Compare
jackwener
left a comment
There was a problem hiding this comment.
Reviewed at exact head 1e6ac506. No P0/P1; one [P2] about how this lands rather than what it does. The token work and the geometry claims check out; the motion guard is real but its scope is narrower than the summary reads.
Deliberately not approving yet, and the reason is the finding itself. This PR is CONFLICTING, and under this repository's rule an approval stays valid across later pushes — so approving now would carry across exactly the rebase that could silently drop the guard described below. I would rather re-check the resolved bytes.
[P2] The conflict is in the file that holds this PR's own guard
The conflicts against live main are apps/desktop/e2e-budget.json and apps/desktop/stories/app-shell.stories.tsx. #4895 has already changed WorkbarInShell to read through isSessionWorkbarCollapsed and bumped the budget to 6; this PR edits the same host and the same WorkbarCollapseKeepsOneToggleInPlace play.
So the two plausible resolutions each break something: keeping #4895's layout helper and dropping the rAF block leaves this change with no motion guard at all, while keeping this PR's version of the host re-breaks the typecheck #4895 fixed. Neither failure announces itself — one leaves a green tier with nothing pinning the ease, the other breaks main the same way it broke today.
Worth stating plainly since three PRs now touch the same two files: the budget is not the hazard. This head is already 16 specs / 31 tests with session-workbar.spec.ts at 6 — it does not carry the stale 5, and neither does #4829. Against live main the only budget delta here is the wording of the electron sentence. The hazard is the story host.
The motion guard discriminates, with one scope caveat
Reasoned against the production CSS rather than executed — the reviewing worktree had no node_modules, so Storybook was not run, and that limits this to a reading:
- Dropping
justify-content: endon the lone right column would let the band toggle travel with the box's left edge, so the per-frameSet(toggleX)would stop being a singleton. That is a genuine guard for the ease this PR adds. - Removing the
widthtransition would failframe.getAnimations()ontransitionProperty === 'width'.WorkbarInShellpassesmotionEnabled, so the fixture does not pintransition: none, and the play refuses to run underprefers-reduced-motion. - The 8px rest assertion is
frame.right - toggle.right === 8, and CI smoke runs at 1280 while the ease is scoped to@media (min-width: 991px)— so smoke is on the wide path, not the stacked one.
The caveat is worth knowing before trusting the headline. "The toggle keeps the same x" reads as one control across both bands, but the per-frame sample follows the workbar-band toggle; the titlebar restore is a second WorkbarToggle, mounted by WorkbarTitlebarActions only once collapsed. The two-band landing is checked at rest (restore.x ≈ parked.x after the hidden waitFor), not frame by frame. That is a reasonable split — a control that does not exist yet cannot be sampled — but the frame-level guarantee covers one band, not the handoff between them.
On the timing the description flags: sampling is a 500 ms wall-clock rAF loop started before the click rather than a fixed frame count, with rest awaited separately. A slow runner can false-red the eased assertion; it should not false-green the toggle-x assertion, which is the right direction for a flake to point.
Verified in the token and geometry changes
- The new token is clean.
--maka-titlebar-gutter-rightis defined once (maka-tokens.css:888) and read in exactly two places — the titlebar strip (shell-layout.css:274) and the workbar's own bar (workbar/shell.css:218).--maka-titlebar-overlay-right-widthsurvives only as its input, so there is no second spelled-out copy of the old expression left to drift. - "16px on every platform" is arithmetic, not an estimate. The new definition is
--space-2 + overlay-right-widthagainst the old--space-6 + …. - Removing
min-width: 320pxis safer than the summary claims.workbar-layout.tssetsSESSION_WORKBAR_MIN_WIDTH = 340andMAX = 600, applied throughclampSizeonrightWidth. The CSS floor was looser than the JS clamp, so it never bound a drag — its only live effect was stopping the collapse short of 0, which is exactly why it had to go.
简体中文
在 exact head 1e6ac506 上评审。无 P0/P1;一条 [P2],讲的是它怎么落地而不是它做了什么。 token 与几何的主张都核过成立;动效守卫是真的,但它的覆盖范围比摘要读起来要窄。
我刻意暂不批准,而理由本身就是发现。 这个 PR 处于 CONFLICTING,而按本仓库规则批准会跨后续 push 继续有效 —— 那就意味着现在批准会一路带过恰恰可能悄悄丢掉下述守卫的那次 rebase。我宁愿在冲突解决后重新核字节。
[P2] 冲突正落在承载本 PR 自身守卫的那个文件里
与实时 main 的冲突是 apps/desktop/e2e-budget.json 与 apps/desktop/stories/app-shell.stories.tsx。#4895 已经把 WorkbarInShell 改为经 isSessionWorkbarCollapsed 读取并把预算提到 6;本 PR 修改的是同一个宿主、同一条 WorkbarCollapseKeepsOneToggleInPlace play。
于是两种看似合理的解法各自会坏一样东西:保留 #4895 的布局取值而丢掉 rAF 块,本次改动就完全没有动效守卫;反过来保留本 PR 版本的宿主,又会把 #4895 修好的 typecheck 重新弄坏。两种失败都不会自己喊出来 —— 一种留下一层绿的测试而没有任何东西钉住这个缓动,另一种以今天同样的方式再次弄挂 main。
既然如今有三个 PR 动同样两个文件,有一句值得直说:预算不是危险所在。 本 head 已经是 16 specs / 31 tests、session-workbar.spec.ts 为 6,没有带着旧的 5,#4829 也没有。相对实时 main,这里唯一的预算差异只是 electron 那句话的措辞。危险在 story 宿主。
动效守卫具备区分力,但有一处范围上的提醒
以下是对照生产 CSS 推演的,不是跑出来的 —— 评审用的工作区没有 node_modules,Storybook 未运行,所以这一节只是阅读所得:
- 去掉右侧单列上的
justify-content: end,band 上的 toggle 就会随盒子左缘一起移动,于是逐帧的Set(toggleX)不再是单元素集合。这确实是本 PR 所加缓动的真守卫。 - 去掉
width过渡会让frame.getAnimations()在transitionProperty === 'width'上失败。WorkbarInShell传了motionEnabled,所以 fixture 不会把transition: none钉死,而且该 play 在prefers-reduced-motion下拒绝运行。 - 静止态的 8px 断言是
frame.right - toggle.right === 8;CI smoke 跑在 1280,而缓动限定在@media (min-width: 991px)—— 所以 smoke 走的是宽屏路径,不是堆叠路径。
这处提醒值得在采信标题之前知道。「toggle 保持同一个 x」读起来像是一个控件跨两条 band,但逐帧采样跟踪的是 workbar band 上那颗;标题栏上的恢复按钮是另一个 WorkbarToggle 实例,由 WorkbarTitlebarActions 在折叠之后才挂载。两条 band 的落位是在静止态核对的(waitFor 到隐藏之后比较 restore.x ≈ parked.x),不是逐帧。这个拆分本身合理 —— 还不存在的控件无法采样 —— 但帧级别的保证覆盖的是一条 band,不是两条之间的交接。
关于描述里点到的时序:采样是点击之前启动的 500 ms 墙钟 rAF 循环,而不是固定帧数,静止态另行等待。慢机器可能让 eased 断言误红,但不应让 toggle-x 断言误绿 —— 这是抖动该有的指向。
token 与几何改动中已核实的部分
- 新 token 是干净的。
--maka-titlebar-gutter-right定义一次(maka-tokens.css:888),恰有两个读者 —— 标题栏条(shell-layout.css:274)与 workbar 自己的条(workbar/shell.css:218)。--maka-titlebar-overlay-right-width只作为它的输入存活,因此不存在第二处拼写出来的旧表达式会漂移。 - 「每平台 16px」是算术,不是估计:新定义为
--space-2 + overlay-right-width,旧写法为--space-6 + …。 - 删掉
min-width: 320px比摘要所称更安全。workbar-layout.ts中SESSION_WORKBAR_MIN_WIDTH = 340、MAX = 600,并经clampSize作用于rightWidth。那条 CSS 下限比 JS 钳制更松,所以它从未约束过任何一次拖拽 —— 它唯一的实际作用就是让折叠停在 0 之前,而这正是它必须被移除的原因。
Automated review notice: This comment was posted by an automated review agent operated by jackwener. It is not an independent human review and does not replace one.
|
Two corrections to my previous comment, and the re-check I said I would do. The head moved while I was posting. That comment reads "exact head The [P2] is discharged — the rebase resolved it correctly. I said I would rather re-check the resolved bytes than approve across the rebase, so here is that check on
Both of the failures I warned about were avoided — the resolution kept this PR's guard and the helper That removes the reason I withheld approval. I am holding it only until required 简体中文对上一条评论的两点更正,以及我说过要做的复核。 发布过程中 head 移动了。 那条评论写的是 exact head 那条 [P2] 已经消解 —— rebase 解得是对的。 我说过宁愿复核解决后的字节、也不愿让批准跨过那次 rebase,所以这是在
我警告的两种失败都没有发生 —— 这次解决同时留住了本 PR 的守卫和 这也消除了我此前不批准的理由。我现在只是在等本 head 上必需的
|
jackwener
left a comment
There was a problem hiding this comment.
Approving at exact head 8556eed3. Required test is green on this head, the rebase is in, and the [P2] I raised is discharged by that rebase rather than argued away.
What the rebase had to get right, and did. The conflict sat in the file carrying this PR's own guard, so either resolution could have quietly cost something. On these bytes:
- the motion guard is intact — the rAF sampling,
getAnimations()and thetoggleXset are all still inWorkbarCollapseKeepsOneToggleInPlace; - #4895's typecheck fix survived alongside it —
WorkbarInShellstill reads throughisSessionWorkbarCollapsed; - the budget is 31 tests in 16 files.
Both halves kept, neither traded for the other.
The findings from the earlier review stand unchanged: the new --maka-titlebar-gutter-right token is defined once and read in exactly two places with no second copy of the old expression left to drift; the 16px shrink is --space-2 against --space-6, arithmetic rather than estimate; and removing min-width: 320px is safer than the summary claims, since workbar-layout.ts clamps 340–600 through clampSize, so the CSS floor never bound a drag and only stopped the collapse short of 0.
Two things to carry forward rather than block on:
- The frame-level motion guarantee covers one band. The per-frame toggle sample follows the workbar-band control; the titlebar restore is a second
WorkbarTogglethat only mounts once collapsed, so the two-band landing is checked at rest. That is a reasonable split, but the headline reads broader than the guard. - The story's distinguishability was reasoned from the production CSS, not executed — the reviewing worktree had no
node_modules, so Storybook was not run.smoke:storybookpassing in CI is the evidence that it runs; what was checked by reading is that it would redden ifjustify-content: endor thewidthtransition went away.
简体中文
在 exact head 8556eed3 上批准。本 head 的必需 test 已绿,rebase 已完成,而我提的那条 [P2] 是被这次 rebase 真正解决的,不是被说服掉的。
这次 rebase 必须做对的事,它做对了。 冲突正落在承载本 PR 自身守卫的那个文件里,所以两种解法都可能悄悄损失一样东西。在当前字节上:
- 动效守卫完好 —— rAF 采样、
getAnimations()与toggleX集合仍都在WorkbarCollapseKeepsOneToggleInPlace中; - #4895 的 typecheck 修复与它并存 ——
WorkbarInShell仍经isSessionWorkbarCollapsed读取; - 预算为 31 tests / 16 files。
两边都保住了,没有用一方换另一方。
先前评审的结论原样成立:新的 --maka-titlebar-gutter-right 定义一次、恰有两个读者,没有留下第二份会漂移的旧表达式;16px 的缩减是 --space-2 对 --space-6,是算术而非估计;删掉 min-width: 320px 比摘要所称更安全 —— workbar-layout.ts 经 clampSize 钳制在 340–600,所以那条 CSS 下限从未约束过任何一次拖拽,它唯一的作用就是让折叠停在 0 之前。
两点带走、但不阻塞:
- 帧级的动效保证覆盖的是一条 band。 逐帧采样跟的是 workbar band 上那颗控件;标题栏的恢复按钮是折叠之后才挂载的第二个
WorkbarToggle,所以两条 band 的落位是在静止态核对的。这个拆分合理,但标题读起来比守卫实际覆盖的范围更宽。 - 该 story 的区分力是对照生产 CSS 推演的,不是跑出来的 —— 评审工作区没有
node_modules,Storybook 未运行。CI 里smoke:storybook通过才是「它确实会跑」的证据;通过阅读确认的是:一旦justify-content: end或width过渡消失,它会变红。
Automated review notice: This comment was posted by an automated review agent operated by jackwener. It is not an independent human review and does not replace one.
Summary
Two things the right workbar got wrong next to the sidebar it mirrors.
The collapse toggle sat 24px in from the plate's right edge on macOS, where the OS draws nothing on that side. The titlebar strip and the workbar's own bar each spelled the right gutter out as
--space-6 + --maka-titlebar-overlay-right-width. That is now one token,--maka-titlebar-gutter-right, built from the bar's own--space-2inset plus whatever the platform reports: 0 on macOS and Linux, the caption-button width on Windows. Both bands read it, so the toggle keeps the same x whether the column is open or collapsed, and only Windows caption buttons push it inward.The column snapped between
display: noneand its width. The sidebar eases with--duration-large/--ease-out-strong; the workbar had nothing. The frame and the overlay panel that shares its grid area now animatewidthandmargin-left, withvisibilitygated by the same duration so a hidden column takes no focus or hit-testing. Their children hold the open width and hang off the right edge (justify-content: endon the lone grid track; an auto margin resolves to 0 against negative free space), so the face and the toggle keep their resting x and the box's left edge sweeps over them, the way the sidebar reveals its own content. Left-anchored, the whole face rode in from the toggle. A pointer drag on the resize handle disables the ease the way the sidebar does. The ease is scoped to@media (min-width: 991px): below that the column stacks under the conversation at full width, so collapsing still removes the row, and the bottom placement keepsdisplay: noneat every width. A modal obscuring the shell now setshiddenon the frame instead of collapsing it, so opening a dialog does not play the ease.Also removed: the
min-width: 320px/max-width: 600pxon the column.workbar-layout.tsalready clamps the value to 340–600, and a CSS floor would stop the collapse short of 0.Refs #2188. That tracker's "8px / 8px toolbar padding" box is ticked, but #4789's description says it left that open; this PR is what lands it.
Verification
product-shell-official-appshell--workbar-collapse-keeps-one-toggle-in-place(from test(desktop): finish moving non-Electron coverage out of the E2E tier #4877) now carries the motion contract too, since it is the one story that mounts both bands: it asserts the 8px gutter before simulating a caption width, then samplesrequestAnimationFrameframes around collapse and restore. Each pass has to see a runningwidthtransition on the frame, the overlay panel's width equal to the frame's on every frame, and the face's right edge and the toggle's x unmoved throughout. The resting state is awaited separately so a slow runner cannot cut the ease short (the first CI run failed exactly there).WorkbarInShellpassesmotionEnabled, becauseShellFrameotherwise pins the fixture attribute that turns every transition off. The caption-width override goes on the document root, where the gutter token reads it, and is cleared in afinally.product-session-workbar--collapse-narrow-stackruns at the smoke runner's 720px viewport (and the stacked viewport in the Storybook UI) and asserts the stacked column keeps its face at full width and that collapsing removes both the frame and the panel.npm run smoke:storybookpasses both. The same run reports 4 unrelated failures (session-list project groups, plus-menu during skill refresh, two slash-menu stories) that fail identically against a Storybook built frommain.Section'sdisplayand the children'swidthare overridden from thecomponentslayer, whichcascade-layers.cssreserves for product CSS; reduced motion is honoured by the globaltransition: noneinstyles/base.css, so the story asserts it is not running underprefers-reduced-motion. The reviews' findings (narrow-window stacking, bottom overlay panels, modal hide, dead rules, a redundant second motion story and its fake titlebar host, a monotonicity check that added no information) are folded in.e2e/session-workbar.spec.tspasses (6/6) in Electron.npm run format,npm run lint,npm run build:renderer,npm run build-storybookall clean.main; left alone in this PR.Real Electron window from the
gitReviewWindowE2E fixture, 1240×820 at DPR 2, cropped to the right 1000×420 CSS px so the toolbar reads. Renderer built frommainat a03dd57 on the left, from this branch on the right. The fixture pins transitions tonone; the capture drops that attribute before clicking collapse.AI use
Select exactly one:
Tool(s) and scope: Claude Code (Fable 5.1) diagnosed the gap and the missing motion over CDP, wrote the CSS, the
WorkbarPanelchange, the story and the E2E adjustment, and ran the measurements; reviewed and directed by the author.Checklist
Does this PR entail a change in behavior?