diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index d1c1386de..dfdab3fd0 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -194,6 +194,7 @@ See `src/tree/models/BaseClusterModel.ts` and `docs/analysis/08-cluster-model-si - [skills/tree-cluster-architecture/SKILL.md](skills/tree-cluster-architecture/SKILL.md) - Required patterns for cluster tree items, dual identity, provider lookup, and regression tests - [skills/telemetry-instrumentation/SKILL.md](skills/telemetry-instrumentation/SKILL.md) - Telemetry instrumentation patterns +- [skills/error-translation/SKILL.md](skills/error-translation/SKILL.md) - Turning infrastructure failures into actionable messages; providers translate, they never show UI ## Terminology diff --git a/.github/skills/error-translation/SKILL.md b/.github/skills/error-translation/SKILL.md new file mode 100644 index 000000000..d2c0bc8a7 --- /dev/null +++ b/.github/skills/error-translation/SKILL.md @@ -0,0 +1,187 @@ +--- +name: error-translation +description: How infrastructure-caused database failures are turned into messages users can act on, via ConnectionDiagnosticsService. Use when adding a discovery plugin, a connection source, a reachability provider, a new tree view or webview surface, when a user reports a confusing or raw driver error, or when asked to improve error messages for Docker, Kubernetes, Atlas, Azure or any other infrastructure-backed connection. +--- + +# Error Translation + +A connection can fail for reasons that have nothing to do with the database: a container was +stopped, a port-forward tunnel died, a service closed the TLS handshake. The driver reports these +as `ECONNREFUSED`, a server-selection timeout, or an OpenSSL alert, none of which tell the user +what to do. + +`ConnectionDiagnosticsService` lets the source that owns the infrastructure explain the failure in +its own words. + +## The one rule + +> **Providers translate. They never show UI and never recover.** + +No dialogs, no notifications, no progress bars, no starting or restarting anything, no retries, no +prompts. A provider receives an error and returns text. + +This is not a style preference. One user action often runs several database commands, several +actions can fail at the same time, and many calls happen on background paths that show nothing. A +provider that showed UI or repaired state would produce duplicate dialogs, dialogs nobody asked +for, and errors that are already obsolete by the time they appear. + +Anything with a side effect belongs at the call site, which alone knows whether the user is +watching, whether the operation was a read or a write, and which surface is right. + +## Adding a provider + +Implement `ConnectionDiagnosticsProvider` and register it in +[ClustersExtension.registerDiscoveryServices](../../../src/documentdb/ClustersExtension.ts), +next to the existing ones. + +```ts +export class MyDiagnosticsProvider implements ConnectionDiagnosticsProvider { + public readonly id = 'my-source'; + + public async explain({ clusterId, error }: ConnectionDiagnosticsRequest): Promise { + if (!isMine(clusterId)) { + return undefined; // not ours: the caller shows the original error + } + if (!looksLikeMyFailure(error)) { + return undefined; // ours, but nothing wrong on our side + } + return l10n.t('...'); + } +} +``` + +`undefined` is always the safe answer: it means "show the original error". + +A provider may answer without inspecting the error at all. Cancellations are therefore filtered +centrally: `explain()` returns `undefined` for a `UserCancelledError` before any provider is asked, +so a wizard the user escaped is never reported as an infrastructure failure. + +### Answer the cheap question first + +`explain()` runs on every foreground failure across the whole extension, so the common case must +cost almost nothing. Order the checks so the fastest rejection happens first. + +- `QuickStartDiagnosticsProvider` scans an in-memory list of managed instances before touching Docker. +- `AtlasDiagnosticsProvider` tests the error message shape before looking up any credentials. +- `KubernetesDiagnosticsProvider` checks its in-memory map before importing the tunnel machinery. + +### Do not cache verdicts + +`explain()` runs once per user-initiated failure, so caching buys nothing and costs correctness: a +memoized "not running" would still be reported right after the user starts the container and +retries. Probe fresh every time. + +If a provider ever does become expensive enough to matter, share in-flight work rather than +caching results, so a repeat attempt still sees the current state. + +### Knowing whether a cluster is yours + +`clusterId` is the only identity that reaches every call site. Stored connection properties do not +travel past the tree item, so a webview, the shell and the playground cannot read them. Pick +whichever of these fits your source: + +| Approach | Example | When | +| --- | --- | --- | +| Look it up in state you already keep | Quick Start reads `listStatuses()` | Your source has a live registry | +| Inspect the connection string | Atlas checks the `mongodb.net` host suffix | The endpoint identifies the source | +| Record it while preparing the connection | Kubernetes remembers `clusterId` in `ensureReachable` | Only the stored properties identify the source | + +The third case is why `ConnectionReachabilityProvider.ensureReachable` takes an optional +`clusterId`: that call is the one moment where both halves are known. + +## Never touch the error + +`explain()` returns text. It does not modify, replace, or attach properties to the error, and +neither should you. A lot of code here inspects errors by identity rather than by text: + +- `instanceof UserCancelledError` decides failure versus cancellation, in roughly 25 places; +- `instanceof QueryError`, `MongoBulkWriteError` and `SettingsHintError` change how a failure is handled; +- `error.code` is read for server codes (115, 235) and socket codes (`ECONNRESET`, `ENOTFOUND`); +- `errorCodeExtractor.ts` reads `error.cause.cause.code` at a **fixed depth**, so an extra wrapper level breaks Collection view error-code detection; +- `extractErrorCode()` parses a `[CODE-12345]` prefix from the **start** of a message, so prepending text breaks the shell and the playground; +- the tRPC boundary rebuilds errors as `{ code, name, message, stack, cause }`, so a custom property never reaches a webview anyway. + +Leave the error alone and none of this can break. + +## Adding a call site + +Call `explain()` where you are about to **render** a failure, then show its message instead of the +raw one and keep the raw text as detail. Rethrow the original error unchanged so telemetry and +every downstream check keep working. + +```ts +const diagnosis = await ConnectionDiagnosticsService.explain({ clusterId, error }); +void vscode.window.showErrorMessage(diagnosis?.message ?? l10n.t('Failed to load ...'), { + modal: true, + detail: error instanceof Error ? error.message : String(error), +}); +``` + +Existing call sites: + +| Surface | File | +| --- | --- | +| Every tree view, below a cluster | [BaseExtendedTreeDataProvider.ts](../../../src/tree/BaseExtendedTreeDataProvider.ts) | +| Cluster connect and list databases | [ClusterItemBase.ts](../../../src/tree/documentdb/ClusterItemBase.ts) | +| Shell connect banner | [DocumentDBShellPty.ts](../../../src/documentdb/shell/DocumentDBShellPty.ts) | +| Query playground | [executePlaygroundCode.ts](../../../src/commands/playground/executePlaygroundCode.ts) | +| Tree-node commands (create, drop, …) | [commandErrorHandling.ts](../../../src/utils/commandErrorHandling.ts) | +| Any webview, via `common.explainOperationFailure` | [appRouter.ts](../../../src/webviews/_integration/appRouter.ts) | + +Tree views need no per-view wiring: `wrapGetChildrenWithErrorAndStateHandling` translates on the way +out, so any provider built on the base class is covered. Commands registered with +`registerCommandWithTreeNodeUnwrappingAndModalErrors` are covered the same way, via the tree node +they receive. + +### Do not call it from background paths + +Background work shows nothing, so translating there costs I/O for no benefit. Leave these alone: +collection and document count badges, index count badges, the Collection view document count, and +the Query Insights stage 1 prefetch. They already swallow their errors on purpose. + +### Webviews + +An explanation cannot ride along on an error across the tRPC boundary, so a webview asks for one: + +```tsx +.catch(async (error) => { + const cause = error instanceof Error ? error.message : String(error); + const explained = await trpcClient.common.explainOperationFailure.query({ message: cause }); + void trpcClient.common.displayErrorMessage.mutate({ + message: explained ?? l10n.t('Error while running the query'), + modal: true, + cause, + }); +}); +``` + +`explainOperationFailure` reads the `clusterId` from the webview's tRPC context and returns `null` +when nothing applies. Only the error MESSAGE crosses the boundary, so a provider that needs an +error's class or `code` cannot be served this way. Never wrap an error to smuggle text through. + +## Writing the message + +- Do not assert what happened. Say "we cannot find", "very likely", "does not appear to be". +- "We" is fine and is the established voice. +- Say what the user can do next, and where. +- Keep it to one paragraph. The message becomes the heading of a modal, where a bulleted block + renders as several lines of bold text; the raw driver error already occupies the detail area. + `AtlasDiagnosticsProvider` returns `summarizeAtlasTlsHandshakeRejection()` for this reason, while + the Discovery-view modal keeps the longer `describeAtlasTlsHandshakeRejection()` as its detail. +- No em dashes or en dashes. +- Wrap every string in `l10n.t()` and run `npm run l10n`. + +```ts +// Good +l10n.t('We cannot find the DocumentDB Local container. It was very likely removed outside VS Code. You can recreate it from the Connections view, which reuses the existing data volume.') + +// Bad: asserts a cause, and offers no next step +l10n.t('The container was removed outside VS Code.') +``` + +## Related + +- [connectionDiagnosticsService.ts](../../../src/services/connectionDiagnosticsService.ts) +- [connectionReachabilityService.ts](../../../src/services/connectionReachabilityService.ts) prepares a connection *before* connecting; this service explains a failure *afterwards* +- [tree-cluster-architecture](../tree-cluster-architecture/SKILL.md) for `clusterId` versus `treeId` +- [telemetry-instrumentation](../telemetry-instrumentation/SKILL.md) diff --git a/docs/ai-and-plans/PRs/876-quickstart-error-translation-review.md b/docs/ai-and-plans/PRs/876-quickstart-error-translation-review.md new file mode 100644 index 000000000..3952f0b2d --- /dev/null +++ b/docs/ai-and-plans/PRs/876-quickstart-error-translation-review.md @@ -0,0 +1,328 @@ +# PR #876 review — "Keep DocumentDB Local state in sync and explain infrastructure-caused failures" + +- Base: `release/0.10.0`, head: `dev/tnaum/quickstart-improvements` +- Scope reviewed: 44 files, +2186/-1017 (diff against the PR base, not `main`) +- Reviewer: agent-assisted code review, 2026-08-09 + +## Resolution status + +All High and Medium findings were fixed on this branch, one commit each, plus the low items that +were genuine defects. + +| Finding | Status | Commit subject | +| --- | --- | --- | +| H1 | Fixed | `fix(quickstart): keep Quick Start reachable when Docker is unavailable` | +| H2 | Fixed | `fix(diagnostics): never translate a cancellation into an infrastructure failure` | +| M1 | Fixed | `fix(quickstart): tell a stopped Docker daemon apart from a removed container` | +| M2 | Fixed | `refactor(quickstart): give diagnostics a genuinely read-only preflight` | +| M3 | Fixed | `fix(atlas): keep the TLS diagnosis to one paragraph` | +| M4 | Fixed | `fix(tree): stop dropping the raw error on the non-modal diagnosis path` | +| M5 | Fixed | `fix(shell): redact cached credentials before logging a connect failure` | +| M6, M7 | Fixed | `fix(quickstart): answer a failed preflight with tree rows, not a modal` | +| M8 | Fixed | `perf(diagnostics): budget the whole explain call, not each provider` | +| L2 | Fixed | folded into the M6/M7 commit (the prompt singleton is gone) | +| L3, L8 | Fixed | `fix(quickstart): show display labels in the managed-instance tooltip` | +| L4 | Fixed | `fix(commands): keep argument unwrapping inside the guarded block` | +| L5 | Fixed | `docs(diagnostics): note that the error is a bare string on the webview path` | +| L7 | Fixed | `fix(quickstart): show progress during an explicit deep refresh` | +| M9 | Fixed | `fix(quickstart): stop re-inspecting the container hydration just adopted` | +| L1 | Open, by choice | Collapsing the root is what makes hydration lazy. Left as a UX decision to confirm, not a defect. | +| L6 | Open, by choice | Deliberate: the provider's premise is that the error shape does not matter. One `docker inspect` per foreground failure is the accepted cost. | +| L9 | Verified | The removed `running` / `stopped` strings have no remaining callers. | + +The test gaps listed at the end are covered by the commits above, except the ones tied to L1 and L6. + +### M9 — the first expansion re-inspects the container it just adopted + +Found while walking the first-run render sequence, after the original review. + +`setStatus()` fires the status emitter unconditionally, and the subscriber in +[ClustersExtension.ts](src/documentdb/ClustersExtension.ts) refreshes the whole Connections tree. So +`adoptContainer()` during hydration queues a tree refresh, which re-enters `getChildren()` *after* +`hydrated` has flipped to `true`. That call therefore captures `wasHydrated === true` and starts +`refreshLiveStateInBackground()`. Since `lastBackgroundRefreshAt` was still `0`, the 5 s cooldown +did not block it. + +Visible effect: on the first expansion the row flashes +`Running · localhost:10260 · Refreshing…` for the duration of one `docker inspect`, and the probe's +`finally` then fires the emitter unconditionally for a second full-tree refresh. + +The `wasHydrated` guard was meant to prevent this, but it only covers the `getChildren()` call that +*triggered* hydration, not the status-event-driven re-render that follows it. +`refreshHydratedState()` already armed the cooldown for exactly this reason (pinned by +`does not start a background live-state probe immediately after explicit refresh`); +`ensureHydrated()` now does the same. + +First expansion of a running instance, before and after: + +| | Before | After | +| --- | --- | --- | +| Docker calls to render the row | 4 | 3 | +| Full-tree refreshes | 2 | 1 | +| `· Refreshing…` flash | yes | no | + +## Summary + +Two independent changes ride in one PR: + +1. **Quick Start state accuracy** — activation-time `reconcile()` becomes demand-driven + `ensureHydrated()`, the root row is now collapsed, expanding the managed cluster preflights the + container, and the tooltip carries Docker host facts. +2. **`ConnectionDiagnosticsService`** — a translation-only provider registry wired into the tree + base class, `ClusterItemBase`, the shell, the playground, tree-node commands and (via + `common.explainOperationFailure`) every webview. + +The second half is well designed: the "providers translate, never act" rule is stated in the code, +in the skill, and enforced by tests; the "never touch the error" analysis is correct and the +identity-check inventory is accurate. The registry, the deadline, the throwing-provider skip and +the untouched-error guarantee are all covered by tests. + +The first half is where the risk sits. Making hydration lazy also made it **fatal**, and the +preflight cannot tell "container removed" from "Docker is not running". + +Findings below are ordered by severity. Line references are to the head of the branch. + +--- + +## High + +### H1 — Quick Start becomes unreachable when Docker is absent or stopped + +`performReconciliation()` dropped both safety nets that the old `reconcile()` had: + +- the outer `try { … } catch { /* best-effort; never block activation */ }` +- the per-call `listByLabel(…).catch(() => [])` + +`ContainerRuntime.listByLabel` has no internal error handling (unlike `inspectContainer`), so with +no `docker` binary, or a stopped daemon, it rejects. That rejection now propagates through +`reconcile()` → `ensureHydrated()` into two unguarded call sites: + +- [src/tree/connections-view/LocalQuickStart/LocalQuickStartItem.ts](src/tree/connections-view/LocalQuickStart/LocalQuickStartItem.ts) — `getChildren()` awaits `ensureHydrated()` with no `try`. `ConnectionsBranchDataProvider.getChildren` runs inside `callWithTelemetryAndErrorHandling`, so the user gets an error toast and an **empty** Quick Start node. +- [src/commands/localQuickStart/openLocalQuickStart.ts](src/commands/localQuickStart/openLocalQuickStart.ts) — the command awaits `ensureHydrated()` **before** `openLocalQuickStartWebview()`, so the webview never opens. + +Net effect: the users who most need Quick Start (no Docker yet) lose both entry points into it. +The existing test `ensureHydrated() remains retryable when Docker discovery fails` pins the +rejection as intended service behaviour, so this has to be fixed at the call sites. + +**Suggested fix.** Keep `ensureHydrated()` rejecting (retry semantics are good), but make both +consumers tolerant: + +```ts +// LocalQuickStartItem.getChildren +try { + await QuickStartService.ensureHydrated(); +} catch { + // Docker may not be installed yet; render the NotInstalled row so Quick Start stays reachable. +} +``` + +```ts +// openLocalQuickStart — the webview is the place that diagnoses Docker, so never gate it on Docker +await QuickStartService.ensureHydrated().catch(() => undefined); +``` + +Add regression tests for both (neither path is covered today). + +### H2 — `UserCancelledError` gets translated into a modal "DocumentDB Local is not running" + +[src/utils/commandErrorHandling.ts](src/utils/commandErrorHandling.ts) calls `explain()` for every +error that is not a `UserFacingError`. `UserCancelledError` is not filtered. + +Two providers do not look at the error at all before answering: + +- `QuickStartDiagnosticsProvider.explain()` deliberately ignores the error shape ("The error shape + does not matter here"). +- `KubernetesDiagnosticsProvider.explain()` ignores it whenever the tunnel is down. + +So: user opens *Create Database* on a Quick Start cluster whose container is stopped, presses Esc → +`UserCancelledError` → **modal** dialog "DocumentDB Local does not appear to be running." The same +applies to `fetchChildrenWithDiagnostics` in +[src/tree/BaseExtendedTreeDataProvider.ts](src/tree/BaseExtendedTreeDataProvider.ts). + +**Suggested fix.** One central guard, since the PR's own thesis is "there is exactly one rule to +remember": + +```ts +// connectionDiagnosticsService.ts +public async explain(request: ConnectionDiagnosticsRequest): Promise { + // A cancelled operation is not a failure; nothing to explain. + if (request.error instanceof UserCancelledError) { + return undefined; + } + … +``` + +This also protects future call sites and belongs in the skill's "Adding a call site" section. + +--- + +## Medium + +### M1 — "The container was very likely removed" is asserted when Docker itself is down + +`ContainerRuntime.inspectContainer` swallows **every** failure and returns `undefined`. So when the +daemon is stopped, `QuickStartServiceImpl.prepareForConnection` sees `!inspected`, sets +`entry.missing = true`, and returns `'missing'`. The provider then says: + +> We cannot find the DocumentDB Local container. It was very likely removed outside VS Code. You +> can recreate it from the Connections view, which reuses the existing data volume. + +That is exactly the assertion the PR's own message-style section forbids, and the suggested +recovery (recreate) is the wrong action. Stopping Docker Desktop is at least as common as removing +a container by hand. + +**Suggested fix.** Distinguish the two before concluding `missing`, e.g. let `inspectContainer` +report "not found" separately from "could not ask" (or consult +`QuickStartService.getDockerReadinessSnapshot()` / a cheap `isDockerReady()` on the `!inspected` +branch) and map daemon-unreachable to the existing `'unavailable'` wording. + +### M2 — Providers mutate state and fire the status emitter, contradicting the stated contract + +`QuickStartDiagnosticsProvider.explain()` → `prepareForConnection()` → `setStatus()` / +`entry.missing = true` / `statusEmitter.fire()` → `ext.connectionsBranchDataProvider.refresh()`. + +The service header and the skill both say providers "never repair state" and "never show UI". A +tree redraw triggered from a translation call is an observable UI side effect, and on a background +failure it would repaint the tree for a user who is not watching. The `silent: true` option is a +signal that `prepareForConnection` is not really a read-only probe. + +**Suggested fix.** Either split a genuinely read-only `inspectManagedInstance()` out of +`prepareForConnection` and have the provider use that, or amend the documented rule to "no UI, no +recovery, state correction allowed" and say so explicitly in the skill. The current wording and the +implementation disagree. + +### M3 — Atlas explanation is promoted from `detail` to the modal's main message + +`describeAtlasTlsHandshakeRejection()` returns four lines including a bullet list. In +`AtlasClusterItem` it is still passed as `detail` (correct). Through the new generic path it becomes +the **message**: + +```ts +void vscode.window.showErrorMessage(diagnosis?.message ?? …, { modal: true, detail: errorMessage }); +``` + +VS Code renders `message` as the large bold heading of a modal, so the user gets a multi-paragraph +bold block and a one-line detail. In the webview non-modal path, `displayErrorMessage` concatenates +`message + " (" + cause + ")"`, producing a very long toast. + +**Suggested fix.** Make `ConnectionDiagnosis` carry `{ summary, detail? }` and let call sites place +each half correctly, or constrain provider messages to a single sentence and keep the elaboration in +`AtlasClusterItem`. + +### M4 — `detail` is silently dropped in the tree base class + +```ts +void vscode.window.showErrorMessage(diagnosis.message, { + modal: false, + detail: error instanceof Error ? error.message : String(error), +}); +``` + +`MessageOptions.detail` is only rendered for modal messages — the repo already documents this in +[src/webviews/_integration/appRouter.ts](src/webviews/_integration/appRouter.ts) ("The content of +the 'detail' field is only shown when modal is true"). Combined with +`context.errorHandling.suppressDisplay = true`, the raw driver error now disappears from this +surface entirely, which is the opposite of the PR's "keep the raw text as detail" rule. + +**Suggested fix.** Mirror `displayErrorMessage`: append the cause to the message for non-modal, or +log it to `ext.outputChannel`. + +### M5 — Raw driver text logged to a shared output channel without masking + +```ts +ext.outputChannel.error( + `[Shell] Failed to connect to "${…}": ${rawMessage}` + (diagnosis ? ` (${diagnosis.providerId}: ${diagnosis.message})` : ''), +); +``` + +The PR description explicitly positions this channel as something users share for remote diagnosis. +Driver errors can embed the connection string (`MongoParseError: Invalid connection string: +mongodb://user:pass@…`), and Quick Start credentials are auto-generated and live in that string. The +repo already has masking helpers (`maskSecrets` in `ContainerRuntime.ts`, +[src/services/localQuickStart/outputMasking.ts](src/services/localQuickStart/outputMasking.ts)). + +**Suggested fix.** Mask before logging, and add a test that a URI-with-credentials never reaches the +channel. + +### M6 — Modal dialog fired from a tree-expand gesture, and awaited inside `getChildren()` + +`offerToStartStoppedInstance()` shows `showInformationMessage(…, { modal: true }, 'Start')` and +`QuickStartClusterItem.getChildren()` awaits it. The node spins until the user answers a **modal**, +then renders empty; the Start command is fired with `void`, so nothing is shown until the status +event lands and the user expands again. + +Elsewhere the codebase uses actionable error-recovery child nodes for exactly this +(`createGenericElementWithContext` with a `commandId`), which is both non-blocking and discoverable. + +**Suggested fix.** Return a child node ("Click here to start DocumentDB Local") instead of a modal, +or at minimum make the notification non-modal and do not await it. + +### M7 — `unavailable` / `missing` / `foreign` render as a silently empty node + +`QuickStartClusterItem.getChildren()` returns `[]` for every non-`ready` verdict. Only `stopped` +produces feedback. For `missing` and `unavailable` the user expands and gets nothing — no toast, no +row, no log line. + +**Suggested fix.** Return the corresponding explanation as an error-recovery child (the provider +already has the exact wording; reuse it rather than duplicating). + +### M8 — The 5-second deadline is per provider, not per call + +`explain()` loops providers sequentially, each wrapped in its own `EXPLAIN_DEADLINE_MS` race. Three +registered providers means up to 15 s before the user sees any error message — on the connect path +that is on top of the driver's own server-selection timeout. + +**Suggested fix.** One deadline for the whole loop, or drop the per-provider budget to ~1.5 s. + +--- + +## Low / polish + +| # | Finding | Where | +| --- | --- | --- | +| L1 | Root row changed from `Expanded` to `Collapsed`. Deliberate (it is what makes hydration lazy), but on a fresh install the primary onboarding affordance now sits behind a chevron. Worth a UX sign-off, and worth calling out in the release notes. | [LocalQuickStartItem.ts](src/tree/connections-view/LocalQuickStart/LocalQuickStartItem.ts) | +| L2 | `stoppedInstancePrompt` is a module-level singleton with no alias key, and the Start command takes no alias. Harmless today (single instance), but the file elsewhere is careful about the multi-instance seam. Key it by alias. | same | +| L3 | Tooltip shows raw identifiers to users: `status.state` (`NotInstalled`, `CredentialsMissing`), `readiness.endpointKind` (`unixSocket`), `readiness.osType` (`linux`). The file already has `dockerProviderLabel` / `executionTargetLabel` for exactly this. | same | +| L4 | `unwrapArgs()` moved outside the `try`, so a throw from unwrapping now bypasses the `UserFacingError` handling. Keep it inside with a `let`. | [commandErrorHandling.ts](src/utils/commandErrorHandling.ts) | +| L5 | `explainOperationFailure` passes a bare `string` as `error`. Documented, but the parameter is typed `unknown`, so nothing stops a future provider from doing `instanceof` and silently never matching from webviews. Consider a distinct `message` field on the request. | [appRouter.ts](src/webviews/_integration/appRouter.ts) | +| L6 | Because `QuickStartDiagnosticsProvider` ignores the error, **every** webview failure on the Quick Start cluster (including a bad query) triggers a `docker inspect`. Cheap, but it contradicts "answer the cheap question first". | [QuickStartDiagnosticsProvider.ts](src/services/localQuickStart/QuickStartDiagnosticsProvider.ts) | +| L7 | `refreshHydratedState()` runs a full Docker reconciliation from a context-menu click with no progress indication and rethrows into the generic handler. Consider `withProgress` on the tree item. | [LocalQuickStartItem.ts](src/tree/connections-view/LocalQuickStart/LocalQuickStartItem.ts) | +| L8 | `escapeMarkdown` escapes `-` and `.`, so tests assert on `documentdb\-local` and `28\.1\.1`. Narrowing the character class would keep the tests readable. | same | +| L9 | Removing the "changed in another window" notification for `start()`/`stop()` drift is a good call, but the two removed l10n strings (`running`, `stopped`) suggest checking no other surface still relies on them. | [QuickStartService.ts](src/services/localQuickStart/QuickStartService.ts) | + +--- + +## Test gaps + +Existing coverage is strong for the new service and providers. Missing: + +1. `LocalQuickStartItem.getChildren()` when `ensureHydrated()` rejects (H1). +2. `openLocalQuickStart()` when `ensureHydrated()` rejects (H1). +3. `registerCommandWithTreeNodeUnwrappingAndModalErrors` with a `UserCancelledError` on a cluster + node — asserting no dialog (H2). +4. `prepareForConnection()` when the Docker daemon is unreachable — asserting it does not report + `missing` (M1). +5. Shell connect-failure logging with a credential-bearing driver message (M5). + +## What is good + +- The "never touch the error" rationale is correct and the identity-check inventory + (`errorCodeExtractor` fixed depth, `extractErrorCode` prefix parsing, tRPC error rebuild) is + accurate; the guard tests pin it. +- One catch in `BaseExtendedTreeDataProvider` covering all four views is the right seam, and the + regression test that background count paths never invoke it is exactly the test that matters. +- The shell no longer disposing the terminal on a failed connect, and reusing + `ShellSessionManager.evaluate()`'s re-initialize as the retry, is an elegant fix to a real bug. +- The `clusterId`-only identity choice, and the three documented ways a provider recognises its own + clusters, keep the design free of a central origin registry. +- `ensureHydrated()` sharing in-flight work with `refreshHydratedState()` and staying retryable + after a failure is the right shape — the problem is only that callers treat rejection as fatal. + +## Recommendation + +Request changes on **H1** and **H2** (both are user-visible regressions with small fixes), and on +**M1** (a message that asserts a wrong cause and recommends the wrong recovery, which the PR's own +style rules forbid). The rest can land as follow-ups. + +Consider splitting the Quick Start lifecycle change from the error-translation framework: they have +different blast radii, and the framework half is ready. diff --git a/l10n/bundle.l10n.json b/l10n/bundle.l10n.json index bdee2777d..40ea317d2 100644 --- a/l10n/bundle.l10n.json +++ b/l10n/bundle.l10n.json @@ -19,6 +19,7 @@ "\"{0}\" is not implemented on \"{1}\".": "\"{0}\" is not implemented on \"{1}\".", "\"{0}\" is on the network share \"\\\\{1}\", which the editor cannot read directly.": "\"{0}\" is on the network share \"\\\\{1}\", which the editor cannot read directly.", "\"{0}\" uses the unsupported URI scheme \"{1}\".": "\"{0}\" uses the unsupported URI scheme \"{1}\".", + "\"{service}\" in namespace \"{namespace}\"": "\"{service}\" in namespace \"{namespace}\"", "\"{user}\" signs in with {method}.": "\"{user}\" signs in with {method}.", "\"mongodb://\" or \"mongodb+srv://\" must be the prefix of the connection string.": "\"mongodb://\" or \"mongodb+srv://\" must be the prefix of the connection string.", "\"registerAzureUtilsExtensionVariables\" must be called before using the vscode-azext-azureutils package.": "\"registerAzureUtilsExtensionVariables\" must be called before using the vscode-azext-azureutils package.", @@ -119,7 +120,6 @@ "{0}\n\nThe query did not complete successfully. Performance metrics shown are partial and measured up to the failure point.": "{0}\n\nThe query did not complete successfully. Performance metrics shown are partial and measured up to the failure point.", "{0}\n\nTip: copy the file into the same filesystem as the editor (for example your WSL or remote home directory), then drop it again, or use the \"Add Kubeconfig Source\" command to browse for it.": "{0}\n\nTip: copy the file into the same filesystem as the editor (for example your WSL or remote home directory), then drop it again, or use the \"Add Kubeconfig Source\" command to browse for it.", "{0} · {1}": "{0} · {1}", - "{0} · Refreshing…": "{0} · Refreshing…", "{0} (Emulator)": "{0} (Emulator)", "{0} {1}": "{0} {1}", "{0} clusters": "{0} clusters", @@ -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.", @@ -403,6 +402,7 @@ "Click here to retry": "Click here to retry", "Click here to revisit credentials": "Click here to revisit credentials", "Click here to set up DocumentDB Local": "Click here to set up DocumentDB Local", + "Click here to start DocumentDB Local": "Click here to start DocumentDB Local", "Click here to update credentials": "Click here to update credentials", "Click here to view the setup log": "Click here to view the setup log", "Click to view resource": "Click to view resource", @@ -500,6 +500,9 @@ "Connects through a Kubernetes node port. This only works if a cluster node address is reachable from this machine.": "Connects through a Kubernetes node port. This only works if a cluster node address is reachable from this machine.", "Connects to the LoadBalancer external address. The connection string is portable if that address is reachable from the client machine.": "Connects to the LoadBalancer external address. The connection string is portable if that address is reachable from the client machine.", "Container host": "Container host", + "Container ID": "Container ID", + "Container image": "Container image", + "Container OS": "Container OS", "Containers run on": "Containers run on", "context unavailable": "context unavailable", "Context unavailable": "Context unavailable", @@ -583,7 +586,9 @@ "Credentials": "Credentials", "Credentials may have expired. Re-authenticate with your cluster or update the kubeconfig.": "Credentials may have expired. Re-authenticate with your cluster or update the kubeconfig.", "Credentials may have expired. Re-authenticate with your cluster.": "Credentials may have expired. Re-authenticate with your cluster.", + "Credentials missing": "Credentials missing", "Credentials updated successfully.": "Credentials updated successfully.", + "Daemon architecture": "Daemon architecture", "daemon not running": "daemon not running", "daemon starting": "daemon starting", "daemon unreachable": "daemon unreachable", @@ -652,6 +657,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", @@ -667,6 +673,8 @@ "Docker Desktop not running": "Docker Desktop not running", "Docker did not become ready before the wait timed out.": "Docker did not become ready before the wait timed out.", "Docker did not respond before the readiness check timed out.": "Docker did not respond before the readiness check timed out.", + "Docker does not appear to be running, so DocumentDB Local cannot be reached. Start Docker, then try again.": "Docker does not appear to be running, so DocumentDB Local cannot be reached. Start Docker, then try again.", + "Docker does not appear to be running. Click here for details": "Docker does not appear to be running. Click here for details", "Docker endpoint": "Docker endpoint", "Docker endpoint unreachable": "Docker endpoint unreachable", "Docker Engine": "Docker Engine", @@ -675,8 +683,10 @@ "Docker is ready now. Nothing has been created on your machine yet.": "Docker is ready now. Nothing has been created on your machine yet.", "Docker is ready. Setup has not run yet.": "Docker is ready. Setup has not run yet.", "Docker must be available in the remote environment where this extension is running.": "Docker must be available in the remote environment where this extension is running.", + "Docker provider": "Docker provider", "Docker reports this only once a connection succeeds, so it stays unknown until then.": "Docker reports this only once a connection succeeds, so it stays unknown until then.", "Docker started, but it is not usable yet. See the details below.": "Docker started, but it is not usable yet. See the details below.", + "Docker version": "Docker version", "Document actions": "Document actions", "Document already exists (skipped)": "Document already exists (skipped)", "Document Editor: Edit the document in JSON format": "Document Editor: Edit the document in JSON format", @@ -694,7 +704,9 @@ "DocumentDB Local": "DocumentDB Local", "DocumentDB Local - Quick Start": "DocumentDB Local - Quick Start", "DocumentDB Local already has data on this machine. What should setup do with it?": "DocumentDB Local already has data on this machine. What should setup do with it?", + "DocumentDB Local cannot be opened. Click here to review its setup": "DocumentDB Local cannot be opened. Click here to review its setup", "DocumentDB Local container deleted.": "DocumentDB Local container deleted.", + "DocumentDB Local does not appear to be running. Start it from the Connections view, then try again.": "DocumentDB Local does not appear to be running. Start it from the Connections view, then try again.", "DocumentDB Local gives you an open-source, fully MongoDB-compatible database for development and testing on your machine.": "DocumentDB Local gives you an open-source, fully MongoDB-compatible database for development and testing on your machine.", "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).": "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).", "DocumentDB Local images are published for x64 and arm64 only.": "DocumentDB Local images are published for x64 and arm64 only.", @@ -822,6 +834,7 @@ "Executing explain(aggregate) for collection: {collection}, pipeline stages: {stageCount}": "Executing explain(aggregate) for collection: {collection}, pipeline stages: {stageCount}", "Executing explain(count) for collection: {collection}": "Executing explain(count) for collection: {collection}", "Executing explain(find) for collection: {collection}": "Executing explain(find) for collection: {collection}", + "Execution target": "Execution target", "Execution Time": "Execution Time", "Execution timed out": "Execution timed out", "Execution timed out.": "Execution timed out.", @@ -860,6 +873,7 @@ "Failed to complete operation after {0} attempts without progress": "Failed to complete operation after {0} attempts without progress", "Failed to connect to \"{0}\"": "Failed to connect to \"{0}\"", "Failed to connect to \"{cluster}\"": "Failed to connect to \"{cluster}\"", + "Failed to connect to \"{cluster}\": {error}": "Failed to connect to \"{cluster}\": {error}", "Failed to connect to VM \"{vmName}\"": "Failed to connect to VM \"{vmName}\"", "Failed to connect: {0}": "Failed to connect: {0}", "Failed to count documents in the source collection.": "Failed to count documents in the source collection.", @@ -1243,6 +1257,7 @@ "Loading Virtual Machines…": "Loading Virtual Machines…", "Loading...": "Loading...", "Loading…": "Loading…", + "Local": "Local", "Local emulators": "Local emulators", "Local port {0} is already used by Kubernetes tunnel \"{1}/{2}\". Choose a different local port for \"{3}/{4}\".": "Local port {0} is already used by Kubernetes tunnel \"{1}/{2}\". Choose a different local port for \"{3}/{4}\".", "Local port-forward required": "Local port-forward required", @@ -1280,6 +1295,7 @@ "MongoDB Atlas asked us to slow down. Wait briefly, then try again.": "MongoDB Atlas asked us to slow down. Wait briefly, then try again.", "MongoDB Atlas blocked this request because IP address {0} isn't on the allowed access list. Add this IP address in MongoDB Atlas, then retry.": "MongoDB Atlas blocked this request because IP address {0} isn't on the allowed access list. Add this IP address in MongoDB Atlas, then retry.", "MongoDB Atlas blocked this request because your IP address isn't on the allowed access list. Add your current IP address in MongoDB Atlas, then retry.": "MongoDB Atlas blocked this request because your IP address isn't on the allowed access list. Add your current IP address in MongoDB Atlas, then retry.", + "MongoDB Atlas closed the TLS connection with an internal error. That is a transport-level rejection rather than a failed sign-in, so it is worth checking whether this machineȁs IP address is on the projectȁs IP access list, and whether the cluster is paused.": "MongoDB Atlas closed the TLS connection with an internal error. That is a transport-level rejection rather than a failed sign-in, so it is worth checking whether this machineȁs IP address is on the projectȁs IP access list, and whether the cluster is paused.", "MongoDB Atlas closed the TLS connection with an internal error. This is a transport-level failure rather than an authentication response, so it is not what an incorrect username or password looks like: those report \"bad auth : Authentication failed\".": "MongoDB Atlas closed the TLS connection with an internal error. This is a transport-level failure rather than an authentication response, so it is not what an incorrect username or password looks like: those report \"bad auth : Authentication failed\".", "MongoDB Atlas could not be reached. Check your connection or proxy settings, then try again.": "MongoDB Atlas could not be reached. Check your connection or proxy settings, then try again.", "MongoDB Atlas could not be reached. The stored credentials are most likely fine.": "MongoDB Atlas could not be reached. The stored credentials are most likely fine.", @@ -1303,6 +1319,7 @@ "N/A": "N/A", "Name": "Name", "name=\"{0}\", family={1}, id={2}, version={3}": "name=\"{0}\", family={1}, id={2}, version={3}", + "Named pipe": "Named pipe", "Namespace": "Namespace", "Namespaces that were scanned but where no DocumentDB target was found. These are grouped here to keep the list of connectable namespaces uncluttered. Expand to see which namespaces were checked.": "Namespaces that were scanned but where no DocumentDB target was found. These are grouped here to keep the list of connectable namespaces uncluttered. Expand to see which namespaces were checked.", "Needs attention · review setup": "Needs attention · review setup", @@ -1407,6 +1424,7 @@ "Not reported yet": "Not reported yet", "not running": "not running", "Not running": "Not running", + "Not set up": "Not set up", "Not signed in to {0}. Please authenticate first.": "Not signed in to {0}. Please authenticate first.", "Not supported yet": "Not supported yet", "Note: This confirmation type can be configured in the extension settings.": "Note: This confirmation type can be configured in the extension settings.", @@ -1557,7 +1575,8 @@ "Provide your MongoDB Atlas Service Account": "Provide your MongoDB Atlas Service Account", "Provider": "Provider", "Provider \"{0}\" does not have resource type \"{1}\".": "Provider \"{0}\" does not have resource type \"{1}\".", - "Provisioning… · localhost:{0}": "Provisioning… · localhost:{0}", + "Provisioning": "Provisioning", + "Provisioning…": "Provisioning…", "Public and private key pair. Never expires, which suits a personal, set-and-forget setup.": "Public and private key pair. Never expires, which suits a personal, set-and-forget setup.", "Public Key": "Public Key", "Pulling official image": "Pulling official image", @@ -1622,6 +1641,7 @@ "Refresh query and query insights": "Refresh query and query insights", "Refresh: {0}": "Refresh: {0}", "Refreshing Azure discovery tree…": "Refreshing Azure discovery tree…", + "Refreshing…": "Refreshing…", "Region": "Region", "Registering Providers...": "Registering Providers...", "Rejects duplicate values.": "Rejects duplicate values.", @@ -1632,6 +1652,7 @@ "Reloading kubeconfig source \"{0}\"…": "Reloading kubeconfig source \"{0}\"…", "Remembered from the last check on this machine that did reach a daemon.": "Remembered from the last check on this machine that did reach a daemon.", "Remind Me Later": "Remind Me Later", + "Remote": "Remote", "Remote extension host": "Remote extension host", "Remote SSH host": "Remote SSH host", "Remote SSH host (Docker)": "Remote SSH host (Docker)", @@ -1659,6 +1680,7 @@ "Resource group \"{0}\" already exists in subscription \"{1}\".": "Resource group \"{0}\" already exists in subscription \"{1}\".", "Resource not found": "Resource not found", "Resource not found.": "Resource not found.", + "Restarting…": "Restarting…", "Result: {0}": "Result: {0}", "Result: Array ({0} elements)": "Result: Array ({0} elements)", "Result: Cursor ({0} documents)": "Result: Cursor ({0} documents)", @@ -1685,12 +1707,12 @@ "Role Assignment {0} created for {1}": "Role Assignment {0} created for {1}", "Role Assignment {0} failed for {1}": "Role Assignment {0} failed for {1}", "Run": "Run", + "Run a command to try connecting again, or close this terminal.": "Run a command to try connecting again, or close this terminal.", "Run All": "Run All", "Run as Is": "Run as Is", "Run the entire file ({0}+Shift+Enter)": "Run the entire file ({0}+Shift+Enter)", "Run this block ({0}+Enter)": "Run this block ({0}+Enter)", - "running": "running", - "Running · localhost:{0}": "Running · localhost:{0}", + "Running": "Running", "Running query…": "Running query…", "Running…": "Running…", "runs in this Codespace": "runs in this Codespace", @@ -1773,6 +1795,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", @@ -1855,6 +1878,7 @@ "Start over": "Start over", "Start the Docker service, then check again.": "Start the Docker service, then check again.", "Started executable: \"{command}\". Connecting to host…": "Started executable: \"{command}\". Connecting to host…", + "Starting": "Starting", "Starting Azure account management wizard": "Starting Azure account management wizard", "Starting Azure sign-in process…": "Starting Azure sign-in process…", "Starting container": "Starting container", @@ -1864,19 +1888,18 @@ "Starting import of {0} file(s) into collection \"{1}\"": "Starting import of {0} file(s) into collection \"{1}\"", "Starting sign-in to tenant: {0}": "Starting sign-in to tenant: {0}", "Starting…": "Starting…", - "Starting… · localhost:{0}": "Starting… · localhost:{0}", "Starts with mongodb:// or mongodb+srv://": "Starts with mongodb:// or mongodb+srv://", "State": "State", "Status: {0}": "Status: {0}", "Still initializing. Keep waiting, view the logs, or start over.": "Still initializing. Keep waiting, view the logs, or start over.", "Stop waiting": "Stop waiting", - "stopped": "stopped", - "Stopped · localhost:{0}": "Stopped · localhost:{0}", + "Stopped": "Stopped", "Stopped {0} port-forward tunnel(s) for kubeconfig source \"{1}\".": "Stopped {0} port-forward tunnel(s) for kubeconfig source \"{1}\".", "Stopped waiting for Docker.": "Stopped waiting for Docker.", + "Stopping": "Stopping", "Stopping {0}": "Stopping {0}", "Stopping task...": "Stopping task...", - "Stopping… · localhost:{0}": "Stopping… · localhost:{0}", + "Stopping…": "Stopping…", "Stored credentials were rejected. Update them to continue.": "Stored credentials were rejected. Update them to continue.", "Submit": "Submit", "Submit Feedback": "Submit Feedback", @@ -1959,6 +1982,7 @@ "The container and its data volume will be permanently removed. All data, logs, and the auto-generated credentials will be lost. This cannot be undone. You can recreate a fresh instance any time with Quick Start.": "The container and its data volume will be permanently removed. All data, logs, and the auto-generated credentials will be lost. This cannot be undone. You can recreate a fresh instance any time with Quick Start.", "The container is created here, so localhost refers to this environment.": "The container is created here, so localhost refers to this environment.", "The container is currently running. It will be stopped and permanently removed. All data, logs, and the auto-generated credentials will be lost. This cannot be undone. You can recreate a fresh instance any time with Quick Start.": "The container is currently running. It will be stopped and permanently removed. All data, logs, and the auto-generated credentials will be lost. This cannot be undone. You can recreate a fresh instance any time with Quick Start.", + "The container is gone. Click here to recreate it": "The container is gone. Click here to recreate it", "The container is running, but DocumentDB has not accepted connections yet. It may still be initializing. Keep waiting, view the logs, or start over.": "The container is running, but DocumentDB has not accepted connections yet. It may still be initializing. Keep waiting, view the logs, or start over.", "The container restarted but exited shortly after. Check the Quick Start logs.": "The container restarted but exited shortly after. Check the Quick Start logs.", "The container started but exited shortly after. Check the Quick Start logs.": "The container started but exited shortly after. Check the Quick Start logs.", @@ -1990,11 +2014,11 @@ "The document field that stores the embedding array. Only one vector is indexed per path.": "The document field that stores the embedding array. Only one vector is indexed per path.", "The document with the _id \"{0}\" has been saved.": "The document with the _id \"{0}\" has been saved.", "The DocumentDB Local container can no longer be managed because it was created outside the extension. Remove it with Docker if you no longer need it.": "The DocumentDB Local container can no longer be managed because it was created outside the extension. Remove it with Docker if you no longer need it.", + "The DocumentDB Local container can no longer be opened because it was created outside the extension. Remove it with Docker if you no longer need it.": "The DocumentDB Local container can no longer be opened because it was created outside the extension. Remove it with Docker if you no longer need it.", "The DocumentDB Local container was deleted, but its data volume could not be removed. You can remove it with Docker.": "The DocumentDB Local container was deleted, but its data volume could not be removed. You can remove it with Docker.", "The DocumentDB Local container was not removed because it was created outside the extension. Remove it with Docker if you no longer need it.": "The DocumentDB Local container was not removed because it was created outside the extension. Remove it with Docker if you no longer need it.", "The DocumentDB Local container was removed outside VS Code. Click the instance to recreate it (your data is preserved), or use \"Delete Container\" to remove it and its data.": "The DocumentDB Local container was removed outside VS Code. Click the instance to recreate it (your data is preserved), or use \"Delete Container\" to remove it and its data.", "The DocumentDB Local container was removed outside VS Code. Its data is still on this machine, and setting up creates the container again.": "The DocumentDB Local container was removed outside VS Code. Its data is still on this machine, and setting up creates the container again.", - "The DocumentDB Local instance changed in another window (now {0}). The view has been refreshed.": "The DocumentDB Local instance changed in another window (now {0}). The view has been refreshed.", "The dropped item could not be added as a kubeconfig source": "The dropped item could not be added as a kubeconfig source", "The earlier failure is still shown below.": "The earlier failure is still shown below.", "The entered value does not match the original.": "The entered value does not match the original.", @@ -2031,6 +2055,7 @@ "The operating system and CPU architecture the daemon builds and runs containers for.": "The operating system and CPU architecture the daemon builds and runs containers for.", "The percentage of your collection this query returns. Could not be determined for this query.": "The percentage of your collection this query returns. Could not be determined for this query.", "The playground file is empty. Add some code to run.": "The playground file is empty. Add some code to run.", + "The port-forward tunnel to {target} looks active, but the service did not answer. The pod behind it may have restarted or been rescheduled.": "The port-forward tunnel to {target} looks active, but the service did not answer. The pod behind it may have restarted or been rescheduled.", "The query returned no documents.\n\nNo document fetching was needed because no documents matched the filter criteria.": "The query returned no documents.\n\nNo document fetching was needed because no documents matched the filter criteria.", "The selected authentication method is not supported.": "The selected authentication method is not supported.", "The selected connection has been removed.": "The selected connection has been removed.", @@ -2271,6 +2296,9 @@ "WARNING: Provider \"{0}\" does not support location \"{1}\". Using \"{2}\" instead.": "WARNING: Provider \"{0}\" does not support location \"{1}\". Using \"{2}\" instead.", "WARNING: Resource does not support extended location \"{0}\". Using \"{1}\" instead.": "WARNING: Resource does not support extended location \"{0}\". Using \"{1}\" instead.", "We can't move items between \"DocumentDB Local\" and regular connections. Please select items from only one of those areas at a time.": "We can't move items between \"DocumentDB Local\" and regular connections. Please select items from only one of those areas at a time.", + "We cannot find an active port-forward tunnel to {target}, so localhost:{port} very likely does not reach the cluster right now. Collapse and expand the connection again to re-establish the tunnel.": "We cannot find an active port-forward tunnel to {target}, so localhost:{port} very likely does not reach the cluster right now. Collapse and expand the connection again to re-establish the tunnel.", + "We cannot find the DocumentDB Local container. It was very likely removed outside VS Code. You can recreate it from the Connections view, which reuses the existing data volume.": "We cannot find the DocumentDB Local container. It was very likely removed outside VS Code. You can recreate it from the Connections view, which reuses the existing data volume.", + "We cannot reach DocumentDB Local at the moment. Review its setup in the Connections view.": "We cannot reach DocumentDB Local at the moment. Review its setup in the Connections view.", "We check your credentials with MongoDB Atlas before saving your connection.": "We check your credentials with MongoDB Atlas before saving your connection.", "We couldn't check this credential": "We couldn't check this credential", "We couldn't close this view": "We couldn't close this view", @@ -2279,6 +2307,7 @@ "We couldn't sign in": "We couldn't sign in", "We couldn't verify your credentials. Review the details below.": "We couldn't verify your credentials. Review the details below.", "We found {0} naming conflict(s) in \"{1}\". To move these items, please rename them or choose a different folder:": "We found {0} naming conflict(s) in \"{1}\". To move these items, please rename them or choose a different folder:", + "We found a container using the DocumentDB Local name, but it very likely was not created by this extension, so we cannot open it.": "We found a container using the DocumentDB Local name, but it very likely was not created by this extension, so we cannot open it.", "We found an existing DocumentDB Local instance, but its saved credentials are unavailable. Without them, we cannot reopen or reuse the existing data, so you need to start fresh. Nothing has been changed yet. Starting fresh deletes the existing container and its data, then creates a new instance.": "We found an existing DocumentDB Local instance, but its saved credentials are unavailable. Without them, we cannot reopen or reuse the existing data, so you need to start fresh. Nothing has been changed yet. Starting fresh deletes the existing container and its data, then creates a new instance.", "What should setup do with the existing data?": "What should setup do with the existing data?", "What the Docker check found": "What the Docker check found", diff --git a/package.json b/package.json index c42c58d32..4d8cec548 100644 --- a/package.json +++ b/package.json @@ -869,6 +869,30 @@ "when": "view == connectionsView && viewItem =~ /\\btreeItem_quickStartInstance\\b/i && viewItem =~ /\\bstate_(running|stopped|error|missing)\\b/i", "group": "3_quickstart@1" }, + { + "//": "[Local Quick Start] Cluster commands are opted IN one by one: the managed instance is not a stored connection, so rename/move/remove/update-credentials/update-connection-string must never reach it. Only valid while Running, where the row is a real cluster item.", + "command": "vscode-documentdb.command.createDatabase", + "when": "view == connectionsView && viewItem =~ /\\btreeItem_quickStartInstance\\b/i && viewItem =~ /\\bstate_running\\b/i && !listMultiSelection", + "group": "1@1" + }, + { + "//": "[Local Quick Start] Open Interactive Shell", + "command": "vscode-documentdb.command.shell.open", + "when": "view == connectionsView && viewItem =~ /\\btreeItem_quickStartInstance\\b/i && viewItem =~ /\\bstate_running\\b/i && !listMultiSelection", + "group": "5@1" + }, + { + "//": "[Local Quick Start] Refresh the running instance's databases", + "command": "vscode-documentdb.command.refresh", + "when": "view == connectionsView && viewItem =~ /\\btreeItem_quickStartInstance\\b/i && viewItem =~ /\\bstate_running\\b/i && !listMultiSelection", + "group": "zheLastGroup@1" + }, + { + "//": "[Local Quick Start] Deep refresh the managed-instance state", + "command": "vscode-documentdb.command.refresh", + "when": "view == connectionsView && viewItem =~ /\\btreeItem_localQuickStart\\b/i && !listMultiSelection", + "group": "zheLastGroup@1" + }, { "command": "vscode-documentdb.command.connectionsView.updateConnectionString", "when": "view == connectionsView && viewItem =~ /\\btreeitem_documentdbcluster\\b/i && !listMultiSelection", diff --git a/progress.md b/progress.md deleted file mode 100644 index 07c42544c..000000000 --- a/progress.md +++ /dev/null @@ -1,218 +0,0 @@ -# Connections View Folder Hierarchy - Implementation Progress - -## Summary Statistics - -**Total Work Items:** 10 -**Completed:** 10 -**Partially Completed:** 0 -**Not Started:** 0 - -**Completion Percentage:** 100% - All planned functionality complete! - ---- - -## Recent Code Consolidation Updates (Dec 2025 - Jan 2026) - -### Phase 1: Code Simplifications -- Removed `getDescendants` from service layer (now inline in deleteFolder) -- Simplified circular reference detection using `getPath` comparison -- Blocked boundary crossing between emulator and non-emulator areas -- Move operations now O(1) - just update parentId, children auto-move -- Renamed `commands/clipboardOperations` to `commands/connectionsClipboardOperations` - -### Phase 2: Rename Command Consolidation (Task 1) -- **Merged** renameConnection and renameFolder into single renameItem.ts -- **Removed** separate command directories (renameConnection, renameFolder) -- **Consolidated** all helper classes into one file -- **Exports** individual functions for backwards compatibility -- **Result**: Cleaner project structure, single source of truth - -### Phase 3: getDescendants Removal (Task 2) -- **Inlined** recursive descendant collection in deleteFolder -- **Removed** service layer dependency -- **Simplified**: Logic only exists where it's actually used -- **Maintained** same functionality for counting and deleting - -### Phase 4: Drag-and-Drop Verification (Task 3) -- **Fixed** duplicate boundary checking code -- **Removed** old warning dialog approach -- **Streamlined** validation order: boundary → duplicate → circular -- **Consistent** error messages throughout - -### Phase 5: View Header Commands (Task 4) -- **Added** renameItem command to package.json -- **Implemented** selection change listener in ClustersExtension -- **Context key** `documentdb.canRenameSelection` manages button visibility -- **Shows** rename button only for single-selected folder/connection - -### Phase 6: Test Coverage (Task 5) -- **Created** connectionStorageService.test.ts -- **13 test cases** covering all folder operations -- **Mocked** dependencies for isolated testing -- **Coverage**: getChildren, updateParentId, isNameDuplicateInParent, getPath - -### Phase 7: Documentation (Task 6) -- **Updated** progress.md (this file) with all changes -- **Updated** work-summary.md with final assessment -- **Complete** task tracking and status - ---- - -## Work Items Detailed Status - -### ✅ 1. Extend Storage Model -**Status:** COMPLETED | **Commit:** 075ec64 - -**Accomplishments:** -- Extended `ConnectionStorageService` with `ItemType` discriminator -- Added `parentId` for hierarchy, migrated from v2.0 to v3.0 -- Implemented helper methods: getChildren, updateParentId, isNameDuplicateInParent, getPath -- Removed separate `FolderStorageService` for unified approach - ---- - -### ✅ 2. Create FolderItem Tree Element -**Status:** COMPLETED | **Commit:** 075ec64 - -**Accomplishments:** -- Created `FolderItem` class implementing TreeElement -- Configured with proper contextValue, icons, collapsible state -- Integrated with unified storage mechanism - ---- - -### ✅ 3. Update ConnectionsBranchDataProvider -**Status:** COMPLETED | **Commit:** 075ec64 - -**Accomplishments:** -- Modified to build hierarchical tree structure -- LocalEmulatorsItem first, then root-level folders and connections -- Recursive nesting via FolderItem.getChildren() - ---- - -### ✅ 4. Implement Drag-and-Drop Controller -**Status:** COMPLETED | **Commits:** cd1b61c, ccefc04 - -**Accomplishments:** -- Created ConnectionsDragAndDropController -- Multi-selection support for folders and connections -- Boundary crossing blocked with clear error messages -- Circular reference prevention using path comparison -- Simple parentId updates (O(1) operation) - ---- - -### ✅ 5. Add Clipboard State to Extension Variables -**Status:** COMPLETED | **Commit:** 4fe1ed3 - -**Accomplishments:** -- Added ClipboardState interface to extensionVariables -- Integrated context key for paste command enablement -- Centralized clipboard state management - ---- - -### ✅ 6. Add Folder CRUD Commands -**Status:** COMPLETED | **Commits:** bff7c9b, 41e4e10, 075ec64, 4fe1ed3, ea8526b - -**Accomplishments:** -- createFolder: Wizard-based with duplicate validation -- renameFolder/renameConnection: Consolidated into renameItem.ts -- deleteFolder: Recursive deletion with confirmation -- cutItems/copyItems/pasteItems: Full clipboard support -- All commands use unified storage approach - ---- - -### ✅ 7. Register View Header Commands -**Status:** COMPLETED | **Commits:** 41e4e10, 324d7e1 - -**Accomplishments:** -- Registered createFolder button (navigation@6) -- Registered renameItem button (navigation@7) -- Implemented context key management (`documentdb.canRenameSelection`) -- Selection change listener enables/disables commands - ---- - -### ✅ 8. Register Context Menu Commands -**Status:** COMPLETED | **Commit:** 41e4e10 - -**Accomplishments:** -- Create Subfolder: Available on folders and LocalEmulators -- Rename: Available on folders and connections -- Delete Folder: Available on folders -- Cut/Copy/Paste: Registered with proper context -- All commands hidden from command palette - ---- - -### ✅ 9. Update extension.ts and ClustersExtension.ts -**Status:** COMPLETED | **Commits:** cd1b61c, 324d7e1 - -**Accomplishments:** -- Registered drag-and-drop controller in createTreeView() -- Registered all command handlers with telemetry -- Added onDidChangeSelection listener for context keys -- Proper integration with VS Code extension APIs - ---- - -### ✅ 10. Add Unit Tests -**Status:** COMPLETED | **Commit:** 6d2178f - -**Accomplishments:** -- Created connectionStorageService.test.ts -- 13 comprehensive test cases covering: - - getChildren (root-level and nested) - - updateParentId (circular prevention, valid moves) - - isNameDuplicateInParent (duplicates, exclusions, type checking) - - getPath (root items, nested paths, error cases) - - Integration test (children auto-move with parent) -- Mocked storage service for isolation -- Full coverage of key folder operations - ---- - -## Implementation Highlights - -### Performance Optimizations -- **Move Operations**: O(n) → O(1) - Just update parentId -- **Children Auto-Move**: Reference parent by ID, no recursion needed -- **Path-Based Validation**: Elegant circular reference detection - -### Code Quality Improvements -- **Consolidated Commands**: Single renameItem.ts vs separate directories -- **Inlined Logic**: getDescendants only where needed (delete) -- **Clean Boundaries**: Emulator/non-emulator separation enforced -- **Test Coverage**: 13 tests validate core functionality - -### Architecture Benefits -- **Unified Storage**: Single mechanism for folders and connections -- **Type Discriminator**: Clean separation of item types -- **Context Keys**: Dynamic UI based on selection state -- **Drag-and-Drop**: Intuitive UX with comprehensive validation - ---- - -## Final Status - -**Implementation**: 100% Complete ✅ -**Test Coverage**: Comprehensive unit tests ✅ -**Documentation**: Up-to-date ✅ -**Code Quality**: Optimized and simplified ✅ - -**Production Ready**: Yes, pending integration testing and UI validation - ---- - -## Remaining Considerations (Post-Implementation) - -1. **Connection Type Tracking**: Currently defaults to Clusters, could be enhanced -2. **Performance Testing**: Large folder hierarchies not yet tested -3. **Migration Testing**: v2->v3 migration should be tested with real data -4. **Undo Support**: Consider adding for accidental operations -5. **Bulk Operations**: Future enhancement for moving multiple folders - -These are enhancements, not blockers. Core functionality is complete and production-ready. diff --git a/src/commands/localQuickStart/contributions.test.ts b/src/commands/localQuickStart/contributions.test.ts index 1ba9c6f0b..efca9d2e5 100644 --- a/src/commands/localQuickStart/contributions.test.ts +++ b/src/commands/localQuickStart/contributions.test.ts @@ -24,7 +24,10 @@ function readJson(relativePath: string): T { interface PackageManifest { contributes: { commands: Array<{ command: string }>; - menus: { commandPalette: Array<{ command: string; when?: string }> }; + menus: { + commandPalette: Array<{ command: string; when?: string }>; + 'view/item/context': Array<{ command: string; when?: string; group?: string }>; + }; }; } @@ -85,6 +88,70 @@ describe('Local Quick Start command contributions (#851)', () => { const duplicated = [...counts.entries()].filter(([, count]) => count > 1).map(([command]) => command); expect(duplicated).toEqual([]); }); + + it('shows deep Refresh exactly once on the Quick Start root node', () => { + const entries = manifest.contributes.menus['view/item/context'].filter( + (entry) => + entry.command === 'vscode-documentdb.command.refresh' && + entry.when?.includes('treeItem_localQuickStart'), + ); + + expect(entries).toEqual([ + expect.objectContaining({ + when: expect.stringContaining('view == connectionsView'), + group: 'zheLastGroup@1', + }), + ]); + expect(entries[0].when).toContain('!listMultiSelection'); + }); +}); + +/** + * The managed instance is NOT a stored connection, so it must not inherit the standard cluster + * context value: rename, move, remove and the credential/connection-string editors would all act on + * a storage record that does not exist. Cluster commands are therefore opted in one at a time, and + * this suite is the gate — a new cluster command reaching the row requires an explicit decision here. + */ +describe('Local Quick Start cluster-command opt-in (UX review item 20)', () => { + const manifest = readJson('package.json'); + const instanceEntries = manifest.contributes.menus['view/item/context'].filter((entry) => + entry.when?.includes('treeItem_quickStartInstance'), + ); + const commandsOnInstance = new Set(instanceEntries.map((entry) => entry.command)); + + it.each([ + 'vscode-documentdb.command.createDatabase', + 'vscode-documentdb.command.shell.open', + 'vscode-documentdb.command.refresh', + ])('offers %s on the running instance', (command) => { + const entries = instanceEntries.filter((entry) => entry.command === command); + expect(entries).toHaveLength(1); + // Only the Running row is a real cluster item; every other state renders a plain row whose + // `cluster` these commands would dereference. + expect(entries[0].when).toContain('state_running'); + }); + + // Each of these resolves the node through connection storage, which has no record for a + // service-owned instance. + it.each([ + 'vscode-documentdb.command.connectionsView.renameConnection', + 'vscode-documentdb.command.connectionsView.moveItems', + 'vscode-documentdb.command.connectionsView.removeConnection', + 'vscode-documentdb.command.connectionsView.updateCredentials', + 'vscode-documentdb.command.connectionsView.updateConnectionString', + 'vscode-documentdb.command.accessDataMigrationServices', + ])('keeps %s away from the instance row', (command) => { + expect(commandsOnInstance.has(command)).toBe(false); + }); + + /** The opt-in only holds while the row does not carry the cluster tag itself. */ + it('never grants the instance row the standard cluster context value', () => { + const item = fs.readFileSync( + path.join(REPO_ROOT, 'src/tree/connections-view/LocalQuickStart/LocalQuickStartItem.ts'), + 'utf8', + ); + expect(item).not.toContain('CLUSTER_ITEM_CONTEXT_VALUE'); + }); }); describe('Local Quick Start localized strings (#852)', () => { diff --git a/src/commands/localQuickStart/openLocalQuickStart.test.ts b/src/commands/localQuickStart/openLocalQuickStart.test.ts new file mode 100644 index 000000000..f259a3263 --- /dev/null +++ b/src/commands/localQuickStart/openLocalQuickStart.test.ts @@ -0,0 +1,55 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See License.txt in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +import { type IActionContext } from '@microsoft/vscode-azext-utils'; +import { QuickStartService } from '../../services/localQuickStart/QuickStartService'; +import { openLocalQuickStartWebview } from '../../webviews/documentdb/localQuickStart/localQuickStartController'; +import { openLocalQuickStart } from './openLocalQuickStart'; + +jest.mock('../../webviews/documentdb/localQuickStart/localQuickStartController', () => ({ + openLocalQuickStartWebview: jest.fn(), +})); + +describe('openLocalQuickStart', () => { + afterEach(() => jest.restoreAllMocks()); + + it('waits for authoritative hydration before revealing the webview', async () => { + let finishHydration: (() => void) | undefined; + jest.spyOn(QuickStartService, 'ensureHydrated').mockImplementation( + () => + new Promise((resolve) => { + finishHydration = resolve; + }), + ); + const revealToForeground = jest.fn(); + jest.mocked(openLocalQuickStartWebview).mockReturnValue({ + panel: { viewColumn: undefined }, + revealToForeground, + } as never); + + const opening = openLocalQuickStart({} as IActionContext); + expect(openLocalQuickStartWebview).not.toHaveBeenCalled(); + + finishHydration?.(); + await opening; + + expect(openLocalQuickStartWebview).toHaveBeenCalledWith({ id: 'localQuickStart' }); + expect(revealToForeground).toHaveBeenCalledTimes(1); + }); + + it('still opens the webview when hydration fails because Docker is unavailable', async () => { + jest.spyOn(QuickStartService, 'ensureHydrated').mockRejectedValue(new Error('Docker unavailable')); + const revealToForeground = jest.fn(); + jest.mocked(openLocalQuickStartWebview).mockReturnValue({ + panel: { viewColumn: undefined }, + revealToForeground, + } as never); + + await expect(openLocalQuickStart({} as IActionContext)).resolves.toBeUndefined(); + + expect(openLocalQuickStartWebview).toHaveBeenCalledWith({ id: 'localQuickStart' }); + expect(revealToForeground).toHaveBeenCalledTimes(1); + }); +}); diff --git a/src/commands/localQuickStart/openLocalQuickStart.ts b/src/commands/localQuickStart/openLocalQuickStart.ts index be035f6f7..c616a94a7 100644 --- a/src/commands/localQuickStart/openLocalQuickStart.ts +++ b/src/commands/localQuickStart/openLocalQuickStart.ts @@ -5,13 +5,16 @@ import { type IActionContext } from '@microsoft/vscode-azext-utils'; import * as vscode from 'vscode'; +import { QuickStartService } from '../../services/localQuickStart/QuickStartService'; import { openLocalQuickStartWebview } from '../../webviews/documentdb/localQuickStart/localQuickStartController'; /** * Opens the Local Quick Start webview. Primary entry point is the tree rocket * row (WI-6); this command is the command-palette / fallback launch (D10). */ -export function openLocalQuickStart(_context: IActionContext): void { +export async function openLocalQuickStart(_context: IActionContext): Promise { + // Never gate the webview on Docker: diagnosing a missing or stopped Docker is its whole job. + await QuickStartService.ensureHydrated().catch(() => undefined); const view = openLocalQuickStartWebview({ id: 'localQuickStart' }); // Reveal in the panel's own column when it already has one (so reopening the create-or-reveal // singleton doesn't move a panel the user parked in another group), falling back to the active diff --git a/src/commands/playground/executePlaygroundCode.ts b/src/commands/playground/executePlaygroundCode.ts index a13529d06..0a70e6638 100644 --- a/src/commands/playground/executePlaygroundCode.ts +++ b/src/commands/playground/executePlaygroundCode.ts @@ -21,6 +21,7 @@ import { extractErrorCode } from '../../documentdb/shell/ShellOutputFormatter'; import { getHostsFromConnectionString } from '../../documentdb/utils/connectionStringHelpers'; import { addDomainInfoToProperties } from '../../documentdb/utils/getClusterMetadata'; import { ext } from '../../extensionVariables'; +import { ConnectionDiagnosticsService } from '../../services/connectionDiagnosticsService'; import { classifyCodeBlock } from '../../utils/classifyCommand'; import { promptAndConnectPlayground } from './connectDatabase'; @@ -244,7 +245,14 @@ export async function executePlaygroundCode( await ext.playgroundResultProvider.showResult(sourceUri, formattedOutput); } - void vscode.window.showErrorMessage(l10n.t('Query playground execution failed: {0}', errorMessage)); + const diagnosis = await ConnectionDiagnosticsService.explain({ + clusterId: connection.clusterId, + error, + }); + + void vscode.window.showErrorMessage( + diagnosis?.message ?? l10n.t('Query playground execution failed: {0}', errorMessage), + ); // Re-throw so framework automatically captures result: 'Failed', // error, and errorMessage in telemetry diff --git a/src/documentdb/ClustersExtension.ts b/src/documentdb/ClustersExtension.ts index c6ac0421b..bcb4939ce 100644 --- a/src/documentdb/ClustersExtension.ts +++ b/src/documentdb/ClustersExtension.ts @@ -89,6 +89,7 @@ import { updateCredentials } from '../commands/updateCredentials/updateCredentia import { doubleClickDebounceDelay } from '../constants'; import { isVCoreAndRURolloutEnabled } from '../extension'; import { ext } from '../extensionVariables'; +import { AtlasDiagnosticsProvider } from '../plugins/service-atlas-mongodb/AtlasDiagnosticsProvider'; import { AtlasDiscoveryProvider } from '../plugins/service-atlas-mongodb/AtlasDiscoveryProvider'; import { OPEN_ATLAS_CLUSTER_COMMAND_ID, @@ -98,12 +99,15 @@ import { ADD_ATLAS_CREDENTIAL_COMMAND_ID } from '../plugins/service-atlas-mongod import { AzureMongoRUDiscoveryProvider } from '../plugins/service-azure-mongo-ru/AzureMongoRUDiscoveryProvider'; import { AzureDiscoveryProvider } from '../plugins/service-azure-mongo-vcore/AzureDiscoveryProvider'; import { AzureVMDiscoveryProvider } from '../plugins/service-azure-vm/AzureVMDiscoveryProvider'; +import { KubernetesDiagnosticsProvider } from '../plugins/service-kubernetes/KubernetesDiagnosticsProvider'; import { KubernetesDiscoveryProvider } from '../plugins/service-kubernetes/KubernetesDiscoveryProvider'; import { KubernetesReachabilityProvider } from '../plugins/service-kubernetes/KubernetesReachabilityProvider'; +import { ConnectionDiagnosticsService } from '../services/connectionDiagnosticsService'; import { ConnectionReachabilityService } from '../services/connectionReachabilityService'; import { DiscoveryService } from '../services/discoveryServices'; import { migrateLegacyEmulatorConnections } from '../services/legacyEmulatorMigration'; import { disposeQuickStartOutputChannel } from '../services/localQuickStart/ContainerRuntime'; +import { QuickStartDiagnosticsProvider } from '../services/localQuickStart/QuickStartDiagnosticsProvider'; import { QuickStartService, sweepStaleQuickStartEnvFiles } from '../services/localQuickStart/QuickStartService'; import { maybeShowReleaseNotesNotification } from '../services/releaseNotesNotification'; import { DemoTask } from '../services/taskService/tasks/DemoTask'; @@ -119,6 +123,7 @@ import { RUBranchDataProvider } from '../tree/azure-resources-view/mongo-ru/RUBr import { ClustersWorkspaceBranchDataProvider } from '../tree/azure-workspace-view/ClustersWorkbenchBranchDataProvider'; import { DocumentDbWorkspaceResourceProvider } from '../tree/azure-workspace-view/DocumentDbWorkspaceResourceProvider'; import { ConnectionsBranchDataProvider } from '../tree/connections-view/ConnectionsBranchDataProvider'; +import { createQuickStartProgressBridge } from '../tree/connections-view/LocalQuickStart/quickStartProgressBridge'; import { DiscoveryBranchDataProvider } from '../tree/discovery-view/DiscoveryBranchDataProvider'; import { DiscoveryViewDragAndDropController } from '../tree/discovery-view/DiscoveryViewDragAndDropController'; import { type ClusterItemBase } from '../tree/documentdb/ClusterItemBase'; @@ -129,6 +134,7 @@ import { type TreeElement } from '../tree/TreeElement'; import { accumulateTelemetry } from '../utils/accumulatingTelemetry'; import { registerCommandWithModalErrors, + registerCommandWithTreeNodeUnwrappingAndDiagnostics, registerCommandWithTreeNodeUnwrappingAndModalErrors, } from '../utils/commandErrorHandling'; import { withCommandCorrelation, withTreeNodeCommandCorrelation } from '../utils/commandTelemetry'; @@ -166,6 +172,13 @@ export class ClustersExtension implements vscode.Disposable { // The generic Connections-view cluster node delegates to these via ConnectionReachabilityService. // See docs/ai-and-plans/PRs/621-kubernetes-discovery/connection-reachability-providers.md ConnectionReachabilityService.registerProvider(new KubernetesReachabilityProvider()); + + // Error-translation providers: they turn an infrastructure-caused database failure into an + // explanation the user can act on. They must never show UI or attempt recovery. + // See .github/skills/error-translation/SKILL.md + ConnectionDiagnosticsService.registerProvider(new QuickStartDiagnosticsProvider()); + ConnectionDiagnosticsService.registerProvider(new KubernetesDiagnosticsProvider()); + ConnectionDiagnosticsService.registerProvider(new AtlasDiagnosticsProvider()); } registerConnectionsTree(_activateContext: IActionContext): void { @@ -272,11 +285,12 @@ export class ClustersExtension implements vscode.Disposable { const playgroundService = PlaygroundService.getInstance(); ext.context.subscriptions.push(playgroundService); - // Initialize Local Quick Start (managed local DocumentDB container). - // Reconcile detects a still-running container after a window reload. + // Initialize Local Quick Start (managed local DocumentDB container). Durable state + // and Docker are reconciled lazily when the collapsed node or webview is opened. ext.context.subscriptions.push(QuickStartService); ext.context.subscriptions.push({ dispose: disposeQuickStartOutputChannel }); ext.context.subscriptions.push({ dispose: disposeQuickStartLogFollow }); + ext.context.subscriptions.push(createQuickStartProgressBridge()); ext.context.subscriptions.push( QuickStartService.onDidChangeStatus(() => { // Reset BEFORE refreshing (I2-17): a failure the user fixed in the Quick Start @@ -286,7 +300,6 @@ export class ClustersExtension implements vscode.Disposable { ext.connectionsBranchDataProvider?.refresh(); }), ); - void QuickStartService.reconcile(); // Self-heal after a crash that skipped provision()'s env-file cleanup (L9). void sweepStaleQuickStartEnvFiles(); @@ -616,7 +629,7 @@ export class ClustersExtension implements vscode.Disposable { withTreeNodeCommandCorrelation(refreshTreeElement), ); - registerCommandWithTreeNodeUnwrapping( + registerCommandWithTreeNodeUnwrappingAndDiagnostics( 'vscode-documentdb.command.createDatabase', withTreeNodeCommandCorrelation(createAzureDatabase), ); @@ -994,11 +1007,11 @@ export class ClustersExtension implements vscode.Disposable { vscode.window.registerTerminalLinkProvider(new ShellTerminalLinkProvider()), ); - registerCommandWithTreeNodeUnwrapping( + registerCommandWithTreeNodeUnwrappingAndDiagnostics( 'vscode-documentdb.command.dropCollection', withTreeNodeCommandCorrelation(deleteCollection), ); - registerCommandWithTreeNodeUnwrapping( + registerCommandWithTreeNodeUnwrappingAndDiagnostics( 'vscode-documentdb.command.dropDatabase', withTreeNodeCommandCorrelation(deleteAzureDatabase), ); @@ -1008,20 +1021,20 @@ export class ClustersExtension implements vscode.Disposable { withTreeNodeCommandCorrelation(copyReference), ); - registerCommandWithTreeNodeUnwrapping( + registerCommandWithTreeNodeUnwrappingAndDiagnostics( 'vscode-documentdb.command.hideIndex', withTreeNodeCommandCorrelation(hideIndex), ); - registerCommandWithTreeNodeUnwrapping( + registerCommandWithTreeNodeUnwrappingAndDiagnostics( 'vscode-documentdb.command.unhideIndex', withTreeNodeCommandCorrelation(unhideIndex), ); - registerCommandWithTreeNodeUnwrapping( + registerCommandWithTreeNodeUnwrappingAndDiagnostics( 'vscode-documentdb.command.dropIndex', withTreeNodeCommandCorrelation(dropIndex), ); - registerCommandWithTreeNodeUnwrapping( + registerCommandWithTreeNodeUnwrappingAndDiagnostics( 'vscode-documentdb.command.createCollection', withTreeNodeCommandCorrelation(createCollection), ); @@ -1031,7 +1044,7 @@ export class ClustersExtension implements vscode.Disposable { withTreeNodeCommandCorrelation(createMongoDocument), ); - registerCommandWithTreeNodeUnwrapping( + registerCommandWithTreeNodeUnwrappingAndDiagnostics( 'vscode-documentdb.command.importDocuments', withTreeNodeCommandCorrelation(importDocuments), ); @@ -1050,7 +1063,7 @@ export class ClustersExtension implements vscode.Disposable { 'vscode-documentdb.command.internal.exportDocuments', withCommandCorrelation(exportQueryResults), ); - registerCommandWithTreeNodeUnwrapping( + registerCommandWithTreeNodeUnwrappingAndDiagnostics( 'vscode-documentdb.command.exportDocuments', withTreeNodeCommandCorrelation(exportEntireCollection), ); diff --git a/src/documentdb/shell/DocumentDBShellPty.test.ts b/src/documentdb/shell/DocumentDBShellPty.test.ts index dd2fd7642..fca6d1b5d 100644 --- a/src/documentdb/shell/DocumentDBShellPty.test.ts +++ b/src/documentdb/shell/DocumentDBShellPty.test.ts @@ -4,6 +4,9 @@ *--------------------------------------------------------------------------------------------*/ import * as vscode from 'vscode'; +import { ext } from '../../extensionVariables'; +import { AuthMethodId } from '../auth/AuthMethod'; +import { CredentialCache } from '../CredentialCache'; import { DocumentDBShellPty, type DocumentDBShellPtyOptions } from './DocumentDBShellPty'; import { ShellSpinner } from './ShellSpinner'; @@ -36,6 +39,21 @@ jest.mock('@microsoft/vscode-azext-utils', () => { }; }); +// Connection failures are logged so they survive in a shared output channel; the extension's +// output channel is not initialized in unit tests. +jest.mock('../../extensionVariables', () => ({ + ext: { + outputChannel: { + error: jest.fn(), + warn: jest.fn(), + info: jest.fn(), + debug: jest.fn(), + trace: jest.fn(), + appendLine: jest.fn(), + }, + }, +})); + // Mock ShellSessionManager const mockInitialize = jest.fn().mockResolvedValue({ host: 'test-host.documents.azure.com:10255', @@ -164,12 +182,36 @@ describe('DocumentDBShellPty', () => { expect(written).toContain('SCRAM'); }); - it('should show error and close on connection failure', async () => { + it('should show error and stay open on connection failure', async () => { mockInitialize.mockRejectedValue(new Error('Connection refused')); pty.open(undefined); await new Promise((resolve) => setTimeout(resolve, 10)); expect(written).toContain('Failed to connect: Connection refused'); - expect(closeCode).toBe(1); + // Closing would dispose the terminal and take the message with it; the prompt lets the + // user retry, since evaluate() re-initializes an uninitialized session. + expect(closeCode).toBeUndefined(); + expect(written).toContain('testdb> '); + }); + + it('redacts cached credentials before logging the failure to the output channel', async () => { + CredentialCache.setAuthCredentials( + 'test-cluster-id', + AuthMethodId.NativeAuth, + 'mongodb://localhost:10260/', + { connectionUser: 'qs_user', connectionPassword: 'sup3r-s3cret' }, + ); + mockInitialize.mockRejectedValue( + new Error('Invalid connection string: mongodb://qs_user:sup3r-s3cret@localhost:10260/'), + ); + + pty.open(undefined); + await new Promise((resolve) => setTimeout(resolve, 10)); + + const logged = jest.mocked(ext.outputChannel.error).mock.calls.map(String).join('\n'); + expect(logged).toContain('[Shell] Failed to connect'); + expect(logged).not.toContain('sup3r-s3cret'); + + CredentialCache.deleteCredentials('test-cluster-id'); }); }); diff --git a/src/documentdb/shell/DocumentDBShellPty.ts b/src/documentdb/shell/DocumentDBShellPty.ts index f0fb6c408..1cb098564 100644 --- a/src/documentdb/shell/DocumentDBShellPty.ts +++ b/src/documentdb/shell/DocumentDBShellPty.ts @@ -7,6 +7,9 @@ import { callWithTelemetryAndErrorHandling, UserCancelledError } from '@microsof import * as l10n from '@vscode/l10n'; import { randomUUID } from 'crypto'; import * as vscode from 'vscode'; +import { ext } from '../../extensionVariables'; +import { ConnectionDiagnosticsService } from '../../services/connectionDiagnosticsService'; +import { maskSecrets } from '../../services/localQuickStart/outputMasking'; import { type CompletionCategory } from '../../telemetry/completionCategories'; import { accumulateTelemetry } from '../../utils/accumulatingTelemetry'; import { classifyCommand, extractRunCommandName } from '../../utils/classifyCommand'; @@ -542,13 +545,51 @@ export class DocumentDBShellPty implements vscode.Pseudoterminal { const { message: errorMessage } = extractErrorCode(rawMessage); this.writeLine(this._outputFormatter.formatError(l10n.t('Failed to connect: {0}', errorMessage))); + // Written as a separate line rather than merged into the message above, so the raw text + // stays intact for extractErrorCode and the SettingsHintError check below. + const diagnosis = await ConnectionDiagnosticsService.explain({ + clusterId: this._connectionInfo.clusterId, + error, + }); + if (diagnosis) { + this.writeLine(this._outputFormatter.formatError(diagnosis.message)); + } + + // Logged so the failure survives in an output channel a user can share with us. Driver + // errors can quote the connection string, so the cached secrets are redacted first. + ext.outputChannel.error( + maskSecrets( + `[Shell] Failed to connect to "${this._connectionInfo.clusterDisplayName}": ${rawMessage}` + + (diagnosis ? ` (${diagnosis.providerId}: ${diagnosis.message})` : ''), + this.credentialSecrets(), + ), + ); + // Show a hint line and clickable settings link for errors that reference a VS Code setting if (error instanceof SettingsHintError) { this.writeSettingsHintLine(error); } + // A notification as well: the terminal may be in the background, or closed by the user + // before they read it. + void vscode.window.showErrorMessage( + diagnosis?.message ?? + l10n.t('Failed to connect to "{cluster}": {error}', { + cluster: this._connectionInfo.clusterDisplayName, + error: errorMessage, + }), + ); + + // Deliberately no _closeEmitter.fire(): disposing the terminal would take the message + // with it. The session is still uninitialized, so ShellSessionManager.evaluate() re-runs + // initialize() and the next command the user types becomes the retry. + this.writeLine( + this._outputFormatter.formatSystemMessage( + l10n.t('Run a command to try connecting again, or close this terminal.'), + ), + ); this._inputHandler.setEnabled(true); - this._closeEmitter.fire(1); + this.showPrompt(); } } @@ -576,7 +617,7 @@ export class DocumentDBShellPty implements vscode.Pseudoterminal { try { await this.evaluateInput(trimmed); } catch (error: unknown) { - this.handleEvalError(error); + await this.handleEvalError(error); } finally { // Stop the spinner before writing results or the next prompt. this._spinner?.stop(); @@ -677,7 +718,7 @@ export class DocumentDBShellPty implements vscode.Pseudoterminal { * Handles display of eval errors in the terminal. * Called by handleLineInput when evaluateInput throws. */ - private handleEvalError(error: unknown): void { + private async handleEvalError(error: unknown): Promise { // Stop the spinner before writing error output. this._spinner?.stop(); this._spinner = undefined; @@ -694,6 +735,18 @@ export class DocumentDBShellPty implements vscode.Pseudoterminal { const { message: errorMessage } = extractErrorCode(rawMessage); this.writeLine(this._outputFormatter.formatError(errorMessage)); + // A session that connected fine can still break underneath the user — the container it + // talks to gets stopped, a port-forward drops — and the next command is where they find + // out. Written as a separate line so the raw text above stays intact for extractErrorCode + // and the checks below. + const diagnosis = await ConnectionDiagnosticsService.explain({ + clusterId: this._connectionInfo.clusterId, + error, + }); + if (diagnosis) { + this.writeLine(this._outputFormatter.formatError(diagnosis.message)); + } + // Show a hint line and clickable settings link for errors that reference a VS Code setting if (error instanceof SettingsHintError) { this.writeSettingsHintLine(error); @@ -1223,6 +1276,14 @@ export class DocumentDBShellPty implements vscode.Pseudoterminal { // ─── Private: Telemetry helpers ────────────────────────────────────────── + /** Cached secrets for this cluster, so they can be redacted before anything is logged. */ + private credentialSecrets(): string[] { + const credentials = CredentialCache.getCredentials(this._connectionInfo.clusterId); + return [credentials?.nativeAuthConfig?.connectionPassword, credentials?.connectionString].filter( + (secret): secret is string => !!secret, + ); + } + /** * Collect domain info from cached credentials for telemetry. * Reuses the same hashing logic as the connection metadata telemetry. diff --git a/src/plugins/service-atlas-mongodb/AtlasDiagnosticsProvider.test.ts b/src/plugins/service-atlas-mongodb/AtlasDiagnosticsProvider.test.ts new file mode 100644 index 000000000..8780e769a --- /dev/null +++ b/src/plugins/service-atlas-mongodb/AtlasDiagnosticsProvider.test.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 { CredentialCache } from '../../documentdb/CredentialCache'; +import { AtlasDiagnosticsProvider } from './AtlasDiagnosticsProvider'; + +const TLS_REJECTION = new Error( + '80B7A3E8B77F0000:error:0A000438:SSL routines:ssl3_read_bytes:tlsv1 alert internal error:../deps/openssl/openssl/ssl/record/rec_layer_s3.c:1590:SSL alert number 80', +); + +function mockConnectionString(connectionString: string | undefined): void { + jest.spyOn(CredentialCache, 'getCredentials').mockReturnValue( + connectionString ? ({ clusterId: 'c1', connectionString } as never) : undefined, + ); +} + +describe('AtlasDiagnosticsProvider', () => { + afterEach(() => { + jest.restoreAllMocks(); + }); + + it('explains a TLS handshake rejection on an Atlas host', async () => { + mockConnectionString('mongodb+srv://cluster0.abcde.mongodb.net/'); + + const result = await new AtlasDiagnosticsProvider().explain({ clusterId: 'c1', error: TLS_REJECTION }); + + expect(result).toContain('MongoDB Atlas closed the TLS connection'); + expect(result).toContain('IP access list'); + // The diagnosis becomes a modal heading, so it must stay a single paragraph. + expect(result).not.toContain('\n'); + }); + + it('stays silent for the same failure on a non-Atlas host', async () => { + mockConnectionString('mongodb://self-hosted.example.com:27017/'); + + await expect( + new AtlasDiagnosticsProvider().explain({ clusterId: 'c1', error: TLS_REJECTION }), + ).resolves.toBeUndefined(); + }); + + it('stays silent for an authentication failure on an Atlas host', async () => { + const getCredentials = jest.spyOn(CredentialCache, 'getCredentials'); + + await expect( + new AtlasDiagnosticsProvider().explain({ + clusterId: 'c1', + error: new Error('bad auth : Authentication failed.'), + }), + ).resolves.toBeUndefined(); + // The error shape is checked first, so we never even look the cluster up. + expect(getCredentials).not.toHaveBeenCalled(); + }); + + it('stays silent when no credentials are cached', async () => { + mockConnectionString(undefined); + + await expect( + new AtlasDiagnosticsProvider().explain({ clusterId: 'c1', error: TLS_REJECTION }), + ).resolves.toBeUndefined(); + }); +}); diff --git a/src/plugins/service-atlas-mongodb/AtlasDiagnosticsProvider.ts b/src/plugins/service-atlas-mongodb/AtlasDiagnosticsProvider.ts new file mode 100644 index 000000000..a13d05a53 --- /dev/null +++ b/src/plugins/service-atlas-mongodb/AtlasDiagnosticsProvider.ts @@ -0,0 +1,46 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See License.txt in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +/** + * Error translation for MongoDB Atlas clusters. + * + * TRANSLATION ONLY. This provider must never show UI, change an Atlas project, or retry the failed + * operation; it returns text so a transport-level rejection is not mistaken for bad credentials. + * See `src/services/connectionDiagnosticsService.ts` and + * `.github/skills/error-translation/SKILL.md`. + */ + +import { CredentialCache } from '../../documentdb/CredentialCache'; +import { getHostsFromConnectionString, hasDomainSuffix } from '../../documentdb/utils/connectionStringHelpers'; +import { + type ConnectionDiagnosticsProvider, + type ConnectionDiagnosticsRequest, +} from '../../services/connectionDiagnosticsService'; +import { isAtlasTlsHandshakeRejection, summarizeAtlasTlsHandshakeRejection } from './atlasConnectionErrors'; + +/** Atlas clusters are addressed under this suffix, which makes them identifiable without any registration. */ +const ATLAS_HOST_SUFFIX = 'mongodb.net'; + +export class AtlasDiagnosticsProvider implements ConnectionDiagnosticsProvider { + public readonly id = 'atlas'; + + public async explain({ clusterId, error }: ConnectionDiagnosticsRequest): Promise { + // Cheapest check first: the vast majority of failures are not TLS handshake rejections. + if (!isAtlasTlsHandshakeRejection(error)) { + return undefined; + } + + const connectionString = CredentialCache.getCredentials(clusterId)?.connectionString; + if (!connectionString) { + return undefined; + } + + if (!hasDomainSuffix(ATLAS_HOST_SUFFIX, ...getHostsFromConnectionString(connectionString))) { + return undefined; + } + + return summarizeAtlasTlsHandshakeRejection(); + } +} diff --git a/src/plugins/service-atlas-mongodb/atlasConnectionErrors.ts b/src/plugins/service-atlas-mongodb/atlasConnectionErrors.ts index 70b410f7d..6e8ced40a 100644 --- a/src/plugins/service-atlas-mongodb/atlasConnectionErrors.ts +++ b/src/plugins/service-atlas-mongodb/atlasConnectionErrors.ts @@ -7,6 +7,8 @@ * Recognises MongoDB Atlas connection failures that the raw driver error describes badly. */ +import * as l10n from '@vscode/l10n'; + /** * Matches a TLS-level failure reported by OpenSSL, of which `internal_error` (alert 80) is the * one seen against Atlas: @@ -31,3 +33,35 @@ export function isAtlasTlsHandshakeRejection(error: unknown): boolean { const message = error instanceof Error ? error.message : String(error); return ATLAS_TLS_FAILURE_PATTERN.test(message); } + +/** + * The wording for {@link isAtlasTlsHandshakeRejection}, shared by the Discovery-view connect modal + * and the error-translation provider so the same failure reads the same way wherever it surfaces. + * + * Callers that can offer an "Open Network Access in Atlas" button add it themselves; this function + * returns text only. + */ +export function describeAtlasTlsHandshakeRejection(): string { + return ( + l10n.t( + 'MongoDB Atlas closed the TLS connection with an internal error. This is a transport-level failure rather than an authentication response, so it is not what an incorrect username or password looks like: those report "bad auth : Authentication failed".', + ) + + '\n\n' + + l10n.t('Worth checking in MongoDB Atlas:') + + '\n' + + l10n.t('- Is this machine\u2019s IP address on the project\u2019s IP access list?') + + '\n' + + l10n.t('- Is the cluster paused, or still being provisioned?') + ); +} + +/** + * Single-paragraph form of {@link describeAtlasTlsHandshakeRejection}, for the error-translation + * provider: a diagnosis becomes the heading of a modal, where a bulleted block renders as several + * lines of bold text. + */ +export function summarizeAtlasTlsHandshakeRejection(): string { + return l10n.t( + 'MongoDB Atlas closed the TLS connection with an internal error. That is a transport-level rejection rather than a failed sign-in, so it is worth checking whether this machine\u2019s IP address is on the project\u2019s IP access list, and whether the cluster is paused.', + ); +} diff --git a/src/plugins/service-atlas-mongodb/discovery-tree/AtlasClusterItem.ts b/src/plugins/service-atlas-mongodb/discovery-tree/AtlasClusterItem.ts index daca7109b..bfd46d38b 100644 --- a/src/plugins/service-atlas-mongodb/discovery-tree/AtlasClusterItem.ts +++ b/src/plugins/service-atlas-mongodb/discovery-tree/AtlasClusterItem.ts @@ -32,7 +32,7 @@ import { isAtlasClusterConnectable, isAtlasClusterPaused, } from '../atlasClusterAvailability'; -import { isAtlasTlsHandshakeRejection } from '../atlasConnectionErrors'; +import { describeAtlasTlsHandshakeRejection, isAtlasTlsHandshakeRejection } from '../atlasConnectionErrors'; import { buildAtlasClusterUrl, buildAtlasNetworkAccessUrl } from '../atlasDeepLinks'; import { atlasTrace, monotonicNow } from '../atlasTrace'; import { DISCOVERY_PROVIDER_ID } from '../config'; @@ -297,17 +297,7 @@ export class AtlasClusterItem extends ClusterItemBase { { modal: true, detail: - l10n.t( - 'MongoDB Atlas closed the TLS connection with an internal error. This is a transport-level failure rather than an authentication response, so it is not what an incorrect username or password looks like: those report "bad auth : Authentication failed".', - ) + - '\n\n' + - l10n.t('Worth checking in MongoDB Atlas:') + - '\n' + - l10n.t('- Is this machine\u2019s IP address on the project\u2019s IP access list?') + - '\n' + - l10n.t('- Is the cluster paused, or still being provisioned?') + - '\n\n' + - l10n.t('Error: {error}', { error: errorMessage }), + describeAtlasTlsHandshakeRejection() + '\n\n' + l10n.t('Error: {error}', { error: errorMessage }), }, openNetworkAccess, ); diff --git a/src/plugins/service-kubernetes/KubernetesDiagnosticsProvider.test.ts b/src/plugins/service-kubernetes/KubernetesDiagnosticsProvider.test.ts new file mode 100644 index 000000000..d34ad9621 --- /dev/null +++ b/src/plugins/service-kubernetes/KubernetesDiagnosticsProvider.test.ts @@ -0,0 +1,85 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See License.txt in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +import { + KubernetesDiagnosticsProvider, + rememberKubernetesCluster, + resetKubernetesClustersForTests, +} from './KubernetesDiagnosticsProvider'; +import { type KubernetesPortForwardMetadata } from './portForwardMetadata'; + +const hasTunnel = jest.fn(); + +jest.mock('./portForwardTunnel', () => ({ + PortForwardTunnelManager: { + getInstance: (): { hasTunnel: jest.Mock } => ({ hasTunnel }), + }, +})); + +const metadata: KubernetesPortForwardMetadata = { + kind: 'kubernetesClusterIpPortForward', + sourceId: 'source-1', + contextName: 'my-context', + namespace: 'databases', + serviceName: 'documentdb', + servicePort: 10260, + localPort: 51234, +}; + +describe('KubernetesDiagnosticsProvider', () => { + beforeEach(() => { + resetKubernetesClustersForTests(); + hasTunnel.mockReset(); + }); + + it('stays silent for a cluster that was never prepared by the reachability provider', async () => { + const result = await new KubernetesDiagnosticsProvider().explain({ + clusterId: 'unknown-cluster', + error: new Error('connect ECONNREFUSED 127.0.0.1:51234'), + }); + + expect(result).toBeUndefined(); + expect(hasTunnel).not.toHaveBeenCalled(); + }); + + it('reports a tunnel that is no longer up', async () => { + rememberKubernetesCluster('k8s-cluster', metadata); + hasTunnel.mockReturnValue(false); + + const result = await new KubernetesDiagnosticsProvider().explain({ + clusterId: 'k8s-cluster', + error: new Error('connect ECONNREFUSED 127.0.0.1:51234'), + }); + + expect(result).toContain('port-forward tunnel'); + expect(result).toContain('documentdb'); + expect(result).toContain('51234'); + expect(hasTunnel).toHaveBeenCalledWith('source-1', 'my-context', 'databases', 'documentdb', 51234); + }); + + it('points past a live tunnel when the service does not answer', async () => { + rememberKubernetesCluster('k8s-cluster', metadata); + hasTunnel.mockReturnValue(true); + + const result = await new KubernetesDiagnosticsProvider().explain({ + clusterId: 'k8s-cluster', + error: new Error('connect ECONNREFUSED 127.0.0.1:51234'), + }); + + expect(result).toContain('looks active'); + }); + + it('stays silent for a live tunnel and a non-transport failure', async () => { + rememberKubernetesCluster('k8s-cluster', metadata); + hasTunnel.mockReturnValue(true); + + await expect( + new KubernetesDiagnosticsProvider().explain({ + clusterId: 'k8s-cluster', + error: new Error('bad auth : Authentication failed.'), + }), + ).resolves.toBeUndefined(); + }); +}); diff --git a/src/plugins/service-kubernetes/KubernetesDiagnosticsProvider.ts b/src/plugins/service-kubernetes/KubernetesDiagnosticsProvider.ts new file mode 100644 index 000000000..96f723497 --- /dev/null +++ b/src/plugins/service-kubernetes/KubernetesDiagnosticsProvider.ts @@ -0,0 +1,96 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See License.txt in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +/** + * Error translation for Kubernetes ClusterIP connections reached through a local port-forward + * tunnel. + * + * TRANSLATION ONLY. This provider must never show UI, restart a tunnel, or retry the failed + * operation; it returns text so the user can tell a dead tunnel from a database problem. + * See `src/services/connectionDiagnosticsService.ts` and + * `.github/skills/error-translation/SKILL.md`. + */ + +import * as l10n from '@vscode/l10n'; +import { + type ConnectionDiagnosticsProvider, + type ConnectionDiagnosticsRequest, +} from '../../services/connectionDiagnosticsService'; +import { type KubernetesPortForwardMetadata } from './portForwardMetadata'; + +/** + * `clusterId` to port-forward metadata, recorded by {@link KubernetesReachabilityProvider} while it + * prepares a connection. + * + * The stored connection properties (where this metadata lives) never travel past the tree item, so + * a failure raised from a webview, the shell or the playground cannot look them up. Keeping the + * mapping inside this plugin avoids a central origin registry: the reachability provider already + * runs at exactly the right moment and already holds both halves. + */ +const knownClusters = new Map(); + +/** Nothing answered on the socket, as opposed to a failure the server did answer with. */ +const NO_ANSWER_SIGNATURES: ReadonlyArray = [ + /econnrefused/i, + /connection refused/i, + /econnreset/i, + /etimedout/i, + /timed? ?out/i, + /server selection/i, + /socket hang ?up/i, +]; + +export function rememberKubernetesCluster(clusterId: string, metadata: KubernetesPortForwardMetadata): void { + knownClusters.set(clusterId, metadata); +} + +/** Test-only: drops the recorded mappings so suites start from a known state. */ +export function resetKubernetesClustersForTests(): void { + knownClusters.clear(); +} + +export class KubernetesDiagnosticsProvider implements ConnectionDiagnosticsProvider { + public readonly id = 'kubernetes-port-forward'; + + public async explain({ clusterId, error }: ConnectionDiagnosticsRequest): Promise { + const metadata = knownClusters.get(clusterId); + if (!metadata) { + return undefined; + } + + const target = l10n.t('"{service}" in namespace "{namespace}"', { + service: metadata.serviceName, + namespace: metadata.namespace, + }); + + // Heavy dependency, so it is only pulled in once we know the cluster is one of ours. + const { PortForwardTunnelManager } = await import('./portForwardTunnel'); + const isTunnelUp = PortForwardTunnelManager.getInstance().hasTunnel( + metadata.sourceId, + metadata.contextName, + metadata.namespace, + metadata.serviceName, + metadata.localPort, + ); + + if (!isTunnelUp) { + return l10n.t( + 'We cannot find an active port-forward tunnel to {target}, so localhost:{port} very likely does not reach the cluster right now. Collapse and expand the connection again to re-establish the tunnel.', + { target, port: String(metadata.localPort) }, + ); + } + + // The tunnel looks up, so a transport failure points past it, at the service or the pod. + const message = error instanceof Error ? error.message : String(error); + if (NO_ANSWER_SIGNATURES.some((signature) => signature.test(message))) { + return l10n.t( + 'The port-forward tunnel to {target} looks active, but the service did not answer. The pod behind it may have restarted or been rescheduled.', + { target }, + ); + } + + return undefined; + } +} diff --git a/src/plugins/service-kubernetes/KubernetesReachabilityProvider.ts b/src/plugins/service-kubernetes/KubernetesReachabilityProvider.ts index 30763e1a9..cfd466594 100644 --- a/src/plugins/service-kubernetes/KubernetesReachabilityProvider.ts +++ b/src/plugins/service-kubernetes/KubernetesReachabilityProvider.ts @@ -4,6 +4,7 @@ *--------------------------------------------------------------------------------------------*/ import { type ConnectionReachabilityProvider } from '../../services/connectionReachabilityService'; +import { rememberKubernetesCluster } from './KubernetesDiagnosticsProvider'; import { getKubernetesPortForwardMetadata } from './portForwardMetadata'; /** @@ -25,12 +26,18 @@ export class KubernetesReachabilityProvider implements ConnectionReachabilityPro return getKubernetesPortForwardMetadata(connectionProperties) !== undefined; } - public async ensureReachable(connectionProperties: Record): Promise { + public async ensureReachable(connectionProperties: Record, clusterId?: string): Promise { const metadata = getKubernetesPortForwardMetadata(connectionProperties); if (!metadata) { return; } + // The only moment where both the clusterId and this connection's port-forward metadata are + // known; recording the pair lets KubernetesDiagnosticsProvider explain later failures. + if (clusterId) { + rememberKubernetesCluster(clusterId, metadata); + } + const { ensureKubernetesPortForward } = await import('./ensureKubernetesPortForward'); await ensureKubernetesPortForward(metadata); } diff --git a/src/plugins/service-kubernetes/discovery-tree/documentdb/KubernetesResourceItem.ts b/src/plugins/service-kubernetes/discovery-tree/documentdb/KubernetesResourceItem.ts index ffaa81a2c..ca51f9c69 100644 --- a/src/plugins/service-kubernetes/discovery-tree/documentdb/KubernetesResourceItem.ts +++ b/src/plugins/service-kubernetes/discovery-tree/documentdb/KubernetesResourceItem.ts @@ -36,6 +36,7 @@ import { type KubeServiceEndpoint, type KubeServiceInfo, } from '../../kubernetesClient'; +import { rememberKubernetesCluster } from '../../KubernetesDiagnosticsProvider'; import { KUBERNETES_PORT_FORWARD_METADATA_PROPERTY, createKubernetesPortForwardMetadata, @@ -446,15 +447,19 @@ export class KubernetesResourceItem extends ClusterItemBase ({ + callWithTelemetryAndErrorHandling: jest.fn(), + UserCancelledError: class UserCancelledError extends Error {}, +})); + +function provider(id: string, explain: ConnectionDiagnosticsProvider['explain']): ConnectionDiagnosticsProvider { + return { id, explain }; +} + +describe('ConnectionDiagnosticsService', () => { + beforeEach(() => { + ConnectionDiagnosticsService.resetForTests(); + }); + + afterEach(() => { + ConnectionDiagnosticsService.resetForTests(); + jest.useRealTimers(); + }); + + it('returns undefined when no provider is registered', async () => { + await expect( + ConnectionDiagnosticsService.explain({ clusterId: 'c1', error: new Error('boom') }), + ).resolves.toBeUndefined(); + }); + + it('returns the first non-undefined explanation and stops asking', async () => { + const second = jest.fn().mockResolvedValue('second'); + ConnectionDiagnosticsService.registerProvider(provider('a', () => Promise.resolve(undefined))); + ConnectionDiagnosticsService.registerProvider(provider('b', () => Promise.resolve('from b'))); + ConnectionDiagnosticsService.registerProvider(provider('c', second)); + + await expect( + ConnectionDiagnosticsService.explain({ clusterId: 'c1', error: new Error('boom') }), + ).resolves.toEqual({ providerId: 'b', message: 'from b' }); + expect(second).not.toHaveBeenCalled(); + }); + + it('skips a throwing provider instead of failing the caller', async () => { + ConnectionDiagnosticsService.registerProvider(provider('a', () => Promise.reject(new Error('provider bug')))); + ConnectionDiagnosticsService.registerProvider(provider('b', () => Promise.resolve('from b'))); + + await expect( + ConnectionDiagnosticsService.explain({ clusterId: 'c1', error: new Error('boom') }), + ).resolves.toEqual({ providerId: 'b', message: 'from b' }); + }); + + it('gives up on a provider that never settles so the original error can still be reported', async () => { + jest.useFakeTimers(); + ConnectionDiagnosticsService.registerProvider(provider('slow', () => new Promise(() => {}))); + + const pending = ConnectionDiagnosticsService.explain({ clusterId: 'c1', error: new Error('boom') }); + await jest.advanceTimersByTimeAsync(5_000); + + await expect(pending).resolves.toBeUndefined(); + }); + + it('spends one deadline in total, not one per provider', async () => { + jest.useFakeTimers(); + const stall = (): Promise => new Promise(() => {}); + ConnectionDiagnosticsService.registerProvider(provider('a', stall)); + ConnectionDiagnosticsService.registerProvider(provider('b', stall)); + ConnectionDiagnosticsService.registerProvider(provider('c', stall)); + + const pending = ConnectionDiagnosticsService.explain({ clusterId: 'c1', error: new Error('boom') }); + await jest.advanceTimersByTimeAsync(5_000); + + await expect(pending).resolves.toBeUndefined(); + }); + + // The deadline used to be a plain race, which leaves the losing side running: later providers + // kept being queried, and an answer arriving after the caller had already been handed + // `undefined` was still reported as an explanation. + it('stops querying providers once the deadline has passed', async () => { + jest.useFakeTimers(); + let releaseFirst: (() => void) | undefined; + const first = jest.fn( + () => + new Promise((resolve) => { + releaseFirst = () => resolve('too late'); + }), + ); + const second = jest.fn().mockResolvedValue('second'); + ConnectionDiagnosticsService.registerProvider(provider('first', first)); + ConnectionDiagnosticsService.registerProvider(provider('second', second)); + + const pending = ConnectionDiagnosticsService.explain({ clusterId: 'c1', error: new Error('boom') }); + await jest.advanceTimersByTimeAsync(5_000); + await expect(pending).resolves.toBeUndefined(); + + jest.mocked(callWithTelemetryAndErrorHandling).mockClear(); + // The slow provider answers after the caller has given up. + releaseFirst?.(); + await jest.advanceTimersByTimeAsync(1); + + expect(second).not.toHaveBeenCalled(); + expect(callWithTelemetryAndErrorHandling).not.toHaveBeenCalled(); + }); + + it('replaces a provider registered twice under the same id', async () => { + ConnectionDiagnosticsService.registerProvider(provider('a', () => Promise.resolve('first'))); + ConnectionDiagnosticsService.registerProvider(provider('a', () => Promise.resolve('second'))); + + await expect( + ConnectionDiagnosticsService.explain({ clusterId: 'c1', error: new Error('boom') }), + ).resolves.toEqual({ providerId: 'a', message: 'second' }); + }); + + it('never modifies the error it was given', async () => { + ConnectionDiagnosticsService.registerProvider(provider('a', () => Promise.resolve('explained'))); + + class CustomError extends Error { + public readonly code = 'ECONNREFUSED'; + } + const error = new CustomError('raw driver text'); + const originalMessage = error.message; + + await ConnectionDiagnosticsService.explain({ clusterId: 'c1', error }); + + expect(error.message).toBe(originalMessage); + expect(error).toBeInstanceOf(CustomError); + expect(error.code).toBe('ECONNREFUSED'); + expect(error.cause).toBeUndefined(); + }); + + it('stays silent for a cancellation, without consulting any provider', async () => { + const explain = jest.fn().mockResolvedValue('should not be used'); + ConnectionDiagnosticsService.registerProvider(provider('a', explain)); + + await expect( + ConnectionDiagnosticsService.explain({ clusterId: 'c1', error: new UserCancelledError() }), + ).resolves.toBeUndefined(); + expect(explain).not.toHaveBeenCalled(); + }); +}); diff --git a/src/services/connectionDiagnosticsService.ts b/src/services/connectionDiagnosticsService.ts new file mode 100644 index 000000000..b5d66d08d --- /dev/null +++ b/src/services/connectionDiagnosticsService.ts @@ -0,0 +1,219 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See License.txt in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +/** + * Translates a failed database operation into an explanation the user can act on, when the cause + * is infrastructure that a provider owns: a container that is no longer running, a port-forward + * tunnel that is no longer up, a TLS handshake the service closed. + * + * ## What this is NOT + * + * This is a TRANSLATION layer, not a recovery layer. Providers registered here must never: + * + * - show a dialog, notification, progress indicator, or any other UI; + * - start, stop, restart, or repair anything; + * - retry the operation that failed, or ask the caller to retry it; + * - prompt the user for input. + * + * They receive an error and return text. Nothing else. + * + * The reason is that one user action often runs several database commands, several actions can + * fail at once, and many of these calls happen on background paths. A provider that shows UI or + * repairs state would produce duplicate dialogs, dialogs the user never asked for, and errors that + * are already obsolete by the time they are displayed. Keeping providers text-only makes all of + * those failure modes impossible by construction. + * + * Anything with a side effect belongs at the CALL SITE, because only the call site knows whether + * the user is watching, whether the operation was a read or a write, and which surface (modal, + * toast, tree node, terminal line) is appropriate. + * + * ## The error is never touched + * + * {@link ConnectionDiagnosticsServiceImpl.explain} returns text and nothing more. It never mutates + * the error, never replaces it, and never attaches properties to it. That is deliberate: a lot of + * code in this repository inspects errors by IDENTITY rather than by text, and all of it would + * break in ways that are hard to notice. + * + * - `instanceof UserCancelledError` decides whether an outcome is a failure or a cancellation; + * - `instanceof QueryError`, `MongoBulkWriteError` and `SettingsHintError` change how a failure is + * handled; + * - `error.code` is read for server codes (115, 235) and socket codes (ECONNRESET, ENOTFOUND); + * - `errorCodeExtractor.ts` reads `error.cause.cause.code` at a FIXED depth, so an extra wrapper + * level would silently break Collection view error-code detection; + * - `extractErrorCode()` parses a `[CODE-12345]` prefix from the START of a message, so prepending + * text would break the shell and the playground; + * - the tRPC boundary rebuilds errors as `{ code, name, message, stack, cause }`, so a custom + * property would not reach a webview anyway. + * + * Leaving the error alone means there is exactly one rule to remember, and it is not a protocol + * about error objects: if you render a database failure, ask {@link explain} first. + * + * ## Relationship to ConnectionReachabilityService + * + * {@link import('./connectionReachabilityService').ConnectionReachabilityService} PREPARES a + * connection before we connect. This service EXPLAINS a failure afterwards. They are deliberately + * separate: `ensureReachable` runs on every connect attempt and must stay silent and cheap, while + * `explain` runs only on failure paths and is allowed a small amount of I/O. + * + * @see .github/skills/error-translation/SKILL.md + */ + +import { callWithTelemetryAndErrorHandling, UserCancelledError } from '@microsoft/vscode-azext-utils'; +import { ext } from '../extensionVariables'; + +export interface ConnectionDiagnosticsRequest { + /** + * The stable cluster identifier, never a `treeId`. This is the only identity that reaches + * every call site (tree items, webviews, the shell, the playground), which is why it is the + * sole key providers get to work with. + */ + readonly clusterId: string; + + /** + * The error the database operation failed with. + * + * Usually an `Error`, but a webview can only send the MESSAGE across the tRPC boundary, so this + * is a plain `string` on that path. A provider that needs an error's class or `code` therefore + * cannot be served from a webview. + */ + readonly error: unknown; +} + +/** + * Turns an infrastructure-caused failure into an explanation. + * + * Implementations MUST NOT show UI, recover, or retry. See the file header: this interface exists + * only to translate errors so users understand what went wrong. + */ +export interface ConnectionDiagnosticsProvider { + /** Stable identifier. Internal only; used for telemetry and de-duplicated registration. */ + readonly id: string; + + /** + * Returns a localized explanation, or `undefined` when this provider does not own the cluster, + * or owns it and sees nothing wrong. `undefined` means "the caller should show the original + * error unchanged", so returning it is always the safe answer. + * + * Implementations should answer the cheap question first (do I own this cluster? does the + * error even look like mine?) so the common case costs close to nothing. + */ + explain(request: ConnectionDiagnosticsRequest): Promise; +} + +export interface ConnectionDiagnosis { + readonly providerId: string; + readonly message: string; +} + +/** + * Budget for one {@link ConnectionDiagnosticsServiceImpl.explain} call, not per provider: the user + * is already waiting for an error, so the wait must not grow with the number of registered sources. + * On expiry we fall back to the original error, which is always a valid outcome. + */ +const EXPLAIN_DEADLINE_MS = 5_000; + +/** + * Registry of {@link ConnectionDiagnosticsProvider}s. + * + * Mirrors the singleton-registry pattern used by `ConnectionReachabilityService`, `DiscoveryService` + * and `MigrationService`: providers are registered once at activation, and call sites simply ask + * "can anyone explain this failure?" without knowing which sources exist. + * + * This class cannot be instantiated directly; use the exported {@link ConnectionDiagnosticsService} + * singleton instead. + */ +class ConnectionDiagnosticsServiceImpl { + private readonly providers: ConnectionDiagnosticsProvider[] = []; + + /** + * Registers a diagnostics provider. A provider with an id that is already registered replaces + * the existing one (last registration wins), which keeps re-activation idempotent. + */ + public registerProvider(provider: ConnectionDiagnosticsProvider): void { + const existingIndex = this.providers.findIndex((candidate) => candidate.id === provider.id); + if (existingIndex >= 0) { + this.providers[existingIndex] = provider; + } else { + this.providers.push(provider); + } + } + + /** + * Asks every registered provider, in registration order, until one returns an explanation. + * + * Never throws and never rejects: a provider that fails or stalls is skipped so the caller can + * still report the original error. Callers are expected to invoke this only from foreground + * paths; background work (tree count badges, prefetches) shows nothing, so translating there + * would cost I/O for no user-visible benefit. + */ + public async explain(request: ConnectionDiagnosticsRequest): Promise { + // Guarded centrally rather than per provider: a provider is allowed to answer without + // inspecting the error at all, so without this a cancelled wizard on a stopped container + // would be reported as an infrastructure failure. + if (request.error instanceof UserCancelledError) { + return undefined; + } + + // The deadline has to reach the loop, not just race it: an abandoned `Promise.race` loser + // keeps querying providers and would report an answer the caller has already stopped + // waiting for. + const expiry = new AbortController(); + const timer = setTimeout(() => expiry.abort(), EXPLAIN_DEADLINE_MS); + try { + return await Promise.race([ + this.askProviders(request, expiry.signal), + new Promise((resolve) => expiry.signal.addEventListener('abort', () => resolve(undefined))), + ]); + } finally { + clearTimeout(timer); + expiry.abort(); + } + } + + private async askProviders( + request: ConnectionDiagnosticsRequest, + expired: AbortSignal, + ): Promise { + for (const provider of this.providers) { + if (expired.aborted) { + return undefined; + } + + let message: string | undefined; + + try { + message = await provider.explain(request); + } catch (error) { + const detail = error instanceof Error ? error.message : String(error); + ext.outputChannel?.debug(`[ConnectionDiagnostics] Provider "${provider.id}" failed: ${detail}`); + continue; + } + + // Checked again after the await: the caller has already been handed `undefined`, so + // reporting this as an explanation would record an answer nobody received. + if (expired.aborted) { + return undefined; + } + + if (message) { + void callWithTelemetryAndErrorHandling('connectionDiagnostics.explained', (context) => { + context.telemetry.properties.diagnosisProviderId = provider.id; + }); + return { providerId: provider.id, message }; + } + } + + return undefined; + } + + /** + * Test-only: clears all registered providers so suites start from a known state. + */ + public resetForTests(): void { + this.providers.length = 0; + } +} + +export const ConnectionDiagnosticsService = new ConnectionDiagnosticsServiceImpl(); diff --git a/src/services/connectionReachabilityService.test.ts b/src/services/connectionReachabilityService.test.ts index d2a14978e..a1c31f030 100644 --- a/src/services/connectionReachabilityService.test.ts +++ b/src/services/connectionReachabilityService.test.ts @@ -31,10 +31,19 @@ describe('ConnectionReachabilityService', () => { await ConnectionReachabilityService.ensureReachable({ some: 'props' }); expect(appliesEnsure).toHaveBeenCalledTimes(1); - expect(appliesEnsure).toHaveBeenCalledWith({ some: 'props' }); + expect(appliesEnsure).toHaveBeenCalledWith({ some: 'props' }, undefined); expect(skipsEnsure).not.toHaveBeenCalled(); }); + it('forwards the clusterId so a provider can record which cluster it prepared', async () => { + const ensure = jest.fn().mockResolvedValue(undefined); + ConnectionReachabilityService.registerProvider(makeProvider('applies', () => true, ensure)); + + await ConnectionReachabilityService.ensureReachable({ some: 'props' }, 'cluster-42'); + + expect(ensure).toHaveBeenCalledWith({ some: 'props' }, 'cluster-42'); + }); + it('is a no-op when connection properties are undefined', async () => { const ensure = jest.fn().mockResolvedValue(undefined); ConnectionReachabilityService.registerProvider(makeProvider('any', () => true, ensure)); diff --git a/src/services/connectionReachabilityService.ts b/src/services/connectionReachabilityService.ts index d547acab3..97c226084 100644 --- a/src/services/connectionReachabilityService.ts +++ b/src/services/connectionReachabilityService.ts @@ -37,8 +37,14 @@ export interface ConnectionReachabilityProvider { * port-forward tunnel). Only called when {@link appliesTo} returned true. May be a no-op if * the connection is already reachable. Heavy, source-specific dependencies should be loaded * lazily inside this method so registering the provider stays cheap. + * + * @param clusterId The stable cluster identifier, when the caller knows it. Providers may use + * this to record a `clusterId` to source-metadata mapping, so that a later failure against the + * same cluster can be attributed back to this source by a + * {@link import('./connectionDiagnosticsService').ConnectionDiagnosticsProvider}. The stored + * connection properties are not available on those later paths. */ - ensureReachable(connectionProperties: Record): Promise; + ensureReachable(connectionProperties: Record, clusterId?: string): Promise; } /** @@ -75,14 +81,17 @@ class ConnectionReachabilityServiceImpl { * connect flow), where it is reported via the existing telemetry/error handling. Connections * with no applicable provider resolve immediately. */ - public async ensureReachable(connectionProperties: Record | undefined): Promise { + public async ensureReachable( + connectionProperties: Record | undefined, + clusterId?: string, + ): Promise { if (!connectionProperties) { return; } for (const provider of this.providers) { if (provider.appliesTo(connectionProperties)) { - await provider.ensureReachable(connectionProperties); + await provider.ensureReachable(connectionProperties, clusterId); } } } diff --git a/src/services/localQuickStart/QuickStartDiagnosticsProvider.test.ts b/src/services/localQuickStart/QuickStartDiagnosticsProvider.test.ts new file mode 100644 index 000000000..bea0ac5ea --- /dev/null +++ b/src/services/localQuickStart/QuickStartDiagnosticsProvider.test.ts @@ -0,0 +1,95 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See License.txt in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +import { QuickStartDiagnosticsProvider } from './QuickStartDiagnosticsProvider'; +import { QuickStartService } from './QuickStartService'; +import { InstanceState, type InstanceStatus } from './quickStartTypes'; + +function status(clusterId: string, alias = 'default'): InstanceStatus { + return { + alias, + displayName: 'DocumentDB Local', + state: InstanceState.Running, + missing: false, + canResumeReadiness: false, + metadata: { clusterId } as InstanceStatus['metadata'], + }; +} + +describe('QuickStartDiagnosticsProvider', () => { + afterEach(() => { + jest.restoreAllMocks(); + }); + + it('stays silent for a cluster it does not manage, without probing Docker', async () => { + jest.spyOn(QuickStartService, 'listStatuses').mockReturnValue([status('quickstart-cluster')]); + const preflight = jest.spyOn(QuickStartService, 'inspectManagedInstance'); + + const result = await new QuickStartDiagnosticsProvider().explain({ + clusterId: 'some-other-cluster', + error: new Error('boom'), + }); + + expect(result).toBeUndefined(); + expect(preflight).not.toHaveBeenCalled(); + }); + + it.each([ + ['stopped', 'not appear to be running'], + ['missing', 'cannot find the DocumentDB Local container'], + ['foreign', 'not created by this extension'], + ['unavailable', 'cannot reach DocumentDB Local'], + ['dockerUnreachable', 'Docker does not appear to be running'], + ] as const)('explains a %s container', async (verdict, expected) => { + jest.spyOn(QuickStartService, 'listStatuses').mockReturnValue([status('quickstart-cluster')]); + jest.spyOn(QuickStartService, 'inspectManagedInstance').mockResolvedValue(verdict); + + const result = await new QuickStartDiagnosticsProvider().explain({ + clusterId: 'quickstart-cluster', + error: new Error('boom'), + }); + + expect(result).toContain(expected); + }); + + it.each(['ready', 'busy'] as const)('stays silent when the container is %s', async (verdict) => { + jest.spyOn(QuickStartService, 'listStatuses').mockReturnValue([status('quickstart-cluster')]); + jest.spyOn(QuickStartService, 'inspectManagedInstance').mockResolvedValue(verdict); + + await expect( + new QuickStartDiagnosticsProvider().explain({ + clusterId: 'quickstart-cluster', + error: new Error('boom'), + }), + ).resolves.toBeUndefined(); + }); + + it('uses the read-only probe, so it never corrects state or shows a warning', async () => { + jest.spyOn(QuickStartService, 'listStatuses').mockReturnValue([status('quickstart-cluster')]); + const readOnly = jest.spyOn(QuickStartService, 'inspectManagedInstance').mockResolvedValue('foreign'); + const preflight = jest.spyOn(QuickStartService, 'prepareForConnection'); + + await new QuickStartDiagnosticsProvider().explain({ clusterId: 'quickstart-cluster', error: new Error('x') }); + + expect(readOnly).toHaveBeenCalledWith('default'); + expect(preflight).not.toHaveBeenCalled(); + }); + + it('re-checks on every failure so a container the user just started is reported as running', async () => { + jest.spyOn(QuickStartService, 'listStatuses').mockReturnValue([status('quickstart-cluster')]); + const preflight = jest + .spyOn(QuickStartService, 'inspectManagedInstance') + .mockResolvedValueOnce('stopped') + .mockResolvedValueOnce('ready'); + const provider = new QuickStartDiagnosticsProvider(); + + const first = await provider.explain({ clusterId: 'quickstart-cluster', error: new Error('boom') }); + const second = await provider.explain({ clusterId: 'quickstart-cluster', error: new Error('boom') }); + + expect(first).toContain('not appear to be running'); + expect(second).toBeUndefined(); + expect(preflight).toHaveBeenCalledTimes(2); + }); +}); diff --git a/src/services/localQuickStart/QuickStartDiagnosticsProvider.ts b/src/services/localQuickStart/QuickStartDiagnosticsProvider.ts new file mode 100644 index 000000000..6e977380f --- /dev/null +++ b/src/services/localQuickStart/QuickStartDiagnosticsProvider.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. + *--------------------------------------------------------------------------------------------*/ + +/** + * Error translation for the managed DocumentDB Local instance. + * + * TRANSLATION ONLY. This provider must never show UI, start or stop the container, or retry the + * failed operation; it returns text so the user can tell a Docker problem from a database problem. + * See `src/services/connectionDiagnosticsService.ts` and + * `.github/skills/error-translation/SKILL.md`. + */ + +import * as l10n from '@vscode/l10n'; +import { type ConnectionDiagnosticsProvider, type ConnectionDiagnosticsRequest } from '../connectionDiagnosticsService'; +import { QuickStartService } from './QuickStartService'; + +export class QuickStartDiagnosticsProvider implements ConnectionDiagnosticsProvider { + public readonly id = 'localQuickStart'; + + public async explain({ clusterId }: ConnectionDiagnosticsRequest): Promise { + // In-memory lookup, so this costs nothing for the clusters that are not Quick Start ones. + const alias = QuickStartService.listStatuses().find( + (status) => status.metadata?.clusterId === clusterId, + )?.alias; + + if (!alias) { + return undefined; + } + + // Deliberately not memoized: this runs once per user-initiated failure, and a cached verdict + // would keep reporting "not running" right after the user started the container. + // + // The error shape does not matter here: if the container is not running, that accounts for + // any failure against it. If it is running, we stay quiet and the original error stands. + switch (await QuickStartService.inspectManagedInstance(alias)) { + case 'stopped': + return l10n.t( + 'DocumentDB Local does not appear to be running. Start it from the Connections view, then try again.', + ); + case 'missing': + return l10n.t( + 'We cannot find the DocumentDB Local container. It was very likely removed outside VS Code. You can recreate it from the Connections view, which reuses the existing data volume.', + ); + case 'foreign': + return l10n.t( + 'We found a container using the DocumentDB Local name, but it very likely was not created by this extension, so we cannot open it.', + ); + case 'dockerUnreachable': + return l10n.t( + 'Docker does not appear to be running, so DocumentDB Local cannot be reached. Start Docker, then try again.', + ); + case 'unavailable': + return l10n.t( + 'We cannot reach DocumentDB Local at the moment. Review its setup in the Connections view.', + ); + case 'busy': + case 'ready': + default: + return undefined; + } + } +} 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 6a321a11a..b15159f53 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, @@ -62,6 +63,8 @@ function mockRuntime(overrides: Partial): IContainerRuntime { return { listByLabel: jest.fn().mockResolvedValue([]), inspectContainer: jest.fn().mockResolvedValue(undefined), + // Docker answering normally is the default, so an empty inspect means the container is gone. + isDockerReady: jest.fn().mockResolvedValue({ outcome: 'ready', daemonReachable: true }), removeContainer: jest.fn().mockResolvedValue(undefined), removeVolume: jest.fn().mockResolvedValue(undefined), isPortFree: jest.fn().mockResolvedValue(true), @@ -243,15 +246,21 @@ describe('QuickStartService — WI-2d registry-driven reconcile (multi-instance) let originalSecretStorage: vscode.SecretStorage; let originalContext: vscode.ExtensionContext; + let originalOutputChannel: typeof ext.outputChannel; + let trace: jest.Mock; beforeEach(() => { originalSecretStorage = ext.secretStorage; originalContext = ext.context; + originalOutputChannel = ext.outputChannel; + trace = jest.fn(); + ext.outputChannel = { trace } as unknown as typeof ext.outputChannel; }); afterEach(() => { ext.secretStorage = originalSecretStorage; ext.context = originalContext; + ext.outputChannel = originalOutputChannel; }); function inspectItem(id: string, opts: { running: boolean; port?: number; image?: string }): unknown { @@ -266,6 +275,7 @@ describe('QuickStartService — WI-2d registry-driven reconcile (multi-instance) function reconcileRuntime(opts: { containers: Array<{ id: string; alias?: string; createdAt?: Date }>; inspect?: Record; + isDockerReady?: jest.Mock; removeContainer?: jest.Mock; removeVolume?: jest.Mock; }): IContainerRuntime { @@ -281,6 +291,7 @@ describe('QuickStartService — WI-2d registry-driven reconcile (multi-instance) inspectContainer: jest.fn((id: string) => Promise.resolve(inspect[id]), ) as unknown as IContainerRuntime['inspectContainer'], + ...(opts.isDockerReady ? { isDockerReady: opts.isDockerReady } : {}), removeContainer: opts.removeContainer ?? jest.fn().mockResolvedValue(undefined), removeVolume: opts.removeVolume ?? jest.fn().mockResolvedValue(undefined), }); @@ -313,6 +324,33 @@ describe('QuickStartService — WI-2d registry-driven reconcile (multi-instance) expect((await listInstances()).map((record) => record.alias).sort()).toEqual([ALIAS_2, DEFAULT_ALIAS].sort()); }); + it('retains Docker host facts collected during reconciliation', async () => { + ext.secretStorage = fakeSecretStorage({}); + ext.context = fakeContext(fakeMemento()); + const readiness: DockerReadiness = { + outcome: 'ready', + environment: 'wsl', + endpointKind: 'unixSocket', + provider: 'dockerEngine', + providerEvidence: 'liveDaemon', + executionTarget: 'wsl', + canContinueAnyway: false, + checkedAtMs: 1, + cliInstalled: true, + cliVersion: 'Docker version 28.1.1', + daemonReachable: true, + osType: 'linux', + daemonArchitecture: 'amd64', + }; + const isDockerReady = jest.fn().mockResolvedValue(readiness); + const service = new QuickStartServiceImpl(reconcileRuntime({ containers: [], isDockerReady })); + + await service.reconcile(); + + expect(isDockerReady).toHaveBeenCalledWith({ suppressCommandEcho: true }); + expect(service.getDockerReadinessSnapshot()).toBe(readiness); + }); + it('surfaces a credential-unavailable instance as CredentialsMissing without removing it or its volume (R2)', async () => { ext.secretStorage = fakeSecretStorage({}); // no secret for ALIAS_2 ext.context = fakeContext(fakeMemento()); @@ -424,6 +462,161 @@ describe('QuickStartService — WI-2d registry-driven reconcile (multi-instance) expect(service.getStatus().missing).toBe(true); }); + // `inspectContainer` reports "could not ask" and "not there" identically, so a stopped daemon + // used to be announced as a container someone had deleted — the tree then offered to recreate + // an instance that was sitting on disk, untouched. + it('refreshLiveState() does not report Missing when Docker cannot be asked', async () => { + ext.secretStorage = fakeSecretStorage({}); + ext.context = fakeContext(fakeMemento()); + await seedInstance(DEFAULT_ALIAS, CONN_1); + + const inspect: Record = { + c1: inspectItem('c1', { running: true, port: 10260, image: 'img:1' }), + }; + const isDockerReady = jest.fn().mockResolvedValue({ outcome: 'ready', daemonReachable: true }); + const service = new QuickStartServiceImpl( + mockRuntime({ + listByLabel: jest + .fn() + .mockResolvedValue([{ id: 'c1', labels: { [QUICK_START_ALIAS_LABEL_KEY]: DEFAULT_ALIAS } }]), + inspectContainer: jest.fn((id: string) => + Promise.resolve(inspect[id]), + ) as unknown as IContainerRuntime['inspectContainer'], + isDockerReady: isDockerReady as unknown as IContainerRuntime['isDockerReady'], + }), + ); + + await service.reconcile(); + expect(service.getStatus().state).toBe(InstanceState.Running); + + // Docker Desktop is stopped: the container is still there, we simply cannot see it. + delete inspect.c1; + isDockerReady.mockResolvedValue({ outcome: 'diagnosed', daemonReachable: false }); + + await service.refreshLiveState(); + + expect(service.getStatus().missing).toBe(false); + // The last known state is kept rather than replaced by a guess. + expect(service.getStatus().state).toBe(InstanceState.Running); + }); + + it('ensureHydrated() lazily reconciles once and shares concurrent work', async () => { + ext.secretStorage = fakeSecretStorage({}); + ext.context = fakeContext(fakeMemento()); + + let finishListing: ((containers: []) => void) | undefined; + const listByLabel = jest.fn( + () => + new Promise<[]>((resolve) => { + finishListing = resolve; + }), + ); + const service = new QuickStartServiceImpl(mockRuntime({ listByLabel })); + + expect(listByLabel).not.toHaveBeenCalled(); + const first = service.ensureHydrated(); + const second = service.ensureHydrated(); + expect(listByLabel).toHaveBeenCalledTimes(1); + + finishListing?.([]); + await Promise.all([first, second]); + await service.ensureHydrated(); + + expect(listByLabel).toHaveBeenCalledTimes(1); + expect(trace).toHaveBeenCalledWith(expect.stringContaining('Lazy hydration requested')); + expect(trace).toHaveBeenCalledWith( + expect.stringContaining('Discovery returned 0 managed container(s) and 0 durable record(s)'), + ); + expect(trace).toHaveBeenCalledWith(expect.stringContaining('Lazy hydration completed')); + }); + + it('ensureHydrated() remains retryable when Docker discovery fails', async () => { + ext.secretStorage = fakeSecretStorage({}); + ext.context = fakeContext(fakeMemento()); + + const listByLabel = jest.fn().mockRejectedValueOnce(new Error('Docker unavailable')).mockResolvedValue([]); + const service = new QuickStartServiceImpl(mockRuntime({ listByLabel })); + + await expect(service.ensureHydrated()).rejects.toThrow('Docker unavailable'); + expect(service.isHydrated).toBe(false); + expect(trace).toHaveBeenCalledWith(expect.stringContaining('Docker state remains unknown')); + expect(trace).toHaveBeenCalledWith(expect.stringContaining('the next Quick Start entry will retry')); + + await service.ensureHydrated(); + expect(service.isHydrated).toBe(true); + expect(listByLabel).toHaveBeenCalledTimes(2); + }); + + it('shares deep reconciliation between hydration and explicit refresh', async () => { + ext.secretStorage = fakeSecretStorage({}); + ext.context = fakeContext(fakeMemento()); + + let finishListing: ((containers: []) => void) | undefined; + const listByLabel = jest.fn( + () => + new Promise<[]>((resolve) => { + finishListing = resolve; + }), + ); + const service = new QuickStartServiceImpl(mockRuntime({ listByLabel })); + + const hydration = service.ensureHydrated(); + const refresh = service.refreshHydratedState(); + expect(listByLabel).toHaveBeenCalledTimes(1); + + finishListing?.([]); + await Promise.all([hydration, refresh]); + + expect(service.isHydrated).toBe(true); + expect(listByLabel).toHaveBeenCalledTimes(1); + }); + + it('does not start a background live-state probe immediately after explicit refresh', async () => { + ext.secretStorage = fakeSecretStorage({}); + ext.context = fakeContext(fakeMemento()); + + const service = new QuickStartServiceImpl(mockRuntime({})); + + await service.refreshHydratedState(); + service.refreshLiveStateInBackground(); + + expect(service.isRefreshingLiveState).toBe(false); + }); + + it('does not start a background live-state probe immediately after lazy hydration', async () => { + ext.secretStorage = fakeSecretStorage({}); + ext.context = fakeContext(fakeMemento()); + await seedInstance(DEFAULT_ALIAS, CONN_1); + + const inspectContainer = jest.fn((id: string) => + Promise.resolve({ + id, + status: 'running', + ports: [{ containerPort: QUICK_START_PORT, hostPort: 10260 }], + image: { originalName: 'img:1' }, + labels: { [QUICK_START_LABEL_KEY]: '1', [QUICK_START_ALIAS_LABEL_KEY]: DEFAULT_ALIAS }, + }), + ) as unknown as IContainerRuntime['inspectContainer']; + const service = new QuickStartServiceImpl( + mockRuntime({ + listByLabel: jest + .fn() + .mockResolvedValue([{ id: 'c1', labels: { [QUICK_START_ALIAS_LABEL_KEY]: DEFAULT_ALIAS } }]), + inspectContainer, + }), + ); + + await service.ensureHydrated(); + const inspectsDuringHydration = jest.mocked(inspectContainer).mock.calls.length; + + // The status events fired during reconciliation re-enter getChildren() once hydration is + // done, so the row would otherwise re-inspect the container it just adopted. + service.refreshLiveStateInBackground(); + + expect(service.isRefreshingLiveState).toBe(false); + expect(inspectContainer).toHaveBeenCalledTimes(inspectsDuringHydration); + }); + it('refreshLiveStateInBackground() de-duplicates and rate-limits the docker probe (M6)', async () => { ext.secretStorage = fakeSecretStorage({}); ext.context = fakeContext(fakeMemento()); @@ -460,6 +653,58 @@ describe('QuickStartService — WI-2d registry-driven reconcile (multi-instance) expect(inspectContainer).toHaveBeenCalledTimes(1); }); + it('publishes an awaitable handle for in-flight lifecycle work', async () => { + ext.secretStorage = fakeSecretStorage({}); + ext.context = fakeContext(fakeMemento()); + await seedInstance(DEFAULT_ALIAS, CONN_1); + + let finishStop!: () => void; + const stopContainer = jest.fn( + () => + new Promise((resolve) => { + finishStop = resolve; + }), + ); + const service = new QuickStartServiceImpl( + mockRuntime({ + listByLabel: jest + .fn() + .mockResolvedValue([{ id: 'c1', labels: { [QUICK_START_ALIAS_LABEL_KEY]: DEFAULT_ALIAS } }]), + inspectContainer: jest.fn((id: string) => + Promise.resolve({ + id, + status: 'running', + ports: [{ containerPort: QUICK_START_PORT, hostPort: 10260 }], + image: { originalName: 'img:1' }, + labels: { [QUICK_START_LABEL_KEY]: '1', [QUICK_START_ALIAS_LABEL_KEY]: DEFAULT_ALIAS }, + }), + ) as unknown as IContainerRuntime['inspectContainer'], + stopContainer, + }), + ); + + await service.reconcile(); + expect(service.getStatus().state).toBe(InstanceState.Running); + + let operationEvents = 0; + service.onDidChangeOperation(() => operationEvents++); + + // The tree cannot own the spinner for work it did not start (the webview and the lifecycle + // commands both reach here), so the wait itself has to be observable. + const stopping = service.stop(); + const operation = service.getInFlightOperation(); + expect(operation?.kind).toBe('stopping'); + expect(operationEvents).toBe(1); + + await new Promise((resolve) => setTimeout(resolve, 0)); + finishStop(); + await stopping; + + await expect(operation?.promise).resolves.toBeUndefined(); + expect(service.getInFlightOperation()).toBeUndefined(); + expect(operationEvents).toBe(2); + }); + it('deleteContainer() refuses to remove a container that is not ours, even when surfaced as Missing (#9)', async () => { ext.secretStorage = fakeSecretStorage({}); ext.context = fakeContext(fakeMemento()); @@ -523,7 +768,7 @@ describe('QuickStartService — WI-2d registry-driven reconcile (multi-instance) warn.mockRestore(); }); - it('start() on a container that drifted to running in another window refreshes without starting', async () => { + it('start() on a container that drifted to running refreshes silently without starting', async () => { ext.secretStorage = fakeSecretStorage({}); ext.context = fakeContext(fakeMemento()); await seedInstance(DEFAULT_ALIAS, CONN_1); @@ -559,12 +804,265 @@ describe('QuickStartService — WI-2d registry-driven reconcile (multi-instance) await service.start(); expect(startContainer).not.toHaveBeenCalled(); // start on an already-running container is a no-op - expect(info).toHaveBeenCalled(); // the user is told the state changed + expect(info).not.toHaveBeenCalled(); expect(service.getStatus().state).toBe(InstanceState.Running); // corrected to the live state expect(service.getStatus().missing).toBe(false); info.mockRestore(); }); + it('stop() on a container that drifted to stopped refreshes silently without stopping', async () => { + ext.secretStorage = fakeSecretStorage({}); + ext.context = fakeContext(fakeMemento()); + await seedInstance(DEFAULT_ALIAS, CONN_1); + + const stopContainer = jest.fn().mockResolvedValue(undefined); + let running = true; + const service = new QuickStartServiceImpl( + mockRuntime({ + listByLabel: jest + .fn() + .mockResolvedValue([{ id: 'c1', labels: { [QUICK_START_ALIAS_LABEL_KEY]: DEFAULT_ALIAS } }]), + inspectContainer: jest.fn((id: string) => + Promise.resolve({ + id, + status: running ? 'running' : 'exited', + ports: [{ containerPort: QUICK_START_PORT, hostPort: 10260 }], + image: { originalName: 'img:1' }, + labels: { [QUICK_START_LABEL_KEY]: '1', [QUICK_START_ALIAS_LABEL_KEY]: DEFAULT_ALIAS }, + }), + ) as unknown as IContainerRuntime['inspectContainer'], + stopContainer, + }), + ); + + await service.reconcile(); + running = false; + const info = jest.spyOn(vscode.window, 'showInformationMessage').mockResolvedValue(undefined); + + await service.stop(); + + expect(stopContainer).not.toHaveBeenCalled(); + expect(info).not.toHaveBeenCalled(); + expect(service.getStatus().state).toBe(InstanceState.Stopped); + info.mockRestore(); + }); + + it('prepareForConnection() updates an externally stopped container and rejects the connection', async () => { + ext.secretStorage = fakeSecretStorage({}); + ext.context = fakeContext(fakeMemento()); + await seedInstance(DEFAULT_ALIAS, CONN_1); + + let running = true; + const service = new QuickStartServiceImpl( + mockRuntime({ + listByLabel: jest + .fn() + .mockResolvedValue([{ id: 'c1', labels: { [QUICK_START_ALIAS_LABEL_KEY]: DEFAULT_ALIAS } }]), + inspectContainer: jest.fn((id: string) => + Promise.resolve({ + id, + status: running ? 'running' : 'exited', + ports: [{ containerPort: QUICK_START_PORT, hostPort: 10260 }], + image: { originalName: 'img:1' }, + labels: { [QUICK_START_LABEL_KEY]: '1', [QUICK_START_ALIAS_LABEL_KEY]: DEFAULT_ALIAS }, + }), + ) as unknown as IContainerRuntime['inspectContainer'], + }), + ); + + await service.reconcile(); + running = false; + + await expect(service.prepareForConnection()).resolves.toBe('stopped'); + expect(service.getStatus().state).toBe(InstanceState.Stopped); + expect(service.getStatus().missing).toBe(false); + }); + + it('prepareForConnection() accepts an owned running container', async () => { + ext.secretStorage = fakeSecretStorage({}); + ext.context = fakeContext(fakeMemento()); + await seedInstance(DEFAULT_ALIAS, CONN_1); + + const service = new QuickStartServiceImpl( + mockRuntime({ + listByLabel: jest + .fn() + .mockResolvedValue([{ id: 'c1', labels: { [QUICK_START_ALIAS_LABEL_KEY]: DEFAULT_ALIAS } }]), + inspectContainer: jest.fn((id: string) => + Promise.resolve({ + id, + status: 'running', + ports: [{ containerPort: QUICK_START_PORT, hostPort: 10260 }], + image: { originalName: 'img:1' }, + labels: { [QUICK_START_LABEL_KEY]: '1', [QUICK_START_ALIAS_LABEL_KEY]: DEFAULT_ALIAS }, + }), + ) as unknown as IContainerRuntime['inspectContainer'], + }), + ); + + await service.reconcile(); + + await expect(service.prepareForConnection()).resolves.toBe('ready'); + expect(service.getStatus().state).toBe(InstanceState.Running); + }); + + it('prepareForConnection() marks an externally removed container as Missing', async () => { + ext.secretStorage = fakeSecretStorage({}); + ext.context = fakeContext(fakeMemento()); + await seedInstance(DEFAULT_ALIAS, CONN_1); + + let present = true; + const service = new QuickStartServiceImpl( + mockRuntime({ + listByLabel: jest + .fn() + .mockResolvedValue([{ id: 'c1', labels: { [QUICK_START_ALIAS_LABEL_KEY]: DEFAULT_ALIAS } }]), + inspectContainer: jest.fn((id: string) => + Promise.resolve( + present + ? { + id, + status: 'running', + ports: [{ containerPort: QUICK_START_PORT, hostPort: 10260 }], + image: { originalName: 'img:1' }, + labels: { + [QUICK_START_LABEL_KEY]: '1', + [QUICK_START_ALIAS_LABEL_KEY]: DEFAULT_ALIAS, + }, + } + : undefined, + ), + ) as unknown as IContainerRuntime['inspectContainer'], + }), + ); + + await service.reconcile(); + present = false; + + await expect(service.prepareForConnection()).resolves.toBe('missing'); + expect(service.getStatus().missing).toBe(true); + }); + + it('inspectManagedInstance() reports the same verdict without correcting state or firing events', async () => { + ext.secretStorage = fakeSecretStorage({}); + ext.context = fakeContext(fakeMemento()); + await seedInstance(DEFAULT_ALIAS, CONN_1); + + let present = true; + const service = new QuickStartServiceImpl( + mockRuntime({ + listByLabel: jest + .fn() + .mockResolvedValue([{ id: 'c1', labels: { [QUICK_START_ALIAS_LABEL_KEY]: DEFAULT_ALIAS } }]), + inspectContainer: jest.fn((id: string) => + Promise.resolve( + present + ? { + id, + status: 'running', + ports: [{ containerPort: QUICK_START_PORT, hostPort: 10260 }], + image: { originalName: 'img:1' }, + labels: { + [QUICK_START_LABEL_KEY]: '1', + [QUICK_START_ALIAS_LABEL_KEY]: DEFAULT_ALIAS, + }, + } + : undefined, + ), + ) as unknown as IContainerRuntime['inspectContainer'], + }), + ); + + await service.reconcile(); + present = false; + const statusChanged = jest.fn(); + service.onDidChangeStatus(statusChanged); + + await expect(service.inspectManagedInstance()).resolves.toBe('missing'); + expect(service.getStatus().missing).toBe(false); + expect(statusChanged).not.toHaveBeenCalled(); + }); + + it('prepareForConnection() does not claim the container was removed when the Docker daemon is down', async () => { + ext.secretStorage = fakeSecretStorage({}); + ext.context = fakeContext(fakeMemento()); + await seedInstance(DEFAULT_ALIAS, CONN_1); + + let daemonUp = true; + const service = new QuickStartServiceImpl( + mockRuntime({ + listByLabel: jest + .fn() + .mockResolvedValue([{ id: 'c1', labels: { [QUICK_START_ALIAS_LABEL_KEY]: DEFAULT_ALIAS } }]), + inspectContainer: jest.fn((id: string) => + Promise.resolve( + daemonUp + ? { + id, + status: 'running', + ports: [{ containerPort: QUICK_START_PORT, hostPort: 10260 }], + image: { originalName: 'img:1' }, + labels: { + [QUICK_START_LABEL_KEY]: '1', + [QUICK_START_ALIAS_LABEL_KEY]: DEFAULT_ALIAS, + }, + } + : undefined, + ), + ) as unknown as IContainerRuntime['inspectContainer'], + isDockerReady: jest + .fn() + .mockImplementation(() => + Promise.resolve( + daemonUp + ? { outcome: 'ready', daemonReachable: true } + : { outcome: 'diagnosed', daemonReachable: false }, + ), + ) as unknown as IContainerRuntime['isDockerReady'], + }), + ); + + await service.reconcile(); + daemonUp = false; + + await expect(service.prepareForConnection()).resolves.toBe('dockerUnreachable'); + expect(service.getStatus().missing).toBe(false); + }); + + it('prepareForConnection() rejects a foreign container that reused the managed id', async () => { + ext.secretStorage = fakeSecretStorage({}); + ext.context = fakeContext(fakeMemento()); + await seedInstance(DEFAULT_ALIAS, CONN_1); + + let owned = true; + const service = new QuickStartServiceImpl( + mockRuntime({ + listByLabel: jest + .fn() + .mockResolvedValue([{ id: 'c1', labels: { [QUICK_START_ALIAS_LABEL_KEY]: DEFAULT_ALIAS } }]), + inspectContainer: jest.fn((id: string) => + Promise.resolve({ + id, + status: 'running', + ports: [{ containerPort: QUICK_START_PORT, hostPort: 10260 }], + image: { originalName: 'img:1' }, + labels: owned + ? { [QUICK_START_LABEL_KEY]: '1', [QUICK_START_ALIAS_LABEL_KEY]: DEFAULT_ALIAS } + : {}, + }), + ) as unknown as IContainerRuntime['inspectContainer'], + }), + ); + + await service.reconcile(); + owned = false; + const warning = jest.spyOn(vscode.window, 'showWarningMessage').mockResolvedValue(undefined); + + await expect(service.prepareForConnection()).resolves.toBe('foreign'); + expect(warning).toHaveBeenCalled(); + warning.mockRestore(); + }); + it('scavenges a STALE provisioning reservation that never produced a container', async () => { ext.secretStorage = fakeSecretStorage({}); const globalState = fakeMemento(); @@ -1029,9 +1527,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 }); @@ -1071,7 +1570,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 () => { @@ -1103,7 +1602,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(); }); @@ -1149,15 +1651,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 3ef847c46..ab9e2b2f6 100644 --- a/src/services/localQuickStart/QuickStartService.ts +++ b/src/services/localQuickStart/QuickStartService.ts @@ -31,6 +31,7 @@ import { AuthMethodId } from '../../documentdb/auth/AuthMethod'; import { ClustersClient } from '../../documentdb/ClustersClient'; import { CredentialCache } from '../../documentdb/CredentialCache'; import { DocumentDBConnectionString } from '../../documentdb/utils/DocumentDBConnectionString'; +import { ext } from '../../extensionVariables'; import { ContainerRuntime, getBoundHostPort, @@ -77,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, @@ -86,23 +89,8 @@ import { /** Stable cache key for CredentialCache / ClustersClient (the default instance). Ephemeral. */ export const QUICK_START_CLUSTER_ID = clusterId(DEFAULT_ALIAS); -/** - * 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), - ); +function traceQuickStart(message: string): void { + ext.outputChannel?.trace(`[LocalQuickStart] ${message}`); } /** @@ -173,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'; } } @@ -214,9 +202,37 @@ interface InstanceRuntimeState { lifecycleBusy: boolean; missing: boolean; pendingReadiness?: PendingReadiness; - errorMessage?: string; + error?: QuickStartMessage; + inFlight?: QuickStartOperation; +} + +/** Long-running work the tree renders progress for. */ +export type QuickStartOperationKind = + | 'provisioning' + | 'starting' + | 'stopping' + | 'restarting' + | 'deleting' + | 'refreshing'; + +/** + * An awaitable handle on in-flight work. The tree hands it to the framework's node-progress state + * (`ext.state.runWithTemporaryDescription`) instead of rendering its own spinner rows. + */ +export interface QuickStartOperation { + readonly kind: QuickStartOperationKind; + readonly promise: Promise; } +export type QuickStartConnectionPreflightResult = + | 'ready' + | 'stopped' + | 'missing' + | 'foreign' + | 'busy' + | 'unavailable' + | 'dockerUnreachable'; + /** * Resolve the credentials for a fresh provision: honor custom Advanced credentials * when BOTH a username and password are supplied (whitespace-only is treated as not @@ -235,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. */ @@ -285,6 +291,12 @@ export class QuickStartServiceImpl { */ private readonly instances = new Map(); + /** First authoritative durable-store/Docker reconciliation, shared by all Quick Start entry points. */ + private hydration: Promise | undefined; + private reconciliation: Promise | undefined; + private hydrated = false; + private dockerReadiness: DockerReadiness | undefined; + /** Lazily get (creating a NotInstalled default for) an alias's runtime state. */ private stateFor(alias: string): InstanceRuntimeState { let entry = this.instances.get(alias); @@ -306,18 +318,69 @@ export class QuickStartServiceImpl { /** Fires whenever the managed-instance status changes (drives the tree). */ public readonly onDidChangeStatus = this.statusEmitter.event; + private readonly operationEmitter = new vscode.EventEmitter(); + /** + * Fires when long-running work starts or finishes. Deliberately separate from + * {@link onDidChangeStatus}, whose listeners rebuild the whole Connections view. + */ + public readonly onDidChangeOperation = this.operationEmitter.event; + + /** + * Publish an awaitable handle for work that is about to start; the returned callback settles it. + * Callers keep their own `provisioning` / `lifecycleBusy` guards — this only exposes the wait. + */ + private beginOperation(alias: string, kind: QuickStartOperationKind): () => void { + const entry = this.stateFor(alias); + let settle!: () => void; + const promise = new Promise((resolve) => { + settle = resolve; + }); + entry.inFlight = { kind, promise }; + this.operationEmitter.fire(); + return () => { + if (entry.inFlight?.promise === promise) { + entry.inFlight = undefined; + } + settle(); + this.operationEmitter.fire(); + }; + } + + /** The long-running work currently in flight for `alias`, if any. */ + public getInFlightOperation(alias: string = DEFAULT_ALIAS): QuickStartOperation | undefined { + const entry = this.stateFor(alias); + if (entry.inFlight) { + return entry.inFlight; + } + return this.backgroundRefresh ? { kind: 'refreshing', promise: this.backgroundRefresh } : undefined; + } + /** * @param runtime Docker IO surface (WI-0). Defaults to the shared {@link ContainerRuntime} * singleton; tests inject a mock so the state machine runs with no real daemon. */ constructor(private readonly runtime: IContainerRuntime = ContainerRuntime) {} + /** Latest Docker host facts collected by setup or deep reconciliation. */ + public getDockerReadinessSnapshot(): DockerReadiness | undefined { + return this.dockerReadiness; + } + + /** Check Docker and retain the result for tree presentation. */ + public async checkDockerReadiness( + request?: Parameters[0], + ): Promise { + const readiness = await this.runtime.isDockerReady(request); + this.dockerReadiness = readiness; + return readiness; + } + public getStatus(alias: string = DEFAULT_ALIAS): QuickStartStatus { const entry = this.stateFor(alias); 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. @@ -359,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, }; @@ -377,16 +440,75 @@ export class QuickStartServiceImpl { public dispose(): void { this.statusEmitter.dispose(); + this.operationEmitter.dispose(); } - private setStatus(alias: string, state: InstanceState, metadata?: InstanceMetadata, errorMessage?: string): void { + /** + * Lazily rebuild runtime state from durable storage and Docker. Concurrent callers share the + * same work, and later callers use the hydrated in-memory state until an explicit reconcile. + */ + public async ensureHydrated(): Promise { + if (this.hydrated) { + return; + } + + if (!this.hydration) { + traceQuickStart('Lazy hydration requested; starting deep reconciliation.'); + this.hydration = this.reconcile() + .then(() => { + this.hydrated = true; + // Arms the background-probe cooldown: reconcile just produced an authoritative + // answer, and the status events it fired re-enter getChildren() once hydration + // is done, where an unarmed cooldown would re-inspect the same container. + this.lastBackgroundRefreshAt = Date.now(); + traceQuickStart('Lazy hydration completed.'); + }) + .catch((error: unknown) => { + traceQuickStart('Lazy hydration failed; the next Quick Start entry will retry.'); + throw error; + }) + .finally(() => { + this.hydration = undefined; + }); + } else { + traceQuickStart('Lazy hydration joined the in-flight request.'); + } + + await this.hydration; + } + + /** Whether the initial durable-store/Docker reconciliation has completed. */ + public get isHydrated(): boolean { + return this.hydrated; + } + + /** Force an authoritative refresh for an explicit Quick Start refresh action. */ + public async refreshHydratedState(): Promise { + traceQuickStart('Explicit node refresh requested; starting deep reconciliation.'); + try { + await this.reconcile(); + this.hydrated = true; + this.lastBackgroundRefreshAt = Date.now(); + traceQuickStart('Explicit node refresh completed.'); + } catch (error) { + traceQuickStart('Explicit node refresh failed; the next explicit refresh will retry.'); + throw error; + } + } + + 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(); } @@ -410,11 +532,11 @@ 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; + const endOperation = this.beginOperation(alias, 'provisioning'); // Starting a fresh run supersedes any container left running by a prior readiness // timeout — drop its retained "Wait longer" state (the run below removes the container). this.stateFor(alias).pendingReadiness = undefined; @@ -489,18 +611,14 @@ export class QuickStartServiceImpl { this.stateFor(alias).port = chosenPort; // --- checking --- - yield stageEvent('checking', 'active', 'Checking Docker…'); - const readiness = await this.runtime.isDockerReady(); + 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 @@ -518,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; } } @@ -540,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); @@ -559,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; @@ -567,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); } @@ -611,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 @@ -622,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). @@ -660,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) @@ -684,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. @@ -697,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 @@ -798,6 +900,7 @@ export class QuickStartServiceImpl { telemetryContext.telemetry.measurements.provisionMs = Date.now() - provisionStartedAt; }); this.stateFor(alias).provisioning = false; + endOperation(); } // Emitted only now — after `finally` cleared `provisioning` — so a "Wait longer" / "Start // over" / "Retry" click triggered by this event never races the still-running guard. @@ -808,7 +911,7 @@ export class QuickStartServiceImpl { private async getProvisioningDockerReadiness(): Promise { try { - const readiness = await this.runtime.isDockerReady({ forceRefresh: true }); + const readiness = await this.checkDockerReadiness({ forceRefresh: true }); return readiness.outcome === 'diagnosed' ? readiness : undefined; } catch { return undefined; @@ -878,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) { @@ -887,14 +989,11 @@ 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; + const endOperation = this.beginOperation(alias, 'provisioning'); const cts = new vscode.CancellationTokenSource(); const onAbort = (): void => cts.cancel(); signal.addEventListener('abort', onAbort, { once: true }); @@ -907,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); @@ -920,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) { @@ -934,9 +1032,13 @@ 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); + // 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' } + : isTimeout + ? { key: 'readinessTimeout', environment: this.dockerReadiness?.environment } + : { key: 'unexpectedFailure', detail: errMessage(error) }; if (!finalized) { this.setStatus(alias, InstanceState.Error, undefined, aborted ? undefined : message); } @@ -946,13 +1048,14 @@ 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. cts.cancel(); cts.dispose(); this.stateFor(alias).provisioning = false; + endOperation(); // §14: resume outcome — booleans/enum + duration only, never names/ports/creds. void callWithTelemetryAndErrorHandling('documentDB.quickstart.resumeReadiness', (telemetryContext) => { telemetryContext.errorHandling.suppressDisplay = true; @@ -1334,23 +1437,109 @@ export class QuickStartServiceImpl { } const live: 'running' | 'stopped' = isRunning(item) ? 'running' : 'stopped'; if (!allowed.includes(live)) { - // Multi-window drift: another window already started/stopped it. Correct the state - // immediately (setStatus clears missing + fires) and tell the user. + // Multi-window / external drift: the requested outcome is already satisfied. Correct + // the state immediately and return quietly rather than distracting the user with a + // notification for a successful no-op. this.setStatus(alias, live === 'running' ? InstanceState.Running : InstanceState.Stopped); - void vscode.window.showInformationMessage( - l10n.t( - 'The DocumentDB Local instance changed in another window (now {0}). The view has been refreshed.', - live === 'running' ? l10n.t('running') : l10n.t('stopped'), - ), - ); return false; } return true; } + /** + * Read-only verdict on a managed instance: no state correction, no events, no UI. Split out of + * {@link prepareForConnection} so the error-translation provider, which must not do any of + * those things, has something safe to call. + */ + public async inspectManagedInstance(alias: string = DEFAULT_ALIAS): Promise { + const entry = this.stateFor(alias); + const containerId = entry.metadata?.containerId; + if (entry.provisioning || entry.lifecycleBusy) { + return 'busy'; + } + if (!containerId || entry.state === InstanceState.CredentialsMissing) { + return 'unavailable'; + } + + const inspected = await this.runtime.inspectContainer(containerId); + if (entry.metadata?.containerId !== containerId) { + return 'busy'; + } + if (!inspected) { + // `inspectContainer` reports "could not ask" and "not there" the same way, so a stopped + // daemon would otherwise be announced as a container someone deleted. + return (await this.classifyUninspectableContainer()) ?? 'missing'; + } + if (!this.isOwnedContainer(inspected, alias)) { + return 'foreign'; + } + return isRunning(inspected) ? 'ready' : 'stopped'; + } + + /** + * Authoritatively validate a managed instance immediately before a tree expansion connects. + * Unlike the root row's background freshness probe, this check blocks only explicit connection + * intent so stale `Running` state can never reach the database client. + * + * Unlike {@link inspectManagedInstance} this corrects the in-memory state and warns about a + * foreign container, so it belongs on paths where the user is waiting for the outcome. + */ + public async prepareForConnection(alias: string = DEFAULT_ALIAS): Promise { + const verdict = await this.inspectManagedInstance(alias); + const entry = this.stateFor(alias); + + switch (verdict) { + case 'missing': + if (!entry.missing) { + entry.missing = true; + this.statusEmitter.fire(); + } + break; + case 'foreign': + void vscode.window.showWarningMessage( + l10n.t( + 'The DocumentDB Local container can no longer be opened because it was created outside the extension. Remove it with Docker if you no longer need it.', + ), + ); + break; + case 'ready': + case 'stopped': { + const nextState = verdict === 'ready' ? InstanceState.Running : InstanceState.Stopped; + if (entry.missing || entry.state !== nextState) { + this.setStatus(alias, nextState); + } + break; + } + default: + break; + } + + return verdict; + } + + /** + * Why an inspect came back empty, when the answer is not "the container is gone": `undefined` + * means Docker answered normally, so the container really has been removed. + */ + private async classifyUninspectableContainer(): Promise<'dockerUnreachable' | 'unavailable' | undefined> { + let readiness: DockerReadiness; + try { + readiness = await this.checkDockerReadiness({ forceRefresh: true, suppressCommandEcho: true }); + } catch { + return 'unavailable'; + } + + if (readiness.daemonReachable) { + return undefined; + } + // An indeterminate probe (a timeout) is not evidence that Docker is down, so it only earns + // the neutral wording. + return readiness.outcome === 'diagnosed' ? 'dockerUnreachable' : 'unavailable'; + } + /** Start a stopped instance (design §11). */ public async start(alias: string = DEFAULT_ALIAS): Promise { - await this.runLifecycle(alias, async () => { + await this.runLifecycle(alias, 'starting', async () => { const id = this.stateFor(alias).metadata?.containerId; if (!id || !(await this.ensureActionable(id, alias, ['stopped']))) { return; @@ -1360,19 +1549,14 @@ 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' }); } }); } /** Stop a running instance (design §11). */ public async stop(alias: string = DEFAULT_ALIAS): Promise { - await this.runLifecycle(alias, async () => { + await this.runLifecycle(alias, 'stopping', async () => { const id = this.stateFor(alias).metadata?.containerId; if (!id || !(await this.ensureActionable(id, alias, ['running']))) { return; @@ -1385,7 +1569,7 @@ export class QuickStartServiceImpl { /** Restart (stop + start) a running instance (design §11). */ public async restart(alias: string = DEFAULT_ALIAS): Promise { - await this.runLifecycle(alias, async () => { + await this.runLifecycle(alias, 'restarting', async () => { const id = this.stateFor(alias).metadata?.containerId; if (!id || !(await this.ensureActionable(id, alias, ['running', 'stopped']))) { return; @@ -1397,12 +1581,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' }); } }); } @@ -1443,80 +1622,84 @@ export class QuickStartServiceImpl { * volume). Returns to NotInstalled. */ public async deleteContainer(alias: string = DEFAULT_ALIAS): Promise<'deleted' | 'refused' | 'busy' | 'error'> { - const outcome = await this.runLifecycle(alias, async (): Promise<'deleted' | 'refused' | 'error'> => { - const entry = this.stateFor(alias); - // #9 guard: if we hold a specific container id/name, re-inspect it first. inspectContainer - // swallows Docker errors and returns undefined, so an undefined result is inconclusive - // here — but a RESOLVED foreign container (a name our old container no longer owns) must - // NEVER be removed: refuse, leave OUR records intact, and let the command surface the - // refusal instead of a false "deleted". - const knownId = entry.metadata?.containerId; - if (knownId) { - const inspected = await this.runtime.inspectContainer(knownId); - if (inspected && !this.isOwnedContainer(inspected, alias)) { - void vscode.window.showWarningMessage( - l10n.t( - 'The DocumentDB Local container was not removed because it was created outside the extension. Remove it with Docker if you no longer need it.', - ), - ); - return 'refused'; + const outcome = await this.runLifecycle( + alias, + 'deleting', + async (): Promise<'deleted' | 'refused' | 'error'> => { + const entry = this.stateFor(alias); + // #9 guard: if we hold a specific container id/name, re-inspect it first. inspectContainer + // swallows Docker errors and returns undefined, so an undefined result is inconclusive + // here — but a RESOLVED foreign container (a name our old container no longer owns) must + // NEVER be removed: refuse, leave OUR records intact, and let the command surface the + // refusal instead of a false "deleted". + const knownId = entry.metadata?.containerId; + if (knownId) { + const inspected = await this.runtime.inspectContainer(knownId); + if (inspected && !this.isOwnedContainer(inspected, alias)) { + void vscode.window.showWarningMessage( + l10n.t( + 'The DocumentDB Local container was not removed because it was created outside the extension. Remove it with Docker if you no longer need it.', + ), + ); + return 'refused'; + } } - } - // Authoritatively resolve OUR containers by label + alias. Unlike inspectContainer, - // listByLabel does NOT swallow errors: a Docker FAILURE throws (so "cannot verify" is never - // mistaken for "already gone", which would wipe records for a still-live container and - // resurface it as a credential-missing ghost — GPT-5.6 review), an empty result means our - // container is confirmed gone, and any hit is a container we created. This also covers a - // stale metadata id whose container was externally replaced by a new labelled same-alias - // one: we remove the LIVE container, not the stale id. - let owned: Array<{ id: string }>; - try { - owned = await this.findManagedContainers(alias, { propagateErrors: true }); - } catch (error) { - return this.reportDeleteFailure(error); - } - // Delete is a full clean slate: remove EVERY label-matched container, not just one. A - // cross-window double-create can leave more than one managed container for the alias - // (reconcile adopts the newest and LEAVES the rest — see pickManagedContainer); removing - // only the first would strand a survivor that resurfaces as a credential-missing ghost. - for (const container of owned) { - // Do NOT swallow a real removal failure on OUR container: if Docker refuses to remove - // it (daemon error, permissions, etc.), the container may remain, so we must not claim - // success or wipe our records. Surface the error and keep the instance so the user can - // retry Delete (GPT-5.6 review: the toast must reflect the ACTUAL outcome). + // Authoritatively resolve OUR containers by label + alias. Unlike inspectContainer, + // listByLabel does NOT swallow errors: a Docker FAILURE throws (so "cannot verify" is never + // mistaken for "already gone", which would wipe records for a still-live container and + // resurface it as a credential-missing ghost — GPT-5.6 review), an empty result means our + // container is confirmed gone, and any hit is a container we created. This also covers a + // stale metadata id whose container was externally replaced by a new labelled same-alias + // one: we remove the LIVE container, not the stale id. + let owned: Array<{ id: string }>; try { - await this.runtime.removeContainer(container.id); + owned = await this.findManagedContainers(alias, { propagateErrors: true }); } catch (error) { return this.reportDeleteFailure(error); } - } - // owned is empty ⇒ our container is confirmed gone; fall through to clear OUR data volume + - // records (the clean-slate Delete of a Missing / already-removed instance). - // Explicit Delete is a full clean slate: drop the data volume too (alias-derived ⇒ ours by - // construction). The container — the only resurrection vector — is now gone, so a volume - // removal failure cannot bring the instance back; it only orphans data that the next - // same-alias provision reclaims. Surface it as a non-blocking warning (not a silent - // swallow) and still complete the delete rather than stranding a container-less instance. - const volumeRemoved = await this.runtime - .removeVolume(volumeName(alias)) - .then(() => true) - .catch(() => false); - if (!volumeRemoved) { - void vscode.window.showWarningMessage( - l10n.t( - 'The DocumentDB Local container was deleted, but its data volume could not be removed. You can remove it with Docker.', - ), - ); - } - // Drop the instance's record AND its credentials in one write — an explicit Delete is a - // full clean slate, so it no longer appears when the tree enumerates instances. - await removeInstance(alias); - await ClustersClient.deleteClient(clusterId(alias)).catch(() => undefined); - CredentialCache.deleteCredentials(clusterId(alias)); - entry.metadata = undefined; - this.setStatus(alias, InstanceState.NotInstalled); - return 'deleted'; - }); + // Delete is a full clean slate: remove EVERY label-matched container, not just one. A + // cross-window double-create can leave more than one managed container for the alias + // (reconcile adopts the newest and LEAVES the rest — see pickManagedContainer); removing + // only the first would strand a survivor that resurfaces as a credential-missing ghost. + for (const container of owned) { + // Do NOT swallow a real removal failure on OUR container: if Docker refuses to remove + // it (daemon error, permissions, etc.), the container may remain, so we must not claim + // success or wipe our records. Surface the error and keep the instance so the user can + // retry Delete (GPT-5.6 review: the toast must reflect the ACTUAL outcome). + try { + await this.runtime.removeContainer(container.id); + } catch (error) { + return this.reportDeleteFailure(error); + } + } + // owned is empty ⇒ our container is confirmed gone; fall through to clear OUR data volume + + // records (the clean-slate Delete of a Missing / already-removed instance). + // Explicit Delete is a full clean slate: drop the data volume too (alias-derived ⇒ ours by + // construction). The container — the only resurrection vector — is now gone, so a volume + // removal failure cannot bring the instance back; it only orphans data that the next + // same-alias provision reclaims. Surface it as a non-blocking warning (not a silent + // swallow) and still complete the delete rather than stranding a container-less instance. + const volumeRemoved = await this.runtime + .removeVolume(volumeName(alias)) + .then(() => true) + .catch(() => false); + if (!volumeRemoved) { + void vscode.window.showWarningMessage( + l10n.t( + 'The DocumentDB Local container was deleted, but its data volume could not be removed. You can remove it with Docker.', + ), + ); + } + // Drop the instance's record AND its credentials in one write — an explicit Delete is a + // full clean slate, so it no longer appears when the tree enumerates instances. + await removeInstance(alias); + await ClustersClient.deleteClient(clusterId(alias)).catch(() => undefined); + CredentialCache.deleteCredentials(clusterId(alias)); + entry.metadata = undefined; + this.setStatus(alias, InstanceState.NotInstalled); + return 'deleted'; + }, + ); // The op returns an explicit outcome; runLifecycle only yields undefined when the alias was // busy (op skipped) or a later best-effort cleanup step threw and was settled to Error. In // both cases nothing was reported deleted, so the caller must not report success. @@ -1528,7 +1711,7 @@ export class QuickStartServiceImpl { /** `Date.now()` when the last background probe settled — drives the cooldown below. */ private lastBackgroundRefreshAt = 0; - /** True while a {@link refreshLiveStateInBackground} probe is in flight (drives the tree's "Refreshing…" hint). */ + /** True while a {@link refreshLiveStateInBackground} probe is in flight. */ public get isRefreshingLiveState(): boolean { return this.backgroundRefresh !== undefined; } @@ -1556,10 +1739,12 @@ export class QuickStartServiceImpl { this.backgroundRefresh = undefined; this.lastBackgroundRefreshAt = Date.now(); // Fire unconditionally (refreshLiveState() itself only fires on a real transition): - // the row is advertising "Refreshing…" and must drop that hint. Safe because the - // cooldown above blocks the re-render from starting another probe. + // the row must drop the progress indicator. Safe because the cooldown above blocks + // the re-render from starting another probe. this.statusEmitter.fire(); + this.operationEmitter.fire(); }); + this.operationEmitter.fire(); } /** @@ -1593,6 +1778,13 @@ export class QuickStartServiceImpl { continue; } if (!inspected) { + // "Could not ask" and "not there" look identical here, so confirm the daemon is + // actually answering before claiming the container was removed — otherwise a + // stopped Docker turns the row into recreate guidance for a container that is + // still on disk. + if ((await this.classifyUninspectableContainer()) !== undefined) { + continue; + } // Container is gone — keep metadata so the user can recreate. Fire only on the // TRANSITION into `missing` (like every sibling branch below): the tree renders // this node expanded, so an unconditional fire would re-enter getChildren() → @@ -1613,79 +1805,122 @@ export class QuickStartServiceImpl { } } - private async runLifecycle(alias: string, op: () => Promise): Promise { + private async runLifecycle( + alias: string, + kind: QuickStartOperationKind, + op: () => Promise, + ): Promise { const entry = this.stateFor(alias); if (entry.provisioning || entry.lifecycleBusy) { return undefined; } entry.lifecycleBusy = true; + const endOperation = this.beginOperation(alias, kind); 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; + endOperation(); } } /** - * Activation reconciliation (design §12 / risk-review): after a window reload the in-memory state - * is lost while containers keep running. Enumerate every known instance — the union of the durable - * store and the live labelled containers (grouped by the `vscode.documentdb.alias` label; an - * absent/empty label is the DEFAULT instance) — and rebuild each alias's state. A credential-less - * labelled container is SURFACED, never removed (R2); a stale pre-create reservation (crashed host) - * is scavenged; a ready record whose container vanished becomes Missing (recoverable via recreate). + * Demand-driven reconciliation (design §12 / risk-review): after a window reload the in-memory + * state is lost while containers keep running. Enumerate every known instance — the union of the + * durable store and the live labelled containers (grouped by the `vscode.documentdb.alias` label; + * an absent/empty label is the DEFAULT instance) — and rebuild each alias's state. A + * credential-less labelled container is SURFACED, never removed (R2); a stale pre-create + * reservation (crashed host) is scavenged; a ready record whose container vanished becomes + * Missing (recoverable via recreate). */ public async reconcile(): Promise { - try { - const containers = (await this.runtime - .listByLabel({ [QUICK_START_LABEL_KEY]: '1' }) - .catch(() => [])) as Array<{ + if (!this.reconciliation) { + traceQuickStart('Deep reconciliation started.'); + this.reconciliation = this.performReconciliation() + .then(() => { + traceQuickStart('Deep reconciliation completed.'); + }) + .catch((error: unknown) => { + traceQuickStart('Deep reconciliation failed; Docker state remains unknown.'); + throw error; + }) + .finally(() => { + this.reconciliation = undefined; + }); + } else { + traceQuickStart('Deep reconciliation joined the in-flight request.'); + } + + await this.reconciliation; + } + + private async performReconciliation(): Promise { + const readiness = this.checkDockerReadiness({ suppressCommandEcho: true }).catch(() => undefined); + const containersPromise = this.runtime.listByLabel({ [QUICK_START_LABEL_KEY]: '1' }) as Promise< + Array<{ id: string; createdAt?: Date; labels?: Record; - }>; - const instances = await listInstances(); - const now = Date.now(); - - // Group live containers by alias (absent/empty alias label ⇒ DEFAULT). - const liveByAlias = new Map>(); - for (const container of containers) { - const alias = container.labels?.[QUICK_START_ALIAS_LABEL_KEY] || DEFAULT_ALIAS; - const bucket = liveByAlias.get(alias); - if (bucket) { - bucket.push(container); - } else { - liveByAlias.set(alias, [container]); - } - } + }> + >; + const [containers, instances] = await Promise.all([containersPromise, listInstances(), readiness]); + const now = Date.now(); - // The DEFAULT always exists; also reconcile every known instance and every live alias. - const aliases = new Set([ - DEFAULT_ALIAS, - ...instances.map((record) => record.alias), - ...liveByAlias.keys(), - ]); - const scavenge = new Set(); - for (const alias of aliases) { - const record = instances.find((existing) => existing.alias === alias); - const outcome = await this.reconcileAlias(alias, record, liveByAlias.get(alias) ?? [], now); - if (outcome.scavenge) { - scavenge.add(alias); - } + traceQuickStart( + `Discovery returned ${containers.length} managed container(s) and ${instances.length} durable record(s).`, + ); + + // Group live containers by alias (absent/empty alias label ⇒ DEFAULT). + const liveByAlias = new Map>(); + for (const container of containers) { + const alias = container.labels?.[QUICK_START_ALIAS_LABEL_KEY] || DEFAULT_ALIAS; + const bucket = liveByAlias.get(alias); + if (bucket) { + bucket.push(container); + } else { + liveByAlias.set(alias, [container]); } + } - // Drop stale pre-create reservations. Scavenge fires ONLY here (activation), never in the - // per-render refreshLiveState. (Adopted instances promote their own record to `ready` - // inside adoptContainer.) Staleness is re-validated inside the store's lock, so a record - // a concurrent finalize/adopt just promoted is never dropped. - if (scavenge.size > 0) { - await scavengeStaleLeases(scavenge); + // The DEFAULT always exists; also reconcile every known instance and every live alias. + const aliases = new Set([ + DEFAULT_ALIAS, + ...instances.map((record) => record.alias), + ...liveByAlias.keys(), + ]); + const scavenge = new Set(); + for (const alias of aliases) { + const record = instances.find((existing) => existing.alias === alias); + const outcome = await this.reconcileAlias(alias, record, liveByAlias.get(alias) ?? [], now); + if (outcome.scavenge) { + scavenge.add(alias); } - } catch { - // Reconciliation is best-effort; never block activation. } + + // Drop stale pre-create reservations. Scavenge fires ONLY here (deep reconciliation), never + // in the per-render refreshLiveState. (Adopted instances promote their own record to `ready` + // inside adoptContainer.) Staleness is re-validated inside the store's lock, so a record a + // concurrent finalize/adopt just promoted is never dropped. + if (scavenge.size > 0) { + await scavengeStaleLeases(scavenge); + } + + const stateCounts = new Map(); + for (const alias of aliases) { + const state = this.stateFor(alias).state; + stateCounts.set(state, (stateCounts.get(state) ?? 0) + 1); + } + const stateSummary = [...stateCounts.entries()] + .map(([state, count]) => `${state}=${count}`) + .sort() + .join(', '); + traceQuickStart(`Reconciled ${aliases.size} instance(s): ${stateSummary}.`); } /** @@ -1720,7 +1955,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 {}; } @@ -1742,7 +1977,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 {}; } @@ -1822,7 +2057,7 @@ export class QuickStartServiceImpl { } /** Singleton Quick Start service. */ -export const QuickStartService = new QuickStartServiceImpl(); +export const QuickStartService: QuickStartServiceImpl = new QuickStartServiceImpl(); /** A stale env file is one older than this; younger ones may belong to a live provision. */ const ENV_FILE_STALE_AFTER_MS = 60 * 60 * 1000; 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 new file mode 100644 index 000000000..02d6bcc80 --- /dev/null +++ b/src/services/localQuickStart/quickStartMessages.ts @@ -0,0 +1,68 @@ +/*--------------------------------------------------------------------------------------------- + * 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 { + // 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': + 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.'); + case 'unexpectedFailure': + default: + // 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.'); + } +} 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/BaseExtendedTreeDataProvider.test.ts b/src/tree/BaseExtendedTreeDataProvider.test.ts new file mode 100644 index 000000000..a3e1f12f8 --- /dev/null +++ b/src/tree/BaseExtendedTreeDataProvider.test.ts @@ -0,0 +1,121 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See License.txt in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +import { type IActionContext } from '@microsoft/vscode-azext-utils'; +import * as vscode from 'vscode'; +import { ConnectionDiagnosticsService } from '../services/connectionDiagnosticsService'; +import { BaseExtendedTreeDataProvider } from './BaseExtendedTreeDataProvider'; +import { type TreeElement } from './TreeElement'; + +jest.mock('../extensionVariables', () => ({ + ext: { + state: { wrapItemInStateHandling: (item: unknown) => item }, + }, +})); + +/** Minimal concrete provider: the behaviour under test lives entirely in the base class. */ +class TestProvider extends BaseExtendedTreeDataProvider { + getChildren(): Promise { + return Promise.resolve([]); + } + getTreeItem(): Promise { + return Promise.resolve({}); + } + public fetch( + element: TreeElement, + context: IActionContext, + fetchFunc: () => Promise, + ): Promise { + return this.wrapGetChildrenWithErrorAndStateHandling(element, context, fetchFunc); + } +} + +function actionContext(): IActionContext { + return { + telemetry: { properties: {}, measurements: {} }, + errorHandling: { issueProperties: {} }, + valuesToMask: [], + ui: undefined, + } as unknown as IActionContext; +} + +const clusterElement = { id: 'view/cluster/db', cluster: { clusterId: 'cluster-1' } } as unknown as TreeElement; + +describe('BaseExtendedTreeDataProvider error translation', () => { + let showErrorMessage: jest.SpyInstance; + + beforeEach(() => { + ConnectionDiagnosticsService.resetForTests(); + // The vscode mock exposes showErrorMessage as a shared jest.fn(), so its call history + // survives restoreAllMocks and has to be cleared explicitly. + showErrorMessage = jest.spyOn(vscode.window, 'showErrorMessage').mockResolvedValue(undefined); + showErrorMessage.mockClear(); + }); + + afterEach(() => { + ConnectionDiagnosticsService.resetForTests(); + jest.restoreAllMocks(); + }); + + it('shows the explanation and suppresses the default notification', async () => { + ConnectionDiagnosticsService.registerProvider({ + id: 'test', + explain: () => Promise.resolve('DocumentDB Local does not appear to be running.'), + }); + const context = actionContext(); + const failure = new Error('connect ECONNREFUSED 127.0.0.1:10260'); + + await expect(new TestProvider().fetch(clusterElement, context, () => Promise.reject(failure))).rejects.toBe( + failure, + ); + + expect(showErrorMessage).toHaveBeenCalledWith( + 'DocumentDB Local does not appear to be running. (connect ECONNREFUSED 127.0.0.1:10260)', + ); + expect(context.errorHandling.suppressDisplay).toBe(true); + expect(context.telemetry.properties.diagnosisProviderId).toBe('test'); + }); + + it('rethrows the original error object untouched', async () => { + ConnectionDiagnosticsService.registerProvider({ id: 'test', explain: () => Promise.resolve('explained') }); + + class CustomError extends Error { + public readonly code = 'ECONNREFUSED'; + } + const failure = new CustomError('raw driver text'); + + await expect( + new TestProvider().fetch(clusterElement, actionContext(), () => Promise.reject(failure)), + ).rejects.toBe(failure); + + expect(failure.message).toBe('raw driver text'); + expect(failure).toBeInstanceOf(CustomError); + expect(failure.code).toBe('ECONNREFUSED'); + }); + + it('leaves the default notification alone when no provider explains the failure', async () => { + const context = actionContext(); + + await expect( + new TestProvider().fetch(clusterElement, context, () => Promise.reject(new Error('boom'))), + ).rejects.toThrow('boom'); + + expect(showErrorMessage).not.toHaveBeenCalled(); + expect(context.errorHandling.suppressDisplay).toBeUndefined(); + }); + + it('does not consult providers for an element that has no cluster', async () => { + const explain = jest.fn().mockResolvedValue('should not be used'); + ConnectionDiagnosticsService.registerProvider({ id: 'test', explain }); + + await expect( + new TestProvider().fetch({ id: 'view/folder' } as TreeElement, actionContext(), () => + Promise.reject(new Error('boom')), + ), + ).rejects.toThrow('boom'); + + expect(explain).not.toHaveBeenCalled(); + }); +}); diff --git a/src/tree/BaseExtendedTreeDataProvider.ts b/src/tree/BaseExtendedTreeDataProvider.ts index a7f6c1d0d..2ab9fdae8 100644 --- a/src/tree/BaseExtendedTreeDataProvider.ts +++ b/src/tree/BaseExtendedTreeDataProvider.ts @@ -6,6 +6,7 @@ import { createContextValue, type IActionContext } from '@microsoft/vscode-azext-utils'; import * as vscode from 'vscode'; import { ext } from '../extensionVariables'; +import { ConnectionDiagnosticsService } from '../services/connectionDiagnosticsService'; import { dispose } from '../utils/vscodeUtils'; import { type ExtendedTreeDataProvider } from './ExtendedTreeDataProvider'; import { type TreeElement } from './TreeElement'; @@ -525,7 +526,7 @@ export abstract class BaseExtendedTreeDataProvider } // 2. Fetch the children of the current element - const children = await childrenFetchFunc(); + const children = await this.fetchChildrenWithDiagnostics(element, context, childrenFetchFunc); context.telemetry.measurements.childrenCount = children?.length ?? 0; // 3. Check if the returned children contain an error node @@ -580,6 +581,42 @@ export abstract class BaseExtendedTreeDataProvider return children; } + /** + * Single point where a failed expansion is translated into something the user can act on + * (a stopped DocumentDB Local container, a port-forward tunnel that is no longer up, an Atlas + * TLS rejection). Placed here rather than in each tree item or each view's provider, so every + * node below a cluster is covered in every view. + * + * The error itself is never modified: we only choose what to display, then rethrow it unchanged + * so telemetry and every downstream identity check keep working. Cluster nodes handle their own + * failures in `ClusterItemBase` and return error children instead of throwing, so they never + * reach this catch. + */ + private async fetchChildrenWithDiagnostics( + element: T, + context: IActionContext, + childrenFetchFunc: () => Promise, + ): Promise { + try { + return await childrenFetchFunc(); + } catch (error) { + // Cluster nodes and everything below them carry the same `cluster` model but share no + // interface, so this is structural rather than an `instanceof`. + const clusterId = (element as { cluster?: { clusterId?: string } }).cluster?.clusterId; + const diagnosis = clusterId ? await ConnectionDiagnosticsService.explain({ clusterId, error }) : undefined; + + if (diagnosis) { + context.telemetry.properties.diagnosisProviderId = diagnosis.providerId; + context.errorHandling.suppressDisplay = true; + // `detail` is only rendered for modal messages, so the raw text is appended instead. + const cause = error instanceof Error ? error.message : String(error); + void vscode.window.showErrorMessage(`${diagnosis.message} (${cause})`); + } + + throw error; + } + } + /** * Determines whether a failing element matches an error-recovery action's contextValue whitelist. * diff --git a/src/tree/connections-view/DocumentDBClusterItem.ts b/src/tree/connections-view/DocumentDBClusterItem.ts index 8d14beaeb..6f57c7642 100644 --- a/src/tree/connections-view/DocumentDBClusterItem.ts +++ b/src/tree/connections-view/DocumentDBClusterItem.ts @@ -53,7 +53,7 @@ export class DocumentDBClusterItem extends ClusterItemBase | undefined): Promise { - await ConnectionReachabilityService.ensureReachable(connectionProperties); + private async ensureConnectionReachable( + connectionProperties: Record | undefined, + clusterId?: string, + ): Promise { + await ConnectionReachabilityService.ensureReachable(connectionProperties, clusterId); } /** diff --git a/src/tree/connections-view/LocalQuickStart/LocalQuickStartItem.credentials.test.ts b/src/tree/connections-view/LocalQuickStart/LocalQuickStartItem.credentials.test.ts index 491b70516..cf71398ad 100644 --- a/src/tree/connections-view/LocalQuickStart/LocalQuickStartItem.credentials.test.ts +++ b/src/tree/connections-view/LocalQuickStart/LocalQuickStartItem.credentials.test.ts @@ -8,6 +8,7 @@ import { CredentialCache } from '../../../documentdb/CredentialCache'; import { QuickStartService } from '../../../services/localQuickStart/QuickStartService'; import { InstanceState, + type DockerReadiness, type InstanceMetadata, type QuickStartStatus, } from '../../../services/localQuickStart/quickStartTypes'; @@ -59,12 +60,13 @@ function runningStatus(): QuickStartStatus { missing: false, canResumeReadiness: false, metadata: { - containerId: 'c1', + containerId: 'deaaf74c692312345678901234567890123456789012345678901234567890', alias: ALIAS, boundPort: 10260, clusterId: CLUSTER_ID, connectionString: CONNECTION_STRING, username: 'qs_user', + imageRef: 'ghcr.io/documentdb/documentdb-local:latest', } as InstanceMetadata, } as QuickStartStatus; } @@ -90,8 +92,11 @@ describe('QuickStartClusterItem — credential source of truth (H5)', () => { beforeEach(() => { jest.clearAllMocks(); CredentialCache.deleteCredentials(CLUSTER_ID); + jest.spyOn(QuickStartService, 'ensureHydrated').mockResolvedValue(undefined); + jest.spyOn(QuickStartService, 'isHydrated', 'get').mockReturnValue(true); jest.spyOn(QuickStartService, 'refreshLiveStateInBackground').mockReturnValue(undefined); jest.spyOn(QuickStartService, 'getStatus').mockReturnValue(runningStatus()); + jest.spyOn(QuickStartService, 'prepareForConnection').mockResolvedValue('ready'); }); afterEach(() => { @@ -131,6 +136,75 @@ describe('QuickStartClusterItem — credential source of truth (H5)', () => { expect(credentials?.nativeAuthConfig).toEqual({ connectionUser: 'qs_user', connectionPassword: 's3cr3t' }); }); + it('does not connect when the authoritative container preflight rejects the stale running row', async () => { + jest.spyOn(QuickStartService, 'prepareForConnection').mockResolvedValue('unavailable'); + + const children = await (await getClusterItem()).getChildren(); + + expect(children.map((child) => child.getTreeItem())).toEqual([ + expect.objectContaining({ label: 'DocumentDB Local cannot be opened. Click here to review its setup' }), + ]); + expect(mockGetClient).not.toHaveBeenCalled(); + }); + + it('offers a Start row instead of a modal when the container is stopped', async () => { + jest.spyOn(QuickStartService, 'prepareForConnection').mockResolvedValue('stopped'); + const prompt = jest.spyOn(vscode.window, 'showInformationMessage'); + + const children = await (await getClusterItem()).getChildren(); + + expect(children.map((child) => child.getTreeItem())).toEqual([ + expect.objectContaining({ + label: 'Click here to start DocumentDB Local', + command: expect.objectContaining({ command: 'vscode-documentdb.command.localQuickStart.start' }), + }), + ]); + expect(prompt).not.toHaveBeenCalled(); + expect(mockGetClient).not.toHaveBeenCalled(); + }); + + it('explains a Docker daemon that is not answering', async () => { + jest.spyOn(QuickStartService, 'prepareForConnection').mockResolvedValue('dockerUnreachable'); + + const children = await (await getClusterItem()).getChildren(); + + expect(children.map((child) => child.getTreeItem())).toEqual([ + expect.objectContaining({ label: 'Docker does not appear to be running. Click here for details' }), + ]); + expect(mockGetClient).not.toHaveBeenCalled(); + }); + + it('shows retained Docker host and container details in the tooltip', async () => { + jest.spyOn(QuickStartService, 'getDockerReadinessSnapshot').mockReturnValue({ + outcome: 'ready', + environment: 'wsl', + endpointKind: 'unixSocket', + provider: 'dockerEngine', + providerEvidence: 'liveDaemon', + executionTarget: 'wsl', + canContinueAnyway: false, + checkedAtMs: 1, + cliInstalled: true, + cliVersion: 'Docker version 28.1.1', + daemonReachable: true, + osType: 'linux', + daemonArchitecture: 'amd64', + } as DockerReadiness); + + const tooltip = (await getClusterItem()).getTreeItem().tooltip as vscode.MarkdownString; + + expect(tooltip.value).toContain('ghcr.io/documentdb/documentdb-local:latest'); + expect(tooltip.value).toContain('**Container ID:** deaaf74c6923'); + expect(tooltip.value).not.toContain('`deaaf74c6923`'); + expect(tooltip.value).not.toContain('deaaf74c692312345678901234567890'); + expect(tooltip.value).toContain('Docker Engine'); + expect(tooltip.value).toContain('Docker version 28.1.1'); + expect(tooltip.value).toContain('amd64'); + expect(tooltip.value).toContain('WSL'); + expect(tooltip.value).toContain('Unix socket'); + expect(tooltip.value).toContain('Linux'); + }); + it('returns no credentials and no client when the secret is gone', async () => { jest.spyOn(QuickStartService, 'readStoredConnectionString').mockResolvedValue(undefined); diff --git a/src/tree/connections-view/LocalQuickStart/LocalQuickStartItem.test.ts b/src/tree/connections-view/LocalQuickStart/LocalQuickStartItem.test.ts index f61505ed7..b108a2578 100644 --- a/src/tree/connections-view/LocalQuickStart/LocalQuickStartItem.test.ts +++ b/src/tree/connections-view/LocalQuickStart/LocalQuickStartItem.test.ts @@ -3,6 +3,7 @@ * Licensed under the MIT License. See License.txt in the project root for license information. *--------------------------------------------------------------------------------------------*/ +import { TreeItemCollapsibleState } from 'vscode'; import { QuickStartService } from '../../../services/localQuickStart/QuickStartService'; import { InstanceState, type QuickStartStatus } from '../../../services/localQuickStart/quickStartTypes'; import { LocalQuickStartItem } from './LocalQuickStartItem'; @@ -11,10 +12,78 @@ import { LocalQuickStartItem } from './LocalQuickStartItem'; // item constructs without a real extension host. jest.mock('../../../utils/icons', () => ({ getResourcesPath: () => '/resources' })); +describe('LocalQuickStartItem — lazy hydration', () => { + afterEach(() => jest.restoreAllMocks()); + + it('starts collapsed and performs no Docker work while only the root row is rendered', () => { + const ensureHydrated = jest.spyOn(QuickStartService, 'ensureHydrated').mockResolvedValue(undefined); + const item = new LocalQuickStartItem('connectionsView/root'); + + expect(item.getTreeItem().collapsibleState).toBe(TreeItemCollapsibleState.Collapsed); + expect(item.getTreeItem().contextValue).toContain('treeItem_localQuickStart'); + expect(ensureHydrated).not.toHaveBeenCalled(); + }); + + it('awaits first hydration without starting a redundant background probe', async () => { + jest.spyOn(QuickStartService, 'isHydrated', 'get').mockReturnValue(false); + const ensureHydrated = jest.spyOn(QuickStartService, 'ensureHydrated').mockResolvedValue(undefined); + const backgroundRefresh = jest + .spyOn(QuickStartService, 'refreshLiveStateInBackground') + .mockReturnValue(undefined); + jest.spyOn(QuickStartService, 'getStatus').mockReturnValue({ + state: InstanceState.NotInstalled, + metadata: undefined, + missing: false, + canResumeReadiness: false, + }); + + await new LocalQuickStartItem('connectionsView/root').getChildren(); + + expect(ensureHydrated).toHaveBeenCalledTimes(1); + expect(backgroundRefresh).not.toHaveBeenCalled(); + }); + + it('uses the background live-state probe after initial hydration', async () => { + jest.spyOn(QuickStartService, 'isHydrated', 'get').mockReturnValue(true); + jest.spyOn(QuickStartService, 'ensureHydrated').mockResolvedValue(undefined); + const backgroundRefresh = jest + .spyOn(QuickStartService, 'refreshLiveStateInBackground') + .mockReturnValue(undefined); + jest.spyOn(QuickStartService, 'getStatus').mockReturnValue({ + state: InstanceState.NotInstalled, + metadata: undefined, + missing: false, + canResumeReadiness: false, + }); + + await new LocalQuickStartItem('connectionsView/root').getChildren(); + + expect(backgroundRefresh).toHaveBeenCalledTimes(1); + }); + + it('still renders the set-up row when hydration fails because Docker is unavailable', async () => { + jest.spyOn(QuickStartService, 'isHydrated', 'get').mockReturnValue(false); + jest.spyOn(QuickStartService, 'ensureHydrated').mockRejectedValue(new Error('Docker unavailable')); + jest.spyOn(QuickStartService, 'refreshLiveStateInBackground').mockReturnValue(undefined); + jest.spyOn(QuickStartService, 'getStatus').mockReturnValue({ + state: InstanceState.NotInstalled, + metadata: undefined, + missing: false, + canResumeReadiness: false, + }); + + const children = await new LocalQuickStartItem('connectionsView/root').getChildren(); + + expect(children.map((child) => child.id)).toEqual(['connectionsView/root/localQuickStart/start']); + }); +}); + describe('LocalQuickStartItem — CredentialsMissing row', () => { afterEach(() => jest.restoreAllMocks()); it('opens Quick Start to review setup without offering deletion in the tree', async () => { + jest.spyOn(QuickStartService, 'ensureHydrated').mockResolvedValue(undefined); + jest.spyOn(QuickStartService, 'isHydrated', 'get').mockReturnValue(false); jest.spyOn(QuickStartService, 'refreshLiveStateInBackground').mockReturnValue(undefined); jest.spyOn(QuickStartService, 'getStatus').mockReturnValue({ state: InstanceState.CredentialsMissing, @@ -44,6 +113,8 @@ describe('LocalQuickStartItem — error recovery nodes (I2-4)', () => { afterEach(() => jest.restoreAllMocks()); async function childIds(status: Partial): Promise { + jest.spyOn(QuickStartService, 'ensureHydrated').mockResolvedValue(undefined); + jest.spyOn(QuickStartService, 'isHydrated', 'get').mockReturnValue(false); jest.spyOn(QuickStartService, 'refreshLiveStateInBackground').mockReturnValue(undefined); jest.spyOn(QuickStartService, 'getStatus').mockReturnValue({ state: InstanceState.Error, diff --git a/src/tree/connections-view/LocalQuickStart/LocalQuickStartItem.ts b/src/tree/connections-view/LocalQuickStart/LocalQuickStartItem.ts index ec2e22e39..fb5afdd53 100644 --- a/src/tree/connections-view/LocalQuickStart/LocalQuickStartItem.ts +++ b/src/tree/connections-view/LocalQuickStart/LocalQuickStartItem.ts @@ -18,11 +18,17 @@ import { CredentialCache } from '../../../documentdb/CredentialCache'; import { DocumentDBConnectionString } from '../../../documentdb/utils/DocumentDBConnectionString'; import { Views } from '../../../documentdb/Views'; import { DocumentDBExperience } from '../../../DocumentDBExperiences'; +import { ext } from '../../../extensionVariables'; import { StorageZone } from '../../../services/connectionStorageService'; -import { QuickStartService } from '../../../services/localQuickStart/QuickStartService'; +import { formatQuickStartMessage } from '../../../services/localQuickStart/quickStartMessages'; +import { + QuickStartService, + type QuickStartConnectionPreflightResult, +} from '../../../services/localQuickStart/QuickStartService'; import { InstanceState, QUICK_START_PORT, + type DockerReadiness, type QuickStartStatus, } from '../../../services/localQuickStart/quickStartTypes'; import { getResourcesPath } from '../../../utils/icons'; @@ -40,6 +46,186 @@ import { buildQuickStartInstanceTreeId, buildQuickStartTreeId } from './quickSta /** Base context token for the managed-instance row; menus gate on this + a state token. */ const INSTANCE_CONTEXT = 'treeItem_quickStartInstance'; +/** + * What the tree shows instead of databases when the container preflight says the instance cannot be + * opened. Rendered as rows rather than a dialog: expanding a node is a browse gesture, and a modal + * would block the expansion until it is answered and then leave the node empty anyway. + */ +function buildPreflightChildren(parentId: string, verdict: QuickStartConnectionPreflightResult): TreeElement[] { + const id = `${parentId}/preflight`; + const open = 'vscode-documentdb.command.localQuickStart.open'; + + switch (verdict) { + case 'stopped': + return [ + createGenericElementWithContext({ + id, + contextValue: 'error', + label: l10n.t('Click here to start DocumentDB Local'), + iconPath: new vscode.ThemeIcon('play'), + commandId: 'vscode-documentdb.command.localQuickStart.start', + }), + ]; + case 'missing': + return [ + createGenericElementWithContext({ + id, + contextValue: 'error', + label: l10n.t('The container is gone. Click here to recreate it'), + iconPath: new vscode.ThemeIcon('warning', new vscode.ThemeColor('list.warningForeground')), + commandId: open, + }), + ]; + case 'dockerUnreachable': + return [ + createGenericElementWithContext({ + id, + contextValue: 'error', + label: l10n.t('Docker does not appear to be running. Click here for details'), + iconPath: new vscode.ThemeIcon('warning', new vscode.ThemeColor('list.warningForeground')), + commandId: open, + }), + ]; + case 'busy': + // Progress belongs on the node itself (see quickStartProgressBridge), not on a child row. + return []; + default: + return [ + createGenericElementWithContext({ + id, + contextValue: 'error', + label: l10n.t('DocumentDB Local cannot be opened. Click here to review its setup'), + iconPath: new vscode.ThemeIcon('warning', new vscode.ThemeColor('list.warningForeground')), + commandId: open, + }), + ]; + } +} + +function escapeMarkdown(value: string): string { + // Only the characters that would actually change how the tooltip renders; the tooltip is not + // trusted, so HTML is inert. + return value.replace(/[\\`*_~[\]<>]/g, '\\$&'); +} + +function instanceStateLabel(state: InstanceState): string { + switch (state) { + case InstanceState.NotInstalled: + return l10n.t('Not set up'); + case InstanceState.Provisioning: + return l10n.t('Provisioning'); + case InstanceState.Starting: + return l10n.t('Starting'); + case InstanceState.Running: + return l10n.t('Running'); + case InstanceState.Stopping: + return l10n.t('Stopping'); + case InstanceState.Stopped: + return l10n.t('Stopped'); + case InstanceState.CredentialsMissing: + return l10n.t('Credentials missing'); + default: + return l10n.t('Error'); + } +} + +function dockerEndpointLabel(readiness: DockerReadiness): string { + switch (readiness.endpointKind) { + case 'unixSocket': + return l10n.t('Unix socket'); + case 'namedPipe': + return l10n.t('Named pipe'); + case 'tcp': + return 'TCP'; + case 'ssh': + return 'SSH'; + default: + return l10n.t('Unknown'); + } +} + +function containerOsLabel(osType: 'linux' | 'windows'): string { + return osType === 'windows' ? l10n.t('Windows') : l10n.t('Linux'); +} + +function shortenContainerId(containerId: string): string { + return /^[0-9a-f]{12,64}$/i.test(containerId) ? containerId.slice(0, 12) : containerId; +} + +function dockerProviderLabel(readiness: DockerReadiness): string { + switch (readiness.provider) { + case 'dockerDesktop': + return l10n.t('Docker Desktop'); + case 'dockerEngine': + return l10n.t('Docker Engine'); + default: + return l10n.t('Unknown'); + } +} + +// Strings shared with the Quick Start webview are spelled identically on purpose, so each reaches +// translators once. Bare acronyms (WSL, SSH, TCP) are left alone — there is nothing to translate. +function executionTargetLabel(readiness: DockerReadiness): string { + switch (readiness.executionTarget) { + case 'wsl': + return 'WSL'; + case 'ssh': + return 'SSH'; + case 'devContainer': + return l10n.t('Dev container'); + case 'codespaces': + return l10n.t('GitHub Codespaces'); + case 'otherRemote': + return l10n.t('Remote'); + default: + return l10n.t('Local'); + } +} + +function buildInstanceTooltip(status: QuickStartStatus, baseTooltip?: vscode.MarkdownString): vscode.MarkdownString { + const metadata = status.metadata; + const readiness = QuickStartService.getDockerReadinessSnapshot(); + const tooltip = new vscode.MarkdownString(baseTooltip?.value ?? `### ${l10n.t('DocumentDB Local')}\n\n`); + tooltip.isTrusted = false; + + if (!baseTooltip) { + tooltip.appendMarkdown(`**${l10n.t('State')}:** ${instanceStateLabel(status.state)}\n\n`); + if (metadata) { + tooltip.appendMarkdown(`**${l10n.t('Host')}:** localhost:${String(metadata.boundPort)}\n\n`); + } + } + + if (metadata) { + tooltip.appendMarkdown('---\n\n'); + tooltip.appendMarkdown( + `**${l10n.t('Container image')}:** ${escapeMarkdown(metadata.imageRef ?? l10n.t('Unknown'))}\n\n`, + ); + tooltip.appendMarkdown( + `**${l10n.t('Container ID')}:** ${escapeMarkdown(shortenContainerId(metadata.containerId))}\n\n`, + ); + } + + if (readiness) { + tooltip.appendMarkdown('---\n\n'); + tooltip.appendMarkdown(`**${l10n.t('Docker provider')}:** ${dockerProviderLabel(readiness)}\n\n`); + if (readiness.cliVersion) { + tooltip.appendMarkdown(`**${l10n.t('Docker version')}:** ${escapeMarkdown(readiness.cliVersion)}\n\n`); + } + if (readiness.daemonArchitecture) { + tooltip.appendMarkdown( + `**${l10n.t('Daemon architecture')}:** ${escapeMarkdown(readiness.daemonArchitecture)}\n\n`, + ); + } + if (readiness.osType) { + tooltip.appendMarkdown(`**${l10n.t('Container OS')}:** ${containerOsLabel(readiness.osType)}\n\n`); + } + tooltip.appendMarkdown(`**${l10n.t('Execution target')}:** ${executionTargetLabel(readiness)}\n\n`); + tooltip.appendMarkdown(`**${l10n.t('Docker endpoint')}:** ${dockerEndpointLabel(readiness)}\n\n`); + } + + return tooltip; +} + /** * Inline managed-instance cluster item (shown only when Running). * @@ -68,12 +254,25 @@ class QuickStartClusterItem extends ClusterItemBase { * state label (e.g. "Running · localhost:10260"). */ public override getTreeItem(): vscode.TreeItem { + const treeItem = buildClusterTreeItem({ id: this.id, contextValue: this.contextValue, cluster: this.cluster }); return { - ...buildClusterTreeItem({ id: this.id, contextValue: this.contextValue, cluster: this.cluster }), + ...treeItem, description: this.descriptionOverride, + tooltip: buildInstanceTooltip( + QuickStartService.getStatus(this.alias), + treeItem.tooltip instanceof vscode.MarkdownString ? treeItem.tooltip : undefined, + ), }; } + public override async getChildren(): Promise { + const preflight: QuickStartConnectionPreflightResult = await QuickStartService.prepareForConnection(this.alias); + if (preflight !== 'ready') { + return buildPreflightChildren(this.id, preflight); + } + return super.getChildren(); + } + public async getCredentials(): Promise { const connectionString = await QuickStartService.readStoredConnectionString(this.alias); if (!connectionString) { @@ -183,18 +382,25 @@ export class LocalQuickStartItem implements TreeElement, TreeElementWithContextV } async getChildren(): Promise { + const wasHydrated = QuickStartService.isHydrated; + try { + await QuickStartService.ensureHydrated(); + } catch { + // Docker may not be installed or running yet, which is precisely the case Quick Start + // exists to fix. Render the durable-state row anyway; the service stays un-hydrated, so + // the next expansion retries. + } + // Never block the row on Docker (review M6): the Connections view re-runs getChildren() on // many unrelated events, so the freshness probe is kicked off in the background (rate-limited // and de-duplicated by the service) and the row is redrawn by onDidChangeStatus when it lands. - QuickStartService.refreshLiveStateInBackground(); + if (wasHydrated) { + QuickStartService.refreshLiveStateInBackground(); + } const status: QuickStartStatus = QuickStartService.getStatus(); const metadata = status.metadata; - /** Append the in-flight-probe hint so a row rendered from cache says so. */ - const withRefreshHint = (description: string): string => - QuickStartService.isRefreshingLiveState ? l10n.t('{0} · Refreshing…', description) : description; - // Missing badge (design §6.1): metadata exists but Docker has no container. if (metadata && status.missing) { return [ @@ -202,7 +408,7 @@ export class LocalQuickStartItem implements TreeElement, TreeElementWithContextV id: `${this.id}/instance`, contextValue: createContextValue([INSTANCE_CONTEXT, 'state_missing']), label: l10n.t('DocumentDB Local'), - description: withRefreshHint(l10n.t('Missing · click to recreate')), + description: l10n.t('Missing · click to recreate'), tooltip: l10n.t( 'The container was removed outside VS Code. Click to recreate it (your data is preserved), or use Delete Container to remove it and its data.', ), @@ -239,7 +445,7 @@ export class LocalQuickStartItem implements TreeElement, TreeElementWithContextV return [ new QuickStartClusterItem( model, - withRefreshHint(l10n.t('Running · localhost:{0}', metadata.boundPort)), + `${instanceStateLabel(status.state)} · localhost:${String(metadata.boundPort)}`, 'state_running', metadata.alias, ), @@ -250,34 +456,40 @@ export class LocalQuickStartItem implements TreeElement, TreeElementWithContextV // lifecycle menus (a stopped container can't be connected to / browsed). if (metadata) { const port = metadata.boundPort; - const row = (stateToken: string, description: string, icon: vscode.ThemeIcon): TreeElement => - createGenericElementWithContext({ - id: `${this.id}/instance`, - contextValue: createContextValue([INSTANCE_CONTEXT, stateToken]), - label: l10n.t('DocumentDB Local'), - description, - iconPath: icon, - }); + const row = (stateToken: string, description: string, icon: vscode.ThemeIcon): TreeElement => { + const id = `${this.id}/instance`; + const contextValue = createContextValue([INSTANCE_CONTEXT, stateToken]); + return { + id, + getTreeItem: (): vscode.TreeItem => ({ + id, + contextValue, + label: l10n.t('DocumentDB Local'), + description, + tooltip: buildInstanceTooltip(status), + iconPath: icon, + }), + }; + }; - const spin = new vscode.ThemeIcon('loading~spin'); + // Transitional states keep their own contextValue (menus gate on it), but neither the + // spinner nor the text: quickStartProgressBridge overlays both. The wording is kept + // identical to the overlay so a registration change can't surface a different string. + const idle = new vscode.ThemeIcon('circle-outline'); switch (status.state) { case InstanceState.Starting: - return [row('state_starting', l10n.t('Starting… · localhost:{0}', port), spin)]; + return [row('state_starting', l10n.t('Starting…'), idle)]; case InstanceState.Stopping: - return [row('state_stopping', l10n.t('Stopping… · localhost:{0}', port), spin)]; + return [row('state_stopping', l10n.t('Stopping…'), idle)]; case InstanceState.Stopped: return [ - row( - 'state_stopped', - withRefreshHint(l10n.t('Stopped · localhost:{0}', port)), - new vscode.ThemeIcon('circle-outline'), - ), + row('state_stopped', `${instanceStateLabel(status.state)} · localhost:${String(port)}`, idle), ]; case InstanceState.Error: 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), @@ -307,11 +519,13 @@ export class LocalQuickStartItem implements TreeElement, TreeElementWithContextV } if (status.state === InstanceState.Provisioning) { + // The one row that still owns its spinner: there is no instance row to attach node + // progress to yet, and this mirrors what `ext.state.showCreatingChild` renders. return [ createGenericElementWithContext({ id: `${this.id}/provisioning`, contextValue: 'treeItem_quickStartProvisioning', - label: l10n.t('Provisioning… · localhost:{0}', String(status.port ?? QUICK_START_PORT)), + label: `${l10n.t('Provisioning…')} · localhost:${String(status.port ?? QUICK_START_PORT)}`, iconPath: new vscode.ThemeIcon('loading~spin'), }), ]; @@ -338,6 +552,15 @@ export class LocalQuickStartItem implements TreeElement, TreeElementWithContextV return children; } + /** Explicit node refresh performs a full durable-store and Docker reconciliation. */ + public async refresh(_context: IActionContext): Promise { + // Reconciliation shells out to Docker, so the node carries the wait. + await ext.state.runWithTemporaryDescription(this.id, l10n.t('Refreshing…'), () => + QuickStartService.refreshHydratedState(), + ); + ext.connectionsBranchDataProvider.refresh(this); + } + private iconPath: IconPath = { light: vscode.Uri.file(path.join(getResourcesPath(), 'icons', 'vscode-documentdb-icon-light-themes.svg')), dark: vscode.Uri.file(path.join(getResourcesPath(), 'icons', 'vscode-documentdb-icon-dark-themes.svg')), @@ -349,7 +572,7 @@ export class LocalQuickStartItem implements TreeElement, TreeElementWithContextV contextValue: this.contextValue, label: l10n.t('DocumentDB Local - Quick Start'), iconPath: this.iconPath, - collapsibleState: vscode.TreeItemCollapsibleState.Expanded, + collapsibleState: vscode.TreeItemCollapsibleState.Collapsed, }; } } diff --git a/src/tree/connections-view/LocalQuickStart/quickStartProgressBridge.test.ts b/src/tree/connections-view/LocalQuickStart/quickStartProgressBridge.test.ts new file mode 100644 index 000000000..515cef535 --- /dev/null +++ b/src/tree/connections-view/LocalQuickStart/quickStartProgressBridge.test.ts @@ -0,0 +1,111 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See License.txt in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +import { type Disposable } from 'vscode'; +import { ext } from '../../../extensionVariables'; +import { QuickStartService, type QuickStartOperation } from '../../../services/localQuickStart/QuickStartService'; +import { createQuickStartProgressBridge } from './quickStartProgressBridge'; +import { buildQuickStartInstanceTreeId } from './quickStartTreeIdentity'; + +jest.mock('../../../extensionVariables', () => ({ + ext: { + state: { + // The real implementation holds the indicator until the wrapped work settles. + runWithTemporaryDescription: jest.fn((_id: string, _description: string, callback: () => Promise) => + callback(), + ), + }, + }, +})); + +const runWithTemporaryDescription = ext.state.runWithTemporaryDescription as jest.MockedFunction< + typeof ext.state.runWithTemporaryDescription +>; + +/** Lets the bridge's deferred `sync` run. */ +const flush = (): Promise => Promise.resolve(); + +function pendingOperation(kind: QuickStartOperation['kind']): QuickStartOperation { + return { kind, promise: new Promise(() => undefined) }; +} + +/** + * Quick Start work is service-owned (it can start from the webview, a command, or a background + * probe), so the row cannot own its own spinner. The bridge is what turns that work into the + * framework's node-progress state. + */ +describe('quickStartProgressBridge', () => { + let notify: () => void; + let subscription: Disposable; + + beforeEach(() => { + runWithTemporaryDescription.mockClear(); + jest.spyOn(QuickStartService, 'onDidChangeOperation').mockImplementation(((listener: () => void) => { + notify = listener; + return { dispose: () => undefined }; + }) as typeof QuickStartService.onDidChangeOperation); + subscription = createQuickStartProgressBridge(); + }); + + afterEach(() => { + subscription.dispose(); + jest.restoreAllMocks(); + }); + + it('applies node progress to the instance row for the whole operation', async () => { + const operation = pendingOperation('starting'); + jest.spyOn(QuickStartService, 'getInFlightOperation').mockReturnValue(operation); + + notify(); + await flush(); + + expect(runWithTemporaryDescription).toHaveBeenCalledTimes(1); + const [id, description, callback] = runWithTemporaryDescription.mock.calls[0]; + expect(id).toBe(buildQuickStartInstanceTreeId()); + expect(description).toBe('Starting…'); + // The framework holds the spinner until the service's own work settles. + expect(callback()).toBe(operation.promise); + }); + + it('does not stack indicators while the same operation is still running', async () => { + jest.spyOn(QuickStartService, 'getInFlightOperation').mockReturnValue(pendingOperation('deleting')); + + notify(); + await flush(); + notify(); + await flush(); + + expect(runWithTemporaryDescription).toHaveBeenCalledTimes(1); + expect(runWithTemporaryDescription.mock.calls[0][1]).toBe('Deleting…'); + }); + + it('picks up the next operation once the previous one has settled', async () => { + const inFlight = jest.spyOn(QuickStartService, 'getInFlightOperation'); + + inFlight.mockReturnValue(pendingOperation('stopping')); + notify(); + await flush(); + + inFlight.mockReturnValue(undefined); + notify(); + await flush(); + + inFlight.mockReturnValue(pendingOperation('refreshing')); + notify(); + await flush(); + + expect(runWithTemporaryDescription).toHaveBeenCalledTimes(2); + expect(runWithTemporaryDescription.mock.calls[1][1]).toBe('Refreshing…'); + }); + + it('stays quiet when nothing is running', async () => { + jest.spyOn(QuickStartService, 'getInFlightOperation').mockReturnValue(undefined); + + notify(); + await flush(); + + expect(runWithTemporaryDescription).not.toHaveBeenCalled(); + }); +}); diff --git a/src/tree/connections-view/LocalQuickStart/quickStartProgressBridge.ts b/src/tree/connections-view/LocalQuickStart/quickStartProgressBridge.ts new file mode 100644 index 000000000..ad5623a26 --- /dev/null +++ b/src/tree/connections-view/LocalQuickStart/quickStartProgressBridge.ts @@ -0,0 +1,60 @@ +/*--------------------------------------------------------------------------------------------- + * 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 Disposable } from 'vscode'; +import { ext } from '../../../extensionVariables'; +import { QuickStartService, type QuickStartOperationKind } from '../../../services/localQuickStart/QuickStartService'; +import { buildQuickStartInstanceTreeId } from './quickStartTreeIdentity'; + +function operationLabel(kind: QuickStartOperationKind): string { + switch (kind) { + case 'provisioning': + return l10n.t('Provisioning…'); + case 'starting': + return l10n.t('Starting…'); + case 'stopping': + return l10n.t('Stopping…'); + case 'restarting': + return l10n.t('Restarting…'); + case 'deleting': + return l10n.t('Deleting…'); + default: + return l10n.t('Refreshing…'); + } +} + +/** + * Hands Quick Start's in-flight work to the tree framework's node-progress state, so the managed + * instance row shows progress the same way every other node does. + * + * Lives outside the tree item because the work is service-owned: it can be started from the + * Quick Start webview or a lifecycle command, and the row only ever renders the resulting state. + */ +export function createQuickStartProgressBridge(): Disposable { + let bridged: Promise | undefined; + + const sync = (): void => { + const operation = QuickStartService.getInFlightOperation(); + if (!operation || operation.promise === bridged) { + return; + } + bridged = operation.promise; + void ext.state + .runWithTemporaryDescription( + buildQuickStartInstanceTreeId(), + operationLabel(operation.kind), + () => operation.promise, + ) + .finally(() => { + if (bridged === operation.promise) { + bridged = undefined; + } + }); + }; + + // Deferred: work can start from inside a tree render, and applying state fires a refresh synchronously. + return QuickStartService.onDidChangeOperation(() => queueMicrotask(sync)); +} diff --git a/src/tree/connections-view/LocalQuickStart/revealQuickStartInstance.test.ts b/src/tree/connections-view/LocalQuickStart/revealQuickStartInstance.test.ts index 629da3787..7a4e2b562 100644 --- a/src/tree/connections-view/LocalQuickStart/revealQuickStartInstance.test.ts +++ b/src/tree/connections-view/LocalQuickStart/revealQuickStartInstance.test.ts @@ -73,6 +73,8 @@ describe('Quick Start tree paths match the ids the tree builds', () => { }); it('matches the managed-instance row id', async () => { + jest.spyOn(QuickStartService, 'ensureHydrated').mockResolvedValue(undefined); + jest.spyOn(QuickStartService, 'isHydrated', 'get').mockReturnValue(true); jest.spyOn(QuickStartService, 'refreshLiveStateInBackground').mockReturnValue(undefined); jest.spyOn(QuickStartService, 'getStatus').mockReturnValue({ state: InstanceState.Running, diff --git a/src/tree/documentdb/ClusterItemBase.ts b/src/tree/documentdb/ClusterItemBase.ts index 0b668c888..0201bb469 100644 --- a/src/tree/documentdb/ClusterItemBase.ts +++ b/src/tree/documentdb/ClusterItemBase.ts @@ -19,6 +19,7 @@ import { type EntraIdAuthConfig, type NativeAuthConfig } from '../../documentdb/ import { type AuthMethodId } from '../../documentdb/auth/AuthMethod'; import { ShellCommandIds } from '../../documentdb/shell/constants'; import { ext } from '../../extensionVariables'; +import { ConnectionDiagnosticsService } from '../../services/connectionDiagnosticsService'; import { regionToDisplayName } from '../../utils/regionToDisplayName'; import { type TreeElement } from '../TreeElement'; import { type TreeElementWithContextValue } from '../TreeElementWithContextValue'; @@ -245,6 +246,10 @@ export abstract class ClusterItemBase { telemetryContext.errorHandling.suppressDisplay = true; @@ -252,6 +257,7 @@ export abstract class ClusterItemBase( + commandId: string, + callback: TreeNodeCommandCallback, + debounce?: number, + telemetryId?: string, +): void { + registerCommand( + commandId, + async (context: IActionContext, ...args: unknown[]) => { + let unwrappedArgs: ReturnType> = []; + try { + unwrappedArgs = unwrapArgs(args); + return await callback(context, ...unwrappedArgs); + } catch (error) { + // A UserFacingError already says what it needs to; leave it to default handling. + if (error instanceof UserFacingError) { + throw error; + } + + const clusterId = (unwrappedArgs[0] as unknown as { cluster?: { clusterId?: string } } | undefined) + ?.cluster?.clusterId; + const diagnosis = clusterId + ? await ConnectionDiagnosticsService.explain({ clusterId, error }) + : undefined; + + if (diagnosis) { + context.telemetry.properties.diagnosisProviderId = diagnosis.providerId; + context.errorHandling.suppressDisplay = true; + // `detail` is only rendered for modal messages, so the raw text is appended. + const cause = error instanceof Error ? error.message : String(error); + void vscode.window.showErrorMessage(`${diagnosis.message} (${cause})`); + } + + // The error itself is never modified, so telemetry and identity checks still work. + throw error; + } + }, + debounce, + telemetryId, + ); +} + /** * Registers a command that unwraps tree node arguments and shows UserFacingErrors in modal dialogs. * This combines the functionality of registerCommandWithTreeNodeUnwrapping and registerCommandWithModalErrors. @@ -121,9 +178,11 @@ export function registerCommandWithTreeNodeUnwrappingAndModalErrors( registerCommand( commandId, async (context: IActionContext, ...args: unknown[]) => { + let unwrappedArgs: ReturnType> = []; try { // Unwrap tree node arguments before passing to the callback - return await callback(context, ...unwrapArgs(args)); + unwrappedArgs = unwrapArgs(args); + return await callback(context, ...unwrappedArgs); } catch (error) { // Only handle UserFacingError specially if (error instanceof UserFacingError) { @@ -142,7 +201,24 @@ export function registerCommandWithTreeNodeUnwrappingAndModalErrors( throw error; } - // For all other error types, just re-throw to use default handling + // The command's tree node carries the cluster, so a failure caused by its + // infrastructure can be explained instead of showing the raw driver error. + const clusterId = (unwrappedArgs[0] as unknown as { cluster?: { clusterId?: string } } | undefined) + ?.cluster?.clusterId; + const diagnosis = clusterId + ? await ConnectionDiagnosticsService.explain({ clusterId, error }) + : undefined; + + if (diagnosis) { + context.telemetry.properties.diagnosisProviderId = diagnosis.providerId; + context.errorHandling.suppressDisplay = true; + await vscode.window.showErrorMessage(diagnosis.message, { + modal: true, + detail: error instanceof Error ? error.message : String(error), + }); + } + + // The error itself is never modified, so telemetry and identity checks still work. throw error; } }, diff --git a/src/webviews/_integration/appRouter.ts b/src/webviews/_integration/appRouter.ts index 30be24471..d6d2f5f79 100644 --- a/src/webviews/_integration/appRouter.ts +++ b/src/webviews/_integration/appRouter.ts @@ -28,6 +28,7 @@ import * as vscode from 'vscode'; import { z } from 'zod'; import { type API } from '../../DocumentDBExperiences'; import { ext } from '../../extensionVariables'; +import { ConnectionDiagnosticsService } from '../../services/connectionDiagnosticsService'; import { showConfirmationAsInSettings } from '../../utils/dialogs/showConfirmation'; import { formatUrlForLogging, isSupportedExternalUrl, openUrl } from '../../utils/openUrl'; import { openSurvey, promptAfterActionEventually } from '../../utils/survey'; @@ -141,6 +142,30 @@ const commonRouter = router({ }, ); }), + /** + * Asks whether a failed operation has a better explanation than the raw driver error, for + * example a DocumentDB Local container that is not running or a port-forward tunnel that is no + * longer up. Returns `null` when nothing applies, which means "show your own message". + * + * Named after the caller's situation (an operation failed) rather than a cause: the providers + * behind it explain container, tunnel and transport problems today, and are free to explain + * other infrastructure later without the name becoming a lie. + * + * Only the error MESSAGE crosses the webview boundary, because that is all tRPC preserves. + * A provider that needs an error's class or `code` cannot be served from a webview. + * + * @see .github/skills/error-translation/SKILL.md + */ + explainOperationFailure: publicProcedure.input(z.object({ message: z.string() })).query(async ({ input, ctx }) => { + // Concrete webview contexts carry a clusterId; the shared base type does not. + const clusterId = (ctx as { clusterId?: string }).clusterId; + if (!clusterId) { + return null; + } + + const diagnosis = await ConnectionDiagnosticsService.explain({ clusterId, error: input.message }); + return diagnosis?.message ?? null; + }), displayErrorMessage: publicProcedure .input( z.object({ diff --git a/src/webviews/documentdb/collectionView/CollectionView.tsx b/src/webviews/documentdb/collectionView/CollectionView.tsx index ecb059634..32f5d6bf2 100644 --- a/src/webviews/documentdb/collectionView/CollectionView.tsx +++ b/src/webviews/documentdb/collectionView/CollectionView.tsx @@ -401,11 +401,13 @@ export const CollectionView = (): JSX.Element => { setCurrentContext((prev) => ({ ...prev, isLoading: false, isFirstTimeLoad: false })); }) - .catch((error) => { + .catch(async (error) => { + const cause = error instanceof Error ? error.message : String(error); + const explained = await trpcClient.common.explainOperationFailure.query({ message: cause }); void trpcClient.common.displayErrorMessage.mutate({ - message: l10n.t('Error while running the query'), + message: explained ?? l10n.t('Error while running the query'), modal: true, - cause: error instanceof Error ? error.message : String(error), + cause, }); }) .finally(() => { diff --git a/src/webviews/documentdb/collectionView/queryInsightsTab/QueryInsightsTab.tsx b/src/webviews/documentdb/collectionView/queryInsightsTab/QueryInsightsTab.tsx index 4854e569a..5abc3337b 100644 --- a/src/webviews/documentdb/collectionView/queryInsightsTab/QueryInsightsTab.tsx +++ b/src/webviews/documentdb/collectionView/queryInsightsTab/QueryInsightsTab.tsx @@ -255,11 +255,13 @@ export const QueryInsightsMain = (): JSX.Element => { 3: l10n.t('AI recommendations'), }; - void trpcClient.common.displayErrorMessage.mutate({ - message: l10n.t('Failed to load {0}', stageNames[stage]), - modal: false, - cause: errorMessage, - }); + void trpcClient.common.explainOperationFailure.query({ message: errorMessage }).then((explained) => + trpcClient.common.displayErrorMessage.mutate({ + message: explained ?? l10n.t('Failed to load {0}', stageNames[stage]), + modal: false, + cause: errorMessage, + }), + ); }, [trpcClient], ); diff --git a/src/webviews/documentdb/documentView/documentView.tsx b/src/webviews/documentdb/documentView/documentView.tsx index ce54f6c51..275da57fd 100644 --- a/src/webviews/documentdb/documentView/documentView.tsx +++ b/src/webviews/documentdb/documentView/documentView.tsx @@ -79,11 +79,13 @@ export const DocumentView = (): JSX.Element => { .then((response) => { setContent(response); }) - .catch((error) => { + .catch(async (error) => { + const cause = error instanceof Error ? error.message : String(error); + const explained = await trpcClient.common.explainOperationFailure.query({ message: cause }); void trpcClient.common.displayErrorMessage.mutate({ - message: l10n.t('Error while loading the document'), + message: explained ?? l10n.t('Error while loading the document'), modal: false, - cause: error instanceof Error ? error.message : String(error), + cause, }); }) .finally(() => { @@ -182,11 +184,13 @@ export const DocumentView = (): JSX.Element => { documentLength = response.length ?? 0; setContent(response); }) - .catch((error) => { + .catch(async (error) => { + const cause = error instanceof Error ? error.message : String(error); + const explained = await trpcClient.common.explainOperationFailure.query({ message: cause }); void trpcClient.common.displayErrorMessage.mutate({ - message: l10n.t('Error while refreshing the document'), + message: explained ?? l10n.t('Error while refreshing the document'), modal: false, - cause: error instanceof Error ? error.message : String(error), + cause, }); }) .finally(() => { @@ -231,11 +235,13 @@ export const DocumentView = (): JSX.Element => { setIsLoading(false); setIsDirty(false); }) - .catch((error) => { + .catch(async (error) => { + const cause = error instanceof Error ? error.message : String(error); + const explained = await trpcClient.common.explainOperationFailure.query({ message: cause }); void trpcClient.common.displayErrorMessage.mutate({ - message: l10n.t('Error saving the document'), + message: explained ?? l10n.t('Error saving the document'), modal: true, // we want to show the error in a modal dialog as it's an important one, failed to save the document - cause: error instanceof Error ? error.message : String(error), + cause, }); }) .finally(() => { 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.test.ts b/src/webviews/documentdb/localQuickStart/localQuickStartRouter.test.ts index a66ef9caf..ffa467ce1 100644 --- a/src/webviews/documentdb/localQuickStart/localQuickStartRouter.test.ts +++ b/src/webviews/documentdb/localQuickStart/localQuickStartRouter.test.ts @@ -31,6 +31,7 @@ jest.mock('../../../services/localQuickStart/ContainerRuntime', () => ({ jest.mock('../../../services/localQuickStart/QuickStartService', () => ({ QuickStartService: { discardTimedOutInstance: jest.fn(), + checkDockerReadiness: mockIsDockerReady, getStatus: mockGetStatus, isBusy: false, provision: jest.fn(), diff --git a/src/webviews/documentdb/localQuickStart/localQuickStartRouter.ts b/src/webviews/documentdb/localQuickStart/localQuickStartRouter.ts index 878f0c190..4b494777a 100644 --- a/src/webviews/documentdb/localQuickStart/localQuickStartRouter.ts +++ b/src/webviews/documentdb/localQuickStart/localQuickStartRouter.ts @@ -21,11 +21,7 @@ import { CancellationTokenLike } from '@microsoft/vscode-processutils'; import * as vscode from 'vscode'; import { z } from 'zod'; -import { - ContainerRuntime, - getQuickStartOutputChannel, - startDockerProvider, -} from '../../../services/localQuickStart/ContainerRuntime'; +import { getQuickStartOutputChannel, startDockerProvider } from '../../../services/localQuickStart/ContainerRuntime'; import { getDockerRecoveryCommandById } from '../../../services/localQuickStart/dockerRecoveryCommands'; import { QuickStartService } from '../../../services/localQuickStart/QuickStartService'; import { @@ -111,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, }; @@ -136,7 +132,7 @@ export const localQuickStartRouter = router({ tctx.actionContext.telemetry.suppressAll = true; } const cancellationToken = ctx.signal ? CancellationTokenLike.fromAbortSignal(ctx.signal) : undefined; - const readiness = await ContainerRuntime.isDockerReady({ + const readiness = await QuickStartService.checkDockerReadiness({ forceRefresh: input?.forceRefresh, resetProviderMemory: input?.resetProviderMemory, suppressCommandEcho: input?.suppressCommandEcho, diff --git a/work-summary.md b/work-summary.md deleted file mode 100644 index 117376ced..000000000 --- a/work-summary.md +++ /dev/null @@ -1,653 +0,0 @@ -# Connections View Folder Hierarchy - Work Summary - -## Overview -This document provides a comprehensive summary of the work completed for implementing folder hierarchy in the DocumentDB Connections View, following a hybrid storage approach with recent simplifications to improve maintainability. - -**Latest Update:** Simplified folder operations by removing boundary crossing support and using path-based circular detection. Move operations are now O(1) complexity. - ---- - -## Completed Work Items - -### 1. ✅ Extend Storage Model -**Commits:** 075ec64 -**Status:** FULLY COMPLETED - -**Actions Taken:** -- Extended `ConnectionStorageService` to support both connections and folders using a unified storage mechanism -- Added `ItemType` enum with `Connection` and `Folder` discriminator values -- Changed `folderId` property to `parentId` for clearer hierarchical relationships -- Implemented migration from v2.0 to v3.0 with automatic defaults -- Added comprehensive helper methods: - - `getChildren(parentId, connectionType)` - Get immediate children - - `getDescendants(parentId, connectionType)` - Recursively get all descendants - - `updateParentId(itemId, connectionType, newParentId)` - Move items with validation - - `isNameDuplicateInParent()` - Check for duplicate names within same parent - - `getPath()` - Generate full hierarchical path -- Removed separate `FolderStorageService` for unified approach - -**Pros:** -- ✅ Single storage mechanism simplifies architecture -- ✅ Type-safe discriminator pattern prevents errors -- ✅ Unified CRUD operations for all items -- ✅ Automatic migration preserves existing data -- ✅ Helper methods enable complex operations with simple APIs -- ✅ Circular reference prevention built into updateParentId - -**Cons:** -- ⚠️ Increased complexity in ConnectionProperties interface -- ⚠️ All connections must now include type and parentId fields (though defaults are provided) -- ⚠️ Migration path adds code complexity - ---- - -### 2. ✅ Create FolderItem Tree Element -**Commits:** 075ec64 -**Status:** FULLY COMPLETED - -**Actions Taken:** -- Created `FolderItem` class implementing `TreeElement` interface -- Set appropriate contextValue (`treeItem_folder`) for VS Code integration -- Configured collapsible state and folder icon -- Implemented `getChildren()` to recursively load folder contents -- Added `storageId` property for move/paste operations -- Refactored to work with unified `ConnectionItem` storage - -**Pros:** -- ✅ Clean separation of concerns -- ✅ Proper integration with VS Code tree view APIs -- ✅ Supports unlimited nesting depth -- ✅ Efficient lazy loading of children - -**Cons:** -- ⚠️ ConnectionType needs to be tracked per folder (currently defaults to Clusters) -- ⚠️ Some code duplication in child rendering logic - ---- - -### 3. ✅ Update ConnectionsBranchDataProvider -**Commits:** 075ec64 -**Status:** FULLY COMPLETED - -**Actions Taken:** -- Modified `getRootItems()` to build hierarchical tree structure -- Placed `LocalEmulatorsItem` first as fixed entry -- Filtered items by `ItemType` to separate folders from connections -- Implemented recursive nesting via `FolderItem.getChildren()` -- Root level shows both folders and connections where `parentId === undefined` - -**Pros:** -- ✅ Clear hierarchical structure -- ✅ Fixed LocalEmulators position preserved -- ✅ Efficient querying with ItemType discrimination -- ✅ Clean separation between root and nested items - -**Cons:** -- ⚠️ Folder/connection type determination needs refinement -- ⚠️ Currently queries both connection types separately - ---- - -### 4. ✅ Implement Drag-and-Drop Controller -**Commits:** cd1b61c -**Status:** FULLY COMPLETED - -**Actions Taken:** -- Created `ConnectionsDragAndDropController` implementing `TreeDragAndDropController` -- Implemented `handleDrag()` to capture draggable items -- Implemented `handleDrop()` with comprehensive validation: - - Multi-selection support - - Boundary crossing warnings (emulator vs non-emulator) - - Duplicate name detection - - Circular reference prevention - - Recursive folder content moving -- Registered controller in `ClustersExtension.ts` - -**Pros:** -- ✅ Intuitive drag-and-drop UX -- ✅ Comprehensive validation prevents data loss -- ✅ Boundary crossing detection protects against configuration errors -- ✅ Supports both moving individual items and entire folder trees -- ✅ Proper integration with VS Code drag-and-drop APIs - -**Cons:** -- ⚠️ Moving across connection types is slower (delete+recreate vs simple update) -- ⚠️ User must confirm boundary crossing for each item (could batch) -- ⚠️ Error handling could be more granular - ---- - -### 5. ✅ Add Clipboard State to Extension Variables -**Commits:** [Current] -**Status:** FULLY COMPLETED - -**Actions Taken:** -- Added `ClipboardState` interface to extensionVariables.ts -- Added `clipboardState` property to ext namespace -- Defined operation types: 'cut' | 'copy' -- Integrated context key management for menu enablement - -**Pros:** -- ✅ Clean typed interface -- ✅ Centralized state management -- ✅ Context key enables/disables paste command appropriately - -**Cons:** -- ⚠️ State persists only during extension lifecycle -- ⚠️ No cross-window clipboard support - ---- - -### 6. ✅ Add Folder CRUD Commands -**Commits:** bff7c9b, 41e4e10, 075ec64, [Current] -**Status:** FULLY COMPLETED - -**Actions Taken:** -- **createFolder**: Prompt-based folder creation with duplicate validation -- **renameFolder**: Rename with sibling name conflict checking -- **deleteFolder**: Recursive deletion with confirmation dialog -- **cutItems**: Cut items to clipboard with context key management -- **copyItems**: Copy items to clipboard with context key management -- **pasteItems**: Complex paste operation with: - - Duplicate name handling (prompts for new name) - - Support for both cut (move) and copy operations - - Recursive copying of folder hierarchies - - Boundary crossing support - - New ID generation for copies - - Connection type migration handling - -**Pros:** -- ✅ All commands follow wizard pattern for consistency -- ✅ Comprehensive validation at every step -- ✅ User prompts prevent data loss -- ✅ Paste operation handles all edge cases -- ✅ Recursive operations preserve folder structure -- ✅ Context-aware paste target determination - -**Cons:** -- ⚠️ Paste operation is complex and may have edge cases -- ⚠️ No undo functionality -- ⚠️ Cut items remain in clipboard if paste fails partway -- ⚠️ Connection type currently hardcoded in some places - ---- - -## Partially Completed Work Items - -### 7. ⚠️ Register View Header Commands -**Status:** PARTIALLY COMPLETED -**Priority:** HIGH - -**Completed:** -- Commands registered in package.json -- Basic infrastructure in place - -**Remaining:** -- Add navigation header buttons for createFolder -- Implement generic renameItem dispatcher -- Add context key `documentdb.canRenameSelection` -- Configure proper menu visibility - -**Pros of Current State:** -- ✅ Foundation is solid - -**Cons of Current State:** -- ⚠️ Commands not accessible from header buttons -- ⚠️ No generic rename command for both folders and connections - ---- - -### 8. ⚠️ Register Context Menu Commands -**Status:** PARTIALLY COMPLETED -**Priority:** HIGH - -**Completed:** -- Basic folder commands in context menu -- Command registration structure - -**Remaining:** -- Add cut/copy/paste to context menu -- Refine contextValue patterns -- Add "when": "never" to hide from command palette -- Configure when clauses for clipboard operations - -**Pros of Current State:** -- ✅ Core commands accessible - -**Cons of Current State:** -- ⚠️ Cut/copy/paste not in context menu yet -- ⚠️ Commands may appear in command palette unnecessarily - ---- - -## Not Started Work Items - -### 9. ⬜ Complete Extension Integration -**Status:** NOT STARTED -**Priority:** MEDIUM - -**Remaining Tasks:** -- Add onDidChangeSelection listener to connectionsTreeView -- Update documentdb.canRenameSelection context key based on selection -- Implement selection-based command enablement - -**Impact:** -- Context-aware command enablement would improve UX -- Selection tracking would enable more sophisticated features - ---- - -### 10. ⬜ Add Unit Tests -**Status:** NOT STARTED -**Priority:** MEDIUM-HIGH - -**Remaining Tasks:** -- Create folderOperations.test.ts -- Test all CRUD operations -- Test hierarchy operations (nesting, moving) -- Test edge cases (circular references, duplicates) -- Test boundary crossing -- Test clipboard operations -- Mock ConnectionStorageService - -**Impact:** -- Critical for ensuring reliability -- Would catch regressions -- Would document expected behavior - ---- - -## Overall Assessment - -### Implementation Quality - -**Strengths:** -1. **Unified Storage Architecture**: The hybrid approach with type discriminators is clean and maintainable -2. **Comprehensive Validation**: Duplicate names, circular references, and boundary crossing are all handled -3. **User Experience**: Prompts guide users through complex operations -4. **Extensibility**: Architecture supports future features (tags, metadata, etc.) -5. **Error Handling**: Most operations have proper error handling and user feedback - -**Areas for Improvement:** -1. **Testing**: No automated tests yet - critical gap -2. **UI Integration**: Header buttons and refined context menus needed -3. **Connection Type Handling**: Currently hardcoded in places, needs proper tracking -4. **Undo Support**: No way to undo accidental operations -5. **Performance**: Large folder hierarchies not yet tested - ---- - -### Completion Status - -**Overall Progress:** 80% complete - -**Functional Completeness:** -- ✅ Core storage layer: 100% -- ✅ Tree view rendering: 100% -- ✅ Drag-and-drop: 100% -- ✅ Clipboard operations: 100% -- ✅ Basic CRUD commands: 100% -- ⚠️ UI integration: 60% -- ⚠️ Context key management: 50% -- ❌ Unit tests: 0% - -**Production Readiness:** ~70% -- Ready for alpha testing with known gaps -- Needs tests before production release -- UI polish required -- Edge case testing needed - ---- - -## Recommended Next Steps - -### Priority 1 (Critical for Production): -1. Add comprehensive unit tests -2. Complete context menu integration -3. Add header button commands -4. Test with large datasets - -### Priority 2 (Important for UX): -1. Implement selection-based command enablement -2. Add undo/redo support or confirmation dialogs -3. Improve error messages -4. Add loading indicators for long operations - -### Priority 3 (Nice to Have): -1. Folder icons/colors customization -2. Folder metadata (description, tags) -3. Bulk operations -4. Folder templates - ---- - -## Technical Debt - -1. **Connection Type Tracking**: Currently defaults to Clusters, needs proper tracking per folder -2. **Error Recovery**: Partial paste failures leave inconsistent state -3. **Code Duplication**: Some logic duplicated between paste and drag-and-drop -4. **Migration Testing**: v2->v3 migration not tested with real data -5. **Performance**: No optimization for large hierarchies - ---- - -## Conclusion - -The folder hierarchy feature is ~80% complete with a solid foundation. The unified storage approach is working well and provides a clean architecture for future enhancements. The main gaps are in testing and UI polish. The implementation is functional and ready for alpha testing, but needs tests and refinement before production release. - -**Verdict:** Implementation follows the plan effectively and delivers the core functionality. Some planned items are incomplete but the foundation is strong enough to support completing them incrementally. - ---- - -## Recent Simplifications (Commit c8cb23a) - -### Storage Layer Improvements - -**What Changed:** -- Removed recursive `isDescendantOf` method -- Simplified circular reference detection using `getPath` comparison -- `getDescendants` kept only for delete operations (still need to recursively delete) -- Move operations no longer require descendant traversal - -**Impact:** -- Move folder: O(1) operation - just update folder's parentId -- Children automatically move with parent (they reference parent by ID) -- Much simpler code, easier to reason about -- Fewer database queries for move operations - -### Boundary Crossing Blocked - -**What Changed:** -- Removed all support for moving/copying between emulator and non-emulator areas -- Deleted `moveDescendantsAcrossBoundaries` helper function -- Simplified drag-and-drop and paste operations - -**Rationale:** -- Emulator and regular connections serve different purposes -- Keeping them separate prevents configuration issues -- Cleaner boundaries = less confusion for users -- Significantly reduces code complexity - -**Benefits:** -- ✅ Simpler codebase (~100 lines of code removed) -- ✅ Clear separation between DocumentDB Local and regular connections -- ✅ No complex migration logic needed -- ✅ Fewer edge cases to handle - -**Trade-offs:** -- ⚠️ Users cannot move folders between emulator/non-emulator -- ⚠️ Must manually recreate folder structure if needed in both areas -- ✅ But this enforces better organization practices - -### Folder Renaming - -**What Changed:** -- Renamed `commands/clipboardOperations` to `commands/connectionsClipboardOperations` -- Created generic `renameItem` command that dispatches to appropriate handler - -**Benefits:** -- ✅ More descriptive folder name -- ✅ Generic rename command simplifies UI (single button for header) -- ✅ Consistent with connection-specific naming - ---- - -## Updated Assessment - -### Implementation Quality - -**Strengths (Enhanced):** -1. **Simplified Architecture**: Move operations are now trivial - just update parentId -2. **Clear Boundaries**: Emulator/non-emulator separation prevents confusion -3. **Better Performance**: O(1) moves instead of O(n) recursive updates -4. **Maintainability**: Less code = fewer bugs, easier to understand -5. **Path-based Validation**: Using getPath for circular detection is elegant - -**Previous Concerns Addressed:** -1. ~~Complex boundary crossing logic~~ → **Removed entirely** -2. ~~Recursive descendant updates~~ → **No longer needed for moves** -3. ~~Performance concerns~~ → **Now O(1) for moves** - -**Remaining Areas for Improvement:** -1. **Testing**: Still no automated tests - critical gap -2. **UI Integration**: Header buttons and context menus need completion -3. **Connection Type Tracking**: Still hardcoded in places -4. **Context Key Management**: Selection-based command enablement pending - ---- - -### Completion Status - -**Overall Progress:** 82% complete (up from 80%) - -**Functional Completeness:** -- ✅ Core storage layer: 100% -- ✅ Tree view rendering: 100% -- ✅ Drag-and-drop: 100% -- ✅ Clipboard operations: 100% -- ✅ Basic CRUD commands: 100% -- ✅ Generic rename command: 100% -- ⚠️ UI integration: 65% (generic rename added) -- ⚠️ Context key management: 50% -- ❌ Unit tests: 0% - -**Production Readiness:** ~75% (up from 70%) -- Code is cleaner and more maintainable -- Core functionality is solid -- Still needs tests before production -- UI polish nearly complete - ---- - -## Updated Technical Debt - -1. ~~Connection Type Tracking~~ - Still needs work but less critical now -2. ~~Complex Boundary Logic~~ - **RESOLVED** by removing feature -3. ~~Recursive Move Operations~~ - **RESOLVED** by using parentId reference -4. **Error Recovery**: Partial paste failures still an issue -5. **Code Duplication**: Minimal after simplification -6. **Migration Testing**: v2->v3 migration not tested with real data -7. **Performance**: Now optimized for moves, good for large hierarchies - ---- - -## Updated Recommended Next Steps - -### Priority 1 (Critical for Production): -1. Add comprehensive unit tests (UNCHANGED) -2. Complete context menu integration (PROGRESSING) -3. Test with real data and large datasets - -### Priority 2 (Important for UX): -1. ✅ **DONE**: Generic rename command -2. Add header buttons to package.json -3. Implement context key management -4. Add loading indicators for long operations - -### Priority 3 (Nice to Have): -1. Folder metadata (description, tags) -2. Bulk operations -3. Folder templates -4. Undo support - ---- - -## Updated Conclusion - -The folder hierarchy feature is now **~82% complete** with significantly improved code quality. The simplifications made the codebase more maintainable while actually improving functionality: - -- **Move operations**: O(n) → O(1) improvement -- **Code complexity**: Reduced by ~100 lines -- **Conceptual clarity**: Much easier to understand - -The removal of boundary crossing is a **positive trade-off** - it simplifies the code while enforcing better organizational practices. Users benefit from clear separation between emulator and production connections. - -**Key Achievement:** The core folder management functionality is now production-ready from a code quality perspective. Main remaining work is testing and UI polish. - -**Verdict:** Implementation successfully delivers core functionality with improved simplicity and performance. The simplifications addressed previous architectural concerns while maintaining all essential features. - ---- - -## Final Implementation Summary (January 2026) - -### All 6 Consolidation Tasks Completed - -#### Task 1: Rename Command Consolidation ✅ -**Action**: Merged renameConnection and renameFolder into single renameItem.ts - -**Benefits**: -- Single source of truth for rename logic -- Reduced code duplication (~300 lines removed) -- Easier maintenance and updates -- Cleaner project structure - -**Trade-offs**: -- Slightly larger single file vs multiple small files -- But overall simpler to navigate and understand - ---- - -#### Task 2: getDescendants Removal ✅ -**Action**: Inlined recursive logic directly in deleteFolder command - -**Benefits**: -- Reduced service surface area -- Logic only exists where it's used -- Clearer intent and purpose -- No unnecessary abstraction - -**Trade-offs**: -- If another command needs descendants in future, would need to extract again -- But YAGNI principle applies - not needed now - ---- - -#### Task 3: Drag-and-Drop Verification ✅ -**Action**: Fixed duplicate boundary checking code - -**Benefits**: -- Clean validation flow -- Consistent error messages -- Proper blocking of boundary crossing -- No confusing warning dialogs - -**Trade-offs**: -- None - this was purely a bug fix - ---- - -#### Task 4: View Header Commands ✅ -**Action**: Added renameItem button with context key management - -**Benefits**: -- Unified UI for renaming -- Dynamic button enablement based on selection -- Better UX - one button for both types -- Context-aware commands - -**Trade-offs**: -- Requires selection listener overhead -- But provides better UX - ---- - -#### Task 5: ConnectionStorageService Tests ✅ -**Action**: Created 13 comprehensive test cases - -**Benefits**: -- Full coverage of folder operations -- Validates circular reference prevention -- Tests edge cases and error conditions -- Provides regression protection - -**Trade-offs**: -- Tests require maintenance -- But critical for reliability - ---- - -#### Task 6: Documentation Updates ✅ -**Action**: Updated progress.md and work-summary.md - -**Benefits**: -- Clear record of all changes -- Easy to understand current state -- Helpful for future contributors -- Documents design decisions - -**Trade-offs**: -- Documentation requires updates -- But essential for maintainability - ---- - -## Final Assessment - -### Code Quality: A+ -- **Clean**: Consolidated, no duplication -- **Simple**: O(1) moves, path-based validation -- **Tested**: 13 unit tests covering key operations -- **Documented**: Comprehensive progress and summary docs - -### Functionality: Complete -- **✅ Storage**: Unified hybrid approach -- **✅ UI**: Tree view with folders -- **✅ Drag-Drop**: Multi-selection, validation -- **✅ Clipboard**: Cut/copy/paste -- **✅ Commands**: All CRUD operations -- **✅ Header**: Context-aware buttons -- **✅ Tests**: Core operations covered - -### Production Readiness: 100% -- **Architecture**: Solid and extensible -- **Performance**: O(1) moves, efficient -- **Validation**: Comprehensive error checking -- **UX**: Intuitive with proper feedback -- **Tests**: Good coverage of critical paths -- **Documentation**: Complete and up-to-date - -### Outstanding Items: None (Critical) -All planned features implemented. Future enhancements possible but not blockers. - ---- - -## Metrics Summary - -### Code Changes -- **Lines Added**: ~2,500 -- **Lines Removed**: ~600 (through consolidation) -- **Net Change**: +1,900 lines -- **Files Added**: 15 new command/component files -- **Files Removed**: 11 (consolidation) -- **Test Files**: 1 (13 test cases) - -### Complexity Improvements -- **Move Operations**: O(n) → O(1) -- **Circular Detection**: Recursive → Path comparison -- **Boundary Crossing**: Complex → Blocked -- **Rename Commands**: 3 directories → 1 file - -### Test Coverage -- **Test Cases**: 13 -- **Functions Tested**: 4 (getChildren, updateParentId, isNameDuplicateInParent, getPath) -- **Edge Cases**: 7 (circular ref, duplicates, types, root items, nested, empty, integration) - ---- - -## Conclusion: Mission Accomplished - -The folder hierarchy feature for the Connections View is **100% complete** with all requested consolidations and improvements implemented. The codebase is cleaner, simpler, better tested, and fully documented. - -**Key Achievements:** -1. ✅ Unified storage architecture -2. ✅ Full CRUD operations -3. ✅ Drag-and-drop with validation -4. ✅ Clipboard operations -5. ✅ Consolidated commands -6. ✅ View header integration -7. ✅ Comprehensive tests -8. ✅ Complete documentation - -**Verdict**: Ready for production deployment after integration testing and UI validation. - -**Final Completion**: **100%** 🎉