[5424] Remove delete user, updates to enable/disable users#4894
[5424] Remove delete user, updates to enable/disable users#4894jvega190 wants to merge 9 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds persisted show-disabled-user filtering to the user management grid, removes delete-user handling from the edit dialog, and adds two 5.0.0 changelog bullets for those removals. ChangesUser Management Changes
Sequence Diagram(s)sequenceDiagram
participant UsersGridUI
participant UserManagement
participant localStorage
participant fetchAll
UsersGridUI->>UserManagement: onShowDisabledChange(checked)
UserManagement->>localStorage: setStoredShowDisabledUsers(user, checked)
UserManagement->>fetchAll: fetchAll({ showDisabled, keyword, offset })
fetchAll-->>UserManagement: PagedArray<User>
UserManagement-->>UsersGridUI: showDisabled, users
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ui/app/src/components/UserManagement/UserManagement.tsx (1)
56-71:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrevent double/racy fetches when toggling “show disabled users.”
Line 132 fires
fetchUsers(keyword, 0)beforesetShowDisabled(checked)is reflected, so the first request uses staleshowDisabledfrom Line 59. Then Line 71 causesuseEffect(Line 74-77) to refetch after state update. This two-request pattern can render stale data if responses arrive out of order.Also applies to: 128-133
🤖 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 `@ui/app/src/components/UserManagement/UserManagement.tsx` around lines 56 - 71, The fetchUsers callback captures the current showDisabled value from closure, but when the toggle handler calls fetchUsers before updating the showDisabled state, the request uses the stale value, causing a double-fetch when the state update triggers a useEffect. To fix this, update the showDisabled state before calling fetchUsers in the toggle handler, or modify the fetchUsers callback to accept showDisabled as a parameter so you can pass the new value directly to it, ensuring the first request uses the correct showDisabled value and avoiding the unnecessary second fetch triggered by the dependency array.
🤖 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 `@ui/app/src/CHANGELOG.md`:
- Line 93: In the CHANGELOG.md file on line 93, add a period after the
abbreviation "etc" to make it "etc." in the TODO comment line. The text should
read "FE2 TODO: image=>image-picker, text=>input, etc. services/contentType
typeMap removed" to follow standard English style conventions for abbreviations.
- Line 18: In the CHANGELOG.md file at line 18, there is a missing space before
the backtick in the text "Replaced by`sxs` prop." Add a space between "by" and
the backtick to read "Replaced by `sxs` prop." For improved clarity and grammar,
consider updating it to "Replaced by the `sxs` prop." instead.
---
Outside diff comments:
In `@ui/app/src/components/UserManagement/UserManagement.tsx`:
- Around line 56-71: The fetchUsers callback captures the current showDisabled
value from closure, but when the toggle handler calls fetchUsers before updating
the showDisabled state, the request uses the stale value, causing a double-fetch
when the state update triggers a useEffect. To fix this, update the showDisabled
state before calling fetchUsers in the toggle handler, or modify the fetchUsers
callback to accept showDisabled as a parameter so you can pass the new value
directly to it, ensuring the first request uses the correct showDisabled value
and avoiding the unnecessary second fetch triggered by the dependency array.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c184db9d-edfe-41f6-9330-60f32700e404
📒 Files selected for processing (9)
ui/app/src/CHANGELOG.mdui/app/src/components/AccountManagement/utils.tsxui/app/src/components/EditUserDialog/EditUserDialogContainer.tsxui/app/src/components/EditUserDialog/EditUserDialogUI.tsxui/app/src/components/EditUserDialog/utils.tsui/app/src/components/UserManagement/UserManagement.tsxui/app/src/components/UsersGrid/UsersGridUI.tsxui/app/src/services/users.tsui/app/src/utils/state.ts
💤 Files with no reviewable changes (1)
- ui/app/src/components/EditUserDialog/EditUserDialogUI.tsx
…instead of a ref, and update UsersGridUI layout for better alignment. Fix getStoredShowDisabledUsers to return false instead of null when no value is found.
…owDisabled parameter
|
Outside diff range comment addressed |
|
@coderabbitai can you check if everything looks good? Please give it a final review before I hand over for human review |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== EditUserDialogContainer.tsx ==="
cat ui/app/src/components/EditUserDialog/EditUserDialogContainer.tsx
echo ""
echo "=== EditUserDialogUI.tsx ==="
cat ui/app/src/components/EditUserDialog/EditUserDialogUI.tsx
echo ""
echo "=== EditUserDialog/utils.ts ==="
cat ui/app/src/components/EditUserDialog/utils.ts
echo ""
echo "=== UserManagement.tsx ==="
cat ui/app/src/components/UserManagement/UserManagement.tsx
echo ""
echo "=== UsersGridUI.tsx ==="
cat ui/app/src/components/UsersGrid/UsersGridUI.tsx
echo ""
echo "=== services/users.ts ==="
cat ui/app/src/services/users.ts
echo ""
echo "=== utils/state.ts ==="
cat ui/app/src/utils/state.ts
echo ""
echo "=== AccountManagement/utils.tsx ==="
cat ui/app/src/components/AccountManagement/utils.tsxLength of output: 69910
There are a few leftover artifacts from the delete feature removal that should be cleaned up before human review:
// These two imports are no longer used (delete UI was removed):
import ConfirmDropdown from '../ConfirmDropdown'; // ← remove
import DeleteRoundedIcon from '`@mui/icons-material/DeleteRounded`'; // ← remove
// These three translation keys in the `translations` object are also orphaned:
confirmHelperText: { ... }, // ← remove
confirmOk: { ... }, // ← remove
confirmCancel: { ... }, // ← remove
const translations = defineMessages({
userDeleted: { // ← remove (delete handler no longer exists)
id: 'userInfoDialog.userDeleted',
defaultMessage: 'User deleted successfully'
},
...
});Everything else — the |
|
@coderabbitai can you check if everything looks good? Please give it a final review before I hand over for human review |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== EditUserDialogUI.tsx ==="
cat ui/app/src/components/EditUserDialog/EditUserDialogUI.tsx
echo ""
echo "=== EditUserDialogContainer.tsx ==="
cat ui/app/src/components/EditUserDialog/EditUserDialogContainer.tsx
echo ""
echo "=== UserManagement.tsx ==="
cat ui/app/src/components/UserManagement/UserManagement.tsx
echo ""
echo "=== UsersGridUI.tsx ==="
cat ui/app/src/components/UsersGrid/UsersGridUI.tsx
echo ""
echo "=== services/users.ts ==="
cat ui/app/src/services/users.ts
echo ""
echo "=== utils/state.ts ==="
cat ui/app/src/utils/state.ts
echo ""
echo "=== AccountManagement/utils.tsx ==="
cat ui/app/src/components/AccountManagement/utils.tsxLength of output: 67785
There's just one remaining artifact — three orphaned translation keys in // ui/app/src/components/EditUserDialog/EditUserDialogUI.tsx
const translations = defineMessages({
externallyManaged: { ... }, // ✅ used
siteName: { ... }, // pre-existing (out of scope)
roles: { ... }, // pre-existing (out of scope)
confirmHelperText: { // ❌ remove — delete UI is gone
id: 'userInfoDialog.helperText',
defaultMessage: 'Delete user "{username}"?'
},
confirmOk: { // ❌ remove
id: 'words.yes',
defaultMessage: 'Yes'
},
confirmCancel: { // ❌ remove
id: 'words.no',
defaultMessage: 'No'
},
invalidMinLength: { ... }, // ✅ used
});Once those three entries are removed, the PR will be clean and ready for human review. Everything else looks solid! 🐇 |
|
@coderabbitai can you check if everything looks good? Please give it a final review before I hand over for human review |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== EditUserDialogUI.tsx ==="
cat ui/app/src/components/EditUserDialog/EditUserDialogUI.tsxLength of output: 12719
All previously flagged artifacts have been removed:
The remaining |
|
@coderabbitai, the last commit hasn't been reviewed. Please review and report findings. |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== Latest commits ==="
git log --oneline --decorate -n 5
echo
echo "=== HEAD commit metadata ==="
git show --no-patch --stat --format=fuller HEAD
echo
echo "=== Files changed in HEAD ==="
git diff-tree --no-commit-id --name-only -r HEAD
echo
echo "=== Diff for HEAD vs HEAD^ ==="
git diff --unified=3 HEAD^ HEAD
echo
echo "=== AST outlines for changed source files ==="
while IFS= read -r file; do
case "$file" in
*.ts|*.tsx|*.js|*.jsx)
echo
echo "--- outline: $file ---"
ast-grep outline "$file" --view expanded || true
;;
esac
done < <(git diff-tree --no-commit-id --name-only -r HEAD)
echo
echo "=== Full contents of changed source files ==="
while IFS= read -r file; do
case "$file" in
*.ts|*.tsx|*.js|*.jsx|*.json|*.md)
echo
echo "--- file: $file ---"
sed -n '1,260p' "$file"
;;
esac
done < <(git diff-tree --no-commit-id --name-only -r HEAD)Length of output: 2672 🏁 Script executed: #!/bin/bash
set -euo pipefail
file="ui/app/src/components/UsersGrid/UsersGridUI.tsx"
echo "=== Outline: $file ==="
ast-grep outline "$file" --view expanded || true
echo
echo "=== Relevant section around the latest change ==="
nl -ba "$file" | sed -n '1,180p'
echo
echo "=== Search for FormControlLabel usage in this file ==="
rg -n -C 4 'FormControlLabel|labelPlacement|Show disabled users|Checkbox' "$file"Length of output: 1328 🏁 Script executed: #!/bin/bash
set -euo pipefail
file="ui/app/src/components/UsersGrid/UsersGridUI.tsx"
echo "=== File contents (first 180 lines) ==="
sed -n '1,180p' "$file"
echo
echo "=== Grep for relevant JSX ==="
grep -n -E 'FormControlLabel|labelPlacement|Show disabled users|Checkbox|Typography' "$file"Length of output: 6045
Findings: no issues found in that last commit. It only updates labelPlacement="start"on the ✅ Action performedReview finished.
|
…o feature/5424 # Conflicts: # ui/app/src/CHANGELOG.md # ui/app/src/utils/state.ts
|
@coderabbitai can you check if everything looks good? Please give it a final review before I hand over for human review |
|
Final pass summary:
I don’t see any remaining issues from my side. Ready for human review. ✅ ✅ Action performedReview finished.
|
2 participants
craftercms/craftercms#5424
Summary by CodeRabbit