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
+182 -33
Antigravity commented 2026-06-01 16:18:38 +00:00
Collaborator
Copy Link
Copy Source

This Pull Request fixes the book upload dialog box's lack of visual feedback during the file ingestion process.

Key Changes:

  1. Interactive ingesting-state Loading State: Added a dedicated, beautiful neon-green loader that displays Saving book to library... with a pulsing neon spinner while the HTTP POST upload request is in flight. This cleanly replaces the form during ingestion, preventing users from clicking disabled/unresponsive inputs or submitting twice.
  2. Premium Glowing Button Loader: Updated .btn-loading styling in BookIngestionModal.razor.css to use a high-contrast white-and-neon-green glowing spinner (border-top-color: var(--nexus-neon)) instead of an invisible black #000 spinner on a faded, disabled dark button.
  3. Build & Test Verified: Successfully verified that the entire solution compiles cleanly with 0 compilation errors (dotnet build) and that all unit/integration tests continue to pass perfectly (dotnet test).
This Pull Request fixes the book upload dialog box's lack of visual feedback during the file ingestion process. ### Key Changes: 1. **Interactive `ingesting-state` Loading State**: Added a dedicated, beautiful neon-green loader that displays `Saving book to library...` with a pulsing neon spinner while the HTTP POST upload request is in flight. This cleanly replaces the form during ingestion, preventing users from clicking disabled/unresponsive inputs or submitting twice. 2. **Premium Glowing Button Loader**: Updated `.btn-loading` styling in `BookIngestionModal.razor.css` to use a high-contrast white-and-neon-green glowing spinner (`border-top-color: var(--nexus-neon)`) instead of an invisible black `#000` spinner on a faded, disabled dark button. 3. **Build & Test Verified**: Successfully verified that the entire solution compiles cleanly with 0 compilation errors (`dotnet build`) and that all unit/integration tests continue to pass perfectly (`dotnet test`).
Antigravity added 1 commit 2026-06-01 16:18:38 +00:00
fix(ingest): implement beautiful upload loading state and fix button loading spinner visibility f81b2acc40
mjasin added 1 commit 2026-06-01 16:27:34 +00:00
ux(reader): implement scroll-to-top on chapter switch and cancel stale background AI tasks f4ef7ba906
Antigravity reviewed 2026-06-01 16:31:29 +00:00
Antigravity left a comment
Author
Collaborator
Copy Link
Copy Source

Code Review — PR #66: Upload Loading State + KnowledgeCoordinator Cancellation + Scroll-to-Top

This PR bundles three independent concerns: (1) the upload loading state UX in BookIngestionModal, (2) cancellation token support in KnowledgeCoordinator, and (3) the scrollToTop JS function + Blazor bridge in ReaderCanvas. All three are well-motivated. The visual upgrade to the button spinner (neon glow replacing the invisible black spinner) and the CancellationTokenSource lifecycle management are both solid additions.

A few issues need resolution before merge, and a handful of smaller items are worth discussing.

🔴 Blocking Issues

1. ScrollToTopAsync is called before the DOM is ready

In LoadChapterAsync, ScrollToTopAsync() is invoked before the await Mediator.Send(...) call that loads the new chapter content. At this point the old DOM (previous chapter's .reader-canvas) is still in the DOM. If the scroll succeeds it will visually reset the old content, only for the new content to be rendered in an unknown scroll position afterward. The correct location is after the new ViewModel has been set and StateHasChanged() has been called (ideally inside an OnAfterRenderAsync pass), not before the chapter data arrives.

2. _graphCts is disposed before the early-return guard in ProcessFullPageAsync

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

if (string.IsNullOrWhiteSpace(fullContent)) return;  // ← exits here without setting _graphCts = null

If fullContent is empty, the method returns leaving _graphCts holding a freshly allocated (and now orphaned) CancellationTokenSource. The next call to ProcessFullPageAsync will cancel and dispose it correctly, so there is no permanent leak — but ClearAsync() + Dispose() both cancel and dispose _graphCts, meaning a CancellationTokenSource cancelled by ClearAsync followed immediately by Dispose() will attempt to cancel an already-cancelled source. More importantly, the early return should either be moved before the CTS setup, or _graphCts should be set to null on the early exit path.

3. Double-disposal race in KnowledgeCoordinator.Dispose

Dispose() cancels and disposes _graphCts, but does not set it to null. If ClearAsync() is awaited concurrently (e.g. from a Blazor component teardown racing with a background graph task), both ClearAsync and Dispose will attempt to cancel and dispose the same CancellationTokenSource, which throws ObjectDisposedException. Set _graphCts = null and _quizCts = null after disposal in Dispose().

🟡 Design / Architecture Concerns

4. ingesting-state display logic is inconsistent with parsing-state

parsing-state condition:

style="@(IsParsing && !IsIndexing ? "display:flex;" : "display:none;")"

ingesting-state condition:

style="@(IsIngesting && !IsIndexing ? "display:flex;" : "display:none;")"

But IsIngesting is set to true at the start of SaveToLibrary, then set to false when IsIndexing = true is set (line 309-310). This means ingesting-state is already hidden by the time IsIndexing becomes true, so the !IsIndexing guard is always vacuously true during the window when ingesting-state is shown. The guard is harmless but misleading — either add a comment explaining why it is there, or remove it to align with the intent.

5. btn-loading class is applied while ingesting-state replaces the form

When IsIngesting is true, the verification-state div is hidden (!IsIngesting in condition), but the "Save to Library" button inside it has btn-loading applied. Since the button is inside a hidden display:none element, this CSS is applied to invisible DOM. The spinner animation runs needlessly, and any CSS filter (e.g. the new drop-shadow) will trigger the GPU compositor for an invisible element. Consider only applying btn-loading when the element is visible, or restructure so that the spinner lives exclusively in ingesting-state.

6. KnowledgeCoordinator is sealed but IDisposable, not IAsyncDisposable

ProcessFullPageAsync and RequestSummaryAndQuizAsync both await async operations. ClearAsync() is also async. Yet the class only implements IDisposable. The Dispose() method calls _graphCts?.Cancel() and _graphCts?.Dispose() synchronously, which is fine for CancellationTokenSource, but it cannot properly await the _graphService.Clear() call that ClearAsync() makes. If a component disposes KnowledgeCoordinator, in-flight graph/quiz state may linger in the UI. Consider implementing IAsyncDisposable and calling await ClearAsync() inside DisposeAsync().

🟢 Minor / Suggestions

7. scrollToTop uses a class selector that could match multiple elements

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

document.querySelector returns the first match. If a future layout ever renders more than one .reader-canvas node (e.g. during SSR pre-render or animated transitions), this will silently scroll the wrong element. Consider accepting an element reference via a DotNetObjectReference or scoping the query to a known container. At minimum, add a comment noting the assumption.

8. ScrollToTopAsync re-imports the JS module if _viewportModule is null

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

This pattern is shared with ScrollToNodeAsync. If _viewportModule is null (before OnAfterRenderAsync(firstRender) fires), this creates a second, untracked IJSObjectReference that is never stored or disposed. In MAUI Hybrid this causes a JS module handle leak. Extract the module load into a shared EnsureViewportModuleAsync() helper that stores the result in _viewportModule, so every call path uses the same cached reference.

9. ProcessFullPageAsync leaves _graphService in loading state on early return

_graphCts = new CancellationTokenSource();
var token = _graphCts.Token;

if (string.IsNullOrWhiteSpace(fullContent)) return; // ← _graphService still shows loading if a previous call set it

If a previous non-empty call ran await _graphService.SetLoading(true) and was then cancelled, a subsequent call with empty fullContent will return without calling SetLoading(false), leaving the graph panel stuck in the loading state. Move the early-return before the SetLoading(true) call, or call await _graphService.SetLoading(false) on the early-return path.

10. Missing StateHasChanged() after setting IsIngesting = true during @onclick path

In SaveToLibrary:

IsIngesting = true;
ErrorMessage = null;
StateHasChanged(); // ← called here

This is correct. However, StateHasChanged() is called synchronously before the await Http.PostAsJsonAsync(...), which is fine. But be aware that on the MAUI Hybrid host the Blazor dispatcher may batch this with the preceding render tick — the ingesting-state shimmer might not be visible for the first ~16ms frame. This is not a bug, just a nuance worth a comment if users report the spinner feeling "late" on slow devices.

## Code Review — PR #66: Upload Loading State + KnowledgeCoordinator Cancellation + Scroll-to-Top This PR bundles three independent concerns: (1) the upload loading state UX in `BookIngestionModal`, (2) cancellation token support in `KnowledgeCoordinator`, and (3) the `scrollToTop` JS function + Blazor bridge in `ReaderCanvas`. All three are well-motivated. The visual upgrade to the button spinner (neon glow replacing the invisible black spinner) and the `CancellationTokenSource` lifecycle management are both solid additions. A few issues need resolution before merge, and a handful of smaller items are worth discussing. --- ### 🔴 Blocking Issues **1. `ScrollToTopAsync` is called before the DOM is ready** In `LoadChapterAsync`, `ScrollToTopAsync()` is invoked *before* the `await Mediator.Send(...)` call that loads the new chapter content. At this point the old DOM (previous chapter's `.reader-canvas`) is still in the DOM. If the scroll succeeds it will visually reset the old content, only for the new content to be rendered in an unknown scroll position afterward. The correct location is *after* the new `ViewModel` has been set and `StateHasChanged()` has been called (ideally inside an `OnAfterRenderAsync` pass), not before the chapter data arrives. **2. `_graphCts` is disposed before the early-return guard in `ProcessFullPageAsync`** ```csharp _graphCts?.Cancel(); _graphCts?.Dispose(); _graphCts = new CancellationTokenSource(); var token = _graphCts.Token; if (string.IsNullOrWhiteSpace(fullContent)) return; // ← exits here without setting _graphCts = null ``` If `fullContent` is empty, the method returns leaving `_graphCts` holding a freshly allocated (and now orphaned) `CancellationTokenSource`. The next call to `ProcessFullPageAsync` will cancel and dispose it correctly, so there is no permanent leak — but `ClearAsync()` + `Dispose()` both cancel and dispose `_graphCts`, meaning a `CancellationTokenSource` cancelled by `ClearAsync` followed immediately by `Dispose()` will attempt to cancel an already-cancelled source. More importantly, the early return should either be moved *before* the CTS setup, or `_graphCts` should be set to `null` on the early exit path. **3. Double-disposal race in `KnowledgeCoordinator.Dispose`** `Dispose()` cancels and disposes `_graphCts`, but does not set it to `null`. If `ClearAsync()` is awaited concurrently (e.g. from a Blazor component teardown racing with a background graph task), both `ClearAsync` and `Dispose` will attempt to cancel and dispose the same `CancellationTokenSource`, which throws `ObjectDisposedException`. Set `_graphCts = null` and `_quizCts = null` after disposal in `Dispose()`. --- ### 🟡 Design / Architecture Concerns **4. `ingesting-state` display logic is inconsistent with `parsing-state`** `parsing-state` condition: ```razor style="@(IsParsing && !IsIndexing ? "display:flex;" : "display:none;")" ``` `ingesting-state` condition: ```razor style="@(IsIngesting && !IsIndexing ? "display:flex;" : "display:none;")" ``` But `IsIngesting` is set to `true` at the *start* of `SaveToLibrary`, then set to `false` when `IsIndexing = true` is set (line 309-310). This means `ingesting-state` is already hidden by the time `IsIndexing` becomes `true`, so the `!IsIndexing` guard is always vacuously true during the window when `ingesting-state` is shown. The guard is harmless but misleading — either add a comment explaining why it is there, or remove it to align with the intent. **5. `btn-loading` class is applied while `ingesting-state` replaces the form** When `IsIngesting` is `true`, the `verification-state` div is hidden (`!IsIngesting` in condition), but the "Save to Library" button inside it has `btn-loading` applied. Since the button is inside a hidden `display:none` element, this CSS is applied to invisible DOM. The spinner animation runs needlessly, and any CSS `filter` (e.g. the new `drop-shadow`) will trigger the GPU compositor for an invisible element. Consider only applying `btn-loading` when the element is visible, or restructure so that the spinner lives exclusively in `ingesting-state`. **6. `KnowledgeCoordinator` is `sealed` but `IDisposable`, not `IAsyncDisposable`** `ProcessFullPageAsync` and `RequestSummaryAndQuizAsync` both `await` async operations. `ClearAsync()` is also async. Yet the class only implements `IDisposable`. The `Dispose()` method calls `_graphCts?.Cancel()` and `_graphCts?.Dispose()` synchronously, which is fine for `CancellationTokenSource`, but it cannot properly await the `_graphService.Clear()` call that `ClearAsync()` makes. If a component disposes `KnowledgeCoordinator`, in-flight graph/quiz state may linger in the UI. Consider implementing `IAsyncDisposable` and calling `await ClearAsync()` inside `DisposeAsync()`. --- ### 🟢 Minor / Suggestions **7. `scrollToTop` uses a class selector that could match multiple elements** ```js const el = document.querySelector('.reader-canvas'); ``` `document.querySelector` returns the *first* match. If a future layout ever renders more than one `.reader-canvas` node (e.g. during SSR pre-render or animated transitions), this will silently scroll the wrong element. Consider accepting an element reference via a `DotNetObjectReference` or scoping the query to a known container. At minimum, add a comment noting the assumption. **8. `ScrollToTopAsync` re-imports the JS module if `_viewportModule` is null** ```csharp var module = _viewportModule ?? await JS.InvokeAsync<IJSObjectReference>("import", "..."); ``` This pattern is shared with `ScrollToNodeAsync`. If `_viewportModule` is null (before `OnAfterRenderAsync(firstRender)` fires), this creates a **second, untracked** `IJSObjectReference` that is never stored or disposed. In MAUI Hybrid this causes a JS module handle leak. Extract the module load into a shared `EnsureViewportModuleAsync()` helper that stores the result in `_viewportModule`, so every call path uses the same cached reference. **9. `ProcessFullPageAsync` leaves `_graphService` in loading state on early return** ```csharp _graphCts = new CancellationTokenSource(); var token = _graphCts.Token; if (string.IsNullOrWhiteSpace(fullContent)) return; // ← _graphService still shows loading if a previous call set it ``` If a previous non-empty call ran `await _graphService.SetLoading(true)` and was then cancelled, a subsequent call with empty `fullContent` will return without calling `SetLoading(false)`, leaving the graph panel stuck in the loading state. Move the early-return *before* the `SetLoading(true)` call, or call `await _graphService.SetLoading(false)` on the early-return path. **10. Missing `StateHasChanged()` after setting `IsIngesting = true` during `@onclick` path** In `SaveToLibrary`: ```csharp IsIngesting = true; ErrorMessage = null; StateHasChanged(); // ← called here ``` This is correct. However, `StateHasChanged()` is called synchronously before the `await Http.PostAsJsonAsync(...)`, which is fine. But be aware that on the MAUI Hybrid host the Blazor dispatcher may batch this with the preceding render tick — the `ingesting-state` shimmer might not be visible for the first ~16ms frame. This is not a bug, just a nuance worth a comment if users report the spinner feeling "late" on slow devices.
src/NexusReader.UI.Shared/Components/Organisms/BookIngestionModal.razor Outdated
@@ -34,2 +34,4 @@
</div>
</div>
<div class="ingesting-state shimmer" style="@(IsIngesting && !IsIndexing ? "display:flex;" : "display:none;")">
Antigravity commented 2026-06-01 16:31:29 +00:00
Author
Collaborator
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.
mjasin marked this conversation as resolved
src/NexusReader.UI.Shared/Components/Organisms/BookIngestionModal.razor
@@ -70,8 +77,8 @@
<div class="actions">
<NexusButton Class="btn-secondary" OnClick="Reset" Disabled="IsIngesting">Back</NexusButton>
<NexusButton Class="@($"btn-primary {(IsIngesting ? "btn-loading" : "")}")"
Antigravity commented 2026-06-01 16:31:29 +00:00
Author
Collaborator
Copy Link
Copy Source

🟢 Button spinner runs inside a display:none parent — unnecessary GPU compositor work

The NexusButton with btn-loading class is inside .verification-state, which is hidden via display:none when IsIngesting is true. The CSS spinner animation (spin 0.8s linear infinite) and the new filter: drop-shadow(...) are applied to DOM nodes that are not rendered to the screen.

While browsers generally optimize display:none subtrees, filter properties can still hold GPU resources depending on the browser's paint/compositor implementation. Since ingesting-state now provides its own dedicated spinner, consider simplifying the button:

<NexusButton Class="btn-primary" OnClick="SaveToLibrary" Disabled="IsIngesting">
    Save to Library
</NexusButton>

The btn-loading state on the button is redundant now that ingesting-state gives full visual feedback.

🟢 **Button spinner runs inside a `display:none` parent — unnecessary GPU compositor work** The `NexusButton` with `btn-loading` class is inside `.verification-state`, which is hidden via `display:none` when `IsIngesting` is true. The CSS spinner animation (`spin 0.8s linear infinite`) and the new `filter: drop-shadow(...)` are applied to DOM nodes that are not rendered to the screen. While browsers generally optimize `display:none` subtrees, `filter` properties can still hold GPU resources depending on the browser's paint/compositor implementation. Since `ingesting-state` now provides its own dedicated spinner, consider simplifying the button: ```razor <NexusButton Class="btn-primary" OnClick="SaveToLibrary" Disabled="IsIngesting"> Save to Library </NexusButton> ``` The `btn-loading` state on the button is redundant now that `ingesting-state` gives full visual feedback.
mjasin marked this conversation as resolved
src/NexusReader.UI.Shared/Components/Organisms/ReaderCanvas.razor Outdated
@@ -308,6 +308,11 @@
StatusMessage = "Wczytywanie treści...";
StateHasChanged();
if (string.IsNullOrEmpty(NavigationService.PendingScrollBlockId))
Antigravity commented 2026-06-01 16:31:29 +00:00
Author
Collaborator
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.
mjasin marked this conversation as resolved
src/NexusReader.UI.Shared/Components/Organisms/ReaderCanvas.razor Outdated
@@ -374,6 +379,19 @@
}
}
public async Task ScrollToTopAsync()
Antigravity commented 2026-06-01 16:31:29 +00:00
Author
Collaborator
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.
mjasin marked this conversation as resolved
src/NexusReader.UI.Shared/Services/KnowledgeCoordinator.cs Outdated
@@ -77,6 +80,11 @@ public sealed partial class KnowledgeCoordinator : IDisposable
public async Task ProcessFullPageAsync(string fullContent, string tenantId = "global", Guid? ebookId = null)
{
_graphCts?.Cancel();
Antigravity commented 2026-06-01 16:31:29 +00:00
Author
Collaborator
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 } ```
mjasin marked this conversation as resolved
src/NexusReader.UI.Shared/Services/KnowledgeCoordinator.cs Outdated
@@ -159,6 +200,12 @@ public sealed partial class KnowledgeCoordinator : IDisposable
public void Dispose()
Antigravity commented 2026-06-01 16:31:29 +00:00
Author
Collaborator
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
Author
Collaborator
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).
mjasin marked this conversation as resolved
src/NexusReader.UI.Shared/wwwroot/js/viewport.js Outdated
@@ -39,2 +39,4 @@
return false;
}
export function scrollToTop() {
Antigravity commented 2026-06-01 16:31:29 +00:00
Author
Collaborator
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() { ```
mjasin marked this conversation as resolved
mjasin added 1 commit 2026-06-01 16:34:01 +00:00
docs: update nexus-code-review guidelines with detailed requirements for posting and resolving comments bd9e9bd036
mjasin added 1 commit 2026-06-01 16:41:06 +00:00
fix(reader): resolve PR #66 code review issues f066253701 
- 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.
Antigravity commented 2026-06-01 16:41:11 +00:00
Author
Collaborator
Copy Link
Copy Source

Code Review Resolutions

Hello! All 10 review items identified in the review have been resolved, build-verified, and fully unit-tested with the latest push:

🔴 Blocking Issues Resolved

  1. ScrollToTopAsync is called before the DOM is ready:

    • Resolution: Shifted the scroll-to-top execution timing to post-render inside LoadChapterAsync (invoked after setting the ViewModel and calling StateHasChanged(), incorporating a 50ms delay to allow the browser a frame to render the new chapter content).

  2. _graphCts is disposed before the early-return guard in ProcessFullPageAsync:

    • Resolution: Repositioned the early-return check to the very beginning of the method. If the page content is empty, it safely calls the CancelAndDisposeCts helper, clears the graph state, ensures the loading spinner is stopped, and exits cleanly.

  3. Double-disposal race in KnowledgeCoordinator.Dispose:

    • Resolution: Implemented the thread-safe CancelAndDisposeCts helper which atomically captures the CTS reference, sets it to null, cancels the source, and disposes of it under a safe try-catch block to eliminate the ObjectDisposedException double-disposal race entirely.

🟡 Design / Architecture Concerns Resolved

  1. ingesting-state display logic is inconsistent with parsing-state:

    • Resolution: Completely simplified state rendering by defining a private IsUploadActive helper property and refactoring the display style of all five step wrappers inside BookIngestionModal.razor to evaluate simple, mutually exclusive single-boolean states.

  2. btn-loading class is applied while ingesting-state replaces the form:

    • Resolution: The mutual exclusivity in step states now ensures that when IsIngesting is true, the verification-state is fully hidden and un-rendered, eliminating redundant hidden GPU compositor and layout calculations.

  3. KnowledgeCoordinator is sealed but IDisposable, not IAsyncDisposable:

    • Resolution: Extended the class to implement IAsyncDisposable. It now safely awaits the async cleanup of dependency services inside DisposeAsync().

🟢 Minor / Suggestions Resolved

  1. scrollToTop uses a class selector that could match multiple elements:

    • Resolution: Updated the JS scrollToTop function inside viewport.js to accept a custom selector string (defaulting to .reader-canvas), added explanatory assumptions comments, and scoped the invocation appropriately.

  2. ScrollToTopAsync re-imports the JS module if _viewportModule is null:

    • Resolution: Introduced a shared EnsureViewportModuleAsync() helper method in ReaderCanvas.razor to guarantee singular import loading and completely avoid untracked reference leaks.

  3. ProcessFullPageAsync leaves _graphService in loading state on early return:

    • Resolution: Updated the early return path to explicitly invoke await _graphService.SetLoading(false), ensuring loading indicators do not hang indefinitely.

  4. Missing StateHasChanged() after setting IsIngesting = true during @onclick path:

    • Resolution: Retained the synchronous rendering logic and added detailed comments in BookIngestionModal.razor describing the dispatcher batching behavior and ensuring future developers are aware of MAUI Hybrid rendering thread nuances.

🧪 Verification

  • Solution built successfully with 0 compiler errors (dotnet build NexusReader.slnx --no-restore).
  • Full test suite passed successfully with 0 failures (20/20 tests passed).
## Code Review Resolutions Hello! All 10 review items identified in the review have been resolved, build-verified, and fully unit-tested with the latest push: ### 🔴 Blocking Issues Resolved 1. **`ScrollToTopAsync` is called before the DOM is ready**: - **Resolution**: Shifted the scroll-to-top execution timing to post-render inside `LoadChapterAsync` (invoked after setting the `ViewModel` and calling `StateHasChanged()`, incorporating a `50ms` delay to allow the browser a frame to render the new chapter content). 2. **`_graphCts` is disposed before the early-return guard in `ProcessFullPageAsync`**: - **Resolution**: Repositioned the early-return check to the very beginning of the method. If the page content is empty, it safely calls the `CancelAndDisposeCts` helper, clears the graph state, ensures the loading spinner is stopped, and exits cleanly. 3. **Double-disposal race in `KnowledgeCoordinator.Dispose`**: - **Resolution**: Implemented the thread-safe `CancelAndDisposeCts` helper which atomically captures the CTS reference, sets it to `null`, cancels the source, and disposes of it under a safe `try-catch` block to eliminate the `ObjectDisposedException` double-disposal race entirely. --- ### 🟡 Design / Architecture Concerns Resolved 4. **`ingesting-state` display logic is inconsistent with `parsing-state`**: - **Resolution**: Completely simplified state rendering by defining a private `IsUploadActive` helper property and refactoring the display style of all five step wrappers inside `BookIngestionModal.razor` to evaluate simple, mutually exclusive single-boolean states. 5. **`btn-loading` class is applied while `ingesting-state` replaces the form**: - **Resolution**: The mutual exclusivity in step states now ensures that when `IsIngesting` is true, the `verification-state` is fully hidden and un-rendered, eliminating redundant hidden GPU compositor and layout calculations. 6. **`KnowledgeCoordinator` is `sealed` but `IDisposable`, not `IAsyncDisposable`**: - **Resolution**: Extended the class to implement `IAsyncDisposable`. It now safely awaits the async cleanup of dependency services inside `DisposeAsync()`. --- ### 🟢 Minor / Suggestions Resolved 7. **`scrollToTop` uses a class selector that could match multiple elements**: - **Resolution**: Updated the JS `scrollToTop` function inside `viewport.js` to accept a custom selector string (defaulting to `.reader-canvas`), added explanatory assumptions comments, and scoped the invocation appropriately. 8. **`ScrollToTopAsync` re-imports the JS module if `_viewportModule` is null**: - **Resolution**: Introduced a shared `EnsureViewportModuleAsync()` helper method in `ReaderCanvas.razor` to guarantee singular import loading and completely avoid untracked reference leaks. 9. **`ProcessFullPageAsync` leaves `_graphService` in loading state on early return**: - **Resolution**: Updated the early return path to explicitly invoke `await _graphService.SetLoading(false)`, ensuring loading indicators do not hang indefinitely. 10. **Missing `StateHasChanged()` after setting `IsIngesting = true` during `@onclick` path**: - **Resolution**: Retained the synchronous rendering logic and added detailed comments in `BookIngestionModal.razor` describing the dispatcher batching behavior and ensuring future developers are aware of MAUI Hybrid rendering thread nuances. --- ### 🧪 Verification - Solution built successfully with **0 compiler errors** (`dotnet build NexusReader.slnx --no-restore`). - Full test suite passed successfully with **0 failures** (`20/20 tests passed`).
mjasin merged commit 036ae26109 into infra/beta-deploy-test 2026-06-01 16:42:09 +00:00
mjasin deleted branch bugfix/book-upload-indicator 2026-06-01 16:42:09 +00:00
Sign in to join this conversation.
Reviewers
No Reviewers
Antigravity
Labels
No items
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
Antigravity M.Wojtaszek mjasin (Marek Jaisński)
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: mjasin/Nexus.Reader#66
Delete Branch "bugfix/book-upload-indicator"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?