-
Notifications
You must be signed in to change notification settings - Fork 84
fix: monitor filter state syncing [ENG-2401] #7239
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
base: main
Are you sure you want to change the base?
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
02f54b3 to
534c6e4
Compare
Greptile SummaryThis PR fixes an out-of-sync state handling issue in the Changes made:
Critical issue found: Confidence Score: 2/5
Important Files Changed
|
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.
Additional Comments (1)
-
clients/admin-ui/src/features/data-discovery-and-detection/action-center/fields/MonitorFieldFilters.tsx, line 162-171 (link)logic: Missing
useEffectto sync local state with applied state changes. The local state is initialized once withuseState(resourceStatus), but ifresourceStatus,dataCategory, orconfidenceBucketchange externally (e.g., from URL changes, reset actions, or other components), the local state won't update. This means the popover will show stale values.Add after line 171:
2 files reviewed, 2 comments
...in-ui/src/features/data-discovery-and-detection/action-center/fields/MonitorFieldFilters.tsx
Show resolved
Hide resolved
chore: minor const change
fb9f642 to
aa39764
Compare
The state doesn't need to be synced in this way. |
Ticket ENG-2401
Description Of Changes
Fixing an out-of-sync state handling where the
MonitorFieldFilterscomponent was not reflecting the current state of the applied filters.Code Changes
Steps to Confirm
Pre-Merge Checklist
CHANGELOG.mdupdatedmaindowngrade()migration is correct and works