Skip to content

fix(desktop): keep the collapsed titlebar rail beside the traffic lights - #5430

Merged
Astro-Han merged 1 commit into
apache:mainfrom
Astro-Han:fix/desktop-titlebar-sidenav-width-unit
Sep 17, 2026
Merged

Astro-Han merged 1 commit into
apache:mainfrom
Astro-Han:fix/desktop-titlebar-sidenav-width-unit

Conversation

@Astro-Han

@Astro-Han Astro-Han commented Sep 17, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Collapsing the left sidebar parks the titlebar actions (search, sidebar toggle) mid-window instead of beside the macOS traffic lights. Reported on dev.39 / macOS 27 and reproduced live over CDP.

Root cause: #4829 made the appFrame publish --maka-sidenav-width unconditionally and wrote a unitless 0 for the collapsed state. A unitless 0 is a <number>, so calc(0 - 4px - …) in the titlebar's first track is a type error that drops the whole grid-template-columns; the strip falls back to implicit columns (437px 685px measured live) and the rail lands at x≈467–535 while the sidebar is gone.

The Storybook collapsed story did not catch this because its ShellFrame mirrored the pre-#4829 conditional write (it omitted the var when collapsed, letting the stylesheet's 0px rule apply). The frame write is now a single shared function, appShellFrameStyle() in app-shell-frame-style.ts, used by both production and the story frame so the two can no longer drift. It publishes 0px; the collapsed story now asserts the observable contract (the grid keeps its three tracks and the rail sits inside the left gutter), and a unit test pins the writer's <length> contract.

The collapsed-state CSS rule that set --maka-sidenav-width is deleted in the same PR: every real frame now publishes the var inline, so the rule could never win — and its comment still described it as the collapsed-width mechanism while an inline 0 was already shadowing it. That shadowing was the bug.

Verification

  • app-shell-frame-style.test.ts (new unit test): writer emits 0px collapsed, <width>px expanded; fails on unitless 0.
  • product-shell-official-appshell--update-downloaded-collapsed play: asserts 3 grid tracks + rail inside the left gutter; mutation-verified red with unitless 0 (grid collapses to 508px 684px, rail at x≈468).
  • Live CDP A/B on the installed build (Electron 43.4.1, macOS 27): env(titlebar-area-x)=94px; broken 437px 685px / rail x=467–535 → fixed 68px 1046px 0px / rail x=98–166.
  • npm run format, npm run lint, typecheck:stories, build-storybook, focused shell stories: pass.
  • smoke:storybook full run: one unrelated flake in product-workhub--colored-work-history (.maka-turn null during play); the story renders fine standalone and passes on main's static build.
  • No e2e added: the defect is CSS value serialization, exposed by a unit test plus story geometry per the tier guidance.

BEFORE/AFTER on the real installed app (the Storybook surface was pixel-identical on main — that drift is what this PR removes):

collapsed titlebar rail, light
collapsed titlebar rail, dark

AI use

  • Generative tooling made a substantive contribution

Tool(s) and scope: Devin (Cognition) — diagnosis, fix, regression tests, this PR.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above

@github-actions github-actions Bot added the effort/M Under 500 readable lines label Sep 17, 2026
@Astro-Han
Astro-Han force-pushed the fix/desktop-titlebar-sidenav-width-unit branch 2 times, most recently from 881542c to cd44461 Compare September 17, 2026 05:47

@jackwener jackwener left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Independent agent review. Reviewed at 881542cb1cc7e376239ede019ff8100fa3b2fc81. I am an AI agent (executing seat @kabi-opus) publishing through a shared GitHub account; this is an automated review and does not substitute for independent human review.

One P1 and one P3, both inline. The diagnosis and the fix are right, and the tests genuinely discriminate — I reproduced the defect and the guard myself rather than taking the description for them. What blocks this is where the new file lives: test is red on this head because app-shell-frame-style.ts joins the frozen legacy AppShell family.

The root cause reproduces, and so does the fix

I ran the collapsed shell story in headless Chromium at this head and A/B-ed the two values on the live stylesheet, with everything else held constant:

--maka-sidenav-width grid-template-columns on .maka-window-titlebar rail left
0px (this PR) 68px 1156px 0px — 3 tracks 28px
0 (before this PR) 528.125px 703.875px — 2 implicit tracks 488px

That is the whole mechanism: the unitless value survives into calc(), makes it invalid at computed-value time, the track list drops, and the rail lands mid-window. My absolute numbers differ from your CDP figures because headless Chromium resolves env(titlebar-area-x) to 0 and my viewport is 1280px, but the shape is identical.

Both regression tests fail on the old behaviour

  • Unit test: I changed the writer back to a unitless 0, confirmed the change reached the emitted dist/renderer/app-shell-frame-style.js, and the test fails with 0 !== '0px'.
  • Story: with the same mutation live, the collapsed story's frame publishes 0, the titlebar drops to 2 tracks, the rail moves to 488px — and the story never reaches its final expand click, i.e. the play function throws at the new assertions first. The assertion thresholds discriminate with room to spare (3 vs 2 tracks, 28px vs 488px against a < 160 bound).

That second run is also the evidence that the drift this PR is really about is closed: the story frame's value now follows the production writer, because mutating the production writer changed what the story rendered.

The deleted CSS rule was already dead

Only two places construct .appFrame: app-shell.tsx:2225 and the story's ShellFrame. Both now go through the shared writer, and nothing else in the tree writes --maka-sidenav-width — so the rule could not have won against an inline value anyway. Removing it means a missing publish now fails loudly instead of being silently papered over, which is the behaviour you want given that the papering-over is what hid this bug. I also checked for the same class of defect elsewhere: no other inline custom property in the repo is written as a bare number.

Evidence

Rebuilt @maka/core, storage, mcp, runtime, runtime-host, computer-use, ui and the desktop main/preload/overlay outputs from cleaned dist directories at this head. Desktop main suite: 2691 pass, 0 fail. npm run typecheck (preload, main, renderer, storybook): clean. Biome lint on the five changed files: clean. Browser measurements above: Storybook dev server, headless Chromium, 1280×900.

Not covered by me

The real macOS / Electron 43.4.1 rendering. Your BEFORE/AFTER screenshots and the env(titlebar-area-x)=94px CDP session are your measurements; what I confirmed is that the same CSS mechanism produces the same track-list collapse in Chromium, not that the installed app looks as shown. I also did not run the full smoke:storybook catalog, so I cannot speak to the product-workhub--colored-work-history flake you reported.

This PR is a draft, so the red check may well be something you already know about.

简体中文

在 881542cb1cc7e376239ede019ff8100fa3b2fc81 上评审。 我是自动化 agent,通过共享 GitHub 账号发布,不能替代人类独立评审。

一条 P1、一条 P3,都在行内。 诊断和修法都站得住,两个回归测试也确实有区分力 —— 这些我自己复现过,没有采信描述。挡住它的是新文件的位置:app-shell-frame-style.ts 落进了被冻结的 legacy AppShell 族,test 在这个 head 上是红的。

我在无头 Chromium 里跑了折叠态 story,在同一份真实样式表上对两个取值做了 A/B:0px 时标题栏保持 3 条轨道(68px 1156px 0px)、rail 在 28px;无单位 0 时塌成 2 条隐式轨道(528.125px 703.875px)、rail 跳到 488px。机制完全成立。

两个回归测试都在旧行为上失败:单测把写入方改回无单位 0 后(确认变异进了 dist)报 0 !== '0px';story 在同样变异下 play 函数在新断言处就抛了,根本没走到最后的展开点击 —— 这同时证明 story 的取值现在确实跟着生产写入方走,漂移被闭上了。

被删掉的 CSS 规则本来就是死的:全树只有两处构造 .appFrame,都走共享写入方,没有别处写这个变量。我也顺手查了同类缺陷,仓库里没有第二处把内联自定义属性写成裸数字的地方。

证据:清空产物后重建全部依赖包与 desktop 产物;desktop main 套件 2691 通过 0 失败;四个 tsconfig 的 typecheck 全绿;改动文件 biome lint 干净。

我没覆盖的:真实 macOS / Electron 43.4.1 的渲染。截图和 env(titlebar-area-x)=94px 的 CDP 实测是你的测量,我只确认了同一条 CSS 机制在 Chromium 里产生同样的轨道塌陷。smoke:storybook 全量目录我也没跑,所以你提到的那条 flake 我无法评论。

这个 PR 目前是 draft,红色检查你可能已经知道。

Comment thread apps/desktop/src/renderer/shell/frame-style.ts
Comment thread apps/desktop/src/renderer/styles/shell-layout.css Outdated

@jackwener jackwener left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Addendum, re-bound to cd4446170d0824d88705007501ff70b1a46a7900. My review above was published against 881542cb1cc7e376239ede019ff8100fa3b2fc81; the head moved while I was writing it. The only difference between the two commits is apps/desktop/renderer-architecture.json, so everything I verified about the fix, the two regression tests and the deleted CSS rule carries over unchanged — the source files are byte-identical.

What changed is the status of the P1, and not in the direction the new commit intends: the architecture gate is still red at this head under the invocation CI actually uses. Details inline.

The P3 about the orphaned comment block in shell-layout.css is unaffected.

简体中文

补充说明,结论重新绑定到 cd4446170d0824d88705007501ff70b1a46a7900。 上面那份评审绑的是 881542cb1cc7e376239ede019ff8100fa3b2fc81,我写的过程中 head 动了。两个 commit 之间只差 apps/desktop/renderer-architecture.json,源码文件逐字节相同,所以我对修法、两个回归测试和被删 CSS 规则的核验全部仍然成立。

变的是那条 P1 的状态,而且方向和新 commit 的意图相反:在 CI 实际使用的调用方式下,架构门禁在这个 head 上仍然是红的。细节在行内。

shell-layout.css 那条 P3 不受影响。

Comment thread apps/desktop/renderer-architecture.json Outdated
@Astro-Han
Astro-Han force-pushed the fix/desktop-titlebar-sidenav-width-unit branch from cd44461 to c815ecd Compare September 17, 2026 06:21
apache#4829 made the appFrame publish --maka-sidenav-width unconditionally and
wrote a unitless 0 for the collapsed state. A unitless 0 is a <number>, so
calc(0 - 4px - …) in the titlebar's first track is a type error that drops
the whole grid-template-columns; the strip falls back to implicit columns
and the rail parks mid-window while the sidebar is gone.

Publish '0px' instead, and move the two column-width vars into a shared
writer (appShellFrameStyle) so the Storybook frame can no longer drift from
the production write — the story had been mirroring the pre-apache#4829
conditional write, which is exactly why the regression shipped silent. The
writer lives in src/renderer/shell/, the zone the architecture ledger
sanctions for new AppShell dependencies, so the frame gains no legacy
import debt. UpdateDownloadedCollapsed now asserts the grid keeps its
three tracks and the rail stays inside the left gutter; a unit test pins
the writer's <length> contract.

The collapsed-state CSS rule that set --maka-sidenav-width is deleted:
every real frame publishes the var inline, so the rule could never win —
and its comment still documented it as the collapsed-width mechanism while
an inline 0 was already shadowing it. That shadowing was the bug.

Generated-by: Devin

Generated with [Devin](https://devin.ai)

Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
@Astro-Han
Astro-Han force-pushed the fix/desktop-titlebar-sidenav-width-unit branch from c815ecd to dda3c01 Compare September 17, 2026 07:48
@Astro-Han
Astro-Han marked this pull request as ready for review September 17, 2026 07:50

@jackwener jackwener left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Re-review at dda3c012833ee239d4bdd47d23ee8fb68f43a4b5. I am an AI agent (executing seat @kabi-opus) publishing through a shared GitHub account; this is an automated review and does not substitute for independent human review.

Both findings are resolved. No remaining findings. Approving.

One disclosure, because it bounds what this is worth: both findings were mine, and this is me verifying fixes to my own findings. That is not an independent check. I kept it to mechanical verification, and it should not be counted as a second opinion.

The file no longer joins the frozen family, and the gate agrees

app-shell-frame-style.ts is now src/renderer/shell/frame-style.ts — a 100% rename, contents unchanged — with its three importers repointed. The ledger delta is three lines: one new dependency path, and app-shell.tsx recorded 13 tokens smaller than the base. Nothing was added to the legacy AppShell set, so nothing needed a target-owner entry.

I ran the invocation CI uses, against the merge base:

$ node scripts/check-renderer-architecture.mjs --base 846f4fbaa1e6dd924001ebeb03be1ebee7b02e0b --strict-base
Renderer architecture check passed against 846f4fbaa1e6dd924001ebeb03be1ebee7b02e0b.

The bare invocation passes too, and the checker's own 112 tests are green. Worth recording because on the earlier head those two disagreed — the bare script passed while --strict-base refused — which is what made the ledger-declaration approach look fine locally.

The comment now sits on the rule it describes

The orphaned paragraph in shell-layout.css is resolved the right way round: the sentence about the wrapper's duplicate flag moved down onto the visibility: hidden rule it actually describes, and the standalone block keeps only the guard note. Comments only — this head and the previous one are byte-identical outside that file.

Evidence

All workspace packages and the desktop main/preload/overlay outputs rebuilt from cleaned dist directories. npm run typecheck (preload, main, renderer, storybook): clean. The new unit test passes; desktop main suite 2691 pass, 0 fail.

The fix's behaviour carries over from the earlier head unchanged, since frame-style.ts is byte-identical to the file I measured there: in headless Chromium, 0px keeps the titlebar's three tracks with the rail in the left gutter, while a unitless 0 collapses it to two implicit tracks and parks the rail mid-window; the unit test goes red on that mutation with the marker confirmed in the emitted dist/; and the collapsed story's play throws at its new assertions before reaching the expand click.

A note on the CI run, since it is now part of this PR's record

The first test run on this exact head failed at Storybook smoke, on product-workhub--filter-work-conversations-narrow — a DOM matcher receiving null. The re-run of the same job on the same commit passed. That story is untouched here, and the only CSS this PR removes is scoped to .appFrame[data-sidebar-state='collapsed'], which the WorkHub stories never render — they mount WorkHubRoot with no app frame or titlebar at all. I also could not reproduce it locally: 3 serial runs at 720×900 plus 4 rounds of the 4-way concurrency the smoke uses, 19 runs, all clean.

Same commit, red then green, is the clearest evidence available that the failure is intermittent rather than caused by this change. The 720×900 variant failed while its wide sibling passed in the same run, which points at timing rather than layout. It may be worth tracking separately — a second story in that file was reported behaving the same way on #5423.

Not covered by me

Real macOS and Electron rendering. The BEFORE/AFTER screenshots and the CDP session are yours; what I confirmed is that the same CSS mechanism produces the same track-list collapse in Chromium. I also did not run the Storybook smoke the way CI does — this machine has no disk headroom for a static build, so my reproduction attempts used a dev server, which is a weaker approximation.

简体中文

在 dda3c012833ee239d4bdd47d23ee8fb68f43a4b5 上复审。两条发现都已解决,没有剩余 finding,给出批准。

先声明:两条发现都是我提的,这次是我验自己的修复,不构成独立检查,只做了机械核对,不应被当成第二意见。

P1:文件 100% 纯改名到 src/renderer/shell/frame-style.ts(内容一字未动),三处 import 跟着改,台账差异三行(一条新依赖路径 + app-shell.tsx 比基线少 13 个 token)。没有往冻结族里添东西,因此不需要 target-owner 条目。我跑了 CI 实际使用的 --base <merge-base> --strict-base:通过;裸跑也通过,检查器自带 112 条测试全绿。这点值得记录,因为在上一个 head 上这两者是不一致的 —— 裸跑绿而严格模式拒绝,那正是"登记台账"的做法在本地看起来没问题的原因。

P3:shell-layout.css 里那段悬空注释的处理方向是对的 —— 关于 wrapper 重复标志位的那句挪到了它真正描述的 visibility: hidden 规则上,独立块只留守卫说明。纯注释改动:这个 head 与上一个除该文件外逐字节相同。

证据:清产物重建全部工作区包与 desktop 的 main/preload/overlay;四项 typecheck 全绿;新增单测通过;desktop main 套件 2691 通过 / 0 失败。修复行为从上一个 head 平移,因为 frame-style.ts 与我当时测量的文件逐字节相同。

关于这次 CI:本 head 上第一次 test 挂在 Storybook smoke 的 product-workhub--filter-work-conversations-narrow(DOM 匹配器收到 null),同一个 commit 重跑该 job 后通过。该 story 本 PR 未动,而本 PR 删掉的 CSS 作用域是 .appFrame[data-sidebar-state='collapsed'],WorkHub 的 story 渲染 WorkHubRoot、树里根本没有 app frame 和标题栏。本地也复现不出:720×900 连跑 3 次 + smoke 同款 4 路并发 4 轮,共 19 次全部干净。同一 commit 先红后绿是目前能拿到的最有力证据,说明这是间歇性失败而非本次改动引起;同一次运行里宽屏兄弟通过、只有 720×900 变体挂,也更像时序问题而非布局问题。这条值得单独跟踪 —— #5423 报过同一文件里另一个 story 表现相同。

未覆盖:真实 macOS 与 Electron 渲染(截图与 CDP 是你的测量);我也没有按 CI 的方式跑 Storybook smoke —— 这台机器磁盘不够做静态构建,复现用的是 dev server,是更弱的近似。

Code review, CI status and merge readiness are separate. This approval covers code only and is not a statement that the PR may be merged.

@Astro-Han
Astro-Han merged commit 9f706ef into apache:main Sep 17, 2026
2 of 3 checks passed
@Astro-Han
Astro-Han deleted the fix/desktop-titlebar-sidenav-width-unit branch September 25, 2026 12:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/M Under 500 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants