FEATURE: Calculator Plugin History#4454
Open
01Dri wants to merge 9 commits into
Open
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
3 issues found across 8 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.Test/Plugins/CalculatorTest.cs">
<violation number="1" location="Flow.Launcher.Test/Plugins/CalculatorTest.cs:98">
P2: History persistence is debounced, so asserting `Items.Count` immediately after `Query` is timing-flaky and may fail before the timer flushes the pending item.</violation>
</file>
<file name="Plugins/Flow.Launcher.Plugin.Calculator/Main.cs">
<violation number="1" location="Plugins/Flow.Launcher.Plugin.Calculator/Main.cs:156">
P2: History entries are persisted even when history is disabled in settings.</violation>
</file>
<file name="Plugins/Flow.Launcher.Plugin.Calculator/Storage/History.cs">
<violation number="1" location="Plugins/Flow.Launcher.Plugin.Calculator/Storage/History.cs:72">
P2: Capacity eviction removes the wrong history item because list order is not kept aligned with recency. `Items.RemoveAt(0)` assumes FIFO ordering, but `Refresh` updates `CalculatedAt` without repositioning the item, so a recently-refreshed item at index 0 can be incorrectly evicted when capacity is reached.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| }; | ||
|
|
||
|
|
||
| if (isValidResultToAddHistory) |
Contributor
There was a problem hiding this comment.
P2: History entries are persisted even when history is disabled in settings.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Plugins/Flow.Launcher.Plugin.Calculator/Main.cs, line 156:
<comment>History entries are persisted even when history is disabled in settings.</comment>
<file context>
@@ -119,43 +123,47 @@ public List<Result> Query(Query query)
+ };
+
+
+ if (isValidResultToAddHistory)
+ {
+ History.AddOrUpdate(new History.PendingHistoryItem(resultObject, expression, action));
</file context>
Suggested change
| if (isValidResultToAddHistory) | |
| if (isValidResultToAddHistory && _settings.EnableHistory) |
| { | ||
| var item = new HistoryItem(_pendingItem.Result, _pendingItem.Expression, _pendingItem.Action); | ||
|
|
||
| if (Items.Count >= MaxItems) |
Contributor
There was a problem hiding this comment.
P2: Capacity eviction removes the wrong history item because list order is not kept aligned with recency. Items.RemoveAt(0) assumes FIFO ordering, but Refresh updates CalculatedAt without repositioning the item, so a recently-refreshed item at index 0 can be incorrectly evicted when capacity is reached.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Plugins/Flow.Launcher.Plugin.Calculator/Storage/History.cs, line 72:
<comment>Capacity eviction removes the wrong history item because list order is not kept aligned with recency. `Items.RemoveAt(0)` assumes FIFO ordering, but `Refresh` updates `CalculatedAt` without repositioning the item, so a recently-refreshed item at index 0 can be incorrectly evicted when capacity is reached.</comment>
<file context>
@@ -0,0 +1,85 @@
+ {
+ var item = new HistoryItem(_pendingItem.Result, _pendingItem.Expression, _pendingItem.Action);
+
+ if (Items.Count >= MaxItems)
+ {
+ Items.RemoveAt(0);
</file context>
Contributor
📝 WalkthroughWalkthroughAdds a debounced, capped calculation history to the Calculator plugin: new storage models (History, HistoryItem, PendingHistoryItem), settings toggle (EnableHistory), UI checkbox and localization, Main.cs wiring to load/persist history and conditionally record results, and unit tests accessing history via reflection. ChangesCalculator History Feature
Sequence Diagram(s)sequenceDiagram
participant User as User Input
participant Plugin as Calculator.Main.Query
participant Eval as Calculator Engine
participant Validity as Validity Check
participant History as History
participant Storage as Settings JSON
User->>Plugin: submits expression
Plugin->>Eval: parse & evaluate
Eval->>Validity: return type/result
Validity->>Plugin: isValidResultToAddHistory
Plugin->>History: AddOrUpdate(PendingHistoryItem) [if valid]
Plugin->>History: GetItemsExcluding(expression) [if enabled]
Plugin->>User: return current result + history items
History->>Storage: persisted in plugin settings (FlushPendingItem)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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}
Contributor
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
Comment on lines
+72
to
+75
| if (Items.Count >= MaxItems) | ||
| { | ||
| Items.RemoveAt(0); | ||
| } |
Contributor
There was a problem hiding this comment.
Fix removal to evict oldest by timestamp, not insertion order.
When the history reaches capacity, RemoveAt(0) removes the first item in the internal list, which is based on insertion order. However, if an existing item is refreshed via Refresh (line 66), its CalculatedAt timestamp is updated but it remains at the same list position. This means a recently refreshed item at index 0 could be evicted even though it's the newest by timestamp, while older (by timestamp) items at higher indices are retained.
🐛 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);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (Items.Count >= MaxItems) | |
| { | |
| Items.RemoveAt(0); | |
| } | |
| if (Items.Count >= MaxItems) | |
| { | |
| 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 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.
Contributor
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.
Summary by cubic
Adds an optional calculation history to the calculator with an 800ms debounce and a 5-item cap. Shows recent calculations below the current result when enabled and improves copy-to-clipboard handling.
Summary of changes
EnableHistoryis on, excluding the active expression. NaN and incomplete expressions are skipped. Clipboard copy is now handled by a reusable helper.Historystore with 800ms debounce and 5-item limit;HistoryItemwith badge icon, timestamp, and subtitle;EnableHistorysetting (off by default); new localized strings; history icon asset. Tests cover history on/off behavior and use a deterministic debounce flush.Query(now centralized).Release Note
Written for commit 6d93ce0. Summary will update on new commits.