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
6 changed files with 182 additions and 33 deletions
+11 -1
View File
@@ -30,4 +30,14 @@ When conducting or receiving a code review for NexusReader, ensure the implement
- [ ] **AI Prompts**: Ensure changes to AI logic do not bypass the `PromptRegistry` or token estimation limits defined in `AiSettings`. - [ ] **AI Prompts**: Ensure changes to AI logic do not bypass the `PromptRegistry` or token estimation limits defined in `AiSettings`.
## 6. Code Review Comments ## 6. Code Review Comments
- [ ] **Specific Linking**: Comments should be linked to specific code. Try to avoid general comments about the entire pull request.
### 6.1 Posting Comments
- [ ] **Code-Linked Comments**: Every review comment **must** be anchored to a specific file and line range using the Gitea inline comment API (`path` + `new_line_num`/`old_line_num`). Free-floating general comments are only acceptable for summary notes that cannot be attributed to a single location.
- [ ] **Severity Prefix**: Prefix each comment with its severity so the author can prioritize: `🔴 Blocking`, `🟡 Design/Architecture`, or `🟢 Minor/Suggestion`.
- [ ] **Actionable Guidance**: Each comment must include a concrete, actionable suggestion — not just a description of the problem. Where applicable, provide a corrected code snippet.
### 6.2 Resolving Comments (Author Responsibility)
- [ ] **Reply Before Resolving**: When a review comment has been addressed, the author **must** reply to the specific thread explaining *how* the issue was resolved (e.g., commit SHA, approach taken, or a reasoned rejection with justification). Do not close a thread without a reply.
- [ ] **Link to Fix**: If the resolution is a code change, include the commit SHA or a reference to the changed line in the reply (e.g., `Fixed in abc1234 — moved the guard before CTS allocation`).
- [ ] **Close Only After Reply**: Mark a thread as **Resolved** only after posting the reply. A thread with no reply must remain open, even if the underlying code has changed.
- [ ] **Rejection Must Be Justified**: If the author disagrees with a comment and chooses not to act on it, they must reply with a clear technical justification. The reviewer then decides whether to accept the reasoning and close the thread, or escalate it.
@@ -27,14 +27,21 @@
</div> </div>
<div class="modal-body"> <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="shimmer-content">
<div class="spinner"></div> <div class="spinner"></div>
<p>Scanning metadata...</p> <p>Scanning metadata...</p>
</div> </div>
</div> </div>
<div class="verification-state" style="@(IsVerifying && !IsParsing && !IsIndexing ? "display:flex;" : "display:none;")"> <div class="ingesting-state shimmer" style="@(IsIngesting ? "display:flex;" : "display:none;")">
mjasin marked this conversation as resolved Outdated
Outdated
Review

🟡 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 ? "display:flex;" : "display:none;")">
@if (Metadata != null) @if (Metadata != null)
{ {
<div class="verification-layout"> <div class="verification-layout">
@@ -70,8 +77,8 @@
<div class="actions"> <div class="actions">
<NexusButton Class="btn-secondary" OnClick="Reset" Disabled="IsIngesting">Back</NexusButton> <NexusButton Class="btn-secondary" OnClick="Reset" Disabled="IsIngesting">Back</NexusButton>
<NexusButton Class="@($"btn-primary {(IsIngesting ? "btn-loading" : "")}")" <NexusButton Class="@($"btn-primary {(IsIngesting ? "btn-loading" : "")}")"
mjasin marked this conversation as resolved
Review

🟢 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.
OnClick="SaveToLibrary" OnClick="SaveToLibrary"
Disabled="IsIngesting"> Disabled="IsIngesting">
@(IsIngesting ? "" : "Save to Library") @(IsIngesting ? "" : "Save to Library")
</NexusButton> </NexusButton>
</div> </div>
@@ -79,9 +86,9 @@
</div> </div>
<div class="upload-state @(_isDragging ? "drag-over" : "")" <div class="upload-state @(_isDragging ? "drag-over" : "")"
style="@(!IsParsing && !IsVerifying && !IsIndexing ? "display:flex;" : "display:none;")" style="@(IsUploadActive ? "display:flex;" : "display:none;")"
@ondragenter="OnDragEnter" @ondragenter="OnDragEnter"
@ondragleave="OnDragLeave"> @ondragleave="OnDragLeave">
<div class="drop-zone"> <div class="drop-zone">
<InputFile id="epub-upload" OnChange="HandleFileSelected" accept=".epub" class="file-input-cover" /> <InputFile id="epub-upload" OnChange="HandleFileSelected" accept=".epub" class="file-input-cover" />
<div class="drop-zone-content"> <div class="drop-zone-content">
@@ -143,6 +150,7 @@
private string? ErrorMessage { get; set; } private string? ErrorMessage { get; set; }
private byte[]? _epubBytes; private byte[]? _epubBytes;
private bool _disposed; private bool _disposed;
private bool IsUploadActive => !IsParsing && !IsVerifying && !IsIngesting && !IsIndexing;
// Allow up to 50 MB // Allow up to 50 MB
private const long MaxFileSize = 50 * 1024 * 1024; private const long MaxFileSize = 50 * 1024 * 1024;
@@ -163,6 +171,8 @@
if (!_disposed) 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); await InvokeAsync(StateHasChanged);
} }
@@ -177,6 +187,8 @@
if (IngestedBookId != Guid.Empty) if (IngestedBookId != Guid.Empty)
{ {
var bookId = IngestedBookId; 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 () => { await InvokeAsync(async () => {
if (_disposed) return; if (_disposed) return;
await CloseModal(); await CloseModal();
@@ -118,8 +118,9 @@
z-index: 10; z-index: 10;
} }
/* Parsing State */ /* Parsing and Ingesting States */
.parsing-state { .parsing-state,
.ingesting-state {
flex: 1; flex: 1;
display: flex; display: flex;
justify-content: center; justify-content: center;
@@ -158,7 +159,8 @@
filter: drop-shadow(0 0 8px rgba(0, 255, 153, 0.3)); filter: drop-shadow(0 0 8px rgba(0, 255, 153, 0.3));
} }
.parsing-state p { .parsing-state p,
.ingesting-state p {
color: var(--nexus-text); color: var(--nexus-text);
font-family: var(--nexus-font-mono, monospace); font-family: var(--nexus-font-mono, monospace);
font-size: 0.9rem; font-size: 0.9rem;
@@ -371,10 +373,11 @@
position: absolute; position: absolute;
width: 20px; width: 20px;
height: 20px; height: 20px;
border: 2px solid rgba(0, 0, 0, 0.1); border: 2px solid rgba(255, 255, 255, 0.2);
border-top-color: #000; border-top-color: var(--nexus-neon, #00ffaa);
border-radius: 50%; border-radius: 50%;
animation: spin 0.8s linear infinite; animation: spin 0.8s linear infinite;
filter: drop-shadow(0 0 4px var(--nexus-neon, #00ffaa));
} }
/* Indexing State */ /* Indexing State */
@@ -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() private async Task InitViewportDetectionAsync()
{ {
try try
{ {
_viewportModule = await JS.InvokeAsync<IJSObjectReference>("import", "./_content/NexusReader.UI.Shared/js/viewport.js"); var module = await EnsureViewportModuleAsync();
_selfReference = DotNetObjectReference.Create(this); _selfReference = DotNetObjectReference.Create(this);
var isMobileViewport = await _viewportModule.InvokeAsync<bool>("isMobileViewport"); var isMobileViewport = await module.InvokeAsync<bool>("isMobileViewport");
await OnViewportChanged(isMobileViewport); await OnViewportChanged(isMobileViewport);
await _viewportModule.InvokeVoidAsync("registerViewportObserver", _selfReference); await module.InvokeVoidAsync("registerViewportObserver", _selfReference);
} }
catch (Exception ex) catch (Exception ex)
{ {
1
@@ -348,16 +357,25 @@
_isLoadingChapter = false; _isLoadingChapter = false;
StateHasChanged(); StateHasChanged();
if (result.IsSuccess && !string.IsNullOrEmpty(NavigationService.PendingScrollBlockId)) if (result.IsSuccess)
{ {
var targetBlockId = NavigationService.PendingScrollBlockId; if (!string.IsNullOrEmpty(NavigationService.PendingScrollBlockId))
NavigationService.PendingScrollBlockId = null; // Clear it to prevent multiple scrolls {
_currentActiveBlockId = targetBlockId; 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 // Give the browser slightly more than one frame to render the loaded blocks
await Task.Delay(150); await Task.Delay(150);
await ScrollToNodeAsync(targetBlockId); await ScrollToNodeAsync(targetBlockId);
await InteractionService.RequestHighlightBlock(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
@@ -365,7 +383,7 @@
{ {
try 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); await module.InvokeVoidAsync("scrollIntoView", id);
} }
catch (Exception ex) catch (Exception ex)
@@ -374,6 +392,19 @@
} }
} }
public async Task ScrollToTopAsync()
{
try
{
var module = await EnsureViewportModuleAsync();
await module.InvokeVoidAsync("scrollToTop", ".reader-canvas");
}
catch (Exception ex)
{
Logger.LogWarning(ex, "Failed to scroll reader canvas to top.");
}
}
private Task HandleUpdate() => InvokeAsync(StateHasChanged); private Task HandleUpdate() => InvokeAsync(StateHasChanged);
private void HandleEscape() private void HandleEscape()
@@ -8,7 +8,7 @@ using Microsoft.Extensions.Logging;
namespace NexusReader.UI.Shared.Services; namespace NexusReader.UI.Shared.Services;
public sealed partial class KnowledgeCoordinator : IDisposable public sealed partial class KnowledgeCoordinator : IDisposable, IAsyncDisposable
{ {
private readonly IKnowledgeService _knowledgeService; private readonly IKnowledgeService _knowledgeService;
private readonly IKnowledgeGraphService _graphService; private readonly IKnowledgeGraphService _graphService;
@@ -17,6 +17,9 @@ public sealed partial class KnowledgeCoordinator : IDisposable
private readonly IReaderInteractionService _interactionService; private readonly IReaderInteractionService _interactionService;
private readonly ILogger<KnowledgeCoordinator> _logger; private readonly ILogger<KnowledgeCoordinator> _logger;
private CancellationTokenSource? _graphCts;
private CancellationTokenSource? _quizCts;
public string CurrentFullPageContent { get; private set; } = string.Empty; public string CurrentFullPageContent { get; private set; } = string.Empty;
/// <summary> /// <summary>
@@ -75,9 +78,38 @@ public sealed partial class KnowledgeCoordinator : IDisposable
} }
} }
private void CancelAndDisposeCts(ref CancellationTokenSource? cts)
{
var localCts = cts;
mjasin marked this conversation as resolved Outdated
Outdated
Review

🔴 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) public async Task ProcessFullPageAsync(string fullContent, string tenantId = "global", Guid? ebookId = null)
{ {
if (string.IsNullOrWhiteSpace(fullContent)) return; 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;
CurrentFullPageContent = fullContent; CurrentFullPageContent = fullContent;
LogGeneratingGraph(tenantId); LogGeneratingGraph(tenantId);
@@ -87,7 +119,9 @@ public sealed partial class KnowledgeCoordinator : IDisposable
try try
{ {
var result = await _knowledgeService.GetGraphDataAsync(fullContent, tenantId, ebookId); var result = await _knowledgeService.GetGraphDataAsync(fullContent, tenantId, ebookId, token);
token.ThrowIfCancellationRequested();
if (result.IsSuccess) if (result.IsSuccess)
{ {
var packet = result.Value; var packet = result.Value;
@@ -103,10 +137,17 @@ public sealed partial class KnowledgeCoordinator : IDisposable
await _graphService.SetLoading(false); await _graphService.SetLoading(false);
} }
catch (OperationCanceledException)
{
_logger.LogInformation("[KnowledgeCoordinator] Graph generation task was canceled.");
}
catch (Exception ex) catch (Exception ex)
{ {
await _graphService.SetLoading(false); if (!token.IsCancellationRequested)
LogGraphError(ex, tenantId); {
await _graphService.SetLoading(false);
LogGraphError(ex, tenantId);
}
} }
} }
@@ -118,11 +159,17 @@ public sealed partial class KnowledgeCoordinator : IDisposable
public async Task<Result<KnowledgePacket>> RequestSummaryAndQuizAsync(string content, string tenantId = "global") public async Task<Result<KnowledgePacket>> RequestSummaryAndQuizAsync(string content, string tenantId = "global")
{ {
CancelAndDisposeCts(ref _quizCts);
_quizCts = new CancellationTokenSource();
var token = _quizCts.Token;
await _quizService.SetHydrating(true); await _quizService.SetHydrating(true);
LogRequestingSummary(tenantId); LogRequestingSummary(tenantId);
try try
{ {
var result = await _knowledgeService.GetSummaryAndQuizAsync(content, tenantId); var result = await _knowledgeService.GetSummaryAndQuizAsync(content, tenantId, cancellationToken: token);
token.ThrowIfCancellationRequested();
if (result.IsSuccess) if (result.IsSuccess)
{ {
var packet = result.Value; var packet = result.Value;
@@ -138,10 +185,19 @@ public sealed partial class KnowledgeCoordinator : IDisposable
LogSummaryWarning(tenantId); LogSummaryWarning(tenantId);
return Result.Fail(result.Errors); 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) catch (Exception ex)
{ {
LogSummaryError(ex, tenantId); if (!token.IsCancellationRequested)
return Result.Fail(new Error("Error requesting summary and quiz").CausedBy(ex)); {
LogSummaryError(ex, tenantId);
return Result.Fail(new Error("Error requesting summary and quiz").CausedBy(ex));
}
return Result.Fail("Task canceled");
mjasin marked this conversation as resolved Outdated
Outdated
Review

🔴 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 } ```
Outdated
Review

🟡 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).
} }
finally finally
{ {
@@ -151,6 +207,9 @@ public sealed partial class KnowledgeCoordinator : IDisposable
public async Task ClearAsync() public async Task ClearAsync()
{ {
CancelAndDisposeCts(ref _graphCts);
CancelAndDisposeCts(ref _quizCts);
CurrentFullPageContent = string.Empty; CurrentFullPageContent = string.Empty;
await _graphService.Clear(); await _graphService.Clear();
await _quizService.SetQuiz(null, null); await _quizService.SetQuiz(null, null);
@@ -159,6 +218,27 @@ public sealed partial class KnowledgeCoordinator : IDisposable
public void Dispose() public void Dispose()
{ {
_interactionService.OnNodeSelected -= HandleNodeSelected; _interactionService.OnNodeSelected -= HandleNodeSelected;
CancelAndDisposeCts(ref _graphCts);
CancelAndDisposeCts(ref _quizCts);
}
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}")] [LoggerMessage(Level = LogLevel.Information, Message = "[KnowledgeCoordinator] Generating full page graph for tenant: {TenantId}")]
@@ -38,3 +38,16 @@ export function scrollIntoView(id) {
} }
return false; return false;
} }
// NOTE: Assumes the selector matches the active scroll container (default '.reader-canvas').
mjasin marked this conversation as resolved Outdated
Outdated
Review

🟢 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;
}
return false;
}