Raise PropertyChanged when ResultViewModel image properties are set - #4624
Open
TrueCrimeDev wants to merge 1 commit into
Open
Raise PropertyChanged when ResultViewModel image properties are set#4624TrueCrimeDev wants to merge 1 commit into
TrueCrimeDev wants to merge 1 commit into
Conversation
The Image, BadgeImage, and PreviewImage setters never raised PropertyChanged, even though the LoadImageAsync comments asserted that "modifying the property" was supposed to trigger the event. The notify call was lost in 2021 when the LazyAsync<ImageSource> wrapper was inlined (e8691c2). The bug usually hides because subsequent identical IcoPath lookups hit ImageCache synchronously in the getter, so the binding sees the loaded image on its first evaluation. But when a brand-new IcoPath comes in and the host doesn't virtualize the row out and back in (e.g., a single-result query like a chat plugin's "Ask" preview), the field is mutated post-await and the binding stays on LoadingImage forever. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Copilot stopped reviewing on behalf of
TrueCrimeDev due to an error
August 17, 2026 15:57
Contributor
|
Caution Review failedAn error occurred during the review process. Please try again later. 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 |
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.
Problem
ResultViewModel.Image,BadgeImage, andPreviewImagenever raisePropertyChangedwhen set.LoadImageAsyncawaits the image load and then assigns the property, and its comment states that "modifying the property" triggers the change notification — but the setters are plain field assignments:Root cause
The notification was lost in e8691c2 (May 2021, "No need for the wrapped Lazy instance in image loading"), which inlined the
LazyAsync<ImageSource>wrapper. That wrapper'supdateCallbackwas what calledOnPropertyChanged(nameof(Image)); when it was removed, nothing replaced it.Why it usually doesn't show
A repeat lookup of the same
IcoPathhitsImageCachesynchronously inside the getter, so the binding already sees the real image the first time it evaluates. The bug only surfaces when a brand-newIcoPatharrives and the host doesn't virtualize the row out and back in — e.g. a single-result query such as a chat plugin's "Ask" preview. There the field is mutated after the await, the binding is never re-evaluated, and the row stays onLoadingImageindefinitely.Fix
Raise
OnPropertyChanged()in all three image setters.ResultViewModelalready derives fromBaseModel, so no other plumbing is needed.Testing
dotnet build Flow.Launcher.sln -c Release— 0 errorsdotnet test -c Release— 433 passedImageLoaderTests.ShellThumbnailFailure_Directory_ReturnsDefaultFolderImageAsyncfails on the first run against a fresh build output on my machine, but it fails identically on unmodifieddev, so it is unrelated to this change.Summary by cubic
Summary of changes
Ensures
ResultViewModelimage properties notify bindings after async loads so icons refresh. Previously,Image,BadgeImage, andPreviewImagesetters did not raisePropertyChanged, so some results stayed onLoadingImage.OnPropertyChanged()in the setters forImage,BadgeImage, andPreviewImage, restoring behavior lost when theLazyAsync<ImageSource>wrapper was removed.dev.Release Note
Fixes an issue where some result icons stayed stuck on the loading image by updating them automatically once the icon finishes loading.
Written for commit 4ee56ab. Summary will update on new commits.