From aba688b72f5d163bb3a1b1067fdb8f271a70848b Mon Sep 17 00:00:00 2001 From: Tomasz Naumowicz Date: Sun, 9 Aug 2026 22:31:12 +0200 Subject: [PATCH 1/2] Report Quick Start situations as typed keys, not as English sentences MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes #865. `QuickStartService` emitted user-facing text on `StageEvent.message` / `StageEvent.error` and `QuickStartStatus.errorMessage`, so the service layer owned copy, and every new message was one more chance to forget `l10n.t`. Five of them had already forgotten: the stage payloads `'Checking Docker…'`, `'Pulling the official image…'`, `'Creating container…'`, `'Starting container…'` and `'Waiting for DocumentDB to accept connections…'` were raw English. They were also dead. The webview labels the checklist from its own `stageLabels()` map and only reads `message` on terminal events, so those five strings were never rendered — untranslated text that nobody could have reported, because nobody could see it. They are gone rather than localized. What remains is a `QuickStartMessage`: a key, plus the data needed to phrase it (`port`, `environment`) and a `detail` field carrying raw daemon or driver text. `detail` is the one thing never translated, because it is evidence rather than copy — and keeping it in its own field is what stops a daemon string being concatenated into a sentence a translator owns. `StageEvent.error` is gone too. It duplicated `message` at every call site except two, where it differed only by being undefined on abort, which the webview then fell back out of. One field and `status` say the same thing. The wording lives in one shared `formatQuickStartMessage`, not one map per surface. Two copies of the same sentence in the tree and the webview could only drift, which is the failure this repo just fixed for the transitional rows. Tests now assert keys instead of sentences, which is what #764 asks for. The M5 regression — the daemon's "Bind for …" text must not reach the user — is now structural: a keyed message has nowhere to put it, and the test pins that `detail` is absent rather than grepping the rendered string. --- l10n/bundle.l10n.json | 2 +- .../QuickStartProvisionDurability.test.ts | 7 +- .../localQuickStart/QuickStartService.test.ts | 26 ++- .../localQuickStart/QuickStartService.ts | 166 ++++++------------ .../localQuickStart/quickStartMessages.ts | 63 +++++++ .../localQuickStart/quickStartTypes.ts | 45 ++++- .../LocalQuickStart/LocalQuickStartItem.ts | 3 +- .../localQuickStart/LocalQuickStart.tsx | 7 +- .../localQuickStart/localQuickStartRouter.ts | 2 +- 9 files changed, 189 insertions(+), 132 deletions(-) create mode 100644 src/services/localQuickStart/quickStartMessages.ts diff --git a/l10n/bundle.l10n.json b/l10n/bundle.l10n.json index cc221ac95..7b9c362de 100644 --- a/l10n/bundle.l10n.json +++ b/l10n/bundle.l10n.json @@ -208,7 +208,6 @@ "A kubeconfig source with identical YAML already exists.": "A kubeconfig source with identical YAML already exists.", "A new connection will be added to your Connections View.\nDo you want to continue?\n\nNote: You can disable these URL handling confirmations in the exension settings.": "A new connection will be added to your Connections View.\nDo you want to continue?\n\nNote: You can disable these URL handling confirmations in the exension settings.", "A playground is already running on this cluster. Wait for it to finish.": "A playground is already running on this cluster. Wait for it to finish.", - "A setup operation is already in progress.": "A setup operation is already in progress.", "A specific problem was identified": "A specific problem was identified", "A value is required to proceed.": "A value is required to proceed.", "A wildcard index key must be the only index key.": "A wildcard index key must be the only index key.", @@ -659,6 +658,7 @@ "Docker access denied": "Docker access denied", "Docker answered too vaguely to name a cause, so setup can still be attempted.": "Docker answered too vaguely to name a cause, so setup can still be attempted.", "Docker became unavailable during setup: {0}": "Docker became unavailable during setup: {0}", + "Docker became unavailable during setup.": "Docker became unavailable during setup.", "Docker check timed out": "Docker check timed out", "Docker CLI": "Docker CLI", "Docker CLI {0} found": "Docker CLI {0} found", diff --git a/src/services/localQuickStart/QuickStartProvisionDurability.test.ts b/src/services/localQuickStart/QuickStartProvisionDurability.test.ts index bdc767ff9..bfc1d2796 100644 --- a/src/services/localQuickStart/QuickStartProvisionDurability.test.ts +++ b/src/services/localQuickStart/QuickStartProvisionDurability.test.ts @@ -357,7 +357,7 @@ describe('QuickStartService — WP-3 provisioning durability and port model', () const events = await collect(service.provision(new AbortController().signal, { port: QUICK_START_PORT })); expect(events.at(-1)).toMatchObject({ stage: 'checking', status: 'error' }); - expect(events.at(-1)?.message).toContain(String(QUICK_START_PORT)); + expect(events.at(-1)?.message).toEqual({ key: 'portInUse', port: QUICK_START_PORT }); expect(service.getStatus().state).toBe(InstanceState.Error); }); @@ -383,8 +383,9 @@ describe('QuickStartService — WP-3 provisioning durability and port model', () const events = await collect(service.provision(new AbortController().signal)); - expect(events.at(-1)?.message).toContain(String(QUICK_START_PORT)); - expect(events.at(-1)?.message).not.toContain('Bind for'); + expect(events.at(-1)?.message).toEqual({ key: 'portInUse', port: QUICK_START_PORT }); + // The daemon's own wording never rides along: a keyed message has nowhere to put it. + expect(events.at(-1)?.message?.detail).toBeUndefined(); }); describe('suggestPort / checkPort (Configure-step validation, L3)', () => { diff --git a/src/services/localQuickStart/QuickStartService.test.ts b/src/services/localQuickStart/QuickStartService.test.ts index c650894bd..297bd3aed 100644 --- a/src/services/localQuickStart/QuickStartService.test.ts +++ b/src/services/localQuickStart/QuickStartService.test.ts @@ -8,7 +8,8 @@ import { ext } from '../../extensionVariables'; import { StorageService } from '../storageService'; import { disposeQuickStartOutputChannel, type IContainerRuntime } from './ContainerRuntime'; -import { getReadinessTimeoutMessage, QuickStartServiceImpl } from './QuickStartService'; +import { formatQuickStartMessage } from './quickStartMessages'; +import { QuickStartServiceImpl } from './QuickStartService'; import { listInstances, PROVISIONING_LEASE_TTL_MS, upsertInstance, writeConnectionString } from './quickStartStore'; import { DEFAULT_ALIAS, @@ -1488,9 +1489,10 @@ describe('QuickStartService — WI-2e-1 provision RR4 volume-wipe gate', () => { expect(events.at(-1)).toMatchObject({ stage: 'error', status: 'error', - message: `Docker became unavailable during setup: daemon disappeared during ${ - failingStage === 'pulling' ? 'pull' : 'run' - }`, + message: { + key: 'dockerUnavailableDuringSetup', + detail: `daemon disappeared during ${failingStage === 'pulling' ? 'pull' : 'run'}`, + }, dockerReadiness: unavailable, }); expect(isDockerReady).toHaveBeenLastCalledWith({ forceRefresh: true }); @@ -1530,7 +1532,7 @@ describe('QuickStartService — WI-2e-1 provision RR4 volume-wipe gate', () => { } expect(retryEvents[0]).toMatchObject({ stage: 'checking', status: 'active' }); - expect(retryEvents.map((event) => event.message)).not.toContain('Setup is already in progress.'); + expect(retryEvents.map((event) => event.message?.key)).not.toContain('setupAlreadyInProgress'); }); it('keeps an image failure on the provisioning path when Docker remains ready', async () => { @@ -1562,7 +1564,10 @@ describe('QuickStartService — WI-2e-1 provision RR4 volume-wipe gate', () => { events.push(event); } - expect(events.at(-1)).toMatchObject({ stage: 'error', error: 'manifest unknown' }); + expect(events.at(-1)).toMatchObject({ + stage: 'error', + message: { key: 'unexpectedFailure', detail: 'manifest unknown' }, + }); expect(events.at(-1)?.dockerReadiness).toBeUndefined(); }); @@ -1608,15 +1613,18 @@ describe('QuickStartService — WI-2e-1 provision RR4 volume-wipe gate', () => { events.push(event); } - expect(events.at(-1)).toMatchObject({ stage: 'error', error: 'manifest unknown' }); + expect(events.at(-1)).toMatchObject({ + stage: 'error', + message: { key: 'unexpectedFailure', detail: 'manifest unknown' }, + }); expect(events.at(-1)?.dockerReadiness).toBeUndefined(); }); it('adds the published-port explanation only for dev-container readiness timeouts', () => { - expect(getReadinessTimeoutMessage('devContainer')).toContain( + expect(formatQuickStartMessage({ key: 'readinessTimeout', environment: 'devContainer' })).toContain( 'published localhost port might not be reachable from inside the dev container', ); - expect(getReadinessTimeoutMessage('linux')).toBe( + expect(formatQuickStartMessage({ key: 'readinessTimeout', environment: 'linux' })).toBe( 'DocumentDB did not accept connections in time. It may still be initializing.', ); }); diff --git a/src/services/localQuickStart/QuickStartService.ts b/src/services/localQuickStart/QuickStartService.ts index 2877a63e0..f3872e7dd 100644 --- a/src/services/localQuickStart/QuickStartService.ts +++ b/src/services/localQuickStart/QuickStartService.ts @@ -78,6 +78,8 @@ import { QUICK_START_OPERATION_LABEL_KEY, QUICK_START_PORT, QUICK_START_PORT_SCAN_LIMIT, + type QuickStartMessage, + type QuickStartMessageKey, type QuickStartStatus, resolveQuickStartImage, type StageEvent, @@ -91,25 +93,6 @@ function traceQuickStart(message: string): void { ext.outputChannel?.trace(`[LocalQuickStart] ${message}`); } -/** - * Surfaced (design §12) when a labelled container + on-disk volume exist but the stored credentials - * are gone, so the cluster can't be opened. Reconcile NEVER removes it (a lost secret does not prove - * the volume is disposable — R2); the user decides (Delete for a clean slate, or restore the secret). - */ -function credentialUnavailableMessage(): string { - return l10n.t( - 'DocumentDB Local has data on disk but its saved credentials are missing, so it cannot be opened. Use "Delete Container" to remove it and start fresh (this erases the data).', - ); -} - -/** Shown when the chosen host port is taken — both by the pre-check and by the Docker bind failure. */ -function portInUseMessage(port: number): string { - return l10n.t( - 'Port {0} is already in use. Go back to Configure to pick a different port, or free it, then try again.', - String(port), - ); -} - /** * Docker's "port is already allocated" bind failure, in the wordings the CLI emits. The port is * pre-checked before the pull, but the pull can take minutes and something else may claim the port @@ -178,8 +161,8 @@ class ReadinessTimeoutError extends Error { * only learns about it once `finally` has cleared the `provisioning` guard. */ class DockerNotReadyError extends Error { - constructor(message: string) { - super(message); + constructor(readonly messageKey: Extract) { + super(messageKey); this.name = 'DockerNotReadyError'; } } @@ -219,7 +202,7 @@ interface InstanceRuntimeState { lifecycleBusy: boolean; missing: boolean; pendingReadiness?: PendingReadiness; - errorMessage?: string; + error?: QuickStartMessage; inFlight?: QuickStartOperation; } @@ -268,22 +251,12 @@ function resolveProvisionCredentials(options?: AdvancedQuickStartOptions): Gener function stageEvent( stage: ProvisionStage, status: StageEvent['status'], - message?: string, - error?: string, + message?: QuickStartMessage, boundPort?: number, timedOut?: boolean, dockerReadiness?: DockerReadiness, ): StageEvent { - return { stage, status, message, error, boundPort, timedOut, dockerReadiness }; -} - -export function getReadinessTimeoutMessage(environment: DockerHostEnvironment | undefined): string { - if (environment === 'devContainer') { - return l10n.t( - 'DocumentDB did not accept connections in time. Docker may be running on the dev container host, so the published localhost port might not be reachable from inside the dev container.', - ); - } - return l10n.t('DocumentDB did not accept connections in time. It may still be initializing.'); + return { stage, status, message, boundPort, timedOut, dockerReadiness }; } /** Cancellable delay that rejects if the signal aborts. */ @@ -407,7 +380,7 @@ export class QuickStartServiceImpl { return { state: entry.state, metadata: entry.metadata, - errorMessage: entry.errorMessage, + error: entry.error, missing: entry.missing, // Known even while provisioning (the port is decided in the wizard, L1/L3), so the tree // row can show the real address instead of assuming the canonical port. @@ -449,7 +422,7 @@ export class QuickStartServiceImpl { state: entry.state, missing: entry.missing, port: entry.metadata?.boundPort ?? entry.port, - errorMessage: entry.errorMessage, + error: entry.error, canResumeReadiness: !entry.provisioning && !entry.lifecycleBusy && entry.pendingReadiness !== undefined, metadata: entry.metadata, }; @@ -523,14 +496,19 @@ export class QuickStartServiceImpl { } } - private setStatus(alias: string, state: InstanceState, metadata?: InstanceMetadata, errorMessage?: string): void { + private setStatus( + alias: string, + state: InstanceState, + metadata?: InstanceMetadata, + error?: QuickStartMessage, + ): void { const entry = this.stateFor(alias); entry.state = state; if (metadata !== undefined) { entry.metadata = metadata; entry.port = metadata.boundPort; } - entry.errorMessage = errorMessage; + entry.error = error; entry.missing = false; this.statusEmitter.fire(); } @@ -554,8 +532,7 @@ export class QuickStartServiceImpl { alias: string = DEFAULT_ALIAS, ): AsyncGenerator { if (this.stateFor(alias).provisioning || this.stateFor(alias).lifecycleBusy) { - const message = l10n.t('Setup is already in progress.'); - yield stageEvent('error', 'error', message, message); + yield stageEvent('error', 'error', { key: 'setupAlreadyInProgress' }); return; } this.stateFor(alias).provisioning = true; @@ -634,18 +611,14 @@ export class QuickStartServiceImpl { this.stateFor(alias).port = chosenPort; // --- checking --- - yield stageEvent('checking', 'active', 'Checking Docker…'); + yield stageEvent('checking', 'active'); const readiness = await this.checkDockerReadiness(); readinessEnvironment = readiness.environment; this.throwIfAborted(signal); const continueAfterIndeterminateReadiness = options?.continueAnyway === true && readiness.outcome === 'indeterminate'; if ((!readiness.cliInstalled || !readiness.daemonReachable) && !continueAfterIndeterminateReadiness) { - throw new DockerNotReadyError( - !readiness.cliInstalled - ? l10n.t('Docker CLI was not found on your PATH. Install Docker and retry.') - : l10n.t('Docker is installed but the daemon is not reachable. Start Docker and retry.'), - ); + throw new DockerNotReadyError(!readiness.cliInstalled ? 'dockerCliMissing' : 'dockerDaemonUnreachable'); } // Remove a pre-existing managed container so the run starts clean (it is labelled as @@ -663,13 +636,9 @@ export class QuickStartServiceImpl { // `finally` removed it) and no `ready` record, so retrying it still works. if (!reusing && !startFresh) { if (existing || hasReadyRecord) { - this.setStatus(alias, InstanceState.CredentialsMissing, undefined, credentialUnavailableMessage()); - yield stageEvent( - 'checking', - 'error', - credentialUnavailableMessage(), - credentialUnavailableMessage(), - ); + const credentialsUnavailable: QuickStartMessage = { key: 'credentialsUnavailable' }; + this.setStatus(alias, InstanceState.CredentialsMissing, undefined, credentialsUnavailable); + yield stageEvent('checking', 'error', credentialsUnavailable); return; } } @@ -685,9 +654,9 @@ export class QuickStartServiceImpl { // step suggests a free port, validates it while the user can still react, and sends it. // Setup never relocates it — a conflict here is a hard, explained error. if (!(await this.runtime.isPortFree(chosenPort))) { - const message = portInUseMessage(chosenPort); + const message: QuickStartMessage = { key: 'portInUse', port: chosenPort }; this.setStatus(alias, InstanceState.Error, undefined, message); - yield stageEvent('checking', 'error', message, message); + yield stageEvent('checking', 'error', message); return; } this.throwIfAborted(signal); @@ -704,7 +673,7 @@ export class QuickStartServiceImpl { } // --- pulling --- - yield stageEvent('pulling', 'active', 'Pulling the official image…'); + yield stageEvent('pulling', 'active'); activeDockerStage = 'pulling'; await this.runtime.pullImage(imageRef, cts.token); activeDockerStage = undefined; @@ -712,7 +681,7 @@ export class QuickStartServiceImpl { yield stageEvent('pulling', 'done'); // --- creating (docker run -d creates and starts) --- - yield stageEvent('creating', 'active', 'Creating container…'); + yield stageEvent('creating', 'active'); if (leaseHeld) { await this.renewProvisioningLease(alias, operationId, chosenPort); } @@ -756,7 +725,7 @@ export class QuickStartServiceImpl { yield stageEvent('creating', 'done'); // --- starting (confirm running, read bound port, follow logs) --- - yield stageEvent('starting', 'active', 'Starting container…'); + yield stageEvent('starting', 'active'); const inspected = await this.runtime.inspectContainer(containerId); // Fall back to the port we actually requested (not the canonical default) if the // inspect can't report the binding, so a custom port stays correct in the success @@ -767,7 +736,7 @@ export class QuickStartServiceImpl { yield stageEvent('starting', 'done'); // --- waiting (wire-protocol readiness, D7) --- - yield stageEvent('waiting', 'active', 'Waiting for DocumentDB to accept connections…'); + yield stageEvent('waiting', 'active'); const connectionString = composeConnectionString(credentials.username, credentials.password, boundPort); // Retain everything a "Wait longer" resume needs BEFORE probing, so a readiness // timeout can keep this running container and finish adoption later (§9.1). @@ -805,23 +774,19 @@ export class QuickStartServiceImpl { await this.finalizeReadyInstance(pending, cts.token, signal); success = true; yield stageEvent('waiting', 'done'); - yield stageEvent( - 'done', - 'done', - l10n.t('DocumentDB Local is running on localhost:{0}.', String(boundPort)), - undefined, - boundPort, - ); + yield stageEvent('done', 'done', { key: 'instanceRunning', port: boundPort }, boundPort); } catch (error) { const aborted = signal.aborted; const dockerReadiness = !aborted && activeDockerStage ? await this.getProvisioningDockerReadiness() : undefined; provisioningDockerFailureKind = dockerReadiness?.failureKind; - let message = aborted ? l10n.t('Setup was cancelled.') : errMessage(error); + const detail = errMessage(error); + let message: QuickStartMessage = aborted ? { key: 'setupCancelled' } : { key: 'unexpectedFailure', detail }; if (!aborted && error instanceof DockerNotReadyError) { this.stateFor(alias).pendingReadiness = undefined; + message = { key: error.messageKey }; this.setStatus(alias, InstanceState.Error, undefined, message); - terminalEvent = stageEvent('checking', 'error', message, message); + terminalEvent = stageEvent('checking', 'error', message); } else if (!aborted && error instanceof ReadinessTimeoutError && containerCreated && containerId) { // The container is running but the database did not accept connections within the // window — it may still be initializing. KEEP it running (finally skips teardown) @@ -829,10 +794,10 @@ export class QuickStartServiceImpl { // "Wait longer" resume finish adoption. The instance sits in Error until then. The // event is buffered and emitted after `finally` (see below) so the flags are clean. readinessTimedOut = true; - channel.appendLine(`[readiness-timeout] ${message}`); - message = getReadinessTimeoutMessage(readinessEnvironment); + channel.appendLine(`[readiness-timeout] ${detail}`); + message = { key: 'readinessTimeout', environment: readinessEnvironment }; this.setStatus(alias, InstanceState.Error, undefined, message); - terminalEvent = stageEvent('waiting', 'error', message, message, undefined, /* timedOut */ true); + terminalEvent = stageEvent('waiting', 'error', message, undefined, /* timedOut */ true); } else { // Any other failure (or cancel) discards the attempt — drop the retained state so a // stale timeout can't offer "Wait longer" against a container we're about to remove. @@ -842,25 +807,17 @@ export class QuickStartServiceImpl { // The port was free at the pre-check but taken while the image downloaded // (M5). Say so in the same words as the pre-check instead of leaking the // raw daemon string; the user re-picks the port in Configure. - message = portInUseMessage(chosenPort); + message = { key: 'portInUse', port: chosenPort }; portTaken = true; } else if (dockerReadiness) { - message = l10n.t('Docker became unavailable during setup: {0}', message); + message = { key: 'dockerUnavailableDuringSetup', detail }; } this.setStatus(alias, InstanceState.Error, undefined, message); } // Buffered and emitted after `finally` (like the timeout event) so a Retry click // driven by this event can't race the still-set `provisioning` guard either // (opus-4.7). On unsubscribe/return() the post-finally yield is simply skipped. - terminalEvent = stageEvent( - 'error', - 'error', - message, - aborted ? undefined : message, - undefined, - undefined, - dockerReadiness, - ); + terminalEvent = stageEvent('error', 'error', message, undefined, undefined, dockerReadiness); } } finally { // Stop the followLogs stream (started with cts.token). Disposing alone @@ -1024,8 +981,7 @@ export class QuickStartServiceImpl { public async *resumeReadiness(signal: AbortSignal, alias: string = DEFAULT_ALIAS): AsyncGenerator { const pending = this.stateFor(alias).pendingReadiness; if (!pending) { - const nothingToResume = l10n.t('There is nothing to resume.'); - yield stageEvent('error', 'error', nothingToResume, nothingToResume); + yield stageEvent('error', 'error', { key: 'nothingToResume' }); return; } if (this.stateFor(alias).provisioning || this.stateFor(alias).lifecycleBusy) { @@ -1033,11 +989,7 @@ export class QuickStartServiceImpl { // observe). Carry the timed-out affordance so the webview keeps the Wait longer / Start // over view instead of flipping to the generic error (opus-4.8) — the container and // `pendingReadiness` are still retained. - // `error` is what the webview renders (it takes precedence over `message`), so it must - // carry the same localized sentence — a bare "in progress" marker reached the message - // bar verbatim and untranslated (#852). - const alreadyRunning = l10n.t('A setup operation is already in progress.'); - yield stageEvent('error', 'error', alreadyRunning, alreadyRunning, undefined, true); + yield stageEvent('error', 'error', { key: 'setupAlreadyInProgress' }, undefined, true); return; } this.stateFor(alias).provisioning = true; @@ -1054,7 +1006,7 @@ export class QuickStartServiceImpl { let resumeResult: 'success' | 'timeout' | 'cancelled' | 'error' = 'error'; try { this.setStatus(alias, InstanceState.Provisioning, undefined, undefined); - yield stageEvent('waiting', 'active', 'Waiting for DocumentDB to accept connections…'); + yield stageEvent('waiting', 'active'); // Stream the container's logs during THIS wait so "View Docker output" shows the live // startup rather than only the stale first-attempt output (opus-4.8). void this.runtime.followLogs(pending.containerId, secretVariants(pending.password), cts.token); @@ -1067,8 +1019,7 @@ export class QuickStartServiceImpl { terminalEvent = stageEvent( 'done', 'done', - l10n.t('DocumentDB Local is running on localhost:{0}.', String(pending.boundPort)), - undefined, + { key: 'instanceRunning', port: pending.boundPort }, pending.boundPort, ); } catch (error) { @@ -1081,9 +1032,9 @@ export class QuickStartServiceImpl { const isTimeout = error instanceof ReadinessTimeoutError; const timedOut = !finalized && (isTimeout || aborted); resumeResult = aborted ? 'cancelled' : isTimeout ? 'timeout' : 'error'; - const message = aborted - ? l10n.t('Still initializing. Keep waiting, view the logs, or start over.') - : errMessage(error); + const message: QuickStartMessage = aborted + ? { key: 'stillInitializing' } + : { key: 'unexpectedFailure', detail: errMessage(error) }; if (!finalized) { this.setStatus(alias, InstanceState.Error, undefined, aborted ? undefined : message); } @@ -1093,7 +1044,7 @@ export class QuickStartServiceImpl { if (!timedOut) { this.stateFor(alias).pendingReadiness = undefined; } - terminalEvent = stageEvent('waiting', 'error', message, aborted ? undefined : message, undefined, timedOut); + terminalEvent = stageEvent('waiting', 'error', message, undefined, timedOut); } finally { signal.removeEventListener('abort', onAbort); // Stop the followLogs stream (started with cts.token) before disposing. @@ -1594,12 +1545,7 @@ export class QuickStartServiceImpl { if (await this.confirmStaysRunning(id)) { this.setStatus(alias, InstanceState.Running); } else { - this.setStatus( - alias, - InstanceState.Error, - undefined, - l10n.t('The container started but exited shortly after. Check the Quick Start logs.'), - ); + this.setStatus(alias, InstanceState.Error, undefined, { key: 'startedButExited' }); } }); } @@ -1631,12 +1577,7 @@ export class QuickStartServiceImpl { if (await this.confirmStaysRunning(id)) { this.setStatus(alias, InstanceState.Running); } else { - this.setStatus( - alias, - InstanceState.Error, - undefined, - l10n.t('The container restarted but exited shortly after. Check the Quick Start logs.'), - ); + this.setStatus(alias, InstanceState.Error, undefined, { key: 'restartedButExited' }); } }); } @@ -1867,7 +1808,10 @@ export class QuickStartServiceImpl { try { return await op(); } catch (error) { - this.setStatus(alias, InstanceState.Error, undefined, errMessage(error)); + this.setStatus(alias, InstanceState.Error, undefined, { + key: 'unexpectedFailure', + detail: errMessage(error), + }); return undefined; } finally { entry.lifecycleBusy = false; @@ -2000,7 +1944,7 @@ export class QuickStartServiceImpl { getQuickStartOutputChannel().appendLine( `DocumentDB Local instance "${alias}" is present but its stored credentials are missing; surfacing as credential-unavailable (not removed).`, ); - this.setStatus(alias, InstanceState.CredentialsMissing, undefined, credentialUnavailableMessage()); + this.setStatus(alias, InstanceState.CredentialsMissing, undefined, { key: 'credentialsUnavailable' }); return {}; } @@ -2022,7 +1966,7 @@ export class QuickStartServiceImpl { entry.missing = true; entry.state = InstanceState.Stopped; entry.port = record.port; - entry.errorMessage = undefined; + entry.error = undefined; this.statusEmitter.fire(); return {}; } diff --git a/src/services/localQuickStart/quickStartMessages.ts b/src/services/localQuickStart/quickStartMessages.ts new file mode 100644 index 000000000..c637570cf --- /dev/null +++ b/src/services/localQuickStart/quickStartMessages.ts @@ -0,0 +1,63 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See License.txt in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +import * as l10n from '@vscode/l10n'; +import { type QuickStartMessage } from './quickStartTypes'; + +/** + * The wording behind every {@link QuickStartMessage}. Shared by the tree and the setup webview so + * one situation cannot end up phrased two ways. + * + * Must stay free of `vscode` imports: the webview bundle imports this module too. Every string is + * built inside the function rather than at module scope, so it resolves against whichever l10n + * bundle the calling surface loaded. + */ +export function formatQuickStartMessage(message: QuickStartMessage): string { + const detail = message.detail?.trim(); + + switch (message.key) { + case 'setupAlreadyInProgress': + return l10n.t('Setup is already in progress.'); + case 'setupCancelled': + return l10n.t('Setup was cancelled.'); + case 'credentialsUnavailable': + return l10n.t( + 'DocumentDB Local has data on disk but its saved credentials are missing, so it cannot be opened. Use "Delete Container" to remove it and start fresh (this erases the data).', + ); + case 'portInUse': + return l10n.t( + 'Port {0} is already in use. Go back to Configure to pick a different port, or free it, then try again.', + String(message.port ?? ''), + ); + case 'dockerCliMissing': + return l10n.t('Docker CLI was not found on your PATH. Install Docker and retry.'); + case 'dockerDaemonUnreachable': + return l10n.t('Docker is installed but the daemon is not reachable. Start Docker and retry.'); + case 'dockerUnavailableDuringSetup': + return detail + ? l10n.t('Docker became unavailable during setup: {0}', detail) + : l10n.t('Docker became unavailable during setup.'); + case 'readinessTimeout': + // A dev container publishes the port on its host, so "it is still starting" would be + // the wrong thing to tell someone whose port is simply not routed. + return message.environment === 'devContainer' + ? l10n.t( + 'DocumentDB did not accept connections in time. Docker may be running on the dev container host, so the published localhost port might not be reachable from inside the dev container.', + ) + : l10n.t('DocumentDB did not accept connections in time. It may still be initializing.'); + case 'stillInitializing': + return l10n.t('Still initializing. Keep waiting, view the logs, or start over.'); + case 'instanceRunning': + return l10n.t('DocumentDB Local is running on localhost:{0}.', String(message.port ?? '')); + case 'nothingToResume': + return l10n.t('There is nothing to resume.'); + case 'startedButExited': + return l10n.t('The container started but exited shortly after. Check the Quick Start logs.'); + case 'restartedButExited': + return l10n.t('The container restarted but exited shortly after. Check the Quick Start logs.'); + default: + return detail ?? l10n.t('Setup failed.'); + } +} diff --git a/src/services/localQuickStart/quickStartTypes.ts b/src/services/localQuickStart/quickStartTypes.ts index 20734aec1..0cd0b49e0 100644 --- a/src/services/localQuickStart/quickStartTypes.ts +++ b/src/services/localQuickStart/quickStartTypes.ts @@ -146,12 +146,49 @@ export const PROVISION_STAGES: readonly ProvisionStage[] = [ 'waiting', ] as const; +/** + * What a Quick Start message says, without saying it. The service reports the situation; the + * surfaces that render it own the wording — the same split the Docker guidance keys already use. + */ +export type QuickStartMessageKey = + | 'setupAlreadyInProgress' + | 'setupCancelled' + | 'credentialsUnavailable' + | 'portInUse' + | 'dockerCliMissing' + | 'dockerDaemonUnreachable' + | 'dockerUnavailableDuringSetup' + | 'readinessTimeout' + | 'instanceRunning' + | 'nothingToResume' + | 'stillInitializing' + | 'startedButExited' + | 'restartedButExited' + | 'unexpectedFailure'; + +/** + * A situation plus the data needed to phrase it. `detail` is the one field that is never + * translated: it carries raw daemon or driver text, which is evidence rather than copy. + */ +export interface QuickStartMessage { + readonly key: QuickStartMessageKey; + /** Host port, for the keys that name one. */ + readonly port?: number; + /** Host environment, for `readinessTimeout`, whose guidance differs per platform. */ + readonly environment?: DockerHostEnvironment; + /** Raw daemon / driver text, rendered verbatim beside the localized copy. */ + readonly detail?: string; +} + /** A single stage transition pushed through the service-level event sink (D13). */ export interface StageEvent { readonly stage: ProvisionStage; readonly status: 'active' | 'done' | 'error'; - readonly message?: string; - readonly error?: string; + /** + * Only carried by terminal events. Intermediate stages are labelled by the surface from + * {@link ProvisionStage}, so they need no payload. + */ + readonly message?: QuickStartMessage; /** The actual bound host port — set on the terminal `done` event (for success guidance). */ readonly boundPort?: number; /** @@ -327,7 +364,7 @@ export type DockerReadiness = DockerReadyReadiness | DockerDiagnosedReadiness | export interface QuickStartStatus { readonly state: InstanceState; readonly metadata?: InstanceMetadata; - readonly errorMessage?: string; + readonly error?: QuickStartMessage; /** * `Missing` badge (design §6.1): the extension holds metadata but Docker has * no matching container (e.g. the user removed it outside the extension). @@ -357,7 +394,7 @@ export interface InstanceStatus { readonly state: InstanceState; readonly missing: boolean; readonly port?: number; - readonly errorMessage?: string; + readonly error?: QuickStartMessage; readonly canResumeReadiness: boolean; readonly metadata?: InstanceMetadata; } diff --git a/src/tree/connections-view/LocalQuickStart/LocalQuickStartItem.ts b/src/tree/connections-view/LocalQuickStart/LocalQuickStartItem.ts index 39035c40e..9de7fb9f5 100644 --- a/src/tree/connections-view/LocalQuickStart/LocalQuickStartItem.ts +++ b/src/tree/connections-view/LocalQuickStart/LocalQuickStartItem.ts @@ -20,6 +20,7 @@ import { Views } from '../../../documentdb/Views'; import { DocumentDBExperience } from '../../../DocumentDBExperiences'; import { ext } from '../../../extensionVariables'; import { StorageZone } from '../../../services/connectionStorageService'; +import { formatQuickStartMessage } from '../../../services/localQuickStart/quickStartMessages'; import { QuickStartService, type QuickStartConnectionPreflightResult, @@ -486,7 +487,7 @@ export class LocalQuickStartItem implements TreeElement, TreeElementWithContextV return [ row( 'state_error', - status.errorMessage ?? l10n.t('Error · click for details'), + status.error ? formatQuickStartMessage(status.error) : l10n.t('Error · click for details'), new vscode.ThemeIcon('warning', new vscode.ThemeColor('list.errorForeground')), ), ...this.createErrorRecoveryChildren(true), diff --git a/src/webviews/documentdb/localQuickStart/LocalQuickStart.tsx b/src/webviews/documentdb/localQuickStart/LocalQuickStart.tsx index 8efe4fa46..0dfc9e02c 100644 --- a/src/webviews/documentdb/localQuickStart/LocalQuickStart.tsx +++ b/src/webviews/documentdb/localQuickStart/LocalQuickStart.tsx @@ -50,6 +50,7 @@ import { import { Collapse } from '@fluentui/react-motion-components-preview'; import * as l10n from '@vscode/l10n'; import { Fragment, type JSX, type ReactNode, useCallback, useEffect, useMemo, useRef, useState } from 'react'; +import { formatQuickStartMessage } from '../../../services/localQuickStart/quickStartMessages'; import { type AdvancedQuickStartOptions, type DockerEndpointKind, @@ -1316,7 +1317,7 @@ export const LocalQuickStart = (): JSX.Element => { settled = true; stopTimer(); setStageStatus((prev) => ({ ...prev, [event.stage]: event.status })); - setSuccessMessage(event.message); + setSuccessMessage(event.message && formatQuickStartMessage(event.message)); setPhase('success'); } else if (event.status === 'error') { settled = true; @@ -1330,7 +1331,9 @@ export const LocalQuickStart = (): JSX.Element => { if (active) next[active] = 'error'; return next; }); - setErrorMessage(event.error ?? event.message ?? l10n.t('Setup failed.')); + setErrorMessage( + event.message ? formatQuickStartMessage(event.message) : l10n.t('Setup failed.'), + ); setTimedOut(event.timedOut === true); if (event.dockerReadiness) { // Docker became unusable mid-run: the remediation belongs beside the diff --git a/src/webviews/documentdb/localQuickStart/localQuickStartRouter.ts b/src/webviews/documentdb/localQuickStart/localQuickStartRouter.ts index 31c1210ea..4b494777a 100644 --- a/src/webviews/documentdb/localQuickStart/localQuickStartRouter.ts +++ b/src/webviews/documentdb/localQuickStart/localQuickStartRouter.ts @@ -107,7 +107,7 @@ export type RouterContext = BaseRouterContext & { function toWebviewStatus(status: QuickStartStatus): QuickStartStatus { return { state: status.state, - errorMessage: status.errorMessage, + error: status.error, missing: status.missing, canResumeReadiness: status.canResumeReadiness, }; From 6190ce69b4ebc4ce51795a100298f168c938d806 Mon Sep 17 00:00:00 2001 From: Tomasz Naumowicz Date: Mon, 10 Aug 2026 08:52:43 +0200 Subject: [PATCH 2/2] Address review: typed key for a repeat resume timeout, keep copy around detail MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two findings from the #879 review, both real. A second readiness timeout in `resumeReadiness` fell through to `unexpectedFailure` and put the raw `ReadinessTimeoutError` text on screen — the one thing this PR set out to stop. It now reports `readinessTimeout` carrying the host environment, so a repeat timeout gets the same dev-container port-routing explanation as the first one; `stillInitializing` stays for a cancelled wait and `unexpectedFailure` for a genuine finalize error. `formatQuickStartMessage` trimmed `detail` and then returned it through `??`, so a whitespace-only detail rendered as an empty message. Detail now collapses to undefined when it carries no evidence, and `unexpectedFailure` keeps a localized sentence around the raw text instead of replacing the copy with it — which is what the field was documented to do. Covered by a new test for the formatter: every key renders something, raw driver text never stands alone, and whitespace-only detail can never blank the message. --- l10n/bundle.l10n.json | 1 + .../localQuickStart/QuickStartService.ts | 6 +- .../quickStartMessages.test.ts | 64 +++++++++++++++++++ .../localQuickStart/quickStartMessages.ts | 9 ++- 4 files changed, 77 insertions(+), 3 deletions(-) create mode 100644 src/services/localQuickStart/quickStartMessages.test.ts diff --git a/l10n/bundle.l10n.json b/l10n/bundle.l10n.json index 7b9c362de..e30867912 100644 --- a/l10n/bundle.l10n.json +++ b/l10n/bundle.l10n.json @@ -1796,6 +1796,7 @@ "Settings:": "Settings:", "Setup did not finish": "Setup did not finish", "Setup did not finish. {0}": "Setup did not finish. {0}", + "Setup failed: {0}": "Setup failed: {0}", "Setup failed.": "Setup failed.", "Setup is already in progress.": "Setup is already in progress.", "Setup progress": "Setup progress", diff --git a/src/services/localQuickStart/QuickStartService.ts b/src/services/localQuickStart/QuickStartService.ts index f3872e7dd..fb0253d4f 100644 --- a/src/services/localQuickStart/QuickStartService.ts +++ b/src/services/localQuickStart/QuickStartService.ts @@ -1032,9 +1032,13 @@ export class QuickStartServiceImpl { const isTimeout = error instanceof ReadinessTimeoutError; const timedOut = !finalized && (isTimeout || aborted); resumeResult = aborted ? 'cancelled' : isTimeout ? 'timeout' : 'error'; + // A repeat timeout is the same situation as the first one, so it earns the same + // environment-aware explanation rather than the raw probe error. const message: QuickStartMessage = aborted ? { key: 'stillInitializing' } - : { key: 'unexpectedFailure', detail: errMessage(error) }; + : isTimeout + ? { key: 'readinessTimeout', environment: this.dockerReadiness?.environment } + : { key: 'unexpectedFailure', detail: errMessage(error) }; if (!finalized) { this.setStatus(alias, InstanceState.Error, undefined, aborted ? undefined : message); } diff --git a/src/services/localQuickStart/quickStartMessages.test.ts b/src/services/localQuickStart/quickStartMessages.test.ts new file mode 100644 index 000000000..6fdc8f80b --- /dev/null +++ b/src/services/localQuickStart/quickStartMessages.test.ts @@ -0,0 +1,64 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See License.txt in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +import { formatQuickStartMessage } from './quickStartMessages'; +import { type QuickStartMessageKey } from './quickStartTypes'; + +/** + * The service reports situations; this module owns the words. A key that renders blank, or that + * renders only untranslated driver text, is the failure mode worth guarding — the reader is left + * with either nothing or an English fragment with no sentence around it. + */ +describe('formatQuickStartMessage', () => { + /** Guards the union itself: a new key with no copy behind it fails here rather than in the UI. */ + const allKeys: QuickStartMessageKey[] = [ + 'setupAlreadyInProgress', + 'setupCancelled', + 'credentialsUnavailable', + 'portInUse', + 'dockerCliMissing', + 'dockerDaemonUnreachable', + 'dockerUnavailableDuringSetup', + 'readinessTimeout', + 'instanceRunning', + 'nothingToResume', + 'stillInitializing', + 'startedButExited', + 'restartedButExited', + 'unexpectedFailure', + ]; + + it.each(allKeys)('renders %s as a non-empty sentence', (key) => { + expect(formatQuickStartMessage({ key }).trim()).not.toBe(''); + }); + + it('keeps a localized sentence around raw driver text', () => { + const rendered = formatQuickStartMessage({ key: 'unexpectedFailure', detail: 'manifest unknown' }); + + expect(rendered).toContain('manifest unknown'); + // `detail` is evidence, not copy: on its own it leaves a non-English reader with nothing. + expect(rendered).not.toBe('manifest unknown'); + }); + + // `detail?.trim()` used to leave an empty string, which `??` happily returned as the message. + it.each(['', ' ', '\n\t'])('never renders blank for whitespace-only detail (%j)', (detail) => { + expect(formatQuickStartMessage({ key: 'unexpectedFailure', detail }).trim()).not.toBe(''); + expect(formatQuickStartMessage({ key: 'dockerUnavailableDuringSetup', detail }).trim()).not.toBe(''); + }); + + it('explains the published-port routing only inside a dev container', () => { + expect(formatQuickStartMessage({ key: 'readinessTimeout', environment: 'devContainer' })).toContain( + 'published localhost port might not be reachable from inside the dev container', + ); + expect(formatQuickStartMessage({ key: 'readinessTimeout', environment: 'linux' })).toBe( + 'DocumentDB did not accept connections in time. It may still be initializing.', + ); + }); + + it('names the port it is talking about', () => { + expect(formatQuickStartMessage({ key: 'portInUse', port: 10333 })).toContain('10333'); + expect(formatQuickStartMessage({ key: 'instanceRunning', port: 10333 })).toContain('10333'); + }); +}); diff --git a/src/services/localQuickStart/quickStartMessages.ts b/src/services/localQuickStart/quickStartMessages.ts index c637570cf..02d6bcc80 100644 --- a/src/services/localQuickStart/quickStartMessages.ts +++ b/src/services/localQuickStart/quickStartMessages.ts @@ -15,7 +15,9 @@ import { type QuickStartMessage } from './quickStartTypes'; * bundle the calling surface loaded. */ export function formatQuickStartMessage(message: QuickStartMessage): string { - const detail = message.detail?.trim(); + // Whitespace-only detail is no evidence at all; collapsing it here keeps every branch below + // from having to decide what an empty string means. + const detail = message.detail?.trim() || undefined; switch (message.key) { case 'setupAlreadyInProgress': @@ -57,7 +59,10 @@ export function formatQuickStartMessage(message: QuickStartMessage): string { return l10n.t('The container started but exited shortly after. Check the Quick Start logs.'); case 'restartedButExited': return l10n.t('The container restarted but exited shortly after. Check the Quick Start logs.'); + case 'unexpectedFailure': default: - return detail ?? l10n.t('Setup failed.'); + // Never return the raw text alone: it is English, and on its own it leaves a reader + // with no translated sentence telling them what it is about. + return detail ? l10n.t('Setup failed: {0}', detail) : l10n.t('Setup failed.'); } }