Repository navigation
Conversation
Implements dual-model embedding architecture: - Adds Nomic Embed v1.5 model (768d) alongside Voyage-4-large (1024d) - Nomic provides ~10% higher retrieval accuracy (86.2% vs 75-77%) - New 'embeddingModel' plugin setting: 'voyage' (default) or 'nomic' - New NOMIC_API_KEY environment variable (optional) - Adds nomic_embedding column to documents table with IVFFlat index - Adds Nomic-specific RPC functions for similarity search - Issue/review_comment creation: both models store embeddings when NOMIC_API_KEY is set - Similarity search: uses configured model for query embedding Ref: ubiquity-os-marketplace#111 Devpool: devpool-directory#5064 (00)
Fixes a bug identified in code review where the Nomic similarity search was missing the current_id filter, allowing the current comment to appear in its own similarity results. Changes: - SQL: Added current_id parameter to find_similar_comments_nomic function and added WHERE id <> current_id filter - TS: Pass current_id to the RPC call Refs: - ubiquity-os-marketplace#111 - devpool-directory/devpool-directory#5064 - Code review: PR ubiquity-os-marketplace#155 comment from coderabbitai
Related issues suggestionsSelect one (or more) issue(s) to link: |
Unused exports (1)
Unused types (1)
|
📝 WalkthroughWalkthroughThis change introduces support for Nomic embeddings as an alternative to Voyage embeddings throughout the system. It adds a new 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
src/adapters/index.ts (1)
14-14: Unused type export flagged by pipeline.
EmbeddingModelis exported but never imported elsewhere. Either use it incomment.ts/issues.tsinstead of the inline"voyage" | "nomic"type, or remove it.src/adapters/supabase/helpers/comment.ts (1)
47-50: Consider using the exportedEmbeddingModeltype fromsrc/adapters/index.ts.The return type
"voyage" | "nomic"is defined inline here but also exported asEmbeddingModelinsrc/adapters/index.ts. Using the shared type would ensure consistency if more models are added.♻️ Proposed change
+import { EmbeddingModel } from "../../../adapters/index"; + -function getEmbeddingModel(context: Context): "voyage" | "nomic" { +function getEmbeddingModel(context: Context): EmbeddingModel { const config = context.config as PluginSettings | undefined; return config?.embeddingModel ?? "voyage"; }This would also address the unused
EmbeddingModelexport flagged by pipeline.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0b4d01a4-18a7-44b9-b610-da1a10db0575
📒 Files selected for processing (11)
README.mdsrc/adapters/index.tssrc/adapters/nomic/helpers/embedding.tssrc/adapters/nomic/helpers/nomic.tssrc/adapters/supabase/helpers/comment.tssrc/adapters/supabase/helpers/issues.tssrc/cron/reprocess.tssrc/types/database.tssrc/types/env.tssrc/types/plugin-input.tssupabase/migrations/20260331000000_nomic_embedding.sql
| export function serializeEmbeddingForDatabase(embedding: number[] | null): string | null { | ||
| return embedding ? JSON.stringify(embedding) : null; | ||
| } |
There was a problem hiding this comment.
Duplicate function — already exists in src/utils/database-embedding.ts.
Pipeline reports this as unused. The same function is defined and imported from src/utils/database-embedding.ts (see context snippet 3 and line 5 of comment.ts). Remove this duplicate.
🗑️ Proposed fix
-
-export function serializeEmbeddingForDatabase(embedding: number[] | null): string | null {
- return embedding ? JSON.stringify(embedding) : null;
-}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function serializeEmbeddingForDatabase(embedding: number[] | null): string | null { | |
| return embedding ? JSON.stringify(embedding) : null; | |
| } |
🧰 Tools
🪛 GitHub Actions: Knip
[error] 54-54: KNIP reported unused exports: 'serializeEmbeddingForDatabase' is unused (Unused exports (1)).
| const embeddings = await this.createEmbeddings([text], inputType); | ||
| return embeddings[0] ?? []; |
There was a problem hiding this comment.
Reject malformed embeddings instead of returning them.
Line 26 can return [], and Lines 67-70 only warn on a dimension mismatch. That pushes bad vectors into vector(768) writes/RPCs and fails later. Throw as soon as the response count or dimensions are wrong.
Possible fix
const embeddings = await this.createEmbeddings([text], inputType);
- return embeddings[0] ?? [];
+ const [embedding] = embeddings;
+ if (!embedding) {
+ throw new Error("Invalid response from Nomic API: expected one embedding");
+ }
+ return embedding;
@@
- const dims = data.embeddings[0]?.length;
- if (dims !== NOMIC_EMBEDDING_DIM) {
- this.context.logger.warn(`Nomic embedding dimension mismatch. Expected ${NOMIC_EMBEDDING_DIM}, got ${dims}`);
+ const invalidEmbedding = data.embeddings.find((embedding) => embedding.length !== NOMIC_EMBEDDING_DIM);
+ if (invalidEmbedding) {
+ throw new Error(`Invalid response from Nomic API: expected ${NOMIC_EMBEDDING_DIM} dimensions, got ${invalidEmbedding.length}`);
}Also applies to: 67-73
| const response = await fetch("https://api.atlas.nomic.ai/v1/embedding/text", { | ||
| method: "POST", | ||
| headers: { | ||
| "Content-Type": "application/json", | ||
| Authorization: `Bearer ${apiKey}`, | ||
| }, | ||
| body: JSON.stringify({ | ||
| texts, | ||
| model: NOMIC_MODEL, | ||
| task_type: inputType, | ||
| truncation: true, | ||
| }), | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does the JS runtime targeted by this repo provide any default timeout for fetch, or do outbound requests need an explicit AbortController/signal to avoid hanging for too long?
💡 Result:
The JS runtime targeted by this repo (likely Bun, Deno, or Node.js based on common modern JS server-side runtimes with fetch) does provide default timeouts for fetch in some cases, but behavior varies: - Bun: Yes, implements a default ~5-minute timeout (increased from 60s in 2023 via PR #6217). Can be disabled with {timeout: false} (Bun-specific, undocumented in types). Official docs recommend AbortSignal.timeout for explicit timeouts, implying no standard signal default but runtime-level timeout exists. Requests hang until this timeout without explicit signal. - Deno: No default timeout for fetch. Requests can hang indefinitely until TCP/gateway timeout unless explicit AbortController/signal provided. Docs and sources confirm reliance on user-provided AbortSignal for timeouts. - Node.js (undici fetch): Yes, multiple defaults (e.g., connect ~10s, headers ~5min/300s). Times out automatically (e.g., UND_ERR_HEADERS_TIMEOUT); configure via custom Agent/dispatcher or AbortSignal. Without repo link, assuming Bun (most matching evidence), outbound requests have a default 5min timeout but need explicit AbortController/signal (or Bun's {timeout: false} to disable) for custom shorter timeouts to avoid hanging up to 5min.
Citations:
- 1: https://bun.com/docs/runtime/networking/fetch
- 2: https://bun.sh/docs/runtime/networking/fetch
- 3: Set
fetchtimeout to 5 minutes oven-sh/bun#6217 - 4: Change default
fetchtimeout to be longer oven-sh/bun#5930 - 5: https://medium.com/deno-the-complete-reference/timeout-fetch-request-in-deno-2bea84dd52de
- 6: Respect timeout passed to fetch oven-sh/bun#16682
- 7: https://medium.com/deno-the-complete-reference/fetch-timeout-in-deno-91731bca80a1
- 8: https://docs.deno.com/api/web/~/fetch
- 9: https://docs.deno.com/runtime/reference/web_platform_apis
- 10: Default fetch timeout too short nodejs/undici#1373
- 11: fetch fails with internal error after 5 minutes nodejs/node#46375
- 12: fetch times out in under 5 seconds nodejs/undici#1531
🏁 Script executed:
# Check for package.json and identify runtime target
cd /tmp && find . -maxdepth 2 -name "package.json" -o -name "deno.json" -o -name "tsconfig.json" | head -20Repository: ubiquity-os-marketplace/text-vector-embeddings
Length of output: 72
🏁 Script executed:
# First, find the exact file
fd -t f "embedding.ts" --path "*nomic*"Repository: ubiquity-os-marketplace/text-vector-embeddings
Length of output: 323
🏁 Script executed:
# Check package.json for runtime clues (bun, node, deno indicators)
cat package.json 2>/dev/null | head -50Repository: ubiquity-os-marketplace/text-vector-embeddings
Length of output: 1956
🏁 Script executed:
# Find and read the embedding.ts file
find . -type f -name "embedding.ts" 2>/dev/null | head -5Repository: ubiquity-os-marketplace/text-vector-embeddings
Length of output: 174
🏁 Script executed:
# Check if the file exists at the specified path
cat -n src/adapters/nomic/helpers/embedding.ts 2>/dev/null | sed -n '35,60p'Repository: ubiquity-os-marketplace/text-vector-embeddings
Length of output: 953
🏁 Script executed:
# Search for AbortController or timeout patterns in the codebase
rg -i "abortcontroller|abortSignal|signal.*controller" --type ts --type jsRepository: ubiquity-os-marketplace/text-vector-embeddings
Length of output: 72
🏁 Script executed:
# Search for other fetch calls in the codebase
rg "fetch\(" --type ts --type js -A 5 | head -80Repository: ubiquity-os-marketplace/text-vector-embeddings
Length of output: 1512
🏁 Script executed:
# Check if there's any timeout configuration or patterns in Nomic adapter
cat -n src/adapters/nomic/helpers/embedding.tsRepository: ubiquity-os-marketplace/text-vector-embeddings
Length of output: 2872
Add a timeout to the Nomic API request.
This fetch() call has no abort signal. A slow or unresponsive Nomic API can block embedding operations indefinitely, holding up writes and similarity lookups. Node.js provides no short default timeout for fetch—requests can hang for several minutes. Add an explicit AbortController with a bounded timeout and handle the AbortError to surface timeout failures clearly.
| const updateData: Record<string, unknown> = { | ||
| doc_type: docType, | ||
| markdown: finalMarkdown, | ||
| embedding: serializeEmbeddingForDatabase(embedding), | ||
| payload: finalPayload, | ||
| modified_at: new Date(), | ||
| }; | ||
|
|
||
| if (nomicEmbedding) { | ||
| updateData.nomic_embedding = serializeEmbeddingForDatabase(nomicEmbedding); | ||
| } |
There was a problem hiding this comment.
Clear stale nomic_embedding on update.
Line 210 only writes nomic_embedding when a fresh vector exists. The _nomic RPCs in supabase/migrations/20260331000000_nomic_embedding.sql Lines 83-90 and 105-112 only check nomic_embedding IS NOT NULL, so a private or shortened issue can remain searchable via its old vector. Persist null here when no new Nomic embedding was produced.
Possible fix
const updateData: Record<string, unknown> = {
doc_type: docType,
markdown: finalMarkdown,
embedding: serializeEmbeddingForDatabase(embedding),
+ nomic_embedding: serializeEmbeddingForDatabase(nomicEmbedding),
payload: finalPayload,
modified_at: new Date(),
};
-
-if (nomicEmbedding) {
- updateData.nomic_embedding = serializeEmbeddingForDatabase(nomicEmbedding);
-}| if (model === "nomic" && isNomicAvailable(this.context)) { | ||
| const nomicEmbedding = await this.context.adapters.nomic.embedding.createEmbedding(embeddingSource); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For nomic-embed-text-v1.5, should retrieval queries use task_type: "search_query"while indexed documents usetask_type: "search_document"?
💡 Result:
Yes, for nomic-embed-text-v1.5, retrieval queries should use task_type: "search_query" while indexed documents should use task_type: "search_document".
Citations:
- 1: https://docs.nomic.ai/reference/endpoints/nomic-embed-text
- 2: https://docs.nomic.ai/api/python-api/generate-embeddings
- 3: https://huggingface.co/nomic-ai/nomic-embed-text-v1.5
- 4: https://zilliz.com/ai-models/nomic-embed-text-v1.5
- 5: https://docs.nomic.ai/api/embeddings-and-retrieval/guides/fast-retrieval-with-resizable-embeddings
- 6: https://docs.nomic.ai/reference/api/embed-text-v-1-embedding-text-post
🏁 Script executed:
# First, check the Nomic embedding helper to see the default behavior
head -40 src/adapters/nomic/helpers/embedding.ts | cat -nRepository: ubiquity-os-marketplace/text-vector-embeddings
Length of output: 1583
🏁 Script executed:
# Check lines around 287 and 354 in the issues file
sed -n '280,295p' src/adapters/supabase/helpers/issues.ts | cat -n
sed -n '350,360p' src/adapters/supabase/helpers/issues.ts | cat -nRepository: ubiquity-os-marketplace/text-vector-embeddings
Length of output: 1335
🏁 Script executed:
# Search for all createEmbedding calls to the Nomic adapter
rg "nomic\.embedding\.createEmbedding" --context 3Repository: ubiquity-os-marketplace/text-vector-embeddings
Length of output: 5137
Pass "search_query" for query-time Nomic embeddings.
The createEmbedding() method defaults to "search_document", but lines 287 and 354 are retrieval queries that require "search_query" for correct embedding semantics. This affects retrieval quality.
Suggested fix
- const nomicEmbedding = await this.context.adapters.nomic.embedding.createEmbedding(embeddingSource);
+ const nomicEmbedding = await this.context.adapters.nomic.embedding.createEmbedding(embeddingSource, "search_query");Apply to both locations (lines 287 and 354).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (model === "nomic" && isNomicAvailable(this.context)) { | |
| const nomicEmbedding = await this.context.adapters.nomic.embedding.createEmbedding(embeddingSource); | |
| if (model === "nomic" && isNomicAvailable(this.context)) { | |
| const nomicEmbedding = await this.context.adapters.nomic.embedding.createEmbedding(embeddingSource, "search_query"); |
| sub.similarity | ||
| FROM ( | ||
| SELECT id AS issue_id, | ||
| ((0.8 * (1 - cosine_distance(query_embedding, nomic_embedding))) + 0.8 * (1 / (1 + l2_distance(query_embedding, nomic_embedding)))) as similarity |
There was a problem hiding this comment.
Fix the blended-score coefficients.
Lines 41 and 64 weight both terms at 0.8, unlike the other RPCs in this file where the weights sum to 1.0. That lets these scores reach 1.6 and makes threshold filtering much looser than intended. The second coefficient looks like it should be 0.2.
Possible fix
- ((0.8 * (1 - cosine_distance(query_embedding, nomic_embedding))) + 0.8 * (1 / (1 + l2_distance(query_embedding, nomic_embedding)))) as similarity
+ ((0.8 * (1 - cosine_distance(query_embedding, nomic_embedding))) + 0.2 * (1 / (1 + l2_distance(query_embedding, nomic_embedding)))) as similarityApply the same change to both find_similar_issues_nomic and find_similar_comments_nomic.
Also applies to: 64-64
Summary
Fixes a bug identified in code review where the Nomic similarity search was missing the current_id filter, allowing the current comment to appear in its own similarity results.
Changes
SQL Migration (20260331000000_nomic_embedding.sql)
current_idparameter tofind_similar_comments_nomicfunctionWHERE id <> current_idfilter to exclude the current commentTypeScript (src/adapters/supabase/helpers/comment.ts)
current_idto thefind_similar_comments_nomicRPC callBug Details
The original implementation in PR #155 called
find_similar_comments_nomicwithout passingcurrent_id, while the equivalent Voyage function (find_similar_comments_annotate) correctly filtered bycurrent_id. This could cause the current comment to appear in its own similarity search results.References