feat: streaming chat, Gemini support, and embedding wrapper cleanup (PR 4 of 4)#175
Open
dpage wants to merge 9 commits into
Open
feat: streaming chat, Gemini support, and embedding wrapper cleanup (PR 4 of 4)#175dpage wants to merge 9 commits into
dpage wants to merge 9 commits into
Conversation
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR integrates three major changes: Google Gemini support as an LLM provider, Server-Sent Events streaming for the web chat interface, and a refactoring that migrates embedding tools from a dedicated package to direct ChangesGemini LLM Provider Support
Web Chat SSE Streaming
Embedding Tool Refactoring
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 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)
Comment |
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Up to standards ✅🟢 Issues
|
| Category | Results |
|---|---|
| Complexity | 4 medium |
🟢 Metrics 113 complexity · 3 duplication
Metric Results Complexity 113 Duplication 3
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
dpage
force-pushed
the
feat/llm-lib-pr3-proxy-adoption
branch
from
0dc2948 to
05b84bf
Compare
dpage
force-pushed
the
feat/llm-lib-pr4-streaming-gemini
branch
from
a0feece to
cf3e0fd
Compare
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
internal/tools/embed_client.go (1)
40-43: ⚡ Quick winNormalize provider input before dispatch/client creation.
At Line 42 and Line 85, provider handling is strict (
cfg.Providerexact match). Values like"OpenAI"or" openai "will fail even though they’re logically valid config inputs. Normalize once and use the normalized value inswitchandllm.NewClient.♻️ Suggested patch
import ( "fmt" + "strings" @@ func newEmbedClient(cfg embedClientConfig) (llm.Client, string, error) { var opts llm.Options - switch cfg.Provider { + provider := strings.ToLower(strings.TrimSpace(cfg.Provider)) + switch provider { @@ default: return nil, "", fmt.Errorf( "unsupported embedding provider: %s (supported: voyage, openai, ollama)", - cfg.Provider, + provider, ) } - client, err := llm.NewClient(cfg.Provider, opts) + client, err := llm.NewClient(provider, opts) if err != nil { - return nil, "", fmt.Errorf("create %s embedding client: %w", cfg.Provider, err) + return nil, "", fmt.Errorf("create %s embedding client: %w", provider, err) }Also applies to: 85-87
🤖 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 `@internal/tools/embed_client.go` around lines 40 - 43, The provider string in newEmbedClient is not normalized before switching and creating the client, so inputs like "OpenAI" or " openai " are rejected; fix by trimming whitespace and lowercasing cfg.Provider at the start of newEmbedClient (e.g., set a local normalized variable from cfg.Provider via strings.TrimSpace and strings.ToLower) and use that normalized value in the switch statement and when calling llm.NewClient so dispatch and client creation accept case/whitespace-insensitive provider names.
🤖 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/changelog.md`:
- Around line 68-75: The Unreleased changelog currently contains conflicting
bullets about web chat streaming; update the entries so they are consistent by
either (a) stating that the web chat now consumes the streaming endpoint
`/api/llm/v1/chat/stream` while still retaining the non-streaming endpoint for
compatibility, and referencing the new helper `web/src/utils/sseChat.js` that
assembles SSE chunks, or (b) revert the new bullet if streaming is not yet
enabled; ensure both bullets convey the same status
(streaming-enabled-with-fallback or not enabled) and remove or rewrite the
earlier non-streaming sentence so the Unreleased section is unambiguous.
In `@internal/config/config.go`:
- Around line 458-459: mergeConfig() currently omits copying Gemini API fields
from src.LLM so yaml-provided gemini_api_key / gemini_api_key_file are ignored;
update mergeConfig() to explicitly copy src.LLM.GeminiAPIKey and
src.LLM.GeminiAPIKeyFile into the runtime config (the same way other LLM fields
are merged), e.g., when merging LLM config fields ensure GeminiAPIKey and
GeminiAPIKeyFile on the destination config are set from src.LLM if present, and
add analogous handling in the same merge block that handles the other LLM fields
referenced in mergeConfig().
In `@web/src/components/ChatInterface.jsx`:
- Around line 1302-1331: handlePromptExecute lacks abort handling so streaming
sseChat calls can't be cancelled; create an AbortController in
handlePromptExecute (after setLoading(true)), save its signal and pass signal to
the sseChat invocation (the same call that supplies
messages/tools/provider/model and onTextChunk), and wire cancellation so user
cancel triggers controller.abort(); also update the catch in handlePromptExecute
to treat AbortError specially (ignore or set appropriate state) and ensure any
outer try/catch that awaits handlePromptExecute recognizes aborts—refer to
symbols handlePromptExecute, AbortController, sseChat, onTextChunk, and
setLoading to locate where to add the controller, pass signal, and handle
AbortError.
---
Nitpick comments:
In `@internal/tools/embed_client.go`:
- Around line 40-43: The provider string in newEmbedClient is not normalized
before switching and creating the client, so inputs like "OpenAI" or " openai "
are rejected; fix by trimming whitespace and lowercasing cfg.Provider at the
start of newEmbedClient (e.g., set a local normalized variable from cfg.Provider
via strings.TrimSpace and strings.ToLower) and use that normalized value in the
switch statement and when calling llm.NewClient so dispatch and client creation
accept case/whitespace-insensitive provider names.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 45165b18-a6a5-472e-8e9e-75b22fe5937e
📒 Files selected for processing (15)
cmd/pgedge-pg-mcp-svr/main.godocs/changelog.mdinternal/config/config.gointernal/embedding/libprovider.gointernal/embedding/libprovider_test.gointernal/embedding/provider.gointernal/embedding/provider_test.gointernal/tools/embed_client.gointernal/tools/generate_embedding.gointernal/tools/search_knowledgebase.gointernal/tools/similarity_search.goweb/src/components/ChatInterface.jsxweb/src/components/Message.jsxweb/src/utils/sseChat.jsweb/src/utils/sseChat.test.js
💤 Files with no reviewable changes (4)
- internal/embedding/provider.go
- internal/embedding/provider_test.go
- internal/embedding/libprovider_test.go
- internal/embedding/libprovider.go
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds GeminiAPIKey / GeminiAPIKeyFile fields to LLMConfig with the same priority resolution as the other providers (env > file > config). The server now registers a "gemini" entry in the proxy's provider map when the key is set. The pgedge-go-llm-lib registers the gemini provider via the existing _ "llm/all" / _ "llm/provider/gemini" imports. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The three embedding-consuming tools (search_knowledgebase, generate_embedding, similarity_search) no longer go through the internal/embedding wrapper layer. A shared internal/tools/embed_client.go helper builds an llm.Client from the per-provider config fields, applying the same defaults the wrapper used to apply. newEmbedClient returns (llm.Client, string, error) where the string is the resolved model name (after defaults such as voyage-3-lite and nomic-embed-text are applied). generate_embedding.go uses this resolved model for metadata display instead of provider.ModelName(); it uses len(vec) for dimensions (populated after the Embed call) and cfg.Embedding.Provider for the provider name. The embedding package now has no consumers outside itself; it is ready to be deleted in the next commit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
After PR 4's tool refactor (previous commit) nothing in the codebase imports internal/embedding. The package is removed entirely — its responsibilities are split between pgedge-go-llm-lib (provider wire code) and internal/tools/embed_client.go (config translation + defaults). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds an sseChat helper that POSTs to the library proxy's streaming endpoint, parses Server-Sent Events, and assembles the final response into the same shape the non-streaming /v1/chat endpoint returns. Text chunks update the assistant message bubble incrementally as they arrive. The agentic loop (chat -> tool_use -> tool_result -> chat ...) keeps working unchanged: sseChat returns once event: done arrives, the caller inspects stop_reason and dispatches the next iteration. Message.jsx now renders streamed partial text while keeping a thinking indicator visible, so the user sees progress as chunks arrive instead of just a spinner. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…(PR 4) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
staticcheck (ST1005) rejects error strings that begin with a capital letter. The two missing-API-key messages copied across from the old embedding factory both started with the provider name. Switch to a "missing <key> for embedding provider '<name>'" phrasing that starts with a lowercase verb. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
handlePromptExecute previously had no abort handling, so the streaming sseChat call could not be cancelled by the user. Mirror the AbortController pattern already used in handleSend: create a controller, pass its signal to sseChat, treat AbortError as a benign cancellation in the catch block. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- mergeConfig() now copies the Gemini API key and key-file fields alongside the other LLM provider fields. Without this, a gemini_api_key in the YAML config would be silently dropped at load time. - newEmbedClient() normalises cfg.Provider via TrimSpace+ToLower before the switch and llm.NewClient call. Inputs like "OpenAI" or " openai " are now accepted; the error message and llm.NewClient receive the normalised value. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
dpage
force-pushed
the
feat/llm-lib-pr4-streaming-gemini
branch
from
cf3e0fd to
3f6070f
Compare
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
4 tasks
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
ChatInterface.jsxnow POSTs to/api/llm/v1/chat/streamand renders the response incrementally as SSE chunks arrive. A newsseChathelper inweb/src/utils/parses the wire format and assembles a final response shape identical to the non-streaming endpoint, so the agentic tool-call loop downstream of the fetch site is unchanged.gemini_api_key/gemini_api_key_fileconfig fields with the same priority chain as the other providers (env > file > config). Registered in the proxy's provider map when set.search_knowledgebase,generate_embedding, andsimilarity_searchnow construct anllm.Clientdirectly via a smallinternal/tools/embed_client.gohelper. Theinternal/embedding/package is deleted entirely (~510 lines removed).This is PR 4 of 4 in the LLM library migration. Based on PR 3 (#174). Will need rebase onto main after PR 1, PR 2, PR 3 land.
Deferred to a follow-up PR
The original PR 4 plan also included:
internal/chat/client.go(the CLI) to usellm.Clientdirectly instead of thechat.LLMClientwrapper.chat.LLMClient,libClient, and the translation helpers added in PR 1.These remain in the codebase because they only affect the CLI and don't change user-visible behaviour. A separate clean-up PR will land them.
Test plan
make testpasses (all 10 server packages, plus the unchanged chat package).npm run buildsucceeds; 93/93 unit tests pass (7 new forsseChat).🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Changed