[Feature Request] "Last opened" history mode #4042
Conversation
…action between query result or last opened
|
🥷 Code experts: Jack251970, taooceros Jack251970, taooceros 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:
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: ✨ Comment |
|
Be a legend 🏆 by adding a before and after screenshot of the changes you made, especially if they are around UI/UX. |
This comment has been minimized.
This comment has been minimized.
📝 WalkthroughWalkthroughSplits the home-page history setting into an umbrella toggle plus two mutually-exclusive modes (query vs last-opened), updates UI/localization and Settings, adds a new History storage/migration and helper actions, promotes HistoryItem to a top-level serializable type, removes legacy QueryHistory, and wires ViewModel/MainWindow to the new APIs. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as MainView
participant VM as MainViewModel
participant Hist as Storage.History
participant Set as Settings
rect rgb(243,248,255)
note right of User: Selecting/executing a result
User->>UI: Select result
UI->>VM: OnResultSelected(result)
VM->>Hist: AddToHistory(result, Set)
alt ShowHistoryOnHomePage = false
Hist-->>VM: no-op
else ShowHistoryQueryResultsForHomePage = true
Hist->>Hist: AddLastQuery(result)
else ShowHistoryLastOpenedResultsForHomePage = true
Hist->>Hist: AddLastOpened(result)
end
VM-->>UI: Continue execution
end
rect rgb(240,255,244)
note right of User: Opening home (empty query)
User->>UI: Focus (empty query)
UI->>VM: Query("")
VM->>Hist: GetHistoryItems(Set)
alt Query mode
Hist-->>VM: QueryHistoryItems (with QueryAction)
else Last-opened mode
Hist-->>VM: LastOpenedHistoryItems (with ExecuteAction)
else Disabled
Hist-->>VM: []
end
VM-->>UI: Render history results
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Flow.Launcher/MainWindow.xaml.cs (1)
324-329: Refresh home page when switching to “last opened”.Line 324 handles
ShowHistoryQueryResultsForHomePage, but the newShowHistoryLastOpenedResultsForHomePagenever triggers a refresh. Toggling the radio button therefore leaves the home page stuck on the previous mode until something else causes a re-query. Please add the same branch for the new property so_viewModel.QueryResults()runs immediately.case nameof(Settings.ShowHomePage): + case nameof(Settings.ShowHistoryLastOpenedResultsForHomePage): case nameof(Settings.ShowHistoryQueryResultsForHomePage): if (_viewModel.QueryResultsSelected() && string.IsNullOrEmpty(_viewModel.QueryText)) { _viewModel.QueryResults();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
Flow.Launcher.Infrastructure/UserSettings/Settings.cs(1 hunks)Flow.Launcher/Languages/en.xaml(1 hunks)Flow.Launcher/MainWindow.xaml.cs(1 hunks)Flow.Launcher/SettingPages/Views/SettingsPaneGeneral.xaml(1 hunks)Flow.Launcher/Storage/History.cs(1 hunks)Flow.Launcher/Storage/HistoryHelper.cs(1 hunks)Flow.Launcher/Storage/HistoryItem.cs(1 hunks)Flow.Launcher/Storage/HistoryItemLegacy.cs(1 hunks)Flow.Launcher/Storage/QueryHistory.cs(0 hunks)Flow.Launcher/ViewModel/MainViewModel.cs(18 hunks)
💤 Files with no reviewable changes (1)
- Flow.Launcher/Storage/QueryHistory.cs
🧰 Additional context used
🧬 Code graph analysis (5)
Flow.Launcher/Storage/HistoryHelper.cs (3)
Flow.Launcher/Storage/History.cs (1)
List(31-40)Flow.Launcher/Storage/HistoryItem.cs (1)
HistoryItem(7-22)Flow.Launcher.Core/Plugin/QueryBuilder.cs (1)
QueryBuilder(7-61)
Flow.Launcher/Storage/History.cs (3)
Flow.Launcher/Storage/HistoryHelper.cs (2)
List(11-22)HistoryHelper(9-45)Flow.Launcher/Storage/HistoryItemLegacy.cs (1)
HistoryItemLegacy(8-13)Flow.Launcher/Storage/HistoryItem.cs (1)
HistoryItem(7-22)
Flow.Launcher/MainWindow.xaml.cs (1)
Flow.Launcher.Infrastructure/UserSettings/Settings.cs (1)
Settings(15-662)
Flow.Launcher/Storage/HistoryItem.cs (1)
Flow.Launcher/Storage/HistoryHelper.cs (2)
Func(24-35)Func(36-44)
Flow.Launcher/ViewModel/MainViewModel.cs (3)
Flow.Launcher/Storage/History.cs (4)
History(10-111)PopulateHistoryFromLegacyHistory(42-54)AddToHistory(19-28)List(31-40)Flow.Launcher.Infrastructure/UserSettings/Settings.cs (2)
Settings(15-662)List(625-661)Flow.Launcher/Storage/HistoryItem.cs (1)
HistoryItem(7-22)
🪛 GitHub Actions: Check Spelling
Flow.Launcher/MainWindow.xaml.cs
[error] 862-862: Forbidden pattern: 'work around' matches a line_forbidden.patterns entry: \bwork[- ]arounds?\b.
[warning] 29-29: Unrecognized spelling: 'NKORE'.
[warning] 119-119: Unrecognized spelling: 'Wnd'.
[warning] 376-376: Unrecognized spelling: 'Wnd'.
[warning] 589-589: Unrecognized spelling: 'Wnd'.
[warning] 591-591: Unrecognized spelling: 'Wnd'.
[warning] 815-815: Unrecognized spelling: 'gamemode'.
[warning] 816-816: Unrecognized spelling: 'gamemode'.
[warning] 819-819: Unrecognized spelling: 'gamemode'.
[warning] 822-822: Unrecognized spelling: 'positionreset'.
[warning] 825-825: Unrecognized spelling: 'positionreset'.
[warning] 841-841: Unrecognized spelling: 'gamemode'.
[warning] 842-842: Unrecognized spelling: 'positionreset'.
[warning] 847-847: Unrecognized spelling: 'positionreset'.
[warning] 965-965: Unrecognized spelling: 'XRatio'.
[warning] 966-966: Unrecognized spelling: 'YRatio'.
[warning] 1180-1180: Unrecognized spelling: 'clocksb'.
[warning] 1181-1181: Unrecognized spelling: 'clocksb'.
[warning] 1182-1182: Unrecognized spelling: 'iconsb'.
[warning] 1183-1183: Unrecognized spelling: 'iconsb'.
[warning] 1188-1188: Unrecognized spelling: 'clocksb'.
[warning] 1189-1189: Unrecognized spelling: 'iconsb'.
[warning] 28-28: Unrecognized spelling: 'NKORE'.
[warning] 70-70: Unrecognized spelling: 'Wnd'.
[warning] 235-235: Unrecognized spelling: 'activing'.
[warning] 281-281: Unrecognized spelling: 'gamemode'.
[warning] 418-418: Unrecognized spelling: 'mainwindow'.
[warning] 506-506: Unrecognized spelling: 'Arrowkeys'.
[warning] 821-821: Unrecognized spelling: 'positionreset'.
Flow.Launcher/Languages/en.xaml
[warning] 25-25: Unrecognized spelling: 'uninstaller'.
Flow.Launcher/ViewModel/MainViewModel.cs
[warning] 26-26: Unrecognized spelling: 'NKORE'.
⏰ 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
| private void AddLastOpened(Result result) | ||
| { | ||
| var item = new HistoryItem | ||
| { | ||
| Title = result.Title, | ||
| SubTitle = result.SubTitle, | ||
| PluginID = result.PluginID, | ||
| RawQuery = result.OriginQuery.RawQuery, | ||
| ExecutedDateTime = DateTime.Now, | ||
| ExecuteAction = result.Action | ||
| }; | ||
|
|
||
| var existing = LastOpenedHistoryItems. | ||
| FirstOrDefault(x => x.Title == item.Title && x.PluginID == item.PluginID); | ||
|
|
||
|
|
||
| if (existing != null) | ||
| { | ||
| existing.ExecutedDateTime = DateTime.Now; | ||
| } | ||
| else | ||
| { | ||
| if (LastOpenedHistoryItems.Count > _maxHistory) | ||
| { | ||
| LastOpenedHistoryItems.RemoveAt(0); | ||
| } | ||
|
|
||
| LastOpenedHistoryItems.Add(item); | ||
| } |
There was a problem hiding this comment.
Reorder and refresh existing “last opened” entries.
When Line 97 finds an existing entry we only bump ExecutedDateTime. The item keeps its stale subtitle/raw query/action and stays in its old position, so recents never bubble to the top and the metadata no longer matches what the user just executed. Please replace the stored entry with the freshly-built item (and enforce the max size once), so the record reflects the latest query and ordering.
- if (existing != null)
- {
- existing.ExecutedDateTime = DateTime.Now;
- }
- else
- {
- if (LastOpenedHistoryItems.Count > _maxHistory)
- {
- LastOpenedHistoryItems.RemoveAt(0);
- }
-
- LastOpenedHistoryItems.Add(item);
- }
+ if (existing != null)
+ {
+ LastOpenedHistoryItems.Remove(existing);
+ }
+
+ if (LastOpenedHistoryItems.Count >= _maxHistory)
+ {
+ LastOpenedHistoryItems.RemoveAt(0);
+ }
+
+ LastOpenedHistoryItems.Add(item);🤖 Prompt for AI Agents
In Flow.Launcher/Storage/History.cs around lines 81 to 109, when AddLastOpened
finds an existing entry the code only updates ExecutedDateTime so the item's
subtitle, raw query, action and list position stay stale; instead, replace the
stored entry with the freshly-built item and move it to the most-recent
position: if an existing item is found, remove it from LastOpenedHistoryItems
and then add the new item (or replace at the same index and move to end), and
only after adding enforce the max size by removing from the start if Count >
_maxHistory so recents bubble to the top and metadata is current.
| private static Func<ActionContext, bool> GetExecuteAction(string pluginId, string rawQuery, string title, string subTitle) | ||
| { | ||
| var plugin = PluginManager.GetPluginForId(pluginId); | ||
|
|
||
| var query = QueryBuilder.Build(rawQuery, PluginManager.NonGlobalPlugins); | ||
| var freshResults = plugin.Plugin | ||
| .QueryAsync(query, CancellationToken.None) | ||
| .GetAwaiter() | ||
| .GetResult(); | ||
| return freshResults?.FirstOrDefault(r => r.Title == title | ||
| && r.SubTitle == subTitle)?.Action; | ||
| } |
There was a problem hiding this comment.
Guard against missing plugins when rebuilding actions.
Line 27 assumes PluginManager.GetPluginForId always returns a plugin and immediately dereferences plugin.Plugin. If the user uninstalls or disables the plugin after the history entry is stored, this becomes null and crashes while populating actions. Please return null (or drop the entry) when the plugin or query cannot be rebuilt, and wrap the QueryAsync call defensively so we don’t bring the UI down.
- var plugin = PluginManager.GetPluginForId(pluginId);
-
- var query = QueryBuilder.Build(rawQuery, PluginManager.NonGlobalPlugins);
- var freshResults = plugin.Plugin
- .QueryAsync(query, CancellationToken.None)
- .GetAwaiter()
- .GetResult();
+ if (string.IsNullOrEmpty(pluginId))
+ {
+ return null;
+ }
+
+ var plugin = PluginManager.GetPluginForId(pluginId);
+ if (plugin?.Plugin == null)
+ {
+ return null;
+ }
+
+ var query = QueryBuilder.Build(rawQuery ?? string.Empty, PluginManager.NonGlobalPlugins);
+ if (query == null)
+ {
+ return null;
+ }
+
+ IReadOnlyList<Result> freshResults;
+ try
+ {
+ freshResults = plugin.Plugin
+ .QueryAsync(query, CancellationToken.None)
+ .GetAwaiter()
+ .GetResult();
+ }
+ catch (Exception e)
+ {
+ App.API.LogException(nameof(HistoryHelper), "Failed to rebuild execute action for history item", e);
+ return null;
+ }🤖 Prompt for AI Agents
In Flow.Launcher/Storage/HistoryHelper.cs around lines 24 to 35, the code
assumes PluginManager.GetPluginForId(pluginId) and plugin.Plugin are non-null
and directly calls QueryAsync, which can throw or be null if a plugin was
uninstalled/disabled; update the method to check for null after GetPluginForId
and return null if the plugin or plugin.Plugin is missing, wrap the QueryAsync
call in a try/catch and return null on exceptions, and also handle null/empty
freshResults before attempting FirstOrDefault so the method safely returns null
(or signals the caller to drop the history entry) when rebuilding the action
fails.
| Title = | ||
| Settings.ShowHistoryQueryResultsForHomePage ? Localize.executeQuery(h.RawQuery) : h.Title, | ||
| SubTitle = Localize.lastExecuteTime(h.ExecutedDateTime), | ||
| IcoPath = Constant.HistoryIcon, | ||
| OriginQuery = new Query { RawQuery = h.Query }, | ||
| Action = _ => | ||
| { | ||
| App.API.BackToQueryResults(); | ||
| App.API.ChangeQuery(h.Query); | ||
| return false; | ||
| }, | ||
| PluginID = h.PluginID, | ||
| OriginQuery = QueryBuilder.Build(h.RawQuery, PluginManager.NonGlobalPlugins), | ||
| Action = Settings.ShowHistoryLastOpenedResultsForHomePage ? h.ExecuteAction : h.QueryAction, | ||
| Glyph = new GlyphInfo(FontFamily: "/Resources/#Segoe Fluent Icons", Glyph: "\uE81C") | ||
| }; |
There was a problem hiding this comment.
Preserve last-opened metadata in history rows.
Line 1330 replaces every subtitle with Localize.lastExecuteTime, so “Last opened” entries lose their original subtitle/path and fuzzy search can no longer match against it. That makes it impossible to tell identical titles apart or search by location. Keep the stored subtitle (and append the timestamp if you still want to surface it) so both information and searchability are retained.
- SubTitle = Localize.lastExecuteTime(h.ExecutedDateTime),
+ SubTitle = Settings.ShowHistoryLastOpenedResultsForHomePage
+ ? $"{h.SubTitle} • {Localize.lastExecuteTime(h.ExecutedDateTime)}"
+ : Localize.lastExecuteTime(h.ExecutedDateTime),🤖 Prompt for AI Agents
In Flow.Launcher/ViewModel/MainViewModel.cs around lines 1328 to 1336, the code
unconditionally replaces the history row SubTitle with
Localize.lastExecuteTime(h.ExecutedDateTime), which discards stored
subtitle/path metadata and breaks fuzzy search; instead preserve the original
stored subtitle from the history record and append (or prepend) the localized
timestamp only when present — for example, if h.SubTitle is empty use only the
timestamp, otherwise combine h.SubTitle and the timestamp with a separator;
ensure null/empty checks are handled so search still matches the original
subtitle text.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
Flow.Launcher/Storage/HistoryHelper.cs (1)
34-44: Guard against missing plugins and avoid blocking synchronous calls.This method has critical issues that were flagged in a previous review and remain unresolved:
- Null reference risks: Line 36-38 assume
PluginManager.GetPluginForIdreturns a non-null plugin andplugin.Pluginis available. If the plugin is uninstalled or disabled, this will crash.- No exception handling: Lines 38-41 call
QueryAsyncsynchronously without try-catch, which can throw exceptions.- Blocking synchronous call: Using
GetAwaiter().GetResult()blocks the thread. SincePopulateActionscalls this method in a loop, it can freeze the UI while querying multiple plugins synchronously.Please apply the defensive null checks and exception handling as suggested in the previous review. Additionally, consider refactoring to use async/await or pre-populate actions asynchronously to avoid blocking the UI thread.
Based on previous review comment.
🧹 Nitpick comments (2)
Flow.Launcher/Storage/HistoryHelper.cs (2)
11-22: Add null check for defensive programming.The
itemsparameter should be validated to prevent potential null reference exceptions.Apply this diff:
internal static List<HistoryItem> PopulateActions(this List<HistoryItem> items, bool isQuery) { + if (items == null) + { + return new List<HistoryItem>(); + } foreach (var item in items)
24-32: Add null check for robustness.The
queryparameter should be validated to prevent passing null or empty values toApp.API.ChangeQuery().Apply this diff:
public static Func<ActionContext, bool> GetQueryAction(string query) { + if (string.IsNullOrEmpty(query)) + { + return null; + } + return _ =>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Flow.Launcher/MainWindow.xaml.cs(1 hunks)Flow.Launcher/Storage/HistoryHelper.cs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
Flow.Launcher/MainWindow.xaml.cs (1)
Flow.Launcher.Infrastructure/UserSettings/Settings.cs (1)
Settings(15-662)
Flow.Launcher/Storage/HistoryHelper.cs (3)
Flow.Launcher/Storage/History.cs (1)
List(31-40)Flow.Launcher/Storage/HistoryItem.cs (1)
HistoryItem(7-22)Flow.Launcher.Core/Plugin/QueryBuilder.cs (1)
QueryBuilder(7-61)
🪛 GitHub Actions: Check Spelling
Flow.Launcher/MainWindow.xaml.cs
[warning] 29-29: unrecognized-spelling: NKORE
[warning] 119-119: unrecognized-spelling: Wnd
[warning] 377-377: unrecognized-spelling: Wnd
[warning] 590-590: unrecognized-spelling: Wnd
[warning] 592-592: unrecognized-spelling: Wnd
[warning] 816-816: unrecognized-spelling: gamemode
[warning] 817-817: unrecognized-spelling: gamemode
[warning] 820-820: unrecognized-spelling: gamemode
[warning] 823-823: unrecognized-spelling: positionreset
[warning] 826-826: unrecognized-spelling: positionreset
[warning] 842-842: unrecognized-spelling: gamemode
[warning] 843-843: unrecognized-spelling: positionreset
[warning] 848-848: unrecognized-spelling: positionreset
[warning] 966-966: unrecognized-spelling: XRatio
[warning] 967-967: unrecognized-spelling: YRatio
[warning] 1181-1181: unrecognized-spelling: clocksb
[warning] 1182-1182: unrecognized-spelling: clocksb
[warning] 1183-1183: unrecognized-spelling: iconsb
[warning] 1184-1184: unrecognized-spelling: iconsb
[warning] 1189-1189: unrecognized-spelling: clocksb
[warning] 1190-1190: unrecognized-spelling: iconsb
[warning] 28-28: unrecognized-spelling: NKORE
[warning] 70-70: unrecognized-spelling: Wnd
[warning] 235-235: unrecognized-spelling: activing
⏰ 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). (2)
- GitHub Check: gitStream.cm
- GitHub Check: build
🔇 Additional comments (1)
Flow.Launcher/MainWindow.xaml.cs (1)
324-329: LGTM! Property handlers correctly updated for history mode split.The changes properly handle both new history settings (
ShowHistoryQueryResultsForHomePageandShowHistoryLastOpenedResultsForHomePage), triggering the appropriate refresh behavior when either setting changes. This aligns well with the existing pattern forShowHomePageand correctly supports the new mutual-exclusivity behavior implemented in the Settings class.
|
@01Dri Many thanks for your contribution👍Will take a look when I get free time |
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 ...
|
2 participants
First, sorry for so many modifications...
Here are some considerations:
History.cs → PopulateHistoryFromLegacyHistory:
This method exists to replace the current history with the new history. This behavior ensures that users don't lose their previous history.
HistoryHelper.cs → GetExecuteAction:
This method adds the 'Open Action' to the last opened history items, but I believe there’s a better way to do this — I need some help with that.