fix(desktop): keep titlebar actions clear of workbar - #4829
Conversation
a0754f2 to
581982b
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed current head 581982bdcfd5c1bfe907ed4ba13f6865a0f87b26 (OPEN). Technical GO — no P0–P2, two non-blocking P3 observations. Note: the PR is currently CONFLICTING, so a rebase is required and will invalidate this review — it is bound to this head only.
What it does and why it holds
Titlebar actions are pushed clear of the workbar by switching the width variable's source without changing its value (rightWidth: layout.workbarWidth vs the frame-inherited workbar width rule; --agents-content-area-gap stays 4px from :root, so the calc cannot break). Both screenshots were examined individually: before, the long title runs to x≈1200 under the panel starting x≈820, overlapping the Share icon; after, the title truncates ~620px with Share fully outside the panel's left edge — matching the fix claim. The new e2e asserts geometry (identity-box right edge ≤ workbar-box left edge), not just visibility, and Share still opens its dialog; the helper's selector move (.maka-workbar-layout-vars → .appFrame) tracks the variable migration, and the old class still carries bottom-panel height (not dead code). The test actually ran and passed in this CI run (Desktop e2e step). Single commit; renderer-architecture metric deltas verified by CI.
P3s (non-blocking)
- CONFLICTING/DIRTY: author must rebase; the head will necessarily drift and this conclusion lapses entirely — re-review on the new head.
- Defensive:
getFrameStyle's reserve has no placement predicate — correct today only because placements are right/bottom. If a left placement ever appears, this needs a matching condition; worth a comment now.
What I could not judge
Real-window rendering on macOS/Windows (gutter variables differ from Linux) — geometry logic, Linux CI e2e, and the author's screenshots interlock under Linux semantics; other platforms should get author/maintainer eyeballs.
Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.
简体中文
评审结论来自自动化审查流程;发布者没有读这份 diff,核的是当前 head 有没有漂移、以及 exact-head 的门禁状态。当前 head 是 581982b,未关闭。技术上无阻断问题,两条观察:先解冲突(rebase 后结论作废重审),另预留左侧布局的条件。等人类拍板。
581982b to
799751f
Compare
|
Rebased onto the latest |
Astro-Han
left a comment
There was a problem hiding this comment.
Follow-up to my review above, new head 799751f532ee1fcb728e1cfc912d7e0bb0b13229 (OPEN, MERGEABLE/BLOCKED awaiting human review). Technical GO — no P0–P2, no remaining observations. The test job is fully green on this head (10m32s, Desktop e2e actually executed and passed, affected workspace tests passed).
What changed in the rebase
The author's own change semantics are untouched; the app-shell.tsx / shell-layout.css line movements are rebase adaptations to main context (e.g. transcriptReadingPosition → ownerActiveId changes coming from main). Prior P3s cleared: ① the conflict is rebased away; ② the placement-guard comment was added.
What I could not judge
macOS/Windows real-window rendering — as before, geometry logic, Linux CI e2e, and screenshots interlock under Linux semantics; other platforms should get author/maintainer eyeballs.
Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.
简体中文
评审结论来自自动化审查流程;发布者没有读这份 diff,核的是当前 head 有没有漂移、以及 exact-head 的门禁状态。当前 head 是 799751f,未关闭,检查全绿。旧观察项都消解了,技术上无阻断问题。合并由人类定。
Astro-Han
left a comment
There was a problem hiding this comment.
Re-reviewed at 799751f against main (a5a99a633), +54/−12 over 6 files. The branch conflicts with main again, only on apps/desktop/renderer-architecture.json (the generated ledger; main moved the same app-shell.tsx count), so it needs one more rebase and a regenerated ledger, not a hand merge.
The overlap is real and the fix is at the owner: the right workbar deliberately shares the titlebar band and sits above it (styles/workbar/shell.css:160-180), the titlebar's right padding only counted the overlay width (shell-layout.css:276), and reserving the workbar width in the titlebar's own gutter is the right place. The previous P3 on the placement comment is fixed (use-workbar-controller.ts:709-711).
P2 (path ①): the reserve ignores the breakpoint that moves the workbar. Placement is decided in CSS: at max-width: 990px the workbar goes full-width below the conversation (styles/workbar/artifacts.css:374-397). The reserve is computed in TS from available && !collapsed alone (use-workbar-controller.ts:707-716), and the width is clamped to 320–600 with no viewport term. Drag the panel to 600, narrow the window under 990: the workbar is no longer on the right, but the titlebar still reserves ~604px, the minmax(0, 1fr) breadcrumb track collapses to a few dozen pixels and, narrower still, the actions leave the band. Recoverable (collapse the panel, widen the window), but a normal path, and the new e2e only asserts a wide window. Smallest fix keeps the breakpoint authority in CSS: inside the same media query set --maka-titlebar-workbar-reserve: 0px on the titlebar, and add the narrow case to the e2e.
P3:
WorkbarHostModel.rightWidth(workbar-host.tsx:88) and its producer (use-workbar-controller.ts:731) have no consumer left now that the width variable is written elsewhere.getFrameStylealso pulls--maka-sidenav-widthinto the workbar controller; the app shell already hashost.rightWidthandselectors.rightCollapsed, so it can build the style itself and the previous item stays alive.- The long comment at
app-shell.tsx:2653-2675explains a conditional that moved to the controller. - The two body images are SVGs on a gist, one theme. The rule for UI changes is real screenshots on user-attachments, light and dark, same viewport, BEFORE from
mainand AFTER from the branch.
Evidence boundary: static read; the narrow-window geometry is computed from the grid tracks and constants, not measured; the gist images were not fetched.
AI-assisted review: drafted with Maka; I verified the media query, the unconditional reserve and the conflicting file myself.
65d8158 to
1e2f324
Compare
|
Addressed the review in
Local verification passed: Desktop tests, typecheck, Electron Please re-review the new head. |
1e2f324 to
6e44fb8
Compare
Hoist the right workbar width to the shared frame and reserve that space in the titlebar while the panel is open. Add a regression test for a long session title with the maximum-width usage panel and verify Share remains clickable. Fixes apache#4826 Generated-by: Maka
6e44fb8 to
7e14635
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Second follow-up, new head 7e14635f33081db55fa64c8ce8242d313ffe8611 (OPEN, MERGEABLE/BLOCKED awaiting human review). Technical GO — no P0–P2, no remaining findings. This head rewrites the mechanism (not a rebase carry-over), so prior conclusions were re-verified, not extended. CI test fully green on this head (10m54s, Desktop e2e actually executed, Electron e2e budget, Storybook build + smoke with both new stories executed).
What changed
Reserve moved from JS-gated (controller getFrameStyle writing an inline var) to CSS-driven: :where(.appFrame:has(.maka-session-workbar[data-placement='right']:not([data-collapsed]))) sets --maka-titlebar-workbar-reserve on the titlebar, consumed by padding-right (0px fallback); a ≤990px media query forces reserve to 0 when the workbar moves below the conversation. The width var is now published at the single .appFrame point (frame = shared owner); workbar-host no longer publishes. Sidenav-collapse inline 0 matches the CSS constant (no behavior change). The old e2e long-title test is replaced by two stories (wide: identity.right ≤ workbar.left + Share clickable; narrow: workbar width = detail width + Share within titlebar); e2e-budget 5→6 accounts for main-side #4694's already-landed persistence test (6 specs on base, counted).
Correctness points verified
data-collapsed={!visible || undefined} matches :not([data-collapsed]); :where() adds no specificity, cascade correct; narrow story hits the 720px viewport via the smoke script's naming convention (storyViewport for narrow ids), consistent with repo convention (observation, not finding). The new CSS keys explicitly on [data-placement='right'] — natively placement-aware, so the previous placement-guard concern no longer applies (the controller no longer owns reserve).
Six-layer pass
Design (real problem, evidenced) → function (frame-publish → titlebar-consume is the true production path) → complexity (single-point publish + CSS state machine replacing JS gating, entropy down) → tests (stories pin geometry incl. narrow path + budget reconciliation) → naming/comments → style.
Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.
简体中文
评审结论来自自动化审查流程;发布者没有读这份 diff,核的是当前 head 有没有漂移、以及 exact-head 的门禁状态。当前 head 是 7e14635,未关闭,检查全绿。机制重写后重审,技术上无阻断问题。合并由人类定。
Astro-Han
left a comment
There was a problem hiding this comment.
Re-reviewed at 7e14635 against main (c0229b0c6), +150/−43 over 7 files, CI green.
The P2 is fixed where the breakpoint lives. The reserve is now a CSS rule: :where(.appFrame:has(.maka-session-workbar[data-placement='right']:not([data-collapsed]))) .maka-window-titlebar sets --maka-titlebar-workbar-reserve (shell-layout.css:327-334), and the same max-width: 990px block that moves the workbar below the conversation zeroes it (artifacts.css:374-377), so placement and reserve can no longer disagree. getFrameStyle is gone, rightWidth is consumed by AppShell, the stale comment is replaced, and the two stories WorkbarKeepsTitlebarClear and NarrowWorkbarClearsTitlebarReserve assert the geometry at both widths through the real AppShell, which is the right tier for this. The body now carries real screenshots on user-attachments, light and dark.
The extra hunk in PlusMenuRefreshHarness was a fair deflake of an existing story (the 150ms timer lost to CI twice on this branch, run 34014988954), but #4897 has since landed the fuller fix on main, so it goes away in the rebase.
One thing before merge: #4895 and #4897 landed on main with the same e2e-budget.json entry, the same story layout state and a fuller version of the PlusMenuRefreshHarness deflake, so this head now conflicts. Please rebase, keep main's text for all three, and drop this branch's harness hunk; the CSS, AppShell and the two new stories rebase clean.
Manual check done by reading: none beyond the stories; the narrow case is the one I could not measure last time and it is now asserted.
AI-assisted review: drafted with Maka; I verified the selector, the media-query reset, the removed controller API, the two stories and the deflake myself.
Summary
main, regenerate the renderer architecture ledger, and align the newly landed per-Session Workbar E2E budget with its six tests.Fixes #4826
Before / after
All captures are real Electron windows at the same 1280×800 viewport with the Workbar set to 600px.
Verification
npm --workspace @maka/desktop run test:distnpm --workspace @maka/desktop run typechecknpm --workspace @maka/desktop run e2e -- e2e/session-workbar.spec.ts— 6 passednpm --workspace @maka/desktop run build-storybook && npm --workspace @maka/desktop run smoke:storybook— 311 stories, 338 theme rendersnpm run check:renderer-architecture -- --base upstream/main— 101 passednpm run check:e2e-budget— 31 tests in 16 filesnpx biome checkon the 7 changed filesgit diff --checkAI use
Select exactly one:
Tool(s) and scope:
Maka (GPT-5.6 Sol) inspected the layout and review feedback, implemented the shared-frame and responsive CSS fix, added regression coverage, captured the Before/After evidence, and prepared the commit and PR description.
Checklist
Does this PR entail a change in behavior?