From aa0465a309202c389845efb5167020f4b81213a4 Mon Sep 17 00:00:00 2001 From: Brooklyn Nicholson Date: Tue, 28 Jul 2026 15:34:15 -0500 Subject: [PATCH] fix(desktop): clear surface height vars from the attached surface, not a detached node MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When the status stack collapses (last subagent/background/queue item finishes), React removes the stack div before the layout-effect cleanup runs. Resolving the surface via closest('[data-chat-surface]') from that now-detached node falls back to the document root, so the stale measured height was cleared from the wrong element and stayed on the surface — inflating --thread-last-message-clearance (the thread's bottom padding) until the next publish. Same latent hazard in the composer's unmount cleanup. Capture the surface root while the node is still attached and clear from it directly. --- .../composer/hooks/use-composer-metrics.ts | 12 ++- .../app/chat/composer/status-stack/index.tsx | 13 ++- .../surface-var-lifecycle.test.tsx | 94 +++++++++++++++++++ 3 files changed, 112 insertions(+), 7 deletions(-) create mode 100644 apps/desktop/src/app/chat/composer/status-stack/surface-var-lifecycle.test.tsx 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 771bf2a9dd39..859e70456c91 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 @@ -2,6 +2,7 @@ import { useAuiState } from '@assistant-ui/react' import { type RefObject, useCallback, useEffect, useRef, useState } from 'react' import { + chatSurfaceRoot, clearSurfaceVar, COMPOSER_HEIGHT_VAR, COMPOSER_SURFACE_HEIGHT_VAR, @@ -168,11 +169,16 @@ export function useComposerMetrics({ composerRef, composerSurfaceRef, editorRef, }, [poppedOut, syncComposerMetrics]) useEffect(() => { - const composer = composerRef.current + // 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). + const root = chatSurfaceRoot(composerRef.current) return () => { - clearSurfaceVar(composer, COMPOSER_HEIGHT_VAR) - clearSurfaceVar(composer, COMPOSER_SURFACE_HEIGHT_VAR) + clearSurfaceVar(root, COMPOSER_HEIGHT_VAR) + clearSurfaceVar(root, COMPOSER_SURFACE_HEIGHT_VAR) } }, [composerRef]) 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 fbbd2b94d5aa..41c84bfdaace 100644 --- a/apps/desktop/src/app/chat/composer/status-stack/index.tsx +++ b/apps/desktop/src/app/chat/composer/status-stack/index.tsx @@ -3,7 +3,7 @@ import { type ReactNode, useEffect, useLayoutEffect, useMemo, useRef } from 'rea import { useNavigate } from 'react-router-dom' import { blurComposerInput } from '@/app/chat/composer/focus' -import { clearSurfaceVar, setSurfaceVar, STATUS_STACK_VAR } from '@/app/chat/surface-vars' +import { chatSurfaceRoot, clearSurfaceVar, setSurfaceVar, STATUS_STACK_VAR } from '@/app/chat/surface-vars' import { AGENTS_ROUTE } from '@/app/routes' import { BillingBanner } from '@/components/billing-banner' import { composerDockCard } from '@/components/chat/composer-dock' @@ -220,11 +220,16 @@ export function ComposerStatusStack({ queue, sessionId }: ComposerStatusStackPro const el = stackRef.current if (!visible || !el) { - clearSurfaceVar(el, STATUS_STACK_VAR) - 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. + const root = chatSurfaceRoot(el) let last = -1 const sync = () => { @@ -242,7 +247,7 @@ export function ComposerStatusStack({ queue, sessionId }: ComposerStatusStackPro return () => { observer.disconnect() - clearSurfaceVar(el, STATUS_STACK_VAR) + clearSurfaceVar(root, STATUS_STACK_VAR) } }, [visible]) diff --git a/apps/desktop/src/app/chat/composer/status-stack/surface-var-lifecycle.test.tsx b/apps/desktop/src/app/chat/composer/status-stack/surface-var-lifecycle.test.tsx new file mode 100644 index 000000000000..a01563aedc34 --- /dev/null +++ b/apps/desktop/src/app/chat/composer/status-stack/surface-var-lifecycle.test.tsx @@ -0,0 +1,94 @@ +import { act, cleanup, render } from '@testing-library/react' +import { MemoryRouter } from 'react-router-dom' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +import { STATUS_STACK_VAR } from '@/app/chat/surface-vars' +import { I18nProvider } from '@/i18n' +import { $goalsBySession, type SessionGoal } from '@/store/goals' + +import { ComposerStatusStack } from './index' + +// The stack measures itself into a surface var — jsdom has no ResizeObserver. +class ResizeObserverStub { + observe() {} + unobserve() {} + disconnect() {} +} + +vi.stubGlobal('ResizeObserver', ResizeObserverStub) + +const SID = 'sess-height-1' + +const goal = (): SessionGoal => ({ status: 'active', title: 'ship the feature', updatedAt: Date.now() }) + +/** + * Regression: when the stack collapses (its last item finishes), React removes + * the stack div BEFORE the layout-effect cleanup runs. Resolving the surface + * from the ref at cleanup time then walks a DETACHED node, misses + * [data-chat-surface], and clears the document root instead — the stale height + * stays on the surface and keeps inflating the thread's bottom clearance + * (`--thread-last-message-clearance`) until the next publish. The effect must + * capture its surface root while the node is still attached. + */ +describe('ComposerStatusStack surface-var lifecycle', () => { + beforeEach(() => { + $goalsBySession.set({}) + }) + + afterEach(() => { + cleanup() + $goalsBySession.set({}) + document.documentElement.style.removeProperty(STATUS_STACK_VAR) + }) + + function renderOnSurface() { + const surface = document.createElement('div') + surface.setAttribute('data-chat-surface', '') + document.body.append(surface) + + const view = render( + + + + + , + { container: surface } + ) + + return { surface, view } + } + + it('publishes its measured height onto the owning surface while visible', () => { + $goalsBySession.set({ [SID]: goal() }) + + const { surface } = renderOnSurface() + + // jsdom measures 0 — the value is irrelevant, the target element is not. + expect(surface.style.getPropertyValue(STATUS_STACK_VAR)).toBe('0px') + expect(document.documentElement.style.getPropertyValue(STATUS_STACK_VAR)).toBe('') + }) + + it('clears the surface var when the stack collapses to nothing', () => { + $goalsBySession.set({ [SID]: goal() }) + + const { surface } = renderOnSurface() + expect(surface.style.getPropertyValue(STATUS_STACK_VAR)).toBe('0px') + + // Last status item goes away → the component renders null and React + // detaches the stack div before the cleanup runs. + act(() => $goalsBySession.set({})) + + expect(surface.style.getPropertyValue(STATUS_STACK_VAR)).toBe('') + }) + + it('clears the surface var on unmount', () => { + $goalsBySession.set({ [SID]: goal() }) + + const { surface, view } = renderOnSurface() + expect(surface.style.getPropertyValue(STATUS_STACK_VAR)).toBe('0px') + + view.unmount() + + expect(surface.style.getPropertyValue(STATUS_STACK_VAR)).toBe('') + }) +})