feat(frontend/purchase-modal): full per-purchase breakdown + per-row skip checkboxes#325
Merged
cristim merged 2 commits intoMay 6, 2026
Conversation
…skip checkboxes Expands the single-bucket Execute Purchase modal (openPurchaseModal) to show all per-purchase details needed for an informed commit decision, and adds per-row checkboxes so users can exclude individual purchases without leaving the modal. Changes: - New columns: Account, Service/Type, Region, Count, Upfront, Monthly Cost, Eff. Savings, Eff. % (reusing effectiveMonthlySavings / effectiveSavingsPct helpers — no re-derivation). - Per-row "Include" checkbox (default checked) + select-all/deselect-all in the table header. - Live totals row (tfoot) that updates on every checkbox toggle: total count, upfront, monthly cost, effective savings, weighted-average effective %. - Execute Purchase button disabled (with tooltip) when zero rows selected. - getPurchaseModalRecommendations() now filters by checkedPurchaseIndices so only checked rows are submitted — no changes to app.ts execute path needed. - clearPurchaseModalRecommendations() resets checkbox state. - CSS: .purchase-modal-commits has max-height + overflow-y:auto with sticky thead/tfoot so the table scrolls cleanly on long lists. - Modal shows monthly totals regardless of the page's cost-period selector — by design (modal is a commit-decision context where monthly is canonical). - 12 new unit tests covering: all-selected default, per-row exclusion, totals math, disabled-button state, select-all/deselect-all, column headers. Closes #320
📝 WalkthroughWalkthroughPer-row selection state and UI were added to the Purchase modal: rows are checked by default when the modal opens, users can toggle per-row Include checkboxes (with a select‑all header), totals update live, and only checked rows are returned by ChangesPer-Row Purchase Selection & Modal Expansion
Sequence DiagramsequenceDiagram
actor User
participant Modal as Purchase Modal
participant State as Selection State
participant Totals as Totals Footer
participant Executor as Execute Path
User->>Modal: Open purchase modal
Modal->>State: Initialize all rows checked
State-->>Modal: Checked indices set
Modal->>User: Render rows (all checked) + totals
User->>Modal: Toggle per-row Include
Modal->>State: Update checked indices
State-->>Modal: Return updated checked set
Modal->>Totals: Recalculate totals for checked rows
Totals-->>Modal: New totals
Modal->>User: Re-render totals & button state
User->>Modal: Click Execute Purchase
Modal->>State: getPurchaseModalRecommendations()
State-->>Modal: Return only checked recommendations
Modal->>Executor: Submit checked recommendations
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 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 |
cristim
added
priority/p1
Member
Author
|
@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.
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/recommendations.ts`:
- Around line 3141-3152: The weighted-average denominator must match
effectiveSavingsPct's logic: when computing weightedEffSavingsNum/Den in the
loop (variables weightedEffSavingsNum, weightedEffSavingsDen, and the code that
computes amortized, effSav, onDemand), prefer rec.on_demand_cost when present
instead of reconstructing onDemand from rec.monthly_cost + rec.savings +
amortized; also include recs where rec.monthly_cost is null but
rec.on_demand_cost exists (so they aren't erroneously skipped). Update the
onDemand assignment to use rec.on_demand_cost ?? (rec.monthly_cost + rec.savings
+ amortized) and keep the term>0 guard (or mirror effectiveSavingsPct's
null-check) so the weighted average aligns with per-row effectiveSavingsPct and
effectiveMonthlySavings(rec).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1f959c43-e0f3-43c7-8960-f132c10b2cf3
📒 Files selected for processing (3)
frontend/src/__tests__/recommendations.test.tsfrontend/src/recommendations.tsfrontend/src/styles/modals.css
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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 (1)
frontend/src/recommendations.ts (1)
3393-3409:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTerm edits leave effective metrics stale in both row and totals.
After Line 3397 mutates
live.term, the row’sEff. Savings/Eff. %cells and the totals footer are not recomputed. Users can change term and still see pre-change economics until they toggle an Include checkbox.💡 Suggested fix
termSelect.addEventListener('change', () => { const live = currentPurchaseRecommendations[idx]; if (!live) return; const newTerm = parseInt(termSelect.value, 10) === 3 ? 3 : 1; live.term = newTerm; // Rebuild this row's payment options for the new term; if current // payment is no longer supported, pick the first valid option and // mirror back to live state. rebuildPaymentOptions( paymentSelect, live.provider as CompatProvider, live.service, newTerm, (live.payment ?? '') as CompatPayment, ); live.payment = paymentSelect.value; + + // Refresh per-row computed metrics that depend on term. + effSavTd.textContent = formatCurrency(effectiveMonthlySavings(live)); + const nextPct = effectiveSavingsPct(live); + effPctTd.classList.toggle('effective-pct-negative', nextPct !== null && nextPct < 0); + effPctTd.textContent = nextPct !== null ? `${nextPct.toFixed(1)}%` : '—'; + + // Keep totals/footer and execute-state in sync with edited term values. + const selectAllCb = document.getElementById('purchase-modal-select-all') as HTMLInputElement | null; + if (selectAllCb) updatePurchaseModalTotals(selectAllCb); });🤖 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/recommendations.ts` around lines 3393 - 3409, After mutating live.term (the object from currentPurchaseRecommendations) you must recompute and re-render that row's effective metrics and the totals footer; update the change handler on termSelect so that after setting live.term (and after rebuildPaymentOptions / live.payment assignment) it calls the same routines used by the Include-toggle path to recalculate row economics and refresh aggregate totals (i.e., invoke the functions that recompute a recommendation's effective savings/percent and the function that updates the totals footer). Locate the include-checkbox handler to find the exact helper names and call those helpers (or extract them if needed) so the row cells and totals stay in sync when term changes.
🧹 Nitpick comments (2)
frontend/src/__tests__/recommendations.test.ts (2)
1477-1488: ⚡ Quick winAlso assert the zero-selection guidance text, not only
disabled.Issue
#320calls out explicit guidance (“Select at least one purchase”). This test currently validates only button state, so the UX contract can regress unnoticed.Proposed assertion addition
const btn = document.getElementById('execute-purchase-btn') as HTMLButtonElement | null; expect(btn?.disabled).toBe(true); + // Pin the disabled-state guidance contract from issue `#320`. + expect(btn?.getAttribute('title') ?? document.getElementById('purchase-modal-disabled-hint')?.textContent ?? '') + .toContain('Select at least one purchase');🤖 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 1477 - 1488, The test should also assert that the zero-selection guidance text appears when all rows are unchecked: after using openPurchaseModal([...]) and unchecking the .purchase-modal-row-include checkboxes (and dispatching 'change'), in addition to checking execute-purchase-btn.disabled === true, query the DOM for the guidance element (e.g., text content matching "Select at least one purchase" or the element that renders that message) and assert it is visible/present; update the test case name/comment if needed to reflect that it verifies both disabled state and guidance text.
1428-1614: ⚡ Quick winAdd a per-test modal-state reset in this describe block.
This suite mutates modal selection state repeatedly; adding a local reset makes test order irrelevant and prevents hidden coupling.
Proposed test-hardening patch
describe('per-row skip checkboxes (issue `#320`)', () => { + beforeEach(() => { + clearPurchaseModalRecommendations(); + }); + const makeRec = (id: string, opts: Partial<{🤖 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 1428 - 1614, The tests in the "per-row skip checkboxes" suite mutate modal selection state across tests; ensure each test resets modal state by calling clearPurchaseModalRecommendations() (or an equivalent reset) before or after openPurchaseModal() so tests are isolated; update the describe block to invoke clearPurchaseModalRecommendations() in a beforeEach (or directly at start of each test) to reset checkboxes, select-all state, and totals so functions like openPurchaseModal, getPurchaseModalRecommendations, and clearPurchaseModalRecommendations operate on a clean modal for every 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.
Outside diff comments:
In `@frontend/src/recommendations.ts`:
- Around line 3393-3409: After mutating live.term (the object from
currentPurchaseRecommendations) you must recompute and re-render that row's
effective metrics and the totals footer; update the change handler on termSelect
so that after setting live.term (and after rebuildPaymentOptions / live.payment
assignment) it calls the same routines used by the Include-toggle path to
recalculate row economics and refresh aggregate totals (i.e., invoke the
functions that recompute a recommendation's effective savings/percent and the
function that updates the totals footer). Locate the include-checkbox handler to
find the exact helper names and call those helpers (or extract them if needed)
so the row cells and totals stay in sync when term changes.
---
Nitpick comments:
In `@frontend/src/__tests__/recommendations.test.ts`:
- Around line 1477-1488: The test should also assert that the zero-selection
guidance text appears when all rows are unchecked: after using
openPurchaseModal([...]) and unchecking the .purchase-modal-row-include
checkboxes (and dispatching 'change'), in addition to checking
execute-purchase-btn.disabled === true, query the DOM for the guidance element
(e.g., text content matching "Select at least one purchase" or the element that
renders that message) and assert it is visible/present; update the test case
name/comment if needed to reflect that it verifies both disabled state and
guidance text.
- Around line 1428-1614: The tests in the "per-row skip checkboxes" suite mutate
modal selection state across tests; ensure each test resets modal state by
calling clearPurchaseModalRecommendations() (or an equivalent reset) before or
after openPurchaseModal() so tests are isolated; update the describe block to
invoke clearPurchaseModalRecommendations() in a beforeEach (or directly at start
of each test) to reset checkboxes, select-all state, and totals so functions
like openPurchaseModal, getPurchaseModalRecommendations, and
clearPurchaseModalRecommendations operate on a clean modal for every test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0a8ecfb1-f044-4ab3-9c42-909985b49653
📒 Files selected for processing (2)
frontend/src/__tests__/recommendations.test.tsfrontend/src/recommendations.ts
12 tasks
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
openPurchaseModal) to show full per-purchase detail: Account, Service/Type, Region, Count, Upfront, Monthly Cost, Effective Savings, and Effective %.<tfoot>) updates on every checkbox toggle; Execute button disabled with tooltip when zero rows selected.getPurchaseModalRecommendations()now filters by checked indices; no changes toapp.tsexecute path needed.Modal shows monthly totals regardless of the page's cost-period selector — by design (the modal is a commit-decision context where monthly is canonical).
Coordination note
Parallel agents are implementing #317, #318, #319 in separate worktrees on the same base branch. This PR touches only the modal area (
openPurchaseModal~L2959,renderPurchaseModalRow~L3267, newupdatePurchaseModalTotals) and does not overlap with the column-map / render area that #317–#319 touch. Minor rebase may be needed after one of them merges.Implementation details
effectiveMonthlySavings/effectiveSavingsPcthelpers (no re-derivation).checkedPurchaseIndices: Set<number>tracks included rows;checkedPurchaseModalInitialisedflag distinguishes "user cleared all" from "modal never opened".clearPurchaseModalRecommendations()resets both, preserving existing close behaviour.styles/modals.css:.purchase-modal-commitswithmax-height: 55vh; overflow-y: auto, stickythead/tfoot.Test plan
npm testfromfrontend/)npm run buildsucceedsCloses #320
Summary by CodeRabbit
New Features
Tests
Style
Docs