Fix Issue that Plugin Cannot Cache Records #3257
Conversation
This comment has been minimized.
This comment has been minimized.
|
Be a legend 🏆 by adding a before and after screenshot of the changes you made, especially if they are around UI/UX. |
📝 WalkthroughWalkthroughThis pull request updates several components for improved type safety and result handling. In the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
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: 0
🧹 Nitpick comments (2)
Flow.Launcher/ViewModel/MainViewModel.cs (2)
417-431: Consider adding a null check
Ifresultsmight be null in some future usage, consider early returning an empty list instead of throwing aNullReferenceException.
1497-1499: Use the actualMaxScoreconstant
Here, a literal100000is assigned, but the comment referencesResult.MaxScore, which isint.MaxValue. To avoid confusion and align with the constant, consider:- result.Score = 100000; //Result.MaxScore; + result.Score = Result.MaxScore;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Flow.Launcher.Core/Plugin/PluginManager.cs(1 hunks)Flow.Launcher.Plugin/Result.cs(1 hunks)Flow.Launcher/ViewModel/MainViewModel.cs(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
🔇 Additional comments (4)
Flow.Launcher/ViewModel/MainViewModel.cs (2)
234-235: Deep cloning is a clean approach here
These lines ensure each plugin's changes won't inadvertently mutate the Flow Launcher's results. Looks good.
1199-1208: Graceful handling of null results
Falling back to_emptyResultis a robust choice, preventing null reference errors.Flow.Launcher.Plugin/Result.cs (1)
269-271: Null default for RecordKey
Changing from empty string to null aligns the code with older data.Flow.Launcher.Core/Plugin/PluginManager.cs (1)
284-284: Upgrade toIReadOnlyList<Result>
Switching fromList<Result>to a read-only list improves immutability and clarity.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Flow.Launcher/ViewModel/MainViewModel.cs (1)
417-431: Optimize the deep clone implementation.The implementation can be improved in the following ways:
- Remove unnecessary
.ToList()call as the input is alreadyIReadOnlyList.- Add null check for the input parameter.
Apply this diff to optimize the implementation:
private static IReadOnlyList<Result> DeepCloneResults(IReadOnlyList<Result> results, CancellationToken token = default) { + if (results == null) + return _emptyResult; + var resultsCopy = new List<Result>(); - foreach (var result in results.ToList()) + foreach (var result in results) { if (token.IsCancellationRequested) { break; } var resultCopy = result.Clone(); resultsCopy.Add(resultCopy); } return resultsCopy; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Flow.Launcher/ViewModel/MainViewModel.cs(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
🔇 Additional comments (3)
Flow.Launcher/ViewModel/MainViewModel.cs (3)
234-236: LGTM! Good use of deep cloning to prevent side effects.The implementation correctly clones the results before updating plugin metadata, preventing potential issues where plugins might modify the list or items during view model updates.
1199-1208: LGTM! Proper handling of null results and deep cloning.The implementation correctly:
- Handles null results by returning an empty list.
- Deep clones results to prevent Flow Launcher from modifying properties like score.
1499-1505: LGTM! Proper score calculation with priority and selected count.The implementation correctly:
- Handles top most records by setting max score.
- Calculates priority score based on metadata priority.
- Adds selected count if enabled.
@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 ...
|
|
@jjw24 I have done my work, please check. |
jjw24
added
Dev branch only
|
🥷 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: To learn more about /:\ gitStream - Visit our Docs |
2 participants
Use null as default value for record key
Deep clone result list before updating results
This can fix issue in #3203. Tested.