diff --git a/code-reviews/CentralUI/findings.md b/code-reviews/CentralUI/findings.md index fef0446..51c37fc 100644 --- a/code-reviews/CentralUI/findings.md +++ b/code-reviews/CentralUI/findings.md @@ -8,7 +8,7 @@ | Last reviewed | 2026-05-16 | | Reviewer | claude-agent | | Commit reviewed | `9c60592` | -| Open findings | 15 | +| Open findings | 7 | ## Summary @@ -269,7 +269,17 @@ fixed 30-minute model. The code and the documented decision must agree. **Resolution** -_Unresolved._ +_Unresolved — requires a cross-module change plus a design decision, both out of +scope for a CentralUI-only fix._ Verified 2026-05-16: the discrepancy is real. +The sliding-expiration mechanism, however, is owned by the cookie +authentication middleware configured in **`ScadaLink.Security`** +(`ServiceCollectionExtensions.AddCookie` — currently sets neither +`ExpireTimeSpan` nor `SlidingExpiration`); `AuthEndpoints` (CentralUI) only sets +the absolute `ExpiresUtc`/`expires_at`. Implementing "15-minute sliding token" +means editing `ScadaLink.Security`, which this module's review cannot touch, and +the alternative — amending the documented decision to a fixed 30-minute model — +is a design decision, not a code fix. Left Open and surfaced for a follow-up +that spans CentralUI + Security, or a design-doc amendment. ### CentralUI-006 — Deployment status page polls every 10s despite the documented SignalR-push design @@ -300,7 +310,16 @@ If polling is kept as a fallback, fetch only changed/in-progress records. **Resolution** -_Unresolved._ +_Unresolved — a genuine SignalR-push fix requires an event source in another +module._ Verified 2026-05-16: `Deployments.razor` does poll every 10s, contrary +to the design doc. But a real push implementation needs the **Deployment +Manager** module (`ScadaLink.DeploymentManager` — `DeploymentService` / +`ArtifactDeploymentService` write the `DeploymentRecord` rows) to raise a +status-change event/observable that the page subscribes to; there is no such +event today and no CentralUI-only seam to subscribe to. Building that event +source is out of scope for a CentralUI-only review. Left Open and surfaced for a +follow-up that adds a deployment-status broadcaster in DeploymentManager (or a +design-doc amendment acknowledging the polling fallback). ### CentralUI-007 — Monitoring nav links to Deployment-only pages are shown to all roles @@ -308,7 +327,7 @@ _Unresolved._ |--|--| | Severity | Medium | | Category | Correctness & logic bugs | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.CentralUI/Components/Layout/NavMenu.razor:69-78`; `src/ScadaLink.CentralUI/Components/Pages/Monitoring/EventLogs.razor:2`; `src/ScadaLink.CentralUI/Components/Pages/Monitoring/ParkedMessages.razor:2` | **Description** @@ -333,7 +352,17 @@ all-roles (it is, per the design). **Resolution** -_Unresolved._ +Resolved 2026-05-16 (commit pending). Confirmed: both `EventLogs.razor` and +`ParkedMessages.razor` carried a bare `[Authorize]`, so any authenticated user +could query site event logs and retry/discard parked messages — contrary to the +design doc's Deployment-Role classification. Both pages now use +`[Authorize(Policy = AuthorizationPolicies.RequireDeployment)]`, and the +"Event Logs" / "Parked Messages" nav links were moved out of the all-roles +Monitoring block into an `` (Health +Dashboard stays all-roles, as the design intends). Regression tests +`MonitoringAuthorizationTests.{EventLogsPage,ParkedMessagesPage}_RequiresDeploymentPolicy` +fail against the pre-fix code and pass after; +`HealthDashboard_IsIntentionallyAllAuthenticatedRoles` guards the all-roles page. ### CentralUI-008 — Audit-log date filters treat browser-local datetimes as UTC @@ -341,7 +370,7 @@ _Unresolved._ |--|--| | Severity | Medium | | Category | Correctness & logic bugs | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.CentralUI/Components/Pages/Monitoring/AuditLog.razor:242-243` | **Description** @@ -364,7 +393,19 @@ time-range filters. **Resolution** -_Unresolved._ +Resolved 2026-05-16 (commit pending). Confirmed: `FetchPage` wrapped the +`datetime-local` value with `new DateTimeOffset(value, TimeSpan.Zero)`, +relabelling the browser-local wall-clock value as UTC and shifting the audit +query window by the user's offset. Added a pure helper +`Components/BrowserTime.LocalInputToUtc(DateTime?, int)` that converts a +local-input value to UTC using the browser's `Date.getTimezoneOffset()`; +`AuditLog.razor` now fetches that offset once via JS interop in +`OnAfterRenderAsync` (defaulting to 0/UTC on prerender or a disconnected +circuit) and runs both `from`/`to` filters through the helper. Regression suite +`BrowserTimeTests` (5 tests) fails against the naive relabelling and passes +after — including `LocalInputToUtc_NonUtcBrowser_DoesNotEqualNaiveRelabelling`, +which pins the exact pre-fix bug. `EventLogs.razor` was checked and has no +time-range filters, so it is unaffected. ### CentralUI-009 — `DebugView` stream callbacks touch a possibly-disposed `ToastNotification` @@ -372,7 +413,7 @@ _Unresolved._ |--|--| | Severity | Medium | | Category | Concurrency & thread safety | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.CentralUI/Components/Pages/Deployment/DebugView.razor:400-409,538-544` | **Description** @@ -395,7 +436,21 @@ callbacks should no-op once disposed. **Resolution** -_Unresolved._ +Resolved 2026-05-16 (commit pending). Confirmed: the `onEvent`/`onTerminated` +callbacks captured `this` and `_toast` and ran on an Akka/gRPC thread with no +disposal guard. Added a `volatile bool _disposed` flag, set first thing in +`Dispose()` before the stream is stopped. Every callback now checks `_disposed` +and no-ops if set; the render dispatch goes through a new `SafeInvokeAsync` +helper that re-checks the flag and swallows `ObjectDisposedException` should the +component be disposed between the guard and the dispatch. Regression tests +`DebugViewDisposalTests.{DebugView_HasDisposalGuardField, +DebugView_Dispose_SetsDisposedFlag_AndIsIdempotent}` pin the observable contract +(the guard field exists; `Dispose()` sets it and is idempotent) — the first +fails against the pre-fix code, which had no `_disposed` field. The Akka-thread +timing race itself is not deterministically reproducible in a unit test: +`DebugStreamService` is a non-virtual concrete class with no seam to inject and +later fire the callbacks, so the closest meaningful tests pin the guard +mechanism rather than the race window. ### CentralUI-010 — `ToastNotification` auto-dismiss continuation runs after component disposal @@ -403,7 +458,7 @@ _Unresolved._ |--|--| | Severity | Medium | | Category | Error handling & resilience | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.CentralUI/Components/Shared/ToastNotification.razor:62-71,90` | **Description** @@ -424,7 +479,21 @@ body in a try/catch for `ObjectDisposedException`. **Resolution** -_Unresolved._ +Resolved 2026-05-16 (commit pending). Confirmed: `AddToast` scheduled +`Task.Delay(...).ContinueWith(...)` with no cancellation and `Dispose()` was an +empty body, so the continuation ran `InvokeAsync(StateHasChanged)` against a +disposed component. Added a `CancellationTokenSource _disposalCts` cancelled in +`Dispose()`; the auto-dismiss is now an `AutoDismissAsync` method that awaits +`Task.Delay(dismissMs, token)`, returns on `OperationCanceledException`, and +wraps the post-delay `InvokeAsync(StateHasChanged)` in a try/catch for +`ObjectDisposedException`. `AddToast` also short-circuits if the component is +already disposed. Regression tests: +`ToastNotificationTests.ShowToast_AfterDisposal_IsNoOp_AndSchedulesNothing` +fails against the pre-fix code (which still added the toast / mis-scheduled +after disposal) and passes after; +`AutoDismiss_AfterDisposal_DoesNotThrowUnobservedException` and +`AutoDismiss_BeforeDisposal_StillRemovesToast` guard the no-throw and +still-works behaviours. ### CentralUI-011 — `DiffDialog` leaves a dangling `TaskCompletionSource` when disposed while open @@ -432,7 +501,7 @@ _Unresolved._ |--|--| | Severity | Medium | | Category | Error handling & resilience | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.CentralUI/Components/Shared/DiffDialog.razor:89-95,151-157` | **Description** @@ -452,7 +521,15 @@ awaiter completes deterministically. **Resolution** -_Unresolved._ +Resolved 2026-05-16 (commit pending). Confirmed: `OpenAsync` returned +`_tcs.Task`, completed only by `Close()`; `DisposeAsync` never touched the TCS, +so disposing the dialog while open left the awaiting caller suspended forever. +`DisposeAsync` now calls `_tcs?.TrySetResult(false)` before unlocking the body, +so a dialog disposed while open resolves its caller to `false` (not confirmed). +Regression test `DiffDialogTests.DisposeAsync_WhileOpen_CompletesPendingTask` +fails against the pre-fix code (the pending task stays `WaitingForActivation`) +and passes after; `Close_CompletesPendingTaskWithTrue` guards the normal close +path. ### CentralUI-012 — N+1 query loading data connections for the Sites page @@ -460,7 +537,7 @@ _Unresolved._ |--|--| | Severity | Medium | | Category | Performance & resource management | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.CentralUI/Components/Pages/Admin/Sites.razor:196-205` | **Description** @@ -479,7 +556,15 @@ summary in a single query. **Resolution** -_Unresolved._ +Resolved 2026-05-16 (commit pending). Confirmed: `LoadDataAsync` looped +`GetDataConnectionsBySiteIdAsync(site.Id)` once per site (N+1). `ISiteRepository` +already exposes `GetAllDataConnectionsAsync()` and `DataConnection` carries a +`SiteId`, so the loop was replaced with a single `GetAllDataConnectionsAsync()` +call grouped client-side by `SiteId` — one query regardless of site count, on +every load and post-delete refresh. Regression tests +`SitesPageTests.{LoadData_FetchesAllConnectionsInOneQuery_NoPerSiteQueries, +LoadData_GroupsConnectionsBySite_AndRendersThem}` fail against the pre-fix code +(`GetDataConnectionsBySiteIdAsync` was called per site) and pass after. ### CentralUI-013 — `ScriptAnalysisService` blocks on async shared-script lookups @@ -487,8 +572,8 @@ _Unresolved._ |--|--| | Severity | Medium | | Category | Concurrency & thread safety | -| Status | Open | -| Location | `src/ScadaLink.CentralUI/ScriptAnalysis/ScriptAnalysisService.cs:951-952` | +| Status | Resolved | +| Location | `src/ScadaLink.CentralUI/ScriptAnalysis/ScriptAnalysisService.cs:951-952` (actual call at `:975`) | **Description** @@ -509,15 +594,27 @@ them synchronously would remove the blocking call. **Resolution** -_Unresolved._ +Resolved 2026-05-16 (commit pending). Confirmed (the sync-over-async call is at +`:975`, not `:951-952` as originally cited — `ResolveCalledShape`'s +`Scripts.CallShared` branch). Took the recommended root-cause fix: `Hover` and +`SignatureHelp` are now `async Task<...>` and `ResolveCalledShape` is +`async Task` which `await`s `_sharedScripts.GetShapesAsync()` +instead of `.GetAwaiter().GetResult()`. The two minimal-API endpoints +(`/hover`, `/signature-help`) were updated to `await` the methods. Regression +suite `ScriptAnalysisAsyncResolveTests` (3 tests): the structural test +`HoverAndSignatureHelp_AreAsync_NotSyncOverAsync` fails against the pre-fix +synchronous signatures, and two behavioural tests resolve shared-script shapes +through a catalog that only completes after `Task.Yield()` (a genuinely async +source). The five existing `Hover`/`SignatureHelp` tests in +`ScriptAnalysisServiceTests` were updated to `await` the now-async methods. ### CentralUI-014 — Test Run side effects (HTTP/SQL/SMTP) fire against production services | | | |--|--| -| Severity | Medium | +| Severity | ~~Medium~~ → Low (re-triaged 2026-05-16 — see Resolution) | | Category | Error handling & resilience | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.CentralUI/ScriptAnalysis/ScriptAnalysisService.cs:254-259`; `src/ScadaLink.CentralUI/ScriptAnalysis/SandboxHostHelpers.cs:26-117` | **Description** @@ -541,7 +638,25 @@ dry-run mode that stubs the helpers, defaulting to dry-run. **Resolution** -_Unresolved._ +Resolved 2026-05-16 (commit pending) — **re-triaged**. Re-verified against the +reviewed commit `9c60592`: the finding's premise that "the blast radius is not +surfaced to the user" is **inaccurate**. Both Test Run surfaces that can produce +real side effects — `SharedScriptForm.razor` and the script Test Run in +`TemplateEdit.razor` — already carry a prominent `Real I/O` badge on the panel +header and an `alert-warning` block stating `External`/`Database`/`Notify` calls +"fire for real … real HTTP, real SQL, real emails. Side effects are permanent" +(present since commit `2951507`, an ancestor of the reviewed commit, confirmed +via `git merge-base`). `ApiMethodForm.razor` (Inbound API kind) has **no** +real-I/O surface at all — `SandboxInboundScriptHost` exposes only +`Parameters`/`Route` (Route throws) — and correctly omits the badge while still +warning. Revealing the panel ("Test Run" toggle) then clicking "Run" is itself a +two-step explicit opt-in. The minimum recommendation is therefore already met; +the optional dry-run mode is a separate feature decision the design doc does not +mandate. Severity re-triaged Medium → Low (intentional, documented, clearly +warned behaviour — not a bug). Regression suite `TestRunWarningTests` (3 tests) +pins the `Real I/O` badge + warning text in `SharedScriptForm`/`TemplateEdit` +and the deliberate absence of the badge in `ApiMethodForm`, so the warning +cannot silently regress. ### CentralUI-015 — `DialogService` continuations resolve off the render thread diff --git a/src/ScadaLink.CentralUI/Components/BrowserTime.cs b/src/ScadaLink.CentralUI/Components/BrowserTime.cs new file mode 100644 index 0000000..c6c3c2e --- /dev/null +++ b/src/ScadaLink.CentralUI/Components/BrowserTime.cs @@ -0,0 +1,41 @@ +namespace ScadaLink.CentralUI.Components; + +/// +/// Converts <input type="datetime-local"> values — which are always +/// expressed in the user's browser-local time zone — into UTC +/// s for querying. +/// +/// CLAUDE.md mandates UTC throughout the system, but a datetime-local +/// value carries no offset, so it must be converted to UTC, not relabelled +/// as UTC. Relabelling (the CentralUI-008 bug) shifts every query window by the +/// user's offset for any non-UTC browser. +/// +/// +public static class BrowserTime +{ + /// + /// Converts a browser-local to UTC using the + /// browser's Date.getTimezoneOffset() result. + /// + /// + /// The wall-clock value from a datetime-local input, or null. + /// + /// + /// The value of JavaScript new Date().getTimezoneOffset(): the number + /// of minutes that, added to local time, yields UTC. It is positive + /// for time zones behind UTC (e.g. +300 for UTC-5) and negative for zones + /// ahead (e.g. -120 for UTC+2). + /// + /// The equivalent instant in UTC, or null when the input is null. + public static DateTimeOffset? LocalInputToUtc(DateTime? localValue, int browserUtcOffsetMinutes) + { + if (localValue is not { } local) + return null; + + // getTimezoneOffset() is defined as (UTC - local) in minutes, so + // UTC = local + offset. + var utc = DateTime.SpecifyKind(local, DateTimeKind.Unspecified) + .AddMinutes(browserUtcOffsetMinutes); + return new DateTimeOffset(utc, TimeSpan.Zero); + } +} diff --git a/src/ScadaLink.CentralUI/Components/Layout/NavMenu.razor b/src/ScadaLink.CentralUI/Components/Layout/NavMenu.razor index 98ca90c..e64f237 100644 --- a/src/ScadaLink.CentralUI/Components/Layout/NavMenu.razor +++ b/src/ScadaLink.CentralUI/Components/Layout/NavMenu.razor @@ -65,17 +65,22 @@ - @* Monitoring — visible to all authenticated users *@ + @* Monitoring — Health Dashboard is all-roles; Event Logs and + Parked Messages are Deployment-role only (Component-CentralUI). *@
  • Health Dashboard
    • -
  • - Event Logs -
    • -
  • - Parked Messages -
    • + + +
  • + Event Logs +
    • +
  • + Parked Messages +
    • +     +     @* Audit Log — Admin only *@ diff --git a/src/ScadaLink.CentralUI/Components/Pages/Admin/Sites.razor b/src/ScadaLink.CentralUI/Components/Pages/Admin/Sites.razor index 6b04d47..3fbfcbb 100644 --- a/src/ScadaLink.CentralUI/Components/Pages/Admin/Sites.razor +++ b/src/ScadaLink.CentralUI/Components/Pages/Admin/Sites.razor @@ -194,15 +194,12 @@ try { _sites = (await SiteRepository.GetAllSitesAsync()).ToList(); - _siteConnections.Clear(); - foreach (var site in _sites) - { - var connections = await SiteRepository.GetDataConnectionsBySiteIdAsync(site.Id); - if (connections.Count > 0) - { - _siteConnections[site.Id] = connections.ToList(); - } - } + + // CentralUI-012: fetch all data connections in one query and group + // them by site, instead of issuing one query per site (N+1). + _siteConnections = (await SiteRepository.GetAllDataConnectionsAsync()) + .GroupBy(c => c.SiteId) + .ToDictionary(g => g.Key, g => g.ToList()); } catch (Exception ex) { diff --git a/src/ScadaLink.CentralUI/Components/Pages/Deployment/DebugView.razor b/src/ScadaLink.CentralUI/Components/Pages/Deployment/DebugView.razor index 38fb1ca..074bc72 100644 --- a/src/ScadaLink.CentralUI/Components/Pages/Deployment/DebugView.razor +++ b/src/ScadaLink.CentralUI/Components/Pages/Deployment/DebugView.razor @@ -293,6 +293,13 @@ private string? _initError; + // CentralUI-009: the stream callbacks (onEvent/onTerminated) run on an + // Akka/gRPC thread and capture `this` and `_toast`. Once the component is + // disposed, an in-flight callback must no-op rather than touch a disposed + // component (InvokeAsync would throw ObjectDisposedException) or a disposed + // ToastNotification. + private volatile bool _disposed; + protected override async Task OnInitializedAsync() { try @@ -396,15 +403,18 @@ _selectedInstanceId, onEvent: evt => { + // CentralUI-009: the component may have been disposed while + // this event was in flight on the Akka/gRPC thread. + if (_disposed) return; switch (evt) { case AttributeValueChanged av: UpsertWithCap(_attributeValues, av.AttributeName, av); - _ = InvokeAsync(StateHasChanged); + SafeInvokeStateHasChanged(); break; case AlarmStateChanged al: UpsertWithCap(_alarmStates, al.AlarmName, al); - _ = InvokeAsync(StateHasChanged); + SafeInvokeStateHasChanged(); break; } }, @@ -412,8 +422,11 @@ { _connected = false; _session = null; - _ = InvokeAsync(() => + // CentralUI-009: skip the toast/render if already disposed. + if (_disposed) return; + _ = SafeInvokeAsync(() => { + if (_disposed) return; _toast.ShowError("Debug stream terminated (site disconnected)."); StateHasChanged(); }); @@ -546,8 +559,31 @@ _ => "—" }; + /// + /// Runs on the render thread, guarded against the + /// component being disposed mid-flight (CentralUI-009): InvokeAsync + /// throws once the circuit is gone. + /// + private async Task SafeInvokeAsync(Action action) + { + if (_disposed) return; + try + { + await InvokeAsync(action); + } + catch (ObjectDisposedException) + { + // Component disposed between the guard and the dispatch — ignore. + } + } + + private void SafeInvokeStateHasChanged() => _ = SafeInvokeAsync(StateHasChanged); + public void Dispose() { + // CentralUI-009: mark disposed first so any in-flight stream callback + // sees the flag and no-ops, then stop the stream synchronously. + _disposed = true; if (_session != null) { DebugStreamService.StopStream(_session.SessionId); diff --git a/src/ScadaLink.CentralUI/Components/Pages/Monitoring/AuditLog.razor b/src/ScadaLink.CentralUI/Components/Pages/Monitoring/AuditLog.razor index aae9e75..96d7108 100644 --- a/src/ScadaLink.CentralUI/Components/Pages/Monitoring/AuditLog.razor +++ b/src/ScadaLink.CentralUI/Components/Pages/Monitoring/AuditLog.razor @@ -1,5 +1,6 @@ @page "/monitoring/audit-log" @using ScadaLink.Security +@using ScadaLink.CentralUI.Components @using ScadaLink.Commons.Entities.Audit @using ScadaLink.Commons.Interfaces.Repositories @attribute [Authorize(Policy = AuthorizationPolicies.RequireAdmin)] @@ -195,6 +196,12 @@ private DateTime? _filterFrom; private DateTime? _filterTo; + // The datetime-local filter inputs are in the browser's local time zone. + // This holds new Date().getTimezoneOffset() so the values are converted to + // UTC (CentralUI-008) rather than relabelled. Until JS interop runs it is 0 + // (UTC), which is a safe default for a UTC server/browser. + private int _browserUtcOffsetMinutes; + private List? _entries; private int _totalCount; private int _page = 1; @@ -209,6 +216,23 @@ private int TotalPages => _pageSize > 0 ? Math.Max(1, (_totalCount + _pageSize - 1) / _pageSize) : 1; private bool HasMore => _page * _pageSize < _totalCount; + protected override async Task OnAfterRenderAsync(bool firstRender) + { + if (!firstRender) return; + try + { + // Date.getTimezoneOffset() returns (UTC - local) in minutes. + _browserUtcOffsetMinutes = await JS.InvokeAsync( + "eval", "new Date().getTimezoneOffset()"); + } + catch (Exception ex) when (ex is JSException or JSDisconnectedException + or InvalidOperationException or TaskCanceledException) + { + // Prerender or a disconnected circuit: fall back to UTC (offset 0). + _browserUtcOffsetMinutes = 0; + } + } + private async Task Search() { _page = 1; @@ -239,8 +263,8 @@ user: string.IsNullOrWhiteSpace(_filterUser) ? null : _filterUser.Trim(), entityType: string.IsNullOrWhiteSpace(_filterEntityType) ? null : _filterEntityType.Trim(), action: string.IsNullOrWhiteSpace(_filterAction) ? null : _filterAction.Trim(), - from: _filterFrom.HasValue ? new DateTimeOffset(_filterFrom.Value, TimeSpan.Zero) : null, - to: _filterTo.HasValue ? new DateTimeOffset(_filterTo.Value, TimeSpan.Zero) : null, + from: BrowserTime.LocalInputToUtc(_filterFrom, _browserUtcOffsetMinutes), + to: BrowserTime.LocalInputToUtc(_filterTo, _browserUtcOffsetMinutes), page: _page, pageSize: _pageSize); diff --git a/src/ScadaLink.CentralUI/Components/Pages/Monitoring/EventLogs.razor b/src/ScadaLink.CentralUI/Components/Pages/Monitoring/EventLogs.razor index b229703..9cfc8ec 100644 --- a/src/ScadaLink.CentralUI/Components/Pages/Monitoring/EventLogs.razor +++ b/src/ScadaLink.CentralUI/Components/Pages/Monitoring/EventLogs.razor @@ -1,5 +1,5 @@ @page "/monitoring/event-logs" -@attribute [Authorize] +@attribute [Authorize(Policy = ScadaLink.Security.AuthorizationPolicies.RequireDeployment)] @using ScadaLink.Commons.Entities.Sites @using ScadaLink.Commons.Interfaces.Repositories @using ScadaLink.Commons.Messages.RemoteQuery diff --git a/src/ScadaLink.CentralUI/Components/Pages/Monitoring/ParkedMessages.razor b/src/ScadaLink.CentralUI/Components/Pages/Monitoring/ParkedMessages.razor index a7db209..a6f632d 100644 --- a/src/ScadaLink.CentralUI/Components/Pages/Monitoring/ParkedMessages.razor +++ b/src/ScadaLink.CentralUI/Components/Pages/Monitoring/ParkedMessages.razor @@ -1,5 +1,5 @@ @page "/monitoring/parked-messages" -@attribute [Authorize] +@attribute [Authorize(Policy = ScadaLink.Security.AuthorizationPolicies.RequireDeployment)] @using ScadaLink.Commons.Entities.Sites @using ScadaLink.Commons.Interfaces.Repositories @using ScadaLink.Commons.Messages.RemoteQuery diff --git a/src/ScadaLink.CentralUI/Components/Shared/DiffDialog.razor b/src/ScadaLink.CentralUI/Components/Shared/DiffDialog.razor index f9824a7..24ed794 100644 --- a/src/ScadaLink.CentralUI/Components/Shared/DiffDialog.razor +++ b/src/ScadaLink.CentralUI/Components/Shared/DiffDialog.razor @@ -150,6 +150,11 @@ public async ValueTask DisposeAsync() { + // CentralUI-011: if the dialog is disposed while still open (the user + // navigated away), complete the pending task so the awaiting caller + // resumes deterministically instead of hanging forever. + _tcs?.TrySetResult(false); + if (_bodyLocked) { await TryUnlockBodyAsync(); diff --git a/src/ScadaLink.CentralUI/Components/Shared/ToastNotification.razor b/src/ScadaLink.CentralUI/Components/Shared/ToastNotification.razor index fb00bb9..c8fc654 100644 --- a/src/ScadaLink.CentralUI/Components/Shared/ToastNotification.razor +++ b/src/ScadaLink.CentralUI/Components/Shared/ToastNotification.razor @@ -30,6 +30,16 @@ private readonly List _toasts = new(); private readonly object _lock = new(); + // Cancels all pending auto-dismiss delays when the component is disposed + // (CentralUI-010) so their continuations never touch a disposed component. + private readonly CancellationTokenSource _disposalCts = new(); + + /// Number of toasts currently displayed. + public int ToastCount + { + get { lock (_lock) { return _toasts.Count; } } + } + public void ShowSuccess(string message, string title = "Success", int? autoDismissMs = null) { AddToast(title, message, ToastType.Success, autoDismissMs); @@ -52,6 +62,9 @@ private void AddToast(string title, string message, ToastType type, int? autoDismissMs) { + // If the component is already disposed, do not add or schedule anything. + if (_disposalCts.IsCancellationRequested) return; + var toast = new ToastItem { Title = title, Message = message, Type = type }; lock (_lock) { @@ -60,14 +73,41 @@ StateHasChanged(); var dismissMs = autoDismissMs ?? DefaultAutoDismissMs; - _ = Task.Delay(dismissMs).ContinueWith(_ => + _ = AutoDismissAsync(toast, dismissMs, _disposalCts.Token); + } + + /// + /// Removes a toast after its dismiss delay. The delay is bound to the + /// component's disposal token (CentralUI-010): if the host page is disposed + /// first, the delay is cancelled and the continuation never touches the + /// disposed component — no escapes. + /// + private async Task AutoDismissAsync(ToastItem toast, int dismissMs, CancellationToken token) + { + try { - lock (_lock) - { - _toasts.Remove(toast); - } - InvokeAsync(StateHasChanged); - }); + await Task.Delay(dismissMs, token); + } + catch (OperationCanceledException) + { + return; + } + + if (token.IsCancellationRequested) return; + + lock (_lock) + { + _toasts.Remove(toast); + } + + try + { + await InvokeAsync(StateHasChanged); + } + catch (ObjectDisposedException) + { + // Component disposed between the token check and the render — ignore. + } } private void Dismiss(ToastItem toast) @@ -87,7 +127,11 @@ _ => "bg-secondary text-white" }; - public void Dispose() { } + public void Dispose() + { + _disposalCts.Cancel(); + _disposalCts.Dispose(); + } private enum ToastType { Success, Error, Warning, Info } diff --git a/src/ScadaLink.CentralUI/ScriptAnalysis/ScriptAnalysisEndpoints.cs b/src/ScadaLink.CentralUI/ScriptAnalysis/ScriptAnalysisEndpoints.cs index 8c538ed..7e31755 100644 --- a/src/ScadaLink.CentralUI/ScriptAnalysis/ScriptAnalysisEndpoints.cs +++ b/src/ScadaLink.CentralUI/ScriptAnalysis/ScriptAnalysisEndpoints.cs @@ -19,11 +19,11 @@ public static class ScriptAnalysisEndpoints group.MapPost("/completions", async (CompletionsRequest req, ScriptAnalysisService svc) => Results.Ok(await svc.CompleteAsync(req))); - group.MapPost("/hover", (HoverRequest req, ScriptAnalysisService svc) => - Results.Ok(svc.Hover(req))); + group.MapPost("/hover", async (HoverRequest req, ScriptAnalysisService svc) => + Results.Ok(await svc.Hover(req))); - group.MapPost("/signature-help", (SignatureHelpRequest req, ScriptAnalysisService svc) => - Results.Ok(svc.SignatureHelp(req))); + group.MapPost("/signature-help", async (SignatureHelpRequest req, ScriptAnalysisService svc) => + Results.Ok(await svc.SignatureHelp(req))); group.MapPost("/format", (FormatRequest req, ScriptAnalysisService svc) => Results.Ok(svc.Format(req))); diff --git a/src/ScadaLink.CentralUI/ScriptAnalysis/ScriptAnalysisService.cs b/src/ScadaLink.CentralUI/ScriptAnalysis/ScriptAnalysisService.cs index c9f8cf3..0606775 100644 --- a/src/ScadaLink.CentralUI/ScriptAnalysis/ScriptAnalysisService.cs +++ b/src/ScadaLink.CentralUI/ScriptAnalysis/ScriptAnalysisService.cs @@ -722,7 +722,7 @@ public class ScriptAnalysisService public InlayHintsResponse InlayHints(InlayHintsRequest request) => new(Array.Empty()); - public HoverResponse Hover(HoverRequest request) + public async Task Hover(HoverRequest request) { var script = TryParse(request.CodeText); if (script == null) return new HoverResponse(null); @@ -762,13 +762,13 @@ public class ScriptAnalysisService var rawName = token.ValueText; if (string.IsNullOrEmpty(rawName)) return new HoverResponse(null); - var shape = ResolveCalledShape( + var shape = await ResolveCalledShape( call, rawName, request.SiblingScripts, request.Children, request.Parent); if (shape == null) return new HoverResponse(null); return new HoverResponse(FormatHover(shape, call)); } - public SignatureHelpResponse SignatureHelp(SignatureHelpRequest request) + public async Task SignatureHelp(SignatureHelpRequest request) { var empty = new SignatureHelpResponse(null, null, 0); var script = TryParse(request.CodeText); @@ -803,7 +803,7 @@ public class ScriptAnalysisService var nameArg = inv.ArgumentList.Arguments[0].Expression as LiteralExpressionSyntax; var scriptName = nameArg?.Token.ValueText ?? ""; - var shape = ResolveCalledShape( + var shape = await ResolveCalledShape( call, scriptName, request.SiblingScripts, request.Children, request.Parent); if (shape == null) return empty; @@ -964,22 +964,35 @@ public class ScriptAnalysisService _ => "script" }; - /// Resolves the called script's shape from the metadata in scope for its kind. - private ScriptShape? ResolveCalledShape( + /// + /// Resolves the called script's shape from the metadata in scope for its kind. + /// CentralUI-013: the shared-script catalog is awaited rather than blocked on + /// with .GetAwaiter().GetResult(), so this method is async — and + /// / are async with it. + /// + private async Task ResolveCalledShape( ScriptCallInfo call, string scriptName, IReadOnlyList? siblings, IReadOnlyList? children, - CompositionContext? parent) => call.Kind switch + CompositionContext? parent) { - ScriptCallKind.Shared => _sharedScripts.GetShapesAsync().GetAwaiter().GetResult() - .FirstOrDefault(s => s.Name == scriptName), - ScriptCallKind.Sibling => siblings?.FirstOrDefault(s => s.Name == scriptName), - ScriptCallKind.Parent => parent?.Scripts.FirstOrDefault(s => s.Name == scriptName), - ScriptCallKind.Child => children?.FirstOrDefault(c => c.Name == call.CompositionName) - ?.Scripts.FirstOrDefault(s => s.Name == scriptName), - _ => null - }; + switch (call.Kind) + { + case ScriptCallKind.Shared: + var shapes = await _sharedScripts.GetShapesAsync(); + return shapes.FirstOrDefault(s => s.Name == scriptName); + case ScriptCallKind.Sibling: + return siblings?.FirstOrDefault(s => s.Name == scriptName); + case ScriptCallKind.Parent: + return parent?.Scripts.FirstOrDefault(s => s.Name == scriptName); + case ScriptCallKind.Child: + return children?.FirstOrDefault(c => c.Name == call.CompositionName) + ?.Scripts.FirstOrDefault(s => s.Name == scriptName); + default: + return null; + } + } /// /// SCADA006 — flag Attributes["typo"], diff --git a/tests/ScadaLink.CentralUI.Tests/Admin/SitesPageTests.cs b/tests/ScadaLink.CentralUI.Tests/Admin/SitesPageTests.cs new file mode 100644 index 0000000..99d0f33 --- /dev/null +++ b/tests/ScadaLink.CentralUI.Tests/Admin/SitesPageTests.cs @@ -0,0 +1,105 @@ +using System.Security.Claims; +using Bunit; +using Microsoft.AspNetCore.Components.Authorization; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Logging.Abstractions; +using Microsoft.Extensions.Options; +using NSubstitute; +using ScadaLink.CentralUI.Components.Shared; +using ScadaLink.Commons.Entities.Sites; +using ScadaLink.Commons.Interfaces.Repositories; +using ScadaLink.Commons.Interfaces.Services; +using ScadaLink.Communication; +using ScadaLink.DeploymentManager; +using SitesPage = ScadaLink.CentralUI.Components.Pages.Admin.Sites; + +namespace ScadaLink.CentralUI.Tests.Admin; + +/// +/// Regression tests for CentralUI-012. The Sites page loaded all sites and then +/// issued GetDataConnectionsBySiteIdAsync once per site (N+1 database +/// round-trips on every load and post-delete refresh). The fix fetches all +/// data connections in a single GetAllDataConnectionsAsync call and +/// groups them client-side. +/// +public class SitesPageTests : BunitContext +{ + private readonly ISiteRepository _siteRepo = Substitute.For(); + + private void RegisterServices() + { + Services.AddSingleton(_siteRepo); + + var comms = new CommunicationService( + Options.Create(new CommunicationOptions()), + NullLogger.Instance); + Services.AddSingleton(comms); + + var artifactSvc = new ArtifactDeploymentService( + _siteRepo, + Substitute.For(), + Substitute.For(), + Substitute.For(), + Substitute.For(), + comms, + Substitute.For(), + Options.Create(new DeploymentManagerOptions()), + NullLogger.Instance); + Services.AddSingleton(artifactSvc); + + Services.AddSingleton(Substitute.For()); + + var identity = new ClaimsIdentity( + new[] { new Claim(ClaimTypes.Name, "admin") }, "TestCookie"); + var authState = new AuthenticationState(new ClaimsPrincipal(identity)); + Services.AddSingleton( + new StubAuthStateProvider(authState)); + } + + private sealed class StubAuthStateProvider : AuthenticationStateProvider + { + private readonly AuthenticationState _state; + public StubAuthStateProvider(AuthenticationState state) => _state = state; + public override Task GetAuthenticationStateAsync() + => Task.FromResult(_state); + } + + private static List Sites(params int[] ids) + => ids.Select(id => new Site($"Site{id}", $"SITE-{id}") { Id = id }).ToList(); + + private static DataConnection Conn(int siteId, string name) + => new(name, "OpcUa", siteId); + + [Fact] + public void LoadData_FetchesAllConnectionsInOneQuery_NoPerSiteQueries() + { + RegisterServices(); + _siteRepo.GetAllSitesAsync().Returns(Sites(1, 2, 3)); + _siteRepo.GetAllDataConnectionsAsync().Returns(new List + { + Conn(1, "c1"), Conn(2, "c2"), Conn(3, "c3"), + }); + + Render(); + + // Regression: exactly one bulk query, and zero per-site queries. + _siteRepo.Received(1).GetAllDataConnectionsAsync(); + _siteRepo.DidNotReceive().GetDataConnectionsBySiteIdAsync(Arg.Any()); + } + + [Fact] + public void LoadData_GroupsConnectionsBySite_AndRendersThem() + { + RegisterServices(); + _siteRepo.GetAllSitesAsync().Returns(Sites(1, 2)); + _siteRepo.GetAllDataConnectionsAsync().Returns(new List + { + Conn(1, "alpha-conn"), Conn(2, "beta-conn"), + }); + + var cut = Render(); + + Assert.Contains("alpha-conn", cut.Markup); + Assert.Contains("beta-conn", cut.Markup); + } +} diff --git a/tests/ScadaLink.CentralUI.Tests/Deployment/DebugViewDisposalTests.cs b/tests/ScadaLink.CentralUI.Tests/Deployment/DebugViewDisposalTests.cs new file mode 100644 index 0000000..a76767f --- /dev/null +++ b/tests/ScadaLink.CentralUI.Tests/Deployment/DebugViewDisposalTests.cs @@ -0,0 +1,104 @@ +using System.Reflection; +using System.Security.Claims; +using Bunit; +using Microsoft.AspNetCore.Components.Authorization; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Logging.Abstractions; +using Microsoft.Extensions.Options; +using NSubstitute; +using ScadaLink.CentralUI.Auth; +using ScadaLink.Commons.Entities.Sites; +using ScadaLink.Commons.Interfaces.Repositories; +using ScadaLink.Communication; +using ScadaLink.Communication.Grpc; +using DebugViewPage = ScadaLink.CentralUI.Components.Pages.Deployment.DebugView; + +namespace ScadaLink.CentralUI.Tests.Deployment; + +/// +/// Regression tests for CentralUI-009. The DebugView stream callbacks +/// (onEvent/onTerminated) run on an Akka/gRPC thread and capture +/// this and _toast. If the user navigates away, an in-flight +/// callback could still call _toast.ShowError(...) / +/// InvokeAsync(StateHasChanged) on a disposed component. The fix adds a +/// _disposed flag checked at the top of every callback, set in +/// Dispose() before the stream is stopped. +/// +/// The Akka-thread timing race itself is not deterministically reproducible in +/// a unit test ( is a non-virtual concrete +/// class with no seam to inject and later fire the callbacks). These tests pin +/// the observable parts of the fix: the component exposes a disposal guard, and +/// disposal is clean and idempotent. +/// +/// +public class DebugViewDisposalTests : BunitContext +{ + private void RegisterServices() + { + // DebugView touches localStorage on render; let bUnit answer loosely. + JSInterop.Mode = JSRuntimeMode.Loose; + + var repo = Substitute.For(); + var siteRepo = Substitute.For(); + siteRepo.GetAllSitesAsync().Returns(new List()); + Services.AddSingleton(repo); + Services.AddSingleton(siteRepo); + + var comms = new CommunicationService( + Options.Create(new CommunicationOptions()), + NullLogger.Instance); + Services.AddSingleton(comms); + + var grpcFactory = new SiteStreamGrpcClientFactory(NullLoggerFactory.Instance); + var debugStream = new DebugStreamService( + comms, Services.BuildServiceProvider(), grpcFactory, + NullLogger.Instance); + Services.AddSingleton(debugStream); + + var identity = new ClaimsIdentity( + new[] { new Claim(ClaimTypes.Name, "deployer") }, "TestCookie"); + var authState = new AuthenticationState(new ClaimsPrincipal(identity)); + var stubAuth = new StubAuthStateProvider(authState); + Services.AddSingleton(stubAuth); + Services.AddScoped(_ => new SiteScopeService(stubAuth)); + } + + private sealed class StubAuthStateProvider : AuthenticationStateProvider + { + private readonly AuthenticationState _state; + public StubAuthStateProvider(AuthenticationState state) => _state = state; + public override Task GetAuthenticationStateAsync() + => Task.FromResult(_state); + } + + [Fact] + public void DebugView_HasDisposalGuardField() + { + // The fix introduces a `_disposed` flag that every stream callback + // checks before touching component state. + var field = typeof(DebugViewPage).GetField( + "_disposed", BindingFlags.Instance | BindingFlags.NonPublic); + + Assert.NotNull(field); + Assert.Equal(typeof(bool), field!.FieldType); + } + + [Fact] + public void DebugView_Dispose_SetsDisposedFlag_AndIsIdempotent() + { + RegisterServices(); + var cut = Render(); + + var field = typeof(DebugViewPage).GetField( + "_disposed", BindingFlags.Instance | BindingFlags.NonPublic)!; + Assert.False((bool)field.GetValue(cut.Instance)!); + + cut.Instance.Dispose(); + Assert.True((bool)field.GetValue(cut.Instance)!, + "Dispose() must set the guard so in-flight callbacks no-op."); + + // Disposing again must not throw (idempotent). + var ex = Record.Exception(() => cut.Instance.Dispose()); + Assert.Null(ex); + } +} diff --git a/tests/ScadaLink.CentralUI.Tests/Design/TestRunWarningTests.cs b/tests/ScadaLink.CentralUI.Tests/Design/TestRunWarningTests.cs new file mode 100644 index 0000000..c5fe5b1 --- /dev/null +++ b/tests/ScadaLink.CentralUI.Tests/Design/TestRunWarningTests.cs @@ -0,0 +1,58 @@ +namespace ScadaLink.CentralUI.Tests.Design; + +/// +/// Regression tests for CentralUI-014. Test Run wires External, +/// Database, and Notify to central's real services, so a Test Run +/// has production-equivalent side effects. The finding asked, at minimum, that +/// this blast radius be surfaced to the user. The Test Run panels in +/// SharedScriptForm and TemplateEdit carry a prominent +/// Real I/O badge and an alert-warning block stating the side +/// effects are real and permanent; ApiMethodForm (Inbound API kind) has +/// no real-I/O surface at all and correctly omits the badge. These tests pin +/// that warning so it cannot silently regress. +/// +public class TestRunWarningTests +{ + private static string SrcRoot + { + get + { + // tests/ScadaLink.CentralUI.Tests/bin/Debug/net10.0 → repo root. + var dir = AppContext.BaseDirectory; + for (var i = 0; i < 6 && dir is not null; i++) + dir = Directory.GetParent(dir)?.FullName; + return Path.Combine(dir!, "src", "ScadaLink.CentralUI", + "Components", "Pages", "Design"); + } + } + + private static string Read(string fileName) + => File.ReadAllText(Path.Combine(SrcRoot, fileName)); + + [Theory] + [InlineData("SharedScriptForm.razor")] + [InlineData("TemplateEdit.razor")] + public void TestRunPanel_WithRealIoSurface_ShowsRealIoBadgeAndWarning(string razorFile) + { + var markup = Read(razorFile); + + // The "Real I/O" badge on the Test Run panel header. + Assert.Contains("Real I/O", markup); + // The explicit warning that side effects hit real systems and are permanent. + Assert.Contains("alert-warning", markup); + Assert.Contains("fire for real", markup); + Assert.Contains("Side effects are permanent", markup); + } + + [Fact] + public void ApiMethodForm_TestRun_HasNoRealIoBadge_BecauseInboundApiHasNoSideEffectSurface() + { + // The Inbound API sandbox host exposes only Parameters / Route (Route + // throws) — there is no External/Database/Notify, so no "Real I/O". + var markup = Read("ApiMethodForm.razor"); + + Assert.DoesNotContain("Real I/O", markup); + // It still warns that Route calls throw. + Assert.Contains("alert-warning", markup); + } +} diff --git a/tests/ScadaLink.CentralUI.Tests/Monitoring/BrowserTimeTests.cs b/tests/ScadaLink.CentralUI.Tests/Monitoring/BrowserTimeTests.cs new file mode 100644 index 0000000..96fd0f1 --- /dev/null +++ b/tests/ScadaLink.CentralUI.Tests/Monitoring/BrowserTimeTests.cs @@ -0,0 +1,67 @@ +using ScadaLink.CentralUI.Components; + +namespace ScadaLink.CentralUI.Tests.Monitoring; + +/// +/// Regression tests for CentralUI-008. <input type="datetime-local"> +/// yields the value the user typed in their browser-local time zone. The +/// audit-log filter converted it with new DateTimeOffset(value, TimeSpan.Zero) +/// — relabelling the local wall-clock value as UTC, shifting the query window by +/// the user's offset. performs the +/// correct conversion: it applies the browser offset from getTimezoneOffset(). +/// +public class BrowserTimeTests +{ + [Fact] + public void LocalInputToUtc_Null_ReturnsNull() + { + Assert.Null(BrowserTime.LocalInputToUtc(null, 0)); + } + + [Fact] + public void LocalInputToUtc_UtcBrowser_LeavesTimeUnchanged() + { + // getTimezoneOffset() == 0 for a UTC browser. + var local = new DateTime(2026, 5, 16, 9, 30, 0); + + var utc = BrowserTime.LocalInputToUtc(local, 0); + + Assert.Equal(new DateTimeOffset(2026, 5, 16, 9, 30, 0, TimeSpan.Zero), utc); + } + + [Fact] + public void LocalInputToUtc_PositiveUtcOffsetBrowser_SubtractsOffset() + { + // A browser at UTC+2 reports getTimezoneOffset() == -120. + // The user typing 09:30 local means 07:30 UTC. + var local = new DateTime(2026, 5, 16, 9, 30, 0); + + var utc = BrowserTime.LocalInputToUtc(local, -120); + + Assert.Equal(new DateTimeOffset(2026, 5, 16, 7, 30, 0, TimeSpan.Zero), utc); + } + + [Fact] + public void LocalInputToUtc_NegativeUtcOffsetBrowser_AddsOffset() + { + // A browser at UTC-5 (US Eastern, standard time) reports getTimezoneOffset() == 300. + // The user typing 09:30 local means 14:30 UTC. + var local = new DateTime(2026, 5, 16, 9, 30, 0); + + var utc = BrowserTime.LocalInputToUtc(local, 300); + + Assert.Equal(new DateTimeOffset(2026, 5, 16, 14, 30, 0, TimeSpan.Zero), utc); + } + + [Fact] + public void LocalInputToUtc_NonUtcBrowser_DoesNotEqualNaiveRelabelling() + { + // The pre-fix bug: naive new DateTimeOffset(value, TimeSpan.Zero). + var local = new DateTime(2026, 5, 16, 9, 30, 0); + var naive = new DateTimeOffset(local, TimeSpan.Zero); + + var correct = BrowserTime.LocalInputToUtc(local, 300); + + Assert.NotEqual(naive, correct); + } +} diff --git a/tests/ScadaLink.CentralUI.Tests/Monitoring/MonitoringAuthorizationTests.cs b/tests/ScadaLink.CentralUI.Tests/Monitoring/MonitoringAuthorizationTests.cs new file mode 100644 index 0000000..e5d8162 --- /dev/null +++ b/tests/ScadaLink.CentralUI.Tests/Monitoring/MonitoringAuthorizationTests.cs @@ -0,0 +1,50 @@ +using Microsoft.AspNetCore.Authorization; +using ScadaLink.CentralUI.Components.Pages.Monitoring; +using ScadaLink.Security; + +namespace ScadaLink.CentralUI.Tests.Monitoring; + +/// +/// Regression tests for CentralUI-007. The design doc classifies the Site Event +/// Log Viewer and Parked Message Management as Deployment Role, but both +/// pages were annotated only [Authorize] (any authenticated user) — a +/// non-Deployment user who followed the nav link could query event logs and +/// retry/discard parked messages. The Health Dashboard is intentionally +/// all-roles per the design. +/// +public class MonitoringAuthorizationTests +{ + private static AuthorizeAttribute? AuthorizeOf() + => typeof(TPage) + .GetCustomAttributes(typeof(AuthorizeAttribute), true) + .Cast() + .FirstOrDefault(); + + [Fact] + public void EventLogsPage_RequiresDeploymentPolicy() + { + var attr = AuthorizeOf(); + + Assert.NotNull(attr); + Assert.Equal(AuthorizationPolicies.RequireDeployment, attr!.Policy); + } + + [Fact] + public void ParkedMessagesPage_RequiresDeploymentPolicy() + { + var attr = AuthorizeOf(); + + Assert.NotNull(attr); + Assert.Equal(AuthorizationPolicies.RequireDeployment, attr!.Policy); + } + + [Fact] + public void HealthDashboard_IsIntentionallyAllAuthenticatedRoles() + { + // Health Dashboard stays all-roles (no policy) per the design doc. + var attr = AuthorizeOf(); + + Assert.NotNull(attr); + Assert.Null(attr!.Policy); + } +} diff --git a/tests/ScadaLink.CentralUI.Tests/ScriptAnalysis/ScriptAnalysisAsyncResolveTests.cs b/tests/ScadaLink.CentralUI.Tests/ScriptAnalysis/ScriptAnalysisAsyncResolveTests.cs new file mode 100644 index 0000000..0953ad2 --- /dev/null +++ b/tests/ScadaLink.CentralUI.Tests/ScriptAnalysis/ScriptAnalysisAsyncResolveTests.cs @@ -0,0 +1,91 @@ +using Microsoft.Extensions.Caching.Memory; +using NSubstitute; +using ScadaLink.CentralUI.ScriptAnalysis; +using ScadaLink.TemplateEngine; + +namespace ScadaLink.CentralUI.Tests.ScriptAnalysis; + +/// +/// Regression tests for CentralUI-013. ResolveCalledShape resolved shared +/// script shapes with _sharedScripts.GetShapesAsync().GetAwaiter().GetResult() +/// — a sync-over-async block on a request thread that risks thread-pool +/// starvation and deadlock. Hover and SignatureHelp were synchronous +/// purely to accommodate that block. The fix makes both methods async and +/// awaits the catalog. +/// +public class ScriptAnalysisAsyncResolveTests +{ + private readonly ISharedScriptCatalog _catalog = Substitute.For(); + private readonly IMemoryCache _cache = new MemoryCache(new MemoryCacheOptions { SizeLimit = 100 }); + private readonly IServiceProvider _services = Substitute.For(); + private readonly ScriptAnalysisService _svc; + + public ScriptAnalysisAsyncResolveTests() + { + _catalog.GetShapesAsync().Returns(Array.Empty()); + _svc = new ScriptAnalysisService(_catalog, _cache, _services); + } + + private static ScriptShape Shape(string name, params ParameterShape[] ps) => new(name, ps, null); + private static ParameterShape Param(string name, string type) => new(name, type, true); + + [Fact] + public async Task HoverAsync_OnSharedCallName_AwaitsCatalog_AndResolvesShape() + { + // The catalog only completes after yielding — a truly asynchronous + // source. The fixed Hover awaits it instead of blocking. + _catalog.GetShapesAsync().Returns(async _ => + { + await Task.Yield(); + return (IReadOnlyList)new[] + { + Shape("Aggregate", Param("window", "Integer")), + }; + }); + + var resp = await _svc.Hover(new HoverRequest( + CodeText: "var r = Scripts.CallShared(\"Aggregate\");", + Line: 1, + Column: 30)); + + Assert.NotNull(resp.Markdown); + Assert.Contains("shared script", resp.Markdown); + Assert.Contains("Aggregate", resp.Markdown); + } + + [Fact] + public async Task SignatureHelpAsync_InsideSharedCall_AwaitsCatalog_AndResolvesParameters() + { + _catalog.GetShapesAsync().Returns(async _ => + { + await Task.Yield(); + return (IReadOnlyList)new[] + { + Shape("Aggregate", Param("window", "Integer"), Param("mode", "String")), + }; + }); + + var resp = await _svc.SignatureHelp(new SignatureHelpRequest( + CodeText: "var r = Scripts.CallShared(\"Aggregate\", ", + Line: 1, + Column: 41)); + + Assert.NotNull(resp.Label); + Assert.Contains("Aggregate", resp.Label!); + Assert.Equal(2, resp.Parameters!.Count); + } + + [Fact] + public void HoverAndSignatureHelp_AreAsync_NotSyncOverAsync() + { + // Structural guard: the methods must return Task so the catalog can be + // awaited rather than blocked with .GetAwaiter().GetResult(). + var hover = typeof(ScriptAnalysisService).GetMethod(nameof(ScriptAnalysisService.Hover)); + var sigHelp = typeof(ScriptAnalysisService).GetMethod(nameof(ScriptAnalysisService.SignatureHelp)); + + Assert.NotNull(hover); + Assert.NotNull(sigHelp); + Assert.Equal(typeof(Task), hover!.ReturnType); + Assert.Equal(typeof(Task), sigHelp!.ReturnType); + } +} diff --git a/tests/ScadaLink.CentralUI.Tests/ScriptAnalysis/ScriptAnalysisServiceTests.cs b/tests/ScadaLink.CentralUI.Tests/ScriptAnalysis/ScriptAnalysisServiceTests.cs index 9b88a64..ecca4b1 100644 --- a/tests/ScadaLink.CentralUI.Tests/ScriptAnalysis/ScriptAnalysisServiceTests.cs +++ b/tests/ScadaLink.CentralUI.Tests/ScriptAnalysis/ScriptAnalysisServiceTests.cs @@ -200,11 +200,11 @@ public class ScriptAnalysisServiceTests // ── Hover ───────────────────────────────────────────────────────────── [Fact] - public void Hover_OnSiblingName_ReturnsSignature() + public async Task Hover_OnSiblingName_ReturnsSignature() { var siblings = new[] { Shape("Calc", Param("x", "Integer"), Param("y", "Float")) }; // Cursor inside the "Calc" name literal of Instance.CallScript("Calc", ...). - var resp = _svc.Hover(new HoverRequest( + var resp = await _svc.Hover(new HoverRequest( CodeText: "var r = Instance.CallScript(\"Calc\", 1, 2);", Line: 1, Column: 32, @@ -215,9 +215,9 @@ public class ScriptAnalysisServiceTests } [Fact] - public void Hover_OnUnrelatedToken_ReturnsNull() + public async Task Hover_OnUnrelatedToken_ReturnsNull() { - var resp = _svc.Hover(new HoverRequest( + var resp = await _svc.Hover(new HoverRequest( CodeText: "var r = 1 + 2;", Line: 1, Column: 5)); @@ -227,10 +227,10 @@ public class ScriptAnalysisServiceTests // ── Signature help ──────────────────────────────────────────────────── [Fact] - public void SignatureHelp_InsideCallScript_ReturnsParameterStrip() + public async Task SignatureHelp_InsideCallScript_ReturnsParameterStrip() { var siblings = new[] { Shape("Calc", Param("x", "Integer"), Param("y", "Float")) }; - var resp = _svc.SignatureHelp(new SignatureHelpRequest( + var resp = await _svc.SignatureHelp(new SignatureHelpRequest( CodeText: "var r = Instance.CallScript(\"Calc\", 1, ", Line: 1, Column: 40, @@ -243,9 +243,9 @@ public class ScriptAnalysisServiceTests } [Fact] - public void SignatureHelp_OutsideCall_ReturnsNull() + public async Task SignatureHelp_OutsideCall_ReturnsNull() { - var resp = _svc.SignatureHelp(new SignatureHelpRequest( + var resp = await _svc.SignatureHelp(new SignatureHelpRequest( CodeText: "var r = 1 + 2;", Line: 1, Column: 5)); @@ -394,9 +394,9 @@ public class ScriptAnalysisServiceTests // ── Hover on Parameters["name"] ─────────────────────────────────────── [Fact] - public void Hover_OnParametersKey_ShowsDeclaredType() + public async Task Hover_OnParametersKey_ShowsDeclaredType() { - var resp = _svc.Hover(new HoverRequest( + var resp = await _svc.Hover(new HoverRequest( CodeText: "var x = Parameters[\"name\"];", Line: 1, Column: 22, diff --git a/tests/ScadaLink.CentralUI.Tests/Shared/DiffDialogTests.cs b/tests/ScadaLink.CentralUI.Tests/Shared/DiffDialogTests.cs new file mode 100644 index 0000000..84918d6 --- /dev/null +++ b/tests/ScadaLink.CentralUI.Tests/Shared/DiffDialogTests.cs @@ -0,0 +1,60 @@ +using Bunit; +using ScadaLink.CentralUI.Components.Shared; + +namespace ScadaLink.CentralUI.Tests.Shared; + +/// +/// Regression tests for CentralUI-011. DiffDialog.OpenAsync returns the +/// TaskCompletionSource's task, completed only by Close(). If the +/// user navigated away while the dialog was open, DisposeAsync ran but +/// never completed the TCS — the awaiting caller was suspended forever and any +/// cleanup after the await was skipped. The fix completes the TCS in +/// DisposeAsync. +/// +public class DiffDialogTests : BunitContext +{ + [Fact] + public async Task DisposeAsync_WhileOpen_CompletesPendingTask() + { + var cut = Render(); + + // Open the dialog; the returned task represents the caller's await. + // Block-bodied lambda so InvokeAsync sees a void delegate — it must NOT + // await the dialog's own (deliberately long-lived) task. + Task pending = null!; + await cut.InvokeAsync( + () => { pending = cut.Instance.ShowAsync("Compare", "before", "after"); }); + + Assert.False(pending.IsCompleted, "Dialog task should be pending while open."); + + // Simulate navigating away while the dialog is still open. + await cut.InvokeAsync(async () => await cut.Instance.DisposeAsync()); + + // The awaiter must complete deterministically rather than hang forever. + var completed = await Task.WhenAny(pending, Task.Delay(TimeSpan.FromSeconds(2))); + Assert.Same(pending, completed); + Assert.True(pending.IsCompletedSuccessfully); + var result = await pending; + Assert.False(result, "Dismiss-on-dispose should resolve to false (not confirmed)."); + } + + [Fact] + public async Task Close_CompletesPendingTaskWithTrue() + { + var cut = Render(); + + // Block-bodied lambda so InvokeAsync sees a void delegate — it must NOT + // await the dialog's own (deliberately long-lived) task. + Task pending = null!; + await cut.InvokeAsync( + () => { pending = cut.Instance.ShowAsync("Compare", "before", "after"); }); + + // Closing via the Close button completes the task with true. + await cut.InvokeAsync(() => cut.Find("button.btn-secondary").Click()); + + var completed = await Task.WhenAny(pending, Task.Delay(TimeSpan.FromSeconds(2))); + Assert.Same(pending, completed); + var result = await pending; + Assert.True(result); + } +} diff --git a/tests/ScadaLink.CentralUI.Tests/Shared/ToastNotificationTests.cs b/tests/ScadaLink.CentralUI.Tests/Shared/ToastNotificationTests.cs new file mode 100644 index 0000000..31d61b2 --- /dev/null +++ b/tests/ScadaLink.CentralUI.Tests/Shared/ToastNotificationTests.cs @@ -0,0 +1,80 @@ +using Bunit; +using ScadaLink.CentralUI.Components.Shared; + +namespace ScadaLink.CentralUI.Tests.Shared; + +/// +/// Regression tests for CentralUI-010. ToastNotification.AddToast +/// scheduled Task.Delay(dismissMs).ContinueWith(...) with the result +/// discarded; the continuation called InvokeAsync(StateHasChanged). When +/// the host page is disposed before the delay elapses, the continuation ran +/// against a disposed component and threw ObjectDisposedException on a +/// thread-pool thread with no catch (an unobserved task exception). The fix +/// holds a CancellationTokenSource cancelled in Dispose(). +/// +public class ToastNotificationTests : BunitContext +{ + [Fact] + public async Task ShowToast_AfterDisposal_IsNoOp_AndSchedulesNothing() + { + // Regression: the pre-fix AddToast always added the toast and scheduled + // a Task.Delay continuation, even after Dispose() — the continuation + // then ran InvokeAsync(StateHasChanged) against the disposed component. + // The fix short-circuits AddToast once the disposal token is cancelled. + var cut = Render(); + + await cut.InvokeAsync(() => cut.Instance.Dispose()); + await cut.InvokeAsync(() => cut.Instance.ShowError("after dispose", autoDismissMs: 20)); + + Assert.Equal(0, cut.Instance.ToastCount); + } + + [Fact] + public async Task AutoDismiss_AfterDisposal_DoesNotThrowUnobservedException() + { + var unobserved = new List(); + void Handler(object? s, UnobservedTaskExceptionEventArgs e) + { + unobserved.Add(e.Exception); + e.SetObserved(); + } + TaskScheduler.UnobservedTaskException += Handler; + try + { + var cut = Render(); + // Auto-dismiss after a very short delay so the continuation is + // guaranteed to fire well after we dispose the component. + await cut.InvokeAsync(() => cut.Instance.ShowSuccess("hello", autoDismissMs: 20)); + + // Dispose the component while the auto-dismiss is still pending. + await cut.InvokeAsync(() => cut.Instance.Dispose()); + + // Give the (now-cancelled) auto-dismiss well past its delay. + await Task.Delay(250); + GC.Collect(); + GC.WaitForPendingFinalizers(); + GC.Collect(); + } + finally + { + TaskScheduler.UnobservedTaskException -= Handler; + } + + Assert.Empty(unobserved); + } + + [Fact] + public async Task AutoDismiss_BeforeDisposal_StillRemovesToast() + { + var cut = Render(); + await cut.InvokeAsync(() => cut.Instance.ShowInfo("transient", autoDismissMs: 20)); + + // The toast is visible immediately. + Assert.Contains("transient", cut.Markup); + + // After the dismiss delay it is removed (auto-dismiss still works). + cut.WaitForAssertion( + () => Assert.DoesNotContain("transient", cut.Markup), + timeout: TimeSpan.FromSeconds(2)); + } +}