Redesign query result summary and settings#21467
Open
aasimkhan30 wants to merge 28 commits into
Open
Conversation
added 10 commits
February 25, 2026 16:53
…ed performance and clarity
… better visibility
…ummary # Conflicts: # extensions/mssql/package.json # extensions/mssql/src/controllers/mainController.ts # extensions/mssql/src/queryResult/queryResultWebViewController.ts # localization/xliff/vscode-mssql.xlf
aasimkhan30
requested review from
Benjin,
allancascante,
caohai,
kburtram,
laurenastrid1,
lewis-sanchez,
manujoseph85 and
ssreerama
as code owners
aasimkhan30
changed the title
PR Changes
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #21467 +/- ##
==========================================
+ Coverage 74.37% 74.45% +0.07%
==========================================
Files 408 409 +1
Lines 128344 128517 +173
Branches 7773 7804 +31
==========================================
+ Hits 95460 95684 +224
+ Misses 32884 32833 -51
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
…n configuration changes and add unit tests for the new behavior
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 22 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
aasimkhan30
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 23 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
added 2 commits
March 10, 2026 15:44
…ummary # Conflicts: # extensions/mssql/src/controllers/mainController.ts # extensions/mssql/src/webviews/pages/QueryResult/queryResultSettingsControl.tsx # extensions/mssql/src/webviews/pages/QueryResult/queryResultSummaryFooter.tsx # localization/xliff/vscode-mssql.xlf
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
extensions/mssql/src/views/statusView.ts:37
FileStatusBarno longer definesrowCount/executionTime, but there are still references tobar.rowCountandbar.executionTimelater in this file (e.g., in the ownership-change handler). That will be a TypeScript compile error; remove/update those remainingshowStatusBarItemcalls (or reintroduce the status bar items if still needed).
public statusQuery: vscode.StatusBarItem;
// Item for language service status
public statusLanguageService: vscode.StatusBarItem;
// Item for SQLCMD Mode
public sqlCmdMode: vscode.StatusBarItem;
// Timer used for displaying a progress indicator on queries
public progressTimerId: NodeJS.Timeout;
public currentLanguageServiceStatus: string;
public queryTimer: NodeJS.Timeout;
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
extensions/mssql/src/webviews/pages/QueryResult/queryResultPane.tsx:207
- The keyboard shortcut handler effect captures
keyBindingsandisExecutionPlanbut they are missing from the dependency array. If keybindings are refreshed (or execution plan availability changes), the handler can use stale values and shortcuts may stop working as expected. Update the dependency list to include all referenced reactive values (and remove unrelated ones).
useEffect(() => {
const handler = (event: KeyboardEvent) => {
let handled = false;
if (
eventMatchesShortcut(
event,
keyBindings[WebviewAction.QueryResultSwitchToResultsTab]?.keyCombination,
)
) {
if (Object.keys(resultSetSummaries ?? {}).length > 0) {
context.setResultTab(qr.QueryResultPaneTabs.Results);
handled = true;
}
} else if (
eventMatchesShortcut(
event,
keyBindings[WebviewAction.QueryResultSwitchToMessagesTab]?.keyCombination,
)
) {
context.setResultTab(qr.QueryResultPaneTabs.Messages);
handled = true;
} else if (
eventMatchesShortcut(
event,
keyBindings[WebviewAction.QueryResultSwitchToQueryPlanTab]?.keyCombination,
)
) {
if (isExecutionPlan) {
context.setResultTab(qr.QueryResultPaneTabs.ExecutionPlan);
handled = true;
}
}
if (handled) {
event.preventDefault();
event.stopPropagation();
}
};
document.addEventListener("keydown", handler, true);
return () => {
document.removeEventListener("keydown", handler, true);
};
}, [tabStates?.resultPaneTab, getGridCount, context, resultSetSummaries]);
Comment on lines
799
to
+803
| // Handle cancellation from the progress dialog (user clicked cancel) | ||
| token?.onCancellationRequested(async () => { | ||
| await this._client.sendNotification(CancelCopy2Notification.type); | ||
| vscode.window.showInformationMessage("Copying results cancelled"); | ||
| resolve(); | ||
| resolve(false); |
There was a problem hiding this comment.
This user-facing message is not localized (hard-coded English). Please move it into the appropriate localization constants and use the localized string here.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (3)
extensions/mssql/src/webviews/pages/QueryResult/queryResultSummaryFooter.tsx:443
- This builds a user-facing status by concatenating localized fragments with a hard-coded separator. That makes word order and punctuation untranslatable; use a parameterized localized string for the cancelled-with-time message instead.
const executionText = cancelled
? executionTimeText
? `${locConstants.queryResult.executionCancelled} - ${executionTimeText}`
: locConstants.queryResult.executionCancelled
: (executionTimeText ?? locConstants.queryResult.executionTimeUnavailable);
extensions/mssql/src/webviews/pages/QueryResult/queryResultSummaryFooter.tsx:457
- This tooltip text concatenates localized text with a hard-coded colon. Use a parameterized localized string so languages that need a different order or punctuation can translate the full message.
const executionTooltipText =
liveExecutionMilliseconds !== undefined
? compactExecutionText === locConstants.queryResult.runningLabel
? locConstants.queryResult.runningLabel
: `${locConstants.queryResult.runningLabel}: ${compactExecutionText}`
: executionText;
extensions/mssql/src/webviews/pages/QueryResult/queryResultSummaryFooter.tsx:151
- This scans arbitrary user-facing message text to infer cancellation, so localized messages or user output containing words like "cancelled" can change the execution status shown in the footer. Track cancellation as structured state from the runner instead of parsing display strings.
function hasCancellationMessage(messages: qr.IMessage[]): boolean {
return messages.some((message) => /cancel(?:ed|led|ing)?/i.test(message?.message ?? ""));
}
Comment on lines
+93
to
+104
| function getRowsAffectedFromMessages(messages: qr.IMessage[]): number | undefined { | ||
| const rowsAffectedRegex = /\(?\s*(\d+)\s+rows?\s+affected\s*\)?/i; | ||
| for (let i = messages.length - 1; i >= 0; i--) { | ||
| const text = messages[i]?.message; | ||
| if (!text) { | ||
| continue; | ||
| } | ||
| const match = text.match(rowsAffectedRegex); | ||
| if (match && match[1] !== undefined) { | ||
| const parsed = Number(match[1]); | ||
| if (!Number.isNaN(parsed)) { | ||
| return parsed; |
Comment on lines
+431
to
+434
| ? rowsAffectedCount > 0 | ||
| ? locConstants.queryResult.rowsAffected(rowsAffectedCount) | ||
| : locConstants.queryResult.noRowsAffected | ||
| : locConstants.queryResult.noRowsAffected; |
Comment on lines
+654
to
+668
| await configuration.update( | ||
| Constants.configOpenQueryResultsInTabByDefault, | ||
| enabled, | ||
| vscode.ConfigurationTarget.Global, | ||
| ); | ||
|
|
||
| const state = this._queryResultStateMap.get(activeUri); | ||
| // Skip the one-time prompt after users explicitly choose their preferred result location. | ||
| await configuration.update( | ||
| Constants.configOpenQueryResultsInTabByDefaultDoNotShowPrompt, | ||
| true, | ||
| vscode.ConfigurationTarget.Global, | ||
| ); | ||
|
|
||
| if (state?.selectionSummary) { | ||
| this._selectionSummaryStatusBarItem.text = state.selectionSummary.text; | ||
| this._selectionSummaryStatusBarItem.tooltip = state.selectionSummary.tooltip; | ||
| this._selectionSummaryStatusBarItem.command = state.selectionSummary.command; | ||
| this._selectionSummaryStatusBarItem.show(); | ||
| } else { | ||
| this._selectionSummaryStatusBarItem.hide(); | ||
| if (enabled) { | ||
| await this.moveCurrentPanelResultToDocumentTab(); |
| border: "1px solid var(--vscode-widget-border)", | ||
| backgroundColor: "var(--vscode-editorWidget-background)", | ||
| color: "var(--vscode-foreground)", | ||
| boxShadow: "0 10px 28px rgba(0, 0, 0, 0.35)", |
Comment on lines
+135
to
+143
| function getLatestExecutionTimeMessage(messages: qr.IMessage[]): string | undefined { | ||
| const prefix = locConstants.queryResult.totalExecutionTimePrefix; | ||
| for (let i = messages.length - 1; i >= 0; i--) { | ||
| const text = messages[i]?.message; | ||
| if (!text) { | ||
| continue; | ||
| } | ||
| if (text.startsWith(prefix) || /execution\s+time/i.test(text)) { | ||
| return text; |
added 2 commits
May 13, 2026 11:10
…proved UI consistency
…and adjust selection restoration logic
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated 9 comments.
Comments suppressed due to low confidence (3)
extensions/mssql/src/webviews/common/locConstants.ts:779
- This localization constant is unused in the source tree, which adds dead code and an unnecessary generated localization entry. Remove it unless it is wired into UI in this PR.
executionLabel: l10n.t("Execution"),
extensions/mssql/src/webviews/pages/QueryResult/queryResultSummaryFooter.tsx:457
- This tooltip text is assembled from a localized label plus a hard-coded colon. Use a parameterized localized string for the full message so punctuation/order can be translated correctly.
const executionTooltipText =
liveExecutionMilliseconds !== undefined
? compactExecutionText === locConstants.queryResult.runningLabel
? locConstants.queryResult.runningLabel
: `${locConstants.queryResult.runningLabel}: ${compactExecutionText}`
: executionText;
extensions/mssql/src/webviews/pages/QueryResult/queryResultSummaryFooter.tsx:247
- The running-time formatter also emits hard-coded English unit suffixes (
s,m,h) in user-facing text. These should be localized or formatted through a shared duration helper.
function formatRunningTimeCompact(milliseconds: number): string {
if (milliseconds < 1000) {
return locConstants.queryResult.runningLabel;
}
if (milliseconds < 60000) {
return `${Math.floor(milliseconds / 1000)}s`;
}
if (milliseconds < 3600000) {
const minutes = Math.floor(milliseconds / 60000);
const seconds = Math.floor((milliseconds % 60000) / 1000);
return seconds > 0 ? `${minutes}m ${seconds}s` : `${minutes}m`;
}
const hours = Math.floor(milliseconds / 3600000);
const minutes = Math.floor((milliseconds % 3600000) / 60000);
return minutes > 0 ? `${hours}h ${minutes}m` : `${hours}h`;
added 2 commits
May 13, 2026 14:34
- Added localization for row counts and execution times in the MSSQL extension. - Updated interfaces and models to include rows affected and elapsed execution time. - Improved the handling of query result messages to reflect the number of rows returned and affected. - Refactored utility functions to streamline row count retrieval. - Enhanced the query result summary footer to display accurate execution metrics. - Updated tests to ensure proper functionality with the new row count handling.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 25 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (2)
extensions/mssql/src/webviews/pages/QueryResult/queryResultSummaryFooter.tsx:163
- When an elapsed time is just under one hour, rounding can make
seconds === 60and this returnscompactMinutes(minutes + 1), which becomes60mfor 59m59.5s instead of rolling over to1h. Handle the hour rollover as well so the compact time does not show a 60-minute value.
if (milliseconds < 3600000) {
const minutes = Math.floor(milliseconds / 60000);
const seconds = Math.round((milliseconds % 60000) / 1000);
if (seconds === 0) {
return locConstants.queryResult.compactMinutes(minutes);
}
if (seconds === 60) {
return locConstants.queryResult.compactMinutes(minutes + 1);
}
return locConstants.queryResult.compactMinutesSeconds(minutes, seconds);
extensions/mssql/src/webviews/pages/QueryResult/queryResultSummaryFooter.tsx:461
- This visual divider is inside the footer status area and will be announced by screen readers as punctuation. Render it with CSS or add
aria-hiddenso the live region only announces the selection summary content.
<span className={classes.divider}>|</span>
| resultId: e.resultId, | ||
| }; | ||
| this._queryResultWebviewController.updateSelectionSummary(); | ||
| this.updateWebviewState(e.uri, state); |
Comment on lines
+97
to
+110
| useEffect(() => { | ||
| if (!context) { | ||
| return; | ||
| } | ||
|
|
||
| context.extensionRpc | ||
| .sendRequest(qr.GetOpenQueryResultsInTabByDefaultRequest.type) | ||
| .then((isEnabled) => { | ||
| setOpenResultsInEditorTabByDefault(isEnabled); | ||
| }) | ||
| .catch((e) => { | ||
| console.error(e); | ||
| }); | ||
| }, [context]); |
Comment on lines
+149
to
+151
| return locConstants.queryResult.compactSeconds( | ||
| seconds < 10 ? seconds.toFixed(1) : Math.round(seconds), | ||
| ); |
Comment on lines
+135
to
+140
| function normalizeStatusText(text?: string): string { | ||
| if (!text) { | ||
| return ""; | ||
| } | ||
| return text.replace(/\$\([^)]+\)\s*/g, "").trim(); | ||
| } |
| </span> | ||
| </Tooltip> | ||
| </div> | ||
| <span className={classes.divider}>|</span> |
Comment on lines
+586
to
+589
| resultWebviewState.isExecuting = true; | ||
| resultWebviewState.executionStartTime = Date.now(); | ||
| resultWebviewState.executionElapsedMilliseconds = undefined; | ||
| resultWebviewState.rowsAffected = undefined; |
…ummary # Conflicts: # extensions/mssql/l10n/bundle.l10n.json # localization/xliff/vscode-mssql.xlf
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.
Description
Before:
Before: (with cell selection)
Now: (With cell selection)
Code Changes Checklist
npm run test)Reviewers: Please read our reviewer guidelines