Fix clearing of results logic & minor adjustment to results update #3524
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes flickering issues by altering the results clearing logic and updating how new results are incorporated so that only the relevant sets appear without an in-between clear.
- Updates results update logic in ResultsViewModel to conditionally bypass clearing
- Introduces a new boolean flag in ResultsForUpdate to control when to clear old results
- Adjusts MainViewModel to compute and pass the clear-results flag while renaming query state variables for clarity
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| Flow.Launcher/ViewModel/ResultsViewModel.cs | Refactored NewResults method to conditionally discard previous results based on a flag |
| Flow.Launcher/ViewModel/ResultsForUpdate.cs | Added a new parameter to indicate if existing results should be cleared |
| Flow.Launcher/ViewModel/MainViewModel.cs | Renamed internal query state variables and updated how the ResultsForUpdate instance is constructed including the clear-results flag |
Comments suppressed due to low confidence (2)
Flow.Launcher/ViewModel/MainViewModel.cs:1443
- The constructor for ResultsForUpdate now expects its first parameter to be PluginMetadata as defined in its record, but here the first argument appears to be a results collection (resultsCopy). Please verify and correct the ordering of arguments to match the record definition.
if (!_resultsUpdateChannelWriter.TryWrite(new ResultsForUpdate(resultsCopy, plugin.Metadata, query, token, reSelect, clearExistingResultsRequired)))
Flow.Launcher/ViewModel/MainViewModel.cs:1439
- [nitpick] There is an inconsistency between the local variable 'clearExistingResultsRequired' and the record property name 'requireClearExistingResults'. Consider unifying their naming to avoid confusion.
var clearExistingResultsRequired = RequireClearExistingResults(query, currentIsHomeQuery);
|
🥷 Code experts: Jack251970 Jack251970 has 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 |
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. |
📝 WalkthroughWalkthroughThe changes refactor how search result clearing is managed in response to user queries. The logic for clearing results is now deferred and made conditional, based on whether the query type or action keyword has changed. New methods and flags are introduced to control this behavior, aiming for more precise and context-aware updates to the result view. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MainViewModel
participant ResultsForUpdate
participant ResultsViewModel
User->>MainViewModel: Enter query
MainViewModel->>MainViewModel: Determine current and previous query states
MainViewModel->>MainViewModel: Call ShouldClearExistingResults()
MainViewModel->>ResultsForUpdate: Create ResultsForUpdate with shouldClearExistingResults flag
MainViewModel->>ResultsViewModel: Send ResultsForUpdate
ResultsViewModel->>ResultsViewModel: NewResults() checks shouldClearExistingResults
alt shouldClearExistingResults is true
ResultsViewModel->>ResultsViewModel: Replace old results with new results
else shouldClearExistingResults is false
ResultsViewModel->>ResultsViewModel: Merge old and new results
end
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ 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 (2)
Flow.Launcher/ViewModel/ResultsForUpdate.cs (1)
12-13: Inconsistent casing for the new constructor parameter
ReSelectFirstResultis Pascal-cased but the newly-addedrequireClearExistingResultsstarts with a lower-case letter.
Keeping the public surface consistent helps IntelliSense discoverability and avoids subtle naming mismatches elsewhere (e.g. LINQ selectors looking for a different property name).- bool requireClearExistingResults = false) + bool RequireClearExistingResults = false)If you adopt the Pascal-case version, remember to update every usage (
.requireClearExistingResults) accordingly.Flow.Launcher/ViewModel/ResultsViewModel.cs (1)
235-244: Enumeration happens twice – cache to avoid unnecessary work
newResultsis anIEnumerable<ResultViewModel>.
WhenrequireClearExistingResults == trueyou enumerate it once (OrderBy) and materialise a list.
If the branch isfalse, you re-enumerate the same sequence duringConcat.Although usually negligible, this double pass becomes noticeable with large result sets or frequent updates.
- var newResults = resultsForUpdates.SelectMany(u => u.Results, (u, r) => new ResultViewModel(r, _settings)); + var newResults = resultsForUpdates + .SelectMany(u => u.Results, (u, r) => new ResultViewModel(r, _settings)) + .ToList(); // Enumerate once ... - if (resultsForUpdates.Any(x => x.requireClearExistingResults)) - return newResults.OrderByDescending(rv => rv.Result.Score).ToList(); + if (resultsForUpdates.Any(x => x.RequireClearExistingResults)) + return newResults.OrderByDescending(rv => rv.Result.Score).ToList(); // already materialised(The snippet also shows the naming change suggested earlier.)
This keeps the algorithm O(n log n) instead of O(2 n log n) and prevents potential side-effects if
_settingsmutates between iterations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Flow.Launcher/ViewModel/MainViewModel.cs(8 hunks)Flow.Launcher/ViewModel/ResultsForUpdate.cs(1 hunks)Flow.Launcher/ViewModel/ResultsViewModel.cs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: gitStream workflow automation
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: build
| // Indicate if to clear existing results so to show only ones from plugins with action keywords | ||
| var clearExistingResultsRequired = RequireClearExistingResults(query, currentIsHomeQuery); | ||
| _lastQuery = query; | ||
| _previousIsHomeQuery = currentIsHomeQuery; | ||
|
|
There was a problem hiding this comment.
Potential race condition when updating _lastQuery / _previousIsHomeQuery
QueryTaskAsync runs once per plugin and executes in parallel.
Each invocation updates the shared fields _lastQuery and _previousIsHomeQuery:
_lastQuery = query;
_previousIsHomeQuery = currentIsHomeQuery;This means later-finishing plugin tasks from the previous query (or simply slower plugins) can overwrite these fields after a new query has already started, causing RequireClearExistingResults to make a decision based on stale data.
Suggestion
• Move the assignments into the outer `QueryResultsAsync` method *before* launching plugin tasks.
• Or guard them with a dedicated lock / atomic exchange and verify the cancellation token hasn’t been triggered.
Example (outside the tasks):
// before launching Tasks
var clearExistingResultsRequired = RequireClearExistingResults(query, currentIsHomeQuery);
_lastQuery = query;
_previousIsHomeQuery = currentIsHomeQuery;
tasks = plugins.Select(p => QueryTaskAsync(p, clearExistingResultsRequired, currentCancellationToken)).ToArray();Then pass clearExistingResultsRequired as a parameter into QueryTaskAsync.
This removes any cross-thread contention and ensures the flag is computed exactly once per user query.
There was a problem hiding this comment.
Not applicable in this instance because we always want to determine whether to clear results based on the current and previous query type (action keyword, global or home page) that will be push to the channel immediately.
This means later-finishing plugin tasks from the previous query (or simply slower plugins) can overwrite these fields after a new query has already started, causing RequireClearExistingResults to make a decision based on stale data.
That is correct expectation the upcoming query should determine whether results are to be cleared. RequireClearExistingResults should make decision based on the immediate previous and current query that will be pushed through to channel even if they come later due to slowness, i.e. if GitHub search query comes late and is from an action keyword, and the previous query is from global action keyword then results still need to be cleared.
Let's keep this comment open so can refer back to in the future if needed.
There was a problem hiding this comment.
@jjw24 I understand your explanation about the design intention. The behavior you describe makes sense - each plugin task needs to independently evaluate whether to clear results based on the immediate previous and current query state when the results are pushed to the channel, regardless of timing.
This is important for situations like your GitHub example, where a late-arriving query with a different action keyword type still needs to clear existing results appropriately.
I noticed the method is called ShouldClearExistingResults in the code but appears as RequireClearExistingResults in the diff. Is this just a naming difference from development, or is there something else I should be aware of?
(•‿•)
There was a problem hiding this comment.
I noticed the method is called ShouldClearExistingResults in the code but appears as RequireClearExistingResults in the diff. Is this just a naming difference from development, or is there something else I should be aware of?
You looking at outdated diff.
There was a problem hiding this comment.
🧩 Analysis chain
@jjw24 You're absolutely right. I was referencing an outdated diff. Thanks for pointing that out - I'll make sure to check the latest code in future reviews.
🏁 Script executed:
#!/bin/bash
# Check the PR diff for the lines containing "ClearExistingResults"
git diff -p origin/master HEAD -- Flow.Launcher/ViewModel/MainViewModel.cs | grep -A 5 -B 5 "ClearExistingResults"
Length of output: 150
@jjw24 You're right - I was looking at an outdated diff. Thank you for the correction. I can now see that the method is consistently named ShouldClearExistingResults in the code, and the variable name in the current code is shouldClearExistingResults. I appreciate you pointing this out, and I'll make sure to reference the most current code in future reviews.
🧬 Code Graph Analysis Results
Relevant snippet from Flow.Launcher/ViewModel/MainViewModel.cs (lines 710-758):
/// <summary>
/// Determines whether the existing search results should be cleared based on the current query and the previous query type.
/// This is needed because of the design that treats plugins with action keywords and global action keywords separately. Results are gathered
/// either from plugins with matching action keywords or global action keyword, but not both. So when the current results are from plugins
/// with matching action keywords and a new result set comes from a new query with the global action keyword, the existing results need to be cleared,
/// and vice versa. The same applies to home page query results.
///
/// There is no need to clear results from global action keyword if a new set of results comes along that is also from global action keywords.
/// This is because the removal of obsolete results is handled in ResultsViewModel.NewResults(ICollection<ResultsForUpdate>).
/// </summary>
/// <param name="query">The current query.</param>
/// <param name="currentIsHomeQuery">A flag indicating if the current query is a home query.</param>
/// <returns>True if the existing results should be cleared, false otherwise.</returns>
private bool ShouldClearExistingResults(Query query, bool currentIsHomeQuery)
{
// If previous or current results are from home query, we need to clear them
if (_previousIsHomeQuery || currentIsHomeQuery)
{
App.API.LogDebug(ClassName, $"Cleared old results");
return true;
}
// If the last and current query are not home query type, we need to check the action keyword
if (_lastQuery?.ActionKeyword != query?.ActionKeyword)
{
App.API.LogDebug(ClassName, $"Cleared old results");
return true;
}
return false;
}Relevant snippet from Flow.Launcher/ViewModel/MainViewModel.cs (lines 600-720):
This snippet shows the usage of the method ShouldClearExistingResults inside the QueryResultsAsync method:
private async Task QueryResultsAsync(bool searchDelay, bool isReQuery = false, bool reSelect = true)
{
_updateSource?.Cancel();
App.API.LogDebug(ClassName, $"Start query with text: <{QueryText}>");
var query = await ConstructQueryAsync(QueryText, Settings.CustomShortcuts, Settings.BuiltinShortcuts);
if (query == null) // shortcut expanded
{
App.API.LogDebug(ClassName, $"Clear query results");
// Hide and clear results again because running query may show and add some results
Results.Visibility = Visibility.Collapsed;
Results.Clear();
// Reset plugin icon
PluginIconPath = null;
PluginIconSource = null;
SearchIconVisibility = Visibility.Visible;
// Hide progress bar again because running query may set this to visible
ProgressBarVisibility = Visibility.Hidden;
return;
}
App.API.LogDebug(ClassName, $"Start query with ActionKeyword <{query.ActionKeyword}> and RawQuery <{query.RawQuery}>");
var currentIsHomeQuery = query.RawQuery == string.Empty;
_updateSource?.Dispose();
var currentUpdateSource = new CancellationTokenSource();
_updateSource = currentUpdateSource;
var currentCancellationToken = _updateSource.Token;
_updateToken = currentCancellationToken;
ProgressBarVisibility = Visibility.Hidden;
_isQueryRunning = true;
// Switch to ThreadPool thread
await TaskScheduler.Default;
if (currentCancellationToken.IsCancellationRequested) return;
// Update the query's IsReQuery property to true if this is a re-query
query.IsReQuery = isReQuery;
ICollection<PluginPair> plugins = Array.Empty<PluginPair>();
if (currentIsHomeQuery)
{
if (Settings.ShowHomePage)
{
plugins = PluginManager.ValidPluginsForHomeQuery();
}
PluginIconPath = null;
PluginIconSource = null;
SearchIconVisibility = Visibility.Visible;
}
else
{
plugins = PluginManager.ValidPluginsForQuery(query);
if (plugins.Count == 1)
{
PluginIconPath = plugins.Single().Metadata.IcoPath;
PluginIconSource = await App.API.LoadImageAsync(PluginIconPath);
SearchIconVisibility = Visibility.Hidden;
}
else
{
PluginIconPath = null;
PluginIconSource = null;
SearchIconVisibility = Visibility.Visible;
}
}
var validPluginNames = plugins.Select(x => $"<{x.Metadata.Name}>");
App.API.LogDebug(ClassName, $"Valid <{plugins.Count}> plugins: {string.Join(" ", validPluginNames)}");
_ = Task.Delay(200, currentCancellationToken).ContinueWith(_ =>
{
// start the progress bar if query takes more than 200 ms and this is the current running query and it didn't finish yet
if (_isQueryRunning)
{
ProgressBarVisibility = Visibility.Visible;
}
},
currentCancellationToken,
TaskContinuationOptions.NotOnCanceled,
TaskScheduler.Default);
Task[] tasks;
if (currentIsHomeQuery)
{
tasks = plugins.Select(plugin => plugin.Metadata.HomeDisabled switch
{
false => QueryTaskAsync(plugin, currentCancellationToken),
true => Task.CompletedTask
}).ToArray();
// Query history results for home page firstly so it will be put on top of the results
if (Settings.ShowHistoryResultsForHomePage)
{
QueryHistoryTask(currentCancellationToken);
}
}
else
{
tasks = plugins.Select(plugin => plugin.Metadata.Disabled switch
{
false => QueryTaskAsync(plugin, currentCancellationToken),
true => Task.CompletedTask
}).ToArray();
}
try
{
await Task.WhenAll(tasks);
}
catch (OperationCanceledException)
{
// nothing to do here
}
if (currentCancellationToken.IsCancellationRequested) return;
_isQueryRunning = false;
if (!currentCancellationToken.IsCancellationRequested)
{
ProgressBarVisibility = Visibility.Hidden;
}
// Local function
async Task QueryTaskAsync(PluginPair plugin, CancellationToken token)
{
App.API.LogDebug(ClassName, $"Wait for querying plugin <{plugin.Metadata.Name}>");
if (searchDelay && !currentIsHomeQuery) // Do not delay for home query
{
var searchDelayTime = plugin.Metadata.SearchDelayTime ?? Settings.SearchDelayTime;
await Task.Delay(searchDelayTime, token);
if (token.IsCancellationRequested) return;
}
await Task.Yield();
var results = currentIsHomeQuery ?
await PluginManager.QueryHomeForPluginAsync(plugin, query, token) :
await PluginManager.QueryForPluginAsync(plugin, query, token);
if (token.IsCancellationRequested) return;
IReadOnlyList<Result> resultsCopy;
if (results == null)
{
resultsCopy = _emptyResult;
}
else
{
resultsCopy = DeepCloneResults(results, token);
}
foreach (var result in resultsCopy)
{
if (string.IsNullOrEmpty(result.BadgeIcoPath))
{
result.BadgeIcoPath = plugin.Metadata.IcoPath;
}
}
if (token.IsCancellationRequested) return;
App.API.LogDebug(ClassName, $"Update results for plugin <{plugin.Metadata.Name}>");
var shouldClearExistingResults = ShouldClearExistingResults(query, currentIsHomeQuery);
_lastQuery = query;
_previousIsHomeQuery = currentIsHomeQuery;
if (!_resultsUpdateChannelWriter.TryWrite(new ResultsForUpdate(resultsCopy, plugin.Metadata, query,
token, reSelect, shouldClearExistingResults)))
{
App.API.LogError(ClassName, "Unable to add item to Result Update Queue");
}
}
void QueryHistoryTask(CancellationToken token)
{
var historyItems = _history.Items.TakeLast(Settings.MaxHistoryResultsToShowForHomePage).Reverse();
var results = GetHistoryItems(historyItems);
if (token.IsCancellationRequested) return;
App.API.LogDebug(ClassName, $"Update results for history");
if (!_resultsUpdateChannelWriter.TryWrite(new ResultsForUpdate(results, _historyMetadata, query,
token)))
{
App.API.LogError(ClassName, "Unable to add item to Result Update Queue");
}
}
}Summary of ShouldClearExistingResults method in MainViewModel.cs:
-
Purpose:
Determines if existing search results should be cleared when a new query is executed. This is necessary because results from plugins with action keywords and global plugins are handled separately, and mixing them requires clearing old results to avoid stale or irrelevant data. -
Parameters:
Query query: The current query object containing the action keyword and raw query.bool currentIsHomeQuery: Indicates if the current query is a home query (empty raw query).
-
Returns:
bool: Returnstrueif existing results should be cleared, otherwisefalse.
-
Logic:
- Clears results if either the previous or current query is a home query.
- Clears results if the action keyword of the last query differs from the current query's action keyword.
- Otherwise, does not clear results.
-
Usage:
Called during the query execution process (QueryResultsAsync) to decide whether to clear existing results before adding new ones.
Additional context:
- The method
ShouldClearExistingResultsis used internally inQueryResultsAsyncto manage the results update queue. - The naming difference mentioned in the user comment (
ShouldClearExistingResultsvsRequireClearExistingResults) appears to be a naming difference in the diff or code review context; the actual method in the current code is namedShouldClearExistingResults. - The method is private and tightly coupled with the query execution and results update logic in
MainViewModel.
No other code snippets from the provided list are directly relevant to the naming or behavior of ShouldClearExistingResults or RequireClearExistingResults in MainViewModel.cs.
@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 ...
|
Problem:
Current result clearing logic creates a seemingly animation of flickering when results switch from:
This is due to the use of result list clearing (MainViewModel.RemoveOldQueryResults) in between these result set changes.
Solution
Don't call
Results.Clear(). Where Flow does the results update inResultsViewModel.NewResultsreturn particular set or combined/concatenated results.Before
After
Tested
Close #3516