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 |
![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.
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. |
There was a problem hiding this comment.
1 issue found across 3 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:167">
P2: Changing UseGlyphIcons/ShowBadges no longer triggers QueryResults(), but ResultViewModel doesn’t listen to Settings changes for ShowGlyph/ShowBadge. This can leave visible results stale until the next query. Consider re-querying (or otherwise refreshing result view models) when these settings change.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Refactor property change handling in MainViewModel.cs so that changes to ShowBadges and ShowBadgesGlobalOnly now trigger QueryResults(), ensuring UI updates reflect badge settings instantly. Also, separate MaxResultsToShow handling for clarity.
Changed PinnedGridReservedResultCount from int to double for more precise reserved result calculation. Updated MaxHeight logic to use Math.Floor for correct item count. Switched to ResultsSelected method for selection checks. Added comment clarifying pinned grid margin calculation.
Move PinnedGridHeightForEmptyQuery PropertyChanged trigger into the PinnedGridReservedResultCount getter to only notify on actual value changes. Update ResultsViewModel to listen for PinnedGridHeightForEmptyQuery changes, improving efficiency and clarity.
Updated the Border margin in PinnedResultGrid.xaml and aligned the PinnedGridHeightForEmptyQuery calculation in MainViewModel.cs to ensure consistent layout spacing.
Centralize UpdatePreviewAsync calls by triggering them via a PropertyChanged handler for PreviewSelectedItem, reducing code duplication. Adjust selection and index reset logic to ensure correct preview behavior when switching between grid and list modes.
Add null checks before assigning SelectedItem from Results, History, and PinnedResults to PreviewSelectedItem. This ensures PreviewSelectedItem is only updated when a valid item is selected, avoiding unintended null assignments.
Jack251970
removed
the
review in progress
|
@01Dri Please help me check and test this feature? I have polished UI and fixed some issues with preview panel. |
There was a problem hiding this comment.
1 issue found across 3 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:178">
P1: Preview refresh is wired to `Settings.PropertyChanged` using `nameof(PreviewSelectedItem)`, so the new trigger is effectively unreachable and preview updates can stop after selection changes.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
UpdatePreviewAsync is now called directly in the PreviewSelectedItem setter to ensure immediate updates. Added a debug log for easier troubleshooting.
Prevent value from being set to 0 when reserving results for the pinned grid layout by using Math.Max(1, ...). This guarantees at least one result is always reserved, even with low MaxResultsToShow settings.
|
@Jack251970 I think I found two issues. First, in grid mode, it seems that the history results are appearing too far down in the results list. Additionally, the history results are not being displayed when the home page is disabled, even though "Show history results" is enabled. I will investigate this further. |
Regarding the first issue, after comparing it with the current version, it appears to be normal behavior. About the second issue, I would like to confirm with you: when the home page is disabled and "Show History Results on Home Page" is enabled, should the flow display the history results or not? |
I think they should not be displayed. |
|
@01Dri Hi, would you remind if I change this PR to draft for onesounds to work on the theme design part? |
Sure, no problem. |
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