Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

🔍 Comprehensive PR Review

PR: #1064 — fix(web): interleave tool calls with text during SSE streaming
Reviewed by: 3 specialized agents (code-review, error-handling, test-coverage)
Date: 2026-04-10

Summary

Minimal, correct fix that adds one boundary guard in the onText SSE handler — exactly mirroring the server-side persistence.ts:72 segmentation rule. No bugs introduced. The change is production-ready.

Verdict: ✅ APPROVE

🟡 Medium Issues (Needs Decision — recommend: defer as issues)

1. Silent .catch on SSE reconnect re-fetch (pre-existing)

📍 packages/web/src/components/chat/ChatInterface.tsx:544

The .catch() on the re-fetch after SSE reconnect has zero logging. If the fetch fails (network error, 500, auth expiry), no breadcrumb exists. The user sees the spinner cleared but no error message, and there's nothing in logs.

This is pre-existing and out-of-scope for this PR. Suggested fix: add console.error with context inside the catch.

Options: Fix now | Create issue | Skip — Recommend: Create Issue

2. No unit test for the new onText boundary guard

📍 packages/web/src/components/chat/ChatInterface.tsx:344-357

The new guard is pure state-reducer logic — no DOM, no hooks, no I/O — and is straightforwardly unit-testable. No test was added. A future change to onText could silently re-introduce issue #1054 with no test catching it.

The component layer currently has zero tests, so this PR isn't uniquely regressing coverage. The test-coverage agent provided a sample test outline:

// src/lib/chat-message-reducer.test.ts
describe('applyOnText — tool-call boundary', () => {
  test('starts a new segment when last message has tool calls', () => {
    // ...
    expect(next).toHaveLength(2);
    expect(next[0].isStreaming).toBe(false);
    expect(next[1].content).toBe('Post-tool text');
  });

  test('appends to same segment when no tool calls', () => {
    // ...
    expect(next).toHaveLength(1);
  });

  test('treats undefined toolCalls same as empty array', () => {
    // ...
    expect(next).toHaveLength(1);
  });
});

Options: Fix now (extract reducer + add tests) | Create issue | Skip — Recommend: Create Issue

🟢 Low Issues

✅ What's Good

• Exact server parity: Guard condition (last.toolCalls?.length ?? 0) > 0 directly mirrors persistence.ts:72. Comment even cites the line number — auditable without digging through history.
• Correct placement: New guard sits AFTER the workflow-status check, preserving existing boundary priority ordering.
• Correct isStreaming transitions: Closing previous message with isStreaming: false and opening new one with isStreaming: true is consistent with all other boundary cases.
• Consistent message shape: New segment object (id, role, content, timestamp, isStreaming, toolCalls: []) matches every other message-creation site in the handler.
• Minimal diff: +16 lines, 0 deletions — the smallest change that fixes fix(web): tool calls not interleaved with text during streaming — always appear at bottom #1054.
• CLAUDE.md compliance: YAGNI ✅, KISS ✅, no silent fallbacks ✅, TypeScript strict ✅

📋 Suggested Follow-up Issues

Reviewed by Archon comprehensive-pr-review workflow
Full artifacts: ~/.archon/workspaces/coleam00/Archon/artifacts/runs/86df3adf4a7a4b1da70719ded519aeb3/review/

Fix Report: PR #1064

Date: 2026-04-10T00:00:00Z
Status: COMPLETE
Branch: archon/task-fix-issue-1054
Commit: dbe559e
Philosophy: Aggressive fix — lean towards fixing everything

Summary

Addressed all 2 MEDIUM findings from the consolidated review. No CRITICAL or HIGH findings existed. The 3 LOW findings were correctly left as-is per reviewer recommendations (YAGNI / pre-existing / out-of-scope).

Fixes Applied

Tests Added

File: packages/web/src/lib/chat-message-reducer.test.ts

All 14 tests pass. The @archon/web test suite count went from 87 to 101 tests (in src/lib/ batch).

Docs Updated

None required.

Skipped Findings

Blocked (Could Not Fix)

None.

Validation