Skip to content

Fix banner selection drift, priority watermark, and per-row Remove guard for pre-progress and queued upgrade batches#557

Merged
NikTilton merged 1 commit into
mainfrom
jschick/banner-followups
May 29, 2026
Merged

Fix banner selection drift, priority watermark, and per-row Remove guard for pre-progress and queued upgrade batches#557
NikTilton merged 1 commit into
mainfrom
jschick/banner-followups

Conversation

@jschick04
Copy link
Copy Markdown
Collaborator

Follow-up branch addressing 3 deferred panel-tracked findings from PR #553. R7-panel converged (4 READY) on spec; 3-slot post-impl panel reviewed implementation, addressed all findings (test-coverage gap for user-acknowledgment clear corrected; spec-revision artifacts stripped from comments). Regression-verified: the user-ack test fails with the production fix temporarily removed, confirming it's a real regression guard.

What's in this PR

Item 1 — Selection drift restore (modal open/close):
The DatabaseToolsModal suppresses the Attention banner from the cycle. Before this fix, the selection-restore logic would lose the user's preference. New _userPreferredItem field (set ONLY by MoveNext / MovePrev — true user navigation) lets RebuildAndReselectLocked restore the user's choice when the suppression lifts.

Item 2a — Priority watermark on newly-arrived items:
When a higher-priority item (Critical, Error) arrives, selection snaps to it via a source-fingerprint diff (newly-arrived only — progress ticks on existing batches don't re-trigger). _priorityStolenSelection tracks the steal so subsequent unrelated rebuilds don't bounce back to a stale lower-priority preference; cleared either when the stolen item leaves the source or when the user explicitly navigates (user-acknowledgment).

Item 2b — Per-row Remove guard during pre-progress and queued window:
Background upgrades start with an empty CurrentEntryName (the WAL-checkpoint phase fires before any per-file progress event), so the per-row Remove guard previously missed files in an active batch's pre-progress window. Additionally, separately-queued (not-yet-started) batches had no BannerProgressEntry at all, so files queued behind an active batch could also miss the guard — both paths led to ReserveFileOperation crashes when the user removed a queued/pre-progress file.

Plumbing:

  • Added BatchFileNames (case-insensitive IReadOnlySet<string>) to BannerProgressEntry plumbed via new FileNames init-property on UpgradeBatchStartedEventArgs (source-compatible — the existing CTS-based primary constructor is preserved).
  • Exposed queued batches via new IDatabaseService.SnapshotQueuedBatches() + QueuedBatchInfo DTO + DatabaseUpgradeService._queuedBatches ConcurrentDictionary. Adds to dict BEFORE TryWrite (with rollback on failure); removes AFTER SafeRaise UpgradeBatchStarted in ProcessBatchAsync so external lookups never miss the file during the active-batch handoff (explicit superset overlap instead of a transition gap).
  • ManageDatabasesTab gains GetCancellableBatchForFile (active + queued) + IsFileInAnyKnownBatch; CancelUpgradesAndAwaitCompletionAsync uses the unified lookup; the stillAlive probe checks batch membership so pendingBatches gates Remove on the real UpgradeBatchCompleted event for both pre-progress and queued files.

Tests added (22 total)

  • BannerCycleStateService: 12 tests (selection-drift restore, priority watermark, bounce-back guard, user-ack clear, modal-suppression interaction)
  • BannerService: 3 tests (BatchFileNames plumbing + case-insensitive + record-with preservation)
  • ManageDatabasesTab: 7 tests (display-lookup narrowness, pre-progress detection, queued-batch detection, cancel-then-remove flow including the ordering + promptness contract)

Out of scope

  • Issue Extracted non-UI classes into separate libraries #3 (MAUI WebView staleness) — verified manually by user; likely fixed as side-effect of Commit D's singleton BannerCycleStateService + StateChanged event pattern. Reopen as separate follow-up if it resurfaces.
  • 2 spec-listed integration tests (SnapshotQueuedBatches_IncludesAllBatchesAfterTryWrite_RemovesAfterConsumerPickup, ProcessBatchAsync_FileVisibleInBothSnapshots_DuringSafeRaiseUpgradeBatchStarted_NoLookupGap) deferred — DatabaseRegistry is internal sealed with no interface, making DatabaseUpgradeService uninstantiable at unit level. The observable consequence (cancel flow awaits batch completion correctly) is covered by the unit-level tests; the consumer-loop ordering invariant has a NOTE on ordering: comment at the call site.

Verification

  • 2,467 unit tests pass; no regressions.
  • User-ack-clear regression test verified by temporarily removing the production fix — test failed as expected, confirming the guard.

Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com

Copilot AI review requested due to automatic review settings May 29, 2026 20:09
@jschick04 jschick04 requested a review from a team as a code owner May 29, 2026 20:09
NikTilton
NikTilton previously approved these changes May 29, 2026
@jschick04 jschick04 marked this pull request as draft May 29, 2026 20:13
@jschick04 jschick04 dismissed NikTilton’s stale review May 29, 2026 20:16

The merge-base changed after approval.

@jschick04 jschick04 force-pushed the jschick/banner-followups branch from fbfd4d0 to 7cbf4ca Compare May 29, 2026 20:16
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR is a follow-up to #553 that hardens banner-cycle behavior (selection restore across modal-driven suppression + “priority watermark” when higher-severity banners arrive) and fixes database-upgrade remove-guard gaps for pre-progress and queued batches, while also moving several UI surfaces (settings save announcements, database management UI, and shared input components) to the updated structure.

Changes:

  • Introduces IBannerCycleStateService/BannerCycleStateService to centralize banner-cycle selection state, including modal-aware Attention suppression, selection restore, and priority-steal tracking.
  • Adds queued-batch visibility (IDatabaseService.SnapshotQueuedBatches, QueuedBatchInfo) and plumbs batch file membership into BannerProgressEntry.BatchFileNames to correctly guard Remove during queued and pre-progress windows.
  • Updates UI composition: BannerHost renders inside ModalChrome + main layout with coordinated visibility; Settings modal no longer hosts database management; adds app-level announcements (IAnnouncementService + AnnouncerHost) and new/updated input components (Toggle, OptionSelect, Checkbox, aria plumbing).

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated no comments.

Show a summary per file
File Description
tests/Unit/EventLogExpert.UI.Tests/TestUtils/BannerHostDependenciesExtensions.cs Adds a shared test DI helper for components that include BannerHost / banner cycle state.
tests/Unit/EventLogExpert.UI.Tests/Settings/SettingsUpgradeProgressBannerTests.cs Removes tests for the deleted Settings-scoped progress banner.
tests/Unit/EventLogExpert.UI.Tests/Settings/SettingsModalTests.cs Adds coverage for settings save announcement and updated accessibility/markup conventions.
tests/Unit/EventLogExpert.UI.Tests/Modal/ModalChromeTests.cs Adds coverage for modal/banner-host wrapper behavior and modal-content visibility state.
tests/Unit/EventLogExpert.UI.Tests/Inputs/ValueSelectTests.cs Adds aria/id behavior tests for ValueSelect.
tests/Unit/EventLogExpert.UI.Tests/Inputs/ToggleTests.cs Adds tests for new Toggle input component.
tests/Unit/EventLogExpert.UI.Tests/Inputs/TextInputTests.cs Adds tests for aria behavior on TextInput.
tests/Unit/EventLogExpert.UI.Tests/Inputs/OptionSelectTests.cs Adds tests for new OptionSelect (replacement for BooleanSelect).
tests/Unit/EventLogExpert.UI.Tests/Inputs/CheckboxTests.cs Adds tests for new Checkbox component (multi-select UI).
tests/Unit/EventLogExpert.UI.Tests/FilterEditor/FilterRowChromeTests.cs Adds tests for filter row chrome button types + toggle-enabled rendering/accessibility.
tests/Unit/EventLogExpert.UI.Tests/DebugLog/DebugLogModalTests.cs Wires banner-host dependencies into modal tests that now include ModalChrome banner region.
tests/Unit/EventLogExpert.UI.Tests/DatabaseTools/Tabs/ManageDatabasesEmptyStateTests.cs Adds coverage for the new empty state component.
tests/Unit/EventLogExpert.UI.Tests/DatabaseTools/Tabs/FakeInlineAlertSurface.cs Adds fake inline-alert surface for Manage tab unit tests.
tests/Unit/EventLogExpert.UI.Tests/DatabaseTools/Tabs/FakeDatabaseService.cs Adds a hand-rolled IDatabaseService fake for event unsubscribe/batch tests.
tests/Unit/EventLogExpert.UI.Tests/Database/DatabaseRecoveryDialogTests.cs Updates recovery dialog tests to include banner-host dependencies.
tests/Unit/EventLogExpert.UI.Tests/Banner/BannerHostTests.cs Expands banner-host coverage (modal location switching, cycle navigation, disposal unsubscribe).
tests/Unit/EventLogExpert.UI.Tests/Banner/BannerCycleStateServiceTests.cs Adds extensive tests for selection restore + priority watermark behavior.
tests/Unit/EventLogExpert.UI.Tests/Banner/AttentionBannerTests.cs Updates to “Open Databases” action + OpenDatabaseToolsAsync() semantics.
tests/Unit/EventLogExpert.UI.Tests/Announcement/AnnouncerHostTests.cs Adds tests for the new app-level live-region announcer host.
tests/Unit/EventLogExpert.Runtime.Tests/DependencyInjection/RuntimeServiceCollectionExtensionsTests.cs Ensures runtime DI includes IAnnouncementService among host-facing abstractions.
tests/Unit/EventLogExpert.Runtime.Tests/Database/DatabaseOperationCoordinatorTests.cs Updates expected upgrade scope to ManageDatabasesTriggered.
tests/Unit/EventLogExpert.Runtime.Tests/Banner/BannerViewSelectorTests.cs Updates selector signature + adds coverage for modal-context Attention suppression flag.
tests/Unit/EventLogExpert.Runtime.Tests/Banner/BannerServiceTests.cs Updates progress slot naming/scope + validates BatchFileNames plumbing behavior.
tests/Unit/EventLogExpert.Runtime.Tests/Announcement/AnnouncementServiceTests.cs Adds unit tests for new runtime announcement service (including “identical message” mutation).
tests/Integration/EventLogExpert.Runtime.IntegrationTests/Database/DatabaseServiceTests.cs Reorders/extends classification tests and adds retry-classification coverage.
src/EventLogExpert/wwwroot/index.html Includes new global CSS for the Manage status banner.
src/EventLogExpert/wwwroot/css/manage-status-banner.css Adds shared styling for Manage tab classification/status banner.
src/EventLogExpert/wwwroot/css/app.css Adds --toggle-accent to support new Toggle/OptionSelect visuals with contrast rationale.
src/EventLogExpert/MauiProgram.cs Registers IBannerCycleStateService as a singleton in the app host DI.
src/EventLogExpert/Main.razor Moves main BannerHost into a banner-region wrapper and sets explicit location.
src/EventLogExpert/Components/Layout/MainLayout.razor Adds AnnouncerHost to the layout footer for app-level SR announcements.
src/EventLogExpert/Adapters/Menu/MauiMenuActionService.cs Changes OpenDatabaseToolsAsync to return Task<bool> (success/failure).
src/EventLogExpert/_Imports.razor Adds UI announcement imports.
src/EventLogExpert.UI/Settings/SettingsUpgradeProgressBanner.razor.css Deletes Settings upgrade banner styling (banner moved/removed).
src/EventLogExpert.UI/Settings/SettingsUpgradeProgressBanner.razor.cs Deletes Settings upgrade banner code-behind.
src/EventLogExpert.UI/Settings/SettingsUpgradeProgressBanner.razor Deletes Settings upgrade banner markup.
src/EventLogExpert.UI/Settings/SettingsModal.razor.css Adjusts padding to avoid clipped focus rings; removes old classification styling.
src/EventLogExpert.UI/Settings/SettingsModal.razor.cs Simplifies settings modal and announces “Settings saved” via IAnnouncementService.
src/EventLogExpert.UI/Settings/SettingsModal.razor Removes embedded database management UI; improves label-for associations; uses Toggle.
src/EventLogExpert.UI/Modal/ModalChrome.razor.css Raises inline-alert overlay z-index to ensure it stays above other modal content.
src/EventLogExpert.UI/Modal/ModalChrome.razor.cs Coordinates banner visibility via IBannerCycleStateService.ModalContentDisplayed.
src/EventLogExpert.UI/Modal/ModalChrome.razor Renders BannerHost inside the modal; toggles inert during inline alerts.
src/EventLogExpert.UI/Menu/MenuBar.razor.cs Renames “Database Tools…” menu item to “Databases…”.
src/EventLogExpert.UI/Inputs/ValueSelect.razor.cs Moves aria parameters to base InputComponent; adds optional Id.
src/EventLogExpert.UI/Inputs/ValueSelect.razor Applies EffectiveAriaLabel precedence and forwards optional id.
src/EventLogExpert.UI/Inputs/Toggle.razor.css Adds new switch-style toggle component styling + focus appearance.
src/EventLogExpert.UI/Inputs/Toggle.razor.cs Adds new Toggle input component implementation.
src/EventLogExpert.UI/Inputs/Toggle.razor Adds new Toggle markup with appropriate aria + role switch.
src/EventLogExpert.UI/Inputs/TextInput.razor Adds aria plumbing to TextInput.
src/EventLogExpert.UI/Inputs/OptionSelect.razor.css Adds new radiogroup “option select” styling with focus appearance and hit-target sizing.
src/EventLogExpert.UI/Inputs/OptionSelect.razor.cs Renames/implements OptionSelect (replacing BooleanSelect) + aria-label composition.
src/EventLogExpert.UI/Inputs/OptionSelect.razor Adds new OptionSelect markup (2 radio options) with aria behavior.
src/EventLogExpert.UI/Inputs/InputComponent.cs Centralizes aria parameters + EffectiveAriaLabel precedence in base input type.
src/EventLogExpert.UI/Inputs/Checkbox.razor.css Adds new checkbox styling using visually-hidden native input pattern.
src/EventLogExpert.UI/Inputs/Checkbox.razor.cs Adds new Checkbox input component implementation.
src/EventLogExpert.UI/Inputs/Checkbox.razor Adds new Checkbox markup with aria plumbing.
src/EventLogExpert.UI/Inputs/BooleanSelect.razor.css Deletes legacy BooleanSelect styling.
src/EventLogExpert.UI/Inputs/BooleanSelect.razor Deletes legacy BooleanSelect markup.
src/EventLogExpert.UI/FilterEditor/SubFilterRow.razor Switches from BooleanSelect to OptionSelect for AND/OR selection.
src/EventLogExpert.UI/FilterEditor/FilterRowChrome.razor.cs Adds a stable label id for aria-labelledby on the enabled toggle.
src/EventLogExpert.UI/FilterEditor/FilterRowChrome.razor Ensures button type=button; uses Toggle for enabled state with aria-labelledby.
src/EventLogExpert.UI/FilterEditor/FilterComparisonEditor.razor Adds label association via AriaLabelledBy on TextInput.
src/EventLogExpert.UI/DatabaseTools/Tabs/ManageDatabasesTab.razor.css Adds layout styling for the new Manage tab (scrolling + sticky strips).
src/EventLogExpert.UI/DatabaseTools/Tabs/ManageDatabasesTab.razor Adds the new Manage tab UI (import, list rows, bulk remove, save strip).
src/EventLogExpert.UI/DatabaseTools/Tabs/ManageDatabasesEmptyState.razor.css Styles empty-state helper text.
src/EventLogExpert.UI/DatabaseTools/Tabs/ManageDatabasesEmptyState.razor Adds empty state instructions referencing Import button.
src/EventLogExpert.UI/DatabaseTools/IInlineAlertSurface.cs Introduces a cascading inline-alert surface for child tabs to raise inline alerts.
src/EventLogExpert.UI/DatabaseTools/DatabaseToolsTab.cs Adds Manage tab to the tab enum and adjusts default tab selection.
src/EventLogExpert.UI/DatabaseTools/DatabaseToolsModal.razor.cs Adds manage-tab close gating + save/discard prompts + reload-open-logs prompt on close.
src/EventLogExpert.UI/DatabaseTools/DatabaseToolsModal.razor Renames modal label to “Databases”, adds Manage tab, and cascades inline-alert surface.
src/EventLogExpert.UI/Database/DatabaseEntryRow.razor.css Redesigns row layout for multi-select and per-row actions; updates upgrading/cancel styling.
src/EventLogExpert.UI/Database/DatabaseEntryRow.razor.cs Adds selection state, pending toggle indicator, per-row cancel/remove focus behavior, retry/restore actions.
src/EventLogExpert.UI/Database/DatabaseEntryRow.razor Adds checkbox selection, pending indicator, inline cancel, restore/retry classification, and always-visible Remove.
src/EventLogExpert.UI/Banner/IBannerCycleStateService.cs Defines banner-cycle state interface (selection, items, modal visibility).
src/EventLogExpert.UI/Banner/BannerHostLocation.cs Adds location enum to toggle whether BannerHost renders in modal vs main layout.
src/EventLogExpert.UI/Banner/BannerHost.razor.cs Refactors BannerHost to render from shared cycle state and support modal/main placement.
src/EventLogExpert.UI/Banner/BannerHost.razor Uses cycle state for selection/pagination; delegates navigation to cycle state service.
src/EventLogExpert.UI/Banner/BannerCycleStateService.cs Implements selection restore + priority watermark + modal-context Attention suppression behavior.
src/EventLogExpert.UI/Banner/AttentionBanner.razor.cs Updates action to open “Databases” and uses boolean-returning OpenDatabaseToolsAsync.
src/EventLogExpert.UI/Banner/AttentionBanner.razor Updates banner text/action label to “Open Databases”.
src/EventLogExpert.UI/Announcement/AnnouncerHost.razor.css Adds visually-hidden styling for the live region.
src/EventLogExpert.UI/Announcement/AnnouncerHost.razor.cs Adds announcer host component wiring to IAnnouncementService.
src/EventLogExpert.UI/Announcement/AnnouncerHost.razor Adds polite live-region markup for app-level announcements.
src/EventLogExpert.UI/_Imports.razor Adds banner namespace import for shared banner components.
src/EventLogExpert.Runtime/Menu/IMenuActionService.cs Changes OpenDatabaseToolsAsync signature to Task<bool>.
src/EventLogExpert.Runtime/DependencyInjection/RuntimeServiceCollectionExtensions.cs Registers IAnnouncementService in runtime DI.
src/EventLogExpert.Runtime/Database/Upgrade/UpgradeProgressScope.cs Renames scope to ManageDatabasesTriggered.
src/EventLogExpert.Runtime/Database/Upgrade/UpgradeBatchStartedEventArgs.cs Adds FileNames init property to carry batch membership snapshot.
src/EventLogExpert.Runtime/Database/Upgrade/QueuedBatchInfo.cs Adds DTO for queued upgrade batches with cancel handle.
src/EventLogExpert.Runtime/Database/IDatabaseService.cs Adds RetryClassificationAsync and SnapshotQueuedBatches.
src/EventLogExpert.Runtime/Database/IDatabaseOperationCoordinator.cs Updates default upgrade scope to ManageDatabasesTriggered.
src/EventLogExpert.Runtime/Database/DatabaseUpgradeService.cs Tracks queued batches in a concurrent dictionary and exposes queued snapshots.
src/EventLogExpert.Runtime/Database/DatabaseService.cs Forwards retry classification and queued batch snapshot methods.
src/EventLogExpert.Runtime/Database/DatabaseOperationCoordinator.cs Updates default upgrade scope to ManageDatabasesTriggered.
src/EventLogExpert.Runtime/Database/DatabaseClassificationService.cs Refactors per-entry classification and adds RetryClassificationAsync.
src/EventLogExpert.Runtime/Banner/IProgressBannerService.cs Renames settings progress slot to ManageDatabasesProgress.
src/EventLogExpert.Runtime/Banner/BannerViewSelector.cs Adds modal-context suppression parameter for Attention inclusion.
src/EventLogExpert.Runtime/Banner/BannerService.cs Updates progress slot naming and plumbs BatchFileNames into BannerProgressEntry.
src/EventLogExpert.Runtime/Banner/BannerProgressEntry.cs Adds BatchFileNames (case-insensitive) to support remove-guard.
src/EventLogExpert.Runtime/Announcement/IAnnouncementService.cs Adds app-level announcement service abstraction.
src/EventLogExpert.Runtime/Announcement/AnnouncementService.cs Implements announcement service with safe subscriber invocation + DOM mutation for identical messages.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@jschick04 jschick04 force-pushed the jschick/banner-followups branch from 7cbf4ca to 9cd753e Compare May 29, 2026 20:48
@jschick04 jschick04 marked this pull request as ready for review May 29, 2026 20:51
Copilot AI review requested due to automatic review settings May 29, 2026 20:51
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.

Comment thread src/EventLogExpert.Runtime/Banner/BannerProgressEntry.cs
Comment thread src/EventLogExpert.Runtime/Database/DatabaseUpgradeService.cs
…guard for pre-progress and queued upgrade batches

Items 1+2a (BannerCycleStateService): added _userPreferredItem (set only by user MoveNext/MovePrev) plus _priorityStolenSelection tracking so newly-arrived higher-priority slices steal selection without bouncing back to a stale low-priority preference on subsequent rebuilds, and the user-preferred restore proceeds correctly after modal suppression even when a higher-priority item coexists. Fingerprint covers the unfiltered source so modal close does not re-trigger priority override.

Item 2b (per-row Remove guard): added BatchFileNames to BannerProgressEntry plumbed via new FileNames init-property on UpgradeBatchStartedEventArgs (signature-compatible — existing CTS ctor preserved). Exposed queued batches via new IDatabaseService.SnapshotQueuedBatches + DatabaseUpgradeService _queuedBatches ConcurrentDictionary; reordered TryRemove after SafeRaise UpgradeBatchStarted so external lookups never miss the file during the active-batch handoff. ManageDatabasesTab gains GetCancellableBatchForFile + IsFileInAnyKnownBatch covering active + queued; CancelUpgradesAndAwaitCompletionAsync uses a unified lookup and the stillAlive probe checks batch membership so pendingBatches gates Remove on the real UpgradeBatchCompleted event for pre-progress and queued files.

Added 22 unit tests across BannerCycleStateService, BannerService, and ManageDatabasesTab covering selection-drift restore, priority watermark + bounce-back guard + user-acknowledgment clear, modal-suppression interaction, BatchFileNames plumbing, queued-batch lookup, and the cancel-then-remove ordering + promptness contract.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.

@jschick04 jschick04 requested a review from NikTilton May 29, 2026 21:16
@NikTilton NikTilton merged commit fee2378 into main May 29, 2026
7 checks passed
@NikTilton NikTilton deleted the jschick/banner-followups branch May 29, 2026 22:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants