fix(local-quickstart): Quick Start UX review follow-ups (delete safety, credential recovery, copy, header, reveal)#794
Merged
guanzhousongmicrosoft merged 6 commits intoJul 14, 2026
Conversation
…cker changes (#2, #9) UX review #2 (P0): a container deleted outside VS Code made Start/Stop/Restart a silent no-op. The guard `!id || !isManaged(id) || !liveStateGuard(...)` short- circuited on `isManaged` (false for a gone container) before the missing- detection ran, so the click did nothing and a stale "Stopped" row lingered. Replace the isManaged+liveStateGuard chain with a single `ensureActionable()` that does one inspect and distinguishes missing / foreign / wrong-live-state: - missing (removed externally): mark the instance Missing directly (the tree row offers click-to-recreate + Delete Container) and show a recovery message. Recreate reuses the existing data volume, so data is preserved. - foreign (id/name resolves to a container we did not create): warn and refuse, never marking it as our Missing instance. - drift (another window changed it): correct the state via setStatus (the old refreshLiveState call was itself a no-op mid-lifecycle) and inform the user. UX review #9: `deleteContainer()` removed on `entry.missing` WITHOUT re-checking ownership, and `metadata.containerId` can be a container NAME a foreign container may later resolve to. Add a hard label precondition at every removal site via the new pure `isOwnedContainer(item, alias)` predicate: re-inspect once and remove the container only when it still carries our label/alias; a foreign container is left untouched (abort, remove nothing incl. the volume); a gone container falls through to clear only our own alias-derived volume + records. Also add a tooltip to the Missing tree row (detail on hover, per TN). Tests: add regressions for the #2 silent no-op, the #9 refuse-foreign-delete path (with entry.missing set, the exact old bypass), and multi-window drift. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8e6849f7-885d-4364-a467-867e10ae56f1
…e Delete-only row (#1) UX review #1 (P0): after a restart, an instance whose container/`ready` record exists but whose saved credentials are gone was a UI dead end. reconcile Case 4 set InstanceState.Error with metadata undefined, so the tree fell through to the rocket empty-state PLUS a passive treeItem_quickStartError row that had no command and no context menu. Delete's when-clause needs treeItem_quickStartInstance + a state_* token, so Delete was unreachable and the user was stuck (Docker CLI only), reproduced on every restart. - Add InstanceState.CredentialsMissing; the two credential-unavailable sites (reconcile Case 4 + provision RR4 volume-wipe gate) now set it instead of Error, and the message is localized via credentialUnavailableMessage(). - Render an actionable, Delete-only instance row for it (treeItem_quickStartInstance + state_credentialsMissing, warning icon, description + tooltip, no click command per TN's Delete-only recommendation). - package.json: the Delete when-clause includes credentialsMissing; the View Logs when-clause is gated to states where logs are meaningful so the row stays Delete-only. - refreshLiveState skips CredentialsMissing so a within-session transition that retains stale metadata can't be re-inspected back to Running/Stopped. Delete recovers the state via the live-lookup + ownership-verified removal path. Data-safety (R2) is preserved: entering the state removes nothing. Tests: update the 4 credential-unavailable assertions to CredentialsMissing; add a LocalQuickStartItem test locking the row's contextValue tokens + no click command. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8e6849f7-885d-4364-a467-867e10ae56f1
…m dashes (#8)
UX review #8 (P1): "Delete Container" used a bespoke getConfirmationWithClick()
modal instead of the shared getConfirmationAsInSettings() that every other
destructive command uses, so it ignored the user's configured confirmationStyle
(word / challenge / click). Feature strings also used em dashes, against the repo
convention for user-facing text.
- deleteQuickStartInstance now calls
getConfirmationAsInSettings(l10n.t('Delete DocumentDB Local container?'), detail,
'delete') — same helper + confirmation word as removeConnection/deleteFolder.
On cancel the word/challenge styles throw UserCancelledError, which the azext
registerCommand wrapper absorbs as a silent cancel (as for the other deletes);
the click style returns false and hits `if (!confirmed) return;`.
- Sweep user-facing em dashes: the two delete detail strings, three LocalQuickStart
webview strings, and the (previously un-localized) port-fallback note, which is
now localized via l10n.t. The rocket empty-state label is left untouched (an
unmerged sibling PR rewrites that exact line).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 8e6849f7-885d-4364-a467-867e10ae56f1
…assword (#7) UX review #7 (P1): the Quick Start "Copy Connection String" action silently included the password. Reuse the shared copy flow (which offers the standard with/without-password QuickPick) for BOTH entry points, and keep the credential-bearing metadata out of the webview. - Command copy: buildQuickStartCopyCredentials() builds password-free ephemeral credentials from the in-memory instance — it strips username+password from the base connection string (the shared flow treats the base as password-free and carries the password only in nativeAuthConfig) and FAILS CLOSED (copies nothing) if the string can't be parsed. copyQuickStartConnectionString then delegates to copyStandardConnectionString so the user gets the with/without-password choice. - Hardening: buildParsedConnectionString now clears the password on the base so "copy without password" is structurally password-free even if a caller passes a credential-bearing string; the password is re-added only in the with-password branch. This also protects the URI-handler saved-connection path. - Webview copy button: the router copyConnectionString mutation now delegates to the shared command instead of writing metadata.connectionString to the clipboard directly (it was a second silent leak). - Webview status payload: toWebviewStatus() strips the credential-bearing metadata from getDockerStatus/getStatus so the password never crosses into the renderer (defense-in-depth; the webview only reads canResumeReadiness). The tree calls QuickStartService.getStatus() directly and keeps full metadata for browsing. Tests: copyConnectionString T-02b (embedded-password base + without-password → no leak) and T-02c (with-password → present once + masked); 3 buildQuickStartCopy Credentials tests (strip, password-free input, fail-closed on parse error). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8e6849f7-885d-4364-a467-867e10ae56f1
UX review #3 (P1): the setup webview hero (h2 title) changed with the phase and
got stuck on "Setting up DocumentDB Local…". That hero() call was shared by the
provisioning, success, and failed phases, so the "Setting up…" title persisted
after setup finished or failed.
Per TN's recommendation, use a fixed header: all three hero() call sites now pass
the static title "DocumentDB Local", and the phase-specific text lives in the
subtitle/body. "Setting up… {elapsed}" is now a subtitle gated on
phase === 'provisioning', so it structurally cannot persist into success/failed.
The dockerNotReady subtitle folds in the "Docker is required" message; the review
title drops "Start" (the primary button still reads "Start DocumentDB Local").
Phase context and screen-reader announcements are unchanged (Announcers, the
role="status" live region, success/error boxes, and Docker metric cards all
remain); a heading-navigation SR user no longer lands on a stale "Setting up…"
h2 on the success/failed screens.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 8e6849f7-885d-4364-a467-867e10ae56f1
…recovery, own-column reveal
Addresses the GPT-5.6 Sol review follow-ups on the Quick Start UX branch.
#1 Delete now reports the ACTUAL outcome ('deleted' | 'refused' | 'busy' |
'error') and the command shows the success toast only on 'deleted'. Container
existence is resolved via the non-swallowing label lookup (listByLabel throws
on a Docker failure, returns [] on no-match) instead of the error-swallowing
inspectContainer, so a Docker outage or a stale/externally-replaced container id
can no longer produce a false 'deleted' plus a credential-missing ghost. Delete
now removes EVERY label-matched container (full clean slate, no duplicate
survivor) and, if only the data volume cannot be removed, surfaces a
non-blocking warning while still completing the delete.
#2 The credential-missing tree row is one-click actionable: a single click
launches Delete (which shows the standard confirmation), so recovery is
discoverable without hunting for the context menu.
#7 The Quick Start webview reveals in its own column (falling back to the active
column) instead of the framework default (column 1), so reopening it no longer
yanks the tab to column 1.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 8e6849f7-885d-4364-a467-867e10ae56f1
Contributor
|
| Check | Status | How to fix |
|---|---|---|
Localization (l10n) |
✅ Passed | |
| ESLint | ✅ Passed | |
| Prettier formatting | ❌ Failed | Run npm run prettier-fix to auto-format, then commit. |
This comment is updated automatically on each push.
guanzhousongmicrosoft
merged commit 2175d79
into
feature/local-quickstart
3 of 4 checks passed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Addresses the DocumentDB Local Quick Start UX review with a focused batch of safety / consistency fixes. Each fix was implemented and validated by a 5-agent rubber-duck panel (GPT-5.4/5.5, Opus 4.6/4.7/4.8) to unanimous consensus, followed by a final holistic pre-merge review that was also 5/5.
What's included (6 commits)
Start/Stop/Restartnow distinguish a missing container from one that is not ours (ensureActionable), and every container / volume removal is gated behind a hard label precondition, so the extension can never remove a container it did not create.InstanceState.CredentialsMissingsurfaces a labelled container whose saved credentials are gone as a single actionable tree row (previously a passive dead end). A single click launches Delete (with the standard confirmation) so recovery is discoverable.getConfirmationAsInSettingsflow; user-facing em dashes were swept from the feature strings.DocumentDB Local); phase / error / success status lives in the body.deleteContainerreturns'deleted' | 'refused' | 'busy' | 'error'and the command shows the success toast only on'deleted'. Container existence is resolved via the non-swallowinglistByLabel(a Docker outage throws →'error';[]→ confirmed gone) instead of the error-swallowinginspectContainer, so a Docker outage or a stale / externally-replaced container id can no longer produce a false "deleted" plus a credential-missing ghost. Delete removes every label-matched container (full clean slate, no duplicate survivor) and, if only the data volume cannot be removed, surfaces a non-blocking warning while still completing the delete. Reopening the Quick Start webview reveals it in its own / active column instead of yanking the tab to column 1.Testing
npm run jesttest— 2820 passingnpm run build(tsc) — cleannpm run lint— cleannpm run l10n:check— syncednpx prettier --check— cleanReview notes
The delete-outcome fix took four review rounds: the panel successively caught a swallowed
removeContainerfailure, the internal error-swallowing ininspectContainer(which made anundefinedresult ambiguous), and a first-match-only duplicate-container ghost — each was fixed and re-verified to 5/5.Not included / follow-ups