fix(admin): validate group combos, fix checkbox UX, remove stale bulk checkboxes - #1427
Conversation
… checkboxes Closes #1404 closes #1405 closes #1408. #1404 - user-selection and select-all checkboxes no longer call renderUsers(), which was rebuilding the entire table and collapsing any open expand panels. Both handlers now patch the affected rows in-place (row-selected class, individual checkbox state, bulk-actions-bar visibility) without touching the expand rows. #1405 - contradictory group combinations (view-only group paired with a write-capable group) are now blocked at two sites: - toggleUserGroup() in the expand panel, before the updateUser API call. The checkbox is reverted immediately on rejection. - saveUser() in the create/edit modal, before the createUser/updateUser API call. The shared validateGroupCombination() utility in users/utils.ts encapsulates the detection logic and returns a human-readable message. #1408 - the user-list checkboxes were already wired to real bulk actions (bulk-delete and bulk-add-to-group) in PR #977; this PR retains that wiring and the UX fix in #1404 removes the last friction point (panel collapse on selection) that made the feature feel broken. Tests: 33 new test cases covering validateGroupCombination unit behaviour, checkbox-does-not-collapse-panel for both individual and select-all, bulk-actions-bar show/hide, and group-combo validation in both the expand panel and the modal.
|
@coderabbitai review |
The #1405 validation added in this branch guarded the inline expand-panel toggle and the create/edit modal, but bulkAddToGroup (wired up by the #1408 bulk-actions bar) computed the resulting group set and called updateUser with no validation. Selecting a view-only-group user and bulk-adding a write group silently created the exact contradictory combination #1405 is meant to prevent. bulkAddToGroup now partitions the selected users: each user's resulting group set is run through validateGroupCombination before any API call. Contradictory users are skipped (never silently written) and named in an error toast so the admin knows which were left unchanged; they stay selected so the offending membership can be fixed and retried. Compatible users are applied and deselected as before. Regression tests (fail-before/pass-after, confirmed against pre-fix code): bulk-adding a write group to a view-only user does not call updateUser and surfaces the error naming the user; the skipped user stays selected; a mixed selection applies to the compatible user and skips only the contradictory one.
|
Addressed the BLOCK on the bulk-add bypass.
Regression tests added in Gates (true exit codes): @coderabbitai review |
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds shared contradictory-group validation to bulk, inline, and modal user flows. Bulk actions now skip invalid users selectively, while checkbox updates patch rows in place so expanded panels remain open. ChangesUser group controls
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AdminUI
participant userList
participant userModals
participant validateGroupCombination
participant UserAPI
participant Toast
AdminUI->>userList: Toggle inline group assignment
userList->>validateGroupCombination: Validate prospective group set
validateGroupCombination-->>userList: Conflict message or null
userList->>Toast: Show error and revert checkbox
userList->>UserAPI: updateUser when valid
AdminUI->>userModals: Save selected groups
userModals->>validateGroupCombination: Validate selected groups
validateGroupCombination-->>userModals: Conflict message or null
userModals->>UserAPI: createUser or updateUser when valid
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@frontend/src/users/userActions.ts`:
- Around line 238-247: Update the bulk update flow around the applicable.map
calls to use Promise.allSettled instead of Promise.all, track each result
against its userId, deselect only fulfilled updates, and reload users whenever
at least one update succeeds. Report rejected entries separately while
preserving selection for failed users, and retain the existing success behavior
for fulfilled updates.
In `@frontend/src/users/userList.ts`:
- Around line 232-244: Update the expand-panel guidance near the group
assignment UI to state that view-only groups cannot coexist with write-capable
groups. Replace the outdated “has no effect” wording while preserving the
existing validation behavior in the handler around validateGroupCombination.
- Around line 351-356: Update the select-all synchronization logic in the
user-list selection handler to determine checked state from the currently
filtered users, rather than comparing the global selectedUserIds.size with
filteredUsers.length. Ensure every visible filtered user is selected before
checking the checkbox, while preserving the requirement that at least one user
is selected.
🪄 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
Run ID: 0eea891b-0590-4588-b7f5-c8612d2ad386
📒 Files selected for processing (5)
frontend/src/__tests__/users.test.tsfrontend/src/users/userActions.tsfrontend/src/users/userList.tsfrontend/src/users/userModals.tsfrontend/src/users/utils.ts
| try { | ||
| await Promise.all( | ||
| applicable.map(({ userId, groups }) => api.updateUser(userId, { groups })), | ||
| ); | ||
|
|
||
| clearSelectedUserIds(); | ||
| // Deselect only the successfully-applied users so any skipped | ||
| // (contradictory) users stay selected for the admin to fix. | ||
| applicable.forEach(({ userId }) => removeSelectedUserId(userId)); | ||
| await loadUsers(); | ||
| showSuccess(`Successfully added ${count} user(s) to group`); | ||
| showSuccess(`Successfully added ${applicable.length} user(s) to group`); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Reconcile partial bulk successes individually.
Promise.all rejects when one request fails even though other updates may have succeeded. Those successful users remain selected and the UI is not reloaded, so retrying operates on stale state. Use Promise.allSettled, deselect fulfilled entries, reload after any success, and report failed entries separately.
🤖 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/users/userActions.ts` around lines 238 - 247, Update the bulk
update flow around the applicable.map calls to use Promise.allSettled instead of
Promise.all, track each result against its userId, deselect only fulfilled
updates, and reload users whenever at least one update succeeds. Report rejected
entries separately while preserving selection for failed users, and retain the
existing success behavior for fulfilled updates.
| // Validate the resulting group combination before hitting the API. | ||
| // A view-only group (all permissions are view:*) combined with a | ||
| // write-capable group is semantically contradictory (issue #1405). | ||
| const combinationError = validateGroupCombination(next, availableGroups); | ||
| if (combinationError) { | ||
| showError(combinationError); | ||
| // Revert the checkbox immediately so the DOM reflects reality. | ||
| const cb = document.querySelector<HTMLInputElement>( | ||
| `.group-assign-checkbox[data-user-id="${userId}"][data-group-id="${groupId}"]`, | ||
| ); | ||
| if (cb) cb.checked = !checked; | ||
| return; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Update the expand-panel guidance to match this validation.
Line 185 says combining lower- and higher-privilege groups “has no effect,” but this handler now rejects that combination. Tell admins that view-only groups cannot coexist with write-capable groups.
🤖 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/users/userList.ts` around lines 232 - 244, Update the
expand-panel guidance near the group assignment UI to state that view-only
groups cannot coexist with write-capable groups. Replace the outdated “has no
effect” wording while preserving the existing validation behavior in the handler
around validateGroupCombination.
| // Keep the select-all checkbox in sync. | ||
| const selectAll = document.getElementById('select-all-users') as HTMLInputElement | null; | ||
| if (selectAll) { | ||
| selectAll.checked = | ||
| selectedUserIds.size > 0 && selectedUserIds.size === filteredUsers.length; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Determine select-all state from the filtered users, not the global count.
Hidden selected users can make these counts equal even when some visible users are unchecked, incorrectly checking select-all.
Proposed fix
- selectAll.checked =
- selectedUserIds.size > 0 && selectedUserIds.size === filteredUsers.length;
+ selectAll.checked =
+ filteredUsers.length > 0 &&
+ filteredUsers.every(user => selectedUserIds.has(user.id));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Keep the select-all checkbox in sync. | |
| const selectAll = document.getElementById('select-all-users') as HTMLInputElement | null; | |
| if (selectAll) { | |
| selectAll.checked = | |
| selectedUserIds.size > 0 && selectedUserIds.size === filteredUsers.length; | |
| } | |
| // Keep the select-all checkbox in sync. | |
| const selectAll = document.getElementById('select-all-users') as HTMLInputElement | null; | |
| if (selectAll) { | |
| selectAll.checked = | |
| filteredUsers.length > 0 && | |
| filteredUsers.every(user => selectedUserIds.has(user.id)); | |
| } |
🤖 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/users/userList.ts` around lines 351 - 356, Update the select-all
synchronization logic in the user-list selection handler to determine checked
state from the currently filtered users, rather than comparing the global
selectedUserIds.size with filteredUsers.length. Ensure every visible filtered
user is selected before checking the checkbox, while preserving the requirement
that at least one user is selected.
fix(admin): validate group combos, fix checkbox UX, remove stale bulk checkboxes
Summary
renderUsers(), which was rebuilding the entire table DOM and collapsing any open expand panels. Both handlers now patch the affected rows in-place (togglerow-selectedclass, sync individual checkbox state, callupdateBulkActionsBar()) without touching the expand rows.saveUser()(pre-API). SharedvalidateGroupCombination()utility inusers/utils.tsencapsulates detection and produces a human-readable error message.Files changed
frontend/src/users/utils.ts-- new exportedvalidateGroupCombination(groupIds, allGroups)functionfrontend/src/users/userList.ts-- importvalidateGroupCombination; add pre-API validation intoggleUserGroup(); replacerenderUsers(filteredUsers)calls in both checkbox handlers with in-place DOM patchingfrontend/src/users/userModals.ts-- importvalidateGroupCombination; add pre-API validation insaveUser()after the existing >= 1 group checkfrontend/src/__tests__/users.test.ts-- 33 new test cases:validateGroupCombinationunit tests, checkbox-does-not-collapse-panel (individual + select-all), bulk-actions-bar show/hide, contradictory-combo rejection in expand panel and modalTest plan
npx jest --testPathPattern=users.test --no-coverage)webpack --mode production)closes #1404 closes #1405 closes #1408
Summary by CodeRabbit
New Features
Bug Fixes