Skip to content

Commit 90ea259

Browse files
tnaum-mstnaum-ms
authored
feat(playground): keep connections across save/reopen + add a Connect-to-Database picker (#758)
2 parents a40194a + 12532c7 commit 90ea259

15 files changed

Lines changed: 1334 additions & 104 deletions

File tree

‎docs/ai-and-plans/PRs/758-query-playground-connections/summary.md‎

Lines changed: 105 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,105 @@
1+
# PR Summary — Query Playground connections across save / reopen + Connect picker
2+
3+
> PR: [microsoft/vscode-documentdb#758](https://github.com/microsoft/vscode-documentdb/pull/758)
4+
> Iterative working summary of what was implemented and **why**. The "why" is the
5+
> point of this doc — it captures the constraints and design forks so future
6+
> reviews/analysis don't have to re-derive them.
7+
8+
## Goal
9+
10+
Make a Query Playground's connection (the cluster + database it runs against) **survive
11+
the things that previously silently dropped it**, and give the user a first-class way to
12+
(re)connect a playground when no connection is in context.
13+
14+
## Background — the original problem
15+
16+
`PlaygroundService` binds each `.documentdb.js` document to a `PlaygroundConnection`
17+
(stable `clusterId` + `databaseName` + display/tree context) in an **in-memory map keyed
18+
by the document URI string**. Query execution reads that binding to know where to run.
19+
20+
Because the key is the URI and the map is never persisted, the binding is lost whenever:
21+
22+
- **untitled → file** — saving a scratch playground to disk (scheme/path changes),
23+
- **file → file (Save As)** — re-saving under a new name (path changes), or
24+
- **reopening a saved playground in a new VS Code session** — the map starts empty.
25+
26+
In every case the file opens **disconnected** with no obvious recovery, which is confusing
27+
because the document looks identical to when it worked.
28+
29+
## Two distinct sub-problems, two mechanisms
30+
31+
The same-session URI change and the cross-session reopen are **fundamentally different**
32+
and cannot share one solution:
33+
34+
| Sub-problem | Is there state to recover from? | Mechanism |
35+
| ------------------------------------- | ----------------------------------------------------------------------------------------- | --------------------------------------------------------- |
36+
| untitled→file, Save As (same session) | **Yes** — the source document is still open and still holds the connection | Content-correlated **transfer at save time** |
37+
| Reopen in a fresh session | **No** — the process restarted; both the binding map _and_ the credential cache are empty | Must be an **actionable affordance** (the Connect picker) |
38+
39+
> **Key realization that shaped the design:** after a real restart there is nothing in
40+
> memory to reconnect _to_ (credentials are gone too), so persistence (a Memento) or an
41+
> in-file hint would be the _only_ ways to auto-restore — both were explicitly ruled out
42+
> as too heavy / user-visible. That leaves an **actionable connect** as the only viable
43+
> path for the cross-session case, which is why the bulk of this PR is a good picker UX
44+
> rather than a persistence layer.
45+
46+
## What changed (by area)
47+
48+
| Area | Change | Why |
49+
| ----------------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
50+
| `PlaygroundService.transferConnectionToSavedFile` | On save of a `file:` playground, find the still-open source playground whose content matches and re-key its connection onto the saved URI. Guards: skip if the saved file is already bound; require **exactly one** content match (ambiguity guard for "Save All"); size-gate via `offsetAt`-based length before any `getText()`. | Covers untitled→file and Save As without persistence, using the only reliable correlation available at that moment: content, while the source is still open. Save (not open) is used because a freshly-created file's model is still **empty** for ~150 ms after `onDidOpen`. |
51+
| New `pickTreeNode` (`utils/pickItem/pickTreeNode.ts`) | A generic, reusable quick pick that **browses the live Connections tree** (folders → clusters → databases) by repeatedly calling the tree's own `getChildren`. | Reuses folder nesting, ordering, icons, and **connect/auth-on-expand** instead of re-implementing them; avoids eagerly connecting to clusters the user didn't choose. Mirrors vscode-cosmosdb's `pickWorkspaceResource` shape. |
52+
| `playground.connect` command (`connectDatabase.ts`) | Prompts via `pickTreeNode` (leaf = `treeItem_database`) and binds the playground. Wired to the previously dead-end **"Not connected" CodeLens and StatusBar**. | Turns the always-visible disconnected indicator into a one-click connect — the primary recovery path for the cross-session case. |
53+
| Connect-on-run (`executePlaygroundCode.ts`) | Running an **unconnected** playground now launches the connect picker and continues the run on success (cancel = no-op). | The most contextual moment to ask for a connection is when the user actually tries to use one; replaces a dead-end info message. |
54+
| Switch-database modal (`connectDatabase.ts`) | Clicking the connection lens **while connected** opens a modal offering to switch databases. The existing binding is only replaced **after** a new database is selected. | Lets users repoint a playground, while guaranteeing that **aborting the picker preserves the current connection** (no "disconnect first, then maybe fail to pick"). |
55+
| In-session restore (`PlaygroundService`) | A **non-persistent** in-memory "last binding per URI" map survives document _close_; on reopen within the same session the binding is restored **iff** the target cluster still has live credentials (`CredentialCache.hasCredentials`). Cleared on explicit `removeConnection` and on dispose. | Cheap quality-of-life win for close→reopen in one session. Deliberately **not** persistence (no Memento, no file mutation), so it is neither approach "A" nor "C"; a true restart correctly falls through to the Connect affordance. Never reads the document body, so it is immune to the empty-model open race. |
56+
57+
## Dual-ID compliance
58+
59+
All cache/credential lookups use the stable **`clusterId`** (never `treeId`), per the
60+
repo's dual-ID rule, so bindings keep working when a connection is moved between folders.
61+
`PlaygroundConnection` stores `clusterId`; the picker reads it from the selected
62+
`DatabaseItem`'s `cluster.clusterId`.
63+
64+
## Picker UX decisions (and why)
65+
66+
These were iterated with the maintainer; the rationale matters for future tweaks:
67+
68+
- **Loading indicator** — picks are passed to `showQuickPick` as a _promise_ so the quick
69+
pick stays visible with a busy spinner while a level loads (a cluster connection can take
70+
seconds). Earlier the quick pick vanished during the wait, which read as a crash.
71+
- **Tree-matching icons** — each pick carries the tree item's own `iconPath` (clusters,
72+
`symbol-folder` for folders, databases), so the picker looks like the tree.
73+
- **Host as a second line (`getDetail`)** — connections show their host(s) so two
74+
same-named connections are distinguishable; `matchOnDetail` makes it searchable.
75+
- **Uniform two-line rows** — every selectable row has a `detail` (folders → `Folder`,
76+
databases → `Database`, clusters → host/`Cluster`) to avoid a ragged mix of one- and
77+
two-line rows.
78+
- **Navigation** — after trying a bottom-anchored Back/Exit-with-grouping layout (modeled
79+
on the Azure credentials browser) it felt heavier to navigate, so the final form is the
80+
simplest: **Back pinned to the top** followed by a separator, **no Exit** (Esc cancels),
81+
**no group headers** — relying on the tree's natural folders-before-connections order.
82+
83+
## Telemetry
84+
85+
- `documentdb.pickTreeNode` — `source` (caller), `leafContextValue`, `outcome`
86+
(`picked` | `cancelled` | `empty`), measurements `stepCount` / `maxDepth`. Added so we can
87+
see whether/how the reusable picker is actually used across features.
88+
- `playground.connect` and `playground.connectOnRun` — outcome of the connect flows.
89+
90+
## Testing
91+
92+
- `pickTreeNode.test.ts` — drill-down, leaf return, exclusion of action/placeholder nodes,
93+
icon and detail passthrough, Back/empty/no-connections states, flat folders-first order,
94+
and cancellation.
95+
- `PlaygroundService.test.ts` — save-time transfer (untitled→file, Save As, ambiguity and
96+
size guards) and in-session restore (restore when credentials present, no restore when
97+
absent or after explicit removal).
98+
- Full Jest suite, lint, and TypeScript build are green.
99+
100+
## Out of scope / explicitly not done
101+
102+
- **No persisted binding store** (Memento) and **no in-file connection hint** — both
103+
rejected as too heavy or user-visible; the cross-session case is handled by the Connect
104+
affordance instead.
105+
- The picker does **not** auto-run anything on connect; it only restores/sets the binding.

0 commit comments

Comments
 (0)