Connection dialog cleanup (post-#302)#303
Merged
erikdarlingdata merged 1 commit intodevfrom Apr 29, 2026
Merged
Conversation
- Rename "(Secondary)" indicator to "(Read-only)" — read-only intent requests routing to a secondary but doesn't guarantee it (the listener may still route to the primary if no readable replica is available). - Deduplicate SqlConnectionStringBuilder construction by adding a GetConnectionString(username, password, db) overload on ServerConnection and routing both the dialog test path and the runtime path through it. - XAML nits: drop redundant Margin and trailing whitespace on the new Read-only intent checkbox. No behavior change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Owner
Author
|
@ClaudioESSilva — small follow-up cleanup on top of your #302. No behavior change intended (the dialog test path now goes through the same If you have a moment to smoke-test the Test Connection button across Windows / SQL Server / Entra MFA on your setup before this lands, that'd be appreciated since I don't have those auth modes available locally. No worries if not — happy to merge on green build + tests. |
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.
Follow-up cleanup after #302 (read-only intent feature by @ClaudioESSilva). No behavior change — just polish.
Changes
(Secondary)indicator to(Read-only)inQuerySessionControl.ApplicationIntent=ReadOnlyrequests secondary routing but doesn't guarantee it — the AG listener will still send the session to the primary if no readable replica is available. The label now reflects what we actually know (the user asked for read-only intent), not an assumption about topology.SqlConnectionStringBuilderconstruction. Previously the encrypt/trust/intent/auth logic was written twice — once inServerConnection.GetConnectionString(ICredentialService, ...)for the runtime path, and once inConnectionDialog.BuildConnectionStringfor the test path (which reads creds from the textboxes before they're saved). Added aGetConnectionString(string? username, string? password, string? databaseName = null)overload onServerConnection; the credential-service overload now resolves creds and delegates to it, and the dialog calls it directly with textbox values. The dialog'sBuildConnectionStringis gone.Margin="0,0,0,6"(the parentStackPanelalready hasMargin="0,0,0,12") and trim trailing whitespace.Test plan
dotnet build src/PlanViewer.Core— cleandotnet build src/PlanViewer.App— 4 pre-existing AVLN3001 warnings, no new onesdotnet build src/PlanViewer.Web— cleandotnet test— 75/75 passServerLabelshows(Read-only)after connect