Intercepteded close event for hidding bg console instead of killing it#301
Conversation
WalkthroughThis pull request introduces changes to control the application's quit behavior and window lifecycle. It sets a global Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![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 (5)
src/main/main.ts (2)
325-338:⚠️ Potential issue | 🟡 Minor
openDevTools()makes the debug menu action one-way; stale comment.The comment on line 326 still says "toggle the devtools," but
openDevTools()only opens and cannot close the DevTools panel. PreviouslytoggleDevTools()let this menu item serve as a true toggle. If the intent is a permanent change, update the comment; if toggling was intentional and this is only meant to guarantee open on first invoke, consideropenDevTools()with{ mode: 'detach' }or revert totoggleDevTools().✏️ Proposed fix
- // Show bg window and toggle the devtools + // Show bg window and open the devtools try {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/main.ts` around lines 325 - 338, The comment for enableBGWindowDebug is misleading because webContents.openDevTools() only opens DevTools (it doesn't toggle); update the comment to reflect that it opens DevTools rather than toggles, or if you want true toggling revert to webContents.toggleDevTools() on globalAny.backgroundWindow.webContents, or call openDevTools({ mode: 'detach' }) if the goal is to ensure it opens in detached mode on first invoke; adjust the comment and use either openDevTools(...) or toggleDevTools() consistently in the enableBGWindowDebug function referencing globalAny.backgroundWindow and its webContents.
325-338:⚠️ Potential issue | 🟡 Minor
openDevTools()is one-directional — DevTools can never be closed via this menu action; stale comment.The function comment on Line 326 still says "toggle the devtools," but
openDevTools()only opens the panel unconditionally. The previoustoggleDevTools()allowed the menu item to also dismiss DevTools. If this is an intentional change (ensure DevTools always opens on first use), update the comment accordingly; otherwise revert totoggleDevTools().✏️ Proposed comment fix (if intentional)
- // Show bg window and toggle the devtools + // Show bg window and open the devtools🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/main.ts` around lines 325 - 338, The comment for enableBGWindowDebug is stale: it says "toggle the devtools" but the code calls globalAny.backgroundWindow.webContents.openDevTools() which only opens DevTools; either update the comment to say "open the devtools" (or "show and open DevTools") to reflect the intentional behavior, or change the call to globalAny.backgroundWindow.webContents.toggleDevTools() to restore toggling behavior; ensure you update the string in the comment near enableBGWindowDebug and verify webContents.toggleDevTools() exists on the backgroundWindow object before switching.src/main/actions/cleanup.js (3)
22-30:⚠️ Potential issue | 🟠 MajorPromise can hang indefinitely if
shutdown-successis never received.If
global.backgroundWindowis truthy but the background renderer crashes during shutdown (or the IPC "shutdown" message is dropped),ipcMain.once("shutdown-success")never fires andgetReadyToQuitAppnever resolves. ThewebAppWindow.on("closed")caller stalls silently with no user feedback and no recovery path. Add a timeout fallback to guarantee resolution.🛡️ Proposed timeout guard
if (global.backgroundWindow) { ipcMain.once("shutdown-success", () => { console.log("shudown sucess"); global.backgroundWindow?.close(); resolve(); }); + setTimeout(() => { + ipcMain.removeAllListeners("shutdown-success"); + global.backgroundWindow?.close(); + resolve(); + }, 5000); } else { resolve(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/actions/cleanup.js` around lines 22 - 30, The current wait for ipcMain.once("shutdown-success") can hang indefinitely; update the shutdown path in getReadyToQuitApp (the block using global.backgroundWindow and ipcMain.once("shutdown-success")) to add a timeout fallback (e.g., 5–10s) that will resolve the promise if the event never arrives, log a warning, and force-close/cleanup global.backgroundWindow; ensure you clear the timeout when the "shutdown-success" handler runs and remove any registered listeners to avoid leaks.
17-31:⚠️ Potential issue | 🟡 Minor
global.isQuittingis never initialised tofalse; close-guard relies onundefinedbeing falsy.The background window's close handler in
startBackgroundProcess.jschecks!global.isQuitting. Becauseglobal.isQuittingis never explicitly set tofalseat startup, the check depends onundefinedbeing falsy — which works, but is fragile and non-obvious. Initialise the flag explicitly so the intent is clear and future refactoring doesn't accidentally break the guard.// e.g. in initGlobalState or top of startBackgroundProcess.js global.isQuitting = false;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/actions/cleanup.js` around lines 17 - 31, Initialize the global quit flag explicitly instead of relying on undefined: set global.isQuitting = false during app startup (e.g., in the init/global state initialization code that runs before startBackgroundProcess) so the close-guard in startBackgroundProcess which checks !global.isQuitting behaves deterministically; ensure getReadyToQuitApp continues to set global.isQuitting = true when initiating shutdown.
22-30:⚠️ Potential issue | 🟠 MajorPromise can hang permanently if
shutdown-successis never received.If
global.backgroundWindowis truthy but the background renderer crashes or never emits"shutdown-success",getReadyToQuitAppnever resolves. The caller (webAppWindow.on("closed")) will silently stall, blocking any post-quit logic. Consider adding a timeout fallback:🛡️ Proposed timeout guard
if (global.backgroundWindow) { ipcMain.once("shutdown-success", () => { console.log("shudown sucess"); global.backgroundWindow?.close(); resolve() }); + setTimeout(() => { + ipcMain.removeAllListeners("shutdown-success"); + global.backgroundWindow?.close(); + resolve(); + }, 5000); } else { resolve(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/actions/cleanup.js` around lines 22 - 30, getReadyToQuitApp can hang when global.backgroundWindow is set because ipcMain.once("shutdown-success") may never fire; add a timeout fallback that resolves the promise after a short delay (e.g., a few seconds) to avoid permanent hangs. Implement this by registering the "shutdown-success" handler (as currently done) and starting a timer; on the event clear the timer and resolve, and on timer expiry remove the event listener, log a warning, force-close backgroundWindow if appropriate, and resolve; reference the existing ipcMain.once("shutdown-success") usage and global.backgroundWindow in your change so the listener and timeout are coordinated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/actions/startBackgroundProcess.js`:
- Around line 72-78: Remove the dead "return false" from the close event handler
on backgroundWindow: inside the backgroundWindow.on('close', (event) => { ... })
callback, rely on event.preventDefault() to cancel the close when
!global.isQuitting and simply call backgroundWindow.hide(); then exit the
handler without returning any value (remove the return false statement).
- Around line 72-78: The close handler on backgroundWindow uses
event.preventDefault() correctly but then returns false which is
dead/misleading; remove the trailing "return false" from the
backgroundWindow.on('close', ...) handler and keep the existing guard using
global.isQuitting, event.preventDefault(), and backgroundWindow.hide() so the
preventDefault logic is the sole mechanism controlling close behavior.
---
Outside diff comments:
In `@src/main/actions/cleanup.js`:
- Around line 22-30: The current wait for ipcMain.once("shutdown-success") can
hang indefinitely; update the shutdown path in getReadyToQuitApp (the block
using global.backgroundWindow and ipcMain.once("shutdown-success")) to add a
timeout fallback (e.g., 5–10s) that will resolve the promise if the event never
arrives, log a warning, and force-close/cleanup global.backgroundWindow; ensure
you clear the timeout when the "shutdown-success" handler runs and remove any
registered listeners to avoid leaks.
- Around line 17-31: Initialize the global quit flag explicitly instead of
relying on undefined: set global.isQuitting = false during app startup (e.g., in
the init/global state initialization code that runs before
startBackgroundProcess) so the close-guard in startBackgroundProcess which
checks !global.isQuitting behaves deterministically; ensure getReadyToQuitApp
continues to set global.isQuitting = true when initiating shutdown.
- Around line 22-30: getReadyToQuitApp can hang when global.backgroundWindow is
set because ipcMain.once("shutdown-success") may never fire; add a timeout
fallback that resolves the promise after a short delay (e.g., a few seconds) to
avoid permanent hangs. Implement this by registering the "shutdown-success"
handler (as currently done) and starting a timer; on the event clear the timer
and resolve, and on timer expiry remove the event listener, log a warning,
force-close backgroundWindow if appropriate, and resolve; reference the existing
ipcMain.once("shutdown-success") usage and global.backgroundWindow in your
change so the listener and timeout are coordinated.
In `@src/main/main.ts`:
- Around line 325-338: The comment for enableBGWindowDebug is misleading because
webContents.openDevTools() only opens DevTools (it doesn't toggle); update the
comment to reflect that it opens DevTools rather than toggles, or if you want
true toggling revert to webContents.toggleDevTools() on
globalAny.backgroundWindow.webContents, or call openDevTools({ mode: 'detach' })
if the goal is to ensure it opens in detached mode on first invoke; adjust the
comment and use either openDevTools(...) or toggleDevTools() consistently in the
enableBGWindowDebug function referencing globalAny.backgroundWindow and its
webContents.
- Around line 325-338: The comment for enableBGWindowDebug is stale: it says
"toggle the devtools" but the code calls
globalAny.backgroundWindow.webContents.openDevTools() which only opens DevTools;
either update the comment to say "open the devtools" (or "show and open
DevTools") to reflect the intentional behavior, or change the call to
globalAny.backgroundWindow.webContents.toggleDevTools() to restore toggling
behavior; ensure you update the string in the comment near enableBGWindowDebug
and verify webContents.toggleDevTools() exists on the backgroundWindow object
before switching.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/actions/cleanup.jssrc/main/actions/startBackgroundProcess.jssrc/main/main.ts
rohitneharabrowserstack
deleted the
ENGG-5314_debug_requestly_bg_window_v2
branch
2 participants
Summary by CodeRabbit
Bug Fixes