-
-
Notifications
You must be signed in to change notification settings - Fork 381
Fix result missing + not updating issues #3513
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Conversation
…d of _updateSource.Token
This comment has been minimized.
This comment has been minimized.
🥷 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. |
1 similar comment
Be a legend 🏆 by adding a before and after screenshot of the changes you made, especially if they are around UI/UX. |
📝 Walkthrough""" WalkthroughThe changes refactor cancellation token handling in the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MainViewModel
participant Plugins
participant UI
User->>MainViewModel: Types query
MainViewModel->>MainViewModel: Dispose old _updateSource, create new _updateSource and _updateToken
MainViewModel->>Plugins: Query plugins with _updateToken
Plugins-->>MainViewModel: Return results or cancel if _updateToken is cancelled
MainViewModel->>UI: Update results display
Assessment against linked issues
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (2)
Flow.Launcher/ViewModel/MainViewModel.cs (2)
48-50
: Consider the lifetime of the new_updateToken
fieldIntroducing a cached
CancellationToken
simplifies null-checking and avoidsObjectDisposedException
, nice work!
However, because_updateToken
is re-assigned on every new query, any long-running background handler that reads this field (instead of the copy it was originally handed) will suddenly be looking at a token unrelated to its own operation. A typical example is theResultsUpdated
event where some plugins may forget to supplye.Token
.If you keep the field, please be sure every async workflow captures the token immediately (e.g.
var token = _updateToken;
) and never reads the field again. Another safe alternative is to remove the field entirely and always capture_updateSource.Token
into a local variable each time you spawn work.
255-257
: UseCanBeCanceled
instead ofdefault
to detect an “empty” token
default(CancellationToken)
andCancellationToken.None
are equivalent, but comparing a struct todefault
is a bit obscure and may cause confusion.
A clearer (and slightly safer) pattern is:-var token = e.Token == default ? _updateToken : e.Token; +var token = !e.Token.CanBeCanceled ? _updateToken : e.Token;This also works for the (rare) case where a plugin intentionally returns
CancellationToken.None
to indicate “never cancelled”.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Flow.Launcher/ViewModel/MainViewModel.cs
(10 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: build
🔇 Additional comments (2)
Flow.Launcher/ViewModel/MainViewModel.cs (2)
1334-1343
: Good cancellation propagation for the progress-bar delayPassing
_updateToken
intoTask.Delay
and its continuation ensures the progress bar never appears for a query that has already been cancelled. This is an elegant, cheap way to keep the UI responsive.
1455-1461
: Consistent use of the shared token in history updates
QueryHistoryTask
now respects_updateToken
, guaranteeing that history results are not shown for obsolete queries. This closes the race that led to issue #3511 – great catch!
This reverts commit 265fd9c.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Didn't seem to resolve the missing results issue. All Program results are gone even when using the action keyword 'progr'. But Chrome stuck around for whatever reason it keeps popping up 🤣 |
@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 ...
|
Damn. That is really weird. I reverted all codes to master branch. And could you please check if this can work? |
Luckily, now we can confirm that this issue comes from |
Have you been able to reproduce the issue? |
No. I have not encountered this issue before. After heavy usage (type |
I have set up python script for testing that. And after about 360/600/1000 times, I still cannot reproduce that issue. |
Typing edge and deleting it repeatedly won't reproduce it i find. I can repro with typing giberrish several times, as you have seen in the logs. Maybe try typing random giberrish, then type |
I have tried this python script: import pyautogui
import random
# Input Alt-Whitespace to open Flow.Launcher
pyautogui.hotkey('alt', 'space')
# Get random character
def get_random_char():
return random.choice('abcdefghijklmnopqrstuvwxyz')
times = 0
while True:
# Get random value 3 to 10
random_value = random.randint(3, 10)
for i in range(random_value):
# Get random character
char = get_random_char()
# Type the character
pyautogui.typewrite(char)
# Sleep for a random time between 0.1 and 0.2 seconds
pyautogui.sleep(random.uniform(0.1, 0.2))
for i in range(random_value):
# Type the character
pyautogui.typewrite(['backspace'])
# Sleep for a random time between 0.0 and 0.15 seconds
pyautogui.sleep(random.uniform(0.0, 0.15))
times += 1
print(f"Times: {times}")
if times % 15 == 0:
str1 = 'chrome'
str2 = 'jsjjdiejn+727+2'
# Type chrome jsjjdiejn+727+2
pyautogui.typewrite(str1)
pyautogui.sleep(random.uniform(0.1, 0.2))
pyautogui.typewrite(str2)
# Sleep for a random time between 0.1 and 0.2 seconds
pyautogui.sleep(random.uniform(0.1, 0.2))
# Type backspace
for h in range(len(str2)):
pyautogui.typewrite(['backspace'])
pyautogui.sleep(random.uniform(0.1, 0.2))
for h in range(len(str1)):
pyautogui.typewrite(['backspace'])
pyautogui.sleep(random.uniform(0.1, 0.2)) But I still cannot reproduce that after 381 times. |
Or could you please share me with a script that you can reproduce the issue on your device? |
Tested both issues resolved. Do you think any additional logging should be added or this is good to merge? |
Good! I think it is good to merge. |
Fix result missing issue
Fix #3508, #3511 from #3314