Skip to content

Commit db5ca20

Browse files
EhabYEhabY
committed
refactor: extract DismissibleNotifier and harden proxy warning
Move showDismissibleNotification from util into a DismissibleNotifier class under core: it holds globalState and constrains keys to a known DismissibleNotificationKey set, wired through ServiceContainer. Rename applySshProxyEnvironment to applySshEnvironment, generalize its doc to "SSH-related environment variables", keep the internal env type unexported, and order the file public-API first. Make the useLocalServer warning a blocking warning modal so the setting is written (via the jsonc settings util, not cfg.update) before ssh spawns, which lets it apply without a window reload.
1 parent d4ed808 commit db5ca20

8 files changed

Lines changed: 280 additions & 275 deletions

File tree

‎src/core/container.ts‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ import { CliCredentialManager } from "./cliCredentialManager";
1515
import { CliManager } from "./cliManager";
1616
import { CommandManager } from "./commandManager";
1717
import { ContextManager } from "./contextManager";
18+
import { DismissibleNotifier } from "./dismissibleNotifier";
1819
import { MementoManager } from "./mementoManager";
1920
import { PathResolver } from "./pathResolver";
2021
import { SecretsManager } from "./secretsManager";
@@ -39,6 +40,7 @@ export class ServiceContainer implements vscode.Disposable {
3940
private readonly speedtestPanelFactory: SpeedtestPanelFactory;
4041
private readonly telemetryService: TelemetryService;
4142
private readonly commandManager: CommandManager;
43+
private readonly dismissibleNotifier: DismissibleNotifier;
4244

4345
constructor(context: vscode.ExtensionContext) {
4446
this.logger = vscode.window.createOutputChannel("Coder", { log: true });
@@ -119,6 +121,7 @@ export class ServiceContainer implements vscode.Disposable {
119121
);
120122

121123
this.commandManager = new CommandManager(this.telemetryService);
124+
this.dismissibleNotifier = new DismissibleNotifier(context.globalState);
122125
}
123126

124127
getPathResolver(): PathResolver {
@@ -173,6 +176,10 @@ export class ServiceContainer implements vscode.Disposable {
173176
return this.commandManager;
174177
}
175178

179+
getDismissibleNotifier(): DismissibleNotifier {
180+
return this.dismissibleNotifier;
181+
}
182+
176183
/** Dispose logger last so telemetry teardown warnings still reach it. */
177184
async dispose(): Promise<void> {
178185
this.commandManager.dispose();

‎src/core/dismissibleNotifier.ts‎

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
import * as vscode from "vscode";
2+
3+
import type { Memento } from "vscode";
4+
5+
const DISMISS = "Don't Show Again";
6+
7+
/** globalState keys under which "Don't Show Again" dismissals are stored. */
8+
export const DISMISSIBLE_NOTIFICATION_KEYS = [
9+
"coder.proxyUseLocalServerWarningDismissed",
10+
] as const;
11+
12+
export type DismissibleNotificationKey =
13+
(typeof DISMISSIBLE_NOTIFICATION_KEYS)[number];
14+
15+
export class DismissibleNotifier {
16+
public constructor(private readonly globalState: Memento) {}
17+
18+
/**
19+
* Show a warning notification with a "Don't Show Again" button that persists
20+
* dismissal under `key`. Returns the chosen action, or undefined if dismissed,
21+
* closed, or already dismissed before. Pass `modal` to block until the user
22+
* answers; non-modal toasts can auto-dismiss, so blocking callers must set it.
23+
*/
24+
public async showDismissible(
25+
key: DismissibleNotificationKey,
26+
message: string,
27+
{ actions = [], modal = false }: { actions?: string[]; modal?: boolean } = {},
28+
): Promise<string | undefined> {
29+
if (this.globalState.get<boolean>(key)) {
30+
return undefined;
31+
}
32+
33+
const choice = await vscode.window.showWarningMessage(
34+
message,
35+
{ modal },
36+
...actions,
37+
DISMISS,
38+
);
39+
40+
if (choice === DISMISS) {
41+
await this.globalState.update(key, true);
42+
return undefined;
43+
}
44+
return choice;
45+
}
46+
}

‎src/remote/environment.ts‎

Lines changed: 49 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -1,20 +1,10 @@
1-
/**
2-
* Sets HTTP_PROXY/HTTPS_PROXY/NO_PROXY on this extension host's process.env so
3-
* the spawned `coder ssh` ProxyCommand inherits them (the coder CLI reads them
4-
* like any Go HTTP client and has no proxy flag). We mutate process.env rather
5-
* than baking them into the SSH config so no credentialed proxy URL is written
6-
* to disk and multiple windows onto the same workspace stay independent.
7-
*
8-
* Best-effort: only processes spawned after it runs inherit the change. Notably
9-
* MS VS Code with `remote.SSH.useLocalServer=false` spawns ssh off a path that
10-
* does not inherit, so propagation there needs `useLocalServer=true`.
11-
*/
12-
131
import { getProxyForUrl } from "../api/proxy";
142

153
import type { WorkspaceConfiguration } from "vscode";
164

17-
export type SshProxyEnvironment = Partial<
5+
type Environment = Record<string, string | undefined>;
6+
type PreviousValue = [key: string, existed: boolean, value: string | undefined];
7+
type SshEnvironment = Partial<
188
Record<"HTTP_PROXY" | "HTTPS_PROXY" | "NO_PROXY", string>
199
>;
2010

@@ -31,13 +21,35 @@ export const SSH_PROXY_SETTINGS: ReadonlyArray<{
3121
{ setting: "coder.proxyBypass", title: "Proxy Bypass" },
3222
];
3323

34-
type Environment = Record<string, string | undefined>;
35-
type PreviousValue = [key: string, existed: boolean, value: string | undefined];
24+
/**
25+
* Sets SSH-related environment variables on this extension host's process.env so
26+
* the spawned `coder ssh` ProxyCommand inherits them. For now that is just the
27+
* proxy configuration (HTTP_PROXY/HTTPS_PROXY/NO_PROXY): the coder CLI reads them
28+
* like any Go HTTP client and has no proxy flag. We mutate process.env rather
29+
* than baking values into the SSH config so no credentialed proxy URL is written
30+
* to disk and multiple windows onto the same workspace stay independent.
31+
*
32+
* Best-effort: only processes spawned afterwards inherit the change. MS VS Code
33+
* with `remote.SSH.useLocalServer=false` spawns ssh off a path that does not
34+
* inherit, so propagation there needs `useLocalServer=true`. Returns a disposable
35+
* that restores the previous values.
36+
*/
37+
export function applySshEnvironment(
38+
baseUrl: string,
39+
cfg: Pick<WorkspaceConfiguration, "get">,
40+
env: Environment = process.env,
41+
): { dispose(): void } {
42+
return applyEnvironment(getSshProxyEnvironment(baseUrl, cfg), env);
43+
}
3644

45+
/**
46+
* The proxy portion of the SSH environment. Exposed so callers can check whether
47+
* a proxy actually applies to a deployment via `.HTTP_PROXY`.
48+
*/
3749
export function getSshProxyEnvironment(
3850
baseUrl: string,
3951
cfg: Pick<WorkspaceConfiguration, "get">,
40-
): SshProxyEnvironment {
52+
): SshEnvironment {
4153
const httpProxy = getSetting(cfg, "http.proxy");
4254
const noProxy = getSetting(cfg, "coder.proxyBypass") ?? getHttpNoProxy(cfg);
4355
const proxy = httpProxy
@@ -50,36 +62,8 @@ export function getSshProxyEnvironment(
5062
};
5163
}
5264

53-
export function applySshProxyEnvironment(
54-
baseUrl: string,
55-
cfg: Pick<WorkspaceConfiguration, "get">,
56-
env: Environment = process.env,
57-
): { dispose(): void } {
58-
return applyEnvironment(getSshProxyEnvironment(baseUrl, cfg), env);
59-
}
60-
61-
function getSetting(
62-
cfg: Pick<WorkspaceConfiguration, "get">,
63-
setting: string,
64-
): string | undefined {
65-
const value = cfg.get<string | null>(setting);
66-
return typeof value === "string" ? value.trim() || undefined : undefined;
67-
}
68-
69-
function getHttpNoProxy(
70-
cfg: Pick<WorkspaceConfiguration, "get">,
71-
): string | undefined {
72-
return (
73-
cfg
74-
.get<string[]>("http.noProxy", [])
75-
.map((value) => value.trim())
76-
.filter(Boolean)
77-
.join(",") || undefined
78-
);
79-
}
80-
8165
function applyEnvironment(
82-
values: SshProxyEnvironment,
66+
values: SshEnvironment,
8367
env: Environment,
8468
): { dispose(): void } {
8569
const previous: PreviousValue[] = [];
@@ -118,3 +102,23 @@ function getEnvKeys(env: Environment, key: string): string[] {
118102
);
119103
return keys.length > 0 ? keys : [key];
120104
}
105+
106+
function getSetting(
107+
cfg: Pick<WorkspaceConfiguration, "get">,
108+
setting: string,
109+
): string | undefined {
110+
const value = cfg.get<string | null>(setting);
111+
return typeof value === "string" ? value.trim() || undefined : undefined;
112+
}
113+
114+
function getHttpNoProxy(
115+
cfg: Pick<WorkspaceConfiguration, "get">,
116+
): string | undefined {
117+
return (
118+
cfg
119+
.get<string[]>("http.noProxy", [])
120+
.map((value) => value.trim())
121+
.filter(Boolean)
122+
.join(",") || undefined
123+
);
124+
}

‎src/remote/remote.ts‎

Lines changed: 37 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,4 @@
11
import { isAxiosError } from "axios";
2-
import { type Api } from "coder/site/src/api/api";
3-
import {
4-
type Workspace,
5-
type WorkspaceAgent,
6-
} from "coder/site/src/api/typesGenerated";
72
import * as fs from "node:fs/promises";
83
import * as os from "node:os";
94
import * as path from "node:path";
@@ -20,18 +15,11 @@ import { extractAgents } from "../api/api-helper";
2015
import { AuthInterceptor } from "../api/authInterceptor";
2116
import { CoderApi } from "../api/coderApi";
2217
import { needToken } from "../api/utils";
23-
import { type Commands } from "../commands";
2418
import {
2519
CONFIG_CHANGE_DEBOUNCE_MS,
2620
watchConfigurationChanges,
2721
} from "../configWatcher";
2822
import { version as cliVersion } from "../core/cliExec";
29-
import { type CliManager } from "../core/cliManager";
30-
import { type ServiceContainer } from "../core/container";
31-
import { type ContextManager } from "../core/contextManager";
32-
import { type StartupMode } from "../core/mementoManager";
33-
import { type PathResolver } from "../core/pathResolver";
34-
import { type SecretsManager } from "../core/secretsManager";
3523
import { toError } from "../error/errorUtils";
3624
import { featureSetForVersion, type FeatureSet } from "../featureSet";
3725
import { Inbox } from "../inbox";
@@ -40,8 +28,6 @@ import {
4028
RemoteSetupTelemetry,
4129
type RemoteSetupTracer,
4230
} from "../instrumentation/remoteSetup";
43-
import { type Logger } from "../logging/logger";
44-
import { type LoginCoordinator } from "../login/loginCoordinator";
4531
import { OAuthSessionManager } from "../oauth/sessionManager";
4632
import {
4733
type CliAuth,
@@ -58,12 +44,11 @@ import {
5844
expandPath,
5945
parseRemoteAuthority,
6046
} from "../util";
61-
import { showDismissibleNotification } from "../util/notifications";
6247
import { vscodeProposed } from "../vscodeProposed";
6348
import { WorkspaceMonitor } from "../workspace/workspaceMonitor";
6449

6550
import {
66-
applySshProxyEnvironment,
51+
applySshEnvironment,
6752
getSshProxyEnvironment,
6853
SSH_PROXY_SETTINGS,
6954
} from "./environment";
@@ -79,6 +64,23 @@ import { SshProcessMonitor } from "./sshProcess";
7964
import { computeSshProperties, sshSupportsSetEnv } from "./sshSupport";
8065
import { WorkspaceStateMachine } from "./workspaceStateMachine";
8166

67+
import type { Api } from "coder/site/src/api/api";
68+
import type {
69+
Workspace,
70+
WorkspaceAgent,
71+
} from "coder/site/src/api/typesGenerated";
72+
73+
import type { Commands } from "../commands";
74+
import type { CliManager } from "../core/cliManager";
75+
import type { ServiceContainer } from "../core/container";
76+
import type { ContextManager } from "../core/contextManager";
77+
import type { DismissibleNotifier } from "../core/dismissibleNotifier";
78+
import type { StartupMode } from "../core/mementoManager";
79+
import type { PathResolver } from "../core/pathResolver";
80+
import type { SecretsManager } from "../core/secretsManager";
81+
import type { Logger } from "../logging/logger";
82+
import type { LoginCoordinator } from "../login/loginCoordinator";
83+
8284
export interface RemoteDetails extends vscode.Disposable {
8385
safeHostname: string;
8486
url: string;
@@ -109,6 +111,7 @@ export class Remote {
109111
private readonly contextManager: ContextManager;
110112
private readonly secretsManager: SecretsManager;
111113
private readonly loginCoordinator: LoginCoordinator;
114+
private readonly dismissibleNotifier: DismissibleNotifier;
112115
private readonly setupTelemetry: RemoteSetupTelemetry;
113116
private readonly authTelemetry: AuthTelemetry;
114117

@@ -123,6 +126,7 @@ export class Remote {
123126
this.contextManager = serviceContainer.getContextManager();
124127
this.secretsManager = serviceContainer.getSecretsManager();
125128
this.loginCoordinator = serviceContainer.getLoginCoordinator();
129+
this.dismissibleNotifier = serviceContainer.getDismissibleNotifier();
126130
this.setupTelemetry = new RemoteSetupTelemetry(
127131
serviceContainer.getTelemetryService(),
128132
);
@@ -209,9 +213,9 @@ export class Remote {
209213

210214
try {
211215
disposables.push(
212-
applySshProxyEnvironment(baseUrl, vscode.workspace.getConfiguration()),
216+
applySshEnvironment(baseUrl, vscode.workspace.getConfiguration()),
213217
);
214-
void this.warnIfProxyEnvNotInherited(baseUrl);
218+
await this.warnIfProxyEnvNotInherited(baseUrl);
215219
// Create OAuth session manager for this remote deployment
216220
const remoteOAuthManager = OAuthSessionManager.create(
217221
{ url: baseUrl, safeHostname: parts.safeHostname },
@@ -837,7 +841,10 @@ export class Remote {
837841
* MS VS Code with `remote.SSH.useLocalServer=false` spawns ssh without
838842
* inheriting process.env, so the proxy variables never reach it. Warn once and
839843
* offer to enable the local server when a proxy applies to this deployment.
840-
* Catches internally so the caller can safely fire-and-forget.
844+
*
845+
* Blocks setup with a modal: the write must land before ssh spawns (which
846+
* happens after setup returns) for it to apply without a reload. Catches
847+
* internally so a failure here never aborts the connection.
841848
*/
842849
private async warnIfProxyEnvNotInherited(baseUrl: string): Promise<void> {
843850
try {
@@ -851,27 +858,24 @@ export class Remote {
851858
}
852859

853860
const ENABLE = "Enable Local Server";
854-
const choice = await showDismissibleNotification(
861+
const choice = await this.dismissibleNotifier.showDismissible(
862+
"coder.proxyUseLocalServerWarningDismissed",
855863
"Your proxy settings may not reach the SSH connection because `remote.SSH.useLocalServer` is disabled. Enable it so Coder can apply the proxy to the connection.",
856-
this.extensionContext.globalState,
857-
{ key: "coder.proxyUseLocalServerWarningDismissed", actions: [ENABLE] },
864+
{ actions: [ENABLE], modal: true },
858865
);
859866
if (choice !== ENABLE) {
860867
return;
861868
}
862869

863-
await cfg.update(
864-
"remote.SSH.useLocalServer",
865-
true,
866-
vscode.ConfigurationTarget.Global,
867-
);
868-
const RELOAD = "Reload";
869-
const reload = await vscode.window.showInformationMessage(
870-
"Local server enabled. Reload the window to apply.",
871-
RELOAD,
870+
// Use the jsonc writer, not cfg.update which can hang during remote setup.
871+
// No reload needed: ssh hasn't spawned yet, so it picks this up on connect.
872+
const ok = await applySettingOverrides(
873+
this.pathResolver.getUserSettingsPath(),
874+
[{ key: "remote.SSH.useLocalServer", value: true }],
875+
this.logger,
872876
);
873-
if (reload === RELOAD) {
874-
await vscode.commands.executeCommand("workbench.action.reloadWindow");
877+
if (!ok) {
878+
this.logger.warn("Failed to enable remote.SSH.useLocalServer");
875879
}
876880
} catch (error) {
877881
this.logger.debug(

‎src/util/notifications.ts‎

Lines changed: 0 additions & 32 deletions
This file was deleted.

0 commit comments

Comments
 (0)