Use new model to clear results & Fix clear existing results when using IResultUpdate #3588
Conversation
This comment has been minimized.
This comment has been minimized.
|
🥷 Code experts: jjw24 Jack251970, jjw24 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:
Activity based on git-commit:
Knowledge based on git-blame: To learn more about /:\ gitStream - Visit our Docs |
|
Be a legend 🏆 by adding a before and after screenshot of the changes you made, especially if they are around UI/UX. |
There was a problem hiding this comment.
Pull Request Overview
This PR updates the results updating mechanism by removing the per-update flag for clearing existing results and instead using a centralized model flag from the main view model. Key changes include:
- In ResultsViewModel.cs, modifying the logic to check a centralized flag (ShouldClearExistingResults) instead of a per-update parameter.
- In ResultsForUpdate.cs, removing the redundant shouldClearExistingResults parameter from the record.
- In MainViewModel.cs, introducing the ShouldClearExistingResults property and updating query flows to set this flag as needed.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| Flow.Launcher/ViewModel/ResultsViewModel.cs | Updates to result handling logic and token checks with added clarifications. |
| Flow.Launcher/ViewModel/ResultsForUpdate.cs | Removal of the now-unneeded shouldClearExistingResults parameter. |
| Flow.Launcher/ViewModel/MainViewModel.cs | Addition of the ShouldClearExistingResults flag and updating its usage. |
📝 Walkthrough""" WalkthroughThe changes refactor how the Changes
Sequence Diagram(s)sequenceDiagram
participant MainVM as MainViewModel
participant UpdateLoop as UpdateActionAsync
participant ResultsVM as ResultsViewModel
MainVM->>UpdateLoop: Enqueue ResultsForUpdate (without clear flag)
UpdateLoop->>UpdateLoop: Compute ShouldClearExistingResults for each item
UpdateLoop->>ResultsVM: Add results with updated clear flag
ResultsVM->>ResultsVM: Clear existing results (ignoring cancellation)
ResultsVM->>ResultsVM: Add new results (check cancellation before adding)
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (1)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
🧹 Nitpick comments (3)
Flow.Launcher/ViewModel/ResultsViewModel.cs (3)
248-248: Fix typo in comment.- // If mainVM has flag to clear existing results, hanlde it here + // If mainVM has flag to clear existing results, handle it here
193-197: Document the implications of skipping cancellation token checks.The comment explains that token checks are skipped after
NewResults, but it doesn't explain why clearing results takes precedence over cancellation. This design decision should be documented more clearly to help future maintainers understand the trade-offs.Consider adding a more detailed comment:
- // Since NewResults may need to clear existing results, do not check token cancellation after this point + // Since NewResults may need to clear existing results based on MainViewModel flag, + // we must complete the clearing operation even if cancellation is requested. + // This ensures UI consistency by preventing partial result sets from being displayed.
350-372: Consider the impact of ignoring cancellation tokens during collection updates.Multiple comments indicate that cancellation tokens are not checked during
RemoveAll,Clear, and other operations. While this ensures results are properly cleared, it could lead to performance issues if large result sets continue processing after the user has cancelled the operation.Consider adding a mechanism to track whether clearing was initiated and allow cancellation after the critical clearing phase.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Flow.Launcher/ViewModel/MainViewModel.cs(3 hunks)Flow.Launcher/ViewModel/ResultsForUpdate.cs(1 hunks)Flow.Launcher/ViewModel/ResultsViewModel.cs(3 hunks)
🧰 Additional context used
🪛 GitHub Actions: Check Spelling
Flow.Launcher/ViewModel/MainViewModel.cs
[warning] 1857-1857: Unrecognized spelling warning for 'VSTHRD'.
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: gitStream.cm
- GitHub Check: gitStream workflow automation
- GitHub Check: gitStream.cm
- GitHub Check: build
🔇 Additional comments (2)
Flow.Launcher/ViewModel/ResultsForUpdate.cs (1)
7-12:✅ Verification successful
Verify all callers of ResultsForUpdate have been updated.
The removal of the
shouldClearExistingResultsparameter is a breaking change. Ensure all instantiations ofResultsForUpdatethroughout the codebase have been updated accordingly.
🏁 Script executed:
#!/bin/bash # Description: Find all instantiations of ResultsForUpdate to verify they've been updated # Search for all uses of ResultsForUpdate constructor ast-grep --pattern 'new ResultsForUpdate($$$)' # Also search for any direct references to ResultsForUpdate rg -A 3 'ResultsForUpdate\s*\(' --type csLength of output: 2443
All
ResultsForUpdatecall sites updatedVerified all three instantiations in
MainViewModel.cs(lines 268–269, 1447–1448, 1475–1476) now match the new constructor signature (4 args or 5 with the optionalReSelectFirstResult). No further changes needed.Flow.Launcher/ViewModel/MainViewModel.cs (1)
1082-1082:✅ Verification successful
Where is
ShouldClearExistingResultsconsumed and reset?The flag is being set but I cannot find where it's consumed or reset to
false. This could lead to incorrect behavior where results are cleared when they shouldn't be.Run the following script to find where this flag is used:
🏁 Script executed:
#!/bin/bash # Description: Search for usage of ShouldClearExistingResults property # Expected: Find where the flag is read and reset # Search for any reference to ShouldClearExistingResults rg -A 5 -B 5 "ShouldClearExistingResults" # Search in ResultsViewModel for potential consumption fd -e cs ResultsViewModel | xargs rg -A 10 "ShouldClearExistingResults|AddResults|NewResults"Length of output: 12823
ShouldClearExistingResults is consumed and reset in ResultsViewModel.NewResults
In Flow.Launcher/ViewModel/ResultsViewModel.cs, method
NewResults(ICollection<ResultsForUpdate>)(the overload at the bottom of the class) you’ll find:if (_mainVM != null && _mainVM.ShouldClearExistingResults) { _mainVM.ShouldClearExistingResults = false; App.API.LogDebug("NewResults", $"Existing results are cleared for query"); return newResults.OrderByDescending(rv => rv.Result.Score).ToList(); }This both reads and immediately resets the flag to
false.Writing to a
boolis atomic in .NET, and the channel-based handoff to the UI thread provides the necessary memory-fencing. There’s no additional thread-safety risk here.No changes required.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull Request Overview
A PR to refactor how existing results are cleared by replacing the local flag with a global flag and removing token cancellation checks to ensure all clear actions execute.
- Remove the local shouldClearExistingResults flag and update references accordingly.
- Introduce a global flag in MainViewModel with proper locking to manage clearing behavior.
- Remove token cancellation checks in the critical sections to guarantee that clear actions are executed.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| Flow.Launcher/ViewModel/ResultsViewModel.cs | Adjusted behavior in NewResults and removal methods to rely on the global flag and removed token cancellation checks. |
| Flow.Launcher/ViewModel/ResultsForUpdate.cs | Removed the shouldClearExistingResults parameter from the record struct. |
| Flow.Launcher/ViewModel/MainViewModel.cs | Added a global flag with locking and updated ResultForUpdate constructor calls accordingly. |
Comments suppressed due to low confidence (2)
Flow.Launcher/ViewModel/MainViewModel.cs:1452
- Verify that removing the shouldClearExistingResults parameter from the ResultsForUpdate record does not affect components still expecting that flag, as the clearing behavior is now managed globally.
_resultsUpdateChannelWriter.TryWrite(new ResultsForUpdate(resultsCopy, plugin.Metadata, query, token, reSelect)))
Flow.Launcher/ViewModel/ResultsViewModel.cs:249
- Ensure that the null check on _mainVM is intentional; if _mainVM is null, confirm that skipping the clear action is the desired behavior.
if (_mainVM != null && _mainVM.CheckShouldClearExistingResultsAndReset())
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull Request Overview
This PR updates the results clearing mechanism to use a global flag instead of a local one and adjusts where cancellation checks occur to ensure clear actions are fully executed. Key changes include:
- Removing the local shouldClearExistingResults flag and related parameter from ResultsForUpdate.
- Updating the AddResults and Update methods in ResultsViewModel to reposition token cancellation checks.
- Adding a global flag with locking in MainViewModel to control clearing of existing results.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| Flow.Launcher/ViewModel/ResultsViewModel.cs | Adjusts execution order and adds cancellation token parameters to results update |
| Flow.Launcher/ViewModel/ResultsForUpdate.cs | Removes the local shouldClearExistingResults parameter |
| Flow.Launcher/ViewModel/MainViewModel.cs | Introduces a global flag and locking mechanism for clearing results |
Comments suppressed due to low confidence (1)
Flow.Launcher/ViewModel/ResultsViewModel.cs:318
- Consider renaming the parameter 'Items' to 'items' to follow standard naming conventions for parameters.
private void AddAll(List<ResultViewModel> Items, CancellationToken token = default)
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@jjw24 I targeted the problem. We need to pass shouldClearExistingResults flag when overriding results. More information in Pass shouldClearExistingResults flag when overriding results. And please use the new package for test. |
This comment has been minimized.
This comment has been minimized.
|
Yeah ok. Seems good so far, I will test drive it one more day to make sure. |
|
This iteration of the change has the code really clear now, I am happy with this change. Thank you for taking the time to do this. |
|
Yeah, good |
@check-spelling-bot Report🔴 Please reviewSee the 📂 files view, the 📜action log, or 📝 job summary for details.
See ❌ Event descriptions for more information. Forbidden patterns 🙅 (1)In order to address this, you could change the content to not match the forbidden patterns (comments before forbidden patterns may help explain why they're forbidden), add patterns for acceptable instances, or adjust the forbidden patterns themselves. These forbidden patterns matched content: s.b. workaround(s)If the flagged items are 🤯 false positivesIf items relate to a ...
|
There was a problem hiding this comment.
Pull Request Overview
This PR improves query result handling by introducing a new property for home query detection and refining the cancellation handling logic during updates.
- Introduces a new IsHomeQuery property in the Query class and updates related logic.
- Refines cancellation checks and logging in result update methods to ensure proper clearing of results.
- Adjusts the ResultsForUpdate record and updates the MainViewModel logic to correctly handle the clear existing results flag.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| Flow.Launcher/ViewModel/ResultsViewModel.cs | Updates cancellation check style and logging for result updates. |
| Flow.Launcher/ViewModel/ResultsForUpdate.cs | Renames the clear existing results flag to use PascalCase. |
| Flow.Launcher/ViewModel/MainViewModel.cs | Refactors home query checking and updates the clear results flag. |
| Flow.Launcher/MainWindow.xaml.cs | Adjusts async call to RefreshFrameAsync to await completion. |
| Flow.Launcher.Plugin/Query.cs | Adds new IsHomeQuery property with internal init support. |
| Flow.Launcher.Core/Plugin/QueryBuilder.cs | Sets IsHomeQuery appropriately for default vs. normal queries. |
| // If the queue already has the item, we need to pass the shouldClearExistingResults flag | ||
| if (queue.TryGetValue(item.ID, out var existingItem)) | ||
| { | ||
| item.ShouldClearExistingResults = shouldClearExistingResults || existingItem.ShouldClearExistingResults; |
There was a problem hiding this comment.
ResultsForUpdate is defined as a record struct with an init-only property for ShouldClearExistingResults. Reassigning this property later in MainViewModel may cause compilation issues. Consider changing the property to allow mutation (e.g., using a mutable property with get; set;) if it needs to be updated after initialization.
Copilot uses AI. Check for mistakes.
|
@coderabbitai review |
jjw24
removed
bug
Follow on with #3524.
CHANGE
IsHomeQueryto indicate home query stateShouldClearExistingResultsForQueryinUpdateActionAsyncIResultUpdateevent to fix the issue that one query consumes much time, the results will be cleared once the results updated (as below)var newResults = NewResults(resultsForUpdates);&if (Count != 0) RemoveAll(newItems.Count);orClear();AddAll(newItems, token);&BulkAddAll(newItems, token);ResultCollectionfor code quality.queue.Issue Example
The first item is from new query while others are from old one. If we do not add clear existing results when using IResultUpdate and this query happens to consume long time, the list will display old items for a long time until the query is completed and updated.
TEST