feat(frontend/recs): add Monthly cost + Effective savings % columns#242
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds two numeric columns to recommendations: "Monthly Cost" and "Effective %". Introduces exported helpers ChangesRecommendations Table: Monthly Cost & Effective Savings % Columns
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 docstrings
🧪 Generate unit tests (beta)
Review rate limit: 3/5 reviews remaining, refill in 19 minutes and 42 seconds. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/recommendations.ts`:
- Around line 133-136: The current transformer sets effective_savings_pct to
Number.NEGATIVE_INFINITY which makes null-like rows sort first; change the row
mapper (effective_savings_pct: (r) => ...) to return null when
effectiveSavingsPct(r) is not a number (i.e., use effectiveSavingsPct(r) ??
null) and update the sorting logic (the comparator used where rows are sorted)
to be null-aware and place nulls last (e.g., treat null as greater than any real
number or explicitly handle null comparisons so null values are considered
larger than any numeric percentage). Ensure references: effective_savings_pct
mapping, effectiveSavingsPct function, and the sort comparator are updated
accordingly.
- Around line 193-199: The helper effectiveSavingsPct is incorrectly clamping a
zero term to 1 year and producing a percentage; update it to return null for
explicit zero-term rows by checking if r.term === 0 (or r.term !== undefined &&
r.term === 0) before computing monthsInTerm, so zero-term returns null and only
undefined/null or positive terms continue to the existing logic that uses
monthsInTerm, amortized, effectiveSavings, and onDemand.
In `@frontend/src/state.ts`:
- Around line 18-19: The typed sort contract (RecommendationsSortColumn) is
missing the new sortable headers 'monthly_cost' and 'effective_savings_pct',
causing a cast in recommendations.ts; update the RecommendationsSortColumn union
to include 'monthly_cost' and 'effective_savings_pct' and ensure any dependent
types (e.g., the sort state/type used by RecommendationsSortState or similar in
recommendations.ts) are updated to use the extended union so the sort state
becomes type-safe and removes the cast.
🪄 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: ca2c373f-4747-4e79-a740-c0e78abb22a8
📒 Files selected for processing (3)
frontend/src/__tests__/recommendations.test.tsfrontend/src/recommendations.tsfrontend/src/state.ts
|
Addressed all 3 actionable CR findings (POSITIVE_INFINITY sentinel, term=0 returns null, RecommendationsSortColumn union extended). @coderabbitai review |
|
Tip For best results, initiate chat on the files or code changes.
[review] |
…loses #220, closes #221) - Extract effectiveMonthlySavings() from pickBestVariantPerCell inline arrow into a named export so the new Effective % column shares the same amortization logic (DRY — no divergent copies). - Add effectiveSavingsPct() helper: amortizes upfront over term, returns null when on_demand_monthly=0 to guard against division by zero. - Monthly Cost column: renders rec.monthly_cost next to Upfront Cost so no-upfront / partial-upfront / all-upfront rows tell the complete cost story without mental arithmetic. - Effective % column: amortized savings as a percentage of on-demand monthly cost; negative values get class="effective-pct-negative" for anomaly visibility; null renders as em-dash. - Both columns wired into SORTABLE_NUMERIC_COLUMNS, SORT_HEADER_LABELS, FILTERABLE_COLUMNS, NUMERIC_COLUMNS, categoricalCellValue, and numericCellValue so sort, filter, and popover infrastructure works identically to existing numeric columns. - state.ts: extend RecommendationsColumnId union with monthly_cost and effective_savings_pct for compile-time typo safety. - Tests: 13 new tests covering helper math (all three payment variants, term=0, on_demand=0, negative pct, undefined monthly_cost), column header presence, cell rendering, sort-header click wiring. Updated two existing tests that hard-coded the column count (9→11).
- effectiveSavingsPct: return null for term=0 (issue #221 acceptance criteria says zero-term rows should render as em-dash, not compute a percentage from a clamped value) - SORTABLE_NUMERIC_COLUMNS: replace NEGATIVE_INFINITY sentinel with POSITIVE_INFINITY so null-pct rows sort to the bottom in ascending order (previously they floated to the top, opposite of the comment) - state.ts RecommendationsSortColumn: extend union with monthly_cost and effective_savings_pct so the dataset cast in renderRecommendationsList is type-safe rather than a widening escape hatch - Update effectiveSavingsPct term=0 test to assert null return value
e292e01 to
e0ad339
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
… sort-column type union (CR feedback on #242) - SORTABLE_NUMERIC_COLUMNS now typed as (r) => number | null; effective_savings_pct extractor returns effectiveSavingsPct(r) directly (no POSITIVE_INFINITY sentinel) - sortedRecommendations null-aware: null rows always placed last regardless of asc/desc direction (split-and-concat via guard clauses in the comparator) - effectiveSavingsPct: explicit r.term === 0 guard (in addition to null check) instead of falsy !r.term, so the anomaly path is unambiguous - state.ts RecommendationsSortColumn already includes monthly_cost and effective_savings_pct (no cast needed); comment updated to confirm - Tests: stale "clamps to 12 months" description updated; new test pins null-last sort in both asc and desc for effective_savings_pct column
|
Addressed CR's 3 actionable items in commit b297b92:
@coderabbitai review |
|
Tip For best results, initiate chat on the files or code changes.
Kicking off a fresh review now. [review] |
Summary
upfront_cost+monthly_cost+savings) without requiring mental arithmetic (closes feat(frontend/recs): add Monthly cost column alongside Upfront cost #220).—whenon_demand_monthly == 0(closes feat(frontend/recs): add Effective savings % column (amortized over term) #221).effectiveMonthlySavings()from the inline arrow inpickBestVariantPerCell(added by fix(frontend/recs): enforce one-variant-per-cell radio selection (closes #224) #231/issue fix(frontend/recs): enforce one-variant-per-cell radio-button selection (select-all currently 6x over-commits) #224) into a named export used by both the sort extractor and the new Effective % helper — no divergent amortization formulas.Test plan
npm testinfrontend/passes (1435 tests, 0 failures).npm run buildcompletes successfully.upfront_cost.effective-pct-negativeclass).—.Summary by CodeRabbit
New Features
Bug Fixes / Improvements
Tests