feat(frontend/recs): cost-period selector (hourly/daily/monthly/yearly)#328
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR adds a cost-period selector (hourly/daily/monthly/yearly) to the recommendations UI, introduces period-aware scaling/formatting helpers, updates headers/columns/summary/fan-out to display scaled values, and persists the selected period in localStorage. Changes are presentation-only; no backend API changes. ChangesCost-Period Scaling & UI Integration
Sequence DiagramsequenceDiagram
actor User
participant UI as Recommendations UI
participant State as state.ts
participant Store as localStorage
participant Render as Rendering Engine
User->>UI: Select period from dropdown (Hourly/Daily/Monthly/Yearly)
UI->>State: setCostPeriod(selectedPeriod)
State->>Store: Persist period to localStorage (cudly.recs.costPeriod)
Store-->>State: ✓ Stored
State-->>UI: Period updated in memory
UI->>Render: Trigger re-render with new period
Render->>Render: Retrieve getCostPeriod()
Render->>Render: scaleCost(monthly, period) for each cost column
Render->>Render: getColumnLabel(column, period) for each header
Render->>Render: formatCostForPeriod(..., period) for display text
Render-->>UI: Updated DOM with scaled values & period-aware labels
UI-->>User: Display costs in selected period
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 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)
Comment |
cristim
added
priority/p2
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
frontend/src/__tests__/recommendations.test.ts (3)
4304-4311: ⚡ Quick winActually assert the dropdown rerender.
Line 4304's test name says the change event rerenders, but the assertion only checks
setCostPeriod. If the handler stops rebuilding the headers/cells, this still passes.Suggested assertion upgrade
test('changing the dropdown calls setCostPeriod and triggers rerender', async () => { - await loadRecommendations(); + (state.getCostPeriod as jest.Mock) + .mockReturnValueOnce('monthly') + .mockReturnValue('hourly'); + await loadRecommendations(); const select = document.querySelector<HTMLSelectElement>('#cost-period-select'); expect(select).not.toBeNull(); select!.value = 'hourly'; select!.dispatchEvent(new Event('change')); expect(state.setCostPeriod).toHaveBeenCalledWith('hourly'); + expect(document.getElementById('recommendations-list')?.textContent).toContain('Savings / hr'); });🤖 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__/recommendations.test.ts` around lines 4304 - 4311, The test only asserts that state.setCostPeriod was called but doesn’t verify the UI rerender; after triggering the change on '#cost-period-select' in the test (which calls state.setCostPeriod via loadRecommendations), wait for the DOM update and assert that a visible element updated accordingly (for example, check a header or a cost cell in the recommendations table changed to the hourly label/value): use the same loadRecommendations/test helpers to await the rerender and then query the DOM (e.g., table header text or a specific cell) to assert it reflects the 'hourly' period, ensuring the handler actually rebuilt the headers/cells rather than just calling setCostPeriod.
4376-4378: ⚡ Quick winReproduce the stale in-memory fallback bug in this test.
This case never sets a prior non-default period, so it would still pass if
getCostPeriod()incorrectly leaked the last in-memory value. Seed a non-monthly value first to cover the bug called out in the PR.Suggested fix
- test('invalid value in localStorage falls back to in-memory default (monthly)', () => { + test('invalid value in localStorage falls back to static default, not prior in-memory state', () => { + setCostPeriodFn('hourly'); localStorage.setItem('cudly.recs.costPeriod', 'invalid-value'); expect(getCostPeriodFn()).toBe('monthly'); });🤖 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__/recommendations.test.ts` around lines 4376 - 4378, Seed a non-default cost period before exercising the invalid-localStorage case so the test fails if getCostPeriodFn() incorrectly uses a stale in-memory value: in the test "invalid value in localStorage falls back to in-memory default (monthly)" first set localStorage.setItem('cudly.recs.costPeriod', 'yearly') and call getCostPeriodFn() (to populate any in-memory cache), then set localStorage.setItem('cudly.recs.costPeriod', 'invalid-value') and finally assert expect(getCostPeriodFn()).toBe('monthly').
3913-4380: ⚡ Quick winAdd at least one non-monthly sort/filter assertion.
The new Issue
#319cases verify labels and formatted values, but they never prove that sorting and numeric filtering use the scaled value. A regression that only changes presentation would still pass this block.Suggested coverage to add
+ test('hourly numeric filter uses scaled savings, not raw monthly savings', async () => { + const recs = [ + { ...mockRec, id: 'r-low', savings: 360, monthly_cost: 360 }, + { ...mockRec, id: 'r-high', savings: 720, monthly_cost: 720 }, + ]; + (state.getCostPeriod as jest.Mock).mockReturnValue('hourly'); + (api.getRecommendations as jest.Mock).mockResolvedValue({ summary: {}, recommendations: recs }); + (state.getRecommendations as jest.Mock).mockReturnValue(recs); + (state.getRecommendationsColumnFilters as jest.Mock).mockReturnValue({ + savings: { kind: 'expr', expr: '>0.75' }, // hourly: 0.5 vs 1.0 + }); + + await loadRecommendations(); + + const rows = document.querySelectorAll('tr.recommendation-row'); + expect(rows).toHaveLength(1); + expect(rows[0]?.textContent).toContain('r-high'); + }); + + test('yearly sorting orders by yearly-scaled savings', async () => { + const recs = [ + { ...mockRec, id: 'r-small', savings: 100, monthly_cost: 100 }, + { ...mockRec, id: 'r-large', savings: 200, monthly_cost: 200 }, + ]; + (state.getCostPeriod as jest.Mock).mockReturnValue('yearly'); + (state.getRecommendationsSort as jest.Mock).mockReturnValue({ column: 'savings', direction: 'asc' }); + (api.getRecommendations as jest.Mock).mockResolvedValue({ summary: {}, recommendations: recs }); + (state.getRecommendations as jest.Mock).mockReturnValue(recs); + + await loadRecommendations(); + + const rows = Array.from(document.querySelectorAll('tr.recommendation-row')); + expect(rows[0]?.textContent).toContain('r-small'); + expect(rows[1]?.textContent).toContain('r-large'); + });🤖 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__/recommendations.test.ts` around lines 3913 - 4380, Add a test that proves sorting/filtering uses scaled values by mocking two recommendations with different monthly savings (e.g., recA monthly_cost/savings 720 and recB 1440), set state.getCostPeriod to a non-monthly period like 'hourly', mock api.getRecommendations/state.getRecommendations to return those two, call loadRecommendations(), then inspect the DOM rows (querySelectorAll('tr.recommendation-row') and their '.savings' cells) to assert the order matches the scaled hourly values (recB should appear after scaling as double recA) or apply a numeric filter threshold and assert only the correctly scaled row remains; reference loadRecommendations, state.getCostPeriod, api.getRecommendations, tr.recommendation-row and .savings in the test.
🤖 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.
Nitpick comments:
In `@frontend/src/__tests__/recommendations.test.ts`:
- Around line 4304-4311: The test only asserts that state.setCostPeriod was
called but doesn’t verify the UI rerender; after triggering the change on
'#cost-period-select' in the test (which calls state.setCostPeriod via
loadRecommendations), wait for the DOM update and assert that a visible element
updated accordingly (for example, check a header or a cost cell in the
recommendations table changed to the hourly label/value): use the same
loadRecommendations/test helpers to await the rerender and then query the DOM
(e.g., table header text or a specific cell) to assert it reflects the 'hourly'
period, ensuring the handler actually rebuilt the headers/cells rather than just
calling setCostPeriod.
- Around line 4376-4378: Seed a non-default cost period before exercising the
invalid-localStorage case so the test fails if getCostPeriodFn() incorrectly
uses a stale in-memory value: in the test "invalid value in localStorage falls
back to in-memory default (monthly)" first set
localStorage.setItem('cudly.recs.costPeriod', 'yearly') and call
getCostPeriodFn() (to populate any in-memory cache), then set
localStorage.setItem('cudly.recs.costPeriod', 'invalid-value') and finally
assert expect(getCostPeriodFn()).toBe('monthly').
- Around line 3913-4380: Add a test that proves sorting/filtering uses scaled
values by mocking two recommendations with different monthly savings (e.g., recA
monthly_cost/savings 720 and recB 1440), set state.getCostPeriod to a
non-monthly period like 'hourly', mock
api.getRecommendations/state.getRecommendations to return those two, call
loadRecommendations(), then inspect the DOM rows
(querySelectorAll('tr.recommendation-row') and their '.savings' cells) to assert
the order matches the scaled hourly values (recB should appear after scaling as
double recA) or apply a numeric filter threshold and assert only the correctly
scaled row remains; reference loadRecommendations, state.getCostPeriod,
api.getRecommendations, tr.recommendation-row and .savings in the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 06b34e69-dc3b-4498-9474-199eeffe54cf
📒 Files selected for processing (3)
frontend/src/__tests__/recommendations.test.tsfrontend/src/recommendations.tsfrontend/src/state.ts
Adds a "Show costs" dropdown in the recs toolbar letting users view all
cost / savings / on-demand columns scaled to hourly · daily · monthly
(default) · yearly. Engineering thinks hourly, finance thinks yearly,
cost-optimisation review thinks monthly — instead of mentally
multiplying by 24 × 30 / 12, one dropdown rescales the displayed values.
Affects display values, headers, sort + filter (uses scaled values so a
"< $1" filter at hourly does what it should), summary card, and fan-out
totals. Does NOT scale: Upfront Cost (one-time), Effective % (already a
ratio), Term (already in years).
Conversion factors (consistent with industry quoting):
hourly = monthly / (24 * 30) — 720 hrs/mo
daily = monthly / 30
yearly = monthly * 12
Selection persists in localStorage (`cudly.recs.costPeriod`). When the
persisted value is corrupted/invalid, getCostPeriod() falls back to the
static default ('monthly') rather than to whatever leaked into the
in-memory cache from a prior setCostPeriod() call — defensive against
storage drift across versions.
Tests: 50+ new test cases under "Issue #319: cost-period selector" plus
"Issue #319: localStorage persistence". The persistence tests install a
Map-backed `localStorage` shim per test (the global setup mocks
localStorage as a noop) so we exercise the real persistence semantics.
Closes #319
Three CR nitpicks, all in __tests__/recommendations.test.ts:
1. "changing the dropdown calls setCostPeriod and triggers rerender"
now seeds getCostPeriod with mockReturnValueOnce('monthly') +
mockReturnValue('hourly') and asserts the rerender actually
rebuilt the savings-column header to "Savings / hr". The previous
version would have passed even if the change handler stopped
rebuilding the DOM.
2. "invalid value in localStorage falls back to ..." now seeds the
in-memory state with a non-default value first (setCostPeriodFn
('hourly')) before injecting the invalid localStorage value, so
the test fails if getCostPeriod() incorrectly returns a stale
in-memory cache instead of re-validating localStorage on each
call. Also renamed for clarity ("static default, not prior
in-memory state").
3. Added two new tests in the "table cells scale with period"
describe block that pin the scaled-numeric contract:
- yearly sort orders savings cells by yearly-scaled value (asc):
two recs pre-sorted DESC are reordered ASC by the production
sort which routes through scaleCost. A no-op scale that
preserves input order would fail.
- hourly numeric filter compares the scaled (per-hour) savings,
not the raw monthly value: two recs (360 and 720 monthly) with
a ">0.75" filter — both pass the raw filter, only r-high passes
after hourly scaling (1.0 vs 0.5). A regression that drops the
scaleCost call inside numericCellValue would surface here.
Plus the rebase onto current feat/multicloud-web-frontend (PR #322's
on-demand monthly column landed there). The base column row now has
14 data columns; on_demand_monthly cell uses formatCostForPeriod so
it scales with the active period like savings/monthly_cost, and
SORTABLE_NUMERIC_COLUMNS.on_demand_monthly + numericCellValue.
on_demand_monthly are wired through scaleCost. Header label for
on_demand_monthly now period-scales via getColumnLabel
("On-Demand Monthly" / "On-Demand / hr" / etc.). TABLE_COL_COUNT
bumped from 14 to 15 to account for the extra column.
235 frontend tests pass.
|
All 3 nitpicks from the prior review are already implemented in this PR — flagging them so the next pass closes the loop:
No code changes needed for these — leaving them as-is. Full suite: 1567/1567 passing. @coderabbitai review |
cristim
force-pushed
the
feat/recs-cost-period-selector
branch
from
44458ff to
5184ad1
Compare
CodeRabbit pass-1 responseRebased onto current Rebase resolution highlights (relevant to reviewers):
Nitpicks fixed (in commit
235 frontend tests + Build frontend + frontend tests pre-commit hooks all green locally. @coderabbitai full review |
🧩 Analysis chainLet me verify those line ranges in the test file before triggering the review. 🏁 Script executed: Length of output: 3558
Triggering a fresh review pass now. ✅ Actions performedReview triggered.
|
|
Tip For best results, initiate chat on the files or code changes.
[full_review] |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/__tests__/recommendations.test.ts`:
- Around line 4508-4527: The test triggers an async rerender when dispatching
the change event in the test for loadRecommendations; to avoid a race, after
select!.dispatchEvent(new Event('change')) await a microtask flush (for example
await Promise.resolve() or await new Promise(r => setTimeout(r, 0))) before
asserting document.getElementById('recommendations-list')?.textContent contains
'Savings / hr'; this ensures the async path in loadRecommendations and the
state.setCostPeriod handler have completed and the DOM was updated.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5cdf99d0-7ff9-4918-84c9-89b957a194f1
📒 Files selected for processing (3)
frontend/src/__tests__/recommendations.test.tsfrontend/src/recommendations.tsfrontend/src/state.ts
| test('changing the dropdown calls setCostPeriod and triggers rerender', async () => { | ||
| // Seed a different return value for the post-change loadRecommendations() | ||
| // call so we can verify the rerender actually rebuilt headers/cells | ||
| // against the new period (not just that setCostPeriod was invoked). | ||
| // CR pass-1 nitpick: the previous version of this test would still pass | ||
| // if the change handler stopped rebuilding the DOM. | ||
| (state.getCostPeriod as jest.Mock) | ||
| .mockReturnValueOnce('monthly') | ||
| .mockReturnValue('hourly'); | ||
| await loadRecommendations(); | ||
| const select = document.querySelector<HTMLSelectElement>('#cost-period-select'); | ||
| expect(select).not.toBeNull(); | ||
| select!.value = 'hourly'; | ||
| select!.dispatchEvent(new Event('change')); | ||
| expect(state.setCostPeriod).toHaveBeenCalledWith('hourly'); | ||
| // The rerender should have rebuilt the savings column header to reflect | ||
| // the new hourly period — pin the DOM update so a future regression that | ||
| // stops invalidating the rendered tree fails this test. | ||
| expect(document.getElementById('recommendations-list')?.textContent).toContain('Savings / hr'); | ||
| }); |
There was a problem hiding this comment.
Await async rerender before asserting updated header text
On Line 4521, the change event can trigger an async rerender path. The assertion on Line 4526 may race and become flaky. Add a microtask flush before checking DOM text.
Proposed fix
select!.value = 'hourly';
select!.dispatchEvent(new Event('change'));
expect(state.setCostPeriod).toHaveBeenCalledWith('hourly');
+ // Let async rerender settle before asserting DOM text.
+ await Promise.resolve();
+ await Promise.resolve();
// The rerender should have rebuilt the savings column header to reflect
// the new hourly period — pin the DOM update so a future regression that
// stops invalidating the rendered tree fails this test.
expect(document.getElementById('recommendations-list')?.textContent).toContain('Savings / hr');📝 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.
| test('changing the dropdown calls setCostPeriod and triggers rerender', async () => { | |
| // Seed a different return value for the post-change loadRecommendations() | |
| // call so we can verify the rerender actually rebuilt headers/cells | |
| // against the new period (not just that setCostPeriod was invoked). | |
| // CR pass-1 nitpick: the previous version of this test would still pass | |
| // if the change handler stopped rebuilding the DOM. | |
| (state.getCostPeriod as jest.Mock) | |
| .mockReturnValueOnce('monthly') | |
| .mockReturnValue('hourly'); | |
| await loadRecommendations(); | |
| const select = document.querySelector<HTMLSelectElement>('#cost-period-select'); | |
| expect(select).not.toBeNull(); | |
| select!.value = 'hourly'; | |
| select!.dispatchEvent(new Event('change')); | |
| expect(state.setCostPeriod).toHaveBeenCalledWith('hourly'); | |
| // The rerender should have rebuilt the savings column header to reflect | |
| // the new hourly period — pin the DOM update so a future regression that | |
| // stops invalidating the rendered tree fails this test. | |
| expect(document.getElementById('recommendations-list')?.textContent).toContain('Savings / hr'); | |
| }); | |
| test('changing the dropdown calls setCostPeriod and triggers rerender', async () => { | |
| // Seed a different return value for the post-change loadRecommendations() | |
| // call so we can verify the rerender actually rebuilt headers/cells | |
| // against the new period (not just that setCostPeriod was invoked). | |
| // CR pass-1 nitpick: the previous version of this test would still pass | |
| // if the change handler stopped rebuilding the DOM. | |
| (state.getCostPeriod as jest.Mock) | |
| .mockReturnValueOnce('monthly') | |
| .mockReturnValue('hourly'); | |
| await loadRecommendations(); | |
| const select = document.querySelector<HTMLSelectElement>('#cost-period-select'); | |
| expect(select).not.toBeNull(); | |
| select!.value = 'hourly'; | |
| select!.dispatchEvent(new Event('change')); | |
| expect(state.setCostPeriod).toHaveBeenCalledWith('hourly'); | |
| // Let async rerender settle before asserting DOM text. | |
| await Promise.resolve(); | |
| await Promise.resolve(); | |
| // The rerender should have rebuilt the savings column header to reflect | |
| // the new hourly period — pin the DOM update so a future regression that | |
| // stops invalidating the rendered tree fails this test. | |
| expect(document.getElementById('recommendations-list')?.textContent).toContain('Savings / hr'); | |
| }); |
🤖 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__/recommendations.test.ts` around lines 4508 - 4527, The
test triggers an async rerender when dispatching the change event in the test
for loadRecommendations; to avoid a race, after select!.dispatchEvent(new
Event('change')) await a microtask flush (for example await Promise.resolve() or
await new Promise(r => setTimeout(r, 0))) before asserting
document.getElementById('recommendations-list')?.textContent contains 'Savings /
hr'; this ensures the async path in loadRecommendations and the
state.setCostPeriod handler have completed and the DOM was updated.
|
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. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 1 file(s) based on 1 unresolved review comment. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
1 participant
Summary
Adds a "Show costs" period selector to the recommendations toolbar — Hourly · Daily · Monthly (default) · Yearly — that scales every cost-bearing column on the page consistently.
Closes #319.
What changes
localStorage('cudly.recs.costPeriod').Cost / hr,Cost / day,Cost / mo,Cost / yr).< $1filter at hourly behaves intuitively.Notable bug-fix folded in
getCostPeriod()now falls back to the static default'monthly'when the persisted value is corrupted/invalid, instead of returning whatever leaked into the in-memory cache from a priorsetCostPeriod()call. Defensive against storage drift across versions and against state-leak in tests.Tests
localStorageshim per test (the globalsetup.tsmockslocalStorageas a noop —getItem→null,setItem→undefined), so the persistence semantics are actually exercised.Sibling PRs (concurrent batch)
on_demand_monthlyper period (one entry in the period-scaling map).Summary by CodeRabbit