feat: apply proxy settings to Electron, Python, and child processes#1679
feat: apply proxy settings to Electron, Python, and child processes#1679snomiao wants to merge 8 commits into
Conversation
Read Comfy.Network.Proxy.* settings from comfy.settings.json on startup: - Configure Chromium via session.defaultSession.setProxy() - Set process.env HTTP_PROXY/HTTPS_PROXY/NO_PROXY for child processes - Inject --http-proxy/--https-proxy/--no-proxy into Python server args - Forward proxy env vars to uv PTY sessions for pip/package installs Ref: #1105 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe pull request adds three proxy settings to the configuration schema and propagates them to startup proxy application (Electron/Chromium), server CLI launch arguments, and virtual environment child-process environments. Changes
Sequence DiagramsequenceDiagram
actor App as Application Startup
participant Cfg as Config<br/>(comfySettings)
participant ElectronSession as Electron Session<br/>(main.ts)
participant ServerBuild as Server Builder<br/>(comfyDesktopApp)
participant UVEnv as Virtual Env<br/>(virtualEnvironment)
participant ServerProc as Server Process
App->>Cfg: Load proxy settings (HttpUrl, HttpsUrl, NoProxy)
Cfg-->>App: Return proxy config
App->>ElectronSession: applyProxySettings()
ElectronSession->>ElectronSession: Build Chromium proxy rules
ElectronSession->>ElectronSession: session.defaultSession.setProxy()
ElectronSession->>App: Set env vars (HTTP/HTTPS/NO_PROXY)
App->>ServerBuild: buildServerArgs()
ServerBuild->>Cfg: Read proxy settings
Cfg-->>ServerBuild: Return proxy config
ServerBuild->>ServerBuild: Inject --http-proxy / --https-proxy / --no-proxy
App->>UVEnv: Initialize uv environment
UVEnv->>UVEnv: Re-export HTTP_PROXY, HTTPS_PROXY, NO_PROXY
UVEnv-->>App: Env for child processes
App->>ServerProc: Launch server with CLI args & env
ServerProc-->>ServerProc: Uses proxies for network requests
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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. Review rate limit: 6/8 reviews remaining, refill in 14 minutes and 41 seconds.Comment |
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… proxies
- Redact embedded credentials from proxy URLs before logging to prevent
secrets from leaking into log files and Sentry crash reports.
- Build proper Chromium per-scheme proxy rules ("http=X;https=Y") when
both HTTP and HTTPS proxies are configured, instead of collapsing them.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
dosubot
Bot
added
size:L
There was a problem hiding this comment.
Pull request overview
Adds support for configuring proxy behavior across the desktop app by reading Comfy.Network.Proxy.* from comfy.settings.json at startup and propagating the settings into Electron networking and spawned processes.
Changes:
- Load proxy settings on app startup and apply them to Electron (
session.defaultSession.setProxy) andprocess.env. - Pass proxy options through to the Python server via CLI args.
- Ensure proxy env vars are available to uv PTY sessions, and extend settings defaults/tests for new keys.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/main.ts |
Loads settings at startup and applies proxy config to Electron + environment variables. |
src/main-process/comfyDesktopApp.ts |
Injects proxy settings into ComfyUI server launch arguments. |
src/virtualEnvironment.ts |
Re-exports proxy env vars into PTY environment setup. |
src/config/comfySettings.ts |
Adds default settings + typings for proxy keys. |
tests/unit/config/comfySettings.test.ts |
Extends settings fixtures to include proxy keys. |
tests/unit/main-process/comfyDesktopApp.test.ts |
Updates settings mocks to account for new proxy keys. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const httpProxy = settings.get('Comfy.Network.Proxy.HttpUrl'); | ||
| const httpsProxy = settings.get('Comfy.Network.Proxy.HttpsUrl'); | ||
| const noProxy = settings.get('Comfy.Network.Proxy.NoProxy'); | ||
|
|
||
| if (!httpProxy && !httpsProxy) return; | ||
|
|
||
| log.info( | ||
| `Applying proxy settings: HTTP=${redactProxyUrl(httpProxy) || '(none)'}, HTTPS=${redactProxyUrl(httpsProxy) || '(none)'}` | ||
| ); | ||
|
|
||
| // Build Chromium proxy rules that respect separate HTTP/HTTPS proxies. | ||
| // Format: "http=<proxy>;https=<proxy>" or a single proxy for both. | ||
| const proxyRules = buildChromiumProxyRules(httpProxy, httpsProxy); | ||
|
|
||
| // Configure Chromium's network stack (affects Electron's own requests and BrowserWindow). | ||
| await session.defaultSession.setProxy({ | ||
| proxyRules, | ||
| proxyBypassRules: noProxy || undefined, | ||
| }); |
There was a problem hiding this comment.
applyProxySettings returns early when both httpProxy and httpsProxy are empty, which also prevents applying NO_PROXY/no_proxy and proxyBypassRules. This means a user cannot configure only bypass rules (e.g., to complement an OS- or externally-provided proxy). Consider applying NO_PROXY and proxyBypassRules whenever noProxy is set, even if no explicit proxy URL is configured.
There was a problem hiding this comment.
Fixed in d14addf: moved NO_PROXY/no_proxy export before the early return so a noProxy-only config is applied even when no explicit proxy URL is configured.
| * Builds Chromium-format proxy rules from separate HTTP and HTTPS proxy URLs. | ||
| * When both are set, uses per-scheme syntax: "http=<proxy>;https=<proxy>". | ||
| * When only one is set, uses it as a catch-all. | ||
| */ | ||
| function buildChromiumProxyRules(httpProxy: string, httpsProxy: string): string { | ||
| if (httpProxy && httpsProxy) { | ||
| return `http=${httpProxy};https=${httpsProxy}`; | ||
| } | ||
| return httpProxy || httpsProxy; | ||
| } | ||
|
|
||
| /** | ||
| * Redacts embedded credentials from a proxy URL for safe logging. | ||
| * E.g. "http://user:pass@host:8080" becomes "http://***@host:8080". | ||
| */ | ||
| function redactProxyUrl(url: string): string { |
There was a problem hiding this comment.
buildChromiumProxyRules injects the values from Comfy.Network.Proxy.*Url directly into Chromium proxy rules (e.g. http=${httpProxy}), but these settings are named/treated as URLs elsewhere (env vars, redaction with new URL()). Chromium/Electron proxyRules typically expects host:port (or username:password@host:port) rather than a full URL; producing http=http://host:port is likely to be rejected. Consider normalizing the configured URL(s) into Chromium's expected proxy rule format before calling session.defaultSession.setProxy().
| * Builds Chromium-format proxy rules from separate HTTP and HTTPS proxy URLs. | |
| * When both are set, uses per-scheme syntax: "http=<proxy>;https=<proxy>". | |
| * When only one is set, uses it as a catch-all. | |
| */ | |
| function buildChromiumProxyRules(httpProxy: string, httpsProxy: string): string { | |
| if (httpProxy && httpsProxy) { | |
| return `http=${httpProxy};https=${httpsProxy}`; | |
| } | |
| return httpProxy || httpsProxy; | |
| } | |
| /** | |
| * Redacts embedded credentials from a proxy URL for safe logging. | |
| * E.g. "http://user:pass@host:8080" becomes "http://***@host:8080". | |
| */ | |
| function redactProxyUrl(url: string): string { | |
| * Normalizes a proxy URL into Chromium `proxyRules` token format. | |
| * Input is expected to be a full URL (e.g. "http://user:pass@host:8080"). | |
| * Output is "user:pass@host:port" or "host:port" (no scheme), which is what | |
| * Electron/Chromium expects in `proxyRules`. | |
| */ | |
| function normalizeProxyUrlForChromium(proxyUrl?: string): string | undefined { | |
| if (!proxyUrl) return undefined; | |
| try { | |
| const parsed = new URL(proxyUrl); | |
| const auth = | |
| parsed.username !== '' | |
| ? `${parsed.username}${parsed.password !== '' ? `:${parsed.password}` : ''}@` | |
| : ''; | |
| const host = parsed.hostname; | |
| const port = parsed.port; | |
| const hostPort = port ? `${host}:${port}` : host; | |
| if (!hostPort) { | |
| // If hostname is missing, fall back to the original string. | |
| return proxyUrl; | |
| } | |
| return `${auth}${hostPort}`; | |
| } catch { | |
| // On parse failure, fall back to the original string so we don't | |
| // accidentally disable proxying entirely. | |
| return proxyUrl; | |
| } | |
| } | |
| /** | |
| * Builds Chromium-format proxy rules from separate HTTP and HTTPS proxy URLs. | |
| * When both are set, uses per-scheme syntax: "http=<proxy>;https=<proxy>". | |
| * When only one is set, uses it as a catch-all. | |
| */ | |
| function buildChromiumProxyRules(httpProxy: string | undefined, httpsProxy: string | undefined): string { | |
| const normalizedHttp = normalizeProxyUrlForChromium(httpProxy); | |
| const normalizedHttps = normalizeProxyUrlForChromium(httpsProxy); | |
| if (normalizedHttp && normalizedHttps) { | |
| return `http=${normalizedHttp};https=${normalizedHttps}`; | |
| } | |
| // If only one proxy is defined, use it as a catch-all. | |
| return normalizedHttp || normalizedHttps || ''; | |
| } | |
| /** | |
| * Redacts embedded credentials from a proxy URL for safe logging. | |
| * E.g. "http://user:pass@host:8080" becomes "http://***@host:8080". | |
| */ | |
| function redactProxyUrl(url?: string): string { |
There was a problem hiding this comment.
Addressed in d14addf: for single-proxy cases, buildChromiumProxyRules now adds the appropriate scheme prefix (http= or https=) so the proxy is only applied to the intended scheme. Chromium's ProxyConfig grammar accepts full URLs as the server value (e.g. https=http://proxy:8080), so the scheme prefix is the key correction here rather than URL normalization.
| async buildServerArgs({ useExternalServer, COMFY_HOST, COMFY_PORT }: DevOverrides): Promise<ServerArgs> { | ||
| // Shallow-clone the setting launch args to avoid mutation. | ||
| const serverArgs: ServerArgs = { | ||
| ...DEFAULT_SERVER_ARGS, | ||
| ...useComfySettings().get('Comfy.Server.LaunchArgs'), | ||
| }; | ||
|
|
||
| if (COMFY_HOST) serverArgs.listen = COMFY_HOST; | ||
| if (COMFY_PORT) serverArgs.port = COMFY_PORT; | ||
|
|
||
| // Inject proxy settings from frontend config into server CLI args. | ||
| const settings = useComfySettings(); | ||
| const httpProxy = settings.get('Comfy.Network.Proxy.HttpUrl'); | ||
| const httpsProxy = settings.get('Comfy.Network.Proxy.HttpsUrl'); | ||
| const noProxy = settings.get('Comfy.Network.Proxy.NoProxy'); | ||
| if (httpProxy) serverArgs['http-proxy'] = httpProxy; | ||
| if (httpsProxy) serverArgs['https-proxy'] = httpsProxy; | ||
| if (noProxy) serverArgs['no-proxy'] = noProxy; |
There was a problem hiding this comment.
buildServerArgs calls useComfySettings() twice in the same method (once for Comfy.Server.LaunchArgs and again for proxy keys). Since this is a singleton accessor, it’s simpler to call it once and reuse the same instance to avoid duplicated lookups and keep the args-building logic easier to follow.
There was a problem hiding this comment.
Fixed in 2298c45: useComfySettings() is now called once at the top of buildServerArgs and the instance reused throughout.
| get: vi.fn(() => true), | ||
| get: vi.fn((key: string) => { | ||
| // Proxy settings should default to empty string (no proxy configured). | ||
| if (key.startsWith('Comfy.Network.Proxy.')) return ''; |
There was a problem hiding this comment.
The mocked ComfySettings.get returns true for all non-proxy keys. This works in current tests but is type-inaccurate for keys used as objects (e.g. Comfy.Server.LaunchArgs) and can mask real issues if future tests start asserting on these values. Consider returning more realistic defaults per key (at least {} for Comfy.Server.LaunchArgs) while keeping proxy keys as empty strings.
| if (key.startsWith('Comfy.Network.Proxy.')) return ''; | |
| if (key.startsWith('Comfy.Network.Proxy.')) return ''; | |
| // Server launch arguments are expected to be an object. | |
| if (key === 'Comfy.Server.LaunchArgs') return {}; | |
| // Default to a truthy value for other keys. |
There was a problem hiding this comment.
Fixed in a8099f0: the mock now returns true for Comfy-Desktop.AutoUpdate, {} for Comfy.Server.LaunchArgs, '' for proxy keys, and undefined as the catch-all fallback.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@src/main-process/comfyDesktopApp.ts`:
- Around line 58-63: The proxy string values (httpProxy, httpsProxy, noProxy)
are not normalized and whitespace-only values slip past the truthy checks and
end up in serverArgs; trim each value (e.g., normalize httpProxy, httpsProxy,
noProxy via .trim() when they are strings) and only assign to
serverArgs['http-proxy'], ['https-proxy'], ['no-proxy'] if the trimmed result is
non-empty, ensuring you handle non-string/undefined safely and preserve existing
keys otherwise.
In `@src/main.ts`:
- Around line 138-139: The early return when both httpProxy and httpsProxy are
empty causes NO_PROXY/no_proxy (and Comfy.Network.Proxy.NoProxy-only configs) to
be skipped; modify the logic in the proxy setup so that exporting
NO_PROXY/no_proxy and handling Comfy.Network.Proxy.NoProxy happens before any
return or ensure those exports run unconditionally (move the NO_PROXY/no_proxy
export code out of the httpProxy/httpsProxy conditional or call it regardless),
update the proxy initialization in the same way for the similar block around the
code referenced (the second block handling httpProxy/httpsProxy) to ensure
NoProxy is not dropped.
- Around line 181-186: buildChromiumProxyRules currently returns a raw proxy URL
for single-proxy cases which causes Electron to treat it as a catch-all; update
the function so when only httpProxy is present it returns "http=<httpProxy>" and
when only httpsProxy is present it returns "https=<httpsProxy>", keeping the
existing combined "http=<httpProxy>;https=<httpsProxy>" when both are set and
returning an empty string/undefined when neither is provided; change references
in the function signature buildChromiumProxyRules(httpProxy, httpsProxy)
accordingly.
In `@tests/unit/main-process/comfyDesktopApp.test.ts`:
- Around line 18-22: The mock for settings.get (vi.fn in
comfyDesktopApp.test.ts) is too broad — it returns '' for proxy keys but true
for everything else which makes values like "Comfy.Server.LaunchArgs"
unrealistic and can hide regressions; update the mock to return realistic,
specific values per key (e.g., return an empty array or the expected launch-args
structure for "Comfy.Server.LaunchArgs", booleans for feature flags, numbers for
numeric keys, etc.) and only default to a safe fallback (null or undefined) when
a key is truly unknown; locate the vi.fn mock in the test, and adjust cases to
explicitly handle "Comfy.Server.LaunchArgs" and any other keys used by
buildServerArgs so the test exercises real shapes rather than a blanket true.
🪄 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: CHILL
Plan: Pro
Run ID: f755018c-8305-4461-9c71-c75696c79a15
📒 Files selected for processing (6)
src/config/comfySettings.tssrc/main-process/comfyDesktopApp.tssrc/main.tssrc/virtualEnvironment.tstests/unit/config/comfySettings.test.tstests/unit/main-process/comfyDesktopApp.test.ts
…x in proxy rules - Move NO_PROXY/no_proxy export before the early return so a noProxy-only config is honoured even when no explicit proxy URL is set - Fix buildChromiumProxyRules to use scheme-specific prefixes (http=/https=) for single-proxy cases, preventing unintended catch-all behaviour when only one scheme's proxy is configured Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ildServerArgs - Call useComfySettings() once and reuse the instance to avoid duplicated singleton lookups - Trim proxy string values before truthy checks so whitespace-only inputs do not emit invalid server CLI args Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Return specific values per key so the mock accurately reflects real types:
- true for Comfy-Desktop.AutoUpdate (boolean)
- {} for Comfy.Server.LaunchArgs (object)
- '' for proxy keys (string)
- undefined as the catch-all fallback
This prevents type-inaccurate returns from masking future regressions.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Summary
Comfy.Network.Proxy.*settings fromcomfy.settings.jsonon app startupsession.defaultSession.setProxy()for Electron's own networkprocess.envHTTP_PROXY/HTTPS_PROXY/NO_PROXYfor all child processes--http-proxy/--https-proxy/--no-proxyinto Python server launch argsPart of a cross-repo feature. Related PRs:
Ref: #1105
Test plan
--http-proxyarg🤖 Generated with Claude Code
┆Issue is synchronized with this Notion page by Unito
Summary by CodeRabbit
New Features
Chores
Tests