Skip to content

Commit 163b184

Browse files
committed
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.
1 parent d99f868 commit 163b184

3 files changed

Lines changed: 81 additions & 22 deletions

File tree

apps/sim/app/(interfaces)/resume/[workflowId]/[executionId]/loading.tsx

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,8 @@
11
import { Skeleton } from '@sim/emcn'
2-
import { DesktopTitleBarLane } from '@/app/_shell/desktop-title-bar'
32

43
export default function ResumeLoading() {
54
return (
6-
<div className='desktop-title-bar-page bg-background'>
7-
<DesktopTitleBarLane />
5+
<div className='bg-background'>
86
<div className='border-b px-4 py-3'>
97
<div className='mx-auto flex max-w-[1200px] items-center justify-between'>
108
<Skeleton className='h-[24px] w-[80px] rounded-[4px]' />

apps/sim/app/_shell/desktop-title-bar-surfaces.test.ts

Lines changed: 79 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -186,6 +186,8 @@ const LANE_EXEMPT: Record<string, string> = {
186186
'Marketing chrome. Verified: every consumer lives under app/(landing)/, and the desktop shell boots to /login or a workspace with no path to those routes.',
187187
'app/playground/page.tsx':
188188
'Verified dev-only: the page calls notFound() unless NEXT_PUBLIC_ENABLE_PLAYGROUND is set.',
189+
'app/_shell/desktop-update-gate.tsx':
190+
'Blocking overlay that centres its content, with no chrome in the lane. The lights stay usable regardless: under `titleBarStyle: hiddenInset` macOS draws them above the web contents, so web UI cannot cover them — this bug class is app chrome sitting *under* the lights, not the reverse.',
189191
'app/workspace/[workspaceId]/w/[workflowId]/components/error/index.tsx':
190192
'Renders <Sidebar>, which owns the workspace lane and its drag region (sidebar.tsx). Padding this root too would double the reservation.',
191193
}
@@ -197,31 +199,90 @@ const LANE_EXEMPT: Record<string, string> = {
197199
* shell's own definition file mentioning its name, would otherwise self-certify as
198200
* covered. Both mistakes were in the first draft of this check and made it unfailable.
199201
*/
200-
const LANE_AWARE_SHELL_USAGE = /<(AuthShell|LogoShell|WorkspaceChrome)\b/
202+
/**
203+
* How a root claims the whole window. `fixed inset-0` counts: it covers the lights just as
204+
* completely as `h-screen`, and was invisible to the first version of this check.
205+
*/
206+
const FILLS_VIEWPORT = /\b(min-h-screen|h-screen)\b|fixed inset-0/
201207

202-
describe('desktop traffic-light lane coverage', () => {
203-
it('leaves no full-viewport root outside workspace chrome unaccounted for', () => {
204-
const appDir = new URL('../', import.meta.url)
205-
const files = readdirSync(appDir, { recursive: true, encoding: 'utf8' })
206-
.filter((f) => f.endsWith('.tsx'))
207-
.map((f) => `app/${f}`)
208+
const LANE_AWARE_SHELL_USAGE = /<(AuthShell|LogoShell|WorkspaceChrome|InterfacesShell)\b/
209+
210+
/**
211+
* Route trees whose every surface is accounted for by one reason.
212+
*
213+
* Prefix form exists because the landing group is dozens of files sharing a single
214+
* justification; listing them individually would be noise, not scrutiny.
215+
*/
216+
const LANE_EXEMPT_PREFIXES: Record<string, string> = {
217+
'app/(landing)/':
218+
'Marketing routes and their overlays. Verified: the desktop shell boots to /login or a workspace and has no navigation path here. `logo-shell.tsx` lives under this prefix but is lane-aware on its own merits, since it is used well outside landing.',
219+
}
220+
221+
const isExempt = (file: string) =>
222+
file in LANE_EXEMPT || Object.keys(LANE_EXEMPT_PREFIXES).some((prefix) => file.startsWith(prefix))
208223

209-
const unaccounted = files.filter((file) => {
210-
const source = read(`../${file.slice('app/'.length)}`)
211-
const fillsViewport = /\b(min-h-screen|h-screen)\b/.test(source)
212-
if (!fillsViewport) return false
213-
// Composition counts: a surface is covered if it wears a shell that reserves the
214-
// lane. `LogoShell` was previously allowlisted as marketing chrome — a claim that
215-
// was simply false, and it hid not-found, the interfaces shell, the desktop handoff
216-
// shell and the public-file access gates behind one wrong sentence.
217-
const laneAware =
218-
source.includes('desktop-title-bar-page') || LANE_AWARE_SHELL_USAGE.test(source)
219-
return !laneAware && !(file in LANE_EXEMPT)
224+
/** Every file under `app/`, so ancestor layouts can be resolved without extra fs calls. */
225+
const ALL_APP_FILES = new Set(
226+
readdirSync(new URL('../', import.meta.url), { recursive: true, encoding: 'utf8' }).map(
227+
(f) => `app/${f}`
228+
)
229+
)
230+
231+
/** The `layout.tsx` files wrapping a route, nearest first. */
232+
function ancestorLayouts(file: string): string[] {
233+
const parts = file.split('/')
234+
const layouts: string[] = []
235+
for (let i = parts.length - 1; i > 0; i--) {
236+
const candidate = `${parts.slice(0, i).join('/')}/layout.tsx`
237+
if (candidate !== file && ALL_APP_FILES.has(candidate)) layouts.push(candidate)
238+
}
239+
return layouts
240+
}
241+
242+
const reservesLane = (source: string) =>
243+
source.includes('desktop-title-bar-page') || LANE_AWARE_SHELL_USAGE.test(source)
244+
245+
const sourceOf = (file: string) => read(`../${file.slice('app/'.length)}`)
246+
247+
/** Full-viewport roots outside a lane-aware layout, paired with whether they reserve. */
248+
function viewportRoots() {
249+
return [...ALL_APP_FILES]
250+
.filter((f) => f.endsWith('.tsx'))
251+
.map((file) => {
252+
const source = sourceOf(file)
253+
return {
254+
file,
255+
fillsViewport: FILLS_VIEWPORT.test(source),
256+
self: reservesLane(source),
257+
inherited: ancestorLayouts(file).some((l) => reservesLane(sourceOf(l))),
258+
}
220259
})
260+
.filter((r) => r.fillsViewport)
261+
}
262+
263+
describe('desktop traffic-light lane coverage', () => {
264+
it('leaves no full-viewport root unaccounted for', () => {
265+
// Covered means the root reserves the lane itself OR sits inside a layout that does —
266+
// chat and the workspace overlays inherit theirs, and requiring the file itself to
267+
// reserve would force a second, doubled reservation on every one of them.
268+
const unaccounted = viewportRoots()
269+
.filter((r) => !r.self && !r.inherited && !isExempt(r.file))
270+
.map((r) => r.file)
221271

222272
expect(unaccounted).toEqual([])
223273
})
224274

275+
it('never reserves the lane twice by nesting', () => {
276+
// A root inside a lane-aware layout that reserves again gets two lots of padding, two
277+
// drag strips and two controllers. Shipped exactly that on the resume loading skeleton,
278+
// and the file-local check could not see it.
279+
const doubled = viewportRoots()
280+
.filter((r) => r.self && r.inherited)
281+
.map((r) => r.file)
282+
283+
expect(doubled).toEqual([])
284+
})
285+
225286
it('never reserves the lane without also making it draggable', () => {
226287
const appDir = new URL('../', import.meta.url)
227288
const orphaned = readdirSync(appDir, { recursive: true, encoding: 'utf8' })

apps/sim/app/f/[token]/public-file-view.tsx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,7 @@ export function PublicFileView({
6868
return (
6969
<div className='light desktop-title-bar-page flex flex-col bg-[var(--bg)]'>
7070
<DesktopTitleBarLane />
71-
<header className='sticky top-0 z-10 flex items-center justify-between gap-4 border-[var(--border)] border-b bg-[var(--bg)] px-4 py-3'>
71+
<header className='sticky top-[var(--desktop-title-bar-height)] z-10 flex items-center justify-between gap-4 border-[var(--border)] border-b bg-[var(--bg)] px-4 py-3'>
7272
<div className='flex min-w-0 items-center gap-3'>
7373
{!brand.logoUrl && (
7474
<>

0 commit comments

Comments
 (0)