Refactor UI thread invocation with DispatcherHelper#4335
Refactor UI thread invocation with DispatcherHelper#4335Jack251970 wants to merge 7 commits intodevfrom
Conversation
Introduce DispatcherHelper for safe UI thread access and replace direct Dispatcher.Invoke calls across the codebase. This centralizes thread invocation logic, reduces boilerplate, and improves maintainability. Some methods are refactored for clarity and UI thread safety.
|
🥷 Code experts: no user but you matched threshold 10 Jack251970 has most 👩💻 activity in the files. See details
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame: ✨ Comment |
|
Be a legend 🏆 by adding a before and after screenshot of the changes you made, especially if they are around UI/UX. |
There was a problem hiding this comment.
Pull request overview
This PR introduces a new DispatcherHelper static class in Flow.Launcher.Core/Resource/ that centralizes UI thread dispatching logic — checking thread access and calling the action directly if already on the UI thread, or marshalling to the dispatcher otherwise. It replaces all the scattered, boilerplate Dispatcher.CheckAccess() guard patterns across the codebase.
Changes:
- New
DispatcherHelperclass added toFlow.Launcher.Core/Resource/withInvokeandInvokeAsyncoverloads handling bothAction/Func<T>andFunc<Task>delegates. - All UI thread invocations throughout
Flow.LauncherandFlow.Launcher.Corereplaced to useDispatcherHelper, removing repetitive thread-check patterns. - Several methods are split into a public dispatcher-aware wrapper (e.g.,
ChangeQueryText) and a private implementation (e.g.,ChangeQueryText1) to align with the new helper's design.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
Flow.Launcher.Core/Resource/DispatcherHelper.cs |
New helper class centralizing dispatcher thread-safety logic |
Flow.Launcher.Core/Resource/Theme.cs |
Replace Dispatcher.InvokeAsync calls with DispatcherHelper.InvokeAsync |
Flow.Launcher.Core/Plugin/JsonRPCPluginSettings.cs |
Replace element-dispatcher calls with DispatcherHelper.Invoke |
Flow.Launcher/ViewModel/MainViewModel.cs |
Replace all dispatcher invocations; extract ChangeQueryText1/ChangeQueryText1Async helpers |
Flow.Launcher/App.xaml.cs |
Replace Current.Dispatcher.Invoke calls with DispatcherHelper |
Flow.Launcher/MainWindow.xaml.cs |
Replace all Dispatcher.Invoke calls with DispatcherHelper.Invoke |
Flow.Launcher/MessageBoxEx.xaml.cs |
Extract Show1 helper method; use DispatcherHelper.Invoke |
Flow.Launcher/ProgressBoxEx.xaml.cs |
Simplify thread-check pattern; extract ReportProgress1 helper |
Flow.Launcher/Msg.xaml.cs |
Replace Dispatcher.InvokeAsync with DispatcherHelper.InvokeAsync |
Flow.Launcher/MsgWithButton.xaml.cs |
Replace Dispatcher.InvokeAsync with DispatcherHelper.InvokeAsync |
Flow.Launcher/Notification.cs |
Replace Application.Current.Dispatcher.Invoke with DispatcherHelper.Invoke |
Flow.Launcher/PublicAPIInstance.cs |
Replace dispatcher call with DispatcherHelper.Invoke |
Flow.Launcher/ReleaseNotesWindow.xaml.cs |
Replace dispatcher calls with DispatcherHelper.Invoke |
Flow.Launcher/Helper/WallpaperPathRetrieval.cs |
Extract GetWallpaperBrush1 helper; use DispatcherHelper.Invoke |
Flow.Launcher/Helper/SingleInstance.cs |
Replace Application.Current?.Dispatcher.Invoke with DispatcherHelper.Invoke |
Flow.Launcher/SettingPages/ViewModels/SettingsPanePluginStoreViewModel.cs |
Replace dispatcher call with DispatcherHelper.Invoke |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Refactored private methods previously suffixed with "1" to use the "Core" suffix instead (e.g., GetWallpaperBrush1 → GetWallpaperBrushCore). Updated all corresponding invocations. This improves naming consistency and aligns with common conventions for core logic methods.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
1 issue found across 16 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Flow.Launcher.Core/Resource/DispatcherHelper.cs">
<violation number="1" location="Flow.Launcher.Core/Resource/DispatcherHelper.cs:98">
P2: Awaiting Dispatcher.InvokeAsync(func) only waits for the delegate to return a Task, not for that Task to complete. This makes InvokeAsync(Func<Task>) return early (and miss exceptions) when called from a non-UI thread. Await the returned Task as well to make behavior consistent with the CheckAccess branch.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Ensure that async functions invoked via dispatcher are awaited until completion, not just until scheduled, by using double await on InvokeAsync. This prevents premature continuation when the delegate itself is asynchronous.
This comment has been minimized.
This comment has been minimized.
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Flow.Launcher/MainWindow.xaml.cs (1)
267-273:⚠️ Potential issue | 🟡 MinorKeep the low-priority caret move.
Line 272 uses
DispatcherHelper.Invoke()without specifying a priority, which defaults toDispatcherPriority.Normal. However, the adjacent comment explicitly states this work must wait until theTextBoxcatches up with the bound query text by using a lower priority likeContextIdle. SinceDispatcherHelper.Invokesupports aDispatcherPriorityparameter (with defaultNormal), the call should passDispatcherPriority.ContextIdleto honor the documented requirement.🛠️ Proposed fix
- DispatcherHelper.Invoke(() => QueryTextBox.CaretIndex = QueryTextBox.Text.Length); + DispatcherHelper.Invoke( + () => QueryTextBox.CaretIndex = QueryTextBox.Text.Length, + DispatcherPriority.ContextIdle);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Flow.Launcher/MainWindow.xaml.cs` around lines 267 - 273, The caret move is currently dispatched with the default (Normal) priority, but the comment requires a lower priority so the TextBox binding can update first; update the call in MainWindow.xaml.cs where you handle MainViewModel.QueryTextCursorMovedToEnd to call DispatcherHelper.Invoke with DispatcherPriority.ContextIdle when setting QueryTextBox.CaretIndex = QueryTextBox.Text.Length (referencing QueryTextBox and _viewModel.QueryTextCursorMovedToEnd) so the dispatch honors the ContextIdle scheduling requirement.
🧹 Nitpick comments (2)
Flow.Launcher/MsgWithButton.xaml.cs (1)
86-93: Unnecessaryasynckeyword on the lambda.The lambda passed to
DispatcherHelper.InvokeAsyncis markedasyncbut contains noawaitexpressions. SincefadeOutStoryboard.Begin()is synchronous, theasynckeyword can be removed.♻️ Suggested fix
- await DispatcherHelper.InvokeAsync(async () => + await DispatcherHelper.InvokeAsync(() => { if (!closing) { closing = true; fadeOutStoryboard.Begin(); } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Flow.Launcher/MsgWithButton.xaml.cs` around lines 86 - 93, The lambda passed to DispatcherHelper.InvokeAsync is marked async but contains no awaits; remove the unnecessary async modifier so the delegate is synchronous (e.g., replace "async () => { ... }" with "() => { ... }") in the call that checks the closing flag and calls fadeOutStoryboard.Begin(), updating the closure around the closing variable and fadeOutStoryboard.Begin() accordingly.Flow.Launcher/Msg.xaml.cs (1)
84-91: Unnecessaryasynckeyword on the lambda.Same as in
MsgWithButton.xaml.cs- the lambda is markedasyncbut contains noawaitexpressions sincefadeOutStoryboard.Begin()is synchronous.♻️ Suggested fix
- await DispatcherHelper.InvokeAsync(async () => + await DispatcherHelper.InvokeAsync(() => { if (!closing) { closing = true; fadeOutStoryboard.Begin(); } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Flow.Launcher/Msg.xaml.cs` around lines 84 - 91, The lambda passed to DispatcherHelper.InvokeAsync in Msg.xaml.cs is marked async but contains no await; remove the unnecessary async modifier from the lambda (the block that checks and sets the closing flag and calls fadeOutStoryboard.Begin()) so the delegate is a synchronous Action/Func<Task> appropriate for InvokeAsync and avoid the redundant async state machine.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Flow.Launcher.Core/Resource/DispatcherHelper.cs`:
- Around line 63-85: The fast-path CheckAccess() in the Dispatcher helper
methods causes callers on the UI thread to run immediately and ignore the
requested DispatcherPriority; update both InvokeAsync(Dispatcher, Action,
DispatcherPriority) and InvokeAsync<T>(Dispatcher, Func<T>, DispatcherPriority)
to keep the null dispatcher guard but remove the CheckAccess() branch and
unconditionally call and await dispatcher.InvokeAsync(..., priority) so the
dispatcher always enqueues the delegate with the requested priority (i.e., await
dispatcher.InvokeAsync(action, priority) and return await
dispatcher.InvokeAsync(func, priority) respectively).
---
Outside diff comments:
In `@Flow.Launcher/MainWindow.xaml.cs`:
- Around line 267-273: The caret move is currently dispatched with the default
(Normal) priority, but the comment requires a lower priority so the TextBox
binding can update first; update the call in MainWindow.xaml.cs where you handle
MainViewModel.QueryTextCursorMovedToEnd to call DispatcherHelper.Invoke with
DispatcherPriority.ContextIdle when setting QueryTextBox.CaretIndex =
QueryTextBox.Text.Length (referencing QueryTextBox and
_viewModel.QueryTextCursorMovedToEnd) so the dispatch honors the ContextIdle
scheduling requirement.
---
Nitpick comments:
In `@Flow.Launcher/Msg.xaml.cs`:
- Around line 84-91: The lambda passed to DispatcherHelper.InvokeAsync in
Msg.xaml.cs is marked async but contains no await; remove the unnecessary async
modifier from the lambda (the block that checks and sets the closing flag and
calls fadeOutStoryboard.Begin()) so the delegate is a synchronous
Action/Func<Task> appropriate for InvokeAsync and avoid the redundant async
state machine.
In `@Flow.Launcher/MsgWithButton.xaml.cs`:
- Around line 86-93: The lambda passed to DispatcherHelper.InvokeAsync is marked
async but contains no awaits; remove the unnecessary async modifier so the
delegate is synchronous (e.g., replace "async () => { ... }" with "() => { ...
}") in the call that checks the closing flag and calls
fadeOutStoryboard.Begin(), updating the closure around the closing variable and
fadeOutStoryboard.Begin() accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ea6d1961-6a5e-4a51-8628-416173c8a2f4
📒 Files selected for processing (16)
Flow.Launcher.Core/Plugin/JsonRPCPluginSettings.csFlow.Launcher.Core/Resource/DispatcherHelper.csFlow.Launcher.Core/Resource/Theme.csFlow.Launcher/App.xaml.csFlow.Launcher/Helper/SingleInstance.csFlow.Launcher/Helper/WallpaperPathRetrieval.csFlow.Launcher/MainWindow.xaml.csFlow.Launcher/MessageBoxEx.xaml.csFlow.Launcher/Msg.xaml.csFlow.Launcher/MsgWithButton.xaml.csFlow.Launcher/Notification.csFlow.Launcher/ProgressBoxEx.xaml.csFlow.Launcher/PublicAPIInstance.csFlow.Launcher/ReleaseNotesWindow.xaml.csFlow.Launcher/SettingPages/ViewModels/SettingsPanePluginStoreViewModel.csFlow.Launcher/ViewModel/MainViewModel.cs
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Refactored method name for clarity and consistency. Updated all internal references to use the new method name. No functional changes were made.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Flow.Launcher.Core/Resource/DispatcherHelper.cs`:
- Around line 89-100: The CI flags the "await await" pattern in InvokeAsync;
replace the single "await await dispatcher.InvokeAsync(func, priority);" with
two statements to avoid the repeated-word pattern: call
dispatcher.InvokeAsync(func, priority), await that DispatcherOperation<Task> to
get the inner Task (e.g. "var innerTask = await dispatcher.InvokeAsync(func,
priority);"), then await the innerTask with a second await (e.g. "await
innerTask;") inside the InvokeAsync method to preserve behavior while satisfying
the forbidden-pattern rule.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5fbff59f-5a9b-4347-a3ef-88d72bb59709
📒 Files selected for processing (5)
Flow.Launcher.Core/Resource/DispatcherHelper.csFlow.Launcher/Helper/WallpaperPathRetrieval.csFlow.Launcher/MessageBoxEx.xaml.csFlow.Launcher/ProgressBoxEx.xaml.csFlow.Launcher/ViewModel/MainViewModel.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- Flow.Launcher/Helper/WallpaperPathRetrieval.cs
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Replaced Application.Current.Dispatcher.Invoke with DispatcherHelper.Invoke when opening the plugin settings window to centralize and abstract dispatcher invocation logic.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Flow.Launcher/ViewModel/MainViewModel.cs (1)
2144-2168:⚠️ Potential issue | 🟠 MajorUse native
Dispatcher.Invokeinstead ofDispatcherHelper.Invoketo preserveRenderpriority scheduling for DWM cloak operations.
DispatcherHelper.Invokeexecutes the action inline whenCheckAccess()is true, bypassing the priority queue entirely. NativeDispatcher.Invoke(..., DispatcherPriority.Render)queues the work atRenderpriority even on the UI thread, respecting the dispatch pipeline. For DWM cloak/uncloak andUpdateLayout()calls, this timing difference can reintroduce the flicker and visibility glitches the code guards against. Replace with directDispatcher.Invokeor extendDispatcherHelperwith a force-dispatch parameter.Applies to both locations: lines 2144–2168 and 2226–2247.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Flow.Launcher/ViewModel/MainViewModel.cs` around lines 2144 - 2168, The current use of DispatcherHelper.Invoke in MainViewModel allows inline execution when CheckAccess() is true and bypasses the Dispatcher priority queue, which can break Render-priority timing for DWMSetCloakForWindow and related UI updates; replace those calls with the native Dispatcher.Invoke(..., DispatcherPriority.Render) (e.g., Application.Current?.Dispatcher.Invoke(action, DispatcherPriority.Render)) in both places where DispatcherHelper.Invoke is used so the work is queued at Render priority even on the UI thread, or alternatively modify DispatcherHelper.Invoke to accept a “force dispatch” flag and call Dispatcher.Invoke when true; ensure you update both the block that calls Win32Helper.DWMSetCloakForWindow and the symmetric second occurrence in MainViewModel so DWM cloak/uncloak and layout updates run at Render priority.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@Flow.Launcher/ViewModel/MainViewModel.cs`:
- Around line 2144-2168: The current use of DispatcherHelper.Invoke in
MainViewModel allows inline execution when CheckAccess() is true and bypasses
the Dispatcher priority queue, which can break Render-priority timing for
DWMSetCloakForWindow and related UI updates; replace those calls with the native
Dispatcher.Invoke(..., DispatcherPriority.Render) (e.g.,
Application.Current?.Dispatcher.Invoke(action, DispatcherPriority.Render)) in
both places where DispatcherHelper.Invoke is used so the work is queued at
Render priority even on the UI thread, or alternatively modify
DispatcherHelper.Invoke to accept a “force dispatch” flag and call
Dispatcher.Invoke when true; ensure you update both the block that calls
Win32Helper.DWMSetCloakForWindow and the symmetric second occurrence in
MainViewModel so DWM cloak/uncloak and layout updates run at Render priority.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b1f29370-138b-4195-8caa-4ea1db730cdd
📒 Files selected for processing (4)
Flow.Launcher.Core/Resource/Theme.csFlow.Launcher/PublicAPIInstance.csFlow.Launcher/SettingPages/ViewModels/SettingsPanePluginStoreViewModel.csFlow.Launcher/ViewModel/MainViewModel.cs
🚧 Files skipped from review as they are similar to previous changes (2)
- Flow.Launcher/PublicAPIInstance.cs
- Flow.Launcher.Core/Resource/Theme.cs
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Flow.Launcher/MainWindow.xaml.cs (1)
267-273:⚠️ Potential issue | 🟡 MinorPass
DispatcherPriority.ContextIdleto the caret dispatch.The inline comment at lines 269–270 explicitly requires this dispatcher to run only after
QueryTextBoxupdates toContextIdlepriority, but the call does not pass that parameter and defaults toNormal. This can cause the caret to move before the text binding completes, leaving it short of the actual string length after history, autocomplete, or programmatic query changes.🔧 Suggested fix
- DispatcherHelper.Invoke(() => QueryTextBox.CaretIndex = QueryTextBox.Text.Length); + DispatcherHelper.Invoke( + () => QueryTextBox.CaretIndex = QueryTextBox.Text.Length, + DispatcherPriority.ContextIdle);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Flow.Launcher/MainWindow.xaml.cs` around lines 267 - 273, The caret dispatch currently uses DispatcherHelper.Invoke without a priority, so it runs at Normal and may execute before the QueryTextBox binding finishes; update the call in the MainWindow case handling MainViewModel.QueryTextCursorMovedToEnd to call DispatcherHelper.Invoke with DispatcherPriority.ContextIdle so the lambda that sets QueryTextBox.CaretIndex = QueryTextBox.Text.Length runs only after the control's text update completes, then keep the existing reset of _viewModel.QueryTextCursorMovedToEnd = false.
🧹 Nitpick comments (1)
Flow.Launcher.Core/Resource/DispatcherHelper.cs (1)
63-87: Document fast-path priority behavior at the API boundary.Consider adding XML-doc remarks that when
CheckAccess()is true, delegates run immediately andDispatcherPriorityis not used for queue ordering. This prevents misuse at call sites expecting deferred scheduling.Based on learnings: In
Flow.Launcher.Core/Resource/DispatcherHelper.cs, theInvokeAsyncfast path viaCheckAccess()is intentional even when a priority is supplied.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Flow.Launcher.Core/Resource/DispatcherHelper.cs` around lines 63 - 87, Add XML documentation to both overloads of InvokeAsync (the void Task InvokeAsync(Dispatcher dispatcher, Action action, ...) and the generic Task<T> InvokeAsync<T>(Dispatcher dispatcher, Func<T> func, ...)) clarifying that when dispatcher.CheckAccess() returns true the delegate is executed synchronously on the current thread and DispatcherPriority is ignored, so callers should not rely on supplied priority for deferred/queued execution; update the summary/remarks to explicitly state this fast-path behavior and intended usage to avoid misuse at call sites expecting scheduling semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Flow.Launcher/MsgWithButton.xaml.cs`:
- Around line 86-93: The dispatcher lambda passed to
DispatcherHelper.InvokeAsync is marked async but contains no awaits; replace the
async lambda with a synchronous Action to avoid unnecessary state machine
allocation — locate the call to DispatcherHelper.InvokeAsync in
MsgWithButton.xaml.cs and change the async lambda to a plain synchronous lambda
or method that checks the closing flag and calls fadeOutStoryboard.Begin()
(preserve the same behavior around the closing variable and
fadeOutStoryboard.Begin()).
---
Outside diff comments:
In `@Flow.Launcher/MainWindow.xaml.cs`:
- Around line 267-273: The caret dispatch currently uses DispatcherHelper.Invoke
without a priority, so it runs at Normal and may execute before the QueryTextBox
binding finishes; update the call in the MainWindow case handling
MainViewModel.QueryTextCursorMovedToEnd to call DispatcherHelper.Invoke with
DispatcherPriority.ContextIdle so the lambda that sets QueryTextBox.CaretIndex =
QueryTextBox.Text.Length runs only after the control's text update completes,
then keep the existing reset of _viewModel.QueryTextCursorMovedToEnd = false.
---
Nitpick comments:
In `@Flow.Launcher.Core/Resource/DispatcherHelper.cs`:
- Around line 63-87: Add XML documentation to both overloads of InvokeAsync (the
void Task InvokeAsync(Dispatcher dispatcher, Action action, ...) and the generic
Task<T> InvokeAsync<T>(Dispatcher dispatcher, Func<T> func, ...)) clarifying
that when dispatcher.CheckAccess() returns true the delegate is executed
synchronously on the current thread and DispatcherPriority is ignored, so
callers should not rely on supplied priority for deferred/queued execution;
update the summary/remarks to explicitly state this fast-path behavior and
intended usage to avoid misuse at call sites expecting scheduling semantics.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dde0ac5f-c0ef-4c10-a904-251c9077f244
📒 Files selected for processing (16)
Flow.Launcher.Core/Plugin/JsonRPCPluginSettings.csFlow.Launcher.Core/Resource/DispatcherHelper.csFlow.Launcher.Core/Resource/Theme.csFlow.Launcher/App.xaml.csFlow.Launcher/Helper/SingleInstance.csFlow.Launcher/Helper/WallpaperPathRetrieval.csFlow.Launcher/MainWindow.xaml.csFlow.Launcher/MessageBoxEx.xaml.csFlow.Launcher/Msg.xaml.csFlow.Launcher/MsgWithButton.xaml.csFlow.Launcher/Notification.csFlow.Launcher/ProgressBoxEx.xaml.csFlow.Launcher/PublicAPIInstance.csFlow.Launcher/ReleaseNotesWindow.xaml.csFlow.Launcher/SettingPages/ViewModels/SettingsPanePluginStoreViewModel.csFlow.Launcher/ViewModel/MainViewModel.cs
| #pragma warning disable VSTHRD001 // Avoid legacy thread switching APIs | ||
|
|
||
| public static class DispatcherHelper | ||
| { |
There was a problem hiding this comment.
@coderabbitai are there any logic changes in this PR
There was a problem hiding this comment.
@jjw24, based on a detailed review of the AI-generated summaries and the diffs, the PR is predominantly a refactoring with no intentional logic changes. However, there are a few subtle behavioral differences worth being aware of:
-
Msg.xaml.cs&MsgWithButton.xaml.cs— Nested dispatch removal:
The old code had a nestedawait Dispatcher.InvokeAsync(...)wrappingfadeOutStoryboard.Begin()inside an outer dispatched block. The new code removes that inner dispatch and callsfadeOutStoryboard.Begin()directly inside theDispatcherHelper.InvokeAsyncdelegate. Since the outer delegate is already guaranteed to run on the UI thread, this is functionally equivalent — but it is a structural change. -
DispatcherHelperfast-path ignoresDispatcherPriority:
In bothInvokeandInvokeAsync, whenCheckAccess()istrue(already on the UI thread), the delegate is executed directly without going through the dispatcher — meaning theDispatcherPriorityparameter is silently ignored in the fast path. Based on a prior interaction, this is an intentional design choice. Call sites that previously relied on priority scheduling (e.g.,DispatcherPriority.RenderinTheme.cs,DispatcherPriority.ApplicationIdleinMainWindow.xaml.cs) will have the priority honored only when called off the UI thread. -
ProgressBoxEx.xaml.cs— CheckAccess branching removed:
ExplicitCheckAccess()+ conditionalInvokelogic was replaced withDispatcherHelper.Invoke(...), which handlesCheckAccessinternally. Functionally equivalent. -
App.xaml.csdisposal — null handling:
Changed from_mainWindow?.Dispatcher.Invoke(_mainWindow.Dispose)(null-conditional, skips entirely if null) toDispatcherHelper.Invoke(_mainWindow?.Dispatcher, _mainWindow.Dispose)(passes a potentially-null dispatcher, whichDispatcherHelperearly-returns on). Functionally equivalent.
In summary: No meaningful logic changes, but items 1 and 2 are the most notable structural differences to validate if you want extra confidence.
🧠 Learnings used
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 4335
File: Flow.Launcher.Core/Resource/DispatcherHelper.cs:63-85
Timestamp: 2026-03-10T04:07:09.482Z
Learning: In Flow.Launcher.Core/Resource/DispatcherHelper.cs, the InvokeAsync overloads use a fast path via CheckAccess(): if already on the UI thread, execute the action/func directly without dispatcher.InvokeAsync, even if a DispatcherPriority is provided. When reviewing this file or related code, assume this is an intentional design choice by the maintainer. Verify that tests or code paths confirm the UI-thread fast-path is employed, and ensure there is no unnecessary dispatching when CheckAccess() is true. Consider adding/maintaining tests that cover InvokeAsync on the UI thread with various priorities, and document that the priority parameter is ignored in the fast-path.
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3854
File: Flow.Launcher/App.xaml.cs:252-275
Timestamp: 2025-10-16T10:48:30.573Z
Learning: In Flow Launcher's App.xaml.cs, the plugin initialization block (lines 252-275) that includes PreStartPluginExecutablePathUpdate, PluginManager.LoadPlugins, PluginManager.InitializePluginsAsync, _mainVM.QueryResults(), and API.SaveAppAllSettings() does not require additional Task.Run wrappers or Dispatcher.InvokeAsync calls according to maintainer Jack251970, as the threading model is already safe as designed.
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3854
File: Flow.Launcher/App.xaml.cs:246-262
Timestamp: 2025-07-21T09:19:49.684Z
Learning: In Flow Launcher's App.xaml.cs, the asynchronous plugin initialization task (containing AbstractPluginEnvironment.PreStartPluginExecutablePathUpdate, PluginManager.LoadPlugins, PluginManager.InitializePluginsAsync, and AutoPluginUpdates) does not require additional try-catch error handling according to maintainer Jack251970, as these operations are designed to handle exceptions internally.
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3672
File: Flow.Launcher/MainWindow.xaml.cs:318-318
Timestamp: 2025-06-08T14:12:12.842Z
Learning: In Flow.Launcher, the App.NotifyIcon static property is initialized in the App class before MainWindow creation, so null checks are not needed when accessing App.NotifyIcon in MainWindow lifecycle methods.
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3672
File: Flow.Launcher/MainWindow.xaml.cs:244-247
Timestamp: 2025-06-08T14:12:21.348Z
Learning: In Flow.Launcher, App.NotifyIcon is created before MainWindow creation, so null checks for App.NotifyIcon are not necessary when accessing it from MainWindow code.
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3593
File: Flow.Launcher/HotkeyControlDialog.xaml:6-6
Timestamp: 2025-09-05T11:56:27.267Z
Learning: In Flow.Launcher's migration to iNKORE.UI.WPF.Modern UI framework, dialog resource keys like PopuBGColor, PopupButtonAreaBGColor, PopupButtonAreaBorderColor, and PopupTextColor are provided by the iNKORE.UI.WPF.Modern framework itself, not defined locally in the codebase theme files.
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 4319
File: Flow.Launcher.Core/Resource/Theme.cs:676-682
Timestamp: 2026-03-08T15:54:13.490Z
Learning: In WPF-based C# code reviews, note that Border.BackgroundProperty, Control.BackgroundProperty, and Panel.BackgroundProperty share the same DependencyProperty instance. Because Border and Control add ownership to Panel.BackgroundProperty, comparing a Setter's Property against Control.BackgroundProperty will match setters registered for Border.BackgroundProperty as well. When reviewing code that relies on setter-property identity for BackgroundProperty, assume reference equality across these types and verify behavior accordingly.
There was a problem hiding this comment.
- No need to call dispatcher again.
- If it is already within the UI thread, the priority cannot be changed.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
@coderabbitai For item 4, previously, some callsites used control.Dispatcher (which would exist if the control exists) or would throw (making the bug obvious). With the helper, does it mean you can end up with missing UI updates that are harder to diagnose
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look at the full DispatcherHelper implementation to confirm null handling
cat Flow.Launcher.Core/Resource/DispatcherHelper.cs
Length of output: 3087
@jjw24, that's a valid concern. Here's the breakdown:
For the specific App.xaml.cs case (item 4):
The old code was already _mainWindow?.Dispatcher.Invoke(...) (null-conditional), so both old and new paths silently skip when _mainWindow is null. No behavioral change there specifically.
The broader risk you're pointing at:
The real exposure is the many other call sites that changed from a non-null-conditional pattern:
// Old — throws NullReferenceException if Application.Current is null (bug is obvious)
Application.Current.Dispatcher.Invoke(someAction);
// New — silently no-ops if Application.Current?.Dispatcher resolves to null (bug is hidden)
DispatcherHelper.Invoke(someAction);Confirmed from the implementation — the no-dispatcher overloads delegate to Application.Current?.Dispatcher, and the dispatcher overloads short-circuit with if (dispatcher == null) return; / return default. There's no logging, no assertion, no exception on null.
This means:
| Scenario | Old behavior | New behavior |
|---|---|---|
Application.Current is null at shutdown |
Throws (noisy) | Silently skips (safe) |
Application.Current is unexpectedly null due to a bug |
Throws (obvious) | Silently skips (hidden) |
Func<T> overload, dispatcher is null |
Throws | Returns default(T) — could propagate a wrong value upstream |
In practice, Application.Current being null outside of shutdown is rare in a WPF app, so the risk is mostly theoretical. The silent-skip behavior is actually preferable during shutdown (e.g., avoiding crashes in Dispose). However, if you'd like stronger diagnostics, a Debug.Assert or a conditional log could be added inside DispatcherHelper for null-dispatcher scenarios without affecting release behavior:
public static void Invoke(Dispatcher dispatcher, Action action, DispatcherPriority priority = DispatcherPriority.Normal)
{
if (dispatcher == null)
{
Debug.Assert(false, "DispatcherHelper.Invoke called with null dispatcher");
return;
}
// ...
}This would preserve silent-skip in release builds while making it diagnosable in debug sessions.
🧠 Learnings used
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 4335
File: Flow.Launcher.Core/Resource/DispatcherHelper.cs:63-85
Timestamp: 2026-03-10T04:07:09.482Z
Learning: In Flow.Launcher.Core/Resource/DispatcherHelper.cs, the InvokeAsync overloads use a fast path via CheckAccess(): if already on the UI thread, execute the action/func directly without dispatcher.InvokeAsync, even if a DispatcherPriority is provided. When reviewing this file or related code, assume this is an intentional design choice by the maintainer. Verify that tests or code paths confirm the UI-thread fast-path is employed, and ensure there is no unnecessary dispatching when CheckAccess() is true. Consider adding/maintaining tests that cover InvokeAsync on the UI thread with various priorities, and document that the priority parameter is ignored in the fast-path.
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3854
File: Flow.Launcher/App.xaml.cs:252-275
Timestamp: 2025-10-16T10:48:30.573Z
Learning: In Flow Launcher's App.xaml.cs, the plugin initialization block (lines 252-275) that includes PreStartPluginExecutablePathUpdate, PluginManager.LoadPlugins, PluginManager.InitializePluginsAsync, _mainVM.QueryResults(), and API.SaveAppAllSettings() does not require additional Task.Run wrappers or Dispatcher.InvokeAsync calls according to maintainer Jack251970, as the threading model is already safe as designed.
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3854
File: Flow.Launcher/App.xaml.cs:246-262
Timestamp: 2025-07-21T09:19:49.684Z
Learning: In Flow Launcher's App.xaml.cs, the asynchronous plugin initialization task (containing AbstractPluginEnvironment.PreStartPluginExecutablePathUpdate, PluginManager.LoadPlugins, PluginManager.InitializePluginsAsync, and AutoPluginUpdates) does not require additional try-catch error handling according to maintainer Jack251970, as these operations are designed to handle exceptions internally.
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3672
File: Flow.Launcher/MainWindow.xaml.cs:318-318
Timestamp: 2025-06-08T14:12:12.842Z
Learning: In Flow.Launcher, the App.NotifyIcon static property is initialized in the App class before MainWindow creation, so null checks are not needed when accessing App.NotifyIcon in MainWindow lifecycle methods.
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3672
File: Flow.Launcher/MainWindow.xaml.cs:244-247
Timestamp: 2025-06-08T14:12:21.348Z
Learning: In Flow.Launcher, App.NotifyIcon is created before MainWindow creation, so null checks for App.NotifyIcon are not necessary when accessing it from MainWindow code.
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3593
File: Flow.Launcher/HotkeyControlDialog.xaml:6-6
Timestamp: 2025-09-05T11:56:27.267Z
Learning: In Flow.Launcher's migration to iNKORE.UI.WPF.Modern UI framework, dialog resource keys like PopuBGColor, PopupButtonAreaBGColor, PopupButtonAreaBorderColor, and PopupTextColor are provided by the iNKORE.UI.WPF.Modern framework itself, not defined locally in the codebase theme files.
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 4319
File: Flow.Launcher.Core/Resource/Theme.cs:676-682
Timestamp: 2026-03-08T15:54:13.490Z
Learning: In WPF-based C# code reviews, note that Border.BackgroundProperty, Control.BackgroundProperty, and Panel.BackgroundProperty share the same DependencyProperty instance. Because Border and Control add ownership to Panel.BackgroundProperty, comparing a Setter's Property against Control.BackgroundProperty will match setters registered for Border.BackgroundProperty as well. When reviewing code that relies on setter-property identity for BackgroundProperty, assume reference equality across these types and verify behavior accordingly.
| return Invoke(Application.Current?.Dispatcher, func, priority); | ||
| } | ||
|
|
||
| public static void Invoke(Dispatcher dispatcher, Action action, DispatcherPriority priority = DispatcherPriority.Normal) |
There was a problem hiding this comment.
@coderabbitai give your opinion on the below three points and address these questions:
-
Potentially changes which dispatcher is used- many replacements now use Application.Current.Dispatcher via the helper, instead of a specific DispatcherObject.Dispatcher (like textBox.Dispatcher). In most WPF apps those are the same UI dispatcher, but if any UI object is associated with a different dispatcher, would this change behaviour? How rare would this type of scenario be?
-
Async semantics may change timing and ordering- I feel because Func is now fully awaited when invoked through the helper, some flows could become more sequential (callers now wait for completion where previously they may have effectively “fire-and-forget”-ed part of the work). Can this expose deadlocks or responsiveness issues if any UI-thread-dispatched async work blocks or takes longer than expected?
-
Harder to see “this is definitely UI-thread” at the callsite- centralization improves consistency, but it can obscure that some operations are happening on the UI thread (and should remain small/fast). Would reviewers now have to trust/inspect the helper?
There was a problem hiding this comment.
@jjw24, great questions — here's my assessment of each point:
1. Potentially changing which dispatcher is used
In practice, this is almost always a non-issue. In a standard single-threaded WPF application, every UIElement.Dispatcher property returns the same underlying Dispatcher instance as Application.Current.Dispatcher — they are literally the same object reference, since all UI elements are created on (and owned by) the same thread.
The scenario where a specific DispatcherObject.Dispatcher differs from Application.Current.Dispatcher requires UI objects to be explicitly created on a separate thread with its own Dispatcher loop (e.g., a splash screen or a secondary window on a dedicated UI thread). This pattern is extremely rare, advanced, and nothing in Flow Launcher's codebase suggests it is used.
Additionally, the helper already provides the escape hatch: where a specific dispatcher is needed, the overload Invoke(Dispatcher dispatcher, ...) is available and is already used — e.g., in App.xaml.cs:
// Specific dispatcher still passed explicitly
DispatcherHelper.Invoke(_mainWindow?.Dispatcher, _mainWindow.Dispose)Risk level: Low. The only real risk would be if a future contributor adds a secondary-thread UI element and uses DispatcherHelper.Invoke(action) (no explicit dispatcher) — but that would be a misuse of the helper in a rare scenario, not a defect in the refactoring itself.
2. Async semantics changing timing and ordering
This is the most substantive concern. Two sub-cases to consider:
a) Func<Task> now fully awaited:
Previously, patterns like dispatcher.InvokeAsync(async () => { ... }) would return a DispatcherOperation<Task> — the outer operation completes when the delegate starts, not when the inner Task completes. Callers who didn't await that inner task effectively fire-and-forgot it. The helper now double-awaits, so callers wait for full completion. This makes flows more sequential and deterministic, which is generally safer — but could expose latency if any dispatched async block takes longer than expected.
b) Deadlock risk:
With InvokeAsync (non-blocking dispatch), deadlock risk is low. The bigger concern would be with Invoke (synchronous) — if called from a background thread with a delegate that itself tries to await something back on the UI thread, that would deadlock. However, the Invoke overloads accept Action/Func<T> (not Func<Task>), so async-in-sync is not directly exposed through the helper's API surface.
Concrete example to watch: In Msg.xaml.cs and MsgWithButton.xaml.cs, the nested await Dispatcher.InvokeAsync(...) around fadeOutStoryboard.Begin() was removed — Begin() is now called directly inside the outer DispatcherHelper.InvokeAsync delegate. This is correct (already on the UI thread), but it's worth confirming that the storyboard's Completed event or any continuation still fires as expected under all paths.
Risk level: Medium. The double-await change is a real semantic shift worth explicitly documenting in the helper's XML doc comments for InvokeAsync(Func<Task>), so future callers understand the contract.
3. Harder to see "this is definitely UI-thread" at the callsite
This is a real but manageable readability trade-off. Previously, Application.Current.Dispatcher.Invoke(...) made it visually obvious that UI-thread dispatch was happening. With the helper, DispatcherHelper.Invoke(...) still signals a dispatcher is being used, but the mechanism is one level of abstraction away.
The main reviewer burden becomes: you now need to trust that DispatcherHelper.Invoke always dispatches to the UI thread rather than seeing it spelled out. A few mitigations worth considering:
- XML doc comments on the helper methods clearly stating "executes on the UI thread via
Application.Current.Dispatcher" would help future reviewers. - The fast-path via
CheckAccess()(as noted in a prior learning) is intentional and correct — but this too is now hidden inside the helper, so reviewers inspecting a callsite won't immediately know whether a dispatch actually occurs. - The
#pragma warning disable VSTHRD001suppression at the file level covers all methods — this is fine, but it means static analysis won't flag misuse.
Recommendation: Consider adding a /// <summary> doc comment to each public method in DispatcherHelper explaining the dispatcher source, priority semantics, and the fast-path behavior. This would reduce the "trust the helper" burden for reviewers significantly.
Summary table:
| Concern | Risk | Mitigation |
|---|---|---|
| Wrong dispatcher used | Low | Explicit-dispatcher overloads exist; single-thread WPF norm |
| Async timing/ordering change | Medium | Func<Task> double-await is a real semantic change; add XML docs |
| Reduced callsite clarity | Low–Medium | Add XML doc comments to the helper |
🧠 Learnings used
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 4335
File: Flow.Launcher.Core/Resource/DispatcherHelper.cs:63-85
Timestamp: 2026-03-10T04:07:09.482Z
Learning: In Flow.Launcher.Core/Resource/DispatcherHelper.cs, the InvokeAsync overloads use a fast path via CheckAccess(): if already on the UI thread, execute the action/func directly without dispatcher.InvokeAsync, even if a DispatcherPriority is provided. When reviewing this file or related code, assume this is an intentional design choice by the maintainer. Verify that tests or code paths confirm the UI-thread fast-path is employed, and ensure there is no unnecessary dispatching when CheckAccess() is true. Consider adding/maintaining tests that cover InvokeAsync on the UI thread with various priorities, and document that the priority parameter is ignored in the fast-path.
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3854
File: Flow.Launcher/App.xaml.cs:252-275
Timestamp: 2025-10-16T10:48:30.573Z
Learning: In Flow Launcher's App.xaml.cs, the plugin initialization block (lines 252-275) that includes PreStartPluginExecutablePathUpdate, PluginManager.LoadPlugins, PluginManager.InitializePluginsAsync, _mainVM.QueryResults(), and API.SaveAppAllSettings() does not require additional Task.Run wrappers or Dispatcher.InvokeAsync calls according to maintainer Jack251970, as the threading model is already safe as designed.
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 4319
File: Flow.Launcher.Core/Resource/Theme.cs:676-682
Timestamp: 2026-03-08T15:54:13.490Z
Learning: In WPF-based C# code reviews, note that Border.BackgroundProperty, Control.BackgroundProperty, and Panel.BackgroundProperty share the same DependencyProperty instance. Because Border and Control add ownership to Panel.BackgroundProperty, comparing a Setter's Property against Control.BackgroundProperty will match setters registered for Border.BackgroundProperty as well. When reviewing code that relies on setter-property identity for BackgroundProperty, assume reference equality across these types and verify behavior accordingly.
No logic changes. Added
DispatcherHelperand replace all UI thread invocation with it.Summary by cubic
Centralized UI-thread invocation behind
DispatcherHelperto make cross-thread UI updates safer and reduce boilerplate. Async delegates are fully awaited to avoid premature continuation.Dispatcher.Invoke/InvokeAsyncandCheckAccesswithDispatcherHelper.Invoke/InvokeAsyncacross windows, view models, dialogs, notifications, settings, plugin update prompts, theme/wallpaper helpers, and public API methods for opening settings and plugin settings windows. Used explicitDispatcherPrioritywhere needed and consolidated nested dispatcher calls (e.g., fade-out storyboard). Renamed private helpers to*Corefor clarity (ShowCore,ChangeQueryTextCore/ChangeQueryTextCoreAsync,ReportProgressCore,GetWallpaperBrushCore).Flow.Launcher.Core/Resource/DispatcherHelperwith sync/async overloads forAction,Func<T>, andFunc<Task>, optionalDispatcherparameter,DispatcherPrioritysupport,CheckAccessshort-circuiting, and null-safe handling whenApplication.Currentor aDispatcheris unavailable. Async delegates use double-await so callers wait for completion.CheckAccessbranches, nested dispatcher invocations, and scatteredApplication.Current.Dispatchercalls.DispatcherHelperis static with minimal allocations.Written for commit 66e9e6f. Summary will update on new commits.