Repository navigation
Added Image Support in SortedAgents - #330
Conversation
|
Warning Review limit reached
More reviews will be available in 53 minutes and 54 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR refactors agentic chat message handling to use a structured ChangesStructured Message Content for Multimodal Agentic Chat
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
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)
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces agentic chat capabilities, allowing models with tool-calling support to perform web searches via Brave Search and web scraping via Cloudflare. Key changes include the implementation of a new agentic chat flow, the addition of BraveSearchTool and BrowserScrapeTool, and updates to the DAO layer to track search-related costs. The sortedagents package was refactored to support structured message content and token usage tracking. Feedback identifies a type mismatch in the agentic session builder that could break conversational context, suggests reorganizing service helper functions, and points out several inconsistencies in the design documentation regarding current implementation details like cost tracking and image support.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/chatservice/sortedagents/llm.go (1)
25-55:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix JSON decoding for
Message.Content(MessageContentinterface)
backend/chatservice/sortedagents/llm.godecodesChatResponsedirectly intoMessage, butMessage.Contentis a non-empty interface and there’s noUnmarshalJSONforMessage, so decoding will fail when the API returnscontentas a string, array, ornull.🛠️ Suggested fix
type Message struct { Role string `json:"role"` Content MessageContent `json:"content"` @@ ExtraContent *ExtraContent `json:"extra_content,omitempty"` } + +func (m *Message) UnmarshalJSON(data []byte) error { + var raw struct { + Role string `json:"role"` + Content json.RawMessage `json:"content"` + ToolCallID string `json:"tool_call_id,omitempty"` + ToolCalls []ToolCall `json:"tool_calls,omitempty"` + ExtraContent *ExtraContent `json:"extra_content,omitempty"` + } + if err := json.Unmarshal(data, &raw); err != nil { + return err + } + + *m = Message{ + Role: raw.Role, + ToolCallID: raw.ToolCallID, + ToolCalls: raw.ToolCalls, + ExtraContent: raw.ExtraContent, + } + + switch { + case len(raw.Content) == 0 || bytes.Equal(raw.Content, []byte("null")): + m.Content = nil + case raw.Content[0] == '"': + var text string + if err := json.Unmarshal(raw.Content, &text); err != nil { + return err + } + m.Content = TextContent(text) + case raw.Content[0] == '[': + var parts ContentParts + if err := json.Unmarshal(raw.Content, &parts); err != nil { + return err + } + m.Content = parts + default: + return fmt.Errorf("unsupported message content shape: %s", raw.Content) + } + + return nil +}🤖 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 `@backend/chatservice/sortedagents/llm.go` around lines 25 - 55, The JSON decoder fails because Message.Content is an interface (MessageContent) with no custom unmarshaler; add an UnmarshalJSON implementation that parses the raw object, decodes all normal fields, and handles the "content" RawMessage specially: if null set Message.Content = nil, if it decodes as a string set Message.Content = TextContent(value), otherwise try decoding as []ContentPart and set Message.Content = ContentParts(parts). Implement this on *Message (referencing the Message type, MessageContent, TextContent and ContentParts) so the decoder can accept "content" as string, array, or null while preserving the rest of Message fields.backend/chatservice/service/service.go (1)
460-471:⚠️ Potential issue | 🟠 Major | ⚡ Quick winImage-bearing requests still bypass the agentic path.
runAgenticChatnow accepts structured content, but this!hasImagesguard means multimodal requests never reach it. Users with tool-calling + image-capable models still lose the sortedagents/web-search flow whenever they attach an image, so the new multimodal wiring is effectively dead for the target case.Suggested fix
- if !hasImages && capabilities.GetSupportToolCalling() && providerSettings != nil && strings.TrimSpace(providerSettings.ApiUrl) != "" { + if capabilities.GetSupportToolCalling() && providerSettings != nil && strings.TrimSpace(providerSettings.ApiUrl) != "" {🤖 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 `@backend/chatservice/service/service.go` around lines 460 - 471, The current conditional prevents agentic flow for image-bearing requests because of the "!hasImages" guard; remove that "!hasImages" check so that when capabilities.GetSupportToolCalling() is true, providerSettings.ApiUrl is set, and webSearchSettings.APIKey exists the code calls runAgenticChat regardless of hasImages (so multimodal requests can use the agentic/sorted-agents + web-search path); ensure runAgenticChat invocation remains the same and that any downstream handling of structured/multimodal content inside runAgenticChat is used for images.
🤖 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 `@backend/chatservice/service/agentic_chat.go`:
- Around line 90-99: The reconstruction currently concatenates text parts then
image parts (using msg.ContentImage -> imageContents and then appending contents
to buildSortedAgentsMessage), which scrambles the original multimodal ordering;
modify the restore logic in agentic_chat.go so you parse per-part metadata
(e.g., include an index/sequence field in MessageContent or a single ordered
parts array) and merge text and image parts by that sequence before calling
buildSortedAgentsMessage; specifically, update how msg.ContentImage is
unmarshaled (MessageContent items should carry order IDs) and change the
contents assembly to sort/merge by that order so messages and
buildSortedAgentsMessage receive the original interleaved part sequence.
- Around line 77-83: The current logic in the message parsing block (checking
strings.HasPrefix/HasSuffix, calling json.Unmarshal into []*pb.MessageContent
and returning an error on unmarshal) treats any bracketed msg.Content as
serialized content and fails the session build on parse error; instead, catch
the json.Unmarshal error and fall back to treating msg.Content as a single plain
text part by appending one pb.MessageContent with Content set to msg.Content to
the contents slice (rather than returning fmt.Errorf), so
bracketed-but-not-serialized strings like "[1, 2, 3]" are replayed as raw text;
update the code around json.Unmarshal and contents append to implement this
fallback handling in the same function/block.
In `@docs/features/agentic_chat.md`:
- Around line 74-91: Update the docs to remove or reword the stale "unsupported"
statements about images in sortedagents and instead state that MessageContent
now supports images via the ContentParts/ContentPart and ImageURL types;
specifically, replace assertions like "We do not currently support images in
sortedagents" and "`sortedagents` does not yet support image input" and the
bullet "Add image support in `sortedagents`" with a concise note that
MessageContent is an interface implemented by TextContent and ContentParts and
that images are represented via ContentPart.Type == "image_url" with an
ImageURL; also fix the fenced code block to specify Go (use ```go) for the
ContentPart/ImageURL example.
---
Outside diff comments:
In `@backend/chatservice/service/service.go`:
- Around line 460-471: The current conditional prevents agentic flow for
image-bearing requests because of the "!hasImages" guard; remove that
"!hasImages" check so that when capabilities.GetSupportToolCalling() is true,
providerSettings.ApiUrl is set, and webSearchSettings.APIKey exists the code
calls runAgenticChat regardless of hasImages (so multimodal requests can use the
agentic/sorted-agents + web-search path); ensure runAgenticChat invocation
remains the same and that any downstream handling of structured/multimodal
content inside runAgenticChat is used for images.
In `@backend/chatservice/sortedagents/llm.go`:
- Around line 25-55: The JSON decoder fails because Message.Content is an
interface (MessageContent) with no custom unmarshaler; add an UnmarshalJSON
implementation that parses the raw object, decodes all normal fields, and
handles the "content" RawMessage specially: if null set Message.Content = nil,
if it decodes as a string set Message.Content = TextContent(value), otherwise
try decoding as []ContentPart and set Message.Content = ContentParts(parts).
Implement this on *Message (referencing the Message type, MessageContent,
TextContent and ContentParts) so the decoder can accept "content" as string,
array, or null while preserving the rest of Message fields.
🪄 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: f7434622-7b71-4508-afc2-82be7a824d3f
📒 Files selected for processing (10)
backend/chatservice/api/agent.gobackend/chatservice/service/agentic_chat.gobackend/chatservice/service/service.gobackend/chatservice/sortedagents/agent_test.gobackend/chatservice/sortedagents/llm.gobackend/chatservice/sortedagents/runner.gobackend/chatservice/sortedagents/runner_test.gobackend/chatservice/sortedagents/session_test.gobackend/chatservice/sortedagents/test_build.godocs/features/agentic_chat.md
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@docs/features/agentic_chat.md`:
- Line 11: The sentence containing the phrase "websearch api key" should be
updated to use proper product/API capitalization and spacing—replace "websearch
api key" with "web search API key" so the sentence reads: "If the current
message supports tool calling, web search API key is present, and provider
settings are set, we route the request through the agentic chat flow." Locate
the sentence with the exact phrase "websearch api key" and apply this
replacement.
🪄 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: f53cbca2-a013-45c3-a248-8186b0ddaaeb
📒 Files selected for processing (6)
backend/chatservice/service/agentic_chat.gobackend/chatservice/service/service.gobackend/chatservice/sortedagents/runner.gobackend/chatservice/sortedagents/runner_test.gobackend/chatservice/sortedagents/session_test.godocs/features/agentic_chat.md
💤 Files with no reviewable changes (1)
- backend/chatservice/service/service.go
🚧 Files skipped from review as they are similar to previous changes (3)
- backend/chatservice/sortedagents/runner_test.go
- backend/chatservice/sortedagents/session_test.go
- backend/chatservice/service/agentic_chat.go
Summary by CodeRabbit
New Features
Refactor
Tests
Documentation