fix: ensure WebSocket notifications are sent before application quits#4381
fix: ensure WebSocket notifications are sent before application quits#4381cjuriartec wants to merge 2 commits intopear-devs:masterfrom
Conversation
|
|
||
| export const backend = createBackend<BackendType, APIServerConfig>({ | ||
| async start(ctx) { | ||
| const { app } = await import('electron'); |
There was a problem hiding this comment.
Is there a reason you used await import?
There was a problem hiding this comment.
Maybe they were not aware that vite does tree-shaking
📝 WalkthroughWalkthroughThe changes add WebSocket notification functionality to gracefully inform connected clients when the backend server or Electron app is shutting down. A new Changes
Sequence Diagram(s)sequenceDiagram
participant App as Electron App
participant Backend as Backend Server
participant WS as WebSocket Handler
participant Client as Connected Clients
App->>Backend: trigger before-quit event
Backend->>WS: call wsNotifyClose()
WS->>WS: prepare PLAYER_STATE_CHANGED message
WS->>Client: broadcast isPlaying: false, position: lastElapsed
Client->>Client: receive shutdown notification
Client->>Client: disconnect gracefully
Backend->>Backend: close server connection
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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)
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
🤖 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/plugins/api-server/backend/routes/websocket.ts`:
- Around line 63-69: wsNotifyClose currently fires the PLAYER_STATE_CHANGED send
synchronously and returns nothing; change wsNotifyClose to return a Promise that
resolves when send has flushed to all clients (or on a short timeout) by
tracking per-socket send completion (use the same sockets collection and the
send function that sends DataTypes.PlayerStateChanged with { isPlaying:false,
position: lastSongInfo?.elapsedSeconds ?? 0 }) and resolving when all sends
complete; then update the main.ts before-quit handler to call
event.preventDefault(), await wsNotifyClose(), and only then call app.quit() so
shutdown waits for the notification to be sent.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: ab073512-da5d-4d2e-b2e0-7c2273b770e8
📒 Files selected for processing (3)
src/plugins/api-server/backend/main.tssrc/plugins/api-server/backend/routes/index.tssrc/plugins/api-server/backend/routes/websocket.ts
| wsNotifyClose = () => { | ||
| console.log(`[API Server WebSocket] NotifyClose called. Sending isPlaying:false to ${sockets.size} clients.`); | ||
| send(DataTypes.PlayerStateChanged, { | ||
| isPlaying: false, | ||
| position: lastSongInfo?.elapsedSeconds ?? 0, | ||
| }); | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Electron, does the before-quit event wait for asynchronous work to finish, and what is the recommended pattern for delaying quit until a WebSocket notification has been flushed (or timed out)?
💡 Result:
No, the before-quit event does not wait for asynchronous work to finish. The event handler is treated as synchronous; any returned Promise is ignored, and Electron proceeds with quitting regardless. The recommended pattern for delaying quit until a WebSocket notification is flushed (or timed out) is to use a state flag in the main process: 1. Listen for the before-quit event. 2. If cleanup (WebSocket flush) is not complete, call event.preventDefault to cancel the quit. 3. Perform the async WebSocket operation (e.g., send notification and await acknowledgment or use a timeout). 4. Set the flag to indicate completion. 5. Call app.quit manually, which will trigger before-quit again but skip prevention due to the flag. Example code: let hasFlushed = false; async function flushWebSocket { // Send notification over WebSocket // Await ack or use Promise.race with timeout, e.g.: return Promise.race([ websocket.sendAndAwaitAck('notification'), new Promise(resolve => setTimeout(resolve, 5000)) // 5s timeout ]); } app.on('before-quit', (event) => { if (!hasFlushed) { event.preventDefault; flushWebSocket.then( => { hasFlushed = true; app.quit; }).catch( => { hasFlushed = true; app.quit; // Timeout or error: still quit }); } }); This pattern is consistently recommended across sources and works for network operations like WebSocket flushes. Use IPC if the WebSocket is in a renderer process.
Citations:
- 1: https://stackoverflow.com/questions/75168222/how-can-i-wait-for-asynchronous-operations-to-complete-when-an-electron-app-is-c
- 2: https://www.electronjs.org/docs/api/app
- 3: https://stackoverflow.com/questions/59866034/prevent-electron-app-shutdown-until-cleanup-complete
- 4: How to wait to complete asynchronous functions during Windows shutdown? electron/electron#9433
- 5: 'will-quit' event seems to terminate early before the promise resolves electron/electron#27201
🏁 Script executed:
# Find the websocket.ts file
fd -type f -name "websocket.ts"Repository: pear-devs/pear-desktop
Length of output: 236
🏁 Script executed:
# Also check the main.ts file that was mentioned
fd -type f -name "main.ts" | grep -i "api-server\|backend"Repository: pear-devs/pear-desktop
Length of output: 236
🏁 Script executed:
# Find the websocket.ts file with correct syntax
fd -t f "websocket.ts"Repository: pear-devs/pear-desktop
Length of output: 116
🏁 Script executed:
# Find main.ts files in the codebase
fd -t f "main.ts" | head -20Repository: pear-devs/pear-desktop
Length of output: 316
🏁 Script executed:
# Read the websocket.ts file to see the implementation
cat -n "src/plugins/api-server/backend/routes/websocket.ts" | head -180Repository: pear-devs/pear-desktop
Length of output: 5877
🏁 Script executed:
# Read the main.ts file for the api-server to see how wsNotifyClose is used
cat -n "src/plugins/api-server/backend/main.ts"Repository: pear-devs/pear-desktop
Length of output: 7381
Return a Promise from wsNotifyClose so the caller can delay shutdown until the notification is flushed.
wsNotifyClose is synchronous and fire-and-forget. Electron's before-quit event ignores Promises from handlers, but the handler can use event.preventDefault() to cancel quit, await async work, then call app.quit() manually. Without a Promise return value, there's no way for the caller to hold process teardown until the WebSocket notification has been written, so this PR still risks missing the final PLAYER_STATE_CHANGED message under the same race condition.
Make wsNotifyClose return a Promise that resolves when the notification has been sent to all clients (or after a reasonable timeout), then update the before-quit handler in main.ts to prevent default, await completion, and manually call app.quit().
Affects lines 63–69 and line 169.
🧰 Tools
🪛 ESLint
[error] 64-64: Replace [API·Server·WebSocket]·NotifyClose·called.·Sending·isPlaying:false·to·${sockets.size}·clients. with ⏎······[API·Server·WebSocket]·NotifyClose·called.·Sending·isPlaying:false·to·${sockets.size}·clients.,⏎····
(prettier/prettier)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/plugins/api-server/backend/routes/websocket.ts` around lines 63 - 69,
wsNotifyClose currently fires the PLAYER_STATE_CHANGED send synchronously and
returns nothing; change wsNotifyClose to return a Promise that resolves when
send has flushed to all clients (or on a short timeout) by tracking per-socket
send completion (use the same sockets collection and the send function that
sends DataTypes.PlayerStateChanged with { isPlaying:false, position:
lastSongInfo?.elapsedSeconds ?? 0 }) and resolving when all sends complete; then
update the main.ts before-quit handler to call event.preventDefault(), await
wsNotifyClose(), and only then call app.quit() so shutdown waits for the
notification to be sent.
3 participants
Problem
When closing the application (e.g. via Cmd+Q on macOS), the WebSocket clients wouldn't consistently receive a final "isPlaying: false" state because the application was terminating before the network buffers could be flushed.
Solution
before-quitevent listener in the main backend process that triggers this notification.as unknown as WSContext<WebSocket>to prevent TypeScript errors.Testing
Verified with Boring Notch connection. The notification is now consistently received before Pear Desktop fully exits.
Summary by CodeRabbit