Repository navigation
v2.1.1 — Local embeddings (semantic search) - #54
Merged
Merged
Conversation
Adds local-first embeddings (nomic-ai/nomic-embed-text-v1.5 via @xenova/transformers + onnxruntime-node) powering semantic Knowledge Graph edges, the Semantic Hubs section in the Backlinks panel, and incremental auto-reindex on note save. Features - New electron/embeddings/ module: HTTP worker binary (cairn-embeddings), spawn/health/dispose lifecycle, model manifest, cosine + UMAP projection, chunked-embed-and-average for long notes, LRU query vector cache - v17 DB migration (note_embeddings table, JSON-TEXT vector storage) - Settings → Embeddings UI: model install/remove/setDefault, reindex + recompute buttons with progress that survives view switches - Graph: 'semantic' edge type with 0.78 cosine threshold + opacity slider - BacklinksPanel: 'Semantic' section showing top-5 similar notes - Incremental reindex on note save (skipped if content_hash unchanged) Fixes - chunkLongText infinite loop → OOM crash (terminates on long input) - Model-install status never detected (transformers.js nested dir layout) - Recompute overwrote search_document embeddings (single-task consolidation) - Worker killed during in-flight requests (inFlightEmbeds guard) - Progress state lost on Settings remount (getStatus now exposes last done/total) - Duplicate concurrent reindex calls (withLock mutex) - UMAP crashed on single-vector input (nNeighbors <= nPoints - 1) - Notes with zero backlinks hid the Semantic section (auto-show) Tests - electron/embeddings/cosine.test.ts (12 tests) - electron/embeddings/projection.test.ts (8 tests) - electron/embeddings/chunking.test.ts (8 tests, incl. OOM regression) - electron/embeddings/query-cache.test.ts (5 tests) 33 tests passing; type-check:all + compile verified clean.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a complete local semantic search subsystem: a packaged HTTP embeddings worker (Node/binary) running Nomic text embeddings via ChangesLocal Embeddings & Semantic Search
Sequence DiagramsequenceDiagram
participant Renderer as Renderer
participant Preload as preload.ts
participant IPCHandlers as IPC handlers
participant EmbeddingsService as embeddings/service
participant EmbeddingsClient as embeddings/client
participant Worker as embeddings-server
participant DB as SQLite
Renderer->>Preload: electron.embeddings.reindex(workspaceId)
Preload->>IPCHandlers: invoke("db:embeddings:reindex")
IPCHandlers->>EmbeddingsService: reindexNotes(workspaceId)
EmbeddingsService->>EmbeddingsClient: embed(chunked texts, search_document)
EmbeddingsClient->>Worker: POST /embed
Worker-->>EmbeddingsClient: {vectors, dim, model}
EmbeddingsClient-->>EmbeddingsService: number[][]
EmbeddingsService->>DB: upsertNoteEmbedding
EmbeddingsService-->>IPCHandlers: ReindexResult
IPCHandlers->>DB: computeSemanticRelationships
IPCHandlers-->>Preload: result
Preload-->>Renderer: {indexed, skipped, total}
Renderer->>Preload: electron.embeddings.search(workspaceId, queryText)
Preload->>IPCHandlers: invoke("db:embeddings:search")
IPCHandlers->>EmbeddingsService: searchAdjacent(queryText)
EmbeddingsService->>EmbeddingsClient: embed([queryText], search_query)
EmbeddingsClient->>Worker: POST /embed
Worker-->>EmbeddingsClient: vectors
EmbeddingsService->>DB: topK similarity
DB-->>EmbeddingsService: AdjacentNote[]
EmbeddingsService-->>IPCHandlers: results
IPCHandlers-->>Preload: AdjacentNote[]
Preload-->>Renderer: displayed in BacklinksPanel
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 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)
Comment |
…ct patterns
- electron/embeddings/client.ts: import execSync from 'child_process' instead
of inline require()
- BacklinksPanel: refactor semantic search into a state-machine SearchState
union {kind:'loading'} | {kind:'results', hits}; setState calls moved to
async callbacks and effect cleanup
- BacklinksPanel: replace autoOpened useState with useRef for the lock flag
- RadialTreeCanvas: add semanticThreshold to useCallback deps array
- note-editor: remove unused SemanticHubsPanel import; delete the now-orphan
component file
- note-editor: explicit eslint-disable for the canonical sync-on-prop-change
pattern (wordCount/semanticContent reset on note.id switch)
npm run lint: 0 errors 0 warnings; exit 0
npm run type-check:all: clean
npm test: 463 tests passing
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (21)
scripts/build-embeddings-binary.js-37-37 (1)
37-37:⚠️ Potential issue | 🟠 Major | ⚡ Quick winLinux binary name mismatches runtime lookup contract.
target.outfor Linux iscairn-embeddings-linux, butelectron/embeddings/client.tsresolves packaged non-Windows binaries asdist-embeddings/cairn-embeddings. On Linux packages, worker discovery will fail and embeddings won’t start.Suggested fix
-if (wantLinux || (!wantMac && !wantWin && !wantLinux && platform === "linux")) targets.push({ id: "node22-linux-x64", out: "cairn-embeddings-linux" }); +if (wantLinux || (!wantMac && !wantWin && !wantLinux && platform === "linux")) targets.push({ id: "node22-linux-x64", out: "cairn-embeddings" });🤖 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 `@scripts/build-embeddings-binary.js` at line 37, The Linux binary output name `cairn-embeddings-linux` defined in the targets array does not match the binary name that the runtime expects when looking up packaged non-Windows binaries in electron/embeddings/client.ts, which resolves to `cairn-embeddings`. Change the Linux target.out value from `cairn-embeddings-linux` to `cairn-embeddings` to align with the runtime's binary discovery contract and ensure the embeddings worker can be located and started on Linux packages.src/components/notes/SemanticHubsPanel.tsx-40-69 (1)
40-69:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrevent stale semantic results from out-of-order async completions.
Overlapping
api.search(...)requests can resolve in reverse order and replace newer results with older content matches. Gate state updates so only the latest request can commit state.Proposed fix
-import React, { useEffect, useState, useCallback } from "react"; +import React, { useEffect, useState, useCallback, useRef } from "react"; @@ export function SemanticHubsPanel({ workspaceId, noteId, content, className, onSelectNote }: Props) { const [results, setResults] = useState<AdjacentNote[]>([]); const [loading, setLoading] = useState(false); const [error, setError] = useState<string | null>(null); const [enabled, setEnabled] = useState(false); + const requestIdRef = useRef(0); @@ const fetchAdjacent = useCallback(async () => { - if (!workspaceId || !content || !enabled) { + const requestId = ++requestIdRef.current; + if (!workspaceId || !content || !enabled) { setResults([]); + setLoading(false); return; } setLoading(true); setError(null); try { const trimmed = content.trim(); if (trimmed.length < 4) { setResults([]); return; } const api = window.electron?.embeddings; if (!api) { setResults([]); return; } const res = await api.search(workspaceId, trimmed, { queryNoteId: noteId, k: 5, }); - setResults(res); + if (requestId === requestIdRef.current) setResults(res); } catch (e) { - setError(e instanceof Error ? e.message : String(e)); - setResults([]); + if (requestId === requestIdRef.current) { + setError(e instanceof Error ? e.message : String(e)); + setResults([]); + } } finally { - setLoading(false); + if (requestId === requestIdRef.current) setLoading(false); } }, [workspaceId, content, noteId, enabled]);Also applies to: 71-76
🤖 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/components/notes/SemanticHubsPanel.tsx` around lines 40 - 69, The fetchAdjacent function has a race condition where overlapping api.search calls can resolve out of order, causing older results to overwrite newer ones. Fix this by creating an AbortController to track the latest request and gate all state updates (setResults, setError, setLoading) so only the most recent request can commit state changes. Create the AbortController before calling api.search, pass its signal to the search call if supported, and wrap the try/catch/finally state updates with a check to ensure the request hasn't been aborted or superseded by a newer request. Also update the useCallback dependency array to include any tracking variables you introduce.src/components/notes/note-editor.tsx-63-64 (1)
63-64:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid double-debouncing semantic content before search.
NoteEditordebounces before passing props, andBacklinksPaneldebounces again (src/components/notes/BacklinksPanel.tsx, Line [42]). This adds ~2.4s latency and can briefly query previous-note text after note switches.Proposed fix
import { SemanticHubsPanel } from "./SemanticHubsPanel"; -import { useDebouncedValue } from "`@/hooks/useDebouncedValue`"; @@ const [showSemanticPanel, setShowSemanticPanel] = useState(false); const [semanticContent, setSemanticContent] = useState(note.content ?? ""); - const debouncedSemanticContent = useDebouncedValue(semanticContent, 1200); @@ <BacklinksPanel note={note} onOpenCard={() => setView("board")} semanticEnabled={showSemanticPanel} - semanticContent={debouncedSemanticContent} + semanticContent={semanticContent} workspaceId={activeWorkspaceId} />Also applies to: 947-948
🤖 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/components/notes/note-editor.tsx` around lines 63 - 64, Remove the debouncing from the NoteEditor component at the useDebouncedValue call for semanticContent (line 63-64). Instead of debouncing semanticContent in NoteEditor and passing the debounced value to BacklinksPanel, pass the raw semanticContent directly so that only BacklinksPanel performs debouncing at its own useDebouncedValue call. This eliminates the double-debouncing issue that causes excessive latency and prevents stale data from previous notes being queried after note switches.src/components/settings/EmbeddingsSettings.tsx-147-153 (1)
147-153:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClear failed installs out of the downloading state.
After
install()throws,progressByModel[modelId]remains{ status: "downloading" }, which keeps the card stuck on “Cancel” until remount. Clear or mark that progress entry incatch/finally.Proposed fix
try { await e.models.install(modelId); - await refreshQuiet(); } catch (err) { console.error("[embeddings] install failed:", err); + setProgressByModel((prev) => { + const { [modelId]: _discard, ...rest } = prev; + return rest; + }); + } finally { + await refreshQuiet(); }🤖 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/components/settings/EmbeddingsSettings.tsx` around lines 147 - 153, The progressByModel state entry for a failed model installation remains stuck in the "downloading" status because it is not cleared or updated when the install() call throws an error. In the catch block where console.error is called, add a setProgressByModel call to either remove the modelId entry entirely or update its status to indicate failure. This will prevent the UI from staying in a "Cancel" state after the installation fails.src/components/settings/EmbeddingsSettings.tsx-167-172 (1)
167-172:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep
config.modelIdin sync when changing the default model.
handleReindexAll()andhandleRecomputeProjections()passconfig.modelId, butsetDefault()only updates the model manifest/status. After choosing a new default, maintenance actions can still run with the old model ID unless the settings config is updated and saved too.Proposed fix
const handleSetDefault = async (modelId: string) => { const e = window.electron?.embeddings; if (!e) return; try { await e.models.setDefault(modelId); + const next = { ...config, modelId }; + setConfig(next); + await e.saveSettings(next); await refreshQuiet(); } catch (err) { console.error("[embeddings] setDefault failed:", err); } };🤖 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/components/settings/EmbeddingsSettings.tsx` around lines 167 - 172, The handleSetDefault function calls await e.models.setDefault(modelId) to update the model manifest but does not synchronize this change with the settings config object. This causes handleReindexAll and handleRecomputeProjections to still reference the old config.modelId when executing later. Update config.modelId to the new modelId after the setDefault call completes and before calling refreshQuiet() to ensure the settings config stays in sync with the selected default model.src/components/settings/EmbeddingsSettings.tsx-114-115 (1)
114-115:⚠️ Potential issue | 🟠 MajorUse the store's
activeWorkspaceIdinstead of dereferencingworkspace.list()[0].id.The component currently maintains local state
activeWorkspaceIdinitialized from the first workspace in the list. Since other components useuseCairnStore((s) => s.activeWorkspaceId)to access the user's currently selected workspace, EmbeddingsSettings should do the same. Multi-workspace users can otherwise runreindex()andrecomputeProjections()against the wrong workspace. Keep the actions disabled whileactiveWorkspaceIdis null (initial state) to ensure the correct workspace is targeted before any operations execute.🤖 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/components/settings/EmbeddingsSettings.tsx` around lines 114 - 115, Remove the local state assignment of activeWorkspaceId from workspace.list()[0].id in the code block around lines 114-115. Instead, use the store's activeWorkspaceId by calling useCairnStore((s) => s.activeWorkspaceId) to access the user's currently selected workspace, which is consistent with how other components retrieve this value. Additionally, add null checks or disabled state conditions to the reindex() and recomputeProjections() action buttons to ensure they remain disabled while activeWorkspaceId is null, preventing operations from running against an incorrect workspace.electron/embeddings/server.ts-43-49 (1)
43-49:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd a hard payload cap for request body reads.
readBodyaccumulates unbounded bytes in memory. A large or repeated POST to/embedcan force OOM and kill the worker process.Suggested fix
+const MAX_EMBED_BODY_BYTES = 1_000_000; // 1 MB + function readBody(req: http.IncomingMessage): Promise<string> { return new Promise((resolve, reject) => { const chunks: Buffer[] = []; - req.on("data", (c: Buffer) => chunks.push(c)); + let total = 0; + req.on("data", (c: Buffer) => { + total += c.length; + if (total > MAX_EMBED_BODY_BYTES) { + reject(Object.assign(new Error("payload too large"), { statusCode: 413 })); + req.destroy(); + return; + } + chunks.push(c); + }); req.on("end", () => resolve(Buffer.concat(chunks).toString("utf8"))); req.on("error", reject); }); }🤖 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 `@electron/embeddings/server.ts` around lines 43 - 49, The readBody function accumulates request body chunks into memory without any size constraints, making it vulnerable to Out-Of-Memory attacks. Add a maximum payload size constant to cap the total bytes that can be accumulated. In the data event handler where chunks are pushed to the array, track the cumulative size of all chunks and reject the promise if the total size exceeds the maximum payload limit. This will prevent unbounded memory allocation in the readBody function.electron/embeddings/pipeline.ts-34-57 (1)
34-57:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReset cached pipeline state when model load fails.
loadPipelinecaches_loadedModelIdand_extractorbefore the load resolves. If the promise rejects once, later calls can keep reusing a rejected cached promise instead of retrying.Suggested fix
export async function loadPipeline( modelId: string = NOMIC_MODEL_ID, onProgress?: ProgressCallback, ): Promise<FeatureExtractionPipeline> { if (_extractor && _loadedModelId === modelId) return _extractor; - _extractor = pipeline("feature-extraction", modelId, { + _extractor = pipeline("feature-extraction", modelId, { quantized: true, progress_callback: onProgress ? (data: unknown) => { if (data && typeof data === "object") { const d = data as Record<string, unknown>; @@ } : undefined, - }); + }).catch((err) => { + if (_loadedModelId === modelId) { + _extractor = null; + _loadedModelId = null; + } + throw err; + }); _loadedModelId = modelId; return _extractor; }🤖 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 `@electron/embeddings/pipeline.ts` around lines 34 - 57, The loadPipeline function caches _extractor and _loadedModelId without awaiting the pipeline promise or handling errors. If the pipeline creation fails, the rejected promise remains cached, causing subsequent calls to return that same rejected promise instead of retrying. Add await to the pipeline call and wrap it in error handling (try-catch or .catch()) to reset both _extractor and _loadedModelId to their initial states when the operation fails, ensuring subsequent calls can retry the load.electron/embeddings/port.ts-6-12 (1)
6-12:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAvoid the free-port TOCTOU race in worker startup.
Line 6 binds a port and Line 9 immediately releases it before the worker process binds, so the port can be stolen and startup becomes flaky under contention. Prefer binding the worker to port
0and reporting the actual bound port, or add spawn-on-bind-failure retry logic tied toEADDRINUSE.🤖 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 `@electron/embeddings/port.ts` around lines 6 - 12, The code has a race condition where the server listening on a randomly assigned port (line 6) is immediately closed before the worker process can bind to that same port (line 9), allowing another process to claim the port in between. Instead of closing the server and passing the port to the worker, either keep the server listening while the worker connects to it, or modify the worker process to bind directly to port 0 and have it report back the actual port it bound to. If using the latter approach, add retry logic with exponential backoff that listens for EADDRINUSE errors and retries the worker spawn when port conflicts occur.electron/embeddings/manifest.ts-85-87 (1)
85-87:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve persisted download progress/speed when reading the manifest.
setEmbeddingModelStatus()stores progress metadata, butgetEmbeddingModelsManifest()drops it by returning fixed0/100progress andundefinedspeed, so in-progress status is lost on refresh/remount.Suggested fix
const entry: EmbeddingModelManifestEntry = { @@ - downloadProgress: status === "installed" ? 100 : 0, - downloadSpeed: undefined, + downloadProgress: + status === "installed" + ? 100 + : status === "downloading" + ? Math.max(0, Math.min(100, stored.downloadProgress ?? 0)) + : 0, + downloadSpeed: status === "downloading" ? stored.downloadSpeed : undefined, error: stored.error, };Also applies to: 108-110
🤖 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 `@electron/embeddings/manifest.ts` around lines 85 - 87, The downloadProgress and downloadSpeed properties in getEmbeddingModelsManifest() are being set to hardcoded values (0/100 and undefined) instead of reading from the stored metadata, causing in-progress download status to be lost on refresh. Replace the hardcoded downloadProgress assignment (which currently returns 100 if status is installed, otherwise 0) with the actual stored progress value, and replace the undefined downloadSpeed with the stored speed value from the metadata object, similar to how error is read from stored.error. Apply this same fix to both occurrences around lines 85-87 and 108-110.electron/embeddings/client.ts-269-283 (1)
269-283:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrevent orphan workers during unhealthy restart.
Line 353 increments
inFlightEmbedsbefore Line 355 callsensureStarted(). When health checks fail, Line 282 callsstopWorker(), but Line 307 suppresses stop due to the current in-flight count, then startup continues and can spawn a new worker without terminating the old one.Suggested fix
export async function ensureStarted(): Promise<number> { @@ - await stopWorker(); + await stopWorker({ force: true }); } @@ export async function embed( texts: string[], task: NomicTask, model = getDefaultModelId(), ): Promise<number[][]> { if (texts.length === 0) return []; - inFlightEmbeds++; + const port = await ensureStarted(); + inFlightEmbeds++; try { - const port = await ensureStarted(); const body = JSON.stringify({ texts, task, model });Also applies to: 305-313, 347-356
🤖 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 `@electron/embeddings/client.ts` around lines 269 - 283, The issue is that when ensureStarted() detects an unhealthy worker and calls stopWorker() at line 282, the stop operation is suppressed due to non-zero inFlightEmbeds count that was already incremented before ensureStarted() was called from the caller code. This allows a new worker to spawn while the old unhealthy one still exists, creating orphans. Modify stopWorker() to accept a parameter that forces termination of the old worker regardless of the inFlightEmbeds count, then pass this force flag when calling stopWorker() from the unhealthy restart path in ensureStarted() to ensure the old unhealthy worker is always properly terminated before any new worker is spawned.electron/db/queries.ts-1352-1360 (1)
1352-1360:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftFilter workspace embeddings by
modelto prevent mixed-vector comparisons.This query currently mixes rows from different embedding models under the same task. That breaks semantic ranking quality and can trigger dimension mismatches downstream after model switches or partial reindex runs.
Suggested contract change
export function getAllEmbeddingsForWorkspace( db: Database.Database, workspaceId: string, task: string, + model?: string, ): NoteEmbeddingRecord[] { - const rows = db.prepare( - "SELECT * FROM note_embeddings WHERE workspace_id = ? AND task = ?" - ).all(workspaceId, task) as NoteEmbeddingRow[]; + const hasModel = typeof model === "string" && model.length > 0; + const rows = hasModel + ? db.prepare( + "SELECT * FROM note_embeddings WHERE workspace_id = ? AND task = ? AND model = ?" + ).all(workspaceId, task, model) + : db.prepare( + "SELECT * FROM note_embeddings WHERE workspace_id = ? AND task = ?" + ).all(workspaceId, task); + const typed = rows as NoteEmbeddingRow[]; - return rows.map(toNoteEmbedding); + return typed.map(toNoteEmbedding); }Update call sites to pass the active model (search, projection recompute, semantic-relationship recompute) so comparisons stay in one embedding space.
🤖 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 `@electron/db/queries.ts` around lines 1352 - 1360, The getAllEmbeddingsForWorkspace function currently filters only by workspace_id and task, mixing embeddings from different embedding models which causes semantic ranking issues and dimension mismatches. Add a model parameter to the function signature, update the WHERE clause in the SQL query to also filter by the model field using an additional placeholder parameter, and then update all call sites of getAllEmbeddingsForWorkspace to pass the active embedding model when invoking the function.electron/db/graph-queries.ts-768-774 (1)
768-774:⚠️ Potential issue | 🟠 Major | ⚡ Quick winIncremental semantic recompute drops valid pairs due to ordering check.
When
entityIdsis provided, only changed IDs are inactivePool. Withif (a.id >= b.id) continue, pairs where changeda.idsorts after unchangedb.idare skipped and never recomputed after deletion.Safe pair canonicalization fix
const tx = db.transaction(() => { + const seen = new Set<string>(); if (activeIds) { for (const id of activeIds) deleteOld.run(id, id); } else { const allIds = vectors.map((v) => v.id); for (const id of allIds) deleteOld.run(id, id); } for (const a of activePool) { for (const b of fullPool) { - if (a.id >= b.id) continue; + if (a.id === b.id) continue; + const [src, tgt] = a.id < b.id ? [a.id, b.id] : [b.id, a.id]; + const key = `${src}|${tgt}`; + if (seen.has(key)) continue; + seen.add(key); const sim = cosine(a.vec, b.vec); if (sim >= SEMANTIC_THRESHOLD) { - upsert.run(a.id, b.id, "semantic", Math.round(sim * 100) / 100, now); + upsert.run(src, tgt, "semantic", Math.round(sim * 100) / 100, now); } } } });🤖 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 `@electron/db/graph-queries.ts` around lines 768 - 774, The ordering check `if (a.id >= b.id) continue;` in the nested loop comparing `activePool` against `fullPool` incorrectly skips valid pairs when `entityIds` is provided. Since `activePool` only contains changed entities while `fullPool` contains all entities, pairs where a changed entity ID sorts after an unchanged entity ID are never recomputed. Replace the directional skip with safe pair canonicalization by always comparing the pair in consistent sorted order using the minimum and maximum of the two IDs, ensuring all relevant semantic similarity pairs are evaluated regardless of which pool contains which entity.src/components/graph/RadialTreeCanvas.tsx-137-140 (1)
137-140:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
semanticThresholdchanges won’t reliably re-render radial cross-edges.
renderTreereadssemanticThreshold(Line 139) butuseCallbackdeps omit it (Line 348), so slider updates can be ignored until another dependency changes.🔧 Suggested fix
- }, [graph, selectedNodeId, hoveredNodeId, labelMode, spacing, onNodeClick, fs]); + }, [graph, selectedNodeId, hoveredNodeId, labelMode, spacing, semanticThreshold, onNodeClick, fs]);Also applies to: 348-348
🤖 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/components/graph/RadialTreeCanvas.tsx` around lines 137 - 140, The renderTree function reads semanticThreshold in the crossEdges filter but the useCallback hook that wraps renderTree omits semanticThreshold from its dependency array, causing stale closures when the threshold value changes. Add semanticThreshold to the dependency array of the useCallback hook at line 348 so that renderTree is recreated whenever the threshold updates and the radial cross-edges re-render with the current threshold value.electron/embeddings/query-cache.test.ts-13-35 (1)
13-35:⚠️ Potential issue | 🟠 Major | ⚡ Quick winLRU test double is out of sync with production cache logic.
get()here promotes keys to MRU, butsearchAdjacent’s real cache does not. This test can pass while production remains FIFO.🤖 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 `@electron/embeddings/query-cache.test.ts` around lines 13 - 35, The LruQueryCache test double implementation is not aligned with the production cache behavior. The get() method in LruQueryCache currently promotes accessed keys to MRU (Most Recently Used) by deleting and re-adding them to the map, but the real searchAdjacent cache does not perform this promotion and maintains FIFO semantics instead. Remove the this.map.delete(key) and this.map.set(key, v) lines from the get() method so it simply retrieves and returns the value without reordering, making the test double match the actual production cache behavior.electron/embeddings/service.ts-149-152 (1)
149-152:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDelete stored embeddings when a note becomes empty.
At Line 149, empty content is counted as
skippedand exits early, but the existing embedding is left intact. Because note-save reindex calls this path, cleared notes can keep appearing in semantic/search results with stale vectors.🤖 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 `@electron/embeddings/service.ts` around lines 149 - 152, When a note's content_text is empty or only whitespace (checked in the condition at line 149), the code currently skips processing by incrementing the skipped counter and continuing to the next iteration, but this leaves any existing embeddings intact in storage, causing stale vector data to persist in search results. Modify this block to delete the stored embedding for the note from your embeddings storage (using an appropriate delete method based on your embeddings data structure) before or instead of just incrementing skipped and continuing, ensuring that cleared notes are completely removed from semantic search results.electron/embeddings/service.ts-205-214 (1)
205-214:⚠️ Potential issue | 🟠 Major | ⚡ Quick winQuery cache currently evicts FIFO, not LRU.
On cache hit (Line 205), recency is not refreshed, so eviction at Line 209 removes oldest insertion, not least-recently-used query.
♻️ Suggested fix
let queryVec = queryCache.get(queryHash); - if (!queryVec) { + if (queryVec) { + // refresh recency for LRU behavior + queryCache.delete(queryHash); + queryCache.set(queryHash, queryVec); + } else { const { vector } = await embedChunkedDocument(embed, trimmed, "search_query", model); queryVec = vector; if (queryCache.size >= QUERY_CACHE_MAX) { const firstKey = queryCache.keys().next().value; if (firstKey) queryCache.delete(firstKey); } queryCache.set(queryHash, queryVec); }🤖 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 `@electron/embeddings/service.ts` around lines 205 - 214, The queryCache uses FIFO eviction instead of LRU because cache hits do not refresh the recency of accessed entries. When queryVec is retrieved from the cache at the start of the if statement checking queryCache.get(queryHash), the entry's position in the cache is not updated to reflect that it was recently accessed. To implement true LRU eviction, delete and re-insert the queryVec entry into queryCache immediately after a successful cache hit to mark it as recently used, ensuring that the FIFO deletion logic at line 209 actually removes the least-recently-used entry instead of the oldest insertion.electron/ipc/embeddings-handlers.ts-64-68 (1)
64-68:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
withLockbroadcasts success even when the task fails.The
finallyblock always sendsstatus: "done"andprogress: 100(Line 67), so listeners receive a false success state on exceptions.🛠️ Suggested fix
- try { - return await p as T; + let failed: unknown = null; + try { + return await p as T; + } catch (e) { + failed = e; + throw e; } finally { slot.setFlag(false); - broadcastProgress(win, { modelId: "", status: "done", progress: 100, loaded: 1, total: 1 }); + broadcastProgress(win, { + modelId: "", + status: failed ? "error" : "done", + progress: failed ? undefined : 100, + loaded: failed ? undefined : 1, + total: failed ? undefined : 1, + error: failed instanceof Error ? failed.message : failed ? String(failed) : undefined, + }); if (slot.current === p) slot.current = null; }🤖 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 `@electron/ipc/embeddings-handlers.ts` around lines 64 - 68, The `finally` block in the `withLock` function always broadcasts status "done" with progress 100 regardless of whether the task succeeded or failed, causing listeners to receive false success messages on exceptions. Move the broadcastProgress call that sends the success status outside the finally block to only execute when the task completes successfully, while keeping the cleanup operations (slot.setFlag(false) and slot.current = null) in the finally block since cleanup must always occur.electron/ipc/db-handlers.ts-152-160 (1)
152-160:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftSerialize per-note incremental reindex jobs to avoid stale overwrite races.
This fire-and-forget path can run concurrent
reindexNotes(..., [id], ...)jobs for the same note on rapid saves. Since writes happen after async embedding, older runs can finish last and overwrite newer vectors/hashes.🤖 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 `@electron/ipc/db-handlers.ts` around lines 152 - 160, The current implementation of the reindexing logic fires off concurrent reindexSingleNoteEmbedding operations for the same note without serialization, which can cause older async jobs to finish after and overwrite newer embedding data. Implement a per-note job queue or serialization mechanism (such as a Map of promises keyed by note id) to ensure that reindexSingleNoteEmbedding calls for the same note are executed sequentially rather than concurrently, preventing stale writes from overwriting fresh embeddings and ensuring the most recent reindex operation's results persist.electron/ipc/db-handlers.ts-27-35 (1)
27-35:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGate semantic recompute on a successful embedding refresh.
reindexSingleNoteEmbeddingalways resolves (it catches internally), so semantic recompute runs even when reindexing is skipped/failed. That can refresh semantic edges from stale vectors.Suggested fix
-async function reindexSingleNoteEmbedding(ctx: DbContext, noteId: string, workspaceId: string): Promise<void> { +async function reindexSingleNoteEmbedding(ctx: DbContext, noteId: string, workspaceId: string): Promise<boolean> { try { const settings = getEmbeddingsSettingsCached(); - if (!settings?.enabled) return; + if (!settings?.enabled) return false; const model = settings.modelId || getEmbeddingModelId(); await reindexNotes(ctx.db, workspaceId, [noteId], model); + return true; } catch (e) { console.warn("[embeddings] incremental reindex failed:", e instanceof Error ? e.message : e); + return false; } } @@ - void reindexSingleNoteEmbedding(ctx, id, note.workspaceId).then(() => { + void reindexSingleNoteEmbedding(ctx, id, note.workspaceId).then((didReindex) => { + if (!didReindex) return; try { computeSemanticRelationships(ctx.db, note.workspaceId, [id]); } catch (e) {Also applies to: 154-160
🤖 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 `@electron/ipc/db-handlers.ts` around lines 27 - 35, The reindexSingleNoteEmbedding function always resolves successfully because its internal try-catch swallows errors, preventing callers from knowing whether reindexing actually succeeded or failed. This causes semantic recompute to proceed even when reindexing is skipped or fails, potentially using stale vectors. Change the return type of reindexSingleNoteEmbedding from Promise<void> to Promise<boolean>, return false when the settings are disabled or when an error occurs, and return true on successful reindex completion. Then update all callers of reindexSingleNoteEmbedding to check the boolean return value and only proceed with semantic recompute if the function returns true, indicating successful reindexing. Apply this same fix to the other similar function referenced at lines 154-160.electron/preload.ts-562-566 (1)
562-566:⚠️ Potential issue | 🟠 MajorAlign embeddings settings bridge types with the actual IPC payload shape.
app:getEmbeddingsSettingsreturns a partial settings object whereenabledandmodelIdare optional, but preload types both as required. This mismatches the actual contract and can mask undefined values at call sites.Suggested fix
- getSettings: () => invoke<{ enabled: boolean; modelId: string } | null>("app:getEmbeddingsSettings"), - saveSettings: (config: { enabled: boolean; modelId: string }) => invoke<{ ok: boolean }>( + getSettings: () => invoke<{ enabled?: boolean; modelId?: string } | null>("app:getEmbeddingsSettings"), + saveSettings: (config: { enabled?: boolean; modelId?: string }) => invoke<{ ok: boolean }>( "app:saveEmbeddingsSettings", { config }, ),🤖 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 `@electron/preload.ts` around lines 562 - 566, The type definition for the getSettings function in the preload.ts file incorrectly marks the enabled and modelId properties as required fields, but the actual app:getEmbeddingsSettings IPC handler returns a partial settings object where these properties are optional. Update the invoke type parameter for getSettings to mark both enabled and modelId as optional properties (using the optional property syntax) so the type accurately reflects the actual IPC contract and prevents masking of undefined values at call sites.
🟡 Minor comments (8)
src/components/notes/note-editor.tsx-29-29 (1)
29-29:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClean up the unused
SemanticHubsPanelimport.This currently triggers the lint warning and should be removed (or the component should be rendered here).
Proposed fix (if not rendering it here)
-import { SemanticHubsPanel } from "./SemanticHubsPanel";🤖 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/components/notes/note-editor.tsx` at line 29, Remove the unused import statement for SemanticHubsPanel in the note-editor.tsx file. Since the component is not being rendered anywhere in the file, the import on line 29 should be deleted to resolve the lint warning and keep the imports clean.Source: Linters/SAST tools
src/components/settings/EmbeddingsSettings.tsx-119-128 (1)
119-128:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDon’t route maintenance progress through the model-download listener.
The supplied preload contract wires
embeddings.models.onProgress()toembeddings:download-progresswith model-download fields, so the!ev.modelIdbranch will not reliably receive reindex/recompute progress. Add a dedicated maintenance progress listener or pollstatus()while maintenance is active.🤖 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/components/settings/EmbeddingsSettings.tsx` around lines 119 - 128, The reindex/recompute progress handling in the window.electron?.embeddings?.models.onProgress listener is unreliable because this listener is wired to embeddings:download-progress which carries model-download specific fields, making the !ev.modelId branch an incorrect routing path. Remove the status checks for "duplicate", "installed", and "ready" conditions from within the !ev.modelId branch of the onProgress listener, and instead create a dedicated maintenance progress listener or implement a polling mechanism using status() while maintenance is active to properly track and update the reindex progress state.electron/embeddings/server.ts-95-97 (1)
95-97:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReturn 400 for malformed JSON/validation errors, not 500.
Input parsing failures in
handleEmbedcurrently fall through to the generic 500 path. These are client request errors and should be reported as 400-class responses.Suggested fix
+class HttpError extends Error { + constructor(public status: number, message: string) { + super(message); + } +} + async function handleEmbed( body: string, res: http.ServerResponse, configuredModel: string, ): Promise<void> { - const parsed = JSON.parse(body); - const req = EmbedRequest.parse({ ...parsed, model: parsed.model ?? configuredModel }); + let parsed: unknown; + try { + parsed = JSON.parse(body); + } catch { + throw new HttpError(400, "invalid JSON body"); + } + const reqParsed = EmbedRequest.safeParse({ + ...(parsed as Record<string, unknown>), + model: (parsed as { model?: unknown })?.model ?? configuredModel, + }); + if (!reqParsed.success) throw new HttpError(400, "invalid embed request"); + const req = reqParsed.data; if (req.task !== ("search_document" as NomicTask) && req.task !== ("search_query" as NomicTask) && req.task !== ("clustering" as NomicTask)) { sendJson(res, 400, { error: `invalid task: ${req.task}` }); return; @@ } catch (e) { const msg = e instanceof Error ? e.message : String(e); emit({ kind: "error", msg }); - sendJson(res, 500, { error: msg }); + const status = e instanceof HttpError ? e.status : 500; + sendJson(res, status, { error: msg }); }Also applies to: 123-130
🤖 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 `@electron/embeddings/server.ts` around lines 95 - 97, In the handleEmbed function, wrap the JSON.parse and EmbedRequest.parse operations in a try-catch block to catch parsing and validation errors. When either operation fails, return a 400 status code response with an appropriate error message instead of allowing the error to propagate to the generic 500 error handler. This applies to all input parsing sections mentioned in the affected lines, ensuring client request errors are properly classified as 400-class responses rather than 500-class server errors.electron/embeddings/client.ts-174-180 (1)
174-180:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winEmit
readyevents to registered progress listeners.
parseStdoutLine()updates internal state forreadybut never forwards that event, so listeners never receive the declared{ kind: "ready" }notification.Suggested fix
case "ready": isReady = true; if (ev.model) workerModel = ev.model; + emitProgress(ev); break;🤖 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 `@electron/embeddings/client.ts` around lines 174 - 180, The switch statement in parseStdoutLine() handles the "ready" event case by updating internal state (isReady and workerModel) but fails to emit the event to registered listeners. Add a call to emitProgress(ev) in the "ready" case handler, similar to how the "progress" case emits events, so that listeners receive the ready notification.src/components/graph/RadialTreeCanvas.tsx-139-139 (1)
139-139:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse inclusive threshold comparison for semantic edges.
This currently uses
>, but the UI communicates a≥threshold, so exact-threshold edges are incorrectly hidden.🤖 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/components/graph/RadialTreeCanvas.tsx` at line 139, The semantic edge filtering logic in RadialTreeCanvas.tsx uses a strict greater-than comparison with semanticThreshold, but the UI communicates an inclusive threshold behavior. Change the `>` operator to `>=` in the condition `(e.weight ?? 1) > semanticThreshold` to ensure that edges with weights equal to the threshold are included rather than hidden, making the behavior consistent with the UI's communicated threshold semantics.electron/embeddings/chunking.test.ts-72-79 (1)
72-79:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHard timing bound (
< 2000ms) is likely CI-flaky.Line 77 makes this test dependent on machine load/perf rather than correctness invariants.
🤖 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 `@electron/embeddings/chunking.test.ts` around lines 72 - 79, Remove the hard timing bound expectation in the test TERMINATES on very long input (regression: infinite loop → OOM) by deleting the expect(elapsed).toBeLessThan(2000) assertion on line 77, as this creates CI flakiness by depending on machine performance rather than actual correctness. Retain the other assertions expect(chunks.length).toBeGreaterThan(0) and expect(chunks.length).toBeLessThan(1000) which validate the actual correctness invariants that the function terminates and produces a reasonable number of chunks from the very long input.src/components/graph/ForceGraphCanvas.tsx-69-69 (1)
69-69:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winThreshold comparison should match the UI’s inclusive semantics.
Filtering uses
>but the control communicates≥. Edges with weight exactly equal to the slider value are currently hidden.🔧 Suggested fix
- if (link.type === "semantic" && (link.weight ?? 1) <= semanticThreshold) continue; + if (link.type === "semantic" && (link.weight ?? 1) < semanticThreshold) continue; ... - (e) => e.type !== "semantic" || (e.weight ?? 1) > semanticThreshold, + (e) => e.type !== "semantic" || (e.weight ?? 1) >= semanticThreshold,Also applies to: 143-145
🤖 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/components/graph/ForceGraphCanvas.tsx` at line 69, The threshold comparison in the semantic link filtering is using `<=` which excludes edges with weight exactly equal to the semanticThreshold, but the UI semantics expect these edges to be included. Change the comparison operator from `<=` to `<` in the condition `(link.weight ?? 1) <= semanticThreshold` on line 69 so that only edges with weight strictly less than the threshold are filtered out. Apply the same fix to the similar threshold comparison at lines 143-145 to ensure consistent behavior throughout the component.electron/ipc/embeddings-handlers.ts-95-96 (1)
95-96:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
queryNoteIdis ignored whenexcludeIdsis present.At Line 95, nullish coalescing means a provided
excludeIdsarray replaces thequeryNoteIdexclusion instead of merging it.♻️ Suggested fix
- const exclude = args.excludeIds ?? (args.queryNoteId ? [args.queryNoteId] : []); + const exclude = [ + ...(args.excludeIds ?? []), + ...(args.queryNoteId ? [args.queryNoteId] : []), + ];🤖 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 `@electron/ipc/embeddings-handlers.ts` around lines 95 - 96, The exclude variable construction at line 95 uses nullish coalescing which causes queryNoteId to be ignored when excludeIds is already provided. Instead of using the nullish coalescing operator to choose between excludeIds or queryNoteId, merge both exclusion criteria together by starting with excludeIds (or an empty array if not provided) and appending queryNoteId to the result when it exists. This ensures both IDs are included in the exclude array passed to searchAdjacent regardless of which parameters are present.
🧹 Nitpick comments (2)
src/components/notes/BacklinksPanel.tsx (1)
45-57: ⚡ Quick winUse the trimmed semantic text for the API call.
The guard uses
debouncedSemantic.trim()but the request sends untrimmed text. That can cause avoidable cache misses and extra embedding work for whitespace-only differences.Proposed fix
useEffect(() => { - if (!semanticEnabled || !workspaceId || !debouncedSemantic.trim() || debouncedSemantic.trim().length < 4) { + const trimmed = debouncedSemantic.trim(); + if (!semanticEnabled || !workspaceId || trimmed.length < 4) { setSemanticHits([]); setSemanticLoading(false); return; } @@ - const hits = await api.search(workspaceId, debouncedSemantic, { + const hits = await api.search(workspaceId, trimmed, { queryNoteId: note.id, k: 5, });🤖 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/components/notes/BacklinksPanel.tsx` around lines 45 - 57, The guard condition trims debouncedSemantic for validation, but the api.search() method call passes the untrimmed debouncedSemantic value, causing inconsistency and unnecessary cache misses. Extract the trimmed semantic text into a variable before the guard condition and use this trimmed variable in both the condition check (instead of calling trim() multiple times) and when passing it to the api.search() method call to ensure the API receives the same trimmed text that passed validation.electron/embeddings/chunking.test.ts (1)
25-49: 🏗️ Heavy liftTesting a copied chunker weakens regression protection.
This suite validates a local reimplementation, not the production function. If
service.tschanges independently, these tests can still pass and miss regressions.🤖 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 `@electron/embeddings/chunking.test.ts` around lines 25 - 49, The test file contains a local reimplementation of the chunkLongText function instead of importing and testing the actual production implementation from service.ts. This means if the production function changes independently, the tests will still pass and miss regressions. Remove the local chunkLongText function implementation from the test file and import the actual production function from its source module (likely service.ts), then update all test cases to use the imported production function instead of the copied version.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4acf6b4d-f12c-4490-8f76-4b021f1ed54a
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (42)
changelogs/v2.1.1.mddocs/plans/local-embeddings.mdelectron/db/graph-queries.tselectron/db/queries.tselectron/db/schema.tselectron/embeddings/chunking.test.tselectron/embeddings/client.tselectron/embeddings/cosine.test.tselectron/embeddings/cosine.tselectron/embeddings/manifest.tselectron/embeddings/nomic.tselectron/embeddings/pipeline.tselectron/embeddings/port.tselectron/embeddings/projection.test.tselectron/embeddings/projection.tselectron/embeddings/query-cache.test.tselectron/embeddings/server.tselectron/embeddings/service.tselectron/embeddings/types.tselectron/ipc/db-handlers.tselectron/ipc/embeddings-handlers.tselectron/ipc/handlers.tselectron/ipc/registry.tselectron/ipc/settings-handlers.tselectron/lib/config-cache.tselectron/main.tselectron/mcp-server.tselectron/mcp/db.tselectron/preload.tspackage.jsonscripts/build-embeddings-binary.jssrc/components/graph/ForceGraphCanvas.tsxsrc/components/graph/KnowledgeGraphView.tsxsrc/components/graph/RadialTreeCanvas.tsxsrc/components/notes/BacklinksPanel.tsxsrc/components/notes/SemanticHubsPanel.tsxsrc/components/notes/note-editor.tsxsrc/components/settings/EmbeddingsSettings.tsxsrc/components/settings/settings-view.tsxsrc/hooks/useDebouncedValue.tssrc/store/slices/graph.tssrc/types/index.ts
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 `@src/components/notes/note-editor.tsx`:
- Line 63: Remove the double debounce on semantic content by eliminating the
useDebouncedValue call that creates debouncedSemanticContent in note-editor.tsx.
Instead of debouncing at this layer, pass the raw semanticContent directly to
the BacklinksPanel component, which already handles debouncing internally.
Update any references to debouncedSemanticContent to use semanticContent
instead, keeping the debounce logic only in the BacklinksPanel component to
avoid unnecessary latency and stale data issues.
🪄 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 Plus
Run ID: 6314247a-a537-49d2-a9a6-2b2aeb915fbb
📒 Files selected for processing (4)
electron/embeddings/client.tssrc/components/graph/RadialTreeCanvas.tsxsrc/components/notes/BacklinksPanel.tsxsrc/components/notes/note-editor.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- src/components/graph/RadialTreeCanvas.tsx
- src/components/notes/BacklinksPanel.tsx
- electron/embeddings/client.ts
- electron/embeddings/service.bench.test.ts: 12 tests measuring chunkLongText / averageVectors / cosine / topK / projectTo2d across tiny→xlarge fixtures. Documents observed ONNX worker timings and memory footprint in footer comment for M-series Macs (cold start, warm embed, batch latencies, RSS). - electron/embeddings/service.ts: emit onProgress per note (was per batch of 16) in reindexNotes and recomputeProjections, plus an initial 0/total tick so the Settings progress bar shows scale immediately and advances per note instead of jumping in steps of 16. npm test: 475 passing; lint clean; type-check clean
Xenova's pipeline() silently fell back to onnxruntime-web (WASM) which pre-allocated a 17.4 GB WebAssembly.Memory arena at idle. Bypass that by loading the tokenizer via xenova's AutoTokenizer and running inference directly through onnxruntime-node's InferenceSession with the native CPU execution provider. electron/embeddings/pipeline.ts: rewrite to use AutoTokenizer.from_pretrained (xenova) for tokenization ort.InferenceSession.create (native onnxruntime-node) for inference inline mean-pooling + L2 normalization (replaces xenova's pipeline) enableCpuMemArena=false, enableMemPattern=false in session options electron/embeddings/client.ts: lower --max-old-space-size from 4096 to 512 (JS heap only; ONNX C++ memory is separate) electron/embeddings/service.bench.test.ts: update documented timings with real M5 Pro measurements (native vs WASM comparison) Measured on M5 Pro (native CPU backend): idle RSS: 444 MB (was 17.4 GB on WASM — 40× improvement) small note: 79 ms (was 156 ms — 2× faster) medium note: 462 ms (was 825 ms — 1.8× faster) batch 16×2KB: 1.2 s (was 2.4 s — 2× faster) Peak RSS during unchunked 32 KB inference is ~20 GB (attention matrix O(seq_len²)); production chunking at 4 KB keeps peak under 1 GB. npm test: 475 passing; lint clean; type-check clean
- pipeline.ts: remove dead WASM env config (numThreads, proxy) — we no longer use onnxruntime-web; fix _pipelinePromise self-reference bug (was always true) - server.ts: remove unused withNomicPrefix import + re-export, redundant NomicTask type import + manual task validation (zod EmbedRequest schema already validates the task enum) - nomic.ts: remove unused exports (taskForIndexing, taskForQuery, taskForClustering, defaultModelId, normalizeModel)
embedChunkedDocument previously sent chunks in batches of 16 to the worker in a single HTTP /embed call. The worker's native ONNX session creates an attention matrix of [batch, 12, seqLen, seqLen] per call, so a 16-chunk batch (16 × 1000 tokens) peaked at 4–5 GB of RSS. Fix: send each chunk as a separate single-text HTTP call. The worker's HTTP server processes them sequentially (one session.run() at a time), keeping peak RSS at ~750 MB regardless of note size. Measured on M5 Pro with 512 MB JS heap cap: 4 KB note (1 chunk): 174 ms, RSS 701 MB 8 KB note (3 chunks): 344 ms, RSS 718 MB 32 KB note (9 chunks): 1.5 s, RSS 743 MB 100 sequential 4 KB: no leak (RSS even dropped to 646 MB) Previously: 32 KB note in one batch → 4 GB peak RSS.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@electron/embeddings/pipeline.ts`:
- Line 46: The single-flight check in the pipeline initialization ignores the
modelId parameter, causing requests with different model IDs to reuse the same
cached pipeline promise and produce embeddings with the wrong model. Modify the
caching logic to be model-aware by either tracking which modelId the cached
_pipelinePromise is associated with or using a map keyed by modelId to store
separate pipeline promises for each model. Only return the cached
_pipelinePromise if the modelId matches the current request, otherwise proceed
to create a new pipeline for the different model.
In `@electron/embeddings/service.bench.test.ts`:
- Around line 124-133: The topK(1000→5) benchmark ceiling value is set too
tightly at 2ms in the CEILINGS object, causing CI flake failures when actual
performance (2.289ms) exceeds this threshold. Increase the ceiling value for the
"topK(1000→5)" entry from 2 to a higher value (such as 3 or more) to accommodate
natural CI variance while still catching real performance regressions.
Alternatively, you could add conditional logic to skip this benchmark when
running in a CI environment by checking for CI environment variables.
🪄 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 Plus
Run ID: abae5469-3bc5-4f52-9a5d-2c453bd7b41d
📒 Files selected for processing (6)
electron/embeddings/client.tselectron/embeddings/nomic.tselectron/embeddings/pipeline.tselectron/embeddings/server.tselectron/embeddings/service.bench.test.tselectron/embeddings/service.ts
💤 Files with no reviewable changes (1)
- electron/embeddings/nomic.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- electron/embeddings/service.ts
- electron/embeddings/client.ts
The reindex button in Settings called reindexNotes() but never computeSemanticRelationships() — so no 'semantic' rows were ever written to relationship_cache. The incremental path (single note save in db-handlers.ts) did call it, but a first-time full reindex from Settings produced embeddings without any semantic edges in the graph. Fix: call computeSemanticRelationships() immediately after a successful reindex in the db:embeddings:reindex handler, passing through the noteIds for incremental mode. Tests: 12 new tests in electron/db/graph-queries.test.ts covering: - No edges when embeddings table empty - Edges created for similar note pairs (cosine ≥ 0.78) - No edges for dissimilar notes - Deduplication (canonical source < target ordering) - Incremental mode (entityIds filter) - Idempotency (calling twice doesn't duplicate) - loadGraph integration: includeAuto, edgeTypes filter, includeAuto=false - Weight rounding to 2 decimal places - Client-side threshold filter simulation npm test: 487 passing; lint clean; type-check clean
Previous guard 'if (result.indexed > 0)' skipped semantic edge computation when all notes already had up-to-date embeddings. But semantic relationships may not exist yet even if embeddings do — e.g. user indexed once before the semantic feature was wired up, then clicks Reindex and nothing gets re-embedded, so no semantic edges are created. Changed to 'if (result.total > 0)' — compute relationships as long as there are notes in scope, regardless of whether any were re-indexed.
Previous behaviour: only note pairs with cosine >= 0.78 were connected.
nomic-embed-text-v1.5 cosines for related-but-not-duplicate notes typically
land in 0.60-0.75 range, so most notes ended up with zero semantic edges
even after a successful reindex.
New behaviour:
- Each note connects to its K=5 most similar peers (regardless of absolute
cosine), provided cosine >= 0.55 floor (filters truly unrelated noise)
- Edges deduplicated via canonical source<target ordering
- k = min(5, pool_size - 1) handles tiny workspaces
- Slider still filters client-side by weight, so users can still tighten
Removed unused SEMANTIC_THRESHOLD constant.
Tests: 16 tests in graph-queries.test.ts covering top-K behaviour, floor,
deduplication, incremental mode, idempotency, weight rounding, and KG
integration (includeAuto, edgeTypes filter, client-side threshold).
npm test: 491 passing; lint clean; type-check clean.
4 new tests in electron/db/graph-queries.test.ts: 1. Topic clusters test — 6 notes representing real-world topics (React Hooks, Vue Composables, Svelte Stores, Python Decorators, Ruby Blocks, Pizza Recipe) with hand-crafted embedding vectors reflecting realistic cosine similarities. Verifies: - Frontend cluster (React/Vue/Svelte) gets 3 inter-cluster edges - Backend cluster (Python/Ruby) gets 1 edge - Unrelated topic (Pizza) has 0 edges - No cross-cluster edges between frontend and backend - Exact weights match computed cosines 2. Slider threshold test — verifies that lowering the threshold reveals weaker edges (React↔Svelte 0.88) while strong edges (React↔Vue 0.98) remain visible at all threshold settings. 3. Slider at 1.0 hides all semantic edges — confirms the default 'off' setting produces a hard-links-only graph. 4. Incremental recompute preserves other clusters' edges — editing one note only recomputes edges touching that note, leaving other cluster edges intact. Added cosineApprox() and round2() helpers for in-test verification. Tests: 495 passing; lint clean; type-check clean.
Previously, each note got a single embedding vector — the averaged result of
all its content. This diluted multi-topic notes: a note with '## Architecture'
and '## Marketing' sections ended up semantically distant from both pure-
architecture and pure-marketing notes, leaving it disconnected in the graph.
Now notes are split by ## and # headers, and each section gets its own
embedding vector. This creates far more — and more precise — connections:
- A multi-topic note connects to architecture notes via its Architecture
section AND marketing notes via its Marketing section
- BacklinksPanel shows which section matched
- KG semantic edges carry source/target section titles
Changes:
- New: electron/embeddings/sections.ts — splitIntoSections() parser
(splits on # and ## only; ### and deeper stay in parent section)
- Schema v18: note_embeddings PK (note_id) → (note_id, section_idx),
adds section_title column. relationship_cache gets source_section_title
and target_section_title columns for semantic edges.
- queries.ts: upsertNoteEmbedding now takes sectionIdx + sectionTitle;
getNoteEmbedding → getNoteEmbeddings (returns array);
deleteNoteEmbeddingSections prunes from a given section_idx onward;
NoteEmbeddingRecord has sectionIdx + sectionTitle fields
- service.ts: reindexNotes splits each note into sections and embeds
each separately; searchAdjacent deduplicates by noteId keeping best
section score + returns sectionTitle; recomputeProjections averages
section vectors per note for the 2D scatter plot
- graph-queries.ts: computeSemanticRelationships compares all section
vectors across all notes, finds top-K nearest sections, maps back to
note-pair level keeping best weight per pair
- BacklinksPanel: shows matching section title under hit title
- GraphEdge type: sourceSectionTitle + targetSectionTitle fields
Tests: 512 passing (21 new — 12 section parser + 9 multi-section scenarios)
- 'A note with 2 sections gets edges from BOTH sections'
- 'Section titles are stored on the semantic edge'
- 'Best weight per note-pair wins when multiple sections match'
- 'Multi-topic note discovers connections via sections'
- 'Plan connects to architecture AND marketing notes but not cooking'
1. pipeline.ts: single-flight cache was not model-aware — if a request for model B arrived while model A was still loading, it would reuse model A's in-flight promise and produce embeddings with the wrong model. Added _pipelinePromiseModelId tracking so the cached promise is only reused when the modelId matches. Cleared on reset/error. 2. service.bench.test.ts: topK(1000→5) ceiling was 2ms, too tight for CI variance (observed 2.289ms). Raised to 5ms — still catches catastrophic regressions while accommodating natural jitter. Tests: 512 passing; lint clean; type-check clean.
…ding Section texts have varying lengths. Without padding:true and truncation:true, the tokenizer produces jagged tensors that ONNX rejects with HTTP 500: 'Unable to create tensor, you should probably activate truncation and/or padding.' This only surfaced after the section-based embeddings change — before, each note was embedded individually (batch size 1), so all tensors were uniform by construction. Now multiple section texts are batched together, requiring padding to the longest sequence in the batch.
The section-based embeddings change accidentally reverted the sequential embedding optimization from v2.1.1. It was batching all sections from up to 16 notes into a single embed() call, which: 1. Caused the ONNX tensor shape error (varying-length texts batched) 2. Recreated the 4GB attention matrix spike we had eliminated 3. Was slower (benchmarks showed sequential is 15× faster for 4KB) Fixed both call sites: - reindexNotes: embed one section text per HTTP call - recomputeProjections: same The padding:true/truncation:true fix from the previous commit is kept as a safety net for the query path, but the primary fix is sequential.
…ote progress 1. Schema: v18 migration was initially deployed without the ALTER TABLE statements for source_section_title / target_section_title on relationship_cache. They were added in a later edit, but databases that already ran v18 had user_version=18 and never re-ran the migration. v19 adds them idempotently (checks PRAGMA table_info before ALTER). 2. reindexNotes: progress now fires per-note (not per-batch of 16). Previous code collected all sections from 16 notes, embedded them, then emitted progress for all 16 at once. Now each note is processed individually: split → embed sections → upsert → emit progress. 3. recomputeProjections: same per-note progress fix. 4. Removed unused 'didAnything' variable in recomputeProjections. Tests: 512 passing; lint clean; type-check clean.
computeAutoRelationships used a DELETE without a type filter, which wiped all relationship_cache rows for each entity — including semantic edges that were computed separately by computeSemanticRelationships. Added 'AND type != \'semantic\'' to the delete statement so only co-mention, keyword, assignee, and wikilink edges are cleared and recomputed. Semantic edges persist until the next reindex/recompute explicitly calls computeSemanticRelationships.
When a node is selected: - Connected edges brighten (opacity 0.85, width 2.5) - Non-connected edges dim to ~12% opacity - Connected nodes stay full opacity - Non-connected nodes dim (#alpha 30) - Tree links (radial) dim to 10% opacity Clicking background clears selection → all edges/nodes reset to normal. Applied to both ForceGraphCanvas and RadialTreeCanvas.
…linksPanel - MCP tool cairn_get_semantic_neighbors(noteId): queries pre-computed relationship_cache for semantic edges, returns neighbor titles, weights, and matching section titles - KG edge hover tooltips: hovering semantic edges in ForceGraph and RadialTree shows 'NoteA › SectionA ↔ NoteB › SectionB · 87%' - Section-level BacklinksPanel: tracks active ## section from CodeMirror cursor, runs a second semantic search (k=3) on section text, renders 'Related to SectionName' group deduplicated against whole-note hits - ForceGraph: onRenderFramePost draws hovered/selected nodes on top of all other nodes/edges; full title (60 chars) on highlight; removed nodeLabel to eliminate duplicate browser tooltip - RadialTree: bring highlighted nodes to front via DOM re-append; full title on highlight; removed 120-edge cap; semantic edges use 0.5 opacity (was 0.1); edges sorted to prioritize semantic/wikilink - Tooltip wrapping: max-w-[280px] break-words in both canvases - Tooltip hide: clears on node hover, background click, mouseleave - toc-utils: findSectionTitleAtOffset + extractSectionTextAtOffset helpers for cursor-based section tracking
Owner
Author
|
@coderabbitai you seem to be stuck are we good to merge? |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== PR STATUS ==="
gh pr view 54 --repo ddutchie/cairn --json state,mergeable,mergeStateStatus,reviewDecision,statusCheckRollup
echo ""
echo "=== OPEN REVIEW COMMENTS ==="
gh pr view 54 --repo ddutchie/cairn --json reviews,comments
echo ""
echo "=== MY (coderabbitai) COMMENTS ==="
gh api repos/ddutchie/cairn/pulls/54/comments --jq '.[] | select(.user.login == "coderabbitai") | {id: .id, path: .path, body: .body[0:120], resolved: .resolved}'
echo ""
echo "=== PR CHECKS ==="
gh pr checks 54 --repo ddutchie/cairnLength of output: 85976
✅ CI: All green
⏸️ CodeRabbit review: Auto-paused + new review in progress The review was automatically paused due to the burst of commits, and there's a new review run processing the latest changes right now. Let me resume to complete it. Before merging, you may want to consider these previously flagged items (not exhaustive):
Let me trigger the full review now so you get complete inline comments on the latest commits. ✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
changelogs/v2.1.1.md (1)
21-21: 💤 Low valueUse American English "afterward" instead of "afterwards".
American English prefers "afterward"; "afterwards" is more common in British English.
✏️ Proposed fix
- ... Embeddings to backfill existing notes; afterwards, the index stays fresh on its own. + ... Embeddings to backfill existing notes; afterward, the index stays fresh on its own.🤖 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 `@changelogs/v2.1.1.md` at line 21, In the changelog entry for the "Incremental Auto-Reindex on Save" feature, replace the word "afterwards" with "afterward" to use American English spelling conventions, which is the preferred standard for this project.Source: Linters/SAST tools
src/components/notes/BacklinksPanel.tsx (1)
104-108: 💤 Low valueRedundant
=== truein boolean expressions.The chained equality
?.kind === "loading" === trueworks but is confusing due to left-to-right associativity parsing as(?.kind === "loading") === true. Since the comparison already yields a boolean (or undefined, which is falsy), the=== truesuffix adds no value.♻️ Simplify boolean expressions
- const sectionLoading = canSectionSearch && sectionSearch?.kind === "loading" === true; + const sectionLoading = canSectionSearch && sectionSearch?.kind === "loading"; const sectionNoteIds = new Set(sectionHits.map((h) => h.noteId)); const semanticHits = search?.kind === "results" ? search.hits.filter((h) => !sectionNoteIds.has(h.noteId)) : []; - const semanticLoading = search?.kind === "loading" === true; + const semanticLoading = search?.kind === "loading";🤖 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/components/notes/BacklinksPanel.tsx` around lines 104 - 108, The boolean expressions for sectionLoading and semanticLoading variables contain redundant `=== true` suffixes that add no value since the comparisons already evaluate to boolean values. Remove the `=== true` portion from both the sectionLoading assignment (where it checks `sectionSearch?.kind === "loading"`) and the semanticLoading assignment (where it checks `search?.kind === "loading"`), leaving just the direct comparison which will evaluate to the appropriate boolean value.src/components/graph/RadialTreeCanvas.tsx (1)
160-164: 💤 Low valueStale comment references removed functionality.
The comment mentions "120-edge cap" but the edge limit (
slice(0, 120)) was removed per the PR changes. Update or remove this comment to avoid confusion.🔧 Suggested fix
- // Prioritise semantic and wikilink edges so they aren't lost in the 120-edge cap - crossEdges.sort((a, b) => { + // Render semantic and wikilink edges first for visual prominence + crossEdges.sort((a, b) => {🤖 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/components/graph/RadialTreeCanvas.tsx` around lines 160 - 164, The comment in the crossEdges sort function references a "120-edge cap" that no longer exists after removing the slice(0, 120) limit from the code. Update the comment to remove the reference to the 120-edge cap and simply describe the current sorting behavior, or remove the comment entirely if it becomes redundant after the update. Ensure the remaining comment accurately reflects what the sort function is actually doing.
🤖 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 `@electron/embeddings/service.ts`:
- Around line 301-312: The hash comparison in the staleness check is using
incompatible hashing schemes that will never match, causing all notes to be
re-embedded unnecessarily. The issue is in the loop where you compute `hash`
from the full note content (`sha256(text)` where text is
`${n.title}\n\n${n.content_text}`) but then compare it against `r.contentHash`
in the `recs.some()` condition, which stores per-section hashes computed
differently. Fix this by either storing a note-level hash that matches your
full-content hash computation, or by changing the comparison logic to check
section-level hashes correctly by recomputing section hashes and comparing them
against the stored per-section hashes in the records. Ensure the hash
computation method is consistent between what you store and what you compare.
In `@electron/mcp/tools/graph.ts`:
- Around line 44-50: The get_semantic_neighbors function lacks workspace
scoping, allowing cross-workspace data exposure. Update the function signature
to extract and validate workspaceId from the args parameter (similar to how
noteId is validated), returning an error if workspaceId is missing. Then pass
the validated workspaceId to the getSemanticNeighbors function call. Finally,
modify the underlying getSemanticNeighbors implementation to filter results by
workspace, ensuring the semantic neighbors query includes a workspace predicate
through note ownership constraints to maintain data isolation consistent with
get_knowledge_graph and get_neighbors.
In `@src/components/notes/markdown-editor.tsx`:
- Around line 180-186: When coordsAtPos(from) returns null in the
markdown-editor.tsx file, the onSelectionChange callback is not being invoked,
leaving stale selection UI displayed to consumers. Modify the selection change
logic to handle the case when coordsFrom is falsy by still calling
onSelectionChange but with a payload that signals to clear the selection state
(such as passing null or undefined for coords, or an explicit flag). This
ensures that consumers can properly clear outdated selection UI when position
coordinates are unavailable.
In `@src/components/notes/toc-utils.ts`:
- Around line 63-69: The loop logic incorrectly breaks out of the iteration
before evaluating whether the current line is a heading. When the cursor offset
falls within a heading line (the condition `offset < lineEnd` becomes true), the
code breaks immediately without checking if that line matches the heading
pattern with the regex `/^(#{1,2})\s+(.+)$/`. This causes the function to return
the previous section title instead of the current one. Reorder the conditions in
the loop so that the heading match check and currentTitle update happen before
the break condition check, ensuring that if the cursor is positioned on a
heading line, that heading is correctly identified as the current title before
exiting the loop.
---
Nitpick comments:
In `@changelogs/v2.1.1.md`:
- Line 21: In the changelog entry for the "Incremental Auto-Reindex on Save"
feature, replace the word "afterwards" with "afterward" to use American English
spelling conventions, which is the preferred standard for this project.
In `@src/components/graph/RadialTreeCanvas.tsx`:
- Around line 160-164: The comment in the crossEdges sort function references a
"120-edge cap" that no longer exists after removing the slice(0, 120) limit from
the code. Update the comment to remove the reference to the 120-edge cap and
simply describe the current sorting behavior, or remove the comment entirely if
it becomes redundant after the update. Ensure the remaining comment accurately
reflects what the sort function is actually doing.
In `@src/components/notes/BacklinksPanel.tsx`:
- Around line 104-108: The boolean expressions for sectionLoading and
semanticLoading variables contain redundant `=== true` suffixes that add no
value since the comparisons already evaluate to boolean values. Remove the `===
true` portion from both the sectionLoading assignment (where it checks
`sectionSearch?.kind === "loading"`) and the semanticLoading assignment (where
it checks `search?.kind === "loading"`), leaving just the direct comparison
which will evaluate to the appropriate boolean value.
🪄 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 Plus
Run ID: 0c3de4be-f050-4ddf-8da1-671304448047
📒 Files selected for processing (23)
changelogs/v2.1.1.mdelectron/db/graph-queries.test.tselectron/db/graph-queries.tselectron/db/queries.tselectron/db/schema.tselectron/embeddings/chunking.test.tselectron/embeddings/pipeline.tselectron/embeddings/sections.test.tselectron/embeddings/sections.tselectron/embeddings/service.bench.test.tselectron/embeddings/service.tselectron/ipc/embeddings-handlers.tselectron/lib/tool-schemas.tselectron/mcp/tools/graph.tselectron/mcp/tools/index.tselectron/preload.tssrc/components/graph/ForceGraphCanvas.tsxsrc/components/graph/RadialTreeCanvas.tsxsrc/components/notes/BacklinksPanel.tsxsrc/components/notes/markdown-editor.tsxsrc/components/notes/note-editor.tsxsrc/components/notes/toc-utils.tssrc/types/index.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- electron/embeddings/chunking.test.ts
- electron/embeddings/service.bench.test.ts
- electron/preload.ts
- electron/ipc/embeddings-handlers.ts
- electron/embeddings/pipeline.ts
- src/components/notes/note-editor.tsx
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
Owner
Author
|
@coderabbitai thoughts on this new feature? |
|
This is a genuinely well-designed feature. Here are my thoughts: 🟢 What's excellentArchitecture — Running the ONNX model in a separate HTTP worker process is the right call. It isolates ~750 MB of model memory from the Electron main process, allows clean lifecycle management (spawn-on-demand, health polling, SIGTERM on quit), and keeps the preload bridge thin. Section-based embeddings — This is the most interesting design decision. Rather than one vector per note, each Incremental correctness — Test coverage — 506-line graph-queries test suite with realistic cluster scenarios, plus section parser, cosine/topK, chunking termination, projection determinism, and LRU cache tests. This is solid for a feature of this complexity. Semantic UX — The similarity slider (off → 1.0) and the BacklinksPanel "Semantic" section with auto-expand are well thought-out affordances. Users can tune sensitivity rather than live with a hard threshold. 🟡 Concerns to weigh before mergingThese were flagged in the review rounds — some matter more than others:
The Linux binary mismatch is a ship-blocker for any cross-platform release. The incremental recompute and race condition are correctness issues but degrade gracefully (a full reindex fixes them). The 📋 Bottom lineThe feature is production-quality in design and testing. It's a meaningful capability addition — local, private semantic search with no cloud dependency is genuinely useful. The Linux binary name is the one thing I'd fix before a cross-platform release; the others can be addressed in a follow-up. If this is Mac-only for v2.1.1, you're likely fine to merge now. Would you like me to open issues for the items you want to track post-merge? ✅ Action performedFull review finished. |
Owner
Author
|
@coderabbitai. those comments have already been adressed? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== 1. Linux binary name (build-embeddings-binary.js) ==="
grep -n "cairn-embeddings" scripts/build-embeddings-binary.js || echo "NOT FOUND"
echo ""
echo "=== client.ts binary resolution ==="
grep -n "cairn-embeddings" electron/embeddings/client.ts || echo "NOT FOUND"
echo ""
echo "=== 2. Incremental recompute pair guard (graph-queries.ts ~line 768) ==="
grep -n "a\.id.*b\.id\|b\.id.*a\.id\|>=\|canonical" electron/db/graph-queries.ts | head -30
echo ""
echo "=== 3. Stale overwrite race (db-handlers.ts ~lines 152-160) ==="
sed -n '140,180p' electron/ipc/db-handlers.ts
echo ""
echo "=== 4. withLock broadcasts done on failure (embeddings-handlers.ts ~lines 60-75) ==="
sed -n '55,80p' electron/ipc/embeddings-handlers.ts
echo ""
echo "=== 5. queryNoteId exclusion (embeddings-handlers.ts ~line 95) ==="
sed -n '85,110p' electron/ipc/embeddings-handlers.tsLength of output: 5928 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== reindexSingleNoteEmbedding implementation ==="
grep -n "reindexSingleNoteEmbedding\|debounce\|pending\|inFlight\|lock\|queue" electron/ipc/db-handlers.ts | head -30
echo ""
echo "=== Full search handler (queryNoteId + excludeIds) ==="
sed -n '110,160p' electron/ipc/embeddings-handlers.ts
echo ""
echo "=== reindexSingleNoteEmbedding body ==="
grep -n -A 20 "async function reindexSingleNoteEmbedding" electron/ipc/db-handlers.tsLength of output: 4400
You're good to merge. 🐇✨ |
This was referenced Jun 19, 2026
Merged
This was referenced Jul 10, 2026
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.
What does this PR do?
Adds local-first embeddings (
nomic-ai/nomic-embed-text-v1.5via@xenova/transformers+onnxruntime-node) powering semantic Knowledge Graph edges, a Semantic Hubs section in the BacklinksPanel, and incremental auto-reindex on note save. Also fixes an infinite-loop OOM crash inchunkLongText, an install-status bug that hid already-downloaded models, and an upsert-overwrite footgun whererecomputeProjectionssilently wipedsearch_documentrows.Type of change
Screenshots / recording
Checklist
npm run type-check:allpassesnpm run lintpasses (not run)npm testpasses (33 new tests inelectron/embeddings/)npm run test:e2epasses (not run — UI behaviour, please run before merging)var(--accent),var(--text-primary), etc.)text-[Npx]pixel font classes — rem equivalents only (text-[0.714rem],text-xs, etc.)handle()and returnIpcResult<T>schema.ts(v17 — note_embeddings table)electron/db/queries.ts— single source of truth (imported by both Electron main process and MCP server); never construct aDatabaseinstance outsidedb/client.ts(Electron) ormcp-server.ts(MCP runtime)Notes for reviewer
Architecture: The embeddings worker runs as a separate HTTP process (
cairn-embeddingsbinary via@yao-pkg/pkg, or vianodein dev with--max-old-space-size=4096). Electron main spawns it on first embed request, monitors health, and disposes it onbefore-quit.reindexNotesis also called inline on note save for incremental reindexing (skipped ifcontent_hashunchanged — so untouched notes never re-embed). Seedocs/plans/local-embeddings.mdfor the original 8-phase plan with all status checkboxes ticked.Critical fixes worth a second look:
electron/embeddings/service.ts:chunkLongText— make sure the regression test (chunking.test.ts > TERMINATES on very long input) stays green; this was an infinite loop that OOM'd the worker.electron/ipc/db-handlers.ts:130-148— the incremental reindex-after-save is fire-and-forget (void reindexSingleNoteEmbedding(...).then(...)). If you'd prefer it awaited, happy to switch, but blocking note saves on the worker felt wrong.electron/db/queries.ts:upsertNoteEmbeddingstill usesnote_idas primary key. Theclusteringtask was removed (everything now usessearch_document, so recompute is UMAP-only), but the schema is unchanged — if you'd rather switch to a compound key(note_id, task), that's a v18 migration. I chose not to so users keep their indexed vectors across the upgrade.Test coverage:
cosine.test.ts,projection.test.ts,chunking.test.ts(incl. OOM regression),query-cache.test.ts— 33 tests passing. The worker binary itself isn't unit-tested (no good way to mock ONNX inference deterministically); end-to-end behaviour was verified manually in dev mode.Not yet wired up:
onnxruntime-node's binaries for Windows/Linux may need a rebuild step inscripts/rebuild-native.js(currently we ship the npm package defaults). Working on Mac arm64; please smoke-test on Windows before releasing cross-platform builds.SemanticHubsPanel.tsxfile still exists in the tree but is unused (the panel was folded intoBacklinksPanel.tsx). Happy to delete or leave for a follow-up.Summary by CodeRabbit
Release Notes