fix(tree): address Copilot review findings for index count + tenant sign-in + multi-select#704
Open
hanhan761 wants to merge 7 commits into
Open
fix(tree): address Copilot review findings for index count + tenant sign-in + multi-select#704hanhan761 wants to merge 7 commits into
hanhan761 wants to merge 7 commits into
Conversation
feat(tree): show index count on Indexes folder node Add lazy-loaded index count as the description on IndexesItem tree items (e.g., "5 indexes" or "1 index"), following the same pattern used by CollectionItem for document counts. The count is fetched asynchronously via listIndexes() and includes search indexes when supported by the platform. Also add a stale-check guard (isCurrent) to IndexesItem so that in-flight count work is discarded when the owning CollectionItem is recreated on refresh or re-expand. Closes microsoft#659 @
fix(tree): hide single-item context menu commands when multiple items selected Add `&& !listMultiSelection` to the `when` clause of 36 context menu commands in the Connections tree view. When multiple items are selected (`listMultiSelection` is true), single-item commands are now hidden, preventing silent failures or unexpected behavior. Commands already supporting multi-select (removeConnection, moveItems) and discovery-view-only commands are intentionally left unchanged. Closes microsoft#668 @
perf(azure): throttle parallel tenant sign-in checks with shared concurrency limiter Replace unbounded Promise.all fan-outs against the Azure Identity endpoint with a shared ConcurrencyLimiter (concurrency: 5). For corporate users with many tenants, the original code issued an unlimited number of parallel isSignedIn calls against Microsoft Entra — the same anti-pattern that caused issues with estimateDocumentCount in microsoft#685. New shared limiter at api-shared/azure/tenantSignInLimiter.ts is imported by both SelectAccountStep and InitializeFilteringStep, ensuring total parallelism is bounded across the wizard. Closes microsoft#688 @
fix(tree): address Copilot review findings for index count feature Three targeted fixes from the PR microsoft#692 Copilot review: 1. loadIndexCount() guard: use typeof check instead of !== undefined so a prior failed fetch (null) can be retried after re-expand. 2. fetchAndUpdateCount(): wrap isLoadingCount reset in try/finally so early returns (stale-check bailout, error) never leave the instance permanently stuck in "loading" state. 3. CollectionItem.getChildren(): pass isCurrent predicate through to IndexesItem so background fetch work is properly discarded when the owning CollectionItem is recreated on refresh/re-expand. @
- Sort combinedIndexes/indexes before assigning to cachedIndexes so getChildren() never mutates the shared array in-place - Log listSearchIndexesForAtlas failures via ext.outputChannel.warn so transient network/auth errors are diagnosable - Log outer index-load failures in fetchAndUpdateCount similarly Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds an asynchronous index-count badge to the Indexes tree node and introduces a shared concurrency limiter for Azure tenant sign-in checks. Also disables single-select context menu actions when multiple tree items are selected.
Changes:
- IndexesItem now loads index counts in the background, caches results, and displays them as a description; CollectionItem wires a stale-check predicate to invalidate background work on refresh/re-expand.
- New shared
tenantSignInLimiter(concurrency 5) wrapsisSignedInfan-out inSelectAccountStepandInitializeFilteringStep. - All single-target context menu commands in
package.jsongated with!listMultiSelection; new l10n entries added.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/tree/documentdb/IndexesItem.ts | Adds background count load, cached indexes, stale-check predicate, and description rendering. |
| src/tree/documentdb/CollectionItem.ts | Adds expansionGeneration and passes an isCurrent predicate to IndexesItem; kicks off background count load. |
| src/plugins/api-shared/azure/tenantSignInLimiter.ts | New shared concurrency limiter (5) for tenant sign-in checks. |
| src/plugins/api-shared/azure/subscriptionFiltering/InitializeFilteringStep.ts | Wraps isSignedIn calls with the shared limiter. |
| src/plugins/api-shared/azure/credentialsManagement/SelectAccountStep.ts | Wraps isSignedIn calls with the shared limiter. |
| package.json | Adds !listMultiSelection guard to single-target context menu commands. |
| l10n/bundle.l10n.json | Adds "1 index" and "{0} indexes" strings. |
Comment on lines
+167
to
+169
| if (!this.cachedIndexes) { | ||
| const client: ClustersClient = await ClustersClient.getClient(this.cluster.clusterId); | ||
| const indexes = await client.listIndexes(this.databaseInfo.name, this.collectionInfo.name); |
| ext.state.notifyChildrenChanged(this.id); | ||
| } | ||
|
|
||
| async getChildren(): Promise<TreeElement[]> { |
| // available — avoids a second round-trip for the same data. Fall | ||
| // back to fetching on-demand if the cache hasn't been populated yet | ||
| // (e.g. count load is still in-flight or failed). | ||
| if (!this.cachedIndexes) { |
| // Build description based on index count state | ||
| let description: string | undefined; | ||
| if (typeof this.indexCount === 'number') { | ||
| description = this.indexCount === 1 ? l10n.t('1 index') : l10n.t('{0} indexes', this.indexCount); |
| // Build description based on index count state | ||
| let description: string | undefined; | ||
| if (typeof this.indexCount === 'number') { | ||
| description = this.indexCount === 1 ? l10n.t('1 index') : l10n.t('{0} indexes', this.indexCount); |
Comment on lines
+162
to
+164
| const myGeneration = ++this.expansionGeneration; | ||
| const parentIsCurrent = this.isCurrent; | ||
| const isCurrent = (): boolean => parentIsCurrent() && this.expansionGeneration === myGeneration; |
- Extract shared fetchAndCacheIndexes() to eliminate duplicate fetch logic - Add inFlightFetch Promise dedup so getChildren() and fetchAndUpdateCount() reuse the same round-trip instead of issuing parallel duplicate requests - Replace generation-counter + isCurrent closure with AbortController (standard, composable cancellation primitive) in CollectionItem - Pass AbortSignal to IndexesItem instead of isCurrent() closure - Add visual states: loading indicator (…) while fetching, "No indexes" badge for empty collections, no badge for undefined/null state - Check signal.aborted before making client calls in fetchAndCacheIndexes - Suppress AbortError logging (expected during refresh/collapse) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…missing l10n strings
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
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 all Copilot review findings on PRs #692 (index count), #693 (multi-select when clauses), and #694 (tenant sign-in limiter).
Changes
Copilot review fixes (new commit)
Previous commits (from ancestor branches)
Verification
🤖 Generated with Claude Code