Skip to content

Adding "Edit" header and "copy as new" function to connection dialog#21509

Open
Benjin wants to merge 8 commits intomainfrom
dev/benjin/editConnectionHeader
Open

Adding "Edit" header and "copy as new" function to connection dialog#21509
Benjin wants to merge 8 commits intomainfrom
dev/benjin/editConnectionHeader

Conversation

@Benjin
Copy link
Copy Markdown
Contributor

@Benjin Benjin commented Mar 6, 2026

Description

Changes the CD header to be explicit when it's creating a new connection versus editing an existing one.
image
image

Also adds buttons to the saved connection pane for copying details to a new connection. Previously, the only option was to edit the connection by clicking the main card.
image

Code Changes Checklist

  • New or updated unit tests added
  • All existing tests pass (npm run test)
  • Code follows contributing guidelines
  • Telemetry/logging updated if relevant
  • No regressions or UX breakage

Reviewers: Please read our reviewer guidelines

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Updates the Connection Dialog UX to distinguish between creating a new connection vs editing an existing saved connection, and adds a “copy as new” flow from saved/recent connections.

Changes:

  • Adds edit/new-draft state to the Connection Dialog webview (controller + shared state) and updates the header title accordingly.
  • Updates the saved/recent connections list UI to support “edit” (primary click) and “copy as new draft” (copy icon).
  • Adds/updates localized strings and unit tests for the new behaviors.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
localization/xliff/vscode-mssql.xlf Adds new trans-units for edit/copy-connection strings.
extensions/mssql/test/unit/connectionDialogWebviewController.test.ts Updates/extends unit tests to validate edit vs new-draft behavior and new state flags.
extensions/mssql/src/sharedInterfaces/connectionDialog.ts Adds isEditingConnection/editingConnectionDisplayName to state and splits reducers into edit vs new-draft.
extensions/mssql/src/reactviews/pages/ConnectionDialog/connectionsListContainer.tsx Updates cards to support multiple action buttons and new edit/copy flows with tooltips.
extensions/mssql/src/reactviews/pages/ConnectionDialog/connectionDialogStateProvider.tsx Wires new reducer actions (loadConnectionForEdit, loadConnectionAsNewDraft) to RPC calls.
extensions/mssql/src/reactviews/pages/ConnectionDialog/components/connectionHeader.component.tsx Dynamically sets header title based on edit state.
extensions/mssql/src/reactviews/common/locConstants.ts Adds new localized string helpers for edit/copy connection UX.
extensions/mssql/src/connectionconfig/connectionDialogWebviewController.ts Implements edit vs new-draft load behaviors and manages new state flags.
extensions/mssql/l10n/bundle.l10n.json Adds the new runtime localization entries generated from l10n.t(...) usage.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

Comment on lines +1452 to +1454
<trans-unit id="++CODE++58a3543188ba49b9c51c596fa21636899f0714f30de345976c64f65a0c0e4412">
<source xml:lang="en">Copy connection to a new profile</source>
</trans-unit>
Copy link

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New trans-units were added to the base vscode-mssql.xlf, but the corresponding localized XLIFF files (e.g. vscode-mssql.de.xlf, ...es.xlf, etc.) were not updated with matching trans-units. The repo’s localization workflow/CI expects the same additions/removals to be mirrored across all localized files, typically with <target state="new"> placeholders, otherwise yarn localization will produce diffs or fail checks.

Suggested change
<trans-unit id="++CODE++58a3543188ba49b9c51c596fa21636899f0714f30de345976c64f65a0c0e4412">
<source xml:lang="en">Copy connection to a new profile</source>
</trans-unit>

Copilot uses AI. Check for mistakes.
@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 6, 2026

PR Changes

Category Target Branch PR Branch Difference
vscode-mssql VSIX 6361 KB 6361 KB ⚪ 0 KB ( 0% )
sql-database-projects VSIX 6122 KB 6122 KB ⚪ 0 KB ( 0% )
data-workspace VSIX 538 KB 538 KB ⚪ 0 KB ( 0% )

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Mar 6, 2026

Codecov Report

❌ Patch coverage is 77.50000% with 18 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.24%. Comparing base (32d639b) to head (2123848).

Files with missing lines Patch % Lines
...nsions/mssql/src/reactviews/common/locConstants.ts 0.00% 18 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #21509      +/-   ##
==========================================
- Coverage   73.25%   73.24%   -0.01%     
==========================================
  Files         328      328              
  Lines       99455    99510      +55     
  Branches     5649     5649              
==========================================
+ Hits        72851    72890      +39     
- Misses      26604    26620      +16     
Files with missing lines Coverage Δ
...nectionconfig/connectionDialogWebviewController.ts 61.70% <100.00%> (+0.64%) ⬆️
...ons/mssql/src/sharedInterfaces/connectionDialog.ts 97.84% <100.00%> (+0.04%) ⬆️
...nsions/mssql/src/reactviews/common/locConstants.ts 29.30% <0.00%> (-0.21%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copilot AI review requested due to automatic review settings March 25, 2026 22:55
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot wasn't able to review this pull request because it exceeds the maximum number of files (300). Try reducing the number of changed files and requesting a review from Copilot again.

"typescript": "^5.8.3"
}
}
{
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No changes in this file

Copilot AI review requested due to automatic review settings March 25, 2026 23:21
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 10 out of 12 changed files in this pull request and generated 3 comments.

Comment on lines +1433 to +1436
await this.updateItemVisibility();
await this.handleAzureMFAEdits("authenticationType");
await this.handleAzureMFAEdits("accountId");
await this.checkReadyToConnect();
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

setConnectionForEdit/setConnectionAsNewDraft call handleAzureMFAEdits("authenticationType") before handleAzureMFAEdits("accountId"). In handleAzureMFAEdits, the authenticationType case overwrites connectionProfile.accountId with the first account option, which can silently change the selected Entra account/tenant when loading an existing Azure MFA profile. Consider skipping the authenticationType call during initial load (only call accountId when authenticationType===AzureMFA), or update handleAzureMFAEdits to not override accountId if it’s already set on the loaded profile.

Copilot uses AI. Check for mistakes.
onSelect();
}
}}
title={locConstants.connectionDialog.connectTo(displayName)}
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Card’s title is still set to connectTo(displayName) even though the primary action is now edit/create-copied (and you’re also wrapping the Card in a Tooltip with that new label). This can produce conflicting/misleading hover text and accessible name. Recommend aligning the Card title/aria labeling with primaryActionTooltip(displayName) (or removing the title attribute if Tooltip provides the label).

Suggested change
title={locConstants.connectionDialog.connectTo(displayName)}
title={cardTooltip}

Copilot uses AI. Check for mistakes.
<div className={buttonContainer}>
{actionButtons.map((actionButton, index) => (
<Tooltip
key={`${index}-${actionButton.tooltip(displayName)}`}
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The key for each action button tooltip/button is derived from actionButton.tooltip(displayName) (a localized string + profile name). Keys should be stable across renders and locales; using localized text can cause unnecessary remounting and makes collisions more likely. Prefer a fixed identifier per action (e.g., "copy"/"delete") or use the array index if the set/order is truly static.

Suggested change
key={`${index}-${actionButton.tooltip(displayName)}`}
key={index}

Copilot uses AI. Check for mistakes.
@aasimkhan30
Copy link
Copy Markdown
Contributor

aasimkhan30 commented Mar 26, 2026

I’m wondering if we should add an edit icon (pencil) to the connection list and stop using row click as the default edit action. Right now, I’m concerned it may not be obvious whether clicking a row is meant to edit the existing connection or simply clone its values into the form.

Another option (which I like better) is to flip the default behavior: make row click clone the connection, and use a dedicated edit button for editing. Cloning feels like the less destructive action, so it may be the safer default. My assumption is that most users who click a connection from this list are probably trying to reuse it to populate the form, while users who want to edit an existing connection are more likely to come in through the specific “Edit Connection” entry point. I don’t have data to back that up yet, though, so this is mostly intuition.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants