Repository navigation
Add Server dialog: scroll the form, pin the buttons, clamp to the work area (#1828) - #1829
Conversation
…k area (#1828) SizeToContent="Height" + ResizeMode="NoResize" with no scroll meant the dialog grew wherever its content took it: on a 1080p work area, selecting SQL auth, Service Principal, or Managed Identity pushed Test Connection / Save / Cancel off the bottom of the screen, reachable only by blind tabbing, with resize disabled as the escape hatch. Both copies (Lite and the Darling Viewer) now mirror the deprecated Dashboard twin's proven shape: the form scrolls in a star row, the status line and buttons are pinned OUTSIDE the scroll so they are always visible, MaxHeight clamps the window to the primary work area, and the window is resizable with a grip (MinWidth/MinHeight floors). Because SizeToContent growth is top-anchored, MaxHeight alone still let mid-session growth slide the footer past the screen edge without hitting the clamp - a SizeChanged handler pulls the window up when its bottom crosses the work area, the SizeToContent-friendly form of the Dashboard's SizeToWorkArea top-pinning. The buttons gain Alt mnemonics (the reporter explicitly looked for them) and Cancel gains IsCancel so Esc closes the dialog, matching EditCredentialProfileDialog and AddMultipleServersDialog. ui-reviewer pass: control/name/handler inventory identical before and after, no orphaned Grid.Row indexes, no access-key collisions, IsCancel verified safe against both CancelButton_Click handlers (guarded same-value DialogResult, no double-close). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ReviewOverview: Fixes #1828 by reworking Parity: Lite and Darling changes are structurally identical (same row restructuring, same Correctness: The row re-indexing looks consistent — inner grid still has 10 Minor finding: left as inline comments — No security-relevant surface here (no new input handling, network, or process use), no SQL touched, no missing-index folklore to worry about. No test coverage added, but that matches the rest of the codebase — WPF dialogs aren't unit tested here. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| private void Dialog_SizeChanged(object sender, SizeChangedEventArgs e) | ||
| { | ||
| var workArea = SystemParameters.WorkArea; | ||
| if (Top + ActualHeight > workArea.Bottom) | ||
| { | ||
| Top = Math.Max(workArea.Top, workArea.Bottom - ActualHeight); |
There was a problem hiding this comment.
Minor: SystemParameters.WorkArea always reflects the primary monitor's work area, not necessarily the monitor this window is actually on. On a multi-monitor setup where the dialog opens on a secondary monitor with a different resolution/work area than the primary, this can clamp/reposition based on the wrong monitor's bounds — in the worst case (secondary monitor shorter than primary), the exact off-screen-footer bug this PR fixes could reproduce, since the MaxHeight binding (line 6 in the .xaml) is also computed once against the primary monitor and never re-evaluated.
Not a regression introduced here — it mirrors the Dashboard's SizeToWorkArea() pattern, which has the same limitation — but worth flagging since screen-overflow correctness is the whole point of this fix. A per-monitor-aware alternative (e.g. System.Windows.Forms.Screen.FromHandle via the window handle, or P/Invoke MonitorFromWindow) would close this gap if it's worth the added complexity.
| private void Dialog_SizeChanged(object sender, SizeChangedEventArgs e) | ||
| { | ||
| var workArea = SystemParameters.WorkArea; | ||
| if (Top + ActualHeight > workArea.Bottom) | ||
| { | ||
| Top = Math.Max(workArea.Top, workArea.Bottom - ActualHeight); |
There was a problem hiding this comment.
Same note as the Lite copy: SystemParameters.WorkArea reflects the primary monitor, not necessarily the monitor this dialog is on, so both this handler and the MaxHeight binding (line 6 in the .xaml) can clamp/reposition against the wrong monitor's bounds on a multi-monitor setup. Inherited from the Dashboard's original pattern rather than a new regression, just flagging since it's the exact class of bug this PR is fixing.
erikdarlingdata#1892, erikdarlingdata#1911 erikdarlingdata#1891 - the Add Server dialogs clamped against SystemParameters.WorkArea, which always reports the PRIMARY monitor. On a secondary monitor shorter than the primary that reproduced the very off-screen-footer bug erikdarlingdata#1828/erikdarlingdata#1829 fixed. The clamp and the height cap now resolve the actual monitor via MonitorFromWindow + GetMonitorInfo, converted from physical pixels to WPF units through the window's own presentation source - an unconverted work area on a 150%-scaled screen would be a worse bug than the one being fixed. Re-evaluated on SourceInitialized and LocationChanged, because the cap it replaced was a one-shot XAML binding read once at load, so a dialog dragged to a shorter screen kept the taller screen's cap forever. Every path that cannot get a real answer falls back to the old behaviour, so single-monitor is unchanged. Win32 rather than WinForms: neither app references WinForms and one lookup is not worth a second UI framework. Shared between both apps so the twins cannot drift. erikdarlingdata#1892 - the Query Store slicer and duration trend filtered the window on collection_time while keying the bars on the interval start. Once erikdarlingdata#1841 made those different instants the chart disagreed with its own range at both edges: an interval starting before the range but collected inside it drew a bar dated before the range began, and the range's final interval - still open when the range ended - was dropped entirely. Both reads now window on the expression they place on, in both apps. A bare collection_time floor stays and is NOT a filter: query_store_stats is a hypertable partitioned on that column and Lite's view unions the parquet archive, so without it Postgres decompresses every chunk and DuckDB cannot skip row groups. An interval is always collected after it starts, so the floor is provably implied and can never decide membership - the same shape as the drill-down collector's last_execution_time bound. Deliberately no upper twin: collection_time can exceed the interval start without bound if the collector was down when the interval closed, so a ceiling would silently drop exactly the rows a restarted collector just caught up on. erikdarlingdata#1911 - Lite's MCP get_alert_settings did not report cpu.mode at all. Reporting the raw enum name would have traded erikdarlingdata#1895's key-level mismatch for a value-level one, since Darling emits "sql"/"total" and validates writes against exactly those. Darling's vocabulary wins as the older public surface; the cross-app literals are pinned by reading Darling's own source, because asserting Lite's constants alone would keep passing the day Darling changed. Verified live against PostgreSQL 18.4 + TimescaleDB 2.28.1 (4002 passed, 10 skipped - the gated live suite genuinely ran). Every new pin red-watched, including the live one. The erikdarlingdata#1902 ratchet correctly caught the new live test using the old teardown idiom; converted to LiveStoreCleanup rather than raising the ceiling. Filed rather than folded in: erikdarlingdata#1921 (the item-timeline overlay still plots at collection_time while the bars it annotates are keyed on the interval start - a product decision about what the overlay is for) and erikdarlingdata#1922 (the live fixture keeps using a connection that CREATE EXTENSION timescaledb killed, masking the real cause). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 participant
Summary
Fixes #1828: on a 1080p work area, selecting SQL auth, Service Principal, or Managed Identity grew the Add Server dialog (
SizeToContent="Height"+ResizeMode="NoResize", no scroll, noMaxHeight) past the screen bottom, taking Test Connection / Save / Cancel with it — reachable only by blind tabbing, with resize disabled as the escape hatch. The reporter's screenshot shows the dialog at the full 1080px screen height. Both copies with the defect are fixed — Lite (reported) and the Darling Viewer (identical structure); the deprecated Dashboard's twin already had the correct shape and supplied the pattern.*/Auto; the form lives in aScrollViewerin the star row; the status line + buttons are pinned outside the scroll so they are always visible at any window height.MaxHeightbound toSystemParameters.WorkArea.Height,ResizeMode="CanResizeWithGrip"(matchingAddMultipleServersDialog),MinWidth/MinHeightfloors.SizeToContentstill auto-sizes compact dialogs; the scrollbar engages only when clamped.SizeToContentgrowth is top-anchored, so mid-session growth could still slide the footer past the screen edge without ever reaching the clamp (ui-reviewer caught this residual — it reproduces in the most common flow: open compact, then pick a tall auth mode). ASizeChangedhandler pulls the window up when its bottom crosses the work area — the SizeToContent-friendly form of the Dashboard'sSizeToWorkArea()top-pinning.IsCancelon Cancel so Esc closes the dialog, matchingEditCredentialProfileDialog/AddMultipleServersDialog. Save deliberately does NOT gainIsDefault— Enter-to-save from a half-filled form is a behavior change beyond this bug.EditCredentialProfileDialogshares the window shape but its fully-expanded content tops out around 550px, so it cannot overflow any real work area — deliberately left untouched.Test plan
x:Name/handler/Grid.Rowinventory identical before and after in both files; no orphaned row indexes; no access-key collisions;IsCancelverified safe against bothCancelButton_Clickhandlers (guarded same-valueDialogResult, no double-close); scrollbar gutter arithmetic checked (widest row 350px vs ~401px available when clamped)🤖 Generated with Claude Code