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
+81 -6
3 changed files with 81 additions and 6 deletions
Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit f4ef7ba906 - Show all commits
src/NexusReader.UI.Shared/Components/Organisms/ReaderCanvas.razor
+18
View File
@@ -308,6 +308,11 @@
StatusMessage = "Wczytywanie treści...";
StateHasChanged();
if (string.IsNullOrEmpty(NavigationService.PendingScrollBlockId))
mjasin marked this conversation as resolved Outdated
Antigravity commented 2026-06-01 16:31:29 +00:00
Outdated
Review
Copy Link
Copy Source

🔴 ScrollToTopAsync is called before chapter content is loaded — wrong position in lifecycle

At this point in LoadChapterAsync, ViewModel still holds the previous chapter's content and the DOM hasn't changed yet. Calling scrollToTop() here resets the scroll position of the old chapter, not the new one. When the new chapter eventually renders, the scroll position is uncontrolled.

The scroll-to-top should happen after the new ViewModel is set and the DOM has updated. The correct pattern:

// After StateHasChanged() that sets the new ViewModel...
_isLoadingChapter = false;
StateHasChanged();

// THEN scroll to top — but only if no pending block scroll is set
if (result.IsSuccess && string.IsNullOrEmpty(NavigationService.PendingScrollBlockId))
{
    await Task.Delay(50); // one render tick
    await ScrollToTopAsync();
}

Alternatively, perform the scroll inside OnAfterRenderAsync after the chapter load flag clears.

🔴 **`ScrollToTopAsync` is called before chapter content is loaded — wrong position in lifecycle** At this point in `LoadChapterAsync`, `ViewModel` still holds the *previous* chapter's content and the DOM hasn't changed yet. Calling `scrollToTop()` here resets the scroll position of the old chapter, not the new one. When the new chapter eventually renders, the scroll position is uncontrolled. The scroll-to-top should happen **after** the new `ViewModel` is set and the DOM has updated. The correct pattern: ```csharp // After StateHasChanged() that sets the new ViewModel... _isLoadingChapter = false; StateHasChanged(); // THEN scroll to top — but only if no pending block scroll is set if (result.IsSuccess && string.IsNullOrEmpty(NavigationService.PendingScrollBlockId)) { await Task.Delay(50); // one render tick await ScrollToTopAsync(); } ``` Alternatively, perform the scroll inside `OnAfterRenderAsync` after the chapter load flag clears.
{
await ScrollToTopAsync();
}
var authState = await AuthStateProvider.GetAuthenticationStateAsync();
var userId = authState.User.FindFirst(System.Security.Claims.ClaimTypes.NameIdentifier)?.Value;
@@ -374,6 +379,19 @@
}
}
public async Task ScrollToTopAsync()
mjasin marked this conversation as resolved Outdated
Antigravity commented 2026-06-01 16:31:29 +00:00
Outdated
Review
Copy Link
Copy Source

🔴 Untracked IJSObjectReference created if _viewportModule is null

var module = _viewportModule ?? await JS.InvokeAsync<IJSObjectReference>("import", "...");

If _viewportModule has not been initialized yet (e.g. this method is called before OnAfterRenderAsync(firstRender) runs), a new IJSObjectReference is created and returned, but never stored in _viewportModule and never disposed. In MAUI Hybrid this creates a JS handle leak on every call.

Extract this into a shared helper:

private async Task<IJSObjectReference> EnsureViewportModuleAsync()
{
    _viewportModule ??= await JS.InvokeAsync<IJSObjectReference>("import", "./_content/NexusReader.UI.Shared/js/viewport.js");
    return _viewportModule;
}

Then use var module = await EnsureViewportModuleAsync(); in both ScrollToNodeAsync and ScrollToTopAsync. This same pattern already exists in ScrollToNodeAsync with the identical risk.

🔴 **Untracked `IJSObjectReference` created if `_viewportModule` is null** ```csharp var module = _viewportModule ?? await JS.InvokeAsync<IJSObjectReference>("import", "..."); ``` If `_viewportModule` has not been initialized yet (e.g. this method is called before `OnAfterRenderAsync(firstRender)` runs), a *new* `IJSObjectReference` is created and returned, but **never stored in `_viewportModule` and never disposed**. In MAUI Hybrid this creates a JS handle leak on every call. Extract this into a shared helper: ```csharp private async Task<IJSObjectReference> EnsureViewportModuleAsync() { _viewportModule ??= await JS.InvokeAsync<IJSObjectReference>("import", "./_content/NexusReader.UI.Shared/js/viewport.js"); return _viewportModule; } ``` Then use `var module = await EnsureViewportModuleAsync();` in both `ScrollToNodeAsync` and `ScrollToTopAsync`. This same pattern already exists in `ScrollToNodeAsync` with the identical risk.
{
try
{
var module = _viewportModule ?? await JS.InvokeAsync<IJSObjectReference>("import", "./_content/NexusReader.UI.Shared/js/viewport.js");
await module.InvokeVoidAsync("scrollToTop");
}
catch (Exception ex)
{
Logger.LogWarning(ex, "Failed to scroll reader canvas to top.");
}
}
private Task HandleUpdate() => InvokeAsync(StateHasChanged);
private void HandleEscape()
src/NexusReader.UI.Shared/Services/KnowledgeCoordinator.cs
+53 -6
View File
@@ -16,6 +16,9 @@ public sealed partial class KnowledgeCoordinator : IDisposable
private readonly IPlatformService _platformService;
private readonly IReaderInteractionService _interactionService;
private readonly ILogger<KnowledgeCoordinator> _logger;
private CancellationTokenSource? _graphCts;
private CancellationTokenSource? _quizCts;
public string CurrentFullPageContent { get; private set; } = string.Empty;
@@ -77,6 +80,11 @@ public sealed partial class KnowledgeCoordinator : IDisposable
public async Task ProcessFullPageAsync(string fullContent, string tenantId = "global", Guid? ebookId = null)
{
_graphCts?.Cancel();
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 } ```
_graphCts?.Dispose();
_graphCts = new CancellationTokenSource();
var token = _graphCts.Token;
if (string.IsNullOrWhiteSpace(fullContent)) return;
CurrentFullPageContent = fullContent;
@@ -87,7 +95,9 @@ public sealed partial class KnowledgeCoordinator : IDisposable
try
{
var result = await _knowledgeService.GetGraphDataAsync(fullContent, tenantId, ebookId);
var result = await _knowledgeService.GetGraphDataAsync(fullContent, tenantId, ebookId, token);
token.ThrowIfCancellationRequested();
if (result.IsSuccess)
{
var packet = result.Value;
@@ -103,10 +113,17 @@ public sealed partial class KnowledgeCoordinator : IDisposable
await _graphService.SetLoading(false);
}
catch (OperationCanceledException)
{
_logger.LogInformation("[KnowledgeCoordinator] Graph generation task was canceled.");
}
catch (Exception ex)
{
await _graphService.SetLoading(false);
LogGraphError(ex, tenantId);
if (!token.IsCancellationRequested)
{
await _graphService.SetLoading(false);
LogGraphError(ex, tenantId);
}
}
}
@@ -118,11 +135,18 @@ public sealed partial class KnowledgeCoordinator : IDisposable
public async Task<Result<KnowledgePacket>> RequestSummaryAndQuizAsync(string content, string tenantId = "global")
{
_quizCts?.Cancel();
_quizCts?.Dispose();
_quizCts = new CancellationTokenSource();
var token = _quizCts.Token;
await _quizService.SetHydrating(true);
LogRequestingSummary(tenantId);
try
{
var result = await _knowledgeService.GetSummaryAndQuizAsync(content, tenantId);
var result = await _knowledgeService.GetSummaryAndQuizAsync(content, tenantId, cancellationToken: token);
token.ThrowIfCancellationRequested();
if (result.IsSuccess)
{
var packet = result.Value;
@@ -138,10 +162,19 @@ public sealed partial class KnowledgeCoordinator : IDisposable
LogSummaryWarning(tenantId);
return Result.Fail(result.Errors);
}
catch (OperationCanceledException)
{
_logger.LogInformation("[KnowledgeCoordinator] Quiz and summary generation task was canceled.");
return Result.Fail("Task canceled");
}
catch (Exception ex)
{
LogSummaryError(ex, tenantId);
return Result.Fail(new Error("Error requesting summary and quiz").CausedBy(ex));
if (!token.IsCancellationRequested)
{
LogSummaryError(ex, tenantId);
return Result.Fail(new Error("Error requesting summary and quiz").CausedBy(ex));
}
return Result.Fail("Task canceled");
}
finally
{
@@ -151,6 +184,14 @@ public sealed partial class KnowledgeCoordinator : IDisposable
public async Task ClearAsync()
{
_graphCts?.Cancel();
_graphCts?.Dispose();
_graphCts = null;
_quizCts?.Cancel();
_quizCts?.Dispose();
_quizCts = null;
CurrentFullPageContent = string.Empty;
await _graphService.Clear();
await _quizService.SetQuiz(null, null);
@@ -159,6 +200,12 @@ public sealed partial class KnowledgeCoordinator : IDisposable
public void Dispose()
mjasin marked this conversation as resolved Outdated
Antigravity commented 2026-06-01 16:31:29 +00:00
Outdated
Review
Copy Link
Copy Source

🔴 Dispose() does not null _graphCts/_quizCts — double-disposal race with ClearAsync

ClearAsync cancels and disposes both CTS fields and sets them to null. Dispose() cancels and disposes them but does not set them to null. If a component calls await coordinator.ClearAsync() and then coordinator.Dispose() (or vice versa, e.g. during hot-reload or navigation teardown), the second call will attempt to .Cancel() an already-disposed CancellationTokenSource, which throws ObjectDisposedException.

Fix:

public void Dispose()
{
    _interactionService.OnNodeSelected -= HandleNodeSelected;

    _graphCts?.Cancel();
    _graphCts?.Dispose();
    _graphCts = null; // ← add this

    _quizCts?.Cancel();
    _quizCts?.Dispose();
    _quizCts = null; // ← add this
}
🔴 **`Dispose()` does not null `_graphCts`/`_quizCts` — double-disposal race with `ClearAsync`** `ClearAsync` cancels and disposes both CTS fields and sets them to `null`. `Dispose()` cancels and disposes them but does **not** set them to `null`. If a component calls `await coordinator.ClearAsync()` and then `coordinator.Dispose()` (or vice versa, e.g. during hot-reload or navigation teardown), the second call will attempt to `.Cancel()` an already-disposed `CancellationTokenSource`, which throws `ObjectDisposedException`. Fix: ```csharp public void Dispose() { _interactionService.OnNodeSelected -= HandleNodeSelected; _graphCts?.Cancel(); _graphCts?.Dispose(); _graphCts = null; // ← add this _quizCts?.Cancel(); _quizCts?.Dispose(); _quizCts = null; // ← add this } ```
Antigravity commented 2026-06-01 16:31:29 +00:00
Outdated
Review
Copy Link
Copy Source

🟡 KnowledgeCoordinator should implement IAsyncDisposable, not just IDisposable

This class owns two async operations (ProcessFullPageAsync, RequestSummaryAndQuizAsync) and an async cleanup path (ClearAsync). Implementing only IDisposable means Dispose() cannot await ClearAsync() — it can only cancel the CTS and skip the _graphService.Clear() / _quizService.SetQuiz(null, null) awaitable teardown.

If a hosting component (ReaderCanvas) disposes this coordinator synchronously, the graph panel may remain in a stale loading state.

Recommended fix:

public sealed partial class KnowledgeCoordinator : IAsyncDisposable
{
    public async ValueTask DisposeAsync()
    {
        _interactionService.OnNodeSelected -= HandleNodeSelected;
        await ClearAsync();
    }
}

Remove the synchronous Dispose() method and update any DI registrations to AddScoped<KnowledgeCoordinator>() (which supports IAsyncDisposable in Blazor's DI container).

🟡 **`KnowledgeCoordinator` should implement `IAsyncDisposable`, not just `IDisposable`** This class owns two async operations (`ProcessFullPageAsync`, `RequestSummaryAndQuizAsync`) and an async cleanup path (`ClearAsync`). Implementing only `IDisposable` means `Dispose()` cannot `await ClearAsync()` — it can only cancel the CTS and skip the `_graphService.Clear()` / `_quizService.SetQuiz(null, null)` awaitable teardown. If a hosting component (`ReaderCanvas`) disposes this coordinator synchronously, the graph panel may remain in a stale loading state. Recommended fix: ```csharp public sealed partial class KnowledgeCoordinator : IAsyncDisposable { public async ValueTask DisposeAsync() { _interactionService.OnNodeSelected -= HandleNodeSelected; await ClearAsync(); } } ``` Remove the synchronous `Dispose()` method and update any DI registrations to `AddScoped<KnowledgeCoordinator>()` (which supports `IAsyncDisposable` in Blazor's DI container).
{
_interactionService.OnNodeSelected -= HandleNodeSelected;
_graphCts?.Cancel();
_graphCts?.Dispose();
_quizCts?.Cancel();
_quizCts?.Dispose();
}
[LoggerMessage(Level = LogLevel.Information, Message = "[KnowledgeCoordinator] Generating full page graph for tenant: {TenantId}")]
src/NexusReader.UI.Shared/wwwroot/js/viewport.js
+10
View File
@@ -38,3 +38,13 @@ export function scrollIntoView(id) {
}
return false;
}
export function scrollToTop() {
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() { ```
const el = document.querySelector('.reader-canvas');
if (el) {
el.scrollTop = 0;
return true;
}
return false;
}