[WIP] 0.10.0 - #820
Draft
Tomasz Naumowicz (tnaum-ms) wants to merge 532 commits into
Draft
Conversation
- Added new fields for wildcard index configuration in CreateIndexDrawer, including wildcardName, wildcardNameEnabled, wildcardPartialText, and wildcardCollationText. - Updated logic to handle the state and rendering of wildcard index options independently from standard index options. - Improved focus management for navigation between main, advanced, and preview pages. - Enhanced styling for better layout and accessibility in the index creation form. - Added tests to ensure independent state management for standard and wildcard index options.
…design - Changed links to the original UX review from `ux-review.md` to `ux-review-iteration-1-2.md` for clarity and accuracy. - Updated related design documents links to reflect the correct files for Index Management UI and Vector index support. - Ensured all mentions of the original review are consistent with the new iteration documentation.
…utput handling
…s and severity adjustments
…style Deletion of an index is irreversible, but the shared confirmIndexAction helper always showed a single-click warning modal, ignoring the documentDB.confirmations.confirmationStyle setting (default: word entry) that governs destructive, irreversible operations. This reduced the default safety gate below what deleteCollection/deleteDatabase provide and silently overrode a public user preference. Route kind:'delete' through getConfirmationAsInSettings (word / challenge / click) like the sibling delete commands, keeping the rich size/usage/ effect detail text. Reversible hide/unhide keep the lighter click modal. UserCancelledError from the word-entry input box is translated to false so the boolean contract holds for both the tree command and the webview router. Addresses MEDIUM-1 of the PR #732 code review.
While any index row is building/creating, the list re-polls every 5s. The refresh() catch block called showError() on every failure, gated only by the refresh generation guard. If the cluster became briefly unreachable (sleep, VPN drop, tier hiccup) the rows stayed 'building', the poll re-armed indefinitely, and a 'Failed to load indexes.' toast stacked every 5 seconds until the connection returned. Gate the toast (and the assertive announcement) on the existing shouldAnnounce signal (initial || source === 'manual'), so background poll failures update only the inline loadFailed banner. Manual and initial loads still surface the error. Addresses MEDIUM-2 of the PR #732 code review.
A row that has a delete / hide / unhide in flight shows a spinner, but its Delete and Hide/Unhide buttons stayed enabled (only disabled for the protected _id_ row or an optimistic Creating row). After confirming an action, the same button remained clickable during the server op and the minimum-spinner tail, so a second confirmation could dispatch a duplicate mutation against the same index and race the first. Add isBusy (already derived from busyNames) to the disabledFocusable condition on both action buttons. Using disabledFocusable (not disabled) keeps the button focusable so its disabled reason/tooltip stays reachable while a second dispatch is impossible during the in-flight op and tail. Addresses LOW-1 of the PR #732 code review.
…mmands
The two advanced editors (partial filter, collation) forward raw, loose
BSON text. buildCreateIndexShellCommand embedded that text verbatim to
preserve BSON constructors and then appended the generated closing
delimiters on the same physical line. Because the loose parser accepts
line comments, an input such as '{ active: true } // only active' — which
direct creation accepts — produced a command whose trailing '// comment'
swallowed the '})' delimiters, so the shell/playground handoff failed to
parse.
Place every option entry on its own physical line and separate entries
with a *leading* comma, so the separator and the closing '})' always start
a fresh line and can never share a line with a trailing comment.
Unterminated block comments are already rejected by the parse in
buildIndexSpec, so every fragment reaching the string builder is valid and
safely isolated. Add parity tests asserting direct creation and the
handoff accept the same commented input and reject the same invalid input.
Addresses LOW-2 of the PR #732 code review (tracking issue #817).
Two defense-in-depth gaps at the host boundary: 1. CreateIndexInputSchema is a plain union and the field member was a non-strict z.object. A payload carrying kind:'vector' but also valid standard 'fields' passed the field member, which stripped the unknown 'kind' — so a malformed vector request silently degraded into a standard index request instead of being rejected. 2. Both field paths used z.string().min(1), accepting whitespace-only field paths that the drawer trims but a crafted/stale webview message could pass through. Add .strict() to both union members so a stray discriminator (or any unknown top-level key) is rejected rather than stripped, and replace the field-path min(1) with a trim-based refine on both the standard and vector schemas. The drawer emits disjoint, trimmed payloads, so normal usage is unaffected. Add tests for a mixed-shape vector payload, a stray top-level key, and whitespace-only standard/vector field paths. Addresses LOW-3 of the PR #732 code review.
The type badge set aria-label={type} while also rendering the same type as
visible text. A non-breaking space is announced as a space, so the
accessible name matched the visible text exactly and some screen readers
could announce a type such as 'Single Field' twice. The visible text
already supplies the accessible name, so the aria-label is removed.
Addresses LOW-4 of the PR #732 code review.
installResizeObserverLoopDetector() added a capture-phase window 'error' listener on every call with no guard. render() invokes it in development, where HMR / React Refresh can re-execute the entry module and leave the old listener attached while installing another. Each detector keeps its own rate counter and can emit the same once-per-burst warning, eroding the signal it exists to provide. Guard installation with a window-scoped sentinel that survives module replacement, so only one listener is ever attached. Dev-only utility; dead-code-eliminated from production. Addresses LOW-5 of the PR #732 code review.
Webview: card method picker, breadcrumb progress (neutral hint dots, completed steps stay bold), accordion 'where do I find these values' guide below the inputs, 'connection' wording, DocumentDB tab icon, honest per-method verify checklist. Errors: single shared isAtlasIpAccessListError predicate (recognises ORG_REQUIRES_ACCESS_LIST and any ACCESS_LIST code); Retry on the reconfigure error bar; add-flow deep links resolve the org live via buildAtlasAccessUrlFor + listOrganizations. Webview and tree classifiers kept separate but cross-commented. Ledger updated.
…xists A container removed outside VS Code is reported by the service as state Stopped WITH missing set, not as a state of its own. The Configure step's guard read only the state, so it classified it as 'stopped' and offered a Start button that could not do anything. Three fixes: - The guard now checks missing before the state, and does not guard a missing instance at all: its container is gone, so recreating is exactly what the user came for. Extracted to existingInstanceGuard.ts with tests, since this is a distinction that is easy to get wrong twice. - The recreate-vs-fresh radio is hidden whenever the guard blocks setup. Showing a choice next to a disabled primary action read as a third, broken control (Fluent also dims unselected radio labels, so it looked disabled as well). The group now carries an explicit question instead of two bare labels. - The panel subscribes to instance status instead of reading it once on open, so a tree action, another window, or a docker rm in a terminal no longer leaves the guard describing an instance that is gone (review N1). The subscription is deliberately cheap: no isDockerReady, no refreshLiveState, and it skips events that change nothing user-visible. Also drops em dashes from this feature's user-facing strings.
…he copy The Configure step showed a notice and a separate radio group, so the same question was effectively asked twice and the pair read as competing controls. They are now one MessageBar: the explanation on top, the choice beneath it. MessageBar renders role=group, which is the right container for a set of related controls, and this panel already nests interactive content in a message bar body. Copy changes: - No title on the notice. 'The DocumentDB Local container is gone' read as an alarm for what is a recoverable, data-preserving situation. - The question is asked once, by the field label. It stays self-contained when no explanatory sentence precedes it. - Radio labels are parallel and shorter: 'Keep the existing data' / 'Erase the existing data and start empty'. - The primary button is fixed at 'Start DocumentDB Local' again. The footer note above it still follows the selection, since 'Nothing else on your machine is changed' is only true for a genuinely fresh install.
N1 (panel reads instance status once per open) was recorded as 'resolved by construction' by the M4 explicit-choice decision. It was not: making the choice explicit removed the inference, but the status snapshot was still fetched on mount and never refreshed. That staleness is how the P-1 guard bug was reachable in practice. Both are now fixed and recorded in a new 11.6. Also notes the process failure: a full audit repeated the document's claim instead of checking it against the code, the same way the phantom TDD blocker in 11.5 came from trusting a summary over the tree.
… and tell them to restart VS Code Closes #855, #856. #856: the Docker setup link was the same on every platform — docs.docker.com/engine/install/, which documents a Linux-only Docker Engine install. Worse, the "Install Docker" button users actually click hardcoded that URL and ignored the guide the host had already resolved, so the per-platform mapping could not have taken effect even once it existed. The CTA now follows the resolved guide, and a missing CLI on Windows or macOS resolves to the Docker Desktop install page. WSL and the remote targets keep the Engine guide: the CLI has to exist in the environment the extension runs in, which is a Linux one whatever the desktop OS is. #855: after installing Docker Desktop, a VS Code that was already running keeps the environment it was launched with, so the new PATH is invisible to it and Docker stays undetected — the setup looks broken even though the install succeeded. The Windows and macOS guidance now says to restart VS Code explicitly, and calls out that reloading the window is not enough: the extension host is respawned from the same main process and inherits the same stale environment. The existing "Check again" action covers the retry after the restart.
…ill has to be created Closes #857. The post-start footer put "Copy Connection String" next to "Open Connection", which read as the next required setup step. A bug-bash user followed it and created the localhost connection by hand, even though Quick Start had already added the instance to the Connections view. The success screen now states outright that the connection already exists and does not need to be created. The copy action leaves the footer — where it competed with the primary action — and becomes an inline link in Next steps, introduced as optional and scoped to what it is actually for: reaching the instance from a client outside VS Code, such as mongosh or the user's own app. The footer keeps Open Connection with a note that it simply takes you to the connection that is already there.
…ction is added by hand Closes #858. Adding a localhost connection manually never checked whether Quick Start's managed instance already served that endpoint, so a second entry appeared for the same local service. The instance is service-owned and in-memory rather than a stored connection, so the existing duplicate scan — which reads stored Emulator connections — could not see it at all. The wizard now compares the new endpoint against the managed instances first. On a match it explains that Quick Start already added the instance and offers to open the existing entry, add a separate connection anyway (a different configuration for the same endpoint is legitimate), or cancel; dismissing the dialog cancels rather than creating the duplicate it just warned about. Host comparison is also normalized. `localhost`, `127.0.0.1`, and `::1` are the same machine, but the stored-connection scan compared host strings verbatim, so the same service added under two spellings read as two different connections. Endpoints now normalize to a canonical `host:port` — loopback forms collapse, a missing port fills in the wire-protocol default, and both the bracketed and bare IPv6 forms parse correctly. `127.0.0.2` is left distinct on purpose: it is loopback at the IP layer, but a service bound to one such address is not reachable on another, so collapsing them would report duplicates that are not.
Two defects from the `0.10.0-bug-bash-1` bash have no webview surface, so no service-level test can reach them. `contributions.test.ts` covers them. It asserts the `commandPalette` gating that #851 was missing, because that defect lived in package.json data rather than in code, and that the Quick Start entry point stays reachable. It also asserts the #852 strings appear in `l10n/bundle.l10n.json`, which is proof they go through `l10n.t()`, since the extractor sees nothing else. #858 was already covered by `localEndpoint.test.ts`.
The success screen's primary action ran `connectionsView.focus` and nothing else. When the Connections view is already the active view in the sidebar — the normal case, since Quick Start is opened FROM it — focusing it changes nothing on screen, so clicking Open Connection appeared to do nothing at all. The code comment claimed it revealed the connection; it never did. It now reveals the managed-instance row, selects it, and expands it, which is what "open" means for a cluster row in this tree: expanding connects and lists the databases. The reveal is progressive, because the instance row is a lazily created child and its parent has to be expanded before the child can be found. The panel still stays open — only the explicit Close button dismisses it. The paths and the navigation move into `revealQuickStartInstance`, shared with the duplicate-endpoint prompt's "Open Existing" (#858), so both routes to the instance land in the same place. A test asserts the path constants match the ids `LocalQuickStartItem` actually builds: nothing in the type system ties them together, and a silent mismatch would turn every "take me to the instance" action back into a no-op. The footer note is reworded to promise what now happens rather than "just takes you there".
…art-bugbash-fixes This branch predates the code-review work that landed on the base, so the overlapping areas are reconciled here rather than rebased away. Resolutions: * `LocalQuickStart.tsx`, Docker guidance maps. Neither side verbatim. The base converted `DOCKER_GUIDANCE` / `DOCKER_GUIDES` from module-scope consts into render-time `dockerGuidance()` / `dockerGuides()`, because `WebviewRegistry` imports the component statically, so a module body runs before `l10n.config()` and a module-scope `l10n.t()` is permanently English (review M1, issue #864). This branch added four keys to those same maps for the Windows and macOS Docker Desktop guidance. Kept the function form, with the four new keys inside it: taking either side verbatim would have shipped the new strings untranslatable, or dropped them. * `LocalQuickStart.tsx`, `handleInstallDocker`. Took this branch's deletion. The callback hardcoded `dockerGuides().install.href` and ignored the guide the host had already resolved, which is defect #856 itself; the per-platform mapping cannot take effect while it exists. * `QuickStartService.provision()` signature. Took the base's `(signal, options, alias = DEFAULT_ALIAS)`; review N5 threaded `alias` consistently and this branch predates it. Kept this branch's hoisted message const. * `QuickStartService.resumeReadiness()` busy event. Took this branch's version. The base yielded `error: 'in progress'`, and the webview renders `event.error ?? event.message`, so that untranslated marker reached the message bar verbatim. A real bug the review missed. * `QuickStartService.provision()` port selection. Took the base wholesale. The auto-relocation path this branch corrected the wording of was removed outright by the explicit-port model (review L3, "no magic after execute"). * `localQuickStartRouter.test.ts`. Adjacent inserts into the same `describe`; kept both. * `l10n/bundle.l10n.json`. Generated, never hand-merged. Discarded both sides and regenerated with `npm run l10n` once the source strings had settled. * `package.json` auto-merged cleanly but wrongly: both sides added the same seven `"when": "never"` `commandPalette` entries at different offsets, so git kept all fourteen. Deduplicated, keeping the block that carries the explanatory `"//"` key.
… code Four things the merge could not flag, because nothing conflicted textually. * `LocalQuickStart.tsx` still read `DOCKER_GUIDES[...]` at the #856 install CTA. The maps became render-time functions (review M1), so the reference is now `dockerGuides()[...]`. Without this the webview bundle does not compile. * `revealQuickStartInstance.test.ts` spied on `QuickStartService.refreshLiveState`. Review I2-5 moved the tree's call to `refreshLiveStateInBackground()`, so the spy no longer intercepted anything and the test ran the real background probe. Repointed. The path constants it guards are unchanged and still correct. * `contributions.test.ts` asserted two strings that the explicit-port model deleted: the old "Choose a different port or free it" wording, now "Go back to Configure to pick a different port", and `Port {0} was busy, using {1} instead.`, which no longer exists at all. The guard against a false claim is kept but repointed at the current contract: no string may tell the user the port was substituted. * Added a uniqueness assertion for `commandPalette`. Both branches added the same seven gating entries at different offsets, and every presence check still passed on the merged duplicate block, so only uniqueness catches that class of merge artifact. `l10n/bundle.l10n.json` is regenerated, not hand-merged.
Three strings this branch adds used an em dash where a colon or a sentence break reads the same. House style has no em or en dashes in product copy. The Docker Desktop restart guidance (#855) and the two optional connection-string lines on the success screen (#857). Wording is otherwise unchanged; the bundle is regenerated.
… and related state management
…al-quickstart # Conflicts: # src/webviews/_integration/appRouter.ts
…mport-export-fix-for-0.10.0 # Conflicts: # l10n/bundle.l10n.json
…nd tree data providers
Contributor
✅ Code Quality Checks
This comment is updated automatically on each push. |
Contributor
📦 Build Size Report
Download artifact · updated automatically on each push. |
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
2 participants
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.
No description provided.