Fix possible null reference when query is cancelled & Add support when image format is not support & Other improvements #3099
Conversation
This comment has been minimized.
This comment has been minimized.
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces changes to the error handling logic within the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (5)
Flow.Launcher/Storage/TopMostRecord.cs (3)
Line range hint
1-8: Critical: Address thread safety concernsThe TODO comment indicates that this class is accessed from multiple threads but lacks thread safety mechanisms. This could lead to race conditions and data corruption when concurrent operations modify the
recordsdictionary.Consider these thread-safety options:
- Use
ConcurrentDictionary<string, Record>instead ofDictionary<string, Record>- Add lock mechanisms around dictionary operations
- Implement the double-checked locking pattern for read operations
Example implementation using
ConcurrentDictionary:- public Dictionary<string, Record> records { get; private set; } = new Dictionary<string, Record>(); + public ConcurrentDictionary<string, Record> records { get; private set; } = new ConcurrentDictionary<string, Record>();
Line range hint
15-21: Enhance null-safety in IsTopMost methodWhile the added null check for
result.OriginQueryis good, the method still has potential null reference issues:
- No null check for the
resultparameter itself- The second return statement could throw if
result.OriginQuerybecomes null between checksConsider this safer implementation:
internal bool IsTopMost(Result result) { - if (records.Count == 0 || (result.OriginQuery != null && !records.ContainsKey(result.OriginQuery.RawQuery))) + if (result?.OriginQuery == null || records.Count == 0 || !records.ContainsKey(result.OriginQuery.RawQuery)) { return false; } // since this dictionary should be very small (or empty) going over it should be pretty fast. return records[result.OriginQuery.RawQuery].Equals(result); }
Line range hint
23-39: Add null checks to Remove and AddOrUpdate methodsBoth methods directly access
result.OriginQuery.RawQuerywithout null checks, which could lead to null reference exceptions when handling cancelled queries.Suggested implementation:
internal void Remove(Result result) { + if (result?.OriginQuery == null) return; records.Remove(result.OriginQuery.RawQuery); } internal void AddOrUpdate(Result result) { + if (result?.OriginQuery == null) return; var record = new Record { PluginID = result.PluginID, Title = result.Title, SubTitle = result.SubTitle }; records[result.OriginQuery.RawQuery] = record; }Flow.Launcher.Infrastructure/Image/ImageLoader.cs (2)
218-227: Good addition of specific exception handling for unsupported formats.The try-catch block appropriately handles NotSupportedException for unsupported image formats, preventing crashes and improving robustness.
However, consider these improvements:
- Instead of setting
image = null, use the MissingImage as fallback- Consider catching other potential exceptions (e.g., OutOfMemoryException) that could occur during full image loading
try { image = LoadFullImage(path); type = ImageType.FullImageFile; } -catch (NotSupportedException) +catch (NotSupportedException ex) { - image = null; + Log.Debug($"|ImageLoader.GetThumbnailResult|Unsupported image format for {path}: {ex.Message}"); + image = ImageCache[Constant.MissingImgIcon, false]; type = ImageType.Error; } +catch (Exception ex) when (ex is OutOfMemoryException or InvalidOperationException) +{ + Log.Exception($"|ImageLoader.GetThumbnailResult|Failed to load full image {path}", ex); + image = ImageCache[Constant.MissingImgIcon, false]; + type = ImageType.Error; +}
220-220: Consider optimizing image format validation.The
LoadFullImagemethod makes multiple attempts to load and resize the image. For unsupported formats, this could mean multiple unnecessary attempts before failing.Consider validating the image format before attempting to load:
+private static bool IsFormatSupported(string path) +{ + try + { + using var stream = File.OpenRead(path); + using var decoder = BitmapDecoder.Create( + stream, + BitmapCreateOptions.IgnoreColorProfile, + BitmapCacheOption.None); + return true; + } + catch (NotSupportedException) + { + return false; + } +} try { + if (!IsFormatSupported(path)) + { + image = ImageCache[Constant.MissingImgIcon, false]; + type = ImageType.Error; + return; + } image = LoadFullImage(path); type = ImageType.FullImageFile; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
Flow.Launcher.Infrastructure/Image/ImageLoader.cs(1 hunks)Flow.Launcher/Storage/TopMostRecord.cs(1 hunks)
This comment has been minimized.
This comment has been minimized.
@check-spelling-bot Report🔴 Please reviewSee the 📂 files view, the 📜action log, or 📝 job summary for details.
See ❌ Event descriptions for more information. If the flagged items are 🤯 false positivesIf items relate to a ...
|
|
🥷 Code experts: onesounds onesounds 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: To learn more about /:\ gitStream - Visit our Docs |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
Flow.Launcher/Storage/TopMostRecord.cs (2)
Line range hint
30-37: Add null check and refactor record creationTwo issues to address:
- Missing null check for
result.OriginQuery- Record creation logic could be extracted for reusability
Apply these changes:
+ private static Record CreateRecord(Result result) => new Record + { + PluginID = result.PluginID, + Title = result.Title, + SubTitle = result.SubTitle + }; internal void AddOrUpdate(Result result) { - var record = new Record - { - PluginID = result.PluginID, - Title = result.Title, - SubTitle = result.SubTitle - }; - records.AddOrUpdate(result.OriginQuery.RawQuery, record, (key, oldValue) => record); + if (result.OriginQuery?.RawQuery == null) return; + var record = CreateRecord(result); + records.AddOrUpdate(result.OriginQuery.RawQuery, record, (_, _) => record); }
42-42: Add parameter validation and optimize initializationConsider adding parameter validation and capacity planning for better performance.
Apply this change:
public void Load(Dictionary<string, Record> dictionary) { - records = new ConcurrentDictionary<string, Record>(dictionary); + if (dictionary == null) + throw new ArgumentNullException(nameof(dictionary)); + + var concurrencyLevel = Environment.ProcessorCount; + var capacity = dictionary.Count; + records = new ConcurrentDictionary<string, Record>(concurrencyLevel, capacity); + foreach (var kvp in dictionary) + { + records.TryAdd(kvp.Key, kvp.Value); + } }Flow.Launcher/Storage/UserSelectedRecord.cs (1)
Line range hint
1-109: Consider using ConcurrentDictionary for thread safety.The class uses regular
Dictionaryfor storing records which isn't thread-safe. Since the PR introduces thread safety improvements elsewhere usingConcurrentDictionary, consider applying the same pattern here to prevent potential race conditions inAdd()andGetSelectedCount()methods.Here's the suggested implementation:
- public Dictionary<int, int> recordsWithQuery { get; private set; } + public ConcurrentDictionary<int, int> recordsWithQuery { get; private set; } public UserSelectedRecord() { - recordsWithQuery = new Dictionary<int, int>(); + recordsWithQuery = new ConcurrentDictionary<int, int>(); } public void Add(Result result) { TransformOldRecords(); var keyWithQuery = GenerateQueryAndResultHashCode(result.OriginQuery, result); - if (!recordsWithQuery.TryAdd(keyWithQuery, 1)) - recordsWithQuery[keyWithQuery]++; + recordsWithQuery.AddOrUpdate(keyWithQuery, 1, (_, count) => count + 1); var keyWithoutQuery = GenerateResultHashCode(result); - if (!recordsWithQuery.TryAdd(keyWithoutQuery, 1)) - recordsWithQuery[keyWithoutQuery]++; + recordsWithQuery.AddOrUpdate(keyWithoutQuery, 1, (_, count) => count + 1); }Flow.Launcher/ViewModel/MainViewModel.cs (2)
1450-1476: Consider enhancing error handling and logging.While the addition of error handling is good, there are a few improvements that could be made:
- Consider catching specific exceptions rather than using a general catch-all
- The debug log could be enhanced to include the full exception details including stack trace for better troubleshooting
Here's a suggested improvement:
try { foreach (var metaResults in resultsForUpdates) { foreach (var result in metaResults.Results) { if (_topMostRecord.IsTopMost(result)) { result.Score = int.MaxValue; } else { var priorityScore = metaResults.Metadata.Priority * 150; result.Score += _userSelectedRecord.GetSelectedCount(result) + priorityScore; } } } bool reSelect = resultsForUpdates.First().ReSelectFirstResult; Results.AddResults(resultsForUpdates, token, reSelect); } -catch (Exception ex) +catch (InvalidOperationException ex) { - Log.Debug("MainViewModel", $"Error in UpdateResultView: {ex.Message}"); + Log.Error("MainViewModel", "Error in UpdateResultView", ex); +} +catch (Exception ex) +{ + Log.Error("MainViewModel", "Unexpected error in UpdateResultView", ex); }
1456-1464: Consider extracting magic numbers into constants.The scoring logic uses a magic number (150) for priority score calculation. This should be extracted into a named constant for better maintainability and clarity.
+private const int PRIORITY_SCORE_MULTIPLIER = 150; + if (_topMostRecord.IsTopMost(result)) { result.Score = int.MaxValue; } else { - var priorityScore = metaResults.Metadata.Priority * 150; + var priorityScore = metaResults.Metadata.Priority * PRIORITY_SCORE_MULTIPLIER; result.Score += _userSelectedRecord.GetSelectedCount(result) + priorityScore; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
Flow.Launcher/MainWindow.xaml(1 hunks)Flow.Launcher/Storage/TopMostRecord.cs(2 hunks)Flow.Launcher/Storage/UserSelectedRecord.cs(2 hunks)Flow.Launcher/ViewModel/MainViewModel.cs(6 hunks)
🔇 Additional comments (6)
Flow.Launcher/Storage/TopMostRecord.cs (3)
1-1: LGTM: Good improvement for thread safety
The switch to ConcurrentDictionary is a solid improvement that will prevent potential race conditions in a multi-threaded environment.
Also applies to: 11-11
15-21:
Strengthen null check protection
While the initial null check is good, the return statement still directly accesses result.OriginQuery.RawQuery without a null check, which could throw a NullReferenceException.
Apply this fix:
- return result.OriginQuery != null && records[result.OriginQuery.RawQuery].Equals(result);
+ if (result.OriginQuery?.RawQuery == null) return false;
+ return records.TryGetValue(result.OriginQuery.RawQuery, out var record) && record.Equals(result);9-11: Verify concurrent access patterns
Let's verify how this class is accessed concurrently to ensure our thread safety improvements are sufficient.
✅ Verification successful
Let me gather more information about how TopMostRecord is used in MainViewModel.cs to understand the concurrent access patterns better.
Based on the analysis of the code, I can now generate the final response:
Thread safety implementation is appropriate
The ConcurrentDictionary usage in TopMostRecord is well-justified. The evidence shows that:
- TopMostRecord is accessed from multiple contexts including async query operations and UI operations
- The access patterns are simple CRUD operations (AddOrUpdate, Remove, IsTopMost) which are all thread-safe with ConcurrentDictionary
- The class is used in result scoring during async query operations while also being modified through UI actions
- All operations on _topMostRecord are atomic and don't require additional synchronization
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usages of TopMostRecord class
rg "TopMostRecord" -B 2 -A 2
# Search for async/await patterns near TopMostRecord usage
ast-grep --pattern 'async $_ $_($$$) {
$$$
TopMostRecord
$$$
}'
Length of output: 1863
Script:
#!/bin/bash
# Look for methods in MainViewModel that use _topMostRecord
rg "_topMostRecord\." -B 5 -A 5 "Flow.Launcher/ViewModel/MainViewModel.cs"
# Search for any async methods in MainViewModel
ast-grep --pattern 'async $_ $_($$$) {
$$$
}' "Flow.Launcher/ViewModel/MainViewModel.cs"
# Look for any thread-related code near TopMostRecord usage
rg "Task|Thread|Parallel|async|await" -B 2 -A 2 "Flow.Launcher/ViewModel/MainViewModel.cs"
Length of output: 11432
Flow.Launcher/Storage/UserSelectedRecord.cs (1)
54-58: LGTM! Null check prevents NullReferenceException.
The added null check properly handles cancelled queries by falling back to result-only hash code generation, preventing the NullReferenceException when accessing query properties.
Flow.Launcher/MainWindow.xaml (1)
311-311: LGTM! Verify ViewModel property binding.
The change from PluginIconPath to PluginIconSource aligns with the PR objectives to improve image format support and null reference handling.
Let's verify the corresponding ViewModel property exists:
✅ Verification successful
The previous command might have failed due to the pattern syntax. Let's try a broader search to:
- Find any occurrence of PluginIconSource property
- Locate the MainViewModel file to verify the property exists
Property binding verified and implemented correctly ✓
The PluginIconSource property exists in MainViewModel.cs with proper implementation:
- Defined as
public ImageSource PluginIconSource { get; private set; } - Properly initialized and managed alongside
PluginIconPath - Used with
ImageLoader.LoadAsync()for loading the image source
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the PluginIconSource property exists in MainViewModel
ast-grep --pattern 'class MainViewModel {
$$$
public ImageSource PluginIconSource { get; set; }
$$$
}'
Length of output: 110
Script:
#!/bin/bash
# Search for PluginIconSource property in all files
rg "PluginIconSource" -A 2 -B 2
# Search for files containing MainViewModel to ensure we're looking in the right places
fd MainViewModel
Length of output: 2148
Flow.Launcher/ViewModel/MainViewModel.cs (1)
26-27: LGTM: Image handling changes look good!
The addition of PluginIconSource property and related imports properly supports asynchronous image loading while maintaining null safety.
Also applies to: 727-728
Jack251970
changed the title
This PR contains those fixes and improvements
It will happen when one query is cancelled and can cause query bar not working like this:
https://github.com/user-attachments/assets/8a0a2c0f-6db6-4061-8ff6-a979408fc6ff
Throw exception from VS:
It add support for images whose formats are not support for preview like this:
Before it is catch in outer try catch sentences, and the icon of images will be
AppMissingIcon.png, which is quite strange for.pngextension.Now just do not show the preview like this:
Introduce concurrent directionary for topmost record.
Make sure TopMostRecord is thread safe.
Fix modified collection enumeration issue when updating results, which can also cause the same problem like 1.
Fix xaml convert issue and use icon loader for PluginActivationIcon.