Repository navigation
fix(savings-history): correct KPI units ($/mo) + Hourly/Monthly/Yearly toggle - #758
Merged
cristim merged 1 commit intoMay 27, 2026
Merged
Conversation
…arly toggle Peak Savings showed /hr while the underlying value was monthly, making "$1.15/hr" appear as ~$840/mo when it was actually ~$1.15/mo. Fixed the label and added a unit dropdown that converts all three KPIs (Period / Avg / Peak) consistently. Closes #750.
cristim
added
triaged
Contributor
📝 WalkthroughWalkthroughThis PR adds a unit selector dropdown to the Savings History widget, allowing users to view KPI totals (Period, Avg, Peak) in hourly, monthly, or yearly formats. The changes introduce unit conversion helpers, update markup to include the selector, modify stats and chart rendering to accept and apply the selected unit, wire dropdown changes to reload the chart, and extend test coverage to validate the new behavior. ChangesAdd unit selector and conversion to Savings History
🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Member
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
![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: 1
🧹 Nitpick comments (1)
frontend/src/__tests__/savings-history.test.ts (1)
1259-1259: ⚡ Quick winReplace fixed 50ms sleeps with a deterministic async flush.
Using hardcoded
setTimeout(..., 50)makes tests slower and can still be timing-sensitive. Prefer a reusable next-tick flush helper.♻️ Proposed refactor
+const flushAsync = () => new Promise((resolve) => setTimeout(resolve, 0)); + ... - await new Promise(resolve => setTimeout(resolve, 50)); + await flushAsync(); expect(getSavingsAnalytics).toHaveBeenCalled(); ... - await new Promise(resolve => setTimeout(resolve, 50)); + await flushAsync(); // Should fire exactly once (no stacked duplicate listeners) expect(getSavingsAnalytics).toHaveBeenCalledTimes(1);Also applies to: 1273-1273
🤖 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 `@frontend/src/__tests__/savings-history.test.ts` at line 1259, Replace the brittle setTimeout-based waits in the test (the lines using await new Promise(resolve => setTimeout(resolve, 50))) with a deterministic microtask flush helper (e.g., add or import a flushPromises / waitForNextTick function that returns Promise.resolve().then(() => setImmediate-like microtask processing) or uses process.nextTick/queueMicrotask) and call that helper instead of the 50ms sleep in savings-history.test.ts (both occurrences around the current lines referencing the 50ms sleep). Ensure you add the helper to the test utilities or import it into the test file and replace both instances so tests no longer rely on timing sleeps.
🤖 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 `@frontend/src/modules/savings-history.ts`:
- Around line 289-293: The Period Savings rendering uses
periodSavingsEl.textContent = formatCurrency(displayTotal) which omits the unit
suffix; update this to append the same unit string used for Avg/Peak so KPI
units are consistent (e.g., formatCurrency(displayTotal) + ' ' + unitSuffix).
Locate where Avg/Peak derive their suffix (the rate/unit variable used elsewhere
in savings-history.ts) and reuse that variable (instead of hardcoding) when
setting periodSavingsEl.textContent so the display reflects
Hourly/Monthly/Yearly selections.
---
Nitpick comments:
In `@frontend/src/__tests__/savings-history.test.ts`:
- Line 1259: Replace the brittle setTimeout-based waits in the test (the lines
using await new Promise(resolve => setTimeout(resolve, 50))) with a
deterministic microtask flush helper (e.g., add or import a flushPromises /
waitForNextTick function that returns Promise.resolve().then(() =>
setImmediate-like microtask processing) or uses process.nextTick/queueMicrotask)
and call that helper instead of the 50ms sleep in savings-history.test.ts (both
occurrences around the current lines referencing the 50ms sleep). Ensure you add
the helper to the test utilities or import it into the test file and replace
both instances so tests no longer rely on timing sleeps.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5e2ac99b-fe63-447a-9ae5-d2aa56957d0c
📒 Files selected for processing (3)
frontend/src/__tests__/savings-history.test.tsfrontend/src/index.htmlfrontend/src/modules/savings-history.ts
Comment on lines
289
to
293
| if (periodSavingsEl) { | ||
| // Period Savings is the cumulative total over the selected date range | ||
| // (no per-unit rate suffix -- it is already a dollar total). | ||
| periodSavingsEl.textContent = formatCurrency(displayTotal); | ||
| } |
Contributor
There was a problem hiding this comment.
Add unit suffix to Period Savings to keep KPI units consistent.
Line 292 renders a converted value (displayTotal) without any unit suffix. Since this number changes when Hourly/Monthly/Yearly is selected, the KPI becomes ambiguous and inconsistent with Avg/Peak.
💡 Suggested fix
if (periodSavingsEl) {
// Period Savings is the cumulative total over the selected date range
- // (no per-unit rate suffix -- it is already a dollar total).
- periodSavingsEl.textContent = formatCurrency(displayTotal);
+ periodSavingsEl.textContent = `${formatCurrency(displayTotal)}${suffix}`;
}🤖 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 `@frontend/src/modules/savings-history.ts` around lines 289 - 293, The Period
Savings rendering uses periodSavingsEl.textContent =
formatCurrency(displayTotal) which omits the unit suffix; update this to append
the same unit string used for Avg/Peak so KPI units are consistent (e.g.,
formatCurrency(displayTotal) + ' ' + unitSuffix). Locate where Avg/Peak derive
their suffix (the rate/unit variable used elsewhere in savings-history.ts) and
reuse that variable (instead of hardcoding) when setting
periodSavingsEl.textContent so the display reflects Hourly/Monthly/Yearly
selections.
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 1 file(s) based on 1 unresolved review comment. A stacked PR containing fixes has been created.
Time taken: |
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.
Summary
Savings History on the Purchases page showed wrong unit labels and lacked a unit toggle:
$1.15(no unit)$1.15/hr$1.15/hrThe underlying values are actually monthly (~$1.15/mo for the single t4g.nano example).
$1.15/hrwould imply ~$840/mo -- clearly wrong.Root cause
renderSavingsStatsblindly appended/hrto all three KPIs, even though every field in the API response is monthly (total_savings,total_period_savings,average_savings_per_period,peak_savings).Fix
/mo(matches API).#savings-unitdropdown (Hourly / Monthly / Yearly).SavingsUnit,convertFromMonthly,unitSuffix,unitLabel,getSelectedUnit.renderSavingsStats+renderSavingsChartconvert and label per the selected unit.feedback_event_listener_dedup).Files changed
frontend/src/modules/savings-history.tsfrontend/src/index.htmlfrontend/src/__tests__/savings-history.test.tsTest plan
/hrassertions updated to/mo; 14 new tests cover unit helpers, each toggle, heading updates, dedup, and chart tooltip.Closes #750.
Summary by CodeRabbit
Release Notes