Conversation
Made-with: Cursor
Made-with: Cursor
Made-with: Cursor
✱ Stainless preview buildsThis PR will update the This comment is auto-generated by GitHub Actions and is automatically kept up to date as you push. |
|
@coderabbitai can you review please? |
|
✅ Actions performedReview triggered.
|
WalkthroughThis pull request refactors the embedding client architecture to support multiple providers through a centralized, provider-agnostic factory pattern. It adds support for Google Vertex AI as an embedding provider alongside existing OpenAI and Google AI Studio options. The changes include a new embedding client factory module, configuration enhancements for Google Cloud project and location settings, a new Vertex AI client constructor, and updated validation logic across initialization code. Configuration guidance and documentation are updated to reflect the newly supported providers. Tests are added for new configuration paths and factory logic. 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/config/config_test.go`:
- Around line 272-326: Add tests that assert EMBEDDING_GOOGLE_CLOUD_* env vars
take precedence over GOOGLE_CLOUD_* when both are set: create two new tests
(e.g., TestLoad_EmbeddingGoogleCloudProject_precedence and
TestLoad_EmbeddingGoogleCloudLocation_precedence) that call t.Setenv for
API_KEY, set EMBEDDING_GOOGLE_CLOUD_PROJECT="explicit-project" and
GOOGLE_CLOUD_PROJECT="fallback-project", call Load(), and assert
cfg.EmbeddingGoogleCloudProject == "explicit-project"; do the same pattern for
EMBEDDING_GOOGLE_CLOUD_LOCATION vs GOOGLE_CLOUD_LOCATION asserting
cfg.EmbeddingGoogleCloudLocation == "explicit-location".
In `@internal/googleai/client.go`:
- Around line 81-102: NewVertexClient should validate its inputs before calling
genai.NewClient: check that the project string is not empty and return a
descriptive error (e.g., "empty project") if it is, and also check that the
location string is not empty and return a descriptive error (e.g., "empty
location") if it is; perform these checks at the start of NewVertexClient
(before genai.NewClient is invoked) so the function fails fast with consistent
messages while leaving the rest of the function (client creation, applying opts,
returning client) unchanged.
In `@internal/service/embedding_client_factory_test.go`:
- Around line 11-112: Add a test case in TestNewEmbeddingClient that ensures
provider name normalization is exercised for the google-vertex provider: create
an EmbeddingClientConfig with Provider set to a mixed-case and whitespace
variant (e.g. " Google-Vertex ") and the same fields as the existing
EmbeddingProviderGoogleVertex case, then assert the same error behavior
(ErrEmbeddingVertexConfig) or success as appropriate; this verifies
NewEmbeddingClient and any provider-trimming/lowercasing logic correctly
normalizes the provider string (referencing NewEmbeddingClient,
EmbeddingClientConfig, and EmbeddingProviderGoogleVertex).
In `@internal/service/embedding_client_factory.go`:
- Around line 41-109: The provider capability matrix is duplicated across
NewEmbeddingClient, ProviderRequiresAPIKey, ProviderRequiresVertexConfig, and
SupportedEmbeddingProviders causing inconsistent checks; consolidate into a
single source of truth by introducing a registry (map/struct) keyed by provider
that declares capabilities (requiresAPIKey, requiresVertexConfig, model support,
factory function) and then refactor NewEmbeddingClient to consult that registry
to build clients, implement a new exported ValidateEmbeddingClientConfig(cfg
EmbeddingClientConfig) (or similar) that uses the same registry to validate
preconditions, and replace the existing ProviderRequiresAPIKey,
ProviderRequiresVertexConfig, and SupportedEmbeddingProviders callers to use the
registry/ValidateEmbeddingClientConfig so all provider metadata lives in one
place.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 09c86b9a-f9ce-48a6-abf6-16cbb485453f
📒 Files selected for processing (13)
.env.examplecmd/api/app.gocmd/backfill-embeddings/main.gointernal/config/config.gointernal/config/config_test.gointernal/googleai/client.gointernal/googleai/client_test.gointernal/service/embedding_client_factory.gointernal/service/embedding_client_factory_test.gointernal/service/embedding_provider.gointernal/service/embedding_provider_test.gointernal/service/webhook_sender.goopenapi.yaml
mattinannt
left a comment
There was a problem hiding this comment.
@xernobyl Thanks a lot for the PR :-) Looks great!
What does this PR do?
Adds Google Vertex AI as an embedding provider (
google-vertex) alongsideopenaiandgoogle(AI Studio).EMBEDDING_PROVIDER=google-vertex; uses Application Default Credentials (no API key).EMBEDDING_GOOGLE_CLOUD_PROJECT,EMBEDDING_GOOGLE_CLOUD_LOCATION(with fallbacks toGOOGLE_CLOUD_PROJECT/GOOGLE_CLOUD_LOCATION).internal/service/embedding_client_factory.gocentralizes client creation and provider validation; API and backfill-embeddings use it.googleai.NewVertexClient(ctx, project, location, opts...)for Vertex backend; existing AI Studio client unchanged..env.exampleand OpenAPI updated forgoogle-vertexand the new env vars.No API contract changes; existing embedding and semantic-search endpoints work with the new provider when configured.
How should this be tested?
make testsandmake tests-coverage(embedding client factory, config, googleai client tests).make build,make fmt,make lint.EMBEDDING_PROVIDER=google-vertex,EMBEDDING_GOOGLE_CLOUD_PROJECT,EMBEDDING_GOOGLE_CLOUD_LOCATION,EMBEDDING_MODEL; ensure ADC (e.g.gcloud auth application-default login); start API, create a feedback record, confirm embedding job and semantic search work.Checklist
Required
make buildmake tests(integration tests intests/)make fmtandmake lint; no new warningsgit pull origin mainmigrations/with goose annotations and ranmake migrate-validateAppreciated
make testsor API contract workflow)docs/if changes were necessarymake tests-coveragefor meaningful logic changes