diff --git a/frontend/src/app/pages/AgentChat/bubbles/streamingIntegrity.test.ts b/frontend/src/app/pages/AgentChat/bubbles/streamingIntegrity.test.ts index a11685a3..de4a4318 100644 --- a/frontend/src/app/pages/AgentChat/bubbles/streamingIntegrity.test.ts +++ b/frontend/src/app/pages/AgentChat/bubbles/streamingIntegrity.test.ts @@ -19,27 +19,30 @@ const here = path.join(process.cwd(), 'src/app/pages/AgentChat/bubbles'); const smooth = fs.readFileSync(path.join(here, 'useSmoothText.ts'), 'utf8'); const bubble = fs.readFileSync(path.join(here, 'MessageBubble.tsx'), 'utf8'); -test('the imperative append verifies its anchor before writing', () => { - const i = smooth.indexOf('nodeRef.current.data = baseRef.current + pending;'); - assert.ok(i > 0, 'the fast path must still exist'); - const guard = smooth.slice(smooth.lastIndexOf('} else if', i), i); - assert.ok(guard.includes('p_anchorIsFresh()'), - 'a length counter alone cannot tell that the markdown re-parse moved the node'); +test('React is the ONLY writer of the revealed text', () => { + // v2 tried to keep the 60fps imperative append and guard it with an anchor-freshness check. + // DRILLED in a real Chromium DOM 2026-08-27: still corrupted 5 of 6 runs. React reconciles a text + // node against ITS OWN previous value, not the live DOM, so once the node is mutated behind its + // back React can skip the update and the injected characters survive. No guard can fix that, + // because the damage is done before any guard could look. + assert.ok(!/\.data\s*=/.test(smooth), 'nothing may write a DOM text node directly'); + assert.ok(!smooth.includes('createTreeWalker'), 'and nothing needs to go hunting for one'); + assert.ok(!smooth.includes('useLayoutEffect'), 'the re-apply effect went with it'); }); -test('the freshness check compares live DOM data, not a counter', () => { - const fn = smooth.slice(smooth.indexOf('const p_anchorIsFresh'), smooth.indexOf('const findLastTextNode')); - assert.ok(fn.includes('node.data === baseRef.current'), 'the base must still describe the node'); - assert.ok(fn.includes('node.isConnected'), 'a detached node is not an anchor'); +test('the reveal still advances, it just advances on commits', () => { + assert.ok(smooth.includes('setCommittedLen(shown);'), 'the velocity model still drives it'); + assert.ok(/const COMMIT_MS = \d+;/.test(smooth)); + const ms = Number(smooth.match(/const COMMIT_MS = (\d+);/)![1]); + assert.ok(ms <= 100, `commits ARE the reveal now; ${ms}ms would read as stepped`); }); -test('a stale anchor commits instead of silently skipping the frame', () => { - // Skipping would stall the reveal; committing hands the whole string back to React, which cannot - // corrupt it. The failure direction has to be "one extra render", never "wrong text". - const i = smooth.indexOf('p_anchorIsFresh()'); - const branch = smooth.slice(i, i + 900); - assert.ok(branch.includes('setCommittedLen(shown);'), 'the stale path must commit'); - assert.ok(!/else\s*\{\s*\}/.test(branch), 'no empty else'); +test('the typed feel is kept rather than dropped', () => { + // hermes renders streamed text with a memoised parse, a caret, and no pacing at all. That is the + // same conclusion one step further; the velocity model is what makes this read like typing. + for (const knob of ['TARGET_LAG_S', 'RATE_SMOOTH_S', 'MAX_CPS']) { + assert.ok(smooth.includes(knob), `${knob} is part of the feel this exists for`); + } }); test('an oversized bubble starts rendered and can only downgrade', () => { diff --git a/frontend/src/app/pages/AgentChat/bubbles/useSmoothText.ts b/frontend/src/app/pages/AgentChat/bubbles/useSmoothText.ts index 77a9dd40..a9d03664 100644 --- a/frontend/src/app/pages/AgentChat/bubbles/useSmoothText.ts +++ b/frontend/src/app/pages/AgentChat/bubbles/useSmoothText.ts @@ -1,4 +1,4 @@ -import { useEffect, useLayoutEffect, useRef, useState } from 'react'; +import { useEffect, useRef, useState } from 'react'; /** * Smoothly reveals streamed text at a steady cadence instead of painting bursty @@ -14,21 +14,33 @@ import { useEffect, useLayoutEffect, useRef, useState } from 'react'; * - The reveal RATE is EMA-smoothed, so a burst ramps the speed up gently and * a lull ramps it down gently; the rate never steps, so the flow never pulses. * - * Render model (v2): the v1 hook setState'd every frame, which re-rendered the - * whole bubble and re-parsed the full markdown tree 60x/s for the entire stream. - * Now the 60fps motion comes from appending the pending characters straight into - * the LAST DOM TEXT NODE under `revealRef` (one block relayout, no React), and - * React only re-renders ("commits") when structure can change: every COMMIT_MS, - * or immediately when the pending slice contains a newline (new block / list item - * / fence line). Inline markers (** ` _) show raw for at most COMMIT_MS before - * the parse formats them, which matches how unclosed markers already looked. + * Render model (v3): REACT OWNS THE TEXT, and nothing else may write it. + * + * v2 painted the 60fps motion by appending straight into the last DOM text node under `revealRef`, + * with React committing only every COMMIT_MS. Two writers for one string, and it corrupted output in + * the field: users pasted back answers containing the same sentence twice at different offsets, + * clipped at both ends ("recallsByVehicle" arriving again as "ecallsByVehicle"), plus raw markdown + * leaking through where rendered text belonged. + * + * The reason it cannot be made safe with a guard: React reconciles a text node against ITS OWN + * previous value, not against the live DOM. Mutate that node behind React's back and, whenever + * React's before/after values happen to match, it skips the update and the injected characters + * survive forever. Drilled in a real Chromium DOM 2026-08-27 -- an anchor-freshness check still + * corrupted 5 of 6 runs, because the damage is done by the time any check could notice. + * + * So the reveal advances only on commits: every COMMIT_MS, or immediately when the pending slice + * contains a newline (new block / list item / fence line). hermes-agent renders streamed text with + * a memoised parse and a blinking caret and no pacing at all, which is the same conclusion one step + * further; keeping the velocity model preserves the typed feel this was built for. */ const TARGET_LAG_S = 0.35; // stay this far behind = the buffer that prevents stalls const RATE_SMOOTH_S = 0.25; // how fast the reveal speed eases toward its target const MAX_CPS = 1000; // cap so a huge paste/burst still reveals smoothly, not instantly const MAX_DT_S = 0.05; // clamp elapsed after a frame drop / tab switch so we don't leap -const COMMIT_MS = 150; // max staleness of the parsed markdown vs the revealed chars +// 60ms ~= 17fps, which still reads as typing. It was 150ms when a per-frame DOM write hid the +// commit cadence; with React the only writer, that cadence IS the reveal, and 150 looked stepped. +const COMMIT_MS = 60; export function useSmoothText( target: string, @@ -47,46 +59,8 @@ export function useSmoothText( const committedRef = useRef(committedLen); const lastCommitAtRef = useRef(0); - // Imperative-tail bookkeeping: which committedLen the DOM reflects, and the text node + its committed baseline that per-frame appends write into. - const domLenRef = useRef(committedLen); const rafRef = useRef(null); const tickRef = useRef<((now: number) => void) | null>(null); - const nodeRef = useRef(null); - const baseRef = useRef(''); - - // The imperative fast path has TWO writers for the same visible text: this hook and React's - // markdown re-parse. They were synchronised only by a length counter, so a re-parse that moved - // the last text node (a closing backtick, a new list item, a link) left `baseRef` describing a - // node that no longer holds it. Comparing the anchor's live data to what we anchored to makes - // that state unrepresentable rather than merely unlikely. - const p_anchorIsFresh = (): boolean => { - const node = nodeRef.current; - return node !== null && node.isConnected && node.data === baseRef.current; - }; - - const findLastTextNode = (): Text | null => { - const root = revealRef.current; - if (!root) return null; - const walker = document.createTreeWalker(root, NodeFilter.SHOW_TEXT); - let last: Text | null = null; - let n: Node | null; - while ((n = walker.nextNode())) last = n as Text; - return last; - }; - - // After each committed render, re-anchor the tail on the fresh DOM and re-apply any chars the reveal position is already past, so a commit never rewinds visible text. - useLayoutEffect(() => { - if (!enabled) return; - const node = findLastTextNode(); - nodeRef.current = node; - baseRef.current = node ? node.data : ''; - domLenRef.current = committedLen; - const shown = Math.floor(posRef.current); - if (node && shown > committedLen) { - node.data = baseRef.current + targetRef.current.slice(committedLen, shown); - } - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [committedLen, enabled]); // ONE persistent loop, keyed only on `enabled`. It must NOT restart per token: an effect that depends on target.length tears the rAF down and rebuilds it on every delta, and that churn is what stalls the reveal. useEffect(() => { @@ -119,20 +93,7 @@ export function useSmoothText( if (shown > committed) { const pending = targetRef.current.slice(committed, shown); const due = now - lastCommitAtRef.current >= COMMIT_MS; - if (pending.includes('\n') || due || nodeRef.current === null) { - committedRef.current = shown; - lastCommitAtRef.current = now; - setCommittedLen(shown); - } else if (domLenRef.current === committed && p_anchorIsFresh()) { - // DOM is in sync with the last commit AND the node we anchored to still holds exactly - // what we anchored to; safe to append imperatively. - nodeRef.current.data = baseRef.current + pending; - } else { - // Either a commit is mid-flight, or the markdown re-parse moved the tail out from under - // our anchor. Appending against a stale base is what re-emitted text the DOM already had - // at the wrong offset, which is the duplicated-and-clipped output users pasted back - // ("recallsByVehicle" arriving again as "ecallsByVehicle"). Commit instead: React owns the - // whole string, so one render costs a frame and cannot corrupt anything (ENG-415). + if (pending.includes('\n') || due) { committedRef.current = shown; lastCommitAtRef.current = now; setCommittedLen(shown); @@ -173,7 +134,6 @@ export function useSmoothText( cpsRef.current = 0; lastRef.current = 0; committedRef.current = enabled ? 0 : target.length; - nodeRef.current = null; setCommittedLen(enabled ? 0 : target.length); } }, [target.length, enabled]);