fix(desktop): keep page headers clear of the traffic lights - #6098
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryMedium Risk Overview Shared lane padding — Introduces CSS shell — Regression checks in Reviewed by Cursor Bugbot for commit 11aa4bf. Configure here. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Greptile SummaryThis PR centralizes desktop title-bar lane padding and applies it across workspace page and resource headers.
Confidence Score: 5/5The PR appears safe to merge, with no blocking failure remaining. No blocking failure remains; the reply from says the narrow guard was replaced, but the current audit still inspects only the two canonical implementation files and therefore leaves the previously reported uniqueness-check limitation outstanding as non-blocking test coverage feedback. Files Needing Attention: apps/sim/app/_shell/desktop-title-bar-surfaces.test.ts
|
| Filename | Overview |
|---|---|
| apps/sim/app/_styles/globals.css | Defines the shared title-bar inset and updates desktop shell, login, and sidebar layout styling. |
| apps/sim/components/page-header-bar.ts | Introduces shared page-header geometry and title-bar lane padding constants. |
| apps/sim/app/workspace/[workspaceId]/components/resource/components/resource-header/resource-header.tsx | Composes the shared lane padding into the bordered resource-header geometry. |
| apps/sim/app/workspace/[workspaceId]/components/workspace-chrome/workspace-chrome.tsx | Publishes fullscreen state to CSS and adjusts collapsed desktop pane and peek-card chrome. |
| apps/sim/app/_shell/desktop-title-bar-surfaces.test.ts | Adds assertions for desktop title-bar variable ownership, pane geometry, embedded-panel reset, and canonical header composition. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
Desktop[Desktop title-bar mode] --> Shell[Workspace content shell]
Shell -->|Sidebar expanded| Zero[Inset remains 0px]
Shell -->|Sidebar collapsed or content fullscreen| Lane[Inset equals title-bar height]
Zero --> Headers[Shared page and resource headers]
Lane --> Headers
Headers --> Clear[Header controls remain clear of native traffic lights]
Shell --> Mothership[Mothership embedded panel]
Mothership --> Reset[Reset inherited inset to 0px]
Reviews (7): Last reviewed commit: "docs(desktop): correct the peek card chr..." | Re-trigger Greptile
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
@cursor review |
a40bdee to
b7f76b5
Compare
|
@cursor review |
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit b1a2886. Configure here.
In the desktop app, collapsing the sidebar hands the top-left of the screen to the content pane — the macOS traffic lights and the sidebar expander then sit over page content rather than over the sidebar. Every top-of-page header bar drew straight underneath them, so back buttons and the integrations/skills switcher were unreadable and unclickable. The reason it hit all of them at once is that the bar's geometry was copied verbatim into seven files, so nothing could reserve that lane in one place. Extract it as PAGE_HEADER_BAR and fold the lane into its top padding via `--workspace-content-title-bar-inset`, the variable the content pane already publishes for exactly this. That variable is 0px everywhere except the macOS desktop app with the sidebar collapsed, so this is inert on the web and on an expanded sidebar, where the lights sit over sidebar chrome that already reserves its own lane. Covers the settings shell, the credential/skill detail layout, the integrations and skills switcher, integration block detail and its fallback, and both upgrade headers. A guard test fails with the file named if any page re-derives the bar instead of composing it.
The first pass found its surfaces by grepping one exact class string, which turned up the minority header. `ResourceHeader` is the same bar written as `px-4 py-[8.5px]`, and it is the one logs, files, tables, knowledge and scheduled tasks use — plus every loading fallback. Those all still drew under the traffic lights, and on the breadcrumb surfaces the occluded element is an interactive popover button, not just a title. Share the lane math as TITLE_BAR_LANE_PT and compose it from both bar geometries, which genuinely differ (the Resource bar is bordered and has a min height). Nothing nests one bar inside the other, so no reset is needed: the only out-of-pane render is the landing tables preview. Move the variable's `0px` default to `:root` alongside the other desktop-title-bar vars, so it is defined for bars outside `.workspace-content-shell` — the standalone settings shell and that landing preview — where an undefined var() inside calc() would be invalid at computed-value time and drop padding-top entirely. That replaces the per-call fallback, leaving one default instead of two that can drift. Fold the assertions into the existing desktop title-bar surface audit rather than a second audit file with its own conventions. The standalone guard is gone: it keyed on one spelling of the geometry, so it was blind to `px-4 py-[8.5px]` — the very re-derivation that made this pass necessary.
Review round 2 found the two arrangements the collapsed-sidebar selector alone gets wrong. A fullscreen route (/upgrade) slides the sidebar to zero width without collapsing it, so `data-sidebar-collapsed` is absent and the lane stayed zero while the pane was in fact sitting under the traffic lights. The pane owns the lane whenever the sidebar is not there to own it, so the selector now matches a new `data-content-fullscreen` as well. The mirror error: the variable is inherited, and the mothership panel is the right half of the pane — never under the lights — yet it embeds whole pages (KnowledgeBase and friends) whose header bars reserve the lane. Those bars were gaining ~38px inside the panel. The panel now zeroes the variable for its subtree. I had checked for exactly this nesting and concluded it did not exist, having looked only at the settings pages that import `Resource` types without rendering `Resource.Header`. `resource-content.tsx` renders the knowledge page itself, which does. An assertion each, both verified to fail when the fix is reverted.
…hadow The floating sidebar pinned both its top and bottom edges, so it always drew at full window height. On a short surface — the settings list — that left a tall empty slab of card hanging below the last entry. The card now hugs its content and caps at the pane height less the traffic-light lane and the bottom gutter. Dropping the bottom pin is most of it: the four `h-full` rules down the chain resolve against an auto-height parent and collapse to content on their own. But nothing would then bound the sidebar's own `flex-1 overflow-y-auto` region, so a long workflow list would be clipped by the card's `overflow-hidden` instead of scrolling. The card is therefore a capped flex column, and the shell is allowed to shrink inside it, which restores a definite height for the chain to resolve against. That rule is scoped to `[data-peek]` and is inert while docked, where the shell is not a flex item. Also eases `--shadow-overlay` in both themes (alpha ~27% lighter, bloom pulled in from 48px to 30px), keeping it clearly above `--shadow-medium` so the scale still reads in order. The peek card drops the shadow entirely and separates on the same `--border` hairline the content pane beside it uses.
b1a2886 to
75eccb9
Compare
|
@cursor review |
…the window edge Two reports against the desktop window chrome. The login page scrolled by exactly the traffic-light lane. `.desktop-title-bar-page` reserved the lane with `margin-top` plus a `calc(100vh - lane)` height, which sums to the viewport on its own — but `body` carries `min-height: 100vh`, and body is a plain block box with no padding, border, or BFC, so that top margin had nothing to collapse against and collapsed through, displacing body itself. The document came out one full lane taller than the viewport while the shell's `calc` saving was re-inflated underneath it. Reserving the lane with padding *inside* the box removes both the collapse and the `calc`: global `box-sizing: border-box` keeps the padding within the `100vh`. Measured in the Electron renderer over CDP — 40px of overflow before, 0 after, with the logo still clear of the lane. Collapsing the sidebar also left a hairline outline traced just inside the window. The shell drops to `p-0` there, but the content pane kept its border and 8px radius, so both drew flush against the square window frame. The pane now drops them exactly when it is flush. Keyed off the ancestor attributes rather than React state, because the title-bar attribute is written pre-paint and a state-driven rule would flash the border before hydration settles.
|
@cursor review |
The card's TSDoc still listed `shadow-overlay` as part of its chrome after the shadow was deliberately dropped, so the comment contradicted the code. It now records that the card is unshadowed on purpose, and documents the content-hugging height and the flex-column cap that make the sidebar's scroll region bound itself.
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 11aa4bf. Configure here.
1 participant
The report
Cause
With the sidebar expanded, the traffic lights and the sidebar expander sit over the sidebar, which reserves that lane itself. Collapse the sidebar in the desktop app and the rail goes to zero width — the content pane becomes full-bleed, so its top-left is now under both.
Every top-of-pane header bar drew straight into that lane. It hit all of them at once because the geometry was re-derived per file rather than shared. The pane already publishes the right number —
--workspace-content-title-bar-inset— but only one file consumed it (the mothership chat), which is why chat was the one surface that looked correct.There are two bar geometries, and they are spelled differently, which is why the first pass of this PR only found one of them:
PAGE_HEADER_BARpx-[16px] pt-[8.5px] pb-[8.5px]ResourceHeaderpx-4 py-[8.5px](identical spacing)On the breadcrumb surfaces (files inside a folder, tables, knowledge) the occluded element is an interactive
BreadcrumbLocationPopoverbutton, not just a title.Fix
Share the lane math as
TITLE_BAR_LANE_PTand compose it from both geometries, which genuinely differ — theResourcebar is bordered and has a min height, so folding them into one constant would change layout in 11 files for no gain.Move the variable's
0pxdefault to:root, next to the other--desktop-title-bar-*vars. Some bars render outside.workspace-content-shell— the standalone settings shell (/account,/organization/[id],/selfhost) and the landing tables preview — where an undefinedvar()insidecalc()is invalid at computed-value time and would silently droppadding-topentirely. A:rootdefault fixes that with one definition rather than a per-call fallback that can drift from the real one.Nothing nests one bar inside the other, so no reset mechanism is needed (verified: the settings pages that import
Resourcetypes never renderResource.Header).Inert everywhere the variable is
0px— i.e. the whole web app and any expanded sidebar.Why not reserve the lane on the content pane instead
One change would have covered every surface, but it would also push the workflow canvas down by 38px. The editor has no top-left chrome, so today the lights simply float over the canvas; forcing a band there is a product decision, not a bug fix. Keeping the reservation on the bars leaves the canvas full-bleed and changes nothing that currently renders correctly.
Testing
Assertions live in the existing
desktop-title-bar-surfaces.test.tsaudit — same subject, same technique, and it already anchors paths toimport.meta.urlrather thanprocess.cwd().:rootwith a0pxdefault, and overridden only for a collapsed sidebar.TITLE_BAR_LANE_PT, andResourceHeadercarries no barepy-[8.5px].Each verified to fail when its regression is reintroduced. An earlier standalone guard was deleted: it keyed on one spelling of the geometry, so it was blind to
px-4 py-[8.5px]— precisely the re-derivation that made the second pass necessary. A guard that can't catch the bug in front of it is worse than none.tsc,biome,check:api-validationclean; 75 tests green across the touched areas. Confirmed Tailwind emitspadding-top: calc(8.5px + var(--workspace-content-title-bar-inset)), and thatcn(PAGE_HEADER_BAR, 'justify-between')produces a class set identical to the original modulo thept-change.Not verified
The pixel result on macOS. I could not confirm in the running desktop app, so the geometry is derived from source and compiled CSS. Worth one look with the sidebar collapsed on Integrations, Tables and Files before merge.
Also unresolved, and pre-existing: the standalone settings shell (
/account/settingsetc.) reserves no lane for its own 248px sidebar on desktop, so the lights sit over it there. Separate surface, separate fix.