Feature: IP scanner discovery (mDNS, ARP/NDP, SNMP, Web)

.claude/skills/feature-review/SKILL.md

@@ -1,6 +1,6 @@
 ---
 name: feature-review
-description: In-depth review of one NETworkManager feature/tool (e.g. Port Scanner, Hosts File Editor, Firewall), one settings page, or one cross-cutting area (profiles, export, status window, dashboard). First builds an understanding of how the feature works and how it integrates (models, view models, views, settings, profiles, docs), then looks for bugs and logic errors, inconsistencies within the feature and against the rest of the application (UI, icons, spacing, context menus, load/refresh/scan logic, texts), performance problems (scan logic and UI responsiveness), and missing essential functions. Use this whenever the user wants to review, audit, check, or go through a NETworkManager tool, feature, settings page or area, or asks what is wrong/missing/inconsistent in a specific part of the app, even if they don't say "review". Not for reviewing a diff/PR (use code-review for that).
+description: In-depth review of one NETworkManager feature/tool (e.g. Port Scanner, Hosts File Editor, Firewall), one settings page, or one cross-cutting area (profiles, export, status window, dashboard). First builds an understanding of how the feature works and how it integrates (models, view models, views, settings, profiles, docs), then looks for bugs and logic errors, inconsistencies within the feature and against the rest of the application (UI, icons, spacing, context menus, load/refresh/scan logic, texts), performance problems (scan logic and UI responsiveness), missing essential functions, and refactorings that reduce duplication or complexity while keeping the code simple (no over-engineering). Use this whenever the user wants to review, audit, check, or go through a NETworkManager tool, feature, settings page or area, or asks what is wrong/missing/inconsistent in a specific part of the app, even if they don't say "review". Not for reviewing a diff/PR (use code-review for that).
 ---
 
 # NETworkManager feature review
@@ -14,8 +14,9 @@ If no scope is given, ask which one, and offer the list of review units from
 [references/integration-map.md](references/integration-map.md).
 
 The goal is a short, trustworthy list of findings the user can act on — not a long list of maybes.
-The application deliberately keeps its code simple and similar across tools; fixes should follow the
-existing patterns and not introduce new abstractions unless the same problem exists in several places.
+The application deliberately keeps its code simple and similar across tools: fixes and refactorings follow the
+existing patterns, and the code after a change should be as simple or simpler than before — never more abstract than
+the problem needs.
 
 ## Workflow
 
@@ -67,7 +68,10 @@ Go through these areas. Skip an area if it doesn't apply, but say so in one line
    ranges, invalid/duplicate/overlapping entries, no network adapter, no admin rights), wrong disposal / event handlers
    that are never unsubscribed (static events!), status messages overwritten by a later event (e.g. a completion
    event after cancel or after an input error), exceptions in fire-and-forget `async Task` methods (`_ = Start()`)
-   that leave flags set.
+   that leave flags set, a failing optional step (one lookup, one host, a system API call) that ends the whole action
+   instead of only leaving that value empty, state carried over from the previous loop iteration / item (e.g. a value
+   of the first redirect kept for the last one), caches or snapshots used without checking their age, and conditions
+   where an unrelated case suppresses a message or action (e.g. "show X if nothing else failed").
 2. **Consistency within the feature** — the same action behaving differently depending on where it is
    triggered (button vs. context menu vs. keyboard shortcut vs. profile vs. run command / other tool), single vs. multi selection,
    texts/terms that differ inside the feature.
@@ -95,6 +99,50 @@ Go through these areas. Skip an area if it doesn't apply, but say so in one line
    placeholders (`{0}`) that translations may break, missing `...` on items that open a dialog.
 8. **Documentation and changelog** — does `Website/docs/application/<feature>.md` (and settings/profile docs)
    match the actual behaviour, options, defaults, context menu and shortcuts?
+9. **Refactoring and simplification** — duplicated or needlessly complex code that makes the feature harder to
+   change, see the rules below. Look at it last, when you know how the feature works. Hints: a file much larger
+   than the same kind of file in the comparison features (`wc -l`), a file that grew a lot (`git log --stat`), the same
+   lines found twice with `git grep`, a helper from the "Shared helpers" list in
+   [references/conventions.md](references/conventions.md) re-implemented in the feature.
+
+#### Refactoring: only if the code gets simpler
+
+Report a refactoring only if it removes real duplication or complexity and the result is simpler than the code
+today. Good reasons:
+
+- **Duplicated logic that must change together**: the same parsing, validation, mapping or field list in two or more
+  places (e.g. an input format parsed by the validator, the model and a count method; CSV, XML and JSON export each
+  listing the same fields; the same command code in several view models). Strongest when the copies already drifted
+  (a case handled in one copy only) — then it is also a bug finding.
+- **Copy-pasted XAML**: the same block repeated for every section or column (cell templates, section headers with a
+  status icon, label/value rows) → a `Style`, `DataTemplate` or small `UserControl`, like the existing ones.
+- **A class or method with several jobs** that are hard to follow together (e.g. a cache with its own locking and
+  timing inside a scanner, a 300-line `Start()`): extract the separate job into a method or a small class when that
+  makes fixes and tests local.
+- **Dead or needless code**: unused settings, parameters, properties and events, branches that can't be reached,
+  interfaces or base classes with one implementation, helpers with one caller that only wrap another call. Removing
+  code is the simplest refactoring.
+
+Don't report:
+
+- Style preferences, renames for taste, "modernizing" code that works, or rewriting a feature.
+- New layers or patterns for a single use: interfaces, base classes, factories, generic frameworks, dependency
+  injection, event aggregators, settings nobody needs, extension points "for later".
+- Merging code that only looks similar but changes for different reasons (e.g. the ping loops of the scanners, the
+  Ping Monitor and the Traceroute): share the small pieces, not the loops.
+
+Rules for the proposal:
+
+- **Name the gain**: fewer lines and fewer places to touch for the next change (e.g. "3 field lists → 1, about −150
+  lines", "a new input format is added in one place"). If the result isn't shorter or clearer, drop the finding.
+- **Smallest construct that works**: a private method, a static helper, a record or a table/list that drives a loop
+  before a new class; a new class before a project-wide abstraction. Reuse the helpers that already exist
+  (`Utilities`, `Converters`, `Validators`, styles).
+- **Behaviour stays identical**: name what must not change (export column order and names, settings names, file
+  formats, texts, public behaviour) and how it is proven (existing unit tests, or a test written *before* the
+  refactoring that compares the old and the new output). Never mix a behaviour change into a refactoring commit.
+- **Order**: after the bug fixes; code-only refactorings first, XAML refactorings only after the current UI was tested
+  on Windows (so visual differences can be attributed), performance refactorings only after measuring.
 
 ### 5. Verify every finding
 
@@ -136,15 +184,19 @@ Compared with: <features from step 3>
 ### Other (security, localization, docs)
 | # | Finding | Proposed fix |
 
+### Refactoring / simplification
+| # | Where | Problem (duplication / complexity) | Proposal | Gain | Must stay identical / test |
+
 ## Not reviewed / unverified
 <areas skipped, findings that need a Windows test>
 
 ## Suggested order
 <what to fix first and what to group into one PR>
 ```
 
-Reference code as `path:line`. Rate each finding (High / Medium / Low) in the finding text when it helps
-prioritize. Proposed fixes describe the smallest change that follows the existing patterns.
+Reference code as `path:line`. Give every finding a short ID that stays the same until it is done (e.g. `B3` for a
+bug, `R2` for a refactoring) — commits and handover files reference it. Rate each finding (High / Medium / Low) in the
+finding text when it helps prioritize. Proposed fixes describe the smallest change that follows the existing patterns.
 
 For larger missing features, add an issue draft at the end that follows the repository's feature request
 form (`.github/ISSUE_TEMPLATE/feature_request.yml`): a short title, the affected feature, "Problem / motivation",
@@ -156,14 +208,18 @@ Don't change code during the review. Ask the user which findings to fix.
 
 When the user wants the work handed over (to another agent or for later), write the result to
 `.claude/reviews/<feature>.md` (kebab-case, e.g. `ip-scanner.md`; the folder is not committed). Write it as an
-implementation plan, not as a report: verified findings with `path:line`, the concrete fix, the work packages / PR
-order, decisions taken and still open, and what must be tested on Windows. Read existing files in `.claude/reviews/`
-and `.claude/plans/` for the area first — they record earlier decisions.
+implementation plan, not as a report: verified findings with ID, severity, `path:line`, scenario and the concrete fix
+(bugs and refactorings in separate tables, refactorings after the bugs), the work packages / PR order, decisions taken
+and still open, and what must be tested on Windows. Read existing files in `.claude/reviews/` and `.claude/plans/`
+for the area first — they record earlier decisions. These files only track **open** work: when an item is done,
+remove it (and its manual tests once they passed); the history is in git and the PRs.
 
 When fixing:
 
 - Follow the conventions and keep the change minimal; extract shared helpers only when the same code
-  already exists in two or more places.
+  already exists in two or more places. Keep the code simple: no new abstraction for a single use.
+- One commit per finding, with its ID in the subject (e.g. `Fix: Port Scanner: ... (B3)`); a refactoring gets its own
+  commit without behaviour changes.
 - Add strings to `Strings.resx` and, in alphabetical order, to `Strings.Designer.cs`.
 - Update the feature documentation and add a changelog entry in `Website/docs/changelog/next-release.md`.
 - Never modify `Source/3rdparty/`.

.claude/skills/feature-review/references/conventions.md

@@ -19,6 +19,7 @@ report that too.
 - Settings and profiles
 - Navigation and run command
 - PowerShell and external processes
+- Shared helpers (reuse before writing new code)
 - Strings, docs and changelog
 
 ## View model basics
@@ -221,6 +222,24 @@ Reference: `Source/NETworkManager.Models/Firewall/Firewall.cs`.
   untrusted: with `cmd.exe /c ...` characters like `&` or `|` run other commands. Only insert values that match the
   expected format (IP address, hostname regex).
 
+## Shared helpers (reuse before writing new code)
+
+Duplicated code often re-implements one of these. A refactoring proposal should use them (or extend them) before it
+adds a new helper. Check the class for the exact methods.
+
+- Host and port input: `HostRangeHelper` (Models: `CreateListFromInput`, `GetHostCount`, `ResolveAsync` with `!`
+  exclusions - IP Scanner, Port Scanner, Ping Monitor), `PortRangeHelper` (Utilities), validators in
+  `NETworkManager.Validators` (one per input format, used by tool, profile dialog and run command).
+- Addresses and names: `IPAddressHelper` (compare / sort, private range, scope id), `MACAddressHelper` (format,
+  compare), `DNSClientHelper` / `DNSClient` (resolve A / AAAA / PTR with the app's DNS settings), `RegexHelper`.
+- Lists and texts: `ListHelper.Modify` (history), `PlaceholderHelper` (`{{Name}}` placeholders), `DateTimeHelper`,
+  `ClipboardHelper`, `DialogHelper` (confirmations, `FormatItemList`).
+- WPF: `BindingProxy`, `DataGridHelper`, `SearchTextBoxHelper`, `PermissionToolTipHelper` (Utilities.WPF),
+  `ObservableRangeCollection` (Controls), converters in `NETworkManager.Converters` (e.g. `PendingToStringConverter`),
+  shared styles in `Source/NETworkManager/Resources/Styles/`.
+- PowerShell: `PowerShellHelper.EscapeSingleQuotes`; arrays (`ToPsArray`) and the shared runspace are still private to
+  `Firewall.cs` - a second user of them is a reason to move them to `PowerShellHelper`.
+
 ## Strings, docs and changelog
 
 - User-visible text comes from `Strings.resx` (English only; other languages via Transifex), added in

Source/NETworkManager.Converters/IPScannerDetectionMethodToStringConverter.cs

@@ -0,0 +1,77 @@
+using System;
+using System.Collections.Generic;
+using System.Globalization;
+using System.Windows.Data;
+using NETworkManager.Localization.Resources;
+using NETworkManager.Models.Network;
+
+namespace NETworkManager.Converters;
+
+/// <summary>
+///     Convert the <see cref="IPScannerDetectionMethod" /> of a host to a translated <see cref="string" />
+///     (e.g. "Ping, TCP").
+/// </summary>
+public sealed class IPScannerDetectionMethodToStringConverter : IValueConverter
+{
+    /// <summary>
+    ///     Convert the <see cref="IPScannerDetectionMethod" /> of a host to a translated <see cref="string" />.
+    /// </summary>
+    /// <param name="value">Probes the host answered as <see cref="IPScannerDetectionMethod" />.</param>
+    /// <param name="targetType"></param>
+    /// <param name="parameter">"Status" to get the status of the host ("Up (detected by Ping, TCP)" / "Down").</param>
+    /// <param name="culture"></param>
+    /// <returns>Translated, comma-separated probes, "-/-" if the host didn't answer any probe.</returns>
+    public object Convert(object value, Type targetType, object parameter, CultureInfo culture)
+    {
+        var detectedBy = value is IPScannerDetectionMethod method ? method : IPScannerDetectionMethod.None;
+
+        var names = GetNames(detectedBy);
+
+        if (parameter as string == "Status")
+            return detectedBy == IPScannerDetectionMethod.None
+                ? Strings.Down
+                : string.Format(Strings.UpDetectedByX, string.Join(", ", names));
+
+        return detectedBy == IPScannerDetectionMethod.None ? "-/-" : string.Join(", ", names);
+    }
+
+    /// <summary>
+    ///     Gets the translated names of the probes a host answered.
+    /// </summary>
+    /// <param name="detectedBy">Probes the host answered.</param>
+    /// <returns>Translated names, empty if the host didn't answer any probe.</returns>
+    private static List<string> GetNames(IPScannerDetectionMethod detectedBy)
+    {
+        var names = new List<string>(5);
+
+        if (detectedBy.HasFlag(IPScannerDetectionMethod.ICMP))
+            names.Add(Strings.Ping);
+
+        if (detectedBy.HasFlag(IPScannerDetectionMethod.TCP))
+            names.Add(Strings.TCP);
+
+        if (detectedBy.HasFlag(IPScannerDetectionMethod.NetBIOS))
+            names.Add(Strings.NetBIOS);
+
+        if (detectedBy.HasFlag(IPScannerDetectionMethod.SNMP))
+            names.Add(Strings.SNMP);
+
+        if (detectedBy.HasFlag(IPScannerDetectionMethod.ARP))
+            names.Add(Strings.ARPNDP);
+
+        return names;
+    }
+
+    /// <summary>
+    ///     !!! Method not implemented !!!
+    /// </summary>
+    /// <param name="value"></param>
+    /// <param name="targetType"></param>
+    /// <param name="parameter"></param>
+    /// <param name="culture"></param>
+    /// <returns></returns>
+    public object ConvertBack(object value, Type targetType, object parameter, CultureInfo culture)
+    {
+        throw new NotImplementedException();
+    }
+}

Source/NETworkManager.Converters/IPScannerHostChangeToStringConverter.cs

@@ -0,0 +1,56 @@
+using System;
+using System.Globalization;
+using System.Windows.Data;
+using NETworkManager.Localization.Resources;
+using NETworkManager.Models.Network;
+
+namespace NETworkManager.Converters;
+
+/// <summary>
+///     Convert the <see cref="IPScannerHostChange" /> of a host (since the previous scan) to a translated
+///     <see cref="string" /> (e.g. "New").
+/// </summary>
+public sealed class IPScannerHostChangeToStringConverter : IValueConverter
+{
+    /// <summary>
+    ///     Convert the <see cref="IPScannerHostChange" /> of a host to a translated <see cref="string" />.
+    /// </summary>
+    /// <param name="value">Change of the host as <see cref="IPScannerHostChange" />.</param>
+    /// <param name="targetType"></param>
+    /// <param name="parameter"></param>
+    /// <param name="culture"></param>
+    /// <returns>Translated change, empty if there is none.</returns>
+    public object Convert(object value, Type targetType, object parameter, CultureInfo culture)
+    {
+        return value is IPScannerHostChange change ? ToLocalizedString(change) : string.Empty;
+    }
+
+    /// <summary>
+    ///     Gets the translated change of a host.
+    /// </summary>
+    /// <param name="change">Change of the host.</param>
+    /// <returns>Translated change, empty for <see cref="IPScannerHostChange.None" />.</returns>
+    public static string ToLocalizedString(IPScannerHostChange change)
+    {
+        return change switch
+        {
+            IPScannerHostChange.New => Strings.HostChange_New,
+            IPScannerHostChange.Gone => Strings.HostChange_Gone,
+            IPScannerHostChange.MACChanged => Strings.HostChange_MACChanged,
+            _ => string.Empty
+        };
+    }
+
+    /// <summary>
+    ///     !!! Method not implemented !!!
+    /// </summary>
+    /// <param name="value"></param>
+    /// <param name="targetType"></param>
+    /// <param name="parameter"></param>
+    /// <param name="culture"></param>
+    /// <returns></returns>
+    public object ConvertBack(object value, Type targetType, object parameter, CultureInfo culture)
+    {
+        throw new NotImplementedException();
+    }
+}