mjasin/Nexus.Reader
1
0
Fork 0
Code Issues 9 Pull Requests 2 Actions Packages Projects Releases Wiki Activity

fix(ingest): implement beautiful upload loading state and fix button loading spinner visibility #66

Merged
mjasin merged 4 commits from bugfix/book-upload-indicator into infra/beta-deploy-test 2026-06-01 16:42:09 +00:00
Conversation 2 Commits 4 Files Changed 6
+99 -45
4 changed files with 99 additions and 45 deletions
Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit f066253701 - Show all commits
src/NexusReader.UI.Shared/Components/Organisms/BookIngestionModal.razor
+11 -6
View File
@@ -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;")">
mjasin marked this conversation as resolved Outdated
Antigravity commented 2026-06-01 16:31:29 +00:00
Outdated
Review
Copy Link
Copy Source

🟡 ingesting-state is hidden by the time IsIndexing could be true — redundant guard

The !IsIndexing condition in the ingesting-state visibility expression is always vacuously true during the visible window of this state. In SaveToLibrary, IsIngesting is set to false before IsIndexing is set to true (lines 309-310), so the two flags are mutually exclusive by construction.

Either:

  1. Add a comment: // !IsIndexing is redundant by construction but kept for defensive consistency with parsing-state
  2. Or remove it: style="@(IsIngesting ? "display:flex;" : "display:none;")"

The current state is not a bug, but it creates a false impression that IsIngesting and IsIndexing can both be true simultaneously.

🟡 **`ingesting-state` is hidden by the time `IsIndexing` could be true — redundant guard** The `!IsIndexing` condition in the `ingesting-state` visibility expression is always vacuously true during the visible window of this state. In `SaveToLibrary`, `IsIngesting` is set to `false` *before* `IsIndexing` is set to `true` (lines 309-310), so the two flags are mutually exclusive by construction. Either: 1. Add a comment: `// !IsIndexing is redundant by construction but kept for defensive consistency with parsing-state` 2. Or remove it: `style="@(IsIngesting ? "display:flex;" : "display:none;")"` The current state is not a bug, but it creates a false impression that `IsIngesting` and `IsIndexing` can both be `true` simultaneously.
<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">
1
 @@ -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();
src/NexusReader.UI.Shared/Components/Organisms/ReaderCanvas.razor
+32 -19
View File
@@ -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)
{
1
 @@ -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();
}
}
}
1
 @@ -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)
{
src/NexusReader.UI.Shared/Services/KnowledgeCoordinator.cs
+51 -18
View File
@@ -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;
mjasin marked this conversation as resolved Outdated
Antigravity commented 2026-06-01 16:31:29 +00:00
Outdated
Review
Copy Link
Copy Source

🔴 CTS is created before the early-return guard — empty fullContent leaks a CancellationTokenSource

If fullContent is empty or whitespace, the method exits here after creating a new CancellationTokenSource and assigning it to _graphCts. This is not a permanent leak (the next call cancels and disposes it), but it allocates unnecessarily and leaves the coordinator in a state where _graphService may still be showing a loading indicator from a previous cancelled call.

Move the guard before the CTS setup and the SetLoading(true) call:

public async Task ProcessFullPageAsync(string fullContent, string tenantId = "global", Guid? ebookId = null)
{
    if (string.IsNullOrWhiteSpace(fullContent)) return; // ← move here

    _graphCts?.Cancel();
    _graphCts?.Dispose();
    _graphCts = new CancellationTokenSource();
    var token = _graphCts.Token;

    // ... rest of method
}
🔴 **CTS is created before the early-return guard — empty `fullContent` leaks a `CancellationTokenSource`** If `fullContent` is empty or whitespace, the method exits here after creating a new `CancellationTokenSource` and assigning it to `_graphCts`. This is not a permanent leak (the next call cancels and disposes it), but it allocates unnecessarily and leaves the coordinator in a state where `_graphService` may still be showing a loading indicator from a previous cancelled call. Move the guard **before** the CTS setup and the `SetLoading(true)` call: ```csharp public async Task ProcessFullPageAsync(string fullContent, string tenantId = "global", Guid? ebookId = null) { if (string.IsNullOrWhiteSpace(fullContent)) return; // ← move here _graphCts?.Cancel(); _graphCts?.Dispose(); _graphCts = new CancellationTokenSource(); var token = _graphCts.Token; // ... rest of method } ```
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;
2
 @@ -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}")]
src/NexusReader.UI.Shared/wwwroot/js/viewport.js
+5 -2
View File
@@ -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').
mjasin marked this conversation as resolved Outdated
Antigravity commented 2026-06-01 16:31:29 +00:00
Outdated
Review
Copy Link
Copy Source

🟢 scrollToTop uses a class selector — fragile with multiple .reader-canvas nodes

const el = document.querySelector('.reader-canvas');

querySelector returns the first matching element. If the DOM ever contains more than one .reader-canvas element (e.g. during animated SSR-to-WASM transition or if a second reader is opened), this silently scrolls the wrong container.

Consider scoping the query or accepting an ElementReference from the Blazor side:

export function scrollToTop(elementRef) {
    const el = elementRef ?? document.querySelector('.reader-canvas');
    if (el) {
        el.scrollTop = 0;
        return true;
    }
    return false;
}

At minimum, add a JSDoc comment:

/** Assumes a single .reader-canvas element exists in the DOM. */
export function scrollToTop() {
🟢 **`scrollToTop` uses a class selector — fragile with multiple `.reader-canvas` nodes** ```js const el = document.querySelector('.reader-canvas'); ``` `querySelector` returns the *first* matching element. If the DOM ever contains more than one `.reader-canvas` element (e.g. during animated SSR-to-WASM transition or if a second reader is opened), this silently scrolls the wrong container. Consider scoping the query or accepting an `ElementReference` from the Blazor side: ```js export function scrollToTop(elementRef) { const el = elementRef ?? document.querySelector('.reader-canvas'); if (el) { el.scrollTop = 0; return true; } return false; } ``` At minimum, add a JSDoc comment: ```js /** Assumes a single .reader-canvas element exists in the DOM. */ export function scrollToTop() { ```
// 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;
}