From 45ed29255d0cb660226994ffd7b57246d79e391e Mon Sep 17 00:00:00 2001 From: Tomasz Naumowicz Date: Thu, 9 Jul 2026 15:30:53 +0200 Subject: [PATCH 1/7] refactor: update summary table formatting for clarity and consistency --- .../PRs/documentdb-quickstart/ux-review.md | 532 ++++++++++++++++++ l10n/bundle.l10n.json | 2 +- .../LocalQuickStart/LocalQuickStartItem.ts | 18 +- 3 files changed, 539 insertions(+), 13 deletions(-) create mode 100644 docs/ai-and-plans/PRs/documentdb-quickstart/ux-review.md diff --git a/docs/ai-and-plans/PRs/documentdb-quickstart/ux-review.md b/docs/ai-and-plans/PRs/documentdb-quickstart/ux-review.md new file mode 100644 index 000000000..8777edd3c --- /dev/null +++ b/docs/ai-and-plans/PRs/documentdb-quickstart/ux-review.md @@ -0,0 +1,532 @@ +# DocumentDB Local Quick Start — UX Review Notes + +**Feature area:** `src/services/localQuickStart/`, `src/commands/localQuickStart/`, +`src/tree/connections-view/LocalQuickStart/`, `src/webviews/documentdb/localQuickStart/` +**Related design docs:** `docs/ai-and-plans/local-quickstart/local-quickstart-v2.md`, +`docs/ai-and-plans/PRs/local-quickstart-poc/`, `docs/ai-and-plans/PRs/local-quickstart-multi-instance/` +**Review date:** 2026-07-09 + +## What this document is + +This is a running log of manager (Tomaz) runtime observations on the Local Quick Start feature, +each enhanced with code research: exact locations, why the current behavior happens, and related +context or prior decisions. **Nothing in this document has been fixed yet** — findings are +discovery notes to inform a later triage/implementation pass. Severity/priority calls and actual +fixes are deliberately deferred to that follow-up. + +--- + +## 1. Legacy emulator connections migration folder + +**Observation:** Is the one-time migration of original "DocumentDB Local" emulator connections to +a new folder still happening? What's the folder called? Sorting could place it somewhere +unexpected. + +**Code findings:** + +- The one-time migration lives in [src/services/legacyEmulatorMigration.ts](../../../../src/services/legacyEmulatorMigration.ts), + invoked from [src/documentdb/ClustersExtension.ts](../../../../src/documentdb/ClustersExtension.ts) + on every activation (`migrateLegacyEmulatorConnections`), guarded by a `globalState` flag + (`documentdb.localQuickStart.legacyEmulatorMigration.completed`) so it only runs once. Yes, it is + still active code — nothing has removed it. +- Destination folder name: **`Local Connections (Legacy)`** (`LEGACY_FOLDER_BASE_NAME`), with a + deterministic id (`vscode-documentdb.legacyLocalConnectionsFolder`) so retries reuse the same + folder instead of duplicating it. If a user already has a root folder with that exact name, the + migration suffixes it `(2)`, `(3)`, … (`uniqueLegacyFolderName()`). +- The folder is created directly under the **root of the regular Clusters/Connections zone** — it + is a normal, renamable, movable folder like any user-created one, not a pinned/reserved node. + Emulators-zone sub-folders are intentionally **flattened**: every migrated connection lands + directly in this one folder (documented as a deliberate scope cut, not a bug). +- The legacy `Emulators` storage zone (and its `LocalEmulatorsItem` tree node) is kept as a + read-only rollback path until migration succeeds, then the tree node is retired — see the + `isLegacyEmulatorMigrationComplete()` gate in + [ConnectionsBranchDataProvider.ts](../../../../src/tree/connections-view/ConnectionsBranchDataProvider.ts) (root items array). + +**Why sorting can surprise:** Root-level items are assembled in `ConnectionsBranchDataProvider.getRootItems()` +in this fixed order: `LocalQuickStartItem` → (legacy `LocalEmulatorsItem`, if migration not yet +complete) → **all cluster folders, alphabetically** → **all ungrouped connections, alphabetically** +→ `New Connection` placeholder. Folders are sorted with a single +`localeCompare(..., { numeric: true })` pass across _all_ root folders — there is no special-casing +that pins "Local Connections (Legacy)" to the top or bottom. So depending on the user's other +folder names, it can land anywhere in the alphabetical list (e.g. between "Azure" and "Backups"), +which is easy to miss on a first look since nothing visually marks it as "this is where your old +stuff went." + +**Open questions for later:** Should this folder be visually distinguished (icon, description +badge) or pinned to a fixed position (e.g., always first/last among folders) so users don't have to +hunt for it after upgrading? Should there be a one-time toast/notification pointing at it instead of +relying on the user noticing it in an alphabetical list? (The migration code comment doesn't +mention a toast — worth confirming whether one exists elsewhere; a search for a notification tied to +`MIGRATION_COMPLETED_KEY` didn't surface one in this pass.) + +--- + +## 2. "Delete Container" confirmation doesn't match the standard delete flow + +**Observation:** The Delete Container modal is fine as a concept, but it should use the same delete +confirmation code path as other destructive actions (e.g., in Settings), and should not use +em dashes. + +**Code findings:** + +- Standard destructive commands — [removeConnection.ts](../../../../src/commands/removeConnection/removeConnection.ts), + [deleteCollection.ts](../../../../src/commands/deleteCollection/deleteCollection.ts), + [deleteDatabase.ts](../../../../src/commands/deleteDatabase/deleteDatabase.ts), + [ConfirmDeleteStep.ts](../../../../src/commands/connections-view/deleteFolder/ConfirmDeleteStep.ts) — + all call **`getConfirmationAsInSettings()`** from + [src/utils/dialogs/getConfirmation.ts](../../../../src/utils/dialogs/getConfirmation.ts). That + function reads the `ext.settingsKeys.confirmationStyle` VS Code setting and dispatches to one of + three styles: type-the-word, a number challenge, or a plain click-confirm — i.e. "confirm like in + settings" is a user-configurable, shared code path. +- `deleteQuickStartInstance()` in + [localQuickStartCommands.ts](../../../../src/commands/localQuickStart/localQuickStartCommands.ts) + instead calls **`getConfirmationWithClick()`** directly — the click-only style, unconditionally, + ignoring the user's configured `confirmationStyle`. It's the odd one out; the only other callers + of `getConfirmationWithClick` directly (bypassing the setting) are `hideIndex`/`unhideIndex` + (non-destructive, reversible operations) and two spots in `QueryInsightsAIService.ts` — none of + which are "delete data permanently" operations like this one. +- Em dashes: the two confirmation `detail` strings in `deleteQuickStartInstance` both contain a + literal em dash — `'…This cannot be undone — you can recreate a fresh instance…'` — plus one in a + code comment above it. Same character also appears in several other user-facing strings in this + feature (e.g., `LocalQuickStart.tsx` next-steps bullets: `'Open Connection — browse your +databases…'`, `'Copy Connection String — use it from…'`, and the "waiting" timeout message + `'…may still be initializing — keep waiting, view the logs, or start over.'`). Worth a sweep of + the whole feature, not just the delete dialog, if em dashes are to be eliminated consistently. + +**Open questions for later:** Should Delete Container switch to `getConfirmationAsInSettings` (word +confirmation would ask the user to type something — worth deciding what word, since there's no +existing "type container name" concept for Quick Start)? Should the whole feature's strings be +swept for em dashes/other punctuation-style consistency in one pass rather than piecemeal? + +--- + +## 3. Empty Quick Start list — tree item wording + +**Observation:** The tree items shown when there's no managed instance yet should be reworded. The +action row should say something like "Click here to…". "Learn more" could be dropped from the list +and instead live in the context menu of the local node. + +**Code findings:** Both rows are built in `LocalQuickStartItem.getChildren()` +([LocalQuickStartItem.ts](../../../../src/tree/connections-view/LocalQuickStart/LocalQuickStartItem.ts)) +for the `NotInstalled` state (no metadata yet): + +- Action row: label `l10n.t('Quick Start — Install & try DocumentDB locally')`, rocket icon, + `contextValue: 'treeItem_quickStartAction'`, bound to command + `vscode-documentdb.command.localQuickStart.open`. +- Learn-more row: label `l10n.t('Learn more…')`, `link-external` icon, + `contextValue: 'treeItem_quickStartLearnMore'`, bound to `vscode.open` with a hardcoded URL to + `https://github.com/microsoft/documentdb`. +- Also note: this same action label/icon/command (`treeItem_quickStartAction` / + `localQuickStart.open`) is reused for the `Missing` state's alternate rendering path (see finding + #12 below) — any rewording of the "click to do X" phrasing should stay consistent across both + surfaces. +- Neither row currently has **any** `view/item/context` entry in `package.json` — a search for + `treeItem_localQuickStart`, `treeItem_quickStartAction`, or `treeItem_quickStartLearnMore` in + `package.json` returns no matches. So today "Learn more" only exists as its own list row; there is + no existing context-menu wiring to move it into, moving it to the parent + (`treeItem_localQuickStart`) node's context menu would be new plumbing, not a small toggle. + +**Open questions for later:** Confirm exact copy for the action row (e.g. "Click here to install +DocumentDB locally with Quick Start" — needs to stay short for a tree row). Decide whether "Learn +more" moves to the parent `DocumentDB Local - Quick Start` node's context menu (new `view/item/context` +entry gated on `treeItem_localQuickStart`) or is dropped from the tree entirely in favor of the +walkthrough/README. + +**Fix applied (2026-07-09):** Implemented the quick, low-risk half of this finding directly (the +"move Learn more into the parent context menu" question above is still open/deferred — this only +covers the reword + drop): + +- Action row label changed from `'Quick Start — Install & try DocumentDB locally'` to + `'Click here to install & try DocumentDB locally'`. +- The separate `'Learn more…'` row (`contextValue: 'treeItem_quickStartLearnMore'`) was removed + entirely from the `NotInstalled` empty state — it is not (yet) relocated to a context menu; that + part of the original ask is left as an open item above, since no `view/item/context` plumbing + exists for `treeItem_localQuickStart` today (would be new work, not a move). +- Both changes are in + [LocalQuickStartItem.ts](../../../../src/tree/connections-view/LocalQuickStart/LocalQuickStartItem.ts), + `getChildren()`'s `NotInstalled` branch. The `Missing` state's row (finding #11) still reuses the + `treeItem_quickStartAction` contextValue/command but keeps its own distinct label + (`'Missing · click to recreate'`) — unaffected by this change. +- Not done as part of this quick fix: the `treeItem_quickStartLearnMore` contextValue is no longer + referenced anywhere, and the external link (`https://github.com/microsoft/documentdb`) is no + longer reachable from the tree at all until/unless it's re-added via a context menu. + +--- + +## 4. Webview header changes with state and can get stuck + +**Observation:** The header should be static; today it changes depending on status/progress/errors, +which is unexpected. It sometimes gets stuck on "Setting up DocumentDB Local…" even when the +operation is actually complete. + +**Code findings** (all in +[LocalQuickStart.tsx](../../../../src/webviews/documentdb/localQuickStart/LocalQuickStart.tsx)): + +- The header ("hero") is rendered by a local `hero(title, subtitle)` helper, called **three + separate times** with different literal titles depending on `phase`: + - `phase === 'dockerNotReady'` → `l10n.t('Docker is required')` + - `phase === 'provisioning' || phase === 'success' || phase === 'failed'` → **the same call**, + `l10n.t('Setting up DocumentDB Local…')`, with the subtitle showing the elapsed timer only + while `phase === 'provisioning'` (empty string otherwise) + - `phase === 'review'` (the initial screen) → `l10n.t('Start DocumentDB Local')` +- This confirms both parts of the observation exactly: + 1. The header text does change across phases (three different literal strings), which reads as + inconsistent/jumping if a user watches the panel through a full run. + 2. **The "stuck" complaint is a real bug, not perception**: `success` and `failed` reuse the exact + same title as `provisioning` — "Setting up DocumentDB Local…" is still shown even once the + instance is fully running (`phase === 'success'`) or once setup has definitively failed + (`phase === 'failed'`). Only the content _below_ the header (the `successBox`/`errorBox`, the + stage checklist, and the action buttons) reflects the actual outcome — the header itself never + catches up. +- There is a `provisioningStatusMessage` computed a few lines above (derived from the active stage + label) that IS phase/stage-aware and correctly blanks itself out once any stage errors — but it's + only surfaced to screen readers via a visually-hidden `aria-live` region, not shown visually. + +**Open questions for later:** Should the header become a single static string for the whole +provisioning/success/failed lifecycle (e.g. "DocumentDB Local"), with all state communicated via +subtitle/body instead? Or should it explicitly branch on `success`/`failed` like the review/error +screens already do for `dockerNotReady`? + +--- + +## 5. "Waiting for DocumentDB to accept connections" wording / intermediate progress + +**Observation:** This message could be reworded, or show some intermediate progress as a sub-info, +since it can take time. + +**Code findings:** + +- Stage label: `STAGE_LABELS.waiting = l10n.t('Waiting for DocumentDB to accept connections')` in + `LocalQuickStart.tsx`. The service-side stage message is + `'Waiting for DocumentDB to accept connections…'` in + [QuickStartService.ts](../../../../src/services/localQuickStart/QuickStartService.ts) (`provision()`, + `--- waiting ---` section). +- This stage wraps `waitForReadiness()`, which polls the wire protocol (a real MongoClient + connection attempt) with a `READINESS_TIMEOUT_MS = 180_000` (3 minutes) budget and a + `PROBE_SERVER_SELECTION_TIMEOUT_MS = 3_000` per attempt — i.e. it can legitimately retry for up to + 3 minutes with no visible change other than the elapsed-time counter in the (currently static, + see #4) header subtitle. +- There is no intermediate sub-status during this stage today — the stage row just shows a spinner + - the same static label the whole time; the per-attempt retry count / time remaining isn't + surfaced. Logs _do_ stream to the "DocumentDB Local Quick Start" output channel during this window + (via `followLogs`, started right after `docker start`), so the information exists, just not + in-panel. +- On timeout, a distinct `ReadinessTimeoutError` is raised (kept separate from hard failures + specifically so the container is preserved and "Wait longer" can resume it) — the `failed` phase + with `timedOut === true` shows a dedicated message: `'The container is running, but DocumentDB has +not accepted connections yet. It may still be initializing — keep waiting, view the logs, or start +over.'` (contains another em dash — see #2). + +**Open questions for later:** What sub-info would be useful here — elapsed time within the stage, +attempt count, a rotating "still working" hint after N seconds, or a link to "View Docker output" +surfaced earlier instead of only at the bottom of the panel? Any reword should stay accurate for +both a fresh pull-to-ready cold start and a plain restart (both funnel through the same stage). + +--- + +## 6. Image pull/download progress tracking + +**Observation:** Do we track progress while the image is downloaded? That's a long-running +operation too. + +**Code findings:** + +- `provision()` yields exactly one `active` `StageEvent` for the pulling stage — `stageEvent('pulling', +'active', 'Pulling the official image…')` — then `await this.runtime.pullImage(imageRef, +cts.token)`, then one `done` event. No intermediate events are yielded between those two. +- `ContainerRuntime.pullImage()` ([ContainerRuntime.ts](../../../../src/services/localQuickStart/ContainerRuntime.ts)) + runs `docker pull` through the shared CLI shell-runner (`@microsoft/vscode-container-client`'s + `DockerClient`), with `stdout`/`stderr` piped only to the output channel via + `MaskedChannelWritable` (line-buffered + secret-masked). Nothing from that stream is captured, + parsed, or forwarded back to the caller — so per-layer Docker progress (`Downloading`, + `Verifying Checksum`, `Extracting`, `Pull complete`, etc., one line per layer per event when not + attached to a TTY) is visible **only** in the raw output channel text, never in the webview. +- So: no, there is no structured progress tracking for the pull today, confirming the observation. + The webview shows a spinner + static label for however long the pull takes, same as "waiting" (#5). + +**Ideas for indeterminate/step progress (no fix applied, just candidates to discuss):** + +- Docker's plain-text pull output (non-TTY) already gives one line per layer per status + transition, including a `": Pull complete"` line once each layer finishes. Since the + number of layers isn't known until Docker starts announcing "Pulling fs layer" lines, a + precise percentage isn't available up front, but a **running count of "N of M layers pulled"** + is derivable simply by tracking, per pull, how many distinct layer ids have been seen so far + (M, growing) vs. how many have reached `Pull complete` (N) — that only requires + `ContainerRuntime.pullImage` to accept an optional line/progress callback instead of writing + straight to the channel, and forwarding parsed counts as extra `StageEvent` fields (mirroring how + `boundPort`/`timedOut` already ride along on `StageEvent` for other stages). + - **Caveat:** because non-TTY docker pull is a plain scrolling text stream, not a JSON event + stream, layer counting means resurrecting a small regex parser over free-form CLI text. It'd be + more robust (and would expose _actual_ byte totals per layer, since Docker does know each + layer's compressed size) to switch this specific call to the Docker Engine API's pull endpoint + directly (e.g. via `dockerode`'s `docker.pull()`, which streams structured JSON progress events + with `id`, `status`, and `progressDetail: { current, total }` per layer) rather than parsing CLI + text — a larger change than the current CLI-wrapper approach, worth flagging as a design + decision rather than a quick tweak. +- On the visual side, since a true 0–100% isn't known in advance without that JSON-stream rework, + an **indeterminate `ProgressBar`** (Fluent UI v9 `ProgressBar` with no `value` prop renders as an + animated indeterminate bar) reads better than a static spinner for a stage that can run tens of + seconds, and is already a component this design system ships — no new dependency. Layered under + it, a small live sub-label like "Downloading… (3 of 6 layers)" would satisfy the "some kind of + progress" ask without needing byte-accurate totals — that count is the cheap parse described + above, not the JSON-stream rework. +- A literal "dot per completed layer" (e.g. a row of small filled/empty dots) is visually appealing + for a small, bounded number of steps, but is a poor fit here specifically because the total layer + count isn't known until the pull starts announcing them — the dot row would have to grow/shrink + as new layers appear, which reads as jittery. The numeric "N of M" counter (or an indeterminate + bar with that counter as a sub-label) avoids that problem while still being honest about what we + actually know. +- Either option is a genuine, if modest, plumbing change (new callback param on `pullImage` / + `IContainerRuntime`, a new `StageEvent` shape, and webview rendering), not a copy tweak — worth + scoping separately from the wording-only findings above. + +--- + +## 7. Running-container tree item doesn't share cluster context commands + +**Observation:** The tree item for a running managed instance doesn't share context menu commands +with regular clusters. Do we still extend the cluster item base class? If not, what would break if +we did? + +**Code findings:** + +- Yes, it still extends the base class: `QuickStartClusterItem extends DocumentDBClusterItem` in + [LocalQuickStartItem.ts](../../../../src/tree/connections-view/LocalQuickStart/LocalQuickStartItem.ts). + It overrides `getTreeItem()` only to force the state-aware description text, and reuses the base + class for icon, tooltip, and (critically) all the actual cluster-browsing behavior — the comment + on the class explicitly says browsing "reuses the base `DocumentDBClusterItem`." +- The reason regular cluster context commands don't show up: the **contextValue is fully replaced**, + not extended. The constructor sets + `this.contextValue = createContextValue([INSTANCE_CONTEXT, stateToken])` — i.e. + `treeItem_quickStartInstance` + `state_running` (etc.) — which **discards** whatever contextValue + the base `DocumentDBClusterItem` constructor/`getTreeItem()` would normally compute (which + includes the `treeitem_documentdbcluster` token). Every regular-cluster `view/item/context` entry + in `package.json` gates on `viewItem =~ /\btreeitem_documentdbcluster\b/i` (e.g. + `updateConnectionString`, `updateCredentials`, `renameConnection`, `moveItems`, + `removeConnection`) — none of those `when` clauses match a Quick Start row, since that token is + never present on it. +- This looks deliberate, not an oversight: the class-level comment says the Quick Start node + "carr[ies] a Quick-Start-specific context value so the instance shows Quick Start lifecycle + actions **instead of** the generic cluster menus." The `INSTANCE_CONTEXT` token instead gates the + Quick Start–specific commands (`start`/`stop`/`restart`/`delete`/`copyConnectionString`/ + `copyPassword`/`viewLogs`), each `when`-clause requiring both `treeItem_quickStartInstance` and a + specific `state_*` token (see `package.json`, `view/item/context` for + `vscode-documentdb.command.localQuickStart.*`). +- **What would likely break if the base contextValue were also kept (appended rather than + replaced):** the generic cluster commands assume storage-backed operations — `removeConnection` + deletes a _stored connection record_, `moveItems`/`renameConnection` operate against + `ConnectionStorageService`, `updateConnectionString`/`updateCredentials` open storage-editing + wizards. The Quick Start instance is explicitly **not** stored in any zone (the model comment says + "the Quick Start managed instance is in-memory (CredentialCache-based) and not stored in any + zone"). Enabling those commands as-is would let a user try to "rename"/"move to folder"/"delete + connection" an item that has no storage record to act on — likely a silent no-op at best, or an + exception/orphaned-state at worst, unless each of those commands were also taught to special-case + the Quick Start row. So the current full-replacement approach avoids a class of bugs, but at the + cost of losing genuinely useful, storage-independent commands too, e.g. "Open in Shell" / "New + Query" style commands if any are gated only on `treeitem_documentdbcluster` without also touching + storage — worth an inventory of exactly which cluster commands are storage-dependent vs. purely + connection-based, since only the latter could be safely re-enabled. + +**Open questions for later:** Would it be worth splitting the base contextValue into a +storage-dependent subset and a connection-only subset, so Quick Start could opt into the latter? Or +is a curated, Quick-Start-specific menu (as today) actually the right long-term shape, just missing +a few commands (e.g., "Open in Shell") that should be added explicitly rather than inherited? + +--- + +## 8. Copy Connection String is a separate, slightly different implementation + +**Observation:** Copy Connection String appears to be reimplemented for Quick Start and behaves +slightly differently — it doesn't ask whether to include the password. + +**Code findings:** + +- Regular clusters: `copyConnectionString()` in + [copyConnectionString.ts](../../../../src/commands/copyConnectionString/copyConnectionString.ts) + calls `context.ui.showQuickPick` with two options — "The connection string will not include the + password" vs. "...will include the password" — gated by `canIncludeNativePassword()`, only + prompting when native-auth credentials are actually present. Well tested (see + `copyConnectionString.test.ts`, e.g. "picks WITH/WITHOUT password" cases). +- Quick Start: `copyQuickStartConnectionString()` in + [localQuickStartCommands.ts](../../../../src/commands/localQuickStart/localQuickStartCommands.ts) + is a completely separate, much shorter function — it reads `metadata.connectionString` (which + already has the generated username/password embedded from `composeConnectionString()`) and copies + it directly to the clipboard with no prompt at all. There's a **separate** `copyQuickStartPassword()` + command for copying just the password, but the "copy connection string" action always includes the + password silently. +- This is a genuine behavioral difference, not just a styling one: for a regular cluster, a user is + asked each time whether the clipboard should carry the plaintext password; for the Quick Start + instance, the password always goes to the clipboard silently whenever "Copy Connection String" is + used. Given Quick Start's own design docs are otherwise careful about credential handling (masked + output-channel logging, env-file instead of CLI args, etc. — see `outputMasking.ts` and the + `D14`/masking comments throughout `QuickStartService.ts`), this asymmetry stands out. + +**Open questions for later:** Should Quick Start's "Copy Connection String" reuse the same +QuickPick-based confirmation as the regular command (auto-answering "Quick Start always uses native +auth" without extra branching), for parity and because it's also copying a plaintext password to the +clipboard? + +--- + +## 9. "Open terminal in the Docker container" — feasibility + +**Observation:** Would it make sense to add a command to open a terminal inside the Docker +container? Can this be done? + +**Code findings / feasibility:** + +- The repo already has precedent for terminal-backed commands: + [openInteractiveShell.ts](../../../../src/commands/openInteractiveShell/openInteractiveShell.ts) + calls `vscode.window.createTerminal({ name, pty, iconPath })` with a custom + `DocumentDBShellPty` (`vscode.Pseudoterminal`) for the integrated mongosh-style shell — that's a + more elaborate mechanism (a custom pty implementation) built for a different purpose (parsing/ + relaying shell I/O), not required here. +- For a plain "shell into the container" command, the much simpler standard VS Code pattern is + `vscode.window.createTerminal({ name, shellPath: 'docker', shellArgs: ['exec', '-it', containerId, +'/bin/bash'] })` (with a `/bin/sh` fallback if `bash` isn't present in the image) — this is exactly + how other Docker-focused VS Code extensions expose "Attach Shell"/"Open in Terminal". No custom + pty is needed since Docker itself drives the interactive TTY. +- The container id is already tracked in-memory (`InstanceRuntimeState.metadata.containerId`) and + exposed via `QuickStartService.getStatus().metadata`, so the command would have everything it + needs without new state plumbing. `ContainerRuntime` already has `execShellInContainer()`, but + that's a fire-and-forget non-interactive `docker exec` used for the sample-data init script — it + writes to the output channel, not a live TTY, so it isn't directly reusable for an interactive + terminal; a new, simple command function (not a `ContainerRuntime` method) would be the more + natural fit, calling `createTerminal` directly the way `openInteractiveShell` does. +- No fundamental blocker found: Docker is already a hard dependency for this whole feature, so + requiring `docker` on PATH for this command adds no new dependency. The main design questions are + which state(s) it should be enabled for (`state_running` only, presumably — a stopped container + can't `exec` into it) and what shell to try first. + +**Open questions for later:** Confirm which context-menu group/icon this would use, and whether it's +worth a fallback prompt if `bash` isn't in the image (the DocumentDB image's shell availability +wasn't checked in this pass). + +--- + +## 10. How are the Quick Start tree icons chosen? + +**Observation:** How are the icons chosen for the Quick Start DocumentDB instances? + +**Code findings:** There's no bespoke Quick Start icon set — every icon is either the generic +extension logo or inherited/generic VS Code `ThemeIcon`s, reused from existing logic: + +- **Root node** (`DocumentDB Local - Quick Start`): the extension's own product icon — + `vscode-documentdb-icon-{light,dark}-themes.svg` from `getResourcesPath()` — same icon used + elsewhere for the extension/product identity, not specific to Quick Start. +- **Running instance row** (`QuickStartClusterItem`): no icon override at all — it falls through to + the inherited `DocumentDBClusterItem.getTreeItem()` icon logic, which picks + `$(plug)` **iff** `cluster.emulatorConfiguration?.isEmulator` is true, else `$(server-environment)`. + Since the Quick Start model always sets `emulatorConfiguration: { isEmulator: true, ... }` (see + `LocalQuickStartItem.getChildren()`), the running row always gets `$(plug)` — the exact same icon + any other connection flagged as an "emulator" gets (including the legacy migrated connections in + finding #1, which also carry `isEmulator: true`). There is nothing that visually distinguishes "the + one managed-by-Quick-Start instance" from "any other connection someone marked as an emulator." +- **Non-running state rows** (Starting/Stopping/Stopped/Error/Missing): plain generic VS Code + `ThemeIcon`s chosen per state in `LocalQuickStartItem.getChildren()` — + `loading~spin` (Starting/Stopping/Provisioning), `circle-outline` (Stopped), `warning` with + `list.errorForeground`/`list.warningForeground` theme color (Error/Missing). None of these relate + to cluster icons at all; they're the same generic vocabulary used for background-task rows + elsewhere in the extension. +- **Empty-state rows:** `rocket` (action) and `link-external` (Learn more) — again generic + `ThemeIcon`s, not custom art. + +**Open questions for later:** Is `$(plug)` (shared with any manually-flagged "emulator" connection) +an acceptable representation for the one managed local instance, or should Quick Start rows carry a +distinct icon (e.g. reusing the rocket used for the root node/action row) so the "this is the +Quick-Start-managed one" identity is visually obvious at a glance, independent of state? + +--- + +## 11. Deleted-externally container isn't handled when the user tries "Start" + +**Observation (repro):** Created an instance via Quick Start, stopped it, then deleted the +_container_ directly via Docker (outside VS Code). Back in VS Code, clicking Start on the (still +"Stopped"-looking) tree row does nothing visible — no error dialog, no tree update — only the raw +Docker error appears in the "DocumentDB Local Quick Start" output channel: + +``` +$ docker container inspect --format '{{json .}}' 57f8311fd9ec77aed5483b6ef36da22890f8a71e1057073f884273ba89c68248 +Error response from daemon: No such container: 57f8311fd9ec77aed5483b6ef36da22890f8a71e1057073f884273ba89c68248 +``` + +**Code findings — this traces to a specific, reproducible gap, not vague flakiness:** + +- `ContainerRuntime.inspectContainer()` ([ContainerRuntime.ts](../../../../src/services/localQuickStart/ContainerRuntime.ts)) + wraps the CLI call in `try { ... } catch { return undefined; }` — it deliberately swallows _all_ + errors, including "No such container," into a plain `undefined` return. That's a reasonable + existence-check contract on its own. But every call is still run through the shared + `makeRunner()`, whose `onCommand` callback unconditionally logs `'$ ' + command` to the output + channel _before_ running, and whose `stdErrPipe` streams the daemon's stderr response into the + same channel regardless of whether the caller ends up treating the failure as "expected." That's + exactly the two lines the user saw — this is the extension's own inspect call being logged, not a + manual terminal session. +- `start()` (`QuickStartService.ts`) calls `this.isManaged(id, alias)` before doing anything else: + ```ts + private async isManaged(containerId, alias) { + const item = await this.runtime.inspectContainer(containerId); + if (!item || item.labels?.[QUICK_START_LABEL_KEY] !== '1') return false; + return this.aliasMatches(...); + } + ``` + When the container has been removed, `inspectContainer` returns `undefined` → `isManaged` returns + `false` — **the exact same `false` it would return for "this container exists but isn't ours."** + `isManaged()` cannot distinguish "gone" from "not managed by us." +- Back in `start()`: + ```ts + if (!id || !(await this.isManaged(id, alias)) || !(await this.liveStateGuard(id, ['stopped']))) { + return; + } + ``` + Because `isManaged` already returned `false`, the `||` short-circuits — **`liveStateGuard()` is + never even called** for this scenario. That matters because `liveStateGuard` is the piece of code + that _would_ have handled this gracefully: it explicitly checks for a `'missing'` live state, calls + `refreshLiveState()`, and shows `vscode.window.showInformationMessage(...)` telling the user the + instance changed in another window. That path exists and is well-built for _other_ window / + external-stop scenarios — it's just unreachable here because `isManaged` gates in front of it and + returns the same "false" for two different situations. +- Net effect: `start()`'s whole body becomes a silent early `return` — no `setStatus(...)` call, so + `statusEmitter` never fires, so `ConnectionsBranchDataProvider.refresh()` (wired in + `ClustersExtension.ts` via `QuickStartService.onDidChangeStatus`) never runs, so the tree keeps + showing the stale "Stopped" row indefinitely until something else (e.g. expanding/collapsing the + node, which re-triggers `LocalQuickStartItem.getChildren()` → `refreshLiveState()`) happens to + refresh it independently. +- Worth noting there's already a very similar, well-designed pattern for a related case: + `refreshLiveState()` itself _does_ correctly set `entry.missing = true` and fire the status event + when a periodic/tree-driven check finds the container gone — and the tree already has full + rendering support for that (`Missing · click to recreate`, bound to reopen the Quick Start panel, + which re-provisions). The gap is specifically that the **lifecycle actions** (`start`/`stop`/ + `restart`, and to a lesser extent `deleteContainer`, which has its own separate fallback lookup) + don't funnel through that same "missing" detection before giving up. + +**Open questions for later (explicitly deferred — discuss when fixing):** + +- Should `isManaged()` (or a new check ahead of it in `start`/`stop`/`restart`) distinguish "no + container found" from "found but not ours," and in the former case do what `liveStateGuard` + already does for the multi-window case — refresh live state + surface a message — rather than + silently returning? +- What should the message/options be? The user's suggestion: tell them the container wasn't found + (possibly deleted), and offer next steps — Delete (clean up the stale metadata/volume) and/or + "reload with same settings" (re-provision using the same image/credentials, similar to how the + existing `Missing` row already offers "click to recreate"). Note the `Missing` row's recreate flow + and the reuse-credentials path in `provision()` (`getReusableCredentials`) may already cover most + of "reload with same settings" — worth checking whether that flow is sufficient once actually + reachable from `start()`/`stop()`/`restart()`, or needs its own affordance. + +--- + +## Summary table + +| # | Topic | Status | +| --- | ------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------- | +| 1 | Legacy migration folder name/placement | Confirmed: `Local Connections (Legacy)`, alphabetically sorted with no pinning | +| 2 | Delete Container confirmation + em dashes | Confirmed: bypasses `getConfirmationAsInSettings`; em dashes present in multiple strings | +| 3 | Empty-state tree row wording/Learn-more placement | **Fix applied:** reworded to "Click here to install & try DocumentDB locally"; "Learn more" row dropped (context-menu relocation left open) | +| 4 | Webview header instability / stuck text | Confirmed real bug: `success`/`failed` reuse the `provisioning` header verbatim | +| 5 | "Waiting for connections" wording/sub-progress | Confirmed: static label for up to 3 minutes, no sub-status shown in-panel | +| 6 | Image pull progress tracking | Confirmed: none; ideas drafted (layer counter, indeterminate `ProgressBar`) | +| 7 | Running-instance row vs. cluster context commands | Confirmed deliberate contextValue replacement; discussed what would break if merged | +| 8 | Copy Connection String reimplementation | Confirmed: separate function, no password-inclusion prompt unlike regular clusters | +| 9 | Open terminal in container | Feasible via `createTerminal` + `docker exec -it`; no blocker found | +| 10 | Icon selection logic | Confirmed: no bespoke icons; inherited/generic `ThemeIcon`s throughout | +| 11 | Externally-deleted container not handled on Start | Root cause found: `isManaged()` conflates "missing" with "not ours," bypassing the existing `liveStateGuard` missing-state handling | diff --git a/l10n/bundle.l10n.json b/l10n/bundle.l10n.json index b49088d6b..080218316 100644 --- a/l10n/bundle.l10n.json +++ b/l10n/bundle.l10n.json @@ -315,6 +315,7 @@ "Choose your provider…": "Choose your provider…", "Choose your Service Provider": "Choose your Service Provider", "Clear Query": "Clear Query", + "Click here to install & try DocumentDB locally": "Click here to install & try DocumentDB locally", "Click here to open the shell": "Click here to open the shell", "Click here to retry": "Click here to retry", "Click here to update credentials": "Click here to update credentials", @@ -1155,7 +1156,6 @@ "Query took {0}ms to complete.\n\nThis may impact user experience.\n\nConsider adding indexes or optimizing your query structure.": "Query took {0}ms to complete.\n\nThis may impact user experience.\n\nConsider adding indexes or optimizing your query structure.", "Query took {0}s to complete.\n\nThis significantly impacts performance and user experience.\n\nImmediate optimization is recommended.": "Query took {0}s to complete.\n\nThis significantly impacts performance and user experience.\n\nImmediate optimization is recommended.", "Quick Actions": "Quick Actions", - "Quick Start — Install & try DocumentDB locally": "Quick Start — Install & try DocumentDB locally", "Reachable": "Reachable", "Reads a kubeconfig YAML file from disk and links to it by path.": "Reads a kubeconfig YAML file from disk and links to it by path.", "Reads clipboard content and saves a copy as a kubeconfig source.": "Reads clipboard content and saves a copy as a kubeconfig source.", diff --git a/src/tree/connections-view/LocalQuickStart/LocalQuickStartItem.ts b/src/tree/connections-view/LocalQuickStart/LocalQuickStartItem.ts index 1ca4a0dd9..6763fe2e1 100644 --- a/src/tree/connections-view/LocalQuickStart/LocalQuickStartItem.ts +++ b/src/tree/connections-view/LocalQuickStart/LocalQuickStartItem.ts @@ -54,8 +54,8 @@ class QuickStartClusterItem extends DocumentDBClusterItem { * Root node "DocumentDB Local - Quick Start" (WI-6). Renders unconditionally * (even with zero saved connections — handled in ConnectionsBranchDataProvider). * - * - No managed instance → a rocket empty-state row that opens the Quick Start - * webview, plus a "Learn more…" link. + * - No managed instance → a single rocket empty-state row that opens the Quick + * Start webview (UX review #3). * - A managed instance → the inline cluster (Running, expand to browse) or a * state row (Stopped/Starting/Stopping/Missing/Error) carrying lifecycle menus. */ @@ -167,23 +167,17 @@ export class LocalQuickStartItem implements TreeElement, TreeElementWithContextV ]; } - // NotInstalled (no metadata) → empty-state rocket + learn more. + // NotInstalled (no metadata) → single empty-state action row (UX review #3: + // reworded to an explicit "Click here to…" call to action; the separate + // "Learn more…" row was dropped as an extra, rarely-used list item). const children: TreeElement[] = [ createGenericElementWithContext({ id: `${this.id}/start`, contextValue: 'treeItem_quickStartAction', - label: l10n.t('Quick Start — Install & try DocumentDB locally'), + label: l10n.t('Click here to install & try DocumentDB locally'), iconPath: new vscode.ThemeIcon('rocket'), commandId: 'vscode-documentdb.command.localQuickStart.open', }), - createGenericElementWithContext({ - id: `${this.id}/learnMore`, - contextValue: 'treeItem_quickStartLearnMore', - label: l10n.t('Learn more…'), - iconPath: new vscode.ThemeIcon('link-external'), - commandId: 'vscode.open', - commandArgs: [vscode.Uri.parse('https://github.com/microsoft/documentdb')], - }), ]; if (status.state === InstanceState.Error && status.errorMessage) { From 9efd5e5ffcc45e755a656ccb5c05efa0cb71db7a Mon Sep 17 00:00:00 2001 From: Tomasz Naumowicz Date: Thu, 9 Jul 2026 15:42:27 +0200 Subject: [PATCH 2/7] updated the structure of the ux review doc --- .../PRs/documentdb-quickstart/ux-review.md | 927 +++++++++--------- 1 file changed, 438 insertions(+), 489 deletions(-) diff --git a/docs/ai-and-plans/PRs/documentdb-quickstart/ux-review.md b/docs/ai-and-plans/PRs/documentdb-quickstart/ux-review.md index 8777edd3c..07984d6b0 100644 --- a/docs/ai-and-plans/PRs/documentdb-quickstart/ux-review.md +++ b/docs/ai-and-plans/PRs/documentdb-quickstart/ux-review.md @@ -1,532 +1,481 @@ -# DocumentDB Local Quick Start — UX Review Notes +# DocumentDB Local Quick Start — UX Review Pack + +> **Who this is for:** anyone about to do a hands-on UX review of the **Local Quick Start** feature +> (installing/running a local DocumentDB container from inside VS Code), or anyone reading back how +> the review unfolded. +> **What this is:** a single catch-up document that captures a round of runtime UX feedback on the +> feature, states what the code _actually does today_ (verified against the current branch), groups +> the findings by user journey, and — for each — offers a **suggestion** the team can react to. +> Most items are still open discovery notes; where a quick, low-risk fix was applied it is stamped. + +- **Feature area:** `src/services/localQuickStart/`, `src/commands/localQuickStart/`, + `src/tree/connections-view/LocalQuickStart/`, `src/webviews/documentdb/localQuickStart/` +- **Working branch:** `dev/tnaum/documentdb-quickstart-ux-review` +- **Related design docs:** [local-quickstart-v2.md](../../local-quickstart/local-quickstart-v2.md), + [local-quickstart-poc/](../local-quickstart-poc/), + [local-quickstart-multi-instance/](../local-quickstart-multi-instance/) +- **Scope of this doc:** the UX-facing surface (tree structure, wording, icons, the provisioning + webview, lifecycle actions, error recovery). Deeper backend/state-machine internals are only + discussed where they explain a user-visible symptom. +- **Review date:** 2026-07-09 + +## How this review was run + +This is a **runtime UX pass**: a person exercised the real feature (install → run → stop → delete → +break-it-externally) and dictated observations, and an AI assistant did the code-checking, root-cause +tracing, and write-up. The intent is that each finding is backed by the exact code path that produces +the behavior, so a later triage/implementation pass doesn't have to re-derive it. + +The sections below are organized **by user journey** (upgrade, first run, the setup webview, the +running instance, destructive actions, lifecycle robustness, possible additions) rather than by the +order the feedback happened to arrive. Each item leads with a one-line **Verdict**, then records the +**Observation** (what the reviewer saw), the **Finding** (what the code actually does and why), and a +**Suggestion** (a concrete, reactable recommendation — not a decision). Heavier design questions with +real trade-offs are pulled into [§13 Open ideas](#13-open-ideas--options-pros--cons) at the end. + +## Legend + +| Marker | Meaning | +| ------------------ | --------------------------------------------------------------------------- | +| ✅ **Fix applied** | A quick, low-risk change was made on this branch and verified | +| ⚠️ **Flag** | Confirmed gap or bug; needs a decision/fix in a follow-up | +| 💡 **Suggestion** | A design/wording recommendation for the team to react to | +| 🔍 **Answered** | A "how does this work?" question, answered from the code (no change needed) | -**Feature area:** `src/services/localQuickStart/`, `src/commands/localQuickStart/`, -`src/tree/connections-view/LocalQuickStart/`, `src/webviews/documentdb/localQuickStart/` -**Related design docs:** `docs/ai-and-plans/local-quickstart/local-quickstart-v2.md`, -`docs/ai-and-plans/PRs/local-quickstart-poc/`, `docs/ai-and-plans/PRs/local-quickstart-multi-instance/` -**Review date:** 2026-07-09 +--- + +## 1. The story in one paragraph + +Local Quick Start adds a **`DocumentDB Local - Quick Start`** root node to the Connections view. From +an empty state a user clicks one row to open a **setup webview**, which checks Docker, pulls the +image, creates + starts a container, waits for readiness, and seeds sample data — reporting progress +as a stage checklist. Once running, the instance appears **inline** in the tree as a browsable +cluster with a Quick-Start-specific context menu (Start/Stop/Restart/Delete/Copy/View Logs). On +upgrade, pre-existing local "emulator" connections are migrated once into a regular folder. This +review hammered the _presentation and robustness_ of that journey: how the migration folder is named +and sorted, how the empty-state rows read, how honest and stable the setup webview's header/progress +is, how the running instance's menu and icons compare to regular clusters, whether destructive +actions match the rest of the extension, and — the one hard bug — what happens when the container is +deleted **outside** VS Code. Most items are wording/consistency polish; one (§4 header) is a real +"stuck UI" bug; one (§9 external delete) is a silent-failure bug with a clear root cause. -## What this document is +--- -This is a running log of manager (Tomaz) runtime observations on the Local Quick Start feature, -each enhanced with code research: exact locations, why the current behavior happens, and related -context or prior decisions. **Nothing in this document has been fixed yet** — findings are -discovery notes to inform a later triage/implementation pass. Severity/priority calls and actual -fixes are deliberately deferred to that follow-up. +## 2. Upgrade & migration (existing users) + +### 2.1 Legacy emulator connections migrate to a folder that can hide in the sort order ⚠️ 🔍 + +**Observation:** Is the one-time migration of original "DocumentDB Local" emulator connections still +happening? What's the folder called? Sorting could place it somewhere unexpected and cause confusion. + +**Finding:** + +- 🔍 Yes, it's still active. The migration lives in + [legacyEmulatorMigration.ts](../../../../src/services/legacyEmulatorMigration.ts), invoked from + [ClustersExtension.ts](../../../../src/documentdb/ClustersExtension.ts) on every activation + (`migrateLegacyEmulatorConnections`), guarded by a `globalState` flag + (`documentdb.localQuickStart.legacyEmulatorMigration.completed`) so it runs once. +- 🔍 The destination folder is named **`Local Connections (Legacy)`** (`LEGACY_FOLDER_BASE_NAME`), + deterministic id `vscode-documentdb.legacyLocalConnectionsFolder`. If a user already has a root + folder with that exact name, it's suffixed `(2)`, `(3)`, … It is a **normal** renamable/movable + folder under the regular Clusters zone — not pinned or reserved. Emulators-zone sub-folders are + intentionally **flattened** into this single folder (documented scope cut, not a bug). The old + `Emulators` zone/`LocalEmulatorsItem` node is kept as a read-only rollback until migration + succeeds, then retired (`isLegacyEmulatorMigrationComplete()` gate in + [ConnectionsBranchDataProvider.ts](../../../../src/tree/connections-view/ConnectionsBranchDataProvider.ts)). +- ⚠️ **Why it can surprise:** root items are assembled in a fixed order (Quick Start node → legacy + emulator node if un-migrated → **all folders, alphabetically** → ungrouped connections → New + Connection). Folders sort with a single `localeCompare(..., { numeric: true })` pass with **no + special-casing** for the legacy folder, so it lands wherever "L…" falls among the user's other + folder names — easy to miss right after an upgrade, since nothing marks it as "your old stuff + moved here." No migration toast surfaced in this pass (a search for a notification tied to + `MIGRATION_COMPLETED_KEY` found none). + +💡 **Suggestion:** Make the destination discoverable rather than relying on alphabetical luck. Cheapest +first: (a) a **one-time toast** after a successful migration ("N local connections were moved to +_Local Connections (Legacy)_") with a "Reveal" action; optionally (b) a distinct folder **description +badge/icon** so it stands out; and/or (c) pin it to a fixed position among folders (first or last). +See [§13.1](#131-surfacing-the-legacy-migration-folder) for the options weighed. --- -## 1. Legacy emulator connections migration folder - -**Observation:** Is the one-time migration of original "DocumentDB Local" emulator connections to -a new folder still happening? What's the folder called? Sorting could place it somewhere -unexpected. - -**Code findings:** - -- The one-time migration lives in [src/services/legacyEmulatorMigration.ts](../../../../src/services/legacyEmulatorMigration.ts), - invoked from [src/documentdb/ClustersExtension.ts](../../../../src/documentdb/ClustersExtension.ts) - on every activation (`migrateLegacyEmulatorConnections`), guarded by a `globalState` flag - (`documentdb.localQuickStart.legacyEmulatorMigration.completed`) so it only runs once. Yes, it is - still active code — nothing has removed it. -- Destination folder name: **`Local Connections (Legacy)`** (`LEGACY_FOLDER_BASE_NAME`), with a - deterministic id (`vscode-documentdb.legacyLocalConnectionsFolder`) so retries reuse the same - folder instead of duplicating it. If a user already has a root folder with that exact name, the - migration suffixes it `(2)`, `(3)`, … (`uniqueLegacyFolderName()`). -- The folder is created directly under the **root of the regular Clusters/Connections zone** — it - is a normal, renamable, movable folder like any user-created one, not a pinned/reserved node. - Emulators-zone sub-folders are intentionally **flattened**: every migrated connection lands - directly in this one folder (documented as a deliberate scope cut, not a bug). -- The legacy `Emulators` storage zone (and its `LocalEmulatorsItem` tree node) is kept as a - read-only rollback path until migration succeeds, then the tree node is retired — see the - `isLegacyEmulatorMigrationComplete()` gate in - [ConnectionsBranchDataProvider.ts](../../../../src/tree/connections-view/ConnectionsBranchDataProvider.ts) (root items array). - -**Why sorting can surprise:** Root-level items are assembled in `ConnectionsBranchDataProvider.getRootItems()` -in this fixed order: `LocalQuickStartItem` → (legacy `LocalEmulatorsItem`, if migration not yet -complete) → **all cluster folders, alphabetically** → **all ungrouped connections, alphabetically** -→ `New Connection` placeholder. Folders are sorted with a single -`localeCompare(..., { numeric: true })` pass across _all_ root folders — there is no special-casing -that pins "Local Connections (Legacy)" to the top or bottom. So depending on the user's other -folder names, it can land anywhere in the alphabetical list (e.g. between "Azure" and "Backups"), -which is easy to miss on a first look since nothing visually marks it as "this is where your old -stuff went." - -**Open questions for later:** Should this folder be visually distinguished (icon, description -badge) or pinned to a fixed position (e.g., always first/last among folders) so users don't have to -hunt for it after upgrading? Should there be a one-time toast/notification pointing at it instead of -relying on the user noticing it in an alphabetical list? (The migration code comment doesn't -mention a toast — worth confirming whether one exists elsewhere; a search for a notification tied to -`MIGRATION_COMPLETED_KEY` didn't surface one in this pass.) +## 3. First run — the empty Quick Start node + +### 3.1 Empty-state tree rows: reword the action, drop "Learn more" ✅ + +**Observation:** The rows shown when there's no managed instance yet should be reworded — the action +row should read like "Click here to…" — and "Learn more" could be dropped from the list (it could +live in the node's context menu instead). + +**Finding:** Both rows are built in `LocalQuickStartItem.getChildren()` +([LocalQuickStartItem.ts](../../../../src/tree/connections-view/LocalQuickStart/LocalQuickStartItem.ts)) +for the `NotInstalled` state: an action row (rocket, `treeItem_quickStartAction`, opens the webview) +and a separate "Learn more…" row (`link-external`, `treeItem_quickStartLearnMore`, opens +`https://github.com/microsoft/documentdb`). Neither row has any `view/item/context` entry in +`package.json`, so there is **no existing context-menu** to move "Learn more" into — that would be new +plumbing. The action label/command is also reused by the `Missing` state (see §9), so wording should +stay consistent across both. + +✅ **Fix applied (2026-07-09):** Implemented the quick half of this directly, in `getChildren()`'s +`NotInstalled` branch: + +- Action row relabeled `'Quick Start — Install & try DocumentDB locally'` → + **`'Click here to install & try DocumentDB locally'`**. +- The `'Learn more…'` row (`treeItem_quickStartLearnMore`) was **removed** from the empty state. It is + **not** relocated to a context menu (deferred — no menu plumbing exists yet), so the external repo + link is no longer reachable from the tree until/unless it's re-added. + +💡 **Suggestion (remaining):** Decide whether "Learn more" comes back as a context-menu entry on the +parent `DocumentDB Local - Quick Start` node (a new `view/item/context` gated on +`treeItem_localQuickStart`), or is dropped for good in favor of the walkthrough/README. Low priority. --- -## 2. "Delete Container" confirmation doesn't match the standard delete flow - -**Observation:** The Delete Container modal is fine as a concept, but it should use the same delete -confirmation code path as other destructive actions (e.g., in Settings), and should not use -em dashes. - -**Code findings:** - -- Standard destructive commands — [removeConnection.ts](../../../../src/commands/removeConnection/removeConnection.ts), - [deleteCollection.ts](../../../../src/commands/deleteCollection/deleteCollection.ts), - [deleteDatabase.ts](../../../../src/commands/deleteDatabase/deleteDatabase.ts), - [ConfirmDeleteStep.ts](../../../../src/commands/connections-view/deleteFolder/ConfirmDeleteStep.ts) — - all call **`getConfirmationAsInSettings()`** from - [src/utils/dialogs/getConfirmation.ts](../../../../src/utils/dialogs/getConfirmation.ts). That - function reads the `ext.settingsKeys.confirmationStyle` VS Code setting and dispatches to one of - three styles: type-the-word, a number challenge, or a plain click-confirm — i.e. "confirm like in - settings" is a user-configurable, shared code path. -- `deleteQuickStartInstance()` in - [localQuickStartCommands.ts](../../../../src/commands/localQuickStart/localQuickStartCommands.ts) - instead calls **`getConfirmationWithClick()`** directly — the click-only style, unconditionally, - ignoring the user's configured `confirmationStyle`. It's the odd one out; the only other callers - of `getConfirmationWithClick` directly (bypassing the setting) are `hideIndex`/`unhideIndex` - (non-destructive, reversible operations) and two spots in `QueryInsightsAIService.ts` — none of - which are "delete data permanently" operations like this one. -- Em dashes: the two confirmation `detail` strings in `deleteQuickStartInstance` both contain a - literal em dash — `'…This cannot be undone — you can recreate a fresh instance…'` — plus one in a - code comment above it. Same character also appears in several other user-facing strings in this - feature (e.g., `LocalQuickStart.tsx` next-steps bullets: `'Open Connection — browse your -databases…'`, `'Copy Connection String — use it from…'`, and the "waiting" timeout message - `'…may still be initializing — keep waiting, view the logs, or start over.'`). Worth a sweep of - the whole feature, not just the delete dialog, if em dashes are to be eliminated consistently. - -**Open questions for later:** Should Delete Container switch to `getConfirmationAsInSettings` (word -confirmation would ask the user to type something — worth deciding what word, since there's no -existing "type container name" concept for Quick Start)? Should the whole feature's strings be -swept for em dashes/other punctuation-style consistency in one pass rather than piecemeal? +## 4. The setup webview — header stability + +### 4.1 The webview header changes with state and gets stuck on "Setting up…" ⚠️ + +**Observation:** The header should be static. Today it changes with status/progress/errors, which is +unexpected — and it sometimes stays on "Setting up DocumentDB Local…" even when setup is actually +complete. + +**Finding** (in [LocalQuickStart.tsx](../../../../src/webviews/documentdb/localQuickStart/LocalQuickStart.tsx)): +the header ("hero") is rendered by a local `hero(title, subtitle)` helper called with **three +different literal titles** depending on `phase`: `'Docker is required'` (dockerNotReady), +`'Setting up DocumentDB Local…'` (**provisioning, success, _and_ failed all share this one call**), +and `'Start DocumentDB Local'` (review). Both halves of the observation are confirmed: + +- ⚠️ The title genuinely changes across phases (three strings), reading as a jumpy header. +- ⚠️ **The "stuck" complaint is a real bug:** `success` and `failed` reuse the `provisioning` title + verbatim, so "Setting up DocumentDB Local…" is still shown even once the instance is running or has + definitively failed. Only the body below (success/error box, stage checklist, buttons) reflects the + outcome — the header never catches up. (A phase/stage-aware `provisioningStatusMessage` exists but + is only piped to a screen-reader-only live region, not shown visually.) + +💡 **Suggestion:** Make the header a **single static string** for the whole panel lifecycle (e.g. +"DocumentDB Local" or "Local Quick Start"), and let the subtitle/body carry all state (elapsed timer, +success/failure boxes already exist). That satisfies "header should be static" and eliminates the +stuck-text bug in one move. If a dynamic header is preferred, it must at minimum branch on +`success`/`failed` the way the `dockerNotReady` screen already does. --- -## 3. Empty Quick Start list — tree item wording +## 5. The setup webview — the "waiting" stage -**Observation:** The tree items shown when there's no managed instance yet should be reworded. The -action row should say something like "Click here to…". "Learn more" could be dropped from the list -and instead live in the context menu of the local node. +### 5.1 "Waiting for DocumentDB to accept connections" can sit static for minutes ⚠️ -**Code findings:** Both rows are built in `LocalQuickStartItem.getChildren()` -([LocalQuickStartItem.ts](../../../../src/tree/connections-view/LocalQuickStart/LocalQuickStartItem.ts)) -for the `NotInstalled` state (no metadata yet): - -- Action row: label `l10n.t('Quick Start — Install & try DocumentDB locally')`, rocket icon, - `contextValue: 'treeItem_quickStartAction'`, bound to command - `vscode-documentdb.command.localQuickStart.open`. -- Learn-more row: label `l10n.t('Learn more…')`, `link-external` icon, - `contextValue: 'treeItem_quickStartLearnMore'`, bound to `vscode.open` with a hardcoded URL to - `https://github.com/microsoft/documentdb`. -- Also note: this same action label/icon/command (`treeItem_quickStartAction` / - `localQuickStart.open`) is reused for the `Missing` state's alternate rendering path (see finding - #12 below) — any rewording of the "click to do X" phrasing should stay consistent across both - surfaces. -- Neither row currently has **any** `view/item/context` entry in `package.json` — a search for - `treeItem_localQuickStart`, `treeItem_quickStartAction`, or `treeItem_quickStartLearnMore` in - `package.json` returns no matches. So today "Learn more" only exists as its own list row; there is - no existing context-menu wiring to move it into, moving it to the parent - (`treeItem_localQuickStart`) node's context menu would be new plumbing, not a small toggle. - -**Open questions for later:** Confirm exact copy for the action row (e.g. "Click here to install -DocumentDB locally with Quick Start" — needs to stay short for a tree row). Decide whether "Learn -more" moves to the parent `DocumentDB Local - Quick Start` node's context menu (new `view/item/context` -entry gated on `treeItem_localQuickStart`) or is dropped from the tree entirely in favor of the -walkthrough/README. - -**Fix applied (2026-07-09):** Implemented the quick, low-risk half of this finding directly (the -"move Learn more into the parent context menu" question above is still open/deferred — this only -covers the reword + drop): - -- Action row label changed from `'Quick Start — Install & try DocumentDB locally'` to - `'Click here to install & try DocumentDB locally'`. -- The separate `'Learn more…'` row (`contextValue: 'treeItem_quickStartLearnMore'`) was removed - entirely from the `NotInstalled` empty state — it is not (yet) relocated to a context menu; that - part of the original ask is left as an open item above, since no `view/item/context` plumbing - exists for `treeItem_localQuickStart` today (would be new work, not a move). -- Both changes are in - [LocalQuickStartItem.ts](../../../../src/tree/connections-view/LocalQuickStart/LocalQuickStartItem.ts), - `getChildren()`'s `NotInstalled` branch. The `Missing` state's row (finding #11) still reuses the - `treeItem_quickStartAction` contextValue/command but keeps its own distinct label - (`'Missing · click to recreate'`) — unaffected by this change. -- Not done as part of this quick fix: the `treeItem_quickStartLearnMore` contextValue is no longer - referenced anywhere, and the external link (`https://github.com/microsoft/documentdb`) is no - longer reachable from the tree at all until/unless it's re-added via a context menu. +**Observation:** This message could be reworded, or show some intermediate progress as sub-info, since +it can take a while. + +**Finding:** The stage label is `'Waiting for DocumentDB to accept connections'`. It wraps +`waitForReadiness()` in [QuickStartService.ts](../../../../src/services/localQuickStart/QuickStartService.ts), +which polls the wire protocol with a **`READINESS_TIMEOUT_MS = 180_000` (3 min)** budget and a 3 s +per-attempt timeout. During that window the stage row shows only a spinner + the same static label — +⚠️ **no in-panel sub-status** (attempt count, elapsed-in-stage, remaining). The information _does_ +exist: logs stream to the "DocumentDB Local Quick Start" output channel via `followLogs` the whole +time, just not surfaced in the panel. On timeout a distinct `ReadinessTimeoutError` (kept separate so +the container is preserved for "Wait longer") shows the `failed`/`timedOut` message. (That message and +several others here contain em dashes — see §8.1.) + +💡 **Suggestion:** Add a lightweight **sub-info line** under this stage while it's active — cheapest is +an **in-stage elapsed timer** ("still initializing… 0:45") and/or a rotating reassurance after N +seconds ("first run generates TLS certs, this can take a minute"). Also consider surfacing the "View +Docker output" link _during_ this stage (it currently only appears at the bottom of the panel). A +reword should stay accurate for both a cold first run and a plain restart, since both funnel through +this stage. --- -## 4. Webview header changes with state and can get stuck - -**Observation:** The header should be static; today it changes depending on status/progress/errors, -which is unexpected. It sometimes gets stuck on "Setting up DocumentDB Local…" even when the -operation is actually complete. - -**Code findings** (all in -[LocalQuickStart.tsx](../../../../src/webviews/documentdb/localQuickStart/LocalQuickStart.tsx)): - -- The header ("hero") is rendered by a local `hero(title, subtitle)` helper, called **three - separate times** with different literal titles depending on `phase`: - - `phase === 'dockerNotReady'` → `l10n.t('Docker is required')` - - `phase === 'provisioning' || phase === 'success' || phase === 'failed'` → **the same call**, - `l10n.t('Setting up DocumentDB Local…')`, with the subtitle showing the elapsed timer only - while `phase === 'provisioning'` (empty string otherwise) - - `phase === 'review'` (the initial screen) → `l10n.t('Start DocumentDB Local')` -- This confirms both parts of the observation exactly: - 1. The header text does change across phases (three different literal strings), which reads as - inconsistent/jumping if a user watches the panel through a full run. - 2. **The "stuck" complaint is a real bug, not perception**: `success` and `failed` reuse the exact - same title as `provisioning` — "Setting up DocumentDB Local…" is still shown even once the - instance is fully running (`phase === 'success'`) or once setup has definitively failed - (`phase === 'failed'`). Only the content _below_ the header (the `successBox`/`errorBox`, the - stage checklist, and the action buttons) reflects the actual outcome — the header itself never - catches up. -- There is a `provisioningStatusMessage` computed a few lines above (derived from the active stage - label) that IS phase/stage-aware and correctly blanks itself out once any stage errors — but it's - only surfaced to screen readers via a visually-hidden `aria-live` region, not shown visually. - -**Open questions for later:** Should the header become a single static string for the whole -provisioning/success/failed lifecycle (e.g. "DocumentDB Local"), with all state communicated via -subtitle/body instead? Or should it explicitly branch on `success`/`failed` like the review/error -screens already do for `dockerNotReady`? +## 6. The setup webview — image pull progress + +### 6.1 No structured progress while the image downloads ⚠️ + +**Observation:** Do we track progress while the image is pulled? It's a long-running op too. + +**Finding:** ⚠️ No. `provision()` yields one `active` event (`'Pulling the official image…'`), awaits +`pullImage()`, then one `done` event — nothing in between. `ContainerRuntime.pullImage()` +([ContainerRuntime.ts](../../../../src/services/localQuickStart/ContainerRuntime.ts)) runs `docker +pull` through the shared CLI shell-runner, piping stdout/stderr **only** to the output channel +(masked, line-buffered). Docker's per-layer progress (`Downloading`/`Extracting`/`Pull complete`) is +therefore visible only as raw channel text, never in the webview — the panel shows a spinner + +static label for the entire pull, same as §5. + +💡 **Suggestion:** Give the pull an **indeterminate but alive** treatment. Since a true 0–100% isn't +known up front without a bigger rework, pair a Fluent UI **indeterminate `ProgressBar`** with a small +live **"N of M layers"** sub-label (derivable by counting distinct layer ids vs. `Pull complete` +lines). This needs modest plumbing (a progress callback on `pullImage`/`IContainerRuntime`, extra +`StageEvent` fields, webview rendering). Full options, including a Docker-Engine-API/`dockerode` +route for real byte totals and why a literal "dot per layer" reads as jittery, are in +[§13.2](#132-image-pull-progress-indicator). --- -## 5. "Waiting for DocumentDB to accept connections" wording / intermediate progress - -**Observation:** This message could be reworded, or show some intermediate progress as a sub-info, -since it can take time. - -**Code findings:** - -- Stage label: `STAGE_LABELS.waiting = l10n.t('Waiting for DocumentDB to accept connections')` in - `LocalQuickStart.tsx`. The service-side stage message is - `'Waiting for DocumentDB to accept connections…'` in - [QuickStartService.ts](../../../../src/services/localQuickStart/QuickStartService.ts) (`provision()`, - `--- waiting ---` section). -- This stage wraps `waitForReadiness()`, which polls the wire protocol (a real MongoClient - connection attempt) with a `READINESS_TIMEOUT_MS = 180_000` (3 minutes) budget and a - `PROBE_SERVER_SELECTION_TIMEOUT_MS = 3_000` per attempt — i.e. it can legitimately retry for up to - 3 minutes with no visible change other than the elapsed-time counter in the (currently static, - see #4) header subtitle. -- There is no intermediate sub-status during this stage today — the stage row just shows a spinner - - the same static label the whole time; the per-attempt retry count / time remaining isn't - surfaced. Logs _do_ stream to the "DocumentDB Local Quick Start" output channel during this window - (via `followLogs`, started right after `docker start`), so the information exists, just not - in-panel. -- On timeout, a distinct `ReadinessTimeoutError` is raised (kept separate from hard failures - specifically so the container is preserved and "Wait longer" can resume it) — the `failed` phase - with `timedOut === true` shows a dedicated message: `'The container is running, but DocumentDB has -not accepted connections yet. It may still be initializing — keep waiting, view the logs, or start -over.'` (contains another em dash — see #2). - -**Open questions for later:** What sub-info would be useful here — elapsed time within the stage, -attempt count, a rotating "still working" hint after N seconds, or a link to "View Docker output" -surfaced earlier instead of only at the bottom of the panel? Any reword should stay accurate for -both a fresh pull-to-ready cold start and a plain restart (both funnel through the same stage). +## 7. The running instance in the tree + +### 7.1 The running-instance row doesn't share regular cluster context commands 🔍 + +**Observation:** The running managed instance doesn't share context-menu commands with regular +clusters. Do we still extend the cluster base class? If not, what would break if we did? + +**Finding:** + +- 🔍 Yes — `QuickStartClusterItem extends DocumentDBClusterItem` + ([LocalQuickStartItem.ts](../../../../src/tree/connections-view/LocalQuickStart/LocalQuickStartItem.ts)). + It overrides `getTreeItem()` only to force the state-aware description, and reuses the base for + icon/tooltip/browsing. +- 🔍 The reason regular commands don't appear: the **contextValue is fully replaced, not extended.** + The constructor sets `contextValue = createContextValue([INSTANCE_CONTEXT, stateToken])` — i.e. + `treeItem_quickStartInstance` + `state_running` — discarding the base's `treeitem_documentdbcluster` + token that every regular-cluster `view/item/context` `when`-clause matches on. This is deliberate + (class comment: show Quick Start lifecycle actions "instead of the generic cluster menus"). +- 🔍 **What would break if the base contextValue were kept too:** the generic commands assume a + **stored** connection — `removeConnection` deletes a storage record, `moveItems`/`renameConnection` + hit `ConnectionStorageService`, `update*` open storage wizards. The Quick Start instance is + explicitly **in-memory only** (not in any storage zone), so those commands would act on a record + that doesn't exist — silent no-ops at best, orphaned-state/exceptions at worst — unless each learned + to special-case this row. So full replacement avoids a bug class, but also throws out genuinely + useful storage-independent commands (e.g. "Open in Shell"). + +💡 **Suggestion:** Keep the curated menu, but **add back the storage-independent commands explicitly** +(most valuable: "Open in Shell" / "New Query" against the running instance). The clean version of +this is splitting the base contextValue into a storage-dependent subset and a connection-only subset +so Quick Start can opt into the latter — weighed in [§13.3](#133-cluster-commands-on-the-quick-start-row). + +### 7.2 "Copy Connection String" is a separate impl and silently includes the password ⚠️ + +**Observation:** Copy Connection String seems reimplemented for Quick Start and behaves slightly +differently — it doesn't ask whether to include the password. + +**Finding:** Regular clusters use `copyConnectionString()` +([copyConnectionString.ts](../../../../src/commands/copyConnectionString/copyConnectionString.ts)), +which prompts via `showQuickPick` ("with password" vs "without password", gated by +`canIncludeNativePassword()`) and is well-tested. Quick Start uses a **separate, shorter** +`copyQuickStartConnectionString()` +([localQuickStartCommands.ts](../../../../src/commands/localQuickStart/localQuickStartCommands.ts)) +that copies `metadata.connectionString` (password already embedded) **with no prompt** — a genuine +behavioral difference, not just styling. ⚠️ A plaintext password always lands on the clipboard +silently, which stands out given how careful the rest of the feature is about credentials (masked +logging, env-file instead of CLI args). There is a _separate_ `copyQuickStartPassword()` command, but +the connection-string action itself never asks. + +💡 **Suggestion:** Reuse the regular command's QuickPick confirmation for parity (Quick Start always +uses native auth, so no extra branching is needed), or — if the extra click is unwanted for a +one-click-local flow — at minimum make the two commands' behavior a **conscious, documented** choice +rather than an incidental divergence. + +### 7.3 Icons are all inherited/generic — nothing marks "this is the Quick Start instance" 🔍💡 + +**Observation:** How are the icons chosen for the Quick Start instances? + +**Finding:** 🔍 There's no bespoke icon set: + +- **Root node:** the extension's product icon (`vscode-documentdb-icon-*.svg`). +- **Running row:** no override — inherits `DocumentDBClusterItem`'s logic, which picks `$(plug)` when + `emulatorConfiguration.isEmulator` is true (always, for Quick Start) else `$(server-environment)`. + So the running instance gets **the same `$(plug)`** as any other connection flagged as an emulator + (including the §2.1 migrated legacy connections). Nothing visually distinguishes "the managed + Quick Start instance." +- **State rows** (Starting/Stopping/Stopped/Error/Missing): generic `ThemeIcon`s (`loading~spin`, + `circle-outline`, `warning` with theme colors). +- **Empty-state rows:** `rocket` and `link-external`. + +💡 **Suggestion:** Give the managed instance a **distinct, state-independent identity** — e.g. reuse +the **rocket** (already the feature's motif on the root/action rows) for the instance row, or a +dedicated brand mark, so users can tell "the Quick-Start-managed one" apart from a manually-flagged +emulator at a glance. Low priority, but cheap. --- -## 6. Image pull/download progress tracking - -**Observation:** Do we track progress while the image is downloaded? That's a long-running -operation too. - -**Code findings:** - -- `provision()` yields exactly one `active` `StageEvent` for the pulling stage — `stageEvent('pulling', -'active', 'Pulling the official image…')` — then `await this.runtime.pullImage(imageRef, -cts.token)`, then one `done` event. No intermediate events are yielded between those two. -- `ContainerRuntime.pullImage()` ([ContainerRuntime.ts](../../../../src/services/localQuickStart/ContainerRuntime.ts)) - runs `docker pull` through the shared CLI shell-runner (`@microsoft/vscode-container-client`'s - `DockerClient`), with `stdout`/`stderr` piped only to the output channel via - `MaskedChannelWritable` (line-buffered + secret-masked). Nothing from that stream is captured, - parsed, or forwarded back to the caller — so per-layer Docker progress (`Downloading`, - `Verifying Checksum`, `Extracting`, `Pull complete`, etc., one line per layer per event when not - attached to a TTY) is visible **only** in the raw output channel text, never in the webview. -- So: no, there is no structured progress tracking for the pull today, confirming the observation. - The webview shows a spinner + static label for however long the pull takes, same as "waiting" (#5). - -**Ideas for indeterminate/step progress (no fix applied, just candidates to discuss):** - -- Docker's plain-text pull output (non-TTY) already gives one line per layer per status - transition, including a `": Pull complete"` line once each layer finishes. Since the - number of layers isn't known until Docker starts announcing "Pulling fs layer" lines, a - precise percentage isn't available up front, but a **running count of "N of M layers pulled"** - is derivable simply by tracking, per pull, how many distinct layer ids have been seen so far - (M, growing) vs. how many have reached `Pull complete` (N) — that only requires - `ContainerRuntime.pullImage` to accept an optional line/progress callback instead of writing - straight to the channel, and forwarding parsed counts as extra `StageEvent` fields (mirroring how - `boundPort`/`timedOut` already ride along on `StageEvent` for other stages). - - **Caveat:** because non-TTY docker pull is a plain scrolling text stream, not a JSON event - stream, layer counting means resurrecting a small regex parser over free-form CLI text. It'd be - more robust (and would expose _actual_ byte totals per layer, since Docker does know each - layer's compressed size) to switch this specific call to the Docker Engine API's pull endpoint - directly (e.g. via `dockerode`'s `docker.pull()`, which streams structured JSON progress events - with `id`, `status`, and `progressDetail: { current, total }` per layer) rather than parsing CLI - text — a larger change than the current CLI-wrapper approach, worth flagging as a design - decision rather than a quick tweak. -- On the visual side, since a true 0–100% isn't known in advance without that JSON-stream rework, - an **indeterminate `ProgressBar`** (Fluent UI v9 `ProgressBar` with no `value` prop renders as an - animated indeterminate bar) reads better than a static spinner for a stage that can run tens of - seconds, and is already a component this design system ships — no new dependency. Layered under - it, a small live sub-label like "Downloading… (3 of 6 layers)" would satisfy the "some kind of - progress" ask without needing byte-accurate totals — that count is the cheap parse described - above, not the JSON-stream rework. -- A literal "dot per completed layer" (e.g. a row of small filled/empty dots) is visually appealing - for a small, bounded number of steps, but is a poor fit here specifically because the total layer - count isn't known until the pull starts announcing them — the dot row would have to grow/shrink - as new layers appear, which reads as jittery. The numeric "N of M" counter (or an indeterminate - bar with that counter as a sub-label) avoids that problem while still being honest about what we - actually know. -- Either option is a genuine, if modest, plumbing change (new callback param on `pullImage` / - `IContainerRuntime`, a new `StageEvent` shape, and webview rendering), not a copy tweak — worth - scoping separately from the wording-only findings above. +## 8. Destructive actions + +### 8.1 "Delete Container" bypasses the shared confirmation and uses em dashes ⚠️ + +**Observation:** The Delete Container modal is fine conceptually, but it should use the same delete +confirmation code path as other destructive actions (like in Settings), and shouldn't use em dashes. + +**Finding:** + +- ⚠️ Every other destructive command — [removeConnection](../../../../src/commands/removeConnection/removeConnection.ts), + [deleteCollection](../../../../src/commands/deleteCollection/deleteCollection.ts), + [deleteDatabase](../../../../src/commands/deleteDatabase/deleteDatabase.ts), + [folder delete](../../../../src/commands/connections-view/deleteFolder/ConfirmDeleteStep.ts) — calls + **`getConfirmationAsInSettings()`** ([getConfirmation.ts](../../../../src/utils/dialogs/getConfirmation.ts)), + which honors the user's `confirmationStyle` setting (type-the-word / number challenge / click). + `deleteQuickStartInstance()` instead calls **`getConfirmationWithClick()`** directly, ignoring that + setting — it's the only _destructive_ command that does (the other direct callers are + non-destructive: `hideIndex`/`unhideIndex`). +- ⚠️ Its two confirmation `detail` strings both contain a literal **em dash** ("…This cannot be undone + — you can recreate…"). The character also appears across the feature's other strings + (`LocalQuickStart.tsx` next-steps bullets, the §5 timeout message). + +💡 **Suggestion:** Switch Delete Container to **`getConfirmationAsInSettings()`** for consistency (decide +the confirmation word — there's no "type the container name" concept yet, so a simple word like +`delete` matches the other delete flows). Separately, do a **one-pass em-dash sweep** of the whole +feature's user-facing strings rather than fixing only the delete dialog. --- -## 7. Running-container tree item doesn't share cluster context commands - -**Observation:** The tree item for a running managed instance doesn't share context menu commands -with regular clusters. Do we still extend the cluster item base class? If not, what would break if -we did? - -**Code findings:** - -- Yes, it still extends the base class: `QuickStartClusterItem extends DocumentDBClusterItem` in - [LocalQuickStartItem.ts](../../../../src/tree/connections-view/LocalQuickStart/LocalQuickStartItem.ts). - It overrides `getTreeItem()` only to force the state-aware description text, and reuses the base - class for icon, tooltip, and (critically) all the actual cluster-browsing behavior — the comment - on the class explicitly says browsing "reuses the base `DocumentDBClusterItem`." -- The reason regular cluster context commands don't show up: the **contextValue is fully replaced**, - not extended. The constructor sets - `this.contextValue = createContextValue([INSTANCE_CONTEXT, stateToken])` — i.e. - `treeItem_quickStartInstance` + `state_running` (etc.) — which **discards** whatever contextValue - the base `DocumentDBClusterItem` constructor/`getTreeItem()` would normally compute (which - includes the `treeitem_documentdbcluster` token). Every regular-cluster `view/item/context` entry - in `package.json` gates on `viewItem =~ /\btreeitem_documentdbcluster\b/i` (e.g. - `updateConnectionString`, `updateCredentials`, `renameConnection`, `moveItems`, - `removeConnection`) — none of those `when` clauses match a Quick Start row, since that token is - never present on it. -- This looks deliberate, not an oversight: the class-level comment says the Quick Start node - "carr[ies] a Quick-Start-specific context value so the instance shows Quick Start lifecycle - actions **instead of** the generic cluster menus." The `INSTANCE_CONTEXT` token instead gates the - Quick Start–specific commands (`start`/`stop`/`restart`/`delete`/`copyConnectionString`/ - `copyPassword`/`viewLogs`), each `when`-clause requiring both `treeItem_quickStartInstance` and a - specific `state_*` token (see `package.json`, `view/item/context` for - `vscode-documentdb.command.localQuickStart.*`). -- **What would likely break if the base contextValue were also kept (appended rather than - replaced):** the generic cluster commands assume storage-backed operations — `removeConnection` - deletes a _stored connection record_, `moveItems`/`renameConnection` operate against - `ConnectionStorageService`, `updateConnectionString`/`updateCredentials` open storage-editing - wizards. The Quick Start instance is explicitly **not** stored in any zone (the model comment says - "the Quick Start managed instance is in-memory (CredentialCache-based) and not stored in any - zone"). Enabling those commands as-is would let a user try to "rename"/"move to folder"/"delete - connection" an item that has no storage record to act on — likely a silent no-op at best, or an - exception/orphaned-state at worst, unless each of those commands were also taught to special-case - the Quick Start row. So the current full-replacement approach avoids a class of bugs, but at the - cost of losing genuinely useful, storage-independent commands too, e.g. "Open in Shell" / "New - Query" style commands if any are gated only on `treeitem_documentdbcluster` without also touching - storage — worth an inventory of exactly which cluster commands are storage-dependent vs. purely - connection-based, since only the latter could be safely re-enabled. - -**Open questions for later:** Would it be worth splitting the base contextValue into a -storage-dependent subset and a connection-only subset, so Quick Start could opt into the latter? Or -is a curated, Quick-Start-specific menu (as today) actually the right long-term shape, just missing -a few commands (e.g., "Open in Shell") that should be added explicitly rather than inherited? +## 9. Lifecycle robustness & error recovery + +### 9.1 A container deleted _outside_ VS Code isn't handled on Start (silent no-op) ⚠️ + +**Observation (repro):** Created an instance via Quick Start, stopped it, then deleted the _container_ +directly via Docker. Back in VS Code, clicking Start on the still-"Stopped"-looking row does nothing +visible — no dialog, no tree update — only the raw Docker error appears in the output channel: + +``` +$ docker container inspect --format '{{json .}}' 57f8311fd9ec… +Error response from daemon: No such container: 57f8311fd9ec… +``` + +**Finding — a specific, reproducible gap:** + +- `ContainerRuntime.inspectContainer()` wraps the CLI call in `try { … } catch { return undefined; }`, + swallowing "No such container" into `undefined`. But the shared `makeRunner()` still logs the + command and streams the daemon's stderr to the channel — that's exactly the two lines the user saw + (the extension's own inspect call, not a manual terminal session). +- `start()` calls `isManaged(id)` first, which returns `false` both when the container **isn't ours** + _and_ when it's **gone** (`inspectContainer` → `undefined`). ⚠️ **`isManaged()` cannot distinguish + "missing" from "not ours."** +- `start()`'s guard is `if (!id || !isManaged || !liveStateGuard(…)) return;`. Because `isManaged` + already returned `false`, the `||` short-circuits and **`liveStateGuard()` never runs** — which is + the very code that _would_ have handled this gracefully (it detects a `'missing'` live state, calls + `refreshLiveState()`, and shows an info message). That path is well-built for the multi-window / + external-stop case; it's just unreachable here. +- Net effect: `start()` early-returns silently, `setStatus` never fires, the tree never refreshes, and + the stale "Stopped" row persists until something else (e.g. collapsing/expanding the node) triggers + `refreshLiveState()` independently. +- Note there's already a good pattern for the related case: `refreshLiveState()` _does_ set + `entry.missing = true` and the tree already renders `Missing · click to recreate`. The gap is that + the **lifecycle actions** (`start`/`stop`/`restart`) don't funnel through that missing-detection + before giving up. + +💡 **Suggestion:** Teach `start`/`stop`/`restart` to distinguish "no container found" from "found but +not ours" (a small change to `isManaged`, or a check ahead of it) and, on "missing," do what +`liveStateGuard` already does for the multi-window case — refresh state to the `Missing` badge and +surface a message with next steps: **Delete** (clean up stale metadata/volume) and/or **Recreate with +same settings** (the existing `Missing` row's recreate flow + `getReusableCredentials()` in +`provision()` likely already covers most of this — worth confirming it's sufficient once reachable +from the lifecycle actions). Full UX options in [§13.4](#134-external-deletemissing-recovery-affordance). --- -## 8. Copy Connection String is a separate, slightly different implementation - -**Observation:** Copy Connection String appears to be reimplemented for Quick Start and behaves -slightly differently — it doesn't ask whether to include the password. - -**Code findings:** - -- Regular clusters: `copyConnectionString()` in - [copyConnectionString.ts](../../../../src/commands/copyConnectionString/copyConnectionString.ts) - calls `context.ui.showQuickPick` with two options — "The connection string will not include the - password" vs. "...will include the password" — gated by `canIncludeNativePassword()`, only - prompting when native-auth credentials are actually present. Well tested (see - `copyConnectionString.test.ts`, e.g. "picks WITH/WITHOUT password" cases). -- Quick Start: `copyQuickStartConnectionString()` in - [localQuickStartCommands.ts](../../../../src/commands/localQuickStart/localQuickStartCommands.ts) - is a completely separate, much shorter function — it reads `metadata.connectionString` (which - already has the generated username/password embedded from `composeConnectionString()`) and copies - it directly to the clipboard with no prompt at all. There's a **separate** `copyQuickStartPassword()` - command for copying just the password, but the "copy connection string" action always includes the - password silently. -- This is a genuine behavioral difference, not just a styling one: for a regular cluster, a user is - asked each time whether the clipboard should carry the plaintext password; for the Quick Start - instance, the password always goes to the clipboard silently whenever "Copy Connection String" is - used. Given Quick Start's own design docs are otherwise careful about credential handling (masked - output-channel logging, env-file instead of CLI args, etc. — see `outputMasking.ts` and the - `D14`/masking comments throughout `QuickStartService.ts`), this asymmetry stands out. - -**Open questions for later:** Should Quick Start's "Copy Connection String" reuse the same -QuickPick-based confirmation as the regular command (auto-answering "Quick Start always uses native -auth" without extra branching), for parity and because it's also copying a plaintext password to the -clipboard? +## 10. Possible additions + +### 10.1 "Open terminal in the container" — feasible, no blocker 🔍💡 + +**Observation:** Would a command to open a terminal inside the Docker container make sense? Can we do +it? + +**Finding:** 🔍 Feasible, no blocker. + +- The repo already uses `vscode.window.createTerminal(...)` for terminal-backed commands + ([openInteractiveShell.ts](../../../../src/commands/openInteractiveShell/openInteractiveShell.ts)), + though that uses a custom `Pseudoterminal` for the integrated shell — more than needed here. +- For a plain "shell into the container," the standard pattern is + `createTerminal({ name, shellPath: 'docker', shellArgs: ['exec', '-it', containerId, '/bin/bash'] })` + (with a `/bin/sh` fallback) — Docker drives the TTY, no custom pty required. +- The container id is already tracked (`metadata.containerId`, via `getStatus()`), so no new state is + needed. `ContainerRuntime.execShellInContainer()` exists but is non-interactive (channel output), + so a new small command calling `createTerminal` directly is the better fit. Docker is already a hard + dependency, so no new prerequisite. + +💡 **Suggestion:** Add an **"Open in Terminal"** context-menu command gated on `state_running` (a +stopped container can't `exec`), trying `bash` then falling back to `sh`. Small, self-contained, and a +natural power-user affordance. Open details (menu group/icon, shell fallback prompt) in +[§13.5](#135-open-terminal-in-container). --- -## 9. "Open terminal in the Docker container" — feasibility - -**Observation:** Would it make sense to add a command to open a terminal inside the Docker -container? Can this be done? - -**Code findings / feasibility:** - -- The repo already has precedent for terminal-backed commands: - [openInteractiveShell.ts](../../../../src/commands/openInteractiveShell/openInteractiveShell.ts) - calls `vscode.window.createTerminal({ name, pty, iconPath })` with a custom - `DocumentDBShellPty` (`vscode.Pseudoterminal`) for the integrated mongosh-style shell — that's a - more elaborate mechanism (a custom pty implementation) built for a different purpose (parsing/ - relaying shell I/O), not required here. -- For a plain "shell into the container" command, the much simpler standard VS Code pattern is - `vscode.window.createTerminal({ name, shellPath: 'docker', shellArgs: ['exec', '-it', containerId, -'/bin/bash'] })` (with a `/bin/sh` fallback if `bash` isn't present in the image) — this is exactly - how other Docker-focused VS Code extensions expose "Attach Shell"/"Open in Terminal". No custom - pty is needed since Docker itself drives the interactive TTY. -- The container id is already tracked in-memory (`InstanceRuntimeState.metadata.containerId`) and - exposed via `QuickStartService.getStatus().metadata`, so the command would have everything it - needs without new state plumbing. `ContainerRuntime` already has `execShellInContainer()`, but - that's a fire-and-forget non-interactive `docker exec` used for the sample-data init script — it - writes to the output channel, not a live TTY, so it isn't directly reusable for an interactive - terminal; a new, simple command function (not a `ContainerRuntime` method) would be the more - natural fit, calling `createTerminal` directly the way `openInteractiveShell` does. -- No fundamental blocker found: Docker is already a hard dependency for this whole feature, so - requiring `docker` on PATH for this command adds no new dependency. The main design questions are - which state(s) it should be enabled for (`state_running` only, presumably — a stopped container - can't `exec` into it) and what shell to try first. - -**Open questions for later:** Confirm which context-menu group/icon this would use, and whether it's -worth a fallback prompt if `bash` isn't in the image (the DocumentDB image's shell availability -wasn't checked in this pass). +## 11. Consolidated flags & suggestions (read this before testing) + +| § | Item | Verdict | Suggested next step | +| ---- | ----------------------------------------------- | ------------------------ | ----------------------------------------------------------------------------------------------------------------------- | +| 2.1 | Legacy migration folder discoverability | ⚠️ Flag | One-time toast + "Reveal"; optional distinct badge/pin ([§13.1](#131-surfacing-the-legacy-migration-folder)) | +| 3.1 | Empty-state wording / drop "Learn more" | ✅ Fix applied | Decide if "Learn more" returns as a context-menu entry | +| 4.1 | Webview header changes / stuck on "Setting up" | ⚠️ Flag (real bug) | Make the header a single static string; body carries state | +| 5.1 | "Waiting for connections" static for minutes | ⚠️ Flag | Add in-stage elapsed/sub-info; surface "View output" earlier | +| 6.1 | No image-pull progress | ⚠️ Flag | Indeterminate `ProgressBar` + "N of M layers" ([§13.2](#132-image-pull-progress-indicator)) | +| 7.1 | Running row lacks regular cluster commands | 🔍 Answered (deliberate) | Add back storage-independent commands explicitly ([§13.3](#133-cluster-commands-on-the-quick-start-row)) | +| 7.2 | Copy Connection String silently adds password | ⚠️ Flag | Reuse the regular QuickPick confirmation, or document the divergence | +| 7.3 | Generic `$(plug)` icon, no distinct identity | 🔍 Answered | Give the instance a distinct icon (reuse the rocket motif) | +| 8.1 | Delete bypasses shared confirmation + em dashes | ⚠️ Flag | Switch to `getConfirmationAsInSettings()`; sweep em dashes feature-wide | +| 9.1 | External container delete not handled on Start | ⚠️ Flag (real bug) | Distinguish missing vs not-ours; route to Missing + recovery ([§13.4](#134-external-deletemissing-recovery-affordance)) | +| 10.1 | "Open terminal in container" | 🔍 Feasible | Add `state_running` "Open in Terminal" command ([§13.5](#135-open-terminal-in-container)) | --- -## 10. How are the Quick Start tree icons chosen? - -**Observation:** How are the icons chosen for the Quick Start DocumentDB instances? - -**Code findings:** There's no bespoke Quick Start icon set — every icon is either the generic -extension logo or inherited/generic VS Code `ThemeIcon`s, reused from existing logic: - -- **Root node** (`DocumentDB Local - Quick Start`): the extension's own product icon — - `vscode-documentdb-icon-{light,dark}-themes.svg` from `getResourcesPath()` — same icon used - elsewhere for the extension/product identity, not specific to Quick Start. -- **Running instance row** (`QuickStartClusterItem`): no icon override at all — it falls through to - the inherited `DocumentDBClusterItem.getTreeItem()` icon logic, which picks - `$(plug)` **iff** `cluster.emulatorConfiguration?.isEmulator` is true, else `$(server-environment)`. - Since the Quick Start model always sets `emulatorConfiguration: { isEmulator: true, ... }` (see - `LocalQuickStartItem.getChildren()`), the running row always gets `$(plug)` — the exact same icon - any other connection flagged as an "emulator" gets (including the legacy migrated connections in - finding #1, which also carry `isEmulator: true`). There is nothing that visually distinguishes "the - one managed-by-Quick-Start instance" from "any other connection someone marked as an emulator." -- **Non-running state rows** (Starting/Stopping/Stopped/Error/Missing): plain generic VS Code - `ThemeIcon`s chosen per state in `LocalQuickStartItem.getChildren()` — - `loading~spin` (Starting/Stopping/Provisioning), `circle-outline` (Stopped), `warning` with - `list.errorForeground`/`list.warningForeground` theme color (Error/Missing). None of these relate - to cluster icons at all; they're the same generic vocabulary used for background-task rows - elsewhere in the extension. -- **Empty-state rows:** `rocket` (action) and `link-external` (Learn more) — again generic - `ThemeIcon`s, not custom art. - -**Open questions for later:** Is `$(plug)` (shared with any manually-flagged "emulator" connection) -an acceptable representation for the one managed local instance, or should Quick Start rows carry a -distinct icon (e.g. reusing the rocket used for the root node/action row) so the "this is the -Quick-Start-managed one" identity is visually obvious at a glance, independent of state? +## 12. Applied changes on this branch + +- **§3.1** — empty-state action row relabeled to "Click here to install & try DocumentDB locally"; the + "Learn more…" row removed. Files: [LocalQuickStartItem.ts](../../../../src/tree/connections-view/LocalQuickStart/LocalQuickStartItem.ts), + [l10n/bundle.l10n.json](../../../../l10n/bundle.l10n.json). Verified via `l10n` / `prettier` / `lint` + / full Jest suite / `build`. + +Everything else in this document is a discovery note or suggestion — no other code was changed. --- -## 11. Deleted-externally container isn't handled when the user tries "Start" +## 13. Open ideas — options, pros & cons -**Observation (repro):** Created an instance via Quick Start, stopped it, then deleted the -_container_ directly via Docker (outside VS Code). Back in VS Code, clicking Start on the (still -"Stopped"-looking) tree row does nothing visible — no error dialog, no tree update — only the raw -Docker error appears in the "DocumentDB Local Quick Start" output channel: +These are the genuinely open design questions with real trade-offs. Recommendations are suggestions +for the team to react to, not decisions. -``` -$ docker container inspect --format '{{json .}}' 57f8311fd9ec77aed5483b6ef36da22890f8a71e1057073f884273ba89c68248 -Error response from daemon: No such container: 57f8311fd9ec77aed5483b6ef36da22890f8a71e1057073f884273ba89c68248 -``` +### 13.1 Surfacing the legacy migration folder (§2.1) -**Code findings — this traces to a specific, reproducible gap, not vague flakiness:** - -- `ContainerRuntime.inspectContainer()` ([ContainerRuntime.ts](../../../../src/services/localQuickStart/ContainerRuntime.ts)) - wraps the CLI call in `try { ... } catch { return undefined; }` — it deliberately swallows _all_ - errors, including "No such container," into a plain `undefined` return. That's a reasonable - existence-check contract on its own. But every call is still run through the shared - `makeRunner()`, whose `onCommand` callback unconditionally logs `'$ ' + command` to the output - channel _before_ running, and whose `stdErrPipe` streams the daemon's stderr response into the - same channel regardless of whether the caller ends up treating the failure as "expected." That's - exactly the two lines the user saw — this is the extension's own inspect call being logged, not a - manual terminal session. -- `start()` (`QuickStartService.ts`) calls `this.isManaged(id, alias)` before doing anything else: - ```ts - private async isManaged(containerId, alias) { - const item = await this.runtime.inspectContainer(containerId); - if (!item || item.labels?.[QUICK_START_LABEL_KEY] !== '1') return false; - return this.aliasMatches(...); - } - ``` - When the container has been removed, `inspectContainer` returns `undefined` → `isManaged` returns - `false` — **the exact same `false` it would return for "this container exists but isn't ours."** - `isManaged()` cannot distinguish "gone" from "not managed by us." -- Back in `start()`: - ```ts - if (!id || !(await this.isManaged(id, alias)) || !(await this.liveStateGuard(id, ['stopped']))) { - return; - } - ``` - Because `isManaged` already returned `false`, the `||` short-circuits — **`liveStateGuard()` is - never even called** for this scenario. That matters because `liveStateGuard` is the piece of code - that _would_ have handled this gracefully: it explicitly checks for a `'missing'` live state, calls - `refreshLiveState()`, and shows `vscode.window.showInformationMessage(...)` telling the user the - instance changed in another window. That path exists and is well-built for _other_ window / - external-stop scenarios — it's just unreachable here because `isManaged` gates in front of it and - returns the same "false" for two different situations. -- Net effect: `start()`'s whole body becomes a silent early `return` — no `setStatus(...)` call, so - `statusEmitter` never fires, so `ConnectionsBranchDataProvider.refresh()` (wired in - `ClustersExtension.ts` via `QuickStartService.onDidChangeStatus`) never runs, so the tree keeps - showing the stale "Stopped" row indefinitely until something else (e.g. expanding/collapsing the - node, which re-triggers `LocalQuickStartItem.getChildren()` → `refreshLiveState()`) happens to - refresh it independently. -- Worth noting there's already a very similar, well-designed pattern for a related case: - `refreshLiveState()` itself _does_ correctly set `entry.missing = true` and fire the status event - when a periodic/tree-driven check finds the container gone — and the tree already has full - rendering support for that (`Missing · click to recreate`, bound to reopen the Quick Start panel, - which re-provisions). The gap is specifically that the **lifecycle actions** (`start`/`stop`/ - `restart`, and to a lesser extent `deleteContainer`, which has its own separate fallback lookup) - don't funnel through that same "missing" detection before giving up. - -**Open questions for later (explicitly deferred — discuss when fixing):** - -- Should `isManaged()` (or a new check ahead of it in `start`/`stop`/`restart`) distinguish "no - container found" from "found but not ours," and in the former case do what `liveStateGuard` - already does for the multi-window case — refresh live state + surface a message — rather than - silently returning? -- What should the message/options be? The user's suggestion: tell them the container wasn't found - (possibly deleted), and offer next steps — Delete (clean up the stale metadata/volume) and/or - "reload with same settings" (re-provision using the same image/credentials, similar to how the - existing `Missing` row already offers "click to recreate"). Note the `Missing` row's recreate flow - and the reuse-credentials path in `provision()` (`getReusableCredentials`) may already cover most - of "reload with same settings" — worth checking whether that flow is sufficient once actually - reachable from `start()`/`stop()`/`restart()`, or needs its own affordance. +| Option | Pros | Cons | +| --------------------------------------------- | --------------------------------------------------------- | ----------------------------------------------------------------- | +| **A. One-time toast + "Reveal" action** | Loud exactly once, at the moment it matters; non-blocking | Transient; a user who dismisses it still has to find the folder | +| **B. Distinct folder icon/description badge** | Persistent visual marker; no extra flow | Adds a special-case to folder rendering; mild clutter | +| **C. Pin to a fixed position (first/last)** | Predictable location regardless of name | Breaks the pure-alphabetical model; users may still not notice it | +| **D. Do nothing (current)** | Zero code | Folder hides in the alphabetical sort; confusing post-upgrade | ---- +> 💡 **Suggested:** **A**, optionally with **B**. The toast solves the "I didn't know my connections +> moved" moment directly; a subtle badge helps the user re-find it later without breaking the sort. + +### 13.2 Image-pull progress indicator (§6.1) + +| Option | Pros | Cons | +| -------------------------------------------------------------- | ----------------------------------------------------------- | ------------------------------------------------------------------------------ | +| **A. Spinner + static label (current)** | Zero work | Looks frozen for tens of seconds on a cold pull | +| **B. Indeterminate `ProgressBar` + "N of M layers" sub-label** | Alive + honest; reuses existing components; modest plumbing | Layer count parsed from free-form CLI text (small regex); no true % | +| **C. Docker Engine API / `dockerode` structured pull** | Real per-layer byte totals → an actual percentage | Larger change; new dependency/route away from the CLI-wrapper approach | +| **D. "Dot per completed layer" row** | Visually pleasant for small bounded step counts | Total layer count unknown until pull starts → row grows/shrinks, reads jittery | + +> 💡 **Suggested:** **B** as the near-term win (alive UI without the C rework), with **C** noted as the +> "if we want a real percentage" follow-up. Avoid **D** — the unknown-until-runtime layer count makes +> the dot row jitter. + +### 13.3 Cluster commands on the Quick Start row (§7.1) + +| Option | Pros | Cons | +| ----------------------------------------------------------------- | ----------------------------------------------------------------- | ------------------------------------------------------------------------ | +| **A. Curated menu only (current)** | No risk of storage commands acting on an in-memory item | Loses genuinely useful storage-independent commands (e.g. Open in Shell) | +| **B. Add specific storage-independent commands explicitly** | Gains the useful ones; still no storage-command risk | Each command re-declared with a Quick-Start `when`-clause | +| **C. Split base contextValue into storage vs connection subsets** | Clean, reusable; Quick Start opts into the connection-only subset | Touches the base cluster item + every command's `when`; larger refactor | + +> 💡 **Suggested:** **B** now (add "Open in Shell"/"New Query" against the running instance), keep **C** +> as the eventual clean-up if more items want the same split. + +### 13.4 External-delete / Missing recovery affordance (§9.1) + +| Option | Pros | Cons | +| ------------------------------------------------------------------ | ---------------------------------------------------------------- | ---------------------------------------------------------------------- | +| **A. Silent early-return (current)** | — | Looks broken; stale row; error only in the output channel | +| **B. Detect missing → set Missing badge + info message** | Reuses the existing `Missing` rendering + `liveStateGuard` style | User still has to choose the next step | +| **C. B + actionable prompt: Delete / Recreate with same settings** | One-click recovery; matches the "click to recreate" Missing row | Slightly more logic; must confirm `getReusableCredentials()` covers it | + +> 💡 **Suggested:** **C**. It turns a silent dead-end into a self-service recovery, and most of the +> machinery (Missing rendering, credential reuse in `provision()`) already exists — the work is mainly +> routing `start`/`stop`/`restart` into it instead of early-returning. + +### 13.5 Open terminal in container (§10.1) + +| Option | Pros | Cons | +| -------------------------------------------------------- | ---------------------------------------------- | ---------------------------------------------------------- | +| **A. `createTerminal` with `docker exec -it … bash`** | Simplest; Docker drives the TTY; no custom pty | Assumes `bash` in the image | +| **B. A + `/bin/sh` fallback** | Works even on minimal images | Two-attempt logic (or a probe) | +| **C. Custom `Pseudoterminal` like the integrated shell** | Full control over I/O | Overkill for "just give me a shell"; more code to maintain | -## Summary table - -| # | Topic | Status | -| --- | ------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------- | -| 1 | Legacy migration folder name/placement | Confirmed: `Local Connections (Legacy)`, alphabetically sorted with no pinning | -| 2 | Delete Container confirmation + em dashes | Confirmed: bypasses `getConfirmationAsInSettings`; em dashes present in multiple strings | -| 3 | Empty-state tree row wording/Learn-more placement | **Fix applied:** reworded to "Click here to install & try DocumentDB locally"; "Learn more" row dropped (context-menu relocation left open) | -| 4 | Webview header instability / stuck text | Confirmed real bug: `success`/`failed` reuse the `provisioning` header verbatim | -| 5 | "Waiting for connections" wording/sub-progress | Confirmed: static label for up to 3 minutes, no sub-status shown in-panel | -| 6 | Image pull progress tracking | Confirmed: none; ideas drafted (layer counter, indeterminate `ProgressBar`) | -| 7 | Running-instance row vs. cluster context commands | Confirmed deliberate contextValue replacement; discussed what would break if merged | -| 8 | Copy Connection String reimplementation | Confirmed: separate function, no password-inclusion prompt unlike regular clusters | -| 9 | Open terminal in container | Feasible via `createTerminal` + `docker exec -it`; no blocker found | -| 10 | Icon selection logic | Confirmed: no bespoke icons; inherited/generic `ThemeIcon`s throughout | -| 11 | Externally-deleted container not handled on Start | Root cause found: `isManaged()` conflates "missing" with "not ours," bypassing the existing `liveStateGuard` missing-state handling | +> 💡 **Suggested:** **B** — the standard, low-maintenance pattern, gated on `state_running`. Reserve **C** +> only if we later want to parse/relay the shell I/O. From 3ac7c33d4cb473efd42dae28fe2c1c940741dee3 Mon Sep 17 00:00:00 2001 From: Tomasz Naumowicz Date: Thu, 9 Jul 2026 17:37:46 +0200 Subject: [PATCH 3/7] chore: tweaking label text. --- l10n/bundle.l10n.json | 2 +- .../connections-view/LocalQuickStart/LocalQuickStartItem.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/l10n/bundle.l10n.json b/l10n/bundle.l10n.json index 080218316..c142fa86f 100644 --- a/l10n/bundle.l10n.json +++ b/l10n/bundle.l10n.json @@ -315,9 +315,9 @@ "Choose your provider…": "Choose your provider…", "Choose your Service Provider": "Choose your Service Provider", "Clear Query": "Clear Query", - "Click here to install & try DocumentDB locally": "Click here to install & try DocumentDB locally", "Click here to open the shell": "Click here to open the shell", "Click here to retry": "Click here to retry", + "Click here to start DocumentDB locally": "Click here to start DocumentDB locally", "Click here to update credentials": "Click here to update credentials", "Click to view resource": "Click to view resource", "Clipboard does not contain a recognizable find() query.": "Clipboard does not contain a recognizable find() query.", diff --git a/src/tree/connections-view/LocalQuickStart/LocalQuickStartItem.ts b/src/tree/connections-view/LocalQuickStart/LocalQuickStartItem.ts index 6763fe2e1..4ebcb0bed 100644 --- a/src/tree/connections-view/LocalQuickStart/LocalQuickStartItem.ts +++ b/src/tree/connections-view/LocalQuickStart/LocalQuickStartItem.ts @@ -174,7 +174,7 @@ export class LocalQuickStartItem implements TreeElement, TreeElementWithContextV createGenericElementWithContext({ id: `${this.id}/start`, contextValue: 'treeItem_quickStartAction', - label: l10n.t('Click here to install & try DocumentDB locally'), + label: l10n.t('Click here to start DocumentDB Local'), iconPath: new vscode.ThemeIcon('rocket'), commandId: 'vscode-documentdb.command.localQuickStart.open', }), From 5ea43acfb25208f1c74b5a47ad95f57e7db82040 Mon Sep 17 00:00:00 2001 From: Tomasz Naumowicz Date: Thu, 9 Jul 2026 18:12:59 +0200 Subject: [PATCH 4/7] refactor: enhance UX review document with detailed findings and suggestions for setup webview improvements --- .../PRs/documentdb-quickstart/ux-review.md | 489 +++++++++++++++++- 1 file changed, 465 insertions(+), 24 deletions(-) diff --git a/docs/ai-and-plans/PRs/documentdb-quickstart/ux-review.md b/docs/ai-and-plans/PRs/documentdb-quickstart/ux-review.md index 07984d6b0..c6e84907a 100644 --- a/docs/ai-and-plans/PRs/documentdb-quickstart/ux-review.md +++ b/docs/ai-and-plans/PRs/documentdb-quickstart/ux-review.md @@ -56,8 +56,11 @@ review hammered the _presentation and robustness_ of that journey: how the migra and sorted, how the empty-state rows read, how honest and stable the setup webview's header/progress is, how the running instance's menu and icons compare to regular clusters, whether destructive actions match the rest of the extension, and — the one hard bug — what happens when the container is -deleted **outside** VS Code. Most items are wording/consistency polish; one (§4 header) is a real -"stuck UI" bug; one (§9 external delete) is a silent-failure bug with a clear root cause. +deleted **outside** VS Code. Most items are wording/consistency polish; one (§4.2 header) is a real +"stuck UI" bug; §9 collects the harder robustness failures — a silent no-op on Start after an external +delete, and a **restart dead end** when saved credentials go missing (hit live during the review) that +also exposed a broader anti-pattern: **tree nodes used as error dialogs**. §8 adds the data/volume +model questions (ephemeral toggle, delete-drops-volume, image-linked volumes, delete-only-our-containers). --- @@ -131,9 +134,80 @@ parent `DocumentDB Local - Quick Start` node (a new `view/item/context` gated on --- -## 4. The setup webview — header stability +## 4. The setup webview — flow, loading & header + +### 4.1 Current flow — what the webview shows, and when 🔍 + +**Observation:** It should be clear what the setup webview renders at each step, so reviewers know +which screen a finding refers to. + +**Finding:** The panel is one React component driven by a `Phase` state machine +(`'loading' | 'review' | 'dockerNotReady' | 'provisioning' | 'success' | 'failed'` in +[LocalQuickStart.tsx](../../../../src/webviews/documentdb/localQuickStart/LocalQuickStart.tsx)). On +open it queries `getDockerStatus`, then branches: + +```text + panel opens + | + v + +-----------------+ getDockerStatus() + | loading |------------------------------. + | (BARE spinner: | | + | "Checking | Docker not ready v + | Docker..." | +----------------------------+ + | no hero/cards) |--------->| dockerNotReady | + +-----------------+ | hero "Docker is required" | + | | 3 readiness cards | + Docker ready | (CLI / daemon / platform) | + | | + "How to fix" + [Retry] ---+ (re-query) + | +----------------------------+ + | + ready & canResumeReadiness? --- yes --> jump to [failed / timedOut] (resume a prior + | timed-out container: Wait longer / Start over) + | no + v + +-----------------+ [Start DocumentDB Local] + | review |----------------------------. + | hero "Start | | + | DocumentDB | v + | Local" | +----------------------+ + | 4 config cards | | provisioning | + | (Image / Port / | | hero "Setting up..." | + | Data / Security| | elapsed timer | + | + Advanced | <-- [Edit | stage checklist: | + | (collapsed)) | settings] --| checking > pulling > | + | + [Start] | | creating > starting >| + +-----------------+ | waiting > done | + ^ | [Cancel] [View output]| + | +----------+-----------+ + | [Start over] success | | failed / timeout + | v v + | +-----------+ +----------------------+ + +------------------- | success | | failed | + | Next steps| | error box | + | [Open] [Copy] | [Retry][Edit] OR | + | [Close] | | [Wait longer][Start | + +-----------+ | over] (on timeout) | + +----------------------+ +``` + +Phase-by-phase, what is actually rendered: + +- **loading** — **only** a full-panel ``. No hero/title, no cards + — the whole panel is the spinner until `getDockerStatus` resolves, then it snaps to the next + layout (see §4.3). +- **dockerNotReady** — `hero("Docker is required")` + **three** readiness cards (Docker CLI / Docker + daemon / Platform, each with a ✓/! badge) + a "How to fix" card + `Retry`. +- **review** — `hero("Start DocumentDB Local")` + **four** config cards (Image / Port / Data / + Security) + a summary + a collapsed **Advanced** panel + `Start DocumentDB Local`. +- **provisioning** — `hero("Setting up DocumentDB Local…")` + elapsed timer + the stage checklist + (checking → pulling → creating → starting → waiting → done) + `Cancel` + `View Docker output`. +- **success** — header still reads "Setting up…" (bug, §4.2) + a success box + Next steps + + `Open Connection` / `Copy Connection String` / `Close`. +- **failed** — header still reads "Setting up…" (§4.2) + an error box + either `Retry` / `Edit +settings`, or (on a readiness timeout) `Wait longer` / `Start over`. -### 4.1 The webview header changes with state and gets stuck on "Setting up…" ⚠️ +### 4.2 The webview header changes with state and gets stuck on "Setting up…" ⚠️ **Observation:** The header should be static. Today it changes with status/progress/errors, which is unexpected — and it sometimes stays on "Setting up DocumentDB Local…" even when setup is actually @@ -158,6 +232,202 @@ success/failure boxes already exist). That satisfies "header should be static" a stuck-text bug in one move. If a dynamic header is preferred, it must at minimum branch on `success`/`failed` the way the `dockerNotReady` screen already does. +### 4.3 The initial "Checking Docker" loading should render chrome + skeleton cards (reuse the metric card) ⚠️💡 + +**Observation:** The initial `loading` ("Checking Docker") state should be reworked to behave like the +**Query Insights** view: the webview chrome renders immediately (title + header/hero), and the four +cards behave like our **metrics cards** — they show a **loading skeleton** while status resolves, then +fill **independently** (at least from a UX perspective; we don't care about splitting the backend now, +that's for later). Worth **reusing the existing metric card** — it's accessible and has tooltip +support, a better experience. + +**Finding:** + +- ⚠️ The `loading` phase renders **only** `` inside an otherwise + empty panel — no hero/title, no cards (§4.1). When `getDockerStatus` resolves, the panel **snaps** + from a bare spinner to the full `dockerNotReady`/`review` layout, which is the jump the observation + describes. Query Insights, by contrast, renders its header + metric row up front and lets the + individual metrics resolve into place. +- 🔍 The readiness/config cards use a **local, simplified** `MetricCard` defined inline in + [LocalQuickStart.tsx](../../../../src/webviews/documentdb/localQuickStart/LocalQuickStart.tsx) + (a plain `Card` with label + value + optional badge) — **no skeleton, no tooltip, weaker a11y.** +- 🔍 Query Insights already ships a purpose-built, accessible metric card: **`MetricBase`** at + [MetricBase.tsx](../../../../src/webviews/documentdb/collectionView/components/queryInsightsTab/components/metricsRow/MetricBase.tsx). + It supports a **loading skeleton** (`value === undefined` → `SkeletonItem`), a **null/unavailable** + placeholder, **tooltip explanations** (`tooltipExplanation`), and a solid **accessibility** pattern + (keyboard-focusable card, an `aria-label` carrying label + value + tooltip, `aria-hidden` children + to avoid double announcement). That's exactly the skeleton + tooltip + a11y behavior the + observation asks for. + +💡 **Suggestion:** + +1. Rework `loading` so it renders the **hero/header immediately** (like every other phase, and like + Query Insights), then renders the readiness cards in **skeleton** state, each flipping to its + value independently as status resolves — no need to split the backend; from the UX side a card can + go skeleton → value on its own. +2. **Reuse `MetricBase`** instead of the local `MetricCard`. It's accessible and has tooltip + + skeleton support out of the box — a better experience than the inline card. Two bits of work: (a) a + small **extension** (e.g. a badge slot for the ✓/! status badge the readiness cards use), and (b) + **relocation to a shared** webview location — it currently lives buried under + `collectionView/queryInsightsTab/`, and importing a query-insights-internal component into the + Quick Start webview is the wrong dependency direction. The move + small extension is worth it: one + accessible, tooltip-capable metric card shared across features. + +### 4.4 Advanced panel: credentials should sit behind a toggle, and "placeholder = default" is ambiguous ⚠️💡 + +**Observation:** In the Advanced section, username + password should be grouped **behind a toggle** — +the user flips "override the default" and only then specifies a username and password. The current +ghost text **"auto"** is confusing (is it the default _value_?). More generally, the panel label says +_"Leave any field blank to keep the automatic default,"_ yet the **Image tag** field shows **"latest"** +as ghost text — so how is the user meant to "keep it blank"? The person who addresses this should +**think through the options**. + +**Finding** (Advanced panel in +[LocalQuickStart.tsx](../../../../src/webviews/documentdb/localQuickStart/LocalQuickStart.tsx), +`renderAdvanced()`): + +- On a fresh provision (`!isRecreate`) the panel renders Port, Image tag, Username, Password (each + always visible) + a "Load sample data" switch, under the label _"Leave any field blank to keep the + automatic default."_ +- ⚠️ **Placeholder means two different things across fields.** Port's placeholder is the literal + default value (`10260`), Image tag's is the literal default tag (`latest`) — but Username/Password + use the word **`'auto'`**, which is a _description of behavior_ ("will be auto-generated"), not a + value you'd type. So "latest" reads as if a value is already set, while "auto" reads as a mode. + Combined with "leave blank to keep the default," the greyed-in `latest` makes it genuinely unclear + whether the field is pre-filled or empty — the exact confusion the observation calls out. +- 🔍 The credentials already follow a **both-or-neither** rule in `advValidation` + (_"Enter both a username and a password, or leave both blank to auto-generate."_). A toggle would + encode that rule structurally instead of leaving it to a validation error after the fact. +- 🔍 On a recreate (`isRecreate`) the credential/tag fields are already hidden and replaced by a note + that the original credentials/image are reused — so this only concerns the fresh-provision path. + +💡 **Suggestion:** + +1. **Group credentials behind a toggle.** Add an "Override generated credentials" (or "Set my own + username & password") switch, **off** by default. Off → show a short read-only note + ("Auto-generated, stored securely," matching the summary card); On → reveal both fields, required + together (reuses the existing both-or-neither validation). This removes the ambiguous `'auto'` + ghost text entirely and makes "I want to override" an explicit, discoverable action. +2. **Resolve the "placeholder = default value" ambiguity** — options for whoever implements it to weigh + (the goal: a user can always tell "blank = default" from "I typed a value"): + - **A. Drop literal-value placeholders.** Leave the input visually empty (or a neutral + "Default") and keep the real default only in the `hint` line under the field (the panel already + renders _"Default {port}"_ / _"Default “latest”"_ / _"Default: auto-generated"_). Then "leave + blank = default" is honest: the box looks empty, the hint states the default. Cheapest and most + consistent. + - **B. Pre-fill the actual default value** (not a placeholder) and let the user edit it. No "blank" + concept at all — what you see is what runs. But then the "leave blank" label must go and empty + must be re-validated/normalized. + - **C. Per-field "Use default" affordance** (checkbox/toggle per field). Most explicit, but heavier + and busier than A for a panel that already has one toggle model in suggestion 1. + - **Leaning A** for consistency + minimal work, paired with suggestion 1 so credentials leave the + grid entirely. Whichever is chosen, apply the **same** convention to every Advanced field so + placeholder never sometimes-means-value and sometimes-means-mode. + +### 4.5 Error/status messages must leave the hero — show a content-area card; run a small UX experiment ⚠️💡 + +**Observation:** We have error/status states (e.g. Docker missing). The core requirement: these +messages **must not live in the hero section** — the hero stays constant and the message becomes a +**card in the content area** with the actions the user can perform. Query Insights already has good +building blocks for this — not just the `MessageBar`, but the **richer composed card** shown when an +RU user opens the Query Insights tab; the two **could be combined**. Because it isn't obvious which +shape reads best, whoever implements this should **render a few options to compare** (a UX +experiment), and we also need to decide **where the Retry action lives** — there's no good home for it +in today's model. + +**Finding:** + +- ⚠️ Today an error is handled by switching to a **dedicated phase that replaces the hero**: + `dockerNotReady` swaps the hero to _"Docker is required"_ and renders a bespoke full-screen layout + (three readiness cards + a "How to fix" card + a bottom `Retry` that calls `loadDockerStatus`); the + `failed` phase similarly shows an error box with the hero **stuck** on _"Setting up…"_ (§4.2). So the + message is **coupled to the hero** — exactly what the observation wants to stop. (A `Retry` _does_ + exist on the docker-not-ready screen, but only as part of that full-screen error phase, not as an + inline card action.) +- 🔍 **Query Insights already ships two reusable shapes** — both leave the surrounding header/title + untouched and put the message in the content area: + - **Concise inline `MessageBar`.** + [`GetPerformanceInsightsCard.tsx`](../../../../src/webviews/documentdb/collectionView/components/queryInsightsTab/components/optimizationCards/custom/GetPerformanceInsightsCard.tsx) + renders a Fluent **`MessageBar`** (`MessageBarBody` + `MessageBarTitle`) when `errorMessage` is set + and **relabels its primary button to "Retry"** — the same handler doubles as retry: + `{errorMessage ? l10n.t('Retry') : l10n.t('Get AI Performance Insights')}`. The card title never + changes. + [`ImprovementCard.tsx`](../../../../src/webviews/documentdb/collectionView/components/queryInsightsTab/components/optimizationCards/ImprovementCard.tsx) + uses the same `MessageBar intent="warning"` / `intent="success"` blocks for per-action state. + - **Richer composed card (the RU-tab card the observation refers to).** When an RU user opens Query + Insights, [`QueryInsightsTab.tsx`](../../../../src/webviews/documentdb/collectionView/components/queryInsightsTab/QueryInsightsTab.tsx) + (the `QUERY_INSIGHTS_PLATFORM_NOT_SUPPORTED_RU` branch) renders a **`MarkdownCardEx`** — an + icon + title (_"Query Insights Not Available"_) + a markdown body explaining _why_ and a + call-to-action (links to start a GitHub discussion) — and **nests a `MessageBar` inside it** for + the concise one-line status. So the "card" and the "message bar" are **already combined** there: + the composed card is the container, the `MessageBar` is the crisp summary line within it. + +💡 **Suggestion:** + +1. **Get the message out of the hero.** Keep the hero **static** (per §4.2) and render error/status as + a **content-area card**, never as a hero-text swap or a `dockerNotReady`/`failed` full-screen phase + that hijacks the title. Applies to docker-not-ready and provisioning `failed`. +2. **Combine the two shapes, and experiment.** The RU card shows the pattern to reuse: a composed + titled card (icon + title + short explanation + actions) that **contains** a `MessageBar` for the + crisp status line. Because the right density isn't obvious, whoever implements this should **render + a few candidate variants to compare** rather than pick one blindly, e.g.: + - **Option A — plain `MessageBar`** with an inline `Retry` (lightest; good for transient errors). + - **Option B — composed card** (`MarkdownCardEx`-style: icon + title + body + action buttons), no + separate bar (richest; good for "Docker is required" with fix steps + links). + - **Option C — combined** (composed card **containing** a `MessageBar`), matching the RU tab. + Render A/B/C against the real states (Docker missing, provisioning failed, readiness timeout) and + pick per-state — a lightweight bar may suit `failed`, while docker-not-ready likely wants the richer + card with fix links. +3. **Retry placement:** put `Retry` **on the chosen card**, next to the other recovery actions (Install + Docker / Troubleshooting; Edit settings / Wait longer / Start over), following the Query Insights + convention of **relabeling the card's primary action to "Retry"** when an error is present — not a + separate bottom-of-panel button. The retry logic already exists (`loadDockerStatus` / `handleStart`); + only its _home_ changes. +4. This pairs with **§9.3** (tree side: errors are actions/dialogs, not passive nodes) — the same + principle on the webview: **a message is an actionable content card, not a state that hijacks the + hero.** A shared, reusable error/status card (composed card + optional `MessageBar` + primary/Retry + - links) would serve docker-not-ready and `failed` and keep the hero constant. + +### 4.6 What does "Cancel" do mid-provision? Consider the verb "Abort" (no full rollback promise) ⚠️💡 + +**Observation (work item):** Investigate what **Cancel** actually does when clicked while provisioning +is in progress. Do we **delete an image that has started downloading**, or just abort? A clean +rollback is hard to implement (and to promise), so **"Abort"** may be a cleaner verb — "Cancel" implies +cancel-and-roll-back, which we can't reliably guarantee. + +**Finding** (traced in `provision()`, +[QuickStartService.ts](../../../../src/services/localQuickStart/QuickStartService.ts)): the answer is +**"abort, with partial cleanup—but the image is kept."** On cancel (the panel's `Cancel` button aborts +the provisioning subscription → `AbortSignal` → `onAbort` fires `cts.cancel()`): + +- 🔍 The in-flight Docker command is **aborted** via the cancellation token — a `docker pull` in + progress stops promptly. +- 🔍 In `finally`, cleanup is **container-scoped only** (decision D12): if a container was created it's + stopped + removed; if a create was attempted but the id wasn't captured, an orphan is swept by + label; the temp env-file is deleted; state resets `Provisioning → NotInstalled`. +- ⚠️ **The image is never deleted.** Nothing calls a `removeImage`; whatever layers `docker pull` + already fetched **stay in Docker's local cache** (by design — a resumable/cached pull makes the next + attempt fast). So Cancel is a **partial** rollback: the half-created _container_ is removed, but the + partially-/fully-downloaded _image_ is retained. +- Net: "Cancel" today reads as "undo everything," but we only undo the container, not the download. The + retained image cache is arguably the _right_ behavior (fast retry) — it just isn't what "Cancel" + promises. + +💡 **Suggestion:** + +1. **Rename the verb to "Abort"** (or "Stop setup") to set honest expectations: we stop the in-progress + operation and remove any half-created container, but we **don't** promise to erase everything — the + cached image layers are kept intentionally. "Cancel" over-promises a rollback we don't perform. +2. **Say what's kept**, right by the button: a one-liner like _"Stops setup. The downloaded image is + kept so a retry is faster."_ removes the ambiguity the observation raises. +3. **Confirm the behavior per phase** as part of the work item (pull / creating / starting / waiting): + verify no orphaned resources survive _besides_ the intentional image cache, and that a mid-pull + abort leaves nothing half-written except cached layers. Decide explicitly that keeping the image is + desired (it is, for retry speed) and document it. +4. If a true "remove everything including the image" is ever wanted, make it a **separate, explicit** + action (e.g. an "Abort and clean up" secondary), never the default — deleting cached layers on every + cancel would make retries slow and is the "clean rollback is tough" case the observation flags. + --- ## 5. The setup webview — the "waiting" stage @@ -286,7 +556,7 @@ emulator at a glance. Low priority, but cheap. --- -## 8. Destructive actions +## 8. Destructive actions & the data / volume model ### 8.1 "Delete Container" bypasses the shared confirmation and uses em dashes ⚠️ @@ -313,6 +583,78 @@ the confirmation word — there's no "type the container name" concept yet, so a `delete` matches the other delete flows). Separately, do a **one-pass em-dash sweep** of the whole feature's user-facing strings rather than fixing only the delete dialog. +### 8.2 Persistence is always on — there's no "ephemeral / no-persistence" option 💡 + +**Observation:** Our default is a persistent data volume. That should be a **toggle** — sometimes a +user wants a throwaway instance with no persistence. + +**Finding:** 🔍 `AdvancedQuickStartOptions` +([quickStartTypes.ts](../../../../src/services/localQuickStart/quickStartTypes.ts)) exposes `port`, +`username`, `password`, `imageTag`, and `loadSampleData` — but **no persistence flag**. +`createAndRunContainer()` **always** mounts a named volume (`volumeName(alias)` at +`QUICK_START_DATA_PATH`). So every instance is persistent by construction; there is no way to opt out +of the on-disk volume. + +💡 **Suggestion:** Add an Advanced toggle **"Persist data across restarts"** (default **on**, keeping +the current zero-decision behavior). When **off**, create the container **without** the named-volume +mount so data lives only in the container's writable layer and is gone on delete — useful for quick +throwaway trials and clean repros. Trade-offs in [§13.6](#136-persistence--the-volume-model). + +### 8.3 A user-initiated delete must always delete the data volume ⚠️ + +**Observation:** When the user deletes a container, we should for sure delete the persisted volume as +well. + +**Finding:** 🔍 The explicit **Delete Container** path already does this: `deleteContainer()` +([QuickStartService.ts](../../../../src/services/localQuickStart/QuickStartService.ts)) calls +`removeVolume(volumeName(alias))` alongside the secret / globalState / registry cleanup — an +intentional "full clean slate." Two caveats to stamp: (a) the credential-unavailable **reconcile** +path deliberately does **not** touch the volume (R2 data-safety — see §9.2), which is correct for an +_automatic_ path but means the _user-initiated_ delete must remain the thing that wipes it; (b) once +§9.2 makes the credential-unavailable state actionable, its Delete must route through this same +volume-removing path. + +💡 **Suggestion:** Keep explicit Delete = remove container **+ volume** (already the case). Ensure +every **user-initiated** delete surface (including the future credential-unavailable / Missing Delete) +funnels through `deleteContainer()` so a volume is never orphaned. Automatic / reconcile paths keep +R2 (never auto-wipe without the user choosing it). + +### 8.4 Data volumes should be linked to the image — no shared data volumes 💡 + +**Observation:** Our persisted volumes should be **linked to the image**. We won't share data volumes +across images; advanced users can do that themselves. + +**Finding:** 🔍 Today the volume is keyed to the **alias** (`${alias}-data`), independent of the +image. To stay safe, the service **pins the image to the volume**: on a recreate it reuses the stored +`imageRef` (from in-memory metadata → `globalState` → default) rather than the user's requested tag, +and custom image/creds are ignored on a Missing-recreate — precisely so an existing volume isn't +opened by a different (possibly older) image version that could corrupt it. So the intent ("one image +↔ one volume") is enforced by _runtime pinning_, but the volume is not _named/keyed_ by image, and +there is no model for multiple images each owning their own volume. + +💡 **Suggestion:** Make the linkage **structural**, not just runtime — key or label the volume by its +image (or record the image on the volume) so "one image ↔ one data volume" is guaranteed even if the +pinning logic changes. Do **not** implement cross-image volume sharing; leave that to advanced users +operating Docker directly. This also sets up a cleaner story if multi-instance / multi-image lands. + +### 8.5 We should only ever delete containers we created (pre-release safety) ⚠️ + +**Observation:** Keep as a review note — not urgent now, but must be addressed **before final +release**: we should be allowed to delete **only** the containers we created. + +**Finding:** 🔍 Removal is already label-guarded in the normal path — `deleteContainer()` only removes +when `entry.missing || isManaged(id)`, and `isManaged()` verifies the `vscode.documentdb.quickstart` +label. But there are softer spots: on `entry.missing` the label re-check is **skipped** (the id comes +from metadata / `findManagedContainer`, which filters by label, so it is _currently_ safe but relies +on that invariant), and `ContainerRuntime.removeContainer()` itself does **no** label check. As the +feature grows (multi-instance, user-named containers, more recovery flows), an id-based delete without +a label re-verify is a latent risk of removing something we didn't create. + +💡 **Suggestion:** Before final release, make "ours" a **hard precondition on every removal** — +re-verify the `vscode.documentdb.quickstart` label immediately before `removeContainer` / +`removeVolume`, regardless of the `missing` shortcut, so no code path can ever remove a container or +volume the extension didn't create. + --- ## 9. Lifecycle robustness & error recovery @@ -358,6 +700,74 @@ same settings** (the existing `Missing` row's recreate flow + `getReusableCreden `provision()` likely already covers most of this — worth confirming it's sufficient once reachable from the lifecycle actions). Full UX options in [§13.4](#134-external-deletemissing-recovery-affordance). +### 9.2 Credential-unavailable state after a restart is a UI dead end (the demo incident) ⚠️ + +**Observation (real incident, mid-review):** An instance was created via Quick Start, then stopped, +then its container/image were removed outside VS Code. After a **VS Code restart** the Quick Start +node showed the rocket _"Click here to start DocumentDB locally"_ row **plus** a warning row +_"DocumentDB Local has data on disk but its saved credentials are missing…"_ (truncated in the tree). +There was **no way from the UI** to delete the leftover container/volume or to start cleanly — no +Delete node, no recreate action. Recovery required manually removing the container and its volume with +Docker, then refreshing. + +**Finding:** + +- On activation, `reconcile()` → `reconcileAlias()` hits **"Case 4"**: a labelled container (or a + `ready` record) exists but the stored secret is unrecoverable, so it calls + `setStatus(alias, InstanceState.Error, undefined, CREDENTIAL_UNAVAILABLE_MESSAGE)` with + **`metadata: undefined`**. By design it **never** removes the container or volume (R2 data-safety: a + lost secret doesn't prove the volume is disposable — the user must choose the wipe). +- Because `metadata` is `undefined`, `LocalQuickStartItem.getChildren()` skips every `metadata && …` + branch and falls through to the **NotInstalled** branch, which renders the rocket action row **plus** + a passive `treeItem_quickStartError` warning row carrying the message. +- ⚠️ That warning row has **no command and no context menu**, and the Delete command's `when` requires + `treeItem_quickStartInstance` + a `state_*` token — which this surfaced state never carries. So + **Delete is unreachable**, and unlike the true `Missing` row (which at least says "click to + recreate") no recovery action is offered. The user is genuinely stuck in the UI. +- ⚠️ **Restart behavior specifically:** in-memory state is rebuilt from durable state on every reload, + so this dead end **reproduces on every restart** — it is not a transient glitch. The + volume-plus-missing-secret combination persists, so each launch re-surfaces the same non-actionable + warning. (Clicking the rocket to "start fresh" doesn't help either: the on-disk volume + missing + credentials block a clean provision, and §9.1's silent-return path compounds it.) +- **How we recovered (for the runbook):** removed the labelled container and its data volume directly + with Docker (`docker rm -f vscode-documentdb-local` + `docker volume rm vscode-documentdb-local-data`), + then refreshed the view; reconcile found nothing and returned to the clean NotInstalled empty state. + +💡 **Suggestion:** The credential-unavailable state must be **actionable**, not a passive warning. Render +it like the `Missing` row — a real instance row (`treeItem_quickStartInstance` + a dedicated +`state_credentialsMissing` token) that exposes **Delete Container** (clean slate: container + volume + +stale records, via `deleteContainer()` per §8.3) and, where safe, **Recreate**. Keep R2 (never +_auto_-wipe) — but give the user a one-click way to _choose_ the wipe. This should share the recovery +UX in [§13.4](#134-external-deletemissing-recovery-affordance), and is a direct instance of the §9.3 +principle below. + +### 9.3 Tree nodes are misused to display errors (they should be actions, not dialogs) ⚠️💡 + +**Observation:** This incident exposed a broader pattern problem — we render an **error message as a +tree node**. Leaf/error nodes in a tree should be **actions the user can take**, not a substitute for +a dialog or a status surface. + +**Finding:** + +- Several Quick Start states render a passive, non-actionable row purely to display text: + `treeItem_quickStartError` (the credential-unavailable / error message, §9.2), and the empty state + also **appends** an error row when `status.state === Error && errorMessage`. These rows have no + command and no menu — they exist only to show a string, which then **truncates** in the tree (the + incident's "…saved credentials are miss…"), can't be copied, and competes visually with the real + action row. +- This contradicts where the rest of the extension landed. The **Kubernetes discovery** review + reached exactly this conclusion and reversed it: classified in-tree error-summary nodes were + **removed** in favor of a **modal** (`showErrorMessage(…, { modal: true })`) plus a single + actionable retry node — see that review's §F/#19 iteration 2 and its post-merge reconciliation note. + Quick Start currently repeats the anti-pattern the K8s feature already retired. + +💡 **Suggestion:** Adopt the same rule feature-wide: **tree rows are actions; errors are dialogs / +status.** Concretely: (a) surface an error/credential-unavailable condition as a **modal** (with detail +in the output channel), and (b) keep **only actionable rows** in the tree (Delete / Recreate / Retry / +"Click here to…"). This removes the passive `treeItem_quickStartError` and the error-append row, and it +directly fixes §9.2's dead end. It also rhymes with §4.2 (the webview header carrying status it +shouldn't) — the same "don't overload one surface with state it isn't meant to hold" theme. + --- ## 10. Possible additions @@ -389,19 +799,29 @@ natural power-user affordance. Open details (menu group/icon, shell fallback pro ## 11. Consolidated flags & suggestions (read this before testing) -| § | Item | Verdict | Suggested next step | -| ---- | ----------------------------------------------- | ------------------------ | ----------------------------------------------------------------------------------------------------------------------- | -| 2.1 | Legacy migration folder discoverability | ⚠️ Flag | One-time toast + "Reveal"; optional distinct badge/pin ([§13.1](#131-surfacing-the-legacy-migration-folder)) | -| 3.1 | Empty-state wording / drop "Learn more" | ✅ Fix applied | Decide if "Learn more" returns as a context-menu entry | -| 4.1 | Webview header changes / stuck on "Setting up" | ⚠️ Flag (real bug) | Make the header a single static string; body carries state | -| 5.1 | "Waiting for connections" static for minutes | ⚠️ Flag | Add in-stage elapsed/sub-info; surface "View output" earlier | -| 6.1 | No image-pull progress | ⚠️ Flag | Indeterminate `ProgressBar` + "N of M layers" ([§13.2](#132-image-pull-progress-indicator)) | -| 7.1 | Running row lacks regular cluster commands | 🔍 Answered (deliberate) | Add back storage-independent commands explicitly ([§13.3](#133-cluster-commands-on-the-quick-start-row)) | -| 7.2 | Copy Connection String silently adds password | ⚠️ Flag | Reuse the regular QuickPick confirmation, or document the divergence | -| 7.3 | Generic `$(plug)` icon, no distinct identity | 🔍 Answered | Give the instance a distinct icon (reuse the rocket motif) | -| 8.1 | Delete bypasses shared confirmation + em dashes | ⚠️ Flag | Switch to `getConfirmationAsInSettings()`; sweep em dashes feature-wide | -| 9.1 | External container delete not handled on Start | ⚠️ Flag (real bug) | Distinguish missing vs not-ours; route to Missing + recovery ([§13.4](#134-external-deletemissing-recovery-affordance)) | -| 10.1 | "Open terminal in container" | 🔍 Feasible | Add `state_running` "Open in Terminal" command ([§13.5](#135-open-terminal-in-container)) | +| § | Item | Verdict | Suggested next step | +| ---- | ---------------------------------------------------- | ------------------------ | ----------------------------------------------------------------------------------------------------------------------------------- | +| 2.1 | Legacy migration folder discoverability | ⚠️ Flag | One-time toast + "Reveal"; optional distinct badge/pin ([§13.1](#131-surfacing-the-legacy-migration-folder)) | +| 3.1 | Empty-state wording / drop "Learn more" | ✅ Fix applied | Decide if "Learn more" returns as a context-menu entry | +| 4.2 | Webview header changes / stuck on "Setting up" | ⚠️ Flag (real bug) | Make the header a single static string; body carries state | +| 4.3 | "Checking Docker" loading is a bare spinner | ⚠️ Flag / 💡 Suggestion | Render header + skeleton cards; reuse & relocate the Query Insights `MetricBase` | +| 4.4 | Advanced: creds not behind a toggle; placeholders | ⚠️ Flag / 💡 Suggestion | Toggle to reveal user/pass; drop literal-value placeholders (keep in `hint`) | +| 4.5 | Errors swap the hero / full-screen phase | ⚠️ Flag / 💡 Suggestion | Move messages out of the hero to a content card; combine `MessageBar` + composed RU-style card; experiment A/B/C; Retry on the card | +| 4.6 | "Cancel" mid-provision: partial rollback, image kept | ⚠️ Flag / 💡 Suggestion | Rename to "Abort"; state the image is kept; confirm per-phase cleanup | +| 5.1 | "Waiting for connections" static for minutes | ⚠️ Flag | Add in-stage elapsed/sub-info; surface "View output" earlier | +| 6.1 | No image-pull progress | ⚠️ Flag | Indeterminate `ProgressBar` + "N of M layers" ([§13.2](#132-image-pull-progress-indicator)) | +| 7.1 | Running row lacks regular cluster commands | 🔍 Answered (deliberate) | Add back storage-independent commands explicitly ([§13.3](#133-cluster-commands-on-the-quick-start-row)) | +| 7.2 | Copy Connection String silently adds password | ⚠️ Flag | Reuse the regular QuickPick confirmation, or document the divergence | +| 7.3 | Generic `$(plug)` icon, no distinct identity | 🔍 Answered | Give the instance a distinct icon (reuse the rocket motif) | +| 8.1 | Delete bypasses shared confirmation + em dashes | ⚠️ Flag | Switch to `getConfirmationAsInSettings()`; sweep em dashes feature-wide | +| 8.2 | Persistence always on — no ephemeral option | 💡 Suggestion | Add an Advanced "Persist data" toggle (default on) ([§13.6](#136-persistence--the-volume-model)) | +| 8.3 | User delete must always drop the volume | ⚠️ Flag | Route every user-initiated delete through `deleteContainer()` (volume incl.) | +| 8.4 | Volumes not structurally linked to the image | 💡 Suggestion | Key/label the volume by image; no cross-image sharing | +| 8.5 | Delete only containers we created | ⚠️ Flag (pre-release) | Re-verify the Quick Start label immediately before every remove | +| 9.1 | External container delete not handled on Start | ⚠️ Flag (real bug) | Distinguish missing vs not-ours; route to Missing + recovery ([§13.4](#134-external-deletemissing-recovery-affordance)) | +| 9.2 | Credential-unavailable state is a restart dead end | ⚠️ Flag (real bug) | Make it an actionable instance row (Delete / Recreate); shares [§13.4](#134-external-deletemissing-recovery-affordance) | +| 9.3 | Tree nodes misused as error dialogs | ⚠️ Flag (pattern) | Errors → modal + output channel; tree rows are actions only | +| 10.1 | "Open terminal in container" | 🔍 Feasible | Add `state_running` "Open in Terminal" command ([§13.5](#135-open-terminal-in-container)) | --- @@ -423,15 +843,19 @@ for the team to react to, not decisions. ### 13.1 Surfacing the legacy migration folder (§2.1) -| Option | Pros | Cons | -| --------------------------------------------- | --------------------------------------------------------- | ----------------------------------------------------------------- | -| **A. One-time toast + "Reveal" action** | Loud exactly once, at the moment it matters; non-blocking | Transient; a user who dismisses it still has to find the folder | -| **B. Distinct folder icon/description badge** | Persistent visual marker; no extra flow | Adds a special-case to folder rendering; mild clutter | -| **C. Pin to a fixed position (first/last)** | Predictable location regardless of name | Breaks the pure-alphabetical model; users may still not notice it | -| **D. Do nothing (current)** | Zero code | Folder hides in the alphabetical sort; confusing post-upgrade | +| Option | Pros | Cons | +| --------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------ | ----------------------------------------------------------------------------------- | +| **A. One-time toast + "Reveal" action** | Loud exactly once, at the moment it matters; non-blocking | Transient; a user who dismisses it still has to find the folder | +| **B. Distinct folder icon/description badge** | Persistent visual marker; no extra flow | Adds a special-case to folder rendering; mild clutter | +| **C. Pin to a fixed position (first/last)** | Predictable location regardless of name | Breaks the pure-alphabetical model; users may still not notice it | +| **D. Do nothing (current)** | Zero code | Folder hides in the alphabetical sort; confusing post-upgrade | +| **E. Prefix the folder name with `_`** | Dead-cheap: `_` sorts before letters, so `_Local Connections (Legacy)` floats to the top of the alphabetical list with **zero** rendering/sort changes | Cosmetic leading `_` in the name; a user can rename it away; not as loud as a toast | > 💡 **Suggested:** **A**, optionally with **B**. The toast solves the "I didn't know my connections > moved" moment directly; a subtle badge helps the user re-find it later without breaking the sort. +> If a code-free quick win is preferred first, **E** (prefix the name with `_`) reliably pins it to the +> top of the existing `localeCompare(..., { numeric: true })` order with no sort/render changes, and +> composes fine with A/B. ### 13.2 Image-pull progress indicator (§6.1) @@ -479,3 +903,20 @@ for the team to react to, not decisions. > 💡 **Suggested:** **B** — the standard, low-maintenance pattern, gated on `state_running`. Reserve **C** > only if we later want to parse/relay the shell I/O. + +### 13.6 Persistence & the volume model (§8.2 / §8.3 / §8.4) + +Three related choices about how the on-disk data volume behaves. They interact: an "ephemeral" option +only makes sense once delete reliably wipes the volume, and both are cleaner if the volume is tied to +its image. + +| Question | Options | Suggested | +| -------------------------- | ----------------------------------------------------------------------------------------------------- | ----------------------------------------------------------------------------------------------- | +| **Persist vs ephemeral** | (A) Always persist (current). (B) Advanced toggle, default persist. (C) Prompt each run. | **B** — a default-on "Persist data across restarts" toggle; no mount when off. | +| **Delete → volume** | (A) Delete container only. (B) Delete container **+ volume** on user delete (current explicit path). | **B**, applied to _every_ user-initiated delete (incl. the future credential-missing Delete). | +| **Volume ↔ image linkage** | (A) Alias-keyed volume + runtime image pinning (current). (B) Structurally key/label volume by image. | **B** — make "one image ↔ one volume" structural; no cross-image sharing (advanced users only). | + +> 💡 **Suggested overall:** default stays "persistent, clean-delete, image-pinned" so the zero-decision +> path is unchanged; add the **ephemeral toggle** for throwaway trials, make the **volume↔image link +> structural**, and never share data volumes across images (advanced users can wire that up in Docker +> themselves). Pre-release, pair this with §8.5 (only ever remove containers/volumes we created). From cfd972e5791cf8cd09c3cd4fd9ad748afd546e0d Mon Sep 17 00:00:00 2001 From: Tomasz Naumowicz Date: Thu, 9 Jul 2026 18:30:33 +0200 Subject: [PATCH 5/7] refactor: add priority legend and detailed flags for UX review findings --- .../PRs/documentdb-quickstart/ux-review.md | 32 +++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/docs/ai-and-plans/PRs/documentdb-quickstart/ux-review.md b/docs/ai-and-plans/PRs/documentdb-quickstart/ux-review.md index c6e84907a..6a2eac551 100644 --- a/docs/ai-and-plans/PRs/documentdb-quickstart/ux-review.md +++ b/docs/ai-and-plans/PRs/documentdb-quickstart/ux-review.md @@ -799,6 +799,38 @@ natural power-user affordance. Open details (menu group/icon, shell fallback pro ## 11. Consolidated flags & suggestions (read this before testing) +**Priority legend:** **P0** blocking — the user gets stuck · **P1** broken/misleading, or a +consistency & safety gap · **P2** polish, expectation, or a smaller feature gap · **P3** +nice-to-have / cosmetic. + +### 11.1 By priority (P0 → P3) + +| Priority | § | Item | Why it ranks here | +| -------- | ---- | --------------------------------------------------- | ---------------------------------------------------------------- | +| **P0** | 9.2 | Credential-missing state is a restart dead end | Real bug, hit live; the user is fully stuck with no UI action | +| **P0** | 9.1 | External container delete → silent no-op on Start | Real bug; stale row, zero feedback, only an output-channel error | +| **P1** | 4.2 | Webview header stuck on "Setting up…" | Real bug; the panel looks unfinished after success/failure | +| **P1** | 9.3 | Tree nodes misused as error dialogs | Pattern; the root cause of 9.2, contradicts the K8s decision | +| **P1** | 4.5 | Errors hijack the hero (should be content cards) | Webview twin of 9.3; ties to the 4.2 header bug | +| **P1** | 7.2 | Copy Connection String silently copies the password | Consistency + a credential surprise vs the regular command | +| **P1** | 8.1 | Delete bypasses the shared confirmation | The only destructive command ignoring `confirmationStyle` | +| **P1** | 8.5 | Only delete containers we created | Data-safety; a pre-release must-fix as the feature grows | +| **P2** | 2.1 | Legacy migration folder hides in the sort | Post-upgrade confusion; connections seem to "vanish" | +| **P2** | 4.3 | "Checking Docker" is a bare spinner | Loading polish; reuse the accessible `MetricBase` | +| **P2** | 4.4 | Advanced: creds toggle + ambiguous placeholders | Confusing defaults ("auto" vs "latest" vs "leave blank") | +| **P2** | 4.6 | "Cancel" implies a rollback it doesn't do | Verb → "Abort"; the image is kept | +| **P2** | 5.1 | "Waiting…" static for minutes | No progress feedback during a long wait | +| **P2** | 6.1 | No image-pull progress | Looks frozen on a cold pull | +| **P2** | 7.1 | Running row lacks regular cluster commands | Missing "Open in Shell" / "New Query" on the instance | +| **P2** | 8.2 | Always persistent — no ephemeral option | Feature gap; no throwaway instance | +| **P2** | 8.3 | User delete must always drop the volume | Mostly done; guarantee it across every user delete | +| **P2** | 8.4 | Volumes not structurally linked to the image | Data-model hardening (pinning is runtime-only today) | +| **P3** | 7.3 | Generic `$(plug)` icon, no distinct identity | Cosmetic; shared with any "emulator" connection | +| **P3** | 10.1 | "Open terminal in the container" | Enhancement; feasible, no blocker | +| **✅** | 3.1 | Empty-state wording / drop "Learn more" | Fix applied on this branch | + +### 11.2 By section (full detail) + | § | Item | Verdict | Suggested next step | | ---- | ---------------------------------------------------- | ------------------------ | ----------------------------------------------------------------------------------------------------------------------------------- | | 2.1 | Legacy migration folder discoverability | ⚠️ Flag | One-time toast + "Reveal"; optional distinct badge/pin ([§13.1](#131-surfacing-the-legacy-migration-folder)) | From 613f17bab10aaf542bc2760da2f290f4ad347d9f Mon Sep 17 00:00:00 2001 From: Tomasz Naumowicz Date: Thu, 9 Jul 2026 18:55:19 +0200 Subject: [PATCH 6/7] Implement code changes to enhance functionality and improve performance --- .../PRs/documentdb-quickstart/ux-review.md | 1557 +++++++++-------- 1 file changed, 799 insertions(+), 758 deletions(-) diff --git a/docs/ai-and-plans/PRs/documentdb-quickstart/ux-review.md b/docs/ai-and-plans/PRs/documentdb-quickstart/ux-review.md index 6a2eac551..a9f35dbdf 100644 --- a/docs/ai-and-plans/PRs/documentdb-quickstart/ux-review.md +++ b/docs/ai-and-plans/PRs/documentdb-quickstart/ux-review.md @@ -1,12 +1,12 @@ # DocumentDB Local Quick Start — UX Review Pack > **Who this is for:** anyone about to do a hands-on UX review of the **Local Quick Start** feature -> (installing/running a local DocumentDB container from inside VS Code), or anyone reading back how -> the review unfolded. -> **What this is:** a single catch-up document that captures a round of runtime UX feedback on the -> feature, states what the code _actually does today_ (verified against the current branch), groups -> the findings by user journey, and — for each — offers a **suggestion** the team can react to. -> Most items are still open discovery notes; where a quick, low-risk fix was applied it is stamped. +> (installing/running a local DocumentDB container from inside VS Code), or anyone triaging the +> findings. +> **What this is:** a single catch-up document that captures a round of runtime UX feedback, states +> what the code _actually does today_ (verified against the current branch), and — for each item — +> offers a **suggestion** and a **status**. Items are **sorted by priority** (P0 → P3). Almost every +> item is **Open**: the operator must choose the direction. - **Feature area:** `src/services/localQuickStart/`, `src/commands/localQuickStart/`, `src/tree/connections-view/LocalQuickStart/`, `src/webviews/documentdb/localQuickStart/` @@ -14,866 +14,832 @@ - **Related design docs:** [local-quickstart-v2.md](../../local-quickstart/local-quickstart-v2.md), [local-quickstart-poc/](../local-quickstart-poc/), [local-quickstart-multi-instance/](../local-quickstart-multi-instance/) -- **Scope of this doc:** the UX-facing surface (tree structure, wording, icons, the provisioning - webview, lifecycle actions, error recovery). Deeper backend/state-machine internals are only - discussed where they explain a user-visible symptom. +- **Scope:** the UX-facing surface (tree structure, wording, icons, the provisioning webview, + lifecycle actions, error recovery). Backend/state-machine internals appear only where they explain + a user-visible symptom. - **Review date:** 2026-07-09 ## How this review was run -This is a **runtime UX pass**: a person exercised the real feature (install → run → stop → delete → -break-it-externally) and dictated observations, and an AI assistant did the code-checking, root-cause -tracing, and write-up. The intent is that each finding is backed by the exact code path that produces -the behavior, so a later triage/implementation pass doesn't have to re-derive it. - -The sections below are organized **by user journey** (upgrade, first run, the setup webview, the -running instance, destructive actions, lifecycle robustness, possible additions) rather than by the -order the feedback happened to arrive. Each item leads with a one-line **Verdict**, then records the -**Observation** (what the reviewer saw), the **Finding** (what the code actually does and why), and a -**Suggestion** (a concrete, reactable recommendation — not a decision). Heavier design questions with -real trade-offs are pulled into [§13 Open ideas](#13-open-ideas--options-pros--cons) at the end. +A person exercised the real feature (install → run → stop → delete → break-it-externally) and dictated +observations; an AI assistant did the code-checking, root-cause tracing, and write-up. Each finding is +backed by the exact code path that produces the behavior, so a later implementation pass doesn't have +to re-derive it. Items are grouped and ordered **by priority**; each carries an **Observation** (what +the reviewer saw), a **Finding** (what the code does and why), a **Suggestion**, and a **Status**. +Heavier design questions with real trade-offs are pulled into [Open ideas](#open-ideas--options-pros--cons). ## Legend -| Marker | Meaning | -| ------------------ | --------------------------------------------------------------------------- | -| ✅ **Fix applied** | A quick, low-risk change was made on this branch and verified | -| ⚠️ **Flag** | Confirmed gap or bug; needs a decision/fix in a follow-up | -| 💡 **Suggestion** | A design/wording recommendation for the team to react to | -| 🔍 **Answered** | A "how does this work?" question, answered from the code (no change needed) | +### Priority ---- +| Priority | Meaning | +| -------- | -------------------------------------------------- | +| **P0** | Blocking — the user gets stuck | +| **P1** | Broken / misleading, or a consistency & safety gap | +| **P2** | Polish, expectation, or a smaller feature gap | +| **P3** | Nice-to-have / cosmetic / acknowledged | -## 1. The story in one paragraph +### Status -Local Quick Start adds a **`DocumentDB Local - Quick Start`** root node to the Connections view. From -an empty state a user clicks one row to open a **setup webview**, which checks Docker, pulls the -image, creates + starts a container, waits for readiness, and seeds sample data — reporting progress -as a stage checklist. Once running, the instance appears **inline** in the tree as a browsable -cluster with a Quick-Start-specific context menu (Start/Stop/Restart/Delete/Copy/View Logs). On -upgrade, pre-existing local "emulator" connections are migrated once into a regular folder. This -review hammered the _presentation and robustness_ of that journey: how the migration folder is named -and sorted, how the empty-state rows read, how honest and stable the setup webview's header/progress -is, how the running instance's menu and icons compare to regular clusters, whether destructive -actions match the rest of the extension, and — the one hard bug — what happens when the container is -deleted **outside** VS Code. Most items are wording/consistency polish; one (§4.2 header) is a real -"stuck UI" bug; §9 collects the harder robustness failures — a silent no-op on Start after an external -delete, and a **restart dead end** when saved credentials go missing (hit live during the review) that -also exposed a broader anti-pattern: **tree nodes used as error dialogs**. §8 adds the data/volume -model questions (ephemeral toggle, delete-drops-volume, image-linked volumes, delete-only-our-containers). +| Status | Meaning | +| ------------------- | ------------------------------------------------------------------------------------------- | +| 🟠 **Open** | Recorded + analyzed; **the operator must choose the direction** (see the item's Suggestion) | +| 🟡 **Acknowledged** | Known and accepted; not planned now — may be revisited | +| ✅ **Implemented** | A change was made on this branch and verified | ---- +### Markers (inline) -## 2. Upgrade & migration (existing users) +| Marker | Meaning | +| ----------------- | ------------------------------------------------------- | +| ⚠️ **Flag** | Confirmed gap or bug | +| 💡 **Suggestion** | A design/wording recommendation to react to | +| 🔍 **Answered** | A "how does this work?" question answered from the code | -### 2.1 Legacy emulator connections migrate to a folder that can hide in the sort order ⚠️ 🔍 +> **For the operator:** almost everything below is **Open**. Each item states a recommended direction +> in its **Suggestion** (and, where there are real trade-offs, an entry under +> [Open ideas](#open-ideas--options-pros--cons)). Pick a direction per item; the review does not decide +> for you. Exactly **one** item is already **Implemented** (item 30). One is **Acknowledged** (item 27). -**Observation:** Is the one-time migration of original "DocumentDB Local" emulator connections still -happening? What's the folder called? Sorting could place it somewhere unexpected and cause confusion. +--- -**Finding:** +## The story in one paragraph -- 🔍 Yes, it's still active. The migration lives in - [legacyEmulatorMigration.ts](../../../../src/services/legacyEmulatorMigration.ts), invoked from - [ClustersExtension.ts](../../../../src/documentdb/ClustersExtension.ts) on every activation - (`migrateLegacyEmulatorConnections`), guarded by a `globalState` flag - (`documentdb.localQuickStart.legacyEmulatorMigration.completed`) so it runs once. -- 🔍 The destination folder is named **`Local Connections (Legacy)`** (`LEGACY_FOLDER_BASE_NAME`), - deterministic id `vscode-documentdb.legacyLocalConnectionsFolder`. If a user already has a root - folder with that exact name, it's suffixed `(2)`, `(3)`, … It is a **normal** renamable/movable - folder under the regular Clusters zone — not pinned or reserved. Emulators-zone sub-folders are - intentionally **flattened** into this single folder (documented scope cut, not a bug). The old - `Emulators` zone/`LocalEmulatorsItem` node is kept as a read-only rollback until migration - succeeds, then retired (`isLegacyEmulatorMigrationComplete()` gate in - [ConnectionsBranchDataProvider.ts](../../../../src/tree/connections-view/ConnectionsBranchDataProvider.ts)). -- ⚠️ **Why it can surprise:** root items are assembled in a fixed order (Quick Start node → legacy - emulator node if un-migrated → **all folders, alphabetically** → ungrouped connections → New - Connection). Folders sort with a single `localeCompare(..., { numeric: true })` pass with **no - special-casing** for the legacy folder, so it lands wherever "L…" falls among the user's other - folder names — easy to miss right after an upgrade, since nothing marks it as "your old stuff - moved here." No migration toast surfaced in this pass (a search for a notification tied to - `MIGRATION_COMPLETED_KEY` found none). - -💡 **Suggestion:** Make the destination discoverable rather than relying on alphabetical luck. Cheapest -first: (a) a **one-time toast** after a successful migration ("N local connections were moved to -_Local Connections (Legacy)_") with a "Reveal" action; optionally (b) a distinct folder **description -badge/icon** so it stands out; and/or (c) pin it to a fixed position among folders (first or last). -See [§13.1](#131-surfacing-the-legacy-migration-folder) for the options weighed. +Local Quick Start adds a **`DocumentDB Local - Quick Start`** root node to the Connections view. From +an empty state a user clicks one row to open a **setup webview**, which checks Docker, pulls the image, +creates + starts a container, waits for readiness, and seeds sample data — reporting progress as a +stage checklist. Once running, the instance appears **inline** in the tree as a browsable cluster with +a Quick-Start-specific context menu (Start/Stop/Restart/Delete/Copy/View Logs). On upgrade, pre-existing +local "emulator" connections are migrated once into a regular folder. This review hammered the +_presentation and robustness_ of that journey. The two **P0** bugs are lifecycle dead ends (a +restart-time credential dead end and a silent no-op on Start after an external delete). The **P1** set +is the real header bug, two error-presentation anti-patterns (tree nodes and the webview hero both used +to display errors), plus destructive-action consistency and safety. The rest is polish, expectation, +data-model, and enhancement work. See [Appendix A](#appendix-a--current-webview-flow-reference) for the +webview flow. --- -## 3. First run — the empty Quick Start node +## Priority index + +| # | Priority | Item | Status | +| --- | -------- | ----------------------------------------------------- | --------------- | +| 1 | **P0** | Credential-missing state is a restart dead end | 🟠 Open | +| 2 | **P0** | External container delete → silent no-op on Start | 🟠 Open | +| 3 | **P1** | Webview header stuck on "Setting up…" | 🟠 Open | +| 4 | **P1** | Tree nodes misused to display errors | 🟠 Open | +| 5 | **P1** | Error/status messages hijack the hero | 🟠 Open | +| 6 | **P1** | "Delete deletes data" vs "recreate/recover" clash | 🟠 Open | +| 7 | **P1** | Copy Connection String silently copies the password | 🟠 Open | +| 8 | **P1** | Delete bypasses the shared confirmation (+ em dashes) | 🟠 Open | +| 9 | **P1** | Only ever delete containers we created | 🟠 Open | +| 10 | **P2** | Legacy migration folder hides in the sort | 🟠 Open | +| 11 | **P2** | Phase out the "local connection" concept | 🟠 Open | +| 12 | **P2** | "Checking Docker" is a bare spinner | 🟠 Open | +| 13 | **P2** | Advanced: creds toggle + ambiguous placeholders | 🟠 Open | +| 14 | **P2** | "Cancel" implies a rollback it doesn't do | 🟠 Open | +| 15 | **P2** | No "What was done" summary on completion | 🟠 Open | +| 16 | **P2** | No upfront "first run takes a few minutes" | 🟠 Open | +| 17 | **P2** | "Load sample data" is all-or-nothing | 🟠 Open | +| 18 | **P2** | "Waiting…" static for minutes | 🟠 Open | +| 19 | **P2** | No image-pull progress | 🟠 Open | +| 20 | **P2** | Running row lacks regular cluster commands | 🟠 Open | +| 21 | **P2** | Credential storage is a bespoke plainer path | 🟠 Open | +| 22 | **P2** | Always persistent — no ephemeral option | 🟠 Open | +| 23 | **P2** | User delete must always drop the volume | 🟠 Open | +| 24 | **P2** | Volumes not structurally linked to the image | 🟠 Open | +| 25 | **P2** | Container keeps running after VS Code closes | 🟠 Open | +| 26 | **P2** | Tree state changes not announced (accessibility) | 🟠 Open | +| 27 | **P3** | Docker prereq only revealed after the click | 🟡 Acknowledged | +| 28 | **P3** | Generic `$(plug)` icon, no distinct identity | 🟠 Open | +| 29 | **P3** | "Open terminal in the container" | 🟠 Open | +| 30 | **P1\*** | Empty-state tree row reworded / "Learn more" dropped | ✅ Implemented | + +> \* Item 30 was a P1-ish wording nit; it's the one item already shipped on this branch, so it's parked +> at the end under Implemented. -### 3.1 Empty-state tree rows: reword the action, drop "Learn more" ✅ +--- -**Observation:** The rows shown when there's no managed instance yet should be reworded — the action -row should read like "Click here to…" — and "Learn more" could be dropped from the list (it could -live in the node's context menu instead). +## P0 — Blocking (the user gets stuck) -**Finding:** Both rows are built in `LocalQuickStartItem.getChildren()` -([LocalQuickStartItem.ts](../../../../src/tree/connections-view/LocalQuickStart/LocalQuickStartItem.ts)) -for the `NotInstalled` state: an action row (rocket, `treeItem_quickStartAction`, opens the webview) -and a separate "Learn more…" row (`link-external`, `treeItem_quickStartLearnMore`, opens -`https://github.com/microsoft/documentdb`). Neither row has any `view/item/context` entry in -`package.json`, so there is **no existing context-menu** to move "Learn more" into — that would be new -plumbing. The action label/command is also reused by the `Missing` state (see §9), so wording should -stay consistent across both. +### 1. Credential-missing state after a restart is a UI dead end (the demo incident) ⚠️ -✅ **Fix applied (2026-07-09):** Implemented the quick half of this directly, in `getChildren()`'s -`NotInstalled` branch: +**Priority:** P0 · **Status:** 🟠 Open — operator to choose the direction (see Suggestion + Open idea O4). -- Action row relabeled `'Quick Start — Install & try DocumentDB locally'` → - **`'Click here to install & try DocumentDB locally'`**. -- The `'Learn more…'` row (`treeItem_quickStartLearnMore`) was **removed** from the empty state. It is - **not** relocated to a context menu (deferred — no menu plumbing exists yet), so the external repo - link is no longer reachable from the tree until/unless it's re-added. +**Observation (real incident, mid-review):** An instance was created via Quick Start, then stopped, +then its container/image were removed outside VS Code. After a **VS Code restart** the Quick Start node +showed the rocket _"Click here to start DocumentDB Local"_ row **plus** a warning row _"DocumentDB Local +has data on disk but its saved credentials are missing…"_ (truncated in the tree). There was **no way +from the UI** to delete the leftover container/volume or to start cleanly — no Delete node, no recreate +action. Recovery required manually removing the container and its volume with Docker, then refreshing. -💡 **Suggestion (remaining):** Decide whether "Learn more" comes back as a context-menu entry on the -parent `DocumentDB Local - Quick Start` node (a new `view/item/context` gated on -`treeItem_localQuickStart`), or is dropped for good in favor of the walkthrough/README. Low priority. +**Finding:** ---- +- On activation, `reconcile()` → `reconcileAlias()` hits **"Case 4"**: a labelled container (or a + `ready` record) exists but the stored secret is unrecoverable, so it calls + `setStatus(alias, InstanceState.Error, undefined, CREDENTIAL_UNAVAILABLE_MESSAGE)` with + **`metadata: undefined`**. By design it **never** removes the container or volume (R2 data-safety: a + lost secret doesn't prove the volume is disposable — the user must choose the wipe). +- Because `metadata` is `undefined`, `LocalQuickStartItem.getChildren()` skips every `metadata && …` + branch and falls through to the **NotInstalled** branch, which renders the rocket action row **plus** a + passive `treeItem_quickStartError` warning row carrying the message. +- ⚠️ That warning row has **no command and no context menu**, and the Delete command's `when` requires + `treeItem_quickStartInstance` + a `state_*` token — which this surfaced state never carries. So + **Delete is unreachable**, and unlike the true `Missing` row (which at least says "click to recreate") + no recovery action is offered. The user is genuinely stuck. +- ⚠️ **Restart behavior specifically:** in-memory state is rebuilt from durable state on every reload, so + this dead end **reproduces on every restart** — not a transient glitch. Clicking the rocket to "start + fresh" doesn't help either: the on-disk volume + missing credentials block a clean provision, and + item 2's silent-return path compounds it. +- **How we recovered (runbook):** removed the labelled container and its data volume directly with Docker + (`docker rm -f vscode-documentdb-local` + `docker volume rm vscode-documentdb-local-data`), then + refreshed; reconcile found nothing and returned to the clean NotInstalled empty state. -## 4. The setup webview — flow, loading & header +💡 **Suggestion:** The credential-unavailable state must be **actionable**, not a passive warning. Render +it like the `Missing` row — a real instance row (`treeItem_quickStartInstance` + a dedicated +`state_credentialsMissing` token) that exposes **Delete Container** (clean slate: container + volume + +stale records, via `deleteContainer()`, cf. item 23) and, where safe, **Recreate**. Keep R2 (never +_auto_-wipe) — but give the user a one-click way to _choose_ the wipe. Shares the recovery UX in Open +idea O4; a direct instance of item 4's principle. -### 4.1 Current flow — what the webview shows, and when 🔍 +### 2. A container deleted _outside_ VS Code isn't handled on Start (silent no-op) ⚠️ -**Observation:** It should be clear what the setup webview renders at each step, so reviewers know -which screen a finding refers to. +**Priority:** P0 · **Status:** 🟠 Open — operator to choose the direction (see Suggestion + Open idea O4). -**Finding:** The panel is one React component driven by a `Phase` state machine -(`'loading' | 'review' | 'dockerNotReady' | 'provisioning' | 'success' | 'failed'` in -[LocalQuickStart.tsx](../../../../src/webviews/documentdb/localQuickStart/LocalQuickStart.tsx)). On -open it queries `getDockerStatus`, then branches: +**Observation (repro):** Created an instance via Quick Start, stopped it, then deleted the _container_ +directly via Docker. Back in VS Code, clicking Start on the still-"Stopped"-looking row does nothing +visible — no dialog, no tree update — only the raw Docker error appears in the output channel: -```text - panel opens - | - v - +-----------------+ getDockerStatus() - | loading |------------------------------. - | (BARE spinner: | | - | "Checking | Docker not ready v - | Docker..." | +----------------------------+ - | no hero/cards) |--------->| dockerNotReady | - +-----------------+ | hero "Docker is required" | - | | 3 readiness cards | - Docker ready | (CLI / daemon / platform) | - | | + "How to fix" + [Retry] ---+ (re-query) - | +----------------------------+ - | - ready & canResumeReadiness? --- yes --> jump to [failed / timedOut] (resume a prior - | timed-out container: Wait longer / Start over) - | no - v - +-----------------+ [Start DocumentDB Local] - | review |----------------------------. - | hero "Start | | - | DocumentDB | v - | Local" | +----------------------+ - | 4 config cards | | provisioning | - | (Image / Port / | | hero "Setting up..." | - | Data / Security| | elapsed timer | - | + Advanced | <-- [Edit | stage checklist: | - | (collapsed)) | settings] --| checking > pulling > | - | + [Start] | | creating > starting >| - +-----------------+ | waiting > done | - ^ | [Cancel] [View output]| - | +----------+-----------+ - | [Start over] success | | failed / timeout - | v v - | +-----------+ +----------------------+ - +------------------- | success | | failed | - | Next steps| | error box | - | [Open] [Copy] | [Retry][Edit] OR | - | [Close] | | [Wait longer][Start | - +-----------+ | over] (on timeout) | - +----------------------+ ``` +$ docker container inspect --format '{{json .}}' 57f8311fd9ec… +Error response from daemon: No such container: 57f8311fd9ec… +``` + +**Finding — a specific, reproducible gap:** + +- `ContainerRuntime.inspectContainer()` wraps the CLI call in `try { … } catch { return undefined; }`, + swallowing "No such container" into `undefined`. But the shared `makeRunner()` still logs the command + and streams the daemon's stderr to the channel — that's exactly the two lines the user saw (the + extension's own inspect call, not a manual terminal session). +- `start()` calls `isManaged(id)` first, which returns `false` both when the container **isn't ours** + _and_ when it's **gone** (`inspectContainer` → `undefined`). ⚠️ **`isManaged()` cannot distinguish + "missing" from "not ours."** +- `start()`'s guard is `if (!id || !isManaged || !liveStateGuard(…)) return;`. Because `isManaged` already + returned `false`, the `||` short-circuits and **`liveStateGuard()` never runs** — the very code that + _would_ have handled this gracefully (it detects a `'missing'` live state, calls `refreshLiveState()`, + and shows an info message). That path is well-built for the multi-window / external-stop case; it's just + unreachable here. +- Net effect: `start()` early-returns silently, `setStatus` never fires, the tree never refreshes, and the + stale "Stopped" row persists until something else (e.g. collapsing/expanding the node) triggers + `refreshLiveState()` independently. +- There's already a good pattern for the related case: `refreshLiveState()` _does_ set `entry.missing = true` + and the tree renders `Missing · click to recreate`. The gap is that the **lifecycle actions** + (`start`/`stop`/`restart`) don't funnel through that missing-detection before giving up. + +💡 **Suggestion:** Teach `start`/`stop`/`restart` to distinguish "no container found" from "found but not +ours" (a small change to `isManaged`, or a check ahead of it) and, on "missing," do what `liveStateGuard` +already does — refresh state to the `Missing` badge and surface a message with next steps: **Delete** and/or +**Recreate with same settings** (the existing `Missing` recreate flow + `getReusableCredentials()` likely +covers most of it — confirm once reachable). Options in Open idea O4. -Phase-by-phase, what is actually rendered: +--- + +## P1 — Broken / misleading, or consistency & safety -- **loading** — **only** a full-panel ``. No hero/title, no cards - — the whole panel is the spinner until `getDockerStatus` resolves, then it snaps to the next - layout (see §4.3). -- **dockerNotReady** — `hero("Docker is required")` + **three** readiness cards (Docker CLI / Docker - daemon / Platform, each with a ✓/! badge) + a "How to fix" card + `Retry`. -- **review** — `hero("Start DocumentDB Local")` + **four** config cards (Image / Port / Data / - Security) + a summary + a collapsed **Advanced** panel + `Start DocumentDB Local`. -- **provisioning** — `hero("Setting up DocumentDB Local…")` + elapsed timer + the stage checklist - (checking → pulling → creating → starting → waiting → done) + `Cancel` + `View Docker output`. -- **success** — header still reads "Setting up…" (bug, §4.2) + a success box + Next steps + - `Open Connection` / `Copy Connection String` / `Close`. -- **failed** — header still reads "Setting up…" (§4.2) + an error box + either `Retry` / `Edit -settings`, or (on a readiness timeout) `Wait longer` / `Start over`. +### 3. The webview header changes with state and gets stuck on "Setting up…" ⚠️ -### 4.2 The webview header changes with state and gets stuck on "Setting up…" ⚠️ +**Priority:** P1 · **Status:** 🟠 Open — operator to choose the direction. -**Observation:** The header should be static. Today it changes with status/progress/errors, which is -unexpected — and it sometimes stays on "Setting up DocumentDB Local…" even when setup is actually -complete. +**Observation:** The header should be static. Today it changes with status/progress/errors, and it +sometimes stays on "Setting up DocumentDB Local…" even when setup is actually complete. **Finding** (in [LocalQuickStart.tsx](../../../../src/webviews/documentdb/localQuickStart/LocalQuickStart.tsx)): -the header ("hero") is rendered by a local `hero(title, subtitle)` helper called with **three -different literal titles** depending on `phase`: `'Docker is required'` (dockerNotReady), -`'Setting up DocumentDB Local…'` (**provisioning, success, _and_ failed all share this one call**), -and `'Start DocumentDB Local'` (review). Both halves of the observation are confirmed: +the header ("hero") is rendered by a local `hero(title, subtitle)` helper called with **three different +literal titles** depending on `phase`: `'Docker is required'` (dockerNotReady), `'Setting up DocumentDB +Local…'` (**provisioning, success, _and_ failed all share this one call**), and `'Start DocumentDB Local'` +(review). Both halves of the observation are confirmed: - ⚠️ The title genuinely changes across phases (three strings), reading as a jumpy header. - ⚠️ **The "stuck" complaint is a real bug:** `success` and `failed` reuse the `provisioning` title verbatim, so "Setting up DocumentDB Local…" is still shown even once the instance is running or has - definitively failed. Only the body below (success/error box, stage checklist, buttons) reflects the - outcome — the header never catches up. (A phase/stage-aware `provisioningStatusMessage` exists but - is only piped to a screen-reader-only live region, not shown visually.) + definitively failed. Only the body below reflects the outcome — the header never catches up. (A + phase/stage-aware `provisioningStatusMessage` exists but is only piped to a screen-reader-only live + region, not shown visually.) 💡 **Suggestion:** Make the header a **single static string** for the whole panel lifecycle (e.g. -"DocumentDB Local" or "Local Quick Start"), and let the subtitle/body carry all state (elapsed timer, -success/failure boxes already exist). That satisfies "header should be static" and eliminates the -stuck-text bug in one move. If a dynamic header is preferred, it must at minimum branch on -`success`/`failed` the way the `dockerNotReady` screen already does. +"DocumentDB Local"), and let the subtitle/body carry all state (elapsed timer, success/failure boxes +already exist). That satisfies "header should be static" and eliminates the stuck-text bug in one move. If +a dynamic header is preferred, it must at minimum branch on `success`/`failed` the way `dockerNotReady` +already does. + +### 4. Tree nodes are misused to display errors (they should be actions, not dialogs) ⚠️💡 -### 4.3 The initial "Checking Docker" loading should render chrome + skeleton cards (reuse the metric card) ⚠️💡 +**Priority:** P1 · **Status:** 🟠 Open — operator to choose the direction. -**Observation:** The initial `loading` ("Checking Docker") state should be reworked to behave like the -**Query Insights** view: the webview chrome renders immediately (title + header/hero), and the four -cards behave like our **metrics cards** — they show a **loading skeleton** while status resolves, then -fill **independently** (at least from a UX perspective; we don't care about splitting the backend now, -that's for later). Worth **reusing the existing metric card** — it's accessible and has tooltip -support, a better experience. +**Observation:** A broader pattern problem exposed by the incident (item 1) — we render an **error message +as a tree node**. Leaf/error nodes should be **actions the user can take**, not a substitute for a dialog +or a status surface. **Finding:** -- ⚠️ The `loading` phase renders **only** `` inside an otherwise - empty panel — no hero/title, no cards (§4.1). When `getDockerStatus` resolves, the panel **snaps** - from a bare spinner to the full `dockerNotReady`/`review` layout, which is the jump the observation - describes. Query Insights, by contrast, renders its header + metric row up front and lets the - individual metrics resolve into place. -- 🔍 The readiness/config cards use a **local, simplified** `MetricCard` defined inline in - [LocalQuickStart.tsx](../../../../src/webviews/documentdb/localQuickStart/LocalQuickStart.tsx) - (a plain `Card` with label + value + optional badge) — **no skeleton, no tooltip, weaker a11y.** -- 🔍 Query Insights already ships a purpose-built, accessible metric card: **`MetricBase`** at - [MetricBase.tsx](../../../../src/webviews/documentdb/collectionView/components/queryInsightsTab/components/metricsRow/MetricBase.tsx). - It supports a **loading skeleton** (`value === undefined` → `SkeletonItem`), a **null/unavailable** - placeholder, **tooltip explanations** (`tooltipExplanation`), and a solid **accessibility** pattern - (keyboard-focusable card, an `aria-label` carrying label + value + tooltip, `aria-hidden` children - to avoid double announcement). That's exactly the skeleton + tooltip + a11y behavior the - observation asks for. +- Several Quick Start states render a passive, non-actionable row purely to display text: + `treeItem_quickStartError` (the credential-unavailable / error message, item 1), and the empty state also + **appends** an error row when `status.state === Error && errorMessage`. These rows have no command and no + menu — they exist only to show a string, which then **truncates** in the tree (the incident's "…saved + credentials are miss…"), can't be copied, and competes visually with the real action row. +- This contradicts where the rest of the extension landed. The **Kubernetes discovery** review reached + exactly this conclusion and reversed it: classified in-tree error-summary nodes were **removed** in favor + of a **modal** (`showErrorMessage(…, { modal: true })`) plus a single actionable retry node. Quick Start + currently repeats the anti-pattern the K8s feature already retired. + +💡 **Suggestion:** Adopt the same rule feature-wide: **tree rows are actions; errors are dialogs / status.** +(a) surface an error/credential-unavailable condition as a **modal** (detail in the output channel), and +(b) keep **only actionable rows** in the tree (Delete / Recreate / Retry / "Click here to…"). This removes +the passive `treeItem_quickStartError` and the error-append row and directly fixes item 1's dead end. It +also rhymes with item 3 (the webview header carrying status it shouldn't) — the same "don't overload one +surface with state it isn't meant to hold" theme. + +### 5. Error/status messages must leave the hero — show a content-area card; run a small UX experiment ⚠️💡 + +**Priority:** P1 · **Status:** 🟠 Open — operator to choose the direction (a UX experiment is requested). + +**Observation:** We have error/status states (e.g. Docker missing). The core requirement: these messages +**must not live in the hero section** — the hero stays constant and the message becomes a **card in the +content area** with the actions the user can perform. Query Insights already has good building blocks — not +just the `MessageBar`, but the **richer composed card** shown when an RU user opens the Query Insights tab; +the two **could be combined**. Because it isn't obvious which shape reads best, whoever implements this +should **render a few options to compare**. We also need to decide **where Retry lives**. -💡 **Suggestion:** +**Finding:** -1. Rework `loading` so it renders the **hero/header immediately** (like every other phase, and like - Query Insights), then renders the readiness cards in **skeleton** state, each flipping to its - value independently as status resolves — no need to split the backend; from the UX side a card can - go skeleton → value on its own. -2. **Reuse `MetricBase`** instead of the local `MetricCard`. It's accessible and has tooltip + - skeleton support out of the box — a better experience than the inline card. Two bits of work: (a) a - small **extension** (e.g. a badge slot for the ✓/! status badge the readiness cards use), and (b) - **relocation to a shared** webview location — it currently lives buried under - `collectionView/queryInsightsTab/`, and importing a query-insights-internal component into the - Quick Start webview is the wrong dependency direction. The move + small extension is worth it: one - accessible, tooltip-capable metric card shared across features. - -### 4.4 Advanced panel: credentials should sit behind a toggle, and "placeholder = default" is ambiguous ⚠️💡 - -**Observation:** In the Advanced section, username + password should be grouped **behind a toggle** — -the user flips "override the default" and only then specifies a username and password. The current -ghost text **"auto"** is confusing (is it the default _value_?). More generally, the panel label says -_"Leave any field blank to keep the automatic default,"_ yet the **Image tag** field shows **"latest"** -as ghost text — so how is the user meant to "keep it blank"? The person who addresses this should -**think through the options**. - -**Finding** (Advanced panel in -[LocalQuickStart.tsx](../../../../src/webviews/documentdb/localQuickStart/LocalQuickStart.tsx), -`renderAdvanced()`): - -- On a fresh provision (`!isRecreate`) the panel renders Port, Image tag, Username, Password (each - always visible) + a "Load sample data" switch, under the label _"Leave any field blank to keep the - automatic default."_ -- ⚠️ **Placeholder means two different things across fields.** Port's placeholder is the literal - default value (`10260`), Image tag's is the literal default tag (`latest`) — but Username/Password - use the word **`'auto'`**, which is a _description of behavior_ ("will be auto-generated"), not a - value you'd type. So "latest" reads as if a value is already set, while "auto" reads as a mode. - Combined with "leave blank to keep the default," the greyed-in `latest` makes it genuinely unclear - whether the field is pre-filled or empty — the exact confusion the observation calls out. -- 🔍 The credentials already follow a **both-or-neither** rule in `advValidation` - (_"Enter both a username and a password, or leave both blank to auto-generate."_). A toggle would - encode that rule structurally instead of leaving it to a validation error after the fact. -- 🔍 On a recreate (`isRecreate`) the credential/tag fields are already hidden and replaced by a note - that the original credentials/image are reused — so this only concerns the fresh-provision path. +- ⚠️ Today an error is handled by switching to a **dedicated phase that replaces the hero**: `dockerNotReady` + swaps the hero to _"Docker is required"_ and renders a bespoke full-screen layout (three readiness cards + + a "How to fix" card + a bottom `Retry`); the `failed` phase similarly shows an error box with the hero + **stuck** on _"Setting up…"_ (item 3). So the message is **coupled to the hero**. +- 🔍 **Query Insights ships two reusable shapes**, both leaving the header/title untouched: + - **Concise inline `MessageBar`.** + [`GetPerformanceInsightsCard.tsx`](../../../../src/webviews/documentdb/collectionView/components/queryInsightsTab/components/optimizationCards/custom/GetPerformanceInsightsCard.tsx) + renders a Fluent `MessageBar` when `errorMessage` is set and **relabels its primary button to "Retry"**: + `{errorMessage ? l10n.t('Retry') : l10n.t('Get AI Performance Insights')}`. The card title never changes. + [`ImprovementCard.tsx`](../../../../src/webviews/documentdb/collectionView/components/queryInsightsTab/components/optimizationCards/ImprovementCard.tsx) + uses the same `MessageBar intent="warning"` / `"success"` blocks for per-action state. + - **Richer composed card (the RU-tab card).** + [`QueryInsightsTab.tsx`](../../../../src/webviews/documentdb/collectionView/components/queryInsightsTab/QueryInsightsTab.tsx) + (the `QUERY_INSIGHTS_PLATFORM_NOT_SUPPORTED_RU` branch) renders a **`MarkdownCardEx`** — icon + title + + markdown body + a call-to-action — and **nests a `MessageBar` inside it** for the concise status line. So + "card" and "message bar" are **already combined** there. 💡 **Suggestion:** -1. **Group credentials behind a toggle.** Add an "Override generated credentials" (or "Set my own - username & password") switch, **off** by default. Off → show a short read-only note - ("Auto-generated, stored securely," matching the summary card); On → reveal both fields, required - together (reuses the existing both-or-neither validation). This removes the ambiguous `'auto'` - ghost text entirely and makes "I want to override" an explicit, discoverable action. -2. **Resolve the "placeholder = default value" ambiguity** — options for whoever implements it to weigh - (the goal: a user can always tell "blank = default" from "I typed a value"): - - **A. Drop literal-value placeholders.** Leave the input visually empty (or a neutral - "Default") and keep the real default only in the `hint` line under the field (the panel already - renders _"Default {port}"_ / _"Default “latest”"_ / _"Default: auto-generated"_). Then "leave - blank = default" is honest: the box looks empty, the hint states the default. Cheapest and most - consistent. - - **B. Pre-fill the actual default value** (not a placeholder) and let the user edit it. No "blank" - concept at all — what you see is what runs. But then the "leave blank" label must go and empty - must be re-validated/normalized. - - **C. Per-field "Use default" affordance** (checkbox/toggle per field). Most explicit, but heavier - and busier than A for a panel that already has one toggle model in suggestion 1. - - **Leaning A** for consistency + minimal work, paired with suggestion 1 so credentials leave the - grid entirely. Whichever is chosen, apply the **same** convention to every Advanced field so - placeholder never sometimes-means-value and sometimes-means-mode. - -### 4.5 Error/status messages must leave the hero — show a content-area card; run a small UX experiment ⚠️💡 - -**Observation:** We have error/status states (e.g. Docker missing). The core requirement: these -messages **must not live in the hero section** — the hero stays constant and the message becomes a -**card in the content area** with the actions the user can perform. Query Insights already has good -building blocks for this — not just the `MessageBar`, but the **richer composed card** shown when an -RU user opens the Query Insights tab; the two **could be combined**. Because it isn't obvious which -shape reads best, whoever implements this should **render a few options to compare** (a UX -experiment), and we also need to decide **where the Retry action lives** — there's no good home for it -in today's model. +1. **Get the message out of the hero.** Keep the hero **static** (item 3) and render error/status as a + **content-area card**, never as a hero swap or a full-screen phase. Applies to docker-not-ready and `failed`. +2. **Combine the two shapes, and experiment.** Render a few candidate variants to compare, e.g.: **A** plain + `MessageBar` + inline Retry (lightest); **B** composed card (`MarkdownCardEx`-style: icon + title + body + + actions); **C** combined (composed card **containing** a `MessageBar`, matching the RU tab). Test A/B/C + against the real states (Docker missing, provisioning failed, readiness timeout) and pick per-state. +3. **Retry placement:** put `Retry` **on the chosen card**, next to the other recovery actions, following the + Query Insights convention of relabeling the card's primary action to "Retry." The retry logic already + exists (`loadDockerStatus` / `handleStart`); only its home changes. +4. Pairs with item 4 (tree side) — the same principle applied to the webview. + +### 6. "Delete deletes data" vs "Recreate / recover" — resolve the contradiction ⚠️💡 + +**Priority:** P1 · **Status:** 🟠 Open — operator to choose the direction (a discussion is requested). + +**Observation:** We tell the user that **deleting deletes the data** — so then what does **recreate / +recovery** mean? The two messages appear to contradict each other. + +**Finding / the tension:** + +- **Delete Container** is a permanent wipe: it removes the container **and the data volume** and all stored + creds/records (items 8 / 23) — "this cannot be undone." +- The recovery flows for `Missing` / credential-unavailable (items 1 / 2) offer **"Recreate with same + settings,"** meaningful only because the **data volume is kept** and stored credentials are reused + (`getReusableCredentials`). "Recreate" = "the container vanished but your data on the volume survived; + rebuild the container onto it." +- Both are true but the vocabulary collides: **Delete** = destroy the data (no recovery); **Recreate** = + recover _because_ the data was not deleted (the container went away, not the volume). + +💡 **Discussion / suggestion:** Make the model explicit by separating **container** from **data**: + +- **Option (a) — one Delete that always wipes both (current).** Then "recreate" appears **only** when the + _container_ was lost externally but the volume remains, and its copy must say _"your data on disk is intact + — this rebuilds the container onto it,"_ never implying it undoes a Delete. +- **Option (b) — split the verbs:** **Remove container (keep data)** vs **Delete everything (container + + data).** Then "recreate" pairs naturally with the keep-data path; no contradiction. Cleaner, more surface. + +> Whichever is chosen, never present "recreate / recover" and "deleting deletes your data" as if they act on +> the **same** object. Pairs with Open idea O4 (recovery affordance) and O6 (volume model). + +### 7. "Copy Connection String" is a separate impl and silently includes the password ⚠️ + +**Priority:** P1 · **Status:** 🟠 Open — operator to choose the direction. + +**Observation:** Copy Connection String seems reimplemented for Quick Start and behaves slightly differently +— it doesn't ask whether to include the password. + +**Finding:** Regular clusters use `copyConnectionString()` +([copyConnectionString.ts](../../../../src/commands/copyConnectionString/copyConnectionString.ts)), which +prompts via `showQuickPick` ("with password" vs "without password", gated by `canIncludeNativePassword()`) +and is well-tested. Quick Start uses a **separate, shorter** `copyQuickStartConnectionString()` +([localQuickStartCommands.ts](../../../../src/commands/localQuickStart/localQuickStartCommands.ts)) that +copies `metadata.connectionString` (password already embedded) **with no prompt** — a genuine behavioral +difference. ⚠️ A plaintext password always lands on the clipboard silently, which stands out given how +careful the rest of the feature is about credentials (masked logging, env-file instead of CLI args). There +is a _separate_ `copyQuickStartPassword()` command, but the connection-string action itself never asks. + +💡 **Suggestion:** Reuse the regular command's QuickPick confirmation for parity (Quick Start always uses +native auth, so no extra branching is needed), or — if the extra click is unwanted for a one-click-local +flow — at minimum make the two commands' behavior a **conscious, documented** choice rather than an +incidental divergence. + +### 8. "Delete Container" bypasses the shared confirmation and uses em dashes ⚠️ + +**Priority:** P1 · **Status:** 🟠 Open — operator to choose the direction. + +**Observation:** The Delete Container modal is fine conceptually, but it should use the same delete +confirmation code path as other destructive actions (like in Settings), and shouldn't use em dashes. **Finding:** -- ⚠️ Today an error is handled by switching to a **dedicated phase that replaces the hero**: - `dockerNotReady` swaps the hero to _"Docker is required"_ and renders a bespoke full-screen layout - (three readiness cards + a "How to fix" card + a bottom `Retry` that calls `loadDockerStatus`); the - `failed` phase similarly shows an error box with the hero **stuck** on _"Setting up…"_ (§4.2). So the - message is **coupled to the hero** — exactly what the observation wants to stop. (A `Retry` _does_ - exist on the docker-not-ready screen, but only as part of that full-screen error phase, not as an - inline card action.) -- 🔍 **Query Insights already ships two reusable shapes** — both leave the surrounding header/title - untouched and put the message in the content area: - - **Concise inline `MessageBar`.** - [`GetPerformanceInsightsCard.tsx`](../../../../src/webviews/documentdb/collectionView/components/queryInsightsTab/components/optimizationCards/custom/GetPerformanceInsightsCard.tsx) - renders a Fluent **`MessageBar`** (`MessageBarBody` + `MessageBarTitle`) when `errorMessage` is set - and **relabels its primary button to "Retry"** — the same handler doubles as retry: - `{errorMessage ? l10n.t('Retry') : l10n.t('Get AI Performance Insights')}`. The card title never - changes. - [`ImprovementCard.tsx`](../../../../src/webviews/documentdb/collectionView/components/queryInsightsTab/components/optimizationCards/ImprovementCard.tsx) - uses the same `MessageBar intent="warning"` / `intent="success"` blocks for per-action state. - - **Richer composed card (the RU-tab card the observation refers to).** When an RU user opens Query - Insights, [`QueryInsightsTab.tsx`](../../../../src/webviews/documentdb/collectionView/components/queryInsightsTab/QueryInsightsTab.tsx) - (the `QUERY_INSIGHTS_PLATFORM_NOT_SUPPORTED_RU` branch) renders a **`MarkdownCardEx`** — an - icon + title (_"Query Insights Not Available"_) + a markdown body explaining _why_ and a - call-to-action (links to start a GitHub discussion) — and **nests a `MessageBar` inside it** for - the concise one-line status. So the "card" and the "message bar" are **already combined** there: - the composed card is the container, the `MessageBar` is the crisp summary line within it. +- ⚠️ Every other destructive command — [removeConnection](../../../../src/commands/removeConnection/removeConnection.ts), + [deleteCollection](../../../../src/commands/deleteCollection/deleteCollection.ts), + [deleteDatabase](../../../../src/commands/deleteDatabase/deleteDatabase.ts), + [folder delete](../../../../src/commands/connections-view/deleteFolder/ConfirmDeleteStep.ts) — calls + **`getConfirmationAsInSettings()`** ([getConfirmation.ts](../../../../src/utils/dialogs/getConfirmation.ts)), + which honors the user's `confirmationStyle` setting (type-the-word / number challenge / click). + `deleteQuickStartInstance()` instead calls **`getConfirmationWithClick()`** directly, ignoring that + setting — the only _destructive_ command that does (the other direct callers are non-destructive: + `hideIndex`/`unhideIndex`). +- ⚠️ Its two confirmation `detail` strings both contain a literal **em dash** ("…This cannot be undone — you + can recreate…"). The character also appears across the feature's other strings (`LocalQuickStart.tsx` + next-steps bullets, the item 18 timeout message). -💡 **Suggestion:** +💡 **Suggestion:** Switch Delete Container to **`getConfirmationAsInSettings()`** for consistency (decide the +confirmation word — a simple `delete` matches the other delete flows). Separately, do a **one-pass em-dash +sweep** of the whole feature's user-facing strings rather than fixing only the delete dialog. -1. **Get the message out of the hero.** Keep the hero **static** (per §4.2) and render error/status as - a **content-area card**, never as a hero-text swap or a `dockerNotReady`/`failed` full-screen phase - that hijacks the title. Applies to docker-not-ready and provisioning `failed`. -2. **Combine the two shapes, and experiment.** The RU card shows the pattern to reuse: a composed - titled card (icon + title + short explanation + actions) that **contains** a `MessageBar` for the - crisp status line. Because the right density isn't obvious, whoever implements this should **render - a few candidate variants to compare** rather than pick one blindly, e.g.: - - **Option A — plain `MessageBar`** with an inline `Retry` (lightest; good for transient errors). - - **Option B — composed card** (`MarkdownCardEx`-style: icon + title + body + action buttons), no - separate bar (richest; good for "Docker is required" with fix steps + links). - - **Option C — combined** (composed card **containing** a `MessageBar`), matching the RU tab. - Render A/B/C against the real states (Docker missing, provisioning failed, readiness timeout) and - pick per-state — a lightweight bar may suit `failed`, while docker-not-ready likely wants the richer - card with fix links. -3. **Retry placement:** put `Retry` **on the chosen card**, next to the other recovery actions (Install - Docker / Troubleshooting; Edit settings / Wait longer / Start over), following the Query Insights - convention of **relabeling the card's primary action to "Retry"** when an error is present — not a - separate bottom-of-panel button. The retry logic already exists (`loadDockerStatus` / `handleStart`); - only its _home_ changes. -4. This pairs with **§9.3** (tree side: errors are actions/dialogs, not passive nodes) — the same - principle on the webview: **a message is an actionable content card, not a state that hijacks the - hero.** A shared, reusable error/status card (composed card + optional `MessageBar` + primary/Retry - - links) would serve docker-not-ready and `failed` and keep the hero constant. - -### 4.6 What does "Cancel" do mid-provision? Consider the verb "Abort" (no full rollback promise) ⚠️💡 - -**Observation (work item):** Investigate what **Cancel** actually does when clicked while provisioning -is in progress. Do we **delete an image that has started downloading**, or just abort? A clean -rollback is hard to implement (and to promise), so **"Abort"** may be a cleaner verb — "Cancel" implies -cancel-and-roll-back, which we can't reliably guarantee. - -**Finding** (traced in `provision()`, -[QuickStartService.ts](../../../../src/services/localQuickStart/QuickStartService.ts)): the answer is -**"abort, with partial cleanup—but the image is kept."** On cancel (the panel's `Cancel` button aborts -the provisioning subscription → `AbortSignal` → `onAbort` fires `cts.cancel()`): - -- 🔍 The in-flight Docker command is **aborted** via the cancellation token — a `docker pull` in - progress stops promptly. -- 🔍 In `finally`, cleanup is **container-scoped only** (decision D12): if a container was created it's - stopped + removed; if a create was attempted but the id wasn't captured, an orphan is swept by - label; the temp env-file is deleted; state resets `Provisioning → NotInstalled`. -- ⚠️ **The image is never deleted.** Nothing calls a `removeImage`; whatever layers `docker pull` - already fetched **stay in Docker's local cache** (by design — a resumable/cached pull makes the next - attempt fast). So Cancel is a **partial** rollback: the half-created _container_ is removed, but the - partially-/fully-downloaded _image_ is retained. -- Net: "Cancel" today reads as "undo everything," but we only undo the container, not the download. The - retained image cache is arguably the _right_ behavior (fast retry) — it just isn't what "Cancel" - promises. +### 9. We should only ever delete containers we created (pre-release safety) ⚠️ -💡 **Suggestion:** +**Priority:** P1 · **Status:** 🟠 Open — operator to choose the direction (pre-release must-fix). + +**Observation:** Not urgent now, but must be addressed **before final release**: we should be allowed to +delete **only** the containers we created. + +**Finding:** 🔍 Removal is already label-guarded in the normal path — `deleteContainer()` only removes when +`entry.missing || isManaged(id)`, and `isManaged()` verifies the `vscode.documentdb.quickstart` label. But +there are softer spots: on `entry.missing` the label re-check is **skipped** (the id comes from metadata / +`findManagedContainer`, which filters by label, so it is _currently_ safe but relies on that invariant), and +`ContainerRuntime.removeContainer()` itself does **no** label check. As the feature grows (multi-instance, +user-named containers, more recovery flows), an id-based delete without a label re-verify is a latent risk. -1. **Rename the verb to "Abort"** (or "Stop setup") to set honest expectations: we stop the in-progress - operation and remove any half-created container, but we **don't** promise to erase everything — the - cached image layers are kept intentionally. "Cancel" over-promises a rollback we don't perform. -2. **Say what's kept**, right by the button: a one-liner like _"Stops setup. The downloaded image is - kept so a retry is faster."_ removes the ambiguity the observation raises. -3. **Confirm the behavior per phase** as part of the work item (pull / creating / starting / waiting): - verify no orphaned resources survive _besides_ the intentional image cache, and that a mid-pull - abort leaves nothing half-written except cached layers. Decide explicitly that keeping the image is - desired (it is, for retry speed) and document it. -4. If a true "remove everything including the image" is ever wanted, make it a **separate, explicit** - action (e.g. an "Abort and clean up" secondary), never the default — deleting cached layers on every - cancel would make retries slow and is the "clean rollback is tough" case the observation flags. +💡 **Suggestion:** Before final release, make "ours" a **hard precondition on every removal** — re-verify the +`vscode.documentdb.quickstart` label immediately before `removeContainer` / `removeVolume`, regardless of the +`missing` shortcut, so no code path can ever remove a container or volume the extension didn't create. --- -## 5. The setup webview — the "waiting" stage +## P2 — Polish, expectation, or feature gap -### 5.1 "Waiting for DocumentDB to accept connections" can sit static for minutes ⚠️ +### 10. Legacy emulator connections migrate to a folder that can hide in the sort order ⚠️🔍 -**Observation:** This message could be reworded, or show some intermediate progress as sub-info, since -it can take a while. +**Priority:** P2 · **Status:** 🟠 Open — operator to choose the direction (see Open idea O1). -**Finding:** The stage label is `'Waiting for DocumentDB to accept connections'`. It wraps -`waitForReadiness()` in [QuickStartService.ts](../../../../src/services/localQuickStart/QuickStartService.ts), -which polls the wire protocol with a **`READINESS_TIMEOUT_MS = 180_000` (3 min)** budget and a 3 s -per-attempt timeout. During that window the stage row shows only a spinner + the same static label — -⚠️ **no in-panel sub-status** (attempt count, elapsed-in-stage, remaining). The information _does_ -exist: logs stream to the "DocumentDB Local Quick Start" output channel via `followLogs` the whole -time, just not surfaced in the panel. On timeout a distinct `ReadinessTimeoutError` (kept separate so -the container is preserved for "Wait longer") shows the `failed`/`timedOut` message. (That message and -several others here contain em dashes — see §8.1.) +**Observation:** Is the one-time migration of original "DocumentDB Local" emulator connections still +happening? What's the folder called? Sorting could place it somewhere unexpected. -💡 **Suggestion:** Add a lightweight **sub-info line** under this stage while it's active — cheapest is -an **in-stage elapsed timer** ("still initializing… 0:45") and/or a rotating reassurance after N -seconds ("first run generates TLS certs, this can take a minute"). Also consider surfacing the "View -Docker output" link _during_ this stage (it currently only appears at the bottom of the panel). A -reword should stay accurate for both a cold first run and a plain restart, since both funnel through -this stage. +**Finding:** ---- +- 🔍 Yes, it's still active. The migration lives in + [legacyEmulatorMigration.ts](../../../../src/services/legacyEmulatorMigration.ts), invoked from + [ClustersExtension.ts](../../../../src/documentdb/ClustersExtension.ts) on every activation + (`migrateLegacyEmulatorConnections`), guarded by a `globalState` flag so it runs once. +- 🔍 The destination folder is **`Local Connections (Legacy)`** (`LEGACY_FOLDER_BASE_NAME`), deterministic id + `vscode-documentdb.legacyLocalConnectionsFolder`; a name clash is suffixed `(2)`, `(3)`, … It is a **normal** + renamable/movable folder under the Clusters zone. Emulators-zone sub-folders are intentionally **flattened** + into this one folder. The old `Emulators` zone/`LocalEmulatorsItem` node is kept as a read-only rollback + until migration succeeds, then retired. +- ⚠️ **Why it can surprise:** root items are assembled Quick Start node → legacy emulator node (if un-migrated) + → **all folders, alphabetically** → ungrouped connections → New Connection. Folders sort with a single + `localeCompare(..., { numeric: true })` pass with **no special-casing**, so the legacy folder lands wherever + "L…" falls — easy to miss right after an upgrade. No migration toast surfaced in this pass. + +💡 **Suggestion:** Make the destination discoverable: (a) a **one-time toast** with a "Reveal" action; and/or +(b) a distinct badge/icon; and/or (c) pin it; the cheapest code-free option is (e) prefix the name with `_`. +See Open idea O1. + +### 11. Phase out the "local connection" concept — a localhost connection is just a connection 💡 + +**Priority:** P2 · **Status:** 🟠 Open — operator to choose the direction (a cleanup epic, not one PR). + +**Observation:** Ideally we **phase out** the notion of a special "local connection" entirely. A connection to +`localhost` is just a **regular connection that happens to point at localhost**. Once that lands there is no +separate "Local Connections" bucket, no emulator zone, no special-casing — which requires a **sweep of the +documentation, menus, commands, and wording**. + +**Finding / scope:** + +- Today the code still carries the legacy split: an `Emulators` storage zone, `LocalEmulatorsItem`, + `emulatorConfiguration.isEmulator`, the migration to `Local Connections (Legacy)` (item 10), and the Quick + Start managed instance (a bespoke non-standard node, item 20). Several menus / `when`-clauses key off + `treeItem_LocalEmulators` and `isEmulator`. +- End state: a localhost connection is a normal `DocumentDBClusterItem` in the regular Clusters zone; + "emulator-ness" (e.g. allow-invalid-TLS) becomes a **property of the connection**, not a separate class / + zone / tree node. + +💡 **Suggestion:** Track as an explicit **deprecation/cleanup epic**: (a) inventory every place that +special-cases "local/emulator"; (b) collapse them onto "a regular connection to localhost"; (c) keep only the +minimal per-connection flags a local target needs. Item 10's legacy folder and the Quick Start node (item 20) +both fold into this end state. + +### 12. The initial "Checking Docker" loading should render chrome + skeleton cards (reuse the metric card) ⚠️💡 + +**Priority:** P2 · **Status:** 🟠 Open — operator to choose the direction. + +**Observation:** The initial `loading` ("Checking Docker") state should behave like the **Query Insights** +view: the chrome renders immediately (title + header), and the cards behave like our **metrics cards** — a +**loading skeleton** while status resolves, then fill **independently** (backend split not needed now). Worth +**reusing the existing metric card** — it's accessible and has tooltip support. + +**Finding:** -## 6. The setup webview — image pull progress +- ⚠️ The `loading` phase renders **only** `` in an otherwise empty panel — + no hero, no cards (see Appendix A). When `getDockerStatus` resolves, the panel **snaps** to the full layout. +- 🔍 The readiness/config cards use a **local, simplified** `MetricCard` inline in `LocalQuickStart.tsx` — no + skeleton, no tooltip, weaker a11y. +- 🔍 Query Insights ships a purpose-built, accessible card: **`MetricBase`** at + [MetricBase.tsx](../../../../src/webviews/documentdb/collectionView/components/queryInsightsTab/components/metricsRow/MetricBase.tsx) + — **loading skeleton** (`value === undefined` → `SkeletonItem`), null/unavailable placeholder, **tooltip + explanations**, and a solid **accessibility** pattern (focusable card, aria-label, aria-hidden children). + +💡 **Suggestion:** (1) Rework `loading` to render the **hero immediately** and the readiness cards in +**skeleton** state, each flipping to its value independently. (2) **Reuse `MetricBase`** instead of the local +`MetricCard` — it needs a small **extension** (a badge slot for the ✓/! readiness badge) and **relocation to a +shared** location (it's currently buried under `collectionView/queryInsightsTab/`; importing it into Quick +Start is the wrong dependency direction). Worth it: one accessible, tooltip-capable card shared across features. + +### 13. Advanced panel: credentials should sit behind a toggle, and "placeholder = default" is ambiguous ⚠️💡 + +**Priority:** P2 · **Status:** 🟠 Open — operator to choose the direction (options to weigh below). + +**Observation:** Username + password should be grouped **behind a toggle** — the user flips "override the +default" and only then specifies them. The ghost text **"auto"** is confusing (is it the default _value_?). +And the panel label says _"Leave any field blank to keep the automatic default,"_ yet the **Image tag** field +shows **"latest"** as ghost text — so how does the user "keep it blank"? + +**Finding** (`renderAdvanced()` in `LocalQuickStart.tsx`): + +- On a fresh provision (`!isRecreate`) the panel renders Port, Image tag, Username, Password (always visible) + + a "Load sample data" switch, under "Leave any field blank to keep the automatic default." +- ⚠️ **Placeholder means two different things.** Port's placeholder is the literal default (`10260`), Image + tag's is the literal default tag (`latest`) — but Username/Password use **`'auto'`**, a _description of + behavior_. So "latest" reads as a set value, while "auto" reads as a mode; combined with "leave blank," the + greyed-in `latest` makes it unclear whether the field is pre-filled or empty. +- 🔍 The credentials already follow a **both-or-neither** rule in `advValidation`; a toggle would encode that + structurally. +- 🔍 On a recreate the credential/tag fields are already hidden — so this only concerns fresh provisioning. + +💡 **Suggestion:** (1) **Group credentials behind a toggle** ("Override generated credentials," off by +default; off → a read-only "Auto-generated, stored securely" note; on → reveal both fields, required +together) — removes the `'auto'` ghost text entirely. (2) **Resolve the placeholder ambiguity**, options to +weigh: **A** drop literal-value placeholders and keep the default only in the `hint` line (cheapest, most +consistent); **B** pre-fill the actual default value and drop the "blank" concept; **C** a per-field "Use +default" affordance (heavier). Leaning **A**. Whichever is chosen, apply the **same** convention to every +Advanced field. + +### 14. What does "Cancel" do mid-provision? Consider the verb "Abort" (no full rollback promise) ⚠️💡 + +**Priority:** P2 · **Status:** 🟠 Open — operator to choose the direction. + +**Observation:** Investigate what **Cancel** does while provisioning is in progress. Do we **delete an image +that started downloading**, or just abort? A clean rollback is hard to promise, so **"Abort"** may be a cleaner +verb — "Cancel" implies cancel-and-roll-back. + +**Finding** (traced in `provision()`, `QuickStartService.ts`): **abort, with partial cleanup — but the image is +kept.** On cancel (Cancel → `AbortSignal` → `onAbort` fires `cts.cancel()`): + +- 🔍 The in-flight Docker command is **aborted** via the cancellation token — a `docker pull` stops promptly. +- 🔍 In `finally`, cleanup is **container-scoped only** (D12): a created container is stopped + removed; an + uncaptured-id orphan is swept by label; the temp env-file is deleted; state resets `Provisioning → +NotInstalled`. +- ⚠️ **The image is never deleted.** Nothing calls `removeImage`; already-fetched layers **stay in Docker's + cache** (by design — a resumable/cached pull makes the next attempt fast). So Cancel is a **partial** rollback: + the container is removed, the image download is retained. + +💡 **Suggestion:** (1) **Rename to "Abort"** (or "Stop setup") — "Cancel" over-promises. (2) **Say what's kept** +by the button ("Stops setup. The downloaded image is kept so a retry is faster."). (3) **Confirm per phase** +(pull/creating/starting/waiting) that nothing orphans besides the intentional image cache. (4) If a full +"remove everything incl. image" is ever wanted, make it a **separate explicit** action, never the default. + +### 15. On completion, show a "What was done" summary card — below the content ⚠️💡 + +**Priority:** P2 · **Status:** 🟠 Open — operator to choose the direction. + +**Observation:** When a deployment completes, the info card should render **below** the other content (not +above), and carry a **summary of what was done** — a table-ish block like the review screen's "what we'll do" +summary, but framed as **"What was done."** -### 6.1 No structured progress while the image downloads ⚠️ +**Finding:** + +- The `review` phase already renders a summary (`renderSummary()` in `LocalQuickStart.tsx`) — a labeled + key/value block (image, port, data, credentials, lifetime). The `success` phase instead shows a free-text + success box + a "Next steps" list, with **no structured recap**, and that content sits high in the layout. +- Because the real values are only known **after** provisioning (a **port fallback** may have moved the + instance off 10260), a recap is the natural place to surface them — today the actual port / credentials / + connection string aren't summarized anywhere in-panel. + +💡 **Suggestion:** Add a **"What was done"** summary card mirroring `renderSummary()`, rendered **below** the +next-steps / actions, listing the **actual** result: image + resolved tag, **actual bound port** (flag if it +differs from the default), data mode (persistent/ephemeral), credentials (auto/custom, stored securely), and +the connection string with its copy affordance. Gives a clean "review → result" symmetry and finally surfaces +the real port. + +### 16. Set an upfront "the first run can take a few minutes" expectation 💡 + +**Priority:** P2 · **Status:** 🟠 Open — operator to choose the direction. + +**Observation:** Whatever we can do to set expectations up front — a cold first run is genuinely long. + +**Finding:** A first provision does a lot — Docker pull, container create/start, a readiness wait of up to +`READINESS_TIMEOUT_MS = 180_000` (3 min, item 18), plus first-run TLS cert generation and sample-data seeding. +The only time cue today is the elapsed counter ticking up. + +💡 **Suggestion:** State the expectation before / at the start of provisioning — a one-liner on the review +screen ("The first run downloads the image and initializes the database; this can take a few minutes") and/or +a provisioning subtitle. Later runs are fast (image cached, item 14), so the copy can distinguish "first run" +from a restart. + +### 17. "Load sample data" is all-or-nothing — add a choice of test data ⚠️💡 + +**Priority:** P2 · **Status:** 🟠 Open — operator to choose the direction. + +**Observation:** The sample-data option is all-or-nothing today. We'll have to add **options to choose the test +data** (which dataset, or none). + +**Finding:** `AdvancedQuickStartOptions.loadSampleData` is a single boolean; when true the image's built-in +init seeds one fixed `sampledb` (users / products / orders / analytics) once, post-readiness (exec-once). +There's no way to pick a dataset, see what it contains, or reseed later. + +💡 **Suggestion:** Replace the boolean with a **dataset choice** (None / the current sample / future named sets), +ideally with a one-line description of each. Keep exec-once semantics per dataset; sets up a later +"reset/reseed." Scope depends on what datasets the image can provide — confirm with the image owners. + +### 18. "Waiting for DocumentDB to accept connections" can sit static for minutes ⚠️ + +**Priority:** P2 · **Status:** 🟠 Open — operator to choose the direction. + +**Observation:** This message could be reworded, or show intermediate progress as sub-info, since it can take a +while. + +**Finding:** The stage label is `'Waiting for DocumentDB to accept connections'`. It wraps `waitForReadiness()` +in `QuickStartService.ts`, which polls the wire protocol with a **`READINESS_TIMEOUT_MS = 180_000` (3 min)** +budget and a 3 s per-attempt timeout. During that window the stage row shows only a spinner + the same static +label — ⚠️ **no in-panel sub-status**. Logs stream to the output channel via `followLogs` the whole time, just +not in the panel. On timeout a distinct `ReadinessTimeoutError` (kept separate so the container is preserved for +"Wait longer") shows the `failed`/`timedOut` message (contains an em dash — item 8). + +💡 **Suggestion:** Add a lightweight **sub-info line** while active — an **in-stage elapsed timer** and/or a +rotating reassurance after N seconds ("first run generates TLS certs, this can take a minute"). Consider +surfacing "View Docker output" _during_ this stage (it currently only appears at the bottom). + +### 19. No structured progress while the image downloads ⚠️ + +**Priority:** P2 · **Status:** 🟠 Open — operator to choose the direction (see Open idea O2). **Observation:** Do we track progress while the image is pulled? It's a long-running op too. **Finding:** ⚠️ No. `provision()` yields one `active` event (`'Pulling the official image…'`), awaits `pullImage()`, then one `done` event — nothing in between. `ContainerRuntime.pullImage()` -([ContainerRuntime.ts](../../../../src/services/localQuickStart/ContainerRuntime.ts)) runs `docker -pull` through the shared CLI shell-runner, piping stdout/stderr **only** to the output channel -(masked, line-buffered). Docker's per-layer progress (`Downloading`/`Extracting`/`Pull complete`) is -therefore visible only as raw channel text, never in the webview — the panel shows a spinner + -static label for the entire pull, same as §5. - -💡 **Suggestion:** Give the pull an **indeterminate but alive** treatment. Since a true 0–100% isn't -known up front without a bigger rework, pair a Fluent UI **indeterminate `ProgressBar`** with a small -live **"N of M layers"** sub-label (derivable by counting distinct layer ids vs. `Pull complete` -lines). This needs modest plumbing (a progress callback on `pullImage`/`IContainerRuntime`, extra -`StageEvent` fields, webview rendering). Full options, including a Docker-Engine-API/`dockerode` -route for real byte totals and why a literal "dot per layer" reads as jittery, are in -[§13.2](#132-image-pull-progress-indicator). +([ContainerRuntime.ts](../../../../src/services/localQuickStart/ContainerRuntime.ts)) runs `docker pull` +through the shared CLI shell-runner, piping stdout/stderr **only** to the output channel. Docker's per-layer +progress is visible only as raw channel text, never in the webview — a spinner + static label for the whole +pull, same as item 18. ---- +💡 **Suggestion:** Give the pull an **indeterminate but alive** treatment — a Fluent UI **indeterminate +`ProgressBar`** + a small live **"N of M layers"** sub-label (count distinct layer ids vs. `Pull complete` +lines). Full options in Open idea O2. -## 7. The running instance in the tree +### 20. The running-instance row doesn't share regular cluster context commands 🔍 -### 7.1 The running-instance row doesn't share regular cluster context commands 🔍 +**Priority:** P2 · **Status:** 🟠 Open — operator to choose the direction (see Open idea O3). -**Observation:** The running managed instance doesn't share context-menu commands with regular -clusters. Do we still extend the cluster base class? If not, what would break if we did? +**Observation:** The running managed instance doesn't share context-menu commands with regular clusters. Do we +still extend the cluster base class? If not, what would break if we did? **Finding:** -- 🔍 Yes — `QuickStartClusterItem extends DocumentDBClusterItem` - ([LocalQuickStartItem.ts](../../../../src/tree/connections-view/LocalQuickStart/LocalQuickStartItem.ts)). - It overrides `getTreeItem()` only to force the state-aware description, and reuses the base for - icon/tooltip/browsing. -- 🔍 The reason regular commands don't appear: the **contextValue is fully replaced, not extended.** - The constructor sets `contextValue = createContextValue([INSTANCE_CONTEXT, stateToken])` — i.e. - `treeItem_quickStartInstance` + `state_running` — discarding the base's `treeitem_documentdbcluster` - token that every regular-cluster `view/item/context` `when`-clause matches on. This is deliberate - (class comment: show Quick Start lifecycle actions "instead of the generic cluster menus"). -- 🔍 **What would break if the base contextValue were kept too:** the generic commands assume a - **stored** connection — `removeConnection` deletes a storage record, `moveItems`/`renameConnection` - hit `ConnectionStorageService`, `update*` open storage wizards. The Quick Start instance is - explicitly **in-memory only** (not in any storage zone), so those commands would act on a record - that doesn't exist — silent no-ops at best, orphaned-state/exceptions at worst — unless each learned - to special-case this row. So full replacement avoids a bug class, but also throws out genuinely - useful storage-independent commands (e.g. "Open in Shell"). - -💡 **Suggestion:** Keep the curated menu, but **add back the storage-independent commands explicitly** -(most valuable: "Open in Shell" / "New Query" against the running instance). The clean version of -this is splitting the base contextValue into a storage-dependent subset and a connection-only subset -so Quick Start can opt into the latter — weighed in [§13.3](#133-cluster-commands-on-the-quick-start-row). - -### 7.2 "Copy Connection String" is a separate impl and silently includes the password ⚠️ - -**Observation:** Copy Connection String seems reimplemented for Quick Start and behaves slightly -differently — it doesn't ask whether to include the password. +- 🔍 Yes — `QuickStartClusterItem extends DocumentDBClusterItem`. It overrides `getTreeItem()` only to force the + state-aware description and reuses the base for icon/tooltip/browsing. +- 🔍 Regular commands don't appear because the **contextValue is fully replaced, not extended:** the constructor + sets `contextValue = createContextValue([INSTANCE_CONTEXT, stateToken])` — `treeItem_quickStartInstance` + + `state_running` — discarding the base's `treeitem_documentdbcluster` token every regular-cluster `when`-clause + matches on. Deliberate (show Quick Start lifecycle actions "instead of the generic cluster menus"). +- 🔍 **What would break if kept too:** the generic commands assume a **stored** connection (`removeConnection`, + `moveItems`/`renameConnection`, `update*` all hit storage). The Quick Start instance is **in-memory only**, so + those would act on a record that doesn't exist — silent no-ops/exceptions — unless each special-cased this row. + So full replacement avoids a bug class, but throws out useful storage-independent commands (e.g. "Open in Shell"). -**Finding:** Regular clusters use `copyConnectionString()` -([copyConnectionString.ts](../../../../src/commands/copyConnectionString/copyConnectionString.ts)), -which prompts via `showQuickPick` ("with password" vs "without password", gated by -`canIncludeNativePassword()`) and is well-tested. Quick Start uses a **separate, shorter** -`copyQuickStartConnectionString()` -([localQuickStartCommands.ts](../../../../src/commands/localQuickStart/localQuickStartCommands.ts)) -that copies `metadata.connectionString` (password already embedded) **with no prompt** — a genuine -behavioral difference, not just styling. ⚠️ A plaintext password always lands on the clipboard -silently, which stands out given how careful the rest of the feature is about credentials (masked -logging, env-file instead of CLI args). There is a _separate_ `copyQuickStartPassword()` command, but -the connection-string action itself never asks. - -💡 **Suggestion:** Reuse the regular command's QuickPick confirmation for parity (Quick Start always -uses native auth, so no extra branching is needed), or — if the extra click is unwanted for a -one-click-local flow — at minimum make the two commands' behavior a **conscious, documented** choice -rather than an incidental divergence. - -### 7.3 Icons are all inherited/generic — nothing marks "this is the Quick Start instance" 🔍💡 +💡 **Suggestion:** Keep the curated menu, but **add back storage-independent commands explicitly** ("Open in +Shell" / "New Query"). The clean version is splitting the base contextValue into storage vs connection subsets — +Open idea O3. -**Observation:** How are the icons chosen for the Quick Start instances? +### 21. Credential storage: reuse the extension's secure-storage patterns 💡 -**Finding:** 🔍 There's no bespoke icon set: +**Priority:** P2 · **Status:** 🟠 Open — operator to choose the direction. -- **Root node:** the extension's product icon (`vscode-documentdb-icon-*.svg`). -- **Running row:** no override — inherits `DocumentDBClusterItem`'s logic, which picks `$(plug)` when - `emulatorConfiguration.isEmulator` is true (always, for Quick Start) else `$(server-environment)`. - So the running instance gets **the same `$(plug)`** as any other connection flagged as an emulator - (including the §2.1 migrated legacy connections). Nothing visually distinguishes "the managed - Quick Start instance." -- **State rows** (Starting/Stopping/Stopped/Error/Missing): generic `ThemeIcon`s (`loading~spin`, - `circle-outline`, `warning` with theme colors). -- **Empty-state rows:** `rocket` and `link-external`. +**Observation:** Flag the plaintext-password posture. Everything else is careful (masked logs, env-files), yet +the generated password is embedded in the stored connection string and is one-click copyable/visible. **Think +about clean storage** — can we use our **storage services**? There's solid secret-management already; we can +**replicate / copy** those patterns. No big refactor needed. -💡 **Suggestion:** Give the managed instance a **distinct, state-independent identity** — e.g. reuse -the **rocket** (already the feature's motif on the root/action rows) for the instance row, or a -dedicated brand mark, so users can tell "the Quick-Start-managed one" apart from a manually-flagged -emulator at a glance. Low priority, but cheap. +**Finding:** ---- +- Quick Start stores its connection string (credentials embedded) via `ext.secretStorage` under a per-alias key + (`secretKey(alias)`) and primes `CredentialCache` from it — a bespoke path, separate from how regular + connections handle secrets. +- The extension already has a mature secure-storage layer (`ConnectionStorageService` + its credential/secret + handling), with established patterns for storing/retrieving credentials cleanly and gating exposure. Quick + Start doesn't reuse it (by design it's "not stored in any zone," item 20), so it re-implements a thinner version. -## 8. Destructive actions & the data / volume model +💡 **Suggestion:** Align Quick Start's credential handling with the existing secure-storage patterns — reuse +`ConnectionStorageService`'s secret mechanisms or **copy the same approach** (replicating is fine). Goal: one +consistent, audited way to store/expose local credentials. Ties into item 11: if local instances become regular +connections, they inherit this storage naturally. -### 8.1 "Delete Container" bypasses the shared confirmation and uses em dashes ⚠️ +### 22. Persistence is always on — there's no "ephemeral / no-persistence" option 💡 -**Observation:** The Delete Container modal is fine conceptually, but it should use the same delete -confirmation code path as other destructive actions (like in Settings), and shouldn't use em dashes. +**Priority:** P2 · **Status:** 🟠 Open — operator to choose the direction (see Open idea O6). -**Finding:** +**Observation:** Our default is a persistent data volume. That should be a **toggle** — sometimes a user wants a +throwaway instance with no persistence. -- ⚠️ Every other destructive command — [removeConnection](../../../../src/commands/removeConnection/removeConnection.ts), - [deleteCollection](../../../../src/commands/deleteCollection/deleteCollection.ts), - [deleteDatabase](../../../../src/commands/deleteDatabase/deleteDatabase.ts), - [folder delete](../../../../src/commands/connections-view/deleteFolder/ConfirmDeleteStep.ts) — calls - **`getConfirmationAsInSettings()`** ([getConfirmation.ts](../../../../src/utils/dialogs/getConfirmation.ts)), - which honors the user's `confirmationStyle` setting (type-the-word / number challenge / click). - `deleteQuickStartInstance()` instead calls **`getConfirmationWithClick()`** directly, ignoring that - setting — it's the only _destructive_ command that does (the other direct callers are - non-destructive: `hideIndex`/`unhideIndex`). -- ⚠️ Its two confirmation `detail` strings both contain a literal **em dash** ("…This cannot be undone - — you can recreate…"). The character also appears across the feature's other strings - (`LocalQuickStart.tsx` next-steps bullets, the §5 timeout message). +**Finding:** 🔍 `AdvancedQuickStartOptions` +([quickStartTypes.ts](../../../../src/services/localQuickStart/quickStartTypes.ts)) exposes `port`, `username`, +`password`, `imageTag`, `loadSampleData` — but **no persistence flag**. `createAndRunContainer()` **always** +mounts a named volume (`volumeName(alias)` at `QUICK_START_DATA_PATH`). Every instance is persistent by +construction; no way to opt out. -💡 **Suggestion:** Switch Delete Container to **`getConfirmationAsInSettings()`** for consistency (decide -the confirmation word — there's no "type the container name" concept yet, so a simple word like -`delete` matches the other delete flows). Separately, do a **one-pass em-dash sweep** of the whole -feature's user-facing strings rather than fixing only the delete dialog. +💡 **Suggestion:** Add an Advanced toggle **"Persist data across restarts"** (default **on**). When **off**, +create the container **without** the named-volume mount so data lives only in the writable layer and is gone on +delete — good for throwaway trials. Trade-offs in Open idea O6. -### 8.2 Persistence is always on — there's no "ephemeral / no-persistence" option 💡 +### 23. A user-initiated delete must always delete the data volume ⚠️ -**Observation:** Our default is a persistent data volume. That should be a **toggle** — sometimes a -user wants a throwaway instance with no persistence. +**Priority:** P2 · **Status:** 🟠 Open — operator to choose the direction. -**Finding:** 🔍 `AdvancedQuickStartOptions` -([quickStartTypes.ts](../../../../src/services/localQuickStart/quickStartTypes.ts)) exposes `port`, -`username`, `password`, `imageTag`, and `loadSampleData` — but **no persistence flag**. -`createAndRunContainer()` **always** mounts a named volume (`volumeName(alias)` at -`QUICK_START_DATA_PATH`). So every instance is persistent by construction; there is no way to opt out -of the on-disk volume. - -💡 **Suggestion:** Add an Advanced toggle **"Persist data across restarts"** (default **on**, keeping -the current zero-decision behavior). When **off**, create the container **without** the named-volume -mount so data lives only in the container's writable layer and is gone on delete — useful for quick -throwaway trials and clean repros. Trade-offs in [§13.6](#136-persistence--the-volume-model). - -### 8.3 A user-initiated delete must always delete the data volume ⚠️ - -**Observation:** When the user deletes a container, we should for sure delete the persisted volume as -well. - -**Finding:** 🔍 The explicit **Delete Container** path already does this: `deleteContainer()` -([QuickStartService.ts](../../../../src/services/localQuickStart/QuickStartService.ts)) calls -`removeVolume(volumeName(alias))` alongside the secret / globalState / registry cleanup — an -intentional "full clean slate." Two caveats to stamp: (a) the credential-unavailable **reconcile** -path deliberately does **not** touch the volume (R2 data-safety — see §9.2), which is correct for an -_automatic_ path but means the _user-initiated_ delete must remain the thing that wipes it; (b) once -§9.2 makes the credential-unavailable state actionable, its Delete must route through this same -volume-removing path. - -💡 **Suggestion:** Keep explicit Delete = remove container **+ volume** (already the case). Ensure -every **user-initiated** delete surface (including the future credential-unavailable / Missing Delete) -funnels through `deleteContainer()` so a volume is never orphaned. Automatic / reconcile paths keep -R2 (never auto-wipe without the user choosing it). - -### 8.4 Data volumes should be linked to the image — no shared data volumes 💡 - -**Observation:** Our persisted volumes should be **linked to the image**. We won't share data volumes -across images; advanced users can do that themselves. - -**Finding:** 🔍 Today the volume is keyed to the **alias** (`${alias}-data`), independent of the -image. To stay safe, the service **pins the image to the volume**: on a recreate it reuses the stored -`imageRef` (from in-memory metadata → `globalState` → default) rather than the user's requested tag, -and custom image/creds are ignored on a Missing-recreate — precisely so an existing volume isn't -opened by a different (possibly older) image version that could corrupt it. So the intent ("one image -↔ one volume") is enforced by _runtime pinning_, but the volume is not _named/keyed_ by image, and -there is no model for multiple images each owning their own volume. - -💡 **Suggestion:** Make the linkage **structural**, not just runtime — key or label the volume by its -image (or record the image on the volume) so "one image ↔ one data volume" is guaranteed even if the -pinning logic changes. Do **not** implement cross-image volume sharing; leave that to advanced users -operating Docker directly. This also sets up a cleaner story if multi-instance / multi-image lands. - -### 8.5 We should only ever delete containers we created (pre-release safety) ⚠️ - -**Observation:** Keep as a review note — not urgent now, but must be addressed **before final -release**: we should be allowed to delete **only** the containers we created. - -**Finding:** 🔍 Removal is already label-guarded in the normal path — `deleteContainer()` only removes -when `entry.missing || isManaged(id)`, and `isManaged()` verifies the `vscode.documentdb.quickstart` -label. But there are softer spots: on `entry.missing` the label re-check is **skipped** (the id comes -from metadata / `findManagedContainer`, which filters by label, so it is _currently_ safe but relies -on that invariant), and `ContainerRuntime.removeContainer()` itself does **no** label check. As the -feature grows (multi-instance, user-named containers, more recovery flows), an id-based delete without -a label re-verify is a latent risk of removing something we didn't create. - -💡 **Suggestion:** Before final release, make "ours" a **hard precondition on every removal** — -re-verify the `vscode.documentdb.quickstart` label immediately before `removeContainer` / -`removeVolume`, regardless of the `missing` shortcut, so no code path can ever remove a container or -volume the extension didn't create. +**Observation:** When the user deletes a container, we should for sure delete the persisted volume as well. ---- +**Finding:** 🔍 The explicit **Delete Container** path already does this: `deleteContainer()` calls +`removeVolume(volumeName(alias))` alongside the secret / globalState / registry cleanup — an intentional "full +clean slate." Two caveats: (a) the credential-unavailable **reconcile** path deliberately does **not** touch the +volume (R2, item 1), correct for an automatic path but it means the _user-initiated_ delete must remain the thing +that wipes it; (b) once item 1 makes the credential-unavailable state actionable, its Delete must route through +this same volume-removing path. -## 9. Lifecycle robustness & error recovery +💡 **Suggestion:** Keep explicit Delete = remove container **+ volume** (already the case). Ensure every +**user-initiated** delete surface funnels through `deleteContainer()` so a volume is never orphaned. Automatic / +reconcile paths keep R2 (never auto-wipe without the user choosing it). -### 9.1 A container deleted _outside_ VS Code isn't handled on Start (silent no-op) ⚠️ +### 24. Data volumes should be linked to the image — no shared data volumes 💡 -**Observation (repro):** Created an instance via Quick Start, stopped it, then deleted the _container_ -directly via Docker. Back in VS Code, clicking Start on the still-"Stopped"-looking row does nothing -visible — no dialog, no tree update — only the raw Docker error appears in the output channel: +**Priority:** P2 · **Status:** 🟠 Open — operator to choose the direction (see Open idea O6). -``` -$ docker container inspect --format '{{json .}}' 57f8311fd9ec… -Error response from daemon: No such container: 57f8311fd9ec… -``` +**Observation:** Our persisted volumes should be **linked to the image**. We won't share data volumes across +images; advanced users can do that themselves. -**Finding — a specific, reproducible gap:** +**Finding:** 🔍 Today the volume is keyed to the **alias** (`${alias}-data`), independent of the image. To stay +safe, the service **pins the image to the volume**: on a recreate it reuses the stored `imageRef` rather than +the user's requested tag, and custom image/creds are ignored on a Missing-recreate — so an existing volume isn't +opened by a different image version that could corrupt it. So the intent ("one image ↔ one volume") is enforced +by _runtime pinning_, but the volume is not _named/keyed_ by image. -- `ContainerRuntime.inspectContainer()` wraps the CLI call in `try { … } catch { return undefined; }`, - swallowing "No such container" into `undefined`. But the shared `makeRunner()` still logs the - command and streams the daemon's stderr to the channel — that's exactly the two lines the user saw - (the extension's own inspect call, not a manual terminal session). -- `start()` calls `isManaged(id)` first, which returns `false` both when the container **isn't ours** - _and_ when it's **gone** (`inspectContainer` → `undefined`). ⚠️ **`isManaged()` cannot distinguish - "missing" from "not ours."** -- `start()`'s guard is `if (!id || !isManaged || !liveStateGuard(…)) return;`. Because `isManaged` - already returned `false`, the `||` short-circuits and **`liveStateGuard()` never runs** — which is - the very code that _would_ have handled this gracefully (it detects a `'missing'` live state, calls - `refreshLiveState()`, and shows an info message). That path is well-built for the multi-window / - external-stop case; it's just unreachable here. -- Net effect: `start()` early-returns silently, `setStatus` never fires, the tree never refreshes, and - the stale "Stopped" row persists until something else (e.g. collapsing/expanding the node) triggers - `refreshLiveState()` independently. -- Note there's already a good pattern for the related case: `refreshLiveState()` _does_ set - `entry.missing = true` and the tree already renders `Missing · click to recreate`. The gap is that - the **lifecycle actions** (`start`/`stop`/`restart`) don't funnel through that missing-detection - before giving up. +💡 **Suggestion:** Make the linkage **structural** — key or label the volume by its image so "one image ↔ one +data volume" is guaranteed even if the pinning logic changes. Do **not** implement cross-image volume sharing; +leave that to advanced users operating Docker directly. -💡 **Suggestion:** Teach `start`/`stop`/`restart` to distinguish "no container found" from "found but -not ours" (a small change to `isManaged`, or a check ahead of it) and, on "missing," do what -`liveStateGuard` already does for the multi-window case — refresh state to the `Missing` badge and -surface a message with next steps: **Delete** (clean up stale metadata/volume) and/or **Recreate with -same settings** (the existing `Missing` row's recreate flow + `getReusableCredentials()` in -`provision()` likely already covers most of this — worth confirming it's sufficient once reachable -from the lifecycle actions). Full UX options in [§13.4](#134-external-deletemissing-recovery-affordance). +### 25. The container keeps running after VS Code closes — decide the desired behavior ⚠️💡 -### 9.2 Credential-unavailable state after a restart is a UI dead end (the demo incident) ⚠️ +**Priority:** P2 · **Status:** 🟠 Open — genuinely undecided; needs a pros/cons evaluation (Open idea O7). -**Observation (real incident, mid-review):** An instance was created via Quick Start, then stopped, -then its container/image were removed outside VS Code. After a **VS Code restart** the Quick Start -node showed the rocket _"Click here to start DocumentDB locally"_ row **plus** a warning row -_"DocumentDB Local has data on disk but its saved credentials are missing…"_ (truncated in the tree). -There was **no way from the UI** to delete the leftover container/volume or to start cleanly — no -Delete node, no recreate action. Recovery required manually removing the container and its volume with -Docker, then refreshing. +**Observation:** Non-standard for an extension-managed local resource (a user may expect it to stop like a dev +server), and can leave an orphaned container holding a port + RAM. -**Finding:** +**Finding:** By design the instance is durable across window reloads — `reconcile()` re-adopts a still-running +container on activation (item 1) — and the success screen explicitly says it "keeps running after VS Code +closes." There's no "stop on exit" option and no prompt on close. -- On activation, `reconcile()` → `reconcileAlias()` hits **"Case 4"**: a labelled container (or a - `ready` record) exists but the stored secret is unrecoverable, so it calls - `setStatus(alias, InstanceState.Error, undefined, CREDENTIAL_UNAVAILABLE_MESSAGE)` with - **`metadata: undefined`**. By design it **never** removes the container or volume (R2 data-safety: a - lost secret doesn't prove the volume is disposable — the user must choose the wipe). -- Because `metadata` is `undefined`, `LocalQuickStartItem.getChildren()` skips every `metadata && …` - branch and falls through to the **NotInstalled** branch, which renders the rocket action row **plus** - a passive `treeItem_quickStartError` warning row carrying the message. -- ⚠️ That warning row has **no command and no context menu**, and the Delete command's `when` requires - `treeItem_quickStartInstance` + a `state_*` token — which this surfaced state never carries. So - **Delete is unreachable**, and unlike the true `Missing` row (which at least says "click to - recreate") no recovery action is offered. The user is genuinely stuck in the UI. -- ⚠️ **Restart behavior specifically:** in-memory state is rebuilt from durable state on every reload, - so this dead end **reproduces on every restart** — it is not a transient glitch. The - volume-plus-missing-secret combination persists, so each launch re-surfaces the same non-actionable - warning. (Clicking the rocket to "start fresh" doesn't help either: the on-disk volume + missing - credentials block a clean provision, and §9.1's silent-return path compounds it.) -- **How we recovered (for the runbook):** removed the labelled container and its data volume directly - with Docker (`docker rm -f vscode-documentdb-local` + `docker volume rm vscode-documentdb-local-data`), - then refreshed the view; reconcile found nothing and returned to the clean NotInstalled empty state. +💡 **Suggestion:** Treat as an **open decision** — pros/cons in Open idea O7. Don't change behavior without +deciding; whatever is chosen should be **explicit** (a setting and/or clear copy) rather than implicit. -💡 **Suggestion:** The credential-unavailable state must be **actionable**, not a passive warning. Render -it like the `Missing` row — a real instance row (`treeItem_quickStartInstance` + a dedicated -`state_credentialsMissing` token) that exposes **Delete Container** (clean slate: container + volume + -stale records, via `deleteContainer()` per §8.3) and, where safe, **Recreate**. Keep R2 (never -_auto_-wipe) — but give the user a one-click way to _choose_ the wipe. This should share the recovery -UX in [§13.4](#134-external-deletemissing-recovery-affordance), and is a direct instance of the §9.3 -principle below. +### 26. Tree state transitions aren't announced — accessibility work item (post-redesign; discuss with Tomasz) ⚠️ -### 9.3 Tree nodes are misused to display errors (they should be actions, not dialogs) ⚠️💡 +**Priority:** P2 · **Status:** 🟠 Open — sequenced work item; **owner/approach to be agreed with Tomasz.** -**Observation:** This incident exposed a broader pattern problem — we render an **error message as a -tree node**. Leaf/error nodes in a tree should be **actions the user can take**, not a substitute for -a dialog or a status surface. +**Observation:** The webview is carefully accessible, but the **tree** state rows (Starting / Stopping / Missing +/ Running) change only their `description` text + icon, with no live-region announcement, so screen-reader users +don't hear the instance change state. This should be worked on **once the webview redesigns finish**, applying +our **learnings from past accessibility testing**. **Discuss with Tomasz.** -**Finding:** +**Finding:** State changes fire `onDidChangeStatus` → `refresh()`, re-rendering the row, but VS Code tree views +have no built-in live region, so a `description` change (e.g. "Starting…" → "Running · localhost:10260") is +silent to assistive tech. The webview, by contrast, uses `Announcer` / `aria-live` deliberately. -- Several Quick Start states render a passive, non-actionable row purely to display text: - `treeItem_quickStartError` (the credential-unavailable / error message, §9.2), and the empty state - also **appends** an error row when `status.state === Error && errorMessage`. These rows have no - command and no menu — they exist only to show a string, which then **truncates** in the tree (the - incident's "…saved credentials are miss…"), can't be copied, and competes visually with the real - action row. -- This contradicts where the rest of the extension landed. The **Kubernetes discovery** review - reached exactly this conclusion and reversed it: classified in-tree error-summary nodes were - **removed** in favor of a **modal** (`showErrorMessage(…, { modal: true })`) plus a single - actionable retry node — see that review's §F/#19 iteration 2 and its post-merge reconciliation note. - Quick Start currently repeats the anti-pattern the K8s feature already retired. - -💡 **Suggestion:** Adopt the same rule feature-wide: **tree rows are actions; errors are dialogs / -status.** Concretely: (a) surface an error/credential-unavailable condition as a **modal** (with detail -in the output channel), and (b) keep **only actionable rows** in the tree (Delete / Recreate / Retry / -"Click here to…"). This removes the passive `treeItem_quickStartError` and the error-append row, and it -directly fixes §9.2's dead end. It also rhymes with §4.2 (the webview header carrying status it -shouldn't) — the same "don't overload one surface with state it isn't meant to hold" theme. +💡 **Suggestion (requirement, sequenced):** After the webview redesign lands, do a dedicated accessibility pass on +the tree — announce meaningful state transitions (within VS Code tree API limits), carry state in accessible +labels, and apply the patterns from prior accessibility testing. **Owner / priority / approach to be agreed with +Tomasz** before implementation. --- -## 10. Possible additions +## P3 — Nice-to-have / cosmetic / acknowledged -### 10.1 "Open terminal in the container" — feasible, no blocker 🔍💡 +### 27. Docker prerequisite is only revealed after opening the webview 🔍 -**Observation:** Would a command to open a terminal inside the Docker container make sense? Can we do -it? +**Priority:** P3 · **Status:** 🟡 Acknowledged — "it is what it is" for now; may be revisited. -**Finding:** 🔍 Feasible, no blocker. +**Observation:** The empty-state row invites "Click here to start DocumentDB Local," but the hard **Docker +requirement** only surfaces once the webview opens (the `dockerNotReady` screen). Recorded so it isn't +re-discovered later as a surprise. -- The repo already uses `vscode.window.createTerminal(...)` for terminal-backed commands - ([openInteractiveShell.ts](../../../../src/commands/openInteractiveShell/openInteractiveShell.ts)), - though that uses a custom `Pseudoterminal` for the integrated shell — more than needed here. -- For a plain "shell into the container," the standard pattern is - `createTerminal({ name, shellPath: 'docker', shellArgs: ['exec', '-it', containerId, '/bin/bash'] })` - (with a `/bin/sh` fallback) — Docker drives the TTY, no custom pty required. -- The container id is already tracked (`metadata.containerId`, via `getStatus()`), so no new state is - needed. `ContainerRuntime.execShellInContainer()` exists but is non-interactive (channel output), - so a new small command calling `createTerminal` directly is the better fit. Docker is already a hard - dependency, so no new prerequisite. +**Finding:** The tree node / action row carries no hint that Docker is required; the prerequisite check +(`getDockerStatus`) runs only after the panel opens, so a user without Docker commits to the click before +learning they need it. -💡 **Suggestion:** Add an **"Open in Terminal"** context-menu command gated on `state_running` (a -stopped container can't `exec`), trying `bash` then falling back to `sh`. Small, self-contained, and a -natural power-user affordance. Open details (menu group/icon, shell fallback prompt) in -[§13.5](#135-open-terminal-in-container). +💡 **Suggestion (acknowledged / not planned now):** If ever revisited, signal the prerequisite earlier — a node +tooltip / description ("Requires Docker") or a hover. ---- +### 28. Icons are all inherited/generic — nothing marks "this is the Quick Start instance" 🔍💡 -## 11. Consolidated flags & suggestions (read this before testing) - -**Priority legend:** **P0** blocking — the user gets stuck · **P1** broken/misleading, or a -consistency & safety gap · **P2** polish, expectation, or a smaller feature gap · **P3** -nice-to-have / cosmetic. - -### 11.1 By priority (P0 → P3) - -| Priority | § | Item | Why it ranks here | -| -------- | ---- | --------------------------------------------------- | ---------------------------------------------------------------- | -| **P0** | 9.2 | Credential-missing state is a restart dead end | Real bug, hit live; the user is fully stuck with no UI action | -| **P0** | 9.1 | External container delete → silent no-op on Start | Real bug; stale row, zero feedback, only an output-channel error | -| **P1** | 4.2 | Webview header stuck on "Setting up…" | Real bug; the panel looks unfinished after success/failure | -| **P1** | 9.3 | Tree nodes misused as error dialogs | Pattern; the root cause of 9.2, contradicts the K8s decision | -| **P1** | 4.5 | Errors hijack the hero (should be content cards) | Webview twin of 9.3; ties to the 4.2 header bug | -| **P1** | 7.2 | Copy Connection String silently copies the password | Consistency + a credential surprise vs the regular command | -| **P1** | 8.1 | Delete bypasses the shared confirmation | The only destructive command ignoring `confirmationStyle` | -| **P1** | 8.5 | Only delete containers we created | Data-safety; a pre-release must-fix as the feature grows | -| **P2** | 2.1 | Legacy migration folder hides in the sort | Post-upgrade confusion; connections seem to "vanish" | -| **P2** | 4.3 | "Checking Docker" is a bare spinner | Loading polish; reuse the accessible `MetricBase` | -| **P2** | 4.4 | Advanced: creds toggle + ambiguous placeholders | Confusing defaults ("auto" vs "latest" vs "leave blank") | -| **P2** | 4.6 | "Cancel" implies a rollback it doesn't do | Verb → "Abort"; the image is kept | -| **P2** | 5.1 | "Waiting…" static for minutes | No progress feedback during a long wait | -| **P2** | 6.1 | No image-pull progress | Looks frozen on a cold pull | -| **P2** | 7.1 | Running row lacks regular cluster commands | Missing "Open in Shell" / "New Query" on the instance | -| **P2** | 8.2 | Always persistent — no ephemeral option | Feature gap; no throwaway instance | -| **P2** | 8.3 | User delete must always drop the volume | Mostly done; guarantee it across every user delete | -| **P2** | 8.4 | Volumes not structurally linked to the image | Data-model hardening (pinning is runtime-only today) | -| **P3** | 7.3 | Generic `$(plug)` icon, no distinct identity | Cosmetic; shared with any "emulator" connection | -| **P3** | 10.1 | "Open terminal in the container" | Enhancement; feasible, no blocker | -| **✅** | 3.1 | Empty-state wording / drop "Learn more" | Fix applied on this branch | - -### 11.2 By section (full detail) - -| § | Item | Verdict | Suggested next step | -| ---- | ---------------------------------------------------- | ------------------------ | ----------------------------------------------------------------------------------------------------------------------------------- | -| 2.1 | Legacy migration folder discoverability | ⚠️ Flag | One-time toast + "Reveal"; optional distinct badge/pin ([§13.1](#131-surfacing-the-legacy-migration-folder)) | -| 3.1 | Empty-state wording / drop "Learn more" | ✅ Fix applied | Decide if "Learn more" returns as a context-menu entry | -| 4.2 | Webview header changes / stuck on "Setting up" | ⚠️ Flag (real bug) | Make the header a single static string; body carries state | -| 4.3 | "Checking Docker" loading is a bare spinner | ⚠️ Flag / 💡 Suggestion | Render header + skeleton cards; reuse & relocate the Query Insights `MetricBase` | -| 4.4 | Advanced: creds not behind a toggle; placeholders | ⚠️ Flag / 💡 Suggestion | Toggle to reveal user/pass; drop literal-value placeholders (keep in `hint`) | -| 4.5 | Errors swap the hero / full-screen phase | ⚠️ Flag / 💡 Suggestion | Move messages out of the hero to a content card; combine `MessageBar` + composed RU-style card; experiment A/B/C; Retry on the card | -| 4.6 | "Cancel" mid-provision: partial rollback, image kept | ⚠️ Flag / 💡 Suggestion | Rename to "Abort"; state the image is kept; confirm per-phase cleanup | -| 5.1 | "Waiting for connections" static for minutes | ⚠️ Flag | Add in-stage elapsed/sub-info; surface "View output" earlier | -| 6.1 | No image-pull progress | ⚠️ Flag | Indeterminate `ProgressBar` + "N of M layers" ([§13.2](#132-image-pull-progress-indicator)) | -| 7.1 | Running row lacks regular cluster commands | 🔍 Answered (deliberate) | Add back storage-independent commands explicitly ([§13.3](#133-cluster-commands-on-the-quick-start-row)) | -| 7.2 | Copy Connection String silently adds password | ⚠️ Flag | Reuse the regular QuickPick confirmation, or document the divergence | -| 7.3 | Generic `$(plug)` icon, no distinct identity | 🔍 Answered | Give the instance a distinct icon (reuse the rocket motif) | -| 8.1 | Delete bypasses shared confirmation + em dashes | ⚠️ Flag | Switch to `getConfirmationAsInSettings()`; sweep em dashes feature-wide | -| 8.2 | Persistence always on — no ephemeral option | 💡 Suggestion | Add an Advanced "Persist data" toggle (default on) ([§13.6](#136-persistence--the-volume-model)) | -| 8.3 | User delete must always drop the volume | ⚠️ Flag | Route every user-initiated delete through `deleteContainer()` (volume incl.) | -| 8.4 | Volumes not structurally linked to the image | 💡 Suggestion | Key/label the volume by image; no cross-image sharing | -| 8.5 | Delete only containers we created | ⚠️ Flag (pre-release) | Re-verify the Quick Start label immediately before every remove | -| 9.1 | External container delete not handled on Start | ⚠️ Flag (real bug) | Distinguish missing vs not-ours; route to Missing + recovery ([§13.4](#134-external-deletemissing-recovery-affordance)) | -| 9.2 | Credential-unavailable state is a restart dead end | ⚠️ Flag (real bug) | Make it an actionable instance row (Delete / Recreate); shares [§13.4](#134-external-deletemissing-recovery-affordance) | -| 9.3 | Tree nodes misused as error dialogs | ⚠️ Flag (pattern) | Errors → modal + output channel; tree rows are actions only | -| 10.1 | "Open terminal in container" | 🔍 Feasible | Add `state_running` "Open in Terminal" command ([§13.5](#135-open-terminal-in-container)) | +**Priority:** P3 · **Status:** 🟠 Open — operator to choose the direction. + +**Observation:** How are the icons chosen for the Quick Start instances? + +**Finding:** 🔍 No bespoke icon set. **Root node:** the extension's product icon. **Running row:** no override — +inherits `DocumentDBClusterItem`'s logic, which picks `$(plug)` when `emulatorConfiguration.isEmulator` is true +(always, for Quick Start), so it gets **the same `$(plug)`** as any other "emulator" connection (including the +item 10 migrated legacy connections). **State rows:** generic `ThemeIcon`s (`loading~spin`, `circle-outline`, +`warning`). **Empty-state rows:** `rocket` and `link-external`. + +💡 **Suggestion:** Give the managed instance a **distinct, state-independent identity** — e.g. reuse the +**rocket** (the feature's motif) for the instance row, so users can tell "the Quick-Start-managed one" apart at +a glance. Low priority, but cheap. + +### 29. "Open terminal in the container" — feasible, no blocker 🔍💡 + +**Priority:** P3 · **Status:** 🟠 Open — operator to choose the direction (see Open idea O5). + +**Observation:** Would a command to open a terminal inside the Docker container make sense? Can we do it? + +**Finding:** 🔍 Feasible, no blocker. The repo already uses `vscode.window.createTerminal(...)`; for a plain +"shell into the container" the standard pattern is +`createTerminal({ name, shellPath: 'docker', shellArgs: ['exec', '-it', containerId, '/bin/bash'] })` (with a +`/bin/sh` fallback) — Docker drives the TTY, no custom pty required. The container id is already tracked +(`metadata.containerId`), and Docker is already a hard dependency. + +💡 **Suggestion:** Add an **"Open in Terminal"** context-menu command gated on `state_running`, trying `bash` +then falling back to `sh`. Details in Open idea O5. --- -## 12. Applied changes on this branch +## Implemented + +### 30. Empty-state tree rows: reword the action, drop "Learn more" ✅ -- **§3.1** — empty-state action row relabeled to "Click here to install & try DocumentDB locally"; the - "Learn more…" row removed. Files: [LocalQuickStartItem.ts](../../../../src/tree/connections-view/LocalQuickStart/LocalQuickStartItem.ts), - [l10n/bundle.l10n.json](../../../../l10n/bundle.l10n.json). Verified via `l10n` / `prettier` / `lint` - / full Jest suite / `build`. +**Priority:** P1\* (wording) · **Status:** ✅ Implemented on this branch. -Everything else in this document is a discovery note or suggestion — no other code was changed. +**Observation:** The rows shown when there's no managed instance yet should be reworded — the action row should +read like "Click here to…" — and "Learn more" could be dropped from the list. + +**Finding:** Both rows were built in `LocalQuickStartItem.getChildren()` +([LocalQuickStartItem.ts](../../../../src/tree/connections-view/LocalQuickStart/LocalQuickStartItem.ts)) for the +`NotInstalled` state: an action row (rocket, `treeItem_quickStartAction`) and a separate "Learn more…" row +(`link-external`, `treeItem_quickStartLearnMore`, opening `https://github.com/microsoft/documentdb`). No +`view/item/context` entry existed for either, so there was **no existing context-menu** to move "Learn more" +into. + +✅ **Implemented (updated 2026-07-09):** + +- The action row is now labeled **`'Click here to start DocumentDB Local'`** (the wording was iterated a couple + of times: `'Quick Start — Install & try DocumentDB locally'` → `'Click here to install & try DocumentDB +locally'` → the current, shorter **`'Click here to start DocumentDB Local'`**). Verified against the current + code in `getChildren()`'s `NotInstalled` branch. +- The `'Learn more…'` row (`treeItem_quickStartLearnMore`) was **removed** from the empty state. It is **not** + relocated to a context menu (no menu plumbing exists yet), so the external repo link is no longer reachable + from the tree. +- Files: [LocalQuickStartItem.ts](../../../../src/tree/connections-view/LocalQuickStart/LocalQuickStartItem.ts), + [l10n/bundle.l10n.json](../../../../l10n/bundle.l10n.json). Verified via `l10n` / `prettier` / `lint` / full + Jest suite / `build`. + +💡 **Remaining (Open, low priority):** Decide whether "Learn more" comes back as a context-menu entry on the +parent `DocumentDB Local - Quick Start` node (a new `view/item/context` gated on `treeItem_localQuickStart`), or +is dropped for good in favor of the walkthrough/README. --- -## 13. Open ideas — options, pros & cons +## Open ideas — options, pros & cons -These are the genuinely open design questions with real trade-offs. Recommendations are suggestions -for the team to react to, not decisions. +Genuinely open design questions with real trade-offs. Recommendations are suggestions to react to, not decisions. -### 13.1 Surfacing the legacy migration folder (§2.1) +### O1. Surfacing the legacy migration folder (item 10) | Option | Pros | Cons | | --------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------ | ----------------------------------------------------------------------------------- | @@ -883,13 +849,11 @@ for the team to react to, not decisions. | **D. Do nothing (current)** | Zero code | Folder hides in the alphabetical sort; confusing post-upgrade | | **E. Prefix the folder name with `_`** | Dead-cheap: `_` sorts before letters, so `_Local Connections (Legacy)` floats to the top of the alphabetical list with **zero** rendering/sort changes | Cosmetic leading `_` in the name; a user can rename it away; not as loud as a toast | -> 💡 **Suggested:** **A**, optionally with **B**. The toast solves the "I didn't know my connections -> moved" moment directly; a subtle badge helps the user re-find it later without breaking the sort. -> If a code-free quick win is preferred first, **E** (prefix the name with `_`) reliably pins it to the -> top of the existing `localeCompare(..., { numeric: true })` order with no sort/render changes, and -> composes fine with A/B. +> 💡 **Suggested:** **A**, optionally with **B**. If a code-free quick win is preferred first, **E** (prefix the +> name with `_`) reliably pins it to the top of the existing `localeCompare(..., { numeric: true })` order with +> no sort/render changes, and composes fine with A/B. -### 13.2 Image-pull progress indicator (§6.1) +### O2. Image-pull progress indicator (item 19) | Option | Pros | Cons | | -------------------------------------------------------------- | ----------------------------------------------------------- | ------------------------------------------------------------------------------ | @@ -898,11 +862,10 @@ for the team to react to, not decisions. | **C. Docker Engine API / `dockerode` structured pull** | Real per-layer byte totals → an actual percentage | Larger change; new dependency/route away from the CLI-wrapper approach | | **D. "Dot per completed layer" row** | Visually pleasant for small bounded step counts | Total layer count unknown until pull starts → row grows/shrinks, reads jittery | -> 💡 **Suggested:** **B** as the near-term win (alive UI without the C rework), with **C** noted as the -> "if we want a real percentage" follow-up. Avoid **D** — the unknown-until-runtime layer count makes -> the dot row jitter. +> 💡 **Suggested:** **B** near-term (alive UI without the C rework), with **C** as the "if we want a real +> percentage" follow-up. Avoid **D** — the unknown-until-runtime layer count makes the dot row jitter. -### 13.3 Cluster commands on the Quick Start row (§7.1) +### O3. Cluster commands on the Quick Start row (item 20) | Option | Pros | Cons | | ----------------------------------------------------------------- | ----------------------------------------------------------------- | ------------------------------------------------------------------------ | @@ -910,10 +873,10 @@ for the team to react to, not decisions. | **B. Add specific storage-independent commands explicitly** | Gains the useful ones; still no storage-command risk | Each command re-declared with a Quick-Start `when`-clause | | **C. Split base contextValue into storage vs connection subsets** | Clean, reusable; Quick Start opts into the connection-only subset | Touches the base cluster item + every command's `when`; larger refactor | -> 💡 **Suggested:** **B** now (add "Open in Shell"/"New Query" against the running instance), keep **C** -> as the eventual clean-up if more items want the same split. +> 💡 **Suggested:** **B** now (add "Open in Shell"/"New Query"), keep **C** as the eventual clean-up if more +> items want the same split. -### 13.4 External-delete / Missing recovery affordance (§9.1) +### O4. External-delete / Missing recovery affordance (items 1 & 2) | Option | Pros | Cons | | ------------------------------------------------------------------ | ---------------------------------------------------------------- | ---------------------------------------------------------------------- | @@ -921,11 +884,10 @@ for the team to react to, not decisions. | **B. Detect missing → set Missing badge + info message** | Reuses the existing `Missing` rendering + `liveStateGuard` style | User still has to choose the next step | | **C. B + actionable prompt: Delete / Recreate with same settings** | One-click recovery; matches the "click to recreate" Missing row | Slightly more logic; must confirm `getReusableCredentials()` covers it | -> 💡 **Suggested:** **C**. It turns a silent dead-end into a self-service recovery, and most of the -> machinery (Missing rendering, credential reuse in `provision()`) already exists — the work is mainly -> routing `start`/`stop`/`restart` into it instead of early-returning. +> 💡 **Suggested:** **C**. Turns a silent dead-end into self-service recovery; most machinery already exists — +> the work is mainly routing `start`/`stop`/`restart` into it instead of early-returning. -### 13.5 Open terminal in container (§10.1) +### O5. Open terminal in container (item 29) | Option | Pros | Cons | | -------------------------------------------------------- | ---------------------------------------------- | ---------------------------------------------------------- | @@ -933,14 +895,13 @@ for the team to react to, not decisions. | **B. A + `/bin/sh` fallback** | Works even on minimal images | Two-attempt logic (or a probe) | | **C. Custom `Pseudoterminal` like the integrated shell** | Full control over I/O | Overkill for "just give me a shell"; more code to maintain | -> 💡 **Suggested:** **B** — the standard, low-maintenance pattern, gated on `state_running`. Reserve **C** -> only if we later want to parse/relay the shell I/O. +> 💡 **Suggested:** **B** — the standard, low-maintenance pattern, gated on `state_running`. Reserve **C** only if +> we later want to parse/relay the shell I/O. -### 13.6 Persistence & the volume model (§8.2 / §8.3 / §8.4) +### O6. Persistence & the volume model (items 22 / 23 / 24) -Three related choices about how the on-disk data volume behaves. They interact: an "ephemeral" option -only makes sense once delete reliably wipes the volume, and both are cleaner if the volume is tied to -its image. +Three related choices about the on-disk data volume. They interact: an "ephemeral" option only makes sense once +delete reliably wipes the volume, and both are cleaner if the volume is tied to its image. | Question | Options | Suggested | | -------------------------- | ----------------------------------------------------------------------------------------------------- | ----------------------------------------------------------------------------------------------- | @@ -948,7 +909,87 @@ its image. | **Delete → volume** | (A) Delete container only. (B) Delete container **+ volume** on user delete (current explicit path). | **B**, applied to _every_ user-initiated delete (incl. the future credential-missing Delete). | | **Volume ↔ image linkage** | (A) Alias-keyed volume + runtime image pinning (current). (B) Structurally key/label volume by image. | **B** — make "one image ↔ one volume" structural; no cross-image sharing (advanced users only). | -> 💡 **Suggested overall:** default stays "persistent, clean-delete, image-pinned" so the zero-decision -> path is unchanged; add the **ephemeral toggle** for throwaway trials, make the **volume↔image link -> structural**, and never share data volumes across images (advanced users can wire that up in Docker -> themselves). Pre-release, pair this with §8.5 (only ever remove containers/volumes we created). +> 💡 **Suggested overall:** default stays "persistent, clean-delete, image-pinned" so the zero-decision path is +> unchanged; add the **ephemeral toggle**, make the **volume↔image link structural**, and never share data +> volumes across images. Pre-release, pair with item 9 (only ever remove containers/volumes we created). + +### O7. Should the container stop when VS Code closes? (item 25) + +| Option | Pros | Cons | +| ------------------------------------- | -------------------------------------------------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------- | +| **A. Keep running (current)** | Survives reloads/crashes; instant to resume; matches "a local server you manage" | Orphaned container holds a port + RAM after close; surprising for users expecting tool-scoped lifetime; can pile up over time | +| **B. Stop on VS Code exit** | Tool-scoped lifetime (like a dev server); no orphans; frees the port | Loses "always-on local DB"; restart cost each session; needs a reliable shutdown hook (VS Code exit cleanup isn't guaranteed) | +| **C. Setting, default keep (+ copy)** | User chooses; safe default; makes the current behavior explicit | A setting adds surface | + +> 💡 **Suggested (for evaluation):** Lean **C** — keep the always-on default (safer, data-preserving, and +> `reconcile()` already relies on it), but add a **setting** ("Stop the local instance when VS Code closes") and +> make the always-on behavior **explicit** in the UI. Genuinely open — evaluate against how users actually treat +> the instance (throwaway vs always-on). + +--- + +## Appendix A — current webview flow (reference) + +The setup panel is one React component driven by a `Phase` state machine +(`'loading' | 'review' | 'dockerNotReady' | 'provisioning' | 'success' | 'failed'` in +[LocalQuickStart.tsx](../../../../src/webviews/documentdb/localQuickStart/LocalQuickStart.tsx)). On open it +queries `getDockerStatus`, then branches: + +```text + panel opens + | + v + +-----------------+ getDockerStatus() + | loading |------------------------------. + | (BARE spinner: | | + | "Checking | Docker not ready v + | Docker..." | +----------------------------+ + | no hero/cards) |--------->| dockerNotReady | + +-----------------+ | hero "Docker is required" | + | | 3 readiness cards | + Docker ready | (CLI / daemon / platform) | + | | + "How to fix" + [Retry] ---+ (re-query) + | +----------------------------+ + | + ready & canResumeReadiness? --- yes --> jump to [failed / timedOut] (resume a prior + | timed-out container: Wait longer / Start over) + | no + v + +-----------------+ [Start DocumentDB Local] + | review |----------------------------. + | hero "Start | | + | DocumentDB | v + | Local" | +----------------------+ + | 4 config cards | | provisioning | + | (Image / Port / | | hero "Setting up..." | + | Data / Security| | elapsed timer | + | + Advanced | <-- [Edit | stage checklist: | + | (collapsed)) | settings] --| checking > pulling > | + | + [Start] | | creating > starting >| + +-----------------+ | waiting > done | + ^ | [Cancel] [View output]| + | +----------+-----------+ + | [Start over] success | | failed / timeout + | v v + | +-----------+ +----------------------+ + +------------------- | success | | failed | + | Next steps| | error box | + | [Open] [Copy] | [Retry][Edit] OR | + | [Close] | | [Wait longer][Start | + +-----------+ | over] (on timeout) | + +----------------------+ +``` + +Phase-by-phase: + +- **loading** — **only** a full-panel `` (no hero/cards; item 12). +- **dockerNotReady** — `hero("Docker is required")` + three readiness cards (Docker CLI / daemon / Platform, each + with a ✓/! badge) + a "How to fix" card + `Retry`. +- **review** — `hero("Start DocumentDB Local")` + four config cards (Image / Port / Data / Security) + a summary + + a collapsed **Advanced** panel + `Start DocumentDB Local`. +- **provisioning** — `hero("Setting up DocumentDB Local…")` + elapsed timer + the stage checklist (checking → + pulling → creating → starting → waiting → done) + `Cancel` + `View Docker output`. +- **success** — header still reads "Setting up…" (bug, item 3) + a success box + Next steps + `Open Connection` / + `Copy Connection String` / `Close`. +- **failed** — header still reads "Setting up…" (item 3) + an error box + either `Retry` / `Edit settings`, or (on + a readiness timeout) `Wait longer` / `Start over`. From bdb4cde1991fe924d96d6d7fbfe91753b860567f Mon Sep 17 00:00:00 2001 From: Tomasz Naumowicz Date: Thu, 9 Jul 2026 22:12:10 +0200 Subject: [PATCH 7/7] refactor: update UX review document with TN recommendations and status clarifications --- .../PRs/documentdb-quickstart/ux-review.md | 289 +++++++++++++----- 1 file changed, 219 insertions(+), 70 deletions(-) diff --git a/docs/ai-and-plans/PRs/documentdb-quickstart/ux-review.md b/docs/ai-and-plans/PRs/documentdb-quickstart/ux-review.md index a9f35dbdf..44edc661d 100644 --- a/docs/ai-and-plans/PRs/documentdb-quickstart/ux-review.md +++ b/docs/ai-and-plans/PRs/documentdb-quickstart/ux-review.md @@ -41,11 +41,11 @@ Heavier design questions with real trade-offs are pulled into [Open ideas](#open ### Status -| Status | Meaning | -| ------------------- | ------------------------------------------------------------------------------------------- | -| 🟠 **Open** | Recorded + analyzed; **the operator must choose the direction** (see the item's Suggestion) | -| 🟡 **Acknowledged** | Known and accepted; not planned now — may be revisited | -| ✅ **Implemented** | A change was made on this branch and verified | +| Status | Meaning | +| ------------------ | ---------------------------------------------------------------------------------------------------------------------------------------------- | +| 🟠 **Open** | Recorded + analyzed; carries a **TN recommendation** ("TN leans towards …") but stays **Open** — a _suggestion_ the team may revisit / replace | +| 🟡 **Open (soft)** | Open, but the recommendation depends on a **technical investigation** (item 25) or is a **soft "leave as-is"** (item 27) | +| ✅ **Implemented** | A change was made on this branch and verified | ### Markers (inline) @@ -55,10 +55,12 @@ Heavier design questions with real trade-offs are pulled into [Open ideas](#open | 💡 **Suggestion** | A design/wording recommendation to react to | | 🔍 **Answered** | A "how does this work?" question answered from the code | -> **For the operator:** almost everything below is **Open**. Each item states a recommended direction -> in its **Suggestion** (and, where there are real trade-offs, an entry under -> [Open ideas](#open-ideas--options-pros--cons)). Pick a direction per item; the review does not decide -> for you. Exactly **one** item is already **Implemented** (item 30). One is **Acknowledged** (item 27). +> **For the operator:** almost everything below is **Open**. Each item records a **TN recommendation** +> ("TN leans towards … because …") — but these are **suggestions, not final decisions**: the team may +> revisit and propose another approach. **TN is open to discussion — if you disagree with a +> recommendation, raise it.** Where there are real trade-offs, see the matching entry under +> [Open ideas](#open-ideas--options-pros--cons). Exactly **one** item is already **Implemented** +> (item 30). --- @@ -81,41 +83,48 @@ webview flow. ## Priority index -| # | Priority | Item | Status | -| --- | -------- | ----------------------------------------------------- | --------------- | -| 1 | **P0** | Credential-missing state is a restart dead end | 🟠 Open | -| 2 | **P0** | External container delete → silent no-op on Start | 🟠 Open | -| 3 | **P1** | Webview header stuck on "Setting up…" | 🟠 Open | -| 4 | **P1** | Tree nodes misused to display errors | 🟠 Open | -| 5 | **P1** | Error/status messages hijack the hero | 🟠 Open | -| 6 | **P1** | "Delete deletes data" vs "recreate/recover" clash | 🟠 Open | -| 7 | **P1** | Copy Connection String silently copies the password | 🟠 Open | -| 8 | **P1** | Delete bypasses the shared confirmation (+ em dashes) | 🟠 Open | -| 9 | **P1** | Only ever delete containers we created | 🟠 Open | -| 10 | **P2** | Legacy migration folder hides in the sort | 🟠 Open | -| 11 | **P2** | Phase out the "local connection" concept | 🟠 Open | -| 12 | **P2** | "Checking Docker" is a bare spinner | 🟠 Open | -| 13 | **P2** | Advanced: creds toggle + ambiguous placeholders | 🟠 Open | -| 14 | **P2** | "Cancel" implies a rollback it doesn't do | 🟠 Open | -| 15 | **P2** | No "What was done" summary on completion | 🟠 Open | -| 16 | **P2** | No upfront "first run takes a few minutes" | 🟠 Open | -| 17 | **P2** | "Load sample data" is all-or-nothing | 🟠 Open | -| 18 | **P2** | "Waiting…" static for minutes | 🟠 Open | -| 19 | **P2** | No image-pull progress | 🟠 Open | -| 20 | **P2** | Running row lacks regular cluster commands | 🟠 Open | -| 21 | **P2** | Credential storage is a bespoke plainer path | 🟠 Open | -| 22 | **P2** | Always persistent — no ephemeral option | 🟠 Open | -| 23 | **P2** | User delete must always drop the volume | 🟠 Open | -| 24 | **P2** | Volumes not structurally linked to the image | 🟠 Open | -| 25 | **P2** | Container keeps running after VS Code closes | 🟠 Open | -| 26 | **P2** | Tree state changes not announced (accessibility) | 🟠 Open | -| 27 | **P3** | Docker prereq only revealed after the click | 🟡 Acknowledged | -| 28 | **P3** | Generic `$(plug)` icon, no distinct identity | 🟠 Open | -| 29 | **P3** | "Open terminal in the container" | 🟠 Open | -| 30 | **P1\*** | Empty-state tree row reworded / "Learn more" dropped | ✅ Implemented | +| # | Priority | Item | Status | +| --- | -------- | ----------------------------------------------------- | -------------- | +| 1 | **P0** | Credential-missing state is a restart dead end | 🟠 Open | +| 2 | **P0** | External container delete → silent no-op on Start | 🟠 Open | +| 3 | **P1** | Webview header stuck on "Setting up…" | 🟠 Open | +| 4 | **P1** | Tree nodes misused to display errors | 🟠 Open | +| 5 | **P1** | Error/status messages hijack the hero | 🟠 Open | +| 6 | **P1** | "Delete deletes data" vs "recreate/recover" clash | 🟠 Open | +| 7 | **P1** | Copy Connection String silently copies the password | 🟠 Open | +| 8 | **P1** | Delete bypasses the shared confirmation (+ em dashes) | 🟠 Open | +| 9 | **P1** | Only ever delete containers we created | 🟠 Open | +| 10 | **P2** | Legacy migration folder hides in the sort | 🟠 Open | +| 11 | **P2** | Phase out the "local connection" concept | 🟠 Open | +| 12 | **P2** | "Checking Docker" is a bare spinner | 🟠 Open | +| 13 | **P2** | Advanced: creds toggle + ambiguous placeholders | 🟠 Open | +| 14 | **P2** | "Cancel" implies a rollback it doesn't do | 🟠 Open | +| 15 | **P2** | No "What was done" summary on completion | 🟠 Open | +| 16 | **P2** | No upfront "first run takes a few minutes" | 🟠 Open | +| 17 | **P2** | "Load sample data" is all-or-nothing | 🟠 Open\* | +| 18 | **P2** | "Waiting…" static for minutes | 🟠 Open\* | +| 19 | **P2** | No image-pull progress | 🟠 Open\* | +| 20 | **P2** | Running row lacks regular cluster commands | 🟠 Open | +| 21 | **P2** | Credential storage is a bespoke plainer path | 🟠 Open | +| 22 | **P2** | Always persistent — no ephemeral option | 🟠 Open | +| 23 | **P2** | User delete must always drop the volume | 🟠 Open | +| 24 | **P2** | Volumes not structurally linked to the image | 🟠 Open | +| 25 | **P2** | Container keeps running after VS Code closes | 🟡 Open\* | +| 26 | **P2** | Tree state changes not announced (accessibility) | 🟠 Open | +| 27 | **P3** | Docker prereq only revealed after the click | 🟡 Open\* | +| 28 | **P3** | Generic `$(plug)` icon, no distinct identity | 🟠 Open | +| 29 | **P3** | "Open terminal in the container" | 🟠 Open | +| 30 | **P1\*** | Empty-state tree row reworded / "Learn more" dropped | ✅ Implemented | > \* Item 30 was a P1-ish wording nit; it's the one item already shipped on this branch, so it's parked > at the end under Implemented. +> +> **Status key:** every item is **🟠 Open** (a few are **🟡 Open (soft)**), plus one **✅ Implemented** +> (item 30). Each Open item carries a **TN recommendation** recorded inline as _"TN leans towards …"_ — +> **a suggestion, not a decision**; the team may revisit. An **Open\*** (items 17, 18, 19) means the +> recommended _direction_ is set but an **investigation** is required before/while implementing (data +> source, real progress signal). **🟡 Open\*** (item 25) means the recommendation (keep always-on) holds +> pending a technical check. **TN is open to discussion — disagree freely.** --- @@ -123,7 +132,12 @@ webview flow. ### 1. Credential-missing state after a restart is a UI dead end (the demo incident) ⚠️ -**Priority:** P0 · **Status:** 🟠 Open — operator to choose the direction (see Suggestion + Open idea O4). +**Priority:** P0 · **Status:** 🟠 Open — TN leaning (a suggestion; the team may revisit). + +> **TN leans towards Delete-only** (no Recreate here) — keep it **clean and simple**. Communicate +> clearly to the user that this is a **corrupted / unusable state that needs to be removed**, and offer +> a single **Delete** action. _Because a lost-secret state is rare, and a clear "this is broken, remove +> it and start over" reads better than trying to recover onto an opaque on-disk volume._ **Observation (real incident, mid-review):** An instance was created via Quick Start, then stopped, then its container/image were removed outside VS Code. After a **VS Code restart** the Quick Start node @@ -163,7 +177,12 @@ idea O4; a direct instance of item 4's principle. ### 2. A container deleted _outside_ VS Code isn't handled on Start (silent no-op) ⚠️ -**Priority:** P0 · **Status:** 🟠 Open — operator to choose the direction (see Suggestion + Open idea O4). +**Priority:** P0 · **Status:** 🟠 Open — TN leaning (a suggestion; the team may revisit). + +> **TN leans towards "show as Missing + a Delete/Remove action"** (not a silent removal) so the user +> **notices something happened**; put the extra detail in the **tooltip**. Keep it **good-enough and +> low-cost** — it's an edge case, so the fix should be solid but not expensive. _Because the goal is +> visible feedback plus a clear way out, not a polished recovery flow._ **Observation (repro):** Created an instance via Quick Start, stopped it, then deleted the _container_ directly via Docker. Back in VS Code, clicking Start on the still-"Stopped"-looking row does nothing @@ -207,7 +226,11 @@ covers most of it — confirm once reachable). Options in Open idea O4. ### 3. The webview header changes with state and gets stuck on "Setting up…" ⚠️ -**Priority:** P1 · **Status:** 🟠 Open — operator to choose the direction. +**Priority:** P1 · **Status:** 🟠 Open — TN leaning (a suggestion; the team may revisit). + +> **TN leans towards a fixed header area** — static across all phases; all state lives in the body / +> subtitle. _Because the header jumping (and sticking on "Setting up…") is unexpected; a constant +> header is calmer and removes the stuck-text bug outright._ **Observation:** The header should be static. Today it changes with status/progress/errors, and it sometimes stays on "Setting up DocumentDB Local…" even when setup is actually complete. @@ -233,7 +256,12 @@ already does. ### 4. Tree nodes are misused to display errors (they should be actions, not dialogs) ⚠️💡 -**Priority:** P1 · **Status:** 🟠 Open — operator to choose the direction. +**Priority:** P1 · **Status:** 🟠 Open — TN leaning (a suggestion; the team may revisit). + +> **TN leans towards unifying** — adopt the rule feature-wide: errors go to a **modal** (+ the output +> channel for detail), and tree rows stay **actionable only**. _Because it matches the rest of the +> extension (the Kubernetes feature already did this) and removes the passive error rows at the root, +> which also fixes item 1's dead end._ **Observation:** A broader pattern problem exposed by the incident (item 1) — we render an **error message as a tree node**. Leaf/error nodes should be **actions the user can take**, not a substitute for a dialog @@ -260,7 +288,11 @@ surface with state it isn't meant to hold" theme. ### 5. Error/status messages must leave the hero — show a content-area card; run a small UX experiment ⚠️💡 -**Priority:** P1 · **Status:** 🟠 Open — operator to choose the direction (a UX experiment is requested). +**Priority:** P1 · **Status:** 🟠 Open — TN leaning (a suggestion; the team may revisit). + +> **TN leans towards static hero + inline error card** (this was my original suggestion when I reported +> the issue). Prototype variants **A/B/C** and pick per-state, with **Retry on the card**. _Because the +> message belongs in the content area as an actionable card, not in the hero._ **Observation:** We have error/status states (e.g. Docker missing). The core requirement: these messages **must not live in the hero section** — the hero stays constant and the message becomes a **card in the @@ -303,7 +335,14 @@ should **render a few options to compare**. We also need to decide **where Retry ### 6. "Delete deletes data" vs "Recreate / recover" — resolve the contradiction ⚠️💡 -**Priority:** P1 · **Status:** 🟠 Open — operator to choose the direction (a discussion is requested). +**Priority:** P1 · **Status:** 🟠 Open — TN leaning (a suggestion; the team may revisit). + +> **TN leans towards no recovery at all** — **Delete always wipes everything**; there is no "keep data" +> / Recreate-onto-surviving-volume path. Keep the code **and** the UX simple. Advanced data-retention +> scenarios are **out of scope** — users who want to retain data can manage their local instances with +> the Docker CLI/tools directly. Missing/error copy just states the instance is gone and should be +> removed; no contradiction because there is no recovery to promise. _Because edge cases should stay +> simple and cheap, not grow a recovery subsystem._ **Observation:** We tell the user that **deleting deletes the data** — so then what does **recreate / recovery** mean? The two messages appear to contradict each other. @@ -332,7 +371,11 @@ recovery** mean? The two messages appear to contradict each other. ### 7. "Copy Connection String" is a separate impl and silently includes the password ⚠️ -**Priority:** P1 · **Status:** 🟠 Open — operator to choose the direction. +**Priority:** P1 · **Status:** 🟠 Open — TN leaning (a suggestion; the team may revisit). + +> **TN leans towards reuse** — use the standard `copyConnectionString()` with/without-password QuickPick +> for parity, instead of the separate silent-copy path. _Because it's a plaintext secret going to the +> clipboard; consistency with every other connection wins._ **Observation:** Copy Connection String seems reimplemented for Quick Start and behaves slightly differently — it doesn't ask whether to include the password. @@ -354,7 +397,11 @@ incidental divergence. ### 8. "Delete Container" bypasses the shared confirmation and uses em dashes ⚠️ -**Priority:** P1 · **Status:** 🟠 Open — operator to choose the direction. +**Priority:** P1 · **Status:** 🟠 Open — TN leaning (a suggestion; the team may revisit). + +> **TN leans towards switching** to `getConfirmationAsInSettings()` (so it honors the user's +> confirmation style) **and updating the wording** — including the feature-wide em-dash sweep. _Because +> a destructive action should match every other destructive command in the extension._ **Observation:** The Delete Container modal is fine conceptually, but it should use the same delete confirmation code path as other destructive actions (like in Settings), and shouldn't use em dashes. @@ -380,7 +427,13 @@ sweep** of the whole feature's user-facing strings rather than fixing only the d ### 9. We should only ever delete containers we created (pre-release safety) ⚠️ -**Priority:** P1 · **Status:** 🟠 Open — operator to choose the direction (pre-release must-fix). +**Priority:** P1 · **Status:** 🟠 Open — TN leaning (a suggestion; the team may revisit). + +> **TN leans towards a hard label precondition on every removal, no exceptions.** Re-verify the +> `vscode.documentdb.quickstart` label immediately before `removeContainer`/`removeVolume`. If a +> container is **not** ours, **do not delete it** — show a warning that it can't be removed because it +> was **created outside the extension**. _Because we must never remove a container/volume we didn't +> create._ **Observation:** Not urgent now, but must be addressed **before final release**: we should be allowed to delete **only** the containers we created. @@ -402,7 +455,11 @@ user-named containers, more recovery flows), an id-based delete without a label ### 10. Legacy emulator connections migrate to a folder that can hide in the sort order ⚠️🔍 -**Priority:** P2 · **Status:** 🟠 Open — operator to choose the direction (see Open idea O1). +**Priority:** P2 · **Status:** 🟠 Open — TN leaning (a suggestion; the team may revisit). + +> **TN leans towards the cheap `_`-prefix only** (Open idea O1 · E) — rename the folder to +> `_Local Connections (Legacy)` so it floats to the top with zero sort/render code. **No toast, no +> badge** for now. _Because the user base is still small; this isn't worth a bigger investment yet._ **Observation:** Is the one-time migration of original "DocumentDB Local" emulator connections still happening? What's the folder called? Sorting could place it somewhere unexpected. @@ -429,7 +486,11 @@ See Open idea O1. ### 11. Phase out the "local connection" concept — a localhost connection is just a connection 💡 -**Priority:** P2 · **Status:** 🟠 Open — operator to choose the direction (a cleanup epic, not one PR). +**Priority:** P2 · **Status:** 🟠 Open — TN suggests deferring to a follow-up issue (the team may revisit). + +> **TN leans towards a separate follow-up issue** — this is a larger cleanup and **should not be part of +> this PR.** Track it independently and sequence it after the Quick Start bugs land. _Because bundling a +> broad deprecation into this PR would balloon its scope._ **Observation:** Ideally we **phase out** the notion of a special "local connection" entirely. A connection to `localhost` is just a **regular connection that happens to point at localhost**. Once that lands there is no @@ -453,7 +514,11 @@ both fold into this end state. ### 12. The initial "Checking Docker" loading should render chrome + skeleton cards (reuse the metric card) ⚠️💡 -**Priority:** P2 · **Status:** 🟠 Open — operator to choose the direction. +**Priority:** P2 · **Status:** 🟠 Open — TN leaning (a suggestion; the team may revisit). + +> **TN leans towards both** — render the chrome + skeleton cards immediately (fill independently) **and** +> reuse + relocate the accessible Query Insights `MetricBase`. _Because it fixes the snap-in jump and +> gives us one shared, accessible metric card._ **Observation:** The initial `loading` ("Checking Docker") state should behave like the **Query Insights** view: the chrome renders immediately (title + header), and the cards behave like our **metrics cards** — a @@ -479,7 +544,12 @@ Start is the wrong dependency direction). Worth it: one accessible, tooltip-capa ### 13. Advanced panel: credentials should sit behind a toggle, and "placeholder = default" is ambiguous ⚠️💡 -**Priority:** P2 · **Status:** 🟠 Open — operator to choose the direction (options to weigh below). +**Priority:** P2 · **Status:** 🟠 Open — TN leaning (a suggestion; the team may revisit). + +> **TN leans towards: (1) credentials behind an "override" toggle**, and **(2) placeholder Option A** — +> the default lives **only in the hint line**, with an explanation that **the default is applied when the +> field is left empty** (no literal-value ghost text). Apply the same convention to every Advanced field. +> _Because "blank = default" only reads honestly when the box actually looks empty._ **Observation:** Username + password should be grouped **behind a toggle** — the user flips "override the default" and only then specifies them. The ghost text **"auto"** is confusing (is it the default _value_?). @@ -508,7 +578,11 @@ Advanced field. ### 14. What does "Cancel" do mid-provision? Consider the verb "Abort" (no full rollback promise) ⚠️💡 -**Priority:** P2 · **Status:** 🟠 Open — operator to choose the direction. +**Priority:** P2 · **Status:** 🟠 Open — TN leaning (a suggestion; the team may revisit). + +> **TN leans towards the "Abort" wording**, and — **even on abort** — **show the "what was done" summary** +> (cf. item 15) so there's full clarity about what happened (e.g. the image was kept). _Because "Abort" +> sets honest expectations and the summary removes any ambiguity about residual state._ **Observation:** Investigate what **Cancel** does while provisioning is in progress. Do we **delete an image that started downloading**, or just abort? A clean rollback is hard to promise, so **"Abort"** may be a cleaner @@ -532,7 +606,11 @@ by the button ("Stops setup. The downloaded image is kept so a retry is faster." ### 15. On completion, show a "What was done" summary card — below the content ⚠️💡 -**Priority:** P2 · **Status:** 🟠 Open — operator to choose the direction. +**Priority:** P2 · **Status:** 🟠 Open — TN leaning (a suggestion; the team may revisit). + +> **TN leans towards yes** — mirror the review screen: **"what we'll do" → "what was done."** It must be +> easy to grasp at a glance (users are busy). _Because the symmetry makes the outcome obvious and +> surfaces the real bound port/creds._ **Observation:** When a deployment completes, the info card should render **below** the other content (not above), and carry a **summary of what was done** — a table-ish block like the review screen's "what we'll do" @@ -555,7 +633,11 @@ the real port. ### 16. Set an upfront "the first run can take a few minutes" expectation 💡 -**Priority:** P2 · **Status:** 🟠 Open — operator to choose the direction. +**Priority:** P2 · **Status:** 🟠 Open — TN leaning (a suggestion; the team may revisit). + +> **TN leans towards yes** — have the one-liner ready (review screen and/or provisioning subtitle), +> distinguishing first-run from a restart. _Because it reframes a long cold start from "stuck" to +> "expected."_ **Observation:** Whatever we can do to set expectations up front — a cold first run is genuinely long. @@ -570,7 +652,13 @@ from a restart. ### 17. "Load sample data" is all-or-nothing — add a choice of test data ⚠️💡 -**Priority:** P2 · **Status:** 🟠 Open — operator to choose the direction. +**Priority:** P2 · **Status:** 🟠 Open — TN leaning; needs investigation (the team may revisit). + +> **TN leans towards a multiselect dropdown** of datasets to preload — **but the data source needs +> discussion.** The current tiny "5 docs per container" sample is weak; consider a **custom-file picker** +> (the Docker image may support loading one). **Whoever implements this must investigate** where the data +> should come from and what the image supports. _Because a real, richer, choosable dataset is far more +> useful than a fixed toy sample._ **Observation:** The sample-data option is all-or-nothing today. We'll have to add **options to choose the test data** (which dataset, or none). @@ -585,7 +673,14 @@ ideally with a one-line description of each. Keep exec-once semantics per datase ### 18. "Waiting for DocumentDB to accept connections" can sit static for minutes ⚠️ -**Priority:** P2 · **Status:** 🟠 Open — operator to choose the direction. +**Priority:** P2 · **Status:** 🟠 Open — TN leaning; needs investigation (the team may revisit). + +> **TN leans towards no fake sub-timer / no simulated progress** (it doesn't help). Prefer **real +> signal**; if none is available, iterate a **rotating one-liner** (toggle ~every 2 s) that explains +> what's happening in general — e.g. _"waiting for the container to initialize"_, _"waiting for data to +> preload"_, _"waiting for network config…"_ — to keep the user engaged. **Real data would be truly +> better — investigative task.** _Because honest signal beats a spinner, and simulated progress is +> worse than none._ **Observation:** This message could be reworded, or show intermediate progress as sub-info, since it can take a while. @@ -603,7 +698,14 @@ surfacing "View Docker output" _during_ this stage (it currently only appears at ### 19. No structured progress while the image downloads ⚠️ -**Priority:** P2 · **Status:** 🟠 Open — operator to choose the direction (see Open idea O2). +**Priority:** P2 · **Status:** 🟠 Open — TN leaning; needs investigation (the team may revisit). + +> **TN leans towards keeping the spinner** (we already show one per sub-progress stage) and adding +> **extra info** rather than a fake intermediate bar. Ask the implementer to **experiment**: an +> **N-of-M layers** progress bar could still help even though **M is a moving target** (our knowledge of +> the layer count grows during the pull); or better, a **megabytes-downloaded counter** — showing data +> flowing is a strong "something is happening" signal. **Investigate** what real numbers Docker gives us +> (layer counts and/or byte totals). _Because real throughput/bytes beats a decorative bar._ **Observation:** Do we track progress while the image is pulled? It's a long-running op too. @@ -620,7 +722,15 @@ lines). Full options in Open idea O2. ### 20. The running-instance row doesn't share regular cluster context commands 🔍 -**Priority:** P2 · **Status:** 🟠 Open — operator to choose the direction (see Open idea O3). +**Priority:** P2 · **Status:** 🟠 Open — TN leaning (a suggestion; the team may revisit). + +> **TN leans towards keeping the replaced contextValue** rather than filtering Quick Start out of every +> menu. Adding `&& not quickstart` to every generic cluster command is worse than adding `|| quickstart` +> to the **few** we want (e.g. Open in Shell). Model the Quick Start row as a **sub-view** of the +> connections view: **replace the connections-view context entry with a sub-view marker**, then +> **explicitly opt specific commands in via `or`**. Ask the implementer to **spend time getting this +> simple and maintainable** for future maintainers (the extra `view`/sub-view layer adds complexity if +> done carelessly). _Because opt-in on a short allowlist is cleaner than opt-out on everything._ **Observation:** The running managed instance doesn't share context-menu commands with regular clusters. Do we still extend the cluster base class? If not, what would break if we did? @@ -644,7 +754,11 @@ Open idea O3. ### 21. Credential storage: reuse the extension's secure-storage patterns 💡 -**Priority:** P2 · **Status:** 🟠 Open — operator to choose the direction. +**Priority:** P2 · **Status:** 🟠 Open — TN leaning (a suggestion; the team may revisit). + +> **TN leans towards attempting to reuse** the `ConnectionStorageService` secure-storage patterns. Can +> be a follow-up, but **better in this release** so we don't have to deal with a connection/credential +> **migration** later. _Because doing it now avoids a migration debt._ **Observation:** Flag the plaintext-password posture. Everything else is careful (masked logs, env-files), yet the generated password is embedded in the stored connection string and is one-click copyable/visible. **Think @@ -667,7 +781,10 @@ connections, they inherit this storage naturally. ### 22. Persistence is always on — there's no "ephemeral / no-persistence" option 💡 -**Priority:** P2 · **Status:** 🟠 Open — operator to choose the direction (see Open idea O6). +**Priority:** P2 · **Status:** 🟠 Open — TN leaning (a suggestion; the team may revisit). + +> **TN leans towards adding the ephemeral toggle** ("Persist data across restarts," default on). _Because +> more options here are easy for the user and genuinely helpful for throwaway trials._ **Observation:** Our default is a persistent data volume. That should be a **toggle** — sometimes a user wants a throwaway instance with no persistence. @@ -684,7 +801,11 @@ delete — good for throwaway trials. Trade-offs in Open idea O6. ### 23. A user-initiated delete must always delete the data volume ⚠️ -**Priority:** P2 · **Status:** 🟠 Open — operator to choose the direction. +**Priority:** P2 · **Status:** 🟠 Open — TN leaning (a suggestion; the team may revisit). + +> **TN leans towards always dropping the volume** on a user delete (every user-initiated delete routes +> through `deleteContainer()`). _Because minimizing moving parts lowers the user's mental load — fewer +> things to understand, more happiness._ **Observation:** When the user deletes a container, we should for sure delete the persisted volume as well. @@ -701,7 +822,10 @@ reconcile paths keep R2 (never auto-wipe without the user choosing it). ### 24. Data volumes should be linked to the image — no shared data volumes 💡 -**Priority:** P2 · **Status:** 🟠 Open — operator to choose the direction (see Open idea O6). +**Priority:** P2 · **Status:** 🟠 Open — TN leaning (a suggestion; the team may revisit). + +> **TN leans towards structural image↔volume linkage, no cross-image sharing.** _Because it keeps the +> model simple to understand for the user (one image, one volume)._ **Observation:** Our persisted volumes should be **linked to the image**. We won't share data volumes across images; advanced users can do that themselves. @@ -718,7 +842,14 @@ leave that to advanced users operating Docker directly. ### 25. The container keeps running after VS Code closes — decide the desired behavior ⚠️💡 -**Priority:** P2 · **Status:** 🟠 Open — genuinely undecided; needs a pros/cons evaluation (Open idea O7). +**Priority:** P2 · **Status:** 🟡 Open · investigate first — TN leans to keeping always-on (the team may revisit). + +> **TN leans towards always-on** for now. The "stop on exit" setting is **worrying**: on extension +> **deactivation** we may not get enough time to clean up before the process is killed — this could be a +> real problem. **Investigate first** how much time VS Code actually gives us on deactivate. Only if we +> truly have that control should we consider a **modal on quit** ("keep the container running?"). The +> suspicion is that we **don't** have that level of control in extensions, so the default stays +> always-on. _Because promising a clean stop we can't guarantee is worse than a clear always-on._ **Observation:** Non-standard for an extension-managed local resource (a user may expect it to stop like a dev server), and can leave an orphaned container holding a port + RAM. @@ -732,7 +863,11 @@ deciding; whatever is chosen should be **explicit** (a setting and/or clear copy ### 26. Tree state transitions aren't announced — accessibility work item (post-redesign; discuss with Tomasz) ⚠️ -**Priority:** P2 · **Status:** 🟠 Open — sequenced work item; **owner/approach to be agreed with Tomasz.** +**Priority:** P2 · **Status:** 🟠 Open — TN leaning; post-redesign a11y work item (the team may revisit). + +> **TN leans towards confirmed** — keep this as a **sequenced post-webview-redesign** accessibility work +> item, applying past accessibility-testing learnings; **owner/approach to be agreed with Tomasz.** No +> change now. **Observation:** The webview is carefully accessible, but the **tree** state rows (Starting / Stopping / Missing / Running) change only their `description` text + icon, with no live-region announcement, so screen-reader users @@ -754,7 +889,12 @@ Tomasz** before implementation. ### 27. Docker prerequisite is only revealed after opening the webview 🔍 -**Priority:** P3 · **Status:** 🟡 Acknowledged — "it is what it is" for now; may be revisited. +**Priority:** P3 · **Status:** 🟡 Open — TN suggests leaving as-is, no tooltip (the team may revisit). + +> **TN leans towards no tooltip** — users will open the webview and find out that Docker is required, +> which is **good enough**. A prerequisite hint may matter more in the future if we add **other container +> runtimes** beyond Docker. _Because it's a cheap-but-unnecessary hint for a single-runtime feature +> today._ **Observation:** The empty-state row invites "Click here to start DocumentDB Local," but the hard **Docker requirement** only surfaces once the webview opens (the `dockerNotReady` screen). Recorded so it isn't @@ -769,7 +909,11 @@ tooltip / description ("Requires Docker") or a hover. ### 28. Icons are all inherited/generic — nothing marks "this is the Quick Start instance" 🔍💡 -**Priority:** P3 · **Status:** 🟠 Open — operator to choose the direction. +**Priority:** P3 · **Status:** 🟠 Open — TN leaning (a suggestion; the team may revisit). + +> **TN leans towards a better, distinct icon set** — use the **rocket for user-created Quick Start +> instances** (so they're easy to tell apart) and **something else** for other/ambient images that just +> happen to be around. _Because the icon should signal "this is the one you created via Quick Start."_ **Observation:** How are the icons chosen for the Quick Start instances? @@ -785,7 +929,12 @@ a glance. Low priority, but cheap. ### 29. "Open terminal in the container" — feasible, no blocker 🔍💡 -**Priority:** P3 · **Status:** 🟠 Open — operator to choose the direction (see Open idea O5). +**Priority:** P3 · **Status:** 🟠 Open — TN leaning; do it if cheap, else file an issue (the team may revisit). + +> **TN leans towards doing it if cheap** (it helps advanced users) — the standard `createTerminal` + +> `docker exec -it` (bash → sh fallback), gated on `state_running`. If it turns out non-trivial, +> **file a new `enhancement`-tagged issue and move on.** _Because it's a nice power-user affordance, not +> a blocker._ **Observation:** Would a command to open a terminal inside the Docker container make sense? Can we do it?