Skip to content

ENG-820 - Deduplicate StreamTranscript's line-buffering logic (parseStream vs appendStreamText) - #233

Merged
druks-operator[bot] merged 1 commit into
mainfrom
agent/ENG-820
Aug 10, 2026
Merged

ENG-820 - Deduplicate StreamTranscript's line-buffering logic (parseStream vs appendStreamText)#233
druks-operator[bot] merged 1 commit into
mainfrom
agent/ENG-820

Conversation

@druks-operator

Copy link
Copy Markdown
Contributor

Linear ticket: ENG-820

Plan

Approach

This is a focused frontend refactor limited to frontend/src/components/StreamTranscript.tsx; no protocol, component-prop, parser-output, or rendered-row changes are intended.

  • Replace parseStream’s independent split('\n') buffering implementation with the ticket-pinned delegation: return appendStreamText(emptyParseState, text, complete).rows. Preserve its exported signature and Fast Refresh exemption so the existing public-helper tests exercise the production parser.
  • Remove all orphaned implementation commentary and the comment-only trailing-newline block from parseStream; any surviving rationale belongs at the single rule-owning incremental parser location.
  • Document the append-only caller contract beside IncrementalParseState or appendStreamText: every new text value extends the previous value as a prefix, making receivedLength suffix slicing valid. Keep the comment as an end-state invariant, not implementation narration or reset-guard rationale.
  • Keep the incremental state shape and StreamTranscript render-time update path unchanged. Verified callers already satisfy the invariant: RunTranscriptLive appends chunks and remounts on transcript identity changes, while paginated backfill uses the loading gate before rendering a different transcript.
  • Leave StreamTranscript.test.ts and StreamTranscript.incremental.test.tsx unchanged. Confirm equivalence by hand-tracing the existing parser inputs, including trailing-newline, no-trailing-newline, blank-interior-line, and empty-text cases. Run npm --prefix frontend test only when a Node toolchain is available; otherwise record it as not_run with the missing-toolchain blocker and rely on frontend CI.

Scope decision

Use the ticket’s minimum treatment for finding #2: document the invariant without adding reset behavior. A shorter-length guard misses same-length and longer replacements, while a complete divergence check requires retaining or validating prior text and introduces behavior current callers do not need.

The diff stays within the IncrementalParseState / appendStreamText / parseStream region. Despite .druks/review/checklist.md requiring an end-to-end review of touched files, pre-existing comments and helpers elsewhere in the roughly 760-line component are explicitly out of scope and remain untouched.

Verification constraints

This sandbox has no Node runtime or package manager on PATH, and frontend/node_modules is absent. Frontend test, build, and lint commands are therefore conditional local checks rather than implementation gates; the configured frontend CI remains authoritative.

Acceptance Criteria

  • AC1: parseStream(text: string, complete: boolean): Row[] remains exported with its Fast Refresh exemption, and its body is the single ticket-pinned delegation return appendStreamText(emptyParseState, text, complete).rows. The duplicate splitting/buffering algorithm, its partial-line narration, and the dead trailing-newline block are removed with no residual commentary in the wrapper.
    • Verification: Inspect frontend/src/components/StreamTranscript.tsx and confirm the wrapper contains only the delegation expression and no independent parsing logic, dead block, or buffering commentary.
  • AC2: An end-state contract comment adjacent to IncrementalParseState or appendStreamText states that each text input must extend the previously passed text as a prefix, which makes suffix slicing by receivedLength sound. It does not narrate the implementation, compare old and new behavior, or editorialize about the deliberately omitted reset guard.
    • Verification: Inspect the incremental parser declaration in frontend/src/components/StreamTranscript.tsx and confirm the comment documents the caller invariant in the required form.
  • AC3: frontend/src/components/StreamTranscript.test.ts and frontend/src/components/StreamTranscript.incremental.test.tsx remain unchanged: no test case is deleted, weakened, or adjusted to accommodate the delegation, and both existing suites continue to pass against the shared parser implementation.
    • Verification: Confirm neither test file appears in the diff. Hand-trace the existing parseStream inputs through appendStreamText, including trailing-newline, no-trailing-newline, blank-interior-line, and empty-text forms, and confirm behavior is unchanged. Run npm --prefix frontend test if a Node toolchain is available; otherwise report that exact command as not_run, identify the missing Node toolchain as the blocker, and rely on On Pull Request Frontend / checks in CI.

Make parseStream a thin wrapper over the incremental parser so there is
one JSONL buffering algorithm, and document the append-only text
invariant that appendStreamText's suffix slicing relies on.

@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.

Verified against ENG-820 and the round-2 plan. The diff is exactly the scoped delegation:

  • parseStream now reads only return appendStreamText(emptyParseState, text, complete).rows — the old split/buffer algorithm, its partial-line narration comment, and the dead trailing-newline no-op block are all gone. No residual commentary remains in the wrapper. (AC1)
  • A caller-contract comment sits directly above IncrementalParseState stating that each text passed to appendStreamText must extend the previous text as a prefix, which is what makes receivedLength suffix slicing sound. It's phrased as an end-state invariant, not implementation narration, and doesn't editorialize about the omitted reset guard. (AC2)
  • Neither StreamTranscript.test.ts nor StreamTranscript.incremental.test.tsx appear in the diff. I hand-traced the existing parseStream test inputs (trailing-newline, no-trailing-newline, blank-interior-line, and empty-text shapes) through appendStreamText and confirmed identical output in each case. On Pull Request Frontend / checks is green for a0f2593 and covers npm --prefix frontend run lint, npm --prefix frontend test, and npm --prefix frontend run build for this exact commit. (AC3)
  • Backend lint/test commands (ruff check, ruff format --check, pytest backend/) have no registered check for this SHA — expected, since on-pull-request-backend.yml path-filters on backend/**/pyproject.toml/uv.lock and this diff touches only frontend/src/components/StreamTranscript.tsx. Not a gap, just out of that workflow's trigger scope.

No blocking findings. This is a clean, minimally-scoped dedup that matches the plan's chosen approach (delegation wrapper, not a parallel algorithm or reset guard) and the ticket's explicit "what good looks like" snippet verbatim.

Nothing to push back on from my side, but flag it if any of the above reads differently to you.

Open findings

None carried from prior rounds (round 1 implementation).

@druks-operator

Copy link
Copy Markdown
Contributor Author

Code review: Clean DRY refactor collapsing parseStream into appendStreamText with no behavioral change; existing behavior-based tests still cover it.
Scrutiny: plan critic ran (1) · evaluation rounds: 1 · line review ran

@druks-operator
druks-operator Bot marked this pull request as ready for review August 10, 2026 12:35
@druks-operator
druks-operator Bot requested a review from czpython as a code owner August 10, 2026 12:35
@druks-operator
druks-operator Bot merged commit e25f542 into main Aug 10, 2026
1 check passed
@druks-operator
druks-operator Bot deleted the agent/ENG-820 branch August 10, 2026 12:35
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