FEATURE: Calculator Plugin History#4454
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
|
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 calculator history persistence and retrieval, with new storage models, query-flow wiring, history-related settings/UI, localized strings, and reflection-based tests. ChangesCalculator History Feature
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
Plugins/Flow.Launcher.Plugin.Calculator/Main.cs (1)
156-159: 💤 Low valueHistory stores normalized expression, not original user input.
The
expressionvariable passed toHistory.AddOrUpdate(line 158) is the normalized form after number formatting and function rewrites (pow/log/lnworkarounds). This means users who type"1,234+5"will see"1234+5"in their history, and users who type"pow(2,3)"will see"(2^3)".This is likely acceptable for most users, but consider whether showing the original
query.Searchinstead would provide a better experience. The normalized form is what was actually computed, so there's a tradeoff.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Plugins/Flow.Launcher.Plugin.Calculator/Main.cs` around lines 156 - 159, The history entry is being stored using the normalized expression variable (used after number formatting and function rewrites) which loses the original user input; change the argument passed to History.AddOrUpdate so it uses the original query string (query.Search) instead of expression when creating the new History.PendingHistoryItem, or choose to store both by passing query.Search as the display/original value and expression as the computed/normalized value to PendingHistoryItem if that type supports it (update PendingHistoryItem constructor if needed) so history shows what the user typed while still preserving the actual computed expression.Plugins/Flow.Launcher.Plugin.Calculator/Storage/HistoryItem.cs (1)
11-12: ⚡ Quick winRemove
[JsonIgnore]fromconstfield.Constants are not serialized by System.Text.Json (or most serializers) by default. Adding
[JsonIgnore]to aconstfield is misleading because it suggests the field might be serialized otherwise, which is never the case.♻️ Proposed fix
- [JsonIgnore] private const string BadgeIconPath = "Images/history.png";🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Plugins/Flow.Launcher.Plugin.Calculator/Storage/HistoryItem.cs` around lines 11 - 12, Remove the misleading [JsonIgnore] attribute applied to the const field BadgeIconPath in the HistoryItem class: locate the private const string BadgeIconPath = "Images/history.png" declaration and delete the [JsonIgnore] attribute so the const remains attribute-free; no other code changes required.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Flow.Launcher.Test/Plugins/CalculatorTest.cs`:
- Around line 102-110: The test CalculationHistory_IsNotStoredWhenDisabled
checks history immediately after calling _plugin.Query without waiting for the
debounce flush used by the plugin (same as
CalculationHistory_IsStoredWhenEnabled), so update the test to wait for the
debounce to complete or call the same helper that forces a flush used by the
other test before asserting; specifically, after _plugin.Query(new Plugin.Query
{ Search = "1+1" }) add the same wait/flush step used in
CalculationHistory_IsStoredWhenEnabled (e.g., await Task.Delay(debounceMs +
margin) or call the test helper that flushes history) then assert
GetHistory().Items.Count is 0, keeping references to _settings.EnableHistory,
_plugin.Query, GetHistory(), and the other test name for locating the helper.
- Around line 91-100: The test fails to account for History's 800ms debounce so
_plugin.Query(...) is called but GetHistory().Items.Count is checked before the
debounce flush; fix by either waiting for the debounce to elapse (e.g.,
Thread.Sleep(900) or an async wait after calling _plugin.Query) or add a
test-only synchronous flush on History (add an internal FlushNow() in History
that stops the _debounceTimer and calls FlushPendingItem()) and call that from
the test before asserting; reference History.FlushNow, History.FlushPendingItem,
_plugin.Query, and GetHistory() to locate the code to change.
- Around line 163-174: GetHistory() is using reflection for a static field but
Main declares a private instance property History; update the reflection to
retrieve the non-public instance property and read it from the plugin instance
used in SetUp() (e.g. use typeof(Main).GetProperty("History",
BindingFlags.NonPublic | BindingFlags.Instance) and call GetValue(_plugin)),
then assert the property exists and returns a History instance; alternatively,
if static behavior was intended change Main.History to a static field, but the
safer fix in the test is to use GetProperty with BindingFlags.Instance and the
_plugin instance.
In `@Plugins/Flow.Launcher.Plugin.Calculator/Main.cs`:
- Line 31: The History property is declared as an instance property but tests
access it via reflection with BindingFlags.Static; change the declaration of
History in Main.cs to a static property (e.g., private static History History {
get; set; } = null!;) so the reflection in CalculatorTest (which expects a
static member) will find it; ensure any usages of History in methods are updated
to reference the static property if needed.
In `@Plugins/Flow.Launcher.Plugin.Calculator/Storage/History.cs`:
- Around line 72-75: The eviction currently removes Items.RemoveAt(0) which uses
list order; change it to remove the item with the oldest CalculatedAt timestamp
so refreshed entries (via Refresh) aren’t evicted; in History.cs locate the
block checking Items.Count >= MaxItems and replace the RemoveAt(0) call with
logic that finds the item with the minimum CalculatedAt (e.g., via
Items.OrderBy(i => i.CalculatedAt).First() or a manual scan) and removes that
item from Items, ensuring you handle any null/zero timestamps consistently.
---
Nitpick comments:
In `@Plugins/Flow.Launcher.Plugin.Calculator/Main.cs`:
- Around line 156-159: The history entry is being stored using the normalized
expression variable (used after number formatting and function rewrites) which
loses the original user input; change the argument passed to History.AddOrUpdate
so it uses the original query string (query.Search) instead of expression when
creating the new History.PendingHistoryItem, or choose to store both by passing
query.Search as the display/original value and expression as the
computed/normalized value to PendingHistoryItem if that type supports it (update
PendingHistoryItem constructor if needed) so history shows what the user typed
while still preserving the actual computed expression.
In `@Plugins/Flow.Launcher.Plugin.Calculator/Storage/HistoryItem.cs`:
- Around line 11-12: Remove the misleading [JsonIgnore] attribute applied to the
const field BadgeIconPath in the HistoryItem class: locate the private const
string BadgeIconPath = "Images/history.png" declaration and delete the
[JsonIgnore] attribute so the const remains attribute-free; no other code
changes required.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f00f3981-70fd-4e0a-bb48-4b468541be79
⛔ Files ignored due to path filters (1)
Plugins/Flow.Launcher.Plugin.Calculator/Images/history.pngis excluded by!**/*.png
📒 Files selected for processing (7)
Flow.Launcher.Test/Plugins/CalculatorTest.csPlugins/Flow.Launcher.Plugin.Calculator/Languages/en.xamlPlugins/Flow.Launcher.Plugin.Calculator/Main.csPlugins/Flow.Launcher.Plugin.Calculator/Settings.csPlugins/Flow.Launcher.Plugin.Calculator/Storage/History.csPlugins/Flow.Launcher.Plugin.Calculator/Storage/HistoryItem.csPlugins/Flow.Launcher.Plugin.Calculator/Views/CalculatorSettings.xaml
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Plugins/Flow.Launcher.Plugin.Calculator/Storage/History.cs (1)
71-74:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix eviction to remove oldest by timestamp, not insertion order.
When capacity is reached,
RemoveAt(0)evicts based on list position. However, when an existing item is refreshed (line 65), itsCalculatedAtis updated but it remains at the same index. This means a recently refreshed item at index 0 will be evicted even though newer (by timestamp) items exist at higher indices.🐛 Proposed fix to remove oldest by timestamp
if (Items.Count >= MaxItems) { - Items.RemoveAt(0); + var oldestItem = Items.OrderBy(x => x.CalculatedAt).First(); + Items.Remove(oldestItem); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Plugins/Flow.Launcher.Plugin.Calculator/Storage/History.cs` around lines 71 - 74, The eviction currently uses Items.RemoveAt(0) which removes by list position; change it to remove the item with the oldest CalculatedAt timestamp so refreshes aren't evicted; in the eviction block in History (the method updating/adding items that uses Items, MaxItems and updates CalculatedAt) compute the item with the minimum CalculatedAt (e.g., via Items.MinBy(i => i.CalculatedAt) or Items.OrderBy(i => i.CalculatedAt).First()) and call Items.Remove(oldest) instead of RemoveAt(0).
🧹 Nitpick comments (2)
Plugins/Flow.Launcher.Plugin.Calculator/Storage/HistoryItem.cs (2)
22-22: 💤 Low valueConsider extracting magic number to a named constant.
The hardcoded score
300would be more maintainable as a named constant (e.g.,private const int HistoryItemScore = 300) to clarify its purpose and make it easier to adjust if needed.♻️ Proposed fix
+ private const int HistoryItemScore = 300; + public HistoryItem(PendingHistoryItem item) { CalculatedAt = item.CalculatedAt; Title = item.Expression; SubTitle = item.SubTitle; - Score = 300; + Score = HistoryItemScore;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Plugins/Flow.Launcher.Plugin.Calculator/Storage/HistoryItem.cs` at line 22, The hardcoded assignment Score = 300 in HistoryItem should be replaced with a named constant to remove the magic number; add a private const int (e.g., HistoryItemScore = 300) in the HistoryItem class and use that constant when setting the Score property (refer to the Score assignment in HistoryItem and the class declaration to locate the change).
11-11: 💤 Low valueRemove redundant [JsonIgnore] attribute.
The
[JsonIgnore]attribute on aprivate constfield is redundant. Private constants are not serialized by System.Text.Json by default, and const fields are implicitly static (which are also excluded from serialization).♻️ Proposed fix
- [JsonIgnore] private const string BadgeIconPath = "Images/history.png"; + private const string BadgeIconPath = "Images/history.png";🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Plugins/Flow.Launcher.Plugin.Calculator/Storage/HistoryItem.cs` at line 11, Remove the redundant [JsonIgnore] attribute applied to the private const field BadgeIconPath in HistoryItem (the line "[JsonIgnore] private const string BadgeIconPath = \"Images/history.png\""); simply delete the [JsonIgnore] token so the field is declared as "private const string BadgeIconPath = \"Images/history.png\"". If that was the only usage of System.Text.Json.Serialization in the file, also remove the now-unused using for cleanliness.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@Plugins/Flow.Launcher.Plugin.Calculator/Storage/History.cs`:
- Around line 71-74: The eviction currently uses Items.RemoveAt(0) which removes
by list position; change it to remove the item with the oldest CalculatedAt
timestamp so refreshes aren't evicted; in the eviction block in History (the
method updating/adding items that uses Items, MaxItems and updates CalculatedAt)
compute the item with the minimum CalculatedAt (e.g., via Items.MinBy(i =>
i.CalculatedAt) or Items.OrderBy(i => i.CalculatedAt).First()) and call
Items.Remove(oldest) instead of RemoveAt(0).
---
Nitpick comments:
In `@Plugins/Flow.Launcher.Plugin.Calculator/Storage/HistoryItem.cs`:
- Line 22: The hardcoded assignment Score = 300 in HistoryItem should be
replaced with a named constant to remove the magic number; add a private const
int (e.g., HistoryItemScore = 300) in the HistoryItem class and use that
constant when setting the Score property (refer to the Score assignment in
HistoryItem and the class declaration to locate the change).
- Line 11: Remove the redundant [JsonIgnore] attribute applied to the private
const field BadgeIconPath in HistoryItem (the line "[JsonIgnore] private const
string BadgeIconPath = \"Images/history.png\""); simply delete the [JsonIgnore]
token so the field is declared as "private const string BadgeIconPath =
\"Images/history.png\"". If that was the only usage of
System.Text.Json.Serialization in the file, also remove the now-unused using for
cleanliness.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 16e2d2a3-7a8a-46ab-a47a-40f9fe95bcb0
📒 Files selected for processing (5)
Flow.Launcher.Test/Plugins/CalculatorTest.csPlugins/Flow.Launcher.Plugin.Calculator/Main.csPlugins/Flow.Launcher.Plugin.Calculator/Storage/History.csPlugins/Flow.Launcher.Plugin.Calculator/Storage/HistoryItem.csPlugins/Flow.Launcher.Plugin.Calculator/Storage/PendingHistoryItem.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- Plugins/Flow.Launcher.Plugin.Calculator/Main.cs
There was a problem hiding this comment.
Currently my result icons aren't getting that clock (history.png) layered on top of them like yours are in the screenshots provded - not sure why
My mistake I didn't realise I needed the results badges to be enabled in the settings
One design concern is that if the user is searching for a numerical query, e.g. a date,
then there would be a lot of history results in the way of what they actually want.
Its a difficult problem to solve since as a query a date and a calculation can look identical
It's a little confusing that the title is not whats being copied to the clipboard
Perhaps it could be query = result?
Another approach that you could consider is switching the current query to that historical query, making it work more like a rewind button (that's what I was used to on my old physical calculator).
They can still use the existing result to copy it after switching - a context menu copy is also an option but perhaps unnecessary.
Instead of a timestamp, I think a time delta would be more helpful.
No need for the Calculated at part either I think, too verbose.
e.g. 2 minutes ago
There was a problem hiding this comment.
I can confirm this is from icons8 which we already give attribution to so is safe to use
https://icons8.com/icon/QDgOnr6UAOmg/time-machine
My concern is that at a small scale its not too readable
I found another alternative, which might be slightly more clear - though still not ideal
https://icons8.com/icons/set/history--os-windows--multi--technique-filled
There was a problem hiding this comment.
4 issues found across 11 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Plugins/Flow.Launcher.Plugin.Calculator/Storage/History.cs (1)
113-122: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winEviction removes the newest item instead of the oldest.
OrderByDescending(x => x.CalculatedAt)sorts newest-first, soItems[0]after this sort is the most recently calculated item.RemoveAt(0)therefore evicts the newest entry and keeps the oldest ones — the opposite of the intended "cap atMaxItemsby dropping the oldest" behavior discussed in the prior review thread.🐛 Proposed fix
private void Add(HistoryItem item) { if (Items.Count >= MaxItems) { - - Items = Items.OrderByDescending(x => x.CalculatedAt).ToList(); - Items.RemoveAt(0); + var oldestItem = Items.OrderBy(x => x.CalculatedAt).First(); + Items.Remove(oldestItem); } Items.Add(item); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Plugins/Flow.Launcher.Plugin.Calculator/Storage/History.cs` around lines 113 - 122, The Add method in History is evicting the wrong entry because it sorts newest-first with OrderByDescending(x => x.CalculatedAt) and then removes index 0, which drops the newest item instead of the oldest. Update the eviction logic in Add so it identifies and removes the oldest HistoryItem by CalculatedAt before appending the new item, while keeping the MaxItems cap behavior intact.
🧹 Nitpick comments (1)
Plugins/Flow.Launcher.Plugin.Calculator/Views/CalculatorSettings.xaml (1)
100-107: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDisabled
TextBlockwon't visually dim like the adjacentComboBox.
TextBlockis not aControl, so it has no built-in template trigger that reacts toIsEnabled(unlike theComboBoxat Line 108-120, which will visibly dim via the default theme when disabled). SettingIsEnabled="{Binding EnableHistory}"here will disable interaction/hit-testing but the label text likely won't appear grayed out, leaving an inconsistent visual cue when history is disabled.💡 Suggested fix: bind Opacity in addition to IsEnabled
<TextBlock Grid.Row="5" Grid.Column="0" Margin="{StaticResource SettingPanelItemRightTopBottomMargin}" VerticalAlignment="Center" FontSize="14" Text="{DynamicResource flowlauncher_plugin_calculator_history_creation_mode}" - IsEnabled="{Binding EnableHistory}" /> + IsEnabled="{Binding EnableHistory}" + Opacity="{Binding EnableHistory, Converter={StaticResource BoolToOpacityConverter}}" />(Requires a
BoolToOpacityConverteror equivalent, or apply aStyle.TriggeronIsEnabledforTextBlock.)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Plugins/Flow.Launcher.Plugin.Calculator/Views/CalculatorSettings.xaml` around lines 100 - 107, The history label TextBlock in CalculatorSettings.xaml is being disabled via IsEnabled, but because TextBlock is not a Control it will not visually dim like the adjacent ComboBox. Update the label binding so the text also reflects the disabled state, either by adding an Opacity binding with a BoolToOpacityConverter or by applying a Style trigger on TextBlock tied to EnableHistory, and keep the existing IsEnabled binding for behavior consistency.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@Plugins/Flow.Launcher.Plugin.Calculator/Storage/History.cs`:
- Around line 113-122: The Add method in History is evicting the wrong entry
because it sorts newest-first with OrderByDescending(x => x.CalculatedAt) and
then removes index 0, which drops the newest item instead of the oldest. Update
the eviction logic in Add so it identifies and removes the oldest HistoryItem by
CalculatedAt before appending the new item, while keeping the MaxItems cap
behavior intact.
---
Nitpick comments:
In `@Plugins/Flow.Launcher.Plugin.Calculator/Views/CalculatorSettings.xaml`:
- Around line 100-107: The history label TextBlock in CalculatorSettings.xaml is
being disabled via IsEnabled, but because TextBlock is not a Control it will not
visually dim like the adjacent ComboBox. Update the label binding so the text
also reflects the disabled state, either by adding an Opacity binding with a
BoolToOpacityConverter or by applying a Style trigger on TextBlock tied to
EnableHistory, and keep the existing IsEnabled binding for behavior consistency.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f0b96d0c-bf32-42eb-9084-07fc70ac4ad8
📒 Files selected for processing (11)
Plugins/Flow.Launcher.Plugin.Calculator/HistoryCreationMode.csPlugins/Flow.Launcher.Plugin.Calculator/HistoryHelper.csPlugins/Flow.Launcher.Plugin.Calculator/Languages/en.xamlPlugins/Flow.Launcher.Plugin.Calculator/Languages/pt-br.xamlPlugins/Flow.Launcher.Plugin.Calculator/Main.csPlugins/Flow.Launcher.Plugin.Calculator/Settings.csPlugins/Flow.Launcher.Plugin.Calculator/Storage/History.csPlugins/Flow.Launcher.Plugin.Calculator/Storage/HistoryItem.csPlugins/Flow.Launcher.Plugin.Calculator/Storage/PendingHistoryItem.csPlugins/Flow.Launcher.Plugin.Calculator/ViewModels/SettingsViewModel.csPlugins/Flow.Launcher.Plugin.Calculator/Views/CalculatorSettings.xaml
✅ Files skipped from review due to trivial changes (1)
- Plugins/Flow.Launcher.Plugin.Calculator/Languages/pt-br.xaml
🚧 Files skipped from review as they are similar to previous changes (3)
- Plugins/Flow.Launcher.Plugin.Calculator/Storage/PendingHistoryItem.cs
- Plugins/Flow.Launcher.Plugin.Calculator/Settings.cs
- Plugins/Flow.Launcher.Plugin.Calculator/Main.cs
…i/Flow.Launcher into feature/history-calc-plugin
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
Plugins/Flow.Launcher.Plugin.Calculator/Main.cs (3)
123-137: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winNaN/
Functionpaths still fall through toConvert.ToDecimal
AtPlugins/Flow.Launcher.Plugin.Calculator/Main.cs:123-137, both branches replaceresultwith a localized string, but execution still continues intoConvert.ToDecimal(result). That throws on the non-numeric text, so the message never surfaces.isValidResultToAddHistoryis also never read, so the intended history guard is ineffective. Return those cases early (or skip the numeric formatting block) and remove the dead flag.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Plugins/Flow.Launcher.Plugin.Calculator/Main.cs` around lines 123 - 137, The NaN and Function handling in Main.cs still continues into the numeric formatting path, causing Convert.ToDecimal to run on localized strings. Update the result-processing logic in the Main method to return early or otherwise skip the rounding/formatting block when result is NaN or a Function, using the existing Localize.flowlauncher_plugin_calculator_not_a_number and Localize.flowlauncher_plugin_calculator_expression_not_complete branches. Remove the unused isValidResultToAddHistory flag if it is no longer needed, since it currently has no effect.
155-162: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKeep the original query in history entries
HistoryItemis built from the normalizedexpression, so history shows(log10(100))instead of the user'slog(100). Storequery.SearchforQuery/Titleand keep the rewritten string only for evaluation/dedupe.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Plugins/Flow.Launcher.Plugin.Calculator/Main.cs` around lines 155 - 162, The history entry is using the normalized expression instead of the user’s original query, so update the history creation path in Main.cs to preserve query.Search for HistoryItem Query and Title while still using the rewritten expression only for evaluation and deduplication. In the flow around HistoryHelper.CreatePendingHistoryItem and History.AddOrUpdate, make sure the stored history text reflects the original user input, and keep the normalized expression only for computation-related logic.
41-41: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRestore the history action on reload. Persisted
HistoryItems lose theirAction, so restored calculator history entries become non-clickable after restart. Rebuild the delegate when materializing history items instead of copying it through storage.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Plugins/Flow.Launcher.Plugin.Calculator/Main.cs` at line 41, The calculator history reload path is restoring HistoryItem data without rebuilding its Action, so loaded entries become non-clickable after restart. Update the history materialization logic in Main and the History/HistoryItem handling so the delegate is recreated when loading persisted items instead of being copied from storage. Use the History and HistoryItem symbols to locate the restore flow and ensure each restored item gets a fresh clickable action.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@Plugins/Flow.Launcher.Plugin.Calculator/Main.cs`:
- Around line 123-137: The NaN and Function handling in Main.cs still continues
into the numeric formatting path, causing Convert.ToDecimal to run on localized
strings. Update the result-processing logic in the Main method to return early
or otherwise skip the rounding/formatting block when result is NaN or a
Function, using the existing
Localize.flowlauncher_plugin_calculator_not_a_number and
Localize.flowlauncher_plugin_calculator_expression_not_complete branches. Remove
the unused isValidResultToAddHistory flag if it is no longer needed, since it
currently has no effect.
- Around line 155-162: The history entry is using the normalized expression
instead of the user’s original query, so update the history creation path in
Main.cs to preserve query.Search for HistoryItem Query and Title while still
using the rewritten expression only for evaluation and deduplication. In the
flow around HistoryHelper.CreatePendingHistoryItem and History.AddOrUpdate, make
sure the stored history text reflects the original user input, and keep the
normalized expression only for computation-related logic.
- Line 41: The calculator history reload path is restoring HistoryItem data
without rebuilding its Action, so loaded entries become non-clickable after
restart. Update the history materialization logic in Main and the
History/HistoryItem handling so the delegate is recreated when loading persisted
items instead of being copied from storage. Use the History and HistoryItem
symbols to locate the restore flow and ensure each restored item gets a fresh
clickable action.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 63fad677-58d4-4ccb-bbde-8c4fc9e72923
📒 Files selected for processing (2)
Plugins/Flow.Launcher.Plugin.Calculator/Main.csPlugins/Flow.Launcher.Plugin.Calculator/Storage/HistoryItem.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- Plugins/Flow.Launcher.Plugin.Calculator/Storage/HistoryItem.cs
|
I made some adjustments. Could you test it again and let me know if everything looks good? Thanks! |
Jack251970
left a comment
Jack251970
left a comment
There was a problem hiding this comment.
Please revert changes in pt-br.xaml.
There was a problem hiding this comment.
Hi, could you please revert changes in this file?
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a74587366d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
4 participants
Changes
HistoryandHistoryItemobjects to persist calculator resultsDebounce Strategy
Currently, the calculator plugin returns results even when the Enter key is not pressed. Because of this behavior, every partial expression can be added to the history while the user is typing.
For example, while the user is building the expression:
Without a debounce mechanism, each intermediate expression is stored in history, generating multiple unnecessary entries.
With the debounce strategy, the plugin waits for a short delay before saving the result. This ensures that only the final or more complete expression is added to history.
Without debounce
11+21+2+31+2+3+41+2+3+4+5With debounce
1+2+3+4+5This keeps the history cleaner and improves the overall user experience.
Note
I'm not fully satisfied with the result title and subtitles yet, so suggestions are welcome!
Technical Implementation Details
Time-Ago Formatting
We implemented custom, localized relative time strings (e.g., "just now", "5 minutes ago"). This was needed because there is no built-in datetime formatting helper in the Flow Launcher codebase. The implementation uses the plugin's own local translation keys (in
en.xaml) to keep everything localized correctly.Debounce vs Immediate Modes
PendingHistoryItemtogether with an 800ms debounce timer to prevent saving incomplete expressions while the user is typing.HistoryItemwithout any debounce.Code Quality & Refactorings
HistoryItem.csso that thePendingHistoryItemconstructor delegates to the primary one, avoiding initialization duplication.CreateClipboardActionWithHistoryinMain.csto delegate directly toCreateClipboardAction, avoiding duplicated copy and error handling logic.AddOrUpdateInternal(HistoryItem)inHistory.csto standardize the update path for both modes.lock (_syncRoot)) on the normalHistoryItemaddition path since it is triggered from the main thread.2026-07-04.19-49-34.mp4
Summary by cubic
Adds an optional calculation history to the calculator plugin, showing up to 5 recent calculations beneath the current result. Supports debounced save-on-query and save-on-enter modes with relative time and a history badge.
Summary of changes
Querynow returns the main result plus recent history whenEnableHistoryis on, excluding the active expression. Main result uses a clear “copy to clipboard” subtitle. Clipboard is handled via shared actions; inOnEntermode history saves only after a successful copy. History items render with a badge and a relative “time ago.”EnableHistory(default off) andHistoryCreationMode(OnQuerywith 800ms debounce,OnEnter). Storage-backedHistorywith a 5-item cap (drops oldest).HistoryItem,PendingHistoryItem, andHistoryHelperfor pending items and localized time-ago strings. History icon, English strings, and settings UI with mode selector and anOnQuerywarning. Unit tests for storing when enabled, suppressing when disabled, and saved fields; tests flush debounce and reset history.Queryin favor of centralized helpers.OnQuerymay capture partial expressions; UI warns; nothing is sent externally.Release Note
Written for commit a7efe5d. Summary will update on new commits.