From 64bcfead343e42a65249c5986b4839070ae205e7 Mon Sep 17 00:00:00 2001 From: Waleed Date: Thu, 30 Jul 2026 17:55:16 -0700 Subject: [PATCH] fix(desktop): clear the traffic lights on every full-viewport surface, and enumerate them in CI (#6109) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 (`/`, 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 `` at the top and only reaches `` 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 `