Skip to content

fix(frontend): Opportunities group rows, payback range and action bar wording #1699

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
cristim merged 2 commits into from
Nov 4, 2026
Merged

fix(frontend): Opportunities group rows, payback range and action bar wording #1699

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c3ef8d1
fix(frontend): Opportunities group rows, payback range and action bar…
cristim Nov 4, 2026
24ae940
test(frontend): scroll the affordance check to the table's real midpoint
cristim Nov 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 76 additions & 5 deletions frontend/src/__tests__/recommendations.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1437,7 +1437,7 @@ describe('Recommendations Module', () => {
// real estate for the dollar figures they're authorising.
expect(summary?.textContent).toMatch(/\$100\/mo/);
expect(summary?.textContent).toMatch(/\$500 upfront/);
expect(summary?.textContent).toMatch(/1 cell\b/);
expect(summary?.textContent).toMatch(/1 row\b/);
// Old bulk-toolbar surface is gone.
expect(document.querySelector('.recommendations-bulk-toolbar')).toBeNull();
});
Expand Down Expand Up @@ -1468,7 +1468,7 @@ describe('Recommendations Module', () => {
// assertion doesn't couple to that separator choice.
expect(summary?.textContent).toMatch(/\$300\s*[–\-]\s*\$500\/mo/);
expect(summary?.textContent).toMatch(/\$1,?100\s*[–\-]\s*\$1,?800 upfront/);
expect(summary?.textContent).toMatch(/2 cells\b/);
expect(summary?.textContent).toMatch(/2 rows\b/);
});

test('action-box summary collapses to single value when all per-cell variant ranges are identical (#281)', async () => {
Expand All @@ -1488,7 +1488,7 @@ describe('Recommendations Module', () => {
// savingsMin==savingsMax==$400; formatSavingsRange collapses to "$400".
expect(summary?.textContent).toMatch(/\$400\/mo/);
expect(summary?.textContent).not.toMatch(/[–\-]\s*\$400/);
expect(summary?.textContent).toMatch(/2 cells\b/);
expect(summary?.textContent).toMatch(/2 rows\b/);
});

test('bottom action box prompts for selection when no row is selected (#273)', async () => {
Expand All @@ -1499,7 +1499,7 @@ describe('Recommendations Module', () => {
// #273: action buttons require an explicit selection. Summary
// prompts the user instead of advertising a default-target count
// that would fire on misclick.
expect(summary?.textContent).toContain('Select cells to act on');
expect(summary?.textContent).toContain('Select rows to act on');
expect(document.querySelector('.recommendations-bulk-toolbar')).toBeNull();
});

Expand Down Expand Up @@ -3366,6 +3366,77 @@ describe('Bundle B: column header filter triggers', () => {
});
});

// Issue #1684: Opportunities group rows, payback card and action-bar wording.
describe('Issue #1684: Opportunities group rows, payback and wording', () => {
const base = { provider: 'aws', cloud_account_id: 'a1684', service: 'rds', resource_type: 'db.t4g.micro', engine: 'postgresql', region: 'eu-west-1', count: 1 };
const variants = [
{ ...base, id: 'v1684-nu', term: 1, payment: 'no-upfront', savings: 10, upfront_cost: 0 },
{ ...base, id: 'v1684-au', term: 1, payment: 'all-upfront', savings: 20, upfront_cost: 120 },
];

async function load(recs: Array<Record<string, unknown>>, selected: string[] = []): Promise<void> {
(api.getRecommendations as jest.Mock).mockResolvedValue({ summary: {}, recommendations: recs, regions: [] });
(state.getRecommendations as jest.Mock).mockReturnValue(recs);
(state.getVisibleRecommendations as jest.Mock).mockReturnValue(recs);
(state.getRecommendationsColumnFilters as jest.Mock).mockReturnValue({});
(state.getSelectedRecommendationIDs as jest.Mock).mockReturnValue(new Set(selected));
await loadRecommendations();
}

const paybackText = (): string =>
document.querySelector('#recommendations-summary [data-kpi="payback"]')?.textContent ?? '';

test('expanded variant rows leave the group identity cells blank', async () => {
await load(variants);
const chevron = document.querySelector<HTMLButtonElement>('tr.rec-cell-summary-row .rec-cell-chevron');
if (chevron?.getAttribute('aria-expanded') !== 'true') chevron!.click();
const row = document.querySelector<HTMLTableRowElement>('tr.rec-variant-row[data-rec-id="v1684-au"]')!;
const cells = Array.from(row.cells).slice(1, 5).map((td) => td.textContent?.trim());
expect(cells).toEqual(['', '', '', '']);
expect(row.textContent).not.toContain('eu-west-1');
// The row is still named in full for assistive tech.
expect(row.querySelector('input[type="checkbox"]')?.getAttribute('aria-label')).toContain('db.t4g.micro (postgresql)');
expect(row.textContent).toContain('All Upfront');
});

test('collapsed group summary puts no space inside the period unit', async () => {
await load(variants);
const range = document.querySelector('tr.rec-cell-summary-row .rec-cell-range')?.textContent ?? '';
expect(range).toMatch(/\$20\/mo/);
expect(range).not.toContain('/ mo');
});

test('group summary skips blank resource type and region', async () => {
const sp = { provider: 'azure', cloud_account_id: 'sub1684', service: 'savingsplans', resource_type: '', region: '', count: 1, upfront_cost: 0 };
await load([
{ ...sp, id: 'sp1684-1', term: 1, payment: 'monthly', savings: 30 },
{ ...sp, id: 'sp1684-3', term: 3, payment: 'monthly', savings: 50 },
]);
const identity = document.querySelector('tr.rec-cell-summary-row .rec-cell-identity')?.textContent ?? '';
expect(identity.trim()).toBe('2 variants');
});

test('payback card never shows a 0.0-month lower bound', async () => {
await load(variants);
// no-upfront has no payback; all-upfront pays back in 120 / 20 = 6 months.
expect(paybackText()).toContain('Up to 6.0 months');
expect(paybackText()).not.toContain('0.0');
});

test('payback card shows the no-value dash when nothing is paid upfront', async () => {
await load(variants, ['v1684-nu']);
expect(paybackText()).toContain('—');
expect(paybackText()).not.toContain('months');
});

test('action bar talks about rows, not cells', async () => {
await load(variants);
expect(document.getElementById('recommendations-action-summary')?.textContent).toBe('(Select rows to act on)');
await load(variants, ['v1684-au']);
expect(document.getElementById('recommendations-action-summary')?.textContent).toMatch(/across 1 row\)$/);
});
});

// Issue #164: cross-column-aware (cascading) distinct values.
// Each column popover must show only values from rows that pass all
// OTHER active filters -- values that produce non-empty results when
Expand Down Expand Up @@ -3542,7 +3613,7 @@ describe('Bundle B: sticky bottom action box', () => {
expect(planBtn.hasAttribute('title')).toBe(false);
expect(planBtn.getAttribute('aria-describedby')).toBe('recommendations-action-disabled-hint');
expect(hint.hidden).toBe(false);
expect(hint.textContent).toContain('Select at least one cell to enable');
expect(hint.textContent).toContain('Select at least one row to enable');
expect(hint.getAttribute('aria-live')).toBe('polite');

// Selection populated → buttons enabled, label shows the selection
Expand Down
77 changes: 52 additions & 25 deletions frontend/src/recommendations.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1595,11 +1595,14 @@ export function formatSavingsRange(min: number, max: number): string {

/** Format a payback-months range, collapsing identical endpoints to a
* single value and rounding to 1 decimal. Used by the Payback Period
* summary card after #279 broadened it to a range. */
* summary card after #279 broadened it to a range. A 0 lower bound is a
* no-upfront choice, which has no payback (shown as NO_VALUE on its own),
* so the range reads "Up to X months" instead of "0.0 – X" (issue #1684). */
function formatPaybackRange(min: number, max: number): string {
const lo = min.toFixed(1);
const hi = max.toFixed(1);
return lo === hi ? `${lo} months` : `${lo} – ${hi} months`;
if (lo === hi) return `${lo} months`;
return min > 0 ? `${lo} – ${hi} months` : `Up to ${hi} months`;
}

/** Format a term range, collapsing "1 yr – 1 yr" to "1 yr". */
Expand Down Expand Up @@ -1809,6 +1812,11 @@ export function periodSuffix(period: CostPeriod): string {
}
}

/** Suffix appended directly to a value ("$20/mo"); periodSuffix spaces it for labels. */
function periodUnit(period: CostPeriod): string {
return periodSuffix(period).replace('/ ', '/');
}

/**
* Format a period-scaled savings range using period-appropriate precision.
* Used for the summary cards and cell-level range displays (issue #319).
Expand Down Expand Up @@ -3323,6 +3331,16 @@ function renderColumnCell(key: state.RecommendationsColumnId, rec: LocalRecommen
}
}

// 'flat' is a single-variant cell, 'sp-child' a single-variant cell under a
// Savings Plans group row, and 'variant' one of several variants of a cell
// whose summary row already shows its identity (issue #1684).
type VariantRowNesting = 'flat' | 'sp-child' | 'variant';

// Columns every variant of a cell shares (cellKey), left blank on 'variant' rows.
const CELL_IDENTITY_KEYS: ReadonlySet<state.RecommendationsColumnId> = new Set(
['provider', 'account', 'service', 'resource_type', 'region'] as state.RecommendationsColumnId[],
);

// Helper: render one variant row (a single LocalRecommendation) with optional
// indentation styling for multi-variant cells.
//
Expand All @@ -3331,7 +3349,7 @@ function renderColumnCell(key: state.RecommendationsColumnId, rec: LocalRecommen
function buildVariantRowMarkup(
rec: LocalRecommendation,
selectedRecs: ReadonlySet<string>,
isNested: boolean,
nesting: VariantRowNesting,
cols: readonly ColumnDef[] = COLUMN_DEFS,
showCheckboxes = true,
): string {
Expand All @@ -3347,7 +3365,7 @@ function buildVariantRowMarkup(
const pct = displaySavingsPct(rec);
const pctClass = pct !== null && pct < 0 ? ' class="num effective-pct-negative"' : ' class="num"';
const pctText = pct === null ? '\u2014' : pct.toFixed(1) + '%';
const nestedClass = isNested ? ' rec-variant-row' : '';
const nestedClass = nesting === 'flat' ? '' : ' rec-variant-row';
const cellCtx = { accountName, badge, pct, pctClass, pctText, period };
// Issue #869: viewer (readonly) sessions get no checkbox or action buttons
// in the leading column; the cell is still emitted so column widths align
Expand Down Expand Up @@ -3375,11 +3393,20 @@ function buildVariantRowMarkup(
return `
<tr class="recommendation-row${nestedClass} ${savingsClass} ${isSelected ? 'selected' : ''}" data-rec-id="${recId}">
${checkboxCell}
${cols.map((c) => renderColumnCell(c.key, rec, cellCtx)).join('')}
${cols.map((c) => (nesting === 'variant' && CELL_IDENTITY_KEYS.has(c.key)
? '<td class="rec-variant-identity"></td>'
: renderColumnCell(c.key, rec, cellCtx))).join('')}
</tr>`;
}


// Escaped "resource type (engine)" for a group summary row; empty when the
// provider sends no resource type (e.g. Azure savings plans).
function cellIdentityLabel(rec: LocalRecommendation): string {
if (!rec.resource_type) return '';
return `${escapeHtml(rec.resource_type)}${rec.engine ? ` (${escapeHtml(rec.engine)})` : ''}`;
}

function buildListMarkup(
recommendations: LocalRecommendation[],
selectedRecs: ReadonlySet<string>,
Expand Down Expand Up @@ -3475,7 +3502,7 @@ function buildListMarkup(
const spLabel = savingsPlansBucketLabel(spSlugs);
const planTypeCount = childCellKeys.length;
// Aggregate savings: sum the best-variant savings across all child cells.
const sfxLabel = periodSuffix(period);
const sfxLabel = periodUnit(period);
let spSavingsTotal = 0;
for (const ck of childCellKeys) {
const cv = groups.get(ck)!;
Expand Down Expand Up @@ -3506,7 +3533,7 @@ function buildListMarkup(
for (const ck of childCellKeys) {
const childVariants = groups.get(ck)!;
if (childVariants.length === 1) {
rows.push(buildVariantRowMarkup(childVariants[0]!, selectedRecs, true, visibleCols, showCheckboxes));
rows.push(buildVariantRowMarkup(childVariants[0]!, selectedRecs, 'sp-child', visibleCols, showCheckboxes));
continue;
}

Expand All @@ -3530,9 +3557,7 @@ function buildListMarkup(
? formatTermShort(selectedChildVariant.term) || NO_VALUE
: formatTermRange(childSummary.termMin, childSummary.termMax);
const cellChevron = iconSvg(isCellExpanded ? 'chevron-down' : 'chevron-right');
const childIdentityParts = [
`${escapeHtml(childRep.resource_type)}${childRep.engine ? ` (${escapeHtml(childRep.engine)})` : ''}`,
];
const childIdentityParts = [cellIdentityLabel(childRep)];
if (visibleKeys.has('region')) childIdentityParts.push(escapeHtml(childRep.region));
childIdentityParts.push(`${childVariants.length} variants`);
const childRangeParts: string[] = [];
Expand All @@ -3551,14 +3576,16 @@ function buildListMarkup(
<td>${escapeHtml(childAccountName)}</td>
<td><span class="service-badge">${escapeHtml(childRep.service)}</span></td>
<td colspan="${summaryColspan}" class="rec-cell-summary-content">
<span class="rec-cell-identity">${childIdentityParts.join(' &mdash; ')}</span>
${childRangeParts.length > 0 ? `<span class="rec-cell-range">${childRangeParts.join(' &middot; ')}</span>` : ''}
<div class="rec-cell-summary-inner">
<span class="rec-cell-identity">${childIdentityParts.filter(Boolean).join(' &mdash; ')}</span>
${childRangeParts.length > 0 ? `<span class="rec-cell-range">${childRangeParts.join(' &middot; ')}</span>` : ''}
</div>
</td>
</tr>`);
if (isCellExpanded) {
const sortedChildVariants = sortVariantsInCell(childVariants);
for (const v of sortedChildVariants) {
rows.push(buildVariantRowMarkup(v, selectedRecs, true, visibleCols, showCheckboxes));
rows.push(buildVariantRowMarkup(v, selectedRecs, 'variant', visibleCols, showCheckboxes));
}
}
}
Expand All @@ -3568,7 +3595,7 @@ function buildListMarkup(
// Non-SP cell (or SP singleton scope -- only one plan type in scope).
if (variants.length === 1) {
// Single-variant: render flat, no group header, no indent.
rows.push(buildVariantRowMarkup(variants[0]!, selectedRecs, false, visibleCols, showCheckboxes));
rows.push(buildVariantRowMarkup(variants[0]!, selectedRecs, 'flat', visibleCols, showCheckboxes));
continue;
}

Expand All @@ -3584,7 +3611,7 @@ function buildListMarkup(
// Summary row savings display: selected variant value if one is selected;
// otherwise the range across all variants.
// issue #319: scale savings by the active period.
const sfxLabel = periodSuffix(period);
const sfxLabel = periodUnit(period);
const scaledSavingsMin = scaleCost(summary.savingsMin, period) ?? summary.savingsMin;
const scaledSavingsMax = scaleCost(summary.savingsMax, period) ?? summary.savingsMax;
const savingsDisplay = selectedVariant
Expand All @@ -3600,9 +3627,7 @@ function buildListMarkup(
: formatTermRange(summary.termMin, summary.termMax);

const chevron = iconSvg(isExpanded ? 'chevron-down' : 'chevron-right');
const identityParts = [
`${escapeHtml(rep.resource_type)}${rep.engine ? ` (${escapeHtml(rep.engine)})` : ''}`,
];
const identityParts = [cellIdentityLabel(rep)];
if (visibleKeys.has('region')) {
identityParts.push(escapeHtml(rep.region));
}
Expand Down Expand Up @@ -3635,15 +3660,17 @@ function buildListMarkup(
<td>${escapeHtml(accountName)}</td>
<td><span class="service-badge">${escapeHtml(rep.service)}</span></td>
<td colspan="${summaryColspan}" class="rec-cell-summary-content">
<span class="rec-cell-identity">${identityParts.join(' &mdash; ')}</span>
${rangeParts.length > 0 ? `<span class="rec-cell-range">${rangeParts.join(' &middot; ')}</span>` : ''}
<div class="rec-cell-summary-inner">
<span class="rec-cell-identity">${identityParts.filter(Boolean).join(' &mdash; ')}</span>
${rangeParts.length > 0 ? `<span class="rec-cell-range">${rangeParts.join(' &middot; ')}</span>` : ''}
</div>
</td>
</tr>`);

if (isExpanded) {
const sortedVariants = sortVariantsInCell(variants);
for (const v of sortedVariants) {
rows.push(buildVariantRowMarkup(v, selectedRecs, true, visibleCols, showCheckboxes));
rows.push(buildVariantRowMarkup(v, selectedRecs, 'variant', visibleCols, showCheckboxes));
}
}
}
Expand Down Expand Up @@ -4226,16 +4253,16 @@ function updateBottomActionBox(visibleCount: number, loadedCount: number): void
} else if (visibleCount === 0) {
summary.textContent = '(0 visible — adjust filters)';
} else if (selectedVisibleCount === 0) {
summary.textContent = '(Select cells to act on)';
summary.textContent = '(Select rows to act on)';
} else if (targetRange.cellCount > 0) {
const savingsText = targetRange.savingsMax > 0
? formatSavingsRange(targetRange.savingsMin, targetRange.savingsMax)
: formatCurrency(0);
const upfrontText = targetRange.upfrontMax > 0
? formatSavingsRange(targetRange.upfrontMin, targetRange.upfrontMax)
: formatCurrency(0);
const cellWord = targetRange.cellCount === 1 ? 'cell' : 'cells';
summary.textContent = `(${savingsText}/mo · ${upfrontText} upfront across ${targetRange.cellCount} ${cellWord})`;
const rowWord = targetRange.cellCount === 1 ? 'row' : 'rows';
summary.textContent = `(${savingsText}/mo · ${upfrontText} upfront across ${targetRange.cellCount} ${rowWord})`;
} else {
// Shouldn't happen given the gating above, but defensive.
summary.textContent = `(${selectedVisibleCount} selected)`;
Expand All @@ -4250,7 +4277,7 @@ function updateBottomActionBox(visibleCount: number, loadedCount: number): void
? 'No recommendations loaded'
: visibleCount === 0
? 'No rows visible — adjust filters'
: 'Select at least one cell to enable';
: 'Select at least one row to enable';

// Compute the selected-visible slice once; both the plan-button gating and
// the hint span need it.
Expand Down
26 changes: 20 additions & 6 deletions frontend/src/styles/components.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -317,11 +317,19 @@ button.secondary:hover {
border: 1px solid var(--cudly-primary);
}

.btn-primary:hover {
.btn-primary:hover:not(:disabled) {
background: var(--cudly-primary-hover);
border-color: var(--cudly-primary-hover);
}

/* A faded primary fill still reads as enabled (issue #1684). */
.btn-primary:disabled {
background: var(--cudly-surface-muted);
border-color: var(--cudly-border-strong);
color: var(--cudly-text-subtle);
opacity: 1;
}

.btn-destructive {
background: var(--cudly-error);
color: white;
Expand Down Expand Up @@ -961,7 +969,7 @@ tr.recommendation-row:hover {
box-shadow: 0 4px 16px rgba(0,0,0,0.13);
padding: 0.6rem 0.75rem;
min-width: 180px;
max-height: 360px;
max-height: calc(100vh - 2rem);
overflow-y: auto;
}
.column-visibility-popover .column-visibility-title {
Expand Down Expand Up @@ -1018,8 +1026,10 @@ tr.rec-cell-summary-row:hover {
background: #eaf0fb;
}

/* Summary row content cell: chevron + identity + range inline */
.rec-cell-summary-content {
/* Summary row content: identity + range inline. The flex box is an inner
* wrapper because a flex <td> stops being a table cell and drops its
* colspan, leaving the spanned columns empty (issue #1684). */
.rec-cell-summary-inner {
display: flex;
align-items: center;
gap: 0.75rem;
Expand Down Expand Up @@ -1063,12 +1073,16 @@ tr.rec-cell-summary-row:hover {
font-size: 0.85em;
}

/* Variant rows nested under a cell summary (expanded state) */
/* Variant rows nested under a cell summary (expanded state). Only the first
* data cell is indented: padding every cell widened the table enough to
* scroll sideways (issue #1684). */
tr.rec-variant-row td:not(.checkbox-col) {
padding-left: 2rem;
background: #fafbfe;
font-size: 0.88em;
}
tr.rec-variant-row > td.checkbox-col + td {
padding-left: 2rem;
}
tr.rec-variant-row:hover td:not(.checkbox-col) {
background: #f0f4fc;
}
Expand Down
Loading
Loading