style(ui): refactor reader layout grid, fix focus mode layout collapse, fix SVG rendering dots, reorganize intelligence toolbar #69

2026-06-05T09:28:09Z

Antigravity commented

2026-06-05 09:28:09 +00:00

Reorganized the reader toolbar and layout grid to improve visual consistency and layout robustness in Focus Mode. Fixed outline SVG rendering bugs that caused icons to show as solid dots.

Closes #70

Reorganized the reader toolbar and layout grid to improve visual consistency and layout robustness in Focus Mode. Fixed outline SVG rendering bugs that caused icons to show as solid dots. Closes #70

Antigravity added 9 commits 2026-06-05 09:28:10 +00:00

feat: add CalloutBox component, update ReaderLayout with theme support, and refactor UI styles 01bc8e37b9

style: refine callout box margins, selection popover positioning, and footer hover contrast 900690875f

refactor: redesign selection AI panel to a compact toolbar with independent summary and quiz actions and improved coordinate calculation fdbe901a80

style: set minimum height for reader-flow-container to ensure full-page layout consistency 5daab6ebfd

feat: implement dynamic absolute positioning for the AI selection toolbar via JS to handle scroll and boundary collisions 2ef8dd4066

feat: implement asynchronous text selection summarization with real-time UI updates and skeleton loading states 89fa5cac19

feat: replace CSS spinners with SVG icons and improve disabled button styling in SelectionAiPanel 41cc9f8f21

style: refine premium light mode theme overrides and layout styling 9a978609ae

style(ui): refactor reader layout grid, fix focus mode layout collapse, fix SVG rendering dots, reorganize intelligence toolbar 510c18d526

Antigravity reviewed 2026-06-05 09:31:38 +00:00

Antigravity left a comment

I have conducted a thorough review of PR #69 following the NexusReader code review standards.

While the PR introduces excellent features like Focus Mode visual refactoring, light/dark theme variables support, and background selection summaries, there are a few issues (including memory leaks via globally registered event listeners, layout bugs due to class name mismatches, and CSS isolation violations) that need to be addressed.

I have left detailed inline comments on the respective files with actionable code suggestions.

I have conducted a thorough review of PR #69 following the NexusReader code review standards. While the PR introduces excellent features like Focus Mode visual refactoring, light/dark theme variables support, and background selection summaries, there are a few issues (including memory leaks via globally registered event listeners, layout bugs due to class name mismatches, and CSS isolation violations) that need to be addressed. I have left detailed inline comments on the respective files with actionable code suggestions.

src/NexusReader.UI.Shared/Components/Molecules/IntelligenceToolbar.razor Outdated

						
				@@ -69,3 +62,3 @@

				    private Task HandleUpdate() => InvokeAsync(StateHasChanged);

				    private async Task HandleThemeChangedAsync() => await InvokeAsync(StateHasChanged);

Antigravity commented

2026-06-05 09:31:38 +00:00

🟢 Minor/Suggestion: The method HandleThemeChangedAsync does not need to be marked as async Task since it only awaits a single task and does not contain other logic.

You can simplify it to avoid state machine generation overhead:

private Task HandleThemeChangedAsync() => InvokeAsync(StateHasChanged);

🟢 Minor/Suggestion: The method `HandleThemeChangedAsync` does not need to be marked as `async Task` since it only awaits a single task and does not contain other logic. You can simplify it to avoid state machine generation overhead: ```csharp private Task HandleThemeChangedAsync() => InvokeAsync(StateHasChanged); ```

mjasin marked this conversation as resolved

src/NexusReader.UI.Shared/Components/Organisms/ReaderCanvas.razor

						
				@@ -428,5 +428,5 @@

				    {

				        await InteractionService.RequestAssistant();

				    }

				    public async ValueTask DisposeAsync()

Antigravity commented

2026-06-05 09:31:38 +00:00

🔴 Blocking: Missing teardown for the text selection listener.

In DisposeAsync, you should invoke a teardown function on the selectionHandler.js module to unregister the document event listeners and clean up memory.

Actionable suggestion:

        try
        {
            var module = await JS.InvokeAsync<IJSObjectReference>("import", "./_content/NexusReader.UI.Shared/js/selectionHandler.js");
            await module.InvokeVoidAsync("destroySelectionListener");
        }
        catch (Exception ex)
        {
            Logger.LogDebug(ex, "Failed to destroy JS selection listener.");
        }

🔴 Blocking: Missing teardown for the text selection listener. In `DisposeAsync`, you should invoke a teardown function on the `selectionHandler.js` module to unregister the document event listeners and clean up memory. Actionable suggestion: ```csharp try { var module = await JS.InvokeAsync<IJSObjectReference>("import", "./_content/NexusReader.UI.Shared/js/selectionHandler.js"); await module.InvokeVoidAsync("destroySelectionListener"); } catch (Exception ex) { Logger.LogDebug(ex, "Failed to destroy JS selection listener."); } ```

mjasin marked this conversation as resolved

src/NexusReader.UI.Shared/Services/KnowledgeCoordinator.cs

						
				@@ -208,0 +229,4 @@

				        try

				        {

				            var result = await RequestSummaryAndQuizAsync(text, tenantId);

				            if (result.IsSuccess)

Antigravity commented

2026-06-05 09:31:38 +00:00

🟡 Design/Architecture: If RequestSummaryAndQuizAsync fails, the background task completes, but the loading state is cleared and no error feedback is provided to the UI or logged.

Consider exposing an error state or logging the failure to improve user feedback.

Actionable suggestion:

            var result = await RequestSummaryAndQuizAsync(text, tenantId);
            if (result.IsSuccess)
            {
                SelectionSummary = result.Value.Summary;
            }
            else
            {
                _logger.LogWarning("[KnowledgeCoordinator] Selection summary request failed: {Errors}", string.Join(", ", result.Errors.Select(e => e.Message)));
                // optionally set an error property to show in UI
            }

🟡 Design/Architecture: If `RequestSummaryAndQuizAsync` fails, the background task completes, but the loading state is cleared and no error feedback is provided to the UI or logged. Consider exposing an error state or logging the failure to improve user feedback. Actionable suggestion: ```csharp var result = await RequestSummaryAndQuizAsync(text, tenantId); if (result.IsSuccess) { SelectionSummary = result.Value.Summary; } else { _logger.LogWarning("[KnowledgeCoordinator] Selection summary request failed: {Errors}", string.Join(", ", result.Errors.Select(e => e.Message))); // optionally set an error property to show in UI } ```

mjasin marked this conversation as resolved

src/NexusReader.UI.Shared/wwwroot/app.css

						
				@@ -89,2 +178,4 @@

				}

				/* Scoped Component overrides for Light Mode (Bypassing Blazor CSS isolation) */

				.theme-light .intelligence-toolbar {

Antigravity commented

2026-06-05 09:31:38 +00:00

🟡 Design/Architecture: Global CSS overrides for light-mode component colors (like .theme-light .intelligence-toolbar) violate the architectural separation of concerns.

Component-specific styling should live inside their respective scoped stylesheets (e.g., IntelligenceToolbar.razor.css) rather than the global app.css. Using !important here also creates styling conflicts (e.g., the scoped stylesheet has a background of #ffffff, but this global rule overrides it with #f5f5f4).

Please move these overrides into IntelligenceToolbar.razor.css.

🟡 Design/Architecture: Global CSS overrides for light-mode component colors (like `.theme-light .intelligence-toolbar`) violate the architectural separation of concerns. Component-specific styling should live inside their respective scoped stylesheets (e.g., `IntelligenceToolbar.razor.css`) rather than the global `app.css`. Using `!important` here also creates styling conflicts (e.g., the scoped stylesheet has a background of `#ffffff`, but this global rule overrides it with `#f5f5f4`). Please move these overrides into `IntelligenceToolbar.razor.css`.

mjasin marked this conversation as resolved

src/NexusReader.UI.Shared/wwwroot/app.css Outdated

Antigravity commented

2026-06-05 09:31:38 +00:00

🟡 Design/Architecture: The class selector .nexus-button here (and .platform-desktop .nexus-button at line 360) is targeting a class that does not exist in any component.

The reusable component NexusButton.razor uses the class .nexus-btn. This class name mismatch prevents platform-specific touch target sizes (such as safe-area-insets, mobile touch targets min-height: var(--touch-target-size)) from being applied to the button element.

Actionable suggestion:

.platform-mobile .nexus-btn {
    min-height: var(--touch-target-size);
    min-width: var(--touch-target-size);
    font-size: 1.1rem;
    padding: 12px 24px;
}

🟡 Design/Architecture: The class selector `.nexus-button` here (and `.platform-desktop .nexus-button` at line 360) is targeting a class that does not exist in any component. The reusable component `NexusButton.razor` uses the class `.nexus-btn`. This class name mismatch prevents platform-specific touch target sizes (such as safe-area-insets, mobile touch targets `min-height: var(--touch-target-size)`) from being applied to the button element. Actionable suggestion: ```css .platform-mobile .nexus-btn { min-height: var(--touch-target-size); min-width: var(--touch-target-size); font-size: 1.1rem; padding: 12px 24px; } ```

mjasin marked this conversation as resolved

src/NexusReader.UI.Shared/wwwroot/js/selectionHandler.js

						
				@@ -39,3 +108,8 @@ export function initSelectionListener(dotNetHelper, container) {

				    document.addEventListener('selectionchange', handleSelection);

Antigravity commented

2026-06-05 09:31:38 +00:00

🔴 Blocking: Registering event listeners on document globally inside initSelectionListener without a corresponding teardown mechanism causes a memory leak.

Every time ReaderCanvas is loaded or the chapter changes, a new selectionchange listener is added to document. These listeners will accumulate over time, leading to degraded performance and multiple handler invocations.

Actionable suggestion:
Track the handler function and export a teardown function:

let currentHandleSelection = null;

export function initSelectionListener(dotNetHelper, container) {
    if (!container) return;
    
    if (currentHandleSelection) {
        document.removeEventListener('selectionchange', currentHandleSelection);
    }

    const handleSelection = () => {
        // ... selection logic ...
    };
    currentHandleSelection = handleSelection;

    document.addEventListener('selectionchange', handleSelection);
    container.addEventListener('mouseup', () => setTimeout(handleSelection, 10));
}

export function destroySelectionListener() {
    if (currentHandleSelection) {
        document.removeEventListener('selectionchange', currentHandleSelection);
        currentHandleSelection = null;
    }
}

🔴 Blocking: Registering event listeners on `document` globally inside `initSelectionListener` without a corresponding teardown mechanism causes a memory leak. Every time `ReaderCanvas` is loaded or the chapter changes, a new `selectionchange` listener is added to `document`. These listeners will accumulate over time, leading to degraded performance and multiple handler invocations. Actionable suggestion: Track the handler function and export a teardown function: ```javascript let currentHandleSelection = null; export function initSelectionListener(dotNetHelper, container) { if (!container) return; if (currentHandleSelection) { document.removeEventListener('selectionchange', currentHandleSelection); } const handleSelection = () => { // ... selection logic ... }; currentHandleSelection = handleSelection; document.addEventListener('selectionchange', handleSelection); container.addEventListener('mouseup', () => setTimeout(handleSelection, 10)); } export function destroySelectionListener() { if (currentHandleSelection) { document.removeEventListener('selectionchange', currentHandleSelection); currentHandleSelection = null; } } ```

mjasin marked this conversation as resolved

src/NexusReader.Web/Program.cs

						
				@@ -205,3 +205,3 @@

				            }

				            await dbContext.Database.MigrateAsync();

Antigravity commented

2026-06-05 09:31:38 +00:00

🟢 Minor/Suggestion: The removal of automatic database migration (MigrateAsync()) at startup is a great architectural choice. It ensures Native AOT compatibility and avoids database locking contention in multi-instance production environments.

Please make sure this is documented in the developer onboarding/setup instructions so manual CLI migrations (dotnet ef database update) are run during local environment setup.

🟢 Minor/Suggestion: The removal of automatic database migration (`MigrateAsync()`) at startup is a great architectural choice. It ensures Native AOT compatibility and avoids database locking contention in multi-instance production environments. Please make sure this is documented in the developer onboarding/setup instructions so manual CLI migrations (`dotnet ef database update`) are run during local environment setup.

mjasin marked this conversation as resolved

mjasin referenced this issue from a commit

2026-06-05 09:45:47 +00:00

fix(reader): resolve PR #69 code review comments under #issuecomment-429

mjasin added 1 commit 2026-06-05 09:45:47 +00:00

fix(reader): resolve PR #69 code review comments under #issuecomment-429 46cc119c81

Antigravity commented

2026-06-05 09:45:52 +00:00

I have resolved all 7 feedback items under #issuecomment-429 (comments 422 to 428):

Comment 422 (CSS Isolation): Moved global light-mode component overrides for .intelligence-toolbar from app.css to scoped IntelligenceToolbar.razor.css and removed !important.
Comment 423 (Selector Mismatch): Replaced .nexus-button with .nexus-btn in app.css to correctly target the NexusButton.razor component's style classes for mobile/desktop.
Comment 424 & 425 (Memory Leaks): Added listener tracking and an exported destroySelectionListener() function in selectionHandler.js, and updated ReaderCanvas.razor to hold reference to _selectionModule and invoke cleanup on DisposeAsync.
Comment 426 (Error Logging): Added warning logs in KnowledgeCoordinator.cs when RequestSummaryAndQuizAsync fails during the selection summary.
Comment 427 (Migrations Documentation): Added manual database migration instructions to the README.md file.
Comment 428 (Async Overhead): Simplified HandleThemeChangedAsync in IntelligenceToolbar.razor to return Task directly instead of being async Task.

Verified via local compilation check (dotnet build passed with 0 errors) and successful test runs (all 20 tests passed).

I have resolved all 7 feedback items under `#issuecomment-429` (comments 422 to 428): 1. **Comment 422 (CSS Isolation)**: Moved global light-mode component overrides for `.intelligence-toolbar` from `app.css` to scoped `IntelligenceToolbar.razor.css` and removed `!important`. 2. **Comment 423 (Selector Mismatch)**: Replaced `.nexus-button` with `.nexus-btn` in `app.css` to correctly target the `NexusButton.razor` component's style classes for mobile/desktop. 3. **Comment 424 & 425 (Memory Leaks)**: Added listener tracking and an exported `destroySelectionListener()` function in `selectionHandler.js`, and updated `ReaderCanvas.razor` to hold reference to `_selectionModule` and invoke cleanup on `DisposeAsync`. 4. **Comment 426 (Error Logging)**: Added warning logs in `KnowledgeCoordinator.cs` when `RequestSummaryAndQuizAsync` fails during the selection summary. 5. **Comment 427 (Migrations Documentation)**: Added manual database migration instructions to the `README.md` file. 6. **Comment 428 (Async Overhead)**: Simplified `HandleThemeChangedAsync` in `IntelligenceToolbar.razor` to return `Task` directly instead of being `async Task`. Verified via local compilation check (`dotnet build` passed with 0 errors) and successful test runs (all 20 tests passed).

mjasin merged commit f18663426b into develop

2026-06-05 09:51:29 +00:00

mjasin deleted branch feature/reader-visual-refactor

2026-06-05 09:51:30 +00:00

Sign in to join this conversation.

2 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: mjasin/Nexus.Reader#69

				`@@ -39,3 +108,8 @@ export function initSelectionListener(dotNetHelper, container) {`
				`document.addEventListener('selectionchange', handleSelection);`