Refactor UI thread invocation with DispatcherHelper#4335
Refactor UI thread invocation with DispatcherHelper#4335Jack251970 wants to merge 5 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.
prlabeler
bot
added
bug
This comment has been minimized.
This comment has been minimized.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
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.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a new centralized WPF dispatcher utility (DispatcherHelper) and replaces direct Dispatcher/CheckAccess usages across many UI-related files, routing synchronous and asynchronous UI work through DispatcherHelper and extracting small core methods where appropriate. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller (View / ViewModel / Helper)
participant Helper as DispatcherHelper
participant Disp as Dispatcher (Application.Current.Dispatcher)
participant UI as UI Control / Window
Caller->>Helper: Invoke(Action) / InvokeAsync(Func<Task>) / Invoke<T>(...)
Helper->>Disp: Resolve Dispatcher (implicit or explicit)
alt Dispatcher is null or Disp.CheckAccess == true
Helper->>UI: Execute action directly
else
Helper->>Disp: Dispatcher.Invoke / InvokeAsync -> marshal to UI
Disp->>UI: Execute action on UI thread
end
UI-->>Caller: result / completion
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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>
No logic changes. Added
DispatcherHelperand replace all UI thread invocation with it.Summary by cubic
Centralized UI-thread access behind a new
DispatcherHelperto make cross-thread UI updates safer and reduce boilerplate. Async delegates are fully awaited to prevent premature continuation.Dispatcher.Invoke/InvokeAsyncandCheckAccesswithDispatcherHelper.Invoke/InvokeAsyncacross windows, view models, dialogs, notifications, settings, plugin update prompts, and theme/wallpaper helpers (e.g.,Theme.RefreshFrameAsync/SetBlurForWindowAsync,ReleaseNotesWindow,JsonRPCPluginSettings,SingleInstance,ProgressBoxEx,MessageBoxEx). Consolidated nested dispatcher calls (e.g., fade-out storyboard) and used explicitDispatcherPrioritywhere needed. Renamed internal helpers to*Core(e.g.,ShowCore,ChangeQueryTextCore/ChangeQueryTextCoreAsync,ReportProgressCore,GetWallpaperBrushCore) for clarity.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. ForFunc<Task>, uses double-await so callers wait for delegate completion.CheckAccessbranches, nested dispatcher invocations, and scatteredApplication.Current.Dispatchercalls.DispatcherHelperis static with minimal allocations.Written for commit 95fc3eb. Summary will update on new commits.