Conversation
Guide new users through essential configuration on first launch: - Step 1: Welcome with skip option - Step 2: birda CLI detection with manual path fallback - Step 3: Model installation with live download progress streaming - Step 4: Species language selection Adds setup_completed flag to AppSettings, re-triggerable via Help menu. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary of ChangesHello @tphakala, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the user onboarding experience by implementing a comprehensive first-run setup wizard. This wizard guides new users through critical initial configurations, including verifying the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughAdds a multi-step Setup Wizard (welcome, CLI, model install, language) and integrates it into the app and menu. Implements model-install progress reporting via IPC, expands settings with Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Renderer as Renderer Process
participant Main as Main Process
participant Installer as Child Process
User->>Renderer: Trigger model install (via Wizard UI)
Renderer->>Main: invoke installModel(name, progressCallback)
Main->>Installer: spawn installer process
Installer-->>Main: stdout/stderr chunks
Main->>Main: buffer & split into trimmed lines
Main->>Renderer: send 'birda:models-install-progress' (line)
Renderer->>Renderer: update install progress UI
Installer->>Main: process exits
Main->>Renderer: resolve installModel result
Renderer->>User: show install completion
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
src/main/ipc/models.ts
Outdated
| const win = BrowserWindow.getFocusedWindow(); | ||
| return installModel(name, (line) => { | ||
| win?.webContents.send('birda:models-install-progress', line); | ||
| }); |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Fixed in bf2cb0b — now using event.sender with isDestroyed() guard instead of getFocusedWindow().
There was a problem hiding this comment.
Fixed in bf2cb0b — now using event.sender with isDestroyed() guard instead of getFocusedWindow().
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a setup wizard for the first launch of the application, which is a great feature for user onboarding. However, it introduces two potential Denial of Service (DoS) vulnerabilities in the IPC handling and process management logic, where the main process can crash if a window is closed during model installation or if the installation process produces excessive output. Additionally, there are minor improvements suggested around code duplication and error handling to enhance maintainability and debuggability.
src/main/birda/models.ts
Outdated
| stdout += text; | ||
| if (onProgress) { | ||
| for (const line of text.split('\n')) { | ||
| const trimmed = line.trim(); | ||
| if (trimmed) onProgress(trimmed); | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| proc.stderr.on('data', (data: Buffer) => { | ||
| stderr += data.toString(); | ||
| const text = data.toString(); | ||
| stderr += text; | ||
| if (onProgress) { | ||
| for (const line of text.split('\n')) { | ||
| const trimmed = line.trim(); | ||
| if (trimmed) onProgress(trimmed); | ||
| } | ||
| } |
There was a problem hiding this comment.
The installModel function accumulates the entire standard output and standard error of the spawned process into memory-resident strings (stdout and stderr) without a size limit. This can lead to a Denial of Service (DoS) vulnerability where an unbounded amount of data could cause memory exhaustion and crash the main process. Additionally, processing large chunks of data with text.split('\n') without a size limit can degrade performance. Consider avoiding this accumulation or implementing a strict buffer limit. Furthermore, the logic for processing stream data and reporting progress is duplicated for both stdout and stderr. Extracting this into a shared helper function would improve maintainability and reduce redundancy.
const reportProgress = (text: string) => {
if (onProgress) {
for (const line of text.split('\n')) {
const trimmed = line.trim();
if (trimmed) onProgress(trimmed);
}
}
};
proc.stdout.on('data', (data: Buffer) => {
const text = data.toString();
stdout += text;
reportProgress(text);
});
proc.stderr.on('data', (data: Buffer) => {
const text = data.toString();
stderr += text;
reportProgress(text);
});There was a problem hiding this comment.
Fixed in bf2cb0b — extracted reportProgress helper with proper line buffering across chunk boundaries, and flushing remainders on close.
src/main/ipc/models.ts
Outdated
| const win = BrowserWindow.getFocusedWindow(); | ||
| return installModel(name, (line) => { | ||
| win?.webContents.send('birda:models-install-progress', line); | ||
| }); |
There was a problem hiding this comment.
The onProgress callback captures a reference to the focused window at the time the IPC call is initiated. If the user closes this window while the model installation is still in progress, subsequent attempts to call win.webContents.send will throw an 'Object has been destroyed' error. Since this occurs within an asynchronous callback outside the main IPC handler's promise chain, it will result in an unhandled exception that crashes the entire Electron main process.
To fix this, you should either check if the window is still valid before sending, or better yet, use _event.sender to identify the window that initiated the request and check its status.
ipcMain.handle('birda:models-install', async (event, name: string) => {
const webContents = event.sender;
return installModel(name, (line) => {
if (!webContents.isDestroyed()) {
webContents.send('birda:models-install-progress', line);
}
});
});There was a problem hiding this comment.
Fixed in bf2cb0b — now using event.sender with isDestroyed() check instead of getFocusedWindow().
| async function handleAcceptAndInstall() { | ||
| if (!licenseModel) return; | ||
| const id = licenseModel.id; | ||
| const name = licenseModel.name; |
There was a problem hiding this comment.
The name variable is declared but never used. It can be removed to clean up the code.
There was a problem hiding this comment.
Fixed in bf2cb0b — removed the unused name variable.
| if (currentStep === 'language' && availableLanguages.length === 0 && birdaStatus?.available) { | ||
| void getAvailableLanguages() | ||
| .then((langs) => (availableLanguages = langs)) | ||
| .catch(() => {}); |
There was a problem hiding this comment.
This catch block currently swallows errors silently. While an error here might be expected if birda is not yet available, it's good practice to log the error for debugging purposes. This can help diagnose unexpected issues during development.
.catch((err) => console.error('Failed to get available languages in wizard effect:', err));
There was a problem hiding this comment.
Intentionally silent — these are expected failures when birda is not yet available. The wizard handles this gracefully by showing the CLI-not-found state on the next step.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/renderer/src/App.svelte`:
- Around line 38-40: handleWizardComplete currently only hides the wizard but
doesn't refresh settings; call the same app-state reload used on component mount
to re-fetch updated settings and update appState and any derived vars (e.g.,
species_language, default_model, birda_path). Specifically, inside
handleWizardComplete invoke the function you use on mount to load the app state
(the initializer that populates appState) and assign/merge its result into
appState (and update any local reactive variables) so the UI reflects changes
made by the wizard.
In `@src/renderer/src/lib/components/SetupWizard.svelte`:
- Around line 176-187: Wrap the setSettings calls in handleSkip and handleFinish
with try/catch/finally so that if setSettings throws you capture the error into
a visible component state (e.g., a new saveError / errorMessage reactive
variable) and still call oncomplete() in the finally block; update handleSkip
and handleFinish to use try { await setSettings(...) } catch (err) { saveError =
String(err) || 'Failed to save settings' } finally { oncomplete() } so the user
isn't stuck and sees an error message.
🧹 Nitpick comments (4)
src/main/ipc/models.ts (1)
13-18: Useevent.senderinstead ofgetFocusedWindow()for reliable progress routing.
BrowserWindow.getFocusedWindow()returnsnullif the window isn't focused at the moment the IPC call arrives (e.g., user alt-tabbed). Since the_eventparameter already carries the sender'swebContents, you can route progress reliably regardless of focus state.♻️ Proposed fix
- ipcMain.handle('birda:models-install', async (_event, name: string) => { - const win = BrowserWindow.getFocusedWindow(); + ipcMain.handle('birda:models-install', async (event, name: string) => { + const sender = event.sender; return installModel(name, (line) => { - win?.webContents.send('birda:models-install-progress', line); + if (!sender.isDestroyed()) { + sender.send('birda:models-install-progress', line); + } }); });src/main/birda/models.ts (1)
51-70: Partial-line delivery across chunk boundaries.
dataevents from child-process streams are not guaranteed to align with newline boundaries. A single line of progress output can be split across twodatacallbacks, causingonProgressto receive truncated fragments. For a progress display this is cosmetic, but it can produce garbled text.Consider buffering incomplete lines (i.e., text after the last
\nin each chunk) and flushing them when the next chunk arrives or the stream closes.Example fix for stdout (apply analogously to stderr)
let stdout = ''; let stderr = ''; + let stdoutRemainder = ''; proc.stdout.on('data', (data: Buffer) => { const text = data.toString(); stdout += text; if (onProgress) { - for (const line of text.split('\n')) { + const chunk = stdoutRemainder + text; + const parts = chunk.split('\n'); + stdoutRemainder = parts.pop() ?? ''; + for (const line of parts) { const trimmed = line.trim(); if (trimmed) onProgress(trimmed); } } });Then flush the remainder in the
closehandler:proc.on('close', (code) => { + if (onProgress && stdoutRemainder.trim()) onProgress(stdoutRemainder.trim()); if (code !== 0) {src/renderer/src/lib/components/SetupWizard.svelte (2)
115-138: Unused variablenameon Line 118.
const name = licenseModel.nameis assigned but never read.Suggested fix
const id = licenseModel.id; - const name = licenseModel.name; licenseModel = null;
165-174: No guard against concurrent fetches when navigating back and forth.If the user rapidly switches to the
modelorlanguagestep while a fetch is already in flight (andavailableModels.lengthis still 0),refreshModels()/getAvailableLanguages()will be called again, potentially leading to redundant network requests or race conditions on state assignment.Consider adding a loading flag (e.g.,
modelsLoading) that's checked in the$effectcondition.
- Use event.sender instead of getFocusedWindow() for reliable IPC progress routing and destroyed-window safety - Re-fetch settings after wizard completion so appState reflects changes - Wrap handleSkip/handleFinish in try/catch to prevent trapping user - Buffer partial lines from child process streams before reporting progress - Remove unused variable Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/renderer/src/lib/components/SetupWizard.svelte`:
- Around line 66-73: browseBirdaPath currently awaits openExecutableDialog()
then calls setSettings and checkBirda without error handling, so rejections are
swallowed and birdaStatus/CLI feedback aren't updated; wrap the post-selection
work in a try/catch inside browseBirdaPath, call setSettings and await
checkBirda inside the try, and in the catch set birdaStatus to an error/failure
state (and optionally log or surface the error via your existing
logger/notification mechanism) so the UI reflects failures from setSettings or
checkBirda; keep openExecutableDialog usage the same and only run the try/catch
when a path is returned.
- Around line 58-63: The checkCli function can throw during setSettings({
birda_path: birdaPath }) leaving birdaStatus null and the UI stuck; wrap the
setSettings call in a try/catch (or perform setSettings in a safe await with
.catch) and on error set birdaStatus to an explicit failure state (or a
string/error object) and/or proceed to call checkBirda() in a finally path so
the spinner is cleared and the UI can show an error; update references to
birdaStatus, setSettings, and checkBirda in checkCli to ensure birdaStatus is
always assigned regardless of setSettings failing.
🧹 Nitpick comments (3)
src/renderer/src/lib/components/SetupWizard.svelte (2)
164-172: Fire-and-forget retry in$effectcan loop on persistent failures.If
refreshModels()fails (settingmodelsErrorbut leavingavailableModelsempty), the conditionavailableModels.length === 0remains true. Any subsequent change to a tracked dependency (e.g., toggling steps back and forth) re-triggers the failing call. Consider adding a flag or a short cooldown to avoid repeated failed fetches.
383-391:Nextbutton is disabled until a model is installed—intentional but worth a note.If the birda CLI isn't available, the user can't install models and can't advance past this step (only go back). If this is intentional gating, consider adding a small hint message when
!birdaStatus?.availableto explain why install buttons aren't working, so the user knows to go back and fix the CLI step first.src/renderer/src/App.svelte (1)
121-133: Settings-to-state mapping is duplicated.The logic that maps
getSettings()results ontoappState(theme, confidence, model) appears both inonMount(Lines 123–128) andhandleWizardComplete(Lines 40–45). Consider extracting a small helper likeapplySettings(settings)to keep them in sync.Example
function applySettings(settings: AppSettings) { appState.theme = settings.theme; appState.analysisConfidence = settings.default_confidence; if (settings.default_model) { appState.selectedModel = settings.default_model; } }Then call
applySettings(settings)in both places.Also applies to: 38-48
Wrap setSettings/checkBirda calls in try/catch to prevent permanent loading spinner when IPC fails. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| <button onclick={nextStep} class="btn btn-primary gap-1"> | ||
| {m.wizard_next()} | ||
| <ChevronRight size={16} /> | ||
| </button> |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Good catch! Fixed in c22fef6 — the Next button on the CLI step is now disabled when birdaStatus?.available is false, preventing users from reaching the model step without a working birda CLI. Also added a defensive warning on the model step that shows a clear message if somehow reached without CLI, instead of the infinite loading spinner.
Prevent users from advancing to the model install step without a working birda CLI, which would trap them in an infinite loading state. Also adds a defensive warning on the model step if reached without CLI. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Issue #1 (HIGH): Ensure CPU provider always available - Combine birda providers with default CPU and deduplicate - Prevents losing CPU fallback if birda doesn't include it Issue #2 (MEDIUM): Remove duplicate GpuCapabilities type - Import GpuCapabilities from shared/types.ts in ipc.ts - Eliminates inline type duplication
1 participant
Summary
setup_completedflag toAppSettingsto track first-run stateTest plan
birda-gui-settings.jsonfrom userData and launch — wizard should appear🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements