Add typed launchers routing menu and FilterPane modal opens through coordinator#548
Merged
jschick04 merged 1 commit intoMay 26, 2026
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Routes production modal opens through IModalCoordinator.PushAsync<> (instead of IModalService.Show<>) so that modal preemption respects the close-veto pipeline introduced in earlier refactor PRs, preventing data-loss scenarios when users have unsaved/running work in an existing modal.
Changes:
- Added typed
IModalCoordinatorlauncher extensions (ModalCoordinatorLaunchers) for common modals (Settings/DatabaseTools/DebugLogs/ReleaseNotes/FilterCache/FilterGroup). - Migrated multiple production call sites (MAUI menu actions, FilterPane, alert/prompt factories) to use the coordinator-based open path.
- Updated keyboard shortcut modal gating to read coordinator state (
ActiveSession) and added unit tests validating launcher delegation + veto-preempt behavior.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/EventLogExpert.UI/Modal/ModalCoordinatorLaunchers.cs |
Adds typed coordinator-based modal launcher extensions that route through the veto pipeline. |
src/EventLogExpert/MauiProgram.cs |
Updates alert/prompt modal open delegates to use IModalCoordinator.PushAsync<> instead of bypassing veto via IModalService.Show<>. |
src/EventLogExpert/Adapters/Menu/MauiMenuActionService.cs |
Switches menu-driven modal opens (including release notes) from IModalService to IModalCoordinator. |
src/EventLogExpert.UI/FilterPane/FilterPane.razor.cs |
Updates FilterPane modal opens (cached filters, filter groups) to use typed coordinator launchers. |
src/EventLogExpert/Adapters/Input/KeyboardShortcutService.cs |
Gates shortcuts based on coordinator session state instead of modal service type state. |
tests/Unit/EventLogExpert.UI.Tests/Modal/ModalCoordinatorLaunchersTests.cs |
Adds unit coverage for typed launcher extensions, including null-guard and veto-preempt regression behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
169370d to
337a63b
Compare
337a63b to
3fd3a17
Compare
3fd3a17 to
f471c0b
Compare
f471c0b to
059a03b
Compare
059a03b to
ab9f267
Compare
ab9f267 to
97ac7ca
Compare
d67b33a
into
jschick/modal-coordinator-refactor
2 checks passed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
PR 4 of the IModalCoordinator SoC refactor. Migrates 5 production modal-open call sites + 1 modal-state read from
IModalService.Show<>(bypasses the close-veto pipeline) toIModalCoordinator.PushAsync<>(routes through it).Stacks on top of #546 (PR 1+2+3 combined; PR 3 was merged in by the user).
Behavior FIX (not just refactor)
PR 3 added
OnRequestCloseAsyncoverrides toSettingsModal(vetoes whenIsCloseBlocked) andDatabaseToolsModal(confirm prompt whenAnyTabIsRunning). But until PR 4, all production callers still went throughIModalService.Show<>— bypassing the new veto handlers. This PR routes them throughPushAsync<>, which fixes two pre-existing data-loss bugs:IsCloseBlocked=true) no longer silently nukes the existing modal.Both were broken since PR 3 landed; the veto handlers existed but no caller exercised them. Testers should verify both flows.
New surface
src/EventLogExpert.UI/Modal/ModalCoordinatorLaunchers.cs— 6 extension methods onIModalCoordinator:OpenSettingsAsync()OpenDatabaseToolsAsync()OpenDebugLogsAsync()OpenReleaseNotesAsync(ReleaseNotesContent)OpenFilterCacheAsync()OpenFilterGroupAsync()All return
Task<ModalOpenResult<bool>>. Each null-guards the coordinator argument.Migrated callers
MauiProgram.cs:120-145AlertDialogServicefactorymodalCoordinator.PushAsync<AlertModal/PromptModal, …>directly;IModalServicedependency removed from the factoryMauiMenuActionService.csIModalServicetoIModalCoordinator;ShowModalAsync<T>helper body changed fromShow<>; return trueto_ = PushAsync<>; return true(the helper covers Settings, DatabaseTools, DebugLogs);ShowReleaseNotesAsyncuses the new extension methodFilterPane.razor.cs[Inject] IModalService ModalService→[Inject] IModalCoordinator ModalCoordinator;OpenCachedFiltersModal/OpenFilterGroupsModaluse extension methodsKeyboardShortcutService.cs_modalService.ActiveModalType is not nullto_modalCoordinator.ActiveSession is not null(SoC consistency — it's a state-read, not a launcher)Veto contract (preserved)
MauiMenuActionService.ShowModalAsync<T>helper:truetrue(non-error; consistent with "user state preserved")false(existing failure path;BannerHostshows the existing error banner)BannerHost.razor.cs:189-195'sif (!success)only fires on actual exceptions, never on user-vetoed close.AlertDialogServicestandalone closures:r.WasOpened && r.Result— veto-preempt collapses tofalse(caller interprets as "user dismissed")r.WasOpened ? r.Result ?? string.Empty : string.Empty— veto returnsstring.Empty(existing "empty-on-cancel" contract)Tests
8 new tests in
ModalCoordinatorLaunchersTests:PushAsync<TModal, bool>via NSubstituteReceived(1)OpenSettingsAsyncreturnsWasOpened=falsewhen the coordinator vetoes918 Runtime + 158 UI = 1,076 tests passing; build 0/0.
Pre-implementation panel
4-slot panel (Opus xhigh + GPT-5.5 + GPT-5.4 + Opus 4.6) iterated R1→R3 to 4/4 unanimous READY. R2 surfaced a critical blind spot: Slot 3 caught that vetoes are reachable TODAY (not deferred to PR 5) because PR 3 added the
OnRequestCloseAsyncoverrides. Three of four slots (and the agent) had missed this — tracked as a panel-miss for the post-PR-review feedback loop.Follow-up work this PR unblocks
DatabaseRecoveryDialogmigrates toModalBasewithModalScope.Critical— now that callers route throughPushAsync, the Critical-scope policy actually fires.