Repository navigation
ux(recommendations): cascading categorical filter distinct values (closes #164) - #822
Conversation
cristim
added
triaged
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 54 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR makes categorical column-filter popovers cross-filter aware by computing distinct values from recommendations filtered by all other active columns' constraints, while preserving the currently-active column's own selections in the dropdown via an ChangesCategorical Cascading Filters
Sequence DiagramsequenceDiagram
participant User
participant openColumnPopover
participant applyColumnFilters
participant buildPopoverContent
participant distinctValuesForColumn
User->>openColumnPopover: Open column popover
openColumnPopover->>applyColumnFilters: Filter recs by all OTHER column filters
applyColumnFilters-->>openColumnPopover: Cross-filtered recommendation set
openColumnPopover->>openColumnPopover: Compute alwaysInclude from column's own active set
openColumnPopover->>buildPopoverContent: Pass filtered recs + alwaysInclude
buildPopoverContent->>distinctValuesForColumn: Request distinct values with alwaysInclude
distinctValuesForColumn-->>buildPopoverContent: Merged distinct set (filtered + always-include)
buildPopoverContent-->>openColumnPopover: Popover content
openColumnPopover-->>User: Rendered popover with cascaded options
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 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 unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
frontend/src/__tests__/recommendations.test.ts (1)
2627-2680: ⚡ Quick winAdd a regression for the actual
alwaysIncludecase.This suite proves cross-column narrowing and own-filter exclusion, but it still doesn’t pin the behavior this PR adds the
alwaysIncludeset for: an active value on the edited column should remain visible even when the other active filters would otherwise remove it from the distinct set. A contradictory case likeprovider=azure+service=ec2would catch a regression here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/__tests__/recommendations.test.ts` around lines 2627 - 2680, Add a regression test that verifies the alwaysInclude behavior: mock state.getRecommendationsColumnFilters to include provider: { values: ['azure'] } and service: { values: ['ec2'] }, call loadRecommendations(), open the service column popover (use the same selector used in existing tests: 'th .column-filter-btn[data-column="service"]' and '.column-filter-popover .column-filter-item input[type="checkbox"]'), and assert that 'ec2' is still present in the popover values (even though azure rows only have 'vm'); this uses the existing helpers loadRecommendations and the mocked state.getRecommendationsColumnFilters to ensure an active value on the edited column (service) is always included.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@frontend/src/__tests__/recommendations.test.ts`:
- Around line 2627-2680: Add a regression test that verifies the alwaysInclude
behavior: mock state.getRecommendationsColumnFilters to include provider: {
values: ['azure'] } and service: { values: ['ec2'] }, call
loadRecommendations(), open the service column popover (use the same selector
used in existing tests: 'th .column-filter-btn[data-column="service"]' and
'.column-filter-popover .column-filter-item input[type="checkbox"]'), and assert
that 'ec2' is still present in the popover values (even though azure rows only
have 'vm'); this uses the existing helpers loadRecommendations and the mocked
state.getRecommendationsColumnFilters to ensure an active value on the edited
column (service) is always included.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5270fafa-a77e-4e2b-b524-75c12559a85b
📒 Files selected for processing (2)
frontend/src/__tests__/recommendations.test.tsfrontend/src/recommendations.ts
|
@coderabbitai review |
Rate Limit Exceeded
|
|
Addressed CR pass-1 findings:
Note: CR re-ping skipped (org-level billing block active). Re-invoke |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
When building a categorical filter popover for column X, apply all OTHER active column filters to the rec set first. This means selecting Provider=AWS before opening the Service popover now shows only AWS services instead of the full cross-provider list, eliminating ghost values that produce zero rows. Mitigation for the "broaden a column" UX: any value that is part of the column's own currently-active filter is always included in the distinct list even if the cross-filtered set would omit it, so the user can change or deselect the existing value without first clearing every other filter. Algorithm: full re-scan of the cross-filtered rec set on each popover open via the existing applyColumnFilters path. Tests: 2 new assertions -- service popover narrows to AWS-only services when Provider=AWS is active; provider popover still shows all providers when Provider=AWS is its own active filter (own-filter exclusion).
…er popover Add a contradictory-filter test (provider=azure + service=ec2) that verifies values from a column's own active filter remain visible in the popover even when cross-filtering by other columns would omit them. This pins the alwaysInclude behavior added in the cascading-filter implementation: without it, opening the service popover while provider=azure is active would drop ec2 from the list entirely, making it impossible to deselect without first clearing the provider filter.
…ilter popover Add a regression test that verifies a service value containing an HTML injection string (<script>alert(1)</script>) is stored in dataset and rendered via textContent -- never interpreted as markup -- when the cascading filter popover builds its checkbox list.
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
Summary
alwaysIncludepin: any value in the column's own active filter is always shown, so the user can change/deselect it without first clearing every other filter.Test plan
cd frontend && npx tsc --noEmit-- cleancd frontend && npx jest src/__tests__/recommendations*.test.ts-- 331 passed, 0 failedCloses #164
Summary by CodeRabbit