Progress Query Management and Thread Safety #4135
Conversation
Refactored `MainWindow.xaml.cs` and `MainViewModel.cs` to prevent redundant query calls when updating the query text programmatically. - Added `_ignoreTextChange` flag in `MainWindow.xaml.cs` to skip `TextChanged` event handling during programmatic updates. - Introduced `SetQueryTextBoxText` method to safely update the query text box without triggering events. - Updated `QueryTextBox_TextChanged1` to respect `_ignoreTextChange`. - Modified `MainViewModel.cs` to directly update `_queryText` and use `SetQueryTextBoxText` for text box synchronization. - Improved logic for clearing query text programmatically. - Added comments to clarify the intent behind these changes.
|
🥷 Code experts: onesounds Jack251970, onesounds 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: ✨ Comment |
|
Be a legend 🏆 by adding a before and after screenshot of the changes you made, especially if they are around UI/UX. |
Jack251970
added
Dev branch only
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughReplaced a single Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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. Comment |
![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/MainWindow.xaml.cs (1)
79-80: Update the section comment to match the flag's purpose.The comment "Search Delay" doesn't accurately describe the
_ignoreTextChangeflag's purpose, which is to suppress TextChanged event processing during programmatic updates. Consider renaming the comment to something like "Text Change Suppression" or "Programmatic Text Update" for clarity.- // Search Delay + // Text Change Suppression private bool _ignoreTextChange = false;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Flow.Launcher/MainWindow.xaml.cs(2 hunks)Flow.Launcher/ViewModel/MainViewModel.cs(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Yusyuriv
Repo: Flow-Launcher/Flow.Launcher PR: 3118
File: Flow.Launcher/ViewModel/MainViewModel.cs:1404-1413
Timestamp: 2024-12-08T21:12:12.060Z
Learning: In the `MainViewModel` class, the `_lastQuery` field is initialized in the constructor and is never null.
📚 Learning: 2024-12-08T21:12:12.060Z
Learnt from: Yusyuriv
Repo: Flow-Launcher/Flow.Launcher PR: 3118
File: Flow.Launcher/ViewModel/MainViewModel.cs:1404-1413
Timestamp: 2024-12-08T21:12:12.060Z
Learning: In the `MainViewModel` class, the `_lastQuery` field is initialized in the constructor and is never null.
Applied to files:
Flow.Launcher/MainWindow.xaml.csFlow.Launcher/ViewModel/MainViewModel.cs
🧬 Code graph analysis (2)
Flow.Launcher/MainWindow.xaml.cs (2)
Flow.Launcher/ViewModel/MainViewModel.cs (1)
Query(1220-1248)Flow.Launcher.Plugin/Query.cs (1)
Query(9-126)
Flow.Launcher/ViewModel/MainViewModel.cs (1)
Flow.Launcher/MainWindow.xaml.cs (3)
MainWindow(37-1510)MainWindow(89-105)SetQueryTextBoxText(1433-1438)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: CodeQL analysis (csharp)
- GitHub Check: Agent
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: build
🔇 Additional comments (4)
Flow.Launcher/MainWindow.xaml.cs (1)
1425-1431: LGTM!The flag check correctly prevents duplicate query execution when text is updated programmatically. The early return is clean and effective.
Flow.Launcher/ViewModel/MainViewModel.cs (3)
759-784: LGTM!The refactoring correctly prevents duplicate query execution by bypassing the QueryText property setter and directly updating the UI through
SetQueryTextBoxText. The comments clearly explain the intent, and the explicitQuerycall on line 771 ensures the query is still executed as expected.
789-823: LGTM!The async implementation correctly mirrors the synchronous
ChangeQueryTextapproach, properly handling dispatcher invocation while preventing duplicate queries through direct field assignment and UI synchronization.
832-914: LGTM!The refactoring at lines 887-893 correctly clears the query text when transitioning to context menu or history views without triggering duplicate queries. The explicit
Querycall on line 901 ensures the query flow continues as intended. The approach is consistent with the changes inChangeQueryTextandChangeQueryTextAsync.
There was a problem hiding this comment.
Pull request overview
This PR refactors the query text update mechanism to prevent duplicate query execution when text is updated programmatically. Instead of using the QueryText property setter which triggers PropertyChanged notifications and subsequent TextChanged events, the code now directly updates the _queryText field and the TextBox UI element using a new SetQueryTextBoxText method protected by an _ignoreTextChange flag.
Key Changes:
- Introduced
_ignoreTextChangeflag andSetQueryTextBoxTextmethod to bypass TextChanged event handling during programmatic updates - Modified three locations in
MainViewModel.cs(ChangeQueryText,ChangeQueryTextAsync, andSelectedResultssetter) to use direct field updates instead of property setters - Updated
QueryTextBox_TextChanged1to respect the_ignoreTextChangeflag
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| Flow.Launcher/MainWindow.xaml.cs | Added _ignoreTextChange flag, SetQueryTextBoxText helper method, and early return in QueryTextBox_TextChanged1 to prevent event handling during programmatic updates |
| Flow.Launcher/ViewModel/MainViewModel.cs | Replaced QueryText property setter calls with direct _queryText field updates and SetQueryTextBoxText calls in three methods to avoid triggering duplicate queries through the binding/event chain |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Exception: Repro: |
Simplified the process of clearing the `QueryText` property by replacing manual updates to the `_queryText` private field and `QueryTextBox` control with the use of the `QueryText` property setter. This change avoids triggering duplicate query calls while maintaining the intended behavior. Updated comments to reflect the new approach.
Fixed in the latest commits |
Replaced the single `_progressQuery` field with a thread-safe `ConcurrentDictionary<Guid, Query>` to enable tracking multiple queries concurrently. Each query is now associated with a unique `Guid` (`updateGuid`) for better isolation and management. Updated progress bar logic to use `_progressQueryDict`, ensuring that progress visibility is tied to the correct query session. Entries in `_progressQueryDict` are properly cleaned up when queries complete or are canceled. These changes improve thread safety, prevent race conditions, and enhance support for concurrent query handling.
Jack251970
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
Flow.Launcher/ViewModel/MainViewModel.cs (2)
886-890: QueryText clearing when leaving results remains compatible with new text-change handlingStoring
_queryTextBeforeLeaveResultsthen using theQueryTextproperty setter (QueryText = string.Empty;) while switchingSelectedResultsaway fromResultskeeps the previous behavior: the subsequentQuery(false)path only drives context menu / history filtering, not a duplicate main results query, so it doesn’t conflict with the new_ignoreTextChange/SetQueryTextBoxTextmechanism. If you ever want to avoid a potential extra context/history re-evaluation here, you could mirror the_ignoreTextChangeapproach, but it’s not necessary for correctness.
41-42: ConcurrentDictionary-based progress tracking looks solid; one redundant cleanup callUsing
_progressQueryDictkeyed by a per-queryupdateGuidis a good way to keep progress-bar visibility correct under overlapping queries while remaining thread-safe. The delayed visibility check:
- uses the same
updateGuidand comparesOriginalQuerystrings, so it won’t light the bar for superseded queries, and- is correctly guarded by the per-call cancellation token.
One tiny cleanup: you call
_progressQueryDict.Remove(updateGuid, out _);both afterTask.WhenAlland again in thefinallyblock. Because thefinallyalways executes for all exit paths (success, cancellation, or earlyreturn), the firstRemoveis redundant.You can simplify to:
- // this should happen once after all queries are done so progress bar should continue - // until the end of all querying - _progressQueryDict.Remove(updateGuid, out _); - if (!currentCancellationToken.IsCancellationRequested) { // update to hidden if this is still the current query ProgressBarVisibility = Visibility.Hidden; } @@ - // this ensures the query is removed from the progress tracking dictionary when this query is canceled or completes - _progressQueryDict.Remove(updateGuid, out _); + // this ensures the query is removed from the progress tracking dictionary when this query is canceled or completes + _progressQueryDict.Remove(updateGuid, out _);Behavior stays the same, with slightly clearer lifecycle.
Also applies to: 1424-1425, 1438-1439, 1493-1494, 1550-1562
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Flow.Launcher/ViewModel/MainViewModel.cs(10 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3593
File: Plugins/Flow.Launcher.Plugin.WebSearch/SettingsControl.xaml.cs:143-165
Timestamp: 2025-09-13T06:04:26.977Z
Learning: In Plugins/Flow.Launcher.Plugin.WebSearch/SettingsControl.xaml, the ListView_SizeChanged handler in the C# file assumes 5 GridViewColumns but the XAML may have more columns. The user Jack251970 indicated this mismatch doesn't need to be fixed when pointed out during PR #3593 review.
Learnt from: Yusyuriv
Repo: Flow-Launcher/Flow.Launcher PR: 3118
File: Flow.Launcher/ViewModel/MainViewModel.cs:1404-1413
Timestamp: 2024-12-08T21:12:12.060Z
Learning: In the `MainViewModel` class, the `_lastQuery` field is initialized in the constructor and is never null.
📚 Learning: 2024-12-08T21:12:12.060Z
Learnt from: Yusyuriv
Repo: Flow-Launcher/Flow.Launcher PR: 3118
File: Flow.Launcher/ViewModel/MainViewModel.cs:1404-1413
Timestamp: 2024-12-08T21:12:12.060Z
Learning: In the `MainViewModel` class, the `_lastQuery` field is initialized in the constructor and is never null.
Applied to files:
Flow.Launcher/ViewModel/MainViewModel.cs
📚 Learning: 2025-06-08T14:12:21.348Z
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3672
File: Flow.Launcher/MainWindow.xaml.cs:244-247
Timestamp: 2025-06-08T14:12:21.348Z
Learning: In Flow.Launcher, App.NotifyIcon is created before MainWindow creation, so null checks for App.NotifyIcon are not necessary when accessing it from MainWindow code.
Applied to files:
Flow.Launcher/ViewModel/MainViewModel.cs
🧬 Code graph analysis (1)
Flow.Launcher/ViewModel/MainViewModel.cs (1)
Flow.Launcher/MainWindow.xaml.cs (3)
MainWindow(37-1510)MainWindow(89-105)SetQueryTextBoxText(1433-1438)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: build
🔇 Additional comments (1)
Flow.Launcher/ViewModel/MainViewModel.cs (1)
751-785: Programmatic ChangeQueryText now cleanly avoids duplicate queriesThe pattern of setting the backing field
_queryText, then usingMainWindow.SetQueryTextBoxTextunder the dispatcher (with_ignoreTextChangeon the window side) ensures programmatic updates (history, autocomplete, dialog-jump reset, etc.) no longer re-enterQueryTextBox_TextChanged1, whileQuery(false, isReQuery: false)/QueryAsync(false, isReQuery: false)still executes exactly once per call. Sync and async variants are consistent and thread-safe with the existing dispatcher checks.Also applies to: 790-824
jjw24
added
the
review in progress
Still reproducible by assigning a custom query hotkey to a web search result then go to the context menu on it. Exception: |
Can you show me a demo video? |
Added a null-conditional operator (`?.`) to the `UpdateSource()` method call on the `BindingExpression` object `be` to prevent a possible `NullReferenceException` if `be` is null. This change improves code robustness and ensures safer execution.
|
Not sure if 556fc61 can help |
jjw24
left a comment
jjw24
left a comment
There was a problem hiding this comment.
The context menu issue still persists. It only happens after assigning a custom query hotkey. After using custom query hotkey, the query on context menu does not clear any more:
|
@jjw24 I cannot reproduce it on my side. Please give me full steps for reproduction. |
|
And have you tested the dev branch? Does it have this problem? |
|
Dev branch doesnt have this problem. Repro:
Note: I did this via a local build not the build artefact from AppVeyor. |
Removed _ignoreTextChange flag and SetQueryTextBoxText method, allowing programmatic query text updates to trigger normal event flow and query logic. Simplified MainViewModel to set QueryText directly and invoke queries, ensuring consistent and streamlined query handling. Also removed unnecessary null check in QueryTextBox_KeyUp.
I reverted changes with Query Text Change Handling:
And it works well for me now. Please review |
|
This change is already enough to resolve possible progress bar issue when typing so fast or use custom query. |
Jack251970
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Flow.Launcher/ViewModel/MainViewModel.cs (1)
1474-1485: Verify thread-safety of ProgressBarVisibility setter.The continuation on line 1474 runs on
TaskScheduler.Default(thread pool), and line 1480 setsProgressBarVisibilityfrom a non-UI thread. The property is bound in XAML withUpdateSourceTrigger=PropertyChanged, andBaseModelprovides no automatic thread marshaling. Setting a bound WPF property from a non-UI thread can cause cross-thread access issues or binding infrastructure failures. Wrap the setter in aDispatcher.Invokecall or marshal back to the UI thread before setting the property.
🧹 Nitpick comments (1)
Flow.Launcher/ViewModel/MainViewModel.cs (1)
408-422: Consider logging if TryAdd fails.While GUID collisions are virtually impossible, the
TryAddoperation on Line 422 could theoretically fail. Consider logging a warning if this occurs to aid debugging in case of unexpected behavior.🔎 Proposed enhancement
- _progressQueryDict.TryAdd(updateGuid, query); + if (!_progressQueryDict.TryAdd(updateGuid, query)) + { + App.API.LogWarn(ClassName, $"Failed to add query to progress dictionary with GUID {updateGuid}"); + }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Flow.Launcher/ViewModel/MainViewModel.cs
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3593
File: Plugins/Flow.Launcher.Plugin.WebSearch/SettingsControl.xaml.cs:143-165
Timestamp: 2025-09-13T06:04:26.977Z
Learning: In Plugins/Flow.Launcher.Plugin.WebSearch/SettingsControl.xaml, the ListView_SizeChanged handler in the C# file assumes 5 GridViewColumns but the XAML may have more columns. The user Jack251970 indicated this mismatch doesn't need to be fixed when pointed out during PR #3593 review.
📚 Learning: 2024-12-08T21:12:12.060Z
Learnt from: Yusyuriv
Repo: Flow-Launcher/Flow.Launcher PR: 3118
File: Flow.Launcher/ViewModel/MainViewModel.cs:1404-1413
Timestamp: 2024-12-08T21:12:12.060Z
Learning: In the `MainViewModel` class, the `_lastQuery` field is initialized in the constructor and is never null.
Applied to files:
Flow.Launcher/ViewModel/MainViewModel.cs
📚 Learning: 2025-06-08T14:12:21.348Z
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3672
File: Flow.Launcher/MainWindow.xaml.cs:244-247
Timestamp: 2025-06-08T14:12:21.348Z
Learning: In Flow.Launcher, App.NotifyIcon is created before MainWindow creation, so null checks for App.NotifyIcon are not necessary when accessing it from MainWindow code.
Applied to files:
Flow.Launcher/ViewModel/MainViewModel.cs
🧬 Code graph analysis (1)
Flow.Launcher/ViewModel/MainViewModel.cs (1)
Flow.Launcher.Plugin/Query.cs (1)
Query(9-126)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (4)
Flow.Launcher/ViewModel/MainViewModel.cs (4)
2-2: LGTM!The
System.Collections.Concurrentimport is correctly added to support the newConcurrentDictionaryusage.
41-41: LGTM!The
ConcurrentDictionary<Guid, Query>declaration correctly enables thread-safe tracking of multiple concurrent queries.
1530-1540: LGTM!The dictionary cleanup on Line 1534 correctly removes the query entry after all plugin tasks complete, ensuring the progress bar can be hidden appropriately.
1542-1546: LGTM!The finally block correctly ensures cleanup of the dictionary entry in all code paths (success, cancellation, or exception), preventing memory leaks.
Jack251970
removed
bug
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Flow.Launcher/ViewModel/MainViewModel.cs (1)
1546-1547: Defensive cleanup is correctly implemented.The
finallyblock ensures the query session is removed from the progress tracking dictionary in all cases (cancellation, exception, or normal completion). This complements the removal at line 1536 and provides robust cleanup.💡 Optional: Clarify dual removal pattern
The comment could note that this cleanup is defensive and also runs on the success path. This helps future maintainers understand why removal happens in two places:
- // this ensures the query is removed from the progress tracking dictionary when this query is canceled or completes + // Defensive cleanup: ensures the query session is removed from the progress tracking dictionary + // even if cancellation or an exception occurs. Safe to call even if already removed at line 1536. _progressQueryDict.Remove(updateGuid, out _);
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Flow.Launcher/ViewModel/MainViewModel.cs
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3593
File: Plugins/Flow.Launcher.Plugin.WebSearch/SettingsControl.xaml.cs:143-165
Timestamp: 2025-09-13T06:04:26.977Z
Learning: In Plugins/Flow.Launcher.Plugin.WebSearch/SettingsControl.xaml, the ListView_SizeChanged handler in the C# file assumes 5 GridViewColumns but the XAML may have more columns. The user Jack251970 indicated this mismatch doesn't need to be fixed when pointed out during PR #3593 review.
📚 Learning: 2024-12-08T21:12:12.060Z
Learnt from: Yusyuriv
Repo: Flow-Launcher/Flow.Launcher PR: 3118
File: Flow.Launcher/ViewModel/MainViewModel.cs:1404-1413
Timestamp: 2024-12-08T21:12:12.060Z
Learning: In the `MainViewModel` class, the `_lastQuery` field is initialized in the constructor and is never null.
Applied to files:
Flow.Launcher/ViewModel/MainViewModel.cs
📚 Learning: 2025-06-08T14:12:21.348Z
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3672
File: Flow.Launcher/MainWindow.xaml.cs:244-247
Timestamp: 2025-06-08T14:12:21.348Z
Learning: In Flow.Launcher, App.NotifyIcon is created before MainWindow creation, so null checks for App.NotifyIcon are not necessary when accessing it from MainWindow code.
Applied to files:
Flow.Launcher/ViewModel/MainViewModel.cs
🧬 Code graph analysis (1)
Flow.Launcher/ViewModel/MainViewModel.cs (1)
Flow.Launcher.Plugin/Query.cs (1)
Query(9-126)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (6)
Flow.Launcher/ViewModel/MainViewModel.cs (6)
2-2: LGTM!The addition of
System.Collections.Concurrentis necessary for theConcurrentDictionaryusage and is correctly placed.
41-41: LGTM!The change from a single
Queryfield toConcurrentDictionary<Guid, Query>correctly enables thread-safe tracking of multiple concurrent queries. Thereadonlymodifier appropriately prevents reassignment.
1410-1411: LGTM!Creating a unique
Guidfor each update session enables proper tracking and filtering of progress state across concurrent queries. The comment clearly explains the purpose.
1424-1424: LGTM!Using
TryAddto register the query session in the dictionary is correct. The return value doesn't need to be checked sinceGuid.NewGuid()collisions are virtually impossible, and even in that astronomically unlikely case, only progress bar tracking would be affected, not query execution.
1479-1480: LGTM!The progress bar visibility logic correctly uses the per-session
updateGuidto determine if progress should be shown. This ensures the progress bar accurately reflects the state of the current query session, even when multiple queries execute concurrently.
1536-1536: LGTM!Removing the progress entry after all query tasks complete is correct. Note that this removal is also performed in the
finallyblock (line 1547), which is safe sinceConcurrentDictionary.Removeis idempotent and provides defensive cleanup.
Progress Query Management and Thread Safety
_progressQueryfield with a thread-safeConcurrentDictionary<Guid, Query>to track multiple in-progress queries, improving reliability in concurrent scenarios. (Flow.Launcher/ViewModel/MainViewModel.cs) [1] [2] [3] [4] [5] [6]QueryResultsAsyncto use the new dictionary for managing progress queries, ensuring correct progress bar visibility and cleanup.This improvement help to resolve possible progress bar issue when typing so fast or use custom query.