[ONCALL-184] fix: bg-process opens for windows#240
Conversation
WalkthroughThe pull request updates two main-process TypeScript files. src/main/main.ts contains widespread formatting and stylistic changes (spacing, semicolons, quote normalization, indentation) and a few minor control-flow/syntax refinements that do not change exported signatures. src/main/menu.ts renames three menu items from "Toggle Developer Tools" to "Debug Requestly" and adds calls to this.enableBGWindowDebug() immediately after this.mainWindow.webContents.toggleDevTools() in those menu actions. No public/exported entity declarations were modified. Estimated code review effort🎯 2 (Simple) | ⏱️ ~10–15 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/main.ts (1)
398-437: Protocol registration needs adjustment: checkprocess.defaultAppinstead of platformThe current approach registers the protocol using
process.execPathandargvon all Windows platforms, but Electron's recommended pattern checksprocess.defaultApp(dev mode) instead:if (process.defaultApp) { // dev mode: pass execPath and app entry point so Windows launches correctly if (process.argv.length >= 2) { app.setAsDefaultProtocolClient("requestly", process.execPath, [path.resolve(process.argv[1])]); } } else { // production: simple form sufficient app.setAsDefaultProtocolClient("requestly"); }The code currently passes
execPathandargvon all Windows builds, including packaged production releases, which diverges from Electron's documented best practice. Platform-only checks don't distinguish between development and packaged environments.Additionally, consider calling this earlier (before or during app initialization) rather than inside
loadingScreenWindow.once("show", ...)to ensure timely registration.
🧹 Nitpick comments (4)
src/main/main.ts (4)
85-181: Tray recreation and proxy-dependent menu entries are handled cleanlyDestroying any existing
traybefore creating a new one avoids duplicate tray icons, and splicing out the “Listening On …” submenu wheniporportare not provided keeps the UI sane while the proxy is still initializing. The Windows‑specific.icoicon path also aligns with platform expectations.If you ever support a port value of
0as a sentinel, consider tightening the check toip == null || port == nullinstead of a generic falsy test, but it’s not required for typical proxy ports.
289-317: Windows-specific focus handling forbackgroundWindowis helpful; consider an extra safety checkThe
enableBGWindowDebughelper now:
- shows and focuses
globalAny.backgroundWindow,- on Windows, briefly sets
alwaysOnTopto bring it to the foreground,- then toggles its DevTools.
That matches the PR goal of making the background process window actually surface on Windows. One small robustness tweak you might consider:
- if (process.platform === "win32") { - globalAny.backgroundWindow.setAlwaysOnTop(true); - setTimeout(() => { - globalAny.backgroundWindow.setAlwaysOnTop(false); - }, 100); - } + if (process.platform === "win32") { + globalAny.backgroundWindow.setAlwaysOnTop(true); + setTimeout(() => { + if ( + globalAny.backgroundWindow && + !globalAny.backgroundWindow.isDestroyed() + ) { + globalAny.backgroundWindow.setAlwaysOnTop(false); + } + }, 100); + }to avoid potential errors if the background window is closed within that 100 ms window.
323-327:onWebAppReadyHandlersqueue for open-url/open-file is solid; requeue on file read error may not be neededThe pattern of:
- pushing handlers into
onWebAppReadyHandlerswhenopen-urloropen-fileevents fire beforewebAppWindowis ready, and- draining them once in
executeOnWebAppReadyHandlers()after the window is ready-to-show,is a clean way to avoid dropping those OS-level events.
Two small observations:
In
handleFileOpen’s catch block you do:logger.error(`Error while reading file ${filePath}`, error); onWebAppReadyHandlers.push(() => handleFileOpen(filePath));For actual I/O errors (file missing, permission issues, etc.), re-queueing won’t fix the problem and won’t get executed anyway until the next time you explicitly call
executeOnWebAppReadyHandlers. It might be clearer to just log the error (and maybe surface it to the user) without pushing a new handler.In the
"open-file"app event, logging"webAppWindow not ready"and pushing a handler into the queue is good; just be aware this is macOS-only behavior and that, by design, those queued handlers run only once per window creation.Neither point is blocking, but cleaning up the requeue in the catch block would simplify the mental model.
Also applies to: 330-338, 351-358, 360-382, 384-393
486-500: Shutdown cleanup and error logging improvements look goodIn
before-quit:
- Removing listeners from
ipcMain,webAppWindow, andbackgroundWindowis a reasonable cleanup step to minimize stray event handling during shutdown (the duplicateipcMain.removeAllListeners()is harmless but redundant).- Adding
process.on("uncaughtException")andprocess.on("unhandledRejection")handlers that log vialoggerduring quit helps capture late-stage failures that might otherwise be lost.If you want to tighten this further, you could drop the second
ipcMain.removeAllListeners()call, but it’s not functionally problematic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/main.ts(18 hunks)src/main/menu.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/main/main.ts (2)
src/main/actions/cleanup.js (2)
getReadyToQuitApp(17-31)getReadyToQuitApp(17-31)src/main/events.js (1)
trackRecentlyAccessedFile(46-66)
🔇 Additional comments (4)
src/main/menu.ts (1)
128-135: Consistent wiring ofenableBGWindowDebugwith DevTools toggles looks goodHooking
this.enableBGWindowDebug()immediately after eachtoggleDevTools()keeps the background window debugging behavior consistent across macOS and default menus, and aligns with the Windows focus fix inenableBGWindowDebug. No functional concerns from this change.Also applies to: 328-335, 391-397
src/main/main.ts (3)
15-24: Import restructuring fromelectronis fineSwitching to the grouped destructured import from
"electron"is stylistic and keeps the main-process dependencies clear; no behavioral change here.
197-205: Frameless macOS window options via spread are correctUsing a
framelessOptionsobject only onprocess.platform === "darwin"and then spreading it into theBrowserWindowoptions keeps the macOS customization isolated without affecting Windows/Linux behavior. This is a clean way to introduce platform-specific tweaks.Also applies to: 220-221
457-467: Setapp usage logging changes are benignThe additional
log.logstatements around Setapp integration ("[SETAPP] build identified"and"[SETAPP] integration complete") are low-risk and should help diagnose issues specific to Setapp builds without affecting other distributions.
| let closingAccepted = false; | ||
| const createWindow = async () => { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Sanity-check where "quit-app" is invoked from the renderer side.
rg -n "quit-app" --type ts --type tsxRepository: requestly/requestly-desktop-app
Length of output: 102
🏁 Script executed:
#!/bin/bash
# Search for "quit-app" invocations
rg -n "quit-app" src/Repository: requestly/requestly-desktop-app
Length of output: 136
🏁 Script executed:
#!/bin/bash
# Get file info and read the relevant sections
wc -l src/main/main.ts
echo "---"
# Read around line 183-184
sed -n '175,195p' src/main/main.ts
echo "---"
# Read around line 276-288
sed -n '270,295p' src/main/main.ts
echo "---"
# Read around line 481-484
sed -n '475,490p' src/main/main.tsRepository: requestly/requestly-desktop-app
Length of output: 1694
🏁 Script executed:
#!/bin/bash
# Search for initiate-app-close in the codebase
rg -n "initiate-app-close" src/Repository: requestly/requestly-desktop-app
Length of output: 155
🏁 Script executed:
#!/bin/bash
# Explore directory structure
ls -la src/
echo "---"
# Look for all TypeScript/JavaScript files that might be renderer code
find src/ -type f -name "*.ts" -o -name "*.tsx" -o -name "*.js" | head -20Repository: requestly/requestly-desktop-app
Length of output: 1300
🏁 Script executed:
#!/bin/bash
# Search in renderer directory for initiate-app-close
rg -n "initiate-app-close" src/renderer/
echo "---"
# Also search for quit-app invocation in renderer
rg -n "quit-app" src/renderer/
echo "---"
# Search for ipcRenderer.on or ipcRenderer.handle patterns
rg -n "ipcRenderer\.(on|handle)" src/renderer/ | head -20Repository: requestly/requestly-desktop-app
Length of output: 2010
🏁 Script executed:
#!/bin/bash
# Check the full initEventHandlers.js file for any close-related handlers
cat -n src/renderer/actions/initEventHandlers.js | grep -i "close\|quit"
echo "---"
# Also check if there are any other files that might handle this
rg -r "initiate-app-close|initiate.*close" src/renderer/Repository: requestly/requestly-desktop-app
Length of output: 78
🏁 Script executed:
#!/bin/bash
# Get more context around the close handler and quit-app handler
sed -n '275,290p' src/main/main.ts
echo "---"
sed -n '475,495p' src/main/main.tsRepository: requestly/requestly-desktop-app
Length of output: 1045
Missing renderer-side handler for initiate-app-close creates unrecoverable window lock
The close/quit coordination pattern requires the renderer to listen for initiate-app-close and respond with ipcRenderer.invoke("quit-app"), but no such handler exists in the renderer code. When a user attempts to close the window, the main process prevents the close and sends initiate-app-close to a listener that doesn't exist, leaving the window permanently stuck and unable to close. Add the corresponding IPC handler in the renderer to complete this pattern.
🤖 Prompt for AI Agents
In src/main/main.ts around lines 183-184, the main process sends
'initiate-app-close' but there is no renderer-side listener, which causes the
window to get stuck; add a renderer-side IPC listener (in your preload script if
contextIsolation is enabled, or in the renderer entry file otherwise) that
listens for 'initiate-app-close' and calls ipcRenderer.invoke('quit-app') to
continue the close flow; if using preload/contextBridge, expose a small API or
directly register ipcRenderer.on('initiate-app-close', () =>
ipcRenderer.invoke('quit-app')) so the renderer responds and allows the main
process to proceed.
2 participants
Summary by CodeRabbit
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.