Merge pull request #456 from wang-kaopu/fix/issue-403-sidebar-locate-performance

perf(sidebar): reduce repeated center-column DOM queries

src/client/Sidebar.tsx

@@ -47,6 +47,7 @@ import { IconPanelBottomOutline16, IconPanelRightOutline16 } from './icons.tsx'
 import { Workbench, type WorkbenchActions } from './split-pane.tsx'
 import { isNarrowWidth, useViewportSize } from './breakpoints.ts'
 import { layoutPushSize } from './layout-push.ts'
+import { resolveCenterColumn } from './center-column.ts'
 import { parseDesktopEnv } from './desktop-env.ts'
 import { getWcoSnapshot, subscribeWco } from './wco.ts'
 import { getShellPreset } from './shell-presets.ts'
@@ -752,8 +753,10 @@ export function Sidebar(props: { ctx: Context; store: SidebarStore }) {
     // (observed: a 1px sliver at the viewport's left edge).
     const locate = (): void => {
       if (disposed) return
-      const col = document.querySelector('#root [data-slot="conversation"]')
-        ?.parentElement as HTMLElement | undefined
+      // Hot path (#403): streaming output mutates #root at token cadence.
+      // Reuse a still-connected center column and only query after boot/HMR
+      // detached the cached node.
+      const col = resolveCenterColumn(centerColRef.current)
       if (col === undefined || !col.isConnected) {
         if (centerColRef.current !== null) {
           centerColRef.current.removeAttribute('data-dsh-center-col')
@@ -791,7 +794,7 @@ export function Sidebar(props: { ctx: Context; store: SidebarStore }) {
       // Mid-drag every frame writes --dsh-sidebar-* on <html>'s style
       // attribute, which is the mutation this watcher observes — relocating
       // per drag frame is pointless (the center column node cannot change
-      // while the pointer is captured) and adds a querySelector to every
+      // while the pointer is captured) and adds locator work to every
       // frame's budget (#315). The 1.5s retry below still covers any node
       // swap that somehow lands mid-drag.
       if (draggingRef.current) return
@@ -818,8 +821,9 @@ export function Sidebar(props: { ctx: Context; store: SidebarStore }) {
     // may end up byte-identical, and the col may be swapped before the
     // subtree watcher attaches). A slow unconditional re-locate makes the
     // panel converge on the real column within a couple of seconds no
-    // matter what sequence the shell used. locate() is cheap when nothing
-    // changed (one querySelector + an identity compare; no forced layout).
+    // matter what sequence the shell used. locate() is query-free while the
+    // cached column stays connected; only a detached/missing cache falls
+    // back to the document selector.
     const retry = window.setInterval(locate, 1500)
     return () => {
       disposed = true

src/client/center-column.ts

@@ -0,0 +1,82 @@
+/** Stable selector for the DSH AppFrame conversation slot. */
+const CENTER_COLUMN_SELECTOR = '#root [data-slot="conversation"]'
+
+/** Match Sidebar's existing last-resort retry cadence (issue #248). */
+export const CENTER_COLUMN_REVALIDATE_MS = 1500
+
+interface CenterColumnResolveOptions {
+  /** Explicit document for tests / callers that do not yet have a cached node. */
+  document?: Document
+  /** Injectable DOM lookup for tests. */
+  query?: () => Element | null
+  /** Injectable clock for deterministic retry-boundary tests. */
+  now?: () => number
+  /** Injectable <html style> fingerprint for deterministic HMR tests. */
+  htmlStyle?: () => string | null
+  /** Override only for tests; production follows the 1.5s safety-net cadence. */
+  revalidateMs?: number
+}
+
+/** Last full DOM validation for each center-column node. Weak keys avoid leaks. */
+const validatedAt = new WeakMap<HTMLElement, number>()
+
+/**
+ * The html-style watcher is an HMR/layout resync signal. Remember the last
+ * fingerprint per document so a style change can bypass the connected-node
+ * fast path immediately, without requiring Sidebar to run a second locator.
+ */
+const documentStyleState = new WeakMap<Document, string | null>()
+
+/**
+ * Resolve the AppFrame center column while keeping streaming mutations cheap.
+ *
+ * Sidebar intentionally keeps both recovery mechanisms that predate #403:
+ * the `#root` subtree MutationObserver catches boot/HMR DOM swaps, and the
+ * 1.5s interval is a last-resort safety net for interleavings no observer is
+ * guaranteed to see (#248). The expensive part was letting every scheduled
+ * locate scan `#root` with querySelector at streaming-token cadence.
+ *
+ * Once a connected column is cached, normal calls therefore reuse it without
+ * a document query. A full query is still forced when either:
+ * - the cached node disconnects;
+ * - `<html style>` changes (the existing HMR/layout resync signal); or
+ * - 1.5s has elapsed since the last full validation (the safety-net cadence).
+ *
+ * This preserves recovery semantics while bounding whole-tree selector work
+ * to low-frequency validation instead of chat mutation frequency.
+ */
+export function resolveCenterColumn(
+  current: HTMLElement | null,
+  options: CenterColumnResolveOptions = {},
+): HTMLElement | undefined {
+  const doc = options.document ?? current?.ownerDocument ?? document
+  const now = (options.now ?? Date.now)()
+  const revalidateMs = options.revalidateMs ?? CENTER_COLUMN_REVALIDATE_MS
+  const htmlStyle = (options.htmlStyle ?? (() => doc.documentElement.getAttribute('style')))()
+
+  const previousStyle = documentStyleState.get(doc)
+  const styleChanged = previousStyle !== undefined && previousStyle !== htmlStyle
+  documentStyleState.set(doc, htmlStyle)
+
+  if (current !== null && current.isConnected) {
+    const lastValidation = validatedAt.get(current)
+
+    // A connected node passed in for the first time is already a trustworthy
+    // cache hit. Start its safety-net window without paying an eager query.
+    if (lastValidation === undefined && !styleChanged) {
+      validatedAt.set(current, now)
+      return current
+    }
+
+    if (!styleChanged && lastValidation !== undefined && now - lastValidation < revalidateMs) {
+      return current
+    }
+  }
+
+  const query = options.query ?? (() => doc.querySelector(CENTER_COLUMN_SELECTOR))
+  const col = query()?.parentElement as HTMLElement | null | undefined
+  if (col === null || col === undefined || !col.isConnected) return undefined
+
+  validatedAt.set(col, now)
+  return col
+}

tests/center-column-locator.spec.ts

@@ -0,0 +1,103 @@
+// @vitest-environment jsdom
+import { describe, expect, it, vi } from 'vitest'
+import {
+  CENTER_COLUMN_REVALIDATE_MS,
+  resolveCenterColumn,
+} from '../src/client/center-column.ts'
+
+interface Fixture {
+  doc: Document
+  col: HTMLDivElement
+  slot: HTMLDivElement
+}
+
+function connectedColumn(doc = document.implementation.createHTMLDocument()): Fixture {
+  const col = doc.createElement('div')
+  const slot = doc.createElement('div')
+  slot.dataset.slot = 'conversation'
+  col.append(slot)
+  doc.body.append(col)
+  return { doc, col, slot }
+}
+
+describe('center-column locator (issue #403)', () => {
+  it('reuses a connected cached column without querying during the hot-path window', () => {
+    const { doc, col, slot } = connectedColumn()
+    let now = 0
+    const query = vi.fn<() => Element | null>(() => slot)
+    const options = { document: doc, query, now: () => now }
+
+    expect(resolveCenterColumn(col, options)).toBe(col)
+    now = CENTER_COLUMN_REVALIDATE_MS - 1
+    expect(resolveCenterColumn(col, options)).toBe(col)
+    expect(query).not.toHaveBeenCalled()
+  })
+
+  it('fully revalidates a connected cache at the 1.5s safety-net boundary', () => {
+    const { doc, col, slot } = connectedColumn()
+    let now = 0
+    const query = vi.fn<() => Element | null>(() => slot)
+    const options = { document: doc, query, now: () => now }
+
+    expect(resolveCenterColumn(col, options)).toBe(col)
+    expect(query).not.toHaveBeenCalled()
+
+    now = CENTER_COLUMN_REVALIDATE_MS
+    expect(resolveCenterColumn(col, options)).toBe(col)
+    expect(query).toHaveBeenCalledTimes(1)
+  })
+
+  it('revalidates immediately when the html style signal changes', () => {
+    const { doc, col, slot } = connectedColumn()
+    let now = 0
+    const query = vi.fn<() => Element | null>(() => slot)
+    const options = { document: doc, query, now: () => now }
+
+    expect(resolveCenterColumn(col, options)).toBe(col)
+    expect(query).not.toHaveBeenCalled()
+
+    doc.documentElement.style.setProperty('--dsh-sidebar-width', '320px')
+    now = 1
+    expect(resolveCenterColumn(col, options)).toBe(col)
+    expect(query).toHaveBeenCalledTimes(1)
+
+    // The same fingerprint is cheap again; one style change causes one full
+    // validation instead of turning later streaming mutations into queries.
+    now = 2
+    expect(resolveCenterColumn(col, options)).toBe(col)
+    expect(query).toHaveBeenCalledTimes(1)
+  })
+
+  it('queries and adopts the replacement as soon as the cached column detaches', () => {
+    const first = connectedColumn()
+    const stale = first.col
+    stale.remove()
+    const { col: replacement, slot } = connectedColumn(first.doc)
+    const query = vi.fn<() => Element | null>(() => slot)
+
+    expect(resolveCenterColumn(stale, { document: first.doc, query, now: () => 1 })).toBe(replacement)
+    expect(query).toHaveBeenCalledTimes(1)
+  })
+
+  it('queries when no column has been cached yet', () => {
+    const { doc, col, slot } = connectedColumn()
+    const query = vi.fn<() => Element | null>(() => slot)
+
+    expect(resolveCenterColumn(null, { document: doc, query, now: () => 0 })).toBe(col)
+    expect(query).toHaveBeenCalledTimes(1)
+  })
+
+  it('rejects a missing or detached query result', () => {
+    const doc = document.implementation.createHTMLDocument()
+    expect(resolveCenterColumn(null, { document: doc, query: () => null, now: () => 0 })).toBeUndefined()
+
+    const detachedCol = doc.createElement('div')
+    const detachedSlot = doc.createElement('div')
+    detachedCol.append(detachedSlot)
+    expect(resolveCenterColumn(null, {
+      document: doc,
+      query: () => detachedSlot,
+      now: () => 1,
+    })).toBeUndefined()
+  })
+})