Added logic for keeping background process runing on closing of debug…#299
Added logic for keeping background process runing on closing of debug…#299rohitneharabrowserstack wants to merge 4 commits intomasterfrom
Conversation
Walkthroughcleanup.js now sets Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ 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 |
![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/actions/cleanup.js (1)
17-49:⚠️ Potential issue | 🟡 Minor
ipcMain.once("shutdown-success")listener is never removed on the timeout path.If the timeout fires first (because the background process never sends
"shutdown-success"), the promise resolves but theipcMain.once("shutdown-success")listener remains registered. If a stale"shutdown-success"message arrives later (e.g., from a new background window in a subsequent session), it will invoke the old callback on a now-destroyed window reference.Remove the listener when the timeout fires.
Sketch
+ const onShutdownSuccess = () => { + clearTimeout(shutdownTimeout); + // ... destroy + resolve + }; + ipcMain.once("shutdown-success", onShutdownSuccess); + const shutdownTimeout = setTimeout(() => { + ipcMain.removeListener("shutdown-success", onShutdownSuccess); // ... destroy + resolve }, 2000);🤖 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 - 49, The ipcMain.once("shutdown-success") handler registered in getReadyToQuitApp can remain after the timeout fires; change the logic to capture the listener callback in a variable (the function passed to ipcMain.once) and, inside the timeout fallback before resolving, remove that listener (using ipcMain.removeListener or ipcMain.off with the same event name and captured callback) so the stale callback cannot run later against global.backgroundWindow; keep the existing destroy/_originalDestroy calls and resolve behavior intact.
🧹 Nitpick comments (3)
src/main/actions/startBackgroundProcess.js (3)
72-120: Consider extracting lifecycle protection into a helper function.The close handler, destroy override, and removeAllListeners override collectively form a single "protect window from destruction" concern spread across ~50 lines in the middle of
startBackgroundProcess. Extracting this into a named helper (e.g.,protectWindowFromDestruction(window)) would improve readability and make it testable independently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/actions/startBackgroundProcess.js` around lines 72 - 120, Extract the window-lifecycle protection logic into a helper named protectWindowFromDestruction(window): move the closeHandler, its attachment, the _preventCloseHandler assignment, the overridden destroy (using originalDestroy and _originalDestroy), and the overridden removeAllListeners (using originalRemoveAllListeners) into that function; have startBackgroundProcess call protectWindowFromDestruction(backgroundWindow) before setupIPCForwardingToBackground and before setting the global flags so behavior is unchanged but the lifecycle-protection concern is isolated and testable.
72-80:event.returnValue = falseis unnecessary in acloseevent handler.In Electron,
event.preventDefault()on thecloseevent is sufficient to prevent the window from closing.event.returnValueis only meaningful in synchronous IPC orbeforeunloadhandlers. Line 75 can be removed for clarity.Proposed fix
const closeHandler = (event) => { event.preventDefault(); - event.returnValue = false; if (!backgroundWindow.isDestroyed()) { backgroundWindow.hide(); } - return false; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/actions/startBackgroundProcess.js` around lines 72 - 80, The close handler sets event.returnValue which is unnecessary for the Electron 'close' event; update the closeHandler (the function assigned where backgroundWindow is handled) to remove the line setting event.returnValue and rely solely on event.preventDefault() and backgroundWindow.hide(); keep the checks for backgroundWindow.isDestroyed() and the final return false if desired, but delete the redundant event.returnValue assignment to simplify the handler.
103-113: Consider a less invasive approach to protecting the close handler.Monkey-patching
removeAllListenersis fragile and brittle across Electron upgrades. WhileremoveListeneris not currently patched or called in the codebase, this approach relies on incomplete protections that could be bypassed.Instead of patching core EventEmitter methods, document that the close handler must not be removed, or refactor to guard the removal at the point where it happens (e.g., skip removing
'close'listeners in any code that callsremoveAllListeners).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/actions/startBackgroundProcess.js` around lines 103 - 113, You currently monkey-patch backgroundWindow.removeAllListeners to protect the closeHandler, which is fragile; instead revert that override and either (A) add a small safe utility or guard at call sites that remove listeners so they skip the 'close' event (e.g., replace calls to backgroundWindow.removeAllListeners(...) with a safeRemoveAllListeners(target, eventName) helper that delegates except when eventName is 'close' or undefined), or (B) centralize listener cleanup in a single function that knows not to remove the 'close' listener; update/remove the override of removeAllListeners and ensure places that previously cleared listeners call the new safe helper or the centralized cleanup, referencing backgroundWindow, closeHandler, removeAllListeners, and removeListener as needed.
🤖 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/cleanup.js`:
- Around line 25-45: The shutdown handler for ipcMain.once("shutdown-success")
can race with the timeout and call destroy/resolve twice; store the timeout
handle returned by setTimeout, then in the "shutdown-success" callback first
clearTimeout(timeoutHandle) and check global.backgroundWindow &&
!global.backgroundWindow.isDestroyed() before calling either
global.backgroundWindow._originalDestroy() or global.backgroundWindow.destroy();
likewise keep the existing isDestroyed() guard in the timeout path and only call
resolve() once per path after performing the guarded destroy. Ensure you
reference ipcMain.once("shutdown-success"), the timeout handle from
setTimeout(...), global.backgroundWindow, _originalDestroy, isDestroyed(), and
resolve() when applying the fix.
In `@src/main/actions/startBackgroundProcess.js`:
- Around line 87-98: The overridden destroy implementation calls
originalDestroy() unconditionally when global.allowBackgroundWindowDestruction
is true which can throw if the window is already destroyed; update the
backgroundWindow.destroy override (the function that binds originalDestroy and
replaces backgroundWindow.destroy) to check backgroundWindow.isDestroyed()
before calling originalDestroy(), mirroring the other guard: only call
originalDestroy() when global.allowBackgroundWindowDestruction is true AND
!backgroundWindow.isDestroyed(), otherwise fall back to hiding when not
destroyed.
---
Outside diff comments:
In `@src/main/actions/cleanup.js`:
- Around line 17-49: The ipcMain.once("shutdown-success") handler registered in
getReadyToQuitApp can remain after the timeout fires; change the logic to
capture the listener callback in a variable (the function passed to
ipcMain.once) and, inside the timeout fallback before resolving, remove that
listener (using ipcMain.removeListener or ipcMain.off with the same event name
and captured callback) so the stale callback cannot run later against
global.backgroundWindow; keep the existing destroy/_originalDestroy calls and
resolve behavior intact.
---
Nitpick comments:
In `@src/main/actions/startBackgroundProcess.js`:
- Around line 72-120: Extract the window-lifecycle protection logic into a
helper named protectWindowFromDestruction(window): move the closeHandler, its
attachment, the _preventCloseHandler assignment, the overridden destroy (using
originalDestroy and _originalDestroy), and the overridden removeAllListeners
(using originalRemoveAllListeners) into that function; have
startBackgroundProcess call protectWindowFromDestruction(backgroundWindow)
before setupIPCForwardingToBackground and before setting the global flags so
behavior is unchanged but the lifecycle-protection concern is isolated and
testable.
- Around line 72-80: The close handler sets event.returnValue which is
unnecessary for the Electron 'close' event; update the closeHandler (the
function assigned where backgroundWindow is handled) to remove the line setting
event.returnValue and rely solely on event.preventDefault() and
backgroundWindow.hide(); keep the checks for backgroundWindow.isDestroyed() and
the final return false if desired, but delete the redundant event.returnValue
assignment to simplify the handler.
- Around line 103-113: You currently monkey-patch
backgroundWindow.removeAllListeners to protect the closeHandler, which is
fragile; instead revert that override and either (A) add a small safe utility or
guard at call sites that remove listeners so they skip the 'close' event (e.g.,
replace calls to backgroundWindow.removeAllListeners(...) with a
safeRemoveAllListeners(target, eventName) helper that delegates except when
eventName is 'close' or undefined), or (B) centralize listener cleanup in a
single function that knows not to remove the 'close' listener; update/remove the
override of removeAllListeners and ensure places that previously cleared
listeners call the new safe helper or the centralized cleanup, referencing
backgroundWindow, closeHandler, removeAllListeners, and removeListener as
needed.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/main/actions/startBackgroundProcess.js (3)
73-80:event.returnValue = falseandreturn falseare ineffective on BrowserWindow'scloseevent.
event.preventDefault()on line 74 is the correct and sufficient way to cancel a BrowserWindowcloseevent.event.returnValueis meaningful forwebContents'beforeunloadevents, not BrowserWindow lifecycle events, andreturn falsehas no effect in Node EventEmitter handlers. Both can be removed to avoid misleading future readers.Suggested cleanup
const closeHandler = (event) => { event.preventDefault(); - event.returnValue = false; if (!backgroundWindow.isDestroyed()) { backgroundWindow.hide(); } - return false; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/actions/startBackgroundProcess.js` around lines 73 - 80, The closeHandler currently sets event.returnValue = false and returns false which are ineffective for a BrowserWindow 'close' event; update closeHandler to rely solely on event.preventDefault() to cancel the close and then hide the backgroundWindow if it's not destroyed (keep the existing backgroundWindow.hide() call guarded by backgroundWindow.isDestroyed()); remove the lines setting event.returnValue and the final return false so the handler only calls event.preventDefault() and performs the hide action.
103-113:removeListener/offare not overridden, leaving a gap in close-handler protection.If the intent is to prevent external code from stripping the close handler, note that
removeListener('close', handler)andoff('close', handler)can still remove it. Whether this matters depends on your threat model — if onlyremoveAllListenersis called by the code paths you're guarding against, this is fine. Otherwise, consider wrappingoff/removeListenersimilarly for the'close'event.Also,
originalRemoveAllListenersis already.bind(backgroundWindow)on line 104, so the.call(backgroundWindow, ...)on lines 108 and 112 is redundant — you can calloriginalRemoveAllListeners(eventName)directly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/actions/startBackgroundProcess.js` around lines 103 - 113, The override only protects removeAllListeners; to fully guard the close handler on backgroundWindow, also wrap/remove override removeListener and off so attempts to remove the 'close' handler are ignored or re-attached: intercept backgroundWindow.removeListener and backgroundWindow.off, check if eventName === 'close' (and optionally match handler === closeHandler) then no-op or re-attach closeHandler, otherwise delegate to the original bound functions; also simplify calls to originalRemoveAllListeners by invoking originalRemoveAllListeners(eventName) directly (remove the redundant .call(backgroundWindow, ...)) and keep references to the originals (originalRemoveAllListeners, originalRemoveListener, originalOff) when adding these overrides.
119-120: Consider documenting the distinction between the two global flags.
global.isBackgroundProcessActive(line 119) andglobal.backgroundProcessStarted(line 120) convey subtly different semantics (current state vs. ever-started). A brief inline comment clarifying each flag's purpose would help future maintainers avoid misuse.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/actions/startBackgroundProcess.js` around lines 119 - 120, Add concise inline comments where global.isBackgroundProcessActive and global.backgroundProcessStarted are set (in the startBackgroundProcess flow) explaining that isBackgroundProcessActive denotes the current run-time state (true while the background loop/process is running) and backgroundProcessStarted denotes an "ever-started" flag (true once the process has been initiated at least once), so maintainers know one is transient/current-state and the other is a one-time marker used for startup logic and metrics. Ensure comments are placed immediately above the two assignments and mention intended usage (e.g., toggled on stop vs. never reset) to prevent misuse.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/main/actions/startBackgroundProcess.js`:
- Around line 73-80: The closeHandler currently sets event.returnValue = false
and returns false which are ineffective for a BrowserWindow 'close' event;
update closeHandler to rely solely on event.preventDefault() to cancel the close
and then hide the backgroundWindow if it's not destroyed (keep the existing
backgroundWindow.hide() call guarded by backgroundWindow.isDestroyed()); remove
the lines setting event.returnValue and the final return false so the handler
only calls event.preventDefault() and performs the hide action.
- Around line 103-113: The override only protects removeAllListeners; to fully
guard the close handler on backgroundWindow, also wrap/remove override
removeListener and off so attempts to remove the 'close' handler are ignored or
re-attached: intercept backgroundWindow.removeListener and backgroundWindow.off,
check if eventName === 'close' (and optionally match handler === closeHandler)
then no-op or re-attach closeHandler, otherwise delegate to the original bound
functions; also simplify calls to originalRemoveAllListeners by invoking
originalRemoveAllListeners(eventName) directly (remove the redundant
.call(backgroundWindow, ...)) and keep references to the originals
(originalRemoveAllListeners, originalRemoveListener, originalOff) when adding
these overrides.
- Around line 119-120: Add concise inline comments where
global.isBackgroundProcessActive and global.backgroundProcessStarted are set (in
the startBackgroundProcess flow) explaining that isBackgroundProcessActive
denotes the current run-time state (true while the background loop/process is
running) and backgroundProcessStarted denotes an "ever-started" flag (true once
the process has been initiated at least once), so maintainers know one is
transient/current-state and the other is a one-time marker used for startup
logic and metrics. Ensure comments are placed immediately above the two
assignments and mention intended usage (e.g., toggled on stop vs. never reset)
to prevent misuse.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/main/actions/startBackgroundProcess.js (3)
81-89: Dead statements:event.returnValue = falseandreturn falsehave no effect here.In Electron's BrowserWindow
closeevent, onlyevent.preventDefault()cancels the close.event.returnValueis a DOMbeforeunloadpattern, and Electron ignores handler return values. Both are no-ops that could mislead future readers.Suggested cleanup
const closeHandler = (event) => { event.preventDefault(); - event.returnValue = false; if (!backgroundWindow.isDestroyed()) { backgroundWindow.hide(); } - return false; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/actions/startBackgroundProcess.js` around lines 81 - 89, The closeHandler contains dead statements—remove the no-op lines `event.returnValue = false` and `return false` and rely solely on `event.preventDefault()` to cancel the BrowserWindow close; keep the existing check `if (!backgroundWindow.isDestroyed()) { backgroundWindow.hide(); }` so that closeHandler only calls event.preventDefault() and hides the backgroundWindow when appropriate.
128-129: Two seemingly overlapping global flags:isBackgroundProcessActiveandbackgroundProcessStarted.Both are set to
trueat the same point. If they have different semantics (e.g., one is never reset during the app lifecycle), a brief inline comment clarifying the distinction would help future readers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/actions/startBackgroundProcess.js` around lines 128 - 129, Two globals — isBackgroundProcessActive and backgroundProcessStarted — are set to true together and their semantics are unclear; decide whether they represent distinct states (e.g., "active" vs "everStarted") or are redundant. If distinct, add a concise inline comment at the assignment in startBackgroundProcess (and update related functions such as stopBackgroundProcess/cleanup handlers) clarifying which flag is reset when the process stops and where each is toggled; if redundant, remove one and update all references to use the remaining flag (and update tests/usages accordingly). Ensure any reset logic (e.g., in stopBackgroundProcess, onError handlers, or process exit hooks) updates only the intended flag(s) per your clarified semantics.
112-122:removeListener/offare not similarly guarded — close handler can still be removed individually.The override catches
removeAllListeners('close')andremoveAllListeners(), but a call likebackgroundWindow.off('close', handler)orbackgroundWindow.removeListener('close', handler)will bypass this protection. If the only concern is framework/library code callingremoveAllListeners, this is fine as-is; just noting the gap.Also,
.call(backgroundWindow, ...)onoriginalRemoveAllListeners(which is already.bind(backgroundWindow)) is redundant — the boundthistakes precedence. Consistent with line 97 which only uses.bind().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/actions/startBackgroundProcess.js` around lines 112 - 122, The current override only protects backgroundWindow.removeAllListeners but not backgroundWindow.removeListener or backgroundWindow.off, so individual removal of the 'close' handler can still occur; update the patch to also wrap/override backgroundWindow.removeListener and backgroundWindow.off (or alias off to removeListener if needed) to check for eventName === 'close' (and undefined if applicable), call the original bound function to perform the removal, then immediately re-attach closeHandler (using backgroundWindow.on("close", closeHandler)); also remove the redundant .call(...) when invoking the already-bound originalRemoveAllListeners/originalRemoveListener to keep usage consistent with the earlier .bind() pattern and reference the symbols backgroundWindow.removeAllListeners, backgroundWindow.removeListener, backgroundWindow.off, originalRemoveAllListeners, and closeHandler when applying the changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/main/actions/startBackgroundProcess.js`:
- Around line 81-89: The closeHandler contains dead statements—remove the no-op
lines `event.returnValue = false` and `return false` and rely solely on
`event.preventDefault()` to cancel the BrowserWindow close; keep the existing
check `if (!backgroundWindow.isDestroyed()) { backgroundWindow.hide(); }` so
that closeHandler only calls event.preventDefault() and hides the
backgroundWindow when appropriate.
- Around line 128-129: Two globals — isBackgroundProcessActive and
backgroundProcessStarted — are set to true together and their semantics are
unclear; decide whether they represent distinct states (e.g., "active" vs
"everStarted") or are redundant. If distinct, add a concise inline comment at
the assignment in startBackgroundProcess (and update related functions such as
stopBackgroundProcess/cleanup handlers) clarifying which flag is reset when the
process stops and where each is toggled; if redundant, remove one and update all
references to use the remaining flag (and update tests/usages accordingly).
Ensure any reset logic (e.g., in stopBackgroundProcess, onError handlers, or
process exit hooks) updates only the intended flag(s) per your clarified
semantics.
- Around line 112-122: The current override only protects
backgroundWindow.removeAllListeners but not backgroundWindow.removeListener or
backgroundWindow.off, so individual removal of the 'close' handler can still
occur; update the patch to also wrap/override backgroundWindow.removeListener
and backgroundWindow.off (or alias off to removeListener if needed) to check for
eventName === 'close' (and undefined if applicable), call the original bound
function to perform the removal, then immediately re-attach closeHandler (using
backgroundWindow.on("close", closeHandler)); also remove the redundant
.call(...) when invoking the already-bound
originalRemoveAllListeners/originalRemoveListener to keep usage consistent with
the earlier .bind() pattern and reference the symbols
backgroundWindow.removeAllListeners, backgroundWindow.removeListener,
backgroundWindow.off, originalRemoveAllListeners, and closeHandler when applying
the changes.
| // Override removeAllListeners to prevent removal of close handler | ||
| const originalRemoveAllListeners = backgroundWindow.removeAllListeners.bind(backgroundWindow); | ||
| backgroundWindow.removeAllListeners = (eventName) => { | ||
| if (eventName === 'close' || eventName === undefined) { |
There was a problem hiding this comment.
why eventName === undefined?
src/main/actions/cleanup.js
Outdated
| resolve(); | ||
| }); | ||
|
|
||
| // Timeout fallback in case shutdown-success never comes |
There was a problem hiding this comment.
can we remove comments?
add the functioning of the code part in PR description itself
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 (1)
src/main/actions/cleanup.js (1)
3-13:⚠️ Potential issue | 🟡 Minor
cleanup()may callapp.quit()whilegetReadyToQuitAppstill proceeds to set up IPC listeners.When
backgroundProcessStartedis falsy (orbackgroundWindowis null),cleanup()callsapp.quit()synchronously. However,app.quit()is async (fires events); execution then continues ingetReadyToQuitApppast line 19. If the caller relies on the promise to know when it's safe to quit, theresolve()at line 52 may or may not fire before the quit completes, depending on event ordering.This is probably fine in practice (if
backgroundProcessStartedis false thenbackgroundWindowis also likely null → line 52 resolves immediately), but the coupling betweencleanup()callingapp.quit()andgetReadyToQuitAppreturning a promise is fragile if any future change makes those conditions diverge.🤖 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 3 - 13, cleanup currently calls app.quit() synchronously which races with the promise returned by getReadyToQuitApp; modify cleanup (and its callers) so it does not directly call app.quit() synchronously: make cleanup return a Promise (or accept a callback) that either resolves after sending the "shutdown" IPC or after listening for the app 'will-quit'/'quit' event if no backgroundWindow exists, and update getReadyToQuitApp to await that Promise before resolving; refer to the functions and symbols cleanup, getReadyToQuitApp, global.backgroundProcessStarted, global.backgroundWindow, and app.quit so you locate and change the behavior consistently.
🧹 Nitpick comments (4)
src/main/actions/startBackgroundProcess.js (3)
104-114: Redundant.call(backgroundWindow, …)on an already-bound function.
originalRemoveAllListenersis already.bind(backgroundWindow)on line 104, so the.call(backgroundWindow, ...)on lines 109 and 113 is redundant. Either use.bind()at capture time or.call()at invocation time — not both.♻️ Simplify to just use the bound function
backgroundWindow.removeAllListeners = (eventName) => { if (eventName === 'close' || eventName === undefined) { - originalRemoveAllListeners.call(backgroundWindow, eventName); + originalRemoveAllListeners(eventName); backgroundWindow.on("close", closeHandler); return backgroundWindow; } - return originalRemoveAllListeners.call(backgroundWindow, eventName); + return originalRemoveAllListeners(eventName); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/actions/startBackgroundProcess.js` around lines 104 - 114, The wrapper around backgroundWindow.removeAllListeners binds originalRemoveAllListeners to backgroundWindow but then redundantly calls it with .call(backgroundWindow, ...); update the wrapper to either stop binding at capture time or (preferably) keep the bind and invoke the bound function directly: replace originalRemoveAllListeners.call(backgroundWindow, eventName) with originalRemoveAllListeners(eventName) in both places inside the backgroundWindow.removeAllListeners override, ensuring the closeHandler reattachment logic (backgroundWindow.on("close", closeHandler)) remains unchanged.
67-69: Switchingdom-readyfromoncetoonre-opens DevTools on every navigation.If the background page ever reloads (e.g., due to a crash or HMR in dev),
openDevTools()will fire again. This is probably intentional for dev/debug, but worth confirming — if the user manually closes DevTools, a page reload will force them open again.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/actions/startBackgroundProcess.js` around lines 67 - 69, The change from backgroundWindow.webContents.once('dom-ready', ...) to .on(...) causes openDevTools() to run on every navigation/reload; revert to using backgroundWindow.webContents.once('dom-ready', ...) so DevTools opens only the first time the background page is ready, or alternatively gate the call with a dev-only check (e.g., isDev/process.env.NODE_ENV) and/or a boolean flag (e.g., devtoolsOpened) in startBackgroundProcess to avoid re-opening after manual close; update the handler around backgroundWindow.webContents and the openDevTools call accordingly.
78-89: Remove dead code fromcloseHandler:event.returnValue = falseandreturn falsehave no effect in Electron main-process BrowserWindowcloseevents.Only
event.preventDefault()is needed to prevent the window from closing. Settingevent.returnValueis a renderer-sidebeforeunloadpattern and doesn't apply to main-processclosehandlers. Event handler return values are also ignored by Electron's BrowserWindow.♻️ Proposed cleanup
const closeHandler = (event) => { event.preventDefault(); - event.returnValue = false; if (!backgroundWindow.isDestroyed()) { backgroundWindow.hide(); } - return false; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/actions/startBackgroundProcess.js` around lines 78 - 89, The closeHandler contains dead code: remove the renderer-style lines event.returnValue = false and the trailing return false from the closeHandler function and keep only event.preventDefault() and the existing backgroundWindow.isDestroyed() check/hide logic; update the registration at backgroundWindow.on("close", closeHandler) and the stored reference backgroundWindow._preventCloseHandler as-is (no other behavior changes).src/main/actions/cleanup.js (1)
26-50: Consider extracting the repeated destroy-or-fallback block into a helper.The same 7-line conditional destruction pattern (
_originalDestroycheck →destroyfallback) is duplicated in theshutdown-successhandler and the timeout handler.♻️ Proposed refactor to reduce duplication
+ const destroyBackgroundWindow = () => { + if (global.backgroundWindow && !global.backgroundWindow.isDestroyed()) { + if (global.backgroundWindow._originalDestroy) { + global.backgroundWindow._originalDestroy(); + } else { + global.backgroundWindow.destroy(); + } + } + }; + ipcMain.once("shutdown-success", () => { clearTimeout(timeoutHandle); - if ( - global.backgroundWindow && - !global.backgroundWindow.isDestroyed() - ) { - if (global.backgroundWindow._originalDestroy) { - global.backgroundWindow._originalDestroy(); - } else { - global.backgroundWindow.destroy(); - } - } + destroyBackgroundWindow(); resolve(); }); timeoutHandle = setTimeout(() => { - if (global.backgroundWindow && !global.backgroundWindow.isDestroyed()) { - if (global.backgroundWindow._originalDestroy) { - global.backgroundWindow._originalDestroy(); - } else { - global.backgroundWindow.destroy(); - } - } + destroyBackgroundWindow(); resolve(); }, 2000);🤖 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 26 - 50, The repeated conditional block that destroys the background window should be extracted into a small helper (e.g., destroyBackgroundWindow) and called from both the ipcMain.once("shutdown-success", ...) handler and the timeout callback; implement destroyBackgroundWindow to check global.backgroundWindow and !global.backgroundWindow.isDestroyed(), then call global.backgroundWindow._originalDestroy() if present otherwise global.backgroundWindow.destroy(), and replace the duplicated 7-line blocks in the ipcMain.once handler and the setTimeout callback with a call to that helper (keep timeoutHandle and resolve logic unchanged).
🤖 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 104-114: The override only hardens
backgroundWindow.removeAllListeners, but backgroundWindow.removeListener and
backgroundWindow.off can still remove the closeHandler; patch both by saving
their originals (e.g., originalRemoveListener, originalOff) and wrapping them to
ignore attempts to remove the 'close' event or the specific closeHandler
(backgroundWindow.off/removeListener when eventName === 'close' or handler ===
closeHandler should be no-ops), while delegating other calls to the originals;
ensure you bind originals to backgroundWindow like originalRemoveAllListeners
and preserve return values and chaining behavior.
---
Outside diff comments:
In `@src/main/actions/cleanup.js`:
- Around line 3-13: cleanup currently calls app.quit() synchronously which races
with the promise returned by getReadyToQuitApp; modify cleanup (and its callers)
so it does not directly call app.quit() synchronously: make cleanup return a
Promise (or accept a callback) that either resolves after sending the "shutdown"
IPC or after listening for the app 'will-quit'/'quit' event if no
backgroundWindow exists, and update getReadyToQuitApp to await that Promise
before resolving; refer to the functions and symbols cleanup, getReadyToQuitApp,
global.backgroundProcessStarted, global.backgroundWindow, and app.quit so you
locate and change the behavior consistently.
---
Nitpick comments:
In `@src/main/actions/cleanup.js`:
- Around line 26-50: The repeated conditional block that destroys the background
window should be extracted into a small helper (e.g., destroyBackgroundWindow)
and called from both the ipcMain.once("shutdown-success", ...) handler and the
timeout callback; implement destroyBackgroundWindow to check
global.backgroundWindow and !global.backgroundWindow.isDestroyed(), then call
global.backgroundWindow._originalDestroy() if present otherwise
global.backgroundWindow.destroy(), and replace the duplicated 7-line blocks in
the ipcMain.once handler and the setTimeout callback with a call to that helper
(keep timeoutHandle and resolve logic unchanged).
In `@src/main/actions/startBackgroundProcess.js`:
- Around line 104-114: The wrapper around backgroundWindow.removeAllListeners
binds originalRemoveAllListeners to backgroundWindow but then redundantly calls
it with .call(backgroundWindow, ...); update the wrapper to either stop binding
at capture time or (preferably) keep the bind and invoke the bound function
directly: replace originalRemoveAllListeners.call(backgroundWindow, eventName)
with originalRemoveAllListeners(eventName) in both places inside the
backgroundWindow.removeAllListeners override, ensuring the closeHandler
reattachment logic (backgroundWindow.on("close", closeHandler)) remains
unchanged.
- Around line 67-69: The change from
backgroundWindow.webContents.once('dom-ready', ...) to .on(...) causes
openDevTools() to run on every navigation/reload; revert to using
backgroundWindow.webContents.once('dom-ready', ...) so DevTools opens only the
first time the background page is ready, or alternatively gate the call with a
dev-only check (e.g., isDev/process.env.NODE_ENV) and/or a boolean flag (e.g.,
devtoolsOpened) in startBackgroundProcess to avoid re-opening after manual
close; update the handler around backgroundWindow.webContents and the
openDevTools call accordingly.
- Around line 78-89: The closeHandler contains dead code: remove the
renderer-style lines event.returnValue = false and the trailing return false
from the closeHandler function and keep only event.preventDefault() and the
existing backgroundWindow.isDestroyed() check/hide logic; update the
registration at backgroundWindow.on("close", closeHandler) and the stored
reference backgroundWindow._preventCloseHandler as-is (no other behavior
changes).
2 participants
In startBackgroundProcess.js, the background window is now shielded from accidental destruction the close event is intercepted to hide instead of close; destroy is overridden so it only destroys when a new quit flag (global.allowBackgroundWindowDestruction) is set; removeAllListeners is wrapped so the close handler can’t be removed; and a backgroundProcessStarted flag is recorded. Initial behavior: the window could be closed/destroyed normally, so accidental teardown was possible.
In cleanup.js, quitting now sets allowBackgroundWindowDestruction, waits for shutdown-success, restores the original destroy when quitting, and adds a 2s fallback timer to force-destroy if the shutdown signal never arrives. Initial behavior: it just listened for shutdown-success and immediately closed/destroyed the window without guarding against missed events.
Summary by CodeRabbit
New Features
Bug Fixes