mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-29 18:46:59 +00:00
fix(desktop): clear surface height vars from the attached surface, not a detached node (#73638)
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.
This commit is contained in:
commit
ed5fd3503d
3 changed files with 112 additions and 7 deletions
|
|
@ -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])
|
||||
|
||||
|
|
|
|||
|
|
@ -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])
|
||||
|
||||
|
|
|
|||
|
|
@ -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(
|
||||
<MemoryRouter>
|
||||
<I18nProvider configClient={null} initialLocale="en">
|
||||
<ComposerStatusStack queue={null} sessionId={SID} />
|
||||
</I18nProvider>
|
||||
</MemoryRouter>,
|
||||
{ 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('')
|
||||
})
|
||||
})
|
||||
Loading…
Add table
Add a link
Reference in a new issue