From e43b9892f3ddc281bbd077c20a9d4d44c00fea48 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 17 Jul 2026 16:03:12 +0300 Subject: [PATCH 1/2] fix(ui): restore resource-row expand chevron to leading edge (closes #1006) The expand/collapse chevron on multi-variant cell summary rows was rendered inline inside the content cell (before the Resource Type text) for readonly/viewer sessions because buildListMarkup zeroed out the leading td.checkbox-col when showCheckboxes was false. Restore the chevron to the far-left column (before Provider) for all roles by: - Always emitting td.checkbox-col as the first cell in cell summary rows (contains the chevron button; no change for admin/editor). - Always emitting an empty th.checkbox-col in the table header for viewers, keeping column alignment consistent. - Always emitting an empty td.checkbox-col in variant rows for viewers, so expanded row columns align with the header and summary rows. - Updating the zero-rows empty-hint colspan to always include 1 for the leading column. SP-group parent rows were already correct (they always emitted the leading cell). This change makes non-SP cell summary rows consistent. Owner decision: chevron belongs at the table's far-left edge, before the Provider column, matching the standard row-expander control placement (QA row 568, issue #1006). Tests updated: readonly grouped-row test now asserts the chevron lives in td.checkbox-col; SP-group variant-row test updated to expect one empty td.checkbox-col per row (no checkbox input). --- .../recommendations-permissions.test.ts | 26 ++++++++++------ frontend/src/recommendations.ts | 30 +++++++++++-------- 2 files changed, 34 insertions(+), 22 deletions(-) diff --git a/frontend/src/__tests__/recommendations-permissions.test.ts b/frontend/src/__tests__/recommendations-permissions.test.ts index 2a5233f1b..588507258 100644 --- a/frontend/src/__tests__/recommendations-permissions.test.ts +++ b/frontend/src/__tests__/recommendations-permissions.test.ts @@ -317,8 +317,10 @@ describe('Recommendations checkbox + row-click gating for viewer role (issue #86 expect(rowCheckboxes.length).toBe(0); }); - test('readonly role: grouped-row summary has no checkbox-col and column span is aligned', async () => { + test('readonly role: grouped-row summary has chevron in leading checkbox-col and column span is aligned (closes #1006)', async () => { // Two variants share the same cell key -- buildListMarkup renders a summary row. + // Issue #1006: the expand chevron must sit in td.checkbox-col at the far-left + // (before Provider), not inline inside the content cell. (api.getRecommendations as jest.Mock).mockResolvedValue({ summary: {}, recommendations: [sampleRecVariantA, sampleRecVariantB], @@ -335,13 +337,18 @@ describe('Recommendations checkbox + row-click gating for viewer role (issue #86 const summaryRow = table!.querySelector('tr.rec-cell-summary-row'); expect(summaryRow).not.toBeNull(); - // Header must not contain a checkbox-col th. + // Header must have a leading th.checkbox-col (empty for viewers, aligns the chevron column). const headerCheckboxCols = table!.querySelectorAll('thead tr th.checkbox-col'); - expect(headerCheckboxCols.length).toBe(0); + expect(headerCheckboxCols.length).toBe(1); - // Summary row must not contain a checkbox-col td (the bug this PR fixes). + // Summary row must have a td.checkbox-col at the far-left containing the chevron button. const summaryCheckboxCols = summaryRow!.querySelectorAll('td.checkbox-col'); - expect(summaryCheckboxCols.length).toBe(0); + expect(summaryCheckboxCols.length).toBe(1); + + // The chevron button lives inside the leading td.checkbox-col, not inline in the content. + const chevron = summaryCheckboxCols[0]!.querySelector('.rec-cell-chevron'); + expect(chevron).not.toBeNull(); + expect(summaryRow!.querySelector('.rec-cell-summary-content .rec-cell-chevron')).toBeNull(); // Effective column count: sum of colspan values in each row must match header th count. const headerColCount = table!.querySelectorAll('thead tr th').length; @@ -501,7 +508,7 @@ describe('Recommendations SP-group child-row checkbox gating (issue #135 + #869) return list!.querySelector('table') as HTMLTableElement; }; - test('readonly role: expanded SP-group child variant rows have no checkbox-col', async () => { + test('readonly role: expanded SP-group child variant rows have an empty leading checkbox-col but no checkbox input', async () => { mockUser('readonly'); await loadRecommendations(); const table = expandSpGroup(); @@ -510,10 +517,11 @@ describe('Recommendations SP-group child-row checkbox gating (issue #135 + #869) const childRows = table.querySelectorAll('tr.rec-variant-row'); expect(childRows.length).toBeGreaterThan(0); - // The bug this guards: child rows must not render a checkbox cell for - // readonly sessions (showCheckboxes must be threaded into the nested call). + // Issue #1006: variant rows carry an empty td.checkbox-col so columns + // stay aligned with the leading cell in the summary/header rows. No + // checkbox input or action buttons appear for readonly sessions. for (const row of Array.from(childRows)) { - expect(row.querySelectorAll('td.checkbox-col').length).toBe(0); + expect(row.querySelectorAll('td.checkbox-col').length).toBe(1); expect(row.querySelectorAll('input[data-rec-id]').length).toBe(0); } }); diff --git a/frontend/src/recommendations.ts b/frontend/src/recommendations.ts index d52b76cf7..6cf8b6f46 100644 --- a/frontend/src/recommendations.ts +++ b/frontend/src/recommendations.ts @@ -2918,10 +2918,9 @@ function buildVariantRowMarkup( const pctText = pct === null ? '\u2014' : pct.toFixed(1) + '%'; const nestedClass = isNested ? ' rec-variant-row' : ''; const cellCtx = { accountName, badge, pct, pctClass, pctText, period }; - // Issue #869: omit the checkbox cell entirely for viewer (readonly) sessions. - // The column header also omits the select-all checkbox, so the column is - // visually absent rather than present-but-empty, matching the no-actions - // experience on Plans and Purchases for the same role. + // Issue #869: viewer (readonly) sessions get no checkbox or action buttons + // in the leading column; the cell is still emitted so column widths align + // with the cell-summary rows (which always carry the expand chevron there). // Issue #120: inline "Plan" button deep-links into the Create Purchase Plan // modal pre-seeded with this rec. Only rendered when the session has // create:plans permission (mirrors the bulk Create Plan button gate). @@ -2930,7 +2929,7 @@ function buildVariantRowMarkup( : ''; const checkboxCell = showCheckboxes ? `${planBtnHtml}` - : ''; + : ``; return ` ${checkboxCell} @@ -3169,10 +3168,11 @@ function buildListMarkup( const chevronButton = ``; - const chevronCell = showCheckboxes - ? `${chevronButton}` - : ''; - const inlineChevron = showCheckboxes ? '' : `${chevronButton} `; + // Issue #1006: the expand chevron always lives in the leading td.checkbox-col, + // before the Provider column, for all roles including readonly viewers. + // This matches the SP-group rows (which always emitted a leading cell) and + // the owner decision to keep the control at the table's far-left edge. + const chevronCell = `${chevronButton}`; rows.push(` @@ -3181,7 +3181,7 @@ function buildListMarkup( ${escapeHtml(accountName)} ${escapeHtml(rep.service)} - ${inlineChevron}${identityParts.join(' — ')} + ${identityParts.join(' — ')} ${rangeParts.length > 0 ? `${rangeParts.join(' · ')}` : ''} `); @@ -3202,7 +3202,9 @@ function buildListMarkup( // can't express the indeterminate state. // Issue #869: skip the tri-state computation entirely for viewer sessions // to avoid dead-code paths when showCheckboxes is false. - let checkboxColHeader = ''; + // Issue #1006: the leading th.checkbox-col is always present (empty for viewers) + // so the column aligns with the chevron cells in every row type. + let checkboxColHeader: string; if (showCheckboxes) { const bestVariants = pickBestVariantPerCell(recommendations); const bestVariantIds = new Set(bestVariants.map((r) => r.id)); @@ -3213,6 +3215,8 @@ function buildListMarkup( const selectAllIndeterminate = selectedBestCount > 0 && selectedBestCount < bestVariants.length; const selectAllDataIndeterminate = ` data-indeterminate="${selectAllIndeterminate ? 'true' : 'false'}"`; checkboxColHeader = ``; + } else { + checkboxColHeader = ``; } return ` @@ -4534,8 +4538,8 @@ function renderRecommendationsList(loadedRecs: LocalRecommendation[]): void { if (emptyResult) { const tbody = container.querySelector('tbody'); if (tbody) { - // colspan = checkbox col (1 when shown, 0 when hidden) + all visible data columns. - const colspan = (showCheckboxes ? 1 : 0) + visibleCols.length; + // colspan = leading col (always 1: checkbox for editors, empty for viewers) + all visible data columns. + const colspan = 1 + visibleCols.length; const tr = document.createElement('tr'); const td = document.createElement('td'); td.setAttribute('colspan', String(colspan)); From 21aad047f71add86dbacd93158969ef4b89f3692 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 17 Jul 2026 16:37:37 +0300 Subject: [PATCH 2/2] test(ui): strengthen readonly DOM contract assertions per CR #1446 Assert that th.checkbox-col and td.checkbox-col are the first cells in their respective rows (not just present), and that variant rows' leading td.checkbox-col has empty text content. Addresses CodeRabbit finding on PR #1446 (actionable comment, minor). --- .../src/__tests__/recommendations-permissions.test.ts | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/frontend/src/__tests__/recommendations-permissions.test.ts b/frontend/src/__tests__/recommendations-permissions.test.ts index 588507258..88291b208 100644 --- a/frontend/src/__tests__/recommendations-permissions.test.ts +++ b/frontend/src/__tests__/recommendations-permissions.test.ts @@ -340,10 +340,16 @@ describe('Recommendations checkbox + row-click gating for viewer role (issue #86 // Header must have a leading th.checkbox-col (empty for viewers, aligns the chevron column). const headerCheckboxCols = table!.querySelectorAll('thead tr th.checkbox-col'); expect(headerCheckboxCols.length).toBe(1); + // The leading checkbox-col must be the FIRST header cell (position check). + const firstHeaderTh = table!.querySelector('thead tr th:first-child'); + expect(firstHeaderTh?.classList.contains('checkbox-col')).toBe(true); // Summary row must have a td.checkbox-col at the far-left containing the chevron button. const summaryCheckboxCols = summaryRow!.querySelectorAll('td.checkbox-col'); expect(summaryCheckboxCols.length).toBe(1); + // The leading checkbox-col must be the FIRST summary cell (position check). + const firstSummaryTd = summaryRow!.querySelector('td:first-child'); + expect(firstSummaryTd?.classList.contains('checkbox-col')).toBe(true); // The chevron button lives inside the leading td.checkbox-col, not inline in the content. const chevron = summaryCheckboxCols[0]!.querySelector('.rec-cell-chevron'); @@ -522,6 +528,10 @@ describe('Recommendations SP-group child-row checkbox gating (issue #135 + #869) // checkbox input or action buttons appear for readonly sessions. for (const row of Array.from(childRows)) { expect(row.querySelectorAll('td.checkbox-col').length).toBe(1); + // The checkbox-col must be the FIRST cell and contain no text (position + empty-content check). + const firstTd = row.querySelector('td:first-child'); + expect(firstTd?.classList.contains('checkbox-col')).toBe(true); + expect(firstTd?.textContent?.trim()).toBe(''); expect(row.querySelectorAll('input[data-rec-id]').length).toBe(0); } });