From 800c8a0458cb6c6d4ef96c5ed0de04d85a06bbc5 Mon Sep 17 00:00:00 2001 From: Brooklyn Nicholson Date: Wed, 29 Jul 2026 17:57:49 -0500 Subject: [PATCH] fix(desktop): stop an unowned publisher poisoning the thread clearance at :root MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The thread's bottom clearance is composer + status-stack + 2rem, and both inputs are measured by JS onto the owning [data-chat-surface]. The surface-var helpers fell back to document.documentElement when they couldn't resolve one — but :root is where every surface's DEFAULTS live, so a single stale write there becomes a global floor under every thread's clearance until reload. An unowned publisher has nowhere to publish to, so it now publishes nowhere. --- .../composer/hooks/use-composer-metrics.ts | 6 +- .../app/chat/composer/status-stack/index.tsx | 9 +- .../desktop/src/app/chat/surface-vars.test.ts | 107 +++++++++++------- apps/desktop/src/app/chat/surface-vars.ts | 32 ++++-- 4 files changed, 91 insertions(+), 63 deletions(-) diff --git a/apps/desktop/src/app/chat/composer/hooks/use-composer-metrics.ts b/apps/desktop/src/app/chat/composer/hooks/use-composer-metrics.ts index 859e70456c9..685644691ef 100644 --- a/apps/desktop/src/app/chat/composer/hooks/use-composer-metrics.ts +++ b/apps/desktop/src/app/chat/composer/hooks/use-composer-metrics.ts @@ -170,10 +170,8 @@ export function useComposerMetrics({ composerRef, composerSurfaceRef, editorRef, useEffect(() => { // Resolve the owning surface while the composer is still attached; the - // unmount cleanup runs after React detached the node, where closest() - // can no longer find [data-chat-surface] and would clear the document - // root instead of this surface (same class of bug as the status stack's - // stale-clearance leak). + // unmount cleanup runs after React detached the node, where closest() can + // no longer find [data-chat-surface]. const root = chatSurfaceRoot(composerRef.current) return () => { diff --git a/apps/desktop/src/app/chat/composer/status-stack/index.tsx b/apps/desktop/src/app/chat/composer/status-stack/index.tsx index 6d526a8dcf6..f3b0cf4f365 100644 --- a/apps/desktop/src/app/chat/composer/status-stack/index.tsx +++ b/apps/desktop/src/app/chat/composer/status-stack/index.tsx @@ -236,12 +236,9 @@ export function ComposerStatusStack({ queue, sessionId }: ComposerStatusStackPro return } - // Resolve the owning surface NOW, while the node is attached. The cleanup - // below runs after the stack collapsed and React removed the div, so - // closest() from the detached node misses [data-chat-surface] and would - // clear the document root instead — leaving the stale height on the - // surface, which keeps inflating the thread's bottom clearance until the - // next publish. + // Resolve the owning surface NOW, while the node is attached: the cleanup + // below runs after the stack collapsed and React removed the div, and a + // detached node can no longer find its [data-chat-surface]. const root = chatSurfaceRoot(el) let last = -1 diff --git a/apps/desktop/src/app/chat/surface-vars.test.ts b/apps/desktop/src/app/chat/surface-vars.test.ts index 438d36c75be..0f0acadcbdd 100644 --- a/apps/desktop/src/app/chat/surface-vars.test.ts +++ b/apps/desktop/src/app/chat/surface-vars.test.ts @@ -1,68 +1,89 @@ -import { describe, expect, it } from 'vitest' +import { afterEach, describe, expect, it } from 'vitest' import { chatSurfaceRoot, clearSurfaceVar, setSurfaceVar, STATUS_STACK_VAR } from './surface-vars' /** - * Measured-height vars must be scoped per chat surface. Session tiles render a - * full ChatView — thread, composer, and status stack — beside the workspace - * pane, so a single global slot let a background tab's tall status stack - * inflate the foreground thread's bottom clearance and shove its - * jump-to-bottom button into mid-screen. + * Regression: the thread's bottom gap going randomly huge and staying huge. + * + * `--thread-last-message-clearance` is `composer + status-stack + 2rem`, and + * both inputs are published by JS onto the owning `[data-chat-surface]`. The + * helpers used to fall back to `document.documentElement` when they couldn't + * find a surface — which is exactly the state a publisher is in once React has + * detached it. `:root` is where every surface's DEFAULTS live, so one such + * write becomes a global floor under every thread's clearance, inherited by + * every tab, until reload. Observed in the wild as + * `calc(56px + 176px + 2rem)`: a 176px status stack that had been gone for + * minutes, on a session that never had one. */ -function surface(): { root: HTMLElement; stack: HTMLElement } { - const root = document.createElement('div') - root.setAttribute('data-chat-surface', '') +describe('surface measured-height vars', () => { + afterEach(() => { + document.documentElement.style.removeProperty(STATUS_STACK_VAR) + document.body.replaceChildren() + }) - const stack = document.createElement('div') - root.append(stack) - document.body.append(root) + function surface() { + const root = document.createElement('div') + root.setAttribute('data-chat-surface', '') - return { root, stack } -} + const publisher = document.createElement('div') + root.append(publisher) + document.body.append(root) -describe('per-surface measured-height vars', () => { - it('publishes onto the owning chat surface, not the document root', () => { - const { root, stack } = surface() + return { publisher, root } + } - setSurfaceVar(stack, STATUS_STACK_VAR, '96px') + it('publishes onto the owning surface, not the document root', () => { + const { publisher, root } = surface() - expect(root.style.getPropertyValue(STATUS_STACK_VAR)).toBe('96px') + setSurfaceVar(publisher, STATUS_STACK_VAR, '176px') + + expect(root.style.getPropertyValue(STATUS_STACK_VAR)).toBe('176px') expect(document.documentElement.style.getPropertyValue(STATUS_STACK_VAR)).toBe('') }) - it('keeps two visible surfaces independent', () => { - const background = surface() - const foreground = surface() + it('never poisons the document root from a detached publisher', () => { + const { publisher } = surface() - setSurfaceVar(background.stack, STATUS_STACK_VAR, '160px') - setSurfaceVar(foreground.stack, STATUS_STACK_VAR, '0px') + // How the real leak happens: React removes the stack/composer div when it + // collapses, orphaning it from its surface, and a pending measurement + // publishes anyway. `closest()` from an orphan finds nothing. + publisher.remove() + setSurfaceVar(publisher, STATUS_STACK_VAR, '176px') - expect(background.root.style.getPropertyValue(STATUS_STACK_VAR)).toBe('160px') - expect(foreground.root.style.getPropertyValue(STATUS_STACK_VAR)).toBe('0px') + expect(document.documentElement.style.getPropertyValue(STATUS_STACK_VAR)).toBe('') }) - it('clearing one surface leaves the other intact', () => { - const a = surface() - const b = surface() - - setSurfaceVar(a.stack, STATUS_STACK_VAR, '48px') - setSurfaceVar(b.stack, STATUS_STACK_VAR, '80px') - clearSurfaceVar(a.stack, STATUS_STACK_VAR) - - expect(a.root.style.getPropertyValue(STATUS_STACK_VAR)).toBe('') - expect(b.root.style.getPropertyValue(STATUS_STACK_VAR)).toBe('80px') - }) - - it('falls back to the document root outside a chat surface (popped-out composer)', () => { + it('never poisons the document root from a publisher outside any surface', () => { const orphan = document.createElement('div') document.body.append(orphan) - expect(chatSurfaceRoot(orphan)).toBe(document.documentElement) + setSurfaceVar(orphan, STATUS_STACK_VAR, '176px') - setSurfaceVar(orphan, STATUS_STACK_VAR, '24px') - expect(document.documentElement.style.getPropertyValue(STATUS_STACK_VAR)).toBe('24px') + expect(document.documentElement.style.getPropertyValue(STATUS_STACK_VAR)).toBe('') + }) - clearSurfaceVar(orphan, STATUS_STACK_VAR) + it('reports no owner for an orphaned or unowned node', () => { + const { publisher, root } = surface() + expect(chatSurfaceRoot(publisher)).toBe(root) + + // A subtree detached whole still resolves — the surface is still an + // ancestor. Only losing the surface from the chain orphans the publisher. + root.remove() + expect(chatSurfaceRoot(publisher)).toBe(root) + + publisher.remove() + expect(chatSurfaceRoot(publisher)).toBeNull() + expect(chatSurfaceRoot(null)).toBeNull() + }) + + it('clears from the captured surface and tolerates a missing one', () => { + const { publisher, root } = surface() + setSurfaceVar(publisher, STATUS_STACK_VAR, '176px') + + clearSurfaceVar(root, STATUS_STACK_VAR) + expect(root.style.getPropertyValue(STATUS_STACK_VAR)).toBe('') + + expect(() => clearSurfaceVar(null, STATUS_STACK_VAR)).not.toThrow() expect(document.documentElement.style.getPropertyValue(STATUS_STACK_VAR)).toBe('') }) }) diff --git a/apps/desktop/src/app/chat/surface-vars.ts b/apps/desktop/src/app/chat/surface-vars.ts index 5176dc11375..16c972e19d3 100644 --- a/apps/desktop/src/app/chat/surface-vars.ts +++ b/apps/desktop/src/app/chat/surface-vars.ts @@ -14,6 +14,14 @@ * read its own measurement through normal CSS inheritance. Remeasuring on * session switch cannot fix this — several surfaces are visible at once, so * there is no single correct value to remeasure to. + * + * A measurement is only ever meaningful to the surface that owns it, so an + * unowned node has nowhere to publish to and must publish NOWHERE. Writing to + * the document root instead is not a graceful fallback: `:root` holds the + * defaults every surface inherits until it publishes its own, so one stale + * write there is a global floor under every thread's bottom clearance, on + * every tab, until reload. That is the "randomly huge gap at the bottom of the + * thread" bug — see surface-vars.test.ts. */ export const COMPOSER_HEIGHT_VAR = '--composer-measured-height' @@ -21,20 +29,24 @@ export const COMPOSER_SURFACE_HEIGHT_VAR = '--composer-surface-measured-height' export const STATUS_STACK_VAR = '--status-stack-measured-height' /** - * The surface owning `el`, falling back to the document root so a composer - * rendered outside a chat surface (the popped-out window) keeps behaving - * exactly as before. + * The surface owning `el`, or null when `el` is detached or outside one. + * + * Null is the honest answer, not a failure to be papered over. Every publisher + * lives inside a `[data-chat-surface]` for its whole visible life (a + * popped-out composer is `position: fixed`, still a descendant), so the only + * way to miss is a node React already removed — whose measurement is stale by + * definition. */ -export function chatSurfaceRoot(el: Element | null): HTMLElement { - return el?.closest('[data-chat-surface]') ?? document.documentElement +export function chatSurfaceRoot(el: Element | null): HTMLElement | null { + return el?.closest('[data-chat-surface]') ?? null } -/** Publish a measured-height var on the surface owning `el`. */ +/** Publish a measured-height var on the surface owning `el`. No owner, no write. */ export function setSurfaceVar(el: Element | null, name: string, value: string): void { - chatSurfaceRoot(el).style.setProperty(name, value) + chatSurfaceRoot(el)?.style.setProperty(name, value) } -/** Clear a measured-height var from the surface owning `el`. */ -export function clearSurfaceVar(el: Element | null, name: string): void { - chatSurfaceRoot(el).style.removeProperty(name) +/** Clear a measured-height var from `root`. */ +export function clearSurfaceVar(root: HTMLElement | null, name: string): void { + root?.style.removeProperty(name) }