Do not cancel tasks which requires clearing existing results #3577
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR updates task handling to ensure that tasks flagged for clearing existing results are not cancelled even if their cancellation tokens have been triggered.
- Adjusts the queue insertion logic in UpdateActionAsync to include tasks that clear existing results.
- Modifies the token selection logic in UpdateResultView to use a default token when any update task is for clearing results.
Comments suppressed due to low confidence (1)
Flow.Launcher/ViewModel/MainViewModel.cs:1878
- Using Distinct().SingleOrDefault() may throw an exception if more than one distinct cancellation token exists. If multiple tokens are expected, consider implementing logic to handle them appropriately.
token = resultsForUpdates.Select(r => r.Token).Distinct().SingleOrDefault();
|
🥷 Code experts: onesounds Jack251970, onesounds have most 👩💻 activity in the files. See details
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. |
This comment has been minimized.
This comment has been minimized.
📝 Walkthrough""" WalkthroughThe changes update the logic in the asynchronous result update queue within Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MainViewModel
participant UpdateQueue
User->>MainViewModel: Initiates result update (may include clear flag)
MainViewModel->>UpdateQueue: Enqueue update (check cancellation token and clear flag)
UpdateQueue-->>MainViewModel: Processes update
alt shouldClearExistingResults is true
MainViewModel->>UpdateResultView: Use default (non-canceled) token
else
MainViewModel->>UpdateResultView: Use derived cancellation token
end
UpdateResultView-->>MainViewModel: Results updated/cleared
Assessment against linked issues
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: 0
🧹 Nitpick comments (1)
Flow.Launcher/ViewModel/MainViewModel.cs (1)
215-216: Minor grammar improvement in comment.The logic correctly ensures clearing tasks are always queued regardless of cancellation status. However, the comment has a minor grammatical issue.
- // If this update task is for clearing existing results, we must need to add it to the queue + // If this update task is for clearing existing results, we must add it to the queue
📜 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] 1846-1846: Spell check warning: VSTHRD is not a recognized word. (unrecognized-spelling)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: gitStream workflow automation
- GitHub Check: gitStream workflow automation
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: build
🔇 Additional comments (1)
Flow.Launcher/ViewModel/MainViewModel.cs (1)
870-879: Excellent implementation for preventing cancellation of clearing tasks.The logic correctly handles the case where clearing tasks should not be cancelled by using a default token. This ensures that UI updates for clearing existing results are always processed, which aligns perfectly with the PR objective.
The implementation is thread-safe and handles the edge case appropriately by falling back to the default token in the catch block.
There was a problem hiding this comment.
Pull Request Overview
This pull request adjusts task cancellation behavior to ensure that tasks meant for clearing existing results are not cancelled. Key changes include adding an extra condition to bypass cancellation tokens, modifying the token passed to result update tasks, and setting a default token in the result view update when clearing results.
Comments suppressed due to low confidence (1)
Flow.Launcher/ViewModel/MainViewModel.cs:1880
- Using SingleOrDefault may throw an exception if there are multiple distinct tokens; consider using FirstOrDefault or ensuring that all tokens are identical.
token = resultsForUpdates.Select(r => r.Token).Distinct().SingleOrDefault();
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@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 ...
|
| @@ -1460,8 +1462,9 @@ void QueryHistoryTask(CancellationToken token) | |||
| _lastQuery = query; | |||
| _previousIsHomeQuery = currentIsHomeQuery; | |||
|
|
|||
| // If this update task is for clearing existing results, do not pass token to make sure it is handled | |||
There was a problem hiding this comment.
What scenario could have this occur?
There was a problem hiding this comment.
If one task which needs to clear existing results, we should make sure it is handled in Results. So we need to ignore its origin token here.
There was a problem hiding this comment.
In the original code if a task is cancelled, it wouldn't get to this part of the code right
There was a problem hiding this comment.
Please see the update flow above in desc.
There was a problem hiding this comment.
This is still in the process to update the results though.
There was a problem hiding this comment.
Hm show me what exactly do you mean.
There was a problem hiding this comment.
Actually in this PR we are just trying to handle the case where token is cancelled but require clearing results still, so we can just capture this where token is cancelled and require clearing are true then return empty result set in that NewResults method.
There was a problem hiding this comment.
@jjw24 Please go ahead with this. I find I am unable to do that
There was a problem hiding this comment.
Actually in this PR we are just trying to handle the case where token is cancelled but require clearing results still, so we can just capture this where token is cancelled and require clearing are true then return empty result set in that NewResults method.
Feel free to change anything you would like
There was a problem hiding this comment.
I am little unfamiliar with this clear logic and the only thing I can change is about handling with token cancellation.
jjw24
added
the
review in progress
jjw24
changed the title
jjw24
removed
review in progress
|
Continue in #3588. |
Follows on from #3553
And I think the update flow is:
Flow.Launcher/Flow.Launcher/ViewModel/MainViewModel.cs
Line 1440 in 2026bb7
Flow.Launcher/Flow.Launcher/ViewModel/MainViewModel.cs
Line 219 in 2026bb7
Flow.Launcher/Flow.Launcher/ViewModel/MainViewModel.cs
Line 1926 in 2026bb7
So I removed all token check after we write in _resultsUpdateChannelWriter if the update task will require result clearing.
Resolve #3576