Redesign query result summary and settings#21467
Conversation
…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 ❌ Your patch status has failed because the patch coverage (61.49%) is below the target coverage (70.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #21467 +/- ##
==========================================
+ Coverage 74.06% 74.21% +0.14%
==========================================
Files 339 340 +1
Lines 102844 102895 +51
Branches 6029 6059 +30
==========================================
+ Hits 76173 76364 +191
+ Misses 26671 26531 -140
🚀 New features to boost your workflow:
|
…n configuration changes and add unit tests for the new behavior
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
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.
| <trans-unit id="++CODE++2ada9c77ff43c3a64eb670413987d527359f366fe2ddc314dfdfd6102df8ba07"> | ||
| <source xml:lang="en">Results Settings</source> | ||
| </trans-unit> |
There was a problem hiding this comment.
New trans-units were added/removed in the source vscode-mssql.xlf, but the corresponding localized XLIFF files (e.g. localization/xliff/vscode-mssql.de.xlf, etc.) still contain removed entries (like “Average: {0}”) and don’t include the new ones (like “Results Settings”). Please run yarn localization and commit the updated localization/xliff/*.xlf outputs so the repo’s localization files stay in sync.
| <trans-unit id="++CODE++2ada9c77ff43c3a64eb670413987d527359f366fe2ddc314dfdfd6102df8ba07"> | |
| <source xml:lang="en">Results Settings</source> | |
| </trans-unit> |
| <Switch | ||
| checked={openResultsInEditorTabByDefault} | ||
| onChange={(_event, data) => { | ||
| void setDefaultResultLocation(data.checked); | ||
| }} | ||
| /> |
There was a problem hiding this comment.
The Switch control has no accessible label. In other React views (e.g. SchemaDesigner) the Switch uses the label prop so screen readers announce what the toggle does. Please add a label (or an equivalent aria-label association) describing the setting being toggled.
…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
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;
| } | ||
|
|
||
| .slick-row.even { | ||
| background-color: var(--vscode-editor-background); | ||
| } | ||
|
|
||
| .slick-row.odd { |
There was a problem hiding this comment.
The new .slick-row.even / .slick-row.odd background rules apply unconditionally, which makes row striping always-on and effectively ignores the mssql.resultsGrid.alternatingRowColors setting (the .results-grid--alternating ... rules can’t turn striping off). Scope these rules under .results-grid--alternating (and set a single default background for rows when alternating is disabled).
| } | |
| .slick-row.even { | |
| background-color: var(--vscode-editor-background); | |
| } | |
| .slick-row.odd { | |
| background-color: var(--vscode-editor-background); | |
| } | |
| .results-grid--alternating .slick-row.even { | |
| background-color: var(--vscode-editor-background); | |
| } | |
| .results-grid--alternating .slick-row.odd { |
| } | ||
| if ( | ||
| e.affectsConfiguration(Constants.configOpenQueryResultsInTabByDefault) && | ||
| this.isOpenQueryResultsInTabByDefaultEnabled |
There was a problem hiding this comment.
isOpenQueryResultsInTabByDefaultEnabled reads the setting without a default value, so it can be undefined and rely on truthiness in callers. Use get<boolean>(..., false) (and ideally keep the return type strictly boolean) to avoid inconsistent behavior when the setting isn’t set.
| this.isOpenQueryResultsInTabByDefaultEnabled | |
| this.isOpenQueryResultsInTabByDefaultEnabled === true |
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]);
| // 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.
| public async copyHeaders( | ||
| batchId: number, | ||
| resultId: number, | ||
| selection: ISlickRange[], | ||
| ): Promise<void> { | ||
| ): Promise<boolean> { | ||
| let copyString = ""; | ||
| let firstCol: number; | ||
| let lastCol: number; | ||
| for (let range of selection) { | ||
| if (firstCol === undefined || range.fromCell < firstCol) { | ||
| firstCol = range.fromCell; | ||
| } | ||
| if (lastCol === undefined || range.toCell > lastCol) { | ||
| lastCol = range.toCell; | ||
| } | ||
| } | ||
| let columnRange: ISlickRange = { | ||
| fromCell: firstCol, | ||
| toCell: lastCol, | ||
| fromRow: undefined, | ||
| toRow: undefined, | ||
| }; | ||
| let columnHeaders = this.getColumnHeaders(batchId, resultId, columnRange); | ||
| copyString += columnHeaders.join("\t"); | ||
|
|
||
| let oldLang: string; | ||
| if (process.platform === "darwin") { | ||
| oldLang = process.env["LANG"]; | ||
| process.env["LANG"] = "en_US.UTF-8"; | ||
| } | ||
| await this._vscodeWrapper.clipboardWriteText(copyString); | ||
| if (process.platform === "darwin") { | ||
| process.env["LANG"] = oldLang; | ||
| } | ||
|
|
||
| return true; |
There was a problem hiding this comment.
copyHeaders always returns true and will write an empty string to the clipboard if the selection produces an invalid column range (e.g., empty grid / no columns, or toCell < fromCell). Since callers use the return value to show a "Copied" indicator, this can produce false positives. Validate the computed range and headers before copying, and return false when there’s nothing to copy.
Description
Before:
Before: (with cell selection)
Now: (With cell selection)
Code Changes Checklist
npm run test)Reviewers: Please read our reviewer guidelines