Reproduce result clear issue #3580
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. |
📝 WalkthroughWalkthroughA private integer field, Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ 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: 1
🧹 Nitpick comments (1)
Flow.Launcher/ViewModel/MainViewModel.cs (1)
197-197: Consider renaming and documenting the field purpose.The field name
cancelIndexis misleading since it's not an index but rather a counter. Consider renaming toclearRequestCountor similar to better reflect its purpose. Also add documentation explaining its role in preventing redundant clear operations.- private int cancelIndex = 0; + /// <summary> + /// Counter to track clear result requests and prevent redundant clear operations. + /// </summary> + private int clearRequestCount = 0;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Flow.Launcher/ViewModel/MainViewModel.cs(2 hunks)
🧰 Additional context used
🪛 GitHub Actions: Check Spelling
Flow.Launcher/ViewModel/MainViewModel.cs
[warning] 1858-1858: Spell check warning: VSTHRD is not a recognized word. (unrecognized-spelling)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: gitStream workflow automation
- GitHub Check: gitStream workflow automation
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: build
@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: no user but you matched threshold 10 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: To learn more about /:\ gitStream - Visit our Docs |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Flow.Launcher/ViewModel/ResultsViewModel.cs (1)
192-202: Refactor the conditional logic for better clarity.The logic correctly implements the intended behavior to clear results even when cancelled, but the nested structure could be simplified for better readability.
Consider this cleaner approach:
- if (token.IsCancellationRequested) - { - if (shouldClearExistingResults) - { - newResults = new List<ResultViewModel>(); - } - else - { - return; - } - } + if (token.IsCancellationRequested && !shouldClearExistingResults) + { + return; + } + + if (token.IsCancellationRequested && shouldClearExistingResults) + { + newResults = new List<ResultViewModel>(); + }This makes the two distinct behaviors more explicit and easier to understand.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Flow.Launcher/ViewModel/MainViewModel.cs(5 hunks)Flow.Launcher/ViewModel/ResultsViewModel.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Flow.Launcher/ViewModel/MainViewModel.cs
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: gitStream workflow automation
- GitHub Check: gitStream.cm
- GitHub Check: build
🔇 Additional comments (2)
Flow.Launcher/ViewModel/ResultsViewModel.cs (2)
188-188: LGTM! Well-designed parameter addition.The optional
shouldClearExistingResultsparameter with a default value offalsemaintains backward compatibility while clearly indicating its purpose. The naming convention is appropriate.
188-202:✅ Verification successful
Verify the fix addresses the original issue completely.
This implementation looks correct for addressing issue #3567 where results weren't clearing when input was cleared. The approach of passing an empty list when
shouldClearExistingResultsis true should trigger the clearing behavior through the existingUpdateResultspipeline.To ensure this fix works as intended, please verify:
🏁 Script executed:
#!/bin/bash # Description: Check how shouldClearExistingResults is used in MainViewModel # Expected: Find usage in MainViewModel.cs that sets this parameter to true when clearing input echo "Searching for shouldClearExistingResults usage in MainViewModel..." rg -A 5 -B 5 "shouldClearExistingResults.*true" echo "Searching for AddResults calls in MainViewModel..." rg -A 3 -B 2 "AddResults.*shouldClearExistingResults"Length of output: 1530
🏁 Script executed:
#!/bin/bash echo "Fetching context around AddResults call in MainViewModel:" rg -A5 -B5 "Results\.AddResults" Flow.Launcher/ViewModel/MainViewModel.cs echo echo "Looking for declaration or assignment of shouldClearExistingResults in the surrounding scope:" rg -A3 -B3 "shouldClearExistingResults" Flow.Launcher/ViewModel/MainViewModel.csLength of output: 3501
🏁 Script executed:
#!/bin/bash echo "Searching for ShouldClearExistingResultsForQuery implementation..." rg -n "ShouldClearExistingResultsForQuery" -A20 -B5 . echo echo "Showing just the signature and return logic:" rg -n "ShouldClearExistingResultsForQuery" -A5 -B0 .Length of output: 9756
✅ Fix Verified: Clearing Behavior Works as Intended
I’ve confirmed that:
- ShouldClearExistingResultsForQuery correctly returns
truewhen the input is cleared (or when switching home/keyword queries).- MainViewModel passes that flag into
Results.AddResults(..., shouldClearExistingResults: true)for both plugin and history result updates.- ResultsViewModel.AddResults replaces
newResultswith an empty list whenshouldClearExistingResultsistrue(and the token is canceled), effectively clearing the UI.No unintended side effects were found—this fully addresses issue #3567 without breaking existing behavior.
Reproduce #3567.
Steps