Uh oh!
There was an error while loading. Please reload this page.
fix(desktop): fuse the collapsed rail with the plate under the traffic lights - #2396
fix(desktop): fuse the collapsed rail with the plate under the traffic lights#2396AidenNovak wants to merge 1 commit into
Conversation
4cbdf7f to
e549ca2Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks — the fusing is clean: same collapsed gate, same cascade layer, token-driven, no !important, and the color-contract test rework (RGBA byte compare + settle sampling) is stronger than a screenshot. Two P2s worth a look before this lands:
- Docked content slides 4px under the drag band. The collapsed margin-block-start: 0 (shell-layout.css:238-240) moves the plate top to y=0, so content starts at 32px — but the titlebar drag strip covers y=4..36 (shell-layout.css:294-297). Workbar tabs and scrolled message content now have their top 4px inside the drag region — exactly the app-region hygiene class that has bitten this repo twice (titlebar buttons, breadcrumb clicks). The docked state should use the
--h-titlebarclearance (or the formula should track the margin), plus a geometry assertion that content.top ≥ strip.bottom. - The three new transitions aren't gated during drag-resize. The width ease is deliberately suppressed under [data-resizing] (shell-layout.css:172-182 comment), but background-color / margin-block-start / border-radius now animate mid-drag when crossing the 160px threshold, and oscillate on repeated crossings. Same gate should cover them (e.g. transition-duration: 0.01ms under [data-resizing], like chat-detail.css:87-90).
P3s: the test hardcodes token values (4px/12px) against this file's own 'never against a literal' philosophy; the macOS traffic-light rationale applies globally (Windows/Linux also get the new rail color/radius); the background-color transition also eases theme switches for 280ms. Approving either way.
e549ca2 to
e1be88cCompareAstro-Han
left a comment
There was a problem hiding this comment.
Thanks for the careful visual work here. I tested the before/after states in the same real Electron window. The seam improvement is real, but quite subtle, while the current approach introduces two interaction risks:
- [P2] Moving the plate to
margin-block-start: 0puts its first 4px of content inside the native titlebar drag region. - [P2] The new background, margin, and radius transitions remain active during sidebar resize, unlike the existing width transition guard.
I think this can be much smaller: keep the plate geometry unchanged and only let the collapsed rail use --agents-content-area-bg. That should remove the relevant color seam without moving content, changing corner geometry, or adding transitions.
If that single paint change is not sufficient, I would prefer keeping the current UI rather than adding layout and interaction side effects for this small visual difference. Thanks again for exploring this carefully.
Astro-Han
commented
Aug 18, 2026
/agentic_review |
Code Review by Qodo🐞 Bugs (0)📘 Rule violations (0)📎 Requirement gaps (0)Great, no issues found!Qodo reviewed your code and found no material issues that require reviewTip of the day💡 Did you know, you can keep summaries lean with Finding overflow, which tucks the rest behind 'View more' |
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks — this is unusually well-reasoned CSS, and I want to be specific about why before I raise anything.
The diagnosis is the good part: the cluster is ~54px and the rail is 48px, so no amount of nudging trafficLightPosition fits it, and the only move left is to stop having a seam under it. Reaching for that instead of shaving pixels is the right instinct. Wearing the plate's own --agents-content-area-bg rather than --color-background-surface — and saying in the comment that the latter is precisely what made #2187 read as a third tone in dark mode — means the fusion holds in both palettes by construction rather than by coincidence. And the note about why the transitions live on the base rules (a state-scoped transition stops applying the instant its selector stops matching, so the expand leg would jump) is exactly the kind of thing that gets silently "cleaned up" by the next person if it is not written down.
Two [P2]s, both about reach rather than correctness. No [P0]/[P1].
The first is the one I would most like your view on: the problem being solved is macOS-only, but the fix is not scoped to macOS. There are no traffic lights in that corner on Windows or Linux, yet those users also lose the floating-plate look when the sidebar is collapsed. This repo already tags <html> with data-os="darwin" (theme-glass.css uses it), so scoping is available and idiomatic here.
Separately, this PR was opened on Aug 7 and its green checks carry the older check names (changes, typecheck, test_workspaces, storybook), so that green predates the current required set and mergeable currently reports UNKNOWN. Worth a rebase so the result reflects today's gates.
Review assisted by AI (Claude Opus 5). Findings were verified against the files at this head; the reviewer is accountable for them.
| light and dark both read as one surface by construction (verified live | ||
| against the shipping window in both palettes: rail and plate report the | ||
| same painted RGBA bytes when collapsed). */ | ||
| .appFrame[data-sidebar-state='collapsed'] .maka-shell-astryx .astryx-app-shell-sidenav { |
There was a problem hiding this comment.
[P2] The whole justification for the fusion — spelled out well in the comment above — is the macOS traffic-light cluster. On Windows and Linux there is no cluster in that corner, so there is nothing for the seam to cross, yet these three rules still apply: the rail changes material, the plate loses its top margin, and the top corners square off whenever the sidebar is collapsed.
So non-macOS users get a visual change to the collapsed state that solves a problem they do not have, and they lose the floating-plate design the expanded state deliberately keeps.
The mechanism is already here: theme-glass.css tags <html> with data-os="darwin" from window.maka.app.info(), so prefixing this block is a small, idiomatic change.
I am raising it as a question rather than asserting it is wrong, because there is a coherent argument the other way — that a single collapsed appearance across platforms is worth more than platform-specific fidelity, and that the flush dock reads fine on its own merits. If that is your reasoning, it belongs in the comment block above alongside the rest, since the current text explains the macOS motivation and leaves a reader assuming the rule is macOS-scoped.
| material and the plate's. The transition lives on the BASE rule so both | ||
| directions animate — a state-scoped transition drops the moment its own | ||
| selector stops matching, which would leave the expand leg jumping. */ | ||
| transition: background-color var(--duration-large) var(--ease-out-strong); |
There was a problem hiding this comment.
[P2] Putting the transition on the base rule is right for the reason you give, and I would not move it. The cost is that it now applies to everybackground-color change on this element, not only the collapse.
The concrete one is theme switching. I looked for a guard — a no-transitions class toggled around palette changes, or similar — and there is none in apps/desktop/src. So on light/dark switch the rail will ease over --duration-large while the surrounding chrome repaints instantly, leaving it visibly lagging behind the rest of the window for the length of a "large" duration. That is a normal user path, not a corner case.
Same consideration in principle for border-radius on .mainColumn at line 126, though corner radius is far less likely to change for unrelated reasons, so in practice this is really about the background.
The narrow fix is to scope the transition by property and state without scoping the rule — e.g. keep the declaration on the base rule but let a [data-theme-switching] (or whatever this app uses for palette changes) suppress it. If no such hook exists, that is worth knowing too, and this comment can stand as the record of the tradeoff.
I checked the transitions you added to .maka-panel-detail and .mainColumn for the same issue — those properties are not touched elsewhere, so they are clean.
jackwener
left a comment
There was a problem hiding this comment.
Exact-head review of e1be88c5a7021f59e4dee5e5cb4199e15a5d7695: NO-GO. The PR description does include the required light/dark before/after evidence, and the current-main merge-tree is clean. I independently confirmed the existing exact-head interaction concerns remain live: the drag-region overlap is line-anchored below; the resize-triggered collapse still changes these three newly-transitioned properties while only the width transition has a [data-resizing] guard; and the unresolved platform-scope/theme-transition threads (3836047653, 3836047655) still match the current code. No additional P0/P1.
The hosted successes are from 2026-08-09's older workflow/base world. After the blockers are fixed on a new head, please rerun today's path-filtered gates; these runs should not be used as approval evidence for the repaired result. COMMENT only; no APPROVE or merge action.
| } | ||
| .appFrame[data-sidebar-state='collapsed'] .maka-shell-astryx .maka-panel-detail { | ||
| margin-block-start: 0; |
There was a problem hiding this comment.
[P2][① normal collapsed-sidebar path] Setting the frame's top margin to 0 changes the geometry without changing --maka-plate-titlebar-clearance: with the live tokens, content starts at 0 + (36px - 4px) = 32px, while the window's sole drag strip spans y=4..36. That leaves the first 4px of the plate's usable content inside -webkit-app-region: drag; anything there that is not explicitly carved out as no-drag can start a window move instead of receiving the pointer. The same numbers and selectors survive the clean merge with current main. Please keep the content edge at or below y=36 (for example, make the clearance track the zero margin) and add a geometry assertion equivalent to content.top >= titlebar.bottom.
Dismissing a stale approval. This is bound to e549ca20; the PR has advanced well past it, and there are currently three unresolved [P0]–[P2] threads at the present head. This repository does not dismiss stale reviews automatically, so leaving the approval in place means the PR would flip straight to mergeable the moment CI goes green, with those findings still open. Re-approval should be at the head that carries the fixes.
…c lights The macOS traffic-light cluster is ~54px wide (x 17..71: 14px discs on 20px centres — main-window.ts, apache#2144) but the collapsed rail is 48px, so the yellow disc straddled the rail/plate seam and the green disc sat on the plate's own top-left corner. No position fits the cluster inside a 48px rail, so the seam is removed from under it instead. Collapsed, the rail puts on the plate's material (--agents-content-area-bg) and the plate docks flush under the titlebar (top margin and top corners to zero): one continuous surface where the lights sit. All three properties ease with the existing collapse width animation (--duration-large). Expanded keeps the floating-plate design untouched. This is the deliberate successor to the repaint apache#2187 removed: that one wore --color-background-surface, which light mode happened to share with the plate and dark mode did not — a third tone under the light cluster. Wearing the plate's own token fuses light and dark by construction. The sidebar-geometry material contract moves with the change: expanded still asserts rail == column (the apache#2187 regression lock), collapsed now asserts rail == plate plus flush margin/radius, compared as painted RGBA bytes so palettes and themes survive.
e1be88c to
c93208eCompareAstro-Han
commented
Aug 30, 2026
Thanks for exploring the traffic-light seam carefully. The underlying state this PR targets has since been removed by #3808: current As a result, the original 48px rail no longer exists for the macOS traffic-light cluster to cross, and applying this CSS now would restore assumptions from an obsolete collapsed-sidebar design. I propose closing this PR as superseded by #3808. Please do push back with a current- AI-assisted review disclosure: Codex assisted with the current-main history and CSS comparison; Astro-Han verified the superseding behavior and owns this closure decision. |
Problem
The macOS traffic-light cluster is ~54px wide (
trafficLightPosition {x:17, y:14}, 14px discs on 20px centres — #2144) but the collapsed rail is 48px: the 🟡 disc straddles the rail/plate seam and the 🟢 disc sits on the plate's top-left corner.Fix (collapsed state only, pure CSS)
The seam is removed from under the cluster: the rail puts on the plate's material and the plate docks flush.
--agents-layout-bg--agents-content-area-bgAll three ease with the existing collapse animation (
--duration-large). Wearing the plate's own token (not--color-background-surface, which was #2187's third-tone bug) fuses light and dark by construction.Same live Electron window, fix toggled on/off via injected CSS. Light + dark, traffic-light positions drawn in (OS buttons aren't in webContents captures).
Notes on scope
sidebar-geometry.spec.ts) was deleted upstream by the test: delete low-value and redundant suite files #2462 test trim; following that direction, the change is now CSS-only and verified live against the shipping window instead (rail and plate report identical painted RGBA bytes in both palettes when collapsed).--agents-layout-bg, plate--agents-content-area-bg) — the fusion still holds: collapsed rail reads the plate's paint.Testing
rail == plate == oklch(1 0 0),margin-top 0px,top-left radius 0px(light); same fusion in dark.