[ONCALL-233] : Postman env imports - remove registering duplicate listener #259
Conversation
WalkthroughAdds a module-scoped registry Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 0
🧹 Nitpick comments (1)
src/renderer/lib/RPCServiceOverIPC.ts (1)
26-73: Optional: Consider a method registry pattern for future robustness.While the current fix is effective, you might consider tracking which methods have already been exposed to prevent re-registration entirely:
private exposedMethods = new Set<string>(); protected exposeMethodOverIPC( exposedMethodName: string, method: (..._args: any[]) => Promise<any> ) { const channelName = `${this.RPC_CHANNEL_PREFIX}${exposedMethodName}`; // Skip if already exposed if (this.exposedMethods.has(channelName)) { console.warn(`Method ${exposedMethodName} already exposed, skipping re-registration`); return; } this.exposedMethods.add(channelName); ipcRenderer.on(channelName, async (_event, args) => { // ... rest of implementation }); }This would make it explicit which methods are registered and prevent any accidental re-registration, though the current approach using
removeAllListenersis simpler and works well.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/renderer/lib/RPCServiceOverIPC.ts
🔇 Additional comments (1)
src/renderer/lib/RPCServiceOverIPC.ts (1)
32-36: Effective fix for duplicate listener accumulation.The pre-listener cleanup correctly prevents duplicate listeners from accumulating during hot reloads or repeated initialization. The
removeAllListenerscall ensures exactly one active listener per channel, which is the correct behavior for this request-response pattern.No channel name collisions exist with other code. RPC services use the
${serviceName}-${methodName}pattern (e.g., "FsManagerRPCService-readDir"), while all other listeners in the codebase use completely distinct hardcoded channel names like "start-proxy-server", "save-root-cert", etc. The fix is safe and can be approved.
|
|
||
| // CRITICAL FIX: Remove all existing listeners before adding a new one | ||
| // This prevents duplicate listeners from hot reloads, multiple builds, etc. | ||
| ipcRenderer.removeAllListeners(channelName); |
There was a problem hiding this comment.
Would it make sense to rather make this idempotent? Because we are removing things, in-flight "requests" or "messages" will be dropped.
Rather, we can return the same instance of channel whenever someone asks for a new one but it exists already. That way, in-flight messages are not impacted and the creator hasn't started sending any messages yet so it doesn't care which instance it gets.
cc: @nsrCodes
There was a problem hiding this comment.
Yeah, removeAllListeners seems destructive for requests in execution, better to keep a track of registered channels and use them when required
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/renderer/lib/RPCServiceOverIPC.ts (1)
28-28: Consider removing debug logging.This
console.logwith "DBG-1" prefix appears to be debug output that may clutter the console in production. Consider removing or guarding it behind a debug flag.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/renderer/lib/RPCServiceOverIPC.ts
🔇 Additional comments (2)
src/renderer/lib/RPCServiceOverIPC.ts (2)
12-15: Verify module state persistence during HMR.Module-level variables are typically re-initialized when modules are hot-reloaded, which would reset
REGISTERED_CHANNELSto an empty Set while existingipcRenderer.onlisteners remain attached to the Electron runtime. This could still result in duplicate listeners after HMR.If this fix works in practice, it suggests either:
- The HMR setup preserves module state
- The renderer process restarts on HMR
- The issue being fixed is about re-initialization within the same session (not actual HMR)
Please confirm the expected behavior in your HMR scenario.
37-42: LGTM!The guard pattern is clean - checking the registry before attaching a listener and registering before attachment ensures correct sequencing.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/main/actions/setupIPCForwarding.js (2)
4-5: Consider movingrequestIdCounterto module scope.The counter is function-scoped, so it resets to 0 each time
setupIPCForwardingToBackgroundis called (e.g., ifbackgroundWindowis recreated). WhileDate.now()mitigates collision risk, a module-scoped counter would be more robust.♻️ Suggested refactor
import { ipcMain } from "electron"; +let requestIdCounter = 0; + export const setupIPCForwardingToBackground = (backgroundWindow) => { - let requestIdCounter = 0; - ipcMain.handle(
9-21: Consider adding a timeout to prevent indefinite hanging.If the background process fails to respond, the Promise never resolves and the
ipcMain.oncelistener remains registered. While this isn't a regression (the previous implementation had the same issue), adding a timeout with cleanup would improve resilience.♻️ Optional: Add timeout handling
return new Promise((resolve) => { const { actualPayload, eventName } = incomingData; // Generate unique ID for this specific request to avoid reply channel collision const requestId = `${eventName}-${Date.now()}-${++requestIdCounter}`; + const replyChannel = `reply-${eventName}-${requestId}`; + const timeout = setTimeout(() => { + ipcMain.removeAllListeners(replyChannel); + resolve({ success: false, data: 'Request timed out' }); + }, 30000); // 30 second timeout + // Each request gets its own unique reply channel - ipcMain.once(`reply-${eventName}-${requestId}`, (responseEvent, responsePayload) => { + ipcMain.once(replyChannel, (responseEvent, responsePayload) => { + clearTimeout(timeout); resolve(responsePayload); });src/renderer/lib/RPCServiceOverIPC.ts (1)
74-79: Minor: ExtractreplyChannelto reduce duplication.The reply channel is computed identically in both the success and error paths. Extracting it before the try block would reduce duplication.
♻️ Suggested refactor
+ const replyChannel = requestId ? `reply-${channelName}-${requestId}` : `reply-${channelName}`; + try { const result = await method(...args); - const replyChannel = requestId ? `reply-${channelName}-${requestId}` : `reply-${channelName}`; ipcRenderer.send(replyChannel, { success: true, data: result, }); } catch (error: any) { - - const replyChannel = requestId ? `reply-${channelName}-${requestId}` : `reply-${channelName}`; ipcRenderer.send(replyChannel, { success: false, data: error.message, }); }Also applies to: 86-91
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/actions/setupIPCForwarding.jssrc/renderer/lib/RPCServiceOverIPC.ts
🔇 Additional comments (4)
src/main/actions/setupIPCForwarding.js (1)
26-38: Verify:setupIPCForwardingToWebAppstill uses the old reply channel pattern.The webapp-to-background path now uses per-request reply channels (
reply-${eventName}-${requestId}), but the background-to-webapp path (lines 31-35) still uses the staticreply-${eventName}channel. If concurrent requests to the webapp are possible, this could have the same collision issue that the other direction is fixing.Is this asymmetry intentional?
src/renderer/lib/RPCServiceOverIPC.ts (3)
12-15: LGTM! Good approach to prevent duplicate listeners during HMR.The module-scoped
REGISTERED_CHANNELSSet with early return is an effective pattern to prevent the duplicate listener registration issue mentioned in the PR title.Also applies to: 38-42
44-54: Verify: Ensureargsis always an array before spreading.At line 63,
method(...args)assumesargsis an array. For the new format,actualPayload(which becomesargs) should be an array. For the old format, if the legacy caller sends a non-array payload, spreading will produce unexpected behavior.Consider adding a defensive check:
♻️ Suggested defensive handling
if (payload && typeof payload === 'object' && 'requestId' in payload && 'args' in payload) { requestId = payload.requestId; args = payload.args; } else { args = payload; } + + // Ensure args is always an array for spreading + if (!Array.isArray(args)) { + args = [args]; + }Also applies to: 63-63
44-54: Protocol consistency looks good.The payload format
{ requestId, args }and reply channel namingreply-${channelName}-${requestId}are consistent with the changes insetupIPCForwarding.js.Also applies to: 74-79
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/main/actions/setupIPCForwarding.js (2)
14-32: Inconsistent indentation and formatting.The code block has inconsistent indentation (e.g., line 16 is over-indented, lines 19-21 have different spacing from lines 27-29). Consider normalizing the indentation for better readability.
🔧 Suggested formatting fix
- if(isRPCCall) { - // Generate unique ID for this specific request to avoid reply channel collision - const requestId = `${eventName}-${Date.now()}-${++requestIdCounter}`; - - // Each request gets its own unique reply channel - ipcMain.once(`reply-${eventName}-${requestId}`, (responseEvent, responsePayload) => { - resolve(responsePayload); - }); - - // Send requestId so background can reply on the correct channel - backgroundWindow.webContents.send(eventName, { requestId, args: actualPayload }); - } - else{ + if (isRPCCall) { + // Generate unique ID for this specific request to avoid reply channel collision + const requestId = `${eventName}-${Date.now()}-${++requestIdCounter}`; + + // Each request gets its own unique reply channel + ipcMain.once(`reply-${eventName}-${requestId}`, (responseEvent, responsePayload) => { + resolve(responsePayload); + }); + + // Send requestId so background can reply on the correct channel + backgroundWindow.webContents.send(eventName, { requestId, args: actualPayload }); + } else { ipcMain.once(`reply-${eventName}`, (responseEvent, responsePayload) => { resolve(responsePayload); }); - + backgroundWindow.webContents.send(eventName, actualPayload); - } + }
9-32: Consider adding a timeout to prevent hanging requests.If the background process never responds (e.g., crashes or encounters an unhandled error), the Promise will never resolve and the
ipcMain.oncelistener will remain registered indefinitely. This is a pre-existing pattern in the non-RPC path as well, but worth noting for future improvement.src/renderer/lib/RPCServiceOverIPC.ts (1)
40-40: Typo: "alrady" should be "already".- // if channel is alrady registered, remove old listener first during HMR + // if channel is already registered, remove old listener first during HMR
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/actions/setupIPCForwarding.jssrc/renderer/lib/RPCServiceOverIPC.ts
🔇 Additional comments (4)
src/main/actions/setupIPCForwarding.js (1)
11-16: The per-request correlation approach looks correct.The unique
requestIdcombining event name, timestamp, and counter effectively prevents reply channel collisions. This properly addresses the duplicate listener issue for RPC calls while preserving backward compatibility for non-RPC calls.src/renderer/lib/RPCServiceOverIPC.ts (3)
12-17: HMR cleanup logic is well-implemented.The module-scoped registry correctly tracks handlers and removes old listeners before attaching new ones. This properly addresses the duplicate listener issue mentioned in the PR title.
Also applies to: 39-46
49-68: Payload handling correctly supports both formats.The backward-compatible detection of old vs. new format is well implemented. Note that line 68 assumes
argsis always an array (to be spread). This appears to match the existing contract whereactualPayloadfromsetupIPCForwarding.jsis expected to be an array of arguments.
79-84: Reply channel naming is consistent with the main process.The dynamic reply channel construction correctly matches the format expected by
setupIPCForwarding.js, ensuring proper correlation for RPC calls while maintaining backward compatibility.Also applies to: 91-96
Summary by CodeRabbit
Bug Fixes
New Features
✏️ Tip: You can customize this high-level summary in your review settings.