diff --git a/frontend/src/__tests__/column-filters.test.ts b/frontend/src/__tests__/column-filters.test.ts new file mode 100644 index 000000000..eacd743bc --- /dev/null +++ b/frontend/src/__tests__/column-filters.test.ts @@ -0,0 +1,133 @@ +/** + * Unit tests for the shared column-filter lib (issue #166). + * + * parseNumericFilter is the core utility extracted from recommendations.ts. + * applyColumnFilters is the generic version that any tab can use. + * recommendations.ts continues to re-export ParsedNumericFilter for backward + * compat with existing consumers. + */ +import { parseNumericFilter, applyColumnFilters } from '../lib/column-filters'; + +// --------------------------------------------------------------------------- +// parseNumericFilter +// --------------------------------------------------------------------------- + +describe('parseNumericFilter (lib)', () => { + const accept = (expr: string, n: number): boolean => { + const r = parseNumericFilter(expr); + if (!r.ok) throw new Error(`unexpected parse failure for "${expr}": ${r.error}`); + return r.predicate(n); + }; + + test('empty / blank returns match-all', () => { + const r = parseNumericFilter(''); + expect(r.ok).toBe(true); + if (r.ok) expect(r.predicate(0)).toBe(true); + expect(parseNumericFilter(' ').ok).toBe(true); + }); + + test('plain number: exact equality', () => { + expect(accept('42', 42)).toBe(true); + expect(accept('42', 43)).toBe(false); + expect(accept('-5', -5)).toBe(true); + expect(accept('3.14', 3.14)).toBe(true); + expect(accept('3.14', 3.15)).toBe(false); + }); + + test('comparators >, >=, <, <=', () => { + expect(accept('>10', 11)).toBe(true); + expect(accept('>10', 10)).toBe(false); + expect(accept('>=10', 10)).toBe(true); + expect(accept('<5', 4)).toBe(true); + expect(accept('<5', 5)).toBe(false); + expect(accept('<=5', 5)).toBe(true); + }); + + test('inclusive range X..Y (order-independent)', () => { + expect(accept('10..20', 10)).toBe(true); + expect(accept('10..20', 20)).toBe(true); + expect(accept('10..20', 15)).toBe(true); + expect(accept('10..20', 9)).toBe(false); + expect(accept('20..10', 15)).toBe(true); + }); + + test('comma-separated terms OR together', () => { + expect(accept('5, >100', 5)).toBe(true); + expect(accept('5, >100', 150)).toBe(true); + expect(accept('5, >100', 50)).toBe(false); + }); + + test('invalid expression returns ok:false', () => { + const r1 = parseNumericFilter('>>5'); + expect(r1.ok).toBe(false); + if (!r1.ok) expect(r1.error).toMatch(/Invalid filter term/); + expect(parseNumericFilter('not-a-number').ok).toBe(false); + expect(parseNumericFilter('1..').ok).toBe(false); + }); +}); + +// --------------------------------------------------------------------------- +// applyColumnFilters (generic) +// --------------------------------------------------------------------------- + +type Row = { id: string; service: string; savings: number }; +type Col = 'service' | 'savings'; + +const rows: Row[] = [ + { id: 'a', service: 'ec2', savings: 10 }, + { id: 'b', service: 'rds', savings: 200 }, + { id: 'c', service: 'ec2', savings: 500 }, +]; + +const extractors = { + categorical: (r: Row, col: Col) => (col === 'service' ? r.service : String(r.savings)), + numeric: (r: Row, _col: Col) => r.savings, +}; + +describe('applyColumnFilters (lib)', () => { + test('empty filters returns a clone of the input', () => { + const out = applyColumnFilters(rows, {}, extractors); + expect(out).toEqual(rows); + expect(out).not.toBe(rows); + }); + + test('categorical set filter narrows by membership', () => { + const out = applyColumnFilters( + rows, + { service: { kind: 'set', values: ['ec2'] } }, + extractors, + ); + expect(out.map((r) => r.id)).toEqual(['a', 'c']); + }); + + test('numeric expr filter narrows by predicate', () => { + const out = applyColumnFilters( + rows, + { savings: { kind: 'expr', expr: '>100' } }, + extractors, + ); + expect(out.map((r) => r.id)).toEqual(['b', 'c']); + }); + + test('multiple filters AND together', () => { + const out = applyColumnFilters( + rows, + { + service: { kind: 'set', values: ['ec2'] }, + savings: { kind: 'expr', expr: '>100' }, + }, + extractors, + ); + expect(out.map((r) => r.id)).toEqual(['c']); + }); + + test('broken numeric expr is skipped (not treated as match-none)', () => { + const out = applyColumnFilters( + rows, + { savings: { kind: 'expr', expr: '>>invalid' } }, + extractors, + ); + // Parse fails -> filter is skipped -> all rows pass + expect(out).toHaveLength(rows.length); + }); +}); diff --git a/frontend/src/lib/column-filters.ts b/frontend/src/lib/column-filters.ts new file mode 100644 index 000000000..35fd08f98 --- /dev/null +++ b/frontend/src/lib/column-filters.ts @@ -0,0 +1,127 @@ +/** + * Shared column-filter primitives (issue #166). + * + * Extracted from recommendations.ts so Plans, History, and RI Exchange + * can reuse the same numeric-filter parser without copy-pasting it. + * + * Each consuming tab owns its own column-id type and filter record (keeping + * column ids type-safe per tab), but the low-level parser and the generic + * apply pipeline live here. + */ + +// --------------------------------------------------------------------------- +// Numeric filter parser +// --------------------------------------------------------------------------- + +export type ParsedNumericFilter = + | { ok: true; predicate: (n: number) => boolean } + | { ok: false; error: string }; + +const MATCH_ALL: ParsedNumericFilter = { ok: true, predicate: () => true }; + +/** + * Parse a numeric filter expression such as ">= 100", "< 50", "10..20", + * or a comma-separated OR list of those. Returns a predicate function on + * success, or an error object on parse failure so callers can surface inline + * validation messages. + * + * Supported syntax (case-insensitive, whitespace-tolerant): + * - `>=N` / `<=N` / `>N` / ` t.trim()).filter((t) => t !== ''); + if (terms.length === 0) return MATCH_ALL; + + const predicates: Array<(n: number) => boolean> = []; + for (const term of terms) { + // Order matters: ">=" / "<=" must be checked before ">" / "<". + let p: ((n: number) => boolean) | null = null; + let m: RegExpMatchArray | null; + if ((m = term.match(/^>=\s*(-?\d+(?:\.\d+)?)$/))) { + const v = Number(m[1]); + p = (n) => n >= v; + } else if ((m = term.match(/^<=\s*(-?\d+(?:\.\d+)?)$/))) { + const v = Number(m[1]); + p = (n) => n <= v; + } else if ((m = term.match(/^>\s*(-?\d+(?:\.\d+)?)$/))) { + const v = Number(m[1]); + p = (n) => n > v; + } else if ((m = term.match(/^<\s*(-?\d+(?:\.\d+)?)$/))) { + const v = Number(m[1]); + p = (n) => n < v; + } else if ((m = term.match(/^(-?\d+(?:\.\d+)?)\s*\.\.\s*(-?\d+(?:\.\d+)?)$/))) { + const lo = Number(m[1]); + const hi = Number(m[2]); + const min = Math.min(lo, hi); + const max = Math.max(lo, hi); + p = (n) => n >= min && n <= max; + } else if ((m = term.match(/^(-?\d+(?:\.\d+)?)$/))) { + const v = Number(m[1]); + p = (n) => n === v; + } + if (p === null) { + return { ok: false, error: `Invalid filter term: "${term}"` }; + } + predicates.push(p); + } + // OR across terms + return { + ok: true, + predicate: (n) => predicates.some((p) => p(n)), + }; +} + +// --------------------------------------------------------------------------- +// Generic filter types +// --------------------------------------------------------------------------- + +export type ColumnFilterKind = + | { kind: 'set'; values: string[] } + | { kind: 'expr'; expr: string }; + +/** + * Apply a column-filter record to a list of rows. Each column maps to either + * a categorical set-filter or a numeric expression filter. All active filters + * are ANDed together. + * + * @param rows - The full list of rows to filter. + * @param filters - A partial record mapping column ids to their active filter. + * @param cellExtractors - Per-column functions that return the raw cell value + * for a given row. Categorical columns return a string; numeric columns + * return a number. + * + * Returns a new array containing only rows that pass all active filters. + * Broken numeric expressions (parse failure) are skipped so the UI can show + * an inline validation error without forcing the user to clear the field first. + */ +export function applyColumnFilters( + rows: readonly TRow[], + filters: Partial>, + cellExtractors: { + categorical: (row: TRow, col: TColumnId) => string; + numeric: (row: TRow, col: TColumnId) => number; + }, +): TRow[] { + const entries = Object.entries(filters) as Array<[TColumnId, ColumnFilterKind]>; + if (entries.length === 0) return [...rows]; + + return rows.filter((row) => { + for (const [col, filter] of entries) { + if (filter.kind === 'set') { + const cellValue = cellExtractors.categorical(row, col); + if (!filter.values.includes(cellValue)) return false; + } else { + const parsed = parseNumericFilter(filter.expr); + if (!parsed.ok) continue; // ignore broken expressions; UI shows the error + const cellNum = cellExtractors.numeric(row, col); + if (!parsed.predicate(cellNum)) return false; + } + } + return true; + }); +} diff --git a/frontend/src/recommendations.ts b/frontend/src/recommendations.ts index 8f977cb3c..6bd3414ba 100644 --- a/frontend/src/recommendations.ts +++ b/frontend/src/recommendations.ts @@ -22,6 +22,11 @@ import type { RecommendationsResponse, LocalRecommendation, RecommendationsSumma import { openModal } from './modal'; import { showSkeletonRows, teardownSkeleton } from './lib/skeleton'; import { canAccess } from './permissions'; +import { parseNumericFilter, applyColumnFilters as applyColumnFiltersLib } from './lib/column-filters'; +// Re-export the shared primitives so existing consumers that import from +// recommendations.ts keep working without import-path churn (issue #166). +export { parseNumericFilter } from './lib/column-filters'; +export type { ParsedNumericFilter } from './lib/column-filters'; // Module state for current purchase modal recommendations let currentPurchaseRecommendations: LocalRecommendation[] = []; @@ -1305,101 +1310,38 @@ function sortIndicator(column: string, active: string, direction: 'asc' | 'desc' // via groupsInSortOrder() supersedes the flat-list sort. The same // SORTABLE_NUMERIC_COLUMNS / SORTABLE_STRING_COLUMNS maps are reused there. -// Numeric filter expression parser. Grammar: -// - empty/whitespace -> match-all -// - "42" -> equals -// - ">X" / "=X" / "<=X" -> comparator -// - "X..Y" -> inclusive range (X and Y both numbers) -// - comma-separated -> OR of any of the above -// Returns a discriminated union so callers can render parse errors -// inline without type-narrowing gymnastics. Whitespace inside terms is -// trimmed; whitespace between terms is allowed. -export type ParsedNumericFilter = - | { ok: true; predicate: (n: number) => boolean } - | { ok: false; error: string }; - -const MATCH_ALL: ParsedNumericFilter = { ok: true, predicate: () => true }; - -export function parseNumericFilter(expr: string): ParsedNumericFilter { - if (!expr || expr.trim() === '') return MATCH_ALL; - const terms = expr.split(',').map((t) => t.trim()).filter((t) => t !== ''); - if (terms.length === 0) return MATCH_ALL; - - const predicates: Array<(n: number) => boolean> = []; - for (const term of terms) { - // Order matters: ">=" / "<=" must be checked before ">" / "<". - let p: ((n: number) => boolean) | null = null; - let m: RegExpMatchArray | null; - if ((m = term.match(/^>=\s*(-?\d+(?:\.\d+)?)$/))) { - const v = Number(m[1]); - p = (n) => n >= v; - } else if ((m = term.match(/^<=\s*(-?\d+(?:\.\d+)?)$/))) { - const v = Number(m[1]); - p = (n) => n <= v; - } else if ((m = term.match(/^>\s*(-?\d+(?:\.\d+)?)$/))) { - const v = Number(m[1]); - p = (n) => n > v; - } else if ((m = term.match(/^<\s*(-?\d+(?:\.\d+)?)$/))) { - const v = Number(m[1]); - p = (n) => n < v; - } else if ((m = term.match(/^(-?\d+(?:\.\d+)?)\s*\.\.\s*(-?\d+(?:\.\d+)?)$/))) { - const lo = Number(m[1]); - const hi = Number(m[2]); - const min = Math.min(lo, hi); - const max = Math.max(lo, hi); - p = (n) => n >= min && n <= max; - } else if ((m = term.match(/^(-?\d+(?:\.\d+)?)$/))) { - const v = Number(m[1]); - p = (n) => n === v; - } - if (p === null) { - return { ok: false, error: `Invalid filter term: "${term}"` }; - } - predicates.push(p); - } - // OR across terms - return { - ok: true, - predicate: (n) => predicates.some((p) => p(n)), - }; -} +// parseNumericFilter and ParsedNumericFilter are now in lib/column-filters.ts +// (issue #166 extraction). Imported at the top of this file and re-exported +// so existing consumers that import from recommendations.ts keep working. -// Apply the per-column filters to a rec list. ANDs all column filters -// together. Categorical: row passes iff its column value (string-form, -// empty/null mapped to "") is in `values`. Numeric: row passes iff -// parseNumericFilter(expr).predicate accepts the value (skipped if +// Apply the per-column filters to a rec list. Routes through the shared +// generic applyColumnFilters from lib/column-filters (issue #166/#570). +// +// ANDs all column filters together. Categorical: row passes iff its column +// value (string-form, empty/null mapped to "") is in `values`. Numeric: row +// passes iff parseNumericFilter(expr).predicate accepts the value (skipped if // parse failed — the popover's inline error tells the user). // // Account uses cloud_account_id for matching; Term uses String(r.term). // All other categorical columns compare on the underlying string field. +// +// Issue #484: numeric predicates compare against the rounded display value so +// exact-match ("123.45") works for rows whose raw value rounds to the typed +// value, and ">N" / "; - if (entries.length === 0) return [...recs]; - - return recs.filter((r) => { - for (const [col, f] of entries) { - if (f.kind === 'set') { - const cellRaw = categoricalCellValue(r, col); - if (!f.values.includes(cellRaw)) return false; - } else { - const parsed = parseNumericFilter(f.expr); - if (!parsed.ok) continue; // ignore broken expressions; UI shows the error - // Issue #484: compare against the rounded display value so exact-match - // ("123.45") works for rows whose raw value rounds to the typed value, - // and ">N" / "( + recs, + filters, + { + categorical: categoricalCellValue, + numeric: (r, col) => roundForDisplay(numericCellValue(r, col), displayPrecision(col, period)), + }, + ); } function categoricalCellValue(r: LocalRecommendation, col: state.RecommendationsColumnId): string {