Fix Overview crosshair disappearing after tab switches / layout passes#864
Fix Overview crosshair disappearing after tab switches / layout passes#864erikdarlingdata merged 1 commit intodevfrom
Conversation
Root cause: the control wired `Unloaded += ...Dispose()` on the crosshair manager, and WPF fires Unloaded for transient reasons (tab virtualization, layout rebuilds, etc.), not just when the control is actually going away. Dispose() clears the manager's lane list, after which ReattachVLines runs over an empty list and the crosshair is gone permanently. Changes: - Remove the Unloaded → Dispose() handler in both Lite and Dashboard copies. The manager holds only managed state (a Popup + lane references) — GC will clean it up with the control. - Remove the now-redundant `_isRefreshing` flag from CorrelatedCrosshairManager. The `lane.VLine == null` check in OnMouseMove is a sufficient "not ready" guard and is self-healing once VLines are recreated. - Wrap ReattachVLines in a try/finally on the control side, with a new idempotent EnsureVLinesAttached() safety net that only creates VLines for lanes where they're still null. - Make CreateVLine catch per-lane exceptions so one failing chart can't prevent the others from recovering. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
| { | ||
| /* If attach fails, return null so OnMouseMove skips this lane. | ||
| Next refresh will try again. */ | ||
| return null; |
There was a problem hiding this comment.
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
| { | ||
| /* If attach fails, return null so OnMouseMove skips this lane. | ||
| Next refresh will try again. */ | ||
| return null; |
There was a problem hiding this comment.
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
| @@ -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); | |||
| } | |||
There was a problem hiding this comment.
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
| /// block — if chart state is invalid, a failure on one lane won't prevent | ||
| /// the others from recovering. | ||
| /// </summary> | ||
| public void ReattachVLines() |
There was a problem hiding this comment.
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
Review summaryNote: PR was already merged by the time this review ran — comments are for follow-up consideration, not blocking. What this PR doesFixes Overview-tab crosshair disappearing after tab switches / layout passes. Root cause: What's good
What needs attention
No tests were added — UI-timing behavior in WPF/Avalonia is hard to cover with the existing harness; calling it out rather than flagging as blocking. Generated by Claude Code |
1 participant
Summary
Crosshair on the Overview tab's correlated timeline lanes was working on first open but disappearing after some combination of auto-refreshes, tab switches, or layout events. Diagnosed via logging: the manager's lane list was going from 5 to 0 unexpectedly.
Root cause: the control had
Unloaded += ...Dispose()on the crosshair manager. WPF firesUnloadedfor transient reasons (tab virtualization, layout rebuilds) — not just when the control is truly going away.Dispose()clears the lane list, after whichReattachVLinesruns over an empty list and the crosshair is permanently gone.Changes
Unloaded → Dispose()handler in both Lite and Dashboard copies. The manager holds only managed state (a Popup + lane references) — GC will clean it up with the control._isRefreshingflag fromCorrelatedCrosshairManager. Thelane.VLine == nullcheck inOnMouseMoveis a sufficient "not ready" guard and is self-healing once VLines are recreated.ReattachVLinesin a try/finally on the control side, with a new idempotentEnsureVLinesAttached()safety net that only creates VLines for lanes where they're still null.CreateVLineso one failing chart can't prevent the others from recovering.Test plan
🤖 Generated with Claude Code