Repository navigation
refactor(frontend): extract parseNumericFilter to shared column-filter lib - #570
Merged
cristim merged 2 commits intoMay 28, 2026
Merged
Conversation
…r lib (closes #166) Move the numeric-filter parser out of recommendations.ts and into frontend/src/lib/column-filters.ts so other tabs (Plans, History, RI Exchange) can reuse it without copying the logic. What changed: - New lib/column-filters.ts exports parseNumericFilter, ParsedNumericFilter, ColumnFilterKind, and a generic applyColumnFilters<TRow, TColumnId> for any tab that wants column-level filtering. - recommendations.ts drops the inline parseNumericFilter definition and imports from the lib; both the function and the type are re-exported so existing consumers that import from recommendations.ts require no changes. - New __tests__/column-filters.test.ts exercises the lib directly (14 cases). The existing parseNumericFilter and applyColumnFilters suites in recommendations.test.ts continue to pass via the re-exports. Adoption: Plans / History / RI Exchange can import parseNumericFilter (or the generic applyColumnFilters) directly from lib/column-filters when they add per-column numeric filters; no further extraction work is needed.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR extracts reusable column-filter primitives—numeric expression parser, filter application pipeline, and type contracts—into a new shared library. The existing Recommendations module is refactored to delegate to this library while preserving its numeric rounding behavior, and comprehensive tests validate both the library and backward compatibility. ChangesShared Column-Filter Library Extraction
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
cristim
added
priority/p2
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Member
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Member
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Member
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
…shared lib Closes the loop on #570: the column-filter lib was extracted (issue #166) but recommendations.ts still contained a local 28-line applyColumnFilters that duplicated the filter loop, only importing parseNumericFilter from the lib. This commit removes the duplicate loop body and routes the exported applyColumnFilters wrapper through applyColumnFiltersLib from lib/column-filters.ts. The wrapper preserves the 2-argument public signature (recs, filters) that tests and internal callers already use, while delegating the generic filter pipeline to the shared implementation. Numeric extractor wiring (issue #484): the wrapper captures getCostPeriod() once at call time and passes an inline extractor (r, col) => roundForDisplay(numericCellValue(r, col), displayPrecision(col, period)) so numeric predicates still compare against the rounded display value, exactly as before. The behavior is identical; the implementation now lives in one place. All 309 recommendation tests and 1909 column-filter suite tests pass.
Member
Author
|
Wired recommendations.ts through the new generic applyColumnFilters in commit 3aafba7 - closes the loop on the extraction. The local 28-line filter loop is replaced by a 7-line wrapper that delegates to applyColumnFiltersLib with categorical/numeric extractors. roundForDisplay behavior is preserved via the numeric extractor. Plans/History/RI Exchange remain greenfield wiring tracked as follow-up. @coderabbitai review |
Contributor
|
✅ Actions performedFull review triggered. |
Contributor
|
Actionable comments posted: 0 |
This was referenced May 28, 2026
cristim
added a commit
that referenced
this pull request
Jun 1, 2026
…lters slice Mirrors the recommendations slice shape (extracted in #570) so the RI Exchange reshape-recommendations table can wire inline column filters via the shared applyColumnFilters helper. Slice is independent of the recommendations slice — column-id shape is reshape-specific (source/target instance types, normalized units, utilization %), and persistence stays out of scope on this iteration to match the existing pattern. Refs #166.
cristim
added a commit
that referenced
this pull request
Jun 1, 2026
Wires per-column filter popovers to the RI Exchange reshape-recommendations table using the helpers extracted in merged #570 (parseNumericFilter + applyColumnFilters). Categorical columns (Source RI, Source/Target instance types, Reason) get a checkbox-list popover; numeric columns (Source/Target count, Utilization %, Normalized used/purchased) get a free-text expression popover that supports `>N`, `>=N`, `<N`, `<=N`, `N..M` ranges, exact match, and comma-separated OR. Numeric predicates compare against the display-rounded cell value so a user typing the displayed figure (e.g. 95.0 for utilization) matches the cell they see. Broken expressions are skipped (inline error in the popover) rather than collapsing the table. Filter state lives in the RI Exchange-specific slice on state.ts; no cross-tab coupling. No drive-by changes to the rest of the RI Exchange page (convertible-RI table, exchange modal, automation settings, history). Test-mocks for the riexchange + riexchange-permissions suites updated to expose the new state getters/setters so the existing happy-path assertions still pass. Refs #166.
cristim
added a commit
that referenced
this pull request
Jun 1, 2026
Adds focused regression coverage for the RI Exchange filter wiring on top of the shared lib (issue #166 follow-up to merged #570): * empty filter record returns a defensive clone of the input * numeric expression filter narrows by predicate * categorical set filter narrows by membership * multiple filters AND together across kinds * broken numeric expressions are skipped rather than treated as match-none * numeric predicates compare against the display-rounded cell value (utilization toFixed(1) regression guard) * clearing a column drops its narrowing The popover / state-slice / button-rendering wiring is exercised by the existing riexchange test suite; this file pins the pure-function contract the lib + extractor + precision composition relies on. Refs #166.
cristim
added a commit
that referenced
this pull request
Jun 3, 2026
…lters slice Mirrors the recommendations slice shape (extracted in #570) so the RI Exchange reshape-recommendations table can wire inline column filters via the shared applyColumnFilters helper. Slice is independent of the recommendations slice — column-id shape is reshape-specific (source/target instance types, normalized units, utilization %), and persistence stays out of scope on this iteration to match the existing pattern. Refs #166.
cristim
added a commit
that referenced
this pull request
Jun 3, 2026
Wires per-column filter popovers to the RI Exchange reshape-recommendations table using the helpers extracted in merged #570 (parseNumericFilter + applyColumnFilters). Categorical columns (Source RI, Source/Target instance types, Reason) get a checkbox-list popover; numeric columns (Source/Target count, Utilization %, Normalized used/purchased) get a free-text expression popover that supports `>N`, `>=N`, `<N`, `<=N`, `N..M` ranges, exact match, and comma-separated OR. Numeric predicates compare against the display-rounded cell value so a user typing the displayed figure (e.g. 95.0 for utilization) matches the cell they see. Broken expressions are skipped (inline error in the popover) rather than collapsing the table. Filter state lives in the RI Exchange-specific slice on state.ts; no cross-tab coupling. No drive-by changes to the rest of the RI Exchange page (convertible-RI table, exchange modal, automation settings, history). Test-mocks for the riexchange + riexchange-permissions suites updated to expose the new state getters/setters so the existing happy-path assertions still pass. Refs #166.
cristim
added a commit
that referenced
this pull request
Jun 3, 2026
Adds focused regression coverage for the RI Exchange filter wiring on top of the shared lib (issue #166 follow-up to merged #570): * empty filter record returns a defensive clone of the input * numeric expression filter narrows by predicate * categorical set filter narrows by membership * multiple filters AND together across kinds * broken numeric expressions are skipped rather than treated as match-none * numeric predicates compare against the display-rounded cell value (utilization toFixed(1) regression guard) * clearing a column drops its narrowing The popover / state-slice / button-rendering wiring is exercised by the existing riexchange test suite; this file pins the pure-function contract the lib + extractor + precision composition relies on. Refs #166.
cristim
added a commit
that referenced
this pull request
Jun 3, 2026
Introduces a Plans-scoped per-column filter slice (PlansColumnId, PlansColumnFilter, PlansColumnFilters) with get/set/clear accessors, mirroring the existing Recommendations slice. Kept as a separate slice so the Plans, History, and RI Exchange follow-ups to PR #570 can land in parallel without contending on each other's state shape. In-memory only — survives tab switches within the SPA, resets on full reload (same lifecycle as recommendationsColumnFilters). Refs #166.
cristim
added a commit
that referenced
this pull request
Jun 3, 2026
Wires the Planned Purchases table to the shared lib/column-filters primitives extracted in PR #570. Each filterable column gets an inline trigger button in its header; clicking opens a popover with either a multi-select (categorical) or a numeric-expression input. Filters AND together, persist in-memory across the SPA session, and are applied at render time. Columns wired: - categorical: provider, service, resource_type, term, payment, status - numeric: count, upfront_cost, estimated_savings Numeric predicates compare against the rounded display value (roundForDisplay + displayPrecisionForPlan) so the issue #484 exact-match contract is preserved here too. Re-renders go through a cached lastLoadedPurchases module slice so popover commits never re-fetch from the API. Popover lives on document.body and is re-anchored after each table re-render. Mock-state additions in plans*.test.ts and xss-purchase-status.test.ts mirror the new accessors; legacy assertions continue to pass. Refs #166. Sibling follow-ups land in parallel for History and RI Exchange.
cristim
added a commit
that referenced
this pull request
Jun 3, 2026
…166) (#789) * refactor(frontend/state): add RiExchangeColumnId + riExchangeColumnFilters slice Mirrors the recommendations slice shape (extracted in #570) so the RI Exchange reshape-recommendations table can wire inline column filters via the shared applyColumnFilters helper. Slice is independent of the recommendations slice — column-id shape is reshape-specific (source/target instance types, normalized units, utilization %), and persistence stays out of scope on this iteration to match the existing pattern. Refs #166. * feat(frontend/riexchange): inline column filters via shared lib Wires per-column filter popovers to the RI Exchange reshape-recommendations table using the helpers extracted in merged #570 (parseNumericFilter + applyColumnFilters). Categorical columns (Source RI, Source/Target instance types, Reason) get a checkbox-list popover; numeric columns (Source/Target count, Utilization %, Normalized used/purchased) get a free-text expression popover that supports `>N`, `>=N`, `<N`, `<=N`, `N..M` ranges, exact match, and comma-separated OR. Numeric predicates compare against the display-rounded cell value so a user typing the displayed figure (e.g. 95.0 for utilization) matches the cell they see. Broken expressions are skipped (inline error in the popover) rather than collapsing the table. Filter state lives in the RI Exchange-specific slice on state.ts; no cross-tab coupling. No drive-by changes to the rest of the RI Exchange page (convertible-RI table, exchange modal, automation settings, history). Test-mocks for the riexchange + riexchange-permissions suites updated to expose the new state getters/setters so the existing happy-path assertions still pass. Refs #166. * test(frontend/riexchange): column-filter regression suite Adds focused regression coverage for the RI Exchange filter wiring on top of the shared lib (issue #166 follow-up to merged #570): * empty filter record returns a defensive clone of the input * numeric expression filter narrows by predicate * categorical set filter narrows by membership * multiple filters AND together across kinds * broken numeric expressions are skipped rather than treated as match-none * numeric predicates compare against the display-rounded cell value (utilization toFixed(1) regression guard) * clearing a column drops its narrowing The popover / state-slice / button-rendering wiring is exercised by the existing riexchange test suite; this file pins the pure-function contract the lib + extractor + precision composition relies on. Refs #166.
cristim
added a commit
that referenced
this pull request
Jun 3, 2026
#791) * refactor(frontend/state): add PlansColumnId + plansColumnFilters slice Introduces a Plans-scoped per-column filter slice (PlansColumnId, PlansColumnFilter, PlansColumnFilters) with get/set/clear accessors, mirroring the existing Recommendations slice. Kept as a separate slice so the Plans, History, and RI Exchange follow-ups to PR #570 can land in parallel without contending on each other's state shape. In-memory only — survives tab switches within the SPA, resets on full reload (same lifecycle as recommendationsColumnFilters). Refs #166. * feat(frontend/plans): inline column filters via shared lib (refs #166) Wires the Planned Purchases table to the shared lib/column-filters primitives extracted in PR #570. Each filterable column gets an inline trigger button in its header; clicking opens a popover with either a multi-select (categorical) or a numeric-expression input. Filters AND together, persist in-memory across the SPA session, and are applied at render time. Columns wired: - categorical: provider, service, resource_type, term, payment, status - numeric: count, upfront_cost, estimated_savings Numeric predicates compare against the rounded display value (roundForDisplay + displayPrecisionForPlan) so the issue #484 exact-match contract is preserved here too. Re-renders go through a cached lastLoadedPurchases module slice so popover commits never re-fetch from the API. Popover lives on document.body and is re-anchored after each table re-render. Mock-state additions in plans*.test.ts and xss-purchase-status.test.ts mirror the new accessors; legacy assertions continue to pass. Refs #166. Sibling follow-ups land in parallel for History and RI Exchange. * test(frontend/plans): column-filter regression suite Adds plans-column-filters.test.ts covering the Planned Purchases table integration with lib/column-filters: - every filterable column header carries a trigger button - clicking opens a portal popover detached to document.body - categorical set filter narrows rows (provider=aws) - numeric expression filter narrows rows (count >= 2) - stacked filters AND together - invalid expressions surface the lib's inline error and apply no filter - (All) tri-state restores the full row set after narrowing The shared parseNumericFilter + applyColumnFilters primitives keep their own coverage in column-filters.test.ts; these tests focus on the Plans-specific wiring. Refs #166. * style(frontend/plans): use unicode escapes for filter-icon + em-dash Matches the canonical recommendations.ts wiring exactly (⛛ filter icon, — em-dash inside the aria-label). Keeps the rendered DOM identical to the Opportunities tab so the two surfaces are indistinguishable at the screen-reader and visual layers. No behavioural change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
parseNumericFilter+ParsedNumericFilterout ofrecommendations.tsintofrontend/src/lib/column-filters.ts(closes refactor(frontend): extract column-filter primitives into a shared module — adopt on Plans / History / RI Exchange #166)applyColumnFilters<TRow, TColumnId>in the lib so Plans / History / RI Exchange can adopt column-level filtering without copy-pasting the parserrecommendations.tsre-exports both the function and the type for backward compat -- no import-path churn for existing consumers__tests__/column-filters.test.tsexercises the lib directly (14 cases); all existingparseNumericFilterandapplyColumnFilterstests inrecommendations.test.tscontinue passing via the re-exportsScope note
Per the issue, this is a lighter-touch PR: extraction + Plans-ready lib. Wiring Plans / History / RI Exchange to the generic
applyColumnFiltersis tracked as follow-on work -- the lib contract is settled, so those PRs are unblocked.Test plan
cd frontend && npx jest --no-coverage-- all 1908 tests passcd frontend && npx tsc --noEmit-- no type errorscolumn-filters.test.tsexercisesparseNumericFilterand genericapplyColumnFiltersdirectly from the libSummary by CodeRabbit
Release Notes
New Features
Refactor
Tests