refactor: Modularize Electron main process into single-responsibility components #704
Conversation
… components Extract the monolithic main.ts (~1000 lines) into focused modules: - electron/constants.ts - Window sizing, port defaults, filenames - electron/state.ts - Shared state container - electron/utils/ - Port availability and icon utilities - electron/security/ - API key management - electron/windows/ - Window bounds and main window creation - electron/server/ - Backend and static server management - electron/ipc/ - IPC handlers with shared channel constants Benefits: - Improved testability with isolated modules - Better discoverability and maintainability - Single source of truth for IPC channels (used by both main and preload) - Clear separation of concerns Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Summary of ChangesHello @Shironex, 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 introduces a significant architectural improvement to the Electron main process by breaking down a large, single file into a well-structured set of modular components. This refactoring aims to enhance the application's long-term maintainability, make the codebase easier to navigate, and facilitate independent testing of different functionalities. The changes streamline the Electron lifecycle management and inter-process communication by centralizing configurations and handlers. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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. Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughExtracts and modularizes Electron main-process logic into new files: centralized constants and state, IPC channel registry and handlers, port/icon utilities, API-key management, window bounds persistence, static/backend servers, and a barrel index; main.ts and preload now orchestrate and consume these modules. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Main as Main (electron)
participant Static as StaticServer
participant Backend as BackendProcess
participant Renderer as Renderer (web)
Main->>Main: findAvailablePort(DEFAULT_STATIC_PORT)
Main->>Static: startStaticServer(port)
Note right of Static: Creates HTTP server serving dist/public
Main->>Main: findAvailablePort(DEFAULT_SERVER_PORT)
Main->>Backend: startServer(port, env)
Note right of Backend: spawn Node process with server entry
Main->>Backend: waitForServer() (poll /api/health)
Backend-->>Main: HTTP 200 (ready)
Main->>Renderer: createWindow() (loads UI)
Renderer->>Main: invoke IPC_CHANNELS.SERVER.GET_URL
Main-->>Renderer: return http://localhost:<serverPort>
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![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 is an excellent and significant refactoring of the Electron main process. Breaking down the monolithic main.ts into single-responsibility modules under the new electron/ directory greatly improves the codebase's structure, maintainability, and testability. The new file organization is clear and logical. The introduction of a shared state object and centralized IPC channel constants are great patterns that enhance consistency and reduce coupling. I've found a couple of minor opportunities for simplification in the server startup logic, but overall this is a very solid and well-executed pull request.
Vite bundles all electron modules into a single main.js file, so __dirname remains apps/ui/dist-electron regardless of source file location. Updated path comments to clarify this behavior. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Address review feedback: - Simplify tsx CLI path lookup by extracting path variables and reducing nested try-catch blocks - Remove redundant try-catch around electronAppExists check in production server path validation Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@apps/ui/src/electron/constants.ts`:
- Around line 25-26: DEFAULT_SERVER_PORT and DEFAULT_STATIC_PORT use parseInt on
env vars and can become NaN for non-numeric values; update the module so after
parsing (parseInt or Number.parseInt) you check for NaN (Number.isNaN(parsed) or
!Number.isFinite(parsed)) and if invalid fall back to the numeric defaults 3008
and 3007 respectively; ensure the exported constants are always valid integers
so downstream code like findAvailablePort and server.listen never receive NaN.
In `@apps/ui/src/electron/ipc/auth-handlers.ts`:
- Around line 15-22: The GET_API_KEY handler returns the API key to any caller;
change the ipcMain.handle(IPC_CHANNELS.AUTH.GET_API_KEY, ...) to accept the IPC
event argument, validate the sender (e.g., check event.sender or
event.senderFrame.getURL()/event.sender.getURL() against an allowlist or compare
to the main window's webContents id) before returning sensitive data, and if the
sender is untrusted return null (or throw) as you already do when
state.isExternalServerMode; update references to IPC_CHANNELS.AUTH.GET_API_KEY,
state.isExternalServerMode, and state.apiKey accordingly.
In `@apps/ui/src/electron/ipc/dialog-handlers.ts`:
- Around line 44-53: The OPEN_FILE IPC handler currently spreads
renderer-provided options after a default properties array, allowing override of
the file-only intent; update the ipcMain.handle for
IPC_CHANNELS.DIALOG.OPEN_FILE so it merges options safely by
whitelisting/merging only allowed fields (e.g., accept extra properties but
replace or filter the properties array to ensure ['openFile'] is always
present), and validate returned paths similar to the OPEN_DIRECTORY handler (use
state.mainWindow and dialog.showOpenDialog result validation) to reject non-file
selections before returning the result to the renderer.
In `@apps/ui/src/electron/ipc/shell-handlers.ts`:
- Around line 43-47: The generated VS Code URL is missing the required slash
after "vscode://file" for Windows paths; update the construction logic around
normalizedPath and encodedPath so the final url always includes a slash
separator (e.g., ensure encodedPath begins with '/' before concatenating into
url), preserving the existing encoding logic (use normalizedPath, encodedPath)
and set url = `vscode://file/${encodedPathWithLeadingSlash}` so both Unix and
Windows paths produce `vscode://file/...`.
🧹 Nitpick comments (8)
apps/ui/src/electron/utils/icon-manager.ts (1)
19-33: Use app root instead of__dirnamefor icon path resolution
__dirnamecan shift between dev and packaged builds, which makes../../dist/publicfragile. Prefer anchoring toapp.getAppPath()so the path is tied to the app root and less sensitive to bundling layout changes. Please confirm the packaged layout still matchesdist/public.♻️ Proposed refactor
- const iconPath = isDev - ? path.join(__dirname, '../../public', iconFile) - : path.join(__dirname, '../../dist/public', iconFile); + const appPath = app.getAppPath(); + const iconPath = isDev + ? path.join(appPath, 'public', iconFile) + : path.join(appPath, 'dist', 'public', iconFile);apps/ui/src/electron/ipc/window-handlers.ts (1)
15-23: Consider removing the unusedsidebarExpandedparameter.The
_sidebarExpandedparameter is accepted but not used since the logic now uses a fixed minimum width. If this API contract is no longer needed, removing the parameter would simplify both the handler and the preload API.♻️ Optional: Remove unused parameter
If backward compatibility isn't required:
- ipcMain.handle(IPC_CHANNELS.WINDOW.UPDATE_MIN_WIDTH, (_, _sidebarExpanded: boolean) => { + ipcMain.handle(IPC_CHANNELS.WINDOW.UPDATE_MIN_WIDTH, () => {And update
preload.ts:- updateMinWidth: (sidebarExpanded: boolean): Promise<void> => - ipcRenderer.invoke(IPC_CHANNELS.WINDOW.UPDATE_MIN_WIDTH, sidebarExpanded), + updateMinWidth: (): Promise<void> => + ipcRenderer.invoke(IPC_CHANNELS.WINDOW.UPDATE_MIN_WIDTH),apps/ui/src/electron/ipc/shell-handlers.ts (1)
14-22: Consider validating URL scheme before opening external links.
shell.openExternal()can execute arbitrary protocol handlers, which poses a security risk if the renderer process is compromised. Consider restricting to safe schemes.🔒 Suggested validation
ipcMain.handle(IPC_CHANNELS.SHELL.OPEN_EXTERNAL, async (_, url: string) => { try { + const parsedUrl = new URL(url); + const allowedSchemes = ['http:', 'https:', 'mailto:']; + if (!allowedSchemes.includes(parsedUrl.protocol)) { + return { success: false, error: `Blocked URL scheme: ${parsedUrl.protocol}` }; + } await shell.openExternal(url); return { success: true };apps/ui/src/electron/windows/main-window.ts (1)
63-71: Simplify redundant URL loading branches.The
isDevfallback (lines 66-68) and production branch (lines 69-71) load the same URL. This can be consolidated.♻️ Suggested simplification
// Load Vite dev server in development or static server in production if (VITE_DEV_SERVER_URL) { state.mainWindow.loadURL(VITE_DEV_SERVER_URL); - } else if (isDev) { - // Fallback for dev without Vite server URL - state.mainWindow.loadURL(`http://localhost:${state.staticPort}`); } else { + // Static server URL (dev fallback or production) state.mainWindow.loadURL(`http://localhost:${state.staticPort}`); }apps/ui/src/electron/windows/window-bounds.ts (1)
110-129: Apply minimum size even when recentering off-screen.The off-screen branch returns width/height without MIN_* enforcement; clamping here keeps behavior consistent and guards against corrupted bounds.
♻️ Suggested refinement
- return { - x: x + Math.floor((width - bounds.width) / 2), - y: y + Math.floor((height - bounds.height) / 2), - width: Math.min(bounds.width, width), - height: Math.min(bounds.height, height), - isMaximized: bounds.isMaximized, - }; + const clampedWidth = Math.max(Math.min(bounds.width, width), MIN_WIDTH_COLLAPSED); + const clampedHeight = Math.max(Math.min(bounds.height, height), MIN_HEIGHT); + return { + x: x + Math.floor((width - clampedWidth) / 2), + y: y + Math.floor((height - clampedHeight) / 2), + width: clampedWidth, + height: clampedHeight, + isMaximized: Boolean(bounds.isMaximized), + };apps/ui/src/electron/server/backend-server.ts (3)
66-88: Simplify redundant tsx resolution fallback logic.The nested try-catch structure is confusing and contains duplicate fallback code (lines 72-78 and 81-87 are identical). The outer catch at line 80 will catch any exception from the inner block, making the inner catch at line 76 redundant for recovery purposes.
♻️ Suggested simplification
- let tsxCliPath: string; - // Check for tsx in app bundle paths - try { - if (electronAppExists(path.join(serverNodeModules, 'dist/cli.mjs'))) { - tsxCliPath = path.join(serverNodeModules, 'dist/cli.mjs'); - } else if (electronAppExists(path.join(rootNodeModules, 'dist/cli.mjs'))) { - tsxCliPath = path.join(rootNodeModules, 'dist/cli.mjs'); - } else { - try { - tsxCliPath = require.resolve('tsx/cli.mjs', { - paths: [path.join(__dirname, '../../../server')], - }); - } catch { - throw new Error("Could not find tsx. Please run 'npm install' in the server directory."); - } - } - } catch { - try { - tsxCliPath = require.resolve('tsx/cli.mjs', { - paths: [path.join(__dirname, '../../../server')], - }); - } catch { - throw new Error("Could not find tsx. Please run 'npm install' in the server directory."); - } - } + let tsxCliPath: string | undefined; + // Check for tsx in app bundle paths + const serverTsxPath = path.join(serverNodeModules, 'dist/cli.mjs'); + const rootTsxPath = path.join(rootNodeModules, 'dist/cli.mjs'); + + try { + if (electronAppExists(serverTsxPath)) { + tsxCliPath = serverTsxPath; + } else if (electronAppExists(rootTsxPath)) { + tsxCliPath = rootTsxPath; + } + } catch { + // electronAppExists failed, fall through to require.resolve + } + + if (!tsxCliPath) { + try { + tsxCliPath = require.resolve('tsx/cli.mjs', { + paths: [path.join(__dirname, '../../../server')], + }); + } catch { + throw new Error("Could not find tsx. Please run 'npm install' in the server directory."); + } + }
95-101: Preserve original error context when server path validation fails.The catch block swallows any exception from
electronAppExistsand throws a generic message, losing the original error context (e.g., permission denied). This is inconsistent with the error handling pattern at lines 44-48.♻️ Suggested fix
try { if (!electronAppExists(serverPath)) { throw new Error(`Server not found at: ${serverPath}`); } - } catch { - throw new Error(`Server not found at: ${serverPath}`); + } catch (error) { + const originalError = error instanceof Error ? error.message : String(error); + throw new Error(`Server not found at: ${serverPath}. Reason: ${originalError}`); }
139-139: Non-null assertion onstate.apiKeymay cause silent failures.Using
state.apiKey!assumesensureApiKey()was called successfully beforestartServer(). IfapiKeyis null/undefined, the server would receiveundefinedas theAUTOMAKER_API_KEYenvironment variable, potentially causing authentication issues that are hard to debug.Consider adding a guard or fallback:
♻️ Suggested fix
// Pass API key to server for CSRF protection - AUTOMAKER_API_KEY: state.apiKey!, + AUTOMAKER_API_KEY: state.apiKey ?? '',Or add an explicit check earlier:
if (!state.apiKey) { throw new Error('API key not initialized. Call ensureApiKey() before starting server.'); }
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/ui/src/electron/windows/main-window.ts`:
- Line 49: The window option titleBarStyle is macOS-only; update the
BrowserWindow creation in main-window.ts to set titleBarStyle only when
process.platform === 'darwin' (e.g., use a conditional to assign 'hiddenInset'
for darwin and a cross-platform default otherwise), ensuring the option is not
unconditionally applied so Windows/Linux get consistent styling; modify the code
path that constructs the BrowserWindow options (where titleBarStyle is currently
set) to use this platform check or a cross-platform alternative.
♻️ Duplicate comments (1)
apps/ui/src/electron/server/backend-server.ts (1)
70-90: Redundant fallback logic in tsx path resolution.The outer
catchblock (lines 81-90) duplicates therequire.resolvefallback already present in lines 77-79. IfelectronAppExiststhrows (which is unlikely given it's a simple existence check), both paths attempt the same resolution.♻️ Simplified flow
let tsxCliPath: string; - // Check for tsx in app bundle paths, fallback to require.resolve const serverTsxPath = path.join(serverNodeModules, 'dist/cli.mjs'); const rootTsxPath = path.join(rootNodeModules, 'dist/cli.mjs'); try { if (electronAppExists(serverTsxPath)) { tsxCliPath = serverTsxPath; } else if (electronAppExists(rootTsxPath)) { tsxCliPath = rootTsxPath; } else { - // Fallback to require.resolve tsxCliPath = require.resolve('tsx/cli.mjs', { paths: [path.join(__dirname, '../../server')], }); } - } catch { - // electronAppExists threw or require.resolve failed - try { - tsxCliPath = require.resolve('tsx/cli.mjs', { - paths: [path.join(__dirname, '../../server')], - }); - } catch { + } catch { throw new Error("Could not find tsx. Please run 'npm install' in the server directory."); - } }
🧹 Nitpick comments (6)
apps/ui/src/electron/server/static-server.ts (3)
62-80: Consider handling the case whereindex.htmlitself fails to load.If the initial file isn't found (line 64 falls back to
index.html), but then readingindex.htmlalso fails, the server returns a generic 500 error. This is acceptable, but logging the error would aid debugging in production.Additionally, the nested callback pattern works but could be simplified with promisified versions if
electronAppStat/electronAppReadFilesupport them.
83-89: Error listener is added but never removed.The
'error'listener on line 88 is added but not cleaned up if an error occurs. While this doesn't cause issues during normal operation, ifstartStaticServeris called again after a failed attempt, the old listener remains. Consider removing the listener after resolution or rejection.♻️ Suggested improvement
return new Promise((resolve, reject) => { + const onError = (err: Error) => reject(err); state.staticServer!.listen(state.staticPort, () => { + state.staticServer!.removeListener('error', onError); logger.info('Static server running at http://localhost:' + state.staticPort); resolve(); }); - state.staticServer!.on('error', reject); + state.staticServer!.on('error', onError); });
95-101:stopStaticServerdoes not wait for close to complete.
server.close()is asynchronous. If you need to ensure the server is fully stopped before proceeding (e.g., during app quit or restart), consider returning a Promise or accepting a callback.♻️ Optional async version
-export function stopStaticServer(): void { +export async function stopStaticServer(): Promise<void> { if (state.staticServer) { logger.info('Stopping static server...'); - state.staticServer.close(); - state.staticServer = null; + await new Promise<void>((resolve) => { + state.staticServer!.close(() => resolve()); + }); + state.staticServer = null; } }apps/ui/src/electron/server/backend-server.ts (1)
167-175: Process event handlers don't prevent multiple firings.Both
'close'and'error'events setstate.serverProcess = null. If both events fire (which can happen), the second handler will execute on a null reference. While currently benign, this could mask issues.♻️ Guard against multiple firings
state.serverProcess.on('close', (code) => { + if (!state.serverProcess) return; serverLogger.info('Process exited with code', code); state.serverProcess = null; }); state.serverProcess.on('error', (err) => { + if (!state.serverProcess) return; serverLogger.error('Failed to start server process:', err); state.serverProcess = null; });apps/ui/src/electron/utils/icon-manager.ts (1)
22-28: Simplify platform branching.The
darwinandelsebranches are identical. Consider consolidating them for clarity.♻️ Suggested simplification
let iconFile: string; if (process.platform === 'win32') { iconFile = 'icon.ico'; - } else if (process.platform === 'darwin') { - iconFile = 'logo_larger.png'; } else { + // macOS, Linux, and others use PNG iconFile = 'logo_larger.png'; }apps/ui/src/electron/windows/main-window.ts (1)
64-72: Redundant conditional branches.Lines 67-71 perform the same action for both
isDevwithoutVITE_DEV_SERVER_URLand production. This can be simplified.♻️ Suggested simplification
// Load Vite dev server in development or static server in production if (VITE_DEV_SERVER_URL) { state.mainWindow.loadURL(VITE_DEV_SERVER_URL); - } else if (isDev) { - // Fallback for dev without Vite server URL - state.mainWindow.loadURL(`http://localhost:${state.staticPort}`); } else { + // Fallback for dev without Vite server URL, or production state.mainWindow.loadURL(`http://localhost:${state.staticPort}`); }
- Guard against NaN ports from non-numeric env variables in constants.ts - Validate IPC sender before returning API key to prevent leaking to untrusted senders (webviews, additional windows) - Filter dialog properties to maintain file-only intent and prevent renderer from requesting directories via OPEN_FILE - Fix Windows VS Code URL paths by ensuring leading slash after 'file' Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
titleBarStyle: 'hiddenInset' is a macOS-only option. Use conditional spread to only apply it when process.platform === 'darwin', ensuring Windows and Linux get consistent default styling. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Shironex
added
Refactor
Summary
main.ts(~1000 lines) into focused, single-responsibility moduleselectron/directory structure with clear separation of concernsChanges
New Module Structure
Benefits
Test plan
npm run build:electronsucceedsnpm run dev:electronlaunches correctly🤖 Generated with Claude Code
Summary by CodeRabbit
Refactor
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.