feat: add active filter count badge to UniversalFilter#1753
Conversation
Wraps the filter TooltipIcon in a MUI Badge that displays the count of active (non-'All') filter values. Badge is invisible when no filters are active via MUI's native invisible prop. Fixes layer5io#1728 Signed-off-by: Udaybir Singh <writetoudaybir@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughUniversalFilter now counts active selected filters and displays that count in a Badge around the filter trigger. The badge is hidden when no non-default filters are active, and tests mock Badge output for DOM assertions. ChangesUniversalFilter active count badge
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/custom/UniversalFilter.tsx (1)
186-198: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a Sistent semantic token for the badge color.
color="primary"relies on MUI’s default color API rather than the project’s theme semantics. Style the badge with the appropriate Sistent semantic palette token so its applied-filter state remains theme-consistent.As per coding guidelines, “Theme-aware UI must use Sistent theme exports and semantic palette tokens rather than raw MUI defaults.”
🤖 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 `@src/custom/UniversalFilter.tsx` around lines 186 - 198, Update the Badge in the UniversalFilter component to replace the raw MUI color="primary" value with the appropriate Sistent semantic palette token from the theme, preserving the existing activeFilterCount visibility behavior.Source: Coding guidelines
src/__testing__/UniversalFilter.test.tsx (1)
115-188: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest badge updates after filters are applied and cleared.
These cases mount independent initial states only. Add a controlled-parent or
rerendertest that changesselectedFiltersfrom active toAll(and vice versa) to verify the badge updates during the user flow required by the issue.🤖 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 `@src/__testing__/UniversalFilter.test.tsx` around lines 115 - 188, Extend the badge tests around UniversalFilter with a rerender or controlled-parent flow that changes selectedFilters between an active value and 'All'. Assert that filter-badge data-content and data-invisible update correctly in both directions, preserving the existing initial-state coverage.
🤖 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.
Inline comments:
In `@src/__testing__/UniversalFilter.test.tsx`:
- Around line 58-65: Update the Badge mock component in UniversalFilter.test.tsx
to replace the any props annotation with an explicit React prop type covering
optional badgeContent, optional invisible, and children; preserve the mock’s
existing rendering and data attributes.
---
Nitpick comments:
In `@src/__testing__/UniversalFilter.test.tsx`:
- Around line 115-188: Extend the badge tests around UniversalFilter with a
rerender or controlled-parent flow that changes selectedFilters between an
active value and 'All'. Assert that filter-badge data-content and data-invisible
update correctly in both directions, preserving the existing initial-state
coverage.
In `@src/custom/UniversalFilter.tsx`:
- Around line 186-198: Update the Badge in the UniversalFilter component to
replace the raw MUI color="primary" value with the appropriate Sistent semantic
palette token from the theme, preserving the existing activeFilterCount
visibility behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0df0062b-85a5-4b55-95d8-89bc12b89cb2
📒 Files selected for processing (2)
src/__testing__/UniversalFilter.test.tsxsrc/custom/UniversalFilter.tsx
| jest.mock('../base/Badge', () => ({ | ||
| Badge: ({ children, badgeContent, invisible }: any) => ( | ||
| <div data-testid="filter-badge" data-content={badgeContent} data-invisible={String(invisible)}> | ||
| {children} | ||
| </div> | ||
| ) | ||
| })); | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-files src/__testing__/UniversalFilter.test.tsx
echo '---'
cat -n src/__testing__/UniversalFilter.test.tsx | sed -n '1,260p'Repository: layer5io/sistent
Length of output: 7096
Type the Badge mock props explicitly. Replace the any here with a small prop type, such as React.PropsWithChildren<{ badgeContent?: React.ReactNode; invisible?: boolean }>.
🤖 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 `@src/__testing__/UniversalFilter.test.tsx` around lines 58 - 65, Update the
Badge mock component in UniversalFilter.test.tsx to replace the any props
annotation with an explicit React prop type covering optional badgeContent,
optional invisible, and children; preserve the mock’s existing rendering and
data attributes.
Source: Coding guidelines
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/custom/UniversalFilter.tsx (1)
244-251: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssociate the quick-range label with the Select.
InputLabelis detached from theSelect, so screen readers may announce the control without its “Quick Ranges” name. AddlabelId={${testId}-quick-range-label}to theSelect.🤖 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 `@src/custom/UniversalFilter.tsx` around lines 244 - 251, Update the Select associated with the quick-range InputLabel to include the matching labelId value `${testId}-quick-range-label`, ensuring the control is programmatically named “Quick Ranges.”src/__testing__/publishedTypeSurfaceDependencies.test.ts (3)
95-97: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDetect side-effect imports in declaration bundles.
specifiersInmissesimport 'side-effect-package';. A package referenced only this way is still required by consumers, but the current detector returns no reference and can let the dependency-contract test pass incorrectly.Extend the matcher and add a detector fixture for side-effect imports.
Proposed fix
const specifiersIn = (source: string): string[] => [ - ...new Set([...source.matchAll(/(?:from|import\()\s*['"]([^'"]+)['"]/g)].map((match) => match[1])) + ...new Set( + [...source.matchAll(/\b(?:from|import(?:\s*\(\s*)?)\s*['"]([^'"]+)['"]/g)].map( + (match) => match[1] + ) + ) ];Also applies to: 214-224
🤖 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 `@src/__testing__/publishedTypeSurfaceDependencies.test.ts` around lines 95 - 97, Update specifiersIn to recognize bare side-effect imports such as import 'package' in addition to from and dynamic import() forms, while preserving extraction of the module specifier. Add a detector fixture covering a declaration bundle containing only a side-effect import and assert that the referenced package is detected by the dependency-contract test.
140-181: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftValidate the published surface with a scoped
tscfixture.This test only scans declaration text and package metadata; it never type-checks a real consumer. Add a scoped fixture, invoke
tsc, verify the fixture appears in--listFiles, and filter diagnostics to that fixture so the test validates actual consumer resolution rather than only regex results.As per coding guidelines, type-contract tests under
src/__testing__/*.test.tsmust use this scopedtscverification approach.🤖 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 `@src/__testing__/publishedTypeSurfaceDependencies.test.ts` around lines 140 - 181, Extend the published type-surface test around the existing dist/index.d.ts checks with a scoped consumer fixture that imports the package and is type-checked via tsc. Verify the fixture is present in tsc’s --listFiles output, filter diagnostics to the fixture, and fail on relevant resolution or type errors while preserving the existing metadata and declaration-text assertions.Source: Coding guidelines
66-77: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRestore the documented optional-peer exemption.
The comments state that
@mui/x-date-pickersis intentionally exempted whileDateTimePickerPropsremains publicly re-exported, butOPTIONAL_PEERS_ON_THE_RECORDcontains onlyreact. Because optional peers are removed from the consumer-guaranteed set, this causes the suite to fail whenever the declaration bundle references@mui/x-date-pickers.Either add the documented exemption until issue
#1749is fixed, or remove the public type reference before narrowing the list.Proposed fix
-const OPTIONAL_PEERS_ON_THE_RECORD = ['react']; +const OPTIONAL_PEERS_ON_THE_RECORD = ['`@mui/x-date-pickers`', 'react'];Based on learnings, derive optional-peer enforcement from
src/__testing__/optionalPeerDependencies.test.tsrather than narrowing the list independently.🤖 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 `@src/__testing__/publishedTypeSurfaceDependencies.test.ts` around lines 66 - 77, Restore the documented `@mui/x-date-pickers` exemption in OPTIONAL_PEERS_ON_THE_RECORD so the published type-surface test remains consistent with the barrel’s DateTimePickerProps re-export, or remove that public type reference before narrowing the exemption list. Prefer reusing the optional-peer enforcement or exemption definition from optionalPeerDependencies.test.ts instead of maintaining a separate independently narrowed list.Source: Learnings
🧹 Nitpick comments (1)
src/custom/UniversalFilter.tsx (1)
290-294: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a Sistent semantic badge color.
color="primary"relies on the raw MUI color API. Use the project’s semantic Sistent palette/token styling for the badge so it remains consistent across themes.As per coding guidelines, “Theme-aware UI must use Sistent theme exports and semantic palette tokens rather than raw MUI defaults.”
🤖 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 `@src/custom/UniversalFilter.tsx` around lines 290 - 294, Update the Badge in UniversalFilter to replace the raw MUI color="primary" value with the project’s Sistent semantic badge color or theme palette token. Preserve the existing activeFilterCount, overlap, and visibility behavior while using the established theme-aware styling approach.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@src/__testing__/publishedTypeSurfaceDependencies.test.ts`:
- Around line 95-97: Update specifiersIn to recognize bare side-effect imports
such as import 'package' in addition to from and dynamic import() forms, while
preserving extraction of the module specifier. Add a detector fixture covering a
declaration bundle containing only a side-effect import and assert that the
referenced package is detected by the dependency-contract test.
- Around line 140-181: Extend the published type-surface test around the
existing dist/index.d.ts checks with a scoped consumer fixture that imports the
package and is type-checked via tsc. Verify the fixture is present in tsc’s
--listFiles output, filter diagnostics to the fixture, and fail on relevant
resolution or type errors while preserving the existing metadata and
declaration-text assertions.
- Around line 66-77: Restore the documented `@mui/x-date-pickers` exemption in
OPTIONAL_PEERS_ON_THE_RECORD so the published type-surface test remains
consistent with the barrel’s DateTimePickerProps re-export, or remove that
public type reference before narrowing the exemption list. Prefer reusing the
optional-peer enforcement or exemption definition from
optionalPeerDependencies.test.ts instead of maintaining a separate independently
narrowed list.
In `@src/custom/UniversalFilter.tsx`:
- Around line 244-251: Update the Select associated with the quick-range
InputLabel to include the matching labelId value `${testId}-quick-range-label`,
ensuring the control is programmatically named “Quick Ranges.”
---
Nitpick comments:
In `@src/custom/UniversalFilter.tsx`:
- Around line 290-294: Update the Badge in UniversalFilter to replace the raw
MUI color="primary" value with the project’s Sistent semantic badge color or
theme palette token. Preserve the existing activeFilterCount, overlap, and
visibility behavior while using the established theme-aware styling approach.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: cbd00669-0971-4d97-b83f-8939ac126815
📒 Files selected for processing (3)
src/__testing__/UniversalFilter.test.tsxsrc/__testing__/publishedTypeSurfaceDependencies.test.tssrc/custom/UniversalFilter.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/testing/UniversalFilter.test.tsx
82f39f3 to
33880e5
Compare
Signed-off-by: Udaybir Singh <writetoudaybir@gmail.com>
|
@KhushamBansal ready for review!! |
KhushamBansal
left a comment
KhushamBansal
left a comment
There was a problem hiding this comment.
Tested it in meshery-ui, LGTM!!
|
@rishiraj38 @YASHMAHAKAL @leecalcote |
Notes for Reviewers
This PR fixes #1728
Signed commits
Changes
src/custom/UniversalFilter.tsxsrc/__testing__/UniversalFilter.test.tsxVerification
Summary by CodeRabbit
New Features
Tests