diff --git a/frontend/src/app/pages/AgentChat/bubbles/MessageBubble.tsx b/frontend/src/app/pages/AgentChat/bubbles/MessageBubble.tsx index 58f6df89..4dd77c61 100644 --- a/frontend/src/app/pages/AgentChat/bubbles/MessageBubble.tsx +++ b/frontend/src/app/pages/AgentChat/bubbles/MessageBubble.tsx @@ -940,7 +940,12 @@ const ChatMessageBubble: React.FC = ({ message, editing = false, onSaveEd // A message longer than ~2 screens of text gets the placeholder + block virtualization treatment (full render in view, reserved-height placeholder off). const isOversized = !isStreaming && displayText.length > oversizedCharThreshold(viewportHeight, viewportWidth); - const [isOversizedInViewport, setIsOversizedInViewport] = useState(false); + // Seeded TRUE so the measure can only ever DOWNGRADE to a placeholder. Starting false meant every + // fresh mount of a long message rendered `{text: ''}` until a measure pass rescued it, and the + // stream handoff mounts a fresh bubble by construction (the streaming copy lives outside the list + // and is keyed `streaming-`), so a long answer flashed blank and re-typed at the end of every + // turn. Cost of the safe direction: one extra render for a long message that mounts off-screen. + const [isOversizedInViewport, setIsOversizedInViewport] = useState(true); const shouldRenderMarkdown = !isOversized || isOversizedInViewport; const markdownWindow = useMemo(() => { if (!shouldRenderMarkdown) { diff --git a/frontend/src/app/pages/AgentChat/bubbles/streamingIntegrity.test.ts b/frontend/src/app/pages/AgentChat/bubbles/streamingIntegrity.test.ts new file mode 100644 index 00000000..a11685a3 --- /dev/null +++ b/frontend/src/app/pages/AgentChat/bubbles/streamingIntegrity.test.ts @@ -0,0 +1,63 @@ +// ENG-415: streamed answers corrupted themselves as they rendered. Two defects in one handoff. +// +// 1. `useSmoothText` paints the 60fps motion OUTSIDE React, writing into the last DOM text node, +// while React's markdown re-parse writes the same text every 150ms. Two writers, synchronised +// only by a length counter. A re-parse that moved the tail (a closing backtick, a list item, a +// link) left `baseRef` describing a node that no longer held it, and `base + pending` then +// re-emitted text the DOM already had at the wrong offset. Users pasted the result back: +// "recallsByVehicle" arriving a second time as "ecallsByVehicle", "lsByVehicle". +// +// 2. `isOversizedInViewport` started false, so a long message on a FRESH mount rendered `{text: ''}` +// until a measure pass rescued it. The stream handoff mounts a fresh bubble by construction, so +// every long answer flashed blank and re-typed at the end of the turn. +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import fs from 'node:fs'; +import path from 'node:path'; + +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('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('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('an oversized bubble starts rendered and can only downgrade', () => { + const i = bubble.indexOf('const [isOversizedInViewport, setIsOversizedInViewport] = useState('); + assert.ok(i > 0); + const decl = bubble.slice(i, i + 120); + assert.ok(decl.includes('useState(true)'), + 'starting false blanks every fresh mount of a long message until a measure rescues it'); +}); + +test('the measure can still place a message off-screen', () => { + // The seed must not disable the placeholder; it only changes which direction the race resolves. + assert.ok(bubble.includes('setIsOversizedInViewport(false)'), 'the downgrade path must survive'); + assert.ok(bubble.includes('IntersectionObserver')); +}); + +test('streaming still forces markdown on, regardless of length', () => { + // isOversized is gated on !isStreaming; a streaming bubble must never take the placeholder path. + const i = bubble.indexOf('const isOversized ='); + assert.ok(bubble.slice(i, i + 160).includes('!isStreaming')); +}); diff --git a/frontend/src/app/pages/AgentChat/bubbles/useSmoothText.ts b/frontend/src/app/pages/AgentChat/bubbles/useSmoothText.ts index acd1f908..77a9dd40 100644 --- a/frontend/src/app/pages/AgentChat/bubbles/useSmoothText.ts +++ b/frontend/src/app/pages/AgentChat/bubbles/useSmoothText.ts @@ -54,6 +54,16 @@ export function useSmoothText( 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; @@ -113,11 +123,20 @@ export function useSmoothText( committedRef.current = shown; lastCommitAtRef.current = now; setCommittedLen(shown); - } else if (domLenRef.current === committed) { - // DOM is in sync with the last commit; safe to append imperatively. + } 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). + committedRef.current = shown; + lastCommitAtRef.current = now; + setCommittedLen(shown); } - // else: a commit is mid-flight; skip this frame's append (≤1 frame). } // Fully revealed AND fully committed: park instead of burning 60fps forever; the growth effect below re-arms. if (posRef.current >= full && committedRef.current >= full) {