Introduce IModalCoordinator with strongly-typed ModalId and async inline-alert disposal#546
Draft
jschick04 wants to merge 5 commits into
Draft
Introduce IModalCoordinator with strongly-typed ModalId and async inline-alert disposal#546jschick04 wants to merge 5 commits into
jschick04 wants to merge 5 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces a new IModalCoordinator as the single coordination surface for modal lifecycle + inline-alert host registration, while also adding a strongly-typed ModalId and adjusting modal/inline-alert completion ordering to avoid stale state observations by subscribers.
Changes:
- Added
IModalCoordinator/ModalCoordinator+ModalSessionto mirrorIModalServicestate and own inline-alert host registration. - Introduced
ModalId(withModalId.None) and updatedIModalService/call sites/tests to use it instead oflong. - Refactored
ModalBaseinline-alert handling to use async disposal (DisposeAsync) and reorderedModalServicecompletion/cancel/show paths to invokeStateChangedbefore resuming awaiters.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/EventLogExpert.Runtime/Modal/IModalCoordinator.cs | New coordinator contract for modal lifecycle + inline-alert host registry. |
| src/EventLogExpert.Runtime/Modal/ModalCoordinator.cs | Implements coordinator mirror + inline-alert host registry. |
| src/EventLogExpert.Runtime/Modal/ModalId.cs | Adds strongly-typed modal identifier with None sentinel. |
| src/EventLogExpert.Runtime/Modal/ModalSession.cs | Adds active-modal snapshot type exposed by the coordinator. |
| tests/Unit/EventLogExpert.Runtime.Tests/Modal/ModalCoordinatorTests.cs | Adds coordinator behavior tests (mirror, registry, disposal). |
| tests/Unit/EventLogExpert.Runtime.Tests/Modal/ModalServiceCompletionOrderingTests.cs | Adds ordering tests for StateChanged vs awaiter resumption. |
| src/EventLogExpert.Runtime/Alerts/IInlineAlertHostBroker.cs | Deleted (responsibility moved into IModalCoordinator). |
| src/EventLogExpert.Runtime/Alerts/InlineAlertHostBroker.cs | Deleted (responsibility moved into ModalCoordinator). |
| tests/Unit/EventLogExpert.Runtime.Tests/Alerts/InlineAlertHostBrokerTests.cs | Deleted (replaced by coordinator tests). |
| src/EventLogExpert.Runtime/Modal/IModalService.cs | Updates surface to use ModalId for ActiveModalId/Complete. |
| src/EventLogExpert.Runtime/Modal/ModalService.cs | Wraps id as ModalId and reorders completion/cancel/show for correct notification sequencing. |
| src/EventLogExpert.UI/Modal/ModalBase.cs | Uses IModalCoordinator, ModalId, and async inline-alert registration disposal. |
| src/EventLogExpert.Runtime/Alerts/AlertDialogService.cs | Routes inline alerts via IModalCoordinator instead of broker. |
| src/EventLogExpert.Runtime/DependencyInjection/RuntimeServiceCollectionExtensions.cs | Registers IModalCoordinator and removes broker registration. |
| src/EventLogExpert/MauiProgram.cs | Resolves IModalCoordinator for AlertDialogService factory wiring. |
| tests/Unit/EventLogExpert.UI.Tests/DebugLog/DebugLogModalTests.cs | Updates test DI setup to provide IModalCoordinator and ModalId. |
| tests/Unit/EventLogExpert.Runtime.Tests/Modal/ModalServiceTests.cs | Updates assertions to use ModalId.None. |
| tests/Unit/EventLogExpert.Runtime.Tests/DependencyInjection/RuntimeServiceCollectionExtensionsTests.cs | Updates DI registration expectations to IModalCoordinator. |
| tests/Unit/EventLogExpert.Runtime.Tests/Alerts/AlertDialogServiceTests.cs | Updates routing tests to use IModalCoordinator.TryGetInlineAlertHost. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
127e880 to
c73e24b
Compare
c73e24b to
1a89e51
Compare
1a89e51 to
4ee620e
Compare
This was referenced May 25, 2026
642b3ae to
ea32471
Compare
…ine-alert disposal
…ost to DI singleton
…ng one BannerService backing store
a0c1bba to
969b29f
Compare
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.
First in the sequential SoC refactor split (squashed to a single PR for shipping; split for easier review).
Scope
IInlineAlertHostBrokerinto the newIModalCoordinator(one front-door for modal lifecycle + inline-alert host registry, one shared source of truth viaIModalService.StateChanged).ModalId(readonly record struct ModalId(long Value)withModalId.Nonesentinel) to match the established codebase pattern (FilterId,FilterGroupId,BannerId, etc.). Internallong _idCounterinModalServiceis preserved; the wrap happens at the public surface.CancellationTokenRegistrationdisposal toawait DisposeAsync()where the enclosing context is genuinely async:ShowInlineAlertAsyncis nowasync;await prior.CancellationRegistration.DisposeAsync()replaces syncDispose().DisposeAsyncCoreawaitsDisposeAsync()on the pending registration.HandleInlineAlertResolvedAsyncis now genuinely async (no more fire-and-forget_ = InvokeAsync(StateHasChanged)); inlines the lock-and-clear path soDisposeAsync()can be awaited.TryClearInlineAlertFromCallbackstays sync — it's only called from thecancellationToken.RegisterActiondelegate where the BCL contract forces sync.ModalService: reorderComplete/CancelActive/Showso state-clear under lock →StateChanged?.Invoke()→ tcs/cancel-delegate ALL fire outside the lock. Without the reorder, an awaiter resuming on aRunContinuationsAsynchronouslycontinuation could observe stale state via subscribers (e.g., the newIModalCoordinatormirror).Tests
ModalCoordinatorTestscovering the coordinator contract (state mirror, host register/unregister, stale-id lazy invalidation, dispose unhooks, push preempts prior).ModalServiceCompletionOrderingTestsverifyingStateChangedfires before awaiter resumes on Complete/CancelActive/Show preempt paths.InlineAlertHostBrokerTests(broker folded into coordinator).ModalId.Files
19 files changed (+652 / -349):
New:
src/EventLogExpert.Runtime/Modal/IModalCoordinator.cssrc/EventLogExpert.Runtime/Modal/ModalCoordinator.cssrc/EventLogExpert.Runtime/Modal/ModalId.cssrc/EventLogExpert.Runtime/Modal/ModalSession.cstests/Unit/EventLogExpert.Runtime.Tests/Modal/ModalCoordinatorTests.cstests/Unit/EventLogExpert.Runtime.Tests/Modal/ModalServiceCompletionOrderingTests.csDeleted:
src/EventLogExpert.Runtime/Alerts/IInlineAlertHostBroker.cssrc/EventLogExpert.Runtime/Alerts/InlineAlertHostBroker.cstests/Unit/EventLogExpert.Runtime.Tests/Alerts/InlineAlertHostBrokerTests.csModified:
src/EventLogExpert.Runtime/Modal/IModalService.cs(ActiveModalId/Complete takeModalId)src/EventLogExpert.Runtime/Modal/ModalService.cs(concurrency reorder +ModalIdpublic surface)src/EventLogExpert.UI/Modal/ModalBase.cs(async refactor +ModalId)src/EventLogExpert.Runtime/Alerts/AlertDialogService.cs(acceptsIModalCoordinator;host is nullflow)src/EventLogExpert.Runtime/DependencyInjection/RuntimeServiceCollectionExtensions.cssrc/EventLogExpert/MauiProgram.cs(factory resolvesIModalCoordinator)ModalIdNotes
OnRequestCloseAsyncveto pipeline,ModalScope.Critical, typed launchers,DatabaseRecoveryDialogmigration,IBannerServicesplit,BannerHostdecomposition,IMenuActionServiceshrink, and re-applying the banner Banner: chevron nav not working and Attention 'Open Settings' shows while Settings is already open #526 stash (which becomes ~250 LOC once the workaround state goes away).