open new window#59
Conversation
WalkthroughRefactors app startup to centralize IPC registrations and window creation: introduces a WindowManager singleton, registers workspace/port/deep-link/window/terminal IPCs once at startup, converts per-window IPC setup to global one-time handlers, and adds a "New Window" menu item that uses windowManager.createWindow(). Changes
Sequence Diagram(s)sequenceDiagram
participant App as Electron App
participant Startup as index.ts (startup)
participant WM as windowManager
participant IPC as ipcMain
participant Menu as Menu
App->>Startup: app.whenReady()
Startup->>IPC: registerWorkspaceIPCs()
Startup->>IPC: registerPortIpcs()
Startup->>IPC: registerDeepLinkIpcs()
Startup->>IPC: registerWindowIPCs()
Startup->>IPC: registerTerminalIPCs(window?) %% one-time guarded
Startup->>WM: create initial window via factory
Menu->>WM: user selects "New Window"
WM->>WM: createWindow() -> MainWindow instance
WM->>Startup: new BrowserWindow tracked
IPC->>WM: "window-create" handler -> WM.createWindow()
IPC->>TM: terminal IPCs emit -> tmux-manager broadcasts to registered windows
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/desktop/src/main/windows/main.ts (2)
124-131: Closing one window now nukes every window
The new multi-window flow can’t work because closing any window iteratesBrowserWindow.getAllWindows()and forcefullydestroy()s each instance. That bypasses Electron’s normal lifecycle and immediately tears down the other windows as well, so users lose every window whenever they close just one. Please drop the loop (or only destroy the window being closed) and rely on the default close semantics instead;destroy()is documented as a force-close that skipsclose/beforeunloadevents, so it’s only for last-resort crashes, not routine shutdown. (electronjs.org)window.on("close", () => { // Clean up terminal processes cleanupTerminal(); - - for (const window of BrowserWindow.getAllWindows()) { - window.destroy(); - } });
47-98: Register port detector listeners only once
Every call toMainWindow()reattachesportDetectorhandlers, so by the time a few windows have been opened you have N identical listeners firing per event. That balloons log noise, repeats config writes, and eventually triggers listener leak warnings (MaxListenersExceeded)—a classic symptom of duplicated listeners. Move these registrations behind a one-time guard (similar to what you already did for terminal IPCs) so they’re added exactly once per process. (openfin.zendesk.com)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/desktop/src/main/index.ts(2 hunks)apps/desktop/src/main/lib/menu.ts(1 hunks)apps/desktop/src/main/lib/terminal-ipcs.ts(1 hunks)apps/desktop/src/main/lib/window-ipcs.ts(1 hunks)apps/desktop/src/main/lib/window-manager.ts(1 hunks)apps/desktop/src/main/windows/main.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
apps/desktop/src/main/index.ts (4)
apps/desktop/src/main/lib/workspace-ipcs.ts (1)
registerWorkspaceIPCs(16-651)apps/desktop/src/main/lib/port-ipcs.ts (1)
registerPortIpcs(9-69)apps/desktop/src/main/lib/deep-link-ipcs.ts (1)
registerDeepLinkIpcs(7-14)apps/desktop/src/lib/electron-app/factories/app/setup.ts (1)
makeAppSetup(14-56)
apps/desktop/src/main/lib/window-manager.ts (1)
apps/desktop/src/main/windows/main.ts (1)
MainWindow(19-134)
| // Register IPC handlers once at startup (not per-window) | ||
| registerWorkspaceIPCs(); | ||
| registerPortIpcs(); | ||
| registerDeepLinkIpcs(); | ||
|
|
||
| await makeAppSetup(() => windowManager.createWindow()); |
There was a problem hiding this comment.
Wire up the new window-create IPC channel
You added registerWindowIPCs() but never invoke it here, so any renderer calling ipcRenderer.invoke("window-create") will reject because no ipcMain.handle was registered. Please import the new helper and call it alongside the other startup registrations so the channel is live before windows request it. (electronjs.org)
-import { registerWorkspaceIPCs } from "main/lib/workspace-ipcs";
-import { registerPortIpcs } from "main/lib/port-ipcs";
-import { registerDeepLinkIpcs } from "main/lib/deep-link-ipcs";
+import { registerWorkspaceIPCs } from "main/lib/workspace-ipcs";
+import { registerPortIpcs } from "main/lib/port-ipcs";
+import { registerDeepLinkIpcs } from "main/lib/deep-link-ipcs";
+import { registerWindowIPCs } from "main/lib/window-ipcs";
…
// Register IPC handlers once at startup (not per-window)
registerWorkspaceIPCs();
registerPortIpcs();
registerDeepLinkIpcs();
+ registerWindowIPCs();Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/desktop/src/main/index.ts around lines 53 to 58, the new
registerWindowIPCs() helper was added but not invoked at startup, so ipcMain has
no handler for the "window-create" channel and
ipcRenderer.invoke("window-create") will reject; import registerWindowIPCs at
the top of the file (if not already) and call registerWindowIPCs() alongside
registerWorkspaceIPCs(), registerPortIpcs(), and registerDeepLinkIpcs() so the
IPC handler is registered before makeAppSetup creates windows.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/desktop/src/main/lib/terminal-ipcs.ts (2)
1-6: Clarify the dual BrowserWindow import pattern.The import pattern imports
BrowserWindowas both a value (forBrowserWindow.fromWebContentson line 35) and a type alias (BrowserWindowType). While this works, it's unconventional.Consider using a clearer pattern:
-import { - BrowserWindow, - type BrowserWindow as BrowserWindowType, - ipcMain, - shell, -} from "electron"; +import { BrowserWindow, ipcMain, shell } from "electron";Then use
BrowserWindowfor both the type and the value throughout the file.
19-103: Consider renaming for clarity.The function name
registerTerminalIPCssuggests per-window registration, but the implementation registers IPC handlers globally only once (guarded byipcHandlersRegistered). Subsequent calls only set up cleanup hooks for additional windows.Consider renaming to better reflect the actual behavior:
export function setupTerminalIPCsForWindow(window: BrowserWindow) { // Initialize once globally if (!ipcHandlersRegistered) { // ... register global handlers } // Register per-window cleanup window.on("closed", cleanup); }Or split into two functions:
function registerGlobalTerminalIPCs() { /* one-time registration */ } export function attachWindowToTerminals(window: BrowserWindow) { /* cleanup only */ }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/desktop/src/main/index.ts(3 hunks)apps/desktop/src/main/lib/terminal-ipcs.ts(1 hunks)apps/desktop/src/main/lib/tmux-manager.ts(3 hunks)apps/desktop/src/main/windows/main.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/desktop/src/main/windows/main.ts
- apps/desktop/src/main/index.ts
🧰 Additional context used
🧬 Code graph analysis (1)
apps/desktop/src/main/lib/terminal-ipcs.ts (1)
apps/desktop/src/main/lib/terminal.ts (1)
cleanup(214-229)
🔇 Additional comments (9)
apps/desktop/src/main/lib/tmux-manager.ts (5)
34-34: LGTM!The
terminalWindowsmap structure correctly supports the new multi-window architecture, allowing multiple windows to receive output from the same terminal.
55-66: LGTM!The registration logic correctly initializes the Set for new terminals and tracks window associations. The logging provides useful debugging information.
68-82: LGTM!The unregistration logic correctly removes windows and cleans up empty Sets to prevent memory leaks.
84-99: LGTM!The window-wide cleanup correctly removes the window from all terminal associations. This is essential for proper lifecycle management when windows close.
640-655: LGTM!The broadcast logic correctly sends terminal data to all registered windows. The
isDestroyed()check is essential to prevent crashes when sending to destroyed windows.apps/desktop/src/main/lib/terminal-ipcs.ts (4)
9-9: LGTM!The one-time initialization guard prevents duplicate IPC handler registrations across multiple window creations.
11-17: LGTM!The one-time tmux manager initialization is correctly guarded and includes proper error handling.
34-41: LGTM!The terminal-create handler correctly identifies the requesting window and registers it to receive output from the newly created terminal. The null check for
senderWindowis appropriate.
105-114: LGTM!The cleanup logic correctly unregisters the window from all terminals when the window closes. The 'closed' event is the appropriate lifecycle hook for this cleanup.
1 participant
Description
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Summary by CodeRabbit
New Features
Refactor