UI polish: real About version, remove dead quick-help and TrendPlot remnants, fix crashes and handler leaks

[BrettKinny]

Summary

UI/CLI polish pass from the v1.0 follow-up review (docs/V1-REVIEW-FOLLOWUP.md, items 14–15 + the UI lows). Deliberately avoids the MainWindow connect/load regions and the OpcUa files touched by #167/#171 so it merges cleanly alongside them.

• About dialog shows the real version: reads AssemblyInformationalVersionAttribute (full MinVer semver, +metadata stripped) instead of AssemblyVersion, which MinVer pins to MAJOR.0.0.0 — v1.0.1 would have displayed "v1.0.0" forever.
• Dead "context-sensitive quick help" feature removed (precedent: the Trend feature deletion in Clean up UI: dispose dialogs, drop dead Trend feature, async disconnect #164): nothing ever bound ShowQuickHelp, yet HelpDialog's tip text advertised it. QuickHelpDialog.cs deleted, the action removed from IKeybindingActions/MainWindow, tip text fixed.
• TrendPlot remnants gone (declared loose end from Clean up UI: dispose dialogs, drop dead Trend feature, async disconnect #164): enum value, both KeybindingManager switch arms, the test assertion, and the stray doc-comment mention.
• Program.cs config-not-found path: the check now runs before Application.Init() — previously the error was written into the alternate screen (lost on shutdown) and the constructed MainWindow leaked. Dispose is now exception-safe via try/finally.
• ScopeView: axis labels are skipped when the plot is narrower than the label instead of crashing the draw loop with Math.Clamp(min > max) in ~13-column terminals.
• Event-handler hygiene: LogView unsubscribes Logger.LogAdded; MainWindow unsubscribes the static Application.SizeChanging handler (now stored in a field) in Dispose.
• NodeDetailsView stale-response guard: out-of-order attribute reads can no longer show node A's details while node B is selected — responses for a node that's no longer current are discarded.
• Dead code/menu accuracy: removed the unwired MainWindow.OnClientDisconnected; the File-menu recording item is now "Toggle Recording" and actually toggles (it previously said "Start Recording..." while Ctrl+R toggled).

Test plan

• dotnet build Opcilloscope.sln -c Release — 0 warnings, 0 errors
• dotnet test -c Release — 613/613 passing (full suite incl. integration)

---

Part of the v1 follow-up punch list (docs/V1-REVIEW-FOLLOWUP.md, items 14–15 + lows).

https://claude.ai/code/session_012Vopnd9vWkzELveHRgZhie

---

Generated by Claude Code

[claude]

Code Review

Overall: clean, well-scoped polish PR. The motivations are clear, the diff is focused, and it successfully avoids the areas touched by #167/#171. A few items worth noting:

---

Issues

1. HelpDialog.cs shows as a binary diff

The diff for App/Dialogs/HelpDialog.cs is listed as binary files differing, which is unexpected for a C# source file. This usually means an encoding change (BOM added/removed) or line-ending conversion (CRLF ↔ LF). The content changes may be correct, but a binary representation in the diff makes review impossible and can introduce stealthy encoding drift into the repo. Worth investigating before merge.

2. "Stop Recording" menu item is now redundant (MainWindow.cs ~line 299–301)

After renaming "Start Recording..." → "Toggle Recording", the menu still contains a separate "Stop Recording" item directly below it:

new MenuItem("Toggle Recording", "", ToggleRecording, shortcutKey: Key.R.WithCtrl),
new MenuItem("Stop Recording", "", () => OnStopRecordingRequested()),

With ToggleRecording handling both start and stop, the second item is confusing — a user who clicks "Toggle Recording" while recording is active will stop it, but "Stop Recording" exists too with no apparent difference. Consider removing "Stop Recording" from the menu, or if the split is intentional (e.g. "Stop Recording" should be greyed out when not recording), add a note explaining the design.

3. Verify _currentNodeId is set in ShowNodeByIdAsync (NodeDetailsView.cs)

The stale-response guard in ShowNodeAsync works correctly because _currentNodeId = nodeId is assigned before the await (line ~171 in the new code). The same guard is added to ShowNodeByIdAsync (lines ~123–126), but the diff doesn't show whether _currentNodeId is assigned at the top of that method. If it isn't, the Equals(_currentNodeId, nodeId) check will always compare against whatever ShowNodeAsync last wrote — which may silently fail to guard concurrent ShowNodeByIdAsync calls. Quick audit recommended.

---

Minor

4. Unnecessary null check in LogView.Dispose (LogView.cs)

if (_logger != null)
{
    _logger.LogAdded -= OnLogAdded;
}

_logger is readonly and injected via the constructor, so it can't be null at Dispose time under normal construction. Unsubscribing without the guard is idiomatic and less noisy — -= on a null event is a no-op. The guard is harmless but adds clutter.

---

What's Good

• GetDisplayVersion() — the doc comment explaining why MinVer freezes AssemblyVersion at MAJOR.0.0.0 is exactly the kind of non-obvious constraint that warrants a comment. The IndexOf('+') + slice approach to strip build metadata is clean.
• Program.cs restructure — moving the config-not-found check before Application.Init() and wrapping Application.Run in a try/finally are both correct. The error now appears on the real console, and the window is always disposed even if LoadConfigFromCommandLine throws.
• ScopeView narrow-terminal fix — minimal and correct. The maxLabelX < plotLeft early-continue is cleaner than trying to clamp a negative range, and the crash path is clearly explained.
• Static event handler leak fix — storing _sizeChangingHandler as a field so it can be unsubscribed in Dispose is the right pattern for static events. The parallel fix in LogView is equally correct.
• Dead code removal — removing OnClientDisconnected, QuickHelpDialog, ShowQuickHelp, and the TrendPlot enum value/switch arms is clean-up that was clearly overdue. The CLAUDE.md update keeps the project structure accurate.

[claude]

Code Review

This is a well-scoped, focused polish pass. All changes address real bugs or remove dead code, and the PR description is thorough. Since this is already merged, the following is post-merge feedback for the team's awareness.

---

Strengths

• GetDisplayVersion() is correct and well-documented. Using AssemblyInformationalVersionAttribute with the +metadata strip is the right MinVer pattern; the fallback to AssemblyVersion is a safe guard.
• Dead code removal is clean and consistent — QuickHelpDialog, TrendPlot enum value, both switch arms, the interface method, and the test assertion are all removed together with no orphaned references.
• ScopeView crash fix is minimal and correct. Checking maxLabelX < plotLeft before calling Math.Clamp properly handles tiny terminals without over-engineering.
• Program.cs try/finally ensures MainWindow is disposed even when Application.Run throws, and moving the config-not-found check before Application.Init() correctly prevents the error from being swallowed in the alternate screen buffer.
• NodeDetailsView stale-response guard is implemented correctly. Capturing nodeId as a local before await and comparing against _currentNodeId inside Application.Invoke (on the UI thread) makes the check race-free.

---

Minor Issues

1. HelpDialog.cs shows as a binary diff

The diff shows Binary files a/App/Dialogs/HelpDialog.cs and b/App/Dialogs/HelpDialog.cs differ. A .cs file showing as binary usually means a line-ending change (CRLF vs LF) or a BOM was added/removed. Harmless in practice but worth confirming .gitattributes is consistent so future diffs stay readable.

2. autoConnectUrl warning still reaches the alternate screen

Program.cs correctly moves the config-not-found error before Application.Init(), but the autoConnectUrl warning is still emitted after Application.Init() inside the try block. That warning will appear in the alternate screen buffer and be lost on exit — the same class of problem the PR fixes for the config-not-found path. Not in scope here, but worth a follow-up ticket.

3. Inconsistent null check in LogView.Dispose

The if (_logger != null) guard around _logger.LogAdded -= OnLogAdded is inconsistent with the other unsubscriptions in the same Dispose method (none of which null-check). If _logger can legitimately be null at disposal time the guard is correct; if not, it's just noise. Either make the field non-nullable or add a short comment explaining the defensive intent.

4. ToggleRecording implementation not visible in diff

The menu now calls ToggleRecording instead of the old OnRecordRequested/OnStopRecordingRequested pair, but the implementation of ToggleRecording is not visible in the diff (presumably it already existed in MainWindow.cs). Worth confirming the method correctly reflects toggled state and keeps parity with Ctrl+R behavior — the old menu had two items that implicitly tracked state via presence; the new single item needs to do that tracking explicitly.

---

Summary

All bug fixes are correct and the dead code removal is thorough. The two actionable follow-ups are the autoConnectUrl alternate-screen warning and confirming the HelpDialog.cs binary diff was an intentional line-ending normalization. Everything else is minor polish.