feat(desktop): add terminal proxy settings with inherited env detection and overrides#2842
Open
vivishko wants to merge 15 commits into
Open
feat(desktop): add terminal proxy settings with inherited env detection and overrides#2842vivishko wants to merge 15 commits into
vivishko wants to merge 15 commits into
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:
📝 WalkthroughWalkthroughAdd terminal-proxy support across DB schema, validation/sanitization, shared utilities, main-process resolution and env application, tRPC endpoints, renderer UI for global and project settings, daemon integration, and unit tests. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Settings UI
participant API as tRPC Backend
participant DB as Local DB
participant Daemon as Terminal Daemon
participant Host as Terminal Host
Note over UI,Host: Save settings or start terminal session
UI->>API: setTerminalProxySettings / update(project.terminalProxyOverride)
API->>DB: validate & upsert terminal proxy settings/override
DB-->>API: OK
API-->>UI: { success: true }
UI->>Daemon: createOrAttach(workspaceId)
Daemon->>API: resolveEffectiveTerminalProxyForWorkspace(workspaceId)
API->>DB: fetch workspace → project.override + settings
DB-->>API: data
API->>API: resolveEffectiveTerminalProxy(projectOverride, settings, inherited)
API-->>Daemon: EffectiveTerminalProxy{state,source,config}
Daemon->>Daemon: applyTerminalProxyToEnv(env, effectiveProxy)
Daemon->>Host: createOrAttach({ env: effectiveEnv })
Host-->>Daemon: sessionReady
sequenceDiagram
participant UI as Settings UI
participant API as tRPC Backend
participant Detector as Inherited Detector
UI->>API: getDetectedInheritedProxy({ refresh? })
API->>Detector: getDetectedInheritedProxy(forceRefresh?)
Detector-->>API: { hasProxy,httpProxy,httpsProxy,noProxy,proxyUrl }
API->>API: maskProxyUrlCredentials for UI
API-->>UI: detected proxy (masked)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes 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 |
![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: 4
🧹 Nitpick comments (5)
apps/desktop/src/main/lib/terminal/daemon/daemon-manager.ts (1)
445-449: Consider fail-open behavior if proxy resolution errors.If
resolveEffectiveTerminalProxyForWorkspace(...)throws, terminal creation currently fails even though proxy config is optional. A defensive fallback to{ state: "none", source: "none" }would improve resilience.♻️ Suggested hardening
- const effectiveProxy = await resolveEffectiveTerminalProxyForWorkspace({ - workspaceId, - }); + let effectiveProxy; + try { + effectiveProxy = await resolveEffectiveTerminalProxyForWorkspace({ + workspaceId, + }); + } catch (error) { + console.warn( + `[DaemonTerminalManager] Failed to resolve terminal proxy for workspace ${workspaceId}; continuing without proxy:`, + error, + ); + effectiveProxy = { state: "none", source: "none" } as const; + } const effectiveEnv = applyTerminalProxyToEnv(env, effectiveProxy);Also applies to: 491-491
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/lib/terminal/daemon/daemon-manager.ts` around lines 445 - 449, When calling resolveEffectiveTerminalProxyForWorkspace before applying proxy settings, wrap the call in a try/catch and on error set effectiveProxy to a defensive fallback object { state: "none", source: "none" } so terminal creation can continue; then pass that effectiveProxy into applyTerminalProxyToEnv (same change should be applied to the other usage of resolveEffectiveTerminalProxyForWorkspace in this file). Ensure the catch logs the error (using the existing logger) for diagnostics but does not rethrow.apps/desktop/src/main/lib/terminal/daemon/daemon-manager.test.ts (1)
617-656: Add the existing-session attach case.This only proves the create-new branch (
daemonAliveSessionIds = new Set()), so it does not lock in the PR requirement that proxy env is applied only when creating a session. A regression that starts passing proxy env on reattach would still pass here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/lib/terminal/daemon/daemon-manager.test.ts` around lines 617 - 656, Add a test covering the existing-session attach branch: set managerInternals.daemonAliveSessionIds to include the session id that should be considered "alive" (use the same paneId/requestId mapping used by DaemonTerminalManager), call manager.createOrAttach like the current test, await the tick, and assert that resolveEffectiveTerminalProxyForWorkspaceCalls is NOT invoked and that mockClient.createOrAttachCalls[0]?.env does NOT contain HTTP_PROXY/HTTPS_PROXY/http_proxy/https_proxy keys (while still containing PATH). Reference DaemonTerminalManager, managerInternals.daemonSessionIdsHydrated, managerInternals.daemonAliveSessionIds, mockEffectiveTerminalProxy, manager.createOrAttach, mockClient.createOrAttachCalls, and resolveEffectiveTerminalProxyForWorkspaceCalls to locate and implement the new test case.packages/local-db/src/schema/zod.ts (1)
271-283: Encode the mode/manual invariant in the exported schemas.These shared Zod contracts still accept impossible persisted shapes like
{ mode: "manual" },{ mode: "enabled" }, or stalemanualpayloads onauto/disabled/inherit. Tightening the schema here keeps the DB contract consistent instead of relying only on router-specific validation.♻️ Possible schema tightening
-export const terminalProxySettingsSchema = z.object({ - mode: z.enum(TERMINAL_PROXY_MODE_GLOBAL), - manual: terminalProxyConfigSchema.optional(), -}); +export const terminalProxySettingsSchema = z.discriminatedUnion("mode", [ + z.object({ mode: z.literal("auto") }), + z.object({ mode: z.literal("disabled") }), + z.object({ + mode: z.literal("manual"), + manual: terminalProxyConfigSchema, + }), +]); export type TerminalProxySettings = z.infer<typeof terminalProxySettingsSchema>; -export const terminalProxyOverrideSchema = z.object({ - mode: z.enum(TERMINAL_PROXY_MODE_PROJECT), - manual: terminalProxyConfigSchema.optional(), -}); +export const terminalProxyOverrideSchema = z.discriminatedUnion("mode", [ + z.object({ mode: z.literal("inherit") }), + z.object({ mode: z.literal("disabled") }), + z.object({ + mode: z.literal("enabled"), + manual: terminalProxyConfigSchema, + }), +]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/local-db/src/schema/zod.ts` around lines 271 - 283, Tighten terminalProxySettingsSchema and terminalProxyOverrideSchema to encode the mode/manual invariant: replace the current loose z.object(...) with a discriminated union (or explicit union of objects) keyed on mode so that when mode is the "manual" literal the manual property is required and validated by terminalProxyConfigSchema, and when mode is any other allowed literal (the non-manual values from TERMINAL_PROXY_MODE_GLOBAL / TERMINAL_PROXY_MODE_PROJECT) manual is prohibited/absent; update the schemas (terminalProxySettingsSchema, terminalProxyOverrideSchema) to use z.discriminatedUnion('mode', [...]) or equivalent z.union(refined objects) so impossible persisted shapes like { mode: "manual" } or stale manual payloads on auto/disabled/inherit are rejected while preserving the existing terminalProxyConfigSchema validation.apps/desktop/src/shared/terminal-proxy.ts (2)
222-251: Minor: Redundant return statement.Lines 247-250 have a condition for
params.effective.state === "none"that returns "No proxy configured", but the final fallback on line 250 also returns the same string. This makes the explicit condition at line 247 redundant.♻️ Simplify by removing redundant condition
if ( params.projectMode === "inherit" && params.globalMode === "auto" && params.effective.state === "manual" ) { return "Using global (inherited)"; } - if (params.effective.state === "none") { - return "No proxy configured"; - } return "No proxy configured"; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/shared/terminal-proxy.ts` around lines 222 - 251, The function getTerminalProxyStateLabel contains a redundant conditional that checks params.effective.state === "none" and returns "No proxy configured", which is identical to the final fallback; remove that explicit if-block (the check for params.effective.state === "none") so the function falls through to the existing final return while preserving all other conditions and behavior.
171-220: Consider clarifying thesourcevalue when global mode is "disabled".At line 192, when
globalSettings.mode === "disabled", the source is set to"none"rather than something more descriptive like"global-disabled". This could make debugging or UI display slightly less intuitive since "none" doesn't indicate where the disabled state originated.However, this is a minor naming consideration and the current implementation is functionally correct.
💡 Optional: More descriptive source value
if (globalSettings.mode === "disabled") { - return { state: "disabled", source: "none" }; + return { state: "disabled", source: "global-disabled" }; }This would require adding
"global-disabled"to theEffectiveTerminalProxySourcetype on line 148-152.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/shared/terminal-proxy.ts` around lines 171 - 220, The code returns source: "none" when globalSettings.mode === "disabled" which hides the origin; update resolveEffectiveTerminalProxyFromSettings so that the branch for globalSettings.mode === "disabled" returns { state: "disabled", source: "global-disabled" } (instead of "none"), and add "global-disabled" to the EffectiveTerminalProxySource union/type so the new literal is allowed; also search for uses of EffectiveTerminalProxy.source (e.g., UI renderers or switch statements) and update any exhaustiveness checks or display strings to handle "global-disabled".
🤖 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/lib/trpc/routers/settings/index.ts`:
- Around line 150-198: The code currently allows proxy URLs with embedded
credentials to be persisted; update the validation to reject credential-bearing
proxy URLs rather than persisting secrets. In sanitizeTerminalProxySettingsInput
(and the shared validateTerminalProxyConfig used there), detect URLs containing
userinfo (username[:password]@) and throw a TRPCError with code "BAD_REQUEST"
and a clear message (e.g., "Proxy URLs with embedded credentials are not
allowed; use secure storage"). Also add a superRefine or custom check to
terminalProxyConfigInputSchema to surface the same validation to callers, and
apply the identical check to the project override write path referenced in the
review so both code paths reject credential-bearing proxyUrl values until
keychain storage is implemented.
In
`@apps/desktop/src/renderer/routes/_authenticated/settings/project/`$projectId/components/ProjectSettings/components/TerminalProxySection/TerminalProxySection.tsx:
- Around line 33-60: The label is being computed from the local draft UI `mode`;
update getEffectiveStateLabel to derive the effective label from the persisted
override (use `currentOverride`/saved settings) and the shared resolver output
rather than the local draft `mode` so it matches what new sessions actually
receive; locate and replace usages inside TerminalProxySection that compute
`effectiveStateLabel` (including other similar computations inside this
component) to call the shared resolver or helper that takes
`currentOverride`/persisted settings and `globalSettings`/detectedHasProxy, and
return the resolved label (use the existing getTerminalProxyStateLabel or the
shared resolver output as the source of truth).
In
`@apps/desktop/src/renderer/routes/_authenticated/settings/terminal/components/TerminalSettings/components/TerminalProxySection/TerminalProxySection.tsx`:
- Around line 183-199: The current JSX in TerminalProxySection renders a single
branch based on detectedQuery.data?.hasProxy which hides per-key values (e.g.,
NO_PROXY) when hasProxy is false and uses detectedValueFallback/“—”
indiscriminately; update the rendering logic in TerminalProxySection to render
each proxy key (HTTP_PROXY, HTTPS_PROXY, NO_PROXY) independently using
detectedQuery.data.httpProxy / httpsProxy / noProxy when present, and otherwise
use detectedValueFallback or the explicit string "No values found" for that
specific key, keeping detectedQuery and detectedValueFallback as the sources and
preserving the surrounding styling/container.
- Around line 43-53: handleModeChange currently updates the local mode state
before calling setTerminalProxySettings and the mutation's onError only shows a
toast, leaving the UI out of sync if the write fails; fix by capturing the
previous mode (e.g., prevMode) before calling handleModeChange or before
updating state, then in setTerminalProxySettings.useMutation's onError revert
the local state back to prevMode (or call
utils.settings.getTerminalProxySettings.invalidate() to refresh from server) so
the selected radio matches persisted settings; apply the same rollback logic to
the other mutation handler referenced around the second occurrence (the other
setTerminalProxySettings/useMutation block) so both mutation failures restore
the prior state.
---
Nitpick comments:
In `@apps/desktop/src/main/lib/terminal/daemon/daemon-manager.test.ts`:
- Around line 617-656: Add a test covering the existing-session attach branch:
set managerInternals.daemonAliveSessionIds to include the session id that should
be considered "alive" (use the same paneId/requestId mapping used by
DaemonTerminalManager), call manager.createOrAttach like the current test, await
the tick, and assert that resolveEffectiveTerminalProxyForWorkspaceCalls is NOT
invoked and that mockClient.createOrAttachCalls[0]?.env does NOT contain
HTTP_PROXY/HTTPS_PROXY/http_proxy/https_proxy keys (while still containing
PATH). Reference DaemonTerminalManager,
managerInternals.daemonSessionIdsHydrated,
managerInternals.daemonAliveSessionIds, mockEffectiveTerminalProxy,
manager.createOrAttach, mockClient.createOrAttachCalls, and
resolveEffectiveTerminalProxyForWorkspaceCalls to locate and implement the new
test case.
In `@apps/desktop/src/main/lib/terminal/daemon/daemon-manager.ts`:
- Around line 445-449: When calling resolveEffectiveTerminalProxyForWorkspace
before applying proxy settings, wrap the call in a try/catch and on error set
effectiveProxy to a defensive fallback object { state: "none", source: "none" }
so terminal creation can continue; then pass that effectiveProxy into
applyTerminalProxyToEnv (same change should be applied to the other usage of
resolveEffectiveTerminalProxyForWorkspace in this file). Ensure the catch logs
the error (using the existing logger) for diagnostics but does not rethrow.
In `@apps/desktop/src/shared/terminal-proxy.ts`:
- Around line 222-251: The function getTerminalProxyStateLabel contains a
redundant conditional that checks params.effective.state === "none" and returns
"No proxy configured", which is identical to the final fallback; remove that
explicit if-block (the check for params.effective.state === "none") so the
function falls through to the existing final return while preserving all other
conditions and behavior.
- Around line 171-220: The code returns source: "none" when globalSettings.mode
=== "disabled" which hides the origin; update
resolveEffectiveTerminalProxyFromSettings so that the branch for
globalSettings.mode === "disabled" returns { state: "disabled", source:
"global-disabled" } (instead of "none"), and add "global-disabled" to the
EffectiveTerminalProxySource union/type so the new literal is allowed; also
search for uses of EffectiveTerminalProxy.source (e.g., UI renderers or switch
statements) and update any exhaustiveness checks or display strings to handle
"global-disabled".
In `@packages/local-db/src/schema/zod.ts`:
- Around line 271-283: Tighten terminalProxySettingsSchema and
terminalProxyOverrideSchema to encode the mode/manual invariant: replace the
current loose z.object(...) with a discriminated union (or explicit union of
objects) keyed on mode so that when mode is the "manual" literal the manual
property is required and validated by terminalProxyConfigSchema, and when mode
is any other allowed literal (the non-manual values from
TERMINAL_PROXY_MODE_GLOBAL / TERMINAL_PROXY_MODE_PROJECT) manual is
prohibited/absent; update the schemas (terminalProxySettingsSchema,
terminalProxyOverrideSchema) to use z.discriminatedUnion('mode', [...]) or
equivalent z.union(refined objects) so impossible persisted shapes like { mode:
"manual" } or stale manual payloads on auto/disabled/inherit are rejected while
preserving the existing terminalProxyConfigSchema validation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b8d442f5-66d1-4120-baa3-e96b22add975
📒 Files selected for processing (19)
apps/desktop/src/lib/trpc/routers/projects/projects.tsapps/desktop/src/lib/trpc/routers/settings/index.tsapps/desktop/src/main/lib/terminal/daemon/daemon-manager.test.tsapps/desktop/src/main/lib/terminal/daemon/daemon-manager.tsapps/desktop/src/main/lib/terminal/terminal-proxy.tsapps/desktop/src/renderer/routes/_authenticated/settings/project/$projectId/components/ProjectSettings/ProjectSettings.tsxapps/desktop/src/renderer/routes/_authenticated/settings/project/$projectId/components/ProjectSettings/components/TerminalProxySection/TerminalProxySection.tsxapps/desktop/src/renderer/routes/_authenticated/settings/project/$projectId/components/ProjectSettings/components/TerminalProxySection/index.tsapps/desktop/src/renderer/routes/_authenticated/settings/terminal/components/TerminalSettings/TerminalSettings.tsxapps/desktop/src/renderer/routes/_authenticated/settings/terminal/components/TerminalSettings/components/TerminalProxySection/TerminalProxySection.tsxapps/desktop/src/renderer/routes/_authenticated/settings/terminal/components/TerminalSettings/components/TerminalProxySection/index.tsapps/desktop/src/renderer/routes/_authenticated/settings/utils/settings-search/settings-search.tsapps/desktop/src/shared/terminal-proxy.test.tsapps/desktop/src/shared/terminal-proxy.tspackages/local-db/drizzle/0038_add_terminal_proxy_settings.sqlpackages/local-db/drizzle/meta/0038_snapshot.jsonpackages/local-db/drizzle/meta/_journal.jsonpackages/local-db/src/schema/schema.tspackages/local-db/src/schema/zod.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
3 issues found across 9 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/renderer/routes/_authenticated/settings/project/$projectId/components/ProjectSettings/components/TerminalProxySection/TerminalProxySection.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/settings/project/$projectId/components/ProjectSettings/components/TerminalProxySection/TerminalProxySection.tsx:100">
P2: Shared mutation rollback uses stale `previousModeRef` during manual project proxy saves, causing failed saves to revert mode to an old selection.</violation>
</file>
<file name="apps/desktop/src/shared/terminal-proxy.ts">
<violation number="1" location="apps/desktop/src/shared/terminal-proxy.ts:87">
P1: Auto/inherited proxy mode incorrectly drops authenticated inherited proxy URLs by validating them with manual credential restrictions, then silently falling back to `none`.</violation>
</file>
<file name="apps/desktop/src/main/lib/terminal/daemon/daemon-manager.ts">
<violation number="1" location="apps/desktop/src/main/lib/terminal/daemon/daemon-manager.ts:451">
P2: Proxy resolution is incorrectly gated by local pre-RPC session cache, so racy/stale cache hits can cause newly created sessions to launch without configured proxy environment.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
apps/desktop/src/shared/terminal-proxy.ts (1)
243-269: Missing label case for "inherit" mode with global "disabled".When
projectMode === "inherit"andglobalMode === "disabled", the function falls through to "No proxy configured" which is technically correct but could be more specific (e.g., "Proxy disabled (global setting)").This is a minor UX consideration and not a bug since the effective state is correctly "none" in this case.
💡 Optional: Add specific label for inherited disabled state
if ( params.projectMode === "inherit" && params.globalMode === "auto" && params.effective.state === "manual" ) { return "Using global (inherited)"; } + if ( + params.projectMode === "inherit" && + params.globalMode === "disabled" + ) { + return "Proxy disabled (global setting)"; + } return "No proxy configured"; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/shared/terminal-proxy.ts` around lines 243 - 269, The label function getTerminalProxyStateLabel should explicitly handle the inherited-disabled case: add a branch that checks params.projectMode === "inherit" && params.globalMode === "disabled" (and optionally params.effective.state === "none") and return a specific string such as "Proxy disabled (global setting)" instead of falling through to the generic "No proxy configured"; update the conditional ordering so this new check runs before the final return.apps/desktop/src/renderer/routes/_authenticated/settings/project/$projectId/components/ProjectSettings/components/TerminalProxySection/TerminalProxySection.tsx (2)
219-219: Placeholder suggests credentials which are rejected by validation.The placeholder
http://user:password@proxy.example.com:8080shows credentials, but the backend validation rejects proxy URLs with embedded credentials. This may confuse users.💡 Suggested placeholder update
<Input id="project-terminal-proxy-url" value={proxyUrl} onChange={(event) => setProxyUrl(event.target.value)} - placeholder="http://user:password@proxy.example.com:8080" + placeholder="http://proxy.example.com:8080" disabled={isBusy} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/settings/project/`$projectId/components/ProjectSettings/components/TerminalProxySection/TerminalProxySection.tsx at line 219, Update the placeholder in the TerminalProxySection component to remove embedded credentials so users aren't misled; replace the current placeholder value "http://user:password@proxy.example.com:8080" used in the proxy URL input with a credential-free example such as "http://proxy.example.com:8080" (or "http://proxy.example.com:8080 (no credentials)") and ensure the Input/placeholder prop in TerminalProxySection.tsx reflects this new value so it aligns with backend validation that rejects URLs with embedded credentials.
130-146: Consider storing previous mode at mutation time, not mode-change time.When
handleModeChangeis called with"enabled", it stores the current mode but doesn't mutate yet (returns early). If the user then clicks "Save Project Proxy",saveManualOverridemutates butpreviousModeRefmay already be stale if the user toggled modes multiple times before saving.However, since
saveManualOverridedoesn't usepreviousModeReffor rollback (it relies onupdateProject.onErrorwhich does usepreviousModeRef), this could cause incorrect rollback if the manual save fails.💡 Store previous mode in saveManualOverride
const saveManualOverride = () => { + previousModeRef.current = mode; updateProject.mutate({ id: projectId, patch: {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/settings/project/`$projectId/components/ProjectSettings/components/TerminalProxySection/TerminalProxySection.tsx around lines 130 - 146, handleModeChange currently updates previousModeRef when the UI mode changes, but saveManualOverride performs the mutation later and relies on previousModeRef in updateProject.onError; to avoid stale rollbacks, capture and set previousModeRef.current right before calling updateProject.mutate inside saveManualOverride (e.g., store the current mode into previousModeRef.current at the top of saveManualOverride), then proceed with the mutation so updateProject.onError can reliably restore the saved previous mode; update any comments or names as needed to make the intent clear.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/settings/project/`$projectId/components/ProjectSettings/components/TerminalProxySection/TerminalProxySection.tsx:
- Line 219: Update the placeholder in the TerminalProxySection component to
remove embedded credentials so users aren't misled; replace the current
placeholder value "http://user:password@proxy.example.com:8080" used in the
proxy URL input with a credential-free example such as
"http://proxy.example.com:8080" (or "http://proxy.example.com:8080 (no
credentials)") and ensure the Input/placeholder prop in TerminalProxySection.tsx
reflects this new value so it aligns with backend validation that rejects URLs
with embedded credentials.
- Around line 130-146: handleModeChange currently updates previousModeRef when
the UI mode changes, but saveManualOverride performs the mutation later and
relies on previousModeRef in updateProject.onError; to avoid stale rollbacks,
capture and set previousModeRef.current right before calling
updateProject.mutate inside saveManualOverride (e.g., store the current mode
into previousModeRef.current at the top of saveManualOverride), then proceed
with the mutation so updateProject.onError can reliably restore the saved
previous mode; update any comments or names as needed to make the intent clear.
In `@apps/desktop/src/shared/terminal-proxy.ts`:
- Around line 243-269: The label function getTerminalProxyStateLabel should
explicitly handle the inherited-disabled case: add a branch that checks
params.projectMode === "inherit" && params.globalMode === "disabled" (and
optionally params.effective.state === "none") and return a specific string such
as "Proxy disabled (global setting)" instead of falling through to the generic
"No proxy configured"; update the conditional ordering so this new check runs
before the final return.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8b3123ea-ed76-4887-9f1d-e990260a1f79
📒 Files selected for processing (9)
apps/desktop/src/lib/trpc/routers/projects/projects.tsapps/desktop/src/lib/trpc/routers/settings/index.tsapps/desktop/src/main/lib/terminal/daemon/daemon-manager.test.tsapps/desktop/src/main/lib/terminal/daemon/daemon-manager.tsapps/desktop/src/renderer/routes/_authenticated/settings/project/$projectId/components/ProjectSettings/components/TerminalProxySection/TerminalProxySection.tsxapps/desktop/src/renderer/routes/_authenticated/settings/terminal/components/TerminalSettings/components/TerminalProxySection/TerminalProxySection.tsxapps/desktop/src/shared/terminal-proxy.test.tsapps/desktop/src/shared/terminal-proxy.tspackages/local-db/src/schema/zod.ts
✅ Files skipped from review due to trivial changes (1)
- apps/desktop/src/lib/trpc/routers/settings/index.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/desktop/src/main/lib/terminal/daemon/daemon-manager.ts
- apps/desktop/src/renderer/routes/_authenticated/settings/terminal/components/TerminalSettings/components/TerminalProxySection/TerminalProxySection.tsx
- apps/desktop/src/main/lib/terminal/daemon/daemon-manager.test.ts
- apps/desktop/src/shared/terminal-proxy.test.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/desktop/src/shared/terminal-proxy.ts (1)
255-281: Consider adding a label for inherited global-disabled state.When
projectMode === "inherit"andglobalMode === "disabled", the function returns "No proxy configured" (line 280). This doesn't distinguish between "no proxy was ever configured" versus "proxy is actively disabled at the global level." Users might benefit from clearer feedback.💡 Optional enhancement for label clarity
if ( params.projectMode === "inherit" && params.globalMode === "auto" && params.effective.state === "manual" ) { return "Using global (inherited)"; } + if ( + params.projectMode === "inherit" && + params.globalMode === "disabled" + ) { + return "Proxy disabled (global setting)"; + } return "No proxy configured";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/shared/terminal-proxy.ts` around lines 255 - 281, Update getTerminalProxyStateLabel to handle the case where params.projectMode === "inherit" and params.globalMode === "disabled" by returning a distinct label (e.g., "Global proxy disabled (inherited)") instead of the generic "No proxy configured"; add this conditional check (inspecting params.effective.state if needed) before the final return so the inherited global-disabled state is reported explicitly, referencing the function getTerminalProxyStateLabel and the param fields params.projectMode, params.globalMode, and params.effective.state to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/desktop/src/shared/terminal-proxy.ts`:
- Around line 255-281: Update getTerminalProxyStateLabel to handle the case
where params.projectMode === "inherit" and params.globalMode === "disabled" by
returning a distinct label (e.g., "Global proxy disabled (inherited)") instead
of the generic "No proxy configured"; add this conditional check (inspecting
params.effective.state if needed) before the final return so the inherited
global-disabled state is reported explicitly, referencing the function
getTerminalProxyStateLabel and the param fields params.projectMode,
params.globalMode, and params.effective.state to locate the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ab9f42ae-2db8-4448-8171-d166117a9f44
📒 Files selected for processing (5)
apps/desktop/src/main/lib/terminal/daemon/daemon-manager.test.tsapps/desktop/src/main/lib/terminal/daemon/daemon-manager.tsapps/desktop/src/renderer/routes/_authenticated/settings/project/$projectId/components/ProjectSettings/components/TerminalProxySection/TerminalProxySection.tsxapps/desktop/src/shared/terminal-proxy.test.tsapps/desktop/src/shared/terminal-proxy.ts
✅ Files skipped from review due to trivial changes (1)
- apps/desktop/src/renderer/routes/_authenticated/settings/project/$projectId/components/ProjectSettings/components/TerminalProxySection/TerminalProxySection.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/desktop/src/shared/terminal-proxy.test.ts
- apps/desktop/src/main/lib/terminal/daemon/daemon-manager.ts
- apps/desktop/src/main/lib/terminal/daemon/daemon-manager.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/renderer/routes/_authenticated/settings/project/`$projectId/components/ProjectSettings/components/TerminalProxySection/TerminalProxySection.tsx:
- Around line 109-119: The onSuccess of
electronTrpc.projects.resetTerminalProxyOverride.useMutation only invalidates
the project query but doesn't clear the local draft, so add code in that
onSuccess callback to reset the local draft state (e.g. call setDraftOverride({
mode: "inherit", /* clear fields */ }) and, if applicable, call
setMode("inherit") or update currentOverride to match the saved value) in
addition to utils.projects.get.invalidate({ id: projectId }); to ensure the UI
leaves manual mode after a successful reset.
In `@apps/desktop/src/shared/terminal-proxy.ts`:
- Around line 104-115: maskProxyUrlCredentials currently returns the raw input
on URL parse failure which can leak credentials for malformed values; in the
catch block sanitize any potential credentials instead of returning urlValue by
detecting patterns like "user:pass@" or "user@" (e.g., regex matching
/^[^\/@\s]+@|^[^\/@\s]+:[^\/@\s]+@/) and replace the credential portion with a
conservative redact such as "***@" or "***:***@" before returning; update the
catch path in maskProxyUrlCredentials to perform this regex-based redaction so
malformed strings never echo cleartext credentials.
- Around line 149-173: The detectInheritedProxyFromEnv function collapses
HTTP_PROXY/HTTPS_PROXY into a single proxyUrl, which loses distinct per-key
values; update detectInheritedProxyFromEnv (and the DetectedInheritedProxy shape
if needed) to preserve httpProxy and httpsProxy exactly as read from env and
stop relying solely on proxyUrl for logic—return httpProxy and httpsProxy fields
(and noProxy) unchanged, set hasProxy based on either httpProxy or httpsProxy
being non-empty, and only include proxyUrl as an optional convenience without
discarding the original per-key values so downstream code (e.g.,
terminal-proxy.ts) can reconstruct session env correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ab5cfe15-a9db-4e0b-8897-4e84760c10f3
📒 Files selected for processing (3)
apps/desktop/src/renderer/routes/_authenticated/settings/project/$projectId/components/ProjectSettings/components/TerminalProxySection/TerminalProxySection.tsxapps/desktop/src/shared/terminal-proxy.test.tsapps/desktop/src/shared/terminal-proxy.ts
✅ Files skipped from review due to trivial changes (1)
- apps/desktop/src/shared/terminal-proxy.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
apps/desktop/src/shared/terminal-proxy.ts (1)
228-232:⚠️ Potential issue | 🟠 MajorAuto mode still collapses inherited
HTTP_PROXY/HTTPS_PROXYinto one URL.The effective state can only carry one
config.proxyUrl, and Lines 278-310 always preferHTTPS_PROXYwhen it is non-empty. That still drops a distinctHTTP_PROXY, and it also turns auto mode into"none"whenHTTPS_PROXYis malformed even ifHTTP_PROXYis valid. The inherited per-key values need to stay separate through the effective-state/env-application path instead of being normalized to a single URL here.Also applies to: 278-310
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/shared/terminal-proxy.ts` around lines 228 - 232, The EffectiveTerminalProxy currently collapses inherited HTTP_PROXY/HTTPS_PROXY into a single config.proxyUrl (see EffectiveTerminalProxy interface) which loses distinct per-key values and causes auto mode to prefer HTTPS_PROXY (logic around lines 278-310), so change the effective state shape to keep separate httpProxy and httpsProxy (or add these fields to EffectiveTerminalProxy and/or TerminalProxyConfig) and update the code that builds the effective state (the block that prefers HTTPS_PROXY) to validate and apply each proxy independently (accept HTTP_PROXY if valid even when HTTPS_PROXY is malformed, and only fall back to "none" if both are invalid), preserving per-key inheritance through the env-application path.
🤖 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/renderer/routes/_authenticated/settings/project/`$projectId/components/ProjectSettings/components/TerminalProxySection/TerminalProxySection.tsx:
- Around line 85-91: The effect that initializes local draft reads
currentOverride but doesn't reset when the selected project changes, causing a
previous unsaved manual draft to leak into a new project; update the component
so the draft is reinitialized when projectId changes by either adding projectId
to the dependency array of the existing useEffect that calls
normalizeOverride(currentOverride) (so
setMode/getManualFields/setProxyUrl/setNoProxy re-run on project change) or add
a new useEffect that watches projectId and resets the local fields (call
normalizeOverride(currentOverride) or clear to defaults and then setMode,
setProxyUrl, setNoProxy) to ensure drafts don't persist across projects.
In `@apps/desktop/src/shared/terminal-proxy-input.ts`:
- Around line 30-43: The schema currently validates the optional `manual` object
unconditionally which causes payloads with a stale/invalid `manual` to fail for
non-manual modes; update terminalProxySettingsInputSchema so that `manual` is
only validated when `value.mode === "manual"` (either by preprocessing/stripping
`manual` for non-manual modes or by moving validation into the mode-specific
branch used in the .superRefine callback) and keep the .superRefine check for
the required presence only for manual mode; make the same change to the other
schema in this file that follows the same pattern so non-manual modes no longer
reject stale `manual` payloads.
---
Duplicate comments:
In `@apps/desktop/src/shared/terminal-proxy.ts`:
- Around line 228-232: The EffectiveTerminalProxy currently collapses inherited
HTTP_PROXY/HTTPS_PROXY into a single config.proxyUrl (see EffectiveTerminalProxy
interface) which loses distinct per-key values and causes auto mode to prefer
HTTPS_PROXY (logic around lines 278-310), so change the effective state shape to
keep separate httpProxy and httpsProxy (or add these fields to
EffectiveTerminalProxy and/or TerminalProxyConfig) and update the code that
builds the effective state (the block that prefers HTTPS_PROXY) to validate and
apply each proxy independently (accept HTTP_PROXY if valid even when HTTPS_PROXY
is malformed, and only fall back to "none" if both are invalid), preserving
per-key inheritance through the env-application path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9363d16f-9f36-4c0b-a014-67a36eea6517
📒 Files selected for processing (10)
apps/desktop/src/lib/trpc/routers/projects/projects.tsapps/desktop/src/lib/trpc/routers/settings/index.tsapps/desktop/src/main/lib/terminal/daemon/daemon-manager.test.tsapps/desktop/src/main/lib/terminal/daemon/daemon-manager.tsapps/desktop/src/renderer/routes/_authenticated/settings/project/$projectId/components/ProjectSettings/components/TerminalProxySection/TerminalProxySection.tsxapps/desktop/src/renderer/routes/_authenticated/settings/terminal/components/TerminalSettings/components/TerminalProxySection/TerminalProxySection.tsxapps/desktop/src/shared/terminal-proxy-input.test.tsapps/desktop/src/shared/terminal-proxy-input.tsapps/desktop/src/shared/terminal-proxy.test.tsapps/desktop/src/shared/terminal-proxy.ts
✅ Files skipped from review due to trivial changes (1)
- apps/desktop/src/renderer/routes/_authenticated/settings/terminal/components/TerminalSettings/components/TerminalProxySection/TerminalProxySection.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/desktop/src/main/lib/terminal/daemon/daemon-manager.ts
- apps/desktop/src/lib/trpc/routers/projects/projects.ts
- apps/desktop/src/main/lib/terminal/daemon/daemon-manager.test.ts
- apps/desktop/src/shared/terminal-proxy.test.ts
vivishko
force-pushed
the
feat/http-proxy-settings
branch
3 times, most recently
from
6147a86 to
6ed4a04
Compare
vivishko
force-pushed
the
feat/http-proxy-settings
branch
from
6ed4a04 to
b2c86f1
Compare
…criminated unions
…nv, and fix project proxy rollback
…reserve HTTP/HTTPS inherited values
vivishko
force-pushed
the
feat/http-proxy-settings
branch
from
b2c86f1 to
968604b
Compare
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
1 participant
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.
Description
Adds terminal proxy configuration support in Desktop with global and project-level controls, plus inherited environment detection.
Main changes:
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Summary by cubic
Add terminal proxy settings with inherited env detection, global modes (auto/manual/disabled), and per‑project overrides (inherit/enabled/disabled). Applies the effective proxy only to new terminal sessions; rejects credential-bearing URLs; masks credentials; renders inherited values per key with lowercase fallbacks; exposes TRPC endpoints; persists settings in
@superset/local-db; adds search items, reset/rollback UX, and tests; and skips resolution when attaching to existing sessions.Written for commit 968604b. Summary will update on new commits.
Summary by CodeRabbit