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.
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. |
|
What are your thoughts about adding/replacing keyboard down arrow to enter pinned result grid view? |
In my first implementation this was harder to do, but now with the new user control for grid mode it might be possible. I’ll take a look. |
|
You're iterating quickly on this pull request. To help protect your rate limits, cubic has paused automatic reviews on new pushes for now—when you're ready for another review, comment |
- Adjust Style
…w.Launcher into feature/pinned-results
I have added this functionality, can you test? thanks! |
|
|
||
| namespace Flow.Launcher | ||
| { | ||
| public partial class ResultGrid : UserControl |
There was a problem hiding this comment.
A couple of things need tweaking with the pinned results grid:
- When no pinned results, either hide the grid or the no pinned results text need to be more visible. This is from fresh install.
- When pinned results is turned on, and query is empty (essentially at homepage) I think the default should be to only highlight the first pinned result rather than both pinned and the result below. This should be consistent including hide/show the search window. Let me know what you think @Jack251970.
-
When navigated down to the results list, hide then show the search window and press down again should continue to scroll down the result list rather than reactivating the pinned results navigation
-
When navigating with up arrow beyond the grid result should go to the last result of the result list (continuous scrolling)
There was a problem hiding this comment.
- It should be a bug.
- Yes, only one item should be highlighted.
There was a problem hiding this comment.
A couple of things need tweaking with the pinned results grid:
1. When no pinned results, either hide the grid or the no pinned results text need to be more visible. This is from fresh install.
2. When pinned results is turned on, and query is empty (essentially at homepage) I think the default should be to only highlight the first pinned result rather than both pinned and the result below. This should be consistent including hide/show the search window. Let me know what you think @Jack251970.
3. When navigated down to the results list, hide then show the search window and press down again should continue to scroll down the result list rather than reactivating the pinned results navigation 4. When navigating with up arrow beyond the grid result should go to the last result of the result list (continuous scrolling)
@Jack251970 @jjw24 I’ve made these adjustments, could you test again?
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