feat(settings): add global proxy settings with runtime apply#2002
Open
liu-qingyuan wants to merge 8 commits intoAndyMik90:developfrom
Open
feat(settings): add global proxy settings with runtime apply#2002liu-qingyuan wants to merge 8 commits intoAndyMik90:developfrom
liu-qingyuan wants to merge 8 commits intoAndyMik90:developfrom
Conversation
|
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds runtime proxy support: UI, i18n, settings validation/normalization, environment management, proxy-aware HTTP utilities (undici dispatcher), IPC validation/application on save, startup application, and unit/integration tests covering proxy flows. Changes
Sequence DiagramsequenceDiagram
participant User
participant Renderer as Renderer (ProxySettings)
participant Main as Main (IPC)
participant Validator as RuntimeProxy
participant Env as process.env / ProxyAgent
participant Network as undiciFetch
User->>Renderer: configure proxy and click Save
Renderer->>Main: invoke settings:save {proxyEnabled, proxyHttpUrl, proxyHttpsUrl}
Main->>Validator: normalizeRuntimeProxyConfig(input)
alt valid
Validator-->>Main: enabled config (httpProxyUrl, httpsProxyUrl)
Main->>Main: persist settings
Main->>Env: applyRuntimeProxyConfig(input)
Env->>Env: set env keys & create/cache ProxyAgent
Main-->>Renderer: {success: true}
Renderer->>Network: later network request
Network->>Env: getProxyAgentFromEnvironment(target)
Env-->>Network: ProxyAgent(proxyUrl)
Network->>Network: undiciFetch(url, { dispatcher })
else invalid
Validator-->>Main: invalid + errors
Main-->>Renderer: {success: false, error: "Invalid proxy settings."}
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🎉 Thanks for your first PR!
A maintainer will review it soon. Please make sure:
- Your branch is synced with
develop - CI checks pass
- You've followed our contribution guide
Welcome to the Auto Claude community!
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces global network proxy support. It adds a new "Proxy" section to the application settings, allowing users to enable and configure HTTP and HTTPS proxy URLs. The implementation includes a new proxy-fetch utility using undici to handle outbound requests with proxy agents, validation logic for proxy URLs, and integration across various modules like OAuth authentication and usage monitoring. Feedback highlights an unsafe type cast in the fetchWithProxy utility and suggests safer data handling when normalizing OpenAI usage responses.
|
|
||
| if (proxyAgent) { | ||
| // Use undici fetch with proxy dispatcher | ||
| return undiciFetch(input as string, { |
Contributor
There was a problem hiding this comment.
Casting input to string is unsafe because the function signature allows URL and Request objects. undici.fetch supports URL objects, but casting them to string via as string will not work correctly. For Request objects, this cast will result in "[object Request]".
Suggested change
| return undiciFetch(input as string, { | |
| return undiciFetch(input as any, { |
Comment on lines
+2322
to
+2328
| case 'openai': { | ||
| const codexData: CodexUsageResponse = isRecord(rawData) | ||
| ? (rawData as CodexUsageResponse) | ||
| : {}; | ||
| normalizedUsage = normalizeCodexResponse(codexData, profileId, profileName, profileEmail); | ||
| break; | ||
| } |
Contributor
There was a problem hiding this comment.
Defaulting codexData to an empty object {} when rawData is not a record might lead to runtime errors in normalizeCodexResponse. It is safer to only attempt normalization if the data is a valid record.
case 'openai': {
if (isRecord(rawData)) {
normalizedUsage = normalizeCodexResponse(rawData as CodexUsageResponse, profileId, profileName, profileEmail);
}
break;
}![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/desktop/src/main/ipc-handlers/settings-handlers.ts (1)
443-472:⚠️ Potential issue | 🟠 MajorPersist the normalized proxy URLs before writing settings.
normalizeRuntimeProxyConfig()can resolve a missing HTTP/HTTPS side from the other URL, but Line 465 still writes the rawnewSettingsobject. Saving only one proxy URL will therefore leavesettings.jsonand the settings UI with one field blank even though Lines 467-472 apply both env vars at runtime. On a later toggle/save, that blank side can get replaced with the hardcoded default instead of the proxy that is actually active.💡 Suggested fix
if (runtimeProxyConfig.state === 'invalid') { const formattedErrors = runtimeProxyConfig.errors.map((entry) => entry.message).join(' '); return { success: false, error: `Invalid proxy settings. ${formattedErrors}`, }; } + + if (runtimeProxyConfig.state === 'enabled') { + newSettings.proxyHttpUrl = runtimeProxyConfig.httpProxyUrl; + newSettings.proxyHttpsUrl = runtimeProxyConfig.httpsProxyUrl; + } // Sync defaultModel when agent profile changes (`#414`)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/ipc-handlers/settings-handlers.ts` around lines 443 - 472, The normalizeRuntimeProxyConfig result is never persisted: after calling normalizeRuntimeProxyConfig() and validating runtimeProxyConfig, update newSettings.proxyHttpUrl and newSettings.proxyHttpsUrl from the normalized values on runtimeProxyConfig (or the property containing normalized URLs) before calling writeFileSync, so the persisted settings.json matches the runtime proxy that will be applied by applyRuntimeProxyConfig; keep using normalizeRuntimeProxyConfig, runtimeProxyConfig, newSettings, writeFileSync and applyRuntimeProxyConfig to locate where to set those normalized URL fields.apps/desktop/src/main/claude-profile/token-refresh.ts (1)
269-296:⚠️ Potential issue | 🟠 MajorReject refresh responses that omit
refresh_token.
ensureValidToken()andreactiveTokenRefresh()both require a non-emptyrefreshResult.refreshToken. Returningsuccess: truewithrefreshToken: undefinedmakes them drop back to the old credential even though this refresh path has already invalidated it.Suggested fix
// Calculate expiry timestamp // expires_in is in seconds, convert to ms and add to current time const expiresIn = typeof data.expires_in === 'number' ? data.expires_in : 28800; // Default 8 hours if not provided const expiresAt = Date.now() + (expiresIn * 1000); + + if (typeof data.refresh_token !== 'string' || data.refresh_token.length === 0) { + return { + success: false, + error: 'Response missing refresh_token', + errorCode: 'invalid_response' + }; + } if (isDebug) { console.warn('[TokenRefresh] Token refresh successful', { newTokenFingerprint: `${data.access_token.slice(0, 12)}...${data.access_token.slice(-4)}`, expiresIn: expiresIn, @@ return { success: true, accessToken: data.access_token, - refreshToken: typeof data.refresh_token === 'string' ? data.refresh_token : undefined, + refreshToken: data.refresh_token, expiresAt, expiresIn };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/claude-profile/token-refresh.ts` around lines 269 - 296, The token refresh handler should reject responses that omit a non-empty refresh_token because ensureValidToken() and reactiveTokenRefresh() require it; in the parsing/return path (inside the token refresh logic that builds the result returned by the function in token-refresh.ts) change the logic that currently sets refreshToken: typeof data.refresh_token === 'string' ? data.refresh_token : undefined to instead treat missing/empty refresh_token as a failure: return success: false with an appropriate error/errorCode (e.g., 'missing_refresh_token' or 'invalid_response') and include a debug log when isDebug is true; update any success logging (the block using data.access_token slices) to only run when you will return success so you don't log a successful refresh when refresh_token is absent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/main/__tests__/ipc-handlers.test.ts`:
- Around line 568-590: The tests mutate process.env via applyRuntimeProxyConfig
when invoking the "settings:save" handler, causing order-dependent failures; to
fix, snapshot the relevant env keys (e.g., HTTP_PROXY and HTTPS_PROXY and
lowercase variants) in a beforeEach and restore them in an afterEach around the
tests that call setupIpcHandlers/ ipcMain.invokeHandler (referencing the tests
in ipc-handlers.test.ts and the applyRuntimeProxyConfig invocation), ensuring
each test restores original process.env values to avoid cross-test pollution.
In `@apps/desktop/src/main/utils/runtime-proxy-config.ts`:
- Around line 168-175: getProxyUrlFromEnvironment currently always prefers
HTTPS_PROXY, preventing HTTP requests from using HTTP_PROXY when both are set;
change the function to resolve by request scheme instead (e.g., add a parameter
like scheme or isHttps to getProxyUrlFromEnvironment) and implement logic that
when the scheme is "https" it prefers HTTPS_PROXY/https_proxy and falls back to
HTTP_PROXY/http_proxy only if missing, and when the scheme is "http" it prefers
HTTP_PROXY/http_proxy and falls back to HTTPS_PROXY/https_proxy; update any
callers to pass the request scheme (or URL) into getProxyUrlFromEnvironment.
- Line 1: Replace the relative import of the AppSettings type in
runtime-proxy-config.ts with the shared path alias: change the current import
statement that references '../../shared/types/settings' to import AppSettings
from '@shared/types/settings' so that the AppSettings type used in this module
(e.g., in any functions or variables referencing AppSettings) comes from the
configured alias.
- Around line 180-186: getProxyAgentFromEnvironment currently creates a new
undici ProxyAgent each call; change it to cache agents keyed by the normalized
proxy URL (use the same normalization as getProxyUrlFromEnvironment) so repeated
calls return the same ProxyAgent instance, and when the proxy URL changes
replace the cached agent and call the agent's disposal method to free resources.
Locate getProxyAgentFromEnvironment and add a module-level Map<string,
ProxyAgent> (or single cached entry if only one URL expected), ensure lookups
use the normalized URL, return the cached agent when present, and on URL change
close/dispose the old ProxyAgent before creating and storing the new one.
In
`@apps/desktop/src/renderer/components/settings/__tests__/AppSettings.proxy-nav.test.tsx`:
- Around line 97-112: The test seeds mockUseSettingsState.error before rendering
so it never verifies the UI transition after save fails; change the setup in the
'keeps dialog open and shows error when app save fails' test to start with
mockUseSettingsState.error = null, render AppSettingsDialog, then call
mockSaveSettings.mockResolvedValue(false) (or have mockSaveSettings resolve
false before firing the save button) and after firing the Save button wait for
mockSaveSettings to be called and then set/mock the store to populate
mockUseSettingsState.error with 'Proxy URL for HTTP is invalid.' (or simulate
the save failure update) and assert onOpenChange not called, mockCommitTheme not
called, and that the error text appears—use the existing mocks mockSaveSettings,
mockUseSettingsState, AppSettingsDialog, onOpenChange and mockCommitTheme to
drive and verify the failed-save state transition.
In `@apps/desktop/src/renderer/components/settings/ProxySettings.tsx`:
- Around line 31-85: Proxy input validation failures are currently silent:
update ProxySettings (the component rendering the Switch and Inputs for
settings.proxyEnabled, proxyHttpUrl, proxyHttpsUrl and using
handleToggle/onSettingsChange) to validate proxy URLs and surface errors in the
UI so saveSettings() failures aren't a silent no-op. Add component state for
validation errors, validate settings.proxyHttpUrl and settings.proxyHttpsUrl on
change and on toggle (or before invoking save), show inline error text and input
error styling beneath each Input when invalid, and disable or block the save
action until inputs are valid; also ensure any error returned by saveSettings is
caught and displayed as a top-level form error so users know which field failed.
- Around line 1-6: The new imports use relative paths; switch them to the
tsconfig path aliases: replace '../ui/label', '../ui/input', '../ui/switch', and
'./SettingsSection' with the renderer alias (use the '@/...' alias for
renderer-local modules) and replace '../../../shared/types' with the shared
alias '@shared/types' so the imports for Label, Input, Switch, SettingsSection
use '@/...' and AppSettings uses '@shared/types'.
---
Outside diff comments:
In `@apps/desktop/src/main/claude-profile/token-refresh.ts`:
- Around line 269-296: The token refresh handler should reject responses that
omit a non-empty refresh_token because ensureValidToken() and
reactiveTokenRefresh() require it; in the parsing/return path (inside the token
refresh logic that builds the result returned by the function in
token-refresh.ts) change the logic that currently sets refreshToken: typeof
data.refresh_token === 'string' ? data.refresh_token : undefined to instead
treat missing/empty refresh_token as a failure: return success: false with an
appropriate error/errorCode (e.g., 'missing_refresh_token' or
'invalid_response') and include a debug log when isDebug is true; update any
success logging (the block using data.access_token slices) to only run when you
will return success so you don't log a successful refresh when refresh_token is
absent.
In `@apps/desktop/src/main/ipc-handlers/settings-handlers.ts`:
- Around line 443-472: The normalizeRuntimeProxyConfig result is never
persisted: after calling normalizeRuntimeProxyConfig() and validating
runtimeProxyConfig, update newSettings.proxyHttpUrl and
newSettings.proxyHttpsUrl from the normalized values on runtimeProxyConfig (or
the property containing normalized URLs) before calling writeFileSync, so the
persisted settings.json matches the runtime proxy that will be applied by
applyRuntimeProxyConfig; keep using normalizeRuntimeProxyConfig,
runtimeProxyConfig, newSettings, writeFileSync and applyRuntimeProxyConfig to
locate where to set those normalized URL fields.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 3c5e17db-a499-4f6b-940f-50d6d5249da8
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json,!**/package-lock.json
📒 Files selected for processing (22)
apps/desktop/package.jsonapps/desktop/src/main/__tests__/ipc-handlers.test.tsapps/desktop/src/main/ai/auth/codex-oauth.tsapps/desktop/src/main/ai/providers/oauth-fetch.tsapps/desktop/src/main/claude-profile/codex-usage-fetcher.tsapps/desktop/src/main/claude-profile/token-refresh.tsapps/desktop/src/main/claude-profile/usage-monitor.tsapps/desktop/src/main/index.tsapps/desktop/src/main/ipc-handlers/__tests__/settings-proxy-runtime.integration.test.tsapps/desktop/src/main/ipc-handlers/settings-handlers.tsapps/desktop/src/main/utils/proxy-fetch.tsapps/desktop/src/main/utils/runtime-proxy-config.tsapps/desktop/src/renderer/components/settings/AppSettings.tsxapps/desktop/src/renderer/components/settings/ProxySettings.tsxapps/desktop/src/renderer/components/settings/__tests__/AppSettings.proxy-nav.test.tsxapps/desktop/src/renderer/components/settings/__tests__/ProxySettings.test.tsxapps/desktop/src/renderer/components/settings/index.tsapps/desktop/src/renderer/stores/settings-store.tsapps/desktop/src/shared/constants/config.tsapps/desktop/src/shared/i18n/locales/en/settings.jsonapps/desktop/src/shared/i18n/locales/fr/settings.jsonapps/desktop/src/shared/types/settings.ts
apps/desktop/src/renderer/components/settings/__tests__/AppSettings.proxy-nav.test.tsx
Outdated
Show resolved
Hide resolved
Comment on lines
+1
to
+6
| import { useTranslation } from 'react-i18next'; | ||
| import { Label } from '../ui/label'; | ||
| import { Input } from '../ui/input'; | ||
| import { Switch } from '../ui/switch'; | ||
| import { SettingsSection } from './SettingsSection'; | ||
| import type { AppSettings } from '../../../shared/types'; |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use the configured aliases for these new imports.
Please switch these new renderer/shared imports to the tsconfig aliases instead of adding more ../ chains. As per coding guidelines, "Use path aliases defined in tsconfig.json for imports: @/* (renderer), @shared/* (shared), @preload/*, @features/*, @components/*, @hooks/*, @lib/*."
🧰 Tools
🪛 GitHub Actions: E2E
[error] CI step failed: npm run test:e2e (npx playwright test --config=e2e/playwright.config.ts) exited with code 1 due to the failing e2e test.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/renderer/components/settings/ProxySettings.tsx` around lines
1 - 6, The new imports use relative paths; switch them to the tsconfig path
aliases: replace '../ui/label', '../ui/input', '../ui/switch', and
'./SettingsSection' with the renderer alias (use the '@/...' alias for
renderer-local modules) and replace '../../../shared/types' with the shared
alias '@shared/types' so the imports for Label, Input, Switch, SettingsSection
use '@/...' and AppSettings uses '@shared/types'.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/main/claude-profile/usage-monitor.ts`:
- Around line 46-48: The isRecord type guard currently treats arrays as objects;
change its predicate so it returns true only for plain objects by excluding
arrays (e.g., add a check like !Array.isArray(value) or use
Object.prototype.toString or value.constructor === Object). Update the isRecord
function (symbol: isRecord) to include this array-exclusion check so callers
like any code accessing json.data or Object.keys(json) won’t mis-handle
top-level arrays.
In `@apps/desktop/src/main/utils/proxy-fetch.ts`:
- Around line 44-52: The Request->RequestInit extraction in requestDerivedInit
omits standard RequestInit fields, which can drop important settings when a
Request is passed; update the requestDerivedInit construction (the ternary
branch when input instanceof Request) to include cache, credentials, integrity,
keepalive, mode, referrer, and referrerPolicy in addition to the existing
method, headers, body, redirect, and signal so the derived RequestInit mirrors
the original Request object's settings.
- Around line 57-62: The double type cast around the undici call (involving
undiciFetch, undiciInput, requestDerivedInit, init, and proxyAgent) hides type
mismatches; add a concise inline comment above that return explaining that
undici's Dispatcher/fetch types differ from the global Fetch API and that the
casts are intentional to bridge those type definitions (e.g., "undici's fetch
types differ from global fetch; casting required to satisfy Response type until
types are aligned") so future maintainers understand why as any / as unknown as
Response are used and can find this spot when improving typings.
- Line 41: Currently getProxyAgentFromEnvironment() is called on every fetch
which constructs a new ProxyAgent each time; change this by memoizing the agent:
store the lastProxyUrl and lastAgent in module-level variables and only call
getProxyAgentFromEnvironment() / construct a new ProxyAgent when
getProxyUrlFromEnvironment() returns a different value (import
getProxyUrlFromEnvironment and ProxyAgent from runtime-proxy-config.ts); update
proxy-fetch.ts to reuse lastAgent for subsequent fetch calls and refresh the
cached agent when the environment proxy URL changes.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 1d60d404-204d-4355-bdae-d23668009f62
📒 Files selected for processing (2)
apps/desktop/src/main/claude-profile/usage-monitor.tsapps/desktop/src/main/utils/proxy-fetch.ts
| return fetch(input, init); | ||
| } | ||
|
|
||
| const proxyAgent = getProxyAgentFromEnvironment(); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
New ProxyAgent created on every fetch call — potential performance concern.
Based on runtime-proxy-config.ts:186, getProxyAgentFromEnvironment() creates a new ProxyAgent instance on each invocation without caching. For high-frequency fetch scenarios, this could introduce unnecessary allocation overhead. Consider memoizing the agent when environment variables haven't changed.
♻️ Suggested memoization approach
+let cachedProxyAgent: ProxyAgent | undefined;
+let cachedProxyUrl: string | undefined;
+
+function getOrCreateProxyAgent(): ProxyAgent | undefined {
+ const currentUrl = getProxyUrlFromEnvironment();
+ if (currentUrl === cachedProxyUrl && cachedProxyAgent) {
+ return cachedProxyAgent;
+ }
+ cachedProxyUrl = currentUrl;
+ cachedProxyAgent = currentUrl ? new ProxyAgent(currentUrl) : undefined;
+ return cachedProxyAgent;
+}
+
export async function fetchWithProxy(
input: string | URL | Request,
init?: RequestInit
): Promise<Response> {
if (isMockedFetch(fetch)) {
return fetch(input, init);
}
- const proxyAgent = getProxyAgentFromEnvironment();
+ const proxyAgent = getOrCreateProxyAgent();Note: You'd need to import getProxyUrlFromEnvironment and ProxyAgent from runtime-proxy-config.ts.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/main/utils/proxy-fetch.ts` at line 41, Currently
getProxyAgentFromEnvironment() is called on every fetch which constructs a new
ProxyAgent each time; change this by memoizing the agent: store the lastProxyUrl
and lastAgent in module-level variables and only call
getProxyAgentFromEnvironment() / construct a new ProxyAgent when
getProxyUrlFromEnvironment() returns a different value (import
getProxyUrlFromEnvironment and ProxyAgent from runtime-proxy-config.ts); update
proxy-fetch.ts to reuse lastAgent for subsequent fetch calls and refresh the
cached agent when the environment proxy URL changes.
Comment on lines
+57
to
+62
| // Use undici fetch with proxy dispatcher | ||
| return undiciFetch(undiciInput, { | ||
| ...requestDerivedInit, | ||
| ...init, | ||
| dispatcher: proxyAgent, | ||
| } as any) as unknown as Response; |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Type casts (as any, as unknown as Response) obscure type safety.
The double cast is necessary because undici's types differ from the global fetch types, but this pattern hides potential incompatibilities. Consider adding a brief inline comment explaining why these casts are required for future maintainers.
// Use undici fetch with proxy dispatcher
+ // Note: undici's RequestInit includes 'dispatcher' which standard RequestInit lacks,
+ // and undici's Response is compatible with but not identical to global Response
return undiciFetch(undiciInput, {
...requestDerivedInit,
...init,
dispatcher: proxyAgent,
} as any) as unknown as Response;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Use undici fetch with proxy dispatcher | |
| return undiciFetch(undiciInput, { | |
| ...requestDerivedInit, | |
| ...init, | |
| dispatcher: proxyAgent, | |
| } as any) as unknown as Response; | |
| // Use undici fetch with proxy dispatcher | |
| // Note: undici's RequestInit includes 'dispatcher' which standard RequestInit lacks, | |
| // and undici's Response is compatible with but not identical to global Response | |
| return undiciFetch(undiciInput, { | |
| ...requestDerivedInit, | |
| ...init, | |
| dispatcher: proxyAgent, | |
| } as any) as unknown as Response; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/main/utils/proxy-fetch.ts` around lines 57 - 62, The double
type cast around the undici call (involving undiciFetch, undiciInput,
requestDerivedInit, init, and proxyAgent) hides type mismatches; add a concise
inline comment above that return explaining that undici's Dispatcher/fetch types
differ from the global Fetch API and that the casts are intentional to bridge
those type definitions (e.g., "undici's fetch types differ from global fetch;
casting required to satisfy Response type until types are aligned") so future
maintainers understand why as any / as unknown as Response are used and can find
this spot when improving typings.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
Comment on lines
+24
to
+32
| function pruneStaleProxyAgents(activeProxyUrls: Set<string>): void { | ||
| for (const [url, agent] of proxyAgentCache.entries()) { | ||
| if (activeProxyUrls.has(url)) { | ||
| continue; | ||
| } | ||
| tryDisposeAgent(agent); | ||
| proxyAgentCache.delete(url); | ||
| } | ||
| } |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/main/ai/providers/oauth-fetch.ts (1)
251-280:⚠️ Potential issue | 🟠 MajorExtract and preserve all Request properties when forwarding through
undiciFetch.This wrapper is typed as
typeof globalThis.fetch, but wheninput instanceof Request, it only extracts the URL and discards the method, body, signal, and other properties unless the caller also passes them ininit. This causes silent corruption: a POST request becomes GET, a request with a signal loses its abort control, and streaming bodies are dropped. The same codebase already handles this correctly inapps/desktop/src/main/utils/proxy-fetch.ts(lines 50–76); apply the same pattern here.🛠️ Suggested fix
return async (input: RequestInfo | URL, init?: RequestInit): Promise<Response> => { + const requestDerivedInit: RequestInit = + input instanceof Request + ? { + method: input.method, + headers: input.headers, + body: input.body ?? undefined, + cache: input.cache, + credentials: input.credentials, + integrity: input.integrity, + keepalive: input.keepalive, + mode: input.mode, + referrer: input.referrer, + referrerPolicy: input.referrerPolicy, + redirect: input.redirect, + signal: input.signal, + } + : {}; + // 1. Get valid OAuth token (auto-refresh if needed) const token = await ensureValidOAuthToken(tokenFilePath, provider); if (!token) { throw new Error('OAuth: No valid token available. Please re-authenticate.'); } // 2. Build headers — strip dummy Authorization, inject real token - const headers = new Headers(init?.headers); + const headers = new Headers(requestDerivedInit.headers ?? init?.headers); headers.delete('authorization'); headers.delete('Authorization'); headers.set('Authorization', `Bearer ${token}`); // ... URL resolution ... - const finalInit = { ...init, headers, dispatcher: proxyAgent } as any; + const finalInit = { ...requestDerivedInit, ...init, headers, dispatcher: proxyAgent } as any;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/ai/providers/oauth-fetch.ts` around lines 251 - 280, When input is a Request the wrapper currently only uses input.url and drops other request properties, corrupting method/body/signal/credentials/etc.; update the logic in oauth-fetch.ts so that when input instanceof Request you copy its properties (method, headers merged with init.headers, body, mode, credentials, cache, redirect, referrer, referrerPolicy, integrity, keepalive, signal, duplex where applicable) into the finalInit object unless explicitly overridden by init, then pass that enriched finalInit to undiciFetch (follow the same merge pattern used in the proxy-fetch implementation, and keep using getProxyAgentFromEnvironment, providerSpec.rewriteUrl, and headers handling as before).
♻️ Duplicate comments (2)
apps/desktop/src/renderer/components/settings/ProxySettings.tsx (1)
2-5: 🛠️ Refactor suggestion | 🟠 MajorSwitch new renderer imports to tsconfig aliases (already flagged previously).
These new relative imports should use project aliases (
@/...) for consistency and maintainability.As per coding guidelines,
apps/desktop/src/**/*.{ts,tsx}: "Use path aliases defined in tsconfig.json for imports:@/*(renderer),@shared/*(shared),@preload/*(preload),@features/*,@components/*,@hooks/*,@lib/*."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/settings/ProxySettings.tsx` around lines 2 - 5, Replace the relative imports with tsconfig path aliases: instead of '../ui/label', '../ui/input', '../ui/switch' and './SettingsSection' import Label, Input, Switch and SettingsSection via the project aliases (e.g. use `@/` or `@components/` paths per the repo convention) so the file imports the components using the tsconfig aliases (referencing the symbols Label, Input, Switch, SettingsSection) rather than relative paths.apps/desktop/src/main/ipc-handlers/__tests__/settings-proxy-runtime.integration.test.ts (1)
138-156:⚠️ Potential issue | 🟡 MinorRestore the incoming proxy env instead of always clearing it.
These tests mutate
process.env, butafterEachalways deletes the keys instead of restoring the incoming values. If the Vitest worker starts with HTTP(S)_PROXY already set, this suite permanently changes the environment for whatever runs next. Snapshot the four proxy keys inbeforeEachand restore them inafterEach, likeapps/desktop/src/main/__tests__/ipc-handlers.test.tsnow does. As per coding guidelines,apps/desktop/**/*.test.{ts,tsx}: "Ensure tests are comprehensive and follow Vitest conventions. Check for proper mocking and test isolation."🛠️ Suggested fix
describe('settings:save proxy runtime integration', () => { + let baselineProxyEnv: Partial<Record<(typeof PROXY_ENV_KEYS)[number], string | undefined>> = {}; + beforeEach(() => { vi.clearAllMocks(); + baselineProxyEnv = Object.fromEntries( + PROXY_ENV_KEYS.map((key) => [key, process.env[key]]) + ) as Partial<Record<(typeof PROXY_ENV_KEYS)[number], string | undefined>>; clearProxyEnv(); @@ afterEach(() => { - clearProxyEnv(); + for (const key of PROXY_ENV_KEYS) { + const originalValue = baselineProxyEnv[key]; + if (typeof originalValue === 'undefined') { + delete process.env[key]; + } else { + process.env[key] = originalValue; + } + } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/ipc-handlers/__tests__/settings-proxy-runtime.integration.test.ts` around lines 138 - 156, The afterEach currently calls clearProxyEnv() which always deletes proxy env vars; instead snapshot the four proxy keys (HTTP_PROXY, HTTPS_PROXY, http_proxy, https_proxy) in beforeEach and restore them in afterEach so tests don't permanently mutate process.env; update the setup around registerSettingsHandlers in apps/desktop/src/main/ipc-handlers/__tests__/settings-proxy-runtime.integration.test.ts to save previous values (e.g., prevProxy = {HTTP_PROXY, HTTPS_PROXY, http_proxy, https_proxy}) in beforeEach and restore those values (reassign or delete if undefined) in afterEach rather than always calling clearProxyEnv(), keeping existing clearProxyEnv usage only for initial clearing if desired.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/main/ai/auth/codex-oauth.ts`:
- Around line 359-365: The proxied token POST requests using undiciFetch with
dispatcher from getProxyAgentFromEnvironment (TOKEN_ENDPOINT) lack an
AbortController timeout and can hang; update both places (the token exchange and
refreshCodexToken) to mirror the bounded AbortController pattern used in
fetchCodexUsage(): create an AbortController, set a timeout (clear it after
response), pass controller.signal to undiciFetch, and ensure the timeout is
cleaned up on success or error so stalled proxy CONNECTs are aborted and
login/refresh won't hang.
In `@apps/desktop/src/main/utils/runtime-proxy-config.ts`:
- Around line 183-205: applyRuntimeProxyConfig updates process.env but doesn't
dispose previous ProxyAgent instances, leaving pooled sockets open; fix this by
pruning/disposing cached agents whenever runtime proxy settings change (both
when normalizing to 'disabled' and after setting new URLs). Implement or call a
helper that iterates the cached ProxyAgent instances (from the same cache used
by getProxyAgentFromEnvironment) and calls their destroy/close method to free
sockets, then invoke that helper in applyRuntimeProxyConfig (referencing
applyRuntimeProxyConfig, getProxyAgentFromEnvironment, ProxyAgent and the
HTTP_PROXY_KEYS/HTTPS_PROXY_KEYS branches) right after changing/deleting
process.env and before returning the normalized result.
- Around line 234-260: getProxyAgentFromEnvironment currently trusts raw env
values and passes proxyUrl into new ProxyAgent(...) which can throw on malformed
URLs; before using a proxy URL (both when populating activeProxyUrls and before
constructing/caching an agent) call validateProxyUrl(proxyUrl) and ensure the
scheme is supported via getRequestScheme(...) (or reject non-http/https), and if
validation fails skip that URL (or return undefined) instead of constructing the
agent; specifically update getProxyAgentFromEnvironment to validate/filter the
HTTP_PROXY/HTTPS_PROXY entries before adding to activeProxyUrls and to validate
the proxyUrl returned by getProxyUrlFromEnvironment (catching validation errors)
before calling new ProxyAgent(proxyUrl) and setting proxyAgentCache to avoid
crashing on bad env values while keeping pruneStaleProxyAgents, proxyAgentCache,
and ProxyAgent usage intact.
In `@apps/desktop/src/renderer/components/settings/AppSettings.tsx`:
- Line 55: The import for ProxySettings uses a relative path; replace it with
the tsconfig path alias per renderer conventions — update the import statement
that references ProxySettings (the named import ProxySettings) to use the
project alias (for example import { ProxySettings } from
'@components/ProxySettings' or the appropriate '@/*' alias) so it follows the
apps/desktop renderer alias rules.
- Line 72: AppSection currently includes 'integrations' and 'api-profiles' but
there are no navigation entries or renderAppSection cases, so those values lead
to an empty pane; either remove those members from the AppSection union or add
explicit UI and render logic: update the navigation list (where
initialSection/open-app-settings is handled) to include nav items for
'integrations' and 'api-profiles', and implement corresponding branches in
renderAppSection (or the component that switches on AppSection) with proper
content; also add an exhaustive never check in the switch/default to surface
unhandled variants at compile time (e.g., a default that asserts never) so
future additions can't silently render null.
---
Outside diff comments:
In `@apps/desktop/src/main/ai/providers/oauth-fetch.ts`:
- Around line 251-280: When input is a Request the wrapper currently only uses
input.url and drops other request properties, corrupting
method/body/signal/credentials/etc.; update the logic in oauth-fetch.ts so that
when input instanceof Request you copy its properties (method, headers merged
with init.headers, body, mode, credentials, cache, redirect, referrer,
referrerPolicy, integrity, keepalive, signal, duplex where applicable) into the
finalInit object unless explicitly overridden by init, then pass that enriched
finalInit to undiciFetch (follow the same merge pattern used in the proxy-fetch
implementation, and keep using getProxyAgentFromEnvironment,
providerSpec.rewriteUrl, and headers handling as before).
---
Duplicate comments:
In
`@apps/desktop/src/main/ipc-handlers/__tests__/settings-proxy-runtime.integration.test.ts`:
- Around line 138-156: The afterEach currently calls clearProxyEnv() which
always deletes proxy env vars; instead snapshot the four proxy keys (HTTP_PROXY,
HTTPS_PROXY, http_proxy, https_proxy) in beforeEach and restore them in
afterEach so tests don't permanently mutate process.env; update the setup around
registerSettingsHandlers in
apps/desktop/src/main/ipc-handlers/__tests__/settings-proxy-runtime.integration.test.ts
to save previous values (e.g., prevProxy = {HTTP_PROXY, HTTPS_PROXY, http_proxy,
https_proxy}) in beforeEach and restore those values (reassign or delete if
undefined) in afterEach rather than always calling clearProxyEnv(), keeping
existing clearProxyEnv usage only for initial clearing if desired.
In `@apps/desktop/src/renderer/components/settings/ProxySettings.tsx`:
- Around line 2-5: Replace the relative imports with tsconfig path aliases:
instead of '../ui/label', '../ui/input', '../ui/switch' and './SettingsSection'
import Label, Input, Switch and SettingsSection via the project aliases (e.g.
use `@/` or `@components/` paths per the repo convention) so the file imports the
components using the tsconfig aliases (referencing the symbols Label, Input,
Switch, SettingsSection) rather than relative paths.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: a928d32d-81d2-43e7-aa17-117e9cbe2685
📒 Files selected for processing (12)
apps/desktop/src/main/__tests__/ipc-handlers.test.tsapps/desktop/src/main/ai/auth/codex-oauth.tsapps/desktop/src/main/ai/providers/oauth-fetch.tsapps/desktop/src/main/claude-profile/codex-usage-fetcher.tsapps/desktop/src/main/claude-profile/usage-monitor.tsapps/desktop/src/main/ipc-handlers/__tests__/settings-proxy-runtime.integration.test.tsapps/desktop/src/main/utils/proxy-fetch.tsapps/desktop/src/main/utils/runtime-proxy-config.tsapps/desktop/src/renderer/components/settings/AppSettings.tsxapps/desktop/src/renderer/components/settings/ProxySettings.tsxapps/desktop/src/renderer/components/settings/__tests__/AppSettings.proxy-nav.test.tsxapps/desktop/src/renderer/components/settings/__tests__/ProxySettings.test.tsx
| import { AdvancedSettings } from './AdvancedSettings'; | ||
| import { DevToolsSettings } from './DevToolsSettings'; | ||
| import { DebugSettings } from './DebugSettings'; | ||
| import { ProxySettings } from './ProxySettings'; |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use a tsconfig alias for the new ProxySettings import.
This new renderer import should follow the project alias convention instead of a relative path.
🔧 Suggested change
-import { ProxySettings } from './ProxySettings';
+import { ProxySettings } from '@/components/settings/ProxySettings';As per coding guidelines, apps/desktop/src/**/*.{ts,tsx}: "Use path aliases defined in tsconfig.json for imports: @/* (renderer), @shared/* (shared), @preload/* (preload), @features/*, @components/*, @hooks/*, @lib/*."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| import { ProxySettings } from './ProxySettings'; | |
| import { ProxySettings } from '@/components/settings/ProxySettings'; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/renderer/components/settings/AppSettings.tsx` at line 55,
The import for ProxySettings uses a relative path; replace it with the tsconfig
path alias per renderer conventions — update the import statement that
references ProxySettings (the named import ProxySettings) to use the project
alias (for example import { ProxySettings } from '@components/ProxySettings' or
the appropriate '@/*' alias) so it follows the apps/desktop renderer alias
rules.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
apps/desktop/src/main/utils/runtime-proxy-config.ts (2)
188-211:⚠️ Potential issue | 🟠 MajorStale
ProxyAgentinstances are not pruned when proxy settings change.When
applyRuntimeProxyConfigdisables proxy or changes URLs, the old cachedProxyAgentinstances inproxyAgentCacheremain alive with open pooled sockets. This causes resource leaks and potential connection reuse to stale proxies.Call
pruneStaleProxyAgentsafter modifyingprocess.env:🔧 Proposed fix
export function applyRuntimeProxyConfig(input: RuntimeProxyInput): RuntimeProxyConfigResult { const normalized = normalizeRuntimeProxyConfig(input); if (normalized.state === 'invalid') { return normalized; } if (normalized.state === 'disabled') { for (const key of HTTP_PROXY_KEYS) { delete process.env[key]; } for (const key of HTTPS_PROXY_KEYS) { delete process.env[key]; } + pruneStaleProxyAgents(new Set()); return normalized; } process.env.HTTP_PROXY = normalized.httpProxyUrl; process.env.http_proxy = normalized.httpProxyUrl; process.env.HTTPS_PROXY = normalized.httpsProxyUrl; process.env.https_proxy = normalized.httpsProxyUrl; + pruneStaleProxyAgents(new Set([normalized.httpProxyUrl, normalized.httpsProxyUrl])); return normalized; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/utils/runtime-proxy-config.ts` around lines 188 - 211, The applyRuntimeProxyConfig function currently updates process.env but does not prune cached ProxyAgent instances; after normalizeRuntimeProxyConfig returns 'disabled' (inside the block that deletes HTTP_PROXY_KEYS and HTTPS_PROXY_KEYS) and after you set process.env.HTTP_PROXY/http_proxy/HTTPS_PROXY/https_proxy for the enabled case, call pruneStaleProxyAgents() to tear down any previous agents in proxyAgentCache so stale pooled sockets are closed; locate applyRuntimeProxyConfig, normalizeRuntimeProxyConfig, HTTP_PROXY_KEYS, HTTPS_PROXY_KEYS and insert pruneStaleProxyAgents() immediately after the environment modifications.
239-266:⚠️ Potential issue | 🟠 MajorValidate proxy URLs from environment before constructing
ProxyAgent.
getProxyAgentFromEnvironmentreads rawprocess.envvalues and passes them directly tonew ProxyAgent(proxyUrl)at line 263. If the environment contains malformed URLs (e.g.,HTTP_PROXY=notaurl), the constructor throwsTypeError [ERR_INVALID_URL], crashing outbound network calls.Use the existing
validateProxyUrlfunction before agent construction:🛡️ Proposed fix
export function getProxyAgentFromEnvironment(target?: string | URL): ProxyAgent | undefined { const httpProxy = process.env.HTTP_PROXY || process.env.http_proxy; const httpsProxy = process.env.HTTPS_PROXY || process.env.https_proxy; - const activeProxyUrls = new Set<string>(); - if (httpProxy) { - activeProxyUrls.add(httpProxy); - } - if (httpsProxy) { - activeProxyUrls.add(httpsProxy); - } + const validatedHttp = validateProxyUrl(httpProxy, 'http'); + const validatedHttps = validateProxyUrl(httpsProxy, 'https'); + + const activeProxyUrls = new Set<string>(); + if (validatedHttp.normalized) { + activeProxyUrls.add(validatedHttp.normalized); + } + if (validatedHttps.normalized) { + activeProxyUrls.add(validatedHttps.normalized); + } pruneStaleProxyAgents(activeProxyUrls); const proxyUrl = getProxyUrlFromEnvironment(target); if (!proxyUrl) { return undefined; } + const scheme = getRequestScheme(target); + const kind = scheme === 'http:' ? 'http' : 'https'; + const validated = validateProxyUrl(proxyUrl, kind); + if (!validated.normalized) { + return undefined; + } + - const cachedAgent = proxyAgentCache.get(proxyUrl); + const cachedAgent = proxyAgentCache.get(validated.normalized); if (cachedAgent) { return cachedAgent; } - const agent = new ProxyAgent(proxyUrl); - proxyAgentCache.set(proxyUrl, agent); + const agent = new ProxyAgent(validated.normalized); + proxyAgentCache.set(validated.normalized, agent); return agent; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/utils/runtime-proxy-config.ts` around lines 239 - 266, getProxyAgentFromEnvironment currently passes raw proxyUrl into new ProxyAgent and can crash on malformed values; before creating or caching an agent (in getProxyAgentFromEnvironment), run validateProxyUrl(proxyUrl) and only construct ProxyAgent(proxyUrl) when validation succeeds; if validateProxyUrl fails, do not construct or cache an agent (return undefined or skip to next proxy), and ensure you still call pruneStaleProxyAgents and consult proxyAgentCache as before (refer to symbols: getProxyAgentFromEnvironment, validateProxyUrl, proxyAgentCache, ProxyAgent).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/desktop/src/main/utils/runtime-proxy-config.ts`:
- Around line 188-211: The applyRuntimeProxyConfig function currently updates
process.env but does not prune cached ProxyAgent instances; after
normalizeRuntimeProxyConfig returns 'disabled' (inside the block that deletes
HTTP_PROXY_KEYS and HTTPS_PROXY_KEYS) and after you set
process.env.HTTP_PROXY/http_proxy/HTTPS_PROXY/https_proxy for the enabled case,
call pruneStaleProxyAgents() to tear down any previous agents in proxyAgentCache
so stale pooled sockets are closed; locate applyRuntimeProxyConfig,
normalizeRuntimeProxyConfig, HTTP_PROXY_KEYS, HTTPS_PROXY_KEYS and insert
pruneStaleProxyAgents() immediately after the environment modifications.
- Around line 239-266: getProxyAgentFromEnvironment currently passes raw
proxyUrl into new ProxyAgent and can crash on malformed values; before creating
or caching an agent (in getProxyAgentFromEnvironment), run
validateProxyUrl(proxyUrl) and only construct ProxyAgent(proxyUrl) when
validation succeeds; if validateProxyUrl fails, do not construct or cache an
agent (return undefined or skip to next proxy), and ensure you still call
pruneStaleProxyAgents and consult proxyAgentCache as before (refer to symbols:
getProxyAgentFromEnvironment, validateProxyUrl, proxyAgentCache, ProxyAgent).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1ba5ee7f-441f-477e-ab19-70876f8ce709
📒 Files selected for processing (1)
apps/desktop/src/main/utils/runtime-proxy-config.ts
Comment on lines
+445
to
+455
| proxyHttpUrl: newSettings.proxyHttpUrl, | ||
| proxyHttpsUrl: newSettings.proxyHttpsUrl, | ||
| }); | ||
|
|
||
| if (runtimeProxyConfig.state === 'invalid') { | ||
| const formattedErrors = runtimeProxyConfig.errors.map((entry) => entry.message).join(' '); | ||
| return { | ||
| success: false, | ||
| error: `Invalid proxy settings. ${formattedErrors}`, | ||
| }; | ||
| } |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/desktop/src/main/ai/providers/oauth-fetch.ts (1)
119-125:⚠️ Potential issue | 🟠 MajorAdd a deadline to the proxied refresh-token POST.
This refresh path now goes through user-configurable proxy state, but it still has no request timeout. A dead proxy or stalled CONNECT can hang
ensureValidOAuthToken()indefinitely instead of letting the caller fail fast and fall back to re-authentication. Mirror the bounded fetch wrapper already used inapps/desktop/src/main/ai/auth/codex-oauth.ts.🛠️ Suggested fix
+ const controller = new AbortController(); + const timeout = setTimeout(() => controller.abort(), 15_000); - const proxyAgent = getProxyAgentFromEnvironment(providerSpec.tokenEndpoint); - const response = await undiciFetch(providerSpec.tokenEndpoint, { - method: 'POST', - headers: { 'Content-Type': 'application/x-www-form-urlencoded' }, - body: body.toString(), - dispatcher: proxyAgent, - }); + let response: Awaited<ReturnType<typeof undiciFetch>>; + try { + const proxyAgent = getProxyAgentFromEnvironment(providerSpec.tokenEndpoint); + response = await undiciFetch(providerSpec.tokenEndpoint, { + method: 'POST', + headers: { 'Content-Type': 'application/x-www-form-urlencoded' }, + body: body.toString(), + signal: controller.signal, + dispatcher: proxyAgent, + }); + } finally { + clearTimeout(timeout); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/ai/providers/oauth-fetch.ts` around lines 119 - 125, The POST to providerSpec.tokenEndpoint uses undiciFetch without a timeout; wrap this proxied request with the same bounded fetch wrapper used in apps/desktop/src/main/ai/auth/codex-oauth.ts so ensureValidOAuthToken() can't hang on a stalled proxy. Replace the direct undiciFetch call (in oauth-fetch.ts) with the bounded/fetch-with-timeout helper, preserving method, headers, body and dispatcher: proxyAgent, and supply a sensible deadline/timeout (match the codex-oauth.ts timeout value) so the request fails fast and callers can fall back to re-authentication.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/main/utils/runtime-proxy-config.ts`:
- Around line 237-238: The current precedence uses raw env values so a malformed
uppercase value blocks a valid lowercase one; update the logic that sets
httpProxy and httpsProxy (and the similar logic in getProxyAgentFromEnvironment)
to first normalize each casing independently via
normalizeEnvProxyUrl(process.env.HTTP_PROXY) and
normalizeEnvProxyUrl(process.env.http_proxy) and then pick the first
non-null/valid normalized result (same for HTTPS_PROXY/https_proxy), ensuring
the active agent selection and resolved proxy URL use the validated proxy string
rather than raw env precedence.
---
Duplicate comments:
In `@apps/desktop/src/main/ai/providers/oauth-fetch.ts`:
- Around line 119-125: The POST to providerSpec.tokenEndpoint uses undiciFetch
without a timeout; wrap this proxied request with the same bounded fetch wrapper
used in apps/desktop/src/main/ai/auth/codex-oauth.ts so ensureValidOAuthToken()
can't hang on a stalled proxy. Replace the direct undiciFetch call (in
oauth-fetch.ts) with the bounded/fetch-with-timeout helper, preserving method,
headers, body and dispatcher: proxyAgent, and supply a sensible deadline/timeout
(match the codex-oauth.ts timeout value) so the request fails fast and callers
can fall back to re-authentication.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: f0f051a4-54c9-4189-9d6b-c01c0182f4f1
📒 Files selected for processing (5)
apps/desktop/src/main/ai/auth/codex-oauth.tsapps/desktop/src/main/ai/providers/oauth-fetch.tsapps/desktop/src/main/ipc-handlers/__tests__/settings-proxy-runtime.integration.test.tsapps/desktop/src/main/utils/runtime-proxy-config.tsapps/desktop/src/renderer/components/settings/AppSettings.tsx
Comment on lines
292
to
298
| return { | ||
| success: true, | ||
| accessToken: data.access_token, | ||
| refreshToken: data.refresh_token, | ||
| refreshToken: typeof data.refresh_token === 'string' ? data.refresh_token : undefined, | ||
| expiresAt, | ||
| expiresIn | ||
| }; |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
http://127.0.0.1:7890Test plan
cd apps/desktop && npm run typecheckcd apps/desktop && npm test -- src/renderer/components/settings/__tests__/AppSettings.proxy-nav.test.tsx src/renderer/components/settings/__tests__/ProxySettings.test.tsxcd apps/desktop && npm test -- src/main/ipc-handlers/__tests__/settings-proxy-runtime.integration.test.tscd apps/desktop && npm run buildAI Disclosure
Summary by CodeRabbit
New Features
Bug Fixes
Tests