Use new model to clear results #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. |
📝 WalkthroughWalkthroughThe changes introduce a thread-safe internal flag Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MainVM
participant ResultsVM
User->>MainVM: Initiate Query
MainVM->>MainVM: Determine if results should clear
alt ShouldClearExistingResults = true
MainVM->>ResultsVM: Signal to clear results
end
MainVM->>ResultsVM: Update with new results
ResultsVM->>ResultsVM: Add/replace results (ignoring cancellation during clear)
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 (3)
✨ 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)
@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 ...
|
Follow on with #3524.
CHANGE
var newResults = NewResults(resultsForUpdates);&if (Count != 0) RemoveAll(newItems.Count);orClear();so that all clear actions will be executed.AddAll(newItems, token);&BulkAddAll(newItems, token);so that we will not add new items from the cancelled queries. It can make sure we only need to clear existing results for one time.ResultCollectionfor code quality.TEST