[Netmanager]: Resolving some API issues in the profile setting Page #2447
Conversation
…loading state handling
…ove redundant loading states
…e profile handling
📝 WalkthroughWalkthroughThis pull request updates multiple areas of the application. The changes add optional properties to user type definitions, refactor components by removing local state management and API calls in favor of custom hooks, and simplify error handling in API functions. Additionally, a deprecated callback prop is removed from a client form, and a naming typo in Redux slices is corrected. Overall, the modifications streamline data flows and improve code consistency and maintainability. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant C as Component (ApiTokens/Clients)
participant H as Custom Hook (useUserClients/useClients)
participant A as API
U->>C: Triggers data load
C->>H: Invoke hook for client data
H->>A: Fetch clients data
A-->>H: Return clients data
H-->>C: Provide clients, isLoading, error
C->>U: Render updated list
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: 0
🧹 Nitpick comments (8)
netmanager-app/core/hooks/useUserClients.ts (1)
17-17: Consider type safety for the dispatched data.The
dataparameter in onSuccess is typed asany. Consider adding proper type definitions to ensure type safety.- onSuccess: (data: any) => { + onSuccess: (data: { clients: Client[] }) => {netmanager-app/core/apis/settings.ts (1)
23-28: Consider explicit error handling for better debugging.While the simplified implementation is cleaner, consider adding error handling with specific error types for better debugging and error reporting.
getUserClientsApi: async (userID: string) => { - return await axiosInstance.get(`${USERS_MGT_URL}/clients`, { + try { + const response = await axiosInstance.get(`${USERS_MGT_URL}/clients`, { params: { user_id: userID }, - }) - .then((response) => response.data); + }); + return response.data; + } catch (error) { + if (axios.isAxiosError(error)) { + throw new Error(`Failed to fetch user clients: ${error.message}`); + } + throw error; + } },netmanager-app/components/Settings/CreateClientForm.tsx (1)
42-72: Consider adding IP address validation.The form allows any string input for IP addresses. Consider adding validation to ensure valid IPv4/IPv6 formats.
const handleSubmit = async () => { setIsLoading(true) try { if (!clientName) { throw new Error("Client name can't be empty") } + const ipRegex = /^(?:(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\.){3}(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)$/; + const validIps = ipAddresses.filter(ip => ip.trim() !== '').every(ip => ipRegex.test(ip)); + if (!validIps) { + throw new Error("Invalid IP address format"); + } const data = { name: clientName, user_id: userInfo?._id, ip_addresses: ipAddresses.filter((ip) => ip.trim() !== ''), }netmanager-app/components/Settings/ApiTokens.tsx (2)
28-29: Remove debug console.log statement.Debug logging should be removed before pushing to production.
const { clients, isLoading, error } = useUserClients(); -console.log(clients)
32-38: Refactor repeated client lookup logic.Multiple functions have similar client lookup logic. Consider extracting this into a helper function.
+const findClient = (clientId: string, clients: Client[] | undefined): Client | undefined => { + return Array.isArray(clients) && clients + ? clients.find((client: Client) => client._id === clientId) + : undefined; +} const hasAccessToken = (clientId: string): boolean => { - const client = - Array.isArray(clients) && clients - ? clients?.find((client: Client) => client._id === clientId) - : undefined + const client = findClient(clientId, clients); return client?.access_token?.token ? true : false }Also applies to: 40-46, 47-53, 55-61
🧰 Tools
🪛 Biome (1.9.4)
[error] 37-37: 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)
netmanager-app/components/Settings/MyProfile.tsx (1)
182-210: Consider form validation before submission.The form submission handler could benefit from input validation before making the API call.
const handleSubmit = async (e: React.FormEvent) => { e.preventDefault(); if (!profile || !currentUser) return; + // Validate required fields + if (!profile.firstName.trim() || !profile.lastName.trim()) { + toast({ + title: "Error", + description: "First name and last name are required", + }); + return; + } + + // Validate email format + const emailRegex = /^[^\s@]+@[^\s@]+\.[^\s@]+$/; + if (!emailRegex.test(profile.email)) { + toast({ + title: "Error", + description: "Invalid email format", + }); + return; + } setIsLoading(true);netmanager-app/components/Settings/Clients.tsx (2)
67-68: Consider handling the error state.While the error is being retrieved from
useClients, it's not being utilized in the UI to show error states to users.Add error handling UI:
const { clients, isLoading, error } = useClients(); const queryClient = useQueryClient(); + + if (error) { + return ( + <div className="flex justify-center items-center h-64 text-red-500"> + <p>Failed to load clients. Please try again later.</p> + </div> + ); + }
119-124: Type annotations look good, but consider extracting filter logic.The type annotations for the client parameter improve type safety. However, the filtering logic could be extracted into a separate utility function for better reusability.
Consider extracting the filter logic:
+ const filterClientsBySearch = (clients: Client[], searchQuery: string) => + clients.filter( + (client: Client) => + client.name.toLowerCase().includes(searchQuery.toLowerCase()) || + (client.user?.email?.toLowerCase().includes(searchQuery.toLowerCase()) ?? false) + ); + const filterClientsByStatus = (clients: Client[], isActive: boolean) => + clients.filter((client: Client) => client.isActive === isActive); // In component: - const filteredClients = clients.filter( - (client: Client) => - client.name.toLowerCase().includes(searchQuery.toLowerCase()) || - (client.user?.email?.toLowerCase().includes(searchQuery.toLowerCase()) ?? false) - ); + const filteredClients = filterClientsBySearch(clients, searchQuery); - const activatedClients = clients.filter((client: Client) => client.isActive).length; - const deactivatedClients = clients.filter((client: Client) => !client.isActive).length; + const activatedClients = filterClientsByStatus(clients, true).length; + const deactivatedClients = filterClientsByStatus(clients, false).length;Also applies to: 138-141
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
netmanager-app/app/types/users.ts(2 hunks)netmanager-app/components/Settings/ApiTokens.tsx(3 hunks)netmanager-app/components/Settings/Clients.tsx(8 hunks)netmanager-app/components/Settings/CreateClientForm.tsx(1 hunks)netmanager-app/components/Settings/MyProfile.tsx(1 hunks)netmanager-app/core/apis/settings.ts(1 hunks)netmanager-app/core/hooks/useUserClients.ts(1 hunks)netmanager-app/core/hooks/users.ts(1 hunks)netmanager-app/core/redux/slices/clientsSlice.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
netmanager-app/components/Settings/ApiTokens.tsx
[error] 37-37: 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] 45-45: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 52-52: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (11)
netmanager-app/core/hooks/useUserClients.ts (2)
13-13: LGTM! Clean and consistent query key format.The removal of the trailing comma improves code consistency.
25-25: LGTM! Improved null safety in clients data handling.The change to use optional chaining with a fallback to an empty array is a more robust approach.
netmanager-app/core/redux/slices/clientsSlice.ts (1)
18-18: LGTM! Fixed property name typo.The property name has been corrected from
accees_tokentoaccess_token, ensuring consistency with the interface definition.netmanager-app/app/types/users.ts (1)
64-64: LGTM! Enhanced user profile type definitions.The addition of
timezoneanddescriptionfields with proper null safety improves the type definitions for user profiles.Also applies to: 109-110
netmanager-app/components/Settings/CreateClientForm.tsx (1)
11-14: Interface looks good after callback removal.The simplified props interface with just
openandonCloseis clean and follows good practices.netmanager-app/core/hooks/users.ts (1)
55-56: 🌐 Web query:What is the standard format for IANA timezone strings and how to validate them in TypeScript?💡 Result:
The IANA (Internet Assigned Numbers Authority) timezone database uses standardized identifiers formatted as **"Area/Location"**, where: - **Area**: Represents continents/oceans (e.g., `Africa`, `America`, `Asia`) - **Location**: Specifies cities/regions (e.g., `London`, `Los_Angeles`, `Abidjan`) Examples: `Europe/Paris`, `America/Chicago`, `Pacific/Honolulu`[1][4]. --- ### Validation Methods in TypeScript #### 1. **Built-in API Check** Use JavaScript's `Intl.DateTimeFormat` to test validity: ```typescript function isValidIANATimezone(timeZone: string): boolean { try { new Intl.DateTimeFormat('en-US', { timeZone }).format(); return true; } catch { return false; } } ``` This method relies on browser/Node.js support for IANA timezones[2][8]. #### 2. **Regex Pattern Matching** Validate format (doesn't confirm existence in IANA DB): ```typescript const IANA_TIMEZONE_REGEX = /^[A-Za-z]{2,}\/(?:[A-Za-z_+-]{2,}|\/)/; const isValidFormat = (tz: string) => IANA_TIMEZONE_REGEX.test(tz); ``` #### 3. **Library-Assisted Validation** Use libraries like `date-fns-tz` for robust handling: ```typescript import { getTimezoneOffset } from 'date-fns-tz'; function isValidTimezone(tz: string): boolean { try { getTimezoneOffset(tz); return true; } catch { return false; } } ``` This checks both format *and* database validity[5]. --- ### Key Considerations - **Case Sensitivity**: Names are case-insensitive (e.g., `AMERICA/NEW_YORK` ≈ `America/New_York`)[1]. - **Aliases**: Some zones are linked (e.g., `Asia/Calcutta` → `Asia/Kolkata`)[4]. - **Dynamic Updates**: Timezone rules change periodically; use up-to-date libraries[10]. For applications requiring precise validation, combine regex checks with API/library methods to ensure both syntax and semantic correctness. Citations: - 1: https://en.wikipedia.org/wiki/List_of_tz_database_time_zones - 2: https://deniapps.com/blog/how-to-get-iana-timezone-name-from-local-datetime - 3: https://community.palantir.com/t/typescript-using-irange-timestamp-and-timezone-format/1621 - 4: https://en.wikipedia.org/wiki/IANA_time_zone_database - 5: https://www.npmjs.com/package/date-fns-tz - 6: https://github.com/angular/angular/issues/48279 - 7: https://ftp.iana.org/tz/tzdb-2022b/theory.html - 8: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/DateTimeFormat/DateTimeFormat - 9: https://github.com/date-fns/date-fns/issues/2642 - 10: https://www.iana.org/time-zonesnetmanager-app/components/Settings/ApiTokens.tsx (1)
72-72: Good use of query invalidation.Properly invalidating the clients query after token generation ensures UI stays in sync with the backend.
netmanager-app/components/Settings/MyProfile.tsx (1)
60-63: Good optimization by removing unnecessary API call.Using currentUser directly from Redux store instead of making an API call improves performance.
netmanager-app/components/Settings/Clients.tsx (3)
22-23: Nice refactoring to use React Query and custom hooks!The introduction of React Query and the custom
useClientshook is a great improvement for managing server state and caching.Also applies to: 39-39
70-98: Good use of React Query's invalidation!The
handleActivateDeactivatefunction correctly invalidates the clients query after successful activation/deactivation.🧰 Tools
🪛 Biome (1.9.4)
[error] 76-76: 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)
179-183: Loading state implementation looks good!The loading state is well-handled with a centered spinner and proper conditional rendering.
|
CC @Codebmk |
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
Refactor
Bug Fixes