Draft
Conversation
…validation on Windows theme change Co-authored-by: Jack251970 <53996452+Jack251970@users.noreply.github.com>
|
Be a legend 🏆 by adding a before and after screenshot of the changes you made, especially if they are around UI/UX. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughWraps resource dictionary updates in defensive checks, adds a private helper to derive CaretBrush values from Foreground safely, freezes newly created brushes for immutability, makes CopyStyle static, hardens dispatcher usage in theme refresh/blur async flows, and limits frame refresh on theme change to the System color scheme. (≈33 words) Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as MainWindow
participant Theme as Theme
participant Disp as Dispatcher
participant Win as Window
UI->>Theme: ActualApplicationThemeChanged(event)
alt ColorScheme == System
Theme->>Disp: RefreshFrameAsync()
Disp-->>Theme: Ensure on UI thread (reinvoke if needed)
Theme->>Theme: Resolve Foreground (including DynamicResource)
Theme->>Theme: GetNewCaretValue(foreground)
Theme->>Theme: Create new SolidColorBrush & Freeze()
Theme->>Win: Apply brush/setters (CopyStyle called statically)
Theme->>Disp: SetBlurForWindowAsync() (dispatcher guard)
else
UI-->>Theme: Skip RefreshFrameAsync
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request suppresses a spurious error dialog that appears when users change Windows themes or accent colors. The error is caused by a WPF framework bug where SystemResources.InvalidateTreeResources incorrectly attempts to clone Color structs as if they were Freezable or Expression objects, resulting in an InvalidCastException that the application cannot prevent.
Changes:
- Added
IsRecoverableSystemResourceExceptionmethod to detect the specific WPF system resource invalidation exception - Integrated the new exception check into
ErrorReporting.Reportto suppress the error dialog while still logging the exception - Added comprehensive documentation explaining the WPF framework issue
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| Flow.Launcher/Helper/ExceptionHelper.cs | Added new method to detect InvalidCastException from WPF's SystemResources.InvalidateTreeResources |
| Flow.Launcher/Helper/ErrorReporting.cs | Integrated the new exception check to prevent error dialog from being shown |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ource invalidation on Windows theme change" This reverts commit 372f142.
Refactored caret and background brush assignment to avoid sharing mutable instances and ensure proper resource referencing. Added GetNewCaretValue helper for safe caret brush creation. Brushes for backgrounds are now frozen for performance. Improved foreground value retrieval and dynamic resource key extraction. Made some methods static for clarity and consistency. Enhances resource management and reliability in theme handling.
coderabbitai
bot
removed
bug
Jack251970
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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.Core/Resource/Theme.cs (1)
814-819:⚠️ Potential issue | 🟡 MinorRemove ToString() when looking up DynamicResource keys in ResourceDictionary.
At line 814, converting
dynamicResource.ResourceKeyto string viaToString()violates WPF resource lookup semantics.ResourceDictionaryrequires matching the actual key object (e.g.,ComponentResourceKey), not its string representation. This inconsistency is evident at line 260, where the code correctly preservesResourceKeyobject identity.Proposed fix
- var resourceKey = dynamicResource.ResourceKey.ToString(); + var resourceKey = dynamicResource.ResourceKey; - if (Resources.Contains(resourceKey)) + if (resourceKey != null && Resources.Contains(resourceKey)) { var colorResource = Resources[resourceKey];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Flow.Launcher.Core/Resource/Theme.cs` around lines 814 - 819, The code is converting dynamicResource.ResourceKey to a string before lookup which breaks WPF resource key identity; in the Theme.cs block around the dynamicResource handling, stop calling ToString() and use the ResourceKey object directly when checking and retrieving from the ResourceDictionary (i.e., replace the use of var resourceKey = dynamicResource.ResourceKey.ToString() and subsequent Resources.Contains(resourceKey)/Resources[resourceKey] calls with checks that use dynamicResource.ResourceKey itself so ComponentResourceKey and other non-string keys resolve correctly).
🤖 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.Core/Resource/Theme.cs`:
- Around line 814-819: The code is converting dynamicResource.ResourceKey to a
string before lookup which breaks WPF resource key identity; in the Theme.cs
block around the dynamicResource handling, stop calling ToString() and use the
ResourceKey object directly when checking and retrieving from the
ResourceDictionary (i.e., replace the use of var resourceKey =
dynamicResource.ResourceKey.ToString() and subsequent
Resources.Contains(resourceKey)/Resources[resourceKey] calls with checks that
use dynamicResource.ResourceKey itself so ComponentResourceKey and other
non-string keys resolve correctly).
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Previously, the theme was refreshed on every theme change event. Now, `_theme.RefreshFrameAsync()` is only called if the color scheme setting is set to "System", preventing unnecessary refreshes in other scenarios.
|
🥷 Code experts: jjw24 jjw24, Jack251970 have 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: ✨ Comment |
Refactored RefreshFrameAsync and SetBlurForWindowAsync to check Dispatcher access before invoking UI updates, ensuring thread safety and reducing unnecessary Dispatcher calls. Also removed redundant code and unused using directive.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Flow.Launcher.Core/Resource/Theme.cs (1)
829-833:⚠️ Potential issue | 🟡 MinorRemove
ToString()and add null check for ResourceKey before ResourceDictionary lookup.
ResourceDictionaryacceptsobjectkeys, not just strings. CallingToString()ondynamicResource.ResourceKeycan fail to find keys if they're non-string types, and doesn't guard against null values.🔧 Proposed fix
- var resourceKey = dynamicResource.ResourceKey.ToString(); + var resourceKey = dynamicResource.ResourceKey; // find key in resource and return color. - if (Resources.Contains(resourceKey)) + if (resourceKey != null && Resources.Contains(resourceKey)) { var colorResource = Resources[resourceKey];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Flow.Launcher.Core/Resource/Theme.cs` around lines 829 - 833, The code in Theme.cs currently calls dynamicResource.ResourceKey.ToString() and then uses Resources.Contains(resourceKey); change this to first check for null on dynamicResource.ResourceKey and, if non-null, use the ResourceKey object itself (not ToString()) when checking and retrieving from the ResourceDictionary. In other words, replace the ToString() usage with a null-checked object reference to dynamicResource.ResourceKey and pass that object into Resources.Contains and subsequent lookup so non-string keys are handled correctly (refer to dynamicResource.ResourceKey and the Resources.Contains(...) / ResourceDictionary lookup in Theme.cs).
♻️ Duplicate comments (2)
Flow.Launcher.Core/Resource/Theme.cs (2)
679-683:⚠️ Potential issue | 🔴 CriticalDispatch the correct callback in
SetBlurForWindowAsync.Line 681 invokes
RefreshFrameAsyncinstead ofSetBlurForWindowAsync, so this method’s blur path is not what runs when called off the UI thread.🔧 Proposed fix
public async Task SetBlurForWindowAsync() { - if (Application.Current?.Dispatcher.CheckAccess() != true) + var dispatcher = Application.Current?.Dispatcher; + if (dispatcher == null) + { + return; + } + + if (!dispatcher.CheckAccess()) { - await Application.Current?.Dispatcher.InvokeAsync(RefreshFrameAsync); + await dispatcher.InvokeAsync(SetBlurForWindowAsync).Task.Unwrap(); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Flow.Launcher.Core/Resource/Theme.cs` around lines 679 - 683, The dispatcher call in SetBlurForWindowAsync is invoking RefreshFrameAsync instead of re-dispatching SetBlurForWindowAsync, so the blur logic isn't executed when called off the UI thread; update the code inside SetBlurForWindowAsync to call await Application.Current?.Dispatcher.InvokeAsync(() => SetBlurForWindowAsync(window, enable, animate)) (or the appropriate delegate that calls SetBlurForWindowAsync with the original parameters) so the same method runs on the UI thread rather than RefreshFrameAsync, preserving await/async semantics and returning afterward.
647-651:⚠️ Potential issue | 🟠 MajorFix dispatcher null-safety and nested task awaiting in
RefreshFrameAsync.Line 649 still uses
Application.Current?.Dispatcher.InvokeAsync(RefreshFrameAsync); ifApplication.Currentis null this becomes an await on null. Also this dispatchesFunc<Task>and should explicitly await the underlying operation task chain.🔧 Proposed fix
public async Task RefreshFrameAsync() { - if (Application.Current?.Dispatcher.CheckAccess() != true) + var dispatcher = Application.Current?.Dispatcher; + if (dispatcher == null) + { + return; + } + + if (!dispatcher.CheckAccess()) { - await Application.Current?.Dispatcher.InvokeAsync(RefreshFrameAsync); + await dispatcher.InvokeAsync(RefreshFrameAsync).Task.Unwrap(); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Flow.Launcher.Core/Resource/Theme.cs` around lines 647 - 651, RefreshFrameAsync currently does await Application.Current?.Dispatcher.InvokeAsync(RefreshFrameAsync) which can await null if Application.Current is null and also dispatches a Func<Task> without awaiting the inner task; fix by caching local var dispatcher = Application.Current?.Dispatcher, null-checking it (return if null), use dispatcher.CheckAccess() on that local, and when dispatching call dispatcher.InvokeAsync(() => RefreshFrameAsync()) and await the returned operation's Task (or Unwrap the Task<Task>) so the inner RefreshFrameAsync is awaited properly; reference RefreshFrameAsync, Application.Current, Dispatcher.InvokeAsync and Dispatcher.CheckAccess in the change.
🤖 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/Theme.cs`:
- Around line 261-290: GetNewCaretValue currently can return non-Brush values
(e.g., a Color) which may be assigned to TextBoxBase.CaretBrushProperty; update
GetNewCaretValue to explicitly handle when foregroundPropertyValue is a Color by
constructing a SolidColorBrush from that Color (set Opacity/Freeze as done for
SolidColorBrush branch) and return that brush; keep existing handling for
DynamicResourceExtension and SolidColorBrush, and ensure the method never
returns a raw Color so CaretBrush always receives a Brush.
- Around line 109-122: The add/remove logic currently sets _oldResource to the
new dictionary before confirming the add and swallows InvalidCastException,
causing _oldResource to become null and later NREs; fix by not assigning
_oldResource until after the new dictionary is successfully added (move the
_oldResource = dictionaryToUpdate assignment inside the try block after
Application.Current.Resources.MergedDictionaries.Add(...)), catch the
InvalidCastException and log the full exception (include ex.ToString()) rather
than swallowing it, and do not set _oldResource = null in the catch—either leave
the prior dictionary in place or restore it if you removed it earlier; also add
a defensive null-check where _oldResource.Source.AbsolutePath is accessed to
avoid NREs.
---
Outside diff comments:
In `@Flow.Launcher.Core/Resource/Theme.cs`:
- Around line 829-833: The code in Theme.cs currently calls
dynamicResource.ResourceKey.ToString() and then uses
Resources.Contains(resourceKey); change this to first check for null on
dynamicResource.ResourceKey and, if non-null, use the ResourceKey object itself
(not ToString()) when checking and retrieving from the ResourceDictionary. In
other words, replace the ToString() usage with a null-checked object reference
to dynamicResource.ResourceKey and pass that object into Resources.Contains and
subsequent lookup so non-string keys are handled correctly (refer to
dynamicResource.ResourceKey and the Resources.Contains(...) / ResourceDictionary
lookup in Theme.cs).
---
Duplicate comments:
In `@Flow.Launcher.Core/Resource/Theme.cs`:
- Around line 679-683: The dispatcher call in SetBlurForWindowAsync is invoking
RefreshFrameAsync instead of re-dispatching SetBlurForWindowAsync, so the blur
logic isn't executed when called off the UI thread; update the code inside
SetBlurForWindowAsync to call await
Application.Current?.Dispatcher.InvokeAsync(() => SetBlurForWindowAsync(window,
enable, animate)) (or the appropriate delegate that calls SetBlurForWindowAsync
with the original parameters) so the same method runs on the UI thread rather
than RefreshFrameAsync, preserving await/async semantics and returning
afterward.
- Around line 647-651: RefreshFrameAsync currently does await
Application.Current?.Dispatcher.InvokeAsync(RefreshFrameAsync) which can await
null if Application.Current is null and also dispatches a Func<Task> without
awaiting the inner task; fix by caching local var dispatcher =
Application.Current?.Dispatcher, null-checking it (return if null), use
dispatcher.CheckAccess() on that local, and when dispatching call
dispatcher.InvokeAsync(() => RefreshFrameAsync()) and await the returned
operation's Task (or Unwrap the Task<Task>) so the inner RefreshFrameAsync is
awaited properly; reference RefreshFrameAsync, Application.Current,
Dispatcher.InvokeAsync and Dispatcher.CheckAccess in the change.
Added early returns if Application.Current is null in RefreshFrameAsync and SetBlurForWindowAsync to prevent null reference exceptions. Updated dispatcher access checks and ensured the correct method is invoked asynchronously for each case, improving robustness and reliability.
Jack251970
force-pushed
the
copilot/fix-cast-exception-bug
branch
from
e90aa02 to
84ed1fa
Compare
auto-merge was automatically disabled
March 1, 2026 04:16
Pull request was converted to draft
Improve IsBlurTheme to check blur support directly in the current theme's resource dictionary, ensuring precise and context-specific detection. Updated related logic and comments for clarity.
Renamed the variable themeName to theme in the UpdateFonts method to improve naming consistency. No functional changes were made.
Reworked how drop shadow effects are added and removed by creating new unsealed Style instances instead of modifying sealed ones. Resources, triggers, and setters are copied, with margin adjustments and effect setter management to ensure reliability. Introduced GetNewWindowBorderStyle helper for style copying.
Explicitly set DispatcherPriority.Render when invoking RefreshFrameAsync and SetBlurForWindowAsync on the application's dispatcher. This ensures theme-related UI changes are processed with rendering priority, improving responsiveness and visual consistency. Also added System.Windows.Threading import for access to DispatcherPriority.
Refactored theme resource dictionary lookups to safely check for "WindowBorderStyle" before accessing, preventing potential exceptions. Updated three methods to handle missing styles gracefully and standardized resource key checks. GetWindowBorderStyleBackground now returns transparent if style is missing.
Renamed IsBlurTheme to IsThemeBlurEnabled and moved system backdrop support checks out of the helper function. Blur availability is now determined by combining theme settings and system capability in the calling code. Updated all usages to reflect the new logic for improved clarity.
Refactored dark mode logic to first use user's ColorScheme preference when "SystemBG" is set to "Auto". Only falls back to system dark mode setting if ColorScheme is not explicitly set to "Dark" or "Light". This ensures user preferences take precedence over system defaults.
Freeze SolidColorBrush instances before assigning them to mainWindow.Background to improve performance and immutability. Applied to both transparent and selected background scenarios.
Replaces GetNewWindowBorderStyle with direct instantiation of a new Style for Border and uses CopyStyle to transfer properties. Removes the old helper method and moves style copying logic inline for improved clarity and maintainability.
Changed assignment of WindowBorderStyle to use Application.Current.Resources for global effect, replacing previous use of _oldResource. This ensures theme changes are applied consistently across the application.
Jack251970
changed the title
This was referenced Mar 4, 2026
Jack251970
removed
bug
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.
This pull request makes several improvements and bug fixes to the theme and window styling logic in
Theme.cs. The main focus is on making theme resource management more robust, improving drop shadow effect handling, ensuring safe brush usage, and enhancing dispatcher/thread safety for UI updates.Theme resource management and robustness:
InvalidCastException, preventing crashes when resource dictionaries contain unexpected types. (UpdateResourceDictionary, Flow.Launcher.Core/Resource/Theme.csL103-R124)RefreshFrameAsync,SetBlurForWindowAsync) to ensure thread safety and avoid exceptions when called from non-UI threads. (RefreshFrameAsync, [1];SetBlurForWindowAsync, [2]Drop shadow and style application:
AddDropShadowEffectToCurrentTheme,RemoveDropShadowEffectFromCurrentTheme, Flow.Launcher.Core/Resource/Theme.csL480-R632)AutoDropShadow, [1] [2]Brush and resource safety:
GetNewCaretValue, Flow.Launcher.Core/Resource/Theme.csR262-R292)SetBlurForWindow, Flow.Launcher.Core/Resource/Theme.csL670-R780)Resource dictionary and style retrieval improvements:
GetWindowBorderStyleBackground, Flow.Launcher.Core/Resource/Theme.csL749-R850)GetWindowBorderStyleBackground, Flow.Launcher.Core/Resource/Theme.csL768-R873)Minor refactors and cleanups:
Most important changes:
Theme resource management and thread safety:
InvalidCastException, preventing crashes when adding new theme resources.RefreshFrameAsyncandSetBlurForWindowAsync. [1] [2]Drop shadow and style handling:
Brush and resource safety:
Resource dictionary and style retrieval: