Remember selected position #3480
Conversation
|
🚨 Warning: Approaching Monthly Automation Limit Monthly PRs automated: 249/250 Your organization has used over 90% of its monthly quota for gitStream automations. Once the quota is reached, new pull requests for this month will not trigger gitStream automations and will be marked as “Skipped”. To avoid interruptions, consider optimizing your automation usage or upgrading your plan by contacting LinearB. |
This comment has been minimized.
This comment has been minimized.
|
🥷 Code experts: Jack251970 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:
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. |
|
@coderabbitai Review it please. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 21322 🏁 Script executed: Length of output: 105307 @onesounds I've reviewed PR #3480 "Remember selected position" and it looks like a solid implementation. The PR addresses an important UX improvement by preventing the selected index from resetting to 0 in two key scenarios:
The implementation uses a comprehensive approach to track and restore the selection:
The code is well-structured with proper validation and edge case handling. The use of a flag to suppress query execution when returning from context menus is an elegant solution to prevent index reset. This change will definitely improve the user experience by making the selection behavior more predictable and preventing frustrating position resets. |
📝 Walkthrough""" WalkthroughThis update introduces mechanisms to better manage the selection state and query execution flow in the application's main window and view models. It adds logic to reset the selected result index when the query text is cleared, enhances the ability to suppress query execution during certain UI state changes, and improves the restoration of previously selected items when switching between query results and context menus. The update also simplifies result update logic and refines visibility management based on the presence of results. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MainWindow
participant MainViewModel
participant ResultsViewModel
User->>MainWindow: Changes query text
MainWindow->>MainViewModel: Update QueryText
alt Query text is empty
MainWindow->>ResultsViewModel: ResetSelectedIndex
end
MainWindow->>MainViewModel: Query (with delay)
MainViewModel->>MainViewModel: Check _blockQueryExecution
alt Not blocked
MainViewModel->>ResultsViewModel: AddResults
ResultsViewModel->>ResultsViewModel: Restore selection if possible
end
Assessment against linked issues
Possibly related PRs
Suggested labels
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (1)
✨ 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 (5)
Flow.Launcher/MainWindow.xaml.cs (1)
1128-1134: Avoid duplicating selection‑reset logic – delegate to ViewModel only
QueryTextBox_TextChanged1now resets the selected index every time the box becomes empty.
The same action is already performed inMainViewModel.ChangeQueryTextAsync(lines 663‑667) and – thanks to the newResultsViewModel.ResetSelectedIndex– can be triggered from there alone. Executing it twice is harmless but unnecessary work and increases the risk of future divergence.- // Reset Index when query null - if (string.IsNullOrEmpty(textBox.Text)) - { - _viewModel.Results.ResetSelectedIndex(); - } + // Index is reset by MainViewModel when neededFlow.Launcher/ViewModel/ResultsViewModel.cs (1)
220-279: Potential performance hot‑spot – lock granularity
UpdateResultsholds_collectionLockwhile updating the whole collection and while running the reselection algorithm (lines 229‑249).
The collection update already executes inResults.Update, which internally locks again; keeping the outer lock around additional work unnecessarily blocks other producer threads (e.g., concurrent plugin result updates).Moving the reselection logic after the update call – once the collection is in a stable state – will shorten the critical section:
lock (_collectionLock) { - Results.Update(newResults, token); - // reselection logic … + Results.Update(newResults, token); } +// reselection logic …Flow.Launcher/ViewModel/MainViewModel.cs (3)
663-667: Duplicate index reset – keep logic in one place
ChangeQueryTextAsyncresets the index when the new text is empty.
BecauseMainWindow.QueryTextBox_TextChanged1already does (or will after refactor) andResultsViewModel.ResetSelectedIndexcan be invoked from outside, consider removing one of the two to avoid maintenance overhead.No code diff provided because you may decide which layer should own this responsibility.
708-748: Persisting selection across views – guard against collection shrinkWhen returning from the context menu you restore the previous item or index (👍).
However, if plugins updated the results while the menu was open,_lastSelectedIndexmay now be out of bounds. You already check its upper bound but not that the list still contains at least one element (could be zero).-else if (_lastSelectedResultIndex >= 0 && _lastSelectedResultIndex < Results.Results.Count) +else if (Results.Results.Count > 0 && + _lastSelectedResultIndex >= 0 && + _lastSelectedResultIndex < Results.Results.Count)
1088-1100: Early return controlled by_blockQueryExecutionmay drop legitimate user queries
_blockQueryExecutionis a single Boolean shared by several code paths.
If a long‑running async method schedulesQueryAsyncafter you set_blockQueryExecution = true, there is a chance the flag is reset before the delayed call executes, letting the query through unexpectedly. Conversely, if it remains true longer than intended, it may discard a user‑typed query.Consider using a more scoped mechanism, e.g. a
SemaphoreSlimor an incrementing token, so you can reliably detect “my suppression period” instead of a global flag.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Flow.Launcher/MainWindow.xaml.cs(1 hunks)Flow.Launcher/ViewModel/MainViewModel.cs(7 hunks)Flow.Launcher/ViewModel/ResultsViewModel.cs(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
Flow.Launcher/MainWindow.xaml.cs (1)
Flow.Launcher/ViewModel/ResultsViewModel.cs (1)
ResetSelectedIndex(176-184)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: build
This comment has been minimized.
This comment has been minimized.
|
@copilot Review it |
There was a problem hiding this comment.
Pull Request Overview
This PR improves how the application retains the selected result index when a query is updated or when returning from the context menu, aiming to create a more consistent user experience.
- Introduces a ResetSelectedIndex method and uses internal properties (_lastSelectedItem, _lastSelectedIndex) to cache and restore selections.
- Updates ChangeQueryText and related asynchronous query logic to optionally suppress execution and preserve selection state on context menu returns.
- Ensures that empty query strings trigger a reset of the selected index in both the view model and view layer.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| Flow.Launcher/ViewModel/ResultsViewModel.cs | Adds selection retention logic and a ResetSelectedIndex method to avoid index resets. |
| Flow.Launcher/ViewModel/MainViewModel.cs | Updates query text change handling with a new suppressQueryExecution flag and restores previous selection on context menu returns. |
| Flow.Launcher/MainWindow.xaml.cs | Calls ResetSelectedIndex when the query becomes empty to ensure proper selection state. |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
Flow.Launcher/ViewModel/ResultsViewModel.cs (2)
264-269: Redundant selection logic outside the lock.This selection fallback logic is already implemented within the lock section above (lines 256-260). Having it both inside and outside the lock creates redundancy and potential race conditions where the selection could be set twice.
- // If no item is selected, select the first one - if (Results.Count > 0 && (SelectedIndex == -1 || SelectedItem == null)) - { - SelectedIndex = 0; - SelectedItem = Results[0]; - }
271-290: Consider simplifying visibility management.The visibility management logic has nested conditions that could be simplified. Additionally, there's more redundant selection logic that's already handled above.
// Visibility update - fix for related issue if (Results.Count > 0) { - // 1. Always ensure index is valid when there are results - if (SelectedIndex == -1 || SelectedItem == null) - { - SelectedIndex = 0; - SelectedItem = Results[0]; - } - - // 2. Update visibility if (Visibility == Visibility.Collapsed) { Visibility = Visibility.Visible; } } else if (Visibility == Visibility.Visible && Results.Count == 0) { Visibility = Visibility.Collapsed; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Flow.Launcher/ViewModel/ResultsViewModel.cs(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
Flow.Launcher/ViewModel/ResultsViewModel.cs (1)
Flow.Launcher/ViewModel/ResultViewModel.cs (2)
ResultViewModel(14-328)ResultViewModel(21-63)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: build
🔇 Additional comments (4)
Flow.Launcher/ViewModel/ResultsViewModel.cs (4)
26-27: Well-designed selection tracking system.Adding these private fields to track the last selected item and index serves as the foundation for preserving selection state when navigating between views or during result updates. The default value of -1 for
_lastSelectedIndexis appropriate as it indicates no selection.
176-187: Good implementation ofResetSelectedIndexmethod.This method properly:
- Resets cached selection state
- Sets selection to the first item when results exist
- Raises property change notifications to update UI bindings
The addition of
_lastSelectedItem = nulladdresses the previous review comment about preventing accidental reselection of stale items.
203-221: Thread-safe selection capture implemented correctly.This code properly captures the current selection on the UI thread using
Dispatcher.Invoke(), addressing the thread safety concern raised in a previous review comment. Storing both the item reference and index provides robust fallback options for selection restoration.
232-261: Comprehensive selection restoration strategy.The implementation follows a well-thought-out prioritization:
- First attempt to find and select the exact same item
- Fall back to the same index position if the item is gone
- Default to the first item if necessary
This approach helps maintain a consistent user experience by preserving selection state as much as possible.
onesounds
added
enhancement
This comment has been minimized.
This comment has been minimized.
| { | ||
| _blockQueryExecution = true; | ||
| // Set querytext without running the query for keep index. if not use this, index will be 0. | ||
| _queryText = _queryTextBeforeLeaveResults; |
There was a problem hiding this comment.
why not just change QueryText
There was a problem hiding this comment.
I noted this in the PR description:
If you simply update the queryText, the index resets to 0 when the results are refreshed. The flow goes like this:
- Remember the previously selected item
- Open the context menu (which clears the query box text)
- Close the context menu (the previous query text is restored)
Even if the selected index is correctly remembered and restored at step 3, it becomes pointless because restoring the queryText causes the index to reset to 0 again.
| _blockQueryExecution = false; | ||
| } | ||
| // restore selected item | ||
| if (Results.Results.Contains(_lastSelectedResultItem)) |
There was a problem hiding this comment.
Would this be slow for large results? And I am curious when will Results not contains the last selected result item? Why not just use _lastSelectedIndex
There was a problem hiding this comment.
OK. Modified it to work using only the index.
| CancellationToken token; | ||
|
|
||
| // Block the query execution when Open ContextMenu is called | ||
| if (!QueryResultsSelected()) |
There was a problem hiding this comment.
I don't understand here. Query shouldn't be executed when context menu is called? It will only call QueryContextMenu?
There was a problem hiding this comment.
I already mentioned this in the PR description. If you open the context menu while results are still loading, both the context menu and the results panel end up being shown once the loading finishes. Please read the PR description before review.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Flow.Launcher/ViewModel/MainViewModel.cs (1)
1339-1345: Consider making this method private or internalThis method currently has public visibility but seems to be used only internally. Consider reducing its visibility scope unless there's a specific reason for it to be public.
-public void ResetSelectionIfQueryEmpty(string queryText) +private void ResetSelectionIfQueryEmpty(string queryText)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Flow.Launcher/ViewModel/MainViewModel.cs(8 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (9)
Flow.Launcher/ViewModel/MainViewModel.cs (9)
37-37: Good addition of the flag to control query executionThis boolean flag is key to the implementation, allowing suppression of query execution during certain UI state transitions, particularly when restoring state after returning from context menus.
642-645: Good enhancement to support suppression of query executionAdding the
suppressQueryExecutionparameter allows for more granular control over when queries are executed during text changes, which is essential for maintaining selection state.
649-666: Properly implemented conditional query executionThe implementation correctly calls
ResetSelectionIfQueryEmptyand conditionally runs the query only whensuppressQueryExecutionis false. This is critical for preserving the selected index when updating the query text.
684-686: Good approach to store selection stateAdding fields to track both the selected item and its index provides a robust mechanism for restoring selection when returning from context menus or other views.
695-701: Essential selection preservation logicThis code properly saves the current selection state when navigating away from query results, which is a key component of the PR's goal to remember selected positions.
714-734: Robust implementation of selection restorationThe try-finally block properly manages the query execution flag to prevent unnecessary queries. The restoration logic correctly handles both the query text and selected item/index when returning from the context menu.
1083-1084: Essential early return to block query executionThis check at the beginning of
QueryAsyncis critical for the implementation, allowing the system to skip query execution during controlled UI state transitions.
1658-1664: Important validation to prevent UI inconsistenciesThese additional checks prevent updates when they shouldn't occur, particularly addressing the noted issue where both the context menu and result list could be shown simultaneously when a context menu is opened during loading.
1675-1679: Simpler, more maintainable result update logicReplacing complex scoring and updating logic with a cleaner approach that leverages the enhanced selection preservation capabilities of
ResultsViewModel. This approach is more maintainable and easier to understand.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@coderabbitai Review it. |
|
Tip For best results, initiate chat on the files or code changes. @onesounds I've reviewed the PR and agree with the previous assessment that this is a solid implementation. Your changes address an important UX issue by maintaining selection state in two critical scenarios:
The technical implementation is particularly well-designed:
This implementation demonstrates careful consideration of the user experience and shows good attention to detail in handling all the edge cases. The code is well-structured and the changes are focused on solving the specific problem without unnecessary modifications. This is a valuable improvement that will make the application feel more polished and predictable for users. |
This comment has been minimized.
This comment has been minimized.
@check-spelling-bot Report🔴 Please reviewSee the 📂 files view, the 📜action log, or 📝 job summary for details.
See ❌ Event descriptions for more information. Forbidden patterns 🙅 (1)In order to address this, you could change the content to not match the forbidden patterns (comments before forbidden patterns may help explain why they're forbidden), add patterns for acceptable instances, or adjust the forbidden patterns themselves. These forbidden patterns matched content: s.b. workaround(s)If the flagged items are 🤯 false positivesIf items relate to a ...
|
|
Fixed in #3503 |
4 participants
Improve SelectedIndex Retention on Query Updates and Context Menu Return
Summary
This PR improves the behavior of
SelectedIndexto prevent it from resetting to 0 in the following scenarios:By avoiding unnecessary index resets, the user experience becomes more consistent and intuitive.
Details
Main Changes
Retain SelectedIndex on context menu return:
Previously, returning from a context menu caused the query to be re-applied, which reset the index to 0.
A new logic now updates the
QueryTextwithout refreshing the result list, preventing index reset in this case.Handle null QueryText:
When
QueryTextbecomesnull, the stored index is now properly cleared.Changed behavior for users with "retain query" mode enabled:
Previously, the index would reset even if the query remained the same.
Now, the index is preserved as long as the query is not cleared.
This is considered more intuitive, so no setting toggle was added.
Additional Fixes
A bug was discovered where opening the context menu before loading completed could result in both the context menu and results being shown at the same time.
The update logic now checks for open context menus and skips result updates if one is active.
Test Cases
During testing, there were a few instances where no index was selected, so further testing is needed.