Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a "Pinned Results" feature: new storage and pinned-result model, settings and UI (list/grid), ViewModel and MainWindow wiring for pin/unpin and execution, ResultHelper signature change, PluginManager helper, and a small uninstall-flow change capturing uninstall result. Changes
Sequence DiagramsequenceDiagram
participant User
participant UI as MainWindow UI
participant VM as MainViewModel
participant Storage as Pinned Storage
participant PluginMgr as PluginManager
User->>UI: Pin item (click / context menu)
UI->>VM: OnPinnedItemClick / Pin command
VM->>Storage: Add(result, query)
Storage->>PluginMgr: Resolve plugin directory / ico path
PluginMgr-->>Storage: Directory / IDs
Storage-->>VM: Item added/updated
VM->>Storage: Persist pinned storage
UI->>VM: RefreshPinnedResults (startup / query change)
VM->>Storage: GetPinnedResultItems(query)
Storage-->>VM: Filtered pinned items
VM->>UI: Render PinnedResults (List/Grid)
User->>UI: Click pinned item
UI->>VM: ExecutePinnedResult(item)
VM->>VM: result.ExecuteAsync(ActionContext)
VM->>UI: Hide window if requested
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 Tip You can disable the changed files summary in the walkthrough.Disable the |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
5 issues found across 13 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Flow.Launcher/SettingPages/ViewModels/SettingsPaneGeneralViewModel.cs">
<violation number="1" location="Flow.Launcher/SettingPages/ViewModels/SettingsPaneGeneralViewModel.cs:151">
P2: PinnedLayouts dropdown is added but its labels are never refreshed in UpdateEnumDropdownLocalizations, so changing language won’t update this dropdown’s localized labels.</violation>
</file>
<file name="Flow.Launcher.Infrastructure/UserSettings/Settings.cs">
<violation number="1" location="Flow.Launcher.Infrastructure/UserSettings/Settings.cs:393">
P2: Public backing auto-property will be serialized separately, creating an unintended extra settings key and bypassing `OnPropertyChanged` when deserialized.</violation>
</file>
<file name="Flow.Launcher/Storage/Pinned.cs">
<violation number="1" location="Flow.Launcher/Storage/Pinned.cs:18">
P2: Evicts the oldest pin before checking for an existing pin, which can drop an unrelated item when re-pinning an already pinned result at max capacity.</violation>
<violation number="2" location="Flow.Launcher/Storage/Pinned.cs:53">
P2: AddOrRemove uses query-aware existence but query-unaware removal, which can remove the wrong pinned item when same result is pinned under multiple queries.</violation>
</file>
<file name="Flow.Launcher/Storage/PinnedResultItem.cs">
<violation number="1" location="Flow.Launcher/Storage/PinnedResultItem.cs:69">
P2: DeepCopy rebuilds OriginQuery from the serialized Query string, which is empty for non-query pinned items, losing the original OriginQuery copied from the underlying Result.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Add one-off context when rerunning by tagging
@cubic-dev-aiwith guidance or docs links (includingllms.txt) - Ask questions if you need clarification on any suggestion
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Replaced ViewModel property bindings in MultiDataTrigger with direct bindings to UI element Visibility. Removed IsContextMenuVisible property and related code from MainViewModel.cs, simplifying visibility logic and improving UI responsiveness.
|
@01Dri Hi, I have reviewed your codes and it generally works well. Apart from some quality improvements in the code, I have made these changes:
Now I think logic is okay for me and I would like to ask your some questions about UI.
|
Hey Jack, thanks for the review! I agree with you about the grid UI. One feature I’ve been wanting to add is keyboard navigation/selection. I wasn’t able to implement it properly before, but I’ll give it another try. Adding a message in the center of the grid when there are no results sounds like a great idea, I’ll go ahead and implement that. Regarding the first item being selected by default, I believe that’s a leftover from my previous attempt at implementing keyboard navigation. The grid UI behaves quite differently from the default flow, which makes some features, like keyboard navigation, more challenging, especially due to the context menu interactions. I’ll add the empty state message to the grid. Do you have any suggestions on how we could further improve the grid UI? I’d really appreciate any ideas or feedback. |
|
@Jack251970 I thought about a grid mode and put together an initial implementation. When the user presses Ctrl + G, Flow enters grid mode, allowing navigation through the grid using the keyboard. I’ve also added an empty state message. Could you test it? Thanks! |
Changed keyboard handling in grid mode: Down/Up keys now select next/previous column instead of row, using SelectNextColumn and SelectPrevColumn. Right/Left keys remain unchanged.
Improved handling of selected indices when toggling between grid and list modes for pinned results. Explicitly disables grid mode when hiding the view to ensure consistent UI state.
Added logic to queue result updates when switching to grid layout for pinned results. Errors are logged if the update channel write fails, ensuring UI stays in sync when home page is disabled.
Added refreshed flag to track pinned/history queries. Introduced QueryEmptyTask to force refresh if no tasks or queries are executed. Improved home page and history result handling. Refactored cancellation exception handling and enhanced error logging for result updates.
Synchronize PreviewSelectedItem and trigger UpdatePreviewAsync to ensure the preview reflects the currently selected pinned item, improving user experience and UI consistency.
Added a PropertyChanged event handler for PinnedResults in MainViewModel. Now, when the SelectedItem property changes, PreviewSelectedItem is updated and UpdatePreviewAsync is called to refresh the preview accordingly.
|
@01Dri All things looks good to me. I wonder if you can add mouse navigation logic also to the Pinned Results like what the result list do? I find that if I use Ctrl+G to enter pinned Grid and I moved mouse to the list below, it will select the focused item in the result list. In that time, both item in pinned Grid and result list will be selected. 
|
|
And if you can, I think we can add the Grid to a new user control for code quality. |
|
@01Dri Another issue is that the height of the main window of the empty query will change if we choose to display pinned Grid. |
Hi Jack, I don’t think I understand the issue. Could you explain it to me? Thanks! |
There was a problem hiding this comment.
3 issues found across 9 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Flow.Launcher/ViewModel/MainViewModel.cs">
<violation number="1" location="Flow.Launcher/ViewModel/MainViewModel.cs:1015">
P1: `IsGridMode` setter now performs selection/property updates that can run off the UI thread via `Hide()` paths after `ConfigureAwait(false)`.</violation>
</file>
<file name="Flow.Launcher/PinnedResultGrid.xaml">
<violation number="1" location="Flow.Launcher/PinnedResultGrid.xaml:43">
P1: Incorrect binding path after DataContext override can prevent pinned-grid click commands from binding.</violation>
<violation number="2" location="Flow.Launcher/PinnedResultGrid.xaml:49">
P2: Visibility trigger uses wrong binding path under overridden DataContext, so empty-state collapse may not fire.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Removed explicit resetting of SelectedResults and PinnedResults SelectedIndex to -1 when toggling grid mode. Now, the selected index is only set to 0 if it was previously -1, preserving selection state more intuitively.
Introduce MouseSelectCommand to ResultListBox and ResultGrid, enabling command-based mouse selection logic in the ViewModel. Refactor IsGridMode handling to use this command. Simplify item selection triggers in ResultGrid and perform code cleanup. Pin/unpin actions now display a success message.
Simplify Enter key command execution with null-conditional operator. Replace StackPanel mouse event with ListBox-level handlers for left and right mouse clicks. Remove OnItemClick and add dedicated handlers for mouse up/down events to improve input handling and code clarity.
The ToolTip="{Binding ShowSubTitleToolTip}" was removed from the StackPanel in the DataTemplate for ResultViewModel in ResultGrid.xaml to simplify the UI and remove unnecessary tooltips.
There was a problem hiding this comment.
3 issues found across 8 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Flow.Launcher/ResultGrid.xaml.cs">
<violation number="1" location="Flow.Launcher/ResultGrid.xaml.cs:76">
P2: Mouse-down selection uses stale cached `curItem` from prior hover instead of resolving the item from the current click target, which can select the wrong result.</violation>
</file>
<file name="Flow.Launcher/ResultListBox.xaml.cs">
<violation number="1" location="Flow.Launcher/ResultListBox.xaml.cs:75">
P2: Mouse-move selection command can fire continuously because `_lastpos` is never updated in `OnMouseMove`.</violation>
</file>
<file name="Flow.Launcher/ViewModel/MainViewModel.cs">
<violation number="1" location="Flow.Launcher/ViewModel/MainViewModel.cs:1430">
P2: Pin/unpin context-menu actions now keep the menu open while using captured pinned-state booleans, causing stale menu state and potentially incorrect repeated operations.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Ensure _lastpos is updated before selecting ListBoxItem and executing MouseSelectCommand in both ResultGrid.xaml.cs and ResultListBox.xaml.cs. Pass true for MouseSelectCommand in ResultGrid and false in ResultListBox for correct context.
I polished and made several adjustments to your changes. 
As you can see in this figure, when I enable the grid, the main window has larger height under empty search than that under non-empty search. I think we should adjust the query item number under empty search. |
Changes
New option to add a result or a query to pinned results.
Added two visual styles for pinned results:
Observations
Preview
example.1.mp4
example.2.mp4
Future