feat: refactor streaming to reduce complexity (pure front-end) #91
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Analyze current front-end streaming architecture - Document 10 key complexity issues with code examples - Create phased TODO with 6 implementation phases - Include architecture diagrams and success metrics Co-authored-by: e0945797 <e0945797@u.nus.edu>
|
You have run out of free Bugbot PR reviews for this billing cycle. This will reset on February 6. To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR implements a major refactoring of the streaming architecture for the PaperDebugger chat system. The changes consolidate fragmented streaming logic into a unified state machine, introduce comprehensive type safety, improve error handling, and add extensive test coverage. The refactor is purely front-end with no backend API changes.
Changes:
- Introduced a centralized streaming state machine that consolidates 9+ handler files into a single cohesive implementation
- Added comprehensive documentation (STREAMING_ARCHITECTURE.md and STREAMING_DESIGN_ANALYSIS.md)
- Implemented unified message types (InternalMessage, DisplayMessage) with bidirectional converters
- Enhanced error handling with retry strategies and recovery mechanisms
- Added 2,975 lines of comprehensive tests covering edge cases, stress tests, and fuzz testing
- Updated ESLint configuration to enforce no-unused-vars rule with underscore prefix exception
Reviewed changes
Copilot reviewed 50 out of 51 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| webapp/_webapp/src/stores/streaming/streaming-state-machine.ts | Core state machine consolidating all streaming event handling with explicit state transitions |
| webapp/_webapp/src/stores/streaming/message-type-handlers.ts | Registry-based handlers for different message types, eliminating duplicate switch statements |
| webapp/_webapp/src/stores/streaming/error-handler.ts | Centralized error handling with configurable recovery strategies and retry logic |
| webapp/_webapp/src/stores/streaming/types.ts | Consolidated type definitions for streaming system including events, states, and handlers |
| webapp/_webapp/src/types/message.ts | Canonical internal message types with factory functions and type guards |
| webapp/_webapp/src/utils/message-converters.ts | Bidirectional converters between API, internal, and display message formats |
| webapp/_webapp/src/utils/stream-request-builder.ts | Pure, testable function for building streaming requests |
| webapp/_webapp/src/utils/stream-event-mapper.ts | Maps API responses to typed StreamEvent objects |
| webapp/_webapp/src/stores/message-store.ts | Unified message store combining finalized and streaming messages |
| webapp/_webapp/src/stores/streaming-message-store.ts | Backward compatibility layer delegating to new state machine |
| webapp/_webapp/src/views/devtools/index.tsx | Updated to use new message types and streaming state machine |
| webapp/_webapp/src/views/chat/helper.ts | Refactored to use DisplayMessage types and unified message store |
| webapp/_webapp/src/views/chat/body/index.tsx | Simplified to use unified message store instead of separate streams |
| webapp/_webapp/src/stores/streaming/tests/*.test.ts | Comprehensive test suites with 2,975 lines covering all scenarios |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| import { describe, it, expect, beforeEach, mock } from "bun:test"; | ||
| import { useStreamingStateMachine } from "../stores/streaming/streaming-state-machine"; | ||
| import { useMessageStore } from "../stores/message-store"; |
There was a problem hiding this comment.
Unused import useMessageStore.
Suggested change
| import { useMessageStore } from "../stores/message-store"; |
| buildStreamRequest, | ||
| validateStreamRequestParams, | ||
| } from "../utils/stream-request-builder"; | ||
| import { StreamEvent } from "../stores/streaming/types"; |
There was a problem hiding this comment.
Unused import StreamEvent.
Suggested change
| import { StreamEvent } from "../stores/streaming/types"; |
| * Tests error handling, recovery strategies, and retry logic. | ||
| */ | ||
|
|
||
| import { describe, it, expect, beforeEach, mock, spyOn } from "bun:test"; |
There was a problem hiding this comment.
Unused import spyOn.
Suggested change
| import { describe, it, expect, beforeEach, mock, spyOn } from "bun:test"; | |
| import { describe, it, expect, beforeEach, mock } from "bun:test"; |
| toDisplayMessage, | ||
| fromDisplayMessage, | ||
| } from "../message-converters"; | ||
| import { InternalMessage, MessageStatus } from "../../types/message"; |
There was a problem hiding this comment.
Unused import MessageStatus.
| }); | ||
|
|
||
| it("ignores chunks for non-existent messages", async () => { | ||
| const stateBefore = useStreamingStateMachine.getState().streamingMessage; |
There was a problem hiding this comment.
Unused variable stateBefore.
Junyi-99
added a commit
that referenced
this pull request
Jan 26, 2026
Co-authored-by: Cursor Agent <cursoragent@cursor.com>
Junyi-99
added a commit
that referenced
this pull request
Jan 26, 2026
Co-authored-by: Cursor Agent <cursoragent@cursor.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request introduces a major refactor and documentation update for the PaperDebugger chat system's streaming architecture, alongside some targeted code quality improvements. The most significant changes are the addition of comprehensive documentation analyzing and describing the new streaming design, the enforcement of stricter TypeScript linting rules, and improvements to error logging practices.
Streaming Architecture Documentation & Analysis
STREAMING_ARCHITECTURE.mddocumenting the new streaming system, including state machine design, handler registry, error handling strategies, unified message store, extension points, and troubleshooting guidance.STREAMING_DESIGN_ANALYSIS.mdproviding a detailed analysis of the previous streaming implementation's complexity, identifying architectural problems and recommending a consolidated state machine, unified store, registry-based handlers, explicit state transitions, and unified error handling.Code Quality Improvements
@typescript-eslint/no-unused-varsrule, ignoring variables prefixed with_, to improve code hygiene and prevent unused variable warnings.LocalStorageAdapterby adding comments to disable ESLint'sno-consolerule for specificconsole.warnstatements, ensuring important warnings are not suppressed and code style remains consistent. [1] [2]