From c3ef8d150b661d0858ffaf9e9cc0f60107ebf093 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 4 Nov 2026 11:54:25 +0100 Subject: [PATCH 1/2] fix(frontend): Opportunities group rows, payback range and action bar wording MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Group summary rows span their columns again: the flex layout moves to an inner wrapper, since a flex drops its colspan. - Expanded variant rows leave the columns the group shares (provider, account, service, resource type, region) blank and indent only the first data cell, so expanding no longer adds a horizontal scroll or shifts the header. - Group summaries skip a blank resource type or region (Azure savings plans read "2 variants", not "— — 2 variants") and print "$20/mo". - Payback Period reads "Up to X months" when the cheapest end has no upfront, instead of a "0.0" lower bound. - The Columns popover grows to the viewport height before it scrolls. - Disabled primary buttons drop the primary fill. - The action bar says rows instead of cells. Closes #1684 Co-Authored-By: Claude Opus 5.5 --- .../src/__tests__/recommendations.test.ts | 81 +++++++++++++- frontend/src/recommendations.ts | 77 ++++++++----- frontend/src/styles/components.css | 26 +++-- .../opportunities-group-layout.spec.ts | 102 ++++++++++++++++++ frontend/tests-e2e/recommendations.spec.ts | 2 +- 5 files changed, 251 insertions(+), 37 deletions(-) create mode 100644 frontend/tests-e2e/opportunities-group-layout.spec.ts diff --git a/frontend/src/__tests__/recommendations.test.ts b/frontend/src/__tests__/recommendations.test.ts index 8a2cd3f2a..2964a2b51 100644 --- a/frontend/src/__tests__/recommendations.test.ts +++ b/frontend/src/__tests__/recommendations.test.ts @@ -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(); }); @@ -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 () => { @@ -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 () => { @@ -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(); }); @@ -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>, selected: string[] = []): Promise { + (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('tr.rec-cell-summary-row .rec-cell-chevron'); + if (chevron?.getAttribute('aria-expanded') !== 'true') chevron!.click(); + const row = document.querySelector('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 @@ -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 diff --git a/frontend/src/recommendations.ts b/frontend/src/recommendations.ts index 6452527f3..e994322b2 100644 --- a/frontend/src/recommendations.ts +++ b/frontend/src/recommendations.ts @@ -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". */ @@ -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). @@ -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 = 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. // @@ -3331,7 +3349,7 @@ function renderColumnCell(key: state.RecommendationsColumnId, rec: LocalRecommen function buildVariantRowMarkup( rec: LocalRecommendation, selectedRecs: ReadonlySet, - isNested: boolean, + nesting: VariantRowNesting, cols: readonly ColumnDef[] = COLUMN_DEFS, showCheckboxes = true, ): string { @@ -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 @@ -3375,11 +3393,20 @@ function buildVariantRowMarkup( return ` ${checkboxCell} - ${cols.map((c) => renderColumnCell(c.key, rec, cellCtx)).join('')} + ${cols.map((c) => (nesting === 'variant' && CELL_IDENTITY_KEYS.has(c.key) + ? '' + : renderColumnCell(c.key, rec, cellCtx))).join('')} `; } +// 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, @@ -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)!; @@ -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; } @@ -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[] = []; @@ -3551,14 +3576,16 @@ function buildListMarkup( ${escapeHtml(childAccountName)} ${escapeHtml(childRep.service)} - ${childIdentityParts.join(' — ')} - ${childRangeParts.length > 0 ? `${childRangeParts.join(' · ')}` : ''} +
+ ${childIdentityParts.filter(Boolean).join(' — ')} + ${childRangeParts.length > 0 ? `${childRangeParts.join(' · ')}` : ''} +
`); 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)); } } } @@ -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; } @@ -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 @@ -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)); } @@ -3635,15 +3660,17 @@ function buildListMarkup( ${escapeHtml(accountName)} ${escapeHtml(rep.service)} - ${identityParts.join(' — ')} - ${rangeParts.length > 0 ? `${rangeParts.join(' · ')}` : ''} +
+ ${identityParts.filter(Boolean).join(' — ')} + ${rangeParts.length > 0 ? `${rangeParts.join(' · ')}` : ''} +
`); 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)); } } } @@ -4226,7 +4253,7 @@ 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) @@ -4234,8 +4261,8 @@ function updateBottomActionBox(visibleCount: number, loadedCount: number): void 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)`; @@ -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. diff --git a/frontend/src/styles/components.css b/frontend/src/styles/components.css index b38adc2cd..0e45af1f2 100644 --- a/frontend/src/styles/components.css +++ b/frontend/src/styles/components.css @@ -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; @@ -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 { @@ -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 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; @@ -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; } diff --git a/frontend/tests-e2e/opportunities-group-layout.spec.ts b/frontend/tests-e2e/opportunities-group-layout.spec.ts new file mode 100644 index 000000000..8fbaee207 --- /dev/null +++ b/frontend/tests-e2e/opportunities-group-layout.spec.ts @@ -0,0 +1,102 @@ +/** + * Opportunities multi-variant group layout (issue #1684). + * + * jsdom has no layout, so the colspan, horizontal-scroll, popover-height and + * disabled-button checks run in Chromium against the built bundle. + */ + +import { expect, test, type Page } from '@playwright/test'; + +import { mockApi, RECS, seedAuth } from './fixtures/recs'; + +const GROUP_BASE = { + provider: 'aws', cloud_account_id: 'acct-001', service: 'rds', resource_type: 'db.r6g.2xlarge', + engine: 'postgresql', region: 'eu-west-1', count: 2, monthly_cost: 300, on_demand_monthly: 500, +}; +const GROUP = [ + { ...GROUP_BASE, id: 'g-1-nu', term: 1, payment: 'no-upfront', savings: 120, upfront_cost: 0, effective_savings_pct: 24 }, + { ...GROUP_BASE, id: 'g-1-au', term: 1, payment: 'all-upfront', savings: 160, upfront_cost: 1800, effective_savings_pct: 32 }, + { ...GROUP_BASE, id: 'g-3-pu', term: 3, payment: 'partial-upfront', savings: 210, upfront_cost: 3600, effective_savings_pct: 42 }, + { ...GROUP_BASE, id: 'g-3-au', term: 3, payment: 'all-upfront', savings: 230, upfront_cost: 7000, effective_savings_pct: 46 }, +]; +const ROWS = [...GROUP, ...RECS]; + +async function openOpportunities(page: Page): Promise { + await page.setViewportSize({ width: 1920, height: 1080 }); + await seedAuth(page); + await mockApi(page); + await page.route('**/api/recommendations*', (route) => + route.fulfill({ json: { recommendations: ROWS, summary: {}, regions: [] } })); + await page.goto('/opportunities'); + await expect(page.locator('.recommendations-filter-live')).toContainText( + `Showing ${ROWS.length} of ${ROWS.length} recommendations`, + ); +} + +const groupRow = (page: Page) => page.locator('tr.rec-cell-summary-row[data-cell-key*="db.r6g.2xlarge"]'); + +test('collapsed group summary spans every column after Service', async ({ page }) => { + await openOpportunities(page); + const geometry = await groupRow(page).evaluate((row) => { + const td = row.querySelector('td.rec-cell-summary-content')!; + const ths = row.closest('table')!.querySelectorAll('thead th'); + return { + display: getComputedStyle(td).display, + tdRight: td.getBoundingClientRect().right, + tableRight: ths[ths.length - 1]!.getBoundingClientRect().right, + }; + }); + expect(geometry.display).toBe('table-cell'); + expect(Math.abs(geometry.tdRight - geometry.tableRight)).toBeLessThanOrEqual(1); +}); + +test('expanding a group adds no horizontal scroll and keeps the header still', async ({ page }) => { + await openOpportunities(page); + const headerLefts = () => page.locator('#recommendations-list thead th').evaluateAll( + (ths) => ths.map((th) => Math.round(th.getBoundingClientRect().left)), + ); + const before = await headerLefts(); + await groupRow(page).locator('.rec-cell-chevron').click(); + await expect(page.locator('tr.rec-variant-row')).toHaveCount(GROUP.length); + + const after = await headerLefts(); + after.forEach((left, i) => expect(Math.abs(left - before[i]!)).toBeLessThanOrEqual(2)); + + const overflow = await page.locator('#recommendations-list table').evaluate((table) => { + const out: string[] = []; + for (let el: HTMLElement | null = table.parentElement; el; el = el.parentElement) { + const ox = getComputedStyle(el).overflowX; + if ((ox === 'auto' || ox === 'scroll') && el.scrollWidth > el.clientWidth + 1) { + out.push(`${el.tagName}#${el.id}.${el.className}: ${el.scrollWidth} > ${el.clientWidth}`); + } + } + const doc = document.documentElement; + if (doc.scrollWidth > doc.clientWidth + 1) out.push(`document: ${doc.scrollWidth} > ${doc.clientWidth}`); + return out; + }); + expect(overflow).toEqual([]); +}); + +test('Columns popover lists every option without an inner scrollbar', async ({ page }) => { + await openOpportunities(page); + await page.locator('.column-visibility-btn').click(); + const popover = page.getByRole('dialog', { name: 'Show/hide columns' }); + await expect(popover).toBeVisible(); + const { scrollHeight, clientHeight } = await popover.evaluate((el) => ({ + scrollHeight: el.scrollHeight, clientHeight: el.clientHeight, + })); + expect(scrollHeight).toBeLessThanOrEqual(clientHeight); +}); + +test('disabled Purchase button does not keep the primary fill', async ({ page }) => { + await openOpportunities(page); + const purchase = page.locator('#bulk-purchase-btn'); + await expect(purchase).toBeDisabled(); + const disabledBg = await purchase.evaluate((el) => getComputedStyle(el).backgroundColor); + + await page.locator('tr.recommendation-row[data-rec-id="r01"]') + .getByRole('checkbox', { name: /^Select / }).check(); + await expect(purchase).toBeEnabled(); + const enabledBg = await purchase.evaluate((el) => getComputedStyle(el).backgroundColor); + expect(disabledBg).not.toBe(enabledBg); +}); diff --git a/frontend/tests-e2e/recommendations.spec.ts b/frontend/tests-e2e/recommendations.spec.ts index f3456fe27..10537d8aa 100644 --- a/frontend/tests-e2e/recommendations.spec.ts +++ b/frontend/tests-e2e/recommendations.spec.ts @@ -76,7 +76,7 @@ test('selection updates the sticky action summary and submits the selected purch await selectRecommendation(page, selected.id); await expect(page.locator('.recommendations-filter-live')).toContainText('1 selected'); - await expect(page.locator('#recommendations-action-summary')).toContainText('across 1 cell'); + await expect(page.locator('#recommendations-action-summary')).toContainText('across 1 row'); await page.getByRole('button', { name: 'Purchase 1 selected' }).click(); await expect(page.getByRole('dialog', { name: 'Review Purchase' })).toBeVisible(); await page.getByRole('button', { name: 'Send for Approval' }).click(); From 24ae940f8d9f82ab59d22fe46ef46dcd4aae478f Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 4 Nov 2026 12:33:39 +0100 Subject: [PATCH 2/2] test(frontend): scroll the affordance check to the table's real midpoint The Opportunities table at 1280px now overflows by about 41px, because variant rows no longer pad every cell, so a fixed 120px scroll never reached the strip. Scroll to half of the actual overflow instead. Co-Authored-By: Claude Opus 5.5 --- frontend/tests-e2e/table-scroll-affordance.spec.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/frontend/tests-e2e/table-scroll-affordance.spec.ts b/frontend/tests-e2e/table-scroll-affordance.spec.ts index 0b07ad3f9..bd2c4f356 100644 --- a/frontend/tests-e2e/table-scroll-affordance.spec.ts +++ b/frontend/tests-e2e/table-scroll-affordance.spec.ts @@ -69,8 +69,10 @@ for (const width of [1280, 390]) { expect(barBox.y).toBeGreaterThan(0); // Scrolling the strip scrolls the table, and both edges fade mid-way. - await bar.evaluate((el) => { el.scrollLeft = 120; }); - await expect.poll(() => tbl.evaluate((el) => el.scrollLeft)).toBe(120); + const mid = await tbl.evaluate((el) => Math.floor((el.scrollWidth - el.clientWidth) / 2)); + expect(mid).toBeGreaterThan(0); + await bar.evaluate((el, x) => { el.scrollLeft = x; }, mid); + await expect.poll(() => tbl.evaluate((el) => el.scrollLeft)).toBe(mid); await expect(tbl).toHaveClass(/scroll-hint-left/); await expect(tbl).toHaveClass(/scroll-hint-right/);