From 986c0dcd6440f4a3def27114ef31af10afc129c4 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 25 May 2026 19:52:02 +0200 Subject: [PATCH] fix(ui/recommendations): preserve (All) tri-state and table header after Clear Closes #700 Two sub-bugs in the column filter popover and the zero-row render path: 1. Clear-categorical handler called checkboxes.forEach + setRecommendationsColumnFilter directly, bypassing commitAll(). That skipped updateAllTriState() so the (All) checkbox remained checked or indeterminate after Clear. Fix: expose commitAll as commitAllRef in the outer scope and delegate to it from the Clear handler, keeping the same call path as the (All) checkbox itself. 2. renderRecommendationsList replaced the entire container (including ) with a

when the filter produced zero rows. Fix: always call buildListMarkup so is preserved, then inject a hint into the empty via DOM methods. Tests: 2 new Issue #700 tests added (tri-state visual state + thead survival), existing empty-state test updated to match new structure. All 1933 frontend tests pass. --- .../src/__tests__/recommendations.test.ts | 68 ++++++++++++++++++- frontend/src/recommendations.ts | 50 ++++++++++---- 2 files changed, 105 insertions(+), 13 deletions(-) diff --git a/frontend/src/__tests__/recommendations.test.ts b/frontend/src/__tests__/recommendations.test.ts index e0ab8dbf8..650bbb7d1 100644 --- a/frontend/src/__tests__/recommendations.test.ts +++ b/frontend/src/__tests__/recommendations.test.ts @@ -587,6 +587,9 @@ describe('Recommendations Module', () => { }); test('shows empty-state message when no recommendations', async () => { + // Issue #700: zero rows now render an empty with a hint cell + // rather than replacing the entire table with a

. The stays + // so column headers remain visible. The hint text lives inside the tbody. (api.getRecommendations as jest.Mock).mockResolvedValue({ summary: {}, recommendations: [], @@ -596,7 +599,12 @@ describe('Recommendations Module', () => { await loadRecommendations(); const list = document.getElementById('recommendations-list'); - expect(list?.innerHTML).toContain('No recommendations match'); + // The table (including ) must still be rendered. + expect(list?.querySelector('thead')).not.toBeNull(); + // The hint cell must be present inside the tbody. + const emptyCell = list?.querySelector('tbody td.empty'); + expect(emptyCell).not.toBeNull(); + expect(emptyCell?.textContent).toMatch(/No rows match/); }); test('stores recommendations in state', async () => { @@ -2222,6 +2230,64 @@ describe('Bundle B: column header filter triggers', () => { expect(state.setRecommendationsColumnFilter).toHaveBeenCalledWith('provider', { kind: 'set', values: [] }); }); + test('Issue #700: Clear resets (All) checkbox to unchecked (not indeterminate)', async () => { + // Bug: the old Clear branch set cb.checked = false on individual boxes but + // never called updateAllTriState(), so the (All) checkbox kept its prior + // state (checked or indeterminate). After the fix, commitAllRef(false) is + // used which calls updateAllTriState() and leaves (All) unchecked. + // + // Simulate real state-store behaviour: setRecommendationsColumnFilter + // updates the store so the next getRecommendationsColumnFilters() call + // sees the cleared filter. Without this the resyncOpenPopover() call + // triggered by the rerender would re-apply the stale filter and overwrite + // the tri-state that updateAllTriState() just set. + (state.getRecommendationsColumnFilters as jest.Mock).mockReturnValue({ + provider: { kind: 'set', values: ['aws'] }, + }); + (state.setRecommendationsColumnFilter as jest.Mock).mockImplementation( + (col: string, val: unknown) => { + (state.getRecommendationsColumnFilters as jest.Mock).mockReturnValue( + val === null ? {} : { [col]: val }, + ); + }, + ); + await loadRecommendations(); + const providerBtn = document.querySelector('th .column-filter-btn[data-column="provider"]'); + providerBtn?.click(); + + // (All) starts in a known state before Clear. + const allBox = document.querySelector('.column-filter-popover .column-filter-all input[type="checkbox"]'); + expect(allBox).not.toBeNull(); + + const clearBtn = document.querySelector('.column-filter-popover .column-filter-clear'); + clearBtn?.click(); + + // After Clear: (All) must be unchecked and not indeterminate. + expect(allBox!.checked).toBe(false); + expect(allBox!.indeterminate).toBe(false); + expect(state.setRecommendationsColumnFilter).toHaveBeenCalledWith('provider', { kind: 'set', values: [] }); + }); + + test('Issue #700: table survives a filter that yields zero rows', async () => { + // Bug: renderRecommendationsList replaced the entire with a

+ // when no rows matched, removing

. After the fix, the header row + // remains visible (with an empty + hint cell) so columns are still + // readable while the user adjusts filters. + (state.getRecommendationsColumnFilters as jest.Mock).mockReturnValue({ + provider: { kind: 'set', values: [] }, + }); + await loadRecommendations(); + + const container = document.getElementById('recommendations-list'); + expect(container?.querySelector('thead')).not.toBeNull(); + // The empty hint cell must be present inside the table body. + const emptyCell = container?.querySelector('tbody td.empty'); + expect(emptyCell).not.toBeNull(); + expect(emptyCell?.textContent).toMatch(/No rows match/); + // No standalone

should replace the table. + expect(container?.querySelector('p.empty')).toBeNull(); + }); + test('Clear button on a numeric column clears the expression filter (null)', async () => { // Numeric columns still use null on Clear: the empty-set semantic // only applies to categorical filters because the set-membership diff --git a/frontend/src/recommendations.ts b/frontend/src/recommendations.ts index 1ab3c5ae2..657ab933e 100644 --- a/frontend/src/recommendations.ts +++ b/frontend/src/recommendations.ts @@ -1636,6 +1636,10 @@ function buildPopoverContent( const checkboxes = new Map(); let input: HTMLInputElement | null = null; let errorEl: HTMLElement | null = null; + // commitAllRef is set inside the categorical else-branch so the Clear + // button handler (which lives outside that branch) can call commitAll() + // and thereby invoke updateAllTriState() — fixing issue #700. + let commitAllRef: ((target: boolean) => void) | null = null; if (NUMERIC_COLUMNS.has(column)) { const label = document.createElement('label'); @@ -1794,6 +1798,8 @@ function buildPopoverContent( updateSPTriState(); rerenderRecommendations(); }; + // Expose commitAll to the Clear button handler outside this else-branch. + commitAllRef = commitAll; checkboxes.forEach((cb) => { cb.addEventListener('change', commit); @@ -1833,15 +1839,18 @@ function buildPopoverContent( state.setRecommendationsColumnFilter(column, null); input.value = ''; if (errorEl) errorEl.textContent = ''; + rerenderRecommendations(); } else { // Issue #482: Clear on a categorical filter sets an explicit empty // allow-list rather than null, so it's distinguishable from "no // filter applied" (which renders as all-checked). The popover's // checkboxes flip unchecked; the table renders 0 rows. - checkboxes.forEach((cb) => { cb.checked = false; }); - state.setRecommendationsColumnFilter(column, { kind: 'set', values: [] }); + // Issue #700: call commitAllRef(false) (the same as commitAll(false) + // inside the categorical branch) so updateAllTriState() is invoked and + // the (All) checkbox reflects the cleared state. commitAllRef also + // calls rerenderRecommendations() internally. + commitAllRef?.(false); } - rerenderRecommendations(); }); footer.appendChild(clearBtn); popover.appendChild(footer); @@ -3553,13 +3562,9 @@ function renderRecommendationsList(loadedRecs: LocalRecommendation[]): void { // Mount once; update is per-render below. mountBottomActionBox(); - if (!recommendations || recommendations.length === 0) { + const emptyResult = !recommendations || recommendations.length === 0; + if (emptyResult) { lastVisibleGroupKeys = []; - // Filter status bar renders even for the empty case (live-region count). - renderFilterStatusBar(loadedRecs?.length ?? 0, 0); - container.innerHTML = '

No recommendations match these filters. Try clearing filters or refreshing.

'; - updateBottomActionBox(0, loadedRecs?.length ?? 0); - return; } // Compute the visible column set once per render — passed to buildListMarkup @@ -3571,15 +3576,36 @@ function renderRecommendationsList(loadedRecs: LocalRecommendation[]): void { // escapeHtml or is a number. The string is built in buildListMarkup. // NOTE: buildListMarkup also populates lastVisibleGroupKeys, so it MUST // run before renderFilterStatusBar (which reads it for the Expand-All button). - container.innerHTML = buildListMarkup(recommendations, selectedIDs, visibleCols); + container.innerHTML = buildListMarkup(recommendations ?? [], selectedIDs, visibleCols); + + // Issue #700: when the filter yields zero rows, preserve the by + // injecting a hint row into the empty rather than replacing the + // entire table with a

. The column headers remain visible so the user + // can see which columns are active while they adjust filters. + if (emptyResult) { + const tbody = container.querySelector('tbody'); + if (tbody) { + // colspan = 1 (checkbox col) + all visible data columns. + const colspan = 1 + visibleCols.length; + const tr = document.createElement('tr'); + const td = document.createElement('td'); + td.setAttribute('colspan', String(colspan)); + td.className = 'empty'; + td.textContent = 'No rows match these filters.'; + tr.appendChild(td); + tbody.appendChild(tr); + } + } + + const visibleCount = recommendations?.length ?? 0; // Filter status: Clear-filters badge + aria-live count + Expand-All toggle. // Rendered AFTER buildListMarkup so lastVisibleGroupKeys is populated. // Mounted as a sibling above the table so it survives the container's // innerHTML rewrite without losing aria-live announcements. - renderFilterStatusBar(loadedRecs?.length ?? 0, recommendations.length); + renderFilterStatusBar(loadedRecs?.length ?? 0, visibleCount); - updateBottomActionBox(recommendations.length, loadedRecs?.length ?? recommendations.length); + updateBottomActionBox(visibleCount, loadedRecs?.length ?? visibleCount); // Per-column filter button: trigger opens the popover anchored to the