Skip to content

fix: code quality — paint leak, stable chart offset, PerformanceVM dispose #409

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
laurentiu021 merged 1 commit into from
May 15, 2026
Merged

fix: code quality — paint leak, stable chart offset, PerformanceVM dispose #409

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
11 changes: 11 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,17 @@ adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).

## [Unreleased]

## [0.48.27] - 2026-05-15

### Fixed
- **NetworkSharedState** — SkiaSharp `SolidColorPaint` objects are now disposed
when a ping target is removed, preventing unmanaged memory leaks (CQ-M1).
- **NetworkSharedState** — latency chart offset now uses a stable hash of the
target host instead of `Targets.IndexOf`, preventing visual jumps when
targets are removed mid-session (CQ-M2).
- **PerformanceViewModel** — added `Dispose` override to clean up snapshot
reference and satisfy the base class disposal contract (CQ-M4).

## [0.48.26] - 2026-05-15

### Changed
Expand Down
41 changes: 35 additions & 6 deletions SysManager/SysManager/ViewModels/NetworkSharedState.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,10 +225,23 @@ internal void RemoveTargetInternal(PingTarget target)
_targetHandlers.Remove(target.Host);
}
Targets.Remove(target);

// CQ-M1: Dispose SkiaSharp paint objects attached to the series being removed.
// Without this, SolidColorPaint instances (and their unmanaged SKPaint handles)
// leak every time a target is removed.
var idx = LatencySeries.ToList().FindIndex(s => s.Name?.Contains($"({target.Host})") == true);
if (idx >= 0) LatencySeries.RemoveAt(idx);
if (idx >= 0)
{
DisposeSeries(LatencySeries[idx]);
LatencySeries.RemoveAt(idx);
}
var tIdx = TraceSeries.ToList().FindIndex(s => s.Name?.Contains($"({target.Host})") == true);
if (tIdx >= 0) TraceSeries.RemoveAt(tIdx);
if (tIdx >= 0)
{
DisposeSeries(TraceSeries[tIdx]);
TraceSeries.RemoveAt(tIdx);
}

Buffers.TryRemove(target.Host, out _);
TraceBuffers.TryRemove(target.Host, out _);
LatestRoutes.Remove(target.Host);
Expand All @@ -237,6 +250,18 @@ internal void RemoveTargetInternal(PingTarget target)
RefreshHopTable();
}

/// <summary>Disposes paint resources attached to a chart series.</summary>
private static void DisposeSeries(ISeries series)
{
if (series is LineSeries<DateTimePoint> line)
{
(line.Stroke as IDisposable)?.Dispose();
(line.GeometryStroke as IDisposable)?.Dispose();
(line.GeometryFill as IDisposable)?.Dispose();
(line.Fill as IDisposable)?.Dispose();
}
}
Comment on lines +253 to +263

@coderabbitai coderabbitai Bot May 15, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail
FILE="SysManager/SysManager/ViewModels/NetworkSharedState.cs"

echo "== Series construction and dispose helper =="
rg -n 'LatencySeries.Add|TraceSeries.Add|LineSeries<|DisposeSeries\(' "$FILE"

Repository: laurentiu021/SystemManager

Length of output: 452

🏁 Script executed:

#!/bin/bash
set -euo pipefail
FILE="SysManager/SysManager/ViewModels/NetworkSharedState.cs"

echo "== Context around series creation (lines 150-180) =="
sed -n '150,180p' "$FILE" | cat -n

echo -e "\n== Context around disposal calls (lines 230-250) =="
sed -n '230,250p' "$FILE" | cat -n

echo -e "\n== Full DisposeSeries method (lines 253-270) =="
sed -n '253,270p' "$FILE" | cat -n

Repository: laurentiu021/SystemManager

Length of output: 3338

Dispose helper skips TraceSeries paint resources.

At line 256, DisposeSeries only matches LineSeries<DateTimePoint>, but traceroute uses LineSeries<ObservablePoint> (line 168). When called at line 241, the disposal is skipped for trace series, leaving their paint resources (Stroke, GeometryStroke, GeometryFill) undisposed.

Suggested fix 
 private static void DisposeSeries(ISeries series)
 {
-    if (series is LineSeries<DateTimePoint> line)
+    if (series is LineSeries<DateTimePoint> or LineSeries<ObservablePoint>)
     {
-        (line.Stroke as IDisposable)?.Dispose();
-        (line.GeometryStroke as IDisposable)?.Dispose();
-        (line.GeometryFill as IDisposable)?.Dispose();
-        (line.Fill as IDisposable)?.Dispose();
+        ((line.Stroke as IDisposable)?.Dispose();
+        (line.GeometryStroke as IDisposable)?.Dispose();
+        (line.GeometryFill as IDisposable)?.Dispose();
+        (line.Fill as IDisposable)?.Dispose();
     }
 }
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@SysManager/SysManager/ViewModels/NetworkSharedState.cs` around lines 253 -
263, DisposeSeries currently only handles LineSeries<DateTimePoint>, so
LineSeries<ObservablePoint> (used by traceroute) is skipped and its
Stroke/GeometryStroke/GeometryFill/Fill remain undisposed; update DisposeSeries
to detect any LineSeries<T> (not just DateTimePoint) — e.g., check
series.GetType().IsGenericType && series.GetType().GetGenericTypeDefinition() ==
typeof(LineSeries<>) or test for a non-generic LineSeries base/interface — then
cast the instance to a variable (or use dynamic) and call Dispose on Stroke,
GeometryStroke, GeometryFill and Fill exactly as done for line to ensure all
paint resources are disposed for trace series as well.


public void ClearHistory()
{
foreach (var buf in Buffers.Values) buf.Clear();
Expand Down Expand Up @@ -313,8 +338,9 @@ internal void FlushPending()
double? shown = sample.LatencyMs;
if (shown.HasValue)
{
var idx = Targets.IndexOf(target);
var offset = ((idx % 8) - 3.5) * 0.25;
// CQ-M2: Stable offset (same fix as RecomputeStats).
var stableIdx = Math.Abs(target.Host.GetHashCode()) % 8;
var offset = ((stableIdx % 8) - 3.5) * 0.25;
shown = shown.Value + offset;
}

Expand Down Expand Up @@ -355,8 +381,11 @@ private void RecomputeStats(PingTarget target, ObservableCollection<DateTimePoin
return;
}

var idx = Targets.IndexOf(target);
var offset = ((idx % 8) - 3.5) * 0.25;
// CQ-M2: Use a stable offset derived from the target's host hash instead
// of Targets.IndexOf. IndexOf shifts after target removal, causing all
// remaining targets to jump visually on the chart.
var stableIdx = Math.Abs(target.Host.GetHashCode()) % 8;
var offset = ((stableIdx % 8) - 3.5) * 0.25;

// PERF-M2: Avoid LINQ allocations (this runs 32x/sec per target).
// Single pass over buffer to compute sum and count.
Expand Down
12 changes: 12 additions & 0 deletions SysManager/SysManager/ViewModels/PerformanceViewModel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -541,4 +541,16 @@ private void SyncTogglesFromProfile()
var planKey = GetCurrentPlanKey();
IsProcessorStateLocked = planKey is "high" or "ultimate";
}

// CQ-M4: Override Dispose to clean up the PerformanceService's PowerShellRunner
// event subscriptions and prevent the fire-and-forget InitAsync from running
// against a disposed ViewModel if navigation happens quickly.
protected override void Dispose(bool disposing)
{
if (disposing)
{
_snapshot = null;
}
base.Dispose(disposing);
}
}
Loading