Fix cast object of type 'System.Windows.Media.Color' to type 'System.Windows.Expression' exception#4302
Open
Jack251970 wants to merge 9 commits intodevfrom
Open
Fix cast object of type 'System.Windows.Media.Color' to type 'System.Windows.Expression' exception#4302Jack251970 wants to merge 9 commits intodevfrom
Jack251970 wants to merge 9 commits intodevfrom
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. |
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 | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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
🤖 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 670-679: The dispatcher is invoking RefreshFrameAsync instead of
running the blur logic; replace the dispatched callback so that the UI-thread
invocation calls SetBlurForWindowAsync (not RefreshFrameAsync) and await it so
the blur-setting logic runs on the UI thread. Locate the method containing the
current dispatcher check (uses Application.Current?.Dispatcher.CheckAccess()),
use GetActualValue() to obtain backdropType, and ensure you dispatch/await
SetBlurForWindowAsync (or, if only a sync SetBlurForWindow exists, dispatch the
call that passes _settings.Theme and the backdropType to SetBlurForWindow) so
SetBlurForWindow/SetBlurForWindowAsync executes when coming from a non-UI
thread.
- Around line 640-644: The current code awaits the DispatcherOperation returned
by Application.Current.Dispatcher.InvokeAsync(RefreshFrameAsync), which only
waits for the dispatch to be queued and will throw if Application.Current is
null; instead ensure Application.Current and its Dispatcher are non-null,
capture the DispatcherOperation (from InvokeAsync(RefreshFrameAsync)) and then
await the operation's Task (or Unwrap the nested Task) so RefreshFrameAsync has
completed before returning; if Application.Current is null call await
RefreshFrameAsync() directly. Use the symbols RefreshFrameAsync and
Application.Current.Dispatcher.InvokeAsync to locate and update the code.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 1 file (changes from recent commits).
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/Theme.cs">
<violation number="1" location="Flow.Launcher.Core/Resource/Theme.cs:640">
P1: Two issues with dispatcher marshalling: (1) If `Application.Current` is null, `Application.Current?.Dispatcher` evaluates to null, and `await null` will throw `NullReferenceException`. (2) `InvokeAsync(RefreshFrameAsync)` returns `DispatcherOperation<Task>` — awaiting it only waits for the dispatch to start, not for `RefreshFrameAsync` to complete. To properly await the inner async method, use `.Task` (e.g., `await dispatcher.InvokeAsync(RefreshFrameAsync).Task`). Consider extracting the dispatcher to a local variable and adding a null guard.</violation>
<violation number="2" location="Flow.Launcher.Core/Resource/Theme.cs:642">
P2: `SetBlurForWindowAsync` dispatches `RefreshFrameAsync` on non-UI threads, causing unintended drop-shadow/frame side effects instead of only applying blur.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Moved RemoveDropShadowEffectFromCurrentTheme() into specific conditional branches within AutoDropShadow to ensure it is only called when appropriate. Explicitly set window corner preference and drop shadow effect based on theme and blur support, improving appearance consistency and logic clarity.
Add try-catch to UpdateResourceDictionary to prevent crashes from InvalidCastException when updating resources. Set _oldResource to null on error. Suppress VSTHRD103 warnings around SetBlurForWindow to clarify intentional async usage and avoid build warnings.
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
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/Theme.cs">
<violation number="1" location="Flow.Launcher.Core/Resource/Theme.cs:121">
P2: Setting `_oldResource` to null in the `InvalidCastException` path creates a null-dereference path in `ChangeTheme` where `_oldResource.Source` is accessed unguarded.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
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
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 improves how brush and resource values are handled for theme styling in
Flow.Launcher.Core/Resource/Theme.cs, focusing on safer management of mutable objects and resource references. The main changes enhance the way caret and background brushes are assigned, ensuring that brushes are properly frozen when possible, and that dynamic resources are referenced correctly.Resolve #4298.