fix(adminui,client): error boundary inside the interactive scope, stale-commit OnSaved, AbCip browse delay emit, DisposeBridge cancel (review R3)
M1: PageErrorBoundary wraps the body of every InteractiveServer page; the inert layout boundary is removed. m1: DisposeBridge cancels its CTS when the 5 s wait times out. m2: a CSV import that commits after the modal closed still raises OnSaved. m3: document SecurityMode=None no-fallback in Client.CLI.md / Client.UI.md. m4: browsePageDelayMs written only when loaded or changed; negative flagged. Claude-Session: https://claude.ai/code/session_01Ef2ozxS7QPanGT8uoATbTe
This commit is contained in:
Joseph Doherty
@@ -328,4 +328,36 @@ public sealed class AbCipDriverPageFormSerializationTests
|
||||
saved.ShouldContain("futureOption");
|
||||
System.Text.Json.Nodes.JsonNode.Parse(saved)!.AsObject().ContainsKey("browsePageDelay").ShouldBeFalse();
|
||||
}
|
||||
|
||||
// ---- review R3-m4: the key is emitted only when loaded or changed from the default; negatives are flagged ----
|
||||
|
||||
[Fact]
|
||||
public void BrowsePageDelayMs_is_omitted_when_never_set_and_left_at_the_default()
|
||||
{
|
||||
var original = """{"futureOption":"keep-me","devices":[{"hostAddress":"ab://10.0.0.1/1,0"}]}""";
|
||||
var loaded = AbCipDriverForm.TryDeserialize(original)!;
|
||||
var form = AbCipDriverForm.FormModel.FromJson(original, loaded);
|
||||
|
||||
var saved = AbCipDriverForm.Serialize(form.ToOptions(loaded.Devices), original, form.EmitsBrowsePageDelay);
|
||||
System.Text.Json.Nodes.JsonNode.Parse(saved)!.AsObject().ContainsKey("browsePageDelayMs").ShouldBeFalse();
|
||||
saved.ShouldContain("futureOption");
|
||||
|
||||
// Control: changing it away from the default DOES write it, and a loaded key is kept even at the default value.
|
||||
form.BrowsePageDelayMs = 125;
|
||||
var changed = AbCipDriverForm.Serialize(form.ToOptions(loaded.Devices), original, form.EmitsBrowsePageDelay);
|
||||
System.Text.Json.Nodes.JsonNode.Parse(changed)!["browsePageDelayMs"]!.GetValue<int>().ShouldBe(125);
|
||||
|
||||
var withKey = """{"browsePageDelayMs":5,"devices":[{"hostAddress":"ab://10.0.0.1/1,0"}]}""";
|
||||
var f2 = AbCipDriverForm.FormModel.FromJson(withKey, AbCipDriverForm.TryDeserialize(withKey)!);
|
||||
f2.EmitsBrowsePageDelay.ShouldBeTrue();
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void BrowsePageDelayMs_negative_is_a_validation_error_zero_is_valid()
|
||||
{
|
||||
var form = new AbCipDriverForm.FormModel { BrowsePageDelayMs = -1 };
|
||||
form.BrowsePageDelayError.ShouldNotBeNull();
|
||||
form.BrowsePageDelayMs = 0;
|
||||
form.BrowsePageDelayError.ShouldBeNull();
|
||||
}
|
||||
}
|
||||
|
||||
@@ -5,7 +5,7 @@ namespace ZB.MOM.WW.OtOpcUa.AdminUI.Tests.Raw;
|
||||
|
||||
/// <summary>
|
||||
/// Issue #604 source-scan guards (Razor has no bUnit): an <c>EditForm</c> with a validator but nothing
|
||||
/// to show its messages is a silent no-op Save, the layout must keep its error boundary, and the
|
||||
/// to show its messages is a silent no-op Save, the interactive pages' error boundary is pinned in PageErrorBoundaryTests, and the
|
||||
/// API-key Delete must not fire straight from the row button.
|
||||
/// </summary>
|
||||
public sealed class AdminUiFormSweepGuardTests
|
||||
@@ -29,15 +29,6 @@ public sealed class AdminUiFormSweepGuardTests
|
||||
offenders.ShouldBeEmpty("these forms validate but never show a message: " + string.Join(", ", offenders));
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void MainLayout_wraps_the_body_in_an_ErrorBoundary()
|
||||
{
|
||||
var text = File.ReadAllText(Path.Combine(ComponentsDir(), "Layout", "MainLayout.razor"));
|
||||
var boundary = text.IndexOf("<ErrorBoundary", StringComparison.Ordinal);
|
||||
boundary.ShouldBeGreaterThan(-1);
|
||||
text.IndexOf("@Body", StringComparison.Ordinal).ShouldBeGreaterThan(boundary);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void ApiKeys_delete_button_goes_through_the_confirm_dialog()
|
||||
{
|
||||
|
||||
@@ -57,6 +57,50 @@ public sealed class ModalStaleOpenGuardTests : IDisposable
|
||||
Field<string>(modal, "_effectiveDeviceId").ShouldBe("");
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task CsvImport_commit_that_lands_after_the_modal_closed_still_raises_OnSaved()
|
||||
{
|
||||
// A write that has landed must refresh the parent's tree even when the operator closed the modal
|
||||
// mid-commit; before the fix the stale guard returned ahead of OnSaved.
|
||||
var saved = await CommitWhileClosed(new RawTagImportResult(1, []));
|
||||
saved.ShouldBe(1);
|
||||
|
||||
// Control: a failed (all-or-nothing) import wrote nothing, so there is nothing to refresh.
|
||||
var savedOnFailure = await CommitWhileClosed(new RawTagImportResult(0, ["row 1: nope"]));
|
||||
savedOnFailure.ShouldBe(0);
|
||||
}
|
||||
|
||||
private async Task<int> CommitWhileClosed(RawTagImportResult outcome)
|
||||
{
|
||||
var gate = new TaskCompletionSource();
|
||||
var svc = Stub<IRawTreeService>(async (method, _) =>
|
||||
{
|
||||
if (method.Name != nameof(IRawTreeService.ImportTagsAsync)) { return null; }
|
||||
await gate.Task;
|
||||
return outcome;
|
||||
});
|
||||
var modal = Make<RawCsvImportModal>(("Svc", svc));
|
||||
var saved = 0;
|
||||
SetParams(modal,
|
||||
("Visible", true), ("DeviceId", "dev-A"), ("TagGroupId", null),
|
||||
("DriverType", "Modbus"),
|
||||
("OnSaved", EventCallback.Factory.Create(this, () => { saved++; })));
|
||||
await Open(modal);
|
||||
|
||||
modal.GetType().GetField("_result", BindingFlags.NonPublic | BindingFlags.Instance)!.SetValue(
|
||||
modal, RawTagCsvMapper.Parse("Modbus", "Name,DataType,TagConfigJson\nok,Float,\"{\"\"region\"\":\"\"HoldingRegisters\"\",\"\"address\"\":1}\""));
|
||||
|
||||
var commit = (Task)modal.GetType().GetMethod("Commit", BindingFlags.NonPublic | BindingFlags.Instance)!
|
||||
.Invoke(modal, null)!;
|
||||
|
||||
SetParams(modal, ("Visible", false));
|
||||
await Open(modal);
|
||||
|
||||
gate.SetResult();
|
||||
await commit;
|
||||
return saved;
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task CsvExport_stale_group_resolve_never_retargets_the_live_open()
|
||||
{
|
||||
|
||||
@@ -0,0 +1,239 @@
|
||||
using System.Reflection;
|
||||
using Microsoft.AspNetCore.Components;
|
||||
using Microsoft.AspNetCore.Components.RenderTree;
|
||||
using Microsoft.AspNetCore.Components.Web;
|
||||
using Microsoft.AspNetCore.Components.Web.Infrastructure;
|
||||
using Microsoft.Extensions.DependencyInjection;
|
||||
using Microsoft.Extensions.Logging.Abstractions;
|
||||
using Shouldly;
|
||||
using Xunit;
|
||||
using ZB.MOM.WW.OtOpcUa.AdminUI.Components.Shared;
|
||||
|
||||
namespace ZB.MOM.WW.OtOpcUa.AdminUI.Tests.Raw;
|
||||
|
||||
/// <summary>
|
||||
/// Issue #604 / review R3-M1: the error boundary must live INSIDE the interactive scope. These tests
|
||||
/// drive the real <see cref="PageErrorBoundary"/> through a real <see cref="Renderer"/> (no bUnit in this
|
||||
/// project): a child that throws while rendering is contained (nothing reaches the renderer's unhandled
|
||||
/// path, i.e. the circuit survives), "Try again" and a location change both recover, and disposing the
|
||||
/// boundary unsubscribes from the navigation manager. A structural guard also pins that every
|
||||
/// interactive page actually wraps its body.
|
||||
/// </summary>
|
||||
#pragma warning disable BL0006
|
||||
// Renderer lives in Microsoft.AspNetCore.Components.RenderTree; subclassing it is the only way to host a
|
||||
// component without a bUnit dependency (same approach as RawBrowseModalStaleOpenTests).
|
||||
public sealed class PageErrorBoundaryTests : IDisposable
|
||||
{
|
||||
private readonly FakeNav _nav = new();
|
||||
private readonly TestRenderer _renderer;
|
||||
|
||||
public PageErrorBoundaryTests()
|
||||
{
|
||||
var sp = new ServiceCollection()
|
||||
.AddSingleton<NavigationManager>(_nav)
|
||||
.AddSingleton<IErrorBoundaryLogger, NullBoundaryLogger>()
|
||||
.BuildServiceProvider();
|
||||
_renderer = new TestRenderer(sp);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task A_throw_in_the_page_body_is_contained_and_Try_again_recovers()
|
||||
{
|
||||
Thrower.ShouldThrow = false;
|
||||
await _renderer.RenderRootAsync();
|
||||
_renderer.Text.ShouldContain("page body ok"); // positive control: the healthy body renders
|
||||
|
||||
Thrower.ShouldThrow = true;
|
||||
await _renderer.Dispatcher.InvokeAsync(() => _renderer.RerenderThrower());
|
||||
|
||||
_renderer.Unhandled.ShouldBeEmpty(); // contained: the circuit would have survived
|
||||
_renderer.Text.ShouldContain("This page hit an unexpected error.");
|
||||
_renderer.Text.ShouldContain("boom");
|
||||
_renderer.Text.ShouldNotContain("page body ok");
|
||||
|
||||
// "Try again" is a live handler, not dead static markup.
|
||||
Thrower.ShouldThrow = false;
|
||||
_renderer.LastOnClickHandlerId.ShouldNotBe(0UL);
|
||||
await _renderer.Dispatcher.InvokeAsync(() =>
|
||||
_renderer.DispatchEventAsync(_renderer.LastOnClickHandlerId, null, new EventArgs()));
|
||||
|
||||
_renderer.Text.ShouldContain("page body ok");
|
||||
_renderer.Unhandled.ShouldBeEmpty();
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task A_location_change_recovers_a_tripped_boundary()
|
||||
{
|
||||
Thrower.ShouldThrow = false;
|
||||
await _renderer.RenderRootAsync();
|
||||
Thrower.ShouldThrow = true;
|
||||
await _renderer.Dispatcher.InvokeAsync(() => _renderer.RerenderThrower());
|
||||
_renderer.Text.ShouldContain("This page hit an unexpected error.");
|
||||
|
||||
Thrower.ShouldThrow = false;
|
||||
await _renderer.Dispatcher.InvokeAsync(() => _nav.NavigateTo("http://localhost/other"));
|
||||
await _renderer.Dispatcher.InvokeAsync(() => Task.CompletedTask); // let the queued Recover run
|
||||
|
||||
_renderer.Text.ShouldContain("page body ok");
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task Disposing_the_boundary_unsubscribes_from_navigation()
|
||||
{
|
||||
Thrower.ShouldThrow = false;
|
||||
await _renderer.RenderRootAsync();
|
||||
FakeNav.HandlerCount(_nav).ShouldBe(1); // positive control: it did subscribe
|
||||
|
||||
_renderer.Dispose();
|
||||
FakeNav.HandlerCount(_nav).ShouldBe(0);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void Every_interactive_page_wraps_its_body_in_PageErrorBoundary_and_the_layout_does_not_pretend_to()
|
||||
{
|
||||
var components = ComponentsDir();
|
||||
var offenders = new List<string>();
|
||||
foreach (var file in Directory.EnumerateFiles(Path.Combine(components, "Pages"), "*.razor", SearchOption.AllDirectories))
|
||||
{
|
||||
var text = File.ReadAllText(file);
|
||||
if (!text.Contains("@rendermode")) { continue; }
|
||||
// Same text the router compiles: the wrapper must open before, and close after, the markup.
|
||||
var open = text.IndexOf("<PageErrorBoundary>", StringComparison.Ordinal);
|
||||
var close = text.LastIndexOf("</PageErrorBoundary>", StringComparison.Ordinal);
|
||||
var code = text.IndexOf("\n@code", StringComparison.Ordinal);
|
||||
var ok = open > -1 && close > open && (code < 0 || close < code);
|
||||
if (!ok) { offenders.Add(Path.GetRelativePath(components, file).Replace('\\', '/')); }
|
||||
}
|
||||
|
||||
offenders.ShouldBeEmpty("interactive pages without a PageErrorBoundary wrapper: " + string.Join(", ", offenders));
|
||||
|
||||
// The static layout is not an ancestor of interactive pages: a boundary there is inert.
|
||||
File.ReadAllText(Path.Combine(components, "Layout", "MainLayout.razor")).ShouldNotContain("<ErrorBoundary");
|
||||
}
|
||||
|
||||
public void Dispose() => _renderer.Dispose();
|
||||
|
||||
private static string ComponentsDir()
|
||||
{
|
||||
var dir = new DirectoryInfo(AppContext.BaseDirectory);
|
||||
while (dir is not null)
|
||||
{
|
||||
var candidate = Path.Combine(dir.FullName, "src", "Server", "ZB.MOM.WW.OtOpcUa.AdminUI", "Components");
|
||||
if (Directory.Exists(candidate)) { return candidate; }
|
||||
dir = dir.Parent;
|
||||
}
|
||||
throw new DirectoryNotFoundException("AdminUI Components directory not found above the test binaries.");
|
||||
}
|
||||
|
||||
/// <summary>Throws from its render when <see cref="ShouldThrow"/> is set.</summary>
|
||||
private sealed class Thrower : ComponentBase
|
||||
{
|
||||
public static volatile bool ShouldThrow;
|
||||
|
||||
protected override void BuildRenderTree(Microsoft.AspNetCore.Components.Rendering.RenderTreeBuilder builder)
|
||||
{
|
||||
if (ShouldThrow) { throw new InvalidOperationException("boom"); }
|
||||
builder.AddMarkupContent(0, "page body ok");
|
||||
}
|
||||
|
||||
public void Poke() => StateHasChanged();
|
||||
}
|
||||
|
||||
private sealed class Host : ComponentBase
|
||||
{
|
||||
internal Thrower? Child;
|
||||
|
||||
protected override void BuildRenderTree(Microsoft.AspNetCore.Components.Rendering.RenderTreeBuilder builder)
|
||||
{
|
||||
builder.OpenComponent<PageErrorBoundary>(0);
|
||||
builder.AddAttribute(1, nameof(PageErrorBoundary.ChildContent), (RenderFragment)(b =>
|
||||
{
|
||||
b.OpenComponent<Thrower>(0);
|
||||
b.AddComponentReferenceCapture(1, o => Child = (Thrower)o);
|
||||
b.CloseComponent();
|
||||
}));
|
||||
builder.CloseComponent();
|
||||
}
|
||||
}
|
||||
|
||||
private sealed class FakeNav : NavigationManager
|
||||
{
|
||||
public FakeNav() => Initialize("http://localhost/", "http://localhost/");
|
||||
|
||||
protected override void NavigateToCore(string uri, NavigationOptions options)
|
||||
{
|
||||
Uri = uri;
|
||||
NotifyLocationChanged(isInterceptedLink: false);
|
||||
}
|
||||
|
||||
public static int HandlerCount(NavigationManager nav)
|
||||
{
|
||||
var field = typeof(NavigationManager).GetField("_locationChanged", BindingFlags.Instance | BindingFlags.NonPublic);
|
||||
field.ShouldNotBeNull("NavigationManager's LocationChanged backing field was renamed; update this probe");
|
||||
return (field.GetValue(nav) as Delegate)?.GetInvocationList().Length ?? 0;
|
||||
}
|
||||
}
|
||||
|
||||
private sealed class NullBoundaryLogger : IErrorBoundaryLogger
|
||||
{
|
||||
public ValueTask LogErrorAsync(Exception exception) => ValueTask.CompletedTask;
|
||||
}
|
||||
|
||||
private sealed class TestRenderer(IServiceProvider sp) : Renderer(sp, NullLoggerFactory.Instance)
|
||||
{
|
||||
private Host? _host;
|
||||
private readonly List<string> _text = new();
|
||||
|
||||
public List<Exception> Unhandled { get; } = new();
|
||||
|
||||
public ulong LastOnClickHandlerId { get; private set; }
|
||||
|
||||
public string Text => string.Concat(_text);
|
||||
|
||||
public override Dispatcher Dispatcher { get; } = Dispatcher.CreateDefault();
|
||||
|
||||
public async Task RenderRootAsync()
|
||||
{
|
||||
await Dispatcher.InvokeAsync(async () =>
|
||||
{
|
||||
_host = (Host)InstantiateComponent(typeof(Host));
|
||||
var id = AssignRootComponentId(_host);
|
||||
await RenderRootComponentAsync(id);
|
||||
});
|
||||
}
|
||||
|
||||
public Task RerenderThrower()
|
||||
{
|
||||
_host!.Child!.Poke();
|
||||
return Task.CompletedTask;
|
||||
}
|
||||
|
||||
protected override void HandleException(Exception exception) => Unhandled.Add(exception);
|
||||
|
||||
protected override Task UpdateDisplayAsync(in RenderBatch renderBatch)
|
||||
{
|
||||
// Each batch replaces the visible text when it carries any: the swap between ChildContent and
|
||||
// ErrorContent is a whole-subtree change, so its frames are all in that batch.
|
||||
var batchText = new List<string>();
|
||||
var frames = renderBatch.ReferenceFrames;
|
||||
for (var i = 0; i < frames.Count; i++)
|
||||
{
|
||||
ref readonly var f = ref frames.Array[i];
|
||||
switch (f.FrameType)
|
||||
{
|
||||
case RenderTreeFrameType.Text:
|
||||
batchText.Add(f.TextContent);
|
||||
break;
|
||||
case RenderTreeFrameType.Markup:
|
||||
batchText.Add(f.MarkupContent);
|
||||
break;
|
||||
case RenderTreeFrameType.Attribute when f.AttributeEventHandlerId != 0:
|
||||
LastOnClickHandlerId = f.AttributeEventHandlerId;
|
||||
break;
|
||||
}
|
||||
}
|
||||
if (batchText.Count > 0) { _text.Clear(); _text.AddRange(batchText); }
|
||||
return Task.CompletedTask;
|
||||
}
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user