Skip to content

Fix Overview crosshair disappearing after tab switches / layout passes #864

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
erikdarlingdata merged 1 commit into from
Apr 19, 2026
+103 −16
Merged

Fix Overview crosshair disappearing after tab switches / layout passes #864

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 19 additions & 1 deletion Dashboard/Controls/CorrelatedTimelineLanesControl.xaml.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,12 @@ public partial class CorrelatedTimelineLanesControl : UserControl
public CorrelatedTimelineLanesControl()
{
InitializeComponent();
Unloaded += (_, _) => _crosshairManager?.Dispose();
/* No Unloaded → Dispose() handler: WPF fires Unloaded for transient
reasons (tab virtualization, layout rebuilds) and Dispose() clears
the crosshair manager's lane list, permanently breaking the crosshair
until the ServerTab is rebuilt. The manager holds only managed state
(a Popup + lane references) — letting GC clean it up with the control
is fine. */
}

/// <summary>
Expand Down Expand Up @@ -69,6 +74,9 @@ public async Task RefreshAsync(int hoursBack, DateTime? fromDate, DateTime? toDa

_crosshairManager?.PrepareForRefresh();

try
{

var cpuTask = _dataService.GetCpuUtilizationAsync(hoursBack, fromDate, toDate);
var waitTask = _dataService.GetTotalWaitStatsTrendAsync(hoursBack, fromDate, toDate);
var blockingTask = _dataService.GetBlockedSessionTrendAsync(hoursBack, fromDate, toDate);
Expand Down Expand Up @@ -225,8 +233,18 @@ public async Task RefreshAsync(int hoursBack, DateTime? fromDate, DateTime? toDa
_crosshairManager?.SetComparisonLabel(ComparisonLabel(comparisonRange.Value, fromDate, hoursBack));
}

/* VLines must be re-attached before SyncXAxes so they're part of
the render set when the chart refreshes. */
_crosshairManager?.ReattachVLines();
SyncXAxes(hoursBack, fromDate, toDate);
}
Comment on lines 77 to +240
Copy link
Copy Markdown
Owner Author

@erikdarlingdata erikdarlingdata Apr 19, 2026

Choose a reason for hiding this comment

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

The body of this try (lines 80-239) is not re-indented and there's a stray blank line at 79. Compiles fine, but the raw file now has ~160 lines sitting at outer-method indent inside a try block, which will confuse anyone reading the file without a diff view. I get that leaving it un-indented minimizes this diff — worth a follow-up tidy-up commit to re-indent the block so future readers don't do a double-take.

Generated by Claude Code

finally
{
/* Safety net: if something threw between PrepareForRefresh() and the
ReattachVLines() call above, VLines are still null. EnsureVLinesAttached
creates them only for lanes where VLine is null, so it's idempotent. */
_crosshairManager?.EnsureVLinesAttached();
}
}

/// <summary>
Expand Down
43 changes: 36 additions & 7 deletions Dashboard/Helpers/CorrelatedCrosshairManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ internal sealed class CorrelatedCrosshairManager : IDisposable
private readonly Popup _tooltip;
private readonly TextBlock _tooltipText;
private DateTime _lastUpdate;
private bool _isRefreshing;

public CorrelatedCrosshairManager()
{
Expand Down Expand Up @@ -144,10 +143,11 @@ public void SetComparisonLabel(string label)

/// <summary>
/// Clears data and VLines. Call before re-populating charts.
/// The OnMouseMove guard relies on lane.VLine == null to detect "not ready",
/// so this is self-healing: once ReattachVLines runs, crosshairs resume.
/// </summary>
public void PrepareForRefresh()
{
_isRefreshing = true;
_tooltip.IsOpen = false;
_comparisonLabel = null;
foreach (var lane in _lanes)
Expand All @@ -162,25 +162,54 @@ public void PrepareForRefresh()

/// <summary>
/// Creates fresh VLine plottables on each lane's chart.
/// Must be called AFTER chart data is populated.
/// Must be called AFTER chart data is populated. Safe to call in a finally
/// block — if chart state is invalid, a failure on one lane won't prevent
/// the others from recovering.
/// </summary>
public void ReattachVLines()
Copy link
Copy Markdown
Owner Author

@erikdarlingdata erikdarlingdata Apr 19, 2026

Choose a reason for hiding this comment

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

Heads-up: after removing the Unloaded → Dispose() handler, nothing in the codebase calls CorrelatedCrosshairManager.Dispose() anymore (the class still implements IDisposable and keeps a Dispose() at line 371). That's now dead code. Two options for a follow-up: (a) wire Dispose() to a real teardown point such as ServerTab close so _lanes.Clear() actually runs when the tab goes away, or (b) drop IDisposable + Dispose() entirely and let GC handle it (matches the comment rationale on the constructor). Leaving IDisposable declared with no caller sets a false expectation for the next reader. Mirror applies in Lite/Helpers/CorrelatedCrosshairManager.cs.

Generated by Claude Code

{
foreach (var lane in _lanes)
{
var vline = lane.Chart.Plot.Add.VerticalLine(0);
lane.VLine = CreateVLine(lane.Chart);
}
}

/// <summary>
/// Creates VLines only for lanes that don't already have one. Idempotent —
/// safe to call from a finally block as a recovery path after an exception
/// in the main refresh flow.
/// </summary>
public void EnsureVLinesAttached()
{
foreach (var lane in _lanes)
{
if (lane.VLine != null) continue;
lane.VLine = CreateVLine(lane.Chart);
}
}

private static ScottPlot.Plottables.VerticalLine? CreateVLine(ScottPlot.WPF.WpfPlot chart)
{
try
{
var vline = chart.Plot.Add.VerticalLine(0);
vline.Color = ScottPlot.Color.FromHex("#FFFFFF").WithAlpha(100);
vline.LineWidth = 1;
vline.LinePattern = ScottPlot.LinePattern.Dashed;
vline.IsVisible = false;
lane.VLine = vline;
return vline;
}
catch
{
/* If attach fails, return null so OnMouseMove skips this lane.
Next refresh will try again. */
return null;
Copy link
Copy Markdown
Owner Author

@erikdarlingdata erikdarlingdata Apr 19, 2026

Choose a reason for hiding this comment

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

Silent catch-all. Everywhere else in CorrelatedTimelineLanesControl.RefreshAsync a Debug.WriteLine(...) accompanies catch (Exception ex) (e.g. line 116, 161, 198). If Plot.Add.VerticalLine ever starts throwing, the only signal will be a silently-missing crosshair on one lane — exactly the class of bug this PR exists to diagnose. At minimum catch (Exception ex) { Debug.WriteLine($"CorrelatedCrosshairManager: VLine attach failed: {ex.Message}"); return null; }.

Generated by Claude Code

}
_isRefreshing = false;
}

private void OnMouseMove(LaneInfo sourceLane, MouseEventArgs e)
{
if (_isRefreshing || sourceLane.VLine == null) return;
if (sourceLane.VLine == null) return;

var now = DateTime.UtcNow;
if ((now - _lastUpdate).TotalMilliseconds < 16) return;
Expand Down
13 changes: 12 additions & 1 deletion Lite/Controls/CorrelatedTimelineLanesControl.xaml.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,12 @@
public CorrelatedTimelineLanesControl()
{
InitializeComponent();
Unloaded += (_, _) => _crosshairManager?.Dispose();
/* No Unloaded → Dispose() handler: WPF fires Unloaded for transient
reasons (tab virtualization, layout rebuilds) and Dispose() clears
the crosshair manager's lane list, permanently breaking the crosshair
until the ServerTab is rebuilt. The manager holds only managed state
(a Popup + lane references) — letting GC clean it up with the control
is fine. */
}

/// <summary>
Expand Down Expand Up @@ -170,11 +175,11 @@
$"cpuRows={refCpuTask.Result?.Count ?? 0}, waitRows={refWaitTask.Result?.Count ?? 0}");

if (refCpuTask.IsCompletedSuccessfully)
AddGhostLine(CpuChart, refCpuTask.Result

Check warning on line 178 in Lite/Controls/CorrelatedTimelineLanesControl.xaml.cs

View workflow job for this annotation

GitHub Actions / build 

Possible null reference argument for parameter 'source' in 'IEnumerable<(double, double)> Enumerable.Select<CpuUtilizationRow, (double, double)>(IEnumerable<CpuUtilizationRow> source, Func<CpuUtilizationRow, (double, double)> selector)'.

Check warning on line 178 in Lite/Controls/CorrelatedTimelineLanesControl.xaml.cs

View workflow job for this annotation

GitHub Actions / build 

Possible null reference argument for parameter 'source' in 'IEnumerable<(double, double)> Enumerable.Select<CpuUtilizationRow, (double, double)>(IEnumerable<CpuUtilizationRow> source, Func<CpuUtilizationRow, (double, double)> selector)'.
.Select(d => (d.SampleTime.Add(timeShift).ToOADate(), (double)d.SqlServerCpu)).ToList(), "#4FC3F7");

if (refWaitTask.IsCompletedSuccessfully)
AddGhostLine(WaitStatsChart, refWaitTask.Result

Check warning on line 182 in Lite/Controls/CorrelatedTimelineLanesControl.xaml.cs

View workflow job for this annotation

GitHub Actions / build 

Possible null reference argument for parameter 'source' in 'IEnumerable<(double, double WaitTimeMsPerSecond)> Enumerable.Select<WaitStatsTrendPoint, (double, double WaitTimeMsPerSecond)>(IEnumerable<WaitStatsTrendPoint> source, Func<WaitStatsTrendPoint, (double, double WaitTimeMsPerSecond)> selector)'.

Check warning on line 182 in Lite/Controls/CorrelatedTimelineLanesControl.xaml.cs

View workflow job for this annotation

GitHub Actions / build 

Possible null reference argument for parameter 'source' in 'IEnumerable<(double, double WaitTimeMsPerSecond)> Enumerable.Select<WaitStatsTrendPoint, (double, double WaitTimeMsPerSecond)>(IEnumerable<WaitStatsTrendPoint> source, Func<WaitStatsTrendPoint, (double, double WaitTimeMsPerSecond)> selector)'.
.Select(d => (d.CollectionTime.AddMinutes(utcOffset).Add(timeShift).ToOADate(), d.WaitTimeMsPerSecond)).ToList(), "#FFB74D");

if (refBlockingTask.IsCompletedSuccessfully)
Expand Down Expand Up @@ -203,11 +208,17 @@
_crosshairManager?.SetComparisonLabel(ComparisonLabel(comparisonRange.Value, fromDate, hoursBack));
}

/* VLines must be re-attached before SyncXAxes so they're part of
the render set when the chart refreshes. */
_crosshairManager?.ReattachVLines();
SyncXAxes(hoursBack, fromDate, toDate, utcOffset);
}
finally
{
/* Safety net: if something threw between PrepareForRefresh() and the
ReattachVLines() call above, VLines are still null. EnsureVLinesAttached
creates them only for lanes where VLine is null, so it's idempotent. */
_crosshairManager?.EnsureVLinesAttached();
_isRefreshing = false;
}
}
Expand Down
43 changes: 36 additions & 7 deletions Lite/Helpers/CorrelatedCrosshairManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ internal sealed class CorrelatedCrosshairManager : IDisposable
private readonly Popup _tooltip;
private readonly TextBlock _tooltipText;
private DateTime _lastUpdate;
private bool _isRefreshing;

public CorrelatedCrosshairManager()
{
Expand Down Expand Up @@ -144,10 +143,11 @@ public void SetComparisonLabel(string label)

/// <summary>
/// Clears data and VLines. Call before re-populating charts.
/// The OnMouseMove guard relies on lane.VLine == null to detect "not ready",
/// so this is self-healing: once ReattachVLines runs, crosshairs resume.
/// </summary>
public void PrepareForRefresh()
{
_isRefreshing = true;
_tooltip.IsOpen = false;
_comparisonLabel = null;
foreach (var lane in _lanes)
Expand All @@ -162,25 +162,54 @@ public void PrepareForRefresh()

/// <summary>
/// Creates fresh VLine plottables on each lane's chart.
/// Must be called AFTER chart data is populated.
/// Must be called AFTER chart data is populated. Safe to call in a finally
/// block — if chart state is invalid, a failure on one lane won't prevent
/// the others from recovering.
/// </summary>
public void ReattachVLines()
{
foreach (var lane in _lanes)
{
var vline = lane.Chart.Plot.Add.VerticalLine(0);
lane.VLine = CreateVLine(lane.Chart);
}
}

/// <summary>
/// Creates VLines only for lanes that don't already have one. Idempotent —
/// safe to call from a finally block as a recovery path after an exception
/// in the main refresh flow.
/// </summary>
public void EnsureVLinesAttached()
{
foreach (var lane in _lanes)
{
if (lane.VLine != null) continue;
lane.VLine = CreateVLine(lane.Chart);
}
}

private static ScottPlot.Plottables.VerticalLine? CreateVLine(ScottPlot.WPF.WpfPlot chart)
{
try
{
var vline = chart.Plot.Add.VerticalLine(0);
vline.Color = ScottPlot.Color.FromHex("#FFFFFF").WithAlpha(100);
vline.LineWidth = 1;
vline.LinePattern = ScottPlot.LinePattern.Dashed;
vline.IsVisible = false;
lane.VLine = vline;
return vline;
}
catch
{
/* If attach fails, return null so OnMouseMove skips this lane.
Next refresh will try again. */
return null;
Copy link
Copy Markdown
Owner Author

@erikdarlingdata erikdarlingdata Apr 19, 2026

Choose a reason for hiding this comment

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

Same silent-catch issue as the Dashboard copy. Kept in sync, but fix belongs in both — log the exception instead of swallowing it. A VLine attach failure here is invisible except as a missing crosshair.

Generated by Claude Code

}
_isRefreshing = false;
}

private void OnMouseMove(LaneInfo sourceLane, MouseEventArgs e)
{
if (_isRefreshing || sourceLane.VLine == null) return;
if (sourceLane.VLine == null) return;

var now = DateTime.UtcNow;
if ((now - _lastUpdate).TotalMilliseconds < 16) return;
Expand Down
Loading