Commit 64bcfea
authored
fix(desktop): clear the traffic lights on every full-viewport surface, and enumerate them in CI (#6109)
* fix(desktop): clear the traffic lights on every auth-shell surface
Only /login reserved the macOS traffic-light lane, so signup drew its logo
underneath the lights — and so did reset-password, sso, verify, the CLI auth
handoff, and the invite pages. The pre-paint script marks the lane on every
desktop route, so any surface that did not reserve it overlapped.
Ownership moves to `AuthShell`, which is the single source of truth for the frame
all of these wear, and it now reserves unconditionally. Per-route gating was the
wrong shape rather than merely incomplete: `/invite/[id]` is a dynamic segment, so
no route list could have covered it. `supportsDesktopTitleBar` therefore drops its
pathname argument — the caller mounting the controller is the signal, and only
`AuthShell` mounts it. Workspace routes never render it and keep their existing
`WorkspaceChrome`-owned listener, so the two never contend for the attribute.
Off the desktop shell `--desktop-title-bar-height` is `0px`, so the reservation and
the drag strip collapse to nothing and `.desktop-title-bar-page` is exactly the
`min-h-screen` these surfaces had before — web is unchanged. With the prop gone the
client auth layout was a bare passthrough, so the route layout renders the shell
directly and the passthrough is deleted.
Measured in the Electron renderer over CDP across /signup, /login, /reset-password
and /cli/auth: lane 40px, logo top 56px, zero overflow on each. /invite/[id]
redirects to login when signed out and was not measured directly.
The surface audit gains a `stripComments` helper that every negative assertion runs
through. These files document the shapes they avoid, so a bare `not.toContain` was
matching the prose explaining the fix and failing on correct code.
* fix(desktop): cover the remaining traffic-light overlaps, and enumerate them in CI
Fixing signup by hand would have been the fourth time this bug was found by a person
hitting it. The audit now enumerates instead of listing what to inspect: it walks every
`.tsx` outside workspace chrome, flags each full-viewport root, and fails unless that
root either composes `.desktop-title-bar-page` or appears in an allowlist with a written
reason. A brand-new page that fills the viewport fails on arrival — verified by adding
one, and by reverting each fix below.
Running it found three more surfaces already overlapping:
- `/oauth-error` is Better Auth's `onAPIError.errorURL`, which is precisely where desktop
OAuth failures land, so the one page a user sees when sign-in breaks drew its content
under the lights.
- `/f/[token]` public file view, same shell family, same origin, reachable in the window.
- The signup and reset-password Suspense fallbacks are viewport-tall *inside* the
lane-reserving shell, so the page overflowed by the lane while the split chunk loaded.
A placeholder needs no viewport height.
Four surfaces are allowlisted with reasons: the two landing shells (the desktop shell
boots to /login or a workspace and has no path to marketing routes), the dev-only
playground, and the embedded resume interface.
Measured over CDP: /oauth-error reserves 40px with zero overflow.
* fix(desktop): cover the shells behind the allowlist, and make the guard fail
Greptile was right on both counts, and the first one is worse than reported.
`LogoShell` was allowlisted as "marketing chrome, not reachable in the desktop
shell". That claim was simply false: it is the frame for `not-found`, the interfaces
shell, the desktop handoff shell, and the public-file access gates — so the password,
email, and SSO gates for `/f/[token]` all still drew under the traffic lights. The
allowlist existed to make risk visible and instead hid four surfaces behind one
unverified sentence. It now carries two entries, both checked: the landing shell (every
consumer lives under `app/(landing)/`) and the playground (calls `notFound()` unless
`NEXT_PUBLIC_ENABLE_PLAYGROUND` is set).
The lane's two halves also travelled separately, so `/oauth-error` and the public-file
view reserved the space without the drag strip — clearing the lights but leaving the
window with no title bar on those pages. `DesktopTitleBarLane` now ships both together
and the audit enforces the pairing.
Both new checks were unfailable when first written, and mutation testing is the only
reason that surfaced:
- the pairing check matched `DesktopTitleBarLane` anywhere in the file, so the import
line satisfied it after the JSX was deleted;
- the coverage check matched `LogoShell` anywhere, so a shell's own definition file
self-certified as covered.
Both now match JSX usage (`/<LogoShell\b/`). Reverting either fix, and deleting either
half of the lane, now fails.
Measured over CDP: /oauth-error and the LogoShell-based 404 each reserve 40px, carry the
drag strip, and overflow by zero, with the logo at 56px.
* test(desktop): strip comments at read time so positive assertions can fail
Cursor caught the mirror of a trap this file already documents. `stripComments` was
applied to negative assertions only, so a positive like
`toContain('desktop-title-bar-page')` still ran on raw source — and `AuthShell`'s TSDoc
names that class, so deleting it from the markup left the assertion passing on a broken
lane reservation.
Stripping now happens in `read`, so every audit constant is comment-free and no
assertion in either direction can match prose. Verified: deleting the class from the
markup while leaving the TSDoc intact now fails two tests, where it previously failed
none.
* fix(desktop): audit workspace routes too, and cover the three that bypass the chrome
Greptile was right that the blanket `workspace/` exclusion was load-bearing in the wrong
direction. It assumed every workspace route reaches the lane through `WorkspaceChrome`,
and three do not: the workspace landing route, the access-denied early return, and the
workflow error boundary.
The exclusion is gone, so workspace files are audited like everything else and
`WorkspaceChrome` joins the lane-aware shells — the normal route passes through its
layout, and the three exceptions had to be dealt with on their merits:
- the landing route (status card and spinner) and access-denied now reserve the lane;
- the error boundary is allowlisted, verified: it renders `<Sidebar>`, which already owns
the workspace lane and its drag region, so padding that root would double it.
Their content was centred, so the lights were never covering text — the real gap was that
none of them rendered a drag strip, leaving the window immovable on those screens.
The guard's granularity is per file, not per JSX root: `workspace/page.tsx` holds two
full-viewport roots and still passes if only one reserves the lane. Verified by mutation
and documented rather than papered over — catching it needs an AST pass, and the check's
job is to stop a whole surface being forgotten, which is how every instance of this bug
has actually shipped.
* fix(desktop): teach the audit about nesting, and stop the resume skeleton double-reserving
Four findings, all correct, and the first is a bug this PR introduced.
The resume loading skeleton reserved the lane while already rendering inside
`(interfaces)/layout.tsx` -> `InterfacesShell` -> `LogoShell`, which reserves it too. Two
lots of padding, two drag strips, two controllers. It came from adding the lane there
before `LogoShell` became lane-aware and never reconciling the two. The skeleton now
reserves nothing and is no longer viewport-tall either — nesting a viewport-tall root
inside a viewport-tall shell overflowed even before this PR.
The public-file header pinned `sticky top-0`, which parks it inside the reserved lane and
under the lights. It now sticks below the lane, inert on web where the variable is `0px`.
Both audit gaps were real:
- The check was file-local, so it could not see the doubling above. It now resolves
ancestor layouts: a root counts as covered when it reserves OR sits inside a layout that
does, and reserving on both levels is its own failure. That also stops the check
demanding a second reservation from chat and the workspace overlays, which correctly
inherit theirs.
- Detection only matched `min-h-screen`/`h-screen`, so `fixed inset-0` roots never
entered it. Now included. With nesting understood, that addition resolved to a single
genuinely uncovered file rather than the ten it flagged beforehand.
Two allowlist entries added, both reasoned rather than assumed: the landing prefix (dozens
of files, one justification), and the desktop update gate — it centres its content, and
under `hiddenInset` macOS draws the lights above the web contents, so web UI cannot cover
them. This bug class is app chrome sitting under the lights, never the reverse.
Verified by mutation: reintroducing the double reservation fails the new check.
* test(desktop): do not credit inherited coverage across a layout's early return
Ancestor resolution is static, so it credits any file under a layout that mentions a
lane-aware shell. That is wrong when the layout returns the surface *instead of* its
chrome: `workspace/[workspaceId]/layout.tsx` returns `<WorkspaceAccessDenied />` at the
top and only reaches `<WorkspaceChrome>` far below, so at runtime the denied page has no
chrome at all. The page does reserve the lane today, but a regression would have read as
inherited and passed.
`SessionExpired` is deliberately not listed: it renders as a sibling within the chrome
tree, so its inherited coverage is real. The distinction is which side of the early return
the surface sits on, not which directory it lives in.
Verified by reverting the access-denied page exactly as described — it now fails.
* test(desktop): count the lane class itself as a viewport claim
Cursor caught the audit failing to watch exactly the files this PR converted. Detection
keyed on `min-h-screen`/`h-screen`/`fixed inset-0`, but converting a surface to
`.desktop-title-bar-page` removes those tokens — the class supplies `min-height: 100vh`
itself. So `/oauth-error`, the public-file view and `AuthShell` dropped out of the check
entirely, and a nested class-only reservation could ship green.
That also means the doubled-reservation check had never actually fired. The mutation I
used to "verify" it removed the lane component as well, so the pairing check caught it and
the doubled check was never exercised. It now fires on Cursor's exact scenario: a nested
class-only reservation, correctly paired, inside a lane-aware shell.
Pulling those files back in exposed a second-order bug: a shell's own definition file sits
under the layout that renders it, so ancestor resolution called `AuthShell` nested inside
itself. Shell definitions are excluded from inheritance.
One limit stays, documented rather than papered over: a root is in scope because of how it
claims the viewport, so deleting the reservation outright drops the file from the check.
That regression is loud, not silent — the surface stops being full height. Closing it
properly means treating every route entry point as a window root, which pulls in seven
account/organization/selfhost pages needing individual assessment. Worth doing separately;
allowlisting them on assumptions is the mistake that produced the `LogoShell` hole.
* fix(desktop): a fixed root escapes ancestor padding — chat drew under the lights
Cursor's sharpest catch, and the audit was actively hiding the bug rather than
missing it. `position: fixed` resolves against the viewport, not the parent, so a
lane-aware shell's `padding-top` never moves it. The chat surfaces sit inside
`LogoShell` and still painted at viewport top, under the traffic lights, while the
check reported them covered — and adding the correct reservation would then have
tripped the nested-reservation check, so the audit pushed toward the wrong answer.
Roots matching `fixed inset-0` no longer inherit coverage and are exempt from the
doubled check. That reclassified seven surfaces, each decided on evidence:
- chat, its loading boundary, the loading state and the voice interface are
full-window roots at `z-[100]` with their own top chrome — all now reserve;
- the file viewer wraps a full-bleed `<iframe>` whose content starts at viewport
top — now reserves;
- session-expired centres its content with nothing in the lane, and the search
modal's `fixed inset-0` is only its scrim (the panel sits at `top-[15%]`) — both
allowlisted with that reasoning rather than an assumption.
Verified by reverting chat to a bare `fixed inset-0` inside `LogoShell`: it is
flagged now and was called covered before.
* fix(desktop): stop the lane controller clobbering a mode another owner set
A regression I introduced last round. `DesktopTitleBarController` seeded `inset`
unconditionally on mount, before its own `getState()` resolved. That was harmless while
only `AuthShell` mounted it — but giving the file viewer a lane put a controller inside
workspace for the first time, exactly where this PR's own comments say `WorkspaceChrome`
owns the mode. Opening a file during native fullscreen therefore snapped the traffic-light
lane back on and jumped the content, and left it wrong permanently if `getState()`
rejected, since the rejection is swallowed.
It now seeds only when no owner has established a mode. The pre-paint script sets the
marker before first paint and `WorkspaceChrome` maintains it, so the unconditional write
was never the thing making the lane correct — it was only ever able to make it wrong.
Adds a controller test covering the case directly: mount during `fullscreen` with a
`getState` that never settles, and the mode survives. Verified it fails against the
previous behaviour.
* test(desktop): assert the lane on LogoShell directly, not via the sweep
Cursor found the one shell the enumeration could go silent on. `LogoShell` sits under the
`app/(landing)/` prefix allowlist, and this PR replaced its `min-h-screen` with
`desktop-title-bar-page` — so stripping the reservation also strips its last viewport
token and the sweep stops watching it. Doubly exempt, and it is the shell behind
not-found, the interfaces shell (chat, resume), the desktop handoff and the public-file
gates.
`AuthShell` already had a dedicated assertion; `LogoShell` now has the same. Verified by
stripping its reservation: caught now, silent before.
This is the documented enumeration limit made concrete rather than a new class of problem
— a root leaves the sweep when it stops claiming the viewport. The general fix is still to
treat route entry points as window roots, which pulls in seven unassessed pages and
belongs in its own change.
* docs(desktop): record why access-denied reserving is not a double reservation
Review read `WorkspaceHostProvider` as rendering the denied page inside the chrome. It is
the other way round: the provider wraps `<WorkspaceChrome>` in the layout, and its 403
branch returns the denied page instead of its children, so the chrome never mounts. The
server-side early return does the same thing higher up.
Both paths therefore bypass the chrome and the page's own reservation is the only one.
Recording that next to the entry, since the file lives under a workspace path where
inherited coverage is the norm and the reading is an easy one to repeat.1 parent ee0157d commit 64bcfea
20 files changed
Lines changed: 444 additions & 87 deletions
File tree
- apps/sim/app
- (auth)
- components
- reset-password
- signup
- (interfaces)
- chat
- [identifier]
- components
- loading-state
- voice-interface
- resume/[workflowId]/[executionId]
- (landing)/components/logo-shell
- _shell
- f/[token]
- oauth-error
- workspace
- [workspaceId]
- components
- files/[fileId]/view
This file was deleted.
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
1 | 1 | | |
2 | | - | |
3 | 2 | | |
| 3 | + | |
4 | 4 | | |
5 | 5 | | |
6 | 6 | | |
7 | 7 | | |
8 | 8 | | |
9 | 9 | | |
10 | 10 | | |
11 | | - | |
12 | | - | |
13 | 11 | | |
14 | 12 | | |
15 | 13 | | |
| |||
21 | 19 | | |
22 | 20 | | |
23 | 21 | | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
| 26 | + | |
| 27 | + | |
| 28 | + | |
| 29 | + | |
24 | 30 | | |
25 | | - | |
| 31 | + | |
26 | 32 | | |
27 | | - | |
28 | | - | |
29 | | - | |
30 | | - | |
31 | | - | |
32 | | - | |
33 | | - | |
34 | | - | |
35 | | - | |
| 33 | + | |
| 34 | + | |
36 | 35 | | |
37 | 36 | | |
38 | 37 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
1 | 1 | | |
2 | | - | |
| 2 | + | |
3 | 3 | | |
4 | 4 | | |
5 | 5 | | |
6 | 6 | | |
7 | 7 | | |
8 | 8 | | |
9 | | - | |
| 9 | + | |
10 | 10 | | |
Lines changed: 3 additions & 1 deletion
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
80 | 80 | | |
81 | 81 | | |
82 | 82 | | |
83 | | - | |
| 83 | + | |
| 84 | + | |
| 85 | + | |
84 | 86 | | |
85 | 87 | | |
86 | 88 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
490 | 490 | | |
491 | 491 | | |
492 | 492 | | |
493 | | - | |
| 493 | + | |
| 494 | + | |
| 495 | + | |
494 | 496 | | |
495 | 497 | | |
496 | 498 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
8 | 8 | | |
9 | 9 | | |
10 | 10 | | |
| 11 | + | |
11 | 12 | | |
12 | 13 | | |
13 | 14 | | |
| |||
440 | 441 | | |
441 | 442 | | |
442 | 443 | | |
443 | | - | |
| 444 | + | |
| 445 | + | |
444 | 446 | | |
445 | 447 | | |
446 | 448 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
1 | 1 | | |
| 2 | + | |
2 | 3 | | |
3 | 4 | | |
4 | 5 | | |
5 | | - | |
| 6 | + | |
| 7 | + | |
6 | 8 | | |
7 | 9 | | |
8 | 10 | | |
| |||
Lines changed: 3 additions & 1 deletion
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
1 | 1 | | |
| 2 | + | |
2 | 3 | | |
3 | 4 | | |
4 | 5 | | |
5 | | - | |
| 6 | + | |
| 7 | + | |
6 | 8 | | |
7 | 9 | | |
8 | 10 | | |
| |||
Lines changed: 3 additions & 1 deletion
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
14 | 14 | | |
15 | 15 | | |
16 | 16 | | |
| 17 | + | |
17 | 18 | | |
18 | 19 | | |
19 | 20 | | |
| |||
524 | 525 | | |
525 | 526 | | |
526 | 527 | | |
527 | | - | |
| 528 | + | |
528 | 529 | | |
529 | 530 | | |
530 | 531 | | |
| 532 | + | |
531 | 533 | | |
532 | 534 | | |
533 | 535 | | |
| |||
Lines changed: 1 addition & 1 deletion
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
2 | 2 | | |
3 | 3 | | |
4 | 4 | | |
5 | | - | |
| 5 | + | |
6 | 6 | | |
7 | 7 | | |
8 | 8 | | |
| |||
0 commit comments