Skip to content

fix(ui): restore resource-row expand chevron to leading edge (closes #1006) #1446

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
Jul 17, 2026
Merged

fix(ui): restore resource-row expand chevron to leading edge (closes #1006) #1446

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
e43b989
fix(ui): restore resource-row expand chevron to leading edge (closes …
cristim Jul 17, 2026
21aad04
test(ui): strengthen readonly DOM contract assertions per CR #1446
cristim Jul 17, 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
36 changes: 27 additions & 9 deletions frontend/src/__tests__/recommendations-permissions.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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],
Expand All @@ -335,13 +337,24 @@ 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);
// 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 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 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<HTMLButtonElement>('.rec-cell-chevron');
expect(chevron).not.toBeNull();
expect(summaryRow!.querySelector('.rec-cell-summary-content .rec-cell-chevron')).toBeNull();
Comment thread
coderabbitai[bot] marked this conversation as resolved.

// Effective column count: sum of colspan values in each row must match header th count.
const headerColCount = table!.querySelectorAll('thead tr th').length;
Expand Down Expand Up @@ -501,7 +514,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();
Expand All @@ -510,10 +523,15 @@ 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);
// 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);
}
});
Expand Down
30 changes: 17 additions & 13 deletions frontend/src/recommendations.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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).
Expand All @@ -2930,7 +2929,7 @@ function buildVariantRowMarkup(
: '';
const checkboxCell = showCheckboxes
? `<td class="checkbox-col"><input type="checkbox" data-rec-id="${recId}" ${isSelected ? 'checked' : ''} aria-label="Select recommendation">${planBtnHtml}</td>`
: '';
: `<td class="checkbox-col"></td>`;
return `
<tr class="recommendation-row${nestedClass} ${savingsClass} ${isSelected ? 'selected' : ''}" data-rec-id="${recId}">
${checkboxCell}
Expand Down Expand Up @@ -3169,10 +3168,11 @@ function buildListMarkup(
const chevronButton = `<button type="button" class="rec-cell-chevron" data-cell-key="${escapeHtml(key)}" aria-expanded="${isExpanded}" aria-label="${isExpanded ? 'Collapse' : 'Expand'} cell variants">
${chevron}
</button>`;
const chevronCell = showCheckboxes
? `<td class="checkbox-col">${chevronButton}</td>`
: '';
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 = `<td class="checkbox-col">${chevronButton}</td>`;

rows.push(`
<tr class="rec-cell-summary-row" data-cell-key="${escapeHtml(key)}">
Expand All @@ -3181,7 +3181,7 @@ 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">${inlineChevron}${identityParts.join(' &mdash; ')}</span>
<span class="rec-cell-identity">${identityParts.join(' &mdash; ')}</span>
${rangeParts.length > 0 ? `<span class="rec-cell-range">${rangeParts.join(' &middot; ')}</span>` : ''}
</td>
</tr>`);
Expand All @@ -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));
Expand All @@ -3213,6 +3215,8 @@ function buildListMarkup(
const selectAllIndeterminate = selectedBestCount > 0 && selectedBestCount < bestVariants.length;
const selectAllDataIndeterminate = ` data-indeterminate="${selectAllIndeterminate ? 'true' : 'false'}"`;
checkboxColHeader = `<th class="checkbox-col"><input type="checkbox" id="select-all-recs" aria-label="Select all recommendations"${selectAllCheckedAttr}${selectAllDataIndeterminate}></th>`;
} else {
checkboxColHeader = `<th class="checkbox-col"></th>`;
}

return `
Expand Down Expand Up @@ -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));
Expand Down
Loading