ENG-762 - A live transcript re-parses its whole buffer twice a second - #186
Conversation
There was a problem hiding this comment.
Verdict: changes requested
The incremental parse itself is right — appendStreamText slices only the new suffix, carries the trailing partial line, and reuses rowsForLines so parseStream and the component share one event mapping. AC1, AC2, AC3 and AC4 are all satisfied by the code and the new tests. The blocker is mechanical: the useEffect + setParseState wiring fails the repo's own lint rule, and CI aborts before the frontend tests or the build ever run.
Blocking
frontend/src/components/StreamTranscript.tsx:41—react-hooks/set-state-in-effectfailsnpm --prefix frontend run lint. Details and two suggested restructurings are in the inline comment. Because thechecksjob exits at lint,npm --prefix frontend testandnpm --prefix frontend run buildare both unverified on01c770f.
Verification profile
| command | result |
|---|---|
uv run ruff check backend |
pass (ran locally, all checks passed) |
npm --prefix frontend run lint |
fail — checks job on 01c770f |
npm --prefix frontend test |
not run — CI skipped it after lint failed; Node is absent from the review sandbox (only Node 18 available, repo needs Vite 8 / Vitest 4) |
npm --prefix frontend run build |
not run — same reason |
uv run pytest backend/ |
not run — no Postgres in the review sandbox; the diff is frontend-only |
uv pip install -e backend/tests/druks-field_notes |
pass |
uv run pytest backend/tests/test_proof_extension*.py |
not run — same Postgres gap |
Acceptance criteria
- AC1 — pass.
appendStreamText(StreamTranscript.tsx:116-141) parsestext.slice(previous.receivedLength)only, retainingreceivedLengthandpartialLine.StreamTranscript.incremental.test.tsx:16-32asserts each JSONL line hitsJSON.parseexactly once across two cumulative renders. - AC2 — pass. The trailing fragment stays buffered while
complete === false, and thecomplete && partialLine !== ''branch flushes it once throughrowsForLines.StreamTranscript.incremental.test.tsx:35-55covers both the split-line and completion-only-flush paths. - AC3 — pass.
RunTranscript.tsxis untouched; bothextensions/ship/AgentCallPage.tsxandextensions/ship/WorkItemPage.tsxstill renderRunTranscript, andRunTranscript.test.tsxkeeps both the progressive-chunk and live-SSE tests. - AC4 — pass.
parseStreamkeeps its full-buffer signature and semantics; the line-splitting is unchanged and only the row loop moved into the sharedrowsForLines, so there is no second event mapping.
Open findings
frontend/src/components/StreamTranscript.tsx:122— ifcompleteever goestrue → false, the already-flushed tail row stays rendered whileparseStreamwould hide it. Neither production caller does this (RunTranscriptLivesetscompleteonce), so it is a note rather than a defect.frontend/src/components/StreamTranscript.tsx:136—[...previous.rows, ...newRows]copies the row array on every append, so total work stays O(n²) in array copies even though JSON parsing is now linear. That is the cost of React immutability and is far cheaper than the re-parse this ticket removes; worth revisiting only if very long transcripts still stutter.frontend/src/components/StreamTranscript.incremental.test.tsx:50-55— the final rerender passes props identical to the previous one, so React skips the effect and the "no duplicate" assertion there is close to vacuous. The meaningfulcompletefalse→true transition immediately above it does carry the assertion.
| const rows = parseState.rows | ||
|
|
||
| useEffect(() => { | ||
| setParseState((previous) => appendStreamText(previous, text, complete)) |
There was a problem hiding this comment.
Blocking — npm --prefix frontend run lint fails on this line. The checks workflow for 01c770f stops here:
frontend/src/components/StreamTranscript.tsx
41:5 error Error: Calling setState synchronously within an effect can trigger cascading renders
react-hooks/set-state-in-effect
The job aborts at lint, so npm --prefix frontend test and npm --prefix frontend run build never ran — the incremental tests added here are unverified by CI.
The constraint: the incremental parse state has to advance when text/complete change without calling setState from an effect body. Two approaches both satisfy it — pick whichever reads better here:
- Adjust state during render (React's documented derived-state escape hatch): keep
useState, track the previoustext/completein state, and callsetParseStatein the render body guarded by a changed-input check. Keeps the value in React state; costs one extra render pass per append. - Hold the parse state in a ref and advance it during render:
const stateRef = useRef(emptyParseState), then advance it whenreceivedLength !== text.length || tailFlushed !== completeand readstateRef.current.rows.appendStreamTextis already a no-op on repeated identical input, so a StrictMode double-render stays correct. This also removes the one-commit lag the current effect introduces, where each arriving chunk paints stale rows before the parsed rows land.
Whichever you choose, keep the stick-to-bottom useEffect keyed on the rows array identity — appendStreamText already preserves the previous array when no rows are added, which is what makes that dependency stable.
Test to add after the fix: extend frontend/src/components/StreamTranscript.incremental.test.tsx with a case that renders inside <React.StrictMode> (or simply rerenders with identical props between appends) and asserts the row count and JSON.parse call counts are unchanged — that pins the idempotence the ref/render-time approach depends on. Please also confirm npm --prefix frontend test and npm --prefix frontend run build go green once lint passes; neither has run on this commit.
Push back if you think a different structure is cleaner — the requirement is only that lint passes without reintroducing the full-buffer re-parse.
Addressed by advancing parse state during render and adding an identical-props idempotence regression test.
There was a problem hiding this comment.
Verdict: pass
The round-1 blocker is resolved. frontend/src/components/StreamTranscript.tsx:38-43 replaces the useEffect + setParseState wiring with React's documented render-time state adjustment, guarded on receivedLength !== text.length || tailFlushed !== complete, so the react-hooks/set-state-in-effect error is gone. The checks job for 10d2064 is green, which means lint, npm --prefix frontend test, and npm --prefix frontend run build all ran and passed — the first commit where the new tests are verified by CI. The render-time approach also removes the one-commit lag the effect introduced.
The new does not parse identical props again test pins the idempotence the render-time approach depends on, which is what round 1 asked for.
Acceptance criteria
- AC1 — pass.
appendStreamTextslicestext.slice(previous.receivedLength)only and retainsreceivedLength/partialLine.StreamTranscript.incremental.test.tsxasserts each JSONL line hitsJSON.parseexactly once across two cumulative renders. - AC2 — pass. The trailing fragment stays buffered while
complete === false; thecomplete && partialLine !== ''branch (StreamTranscript.tsx:131) flushes it once throughrowsForLines. The split-line and completion-only-flush cases are both covered. - AC3 — pass.
RunTranscript.tsxis untouched, both pages still renderRunTranscript, andRunTranscript.test.tsxretains progressive-chunk and live-SSE coverage. - AC4 — pass.
parseStreamkeeps its signature and full-buffer semantics; both entry points sharerowsForLines, so there is no second event mapping.
Verification profile
npm --prefix frontend run lint— pass (checks,10d2064).npm --prefix frontend test— pass (checks,10d2064).npm --prefix frontend run build— pass (checks,10d2064).uv run ruff check backend— pass, ran locally, "All checks passed!".uv run pytest backend/,uv pip install -e backend/tests/druks-field_notes,uv run pytest backend/tests/test_proof_extension*.py— not run; no Postgres in this sandbox, and the backend PR workflow did not trigger because the diff touches no backend paths. The diff is frontend-only, so no backend regression is possible.
Open findings
Carried forward from round 1, all still open and all non-blocking:
frontend/src/components/StreamTranscript.tsx:131— ifcompleteever transitionstrue → false, the already-flushed tail row stays rendered whereparseStreamwould hide it. Neither production caller does this.frontend/src/components/StreamTranscript.tsx:137—[...previous.rows, ...newRows]copies the row array on every append, so array-copy work stays O(n²) even though JSON parsing is now linear. Far cheaper than the re-parse this PR removes; revisit only if very long transcripts still stutter.frontend/src/components/StreamTranscript.incremental.test.tsx— the last rerender in the third test still passes props identical to the previous one, so its "no duplicate" assertion is close to vacuous. Partly mitigated now: the newdoes not parse identical props againtest carries a real assertion for that path, and the meaningfulcompletefalse→true transition above it carries its own.
Push back on any of this if you read it differently.
|
Code review: Correctly switches StreamTranscript to O(1)-per-chunk incremental parsing with a well-targeted render-time state-update pattern and behavior-focused tests, but leaves the old parseStream buffering logic duplicated and now dead in production (only the untouched legacy test file still calls it), and the new incremental state trusts text to only ever grow without documenting or guarding that assumption — filed ENG-820 to fold parseStream into a thin wrapper over appendStreamText and document/guard the append-only contract. |
Linear ticket: ENG-762
Plan
Scope and existing contract
This is a frontend-only optimization in the shared transcript renderer.
RunTranscriptalready supplies an accumulatedtextstring toStreamTranscriptfor both progressive 256 KB backfill and live SSE appends. Keep these contracts unchanged:StreamTranscript({ text, complete = false }: { text: string; complete?: boolean }){ text: string; nextOffset: number; eof: boolean }transcript.chunkwith{ text: string }agent_call.finishedAgentCallPage.tsxandWorkItemPage.tsxshould continue to use the sharedRunTranscript; neither page needs its own parsing state.Code changes
In
frontend/src/components/StreamTranscript.tsx, replace the full-bufferuseMemo(() => parseStream(text, complete), [text, complete])derivation with retained incremental parse state. Initialize that state from the firsttextvalue, then track:texthas been received,For each accumulated-text update, take only
text.slice(previousReceivedLength), combine it with the buffered partial line, parse newline-terminated lines through the existingtryParse/rowsForEventpath, and append only the resulting new rows. Retain the remaining unterminated line whencompleteis false. Whencompletechanges to true, parse and append the buffered final line even iftextitself did not change.Preserve the exported
parseStream(text, complete)helper and its current full-buffer semantics for existing unit coverage. Factor the line-to-row work so full and incremental entry points share the same event interpretation and cannot diverge. Keep the stick-to-bottom effect dependent on the appendedrowsstate so newly added rows retain the existing scrolling behavior.Treat the
textprop as append-only during a mounted transcript instance. That matches both verified callers: live transcripts are keyed bytranscriptKey, and static transcript identity changes pass through the loading branch, unmounting the old renderer. Do not add a full-prefixstartsWithvalidation on every update, because scanning the old buffer per append would preserve the quadratic main-thread cost this ticket removes.Tests
Add component-level incremental coverage alongside the existing transcript tests (using a TSX test file if JSX rendering is introduced). Render one terminated JSONL event, rerender with the accumulated first and second events, and spy on exact
JSON.parseinputs so the test proves the first line remains at one parse while the second line is parsed once. Assert both resulting rows render in order.Cover trailing-line state transitions: provide a JSON event split across two cumulative updates and assert it remains hidden until its newline arrives, then add a final unterminated event and toggle
completeto true to assert it is appended once without duplicating earlier rows.Retain
frontend/src/components/RunTranscript.test.tsxcoverage for both integration paths: progressive paginated backfill and livetranscript.chunkdelivery throughagent_call.finished. Existingfrontend/src/components/StreamTranscript.test.tsparser cases continue protecting all current row mappings and suppression rules.Out of scope
Acceptance criteria
AC1
Description:
StreamTranscriptincrementally appends parsed rows instead of re-runningparseStream(text, complete)over the accumulated transcript. Its retained state tracks the received offset and the unrendered trailing fragment, and an appended update parses only the new suffix plus that fragment; already-rendered lines are not parsed again.Verification: Inspect
frontend/src/components/StreamTranscript.tsxand a component regression test that feeds one terminated JSONL line, rerenders with a second cumulative line, and asserts via exactJSON.parsecalls that each line was parsed once.AC2
Description: While
complete === false, a trailing line without\nremains hidden across updates. When a later update supplies its terminator, or whencompletebecomestruewithout additional text, that buffered line is converted through the existing line-to-row logic and appended exactly once in stream order.Verification: A frontend component test covers a JSON line split across updates and the completion-only flush of a final unterminated line, asserting that neither branch renders a duplicate row.
AC3
Description: The shared transcript contract remains compatible with both production callers:
AgentCallPageandWorkItemPagecontinue usingRunTranscript, static paginated backfill continues consuming{ text, nextOffset, eof }, and live tailing continues consumingtranscript.chunkpayloads shaped as{ text }untilagent_call.finished. Both paths pass cumulative text to the incrementalStreamTranscript.Verification: Inspect the two page call sites and
RunTranscript.tsx;RunTranscript.test.tsxretains coverage for progressive static chunks and live SSE chunks reaching the shared renderer.AC4
Description: The exported
parseStream(text: string, complete: boolean): Row[]behavior remains available for full-buffer parsing, and the incremental component reuses the same existing line conversion, noise suppression, raw-line fallback, and row ordering rather than introducing a second event mapping.Verification: Inspect the parser structure and the existing
StreamTranscriptunit tests covering Claude, Codex, harness-result, tool, noise, unknown, and raw event behavior.Ruled out
useMemoand only memoizeStreamRowrendering: row memoization happens after splitting andJSON.parse, so every append would still redo the growing main-thread parse and retain quadratic work._TRANSCRIPT_POLL_SECONDSfrequency or increase transport chunk sizes: this would only lower the number of full re-parses, would not make parsing linear, and would leave the paginated-backfill form of the same issue intact.text.slice(previousLength)without buffering the prior trailing fragment: a JSONL event split across fetch or SSE chunks would be parsed as separate malformed/raw fragments or lost instead of producing its intended row once complete.text.startsWith(previousText)to support arbitrary in-place transcript replacement: both production callers already remount on transcript identity changes, while repeatedly scanning the entire old prefix would reintroduce growing per-update work even after JSON parsing became incremental.RunTranscriptLiveand the static backfill loop: duplicating parser state across transport paths would create separate partial-line and completion behavior for live and static transcripts and strand the sharedStreamTranscriptabstraction used by both pages.Reference repositories
These related repos may hold useful build. They are NOT pre-cloned — if one is relevant to your task, clone it yourself:
Repos:
czpython/drukbox-python-sdk— SDK for the Drukbox HTTP host APIczpython/drukbox— service for provisioning sandbox hosts across providersAcceptance Criteria
StreamTranscriptincrementally appends parsed rows instead of re-runningparseStream(text, complete)over the accumulated transcript. Its retained state tracks the received offset and the unrendered trailing fragment, and an appended update parses only the new suffix plus that fragment; already-rendered lines are not parsed again.frontend/src/components/StreamTranscript.tsxand a component regression test that feeds one terminated JSONL line, rerenders with a second cumulative line, and asserts via exactJSON.parsecalls that each line was parsed once.complete === false, a trailing line without\nremains hidden across updates. When a later update supplies its terminator, or whencompletebecomestruewithout additional text, that buffered line is converted through the existing line-to-row logic and appended exactly once in stream order.AgentCallPageandWorkItemPagecontinue usingRunTranscript, static paginated backfill continues consuming{ text, nextOffset, eof }, and live tailing continues consumingtranscript.chunkpayloads shaped as{ text }untilagent_call.finished. Both paths pass cumulative text to the incrementalStreamTranscript.RunTranscript.tsx;RunTranscript.test.tsxretains coverage for progressive static chunks and live SSE chunks reaching the shared renderer.parseStream(text: string, complete: boolean): Row[]behavior remains available for full-buffer parsing, and the incremental component reuses the same existing line conversion, noise suppression, raw-line fallback, and row ordering rather than introducing a second event mapping.StreamTranscriptunit tests covering Claude, Codex, harness-result, tool, noise, unknown, and raw event behavior.