[eric] chat: a streamed answer cannot corrupt itself, and a long one never renders blank (ENG-415)

This commit is contained in:
ciregenz
2026-08-27 14:23:31 -07:00
parent e59b6efae1
commit 4cb9662294
3 changed files with 91 additions and 4 deletions
@@ -940,7 +940,12 @@ const ChatMessageBubble: React.FC<Props> = ({ 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-<id>`), 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) {
@@ -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'));
});
@@ -54,6 +54,16 @@ export function useSmoothText(
const nodeRef = useRef<Text | null>(null);
const baseRef = useRef<string>('');
// 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) {