Issues resolved from a review#550
Merged
Merged
Conversation
… telemetry C1/C2: actionableRecommendationCount was incremented unconditionally for every improvement, including those with action:'none'. This overcounted actionable items and caused divergence from per-action counters. Move the increment inside each actionable switch case (create/drop/modify) so 'none' recommendations are excluded from actionableRecommendationCount.
…eact.Ref C3/C4: 'import type React from react' shadows the React namespace making React.Ref inaccessible for type checking. Switch to 'import type * as React' which is the established pattern in this codebase and keeps React.Ref accessible.
…dren C6: when the user cancels a connection attempt, ClusterItemBase was setting clustersClient=null and falling through to the generic failure path which logged 'Could not connect...' and rendered an error/retry tree node. Return early with an empty array on UserCancelledError so cancellation is silent — no error message, no retry node shown to the user.
C5/S2-S6: Replace (error as Error).message with the safe extraction pattern
'error instanceof Error ? error.message : String(error)' in catch blocks
across AzureVMResourceItem, DocumentDBClusterItem, RUCoreResourceItem,
VCoreResourceItem, DocumentDBResourceItem and MongoRUResourceItem.
S1: Replace file-wide eslint-disable for react-hooks/refs with a narrower
eslint-disable-next-line comment on the specific ref={gridRef} line in
DataViewPanelTable.tsx to avoid masking future violations.
…wPanelTable S1: The narrower per-line suppression required two separate comments and was fragile. Restore the file-wide disable with an explanatory comment — SlickgridReact's imperative ref API inherently requires ref access patterns that trigger this rule throughout the file.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR applies a set of small fixes across the extension and webviews, primarily addressing lint/type-import issues, safer error handling, and more accurate telemetry counting for Query Insights AI recommendations.
Changes:
- Centralize/simplify lint suppression and React type imports in a few webview components.
- Make error logging/user messaging safer by avoiding unsafe
error.messageaccess. - Adjust Query Insights telemetry to count only actionable recommendations (create/drop/modify) and handle cancellation by returning an empty children list.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/webviews/documentdb/collectionView/components/resultsTab/DataViewPanelTable.tsx | Moves react-hooks/refs eslint suppression to file scope for Slickgrid ref usage. |
| src/webviews/documentdb/collectionView/components/queryInsightsTab/components/queryPlanSummary/StageDetailCard.tsx | Switches to import type * as React for React namespace typing. |
| src/webviews/documentdb/collectionView/components/queryInsightsTab/components/optimizationCards/custom/GetPerformanceInsightsCard.tsx | Switches to import type * as React for React namespace typing. |
| src/webviews/documentdb/collectionView/collectionViewRouter.ts | Updates telemetry counting to increment actionable counts only for create/drop/modify actions. |
| src/tree/documentdb/ClusterItemBase.ts | Returns an empty children array on user-cancelled connection attempts. |
| src/tree/connections-view/DocumentDBClusterItem.ts | Uses instanceof Error guard when rendering/logging error messages. |
| src/tree/azure-resources-view/mongo-ru/RUCoreResourceItem.ts | Uses instanceof Error guard when rendering/logging error messages. |
| src/tree/azure-resources-view/documentdb/VCoreResourceItem.ts | Uses instanceof Error guard when rendering/logging error messages. |
| src/services/ai/QueryInsightsAIService.ts | Updates telemetry counting to increment actionable counts only for create/drop/modify actions. |
| src/plugins/service-azure-vm/discovery-tree/vm/AzureVMResourceItem.ts | Uses instanceof Error guard when rendering/logging error messages. |
| src/plugins/service-azure-mongo-vcore/discovery-tree/documentdb/DocumentDBResourceItem.ts | Uses instanceof Error guard when rendering/logging error messages. |
| src/plugins/service-azure-mongo-ru/discovery-tree/documentdb/MongoRUResourceItem.ts | Uses instanceof Error guard when rendering/logging error messages. |
| * Licensed under the MIT License. See License.txt in the project root for license information. | ||
| *--------------------------------------------------------------------------------------------*/ | ||
|
|
||
| /* eslint-disable react-hooks/refs -- SlickgridReact requires ref for imperative grid API access */ |
There was a problem hiding this comment.
The file-level eslint-disable react-hooks/refs suppresses this rule for the entire component file, which can hide new/ref-related violations added later. Consider narrowing the suppression to the specific line(s) that need it (e.g., eslint-disable-next-line/eslint-disable-line around the ref usage) so the rest of the file remains linted normally.
| case 'modify': | ||
| actionableRecommendationCount++; | ||
| modifyRecommendationCount++; | ||
| break; |
There was a problem hiding this comment.
improvement.action is typed as 'create' | 'drop' | 'none' | 'modify' (see parseAIResponse). Since this switch only handles create/drop/modify, it would be clearer to add an explicit case 'none': break; (and optionally a default) to document that none is intentionally excluded from actionableRecommendationCount and to make future action additions easier to reason about.
Suggested change
| break; | |
| break; | |
| case 'none': | |
| // Intentionally excluded from actionable recommendations | |
| break; |
| case 'modify': | ||
| actionableRecommendationCount++; | ||
| modifyRecommendationCount++; | ||
| break; |
There was a problem hiding this comment.
rec.action can include 'none' in the AI response schema. This switch intentionally counts only create/drop/modify as actionable, but adding an explicit case 'none': break; (and optionally a default) would make that intent obvious and avoid ambiguity if new action values are introduced later.
Suggested change
| break; | |
| break; | |
| case 'none': | |
| // Explicitly treat 'none' as non-actionable. | |
| break; | |
| default: | |
| // Future or unknown action types are treated as non-actionable. | |
| break; |
Previously, when a cached ClustersClient existed, getClient() called client._mongoClient.connect() without hooking up the abortSignal, making the cancellable progress UI ineffective for reconnections. Now the cached client branch mirrors the fresh-connection pattern: check for pre-aborted signal, wire an abort listener that closes the MongoClient, and convert aborted errors to UserCancelledError.
sajeetharan
approved these changes
Apr 2, 2026
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.
No description provided.