feat(web): improve streaming chat continuity readability#1617
Conversation
📝 WalkthroughWalkthroughThis PR consolidates consecutive system-status SSE messages into a single evolving chat message via a new ChangesSystem Status Message Consolidation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/web/src/lib/system-status-reducer.test.ts (1)
5-9: ⚡ Quick winReset
idCounterbefore each test to prevent order-dependent failures.
idCounteris module-level state and is never reset. Test 1's assertionid: 'msg-1'is currently correct only because it runs first and no earlier test callsmakeId(). Adding any new test that callsmakeId()before test 1 will silently produce the wrong ID and break the expectation.🛠️ Proposed fix
+import { describe, expect, test, beforeEach } from 'bun:test'; let idCounter = 0; function makeId(): string { idCounter++; return `msg-${String(idCounter)}`; } describe('applySystemStatus', () => { + beforeEach(() => { + idCounter = 0; + }); + test('appends a new system message when previous message is not system', () => {As per coding guidelines: "keep tests deterministic with no flaky timing or network dependence without guardrails".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/web/src/lib/system-status-reducer.test.ts` around lines 5 - 9, Tests rely on module-level idCounter (used by makeId) but it is never reset, causing order-dependent failures; fix by resetting that module state before each test—e.g., in the test file add a beforeEach that calls jest.resetModules() and re-requires the module (or export a resetIdCounter helper from the module and call it in beforeEach) so idCounter is zeroed before every test, ensuring makeId produces deterministic 'msg-1' in the first call.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/web/src/components/chat/MessageBubble.tsx`:
- Around line 212-215: In MessageBubble (component MessageBubble / JSX block
containing the two spans), remove the redundant hidden assistive span <span
className="sr-only">Thinking</span> so screen readers don't announce "Thinking"
twice; keep the visible <span className="font-medium">Thinking</span> and ensure
the surrounding container (the div with className "flex items-center gap-2 py-1
text-sm text-text-tertiary") remains unchanged.
---
Nitpick comments:
In `@packages/web/src/lib/system-status-reducer.test.ts`:
- Around line 5-9: Tests rely on module-level idCounter (used by makeId) but it
is never reset, causing order-dependent failures; fix by resetting that module
state before each test—e.g., in the test file add a beforeEach that calls
jest.resetModules() and re-requires the module (or export a resetIdCounter
helper from the module and call it in beforeEach) so idCounter is zeroed before
every test, ensuring makeId produces deterministic 'msg-1' in the first call.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fa42cc45-324f-4de5-951d-56a4a39053e6
📒 Files selected for processing (5)
packages/web/src/components/chat/ChatInterface.tsxpackages/web/src/components/chat/MessageBubble.tsxpackages/web/src/components/chat/ToolCallCard.tsxpackages/web/src/lib/system-status-reducer.test.tspackages/web/src/lib/system-status-reducer.ts
Review SummaryVerdict: ready-to-merge This PR cleanly extracts consecutive system-status SSE rows into a single evolving chat line via a new Blocking issues(none) Suggested fixes
Minor / nice-to-have
Compliments
Reviewed via maintainer-review-pr workflow (Pi/Minimax). Aspects run: code-review, test-coverage, comment-quality. |
Review SummaryVerdict: ready-to-merge Three focused UI improvements: a Blocking issuesNone. Suggested fixesNone. Minor / nice-to-have
Compliments
Reviewed via maintainer-review-pr workflow (Pi/Minimax). Aspects run: code-review, comment-quality. |
Summary
Describe this PR in 2-5 bullets:
UX Journey
Before
After
Architecture Diagram
Before
After
Connection inventory (list every module-to-module edge, mark changes):
Label Snapshot
risk: lowsize: Swebweb:chatChange Metadata
featurewebLinked Issue
Validation Evidence (required)
Commands and result summary:
bun --filter @archon/web type-check bun --filter @archon/web testbun run validatewas skipped because this PR is a bounded web-only UI slice and the touched surface is covered by package-local validation.Security Impact (required)
No)No)No)No)Yes, describe risk and mitigation:Compatibility / Migration
Yes)No)No)Human Verification (required)
What was personally validated beyond CI:
Side Effects / Blast Radius (required)
ChatInterface,MessageBubble,ToolCallCard)Rollback Plan (required)
5f23b691andf4c732c4, or revert the PR merge ondevRisks and Mitigations
List real risks in this PR (or write
None).systemrows only; assistant/tool messages remain separate and ordered.Summary by CodeRabbit
Release Notes
New Features
Style
Tests