Skip to content

ENG-762 - A live transcript re-parses its whole buffer twice a second - #186

Merged
druks-operator[bot] merged 2 commits into
mainfrom
agent/ENG-762
Aug 5, 2026
Merged

ENG-762 - A live transcript re-parses its whole buffer twice a second#186
druks-operator[bot] merged 2 commits into
mainfrom
agent/ENG-762

Conversation

@druks-operator

@druks-operator druks-operator Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Linear ticket: ENG-762

Plan

Scope and existing contract

This is a frontend-only optimization in the shared transcript renderer. RunTranscript already supplies an accumulated text string to StreamTranscript for both progressive 256 KB backfill and live SSE appends. Keep these contracts unchanged:

  • StreamTranscript({ text, complete = false }: { text: string; complete?: boolean })
  • paginated transcript response: { text: string; nextOffset: number; eof: boolean }
  • live event: transcript.chunk with { text: string }
  • terminal live event: agent_call.finished

AgentCallPage.tsx and WorkItemPage.tsx should continue to use the shared RunTranscript; neither page needs its own parsing state.

Code changes

  1. In frontend/src/components/StreamTranscript.tsx, replace the full-buffer useMemo(() => parseStream(text, complete), [text, complete]) derivation with retained incremental parse state. Initialize that state from the first text value, then track:

    • the rows already produced,
    • how much of the accumulated text has been received,
    • the trailing, not-yet-renderable partial line,
    • whether completion has already flushed that tail.
  2. For each accumulated-text update, take only text.slice(previousReceivedLength), combine it with the buffered partial line, parse newline-terminated lines through the existing tryParse / rowsForEvent path, and append only the resulting new rows. Retain the remaining unterminated line when complete is false. When complete changes to true, parse and append the buffered final line even if text itself did not change.

  3. 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 appended rows state so newly added rows retain the existing scrolling behavior.

  4. Treat the text prop as append-only during a mounted transcript instance. That matches both verified callers: live transcripts are keyed by transcriptKey, and static transcript identity changes pass through the loading branch, unmounting the old renderer. Do not add a full-prefix startsWith validation on every update, because scanning the old buffer per append would preserve the quadratic main-thread cost this ticket removes.

Tests

  1. 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.parse inputs 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.

  2. 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 complete to true to assert it is appended once without duplicating earlier rows.

  3. Retain frontend/src/components/RunTranscript.test.tsx coverage for both integration paths: progressive paginated backfill and live transcript.chunk delivery through agent_call.finished. Existing frontend/src/components/StreamTranscript.test.ts parser cases continue protecting all current row mappings and suppression rules.

Out of scope

  • No backend polling, transcript endpoint, response-shape, SSE event, or cache-policy changes.
  • No page-specific transcript parser or UI redesign.
  • No change to transcript row presentation or event suppression rules.

Acceptance criteria

AC1

Description: StreamTranscript incrementally appends parsed rows instead of re-running parseStream(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.tsx and a component regression test that feeds one terminated JSONL line, rerenders with a second cumulative line, and asserts via exact JSON.parse calls that each line was parsed once.

AC2

Description: While complete === false, a trailing line without \n remains hidden across updates. When a later update supplies its terminator, or when complete becomes true without 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: AgentCallPage and WorkItemPage continue using RunTranscript, static paginated backfill continues consuming { text, nextOffset, eof }, and live tailing continues consuming transcript.chunk payloads shaped as { text } until agent_call.finished. Both paths pass cumulative text to the incremental StreamTranscript.

Verification: Inspect the two page call sites and RunTranscript.tsx; RunTranscript.test.tsx retains 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 StreamTranscript unit tests covering Claude, Codex, harness-result, tool, noise, unknown, and raw event behavior.

Ruled out

  • Keep the full-buffer useMemo and only memoize StreamRow rendering: row memoization happens after splitting and JSON.parse, so every append would still redo the growing main-thread parse and retain quadratic work.
  • Reduce _TRANSCRIPT_POLL_SECONDS frequency 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.
  • Parse only 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.
  • Validate every append with 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.
  • Move incremental parsing independently into RunTranscriptLive and 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 shared StreamTranscript abstraction 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:

git clone https://github.com/<full_name> /home/exedev/work/related/<name>
  • Auth is already configured (git credential helper); clone the plain HTTPS URL.
  • Clone only what you actually need — skip repos that aren't relevant.
  • These are read-only references. Don't modify or push to them; the harness only commits to the assigned PR branch.
  • If a clone fails (repo renamed, deleted, or inaccessible), carry on without it — it's lost context, not a blocker.

Repos:

  • czpython/drukbox-python-sdk — SDK for the Drukbox HTTP host API
  • czpython/drukbox — service for provisioning sandbox hosts across providers

Acceptance Criteria

  • AC1: StreamTranscript incrementally appends parsed rows instead of re-running parseStream(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.tsx and a component regression test that feeds one terminated JSONL line, rerenders with a second cumulative line, and asserts via exact JSON.parse calls that each line was parsed once.
  • AC2: While complete === false, a trailing line without \n remains hidden across updates. When a later update supplies its terminator, or when complete becomes true without 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: The shared transcript contract remains compatible with both production callers: AgentCallPage and WorkItemPage continue using RunTranscript, static paginated backfill continues consuming { text, nextOffset, eof }, and live tailing continues consuming transcript.chunk payloads shaped as { text } until agent_call.finished. Both paths pass cumulative text to the incremental StreamTranscript.
    • Verification: Inspect the two page call sites and RunTranscript.tsx; RunTranscript.test.tsx retains coverage for progressive static chunks and live SSE chunks reaching the shared renderer.
  • AC4: 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 StreamTranscript unit tests covering Claude, Codex, harness-result, tool, noise, unknown, and raw event behavior.

@druks-reviewer druks-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. frontend/src/components/StreamTranscript.tsx:41react-hooks/set-state-in-effect fails npm --prefix frontend run lint. Details and two suggested restructurings are in the inline comment. Because the checks job exits at lint, npm --prefix frontend test and npm --prefix frontend run build are both unverified on 01c770f.

Verification profile

command result
uv run ruff check backend pass (ran locally, all checks passed)
npm --prefix frontend run lint failchecks 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) parses text.slice(previous.receivedLength) only, retaining receivedLength and partialLine. StreamTranscript.incremental.test.tsx:16-32 asserts each JSONL line hits JSON.parse exactly once across two cumulative renders.
  • AC2 — pass. The trailing fragment stays buffered while complete === false, and the complete && partialLine !== '' branch flushes it once through rowsForLines. StreamTranscript.incremental.test.tsx:35-55 covers both the split-line and completion-only-flush paths.
  • AC3 — pass. RunTranscript.tsx is untouched; both extensions/ship/AgentCallPage.tsx and extensions/ship/WorkItemPage.tsx still render RunTranscript, and RunTranscript.test.tsx keeps both the progressive-chunk and live-SSE tests.
  • AC4 — pass. parseStream keeps its full-buffer signature and semantics; the line-splitting is unchanged and only the row loop moved into the shared rowsForLines, so there is no second event mapping.

Open findings

  • frontend/src/components/StreamTranscript.tsx:122 — if complete ever goes true → false, the already-flushed tail row stays rendered while parseStream would hide it. Neither production caller does this (RunTranscriptLive sets complete once), 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 meaningful complete false→true transition immediately above it does carry the assertion.

const rows = parseState.rows

useEffect(() => {
setParseState((previous) => appendStreamText(previous, text, complete))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Adjust state during render (React's documented derived-state escape hatch): keep useState, track the previous text/complete in state, and call setParseState in the render body guarded by a changed-input check. Keeps the value in React state; costs one extra render pass per append.
  2. Hold the parse state in a ref and advance it during render: const stateRef = useRef(emptyParseState), then advance it when receivedLength !== text.length || tailFlushed !== complete and read stateRef.current.rows. appendStreamText is 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.

@druks-operator
druks-operator Bot dismissed druks-reviewer[bot]’s stale review August 5, 2026 07:44

Addressed by advancing parse state during render and adding an identical-props idempotence regression test.

@druks-reviewer druks-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. appendStreamText slices text.slice(previous.receivedLength) only and retains receivedLength / partialLine. StreamTranscript.incremental.test.tsx asserts each JSONL line hits JSON.parse exactly once across two cumulative renders.
  • AC2 — pass. The trailing fragment stays buffered while complete === false; the complete && partialLine !== '' branch (StreamTranscript.tsx:131) flushes it once through rowsForLines. The split-line and completion-only-flush cases are both covered.
  • AC3 — pass. RunTranscript.tsx is untouched, both pages still render RunTranscript, and RunTranscript.test.tsx retains progressive-chunk and live-SSE coverage.
  • AC4 — pass. parseStream keeps its signature and full-buffer semantics; both entry points share rowsForLines, 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 — if complete ever transitions true → false, the already-flushed tail row stays rendered where parseStream would 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 new does not parse identical props again test carries a real assertion for that path, and the meaningful complete false→true transition above it carries its own.

Push back on any of this if you read it differently.

@druks-operator

druks-operator Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

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.

@druks-operator
druks-operator Bot marked this pull request as ready for review August 5, 2026 07:51
@druks-operator
druks-operator Bot requested a review from czpython as a code owner August 5, 2026 07:51
@druks-operator
druks-operator Bot merged commit cea8133 into main Aug 5, 2026
1 check passed
@druks-operator
druks-operator Bot deleted the agent/ENG-762 branch August 5, 2026 20:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant