Revert "[ONCALL-125] chore: stop api client request to be proxied" #228
Conversation
This reverts commit 9ca7b67.
|
Issue reopened: ONCALL-125 Remove proxy from intercepting API Client requests |
WalkthroughThe PR refactors Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Possibly related PRs
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: 1
🧹 Nitpick comments (1)
src/main/actions/getProxiedAxios.ts (1)
65-67: Consider improving error visibility.The try-catch block logs errors but otherwise silently continues. If proxy setup fails, the application will fall back to the default axios instance (line 80-81), which might not be obvious to operators or developers debugging proxy-related issues.
Consider adding more prominent error handling, such as throwing the error in development mode or emitting a warning that's more visible than a console.error.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/actions/getProxiedAxios.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/main/actions/getProxiedAxios.ts (1)
src/main/actions/makeApiClientRequest.js (1)
axios(70-70)
🔇 Additional comments (2)
src/main/actions/getProxiedAxios.ts (2)
63-64: LGTM!The function calls correctly match the updated signature. The distinction between the two instances (with and without session cookies) is properly maintained via the
addStoredCookiesparameter.
33-36: Signature change verified — all call sites correctly updated.The verification confirms that
createAxiosInstanceis only called within the same file at lines 63 and 64, and both call sites correctly use the new signature. There are no other callers in the codebase, so the removal of theenableRQProxyparameter poses no compatibility risk.
| const instance = axios.create({ | ||
| proxy: false, | ||
| httpAgent: new HttpsProxyAgent(`http://${config.ip}:${config.port}`), | ||
| httpsAgent: new PatchedHttpsProxyAgent({ | ||
| host: config.ip, | ||
| port: config.port, | ||
| ca: readFileSync(config.rootCertPath), | ||
| }), | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
Verify HttpsProxyAgent usage for HTTP traffic.
Line 39 uses HttpsProxyAgent for the httpAgent property, which handles HTTP requests. While HttpsProxyAgent might support HTTP, this configuration seems unusual. Typically, HTTP and HTTPS agents are different types.
Verify that HttpsProxyAgent correctly handles plain HTTP requests, or consider whether a dedicated HTTP proxy agent should be used instead.
🌐 Web query:
Does HttpsProxyAgent from https-proxy-agent support HTTP requests or only HTTPS?
💡 Result:
HttpsProxyAgent is an http.Agent that issues an HTTP CONNECT to the proxy and is intended for proxying HTTPS (and other CONNECT-using protocols like WebSocket). It’s designed for use with Node's https module — for plain HTTP requests you should use HttpProxyAgent (or proxy-agent which selects the right agent). [1][2]
Sources:
[1] https-proxy-agent README (npm).
[2] proxy-agent README / mapping (Skypack).
Replace HttpsProxyAgent with HttpProxyAgent for HTTP traffic at line 39.
HttpsProxyAgent is designed for proxying HTTPS (and other CONNECT-using protocols like WebSocket). For plain HTTP requests, use HttpProxyAgent instead.
Update line 39 from:
httpAgent: new HttpsProxyAgent(`http://${config.ip}:${config.port}`),
to:
httpAgent: new HttpProxyAgent(`http://${config.ip}:${config.port}`),
Ensure HttpProxyAgent is imported from the https-proxy-agent package.
🤖 Prompt for AI Agents
In src/main/actions/getProxiedAxios.ts around lines 37 to 45, the httpAgent is
incorrectly constructed with HttpsProxyAgent for plain HTTP traffic; replace the
HttpsProxyAgent instantiation at line 39 with HttpProxyAgent (i.e., use new
HttpProxyAgent(`http://${config.ip}:${config.port}`)) and ensure HttpProxyAgent
is imported from the https-proxy-agent package instead of HttpsProxyAgent; keep
the httpsAgent using PatchedHttpsProxyAgent and other config unchanged.
Reverts #214
was causing issues with hitting localhost. Another potential fix has been raised here (with the explanation of the issue)
#227
Summary by CodeRabbit