feat: unified chat handler#92
Conversation
📝 WalkthroughWalkthroughThis PR refactors the proxy handlers from direct Axum functions to a trait-based adapter pattern. A new ChangesAdapter-Based Handler Refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/proxy/handlers/responses/mod.rs (1)
89-91: 💤 Low valueThe
serialize_stream_itemoverride is redundant.The implementation is identical to the default provided by
FormatHandlerAdapter. SinceSelf::Format = ResponsesApiFormat, the default would produce the same result.♻️ Remove redundant override
- fn serialize_stream_item(chunk: &Self::StreamChunk) -> SseEvent { - serialize_stream_event(chunk) - } - fn stream_error_event(error: &GatewayError) -> Option<SseEvent> {The
serialize_stream_eventhelper can also be removed if only used here, or kept if used elsewhere.🤖 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 `@src/proxy/handlers/responses/mod.rs` around lines 89 - 91, Remove the redundant override of serialize_stream_item in the impl for FormatHandlerAdapter (where Self::Format = ResponsesApiFormat) — delete the fn serialize_stream_item(chunk: &Self::StreamChunk) -> SseEvent { serialize_stream_event(chunk) } so the default implementation from FormatHandlerAdapter is used; if the helper function serialize_stream_event is only referenced by that removed method, remove serialize_stream_event as well, otherwise leave it in place for other callers.src/proxy/handlers/format_handler.rs (1)
255-265: 💤 Low valueConsider extracting stream state into a named struct for readability.
The 8-element tuple state in
unfoldis functional but makes the code harder to follow. A named struct would improve clarity, though this is optional given the localized scope.♻️ Optional refactor to improve readability
struct StreamState<A: FormatHandlerAdapter> { stream: ChatResponseStream<AdapterFormat<A>>, span: Span, request_ctx: RequestContext, should_terminate: bool, saw_item: bool, usage_rx: Option<oneshot::Receiver<Usage>>, output_collector: AdapterCollector<A>, first_output_arrived: bool, }This would make the unfold closure parameters and return values more self-documenting.
🤖 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 `@src/proxy/handlers/format_handler.rs` around lines 255 - 265, The 8-tuple used as the state parameter to futures::stream::unfold is hard to read; introduce a small named struct (e.g., StreamState<A: FormatHandlerAdapter>) that contains the fields currently in the tuple (stream: ChatResponseStream<AdapterFormat<A>>, span: Span, request_ctx: RequestContext, should_terminate: bool, saw_item: bool, usage_rx: Option<oneshot::Receiver<Usage>>, output_collector: AdapterCollector<A>, first_output_arrived: bool) and replace the tuple in the unfold call and closure signature with that struct to make the code self-documenting and improve maintainability.src/proxy/handlers/messages/mod.rs (1)
105-107: 💤 Low valueThe
serialize_stream_itemoverride is redundant.Same as in
ResponsesAdapter, this implementation duplicates the default behavior sinceSelf::Format = AnthropicMessagesFormat.♻️ Remove redundant override
- fn serialize_stream_item(chunk: &Self::StreamChunk) -> SseEvent { - serialize_stream_event(chunk) - } - fn stream_error_event(error: &GatewayError) -> Option<SseEvent> {🤖 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 `@src/proxy/handlers/messages/mod.rs` around lines 105 - 107, The method override serialize_stream_item in the impl for MessagesAdapter is redundant because it simply calls serialize_stream_event and Self::Format is AnthropicMessagesFormat (same default behavior as ResponsesAdapter); remove the serialize_stream_item function from the impl so the default implementation is used, leaving serialize_stream_event and the impl unchanged otherwise to avoid duplication.
🤖 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.
Nitpick comments:
In `@src/proxy/handlers/format_handler.rs`:
- Around line 255-265: The 8-tuple used as the state parameter to
futures::stream::unfold is hard to read; introduce a small named struct (e.g.,
StreamState<A: FormatHandlerAdapter>) that contains the fields currently in the
tuple (stream: ChatResponseStream<AdapterFormat<A>>, span: Span, request_ctx:
RequestContext, should_terminate: bool, saw_item: bool, usage_rx:
Option<oneshot::Receiver<Usage>>, output_collector: AdapterCollector<A>,
first_output_arrived: bool) and replace the tuple in the unfold call and closure
signature with that struct to make the code self-documenting and improve
maintainability.
In `@src/proxy/handlers/messages/mod.rs`:
- Around line 105-107: The method override serialize_stream_item in the impl for
MessagesAdapter is redundant because it simply calls serialize_stream_event and
Self::Format is AnthropicMessagesFormat (same default behavior as
ResponsesAdapter); remove the serialize_stream_item function from the impl so
the default implementation is used, leaving serialize_stream_event and the impl
unchanged otherwise to avoid duplication.
In `@src/proxy/handlers/responses/mod.rs`:
- Around line 89-91: Remove the redundant override of serialize_stream_item in
the impl for FormatHandlerAdapter (where Self::Format = ResponsesApiFormat) —
delete the fn serialize_stream_item(chunk: &Self::StreamChunk) -> SseEvent {
serialize_stream_event(chunk) } so the default implementation from
FormatHandlerAdapter is used; if the helper function serialize_stream_event is
only referenced by that removed method, remove serialize_stream_event as well,
otherwise leave it in place for other callers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2ba95f1f-e200-4d36-914b-e3c7af5a9036
📒 Files selected for processing (12)
src/proxy/handlers/chat_completions/mod.rssrc/proxy/handlers/chat_completions/span_attributes/mod.rssrc/proxy/handlers/chat_completions/span_attributes/stream_output.rssrc/proxy/handlers/format_handler.rssrc/proxy/handlers/messages/mod.rssrc/proxy/handlers/messages/span_attributes/mod.rssrc/proxy/handlers/messages/span_attributes/stream_output.rssrc/proxy/handlers/mod.rssrc/proxy/handlers/responses/mod.rssrc/proxy/handlers/responses/span_attributes/mod.rssrc/proxy/handlers/responses/span_attributes/stream_output.rssrc/proxy/mod.rs
💤 Files with no reviewable changes (3)
- src/proxy/handlers/messages/span_attributes/mod.rs
- src/proxy/handlers/responses/span_attributes/mod.rs
- src/proxy/handlers/chat_completions/span_attributes/mod.rs
Summary by CodeRabbit