Skip to content

feat(desktop): add HTTP proxy setting in advanced preferences#350

Merged
hqhq1025 merged 2 commits into
OpenCoworkAI:mainfrom
gandrenacci:feat/proxy-settings
May 23, 2026
Merged

feat(desktop): add HTTP proxy setting in advanced preferences#350
hqhq1025 merged 2 commits into
OpenCoworkAI:mainfrom
gandrenacci:feat/proxy-settings

Conversation

@gandrenacci
Copy link
Copy Markdown
Contributor

Summary

  • Adds a new proxyUrl preference in Settings → Advanced. The URL is applied to Chromium's network stack (session.defaultSession.setProxy) and to Node's HTTP_PROXY /
    HTTPS_PROXY env vars, so both renderer fetches and main-process outbound traffic (LLM APIs, update feed) route through the configured proxy.
  • Applied immediately on save and at boot — no restart needed. Empty string disables the proxy.
  • Persisted in ~/.config/open-codesign/preferences.json under a bumped schemaVersion (8 → 9). Defaults to '', so existing installs are unaffected.
  • i18n strings added for en / es / pt-BR / zh-CN.

Four principles

  • Compatibility ✅ — additive field, defaults to empty, older preferences upgrade cleanly via the existing migration path.
  • Upgradeability ✅ — schemaVersion bumped to 9; parsePersistedFile ignores unknown keys from stale builds.
  • No bloat ✅ — no new dependencies; uses only electron.session and Node env vars already in the runtime.
  • Elegance ✅ — single applyProxyConfig(url) helper, commit-on-blur input so we don't thrash the file/proxy on every keystroke.

Test plan

  • pnpm lint
  • pnpm typecheck
  • pnpm test (1837 tests across workspace — all green; new vitest cases cover proxy round-trip, env-var clear, whitespace trimming, and rejection of non-string
    proxyUrl)
  • Manual: set proxy in Settings → Advanced, confirm LLM request routes through it, then clear and confirm direct connection resumes — all without restart

Notes

Reference: maintainer green-light on the proxy-settings discussion. Changeset included (.changeset/proxy-settings.md): @open-codesign/desktop minor, @open-codesign/i18n
patch.

Lets users route Chromium and Node outbound traffic through a custom
proxy via Settings → Advanced. The URL is persisted in preferences.json
and applied immediately on save and at boot, with no restart needed.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review mode: initial

Findings

  • [Minor] applyProxyConfig only sets uppercase HTTP_PROXY / HTTPS_PROXY environment variables but does not handle lowercase equivalents (http_proxy / https_proxy). Node's http module prefers lowercase when both are present, so if a user already has a lowercase proxy set system-wide, the app's proxy setting may be silently ignored. For complete coverage, also set (and clear) the lowercase variants.
    Suggested fix: In apps/desktop/src/main/preferences-ipc.ts, applyProxyConfig, add:
    if (cleanUrl.length > 0) {
      process.env['HTTP_PROXY'] = cleanUrl;
      process.env['HTTPS_PROXY'] = cleanUrl;
      process.env['http_proxy'] = cleanUrl;
      process.env['https_proxy'] = cleanUrl;
    } else {
      delete process.env['HTTP_PROXY'];
      delete process.env['HTTPS_PROXY'];
      delete process.env['http_proxy'];
      delete process.env['https_proxy'];
    }

Summary

The PR cleanly adds an HTTP proxy field to Settings → Advanced, persisted via bumped schemaVersion (8→9) and applied immediately to both Chromium's network stack and Node's env vars. Tests cover round-trip, env var clear, trimming, and non-string rejection. No new dependencies, i18n strings in all four locales, and a changeset included. Good adherence to project constraints.

Residual observations

  • The applyProxyConfig JSDoc mentions it is safe to call before session.defaultSession is ready, but in that case the Chromium side is silently skipped. Currently all calls happen after app.whenReady(), so this is not a bug, but if future code calls it earlier the renderer proxy would not be applied without a warning. Consider logging when the session is unavailable.

Testing

  • New Vitest tests cover applyProxyConfig (set, clear, trim) and preferences:v1:update round-trip with proxyUrl. Existing tests updated for schema version. Automated CI passes.
  • Manual test steps described in PR body (set proxy, confirm routing, clear) are appropriate.

Open-CoDesign Bot

@github-actions github-actions Bot added docs Documentation area:desktop apps/desktop (Electron shell, renderer) labels May 20, 2026
…ssing session

Node's http module prefers lowercase http_proxy/https_proxy when both
spellings exist, so leaving them untouched could let a stale shell value
silently override the in-app proxy choice. Also log a warning when
applyProxyConfig is called before session.defaultSession is ready so a
future early caller isn't met with a silent Chromium-side skip.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review mode: follow-up after new commits

Findings

No new findings. The previous Minor finding (missing lowercase http_proxy/https_proxy env var handling) has been addressed in this revision — applyProxyConfig now writes and clears both uppercase and lowercase spellings, and a logger.warn call covers the missing-session case. The accompanying test coverage (lowercase mirror + stale lowercase clearance) confirms correctness.

Summary

The PR is ready to merge. All material issues from the initial review are resolved. The implementation correctly handles env var casing, immediate proxy application, persistence via schema bump (8→9), and i18n for four locales. No new dependencies, no direct SDK imports, no silent fallbacks for user-visible behavior. Changeset included.

Residual observations

  • The MemoryTab.tsx now includes proxyUrl: '' in its local DEFAULT_PREFS to satisfy type conformance. This is harmless but technically dead state since memory settings don't render a proxy field. Not worth a follow-up, just an observation.

Testing

  • Existing and new Vitest tests pass (1837 workspace tests green).
  • Manual test plan from PR body covers set/clear proxy with real LLM routing.

Open-CoDesign Bot

@hqhq1025 hqhq1025 merged commit bf86fea into OpenCoworkAI:main May 23, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:desktop apps/desktop (Electron shell, renderer) docs Documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants