From c3bb5d575409b6af52593fdb46af55b073f6e32f Mon Sep 17 00:00:00 2001 From: Oleg <18466855+EvaTheSalmon@users.noreply.github.com> Date: Mon, 27 Jul 2026 17:13:30 +0300 Subject: [PATCH] feat(ui): route command exceptions through report+log pipe Command-exception handling was eight copies of the same ThrownExceptions -> ObserveOn -> panel.ReportError block, and none of them logged: the user saw the message but the exception type and stack never reached the log, leaving field-reported errors undiagnosable. Extract one ReportThrownExceptions(command, panel, logger, context) extension that both reports to the message panel and logs the exception with its stack via the caller's ILogger, so the log line carries the originating class through {SourceContext}. Route all eight sites through it and drop the redundant ObserveOn, since the panel self-marshals. Close the failure paths the review surfaced along the way: guard the three bare MainWindowViewModel subscriptions so a throw in the callback body is reported instead of escaping to the dispatcher, and wire GridStyleEditorViewModel.SaveCommand faults to the editor's own ErrorMessage plus the log. That command had no ThrownExceptions handler and crashed the process on an unexpected save error. No IMessageSink interface: MessagePanelViewModel has one implementation, is not mocked, and crosses no layer boundary, so consumers keep the concrete panel. No happy-path behaviour change; on failure the same panel message appears and a stack-carrying log line is added. --- Docs/architecture/error-reporting.md | 24 ++- Docs/architecture/headless-ui-testing.md | 4 +- .../Configuration/GridStyleEditorFacade.cs | 2 +- .../Configuration/IGridStyleEditorFacade.cs | 16 ++ .../ClipboardViewModelCanExecuteTests.cs | 8 +- .../SemiStep.Tests/UI/Helpers/UIFixture.cs | 16 +- .../MainWindowViewModelReportingTests.cs | 73 +++++++++ .../UI/MessagePanelReportingTests.cs | 23 ++- ...ReactiveCommandReportingExtensionsTests.cs | 88 +++++++++++ .../RecipeFileViewModelCanExecuteTests.cs | 7 +- .../RecipeFileViewModelSaveResultTests.cs | 9 +- .../GridStyleEditorViewModelTests.cs | 90 ++++++++++- .../GridStyleEditorWindowOwnerRoutingTests.cs | 8 +- .../StyleEditor/GridStyleEditorWindowTests.cs | 14 +- .../Clipboard/ClipboardViewModel.cs | 19 ++- .../MainWindow/MainWindowViewModel.cs | 40 +++-- .../MessageService/ExceptionReporter.cs | 14 ++ .../ReactiveCommandReportingExtensions.cs | 24 +++ .../RecipeFile/RecipeFileViewModel.cs | 19 ++- .../StyleEditor/GridStyleEditorViewModel.cs | 32 +++- SemiStep/SemiStep.UI/UiDi.cs | 2 +- .../20260727-pr-f1-report-log-extension.md | 148 ++++++++++++++++++ 22 files changed, 619 insertions(+), 61 deletions(-) create mode 100644 SemiStep/SemiStep.Core/Configuration/IGridStyleEditorFacade.cs create mode 100644 SemiStep/SemiStep.Tests/UI/MainWindow/MainWindowViewModelReportingTests.cs create mode 100644 SemiStep/SemiStep.Tests/UI/MessageService/ReactiveCommandReportingExtensionsTests.cs create mode 100644 SemiStep/SemiStep.UI/MessageService/ExceptionReporter.cs create mode 100644 SemiStep/SemiStep.UI/MessageService/ReactiveCommandReportingExtensions.cs create mode 100644 docs/plans/completed/20260727-pr-f1-report-log-extension.md diff --git a/Docs/architecture/error-reporting.md b/Docs/architecture/error-reporting.md index 252af7d..c74ec24 100644 --- a/Docs/architecture/error-reporting.md +++ b/Docs/architecture/error-reporting.md @@ -46,8 +46,28 @@ read `Errors[0]` directly. discrete one-shot channel. `RecipeCoordinator` bridges each fault to the operation channel via `ReportError(error.Message)` — the message text is owned in Core. The persistent "disconnected" label stays in the status bar. -- **Exception handlers** (`ReportError($"... {ex.Message}")` on `ThrownExceptions`) are not - `Result`-based and stay as-is. +- **Command exceptions** (a `ReactiveCommand` fault on `ThrownExceptions`) are not `Result`-based; + they route through `ReportThrownExceptions` (see below) rather than a hand-written handler. - **One deliberate exception** to the `"; "` idiom: the clipboard *paste* failure lists each error on its own line (`Environment.NewLine`), because a rejected paste can carry many per-step errors that read better stacked. It still surfaces every error (never just `Errors[0]`). + +## Command-exception pipe (report + log) + +A command whose `Execute` throws (file I/O, PLC calls) faults on `ReactiveCommand.ThrownExceptions`. +These faults route through one extension: + +- `IDisposable ReportThrownExceptions(this ReactiveCommand, MessagePanelViewModel panel, ILogger logger, string context)` + (`SemiStep.UI/MessageService/ReactiveCommandReportingExtensions.cs`). Per thrown exception it both + `logger.LogError(ex, "{Context} failed", context)` **and** `panel.ReportError($"{context}: {ex.Message}")`. + The panel keeps the user-facing message it always showed; the log now carries the exception type and + full stack that the message drops. + +The `logger` argument is the caller's own `ILogger`, so the Serilog `{SourceContext}` field names +the originating view model in every logged fault. The extension takes the concrete +`MessagePanelViewModel`, not an `IMessageSink` abstraction — one panel implementation exists, so the +seam stays concrete (matches `ResultReportingExtensions`). + +Modal dialogs are the exception: `GridStyleEditorViewModel.SaveCommand` surfaces its fault on the +editor's own `ErrorMessage` property (and logs), not the shared panel, because a modal owns its error +surface while it is open. diff --git a/Docs/architecture/headless-ui-testing.md b/Docs/architecture/headless-ui-testing.md index a8e4ada..7a23381 100644 --- a/Docs/architecture/headless-ui-testing.md +++ b/Docs/architecture/headless-ui-testing.md @@ -17,8 +17,8 @@ stand-in. See `SemiStep.Tests/UI/MainWindow/MainWindowExitFlowTests.cs` for a wo `RunJobs` remains necessary only to pump work the test never awaits: - fire-and-forget `async void` continuations (e.g. a `Closing` event handler that awaits a dialog); -- `ObserveOn(RxApp.MainThreadScheduler)` deliveries, such as `ThrownExceptions` reports posted to - the message panel. +- `ObserveOn(RxApp.MainThreadScheduler)` deliveries, such as the `MainWindowViewModel` sync-time + `Observable.Interval` tick that reposts `LastSyncTimeText` on the UI scheduler. Call `Dispatcher.UIThread.RunJobs()` after triggering such work and before asserting on its effect. diff --git a/SemiStep/SemiStep.Core/Configuration/GridStyleEditorFacade.cs b/SemiStep/SemiStep.Core/Configuration/GridStyleEditorFacade.cs index 2c24c75..062e734 100644 --- a/SemiStep/SemiStep.Core/Configuration/GridStyleEditorFacade.cs +++ b/SemiStep/SemiStep.Core/Configuration/GridStyleEditorFacade.cs @@ -12,7 +12,7 @@ namespace SemiStep.Core.Configuration; /// row height, spacing, panel height) is the caller's responsibility — the editor view model enforces /// those bounds before invoking . /// -public sealed class GridStyleEditorFacade +public sealed class GridStyleEditorFacade : IGridStyleEditorFacade { private readonly GridStyleWriter _gridStyleWriter = new(); diff --git a/SemiStep/SemiStep.Core/Configuration/IGridStyleEditorFacade.cs b/SemiStep/SemiStep.Core/Configuration/IGridStyleEditorFacade.cs new file mode 100644 index 0000000..6c6abe7 --- /dev/null +++ b/SemiStep/SemiStep.Core/Configuration/IGridStyleEditorFacade.cs @@ -0,0 +1,16 @@ +using FluentResults; + +namespace SemiStep.Core.Configuration; + +/// +/// The style-editor seam consumed by the UI editor view model. Declared as an interface so the +/// view model depends on an abstraction it can mock, rather than the concrete Core facade. +/// +public interface IGridStyleEditorFacade +{ + Task> Load(string configDir); + + Result Validate(GridStyleOptions options); + + Result Save(string configDir, GridStyleOptions options); +} diff --git a/SemiStep/SemiStep.Tests/UI/Clipboard/ClipboardViewModelCanExecuteTests.cs b/SemiStep/SemiStep.Tests/UI/Clipboard/ClipboardViewModelCanExecuteTests.cs index d5d5cbb..30c0dfd 100644 --- a/SemiStep/SemiStep.Tests/UI/Clipboard/ClipboardViewModelCanExecuteTests.cs +++ b/SemiStep/SemiStep.Tests/UI/Clipboard/ClipboardViewModelCanExecuteTests.cs @@ -4,6 +4,8 @@ using FluentAssertions; +using Microsoft.Extensions.Logging.Abstractions; + using SemiStep.Core.Recipes.Clipboard; using SemiStep.Core.Recipes.Helpers; using SemiStep.Tests.Core.Helpers; @@ -39,7 +41,8 @@ public async ValueTask InitializeAsync() _surface, clipboardSerializer, importedRecipeValidator, - _fixture.MessagePanel); + _fixture.MessagePanel, + NullLogger.Instance); } public async ValueTask DisposeAsync() @@ -113,7 +116,8 @@ public void Cut_CanExecuteTrue_WhenSelectionExistsBeforeConstruction() _surface, new ClipboardSerializer(_fixture.RecipeMetadataRegistry), new ImportedRecipeValidator(_fixture.RecipeMetadataRegistry), - _fixture.MessagePanel); + _fixture.MessagePanel, + NullLogger.Instance); ((ICommand)clipboard.CutStepCommand).CanExecute(null).Should().BeTrue(); } diff --git a/SemiStep/SemiStep.Tests/UI/Helpers/UIFixture.cs b/SemiStep/SemiStep.Tests/UI/Helpers/UIFixture.cs index 5643ce2..b06df92 100644 --- a/SemiStep/SemiStep.Tests/UI/Helpers/UIFixture.cs +++ b/SemiStep/SemiStep.Tests/UI/Helpers/UIFixture.cs @@ -140,7 +140,8 @@ public void SeedRecipe(int stepCount) } public MainWindowViewModel CreateMainWindowViewModel( - Func? styleEditorFactory = null) + Func? styleEditorFactory = null, + ILogger? logger = null) { var grid = CreateActiveSurface(); @@ -153,9 +154,13 @@ public MainWindowViewModel CreateMainWindowViewModel( grid, clipboardSerializer, importedRecipeValidator, - MessagePanel); + MessagePanel, + NullLogger.Instance); - var recipeFile = new RecipeFileViewModel(Coordinator, MessagePanel); + var recipeFile = new RecipeFileViewModel( + Coordinator, + MessagePanel, + NullLogger.Instance); var plcMonitor = new PlcMonitorViewModel( Coordinator, @@ -166,7 +171,8 @@ public MainWindowViewModel CreateMainWindowViewModel( () => new GridStyleEditorViewModel( new GridStyleEditorFacade(), @"C:\does-not-exist", - AppConfiguration.GridStyle)); + AppConfiguration.GridStyle, + NullLogger.Instance)); return new MainWindowViewModel( Coordinator, @@ -177,7 +183,7 @@ public MainWindowViewModel CreateMainWindowViewModel( MessagePanel, plcMonitor, gridStyleEditorViewModelFactory, - NullLogger.Instance); + logger ?? NullLogger.Instance); } public void SetSyncEnabled(bool isSyncEnabled) diff --git a/SemiStep/SemiStep.Tests/UI/MainWindow/MainWindowViewModelReportingTests.cs b/SemiStep/SemiStep.Tests/UI/MainWindow/MainWindowViewModelReportingTests.cs new file mode 100644 index 0000000..45f2def --- /dev/null +++ b/SemiStep/SemiStep.Tests/UI/MainWindow/MainWindowViewModelReportingTests.cs @@ -0,0 +1,73 @@ +using System; + +using Avalonia.Headless.XUnit; + +using FluentAssertions; + +using Microsoft.Extensions.Logging; + +using SemiStep.Tests.Helpers; +using SemiStep.Tests.UI.Helpers; +using SemiStep.UI.MainWindow; +using SemiStep.UI.MessageService; + +using Xunit; + +namespace SemiStep.Tests.UI.MainWindow; + +[Trait("Component", "UI")] +[Trait("Area", "MessageReporting")] +[Trait("Category", "Unit")] +public sealed class MainWindowViewModelReportingTests : IAsyncLifetime +{ + private readonly UIFixture _fixture = new(); + private readonly RecordingLogger _logger = new(); + private MainWindowViewModel _viewModel = null!; + + public async ValueTask InitializeAsync() + { + await _fixture.InitializeAsync(); + _viewModel = _fixture.CreateMainWindowViewModel(logger: _logger); + } + + public ValueTask DisposeAsync() + { + _viewModel.Dispose(); + return _fixture.DisposeAsync(); + } + + [AvaloniaTheory] + [InlineData("PLC state update")] + [InlineData("PLC conflict handling")] + [InlineData("Sync time refresh")] + public void OnSubscriptionError_ReportsToPanelAndLogsException(string context) + { + var failure = new InvalidOperationException("boom"); + + _viewModel.OnSubscriptionError(context)(failure); + + _fixture.MessagePanel.Entries.Should().ContainSingle( + entry => entry.Severity == MessageSeverity.Error && entry.Message == $"{context}: boom"); + + var logged = _logger.Entries.Should().ContainSingle().Subject; + logged.Level.Should().Be(LogLevel.Error); + logged.Exception.Should().BeSameAs(failure); + } + + [AvaloniaFact] + public void Guarded_ThrowInOnNextBody_ReportsToPanelAndLogs_WithoutEscaping() + { + var failure = new InvalidOperationException("boom"); + + var act = () => _viewModel.Guarded("Sync time refresh", () => throw failure); + + act.Should().NotThrow("a throw in the onNext body must be contained, not propagated to the pipeline"); + + _fixture.MessagePanel.Entries.Should().ContainSingle( + entry => entry.Severity == MessageSeverity.Error && entry.Message == "Sync time refresh: boom"); + + var logged = _logger.Entries.Should().ContainSingle().Subject; + logged.Level.Should().Be(LogLevel.Error); + logged.Exception.Should().BeSameAs(failure); + } +} diff --git a/SemiStep/SemiStep.Tests/UI/MessagePanelReportingTests.cs b/SemiStep/SemiStep.Tests/UI/MessagePanelReportingTests.cs index fdfbe5d..6f5df01 100644 --- a/SemiStep/SemiStep.Tests/UI/MessagePanelReportingTests.cs +++ b/SemiStep/SemiStep.Tests/UI/MessagePanelReportingTests.cs @@ -7,6 +7,8 @@ using FluentAssertions; +using Microsoft.Extensions.Logging.Abstractions; + using ReactiveUI; using SemiStep.Core.Recipes.Clipboard; @@ -47,7 +49,10 @@ public async ValueTask DisposeAsync() [AvaloniaFact] public async Task RecipeFile_Save_ReportsSuccess() { - var recipeFile = new RecipeFileViewModel(_fixture.Coordinator, _fixture.MessagePanel); + var recipeFile = new RecipeFileViewModel( + _fixture.Coordinator, + _fixture.MessagePanel, + NullLogger.Instance); var filePath = Path.Combine(Path.GetTempPath(), $"semistep-save-{Guid.NewGuid():N}.csv"); recipeFile.SaveFileInteraction.RegisterHandler(context => context.SetOutput(filePath)); @@ -69,7 +74,10 @@ public async Task RecipeFile_Save_ReportsSuccess() [AvaloniaFact] public async Task RecipeFile_Load_ReportsSuccess() { - var recipeFile = new RecipeFileViewModel(_fixture.Coordinator, _fixture.MessagePanel); + var recipeFile = new RecipeFileViewModel( + _fixture.Coordinator, + _fixture.MessagePanel, + NullLogger.Instance); var filePath = Path.Combine(Path.GetTempPath(), $"semistep-load-{Guid.NewGuid():N}.csv"); recipeFile.SaveFileInteraction.RegisterHandler(context => context.SetOutput(filePath)); recipeFile.OpenFileInteraction.RegisterHandler(context => context.SetOutput(filePath)); @@ -94,7 +102,10 @@ public async Task RecipeFile_Load_ReportsSuccess() [AvaloniaFact] public async Task RecipeFile_LoadMissingFile_ReportsError() { - var recipeFile = new RecipeFileViewModel(_fixture.Coordinator, _fixture.MessagePanel); + var recipeFile = new RecipeFileViewModel( + _fixture.Coordinator, + _fixture.MessagePanel, + NullLogger.Instance); var missingPath = Path.Combine(Path.GetTempPath(), $"semistep-missing-{Guid.NewGuid():N}.csv"); recipeFile.OpenFileInteraction.RegisterHandler(context => context.SetOutput(missingPath)); @@ -149,7 +160,8 @@ public async Task Clipboard_PasteInvalidContent_ReportsError() _surface, clipboardSerializer, importedRecipeValidator, - _fixture.MessagePanel); + _fixture.MessagePanel, + NullLogger.Instance); var window = new Window(); window.Show(); @@ -187,7 +199,8 @@ public async Task Clipboard_PasteValidContent_LeavesNoOperationError() _surface, clipboardSerializer, importedRecipeValidator, - _fixture.MessagePanel); + _fixture.MessagePanel, + NullLogger.Instance); var window = new Window(); window.Show(); diff --git a/SemiStep/SemiStep.Tests/UI/MessageService/ReactiveCommandReportingExtensionsTests.cs b/SemiStep/SemiStep.Tests/UI/MessageService/ReactiveCommandReportingExtensionsTests.cs new file mode 100644 index 0000000..9160181 --- /dev/null +++ b/SemiStep/SemiStep.Tests/UI/MessageService/ReactiveCommandReportingExtensionsTests.cs @@ -0,0 +1,88 @@ +using System; +using System.Reactive; +using System.Reactive.Linq; + +using Avalonia.Headless.XUnit; + +using FluentAssertions; + +using Microsoft.Extensions.Logging; + +using ReactiveUI; + +using SemiStep.Tests.Helpers; +using SemiStep.UI.MessageService; + +using Xunit; + +namespace SemiStep.Tests.UI.MessageService; + +[Trait("Component", "UI")] +[Trait("Area", "MessageReporting")] +[Trait("Category", "Unit")] +public sealed class ReactiveCommandReportingExtensionsTests +{ + [AvaloniaFact] + public async Task ReportThrownExceptions_ThrowingCommand_ReportsSinglePanelErrorAndLogsException() + { + var panel = new MessagePanelViewModel(); + var logger = new RecordingLogger(); + var failure = new InvalidOperationException("boom"); + var command = ReactiveCommand.Create(_ => throw failure); + + using var subscription = command.ReportThrownExceptions(panel, logger, "Copy failed"); + + try + { + await ExecuteSwallowing(command); + + var entry = panel.Entries.Should().ContainSingle().Subject; + entry.Severity.Should().Be(MessageSeverity.Error); + entry.Message.Should().Be("Copy failed: boom"); + + var logged = logger.Entries.Should().ContainSingle().Subject; + logged.Level.Should().Be(LogLevel.Error); + logged.Exception.Should().BeSameAs(failure); + } + finally + { + command.Dispose(); + panel.Dispose(); + } + } + + [AvaloniaFact] + public async Task ReportThrownExceptions_NonThrowingCommand_ProducesNoPanelEntryAndNoLog() + { + var panel = new MessagePanelViewModel(); + var logger = new RecordingLogger(); + var command = ReactiveCommand.Create(_ => Unit.Default); + + using var subscription = command.ReportThrownExceptions(panel, logger, "Copy failed"); + + try + { + await command.Execute(); + + panel.Entries.Should().BeEmpty(); + logger.Entries.Should().BeEmpty(); + } + finally + { + command.Dispose(); + panel.Dispose(); + } + } + + private static async Task ExecuteSwallowing(ReactiveCommand command) + { + try + { + await command.Execute(); + } + catch (InvalidOperationException) + { + // The command routes the throw to ThrownExceptions; Execute also rethrows to the awaiter. + } + } +} diff --git a/SemiStep/SemiStep.Tests/UI/RecipeFile/RecipeFileViewModelCanExecuteTests.cs b/SemiStep/SemiStep.Tests/UI/RecipeFile/RecipeFileViewModelCanExecuteTests.cs index 8e982b5..f5748b8 100644 --- a/SemiStep/SemiStep.Tests/UI/RecipeFile/RecipeFileViewModelCanExecuteTests.cs +++ b/SemiStep/SemiStep.Tests/UI/RecipeFile/RecipeFileViewModelCanExecuteTests.cs @@ -4,6 +4,8 @@ using FluentAssertions; +using Microsoft.Extensions.Logging.Abstractions; + using SemiStep.Tests.UI.Helpers; using SemiStep.UI.RecipeFile; @@ -23,7 +25,10 @@ public sealed class RecipeFileViewModelCanExecuteTests : IAsyncLifetime public async ValueTask InitializeAsync() { await _fixture.InitializeAsync(); - _recipeFile = new RecipeFileViewModel(_fixture.Coordinator, _fixture.MessagePanel); + _recipeFile = new RecipeFileViewModel( + _fixture.Coordinator, + _fixture.MessagePanel, + NullLogger.Instance); } public async ValueTask DisposeAsync() diff --git a/SemiStep/SemiStep.Tests/UI/RecipeFile/RecipeFileViewModelSaveResultTests.cs b/SemiStep/SemiStep.Tests/UI/RecipeFile/RecipeFileViewModelSaveResultTests.cs index a21ddc4..81f900a 100644 --- a/SemiStep/SemiStep.Tests/UI/RecipeFile/RecipeFileViewModelSaveResultTests.cs +++ b/SemiStep/SemiStep.Tests/UI/RecipeFile/RecipeFileViewModelSaveResultTests.cs @@ -5,6 +5,8 @@ using FluentAssertions; +using Microsoft.Extensions.Logging.Abstractions; + using SemiStep.Tests.Core.Helpers; using SemiStep.Tests.UI.Helpers; @@ -28,7 +30,10 @@ public sealed class RecipeFileViewModelSaveResultTests : IAsyncLifetime public async ValueTask InitializeAsync() { await _fixture.InitializeAsync(); - _recipeFile = new RecipeFileViewModel(_fixture.Coordinator, _fixture.MessagePanel); + _recipeFile = new RecipeFileViewModel( + _fixture.Coordinator, + _fixture.MessagePanel, + NullLogger.Instance); } public async ValueTask DisposeAsync() @@ -166,7 +171,7 @@ public async Task SaveAsRecipe_ThrowingPicker_ReportsSaveAsFailed() var saveAs = async () => await _recipeFile.SaveAsRecipeCommand.Execute(); await saveAs.Should().ThrowAsync(); - // The ThrownExceptions report is posted to the dispatcher via ObserveOn. + // The report reaches the panel via MessagePanel's own UI-thread marshalling; drain any queued work. Dispatcher.UIThread.RunJobs(); var errorEntry = _fixture.MessagePanel.Entries .Should().ContainSingle(e => e.Severity == MessageSeverity.Error).Subject; diff --git a/SemiStep/SemiStep.Tests/UI/StyleEditor/GridStyleEditorViewModelTests.cs b/SemiStep/SemiStep.Tests/UI/StyleEditor/GridStyleEditorViewModelTests.cs index b0c5422..d634d3e 100644 --- a/SemiStep/SemiStep.Tests/UI/StyleEditor/GridStyleEditorViewModelTests.cs +++ b/SemiStep/SemiStep.Tests/UI/StyleEditor/GridStyleEditorViewModelTests.cs @@ -1,9 +1,21 @@ -using Avalonia.Headless.XUnit; +using System; +using System.Reactive; +using System.Reactive.Linq; + +using Avalonia.Headless.XUnit; using Avalonia.Media; using FluentAssertions; +using FluentResults; + +using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Logging.Abstractions; + +using ReactiveUI; + using SemiStep.Core.Configuration; +using SemiStep.Tests.Helpers; using SemiStep.UI.Localization; using SemiStep.UI.StyleEditor; @@ -247,8 +259,82 @@ public async Task LoadAsync_MissingConfigDir_SetsErrorMessageAndDoesNotReseed() viewModel.CellFontSize.Should().Be(18); } + [AvaloniaFact] + public async Task SaveCommand_WhenSaveThrows_SurfacesErrorMessageAndLogs_WithoutCrashing() + { + var logger = new RecordingLogger(); + var failure = new InvalidOperationException("disk gone"); + var viewModel = new GridStyleEditorViewModel( + new ThrowingFacade(failure), + ConfigDir, + GridStyleOptions.Default, + logger); + + viewModel.CanSave.Should().BeTrue("the seeded default draft is valid, so SaveCommand can execute"); + + await ExecuteSwallowing(viewModel.SaveCommand); + + viewModel.ErrorMessage.Should().Be("Save failed: disk gone"); + var logged = logger.Entries.Should().ContainSingle().Subject; + logged.Level.Should().Be(LogLevel.Error); + logged.Exception.Should().BeSameAs(failure); + } + + [AvaloniaFact] + public void ReportSaveException_SurfacesOnEditorErrorMessage_AndLogsWithException() + { + var logger = new RecordingLogger(); + var viewModel = new GridStyleEditorViewModel( + new GridStyleEditorFacade(), + ConfigDir, + GridStyleOptions.Default, + logger); + var failure = new InvalidOperationException("boom"); + + viewModel.ReportSaveException(failure); + + viewModel.ErrorMessage.Should().Be("Save failed: boom"); + var logged = logger.Entries.Should().ContainSingle().Subject; + logged.Level.Should().Be(LogLevel.Error); + logged.Exception.Should().BeSameAs(failure); + } + private static GridStyleEditorViewModel CreateViewModel(GridStyleOptions source) { - return new GridStyleEditorViewModel(new GridStyleEditorFacade(), ConfigDir, source); + return new GridStyleEditorViewModel( + new GridStyleEditorFacade(), + ConfigDir, + source, + NullLogger.Instance); + } + + private static async Task ExecuteSwallowing(ReactiveCommand command) + { + try + { + await command.Execute(); + } + catch (InvalidOperationException) + { + // The command routes the throw to ThrownExceptions; Execute also rethrows to the awaiter. + } + } + + private sealed class ThrowingFacade(Exception failure) : IGridStyleEditorFacade + { + public Task> Load(string configDir) + { + return Task.FromResult(Result.Ok(GridStyleOptions.Default)); + } + + public Result Validate(GridStyleOptions options) + { + return Result.Ok(); + } + + public Result Save(string configDir, GridStyleOptions options) + { + throw failure; + } } } diff --git a/SemiStep/SemiStep.Tests/UI/StyleEditor/GridStyleEditorWindowOwnerRoutingTests.cs b/SemiStep/SemiStep.Tests/UI/StyleEditor/GridStyleEditorWindowOwnerRoutingTests.cs index 8da0d70..2bdf4aa 100644 --- a/SemiStep/SemiStep.Tests/UI/StyleEditor/GridStyleEditorWindowOwnerRoutingTests.cs +++ b/SemiStep/SemiStep.Tests/UI/StyleEditor/GridStyleEditorWindowOwnerRoutingTests.cs @@ -5,6 +5,8 @@ using FluentAssertions; +using Microsoft.Extensions.Logging.Abstractions; + using SemiStep.Core.Configuration; using SemiStep.Tests.Config.Helpers; @@ -115,7 +117,11 @@ public async Task SaveThenExitNow_DrivesRealGlue_CascadesToOwnerClose() using var tempDir = CopyShippedConfig("MBE"); var facade = new GridStyleEditorFacade(); var loaded = (await facade.Load(tempDir.Path)).Value; - var viewModel = new GridStyleEditorViewModel(facade, tempDir.Path, loaded); + var viewModel = new GridStyleEditorViewModel( + facade, + tempDir.Path, + loaded, + NullLogger.Instance); var editor = new GridStyleEditorWindow { ViewModel = viewModel }; // Modal show establishes the Owner relationship and fires the WhenActivated glue that diff --git a/SemiStep/SemiStep.Tests/UI/StyleEditor/GridStyleEditorWindowTests.cs b/SemiStep/SemiStep.Tests/UI/StyleEditor/GridStyleEditorWindowTests.cs index 89e9758..2fe116d 100644 --- a/SemiStep/SemiStep.Tests/UI/StyleEditor/GridStyleEditorWindowTests.cs +++ b/SemiStep/SemiStep.Tests/UI/StyleEditor/GridStyleEditorWindowTests.cs @@ -5,6 +5,8 @@ using FluentAssertions; +using Microsoft.Extensions.Logging.Abstractions; + using SemiStep.Core.Configuration; using SemiStep.Tests.Config.Helpers; using SemiStep.Tests.Helpers; @@ -28,7 +30,11 @@ public async Task Save_AfterEditingColorAndNumeric_PersistsToYaml_AndReloads() var configDir = tempDir.Path; var loaded = (await facade.Load(configDir)).Value; - var viewModel = new GridStyleEditorViewModel(facade, configDir, loaded); + var viewModel = new GridStyleEditorViewModel( + facade, + configDir, + loaded, + NullLogger.Instance); viewModel.SelectionBackground = Color.Parse("#123456"); viewModel.CellFontSize = loaded.CellFontSize + 1; @@ -56,7 +62,11 @@ public async Task NoEditThenDiscard_LeavesFileByteIdentical() var loaded = (await facade.Load(configDir)).Value; // Construct and discard the VM without invoking Save (mirrors the Cancel path). - _ = new GridStyleEditorViewModel(facade, configDir, loaded); + _ = new GridStyleEditorViewModel( + facade, + configDir, + loaded, + NullLogger.Instance); var after = await File.ReadAllBytesAsync(filePath, TestContext.Current.CancellationToken); after.Should().Equal(before); diff --git a/SemiStep/SemiStep.UI/Clipboard/ClipboardViewModel.cs b/SemiStep/SemiStep.UI/Clipboard/ClipboardViewModel.cs index b41bc2e..e271d4e 100644 --- a/SemiStep/SemiStep.UI/Clipboard/ClipboardViewModel.cs +++ b/SemiStep/SemiStep.UI/Clipboard/ClipboardViewModel.cs @@ -8,6 +8,8 @@ using FluentResults; +using Microsoft.Extensions.Logging; + using ReactiveUI; using SemiStep.Core.Recipes; @@ -27,6 +29,7 @@ public class ClipboardViewModel : ReactiveObject, IDisposable private readonly CompositeDisposable _disposables = new(); private readonly ImportedRecipeValidator _importedRecipeValidator; + private readonly ILogger _logger; private readonly MessagePanelViewModel _messagePanel; private readonly IRecipeGridSurface _recipeGrid; private IClipboard? _clipboard; @@ -36,13 +39,15 @@ public ClipboardViewModel( IRecipeGridSurface recipeGrid, ClipboardSerializer clipboardSerializer, ImportedRecipeValidator importedRecipeValidator, - MessagePanelViewModel messagePanel) + MessagePanelViewModel messagePanel, + ILogger logger) { _coordinator = coordinator; _recipeGrid = recipeGrid; _clipboardSerializer = clipboardSerializer; _importedRecipeValidator = importedRecipeValidator; _messagePanel = messagePanel; + _logger = logger; var canCopyOrCut = _recipeGrid.CanDeleteStep; var canEdit = _coordinator.CanEditRecipe; @@ -53,19 +58,13 @@ public ClipboardViewModel( canEdit.CombineLatest(canCopyOrCut, (left, right) => left && right)); PasteStepCommand = ReactiveCommand.CreateFromTask(PasteStepsAsync, canEdit); - CopyStepCommand.ThrownExceptions - .ObserveOn(RxSchedulers.MainThreadScheduler) - .Subscribe(ex => _messagePanel.ReportError($"Copy failed: {ex.Message}")) + CopyStepCommand.ReportThrownExceptions(_messagePanel, _logger, "Copy failed") .DisposeWith(_disposables); - CutStepCommand.ThrownExceptions - .ObserveOn(RxSchedulers.MainThreadScheduler) - .Subscribe(ex => _messagePanel.ReportError($"Cut failed: {ex.Message}")) + CutStepCommand.ReportThrownExceptions(_messagePanel, _logger, "Cut failed") .DisposeWith(_disposables); - PasteStepCommand.ThrownExceptions - .ObserveOn(RxSchedulers.MainThreadScheduler) - .Subscribe(ex => _messagePanel.ReportError($"Paste failed: {ex.Message}")) + PasteStepCommand.ReportThrownExceptions(_messagePanel, _logger, "Paste failed") .DisposeWith(_disposables); } diff --git a/SemiStep/SemiStep.UI/MainWindow/MainWindowViewModel.cs b/SemiStep/SemiStep.UI/MainWindow/MainWindowViewModel.cs index 0ab3008..ad23a6b 100644 --- a/SemiStep/SemiStep.UI/MainWindow/MainWindowViewModel.cs +++ b/SemiStep/SemiStep.UI/MainWindow/MainWindowViewModel.cs @@ -68,30 +68,32 @@ public MainWindowViewModel( .ToProperty(this, x => x.IsTransposedOrientation) .DisposeWith(_disposables); - ToggleSyncCommand.ThrownExceptions - .ObserveOn(RxSchedulers.MainThreadScheduler) - .Subscribe(ex => messagePanel.ReportError($"Sync toggle failed: {ex.Message}")) + ToggleSyncCommand.ReportThrownExceptions(MessagePanel, _logger, "Sync toggle failed") .DisposeWith(_disposables); - OpenStyleEditorCommand.ThrownExceptions - .ObserveOn(RxSchedulers.MainThreadScheduler) - .Subscribe(ex => messagePanel.ReportError($"Style editor failed: {ex.Message}")) + OpenStyleEditorCommand.ReportThrownExceptions(MessagePanel, _logger, "Style editor failed") .DisposeWith(_disposables); _coordinator.Mutated += OnCoordinatorMutated; _disposables.Add(Disposable.Create(() => _coordinator.Mutated -= OnCoordinatorMutated)); _coordinator.PlcStateChanged - .Subscribe(_ => RaiseConnectionStateProperties()) + .Subscribe( + _ => Guarded("PLC state update", RaiseConnectionStateProperties), + OnSubscriptionError("PLC state update")) .DisposeWith(_disposables); _coordinator.PlcRecipeConflictDetected - .Subscribe(conflict => _ = HandleConflictAsync(conflict.Local, conflict.Plc)) + .Subscribe( + conflict => Guarded("PLC conflict handling", () => _ = HandleConflictAsync(conflict.Local, conflict.Plc)), + OnSubscriptionError("PLC conflict handling")) .DisposeWith(_disposables); Observable.Interval(TimeSpan.FromSeconds(1)) .ObserveOn(RxSchedulers.MainThreadScheduler) - .Subscribe(_ => this.RaisePropertyChanged(nameof(LastSyncTimeText))) + .Subscribe( + _ => Guarded("Sync time refresh", () => this.RaisePropertyChanged(nameof(LastSyncTimeText))), + OnSubscriptionError("Sync time refresh")) .DisposeWith(_disposables); } @@ -249,6 +251,26 @@ private void OnCoordinatorMutated(MutationSignal signal) RaiseAllStateProperties(); } + internal Action OnSubscriptionError(string context) + { + return ex => ExceptionReporter.ReportAndLog(MessagePanel, _logger, context, ex); + } + + // A throw inside a subscription's onNext body is NOT routed to onError: Rx disposes the + // subscription and rethrows up the pipeline (fatal for a dispatcher-scheduled tick). Wrapping + // the body here contains it on the same report + log path as a source-observable error. + internal void Guarded(string context, Action body) + { + try + { + body(); + } + catch (Exception ex) + { + ExceptionReporter.ReportAndLog(MessagePanel, _logger, context, ex); + } + } + private void RaiseAllStateProperties() { this.RaisePropertyChanged(nameof(WindowTitle)); diff --git a/SemiStep/SemiStep.UI/MessageService/ExceptionReporter.cs b/SemiStep/SemiStep.UI/MessageService/ExceptionReporter.cs new file mode 100644 index 0000000..9c3aad7 --- /dev/null +++ b/SemiStep/SemiStep.UI/MessageService/ExceptionReporter.cs @@ -0,0 +1,14 @@ +using System; + +using Microsoft.Extensions.Logging; + +namespace SemiStep.UI.MessageService; + +internal static class ExceptionReporter +{ + public static void ReportAndLog(MessagePanelViewModel panel, ILogger logger, string context, Exception exception) + { + logger.LogError(exception, "{Context}", context); + panel.ReportError($"{context}: {exception.Message}"); + } +} diff --git a/SemiStep/SemiStep.UI/MessageService/ReactiveCommandReportingExtensions.cs b/SemiStep/SemiStep.UI/MessageService/ReactiveCommandReportingExtensions.cs new file mode 100644 index 0000000..379374f --- /dev/null +++ b/SemiStep/SemiStep.UI/MessageService/ReactiveCommandReportingExtensions.cs @@ -0,0 +1,24 @@ +using System; + +using Microsoft.Extensions.Logging; + +using ReactiveUI; + +namespace SemiStep.UI.MessageService; + +public static class ReactiveCommandReportingExtensions +{ + public static IDisposable ReportThrownExceptions( + this ReactiveCommand command, + MessagePanelViewModel panel, + ILogger logger, + string context) + { + ArgumentNullException.ThrowIfNull(command); + ArgumentNullException.ThrowIfNull(panel); + ArgumentNullException.ThrowIfNull(logger); + ArgumentNullException.ThrowIfNull(context); + + return command.ThrownExceptions.Subscribe(ex => ExceptionReporter.ReportAndLog(panel, logger, context, ex)); + } +} diff --git a/SemiStep/SemiStep.UI/RecipeFile/RecipeFileViewModel.cs b/SemiStep/SemiStep.UI/RecipeFile/RecipeFileViewModel.cs index af393b9..40b8d49 100644 --- a/SemiStep/SemiStep.UI/RecipeFile/RecipeFileViewModel.cs +++ b/SemiStep/SemiStep.UI/RecipeFile/RecipeFileViewModel.cs @@ -3,6 +3,8 @@ using System.Reactive.Disposables.Fluent; using System.Reactive.Linq; +using Microsoft.Extensions.Logging; + using ReactiveUI; using SemiStep.UI.Coordinator; @@ -15,14 +17,17 @@ public class RecipeFileViewModel : ReactiveObject, IDisposable private readonly RecipeCoordinator _coordinator; private readonly CompositeDisposable _disposables = new(); + private readonly ILogger _logger; private readonly MessagePanelViewModel _messagePanel; public RecipeFileViewModel( RecipeCoordinator coordinator, - MessagePanelViewModel messagePanel) + MessagePanelViewModel messagePanel, + ILogger logger) { _coordinator = coordinator; _messagePanel = messagePanel; + _logger = logger; OpenFileInteraction = new Interaction(); SaveFileInteraction = new Interaction(); @@ -34,19 +39,13 @@ public RecipeFileViewModel( LoadRecipeCommand = ReactiveCommand.CreateFromTask(LoadRecipeAsync, canEdit); NewRecipeCommand = ReactiveCommand.Create(NewRecipe, canEdit); - SaveRecipeCommand.ThrownExceptions - .ObserveOn(RxSchedulers.MainThreadScheduler) - .Subscribe(ex => _messagePanel.ReportError($"Save failed: {ex.Message}")) + SaveRecipeCommand.ReportThrownExceptions(_messagePanel, _logger, "Save failed") .DisposeWith(_disposables); - SaveAsRecipeCommand.ThrownExceptions - .ObserveOn(RxSchedulers.MainThreadScheduler) - .Subscribe(ex => _messagePanel.ReportError($"Save As failed: {ex.Message}")) + SaveAsRecipeCommand.ReportThrownExceptions(_messagePanel, _logger, "Save As failed") .DisposeWith(_disposables); - LoadRecipeCommand.ThrownExceptions - .ObserveOn(RxSchedulers.MainThreadScheduler) - .Subscribe(ex => _messagePanel.ReportError($"Load failed: {ex.Message}")) + LoadRecipeCommand.ReportThrownExceptions(_messagePanel, _logger, "Load failed") .DisposeWith(_disposables); } diff --git a/SemiStep/SemiStep.UI/StyleEditor/GridStyleEditorViewModel.cs b/SemiStep/SemiStep.UI/StyleEditor/GridStyleEditorViewModel.cs index 26b0344..667f70c 100644 --- a/SemiStep/SemiStep.UI/StyleEditor/GridStyleEditorViewModel.cs +++ b/SemiStep/SemiStep.UI/StyleEditor/GridStyleEditorViewModel.cs @@ -1,10 +1,13 @@ -using System.Reactive; +using System; +using System.Reactive; using System.Runtime.CompilerServices; using Avalonia.Media; using FluentResults; +using Microsoft.Extensions.Logging; + using ReactiveUI; using SemiStep.Core.Configuration; @@ -33,30 +36,41 @@ public sealed class GridStyleEditorViewModel : ReactiveObject public const int MinPanelMaxHeight = 20; public const int MaxPanelMaxHeight = 2000; - private readonly GridStyleEditorFacade _gridStyleEditorFacade; + private readonly IGridStyleEditorFacade _gridStyleEditorFacade; private readonly string _configDir; + private readonly ILogger _logger; private GridStyleOptions _source; private bool _seeding; - public GridStyleEditorViewModel(GridStyleEditorFacade gridStyleEditorFacade, StartupOptions startupOptions) - : this(gridStyleEditorFacade, startupOptions.ConfigDir, GridStyleOptions.Default) + public GridStyleEditorViewModel( + IGridStyleEditorFacade gridStyleEditorFacade, + StartupOptions startupOptions, + ILogger logger) + : this(gridStyleEditorFacade, startupOptions.ConfigDir, GridStyleOptions.Default, logger) { } public GridStyleEditorViewModel( - GridStyleEditorFacade gridStyleEditorFacade, + IGridStyleEditorFacade gridStyleEditorFacade, string configDir, - GridStyleOptions source) + GridStyleOptions source, + ILogger logger) { _gridStyleEditorFacade = gridStyleEditorFacade; _configDir = configDir; _source = source; + _logger = logger; SaveCommand = ReactiveCommand.Create( Save, this.WhenAnyValue(viewModel => viewModel.CanSave)); + // Modal editor: a save fault must surface on the editor's own ErrorMessage, not the + // shared message panel hidden behind the dialog. Subscription lifetime tracks this + // transient dialog VM, which is neither IDisposable nor pooled. + SaveCommand.ThrownExceptions.Subscribe(ReportSaveException); + Seed(source); } @@ -288,6 +302,12 @@ private bool Save() return true; } + internal void ReportSaveException(Exception exception) + { + _logger.LogError(exception, "Style editor save failed"); + ErrorMessage = $"Save failed: {exception.Message}"; + } + private void Seed(GridStyleOptions options) { _seeding = true; diff --git a/SemiStep/SemiStep.UI/UiDi.cs b/SemiStep/SemiStep.UI/UiDi.cs index eae906a..9e87c10 100644 --- a/SemiStep/SemiStep.UI/UiDi.cs +++ b/SemiStep/SemiStep.UI/UiDi.cs @@ -23,7 +23,7 @@ public static class UiDi public static IServiceCollection AddUi(this IServiceCollection services) { services.AddSingleton(sp => sp.GetRequiredService().GridStyle); - services.AddSingleton(); + services.AddSingleton(); services.AddSingleton(_ => RxSchedulers.MainThreadScheduler); services.AddSingleton(); services.AddSingleton(); diff --git a/docs/plans/completed/20260727-pr-f1-report-log-extension.md b/docs/plans/completed/20260727-pr-f1-report-log-extension.md new file mode 100644 index 0000000..1de0ff3 --- /dev/null +++ b/docs/plans/completed/20260727-pr-f1-report-log-extension.md @@ -0,0 +1,148 @@ +# PR-F1: Report+Log command-exception extension + +## Overview + +The command-boundary ring of the error pipe (see `20260727-error-reporting-pipe-roadmap.md`). Today +the `ThrownExceptions → ObserveOn(MainThreadScheduler) → panel.ReportError($"…{ex.Message}")` block is +copy-pasted 8× and **logs nothing** — user-visible errors discard the exception type and stack. This +PR extracts one extension that reports **and** logs with the stack, wires every existing site through +it, closes three unguarded `Subscribe` faults and one unsubscribed command that crashes today. + +No `IMessageSink` — consumers keep the concrete `MessagePanelViewModel` (roadmap decision). No +behavior change on the happy path; on failure, the same panel message appears and a stack-carrying +log line is added. + +## Context (grounded) + +- The 8 identical blocks: `ClipboardViewModel.cs:56-69` (3), `RecipeFileViewModel.cs:37-50` (3), + `MainWindowViewModel.cs:71-79` (2). Each is `command.ThrownExceptions.ObserveOn(RxSchedulers.MainThreadScheduler).Subscribe(ex => _messagePanel.ReportError($"…: {ex.Message}"))`. +- The `ObserveOn` is redundant: `MessagePanelViewModel.PostOnUiThread` (`:211-221`) self-marshals. +- Existing extension home: `ResultReportingExtensions` (`SemiStep.UI/MessageService/ResultReportingExtensions.cs:14`) already extends the concrete `MessagePanelViewModel`. +- `ILogger` → class-named logs: the Serilog template prints `{SourceContext}` (`Program.cs`), and the MEL→Serilog bridge maps `ILogger`'s category to it. `MainWindowViewModel` already injects `ILogger` (`:32,47`); `ClipboardViewModel`, `RecipeFileViewModel`, `GridStyleEditorViewModel` do not. +- Three bare `Subscribe` (no `onError`, fatal-path today): `MainWindowViewModel.cs:85` (`PlcStateChanged`), `:89` (`PlcRecipeConflictDetected`), `:92-94` (`Interval` tick). +- Live crash path: `GridStyleEditorViewModel.SaveCommand` (`:56-58`) has no `ThrownExceptions` subscription; `Save()` (`:272`) does file I/O. A throw hits `RxApp.DefaultExceptionHandler` → crash. The editor has its own `ErrorMessage` surface (`:65`). +- DI: all VMs are container-registered (`UiDi.cs:36-44`); adding an `ILogger` ctor param resolves automatically. Direct test construction sites that need the new arg: + - `ClipboardViewModel`: `ClipboardViewModelCanExecuteTests.cs:37,111`, `UIFixture.cs:151`, `MessagePanelReportingTests.cs:147,185`. + - `RecipeFileViewModel`: `UIFixture.cs:158`, `MessagePanelReportingTests.cs:50,72,97`, `RecipeFileViewModelCanExecuteTests.cs:26`, `RecipeFileViewModelSaveResultTests.cs:31`. + - `GridStyleEditorViewModel`: `UIFixture.cs:166`, `GridStyleEditorViewModelTests.cs:252`, `GridStyleEditorWindowOwnerRoutingTests.cs:118`, `GridStyleEditorWindowTests.cs:31,59`. + +## Development Approach + +- Regular (code, then tests). Warnings are errors — build must stay clean. +- Tests use `NullLogger.Instance` (`Microsoft.Extensions.Logging.Abstractions`) except where a test asserts logging, which uses a capturing `ILogger`. +- Run `dotnet build SemiStep.slnx` and `dotnet test …` after each task; all green before the next. +- `dotnet format SemiStep.slnx` before finishing. + +## Acceptance Evidence + +Automatable: +1. `ReportThrownExceptions`: a command whose execute throws produces exactly one panel entry AND one `LogError(ex,…)` carrying the exception (capturing logger). `--filter "FullyQualifiedName~ReportThrownExceptions"`. +2. The three `MainWindowViewModel` subscriptions: a throw in each callback reports to the panel instead of escaping. `--filter "FullyQualifiedName~MainWindowViewModel"`. +3. `GridStyleEditorViewModel.SaveCommand`: a throw in `Save()` surfaces on `ErrorMessage` and logs, without crashing. `--filter "FullyQualifiedName~GridStyleEditor"`. +4. Regression: existing `MessagePanelReportingTests` stay green (same panel messages). + +Full suite green + `dotnet build SemiStep.slnx` (0 warnings) is the gate. + +## Progress Tracking + +Mark `[x]` on completion; `➕` new tasks; `⚠️` blockers. + +## Implementation Steps + +### Task 1: Add the `ReportThrownExceptions` extension + +**Files:** +- Create: `SemiStep/SemiStep.UI/MessageService/ReactiveCommandReportingExtensions.cs` +- Create: `SemiStep/SemiStep.Tests/UI/MessageService/ReactiveCommandReportingExtensionsTests.cs` + +- [x] add `static IDisposable ReportThrownExceptions(this ReactiveCommand command, MessagePanelViewModel panel, ILogger logger, string context)` that subscribes `ThrownExceptions` and, per exception, `logger.LogError(ex, "{Context} failed", context)` then `panel.ReportError($"{context}: {ex.Message}")`; no `ObserveOn` +- [x] `System` usings first, then others; tabs; braces on new line +- [x] test: a throwing command yields exactly one `ReportError` on the panel (assert via a real `MessagePanelViewModel`) and one `LogError` with the thrown exception (capturing `ILogger`) +- [x] test: a non-throwing execute produces no panel entry and no log +- [x] run `--filter "FullyQualifiedName~ReportThrownExceptions"` — green before next task + +### Task 2: Rewire `ClipboardViewModel` + +**Files:** +- Modify: `SemiStep/SemiStep.UI/Clipboard/ClipboardViewModel.cs` +- Modify: `SemiStep/SemiStep.Tests/UI/Helpers/UIFixture.cs` +- Modify: `SemiStep/SemiStep.Tests/UI/Clipboard/ClipboardViewModelCanExecuteTests.cs` +- Modify: `SemiStep/SemiStep.Tests/UI/MessagePanelReportingTests.cs` + +- [x] add `ILogger` ctor param + field +- [x] replace the three `ThrownExceptions` blocks (`:56-69`) with `command.ReportThrownExceptions(_messagePanel, _logger, "Copy failed"/"Cut failed"/"Paste failed").DisposeWith(_disposables)` (keep the existing user-facing text; extension appends `": {ex.Message}"`) +- [x] update the 5 direct construction sites to pass `NullLogger.Instance` +- [x] confirm existing clipboard reporting tests still pass (same messages) +- [x] run clipboard-related filters — green before next task + +### Task 3: Rewire `RecipeFileViewModel` + +**Files:** +- Modify: `SemiStep/SemiStep.UI/RecipeFile/RecipeFileViewModel.cs` +- Modify: `SemiStep/SemiStep.Tests/UI/Helpers/UIFixture.cs` +- Modify: `SemiStep/SemiStep.Tests/UI/MessagePanelReportingTests.cs` +- Modify: `SemiStep/SemiStep.Tests/UI/RecipeFile/RecipeFileViewModelCanExecuteTests.cs` +- Modify: `SemiStep/SemiStep.Tests/UI/RecipeFile/RecipeFileViewModelSaveResultTests.cs` + +- [x] add `ILogger` ctor param + field +- [x] replace the three `ThrownExceptions` blocks (`:37-50`) with the extension ("Save failed"/"Save As failed"/"Load failed") +- [x] update the 5 direct construction sites to pass `NullLogger.Instance` +- [x] run recipe-file filters — green before next task + +### Task 4: Rewire `MainWindowViewModel` + guard the three bare subscriptions + +**Files:** +- Modify: `SemiStep/SemiStep.UI/MainWindow/MainWindowViewModel.cs` +- Create/Modify: `SemiStep/SemiStep.Tests/UI/MainWindow/MainWindowViewModelReportingTests.cs` + +- [x] replace the two `ThrownExceptions` blocks (`:71-79`) with the extension ("Sync toggle failed"/"Style editor failed"); `_logger` already exists +- [x] give `PlcStateChanged` (`:85`), `PlcRecipeConflictDetected` (`:89`), and the `Interval` tick (`:94`) an `onError` that `_logger.LogError(ex,…)` + `MessagePanel.ReportError(…)` (do not let a callback throw escape to the fatal path) +- [x] test: a forced throw in each of the three callbacks reports to the panel and does not crash the headless app +- [x] run `--filter "FullyQualifiedName~MainWindowViewModel"` — green before next task + +### Task 5: Wire `GridStyleEditorViewModel.SaveCommand` to its own surface + log + +**Files:** +- Modify: `SemiStep/SemiStep.UI/StyleEditor/GridStyleEditorViewModel.cs` +- Modify: `SemiStep/SemiStep.Tests/UI/Helpers/UIFixture.cs` +- Modify: `SemiStep/SemiStep.Tests/UI/StyleEditor/GridStyleEditorViewModelTests.cs` +- Modify: `SemiStep/SemiStep.Tests/UI/StyleEditor/GridStyleEditorWindowOwnerRoutingTests.cs` +- Modify: `SemiStep/SemiStep.Tests/UI/StyleEditor/GridStyleEditorWindowTests.cs` + +- [x] add `ILogger` to both constructors (thread it through the delegating `:42` ctor) +- [x] subscribe `SaveCommand.ThrownExceptions` → `_logger.LogError(ex, "Style editor save failed")` + set `ErrorMessage` to a message (the editor's own surface, not the shared panel); dispose with the VM +- [x] update the 4+ direct construction sites to pass `NullLogger.Instance` +- [x] test: a forced throw in `Save()` sets `ErrorMessage`, logs, and does not crash +- [x] run `--filter "FullyQualifiedName~GridStyleEditor"` — green before next task + +### Task 6: Verify + document + +**Files:** +- Modify: `Docs/architecture/error-reporting.md` + +- [x] full suite: `dotnet test SemiStep/SemiStep.Tests/SemiStep.Tests.csproj` (1472 passed, 0 failed) +- [x] `dotnet build SemiStep.slnx` — 0 warnings, 0 errors +- [x] `dotnet format SemiStep.slnx` (no changes) +- [x] update `error-reporting.md`: the exception-handler line now reads "route through `ReportThrownExceptions` (reports + logs with stack)"; note the concrete-panel decision +- [ ] ⚠️ move this plan to `docs/plans/completed/` (deferred to delivery — plan stays until branch is shipped; not performed) + +## Post-Completion + +- Open the PR: "feat(ui): route command exceptions through report+log extension". Note in the body that user-visible errors now carry a class-named stack trace to the log; no happy-path behavior change. +- Follow-on: PR-F2 (global backstop), then #113 rebased on both. + +**Executed by exec:** +- branch: report-log-extension + +## Verify it yourself + +The change is invisible on the happy path; it only shows up when a command or subscription faults. Prove it with the tests that reproduce each fault. + +- **Command exception reports + logs (the core outcome):** + `dotnet test SemiStep/SemiStep.Tests/SemiStep.Tests.csproj --filter "FullyQualifiedName~ReportThrownExceptions"` — a throwing `ReactiveCommand` produces exactly one panel entry AND one `LogError` carrying the same exception instance. Before this branch the log entry did not exist (handlers only reported). +- **Style-editor save crash is closed (was a live crash):** + `dotnet test ... --filter "FullyQualifiedName~GridStyleEditor"` — `SaveCommand_WhenSaveThrows_...` drives `SaveCommand.Execute()` through a throwing facade and asserts `ErrorMessage` is set + logged, no crash. On `master`, `SaveCommand` has no `ThrownExceptions` subscription, so that throw hits `RxApp.DefaultExceptionHandler` and kills the process. +- **Subscription-callback throws are contained (was fatal for the Interval tick):** + `dotnet test ... --filter "FullyQualifiedName~MainWindowViewModel"` — `Guarded_ThrowInOnNextBody_...` forces a throw inside the guarded onNext and asserts it reports without escaping. +- **No happy-path regression / no doubled log:** full suite `dotnet test SemiStep/SemiStep.Tests/SemiStep.Tests.csproj` (1474 pass) and `dotnet build SemiStep.slnx` (0 warnings). Existing reporting tests still assert the same user-facing panel messages ("Copy failed:", "Load failed:", …). +- **Manual (optional):** run the app, trigger a command failure (e.g. paste malformed clipboard content), confirm the panel message is unchanged AND the log file now carries the exception with `SourceContext` = the originating class.