-
Notifications
You must be signed in to change notification settings - Fork 84
Refactor request manager filters #6839
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…-request-manager-filters
…-request-manager-filters
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
|
@greptileai review |
…-request-manager-filters
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Greptile Overview
Greptile Summary
This PR successfully refactors the privacy request filter system from Redux-based state management to URL-based state using the nuqs library. The new implementation keeps filter state synchronized with URL query parameters, making filters shareable and persistent across page reloads.
Key changes:
- Created
usePrivacyRequestsFiltershook to manage filter state with URL synchronization - Built new
PrivacyRequestFiltersModalusing Ant Design components - Removed Redux dependency from
PrivacyRequestsDashboard - Added
useCallbackwrapper toresetPaginationinusePaginationhook for dependency array stability
Issue found:
- Form in modal won't sync when
modalFilterschanges externally (requiresuseEffectto update form values)
Confidence Score: 4/5
- This PR is mostly safe to merge with one logical issue that should be fixed
- The refactoring is well-structured and removes technical debt by eliminating Redux state. The hook implementation is clean and the URL synchronization pattern is consistent with the codebase. However, there's a form sync issue in the modal that could cause stale data to display if filters change externally while the modal is open. This is fixable with a simple useEffect.
- Pay close attention to
clients/admin-ui/src/features/privacy-requests/dashboard/PrivacyRequestFiltersModal.tsx- the form needs to sync when modalFilters change
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| clients/admin-ui/src/features/privacy-requests/dashboard/hooks/usePrivacyRequestsFilters.ts | 4/5 | New hook to manage privacy request filters with URL sync via nuqs, replacing Redux state |
| clients/admin-ui/src/features/privacy-requests/dashboard/PrivacyRequestFiltersModal.tsx | 3/5 | New filter modal using Ant Design components; form won't sync when modalFilters change externally |
| clients/admin-ui/src/features/privacy-requests/dashboard/PrivacyRequestsDashboard.tsx | 5/5 | Refactored to use new filter hook and modal, removing Redux dependency |
6 files reviewed, 1 comment
clients/admin-ui/src/features/privacy-requests/dashboard/PrivacyRequestFiltersModal.tsx
Show resolved
Hide resolved
| "search", | ||
| parseAsString.withDefault("").withOptions({ throttleMs: 100 }), | ||
| ); | ||
| const filters = useSelector(selectPrivacyRequestFilters); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love to see these Redux usages going away.
jpople
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested locally, working as expected, code looks good to me. Nice refactor!
…-request-manager-filters
Ticket ENG-1426
Description Of Changes
This PR replaces the privacy request filter functions and filter modal with all new components and hook that use updated libraries and code patterns. The filter state is no longer kept in a Redux, instead it is kept in sync with the url params using the nuqs library. The filter modal now uses ant components and form.
Code Changes
Steps to Confirm
a) Results are updated to reflect the filter
b) The url gets updated to include as queryParams all the filters you've applied
c) If you reload the page with filters applied, the page should show the same results and the filter modal should show all the filters applied
Pre-Merge Checklist
CHANGELOG.mdupdatedmaindowngrade()migration is correct and works