Repository navigation
v2.1.4 - feat: embed model swap + semantic search + UMAP canvas - #56
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR migrates the local embedding stack from ChangesLocal Embeddings Upgrade: bge-small model, semantic features, onboarding
Sequence Diagram(s)sequenceDiagram
rect rgba(99, 102, 241, 0.5)
Note over Renderer,DB: Startup reindex check
end
participant Renderer
participant preload
participant embeddings_handlers
participant DB
participant EmbeddingsWorker
Renderer->>preload: embeddings.needsReindex()
preload->>embeddings_handlers: IPC embeddings:needsReindex
embeddings_handlers->>DB: getEmbeddingsSettingsCached() + stored model query
DB-->>embeddings_handlers: { currentModel }
embeddings_handlers-->>Renderer: { needed: true, reason: "model_changed" }
Renderer->>Renderer: show ReindexModal (pending phase)
Renderer->>preload: embeddings.reindex(workspaceId)
preload->>embeddings_handlers: IPC db:embeddings:reindex
embeddings_handlers->>EmbeddingsWorker: reindexNotes(model: EMBED_MODEL_ID)
EmbeddingsWorker-->>Renderer: onProgress events (done/total)
EmbeddingsWorker-->>Renderer: onProgress { done: true }
Renderer->>Renderer: transition to "done" phase
rect rgba(16, 185, 129, 0.5)
Note over Agent,service_ts: search_notes_semantic MCP tool
end
participant Agent
participant chat_executor
participant graph_ts
participant service_ts
Agent->>chat_executor: executeTool("search_notes_semantic", args)
chat_executor->>graph_ts: search_notes_semantic(db, args)
graph_ts->>graph_ts: validate workspaceId + query
graph_ts->>service_ts: searchAdjacent(db, workspaceId, query, k, EMBED_MODEL_ID)
service_ts-->>graph_ts: results[]
graph_ts-->>Agent: { query, model, results }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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 docstrings
🧪 Generate unit tests (beta)
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: 13
🧹 Nitpick comments (1)
electron/lib/tool-schemas.ts (1)
351-351: ⚡ Quick winConstrain
kin the schema to prevent oversized semantic queries.
kcurrently accepts any number. Adding integer/positive/max bounds at schema level keeps the tool contract tight and avoids expensive accidental requests.Proposed fix
- k: z.number().optional().describe("Max results (default: 5)"), + k: z.number().int().positive().max(50).optional().describe("Max results (default: 5, max: 50)"),🤖 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/lib/tool-schemas.ts` at line 351, The `k` parameter in the schema currently accepts any number without constraints, which could lead to expensive semantic queries if large values are passed. Modify the `k` field definition to add bounds by chaining constraints to the z.number() call: ensure it accepts only integers using .int(), ensure it is positive using .positive(), and set a reasonable maximum value using .max() to prevent oversized requests and keep the tool contract tight.
🤖 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/manifest.ts`:
- Around line 199-208: After the manifest cleanup loop that removes unsupported
keys from the manifest object, add logic to also reset the default model ID if
it is no longer supported. After calling writeManifestFile(manifest), read the
current default model ID using readDefaultModelId() and check if it exists in
SUPPORTED_EMBEDDING_MODELS. If the default model ID is not in the supported
models, reset it to a valid default value by calling the appropriate method to
write a new default model ID to the default-model file, ensuring consistency
between the cleaned manifest and the persisted default model configuration.
- Around line 173-188: The orphan pruning logic has a symlink vulnerability
where fs.statSync follows symlinks and fs.rmSync could delete directories
outside the cache root. Replace fs.statSync with fs.lstatSync in the directory
validation check to prevent following symlinks, and add path validation using
path.resolve to ensure the resolved modelDir path stays within MODELS_DIR before
attempting recursive deletion with fs.rmSync, preventing accidental deletion of
unintended directories.
In `@electron/ipc/embeddings-handlers.ts`:
- Around line 92-97: The query in the note_embeddings check is selecting only
one distinct model with LIMIT 1, which misses model mismatches when multiple
different models exist in the table. Instead of selecting a model and comparing
it, change the query to check for the existence of any row where the model does
NOT match the current model parameter. Replace the SELECT DISTINCT approach with
a query that returns a row only if a mismatch is found (for example, using a
WHERE clause to filter for rows with a different model value), so that the
condition correctly identifies when reindexing is needed regardless of how many
different models are in the database.
In `@electron/lib/tool-schemas.ts`:
- Around line 346-354: The tool semantic_search_notes is being misclassified as
a write operation because its name does not start with one of the recognized
read operation prefixes (get_, list_, search_) that the toolCategory() function
checks for. Rename the tool from semantic_search_notes to search_notes_semantic
(or another name that begins with search_) to ensure it matches the naming
pattern that toolCategory() uses to correctly classify it as a read operation.
In `@electron/mcp/tools/graph.ts`:
- Around line 273-280: In the searchAdjacent function call, the k parameter is
currently checked for being a positive number but lacks upper bounds clamping,
which allows excessively large values to propagate and cause SQL placeholder
list overflow. Replace the current k parameter logic that checks typeof k ===
"number" && k > 0 ? k : 5 with a clamping approach that not only validates k as
a positive number but also caps it to a reasonable maximum value (such as 100 or
1000) and ensures it is converted to an integer using Math.floor or Math.trunc
to prevent floating-point issues.
In `@src/components/graph/SemanticMapCanvas.tsx`:
- Line 254: In the SemanticMapCanvas.tsx file, the className attribute on line
254 contains the raw Tailwind color token text-white, which violates the
repository's color-token rule requiring all colors to use CSS variables. Replace
text-white with text-[var(--text-primary)] or the appropriate CSS variable token
that matches your repository's theme variables (following the same pattern
already used for bg-[var(--accent)] in the same className), ensuring consistency
with the codebase's CSS variable naming conventions.
- Line 296: The cursor style on line 296 reads isDragging.current from a ref,
which doesn't trigger re-renders when the ref changes, preventing the cursor
from updating during drag operations. Convert isDragging from a ref to React
state using useState, ensure the state is updated whenever drag operations begin
and end (replace all isDragging.current assignments with setState calls), and
update the style prop to use the state value instead of the ref. Additionally,
replace the hardcoded text-white class on line 254 with
text-[var(--text-primary)] to comply with the color token guidelines.
In `@src/components/layout/ReindexModal.tsx`:
- Around line 180-191: Replace the raw Tailwind color name `text-white` in the
className attributes of both buttons (the "Reindex Now" button around line 180
and the dismiss button around line 190) with a semantic CSS variable that
represents white text color, following the color-token contract established in
your design system. This ensures theme consistency and prevents color drift from
the theming system.
- Around line 48-56: The onProgress callback for electron.embeddings.models is
listening to a global channel, so any model download error will trigger the
error state in this modal even if unrelated to the current reindex. Scope the
error handling by adding a check before setting the error state in the else if
(ev.status === "error") condition to verify that the error event is actually
related to the current reindex operation being tracked in this modal, such as by
checking if a reindex is currently active or by verifying the event corresponds
to the model being downloaded in this context.
In `@src/components/onboarding/StepEmbeddings.tsx`:
- Around line 131-132: In the StepEmbeddings component, replace all Tailwind
opacity modifiers (like the `/40` alpha syntax on the accent color) and raw
color utilities (like raw `white` utilities) with semantic CSS variable-based
colors using `color-mix(in srgb, var(--token) X%, transparent)` syntax for alpha
variants. Convert the border and background color declarations throughout the
component to reference CSS variables (--accent, --border, --surface-2, etc.) and
where transparency is needed, use the color-mix function to define the alpha
value instead of Tailwind's opacity syntax, ensuring all color usage follows the
coding guidelines of using only CSS variables and the specified color-mix
pattern for alpha variants.
In `@src/components/search/search-panel.tsx`:
- Around line 247-248: The focused index calculation is based on the grouped UI
order of noteResults and taskResults (shown in lines 343-368), but the Enter key
handler at line 234 selects from the original filtered array using
filtered[focused], causing a mismatch between visual focus and what gets
selected. Fix this by determining which group (noteResults or taskResults) the
currently focused item belongs to based on the focus index, then select from the
appropriate grouped array instead of the original filtered array when handling
the Enter key press.
- Around line 280-295: The Tooltip component directly wraps a disabled button,
which prevents the tooltip from displaying since disabled buttons don't emit
trigger events. To fix this, move the disabled button inside a wrapper element
(such as a div) that remains interactive and can trigger the tooltip. The
wrapper should contain the button with the toggleSemantic onClick handler and
the Sparkles icon, while the Tooltip still wraps the outer container. This
allows the tooltip with the "enable embeddings" help text to display even when
embeddingsReady is false and the button is disabled.
- Around line 170-205: The semantic search in semanticTimer.current fires an
asynchronous request but merges results without verifying the query is still
fresh, allowing stale responses from older queries to contaminate newer search
results. Capture the trimmed query value at the beginning of the semantic search
request, then before merging results in the setResults call around line 194,
check that the current trimmed value still matches the query that was originally
searched. Only proceed with the merge if the queries match, ensuring stale
responses from outdated queries are discarded.
---
Nitpick comments:
In `@electron/lib/tool-schemas.ts`:
- Line 351: The `k` parameter in the schema currently accepts any number without
constraints, which could lead to expensive semantic queries if large values are
passed. Modify the `k` field definition to add bounds by chaining constraints to
the z.number() call: ensure it accepts only integers using .int(), ensure it is
positive using .positive(), and set a reasonable maximum value using .max() to
prevent oversized requests and keep the tool contract tight.
🪄 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: f6414e55-ef98-4288-827f-ef3985cd7e42
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (35)
changelogs/v2.1.4.mdelectron-builder.ymlelectron/db/queries.tselectron/embeddings/client.tselectron/embeddings/manifest.tselectron/embeddings/nomic.tselectron/embeddings/pipeline.tselectron/embeddings/prefix.tselectron/embeddings/query-cache.test.tselectron/embeddings/server.tselectron/embeddings/service.bench.test.tselectron/embeddings/service.tselectron/embeddings/types.tselectron/ipc/chat-executor.tselectron/ipc/embeddings-handlers.tselectron/lib/pi-agent-loop.tselectron/lib/pi-agent-prompt.tselectron/lib/tool-schemas.tselectron/lib/tools.tselectron/mcp/tools/graph.tselectron/mcp/tools/index.tselectron/native-deps-guard.test.tselectron/preload.tspackage.jsonscripts/build-embeddings-binary.jsscripts/generate-licenses.jssrc/app/page.tsxsrc/components/graph/SemanticMapCanvas.tsxsrc/components/insights/InsightsView.tsxsrc/components/layout/ReindexModal.tsxsrc/components/onboarding/StepEmbeddings.tsxsrc/components/onboarding/index.tsxsrc/components/onboarding/shared.tsxsrc/components/search/search-panel.tsxsrc/components/settings/EmbeddingsSettings.tsx
💤 Files with no reviewable changes (1)
- electron/embeddings/nomic.ts
Comment on lines
+346
to
+354
| semantic_search_notes: { | ||
| description: "Semantic search over notes using local embeddings. Sends a natural-language query, embeds it, and returns matching notes ranked by cosine similarity. Use this when keyword search (search_notes) would miss conceptually related notes — e.g. querying 'auth flow' should find notes titled 'Login pipeline'. Requires embeddings to be enabled and indexed. Returns noteId, title, score (0–1), sectionTitle for each match.", | ||
| schema: z.object({ | ||
| workspaceId: sId, | ||
| query: sStr.describe("Natural-language query"), | ||
| k: z.number().optional().describe("Max results (default: 5)"), | ||
| }), | ||
| }, | ||
|
|
There was a problem hiding this comment.
semantic_search_notes is misclassified as a write tool.
Because the tool name doesn’t match get_/list_/search_, toolCategory() (Line 470+) will classify it as "write". This is a read-only operation and should be categorized as "read" to avoid incorrect behavior in any read/write policy paths.
Proposed fix
function toolCategory(name: string): ToolCategory {
if (name.startsWith("delete_")) return "delete";
if (
name.startsWith("get_") ||
name.startsWith("list_") ||
- name.startsWith("search_")
+ name.startsWith("search_") ||
+ name.startsWith("semantic_search_")
) return "read";
return "write";
}📝 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.
Suggested change
| semantic_search_notes: { | |
| description: "Semantic search over notes using local embeddings. Sends a natural-language query, embeds it, and returns matching notes ranked by cosine similarity. Use this when keyword search (search_notes) would miss conceptually related notes — e.g. querying 'auth flow' should find notes titled 'Login pipeline'. Requires embeddings to be enabled and indexed. Returns noteId, title, score (0–1), sectionTitle for each match.", | |
| schema: z.object({ | |
| workspaceId: sId, | |
| query: sStr.describe("Natural-language query"), | |
| k: z.number().optional().describe("Max results (default: 5)"), | |
| }), | |
| }, | |
| function toolCategory(name: string): ToolCategory { | |
| if (name.startsWith("delete_")) return "delete"; | |
| if ( | |
| name.startsWith("get_") || | |
| name.startsWith("list_") || | |
| name.startsWith("search_") || | |
| name.startsWith("semantic_search_") | |
| ) return "read"; | |
| return "write"; | |
| } |
🤖 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/lib/tool-schemas.ts` around lines 346 - 354, The tool
semantic_search_notes is being misclassified as a write operation because its
name does not start with one of the recognized read operation prefixes (get_,
list_, search_) that the toolCategory() function checks for. Rename the tool
from semantic_search_notes to search_notes_semantic (or another name that begins
with search_) to ensure it matches the naming pattern that toolCategory() uses
to correctly classify it as a read operation.
Comment on lines
+131
to
+132
| ? "border-[var(--accent)]/40 bg-[var(--accent-dim)]" | ||
| : "border-[var(--border)] bg-[var(--surface-2)]" |
There was a problem hiding this comment.
Use semantic color tokens + color-mix for alpha variants in this step.
Line 131 uses /40 alpha syntax, and Line 146 / Line 177 use raw white utilities. Please switch these to CSS variables and color-mix(in srgb, var(--token) X%, transparent) where alpha is needed.
As per coding guidelines, **/*.{css,scss,ts,tsx}: “All colours must use CSS variables … never raw Tailwind colour names,” and “Use color-mix(in srgb, var(--token) X%, transparent) for alpha variants.”
Also applies to: 146-147, 177-177
🤖 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/onboarding/StepEmbeddings.tsx` around lines 131 - 132, In the
StepEmbeddings component, replace all Tailwind opacity modifiers (like the `/40`
alpha syntax on the accent color) and raw color utilities (like raw `white`
utilities) with semantic CSS variable-based colors using `color-mix(in srgb,
var(--token) X%, transparent)` syntax for alpha variants. Convert the border and
background color declarations throughout the component to reference CSS
variables (--accent, --border, --surface-2, etc.) and where transparency is
needed, use the color-mix function to define the alpha value instead of
Tailwind's opacity syntax, ensuring all color usage follows the coding
guidelines of using only CSS variables and the specified color-mix pattern for
alpha variants.
Source: Coding guidelines
- Swap embedding model Nomic → bge-small-en-v1.5 (33MB, 384-dim, 4× smaller) - Migrate @xenova/transformers v2 → @huggingface/transformers v4.2.0 - Drop Intel macOS (darwin-x64) build target (onnxruntime-node 1.24.3) - New ReindexModal prompts on startup when model changes detected - New embeddings onboarding step (toggle + model download with progress) - Semantic search in global ⌘K panel (Sparkles toggle, merges with keyword hits) - New semantic_search_notes MCP tool for agents and external MCP clients - New Semantic Map Insights canvas (D3 scatter of UMAP projections) - Surface recomputeProjections inline on canvas empty/stale states - New IPC: embeddings:projections, embeddings:needsReindex - Auto-delete orphaned models on settings panel open - Fix sharp stub to export truthy function (was null, crashed Node 25 worker) - Fix get_semantic_neighbors routing in chat-executor dispatch
ddutchie
force-pushed
the
ddutchie/betterembeddings
branch
from
June 20, 2026 01:01
1809a61 to
13443b3
Compare
- manifest.ts: reset default model ID if unsupported after orphan pruning; use lstatSync to skip symlinked dirs; validate resolved path stays within MODELS_DIR before recursive delete (path traversal protection) - embeddings-handlers.ts: needsReindex query now checks for ANY row with mismatched model (was SELECT DISTINCT LIMIT 1, missed partial states) - tool-schemas.ts: rename semantic_search_notes → search_notes_semantic so toolCategory() classifies it as a read op; add k bounds (int, positive, max 100) - graph.ts: clamp k parameter (Math.floor, max 100) - SemanticMapCanvas: text-white → text-[var(--surface)] - ReindexModal: text-white → text-[var(--surface)]; scope error handler to only fire during active reindex (was triggering on unrelated model downloads) - StepEmbeddings: text-white → text-[var(--surface)]; accent /40 opacity → color-mix(in srgb, var(--accent) 40%, transparent) - search-panel: stale query guard (discard async results if query changed during debounce); fix Enter key index to match grouped UI order (notes then tasks, not interleaved filtered array order)
Owner
Author
|
Skipped findings (with reasons): SemanticMapCanvas isDragging ref→state — Already fixed in the prior lint cleanup commit. Line 300 already uses the dragging state variable, not the ref. |
|
Tip For best results, initiate chat on the files or code changes.
✏️ Learnings added
|
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 `@changelogs/v2.1.4.md`:
- Line 19: Remove the trailing spaces inside the inline code spans on the
changelog entry for bge-small query prefix. The backtick-enclosed terms
`search_query: ` and `search_document: ` currently have spaces before the
closing backticks, which violates markdown formatting rules (MD038). Trim these
spaces so the code spans end immediately after the colons without trailing
whitespace.
🪄 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: 861924d0-2d0f-459f-87f2-be5f7417e1d7
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (37)
changelogs/v2.1.4.mdelectron-builder.ymlelectron/db/queries.tselectron/embeddings/client.tselectron/embeddings/manifest.tselectron/embeddings/nomic.tselectron/embeddings/pipeline.tselectron/embeddings/prefix.tselectron/embeddings/query-cache.test.tselectron/embeddings/server.tselectron/embeddings/service.bench.test.tselectron/embeddings/service.tselectron/embeddings/types.tselectron/ipc/chat-executor.tselectron/ipc/embeddings-handlers.tselectron/lib/pi-agent-loop.tselectron/lib/pi-agent-prompt.tselectron/lib/tool-schemas.tselectron/lib/tools.tselectron/mcp/tools/graph.tselectron/mcp/tools/index.tselectron/native-deps-guard.test.tselectron/preload.tselectron/sharp-stub/index.d.tselectron/sharp-stub/index.jspackage.jsonscripts/build-embeddings-binary.jsscripts/generate-licenses.jssrc/app/page.tsxsrc/components/graph/SemanticMapCanvas.tsxsrc/components/insights/InsightsView.tsxsrc/components/layout/ReindexModal.tsxsrc/components/onboarding/StepEmbeddings.tsxsrc/components/onboarding/index.tsxsrc/components/onboarding/shared.tsxsrc/components/search/search-panel.tsxsrc/components/settings/EmbeddingsSettings.tsx
💤 Files with no reviewable changes (1)
- electron/embeddings/nomic.ts
✅ Files skipped from review due to trivial changes (7)
- electron/sharp-stub/index.d.ts
- electron-builder.yml
- scripts/generate-licenses.js
- src/components/settings/EmbeddingsSettings.tsx
- electron/lib/pi-agent-prompt.ts
- electron/lib/pi-agent-loop.ts
- scripts/build-embeddings-binary.js
🚧 Files skipped from review as they are similar to previous changes (22)
- electron/embeddings/query-cache.test.ts
- electron/ipc/chat-executor.ts
- electron/embeddings/prefix.ts
- src/app/page.tsx
- electron/preload.ts
- electron/embeddings/server.ts
- electron/embeddings/types.ts
- src/components/insights/InsightsView.tsx
- src/components/onboarding/shared.tsx
- src/components/onboarding/index.tsx
- electron/embeddings/manifest.ts
- electron/ipc/embeddings-handlers.ts
- package.json
- electron/embeddings/client.ts
- electron/embeddings/service.bench.test.ts
- electron/embeddings/pipeline.ts
- src/components/onboarding/StepEmbeddings.tsx
- electron/db/queries.ts
- electron/embeddings/service.ts
- src/components/layout/ReindexModal.tsx
- src/components/search/search-panel.tsx
- src/components/graph/SemanticMapCanvas.tsx
Owner
Author
|
@coderabbitai whats left to address. please provide final prompt for agent and double check if things have been adressed |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Check open review comments and PR status
gh pr view 56 --repo ddutchie/cairn --json state,reviewDecision,reviews,comments | jq '{state, reviewDecision}'
echo "--- Open review threads ---"
gh api graphql -F owner=ddutchie -F name=cairn -F pr=56 -f query='
query($owner: String!, $name: String!, $pr: Int!) {
repository(owner: $owner, name: $name) {
pullRequest(number: $pr) {
reviewThreads(first: 50) {
nodes {
isResolved
comments(first: 1) {
nodes {
author { login }
body
}
}
}
}
}
}
}' | jq '.data.repository.pullRequest.reviewThreads.nodes[] | select(.isResolved == false) | {isResolved, firstComment: .comments.nodes[0].body[:120]}'Length of output: 1408 🏁 Script executed: #!/bin/bash
gh api graphql -F owner=ddutchie -F name=cairn -F pr=56 -f query='
query($owner: String!, $name: String!, $pr: Int!) {
repository(owner: $owner, name: $name) {
pullRequest(number: $pr) {
reviewThreads(first: 50) {
nodes {
isResolved
path
line
comments(first: 10) {
nodes {
author { login }
body
}
}
}
}
}
}
}' | jq '.data.repository.pullRequest.reviewThreads.nodes[] | select(.isResolved == false)'Length of output: 8974
✅ Already addressed
🔴 Still open (2 code issues)Thread A — Thread B — 📋 Pre-merge checklist (from PR description)
🤖 Final agent prompt |
- StepEmbeddings: bg-white → bg-[var(--accent-fg,#fff)] on toggle knob (matches button.tsx pattern, respects --accent-fg if defined) - search-panel: wrap disabled Sparkles button in <span> so Tooltip trigger lands on interactive element, not the disabled button (fixes tooltip not showing when embeddings are disabled)
Owner
Author
|
@coderabbitai |
|
Tip For best results, initiate chat on the files or code changes.
The two remaining open code issues from the agent prompt are still outstanding:
Plus the pre-merge checklist items: |
Owner
Author
|
@coderabbitai everything should be addressed. Give approval and your thoughts on this PR. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Check open review threads
gh api graphql -F owner=ddutchie -F name=cairn -F pr=56 -f query='
query($owner: String!, $name: String!, $pr: Int!) {
repository(owner: $owner, name: $name) {
pullRequest(number: $pr) {
reviewThreads(first: 50) {
nodes {
isResolved
path
line
comments(first: 5) {
nodes {
author { login }
body
}
}
}
}
}
}
}' | jq '.data.repository.pullRequest.reviewThreads.nodes[] | select(.isResolved == false) | {path, line, firstComment: .comments.nodes[0].body[:150]}'
echo "--- PR state ---"
gh pr view 56 --repo ddutchie/cairn --json state,title,headRefNameLength of output: 1418 🏁 Script executed: #!/bin/bash
# Check current state of the two files with open threads
echo "=== StepEmbeddings.tsx lines around 125-185 ==="
fd "StepEmbeddings.tsx" --exec cat {} | head -200
echo ""
echo "=== search-panel.tsx lines around 280-320 ==="
fd "search-panel.tsx" --exec grep -n "Sparkles\|semantic\|Tooltip\|disabled\|span\|cursor-not-allowed\|embeddingsReady\|color-mix\|white\|/40" {} | head -60
echo ""
echo "=== tool-schemas.ts - search_notes_semantic entry ==="
fd "tool-schemas.ts" --exec grep -n "semantic\|search_notes\|toolCategory\|CHAT_ONLY" {} | head -20Length of output: 12039
Overall thoughts on PR This is a substantial, well-executed feature PR. The model swap (Nomic 768-dim → bge-small 384-dim) is handled cleanly — the The new surfaces — semantic ⌘K toggle with confidence scores, The only outstanding pre-merge items remain operational (not code issues): (ﾐ ᵕ̤ ฅ)⋆* 🐇✨ [approve] |
This was referenced Jun 21, 2026
This was referenced Jul 11, 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?
Swaps the local embedding model from Nomic (137 MB, 768-dim) to bge-small-en-v1.5 (33 MB, 384-dim) via the new official
@huggingface/transformersv4, and exposes embeddings search/projections to users and agents for the first time: semantic search in the global ⌘K panel, a newsemantic_search_notesMCP tool, a new "Semantic Map" Insights canvas with inline UMAP recompute, an embeddings onboarding step, and a reindex-on-model-change modal.Type of change
Screenshots / recording
TODO — add before merge:
Sparkles 87%badgeChecklist
npm run type-check:allpassesnpm run lintpasses (not run — please run before merge)npm testpasses (519/519)npm run test:e2epasses (run before merging — UI changes present)var(--accent),var(--text-primary), etc.)text-[Npx]pixel font classes — rem equivalents only (text-[0.714rem],text-xs, etc.)handle()and returnIpcResult<T>—embeddings:projections,embeddings:needsReindexschema.ts(no schema change —dim_x/dim_y/proj_stalecolumns already existed; new query only)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) —getNoteProjections()added theredependenciesordevDependenciesadded toROLE_MAPinscripts/generate-licenses.jsandlicenses.jsonregenerated (needs check:@huggingface/transformersreplaced@xenova/transformers;onnxruntime-nodebumped to 1.24.3)electron/mcp/tools/index.tsdispatch +electron/lib/tool-schemas.tsZod schema —semantic_search_notesNotes for reviewer
onnxruntime-node1.24.3 (the version@huggingface/transformersv4 pins) no longer ships adarwin-x64prebuilt.electron-builder.ymlmacOS arch is now[arm64]only. Intel Mac users should stay on v2.1.3. Flagging since this is the first platform drop.vectorcolumn isTEXT(JSON-serialized), so 768→384 needs no migration; model ID mismatch inreindexNotestriggers automatic re-embed on next reindex.ReindexModalprompts the user on startup when a mismatch is detected.graph.ts—semantic_search_notesusesawait import("../../embeddings/service")rather than a top-level import. This is deliberate:service.ts→manifest.tscallsapp.getPath("userData")at module load, which isn't available in the MCP test environment. Static import broke 6 test suites (mcp-server, chat-executor, handlers, context-audit, pi-agent-loop, payload-baseline). Dynamic import defers evaluation to call time.get_semantic_neighborsrouting fix is incidental — chat-executor was missing this case in its dispatch group, so AI chat calls to this pre-existing MCP tool fell through to "Unknown tool". Folded into the same case group asget_knowledge_graph/get_neighbors. Not strictly part of this feature but tests would have caught it once the suite was green again.@huggingface/transformersis new,@xenova/transformersis removed. I didn't runscripts/generate-licenses.jsbecause I'm not sure if it needsROLE_MAPupdates first. Please advise or run locally.npm run test:e2ebefore merge.Summary by CodeRabbit
New Features
Improvements
Bug Fixes