-
Notifications
You must be signed in to change notification settings - Fork 84
refactor: bulk action select [ENG-1736] #6835
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
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
8fa6acc to
24eb283
Compare
24eb283 to
5549871
Compare
5549871 to
39f906d
Compare
39f906d to
09cef13
Compare
09cef13 to
922d733
Compare
67b04d6 to
2cb7e0c
Compare
2cb7e0c to
8e1d487
Compare
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
Refactors bulk selection UX to properly handle multi-page selections using an inclusive/exclusive mode pattern. When users select "all", the system switches to exclusive mode and tracks only deselected items, enabling bulk actions across all pages with excluded URNs sent to the backend.
Key changes:
- New
useBulkListSelecthook manages selection state with delta tracking for both inclusive (selected items) and exclusive (deselected items) modes - Backend integration updated to accept
excluded_resource_urnsin bulk action requests - New
getAllowedActionsendpoint dynamically determines available actions based on filtered results - Selection state properly resets when filters change
Issues found:
- Mode state not reset in
resetListSelect()causes incorrect behavior after clearing selections - Missing dependency in useEffect could cause stale closures
- Inefficient O(n²) array filtering in
inverseDeltacalculation
Confidence Score: 3/5
- Refactor has a logical bug in state reset that will cause incorrect selection behavior
- Core functionality is well-designed with proper backend integration, but the
resetListSelectfunction has a critical bug where it doesn't reset the mode state. This will cause incorrect behavior when users clear all selections and start selecting again - the system could remain in "exclusive" mode when it should be "inclusive", leading to inverted selection logic. The missing useEffect dependency could also cause subtle bugs with stale closures. - Pay close attention to
useBulkListSelect.ts- the resetListSelect function needs to reset mode state
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| clients/admin-ui/src/features/data-discovery-and-detection/action-center/fields/useBulkListSelect.ts | 3/5 | New hook for managing bulk list selection with inclusive/exclusive modes. Contains inefficient array filtering logic that could impact performance. |
| clients/admin-ui/src/features/data-discovery-and-detection/action-center/fields/page.tsx | 4/5 | Main page refactored to use new bulk selection hook, properly handles multi-page selection with excluded URNs. Mode reset correctly clears only delta state. |
| clients/admin-ui/src/features/data-discovery-and-detection/action-center/fields/useBulkActions.ts | 5/5 | Updated to accept excluded URNs parameter for bulk actions. Clean implementation with proper error handling. |
8 files reviewed, 3 comments
...admin-ui/src/features/data-discovery-and-detection/action-center/fields/useBulkListSelect.ts
Show resolved
Hide resolved
| const resetListSelect = () => { | ||
| setDeltaState([]); | ||
| }; |
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.
logic: mode state not reset when clearing selections - causes incorrect behavior if user clears all selections then starts selecting again
| const resetListSelect = () => { | |
| setDeltaState([]); | |
| }; | |
| const resetListSelect = () => { | |
| setDeltaState([]); | |
| setMode("inclusive"); | |
| }; |
Context Used: Rule from dashboard - Prefer readonly mapping objects over logic evaluations and mutable arrays when determining available... (source)
clients/admin-ui/src/features/data-discovery-and-detection/action-center/fields/page.tsx
Show resolved
Hide resolved
8e1d487 to
b4f3bc6
Compare
clients/admin-ui/src/features/data-discovery-and-detection/action-center/fields/page.tsx
Show resolved
Hide resolved
clients/admin-ui/src/features/data-discovery-and-detection/action-center/fields/page.tsx
Show resolved
Hide resolved
...admin-ui/src/features/data-discovery-and-detection/action-center/fields/useBulkListSelect.ts
Outdated
Show resolved
Hide resolved
...admin-ui/src/features/data-discovery-and-detection/action-center/fields/useBulkListSelect.ts
Outdated
Show resolved
Hide resolved
clients/admin-ui/src/features/data-discovery-and-detection/action-center/fields/page.tsx
Outdated
Show resolved
Hide resolved
clients/admin-ui/src/features/data-discovery-and-detection/action-center/fields/page.tsx
Outdated
Show resolved
Hide resolved
|
Smoke testing in Vercel build seems to be working well. Left a few code improvement comments, but nothing too serious. |
b4f3bc6 to
7961ee6
Compare
7961ee6 to
c37e855
Compare
c37e855 to
5b96b75
Compare
5b96b75 to
f5d8f32
Compare
wip: lots more chore: linting fix: making linter happy fix: selected field count refactor: cleaning up some utils chore: linting and minor bugs feat: tray action integration refactor: less action code chore: adding toasts refactor: minor updates and fixes fix: typos fix: linting chore: updating utils chore: fix extra toast messages fix: add another missing return refactor: use lodash refactor: bullk action select wip: multi select wip: more wip wip: continued wip:more more wip chore: more wip chore: rename file wip: adding excluded and working towards clean linting chore: linting fixes and comments fix: mode escape hatch wip stuff chore: linting refactor: simple bulk select fix: using correct hook wip: fixing switch logic wip: refinements wip: naming updates refactor: naming cleanups chore: linting chore: remove old hook wip: adding in dynamic allowed actions chore: updating types chore: continued wip feat: dynamic allowed bulk actions chore: minor bug refactor: moving around logic nit: removing extra space fix: reset mode refactor: requested changes fix: proper labeling
f5d8f32 to
0f1078c
Compare
gilluminate
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.
Nice work! looks good
Ticket ENG-1736
Description Of Changes
Fixing the UX interactions for bulk actions to properly handle multi-page selections and bulk deselections.
Adds enabling/disabling of actions for bulk actions based on BE response.
Code Changes
useBulkListSelectto handle selectable list logic where bulk actions are availableSteps to Confirm
Multi-select
Bulk-Select
Select Allcheckbox6 Deselect another field
Pre-Merge Checklist
CHANGELOG.mdupdatedmaindowngrade()migration is correct and works