[ONCALL-187] logs: debug logs for no-proxy cx issue #245
Conversation
WalkthroughAdds debug logging across renderer and main proxy startup flows and changes background window initialization in startBackgroundProcess: initialization is wrapped in a try/catch, creation is short-circuited if a background window already exists (resolving early), creation now stores the window in state and enables webContents, proceeds to load the URL and sets up IPC forwarding, and resolves on did-finish-load. Errors during the flow are caught, logged, and cause the function to resolve false. Other edits are formatting and additional console diagnostics. Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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
🧹 Nitpick comments (2)
src/renderer/actions/proxy/startProxyServer.ts (2)
56-56: Use the logger utility for consistency.The codebase uses the
loggerutility (imported on line 13 and used on line 84), but this debug log usesconsole.log. Consider usinglogger.log()for consistency with the existing logging pattern throughout this file.Apply this diff:
- console.log("DEBUG:", "Proxy assigned to window object"); + logger.log("DEBUG: Proxy assigned to window object");
59-73: Use the logger utility consistently.Similar to the previous comment, these debug logs use
console.loginstead of the importedloggerutility. For consistency and better log management, uselogger.log()throughout.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/actions/startBackgroundProcess.js(1 hunks)src/renderer/actions/initEventHandlers.js(2 hunks)src/renderer/actions/proxy/startProxyServer.ts(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/renderer/actions/proxy/startProxyServer.ts (3)
src/renderer/actions/helperSocketServer.ts (2)
startHelperSocketServer(107-139)stopHelperSocketServer(97-104)src/renderer/actions/shutdown.js (1)
addShutdownHandler(8-10)src/renderer/actions/startHelperServer.js (2)
stopHelperServer(44-49)stopHelperServer(44-49)
🔇 Additional comments (6)
src/renderer/actions/proxy/startProxyServer.ts (1)
144-148: LGTM: Syntax fix for shutdown handler.The addition of the closing
});properly completes theaddShutdownHandlercallback. This appears to fix a syntax error from the previous code.src/renderer/actions/initEventHandlers.js (2)
30-38: LGTM: Debug logs improve startup traceability.The debug logs at initialization and after proxy server startup will help trace the IPC event handler lifecycle and correlate with proxy startup events logged in
startProxyServer.ts.
40-62: LGTM: Debug logs aid app detection tracing.The debug logs before and after processing the app detection will help trace which apps are being detected and their activation status. The empty lines after the logs improve readability.
src/main/actions/startBackgroundProcess.js (3)
27-27: LGTM: Debug log traces background process startup.The debug log at the entry point helps track when the background process initialization begins.
59-82: LGTM: Proper error handling added.Wrapping the initialization flow in a try/catch block and waiting for
did-finish-loadbefore resolving improves error handling and ensures the background process is fully loaded before proceeding. The DevTools logic is preserved correctly.
83-89: LGTM: Error handling with graceful failure.The catch block properly logs errors and resolves with
falseto indicate failure, maintaining backward compatibility for callers that check the boolean result.
| } else { | ||
| logger.log( | ||
| "startBackgroundProcess: A background windows already exists. Cancelling." | ||
| ); | ||
|
|
||
| global.backgroundWindow = backgroundWindow; | ||
| resolve(true); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the file and examine its size
wc -l src/main/actions/startBackgroundProcess.jsRepository: requestly/requestly-desktop-app
Length of output: 120
🏁 Script executed:
# Search for calls to startBackgroundProcess
rg -nP -C5 'startBackgroundProcess\s*\(' --type=jsRepository: requestly/requestly-desktop-app
Length of output: 57
🏁 Script executed:
# Read the entire startBackgroundProcess.js file
cat -n src/main/actions/startBackgroundProcess.jsRepository: requestly/requestly-desktop-app
Length of output: 3609
🏁 Script executed:
# Search for calls to startBackgroundProcess
rg -nP 'startBackgroundProcess' --type=jsRepository: requestly/requestly-desktop-app
Length of output: 937
🏁 Script executed:
# Search for uses of global.backgroundWindow
rg -nP 'global\.backgroundWindow' --type=jsRepository: requestly/requestly-desktop-app
Length of output: 648
🏁 Script executed:
# Search for uses of global.isBackgroundProcessActive
rg -nP 'global\.isBackgroundProcessActive' --type=jsRepository: requestly/requestly-desktop-app
Length of output: 167
🏁 Script executed:
# Check if there's a git history for this file to understand if this is a recent change
git log --oneline -10 src/main/actions/startBackgroundProcess.jsRepository: requestly/requestly-desktop-app
Length of output: 100
🏁 Script executed:
# Let's also check the exact state - is global.backgroundWindow defined elsewhere?
rg -nP 'global\.backgroundWindow\s*=' --type=jsRepository: requestly/requestly-desktop-app
Length of output: 170
Remove the early return or ensure global.backgroundWindow and initialization state are properly managed.
The early return when backgroundWindow already exists (lines 55-56) is problematic. If this function is called a second time when a window already exists:
global.backgroundWindowis never assigned (line 59 is skipped), leaving it undefined or stale- Code in
initPrimaryStorage.jsandcleanup.jsthat checksif (global.backgroundWindow)will fail silently - IPC forwarding is not re-established (line 75)
- The did-finish-load event listener is not attached, so
resolve(true)is called immediately rather than waiting for the window to finish loading
The log message "Cancelling" suggests this is a guard against multiple simultaneous calls, but the guard is incomplete. Either complete the full initialization sequence even when the window exists, or ensure that global.backgroundWindow and other global state are already set from the previous call before returning early.
🤖 Prompt for AI Agents
In src/main/actions/startBackgroundProcess.js around lines 50-57, the early
return when a background window already exists skips setting or re-validating
global.backgroundWindow, skips re-establishing IPC forwarding and the
did-finish-load listener, and leaves initPrimaryStorage.js/cleanup.js checks
unreliable; remove the premature return and instead either (A) when a window
exists, reassign/validate global.backgroundWindow, reattach IPC forwarding
handlers, and if the window hasn’t finished loading attach the did-finish-load
listener so resolve(true) is only called after load, or (B) explicitly ensure
all global state (global.backgroundWindow and any initialization flags) were set
previously and still valid before returning; implement one of these flows so the
function always leaves global state and event/IPC wiring consistent and resolves
only after load completion.
| // fix-me: this does not remove existing handlers when restarting. | ||
| // For now that doesn't have side effects | ||
| addShutdownHandler(() => { | ||
| stopProxyServer(); | ||
| stopHelperServer(); | ||
| stopHelperSocketServer(); | ||
| }) | ||
| }); |
There was a problem hiding this comment.
Potential memory leak from accumulating shutdown handlers.
The comment on line 142 correctly identifies that addShutdownHandler does not remove existing handlers when the proxy server is restarted. Each call to startProxyServer will add new handlers, potentially causing multiple identical handlers to execute on shutdown and accumulating memory over time.
Consider one of these approaches:
- Track and remove previous handlers before adding new ones
- Use a singleton pattern to ensure handlers are registered only once
- Clear all handlers before registration
Would you like me to generate a solution that prevents handler accumulation?
🤖 Prompt for AI Agents
In src/renderer/actions/proxy/startProxyServer.ts around lines 142 to 148, add
logic to prevent accumulating shutdown handlers when startProxyServer is called
multiple times: either store the handler ID returned by addShutdownHandler and
remove the previous handler before adding a new one, or track a module-level
boolean/singleton to register the shutdown handlers only once; ensure
stopProxyServer/stopHelperServer/stopHelperSocketServer are still invoked on
shutdown and that any previous handler references are cleared or replaced to
avoid memory leaks.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/renderer/actions/proxy/startProxyServer.ts (1)
144-150: Unresolved: Shutdown handlers accumulate on each restart.The past review comment correctly identified that
addShutdownHandlerdoes not remove existing handlers whenstartProxyServeris called multiple times. Each restart will register duplicate handlers, leading to:
- Memory accumulation as handlers pile up
- Multiple executions of
stopProxyServer(),stopHelperServer(), andstopHelperSocketServer()on shutdownBased on the relevant code snippet from
src/renderer/actions/shutdown.js,addShutdownHandlersimply pushes handlers to an array without deduplication.Consider implementing one of these solutions:
- Return a cleanup function from
addShutdownHandlerand call it before adding a new handler- Use a module-level flag to ensure handlers are registered only once
- Clear existing handlers before registration
🧹 Nitpick comments (2)
src/renderer/actions/proxy/startProxyServer.ts (2)
59-73: Consider usingconsole.debug()and improving comment clarity.The debug logs effectively trace the proxy state during startup and refresh. However, consider:
- Using
console.debug()instead ofconsole.log("DEBUG:", ...)for idiomatic debug logging- Clarifying the comments on lines 60-61 to: "window.proxy is undefined on initial startup but will be defined on refresh or restart"
Apply this diff to use
console.debug():- console.log( - "DEBUG:", - `onStartup window.proxy is undefined, else not: ${window.proxy}` - ); + console.debug( + `onStartup window.proxy is undefined, else not: ${window.proxy}` + ); if (window.proxy) { - console.log( - "DEBUG:", - `onRefresh window.proxy will be there: ${window.proxy}` - ); + console.debug( + `onRefresh window.proxy will be there: ${window.proxy}` + ); window.proxy.close(); }
115-132: Clarify the comment on line 116.The debug logs effectively trace helper server startup. However, the comment on line 116 is unclear: "No proxy comes for the case when there is success is false in result".
Consider revising to: "Helper server startup is skipped when result.success is false" or "Helper server requires successful proxy startup".
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/renderer/actions/proxy/startProxyServer.ts(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/renderer/actions/proxy/startProxyServer.ts (3)
src/renderer/actions/helperSocketServer.ts (2)
startHelperSocketServer(107-139)stopHelperSocketServer(97-104)src/renderer/actions/shutdown.js (1)
addShutdownHandler(8-10)src/renderer/actions/startHelperServer.js (2)
stopHelperServer(44-49)stopHelperServer(44-49)
🔇 Additional comments (6)
src/renderer/actions/proxy/startProxyServer.ts (6)
16-19: LGTM! Import formatting improved.The multi-line import format enhances readability.
53-56: Debug log added for proxy assignment tracking.The log statement helps trace when the proxy is assigned to the window object, which aligns with the PR's debugging objectives.
80-88: Debug log added for tracing stopProxyServer call.The log helps trace the proxy shutdown during server restart, which is valuable for debugging the no-proxy connection issue mentioned in the PR.
92-93: Debug log for proxy configuration values.Logging the proxy IP and target port provides essential diagnostic information for connection issues.
100-107: Port availability logging added.The debug logs effectively trace port availability checks, which is crucial for diagnosing proxy startup failures.
134-142: Socket server port logging added.The debug log provides visibility into socket server port allocation, completing the diagnostic coverage of the proxy startup flow.
Summary by CodeRabbit
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.