fix: app crash on clicking close button#7637
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdded coordinated shutdown: each watcher class gained a Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant MainApp as App/Index
participant CloseAll as CloseAllWatchers
participant Collection as collectionWatcher
participant Workspace as workspaceWatcher
participant ApiSpec as apiSpecWatcher
participant DotEnv as dotEnvWatcher
User->>MainApp: trigger quit
MainApp->>CloseAll: invoke closeAllWatchers()
CloseAll->>Collection: collectionWatcher.closeAllWatchers()
CloseAll->>Workspace: workspaceWatcher.closeAllWatchers()
CloseAll->>ApiSpec: apiSpecWatcher.closeAllWatchers()
Workspace->>DotEnv: dotEnvWatcher.closeAll()
Collection->>Collection: iterate watchers & close()
Workspace->>Workspace: iterate watchers & close()
ApiSpec->>ApiSpec: iterate watchers & close()
CloseAll->>MainApp: return (shutdown continues)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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 |
…e, and api spec watcher cleanup on app close method
chirag-bruno
force-pushed
the
fix/app-crash-on-close
branch
from
667cd65 to
90fb7c8
Compare
chirag-bruno
requested review from
bijin-bruno,
helloanoop,
lohit-bruno and
naman-bruno
as code owners
Close all chokidar file watchers (collection, workspace, apiSpec) before
the Node environment is torn down. The native FSEvents watchers run on
their own threads and their cleanup races with FreeEnvironment, causing
an abort when fse_instance_destroy tries to lock a destroyed mutex.
Watchers are closed in both mainWindow.on('close') and app.on('before-quit')
to cover the native close button path and the app.exit() path.
![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
🧹 Nitpick comments (1)
packages/bruno-electron/src/app/collection-watcher.js (1)
962-971: Reset companion watcher state during bulk close.
closeAllWatchers()resetsthis.watchersbut leavesthis.loadingStatesandthis.tempDirectoryMapuntouched. Clearing those too keepsCollectionWatcherinternally consistent after shutdown cleanup.Suggested patch
closeAllWatchers() { for (const [watchPath, watcher] of Object.entries(this.watchers)) { if (watcher) { try { watcher.close(); } catch (err) {} } } this.watchers = {}; + this.loadingStates = {}; + this.tempDirectoryMap = {}; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-electron/src/app/collection-watcher.js` around lines 962 - 971, closeAllWatchers() currently clears this.watchers but leaves companion state in this.loadingStates and this.tempDirectoryMap, causing inconsistent state after shutdown; update closeAllWatchers (the method that iterates over this.watchers and calls watcher.close()) to also clear/reset this.loadingStates and this.tempDirectoryMap (e.g., assign empty objects or appropriate initial values) after closing watchers so the CollectionWatcher instance is fully cleaned up.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/bruno-electron/src/index.js`:
- Around line 390-392: The current window 'close' handler prematurely calls
collectionWatcher.closeAllWatchers(), workspaceWatcher.closeAllWatchers(), and
apiSpecWatcher.closeAllWatchers() even when the user cancels the unsaved-changes
dialog; move those calls out of the window 'close' event and invoke them only in
the confirmed quit path (either inside the
ipcMain.handle('main:complete-quit-flow') handler or in the app 'before-quit'
cleanup) so watchers are only closed when quitting is finalized.
---
Nitpick comments:
In `@packages/bruno-electron/src/app/collection-watcher.js`:
- Around line 962-971: closeAllWatchers() currently clears this.watchers but
leaves companion state in this.loadingStates and this.tempDirectoryMap, causing
inconsistent state after shutdown; update closeAllWatchers (the method that
iterates over this.watchers and calls watcher.close()) to also clear/reset
this.loadingStates and this.tempDirectoryMap (e.g., assign empty objects or
appropriate initial values) after closing watchers so the CollectionWatcher
instance is fully cleaned up.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d79ea9a0-c840-4753-a085-51752354fb54
📒 Files selected for processing (4)
packages/bruno-electron/src/app/apiSpecsWatcher.jspackages/bruno-electron/src/app/collection-watcher.jspackages/bruno-electron/src/app/workspace-watcher.jspackages/bruno-electron/src/index.js
packages/bruno-electron/src/index.js
Outdated
| collectionWatcher.closeAllWatchers(); | ||
| workspaceWatcher.closeAllWatchers(); | ||
| apiSpecWatcher.closeAllWatchers(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Locate quit-flow entry points and handlers.
rg -n -C4 "main:start-quit-flow|start-quit-flow|before-quit|window-all-closed|app\.quit\(" --type=js --type=ts
# 2) Locate renderer/main IPC paths that can cancel or defer quit.
rg -n -C6 "ipc(Main|Renderer)\.(on|handle|send|invoke)\('main:start-quit-flow'|confirm.*quit|cancel.*quit|quit.*confirm|quit.*cancel" --type=js --type=ts
# Expected:
# - If quit can be canceled, move watcher cleanup to the confirmed quit path (or before-quit only).
# - If quit cannot be canceled, current ordering is safe.Repository: usebruno/bruno
Length of output: 10683
🏁 Script executed:
cat -n packages/bruno-app/src/providers/App/ConfirmAppClose/index.jsRepository: usebruno/bruno
Length of output: 1105
🏁 Script executed:
rg -n "main:cancel-quit|cancelQuit|reopen|restart.*watcher|closeAllWatchers.*false|watchCollection.*open" --type=js --type=ts packages/bruno-app packages/bruno-electronRepository: usebruno/bruno
Length of output: 1870
🏁 Script executed:
cat -n packages/bruno-app/src/providers/App/ConfirmAppClose/SaveRequestsModal/index.jsRepository: usebruno/bruno
Length of output: 169
🏁 Script executed:
rg -n "main:complete-quit|destroy|quit|abort" --type=js -A3 -B3 packages/bruno-app/src/providers/App/ConfirmAppCloseRepository: usebruno/bruno
Length of output: 648
🏁 Script executed:
fd -t f "SaveRequestsModal" packages/bruno-app/src/providers/App/ConfirmAppCloseRepository: usebruno/bruno
Length of output: 131
🏁 Script executed:
cat -n packages/bruno-app/src/providers/App/ConfirmAppClose/SaveRequestsModal.jsRepository: usebruno/bruno
Length of output: 11643
🏁 Script executed:
rg -n "completeQuitFlow" --type=js -B5 -A10 packages/bruno-app/src/providers/ReduxStore/slices/appRepository: usebruno/bruno
Length of output: 152
🏁 Script executed:
rg -n "completeQuitFlow|main:complete-quit" --type=js -B3 -A8 packages/bruno-electron/srcRepository: usebruno/bruno
Length of output: 892
🏁 Script executed:
fd -t f "slices" packages/bruno-app/src/providers/ReduxStore | head -20Repository: usebruno/bruno
Length of output: 40
🏁 Script executed:
rg -n "completeQuitFlow" --type=js packages/bruno-app/srcRepository: usebruno/bruno
Length of output: 673
🏁 Script executed:
cat -n packages/bruno-app/src/providers/ReduxStore/slices/app.js | sed -n '280,295p'Repository: usebruno/bruno
Length of output: 681
🏁 Script executed:
rg -n "main:complete-quit-flow|mainWindow.destroy" --type=js -B2 -A2 packages/bruno-electron/src/index.jsRepository: usebruno/bruno
Length of output: 40
🏁 Script executed:
rg -n "closeAllWatchers|openAllWatchers|watchCollection|watchFilesystem" --type=js packages/bruno-electron/src | grep -E "(open|restart|resume|enable)" | head -20Repository: usebruno/bruno
Length of output: 40
🏁 Script executed:
cat -n packages/bruno-electron/src/index.js | sed -n '385,410p'Repository: usebruno/bruno
Length of output: 990
Move watcher cleanup to the confirmed quit path to avoid breaking file watching if the user cancels the close dialog.
Watchers are closed at lines 390–392 when the window-close event fires, but this event is cancelable via the user's response to the unsaved-changes dialog. If the user clicks "Cancel", the watchers remain closed for the remainder of the session. Move the closeAllWatchers() calls to ipcMain.handle('main:complete-quit-flow') or rely solely on the before-quit event cleanup instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bruno-electron/src/index.js` around lines 390 - 392, The current
window 'close' handler prematurely calls collectionWatcher.closeAllWatchers(),
workspaceWatcher.closeAllWatchers(), and apiSpecWatcher.closeAllWatchers() even
when the user cancels the unsaved-changes dialog; move those calls out of the
window 'close' event and invoke them only in the confirmed quit path (either
inside the ipcMain.handle('main:complete-quit-flow') handler or in the app
'before-quit' cleanup) so watchers are only closed when quitting is finalized.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/bruno-electron/src/index.js (1)
394-399:⚠️ Potential issue | 🔴 CriticalWatchers are closed before the quit flow is confirmed—user cancel leaves file watching broken.
The
closeevent callscloseAllWatchers()at line 396, then emitsmain:start-quit-flowat line 398. Per the IPC flow, the renderer shows a save dialog, and if the user cancels,mainWindow.destroy()is never called. However, watchers are already closed and won't be re-opened, breaking file watching for the remainder of the session.Move
closeAllWatchers()to themain:complete-quit-flowhandler inpackages/bruno-electron/src/ipc/collection.js, or remove it from thecloseevent and rely solely onbefore-quitcleanup.Suggested fix in ipc/collection.js
ipcMain.handle('main:complete-quit-flow', () => { + // Close all watchers before destroying the window + const collectionWatcher = require('../app/collection-watcher'); + const WorkspaceWatcher = require('../app/workspace-watcher'); + const ApiSpecWatcher = require('../app/apiSpecsWatcher'); + collectionWatcher.closeAllWatchers(); + // Note: workspaceWatcher and apiSpecWatcher instances need to be passed or accessed here mainWindow.destroy(); });And remove from the
closeevent:mainWindow.on('close', (e) => { e.preventDefault(); - closeAllWatchers(); terminalManager.cleanup(mainWindow.webContents); ipcMain.emit('main:start-quit-flow'); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-electron/src/index.js` around lines 394 - 399, The close handler on mainWindow currently calls closeAllWatchers() before emitting 'main:start-quit-flow', which shuts watchers even if the renderer cancel aborts quit; remove the closeAllWatchers() call from the mainWindow.on('close', ...) handler and instead invoke closeAllWatchers() inside the 'main:complete-quit-flow' IPC handler (the handler for 'main:complete-quit-flow' in ipc/collection.js), or perform watcher cleanup in the existing before-quit logic—update references to closeAllWatchers(), terminalManager.cleanup(...) and ipcMain.emit('main:start-quit-flow') so only the quit-confirmation flow triggers watcher shutdown and not the initial close event.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/bruno-electron/src/index.js`:
- Around line 394-399: The close handler on mainWindow currently calls
closeAllWatchers() before emitting 'main:start-quit-flow', which shuts watchers
even if the renderer cancel aborts quit; remove the closeAllWatchers() call from
the mainWindow.on('close', ...) handler and instead invoke closeAllWatchers()
inside the 'main:complete-quit-flow' IPC handler (the handler for
'main:complete-quit-flow' in ipc/collection.js), or perform watcher cleanup in
the existing before-quit logic—update references to closeAllWatchers(),
terminalManager.cleanup(...) and ipcMain.emit('main:start-quit-flow') so only
the quit-confirmation flow triggers watcher shutdown and not the initial close
event.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 41fa87d7-33dc-4630-8c12-6cdd7b337d57
📒 Files selected for processing (4)
packages/bruno-electron/src/app/apiSpecsWatcher.jspackages/bruno-electron/src/app/collection-watcher.jspackages/bruno-electron/src/app/workspace-watcher.jspackages/bruno-electron/src/index.js
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/bruno-electron/src/app/apiSpecsWatcher.js
The close event is cancelable — if the user cancels the unsaved changes dialog, watchers would remain closed for the rest of the session. Move closeAllWatchers() to before-quit which only fires on actual quit.
3 participants
Description
Added collection, workspace, and api spec watcher cleanup on app close method
JIRA: https://usebruno.atlassian.net/browse/BRU-3000
Issue: #3675
Contribution Checklist:
Note: Keeping the PR small and focused helps make it easier to review and merge. If you have multiple changes you want to make, please consider submitting them as separate pull requests.
Publishing to New Package Managers
Please see here for more information.
Summary by CodeRabbit