Streamline codebase: remove dead code and consolidate duplicated logic

[BrettKinny]

• Consolidate duplicated SanitizeUrlForFilename (49 lines duplicated in
  CsvRecordingManager and ConfigurationService) into ConnectionIdentifier,
  reusing its existing protocol-stripping and length-limiting helpers
• Replace SubscriptionManager's 17-branch data type if-chain and
  NodeBrowser's private type dictionary with a shared DataTypeResolver
• Extract duplicated accent-button ColorScheme (6 dialogs) and flat
  menu/status bar ColorScheme (4 sites in MainWindow) into ThemeStyler
  factory methods; remove ThemeStyler's unused Create*/ApplyTo* helpers
• Remove dead code with no production callers:
  • KeybindingContext.TrendPlot (no TrendPlotView exists) and mappings
  • SubscriptionManager.RemoveNodeByNodeIdAsync
  • FocusManager.FocusPrevious/GetPreviousIndex; FocusPane now private
  • RecentFilesManager.CleanupMissingFiles
  • UiThread.RunDelayed
  • TaskExtensions.FireAndForget(Action) overload
• Sync CLAUDE.md project structure with reality: remove nonexistent docs
  files, add missing views/dialogs/test files

Net: -455 lines. All 603 tests pass.

https://claude.ai/code/session_019zPGxdYgE9aVJ7Ai2uxV7y

[claude]

Code Review

PR: Streamline codebase: remove dead code and consolidate duplicated logic
Net change: -455 lines across 31 files | 603 tests passing

---

Overview

This is a focused cleanup PR with five distinct work items: consolidating SanitizeUrlForFilename, extracting DataTypeResolver, centralizing ThemeStyler factories, removing dead code, and wiring up previously-defined-but-unused config settings (SamplingInterval, QueueSize, security mode/policy). All changes are well-scoped and the approach is sound.

---

What's Done Well

DataTypeResolver extraction (OpcUa/DataTypeResolver.cs)
The 17-branch if-chain in SubscriptionManager.ResolveDataType and the duplicate Dictionary<uint, string> in NodeBrowser are replaced cleanly. The comment documenting the OPC UA spec invariant (identifier == BuiltInType enum value) is exactly the right thing to write here.

SanitizeUrlForFilename consolidation (Utilities/ConnectionIdentifier.cs)
Delegating to the existing SanitizeComponent + RemoveProtocolPrefix + LimitLength helpers is the correct approach — those helpers already exist for exactly this purpose. Tests migrated to ConnectionIdentifierTests.cs with full coverage of the moved scenarios.

ThemeStyler factory methods
Replacing 6 identical 5-property ColorScheme blocks with CreateAccentButtonScheme(theme) and 4 more with CreateFlatBarScheme(theme) is a clean win. The rename from ApplyTo(View) → ApplyToFrame(FrameView) tightens the type contract and removes the ambiguity about what views it applied to.

Config settings now actually wired through
The propagation chain ConnectAsync → stored fields → InitializeSubscriptionAsync → SubscriptionManager is clean. The clamping in the SamplingInterval and QueueSize setters is appropriate, and the doc comments on OpcilloscopeConfig are updated to reflect that these settings are now live. The new integration test ConnectAsync_AppliesSubscriptionSettings verifies end-to-end propagation correctly.

---

Issues and Suggestions

1. DataTypeResolver uses integer cast instead of SDK constants (minor risk)

// DataTypeResolver.cs:814
var type = id <= (uint)BuiltInType.ByteString ? (BuiltInType)id : BuiltInType.Variant;

The old code used DataTypeIds.Boolean.Equals(dataTypeNodeId) etc., which is authoritative per the SDK. The new code bets on the OPC UA spec guarantee that namespace-0 numeric identifiers equal BuiltInType enum values. The comment documents this correctly and the spec does guarantee it, but if the SDK ever diverges (e.g. a future BuiltInType entry added at a different offset), this would silently mis-classify. Low risk, but consider asserting or adding a debug-build check.

2. SamplingInterval clamping allows 0 — should this be documented more prominently?

set => _samplingInterval = Math.Max(0, Math.Min(10000, value));

0 means "as fast as the server allows" per OPC UA spec, which can have surprising performance implications on busy servers. This is documented in the XML doc comment and the CLAUDE.md update, but worth a mention in the ConnectAsync param doc too (the signature now accepts samplingInterval with no inline guidance on what 0 means).

3. FocusPane made private — consider whether callers exist outside the test boundary

// App/FocusManager.cs
-    public void FocusPane(int index)
+    private void FocusPane(int index)

The PR is right to make this private since it has no production callers, but GetPreviousIndex was internal (not public) specifically to keep it testable without exposing it publicly. Now that FocusPrevious/GetPreviousIndex are gone, the remaining GetNextIndex is still internal static — which is consistent.

4. Test for SamplingInterval_AppliedToNewMonitoredItems doesn't verify the OPC UA layer

// SubscriptionManagerIntegrationTests.cs:1358
Assert.Equal(1000, subscriptionManager.SamplingInterval);
Assert.Equal(25u, subscriptionManager.QueueSize);

This only asserts the property values, not that the MonitoredItem created on the OPC UA subscription actually had SamplingInterval = 1000 applied. That would require checking node's underlying MonitoredItem.SamplingInterval (the revised value the server negotiated). Not blocking, but the test name implies more than it verifies.

5. ThemeStyler.ApplyToFrame now requires FrameView but callers may have passed other View subtypes

The old ApplyTo(View view) worked on any View. The compiler will catch any existing non-FrameView callers since the signature changed, so if the build passes, this is safe. Just confirming the PR doesn't bypass this.

6. Removed FireAndForget(Action<Exception>) leaves a gap for custom error handling

The removed overload was the only way to pass a custom error callback. The remaining overload logs via the Logger abstraction. Any future code that needs to propagate errors synchronously from a FireAndForget pattern would need Application.Invoke or a different approach. This is acceptable given no current callers, but worth a note in the commit/PR if the team anticipates adding error-surfacing features.

---

Nitpicks

• DataTypeResolver.TryGetBuiltInName has an out-param pattern. Its callers in NodeBrowser ignore the bool return and just check name != null. The two-step is fine, but a string? GetBuiltInName(NodeId) returning null on miss would read more naturally there.
• ConnectionIdentifier.SanitizeUrlForFilename now accepts string? (nullable), which is an improvement over the old string that accepted null via null! in the old tests.

---

Summary

This is a high-quality, conservative cleanup. The behavioral improvements (SamplingInterval/QueueSize propagation, security settings) are bundled appropriately since they're tied to the dead-code removal. All removed code was provably unused, test coverage is maintained or improved, and the net simplification is real. The one area worth a second look before merging is the DataTypeResolver integer-cast assumption — the OPC UA spec guarantees it, but a brief comment linking to the spec section would make future maintainers more confident.

Verdict: Approve with minor suggestions — the codebase is cleaner and the 603-test suite gives confidence nothing regressed.

[claude]

Code Review

Overview

This PR is a clean sweep of dead code and duplicated logic: -455 net lines, all 603 tests passing. It tackles four independent concerns in one shot — URL sanitization consolidation, data type resolver extraction, accent/flat-bar ColorScheme factory methods in ThemeStyler, and dead-API removal. It also activates previously-stubbed QueueSize support throughout the config → ConnectionManager → SubscriptionManager pipeline.

---

What works well

DataTypeResolver extraction (removing the 17-branch if-chain) is the right call. The new dictionary approach is cleaner and correctly covers types 16–25 (XmlElement…DiagnosticInfo) that the old ResolveDataType fell through to BuiltInType.Variant with only dataTypeNodeId.ToString() as the name. The new implementation returns the human-readable name for those types, which is a subtle improvement.

SanitizeUrlForFilename consolidation is well-done. Delegating to the existing RemoveProtocolPrefix + SanitizeComponent + LimitLength helpers in ConnectionIdentifier is exactly right — the logic was already there, just not called. Tests migrated correctly to the new location.

ThemeStyler factory methods (CreateAccentButtonScheme / CreateFlatBarScheme) are the right abstraction level — they return a ColorScheme value rather than mutating a view, making them easy to compose and test without needing live Terminal.Gui instances.

ApplyTo(View) → ApplyToFrame(FrameView): narrowing the parameter type to match actual usage is a good correctness move. The previous signature was misleadingly broad.

Dead code removals are well-justified:

• FocusPane made private — only called internally; the public surface was FocusNext.
• RemoveNodeByNodeIdAsync — no production callers; RemoveNodeAsync(clientHandle) is the correct API.
• CleanupMissingFiles — GetExistingFiles() already filters dynamically; the persist-on-cleanup variant was never wired up.

QueueSize activation is a bonus behavioral fix. The config model had the field for a long time with a "reserved for future use" comment; this finally wires it end-to-end. The new integration test ConnectAsync_AppliesSubscriptionSettings verifies the full flow correctly.

---

Minor issues

Doc/code discrepancy — SamplingInterval range:
OpcilloscopeConfig.cs (and CLAUDE.md) now document SamplingIntervalMs as "Valid range: 0–10000 ms", but SubscriptionManager.SamplingInterval's setter clamps to Math.Max(0, Math.Min(60000, value)). The upper bound is actually 60 000 ms. Worth aligning — either update the property's setter or the doc comment.

Test placement anomaly:
QueueSize_ClampsToValidRange in SubscriptionManagerIntegrationTests.cs doesn't actually do anything async or require a live server — it's a pure property test. It would sit more naturally in SubscriptionManagerTests.cs (unit tests), matching the pattern followed by the existing PublishingInterval and SamplingInterval clamping tests referenced in the comment on line 1289 of the diff. Low priority, but keeps the integration test file focused on server interactions.

Old http:// / https:// prefix handling not carried forward:
The deleted implementations in CsvRecordingManager and ConfigurationService both stripped plain https:// and http:// prefixes. The new SanitizeUrlForFilename delegates to RemoveProtocolPrefix, which (based on the diff) only handles opc.tcp:// and opc.https://. For an OPC UA client this almost certainly doesn't matter in practice, but it's worth a comment on SanitizeUrlForFilename noting the supported prefixes, or a quick confirmation that RemoveProtocolPrefix already covers the full set.

---

Summary

Solid cleanup PR — well-scoped, methodical, and the tests back every change. The only things worth a follow-up are the SamplingInterval range discrepancy between the doc comment (10 000 ms) and the actual clamp (60 000 ms), and the test placement for the QueueSize clamping test.

Review by Claude · Model: claude-sonnet-4-6