feat(frontend/recs): group rows by cell with savings range + collapse/expand (closes #225, closes #226)#253
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:
📝 WalkthroughWalkthroughThe PR refactors the recommendations table from flat per-row rendering to a cell-grouped model. Multi-variant cells now render a summary row by default and expand on demand, with helpers computing per-cell and page-level savings ranges. An "Expand all" toggle was added to the filter bar, and new TypeScript/CSS definitions support the grouped UI. ChangesCell Grouping & Expansion UX
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 1510-1522: The default "visible" purchase target must be computed
at cell (group) granularity rather than per-recommendation element: update the
logic that builds the default visible selection (currently using
recommendations/selectedRecs and lastVisibleGroupKeys) to treat each group key
in sortedKeys as one visible target when groups.get(key)?.length > 1 (i.e. use
groupRecsByCell(groups) and lastVisibleGroupKeys as the single selectable unit),
so the action text and default submit operate on one summary row per cell
instead of on every sibling variant; adjust any code that derives the count or
list of visible targets (where selectedRecs or the expand-all behavior is
computed) to use the group key list (lastVisibleGroupKeys or sortedKeys filtered
for multi-variant groups) rather than the raw recommendations array.
- Around line 1467-1493: The code injects rec.provider raw into class names and
text in buildVariantRowMarkup (and the similar block at 1559-1563), so
sanitize/whitelist it before interpolation: validate rec.provider against an
allowed provider list or normalize it to a safe token (e.g., map to lowercase
alphanumerics/hyphens, fallback 'unknown-provider') and use that sanitized token
for the CSS class while still escaping the displayed text with
escapeHtml(rec.provider) or a mapped display name; replace direct uses of
rec.provider and rec.provider.toUpperCase() in the template with the sanitized
class token and escaped display string respectively, and apply the same change
to the other analogous function/markup block.
🪄 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: c4724c87-8107-4905-b6d3-4f0113a5fd9a
📒 Files selected for processing (4)
frontend/src/__tests__/recommendations.test.tsfrontend/src/css-modules.d.tsfrontend/src/recommendations.tsfrontend/src/styles/components.css
…ell purchase target - Add providerBadgeClass() helper that whitelists provider to aws|azure|gcp before injecting into CSS class attributes; use escapeHtml(providerDisplayName()) for the badge display text. Applied in both the summary row and variant row markup paths. - resolvePurchaseTarget() now uses pickBestVariantPerCell(visible) for the default (no-selection) path so the purchase target is exactly one rec per physical resource, matching the collapsed grouped table view. Submitting all visible variants of a multi-variant cell would create conflicting reservations. - updateBottomActionBox() derives visibleCellCount from pickBestVariantPerCell for the default path and shows "N cells visible" rather than "N visible" to match the cell-grouped table count. - Update affected tests: #111 (a)/(b)/(c)/(d) fixtures changed to use distinct resource_type values per rec so each rec occupies its own cell and all appear in the default purchase target (triggering fan-out). Bundle B summary text assertions updated to match "cells visible" format.
CodeRabbit review — round 2 responseAddressed both actionable findings from the round 1 review: Finding 1 — provider injection into CSS class (security)
Finding 2 — cell purchase target regression
Test updates
All 1452 tests pass, TypeScript clean, build clean. @coderabbitai review |
|
Tip For best results, initiate chat on the files or code changes.
Triggering a fresh review now. [review] |
…/expand (closes #225, closes #226) - Group recommendation rows by physical-resource cell (provider|account|service| region|resource_type|engine) using the same cellKey as #224's radio enforcement. - Multi-variant cells render as a collapsed summary row by default (chevron to expand); single-variant cells render flat with no group overhead. - Per-cell summary row shows savings range ($X – $Y/mo), upfront range, and term range; collapses to a single value when all variants are uniform. - When a variant is selected (radio from #224), the summary row shows that variant's values with a "+N variants" affordance instead of the range. - Page-level range banner at the top of the table: "Recommended range: $X – $Y/mo across N cells" (sum of per-cell min/max across visible cells). - Expand-All/Collapse-All toggle added to the filter-status bar; only shown when at least one multi-variant cell is present. - Sort operates at cell level: groups ordered by selected-variant value if one is selected in the cell, else by max value across variants. String columns use the shared cell-identity value. Within a cell, variants sort by (term ASC, payment fixed-order: no-upfront < partial < all-upfront < monthly). - Filter operates at cell level: any variant matching a filter keeps the entire cell visible. - Adds css-modules.d.ts to fix a pre-existing TS 5.9 strict-mode error on the `import './styles.css'` side-effect import in index.ts. - Removes now-unused sortedRecommendations flat-list sort helper (superseded by groupsInSortOrder which reuses the same SORTABLE_* column maps). - Exports resetExpandedCells() for test isolation of module-level expand state. - Adds 16 new tests (6 pure-helper, 6 DOM integration, 4 expand/collapse) and updates the existing #224 radio-enforcement test to expand the cell first.
…ell purchase target - Add providerBadgeClass() helper that whitelists provider to aws|azure|gcp before injecting into CSS class attributes; use escapeHtml(providerDisplayName()) for the badge display text. Applied in both the summary row and variant row markup paths. - resolvePurchaseTarget() now uses pickBestVariantPerCell(visible) for the default (no-selection) path so the purchase target is exactly one rec per physical resource, matching the collapsed grouped table view. Submitting all visible variants of a multi-variant cell would create conflicting reservations. - updateBottomActionBox() derives visibleCellCount from pickBestVariantPerCell for the default path and shows "N cells visible" rather than "N visible" to match the cell-grouped table count. - Update affected tests: #111 (a)/(b)/(c)/(d) fixtures changed to use distinct resource_type values per rec so each rec occupies its own cell and all appear in the default purchase target (triggering fan-out). Bundle B summary text assertions updated to match "cells visible" format.
e22afd6 to
5201c69
Compare
|
Rebased onto current |
|
Tip For best results, initiate chat on the files or code changes.
[review] |
Summary
Closes #225 and #226. This PR merges both issues into a single feature since they're tightly coupled (cell grouping is the visual anchor that makes savings range legible).
(provider|account|service|region|resource_type|engine)— the same key as fix(frontend/recs): enforce one-variant-per-cell radio-button selection (select-all currently 6x over-commits) #224's radio enforcement. Multi-variant cells render as a collapsed summary row with a chevron; single-variant cells render flat (no overhead). Expand-all toggle added to the filter-status bar.$X – $Y/mosavings range across all (term, payment) variants. Collapses to$Xwhen all variants are uniform. When a variant is selected (radio from fix(frontend/recs): enforce one-variant-per-cell radio-button selection (select-all currently 6x over-commits) #224), the summary row shows that variant's value with(+N variants)instead of the range. A page-level range banner at the top of the table shows the aggregate across all visible cells.Design choices (UX call — flag anything to change in CR comments)
colspan=8(the remaining columns after the 4 identity ones).padding-left: 2remon all cells except the checkbox column. Summary rows have a slightly distinct background (#f3f6fb). No new CSS framework or component library.css-modules.d.tsaddition — fixes a pre-existing TypeScript 5.9 strict-mode error on theimport './styles.css'side-effect import inindex.ts. The base branch had this error but CI passed because of bundler resolution; adding the declaration makes localtsc --noEmitclean as well.Out of scope
Test plan
npm test)--noEmitcleannpm run buildcompiles without errorsgroupRecsByCellpure helper: 2 testscellSummarypure helper: 3 testspageLevelRangepure helper: 2 tests🤖 Generated with claude-flow
Summary by CodeRabbit
Release Notes