Repository navigation
v2.1.7 - Splash Screen, Better LLM/Embeddings Server - #57
Merged
Merged
Conversation
Migration and Reindex Model intergrated in splash. Allows update if main process crashes.
|
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:
📝 WalkthroughWalkthroughReplaces separate embeddings-server and llama-server child processes with a single ChangesUnified Runtime-Server Refactor
Sequence Diagram(s)sequenceDiagram
participant Main as electron/main
participant Splash as BootSplash
participant Boot as runBootSequence
participant Runtime as runtime/client
participant Server as runtime-server
participant DB as SQLite DB
participant Renderer as Renderer
Main->>Splash: create()
Main->>Boot: runBootSequence(splash, ctx)
Boot->>Boot: checkForUpdates() [prod]
Boot->>DB: runMigrations()
Boot->>Runtime: ensureStarted()
Runtime->>Server: spawn process
Server-->>Runtime: stdout {type:"listening", port}
Boot->>Server: GET /health (via runtimeFetch)
Boot->>DB: reindexNotes + computeSemanticRelationships
Boot->>Main: syncNotesFromDisk
Boot-->>Splash: splash:progress "Ready"
Main->>Main: createWindow() [show: false → show]
Main->>Splash: close()
Renderer->>Runtime: window.electron.runtime.embeddings.status()
Runtime->>Server: GET /health
Server-->>Renderer: EmbeddingsStatus
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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 unit tests (beta)
Comment |
ddutchie
force-pushed
the
ddutchie/entryandsplash
branch
from
June 21, 2026 22:58
5974d85 to
85e1550
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/settings/EmbeddingsSettings.tsx (1)
190-220:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winEarly return doesn't guard against missing
runtimewhen enabling embeddings.The early return on line 193 checks
if (!e) return;butrt(fromwindow.electron?.runtime) is used on line 201 without a guard. Ifwindow.electron.embeddingsexists butwindow.electron.runtimedoesn't, the settings will be saved butensureStarted()silently becomesawait undefined.Consider adding
rtto the guard:🛡️ Suggested fix
const handleToggleEnabled = async (enabled: boolean) => { const e = window.electron?.embeddings; const rt = window.electron?.runtime; - if (!e) return; + if (!e || !rt) return; const next = { ...config, enabled };🤖 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 190 - 220, The handleToggleEnabled function has an early return guard that checks if window.electron.embeddings (stored as e) is available, but it does not verify that window.electron.runtime (stored as rt) exists before attempting to use it when enabled is true. If embeddings exists but runtime does not, the code will silently attempt to call await rt?.embeddings.ensureStarted() which evaluates to await undefined. Add rt to the early return condition alongside e so the function returns early if either the embeddings or runtime module is unavailable, preventing silent failures during the runtime startup flow.
🧹 Nitpick comments (6)
electron/runtime/adapters/llama.ts (2)
573-577: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueUse
execFileSyncfor the quarantine removal command.Same shell injection concern applies here. The filename comes from the extracted archive, and while the archive source is trusted, using
execFileSyncis safer:- try { execSync(`xattr -rd com.apple.quarantine "${destPath}"`, { stdio: "ignore" }); } catch { /* ignore */ } + try { execFileSync("xattr", ["-rd", "com.apple.quarantine", destPath], { stdio: "ignore" }); } catch { /* ignore */ }This requires importing
execFileSyncfromchild_process.🤖 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/runtime/adapters/llama.ts` around lines 573 - 577, Replace the use of execSync with execFileSync for the quarantine removal command in the Darwin platform check block. The current code uses execSync with the xattr command as a shell string which is a security concern. Instead, use execFileSync to invoke the xattr command directly with the destPath as an argument array, passing xattr as the command and an array containing the flags and destination path as separate arguments. Additionally, ensure that execFileSync is imported from the child_process module at the top of the file alongside execSync if not already present.Source: Linters/SAST tools
528-544: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider using
execFilewith argument arrays for defense-in-depth.The static analysis flagged shell command construction. While the paths are internally derived and the risk is low, using
execFilewith argument arrays eliminates shell interpretation entirely:- await new Promise<void>((resolve, reject) => { - const command = process.platform === "win32" - ? `tar -xf "${tempArchive}" -C "${extractDir}"` - : `tar -xzf "${tempArchive}" -C "${extractDir}"`; - exec(command, (err) => { + await new Promise<void>((resolve, reject) => { + const tarArgs = process.platform === "win32" + ? ["-xf", tempArchive, "-C", extractDir] + : ["-xzf", tempArchive, "-C", extractDir]; + execFile("tar", tarArgs, (err) => {This avoids shell metacharacter interpretation if paths ever contain unexpected characters.
🤖 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/runtime/adapters/llama.ts` around lines 528 - 544, Replace the two `exec` calls in the tar extraction logic with `execFile` for improved security. For the initial tar command (lines 528-534), use `execFile` to run tar or PowerShell directly with command arguments as an array instead of constructing a shell command string. Similarly, for the PowerShell fallback command (lines 535-540), use `execFile` with PowerShell as the executable and the command and arguments passed separately. This eliminates shell interpretation of potentially dangerous metacharacters in the tempArchive and extractDir paths.Source: Linters/SAST tools
electron/runtime/server.ts (2)
205-237: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider validating
modelIdbefore passing to adapters.The JSON body is parsed with type assertions (
as { modelId: string }) but if the client sends{}or{"modelId": null}, the adapter receivesundefined/nulland throws a generic "not supported" error. Adding a quick check would provide clearer error messages:const { modelId } = JSON.parse(body) as { modelId: string }; if (!modelId) throw new HttpError(400, "modelId is required");This pattern applies to all model management endpoints (install, remove, setDefault, etc.).
🤖 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/runtime/server.ts` around lines 205 - 237, Add input validation for the modelId parameter in all three model management endpoints (the POST handlers for /v1/embeddings/models/install, /v1/embeddings/models/remove, and /v1/embeddings/models/setDefault). After parsing and destructuring the modelId from the JSON body in each endpoint, check if modelId is empty or falsy and throw or return a 400 error response with a clear message like "modelId is required" before calling the respective adapter methods (installModel, removeModel, setDefaultModelId). This ensures that invalid input is caught early with a meaningful error message instead of being passed to the adapters which would throw generic errors.
332-351: 🧹 Nitpick | 🔵 Trivial | ⚖️ Poor tradeoffChat completions proxy doesn't support streaming responses.
The proxy reads the full response body before forwarding (
await proxyRes.text()). If the client requests"stream": true, they won't receive incremental tokens. If streaming is needed, consider piping the response directly:// For streaming support: const proxyRes = await fetch(...); res.writeHead(proxyRes.status, Object.fromEntries(proxyRes.headers)); Readable.fromWeb(proxyRes.body).pipe(res);If streaming isn't required for the current use case, this can be deferred.
🤖 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/runtime/server.ts` around lines 332 - 351, The chat completions endpoint handler currently buffers the entire response using await proxyRes.text() before sending it to the client, which prevents streaming responses even when the client requests stream: true. Instead of reading and buffering the full response body, modify the fetch handler in the POST /v1/llm/chat/completions endpoint to pipe the response directly to the client by writing the headers first with res.writeHead using the proxy response headers, then piping the response body directly using Readable.fromWeb(proxyRes.body).pipe(res) to support incremental streaming of tokens.electron/splash/bootsplash.ts (1)
114-120: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winReplace hardcoded splash hex colors with CSS-token variables.
The splash palette currently hardcodes multiple hex values; align this with the project color-token policy by sourcing these from CSS variables/theme tokens instead of literals.
As per coding guidelines: “All colours must use CSS variables … and never raw Tailwind colour names.”Also applies to: 360-380
🤖 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/splash/bootsplash.ts` around lines 114 - 120, The splash palette CSS variables in the bootsplash.ts file are using hardcoded hex color values instead of CSS-token variables. Replace all the hardcoded hex values (such as `#e8e8e8`, `#666`, `#8b7bd8`, `#222`, `#22c55e`, `#ef4444`, `#0d0d0d`) for the CSS custom properties --splash-text, --splash-text-dim, --splash-accent, --splash-progress-bg, --splash-success, --splash-error, and --splash-background with corresponding CSS variable references from the project's color-token system. Apply this same change to the additional occurrences mentioned in the comment that appear around lines 360-380 in the same file.Source: Coding guidelines
src/components/settings/AISettings.tsx (1)
134-143: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueNo-op
setServerStatusin catch block.Line 141 does nothing:
setServerStatus((prev) => ({ ...prev }))creates a new object with the same values. This appears to be leftover from when error was set on failure. Consider removing or logging the error to state if needed.♻️ Suggested cleanup
async function handleStartServer(modelId: string) { if (!window.electron || !window.electron.runtime) return; try { await window.electron.runtime.llm.server.start(modelId, aiConfig.contextLimit); await refreshLlamaState(); } catch (e) { console.error("Failed to start llama server:", e); - setServerStatus((prev) => ({ ...prev })); } }🤖 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/AISettings.tsx` around lines 134 - 143, The setServerStatus call in the catch block of handleStartServer is a no-op that spreads the previous state without making any changes, which appears to be leftover code. Either remove this line entirely if error state doesn't need to be tracked, or replace it with a proper state update that sets an error flag or message on the serverStatus to indicate that the server start operation failed.
🤖 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.7.md`:
- Line 25: The release notes at line 25 in changelogs/v2.1.7.md currently state
that `onnxruntime-node` ships "inside the app asar" along with the other runtime
dependencies, but this is inaccurate because `onnxruntime-node` is explicitly
unpacked from the asar archive at runtime via the `asarUnpack` configuration in
`electron-builder.yml`. Update the release notes to clarify that while packages
like `@huggingface/transformers`, `onnxruntime-common`, `sharp`, and `umap-js`
remain inside the asar, `onnxruntime-node` is unpacked from asar and does not
stay inside it at runtime.
In `@electron/embeddings/service.ts`:
- Line 14: Remove the eager import of `../runtime/client` (the line with `import
{ embed as runtimeEmbed } from "../runtime/client"`) and instead use dynamic
import to lazy-load the runtimeEmbed function. Since the EmbedFn type already
returns a Promise, you can safely replace the top-level import with a dynamic
import call inside the function that uses runtimeEmbed. This will defer the
evaluation of app.getPath("userData") until runtime, preventing module
initialization failures in MCP standalone execution paths. Apply this same
lazy-loading pattern to all uses of runtimeEmbed in the file, including the
reference at line 23.
In `@electron/preload.ts`:
- Around line 638-646: The onProgress callback type definition in the preload.ts
file declares fields `bytesReceived` and `bytesTotal`, but the actual
"runtime:download-progress" events emit `loaded` and `total` instead. Update the
callback parameter type in the onProgress method to match the actual emitted
event fields by replacing `bytesReceived` with `loaded` and `bytesTotal` with
`total`, ensuring type safety for consumers of this API.
In `@electron/runtime/client.ts`:
- Around line 198-199: The `bootError` variable is assigned when boot errors
occur but is never cleared upon successful boot recovery, causing old error
messages to persist in the status even after a later successful startup. Reset
`bootError` to an empty string or null in all code paths that indicate
successful boot completion, not just where errors are assigned around line 198.
Check the event handlers and success paths indicated at lines 206-213, 300-303,
and 500-501 to ensure `bootError` is properly cleared alongside any successful
boot confirmations.
- Around line 297-304: The startup wait loop in the runtime client is
calculating timeout incorrectly. The loop at line 297 iterates 600 times with a
200ms sleep plus checkHealth timeout (default 1000ms) per iteration, totaling
approximately 12 minutes instead of the documented 120 seconds. To fix this,
either reduce the loop iteration count to approximately 100 (to match 120s with
current sleep and health check durations), or reduce the sleep duration and
checkHealth timeout values proportionally so the total execution time aligns
with 120 seconds. Apply the same fix to the similar timeout logic mentioned at
lines 307-308.
In `@electron/runtime/port-discovery.ts`:
- Around line 107-116: The execSync("sleep 0.2") call in the while loop blocks
the event loop and is non-portable since sleep may not exist on all platforms
(particularly Windows). Replace this blocking synchronous sleep with a
non-blocking asynchronous delay mechanism by converting the loop to support
async/await and using a Promise-based delay function instead of execSync. This
allows the event loop to remain responsive and eliminates the platform-specific
dependency on the sleep command.
- Around line 97-101: The spawnedRuntime child process is spawned with stdout
and stderr set to pipe mode, but the output streams are never consumed, which
can cause buffer overflow and stall the process. Attach event listeners to the
stdout and stderr streams of the spawnedRuntime instance to consume the piped
output, either by logging it, piping it elsewhere, or simply draining the data
to prevent buffers from filling up and blocking the child process.
- Around line 150-154: The fetch call to http://127.0.0.1:${port}/v1/embed lacks
an abort timeout mechanism, which means requests can hang indefinitely if the
server accepts the connection but never responds. Modify this fetch call by
creating an AbortController, setting a timeout that aborts the controller after
a reasonable duration, passing the controller's signal in the fetch options, and
catching the resulting AbortError in the same error handling block where other
unreachable-runtime cases are handled (treating timeout failures identically to
connection failures).
In `@electron/splash/boot-sequence.ts`:
- Around line 96-124: The update-downloaded and error event listeners registered
with autoUpdater.once() remain attached to the autoUpdater object if the 300s
timeout fires before the download completes. This causes quitAndInstall() to be
called unexpectedly if the download finishes later in the background. In the
finally block after clearing the download-progress listener, add calls to
autoUpdater.off() to deregister both the update-downloaded and error listeners
to ensure these events cannot fire and trigger an unexpected app restart after
the timeout has already rejected the Promise.
In `@electron/splash/bootsplash.ts`:
- Around line 299-303: The code in the done row creation block uses innerHTML
with a concatenated label, which creates an XSS vulnerability if label ever
contains untrusted input. Replace the innerHTML assignment in row with DOM
element creation instead: create the SVG element separately using
document.createElementNS or createElement with proper attribute setting, then
create a separate text node or span element for the label using textContent (not
innerHTML), and append both the SVG and label elements to the row using
appendChild calls rather than string concatenation.
In `@scripts/capture-runtime-baseline.sh`:
- Line 89: The parameter expansion in the rel variable assignment uses an
unquoted $d variable which causes the shell to treat it as a glob pattern,
leading to potential misbehavior when directory paths contain special
characters. Quote the $d variable within the parameter expansion in the rel
assignment to ensure the prefix pattern is treated as a literal string rather
than a glob pattern.
- Around line 124-127: The `port_file` variable is being directly interpolated
into the Python code string on line 126, which can fail if the path contains
special characters like single quotes. Instead of embedding `$port_file` in the
python3 -c string literal, pass the file path as an argument to the Python
command and access it via command-line arguments within the Python code. This
approach is more robust and handles special characters safely.
In `@src/components/layout/ReindexModal.tsx`:
- Around line 46-48: The ReindexModal component is subscribing to the wrong
event source via window.electron?.runtime?.embeddings?.onProgress which is for
model installation, not reindex operations, causing the status checks for
ev.status values to never trigger. Instead of listening to this event, you need
to track reindex progress through the callback passed to reindexNotes() or by
polling window.electron?.embeddings?.status(). Replace the current onProgress
subscription with the appropriate mechanism that receives the (done: number,
total: number) callback signature used by the reindex operation, and update the
progress update logic to handle this different data format instead of checking
for ev.status values.
In `@src/components/search/search-panel.tsx`:
- Around line 130-137: The readiness check for semantic search is updated to use
window.electron?.runtime?.embeddings?.status() but the actual semantic search
call around lines 169-177 still references the old
window.electron.embeddings.search path. Update the semantic search call to use
the new runtime.embeddings namespace to match the readiness check, ensuring
consistency between when the feature is considered ready and when it actually
executes the search operation.
---
Outside diff comments:
In `@src/components/settings/EmbeddingsSettings.tsx`:
- Around line 190-220: The handleToggleEnabled function has an early return
guard that checks if window.electron.embeddings (stored as e) is available, but
it does not verify that window.electron.runtime (stored as rt) exists before
attempting to use it when enabled is true. If embeddings exists but runtime does
not, the code will silently attempt to call await rt?.embeddings.ensureStarted()
which evaluates to await undefined. Add rt to the early return condition
alongside e so the function returns early if either the embeddings or runtime
module is unavailable, preventing silent failures during the runtime startup
flow.
---
Nitpick comments:
In `@electron/runtime/adapters/llama.ts`:
- Around line 573-577: Replace the use of execSync with execFileSync for the
quarantine removal command in the Darwin platform check block. The current code
uses execSync with the xattr command as a shell string which is a security
concern. Instead, use execFileSync to invoke the xattr command directly with the
destPath as an argument array, passing xattr as the command and an array
containing the flags and destination path as separate arguments. Additionally,
ensure that execFileSync is imported from the child_process module at the top of
the file alongside execSync if not already present.
- Around line 528-544: Replace the two `exec` calls in the tar extraction logic
with `execFile` for improved security. For the initial tar command (lines
528-534), use `execFile` to run tar or PowerShell directly with command
arguments as an array instead of constructing a shell command string. Similarly,
for the PowerShell fallback command (lines 535-540), use `execFile` with
PowerShell as the executable and the command and arguments passed separately.
This eliminates shell interpretation of potentially dangerous metacharacters in
the tempArchive and extractDir paths.
In `@electron/runtime/server.ts`:
- Around line 205-237: Add input validation for the modelId parameter in all
three model management endpoints (the POST handlers for
/v1/embeddings/models/install, /v1/embeddings/models/remove, and
/v1/embeddings/models/setDefault). After parsing and destructuring the modelId
from the JSON body in each endpoint, check if modelId is empty or falsy and
throw or return a 400 error response with a clear message like "modelId is
required" before calling the respective adapter methods (installModel,
removeModel, setDefaultModelId). This ensures that invalid input is caught early
with a meaningful error message instead of being passed to the adapters which
would throw generic errors.
- Around line 332-351: The chat completions endpoint handler currently buffers
the entire response using await proxyRes.text() before sending it to the client,
which prevents streaming responses even when the client requests stream: true.
Instead of reading and buffering the full response body, modify the fetch
handler in the POST /v1/llm/chat/completions endpoint to pipe the response
directly to the client by writing the headers first with res.writeHead using the
proxy response headers, then piping the response body directly using
Readable.fromWeb(proxyRes.body).pipe(res) to support incremental streaming of
tokens.
In `@electron/splash/bootsplash.ts`:
- Around line 114-120: The splash palette CSS variables in the bootsplash.ts
file are using hardcoded hex color values instead of CSS-token variables.
Replace all the hardcoded hex values (such as `#e8e8e8`, `#666`, `#8b7bd8`, `#222`,
`#22c55e`, `#ef4444`, `#0d0d0d`) for the CSS custom properties --splash-text,
--splash-text-dim, --splash-accent, --splash-progress-bg, --splash-success,
--splash-error, and --splash-background with corresponding CSS variable
references from the project's color-token system. Apply this same change to the
additional occurrences mentioned in the comment that appear around lines 360-380
in the same file.
In `@src/components/settings/AISettings.tsx`:
- Around line 134-143: The setServerStatus call in the catch block of
handleStartServer is a no-op that spreads the previous state without making any
changes, which appears to be leftover code. Either remove this line entirely if
error state doesn't need to be tracked, or replace it with a proper state update
that sets an error flag or message on the serverStatus to indicate that the
server start operation failed.
🪄 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: c5396635-d698-4b7b-a1c4-bfa8fe22b9f1
⛔ Files ignored due to path filters (1)
electron/splash/cairn-icon.pngis excluded by!**/*.png
📒 Files selected for processing (32)
.gitignorechangelogs/v2.1.7.mdelectron-builder.ymlelectron/bundle-guard.test.tselectron/embeddings/service.tselectron/ipc/handlers.tselectron/ipc/runtime-handlers.tselectron/main.tselectron/mcp-server.tselectron/mcp/tools/graph.tselectron/preload.tselectron/runtime/adapters/embeddings.tselectron/runtime/adapters/llama.tselectron/runtime/adapters/types.tselectron/runtime/client.tselectron/runtime/model-manager.test.tselectron/runtime/model-manager.tselectron/runtime/port-discovery.tselectron/runtime/server.tselectron/splash/boot-sequence.tselectron/splash/bootsplash.tspackage.jsonscripts/build.jsscripts/capture-runtime-baseline.shsrc/app/page.tsxsrc/components/graph/SemanticMapCanvas.tsxsrc/components/layout/ReindexModal.tsxsrc/components/onboarding/StepEmbeddings.tsxsrc/components/onboarding/index.tsxsrc/components/search/search-panel.tsxsrc/components/settings/AISettings.tsxsrc/components/settings/EmbeddingsSettings.tsx
💤 Files with no reviewable changes (1)
- src/app/page.tsx
…een (v2.1.7) - Splash screen with boot sequence: update check, migrations, embeddings reindex, notes sync - Auto-updater with live download progress (prevents broken-build launch) - Unified runtime: single process for LLM + embeddings behind one HTTP API - Adapter interface for future runtimes (MLX etc.) - SHA256 verification on all model downloads + manifest migration - runtime:* IPC namespace + preload API surface - Runtime auto-start on boot when embeddings enabled - Migrate all renderer components to runtime.* channels - Packaged app: ship onnxruntime-common + sharp stub (fixes prod crash) - Bundle-guard: RUNTIME_SHIPPED classification - Security: execFile instead of exec, modelId validation, streaming chat proxy - Baselines captured (packaged app): idle +158MB, both-active -25MB
ddutchie
force-pushed
the
ddutchie/entryandsplash
branch
from
June 22, 2026 01:23
b703f3a to
420348f
Compare
Owner
Author
|
search-panel.tsx — skip by design: This is intentional — by design, DB-touching operations (search, reindex, projections, recompute) stay on the legacy embeddings:* IPC channels because they go through SQLite directly in the Electron main process. Only model management and server control were migrated to the new runtime:* namespace. The status() probe uses runtime.embeddings.status() because it checks the runtime process health, while the actual search() call uses embeddings.search() because it queries the SQLite note_embeddings table. These are two different concerns — one is runtime lifecycle, the other is DB access. bootsplash.ts hex colors — skip by design: The splash window is an isolated BrowserWindow that loads its HTML inline (not from the Next.js static export), so it doesn't have access to the main app's CSS custom properties (--accent, --text-primary, etc.). The hex values in --splash-* variables are a separate palette scoped to the splash window only. Trying to reference the main app's tokens would fail because those tokens are defined in the renderer's :root which doesn't exist in the splash window's document context. The splash already adapts to light/dark via resolvePalette() which reads theme.json and swaps the hex values accordingly. |
|
Tip For best results, initiate chat on the files or code changes.
I'll withdraw those comments and note these design decisions for future reviews. ✏️ Learnings added
|
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
electron/runtime/model-manager.ts (1)
111-118: 🧹 Nitpick | 🔵 Trivial | ⚖️ Poor tradeoffNon-atomic write could truncate manifest on crash.
writeFileSyncwrites in place. If the process crashes mid-write, the manifest file could be left truncated/corrupted. Consider write-to-temp-then-rename for atomicity.♻️ Atomic write pattern
export function writeManifest(manifestPath: string, manifest: Record<string, ManifestEntry>): void { try { fs.mkdirSync(path.dirname(manifestPath), { recursive: true }); - fs.writeFileSync(manifestPath, JSON.stringify(manifest, null, 2), "utf8"); + const tmpPath = `${manifestPath}.tmp`; + fs.writeFileSync(tmpPath, JSON.stringify(manifest, null, 2), "utf8"); + fs.renameSync(tmpPath, manifestPath); } catch (e) { console.error("[model-manager] Failed to write manifest:", e); } }🤖 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/runtime/model-manager.ts` around lines 111 - 118, The writeManifest function uses writeFileSync which writes directly to the manifest file, leaving it vulnerable to truncation if the process crashes mid-write. Modify the function to first write the manifest content to a temporary file in the same directory as the target manifest path, then use fs.renameSync to atomically move the temporary file to the final manifest path location. This ensures the original manifest file is never left in a corrupted state. Make sure to clean up the temporary file if the rename operation fails.
🤖 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/runtime/model-manager.ts`:
- Around line 56-71: The computeFileSha256 function opens a file descriptor with
fs.openSync but may leak it if hash.update throws an exception during the loop,
since fs.closeSync is only called in the happy path. Refactor the function to
use try-finally to ensure the file descriptor is always closed regardless of
whether an exception occurs. Move the fs.closeSync call into a finally block so
cleanup is guaranteed, while keeping the existing catch block to return null on
any errors.
In `@electron/runtime/server.ts`:
- Around line 248-258: The POST endpoint for "/v1/llm/models/install" is missing
validation for the modelId parameter before calling llamaAdapter.installModel().
After destructuring modelId from the parsed JSON body, add a validation check to
ensure modelId is present and not empty, similar to the pattern used in the
embeddings endpoint. If validation fails, send a JSON error response with status
400 and an appropriate error message, then return early before attempting to
call llamaAdapter.installModel().
- Around line 336-359: The response headers being sent back to the client are
hardcoded with Content-Type application/json, which breaks Server-Sent Events
streaming when the llama-server returns Content-Type text/event-stream. Instead
of hardcoding the Content-Type header in the res.writeHead call within the chat
completions endpoint, extract the Content-Type header from the proxyRes object
returned by the llama-server fetch call and pass it through to the client
response. This ensures that streaming clients receive the correct content type
header that the upstream server is sending.
- Around line 281-285: The `/v1/llm/server/stop` endpoint handler is missing
error handling around the `llamaAdapter.stop()` call. Wrap the `await
llamaAdapter.stop()` statement in a try-catch block, similar to the error
handling pattern used in the `/v1/llm/server/start` endpoint (around lines
270-280). In the catch block, log the error appropriately and send an error
response to the client using `sendJson()` with an appropriate HTTP status code
and error details, ensuring the client receives a response instead of hanging if
the stop operation fails.
In `@scripts/capture-runtime-baseline.sh`:
- Around line 52-53: The process filtering in the ps aux grep patterns at lines
52-53, 60-61, and 68-69 use overly generic "Electron" and "Electron Helper"
filters that can match unrelated Electron applications running on the system,
inflating the RSS memory baseline. Modify these grep patterns to be more
specific to Cairn-related processes by adding additional constraints or patterns
that uniquely identify Cairn's Electron processes (such as including the Cairn
app name, specific window titles, or other Cairn-specific identifiers in the
grep expression) to ensure only Cairn processes are captured and counted.
- Line 135: The curl command that health-checks the runtime endpoint at
http://127.0.0.1:$RUNTIME_PORT/health lacks a timeout parameter, allowing it to
block indefinitely if the service is unresponsive. Add a timeout flag to the
curl command to ensure the script continues even if the runtime becomes stalled
or unresponsive.
---
Nitpick comments:
In `@electron/runtime/model-manager.ts`:
- Around line 111-118: The writeManifest function uses writeFileSync which
writes directly to the manifest file, leaving it vulnerable to truncation if the
process crashes mid-write. Modify the function to first write the manifest
content to a temporary file in the same directory as the target manifest path,
then use fs.renameSync to atomically move the temporary file to the final
manifest path location. This ensures the original manifest file is never left in
a corrupted state. Make sure to clean up the temporary file if the rename
operation fails.
🪄 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: 1bafa317-aee9-49e4-9261-147acbef6076
📒 Files selected for processing (31)
.gitignorechangelogs/v2.1.7.mdelectron-builder.ymlelectron/bundle-guard.test.tselectron/embeddings/service.tselectron/ipc/handlers.tselectron/ipc/runtime-handlers.tselectron/main.tselectron/mcp-server.tselectron/mcp/tools/graph.tselectron/preload.tselectron/runtime/adapters/embeddings.tselectron/runtime/adapters/llama.tselectron/runtime/adapters/types.tselectron/runtime/client.tselectron/runtime/model-manager.test.tselectron/runtime/model-manager.tselectron/runtime/port-discovery.tselectron/runtime/server.tselectron/splash/boot-sequence.tselectron/splash/bootsplash.tspackage.jsonscripts/build.jsscripts/capture-runtime-baseline.shsrc/components/graph/SemanticMapCanvas.tsxsrc/components/layout/ReindexModal.tsxsrc/components/onboarding/StepEmbeddings.tsxsrc/components/onboarding/index.tsxsrc/components/search/search-panel.tsxsrc/components/settings/AISettings.tsxsrc/components/settings/EmbeddingsSettings.tsx
✅ Files skipped from review due to trivial changes (3)
- .gitignore
- src/components/layout/ReindexModal.tsx
- changelogs/v2.1.7.md
🚧 Files skipped from review as they are similar to previous changes (25)
- electron/mcp-server.ts
- package.json
- src/components/graph/SemanticMapCanvas.tsx
- electron-builder.yml
- electron/runtime/model-manager.test.ts
- electron/embeddings/service.ts
- src/components/search/search-panel.tsx
- src/components/onboarding/index.tsx
- electron/bundle-guard.test.ts
- scripts/build.js
- electron/ipc/handlers.ts
- electron/runtime/adapters/types.ts
- src/components/onboarding/StepEmbeddings.tsx
- electron/mcp/tools/graph.ts
- electron/runtime/adapters/embeddings.ts
- electron/splash/bootsplash.ts
- electron/preload.ts
- electron/runtime/port-discovery.ts
- electron/splash/boot-sequence.ts
- src/components/settings/EmbeddingsSettings.tsx
- electron/main.ts
- electron/runtime/client.ts
- electron/ipc/runtime-handlers.ts
- src/components/settings/AISettings.tsx
- electron/runtime/adapters/llama.ts
…ontent-type - model-manager.ts: try-finally for fd cleanup, atomic writeManifest via tmp+rename - server.ts: modelId validation on LLM install/remove, try-catch on stop, pass-through content-type for SSE streaming - capture-runtime-baseline.sh: curl --max-time 5, grep Cairn-specific not generic Electron
Owner
Author
|
All fixes committed and pushed. Here's the summary: Fixed: model-manager.ts — computeFileSha256 fd leak: moved closeSync to finally block ReindexModal.tsx — already correct. Reindex progress is broadcast on embeddings:download-progress (via broadcastProgress in withLock), and embeddings.models.onProgress listens on that channel. The reviewer incorrectly assumed these were separate channels. |
|
Tip For best results, initiate chat on the files or code changes.
On (ᵕ—ᴗ—) 🐇 ✏️ Learnings added
|
Owner
Author
|
@coderabbitai. What is still unaddressed please provide prompt. Also please double check unresolved comments |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# List all review comments on the PR with their resolved state
gh api graphql -f query='
{
repository(owner: "ddutchie", name: "cairn") {
pullRequest(number: 57) {
reviewThreads(first: 50) {
nodes {
isResolved
isOutdated
comments(first: 1) {
nodes {
author { login }
body
path
line
url
}
}
}
}
}
}
}' | jq '.data.repository.pullRequest.reviewThreads.nodes[] | select(.isResolved == false and .isOutdated == false) | { path: .comments.nodes[0].path, line: .comments.nodes[0].line, author: .comments.nodes[0].author.login, url: .comments.nodes[0].url, body: .comments.nodes[0].body[0:300] }'Length of output: 1314 🏁 Script executed: #!/bin/bash
# Get the thread node ID and resolve it
THREAD_ID=$(gh api graphql -f query='
{
repository(owner: "ddutchie", name: "cairn") {
pullRequest(number: 57) {
reviewThreads(first: 50) {
nodes {
id
isResolved
comments(first: 1) {
nodes {
url
}
}
}
}
}
}
}' | jq -r '.data.repository.pullRequest.reviewThreads.nodes[] | select(.isResolved == false) | select(.comments.nodes[0].url | contains("r3449202643")) | .id')
echo "Thread ID: $THREAD_ID"
gh api graphql -f query='
mutation($threadId: ID!) {
resolveReviewThread(input: { threadId: $threadId }) {
thread {
isResolved
}
}
}' -f threadId="$THREAD_ID"Length of output: 886
The only open thread was the Full status summary:
Everything is addressed. The PR is clean. 🐇 [resolve] |
This was referenced Jun 22, 2026
Merged
12 of 17 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Introduces a splash screen with a multi-step boot sequence (update check, migrations, embeddings reindex, notes sync) that runs before the main window opens, and refactors the separate embeddings-server and llama-server child processes into a single unified
runtime-serverwith an adapter interface. The auto-updater runs during boot with live download progress so a broken build can heal itself before the renderer loads.Type of change
Screenshots / recording
Checklist
npm run type-check:allpassesnpm run lintpassesnpm testpasses (runsnpm run compilefirst soelectron/bundle-guard.test.tsactually executes —npm run test:bundleto run just that)npm run test:e2epasses (run before merging UI changes or cutting a release)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.tselectron/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)dependenciesordevDependenciesadded toROLE_MAPinscripts/generate-licenses.jsandlicenses.jsonregeneratedelectron/mcp/tools/index.tsdispatch +electron/lib/tool-schemas.tsZod schema--external:<pkg>flag in thecompilescript: updated the appropriate allowlist group (RUNTIME_PROVIDED/OPTIONAL_TRANSITIVE/SUBPROCESS_ONLY/SHIPPED_NATIVE) inelectron/bundle-guard.test.tsand (ifSHIPPED_NATIVE) shipped the package viaelectron-builder.ymlNotes for reviewer
package.jsonstays at 2.1.6; the release script handles versioning. Changelog ischangelogs/v2.1.7.md.BootSplash+boot-sequence.tsfor the first time. The auto-updater integration with live download progress is part of that new feature, not a fix for an existing bug.runtime-serverreplaces the previous separate embeddings-server + llama-server child processes. SHA256 verification,runtime:*IPC namespace, and the adapter interface are all new.scripts/runtime-baselines/.embeddings:*/llama:*IPC channels retained for DB operations (reindex, search, projections) — only model management + server control migrated toruntime:*.Summary by CodeRabbit
Release Notes
New Features
Improvements
Tests/Chores