mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-31 19:16:29 +00:00
fix(desktop): stop an unowned publisher poisoning the thread clearance at :root
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.
This commit is contained in:
parent
92856bc28a
commit
800c8a0458
4 changed files with 91 additions and 63 deletions
|
|
@ -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 () => {
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
||||
|
|
|
|||
|
|
@ -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('')
|
||||
})
|
||||
})
|
||||
|
|
|
|||
|
|
@ -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<HTMLElement>('[data-chat-surface]') ?? document.documentElement
|
||||
export function chatSurfaceRoot(el: Element | null): HTMLElement | null {
|
||||
return el?.closest<HTMLElement>('[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)
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue