[Netmanager-App] Organization Settings Page #2454
Conversation
…s retrieval and updates
…er to Redux store
…es API, and create useRoles hook for fetching roles
…prove API integration
… update role handling
…twork handling and improve role creation logic
…onse types, implement groupMembers API for fetching and inviting users
📝 WalkthroughWalkthroughThis pull request introduces new React components, API modules, custom hooks, and Redux slices to enhance organization, group, role, and team management. Several pages and UI components have been added or updated, including multi-step dialogs for organization creation and improved settings interfaces. In addition, TypeScript interfaces for groups and roles have been introduced, and new dependencies have been added. Layout adjustments and minor formatting fixes are also included. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant D as CreateOrganizationDialog
participant F as Form Handler
participant API as Organization API
participant T as Toast Notifications
U->>D: Open creation dialog
D->>F: Collect organization details
F->>API: Call createGroupApi(details)
API-->>F: Return creation response
F->>T: Show success/failure notification
D->>F: Proceed with adding sites and devices
F->>API: Assign sites/devices via API calls
API-->>F: Return assignment response
F->>T: Notify update status
D->>F: Initiate member invitations
F->>API: Call inviteUserToGroupTeam
API-->>F: Return invitation response
F->>T: Final notification
Suggested labels
Suggested reviewers
Poem
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🔭 Outside diff range comments (1)
netmanager-app/core/redux/slices/teamSlice.ts (1)
1-38:⚠️ Potential issueRemove duplicate roles slice implementation
This appears to be identical to
rolesSlice.ts. We should remove this duplicate implementation and userolesSlice.tsinstead.Consider removing this file and updating any imports to use
rolesSlice.ts.
🧹 Nitpick comments (8)
netmanager-app/app/types/roles.ts (1)
16-19: Consider consolidating manager informationThe manager information is spread across multiple fields. Consider consolidating these into a manager object for better organization and maintainability.
- grp_manager: string; - grp_manager_username: string; - grp_manager_firstname: string; - grp_manager_lastname: string; + grp_manager: { + id: string; + username: string; + firstname: string; + lastname: string; + };netmanager-app/core/redux/slices/rolesSlice.ts (1)
36-37: Consider adding selector functionsFor better encapsulation and reusability, consider adding selector functions to access the state.
// Add these exports: export const selectRoles = (state: RootState) => state.roles.roles; export const selectRolesLoading = (state: RootState) => state.roles.isLoading; export const selectRolesError = (state: RootState) => state.roles.error;netmanager-app/app/types/groups.ts (1)
25-28: Consider exporting the RolePermission interface.The RolePermission interface is used in the exported GroupMember interface but isn't exported itself. This could cause issues if other files need to reference this type directly.
-interface RolePermission { +export interface RolePermission { _id: string; permission: string; };netmanager-app/app/(authenticated)/organizations/[id]/page.tsx (1)
1-1: Clean up commented code.Remove or implement the commented imports and hooks. If they're needed for future implementation, consider adding a TODO comment explaining the intent.
Also applies to: 6-6, 9-9
netmanager-app/components/Organization/organization-profile.tsx (1)
16-16: Remove commented out codeCommented out code should be removed as it adds unnecessary noise to the codebase.
-// const dispatch = useAppDispatch()netmanager-app/components/Organization/List.tsx (1)
16-20: Add loading state indicatorThe component captures loading state but doesn't display a loading indicator to users.
Consider adding a loading spinner:
const status = isLoading ? "loading" : error ? "failed" : "success" + if (status === "loading") { + return <LoadingSpinner /> + } if (status === "failed") { return <div>Error loading organizations. Please try again.</div> }netmanager-app/components/Organization/organization-roles.tsx (1)
12-13: Remove unnecessary empty lines.These consecutive empty lines don't add to code readability.
- -netmanager-app/components/Organization/team-members.tsx (1)
72-81: Improve role management implementation.Current implementation has several areas for improvement:
- Hardcoded role options
- Missing confirmation for role changes
- No loading state during updates
+const ROLE_OPTIONS = { + MEMBER: "Member", + MANAGER: "Manager", + ADMIN: "Admin" +} as const + +const handleRoleChange = async (memberId: string, newRole: string) => { + if (await confirmDialog(`Are you sure you want to change this user's role to ${newRole}?`)) { + handleUpdateRole(memberId, newRole) + } +} + -<Select value={member.role} onValueChange={(value) => handleUpdateRole(member.id, value)}> +<Select + value={member.role} + onValueChange={(value) => handleRoleChange(member.id, value)} + disabled={isUpdating} +> <SelectContent> - <SelectItem value="Member">Member</SelectItem> - <SelectItem value="Manager">Manager</SelectItem> - <SelectItem value="Admin">Admin</SelectItem> + {Object.entries(ROLE_OPTIONS).map(([key, value]) => ( + <SelectItem key={key} value={value}>{value}</SelectItem> + ))} </SelectContent> </Select>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
netmanager-app/app/(authenticated)/organizations/[id]/page.tsx(1 hunks)netmanager-app/app/(authenticated)/organizations/page.tsx(1 hunks)netmanager-app/app/types/groups.ts(1 hunks)netmanager-app/app/types/roles.ts(1 hunks)netmanager-app/components/Organization/List.tsx(1 hunks)netmanager-app/components/Organization/organization-profile.tsx(1 hunks)netmanager-app/components/Organization/organization-roles.tsx(1 hunks)netmanager-app/components/Organization/team-members.tsx(1 hunks)netmanager-app/core/apis/organizations.ts(1 hunks)netmanager-app/core/apis/roles.ts(1 hunks)netmanager-app/core/hooks/useGroups.ts(1 hunks)netmanager-app/core/hooks/useRoles.ts(1 hunks)netmanager-app/core/redux/slices/groupsSlice.ts(1 hunks)netmanager-app/core/redux/slices/rolesSlice.ts(1 hunks)netmanager-app/core/redux/slices/teamSlice.ts(1 hunks)netmanager-app/core/redux/store.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- netmanager-app/app/(authenticated)/organizations/page.tsx
🔇 Additional comments (4)
netmanager-app/core/redux/store.ts (1)
8-8: LGTM: Groups reducer integrationThe addition of the groups reducer aligns well with the organization settings page requirements.
Also applies to: 18-18
netmanager-app/app/types/roles.ts (1)
1-37: LGTM: Well-structured TypeScript interfacesThe interfaces are well-defined with clear type definitions for permissions, groups, roles, and API responses. This provides good type safety for the organization settings page implementation.
netmanager-app/core/redux/slices/rolesSlice.ts (1)
1-38: LGTM: Well-implemented Redux sliceThe roles slice follows Redux Toolkit best practices with clear state management and type safety.
netmanager-app/app/(authenticated)/organizations/[id]/page.tsx (1)
8-32: Add error handling for invalid organization IDs.The component doesn't handle cases where the organization ID might be invalid. Consider adding error boundaries or validation.
❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Check if there's error handling in the parent components rg -A 5 "catch.*organization.*id"Length of output: 35
Action Required: Implement Error Handling for Invalid Organization IDs
The file
netmanager-app/app/(authenticated)/organizations/[id]/page.tsxdoes not include any error handling logic for scenarios where an invalid organization ID is provided. Our search (usingrg) did not yield any evidence of error boundaries or try/catch constructs related to organization ID validation in this component or its parent components. This confirms that the review comment regarding adding error handling is valid.
- File Affected:
netmanager-app/app/(authenticated)/organizations/[id]/page.tsx- Issue: Missing error handling (validation/error boundaries) for invalid organization IDs.
- Recommendation: Implement appropriate error handling—either add validation before rendering or wrap the component with an error boundary to handle cases where the organization ID is invalid.
| @@ -14,6 +15,7 @@ export const store = configureStore({ | |||
| grids: gridsReducer, | |||
| cohorts: cohortsReducer, | |||
| clients: clientsRudcer, | |||
There was a problem hiding this comment.
Update variable name to match import
The variable name needs to be updated to match the corrected import.
- clients: clientsRudcer,
+ clients: clientsReducer,📝 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.
| clients: clientsRudcer, | |
| clients: clientsReducer, |
| @@ -5,6 +5,7 @@ import devicesReducer from "./slices/devicesSlice"; | |||
| import cohortsReducer from "./slices/cohortsSlice"; | |||
| import gridsReducer from "./slices/gridsSlice"; | |||
| import clientsRudcer from "./slices/clientsSlice"; | |||
There was a problem hiding this comment.
Fix typo in import name
There's a typo in the import name: clientsRudcer should be clientsReducer.
-import clientsRudcer from "./slices/clientsSlice";
+import clientsReducer from "./slices/clientsSlice";📝 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.
| import clientsRudcer from "./slices/clientsSlice"; | |
| import clientsReducer from "./slices/clientsSlice"; |
| interface Groups { | ||
| _id: string | ||
| grp_status: "ACTIVE" | "INACTIVE" | ||
| grp_profile_picture: string | ||
| grp_title: string | ||
| grp_description: string | ||
| grp_website: string | ||
| grp_industry: string | ||
| grp_country: string | ||
| grp_timezone: string | ||
| createdAt: string | ||
| numberOfGroupUsers: number | ||
| grp_users: UserDetails[] | ||
| grp_manager: UserDetails | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix naming inconsistency and remove duplicate interface.
- The slice variable name
sitesSlicedoesn't match its name "groups". - The
Groupsinterface duplicates theGroupinterface from@/app/types/groups.ts.
-interface Groups {
- _id: string
- grp_status: "ACTIVE" | "INACTIVE"
- grp_profile_picture: string
- grp_title: string
- grp_description: string
- grp_website: string
- grp_industry: string
- grp_country: string
- grp_timezone: string
- createdAt: string
- numberOfGroupUsers: number
- grp_users: UserDetails[]
- grp_manager: UserDetails
- }
+import { Group } from "@/app/types/groups";
-const sitesSlice = createSlice({
+const groupsSlice = createSlice({Update the export statement accordingly:
-export const { setGroups, setLoading, setError } = sitesSlice.actions;
-export default sitesSlice.reducer;
+export const { setGroups, setLoading, setError } = groupsSlice.actions;
+export default groupsSlice.reducer;Also applies to: 33-33
| const { data, isLoading, error } = useQuery({ | ||
| queryKey: ["clients", groupId], | ||
| queryFn: () => | ||
| roles.getRolesDetailsApi(groupId || ""), | ||
| onSuccess: (data: any) => { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix inconsistent query key and improve type safety.
The query key uses "clients" which seems inconsistent with the role-related functionality. Also, the empty string fallback for groupId could mask errors.
const { data, isLoading, error } = useQuery({
- queryKey: ["clients", groupId],
+ queryKey: ["roles", "org", groupId],
queryFn: () =>
- roles.getRolesDetailsApi(groupId || ""),
+ roles.getRolesDetailsApi(groupId),📝 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.
| const { data, isLoading, error } = useQuery({ | |
| queryKey: ["clients", groupId], | |
| queryFn: () => | |
| roles.getRolesDetailsApi(groupId || ""), | |
| onSuccess: (data: any) => { | |
| const { data, isLoading, error } = useQuery({ | |
| queryKey: ["roles", "org", groupId], | |
| queryFn: () => | |
| roles.getRolesDetailsApi(groupId), | |
| onSuccess: (data: any) => { |
| onSuccess: (data: any) => { | ||
| dispatch(setRoles(data)); | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove 'any' type and add proper typing.
Using 'any' type defeats TypeScript's type checking. Consider defining proper types for the API response.
- onSuccess: (data: any) => {
+ onSuccess: (data: RolesState) => {
dispatch(setRoles(data));
},📝 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.
| onSuccess: (data: any) => { | |
| dispatch(setRoles(data)); | |
| }, | |
| onSuccess: (data: RolesState) => { | |
| dispatch(setRoles(data)); | |
| }, |
| const handleSubmit = async (e: React.FormEvent) => { | ||
| e.preventDefault() | ||
| try { | ||
| await groups.updateGroupDetailsApi(organizationId, { | ||
| ...organization, | ||
| grp_title, | ||
| grp_description | ||
| }) | ||
| } catch (error) { | ||
| console.error("Failed to update organization profile", error) | ||
|
|
||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling with user feedback
Currently, errors are only logged to the console. Users should be notified when the update fails.
Consider adding a toast notification or error message in the UI:
const handleSubmit = async (e: React.FormEvent) => {
e.preventDefault()
+ const toast = useToast()
try {
await groups.updateGroupDetailsApi(organizationId, {
...organization,
grp_title,
grp_description
})
+ toast.success("Organization profile updated successfully")
} catch (error) {
console.error("Failed to update organization profile", error)
+ toast.error("Failed to update organization profile. Please try again.")
}
}📝 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.
| const handleSubmit = async (e: React.FormEvent) => { | |
| e.preventDefault() | |
| try { | |
| await groups.updateGroupDetailsApi(organizationId, { | |
| ...organization, | |
| grp_title, | |
| grp_description | |
| }) | |
| } catch (error) { | |
| console.error("Failed to update organization profile", error) | |
| } | |
| } | |
| const handleSubmit = async (e: React.FormEvent) => { | |
| e.preventDefault() | |
| const toast = useToast() | |
| try { | |
| await groups.updateGroupDetailsApi(organizationId, { | |
| ...organization, | |
| grp_title, | |
| grp_description | |
| }) | |
| toast.success("Organization profile updated successfully") | |
| } catch (error) { | |
| console.error("Failed to update organization profile", error) | |
| toast.error("Failed to update organization profile. Please try again.") | |
| } | |
| } |
| const handleCreateRole = async (e: React.FormEvent) => { | ||
| e.preventDefault(); | ||
|
|
||
| if (!network) { | ||
| console.error("Network is null"); | ||
| return; | ||
| } | ||
|
|
||
| const data = { | ||
| role_name: newRoleName, | ||
| network_id: network._id, | ||
| group_id: organizationId, | ||
| }; | ||
|
|
||
| try { | ||
| const newRole = await roles.createRoleApi(data); | ||
| console.log("Role created successfully:", newRole); | ||
| } catch (error) { | ||
| console.error("Error creating role:", error); | ||
| } | ||
|
|
||
| setNewRoleName(""); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling and remove console statements.
The current implementation has several areas for improvement:
- Console statements should be replaced with proper error logging
- User feedback is missing for successful/failed operations
- Network error could be more user-friendly
const handleCreateRole = async (e: React.FormEvent) => {
e.preventDefault();
if (!network) {
- console.error("Network is null");
+ toast.error("Please select a network first");
return;
}
const data = {
role_name: newRoleName,
network_id: network._id,
group_id: organizationId,
};
try {
const newRole = await roles.createRoleApi(data);
- console.log("Role created successfully:", newRole);
+ toast.success("Role created successfully");
} catch (error) {
- console.error("Error creating role:", error);
+ toast.error("Failed to create role. Please try again.");
+ logger.error("Error creating role:", error);
}
setNewRoleName("");
};📝 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.
| const handleCreateRole = async (e: React.FormEvent) => { | |
| e.preventDefault(); | |
| if (!network) { | |
| console.error("Network is null"); | |
| return; | |
| } | |
| const data = { | |
| role_name: newRoleName, | |
| network_id: network._id, | |
| group_id: organizationId, | |
| }; | |
| try { | |
| const newRole = await roles.createRoleApi(data); | |
| console.log("Role created successfully:", newRole); | |
| } catch (error) { | |
| console.error("Error creating role:", error); | |
| } | |
| setNewRoleName(""); | |
| }; | |
| const handleCreateRole = async (e: React.FormEvent) => { | |
| e.preventDefault(); | |
| if (!network) { | |
| toast.error("Please select a network first"); | |
| return; | |
| } | |
| const data = { | |
| role_name: newRoleName, | |
| network_id: network._id, | |
| group_id: organizationId, | |
| }; | |
| try { | |
| const newRole = await roles.createRoleApi(data); | |
| toast.success("Role created successfully"); | |
| } catch (error) { | |
| toast.error("Failed to create role. Please try again."); | |
| logger.error("Error creating role:", error); | |
| } | |
| setNewRoleName(""); | |
| }; |
| const newName = prompt("Enter new role name:", role.name) | ||
| if (newName) handleUpdateRole(role._id, newName) | ||
| }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace browser prompt with a proper modal dialog.
Using the browser's native prompt:
- Provides poor user experience
- Lacks input validation
- Doesn't match the application's styling
Consider using a custom modal component with proper validation:
-const newName = prompt("Enter new role name:", role.name)
-if (newName) handleUpdateRole(role._id, newName)
+openRoleUpdateModal({
+ currentName: role.name,
+ onConfirm: (newName) => {
+ if (newName && newName.trim()) {
+ handleUpdateRole(role._id, newName.trim())
+ }
+ }
+})📝 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.
| const newName = prompt("Enter new role name:", role.name) | |
| if (newName) handleUpdateRole(role._id, newName) | |
| }} | |
| openRoleUpdateModal({ | |
| currentName: role.name, | |
| onConfirm: (newName) => { | |
| if (newName && newName.trim()) { | |
| handleUpdateRole(role._id, newName.trim()) | |
| } | |
| } | |
| }) | |
| }} |
| axiosError.response?.data?.message || "Failed to fetch grids summary" | ||
| ); |
There was a problem hiding this comment.
Fix incorrect error messages in API functions.
Several error messages are incorrectly copy-pasted and don't match their respective operations:
- getGroupMembersApi: "Failed to fetch grids summary"
- inviteUserToGroupTeam: "Failed to fetch grids summary"
- acceptGroupTeamInvite: "Failed to fetch grids summary"
// In getGroupMembersApi
-axiosError.response?.data?.message || "Failed to fetch grids summary"
+axiosError.response?.data?.message || "Failed to fetch group members"
// In inviteUserToGroupTeam
-axiosError.response?.data?.message || "Failed to fetch grids summary"
+axiosError.response?.data?.message || "Failed to send invitation"
// In acceptGroupTeamInvite
-axiosError.response?.data?.message || "Failed to fetch grids summary"
+axiosError.response?.data?.message || "Failed to accept invitation"Also applies to: 93-94, 108-109
| acceptGroupTeamInvite: async (groupId: string, userEmail: string) => { | ||
| try { | ||
| const response = await axiosInstance.post( | ||
| `${USERS_MGT_URL}/requests/emails/groups/${groupId}`, | ||
| { emails: [userEmail] } | ||
| ); | ||
| return response.data; | ||
| } catch (error) { | ||
| const axiosError = error as AxiosError<ErrorResponse>; | ||
| throw new Error( | ||
| axiosError.response?.data?.message || "Failed to fetch grids summary" | ||
| ); | ||
| } | ||
| }, |
There was a problem hiding this comment.
Fix incorrect endpoint in acceptGroupTeamInvite function.
The function appears to be incorrectly copy-pasted from inviteUserToGroupTeam:
- Uses the same endpoint as invitation
- Has identical request body structure
- Doesn't match the intended accept invitation functionality
The endpoint and request structure should be different for accepting an invitation:
acceptGroupTeamInvite: async (groupId: string, userEmail: string) => {
try {
const response = await axiosInstance.post(
- `${USERS_MGT_URL}/requests/emails/groups/${groupId}`,
- { emails: [userEmail] }
+ `${USERS_MGT_URL}/groups/${groupId}/accept-invite`,
+ { email: userEmail }
);
return response.data;
} catch (error) {
const axiosError = error as AxiosError<ErrorResponse>;
throw new Error(
- axiosError.response?.data?.message || "Failed to fetch grids summary"
+ axiosError.response?.data?.message || "Failed to accept invitation"
);
}
},📝 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.
| acceptGroupTeamInvite: async (groupId: string, userEmail: string) => { | |
| try { | |
| const response = await axiosInstance.post( | |
| `${USERS_MGT_URL}/requests/emails/groups/${groupId}`, | |
| { emails: [userEmail] } | |
| ); | |
| return response.data; | |
| } catch (error) { | |
| const axiosError = error as AxiosError<ErrorResponse>; | |
| throw new Error( | |
| axiosError.response?.data?.message || "Failed to fetch grids summary" | |
| ); | |
| } | |
| }, | |
| acceptGroupTeamInvite: async (groupId: string, userEmail: string) => { | |
| try { | |
| const response = await axiosInstance.post( | |
| `${USERS_MGT_URL}/groups/${groupId}/accept-invite`, | |
| { email: userEmail } | |
| ); | |
| return response.data; | |
| } catch (error) { | |
| const axiosError = error as AxiosError<ErrorResponse>; | |
| throw new Error( | |
| axiosError.response?.data?.message || "Failed to accept invitation" | |
| ); | |
| } | |
| }, |
…ng group members and update teamSlice for state management
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
netmanager-app/core/redux/slices/teamSlice.ts (1)
17-34: Consider enhancing state management granularity and consistency.While the reducers are well-implemented, consider these improvements:
- The loading state could be more granular to handle different operations (e.g., fetching, adding, removing team members).
- The
setLoadingreducer should probably reset the error state for consistency, as other reducers reset related states.Here's a suggested enhancement:
interface TeamState { team: GroupMember[]; - isLoading: boolean; + loadingStates: { + fetch: boolean; + add: boolean; + remove: boolean; + }; error: string | null; } const initialState: TeamState = { team: [], - isLoading: false; + loadingStates: { + fetch: false, + add: false, + remove: false, + }, error: null, }; reducers: { setTeamMember(state, action: PayloadAction<GroupMember[]>) { state.team = action.payload; - state.isLoading = false; + state.loadingStates = { + fetch: false, + add: false, + remove: false, + }; state.error = null; }, - setLoading(state, action: PayloadAction<boolean>) { - state.isLoading = action.payload; + setLoading(state, action: PayloadAction<{type: keyof TeamState['loadingStates']; status: boolean}>) { + state.loadingStates[action.payload.type] = action.payload.status; + if (action.payload.status) { + state.error = null; + } }, setError(state, action: PayloadAction<string>) { state.error = action.payload; - state.isLoading = false; + state.loadingStates = { + fetch: false, + add: false, + remove: false, + }; }, }netmanager-app/core/hooks/useGroups.ts (3)
46-52: Replace console logs with proper error handling.Consider using a toast notification system or error boundary for better user feedback instead of console logs.
- onSuccess: () => { - console.log("Grid details fetched successfully"); - }, + onSuccess: () => { + toast.success("Group details fetched successfully"); + }, - onError: (error: AxiosError<ErrorResponse>) => { - console.error( - "Failed to fetch grid details:", - error.response?.data?.message - ); - }, + onError: (error: AxiosError<ErrorResponse>) => { + toast.error(`Failed to fetch group details: ${error.response?.data?.message}`); + },
71-74: Replace console logs with proper error handling.Consider using a toast notification system for better user feedback instead of console logs.
- console.error( - "Failed to update grid details:", - error.response?.data?.message - ); + toast.error(`Failed to update group details: ${error.response?.data?.message}`);
94-94: Replace console logs with proper error handling.Consider using a toast notification system for better user feedback instead of console logs.
- console.error("Failed to create grid:", error.response?.data?.message); + toast.error(`Failed to create group: ${error.response?.data?.message}`);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
netmanager-app/core/hooks/useGroups.ts(1 hunks)netmanager-app/core/redux/slices/teamSlice.ts(1 hunks)
🔇 Additional comments (8)
netmanager-app/core/redux/slices/teamSlice.ts (3)
1-9: Clean and well-structured type definitions!The TypeScript interface and imports are well-organized, with proper type definitions and clear structure.
11-15: Well-defined initial state!The initial state is properly typed and initialized with sensible default values.
36-37: Clean exports following Redux best practices!The exports are well-structured, following Redux Toolkit conventions and enabling proper tree-shaking.
netmanager-app/core/hooks/useGroups.ts (5)
1-16: LGTM! Well-organized imports and error handling.The imports are comprehensive and the ErrorResponse interface is properly defined.
18-40: LGTM! Robust implementation with proper error handling.The hook effectively manages group data with Redux integration and safe defaults.
57-57: Fix incorrect fallback value in useGroupsDetailsThe mutation result includes
|| []as a fallback, which doesn't make sense for a single group details query.Apply this diff to fix the return type:
- getGroupDetails: mutation.mutate || [], + getGroupDetails: mutation.mutate,
66-66: Fix incorrect API function in useUpdateGroupDetailsThe hook is using
getGroupDetailsApiinstead ofupdateGroupDetailsApi.Apply this diff to fix the API call:
- mutationFn: async () => await groups.getGroupDetailsApi(gridId), + mutationFn: async (data: Group) => await groups.updateGroupDetailsApi(gridId, data),
99-99: Fix inconsistent naming in useCreateGroup return valueThe return object uses "Grid" instead of "Group" in property names.
Apply this diff to maintain naming consistency:
- createGrid: mutation.mutate, + createGroup: mutation.mutate,
…rror handling, and improve API integration for roles and groups
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
netmanager-app/components/Settings/ApiTokens.tsx (2)
145-145: Use optional chaining
Instead of chaining multiple conditions, you can simplify the expression:-return client && client.access_token && client.access_token.token +return client?.access_token?.token🧰 Tools
🪛 Biome (1.9.4)
[error] 145-145: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
149-149: Use optional chaining
Similarly, consider this approach for expires:-return client && client.access_token && client.access_token.expires +return client?.access_token?.expires
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
netmanager-app/components/Settings/ApiTokens.tsx(6 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
netmanager-app/components/Settings/ApiTokens.tsx
[error] 140-140: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
(lint/complexity/noUselessTernary)
[error] 145-145: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 150-150: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (19)
netmanager-app/components/Settings/ApiTokens.tsx (19)
1-2: Use of 'use client' is correct
No concerns here for enabling client-side rendering.
13-30: Imports appear well organized
This block of imports provides additional UI components and icons for sorting, searching, pagination, and alerts. It's logically consistent and well-structured.
32-35: ITEMS_PER_PAGE and new type definitions
The introduction of ITEMS_PER_PAGE and typed sort fields is a solid approach, ensuring clarity and maintainability for sorting logic.
38-38: Usage of queryClient
Fetching the queryClient from React Query is a standard pattern. No issues.
47-48: useUserClients hook usage
Retrieving the clients, loading state, and error from this custom hook is neat and keeps concerns separated.
49-52: Pagination & sorting states
Storing searchQuery, currentPage, sortField, and sortOrder in component state is straightforward and effective.
54-61: Toggling sort order
The handleSort function is a concise way to toggle between ascending and descending modes for the selected field. This is a clean approach.
63-73: Sorting logic
The sorting implementation covers both string fields and date fields, which is well-handled in this snippet.Also applies to: 77-87
89-95: Client search filter
Filtering by both name and IP addresses effectively handles partial matches. This is very user-friendly for locating clients quickly.
97-97: Creating 'sortedClients'
Chaining the sorted array after filtering is a clean approach, separating responsibilities nicely.
99-103: Pagination calculations
Using totalPages and slicing the sorted list into currentClients is standard and straightforward.
104-136: Pagination UI
The getPageNumbers function captures ellipses for page jumps neatly, ensuring a dynamic pagination experience across large page sets.
219-225: Loading state
Displaying a loading spinner while data is being fetched is a clear way to communicate progress to users.
227-237: Error handling
Returning an alert component when an error occurs enriches user feedback and helps prevent silent failures.
240-241: Header design
The two-line structure, with a page heading and a button, follows common UI patterns and is easy to follow.
248-279: Search input and sorting dropdown
Providing a quick search bar alongside sorting menu items improves usability by enabling users to quickly locate and reorder client entries.
281-379: Table structure
The table neatly displays client information, with an intuitive way to generate or copy tokens. The fallback row for empty sets is a thoughtful inclusion.
382-418: Pagination controls
Offering next/previous buttons with page links in between provides a straightforward way to navigate through multiple pages of clients.
420-421: Client form dialogs
The modals for editing and creating clients maintain a clean layout, compartmentalizing actions effectively.
| const client = clients?.find((client: Client) => client._id === clientId) | ||
| return client?.access_token?.token ? true : false |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify the boolean expression
To avoid using a ternary for a boolean check, consider returning a double negation:
-return client?.access_token?.token ? true : false
+return !!client?.access_token?.token📝 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.
| const client = clients?.find((client: Client) => client._id === clientId) | |
| return client?.access_token?.token ? true : false | |
| const client = clients?.find((client: Client) => client._id === clientId) | |
| return !!client?.access_token?.token |
🧰 Tools
🪛 Biome (1.9.4)
[error] 140-140: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
(lint/complexity/noUselessTernary)
| if (sortField === "isActive") { | ||
| return sortOrder === "asc" ? Number(b.isActive) - Number(a.isActive) : Number(a.isActive) - Number(b.isActive) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Double-check 'isActive' sorting logic
Currently, for ascending order, the code places active clients last (which might be reversed from typical ascending expectations). Confirm this is intentional.
If you want active clients first for ascending order, consider changing the logic to:
-return sortOrder === "asc" ? Number(b.isActive) - Number(a.isActive) : Number(a.isActive) - Number(b.isActive)
+return sortOrder === "asc" ? Number(a.isActive) - Number(b.isActive) : Number(b.isActive) - Number(a.isActive)📝 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.
| if (sortField === "isActive") { | |
| return sortOrder === "asc" ? Number(b.isActive) - Number(a.isActive) : Number(a.isActive) - Number(b.isActive) | |
| } | |
| if (sortField === "isActive") { | |
| - return sortOrder === "asc" ? Number(b.isActive) - Number(a.isActive) : Number(a.isActive) - Number(b.isActive) | |
| + return sortOrder === "asc" ? Number(a.isActive) - Number(b.isActive) : Number(b.isActive) - Number(a.isActive) | |
| } |
- Reorganize interface definitions in useGroups and useRoles hooks - Remove unnecessary type imports and adjust type annotations - Simplify React Query configuration in useRoles hook - Minor code structure improvements for better readability
- Modify industry options in Create-group component to use more precise labels - Remove commented placeholder code in organization-related components - Standardize industry value and label formatting
- Remove commented placeholder code for device and member creation - Streamline site creation mutation by passing only the new group ID - Maintain clean and focused code structure in CreateOrganizationDialog component
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
netmanager-app/components/Organization/Create-group.tsx (1)
413-413:⚠️ Potential issueFix form field access in device and member forms.
The code is trying to access non-existent
fieldsproperty. This is a duplicate of past review comments but hasn't been addressed yet.- {addDevicesForm.fields.devices.map((field, index) => ( + {addDevicesForm.getValues().devices.map((_, index) => (- {inviteMembersForm.fields.members.map((field, index) => ( + {inviteMembersForm.getValues().members.map((_, index) => (Also applies to: 476-476
🧹 Nitpick comments (3)
netmanager-app/components/Organization/Create-group.tsx (3)
73-81: Standardize the casing in industry values.The industry values have inconsistent casing. Some values start with uppercase ("Manufacturing", "Healthcare") while others are lowercase ("healthcare", "finance").
const industries = [ { value: "Manufacturing", label: "Manufacturing" }, { value: "Information Technology", label: "Information Technology" }, - { value: "healthcare", label: "Healthcare" }, - { value: "finance", label: "Finance" }, - { value: "education", label: "Education" }, - { value: "retail", label: "Retail" }, - { value: "other", label: "Other" }, + { value: "Healthcare", label: "Healthcare" }, + { value: "Finance", label: "Finance" }, + { value: "Education", label: "Education" }, + { value: "Retail", label: "Retail" }, + { value: "Other", label: "Other" }, ]
97-97: Reset form state when dialog is closed.The dialog state and forms should be reset when the dialog is closed without submission to ensure a clean state for the next opening.
const [open, setOpen] = useState(false) + + const handleOpenChange = (newOpen: boolean) => { + setOpen(newOpen) + if (!newOpen) { + resetForms() + } + } return ( - <Dialog open={open} onOpenChange={setOpen}> + <Dialog open={open} onOpenChange={handleOpenChange}>Also applies to: 542-542
193-199: Enhance error handling with specific error messages.The error handling could be more specific to help users understand and resolve issues.
} catch (error) { + const errorMessage = error instanceof Error + ? error.message + : "An unexpected error occurred. Please try again."; toast({ title: "Error", - description: "Failed to create organization. Please try again.", + description: errorMessage, variant: "destructive", }) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
netmanager-app/components/Organization/Create-group.tsx(1 hunks)netmanager-app/components/Organization/organization-roles.tsx(1 hunks)netmanager-app/components/Organization/team-members.tsx(1 hunks)netmanager-app/core/hooks/useGroups.ts(1 hunks)netmanager-app/core/hooks/useRoles.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- netmanager-app/core/hooks/useRoles.ts
- netmanager-app/components/Organization/organization-roles.tsx
- netmanager-app/components/Organization/team-members.tsx
🔇 Additional comments (4)
netmanager-app/core/hooks/useGroups.ts (4)
28-37: Add proper typing for the success callback.The
anytype in the onSuccess callback should be replaced with a proper interface.
67-82: LGTM! Well-implemented mutation hook.The hook demonstrates good practices:
- Proper typing with Partial
- Appropriate query invalidation
- Good error handling with fallback message
104-123: LGTM! Well-structured hook with proper typing.The implementation demonstrates good practices with proper type safety and error handling.
132-140: Improve error handling in useInviteUserToGroup.The member check uses
anytype and the error message could be more descriptive.
| interface TeamMembersResponse { | ||
| group_members: Array<{ | ||
| id: string; | ||
|
|
||
| }>; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Complete the TeamMembersResponse interface.
The interface is missing essential member properties. Consider adding properties like email, name, role, etc., to ensure proper type safety throughout the application.
interface TeamMembersResponse {
group_members: Array<{
id: string;
+ email: string;
+ name: string;
+ role: string;
+ status: string;
+ createdAt: string;
+ updatedAt: string;
}>;
}📝 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.
| interface TeamMembersResponse { | |
| group_members: Array<{ | |
| id: string; | |
| }>; | |
| } | |
| interface TeamMembersResponse { | |
| group_members: Array<{ | |
| id: string; | |
| email: string; | |
| name: string; | |
| role: string; | |
| status: string; | |
| createdAt: string; | |
| updatedAt: string; | |
| }>; | |
| } |
| onError: (error: AxiosError<ErrorResponse>) => { | ||
| console.error("Failed to create group:", error.response?.data?.message); | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Maintain consistent error handling pattern.
Instead of logging to console, dispatch the error to Redux to maintain consistency with other hooks.
-onError: (error: AxiosError<ErrorResponse>) => {
- console.error("Failed to create group:", error.response?.data?.message);
-},
+onError: (error: AxiosError<ErrorResponse>) => {
+ dispatch(setError(error.response?.data?.message || "Failed to create group"));
+},📝 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.
| onError: (error: AxiosError<ErrorResponse>) => { | |
| console.error("Failed to create group:", error.response?.data?.message); | |
| }, | |
| onError: (error: AxiosError<ErrorResponse>) => { | |
| dispatch(setError(error.response?.data?.message || "Failed to create group")); | |
| }, |
| const { data, isLoading, error } = useQuery({ | ||
| queryKey: ["groupDetails", groupId], | ||
| queryFn: () => groups.getGroupDetailsApi(groupId), | ||
| onSuccess: (data: any) => { | ||
| dispatch(setGroup(data.group)); | ||
| }, | ||
| onError: (error: Error) => { | ||
| dispatch(setError(error.message)); | ||
| }, | ||
| }) | ||
|
|
||
| return { | ||
| group: data?.group ?? [], | ||
| isLoading, | ||
| error, | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type safety and fix default value.
Two issues need attention:
- The
anytype in onSuccess callback should be replaced with proper typing - The default value for group should be
nullinstead of an empty array
+interface GroupDetailsResponse {
+ group: Group;
+}
-const { data, isLoading, error } = useQuery({
+const { data, isLoading, error } = useQuery<GroupDetailsResponse, Error>({
queryKey: ["groupDetails", groupId],
queryFn: () => groups.getGroupDetailsApi(groupId),
- onSuccess: (data: any) => {
+ onSuccess: (data: GroupDetailsResponse) => {
dispatch(setGroup(data.group));
},
onError: (error: Error) => {
dispatch(setError(error.message));
},
})
return {
- group: data?.group ?? [],
+ group: data?.group ?? null,
isLoading,
error,
};📝 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.
| const { data, isLoading, error } = useQuery({ | |
| queryKey: ["groupDetails", groupId], | |
| queryFn: () => groups.getGroupDetailsApi(groupId), | |
| onSuccess: (data: any) => { | |
| dispatch(setGroup(data.group)); | |
| }, | |
| onError: (error: Error) => { | |
| dispatch(setError(error.message)); | |
| }, | |
| }) | |
| return { | |
| group: data?.group ?? [], | |
| isLoading, | |
| error, | |
| }; | |
| interface GroupDetailsResponse { | |
| group: Group; | |
| } | |
| const { data, isLoading, error } = useQuery<GroupDetailsResponse, Error>({ | |
| queryKey: ["groupDetails", groupId], | |
| queryFn: () => groups.getGroupDetailsApi(groupId), | |
| onSuccess: (data: GroupDetailsResponse) => { | |
| dispatch(setGroup(data.group)); | |
| }, | |
| onError: (error: Error) => { | |
| dispatch(setError(error.message)); | |
| }, | |
| }); | |
| return { | |
| group: data?.group ?? null, | |
| isLoading, | |
| error, | |
| }; |
…ion with Group Management - Replace manual site and device creation with checkbox-based selection - Add support for selecting existing sites and devices - Implement group assignment logic for selected sites and devices - Update form validation and schema to support new selection approach - Remove deprecated input fields and commented code - Enhance UI with dynamic group tag display
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
netmanager-app/components/Organization/Create-group.tsx (2)
613-613: Consider accessibility for long forms.The dialog's max height constraint with overflow might cause accessibility issues for keyboard users.
- <DialogContent className="sm:max-w-[600px] max-h-[80vh] overflow-y-auto"> + <DialogContent className="sm:max-w-[600px] max-h-[80vh] overflow-y-auto focus:outline-none">
392-419: Add loading states for site checkboxes.The site selection UI doesn't handle loading states for individual sites, which could lead to a poor user experience.
<Checkbox id={`site-${site._id}`} + disabled={isLoadingSites} checked={addSitesForm.getValues().sites.some((s) => s._id === site._id)} onCheckedChange={(checked) => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
netmanager-app/components/Organization/Create-group.tsx(1 hunks)
🔇 Additional comments (4)
netmanager-app/components/Organization/Create-group.tsx (4)
30-35: Enhance URL validation for better security.The URL validation suggestion from the past review is still valid and should be implemented to improve security.
183-183: Type the form submission data properly.The suggestion to type the form submission data from the past review is still valid and should be implemented.
541-541: Fix incorrect form field access.The suggestion to fix the form field access from the past review is still valid and should be implemented.
624-629: Replace placeholder API with actual implementation.The suggestion to replace the placeholder API from the past review is still valid and should be implemented.
| const { sites: groupsData, isLoading: isLoadingGroups, error: groupsError } = useSites() | ||
| const { data: allSites, isLoading: isLoadingSites } = useQuery(["sites"], () => sites.getAllSites()) | ||
| const { data: allDevices, isLoading: isLoadingDevices } = useQuery(["devices"], () => sites.getAllDevices()) |
There was a problem hiding this comment.
Add error handling for data fetching.
The queries for sites and devices lack error handling, which could lead to runtime errors if the requests fail.
- const { data: allSites, isLoading: isLoadingSites } = useQuery(["sites"], () => sites.getAllSites())
- const { data: allDevices, isLoading: isLoadingDevices } = useQuery(["devices"], () => sites.getAllDevices())
+ const { data: allSites, isLoading: isLoadingSites, error: sitesError } = useQuery(["sites"], () => sites.getAllSites())
+ const { data: allDevices, isLoading: isLoadingDevices, error: devicesError } = useQuery(["devices"], () => sites.getAllDevices())📝 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.
| const { sites: groupsData, isLoading: isLoadingGroups, error: groupsError } = useSites() | |
| const { data: allSites, isLoading: isLoadingSites } = useQuery(["sites"], () => sites.getAllSites()) | |
| const { data: allDevices, isLoading: isLoadingDevices } = useQuery(["devices"], () => sites.getAllDevices()) | |
| const { sites: groupsData, isLoading: isLoadingGroups, error: groupsError } = useSites() | |
| const { data: allSites, isLoading: isLoadingSites, error: sitesError } = useQuery(["sites"], () => sites.getAllSites()) | |
| const { data: allDevices, isLoading: isLoadingDevices, error: devicesError } = useQuery(["devices"], () => sites.getAllDevices()) |
| const sitesToUpdate = finalData.sites.map((site) => ({ | ||
| ...site, | ||
| groups: [...site.groups, newGroupId], | ||
| })) | ||
| await updateSitesMutation.mutateAsync({ | ||
| siteIds: sitesToUpdate.map((site) => site._id), | ||
| updateData: { groups: sitesToUpdate[0].groups }, | ||
| }) |
There was a problem hiding this comment.
Improve error handling for batch updates.
The site update logic assumes all sites have the same groups, which might not be accurate. Each site should maintain its own group list.
- await updateSitesMutation.mutateAsync({
- siteIds: sitesToUpdate.map((site) => site._id),
- updateData: { groups: sitesToUpdate[0].groups },
- })
+ await Promise.all(
+ sitesToUpdate.map((site) =>
+ updateSitesMutation.mutateAsync({
+ siteIds: [site._id],
+ updateData: { groups: site.groups },
+ })
+ )
+ )📝 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.
| const sitesToUpdate = finalData.sites.map((site) => ({ | |
| ...site, | |
| groups: [...site.groups, newGroupId], | |
| })) | |
| await updateSitesMutation.mutateAsync({ | |
| siteIds: sitesToUpdate.map((site) => site._id), | |
| updateData: { groups: sitesToUpdate[0].groups }, | |
| }) | |
| const sitesToUpdate = finalData.sites.map((site) => ({ | |
| ...site, | |
| groups: [...site.groups, newGroupId], | |
| })) | |
| - await updateSitesMutation.mutateAsync({ | |
| - siteIds: sitesToUpdate.map((site) => site._id), | |
| - updateData: { groups: sitesToUpdate[0].groups }, | |
| - }) | |
| + await Promise.all( | |
| + sitesToUpdate.map((site) => | |
| + updateSitesMutation.mutateAsync({ | |
| + siteIds: [site._id], | |
| + updateData: { groups: site.groups }, | |
| + }) | |
| + ) | |
| + ) |
- Install @types/react-select-country-list as a dev dependency - Update package-lock.json and package.json to include type definitions - Prepare for improved type safety when using react-select-country-list
- Replace useSites with specialized useGroups, useSites, and useDevices hooks - Simplify data fetching and error handling for groups, sites, and devices - Update component to use new hook return types and error management - Fix apostrophe escaping in form descriptions
…Experience - Update dialog layout with a wider content area and progress indicator - Implement multi-step navigation with chevron icons and back/next buttons - Add Card and Avatar components to improve site and device selection UI - Refactor form layout with grid-based design and improved spacing - Enhance visual hierarchy and interaction with progress tracking - Add dynamic avatar generation for sites and devices - Improve form step transitions and user guidance
…ialog - Implement search filters for sites and devices with real-time filtering - Add search input fields with Lucide search icon - Introduce state management for site and device search - Use useMemo for efficient filtering of sites and devices - Add icon decorations to form labels for improved visual hierarchy - Disable "Next" buttons when no sites or devices are selected - Improve overall user experience with interactive search and selection
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
netmanager-app/components/Organization/Create-group.tsx (3)
50-55:⚠️ Potential issueEnhance URL validation for better security.
The URL validation for website and profile picture needs stronger security constraints.
Apply this diff to improve URL validation:
grp_website: z .string() .url({ message: "Please enter a valid URL." }) + .regex(/^https:\/\//, { message: "URL must use HTTPS protocol" }) + .max(200, { message: "URL must not exceed 200 characters" }) .optional() .or(z.literal("")), grp_profile_picture: z .string() .url({ message: "Please enter a valid URL for the profile picture." }) + .regex(/^https:\/\//, { message: "URL must use HTTPS protocol" }) + .regex(/\.(jpg|jpeg|png|gif)$/, { message: "URL must point to an image file" }) + .max(200, { message: "URL must not exceed 200 characters" }) .optional() .or(z.literal("")),
211-211: 🛠️ Refactor suggestionType the form submission data properly.
Using
anytype for form data reduces type safety.Create a proper interface combining all form schemas:
+interface OrganizationFormData extends + z.infer<typeof createOrgSchema> & + z.infer<typeof addSitesSchema> & + z.infer<typeof addDevicesSchema> & + z.infer<typeof inviteMembersSchema> {} -const onSubmit = async (data: any) => { +const onSubmit = async (data: OrganizationFormData) => {
641-641:⚠️ Potential issueFix incorrect form field access.
The
fieldsproperty doesn't exist on the form object.Apply this diff:
- {inviteMembersForm.fields.members.map((field, index) => ( + {inviteMembersForm.getValues().members.map((_, index) => (
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
netmanager-app/components/Organization/Create-group.tsx(1 hunks)
🔇 Additional comments (1)
netmanager-app/components/Organization/Create-group.tsx (1)
747-751: Replace placeholder API with actual implementation.The
createOrganizationfunction is still using a placeholder implementation.Run the following script to verify organization-related API implementations:
#!/bin/bash # Search for organization-related API implementations rg -A 5 "createOrganization|createGroup" --type ts --type tsx
| await updateSitesMutation.mutateAsync({ | ||
| siteIds: sitesToUpdate.map((site) => site._id), | ||
| updateData: { groups: sitesToUpdate[0].groups }, | ||
| }) |
There was a problem hiding this comment.
Improve batch update logic for sites and devices.
The current implementation assumes all sites/devices should have the same groups, which might not be accurate. Each item should maintain its own group list.
Apply this diff to fix the batch update logic:
- await updateSitesMutation.mutateAsync({
- siteIds: sitesToUpdate.map((site) => site._id),
- updateData: { groups: sitesToUpdate[0].groups },
- })
+ await Promise.all(
+ sitesToUpdate.map((site) =>
+ updateSitesMutation.mutateAsync({
+ siteIds: [site._id],
+ updateData: { groups: site.groups },
+ })
+ )
+ )
- await updateDevicesMutation.mutateAsync({
- deviceIds: devicesToUpdate.map((device) => device._id),
- updateData: { groups: devicesToUpdate[0].groups },
- })
+ await Promise.all(
+ devicesToUpdate.map((device) =>
+ updateDevicesMutation.mutateAsync({
+ deviceIds: [device._id],
+ updateData: { groups: device.groups },
+ })
+ )
+ )Also applies to: 239-242
…rove State Management - Streamline form logic by removing unnecessary state and search functionality - Simplify site and device selection with direct select input - Remove complex filtering and checkbox-based selection - Improve mutation handling with more focused API calls - Enhance form step navigation and error handling - Remove unused imports and simplify component structure
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
netmanager-app/components/Organization/Create-group.tsx(1 hunks)
🔇 Additional comments (4)
netmanager-app/components/Organization/Create-group.tsx (4)
44-49: Enhance URL validation for better security.The URL validation needs to be more specific and secure.
Apply this diff to improve URL validation:
grp_website: z .string() .url({ message: "Please enter a valid URL." }) + .regex(/^https:\/\//, { message: "URL must use HTTPS protocol" }) + .max(200, { message: "URL must not exceed 200 characters" }) .optional() .or(z.literal("")), grp_profile_picture: z .string() .url({ message: "Please enter a valid URL for the profile picture." }) + .regex(/^https:\/\//, { message: "URL must use HTTPS protocol" }) + .regex(/\.(jpg|jpeg|png|gif)$/, { message: "URL must point to an image file" }) + .max(200, { message: "URL must not exceed 200 characters" }) .optional() .or(z.literal("")),
678-682: Replace placeholder API with actual implementation.The
createOrganizationfunction is still using a simulated API call with hardcoded response.Run the following script to verify organization-related API implementations:
#!/bin/bash # Search for organization-related API implementations rg -A 5 "createOrganization|createGroup" --type ts --type tsx
240-243: Improve batch update logic for sites and devices.The current implementation assumes all sites/devices should have the same groups, which might not be accurate.
Apply this diff to fix the batch update logic:
- await updateSitesMutation.mutateAsync({ - siteIds: sitesToUpdate.map((site) => site._id), - updateData: { groups: sitesToUpdate[0].groups }, - }) + await Promise.all( + sitesToUpdate.map((site) => + updateSitesMutation.mutateAsync({ + siteIds: [site._id], + updateData: { groups: site.groups }, + }) + ) + ) - await updateDevicesMutation.mutateAsync({ - deviceIds: devicesToUpdate.map((device) => device._id), - updateData: { groups: devicesToUpdate[0].groups }, - }) + await Promise.all( + devicesToUpdate.map((device) => + updateDevicesMutation.mutateAsync({ + deviceIds: [device._id], + updateData: { groups: device.groups }, + }) + ) + )Also applies to: 262-265
651-674: Well-implemented dialog with proper accessibility!The dialog implementation follows accessibility best practices:
- Uses proper ARIA attributes through the UI components
- Shows clear progress indication
- Handles loading states appropriately
| const inviteMembers = async (data: z.infer<typeof inviteMembersSchema>) => { | ||
| // Simulating API call | ||
| await new Promise((resolve) => setTimeout(resolve, 1000)) | ||
| return { success: true, invitedMembers: data.members.length } | ||
| } |
There was a problem hiding this comment.
Replace placeholder API with actual implementation.
The inviteMembers function is using a simulated API call with hardcoded response.
Replace the placeholder with actual API implementation that sends invitations to the provided email addresses.
…nctionality - Enhance groupsApi with new methods for device and site group assignment - Update useGroups hooks to support new API methods with improved error handling - Add new hooks for assigning devices and sites to groups - Implement useInviteUserToGroup with more robust member checking - Add new hooks for accepting group invites and updating group members - Improve type safety and error management in API and hook implementations
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
netmanager-app/core/hooks/useGroups.ts (2)
14-19: 🛠️ Refactor suggestionEnhance TeamMembersResponse interface with complete member properties.
The interface is missing essential member properties.
interface TeamMembersResponse { group_members: Array<{ id: string; + email: string; + name: string; + role: string; + status: string; + joinedAt: string; + lastActive: string; }>; }
152-175: 🛠️ Refactor suggestionImprove error handling in useInviteUserToGroup hook.
The error handling could be more specific and informative.
export const useInviteUserToGroup = (groupId: string) => { const queryClient = useQueryClient() const dispatch = useDispatch() return useMutation({ mutationFn: async (userEmail: string) => { const response = await groupMembers.getGroupMembersApi(groupId) const existingMembers = response.group_members || [] - const isExistingMember = existingMembers.some((member: any) => member.email === userEmail) + const isExistingMember = existingMembers.some( + (member: TeamMembersResponse['group_members'][0]) => + member.email === userEmail + ) if (isExistingMember) { - throw new Error("User is already a member of this group") + throw new Error(`User ${userEmail} is already a member of group ${groupId}`) } return groupMembers.inviteUserToGroupTeam(groupId, userEmail) }, onError: (error: Error) => { - dispatch(setError(error.message || "Failed to invite user to group")) + dispatch(setError( + error.message || + `Failed to invite user to group ${groupId}. Please try again.` + )) }, }) }netmanager-app/components/Organization/Create-group.tsx (1)
37-49: 🛠️ Refactor suggestionEnhance URL validation rules for better security.
The URL validation could be more strict to ensure security.
const createOrgSchema = z.object({ grp_website: z .string() - .url({ message: "Please enter a valid URL." }) + .url({ message: "Please enter a valid URL." }) + .regex(/^https:\/\//, { message: "URL must use HTTPS protocol" }) + .max(2048, { message: "URL is too long" }) .min(1, { message: "Website is required." }), grp_profile_picture: z .string() - .url({ message: "Please enter a valid URL for the profile picture." }) + .url({ message: "Please enter a valid URL for the profile picture." }) + .regex(/^https:\/\//, { message: "URL must use HTTPS protocol" }) + .regex(/\.(jpg|jpeg|png|gif|webp)$/i, { message: "URL must point to an image file" }) + .max(2048, { message: "URL is too long" }) .optional() .or(z.literal("")), })
🧹 Nitpick comments (8)
netmanager-app/core/apis/organizations.ts (3)
1-11: Enhance error handling with error types.Consider creating specific error types for different failure scenarios to improve error handling and debugging.
+class GroupApiError extends Error { + constructor( + message: string, + public statusCode?: number, + public errorCode?: string + ) { + super(message); + this.name = 'GroupApiError'; + } +} interface ErrorResponse { message: string; + statusCode?: number; + errorCode?: string; }
12-78: Add specific response types for API methods.The API methods would benefit from explicit return types to ensure type safety throughout the application.
+interface GroupResponse { + group: Group; + message?: string; +} +interface GroupsResponse { + groups: Group[]; + message?: string; +} export const groupsApi = { - getGroupsApi: async () => { + getGroupsApi: async (): Promise<GroupsResponse> => { - getGroupDetailsApi: async (groupId: string) => { + getGroupDetailsApi: async (groupId: string): Promise<GroupResponse> => {
80-120: Improve type safety in member management methods.The member management methods could benefit from stricter typing and validation.
+interface GroupMember { + email: string; + role: string; + status: string; +} +interface GroupMemberResponse { + member: GroupMember; + message?: string; +} export const groupMembers = { - getGroupMembersApi: async (groupId: string) => { + getGroupMembersApi: async (groupId: string): Promise<{ group_members: GroupMember[] }> => { - inviteUserToGroupTeam: async (groupId: string, userEmail: string) => { + inviteUserToGroupTeam: async (groupId: string, userEmail: string): Promise<GroupMemberResponse> => {netmanager-app/components/Organization/organization-profile.tsx (3)
22-39: Enhance form validation with debouncing.Consider adding debounce to form updates to prevent excessive API calls during typing.
+import { useDebouncedCallback } from 'use-debounce'; export function OrganizationProfile({ organizationId }: OrganizationProfileProps) { const queryClient = useQueryClient() const { group, isLoading, error } = useGroupsDetails(organizationId) + const debouncedUpdate = useDebouncedCallback( + (data: typeof formData) => updateMutation.mutate(data), + 500 + );
41-68: Implement optimistic updates for better UX.Consider implementing optimistic updates to improve the user experience during form submissions.
const updateMutation = useMutation({ mutationFn: (data: typeof formData) => groupsApi.updateGroupDetailsApi(organizationId, data), + onMutate: async (newData) => { + await queryClient.cancelQueries(["groupDetails", organizationId]) + const previousData = queryClient.getQueryData(["groupDetails", organizationId]) + queryClient.setQueryData(["groupDetails", organizationId], { group: newData }) + return { previousData } + }, onSuccess: () => { queryClient.invalidateQueries(["groupDetails", organizationId]) toast({ title: "Profile Updated", description: "The organization profile has been successfully updated.", }) }, + onError: (err, newData, context) => { + queryClient.setQueryData(["groupDetails", organizationId], context?.previousData) + toast({ + title: "Update Failed", + description: "There was an error updating the organization profile. Please try again.", + variant: "destructive", + }) + }, })
78-155: Enhance accessibility with ARIA labels and keyboard navigation.The form could benefit from improved accessibility features.
<Card className="w-full"> - <CardHeader className="bg-primary/5 border-b"> + <CardHeader className="bg-primary/5 border-b" role="banner"> - <CardTitle className="text-3xl font-bold">Organization Profile</CardTitle> + <CardTitle className="text-3xl font-bold" role="heading" aria-level="1"> + Organization Profile + </CardTitle> </CardHeader> - <CardContent className="p-6"> + <CardContent className="p-6" role="main"> <form onSubmit={handleSubmit} className="space-y-8" + role="form" + aria-label="Organization profile form" >netmanager-app/core/hooks/useGroups.ts (1)
23-39: Improve query key structure for better cache management.Consider using more specific query keys for better cache control.
const { data, isLoading, error } = useQuery({ - queryKey: ["groups"], + queryKey: ["organizations", "groups", { type: "summary" }], queryFn: () => groupsApi.getGroupsApi(), onSuccess: (data: any) => { dispatch(setGroups(data.groups)) }, onError: (error: Error) => { dispatch(setError(error.message)) }, })netmanager-app/components/Organization/Create-group.tsx (1)
147-163: Add proper type definitions for form submission data.The form submission could benefit from stricter typing.
+interface CreateOrganizationData extends z.infer<typeof createOrgSchema> { + grp_status?: string; + grp_created_at?: string; + grp_updated_at?: string; +} -const onCreateOrganization = async (data: z.infer<typeof createOrgSchema>) => { +const onCreateOrganization = async (data: CreateOrganizationData) => { try { await createGroup(data) toast({ title: "Organization created", description: `Successfully created organization: ${data.grp_title}`, }) setCurrentStep(2) } catch (error) { + const err = error as Error toast({ title: "Error", - description: `Failed to create organization: ${(error as Error).message}`, + description: `Failed to create organization: ${err.message}`, variant: "destructive", }) - console.error("Error creating organization:", error) + console.error("Error creating organization:", err) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
netmanager-app/components/Organization/Create-group.tsx(1 hunks)netmanager-app/components/Organization/organization-profile.tsx(1 hunks)netmanager-app/core/apis/organizations.ts(1 hunks)netmanager-app/core/hooks/useGroups.ts(1 hunks)
| {memberFields.map((field, index) => ( | ||
| <FormField | ||
| key={field.id} | ||
| control={inviteMembersForm.control} | ||
| name={`members.${index}.email`} | ||
| render={({ field }) => ( | ||
| <FormItem> | ||
| <FormLabel> | ||
| <Users className="w-4 h-4 inline-block mr-2" /> | ||
| Member Email | ||
| </FormLabel> | ||
| <FormControl> | ||
| <Input type="email" placeholder="Enter member email" {...field} /> | ||
| </FormControl> | ||
| <FormMessage /> | ||
| </FormItem> | ||
| )} | ||
| /> | ||
| ))} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance email validation and add duplicate check.
The email validation could be more comprehensive.
+const validateEmails = (emails: string[]) => {
+ const uniqueEmails = new Set(emails);
+ if (uniqueEmails.size !== emails.length) {
+ return "Duplicate email addresses are not allowed";
+ }
+ return true;
+};
{memberFields.map((field, index) => (
<FormField
key={field.id}
control={inviteMembersForm.control}
name={`members.${index}.email`}
render={({ field }) => (
<FormItem>
<FormLabel>
<Users className="w-4 h-4 inline-block mr-2" />
Member Email
</FormLabel>
<FormControl>
<Input
type="email"
placeholder="Enter member email"
{...field}
+ onChange={(e) => {
+ field.onChange(e);
+ const allEmails = memberFields.map(
+ (f, i) => i === index ? e.target.value : f.email
+ );
+ const validation = validateEmails(allEmails);
+ if (typeof validation === "string") {
+ inviteMembersForm.setError(`members.${index}.email`, {
+ message: validation
+ });
+ }
+ }}
/>
</FormControl>
<FormMessage />
</FormItem>
)}
/>
))}📝 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.
| {memberFields.map((field, index) => ( | |
| <FormField | |
| key={field.id} | |
| control={inviteMembersForm.control} | |
| name={`members.${index}.email`} | |
| render={({ field }) => ( | |
| <FormItem> | |
| <FormLabel> | |
| <Users className="w-4 h-4 inline-block mr-2" /> | |
| Member Email | |
| </FormLabel> | |
| <FormControl> | |
| <Input type="email" placeholder="Enter member email" {...field} /> | |
| </FormControl> | |
| <FormMessage /> | |
| </FormItem> | |
| )} | |
| /> | |
| ))} | |
| +const validateEmails = (emails: string[]) => { | |
| + const uniqueEmails = new Set(emails); | |
| + if (uniqueEmails.size !== emails.length) { | |
| + return "Duplicate email addresses are not allowed"; | |
| + } | |
| + return true; | |
| +}; | |
| {memberFields.map((field, index) => ( | |
| <FormField | |
| key={field.id} | |
| control={inviteMembersForm.control} | |
| name={`members.${index}.email`} | |
| render={({ field }) => ( | |
| <FormItem> | |
| <FormLabel> | |
| <Users className="w-4 h-4 inline-block mr-2" /> | |
| Member Email | |
| </FormLabel> | |
| <FormControl> | |
| <Input | |
| type="email" | |
| placeholder="Enter member email" | |
| {...field} | |
| + onChange={(e) => { | |
| + field.onChange(e); | |
| + const allEmails = memberFields.map( | |
| + (f, i) => i === index ? e.target.value : f.email | |
| + ); | |
| + const validation = validateEmails(allEmails); | |
| + if (typeof validation === "string") { | |
| + inviteMembersForm.setError(`members.${index}.email`, { | |
| + message: validation | |
| + }); | |
| + } | |
| + }} | |
| /> | |
| </FormControl> | |
| <FormMessage /> | |
| </FormItem> | |
| )} | |
| /> | |
| ))} |
| {addSitesForm.watch("sites").map((site, index) => ( | ||
| <Card key={site._id} className="p-4"> | ||
| <CardContent className="space-y-2"> | ||
| <div className="font-semibold">{site.name}</div> | ||
| <FormField | ||
| control={addSitesForm.control} | ||
| name={`sites.${index}.groups`} | ||
| render={({ field }) => ( | ||
| <FormItem> | ||
| <FormLabel>Group Titles</FormLabel> | ||
| <FormControl> | ||
| <Input | ||
| placeholder="Enter group titles separated by commas" | ||
| {...field} | ||
| onChange={(e) => field.onChange(e.target.value.split(",").map((s) => s.trim()))} | ||
| /> | ||
| </FormControl> | ||
| <FormDescription>Enter existing group titles and the new group title</FormDescription> | ||
| <FormMessage /> | ||
| </FormItem> | ||
| )} | ||
| /> | ||
| </CardContent> | ||
| </Card> | ||
| ))} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for group assignments in sites.
The group assignment could benefit from additional validation.
+const validateGroupTitles = (titles: string[]) => {
+ const uniqueTitles = new Set(titles);
+ if (uniqueTitles.size !== titles.length) {
+ return "Duplicate group titles are not allowed";
+ }
+ if (titles.some(title => title.length < 2)) {
+ return "Group titles must be at least 2 characters long";
+ }
+ return true;
+};
{addSitesForm.watch("sites").map((site, index) => (
<Card key={site._id} className="p-4">
<CardContent className="space-y-2">
<div className="font-semibold">{site.name}</div>
<FormField
control={addSitesForm.control}
name={`sites.${index}.groups`}
render={({ field }) => (
<FormItem>
<FormLabel>Group Titles</FormLabel>
<FormControl>
<Input
placeholder="Enter group titles separated by commas"
{...field}
onChange={(e) => {
const titles = e.target.value.split(",").map((s) => s.trim());
+ const validation = validateGroupTitles(titles);
+ if (typeof validation === "string") {
+ addSitesForm.setError(`sites.${index}.groups`, {
+ message: validation
+ });
+ return;
+ }
field.onChange(titles);
}}
/>
</FormControl>
<FormDescription>Enter existing group titles and the new group title</FormDescription>
<FormMessage />
</FormItem>
)}
/>
</CardContent>
</Card>
))}📝 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.
| {addSitesForm.watch("sites").map((site, index) => ( | |
| <Card key={site._id} className="p-4"> | |
| <CardContent className="space-y-2"> | |
| <div className="font-semibold">{site.name}</div> | |
| <FormField | |
| control={addSitesForm.control} | |
| name={`sites.${index}.groups`} | |
| render={({ field }) => ( | |
| <FormItem> | |
| <FormLabel>Group Titles</FormLabel> | |
| <FormControl> | |
| <Input | |
| placeholder="Enter group titles separated by commas" | |
| {...field} | |
| onChange={(e) => field.onChange(e.target.value.split(",").map((s) => s.trim()))} | |
| /> | |
| </FormControl> | |
| <FormDescription>Enter existing group titles and the new group title</FormDescription> | |
| <FormMessage /> | |
| </FormItem> | |
| )} | |
| /> | |
| </CardContent> | |
| </Card> | |
| ))} | |
| const validateGroupTitles = (titles: string[]) => { | |
| const uniqueTitles = new Set(titles); | |
| if (uniqueTitles.size !== titles.length) { | |
| return "Duplicate group titles are not allowed"; | |
| } | |
| if (titles.some(title => title.length < 2)) { | |
| return "Group titles must be at least 2 characters long"; | |
| } | |
| return true; | |
| }; | |
| {addSitesForm.watch("sites").map((site, index) => ( | |
| <Card key={site._id} className="p-4"> | |
| <CardContent className="space-y-2"> | |
| <div className="font-semibold">{site.name}</div> | |
| <FormField | |
| control={addSitesForm.control} | |
| name={`sites.${index}.groups`} | |
| render={({ field }) => ( | |
| <FormItem> | |
| <FormLabel>Group Titles</FormLabel> | |
| <FormControl> | |
| <Input | |
| placeholder="Enter group titles separated by commas" | |
| {...field} | |
| onChange={(e) => { | |
| const titles = e.target.value.split(",").map((s) => s.trim()); | |
| const validation = validateGroupTitles(titles); | |
| if (typeof validation === "string") { | |
| addSitesForm.setError(`sites.${index}.groups`, { | |
| message: validation | |
| }); | |
| return; | |
| } | |
| field.onChange(titles); | |
| }} | |
| /> | |
| </FormControl> | |
| <FormDescription> | |
| Enter existing group titles and the new group title | |
| </FormDescription> | |
| <FormMessage /> | |
| </FormItem> | |
| )} | |
| /> | |
| </CardContent> | |
| </Card> | |
| ))} |
- Implement group activation/deactivation functionality in OrganizationList - Add new useActivateGroup hook to handle group status updates - Introduce Power icon button for toggling group status - Add toast notifications for successful and failed status changes - Improve table layout and action button design - Update component to support dynamic group status management
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
netmanager-app/components/Organization/List.tsx (2)
69-76: Move filtering and pagination to the API level.Current implementation performs filtering and pagination client-side, which won't scale well with large datasets. Consider implementing server-side filtering and pagination:
- Update the
useGroupshook to accept pagination parameters and filters- Modify the API to support pagination and filtering
- Return total count from the API for pagination calculations
- Update the component to use server-side operations
This will improve performance and reduce client-side memory usage.
112-127: Improve error handling in handleActivateGroup.The error handling could be more informative and include proper error logging:
} catch (error) { + console.error('Failed to update group status:', error); toast({ title: "Error", - description: "Failed to update group status. Please try again.", + description: error instanceof Error ? error.message : "Failed to update group status. Please try again.", variant: "destructive", }) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
netmanager-app/components/Organization/List.tsx(1 hunks)netmanager-app/core/hooks/useGroups.ts(1 hunks)
🔇 Additional comments (5)
netmanager-app/core/hooks/useGroups.ts (5)
14-18: Complete the TeamMembersResponse interface.The interface is missing essential member properties that would be needed for proper type safety.
interface TeamMembersResponse { group_members: Array<{ id: string; + email: string; + name: string; + role: string; + status: string; + createdAt: string; + updatedAt: string; }>; }
20-39: Add proper typing for the success callback.The
anytype in the onSuccess callback should be replaced with a proper interface.+interface GroupsResponse { + groups: Group[]; +} -onSuccess: (data: any) => { +onSuccess: (data: GroupsResponse) => {
41-60: Improve type safety and fix default value.The hook needs proper typing and consistent default values.
+interface GroupDetailsResponse { + group: Group; +} -const { data, isLoading, error } = useQuery({ +const { data, isLoading, error } = useQuery<GroupDetailsResponse, Error>({ queryKey: ["groupDetails", groupId], queryFn: () => groupsApi.getGroupDetailsApi(groupId), - onSuccess: (data: any) => { + onSuccess: (data: GroupDetailsResponse) => { dispatch(setGroup(data.group)); }, onError: (error: Error) => { dispatch(setError(error.message)); }, }) return { - group: data?.group ?? [], + group: data?.group ?? null, isLoading, error, };
152-175: Improve error handling in useInviteUserToGroup.The member check uses
anytype and the error message could be more descriptive.+interface GroupMember { + email: string; + id: string; +} -const isExistingMember = existingMembers.some((member: any) => member.email === userEmail) +const isExistingMember = existingMembers.some((member: GroupMember) => member.email === userEmail) if (isExistingMember) { - throw new Error("User is already a member of this group") + throw new Error(`User with email ${userEmail} is already a member of this group`) }
73-75: Maintain consistent error handling pattern.Error handling should be consistent across all hooks. Instead of mixing console logging and Redux dispatching, standardize on using Redux for error management.
-onError: (error: AxiosError<ErrorResponse>) => { - console.error("Failed to create group:", error.response?.data?.message); -}, +onError: (error: AxiosError<ErrorResponse>) => { + dispatch(setError(error.response?.data?.message || "Failed to create group")); +},Also applies to: 90-93, 108-110, 125-127, 140-142, 171-173, 187-189, 203-205, 221-223
| const sortOrganizations = (orgsToSort: any[]) => { | ||
| return [...orgsToSort].sort((a, b) => { | ||
| if (sortField === "createdAt") { | ||
| const dateA = new Date(a.createdAt || 0).getTime() | ||
| const dateB = new Date(b.createdAt || 0).getTime() | ||
| return sortOrder === "asc" ? dateA - dateB : dateB - dateA | ||
| } | ||
|
|
||
| let compareA = a[sortField] | ||
| let compareB = b[sortField] | ||
|
|
||
| if (typeof compareA === "string") { | ||
| compareA = compareA.toLowerCase() | ||
| compareB = compareB.toLowerCase() | ||
| } | ||
|
|
||
| if (compareA < compareB) return sortOrder === "asc" ? -1 : 1 | ||
| if (compareA > compareB) return sortOrder === "asc" ? 1 : -1 | ||
| return 0 | ||
| }) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add proper typing to sortOrganizations function.
The function uses any[] type which reduces type safety. Consider adding proper typing:
-const sortOrganizations = (orgsToSort: any[]) => {
+const sortOrganizations = (orgsToSort: Group[]) => {📝 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.
| const sortOrganizations = (orgsToSort: any[]) => { | |
| return [...orgsToSort].sort((a, b) => { | |
| if (sortField === "createdAt") { | |
| const dateA = new Date(a.createdAt || 0).getTime() | |
| const dateB = new Date(b.createdAt || 0).getTime() | |
| return sortOrder === "asc" ? dateA - dateB : dateB - dateA | |
| } | |
| let compareA = a[sortField] | |
| let compareB = b[sortField] | |
| if (typeof compareA === "string") { | |
| compareA = compareA.toLowerCase() | |
| compareB = compareB.toLowerCase() | |
| } | |
| if (compareA < compareB) return sortOrder === "asc" ? -1 : 1 | |
| if (compareA > compareB) return sortOrder === "asc" ? 1 : -1 | |
| return 0 | |
| }) | |
| } | |
| const sortOrganizations = (orgsToSort: Group[]) => { | |
| return [...orgsToSort].sort((a, b) => { | |
| if (sortField === "createdAt") { | |
| const dateA = new Date(a.createdAt || 0).getTime() | |
| const dateB = new Date(b.createdAt || 0).getTime() | |
| return sortOrder === "asc" ? dateA - dateB : dateB - dateA | |
| } | |
| let compareA = a[sortField] | |
| let compareB = b[sortField] | |
| if (typeof compareA === "string") { | |
| compareA = compareA.toLowerCase() | |
| compareB = compareB.toLowerCase() | |
| } | |
| if (compareA < compareB) return sortOrder === "asc" ? -1 : 1 | |
| if (compareA > compareB) return sortOrder === "asc" ? 1 : -1 | |
| return 0 | |
| }) | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
netmanager-app/components/Organization/Create-group.tsx (2)
161-161: Remove console.log statement.Remove the console.log statement from the error handling code as it's not suitable for production.
- console.error("Error creating organization:", error)
207-226: Optimize member invitations with batch processing.The current implementation invites members sequentially, which could be slow for multiple invitations. Consider using batch processing.
const onInviteMembers = async (data: z.infer<typeof inviteMembersSchema>) => { - // const groupId = createOrgForm.getValues().grp_title + const groupId = createOrgForm.getValues().grp_title try { - for (const member of data.members) { - await inviteUserToGroup(member.email) - } + await Promise.all( + data.members.map((member) => + inviteUserToGroup({ email: member.email, groupId }) + ) + ) toast({ title: "Invitations sent", description: "Successfully sent invitations to the new members.",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
netmanager-app/components/Organization/Create-group.tsx(1 hunks)
🔇 Additional comments (4)
netmanager-app/components/Organization/Create-group.tsx (4)
43-48: Enhance URL validation for better security.Consider adding more specific URL validation rules:
- Whitelist allowed protocols (https)
- Validate allowed domains for profile pictures
- Add max length constraints
144-145: Add error handling for data fetching.The queries for sites and devices lack error handling, which could lead to runtime errors if the requests fail.
576-594: Enhance email validation and add duplicate check.The email validation could be more comprehensive.
448-472: Add validation for group assignments in sites.The group assignment could benefit from additional validation.
| const { mutate: createGroup, isLoading: isCreatingGroup } = useCreateGroup() | ||
| const { mutate: assignDevicesToGroup, isLoading: isAssigningDevices } = useAssignDevicesToGroup() | ||
| const { mutate: assignSitesToGroup, isLoading: isAssigningSites } = useAssignSitesToGroup() | ||
| const { mutate: inviteUserToGroup, isLoading: isInvitingMembers } = useInviteUserToGroup("") |
There was a problem hiding this comment.
Fix incorrect hook initialization.
The empty string passed to useInviteUserToGroup("") seems incorrect. This hook should probably receive the group ID as an argument when it's available.
- const { mutate: inviteUserToGroup, isLoading: isInvitingMembers } = useInviteUserToGroup("")
+ const groupId = createOrgForm.getValues().grp_title
+ const { mutate: inviteUserToGroup, isLoading: isInvitingMembers } = useInviteUserToGroup(groupId)📝 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.
| const { mutate: inviteUserToGroup, isLoading: isInvitingMembers } = useInviteUserToGroup("") | |
| const groupId = createOrgForm.getValues().grp_title | |
| const { mutate: inviteUserToGroup, isLoading: isInvitingMembers } = useInviteUserToGroup(groupId) |
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (6)
netmanager-app/components/Organization/organization-profile.tsx (2)
104-106:⚠️ Potential issueHandle empty organization name in avatar fallback.
The avatar fallback could throw an error if the organization name is empty.
Apply this diff to safely handle empty strings:
- <AvatarFallback className="text-2xl"> - {formData.grp_title.slice(0, 2).toUpperCase()} - </AvatarFallback> + <AvatarFallback className="text-2xl"> + {formData.grp_title ? formData.grp_title.slice(0, 2).toUpperCase() : "ORG"} + </AvatarFallback>
33-37:⚠️ Potential issueAdd missing
grp_statusto form data state.The form data state is missing the
grp_statusfield that's used in the UI.Apply this diff to add the missing field:
const [formData, setFormData] = useState({ grp_title: "", grp_description: "", grp_website: "", + grp_status: "", });netmanager-app/core/hooks/useGroups.ts (2)
192-194: 🛠️ Refactor suggestionAdd proper typing for member check.
The member check uses
anytype which reduces type safety.Apply this diff to improve type safety:
+interface GroupMember { + email: string; + id: string; +} - const isExistingMember = existingMembers.some((member: any) => member.email === userEmail); + const isExistingMember = existingMembers.some((member: GroupMember) => member.email === userEmail);
26-28: 🛠️ Refactor suggestionAdd proper typing for success callback.
The
anytype in the onSuccess callback should be replaced with a proper interface.Apply this diff to improve type safety:
+interface GroupsResponse { + groups: Group[]; +} - onSuccess: (data: any) => { + onSuccess: (data: GroupsResponse) => { dispatch(setGroups(data.groups)); },netmanager-app/components/Organization/List.tsx (1)
67-91: 🛠️ Refactor suggestionAdd proper typing to sortOrganizations function.
The function uses
any[]type which reduces type safety.Apply this diff to improve type safety:
-const sortOrganizations = (orgsToSort: any[]) => { +const sortOrganizations = (orgsToSort: Group[]) => {netmanager-app/components/Organization/team-members.tsx (1)
96-96: 🛠️ Refactor suggestionReplace console.error with proper logging.
Direct console logging should be avoided in production code.
Apply this diff to use proper logging:
-console.error("Error inviting member:", error); +logger.error("Error inviting member:", { error, email: newMemberEmail });
🧹 Nitpick comments (7)
netmanager-app/components/Organization/organization-roles.tsx (1)
161-193: Consider server-side pagination for large datasets.
Current pagination model fetches all roles at once. For organizations with many roles, server-side pagination would be more efficient and scalable.netmanager-app/components/Organization/organization-profile.tsx (2)
59-67: Improve error message in updateMutation.The error message could be more descriptive to help with debugging.
Apply this diff to enhance error handling:
onError: (error: Error) => { console.error("Failed to update organization profile", error); toast({ title: "Update Failed", description: - "There was an error updating the organization profile. Please try again.", + `Failed to update organization profile: ${error.message || "Unknown error"}. Please try again.`, variant: "destructive", }); },
77-80: Add loading state to submit button.The submit button should be disabled during form submission.
Apply this diff to improve user experience:
const handleSubmit = (e: React.FormEvent) => { e.preventDefault(); + if (updateMutation.isLoading) return; updateMutation.mutate(formData); };netmanager-app/components/Organization/team-members.tsx (1)
321-369: Improve dialog state management.The current implementation uses a shared dialog state which could lead to race conditions.
Apply this diff to use member-specific dialog states:
-const [isDialogOpen, setIsDialogOpen] = useState(false); +const [openDialogId, setOpenDialogId] = useState<string | null>(null); // Then in the Dialog component: -<Dialog open={isDialogOpen} onOpenChange={setIsDialogOpen}> +<Dialog + open={openDialogId === member._id} + onOpenChange={(open) => setOpenDialogId(open ? member._id : null)} +>netmanager-app/components/Organization/create-group.tsx (3)
62-82: Consider strengthening the URL validation for website and profile picture.The current URL validation only checks if the string is a valid URL format. Consider adding additional validation to:
- Ensure HTTPS protocol
- Validate against allowed domains
- Check for common image file extensions for profile pictures
const createOrgSchema = z.object({ // ... other fields ... grp_website: z .string() - .url({ message: "Please enter a valid URL." }) + .url({ message: "Please enter a valid HTTPS URL." }) + .refine((url) => url.startsWith('https://'), { message: "URL must use HTTPS protocol" }) .min(1, { message: "Website is required." }), grp_profile_picture: z .string() - .url({ message: "Please enter a valid URL for the profile picture." }) + .url({ message: "Please enter a valid HTTPS URL for the profile picture." }) + .refine((url) => url.startsWith('https://'), { message: "URL must use HTTPS protocol" }) + .refine( + (url) => /\.(jpg|jpeg|png|gif|webp)$/i.test(url), + { message: "URL must point to a valid image file" } + ) .optional() .or(z.literal("")), });
716-716: Consider accessibility improvements for the dialog.The dialog's max height and overflow settings might cause accessibility issues. Consider adding ARIA attributes and ensuring keyboard navigation works with overflow content.
- <DialogContent className="sm:max-w-[900px] max-h-[85vh] overflow-y-auto"> + <DialogContent + className="sm:max-w-[900px] max-h-[85vh] overflow-y-auto" + role="dialog" + aria-modal="true" + aria-labelledby="organization-dialog-title" + > <DialogHeader> - <DialogTitle className="text-2xl font-bold"> + <DialogTitle id="organization-dialog-title" className="text-2xl font-bold">
377-382: Consider dynamic timezone options.The timezone options are hardcoded with only two additional cities. Consider making this more dynamic or region-based.
- timezones={{ - ...allTimezones, - "America/Lima": "Lima", - "Europe/London": "London", - }} + timezones={allTimezones} + labelStyle="original" + className="timezone-select"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
netmanager-app/app/(authenticated)/organizations/[id]/page.tsx(1 hunks)netmanager-app/app/(authenticated)/organizations/page.tsx(1 hunks)netmanager-app/components/Organization/List.tsx(1 hunks)netmanager-app/components/Organization/create-group.tsx(1 hunks)netmanager-app/components/Organization/organization-profile.tsx(1 hunks)netmanager-app/components/Organization/organization-roles.tsx(1 hunks)netmanager-app/components/Organization/team-members.tsx(1 hunks)netmanager-app/core/hooks/useGroups.ts(1 hunks)netmanager-app/core/hooks/useRoles.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- netmanager-app/app/(authenticated)/organizations/page.tsx
🔇 Additional comments (7)
netmanager-app/components/Organization/organization-roles.tsx (3)
70-73: Remove console error usage for better error handling.
Consider switching to user-facing feedback (e.g., toast/error notification) instead ofconsole.error.
90-96: Remove console error usage for better error handling.
Consider switching to user-facing feedback (e.g., toast/error notification) instead ofconsole.error.
306-310: Replace browser prompt with a custom modal.
A modal ensures consistent styling, validation, and user experience compared to a nativeprompt.netmanager-app/core/hooks/useRoles.ts (1)
38-38: Remove "any" type and use proper typing.
Define and use a specific interface or type for the query results to maximize type safety.netmanager-app/app/(authenticated)/organizations/[id]/page.tsx (1)
37-68: Implement error handling and loading states for invalid or inaccessible data.
If organization data is missing or cannot be loaded, show an appropriate error or fallback message for better user experience.netmanager-app/components/Organization/List.tsx (1)
257-263: LGTM! Good empty state handling.The empty state is properly handled with a clear message when no organizations are found.
netmanager-app/components/Organization/create-group.tsx (1)
272-278: LGTM: Proper form reset implementation.The
resetFormsfunction correctly resets all forms and the step counter, ensuring a clean state for new organization creation.
| export const useRoles = () => { | ||
| const dispatch = useAppDispatch(); | ||
|
|
||
| const { data, isLoading, error } = useQuery<AxiosError<ErrorResponse>>({ |
There was a problem hiding this comment.
Incorrect usage of type parameters in useQuery.
Currently, useQuery<AxiosError<ErrorResponse>> sets the data type to the error rather than the actual response data. Adjust the generic parameters to correctly reflect the returned data shape.
|
|
||
| export const useAssignDevicesToGroup = () => { | ||
| const queryClient = useQueryClient(); | ||
| const dispatch = useDispatch(); | ||
|
|
||
| return useMutation({ | ||
| mutationFn: ({ | ||
| deviceIds, | ||
| groups, | ||
| }: { | ||
| deviceIds: string[]; | ||
| groups: string[]; | ||
| }) => groupsApi.assignDevicesToGroupApi(deviceIds, groups), | ||
| onSuccess: () => { | ||
| queryClient.invalidateQueries(["groups"]); | ||
| queryClient.invalidateQueries(["devices"]); | ||
| }, | ||
| onError: (error: AxiosError<ErrorResponse>) => { |
There was a problem hiding this comment.
Implement the role update logic.
The role update logic is incomplete and only shows success messages without actual updates.
Apply this diff to implement the update logic:
const handleUpdateRole = async () => {
- if (!selectedMember || !newRoleId) return;
+ if (!selectedMember) {
+ toast({
+ title: "Error",
+ description: "No member selected for role update",
+ variant: "destructive",
+ });
+ return;
+ }
+ if (!newRoleId) {
+ toast({
+ title: "Error",
+ description: "Please select a role to assign",
+ variant: "destructive",
+ });
+ return;
+ }
try {
+ await groupMembers.updateGroupTeam(selectedMember._id, selectedMember.email, newRoleId);
toast({
title: "Role updated",
description: "The member's role has been successfully updated.",
});
setIsDialogOpen(false);
} catch (error) {
- console.error("Error updating role:", error);
+ const errorMessage = error instanceof Error ? error.message : "Unknown error";
toast({
title: "Error",
- description: "Failed to update role. Please try again.",
+ description: `Failed to update role: ${errorMessage}. Please try again.`,
variant: "destructive",
});
}
};📝 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.
| export const useAssignDevicesToGroup = () => { | |
| const queryClient = useQueryClient(); | |
| const dispatch = useDispatch(); | |
| return useMutation({ | |
| mutationFn: ({ | |
| deviceIds, | |
| groups, | |
| }: { | |
| deviceIds: string[]; | |
| groups: string[]; | |
| }) => groupsApi.assignDevicesToGroupApi(deviceIds, groups), | |
| onSuccess: () => { | |
| queryClient.invalidateQueries(["groups"]); | |
| queryClient.invalidateQueries(["devices"]); | |
| }, | |
| onError: (error: AxiosError<ErrorResponse>) => { | |
| const handleUpdateRole = async () => { | |
| if (!selectedMember) { | |
| toast({ | |
| title: "Error", | |
| description: "No member selected for role update", | |
| variant: "destructive", | |
| }); | |
| return; | |
| } | |
| if (!newRoleId) { | |
| toast({ | |
| title: "Error", | |
| description: "Please select a role to assign", | |
| variant: "destructive", | |
| }); | |
| return; | |
| } | |
| try { | |
| await groupMembers.updateGroupTeam(selectedMember._id, selectedMember.email, newRoleId); | |
| toast({ | |
| title: "Role updated", | |
| description: "The member's role has been successfully updated.", | |
| }); | |
| setIsDialogOpen(false); | |
| } catch (error) { | |
| const errorMessage = error instanceof Error ? error.message : "Unknown error"; | |
| toast({ | |
| title: "Error", | |
| description: `Failed to update role: ${errorMessage}. Please try again.`, | |
| variant: "destructive", | |
| }); | |
| } | |
| }; |
| <Input | ||
| id="email" | ||
| type="email" | ||
| placeholder="Enter email to invite" | ||
| value={newMemberEmail} | ||
| onChange={(e) => setNewMemberEmail(e.target.value)} | ||
| required | ||
| /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add email validation before submission.
The email input should be validated before sending the invitation.
Add email validation utility:
// Add to imports
import { isValidEmail } from "@/utils/validation";
// Create in @/utils/validation.ts if not exists
export const isValidEmail = (email: string): boolean => {
const emailRegex = /^[^\s@]+@[^\s@]+\.[^\s@]+$/;
return emailRegex.test(email);
}Then apply this diff:
const handleInvite = async (e: React.FormEvent) => {
e.preventDefault();
+ if (!isValidEmail(newMemberEmail)) {
+ toast({
+ title: "Invalid Email",
+ description: "Please enter a valid email address",
+ variant: "destructive",
+ });
+ return;
+ }
inviteUserMutation.mutate(newMemberEmail, {| }; | ||
|
|
||
| const onAddSites = async (data: z.infer<typeof addSitesSchema>) => { | ||
| const groupId = createOrgForm.getValues().grp_title; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use state management for storing the group ID.
Using getValues() to retrieve the group ID in multiple places is error-prone. Consider storing the group ID in state after organization creation.
+ const [groupId, setGroupId] = useState<string>("");
const onCreateOrganization = async (data: z.infer<typeof createOrgSchema>) => {
try {
await createGroup(data);
+ setGroupId(data.grp_title);
// ... rest of the code
const onAddSites = async (data: z.infer<typeof addSitesSchema>) => {
- const groupId = createOrgForm.getValues().grp_title;
try {
await assignSitesToGroup({
siteIds: data.sites.map((site) => site._id),
groups: [groupId],
});
const onAddDevices = async (data: z.infer<typeof addDevicesSchema>) => {
- const groupId = createOrgForm.getValues().grp_title;
try {
await assignDevicesToGroup({
deviceIds: data.devices.map((device) => device._id),
groups: [groupId],
});Also applies to: 230-230
| const onInviteMembers = async (data: z.infer<typeof inviteMembersSchema>) => { | ||
| // const groupId = createOrgForm.getValues().grp_title | ||
| try { | ||
| for (const member of data.members) { | ||
| await inviteUserToGroup(member.email); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize member invitation process.
The current implementation invites members sequentially, which could be slow for multiple invitations. Consider using Promise.all for parallel processing.
const onInviteMembers = async (data: z.infer<typeof inviteMembersSchema>) => {
try {
- for (const member of data.members) {
- await inviteUserToGroup(member.email);
- }
+ await Promise.all(
+ data.members.map((member) => inviteUserToGroup(member.email))
+ );
// ... rest of the code📝 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.
| const onInviteMembers = async (data: z.infer<typeof inviteMembersSchema>) => { | |
| // const groupId = createOrgForm.getValues().grp_title | |
| try { | |
| for (const member of data.members) { | |
| await inviteUserToGroup(member.email); | |
| } | |
| const onInviteMembers = async (data: z.infer<typeof inviteMembersSchema>) => { | |
| // const groupId = createOrgForm.getValues().grp_title | |
| try { | |
| await Promise.all( | |
| data.members.map((member) => inviteUserToGroup(member.email)) | |
| ); | |
| // ... rest of the code | |
| } catch (error) { | |
| // error handling logic (if any) | |
| } | |
| }; |
| const { mutate: assignSitesToGroup, isLoading: isAssigningSites } = | ||
| useAssignSitesToGroup(); | ||
| const { mutate: inviteUserToGroup, isLoading: isInvitingMembers } = | ||
| useInviteUserToGroup(""); |
There was a problem hiding this comment.
Fix the empty string parameter in useInviteUserToGroup hook.
The useInviteUserToGroup hook is initialized with an empty string parameter, which might cause issues when inviting users.
- const { mutate: inviteUserToGroup, isLoading: isInvitingMembers } =
- useInviteUserToGroup("");
+ const { mutate: inviteUserToGroup, isLoading: isInvitingMembers } =
+ useInviteUserToGroup(createOrgForm.getValues().grp_title);📝 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.
| useInviteUserToGroup(""); | |
| const { mutate: inviteUserToGroup, isLoading: isInvitingMembers } = | |
| useInviteUserToGroup(createOrgForm.getValues().grp_title); |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
netmanager-app/core/hooks/useRoles.ts (1)
14-24:⚠️ Potential issueFix incorrect type parameters and improve type safety.
The current implementation has several type-related issues:
- useQuery's type parameter is incorrectly set to AxiosError
- 'any' type in onSuccess defeats TypeScript's type safety
- Unnecessary type assertion with UseQueryOptions
Apply this diff to fix the type issues:
- const { data, isLoading, error } = useQuery<AxiosError<ErrorResponse>>({ + const { data, isLoading, error } = useQuery<{ roles: Role[] }, AxiosError<ErrorResponse>>({ queryKey: ["roles"], queryFn: () => roles.getRolesApi(), - onSuccess: (data: any) => { + onSuccess: (data: { roles: Role[] }) => { dispatch(setRoles(data.roles)); }, onError: (error: AxiosError<ErrorResponse>) => { dispatch(setError(error.message)); }, - } as UseQueryOptions<AxiosError<ErrorResponse>>); + });Note: You'll need to define the
Roleinterface based on your API response shape.
🧹 Nitpick comments (2)
netmanager-app/components/Organization/organization-profile.tsx (2)
17-21: Watch out for non-Latin or special characters informatTitle
While this regex-based replacement works for standard words, it may not handle accents or non-Latin scripts gracefully. If you anticipate organization names containing such characters, consider a more locale-aware approach.
131-131: Avoid applyingformatTitleon each keystroke in input fields
CallingformatTitledirectly in thevalueprop can cause immediate transformations with underscores/dashes removed. This may feel jarring to users who want to preserve certain characters or see their exact input. Consider formatting after submission or providing a preview so users clearly understand any modifications.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
netmanager-app/components/Organization/create-group.tsx(1 hunks)netmanager-app/components/Organization/organization-profile.tsx(1 hunks)netmanager-app/core/hooks/useRoles.ts(1 hunks)
🔇 Additional comments (6)
netmanager-app/components/Organization/organization-profile.tsx (3)
32-36: Reflectgrp_statusinformData, if editing status is intended
This component referencesgrp_statuswhen rendering active/inactive status but omits it from the form state. If you plan to let users change the status here, include it in the form data.
48-67: Nicely handled asynchronous updates and user notifications
TheuseMutationsetup cleanly handles success and error states, providing clear feedback. The design is straightforward and adheres to React Query best practices.
100-105: Safely handle an empty organization title in Avatar fallback
Ifgrp_titleis empty,.slice(0, 2).toUpperCase()will produce an empty string. Provide a more explicit fallback to avoid a blank avatar or potential edge cases.netmanager-app/components/Organization/create-group.tsx (3)
166-166: Pass a Proper Group ID touseInviteUserToGroup()
A static empty string parameter may cause issues when inviting users to different groups. Use a dynamic, valid group ID.
240-242: Invite Members in Parallel
Inviting members one by one can be slow for bulk operations. Consider usingPromise.allfor parallel invitations.
193-221: Store the Group ID Instead of Using Title
Reliance ongetValues().grp_titleas the group ID can break if the title changes. Use state or store the ID returned from the backend to maintain consistency.
| export const useGroupRoles = (groupId: string) => { | ||
| const dispatch = useAppDispatch(); | ||
|
|
||
| const { data, isLoading, error } = useQuery({ | ||
| queryKey: ["grouproles", groupId], | ||
| queryFn: () => roles.getOrgRolesApi(groupId || ""), | ||
| onSuccess: (data: any) => { | ||
| dispatch(setRoles(data)); | ||
| }, | ||
| onError: (error: Error) => { | ||
| dispatch(setError(error.message)); | ||
| }, | ||
| }); | ||
|
|
||
| return { | ||
| grproles: data?.group_roles || [], | ||
| isLoading, | ||
| error: error as Error | null, | ||
| }; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type safety and error handling in useGroupRoles.
The current implementation has several issues:
- Uses 'any' type in onSuccess
- Inconsistent error typing
- Empty string fallback for groupId could mask errors
- Query key could be more descriptive
Apply this diff to improve the implementation:
export const useGroupRoles = (groupId: string) => {
const dispatch = useAppDispatch();
- const { data, isLoading, error } = useQuery({
- queryKey: ["grouproles", groupId],
- queryFn: () => roles.getOrgRolesApi(groupId || ""),
- onSuccess: (data: any) => {
+ const { data, isLoading, error } = useQuery<{ group_roles: GroupRole[] }, AxiosError<ErrorResponse>>({
+ queryKey: ["roles", "group", groupId],
+ queryFn: () => {
+ if (!groupId) {
+ throw new Error("Group ID is required");
+ }
+ return roles.getOrgRolesApi(groupId);
+ },
+ onSuccess: (data: { group_roles: GroupRole[] }) => {
dispatch(setRoles(data));
},
- onError: (error: Error) => {
+ onError: (error: AxiosError<ErrorResponse>) => {
dispatch(setError(error.message));
},
});
return {
grproles: data?.group_roles || [],
isLoading,
- error: error as Error | null,
+ error,
};
};Note: You'll need to define the GroupRole interface based on your API response shape.
📝 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.
| export const useGroupRoles = (groupId: string) => { | |
| const dispatch = useAppDispatch(); | |
| const { data, isLoading, error } = useQuery({ | |
| queryKey: ["grouproles", groupId], | |
| queryFn: () => roles.getOrgRolesApi(groupId || ""), | |
| onSuccess: (data: any) => { | |
| dispatch(setRoles(data)); | |
| }, | |
| onError: (error: Error) => { | |
| dispatch(setError(error.message)); | |
| }, | |
| }); | |
| return { | |
| grproles: data?.group_roles || [], | |
| isLoading, | |
| error: error as Error | null, | |
| }; | |
| }; | |
| export const useGroupRoles = (groupId: string) => { | |
| const dispatch = useAppDispatch(); | |
| const { data, isLoading, error } = useQuery<{ group_roles: GroupRole[] }, AxiosError<ErrorResponse>>({ | |
| queryKey: ["roles", "group", groupId], | |
| queryFn: () => { | |
| if (!groupId) { | |
| throw new Error("Group ID is required"); | |
| } | |
| return roles.getOrgRolesApi(groupId); | |
| }, | |
| onSuccess: (data: { group_roles: GroupRole[] }) => { | |
| dispatch(setRoles(data)); | |
| }, | |
| onError: (error: AxiosError<ErrorResponse>) => { | |
| dispatch(setError(error.message)); | |
| }, | |
| }); | |
| return { | |
| grproles: data?.group_roles || [], | |
| isLoading, | |
| error, | |
| }; | |
| }; |
Summary of Changes (What does this PR do?)
Status of maturity (all need to be checked before merging):
How should this be manually tested?
What are the relevant tickets?
Screenshots (optional)
Summary by CodeRabbit
New Features
UI Enhancements