From f0662537017bcb8c77c93606b04e310be56ce55d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Marek=20Jasi=C5=84ski?= <jasins.marek@gmail.com>
Date: Mon, 1 Jun 2026 18:41:00 +0200
Subject: [PATCH] fix(reader): resolve PR #66 code review issues

- Shifted ScrollToTopAsync timing to post-render inside LoadChapterAsync.
- Refactored viewport module loading using EnsureViewportModuleAsync to prevent leaks.
- Added selector parameter to scrollToTop in viewport.js.
- Implemented IAsyncDisposable and thread-safe CTS management in KnowledgeCoordinator.
- Fixed early-return loading status and memory leaks in KnowledgeCoordinator.
- Simplified BookIngestionModal state rendering with mutual exclusivity.
- Documented thread-affinity dispatching (InvokeAsync) in BookIngestionModal.
---
 .../Organisms/BookIngestionModal.razor        | 17 +++--
 .../Components/Organisms/ReaderCanvas.razor   | 51 +++++++++-----
 .../Services/KnowledgeCoordinator.cs          | 69 ++++++++++++++-----
 .../wwwroot/js/viewport.js                    |  7 +-
 4 files changed, 99 insertions(+), 45 deletions(-)

diff --git a/src/NexusReader.UI.Shared/Components/Organisms/BookIngestionModal.razor b/src/NexusReader.UI.Shared/Components/Organisms/BookIngestionModal.razor
index 68baf6c..d2abe30 100644
--- a/src/NexusReader.UI.Shared/Components/Organisms/BookIngestionModal.razor
+++ b/src/NexusReader.UI.Shared/Components/Organisms/BookIngestionModal.razor
@@ -27,21 +27,21 @@
             </div>
 
             <div class="modal-body">
-                <div class="parsing-state shimmer" style="@(IsParsing && !IsIndexing ? "display:flex;" : "display:none;")">
+                <div class="parsing-state shimmer" style="@(IsParsing ? "display:flex;" : "display:none;")">
                     <div class="shimmer-content">
                         <div class="spinner"></div>
                         <p>Scanning metadata...</p>
                     </div>
                 </div>
 
-                <div class="ingesting-state shimmer" style="@(IsIngesting && !IsIndexing ? "display:flex;" : "display:none;")">
+                <div class="ingesting-state shimmer" style="@(IsIngesting ? "display:flex;" : "display:none;")">
                     <div class="shimmer-content">
                         <div class="spinner"></div>
                         <p>Saving book to library...</p>
                     </div>
                 </div>
                 
-                <div class="verification-state" style="@(IsVerifying && !IsParsing && !IsIndexing && !IsIngesting ? "display:flex;" : "display:none;")">
+                <div class="verification-state" style="@(IsVerifying ? "display:flex;" : "display:none;")">
                     @if (Metadata != null)
                     {
                         <div class="verification-layout">
@@ -86,9 +86,9 @@
                 </div>
                 
                 <div class="upload-state @(_isDragging ? "drag-over" : "")" 
-                     style="@(!IsParsing && !IsVerifying && !IsIndexing && !IsIngesting ? "display:flex;" : "display:none;")"
-                     @ondragenter="OnDragEnter" 
-                     @ondragleave="OnDragLeave">
+                      style="@(IsUploadActive ? "display:flex;" : "display:none;")"
+                      @ondragenter="OnDragEnter" 
+                      @ondragleave="OnDragLeave">
                     <div class="drop-zone">
                         <InputFile id="epub-upload" OnChange="HandleFileSelected" accept=".epub" class="file-input-cover" />
                         <div class="drop-zone-content">
@@ -150,6 +150,7 @@
     private string? ErrorMessage { get; set; }
     private byte[]? _epubBytes;
     private bool _disposed;
+    private bool IsUploadActive => !IsParsing && !IsVerifying && !IsIngesting && !IsIndexing;
     
     // Allow up to 50 MB
     private const long MaxFileSize = 50 * 1024 * 1024;
@@ -170,6 +171,8 @@
         
         if (!_disposed)
         {
+            // Dispatch the state change to the Blazor synchronization context
+            // because this event is triggered asynchronously from a SignalR / WebSocket background thread.
             await InvokeAsync(StateHasChanged);
         }
 
@@ -184,6 +187,8 @@
             if (IngestedBookId != Guid.Empty)
             {
                 var bookId = IngestedBookId;
+                // Dispatch UI updates and navigation back to the Blazor thread
+                // to avoid thread affinity issues and potential UI lockups in MAUI/Web applications.
                 await InvokeAsync(async () => {
                     if (_disposed) return;
                     await CloseModal();
diff --git a/src/NexusReader.UI.Shared/Components/Organisms/ReaderCanvas.razor b/src/NexusReader.UI.Shared/Components/Organisms/ReaderCanvas.razor
index e45c1ee..98aa195 100644
--- a/src/NexusReader.UI.Shared/Components/Organisms/ReaderCanvas.razor
+++ b/src/NexusReader.UI.Shared/Components/Organisms/ReaderCanvas.razor
@@ -159,15 +159,24 @@
         }
     }
 
+    private async ValueTask<IJSObjectReference> EnsureViewportModuleAsync()
+    {
+        if (_viewportModule == null)
+        {
+            _viewportModule = await JS.InvokeAsync<IJSObjectReference>("import", "./_content/NexusReader.UI.Shared/js/viewport.js");
+        }
+        return _viewportModule;
+    }
+
     private async Task InitViewportDetectionAsync()
     {
         try
         {
-            _viewportModule = await JS.InvokeAsync<IJSObjectReference>("import", "./_content/NexusReader.UI.Shared/js/viewport.js");
+            var module = await EnsureViewportModuleAsync();
             _selfReference = DotNetObjectReference.Create(this);
-            var isMobileViewport = await _viewportModule.InvokeAsync<bool>("isMobileViewport");
+            var isMobileViewport = await module.InvokeAsync<bool>("isMobileViewport");
             await OnViewportChanged(isMobileViewport);
-            await _viewportModule.InvokeVoidAsync("registerViewportObserver", _selfReference);
+            await module.InvokeVoidAsync("registerViewportObserver", _selfReference);
         }
         catch (Exception ex)
         {
@@ -308,11 +317,6 @@
         StatusMessage = "Wczytywanie treści...";
         StateHasChanged();
 
-        if (string.IsNullOrEmpty(NavigationService.PendingScrollBlockId))
-        {
-            await ScrollToTopAsync();
-        }
-
         var authState = await AuthStateProvider.GetAuthenticationStateAsync();
         var userId = authState.User.FindFirst(System.Security.Claims.ClaimTypes.NameIdentifier)?.Value;
 
@@ -353,16 +357,25 @@
         _isLoadingChapter = false;
         StateHasChanged();
 
-        if (result.IsSuccess && !string.IsNullOrEmpty(NavigationService.PendingScrollBlockId))
+        if (result.IsSuccess)
         {
-            var targetBlockId = NavigationService.PendingScrollBlockId;
-            NavigationService.PendingScrollBlockId = null; // Clear it to prevent multiple scrolls
-            _currentActiveBlockId = targetBlockId;
+            if (!string.IsNullOrEmpty(NavigationService.PendingScrollBlockId))
+            {
+                var targetBlockId = NavigationService.PendingScrollBlockId;
+                NavigationService.PendingScrollBlockId = null; // Clear it to prevent multiple scrolls
+                _currentActiveBlockId = targetBlockId;
 
-            // Give the browser slightly more than one frame to render the loaded blocks
-            await Task.Delay(150);
-            await ScrollToNodeAsync(targetBlockId);
-            await InteractionService.RequestHighlightBlock(targetBlockId);
+                // Give the browser slightly more than one frame to render the loaded blocks
+                await Task.Delay(150);
+                await ScrollToNodeAsync(targetBlockId);
+                await InteractionService.RequestHighlightBlock(targetBlockId);
+            }
+            else
+            {
+                // Reset scroll to top now that the new content DOM is rendered
+                await Task.Delay(50); // Give the browser a frame to render the new chapter content
+                await ScrollToTopAsync();
+            }
         }
     }
 
@@ -370,7 +383,7 @@
     {
         try
         {
-            var module = _viewportModule ?? await JS.InvokeAsync<IJSObjectReference>("import", "./_content/NexusReader.UI.Shared/js/viewport.js");
+            var module = await EnsureViewportModuleAsync();
             await module.InvokeVoidAsync("scrollIntoView", id);
         }
         catch (Exception ex)
@@ -383,8 +396,8 @@
     {
         try
         {
-            var module = _viewportModule ?? await JS.InvokeAsync<IJSObjectReference>("import", "./_content/NexusReader.UI.Shared/js/viewport.js");
-            await module.InvokeVoidAsync("scrollToTop");
+            var module = await EnsureViewportModuleAsync();
+            await module.InvokeVoidAsync("scrollToTop", ".reader-canvas");
         }
         catch (Exception ex)
         {
diff --git a/src/NexusReader.UI.Shared/Services/KnowledgeCoordinator.cs b/src/NexusReader.UI.Shared/Services/KnowledgeCoordinator.cs
index dd34ba1..cdf2008 100644
--- a/src/NexusReader.UI.Shared/Services/KnowledgeCoordinator.cs
+++ b/src/NexusReader.UI.Shared/Services/KnowledgeCoordinator.cs
@@ -8,7 +8,7 @@ using Microsoft.Extensions.Logging;
 
 namespace NexusReader.UI.Shared.Services;
 
-public sealed partial class KnowledgeCoordinator : IDisposable
+public sealed partial class KnowledgeCoordinator : IDisposable, IAsyncDisposable
 {
     private readonly IKnowledgeService _knowledgeService;
     private readonly IKnowledgeGraphService _graphService;
@@ -78,15 +78,39 @@ public sealed partial class KnowledgeCoordinator : IDisposable
         }
     }
 
+    private void CancelAndDisposeCts(ref CancellationTokenSource? cts)
+    {
+        var localCts = cts;
+        cts = null;
+        if (localCts != null)
+        {
+            try
+            {
+                localCts.Cancel();
+            }
+            catch (ObjectDisposedException) { }
+            finally
+            {
+                localCts.Dispose();
+            }
+        }
+    }
+
     public async Task ProcessFullPageAsync(string fullContent, string tenantId = "global", Guid? ebookId = null)
     {
-        _graphCts?.Cancel();
-        _graphCts?.Dispose();
+        if (string.IsNullOrWhiteSpace(fullContent))
+        {
+            CancelAndDisposeCts(ref _graphCts);
+            await _graphService.Clear();
+            await _graphService.SetLoading(false);
+            CurrentFullPageContent = string.Empty;
+            return;
+        }
+
+        CancelAndDisposeCts(ref _graphCts);
         _graphCts = new CancellationTokenSource();
         var token = _graphCts.Token;
 
-        if (string.IsNullOrWhiteSpace(fullContent)) return;
-
         CurrentFullPageContent = fullContent;
         LogGeneratingGraph(tenantId);
         
@@ -135,8 +159,7 @@ public sealed partial class KnowledgeCoordinator : IDisposable
 
     public async Task<Result<KnowledgePacket>> RequestSummaryAndQuizAsync(string content, string tenantId = "global")
     {
-        _quizCts?.Cancel();
-        _quizCts?.Dispose();
+        CancelAndDisposeCts(ref _quizCts);
         _quizCts = new CancellationTokenSource();
         var token = _quizCts.Token;
 
@@ -184,13 +207,8 @@ public sealed partial class KnowledgeCoordinator : IDisposable
 
     public async Task ClearAsync()
     {
-        _graphCts?.Cancel();
-        _graphCts?.Dispose();
-        _graphCts = null;
-
-        _quizCts?.Cancel();
-        _quizCts?.Dispose();
-        _quizCts = null;
+        CancelAndDisposeCts(ref _graphCts);
+        CancelAndDisposeCts(ref _quizCts);
 
         CurrentFullPageContent = string.Empty;
         await _graphService.Clear();
@@ -201,11 +219,26 @@ public sealed partial class KnowledgeCoordinator : IDisposable
     {
         _interactionService.OnNodeSelected -= HandleNodeSelected;
 
-        _graphCts?.Cancel();
-        _graphCts?.Dispose();
+        CancelAndDisposeCts(ref _graphCts);
+        CancelAndDisposeCts(ref _quizCts);
+    }
 
-        _quizCts?.Cancel();
-        _quizCts?.Dispose();
+    public async ValueTask DisposeAsync()
+    {
+        _interactionService.OnNodeSelected -= HandleNodeSelected;
+
+        CancelAndDisposeCts(ref _graphCts);
+        CancelAndDisposeCts(ref _quizCts);
+
+        try
+        {
+            await _graphService.Clear();
+            await _quizService.SetQuiz(null, null);
+        }
+        catch (Exception ex)
+        {
+            _logger.LogWarning(ex, "Error clearing services during KnowledgeCoordinator disposal.");
+        }
     }
 
     [LoggerMessage(Level = LogLevel.Information, Message = "[KnowledgeCoordinator] Generating full page graph for tenant: {TenantId}")]
diff --git a/src/NexusReader.UI.Shared/wwwroot/js/viewport.js b/src/NexusReader.UI.Shared/wwwroot/js/viewport.js
index d4cb965..e456f4c 100644
--- a/src/NexusReader.UI.Shared/wwwroot/js/viewport.js
+++ b/src/NexusReader.UI.Shared/wwwroot/js/viewport.js
@@ -39,8 +39,10 @@ export function scrollIntoView(id) {
     return false;
 }
 
-export function scrollToTop() {
-    const el = document.querySelector('.reader-canvas');
+// NOTE: Assumes the selector matches the active scroll container (default '.reader-canvas').
+// Scoping is flexible to avoid issues if SSR pre-render or animated layouts render multiple wrappers.
+export function scrollToTop(selector = '.reader-canvas') {
+    const el = document.querySelector(selector);
     if (el) {
         el.scrollTop = 0;
         return true;
@@ -48,3 +50,4 @@ export function scrollToTop() {
     return false;
 }
 
+