[Netmanager]: Clients page with permissions restrictions #2405
Conversation
📝 WalkthroughWalkthroughThis pull request introduces a comprehensive client management system in the NetManager application. The changes include a new Changes
Possibly related PRs
Suggested Reviewers
Poem
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: 3
🧹 Nitpick comments (11)
netmanager-app/app/types/clients.ts (1)
3-15: Consider clarifying naming conventionsThe fields in
AccessToken(e.g.,_id,__v, etc.) seem to reflect MongoDB schema naming, which is fine if it matches your backend. But consider standardizing naming conventions across your TypeScript code to avoid confusion, especially with underscores vs. camelCase for fields likeclient_id.netmanager-app/core/apis/analytics.ts (2)
30-32: Ensure robust error handlingCurrent usage of
.then((response) => response.data)is valid, but consider adding more error handling or usingasync/awaitwith try-catch to handle network or server failures more gracefully.
34-38: Unify activation and deactivation logicWhile named for activation, this endpoint could also handle deactivation, which might cause confusion. Consider making a more general endpoint or function name (e.g.,
updateUserClientStatusApi) to handle both states, or splitting the functionality clearly.netmanager-app/app/pageAccess.tsx (2)
6-9: Add tests for the permission-based HOCA well-tested HOC ensures that permission checks and redirects behave as expected. Implementing unit tests or integration tests can catch potential user flow issues early.
17-28: Provide feedback instead of returning nullWhen the user lacks permission (or while checking permissions), returning
nullcan abruptly hide the entire page. Consider rendering a spinner, placeholder, or an informative message until the check completes. This improves usability by informing users about what's happening.Also applies to: 30-36
netmanager-app/app/(authenticated)/clients/dialogs.tsx (1)
19-36: Refactor repeated dialog logicBoth
ActivateClientDialogandDeactivateClientDialogshare similar structure. Consider creating a reusable, parameterized dialog component to reduce duplication and improve maintainability. For example:+function ClientActionDialog({ + open, + onOpenChange, + onConfirm, + clientName, + actionLabel, + title, + description +}: { + open: boolean; + onOpenChange: (open: boolean) => void; + onConfirm: () => void; + clientName?: string; + actionLabel: string; + title: string; + description: string; +}) { + return ( + <AlertDialog open={open} onOpenChange={onOpenChange}> + <AlertDialogContent> + <AlertDialogHeader> + <AlertDialogTitle>{title}</AlertDialogTitle> + <AlertDialogDescription> + {description} {clientName}? This action cannot be undone. + </AlertDialogDescription> + </AlertDialogHeader> + <AlertDialogFooter> + <AlertDialogCancel>Cancel</AlertDialogCancel> + <AlertDialogAction onClick={onConfirm}>{actionLabel}</AlertDialogAction> + </AlertDialogFooter> + </AlertDialogContent> + </AlertDialog> + ); +}Also applies to: 38-55
netmanager-app/app/(authenticated)/clients/columns.tsx (1)
42-47: Consider consolidating token expiry logic
You compute token expiration here very similarly to how it's done inpage.tsx. Extracting this logic into a shared utility could help you avoid duplication and keep your code DRY.netmanager-app/app/(authenticated)/clients/data-table.tsx (2)
52-57: Improve filter input accessibility
Consider adding anaria-labelor a more descriptive placeholder text for the filter input, e.g. “Filter clients by name,” to enhance accessibility for screen readers.
74-88: Add loading or empty states for better UX
Currently, you show "No results." when there is no data. You could expand on this to handle scenarios where data is still loading or to provide additional info for the user.netmanager-app/app/(authenticated)/clients/page.tsx (2)
13-26: Avoid duplicating the expiry logic
TheformatDatefunction duplicates the token expiry logic seen incolumns.tsx. Consider moving it into a shared helper for consistency and easier maintenance.🧰 Tools
🪛 Biome (1.9.4)
[error] 16-16: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
90-95: Leverage optional chaining
This filter could be made more concise by employing optional chaining onaccess_token.- .filter((client) => client.access_token && client.access_token.expires) + .filter((client) => client.access_token?.expires)🧰 Tools
🪛 Biome (1.9.4)
[error] 91-91: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
netmanager-app/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
netmanager-app/app/(authenticated)/clients/columns.tsx(1 hunks)netmanager-app/app/(authenticated)/clients/data-table.tsx(1 hunks)netmanager-app/app/(authenticated)/clients/dialogs.tsx(1 hunks)netmanager-app/app/(authenticated)/clients/page.tsx(1 hunks)netmanager-app/app/pageAccess.tsx(1 hunks)netmanager-app/app/types/clients.ts(1 hunks)netmanager-app/components/ui/alert-dialog.tsx(1 hunks)netmanager-app/components/ui/button.tsx(2 hunks)netmanager-app/core/apis/analytics.ts(2 hunks)netmanager-app/package.json(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- netmanager-app/components/ui/button.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
netmanager-app/app/(authenticated)/clients/page.tsx
[error] 91-91: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 16-16: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
netmanager-app/app/(authenticated)/clients/columns.tsx
[error] 48-48: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
🔇 Additional comments (8)
netmanager-app/app/types/clients.ts (1)
1-2: Imports look goodEverything here appears straightforward, and referencing the
UserDetailsinterface from./usersis appropriate for maintaining type consistency.netmanager-app/app/(authenticated)/clients/dialogs.tsx (1)
1-17: Dialog props are well-structuredUsing a dedicated interface for dialog props is a clean approach that ensures both dialogs are consistent and type-safe.
netmanager-app/app/(authenticated)/clients/columns.tsx (1)
16-19: Interface definition is well-structured
TheColumnPropsinterface succinctly defines the callback functions for activating and deactivating clients, promoting clear usage and strong typing.netmanager-app/app/(authenticated)/clients/data-table.tsx (1)
29-46: Well-structured table configuration
Your usage ofuseReactTablefor sorting, filtering, and pagination is neatly integrated, making the table easy to maintain and extend. Nicely done.netmanager-app/components/ui/alert-dialog.tsx (1)
1-141: Alert dialog implementation is solid
UsingReact.forwardRefin tandem with Radix UI primitives is cleanly executed. The code is readable and appears to cover key accessibility features.netmanager-app/app/(authenticated)/clients/page.tsx (1)
155-156: Confirm correctness of the permission key
Double-check that'CREATE_UPDATE_AND_DELETE_NETWORK_DEVICES'is the intended permission, as an incorrect key could lead to unintended access issues.Would you like me to generate a shell script to search for references to this permission across the repository to confirm its usage?
netmanager-app/package.json (2)
13-13: LGTM! Consistent versioning with other Radix UI components.The new alert dialog package follows the same major version as other Radix UI components, maintaining consistency across the UI library.
28-28: Consider code splitting for the table component.The TanStack Table package is a substantial library. Since it's used only on the clients page, consider implementing dynamic imports to reduce the initial bundle size.
Example implementation:
// In your clients page component const DataTable = dynamic(() => import('@/components/data-table'), { loading: () => <TableSkeleton />, ssr: false })Also, let's verify the security status of these new dependencies:
✅ Verification successful
Code splitting suggestion is valid and well-suited for this implementation
The analysis confirms that @tanstack/react-table is exclusively used in the clients page through a well-isolated table component. The suggested dynamic import approach is appropriate and can be implemented as proposed, which will help optimize the initial bundle size.
The current clean separation of the table component into its own files makes this refactoring straightforward to implement.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for known vulnerabilities in new packages echo "Checking @radix-ui/react-alert-dialog..." npm audit --json @radix-ui/react-alert-dialog echo "Checking @tanstack/react-table..." npm audit --json @tanstack/react-table # Check if versions are latest stable echo "Checking latest versions..." npm view @radix-ui/react-alert-dialog version npm view @tanstack/react-table versionLength of output: 1601
Script:
#!/bin/bash # Find all imports of @tanstack/react-table and potential table components echo "Searching for @tanstack/react-table imports..." rg "@tanstack/react-table" -l echo -e "\nSearching for potential table component imports..." rg "import.*Table" -l echo -e "\nChecking for existing dynamic imports..." rg "dynamic\(" -l echo -e "\nChecking package size..." npm view @tanstack/react-table dist.sizeLength of output: 4411
Script:
#!/bin/bash # Check the actual implementation of the table component echo "Checking clients page implementation..." cat netmanager-app/app/\(authenticated\)/clients/page.tsx echo -e "\nChecking data-table implementation..." cat netmanager-app/app/\(authenticated\)/clients/data-table.tsxLength of output: 8992
| export interface Client { | ||
| _id: string | ||
| isActive: boolean | ||
| ip_addresses: string[] | ||
| name: string | ||
| client_secret: string | ||
| user: UserDetails | ||
| access_token: AccessToken | ||
| } |
There was a problem hiding this comment.
Hash sensitive credentials for security
Storing client_secret in plain text could pose security risks if the data is ever exposed. It is a best practice to store such credentials as securely hashed values on the server side. Consider clarifying or documenting how client_secret is stored and used.
| const accessToken = row.original.access_token | ||
| if (!accessToken || !accessToken.expires) return "N/A" | ||
| const expires = new Date(accessToken.expires) | ||
| if (isNaN(expires.getTime())) return "Invalid Date" |
There was a problem hiding this comment.
Use Number.isNaN for safer numeric checks
Relying on isNaN() can lead to type coercion issues. It's best to use Number.isNaN() for a more explicit numeric check.
- if (isNaN(expires.getTime())) return "Invalid Date"
+ if (Number.isNaN(expires.getTime())) return "Invalid Date"📝 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 (isNaN(expires.getTime())) return "Invalid Date" | |
| if (Number.isNaN(expires.getTime())) return "Invalid Date" |
🧰 Tools
🪛 Biome (1.9.4)
[error] 48-48: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
netmanager-app/app/(authenticated)/clients/page.tsx (1)
15-15:⚠️ Potential issueUse Number.isNaN to avoid unintended type coercion.
This issue has been flagged before. Replacing global
isNaNwithNumber.isNaNensures greater reliability and avoids unwanted type conversion.Apply this diff to fix:
- if (isNaN(date.getTime())) return "Invalid Date" + if (Number.isNaN(date.getTime())) return "Invalid Date"🧰 Tools
🪛 Biome (1.9.4)
[error] 15-15: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
🧹 Nitpick comments (1)
netmanager-app/app/(authenticated)/clients/page.tsx (1)
90-99: Consider merging the double filter into a single pass.Using optional chaining can simplify the logic and reduce overhead from multiple filter calls. This is purely optional but can streamline the code.
Possible approach:
- const nearestExpiringToken = clients - .filter((client) => client.access_token && client.access_token.expires) - .filter((client) => { - const expiryDate = new Date(client.access_token.expires) - return expiryDate > new Date() - }) - .sort((a, b) => { - const dateA = new Date(a.access_token.expires).getTime() - const dateB = new Date(b.access_token.expires).getTime() - return dateA - dateB - })[0] + const nearestExpiringToken = clients + .filter((client) => { + const expiryDate = new Date(client.access_token?.expires ?? '') + return !Number.isNaN(expiryDate.getTime()) && expiryDate > new Date() + }) + .sort((a, b) => { + const dateA = new Date(a.access_token?.expires ?? '').getTime() + const dateB = new Date(b.access_token?.expires ?? '').getTime() + return dateA - dateB + })[0]🧰 Tools
🪛 Biome (1.9.4)
[error] 90-90: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
netmanager-app/app/(authenticated)/clients/page.tsx(1 hunks)netmanager-app/components/sidebar.tsx(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
netmanager-app/app/(authenticated)/clients/page.tsx
[error] 90-90: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 15-15: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
| <PermissionGuard permission="CREATE_UPDATE_AND_DELETE_NETWORK_ROLES"> | ||
| <li> | ||
| <Link | ||
| href="/clients" | ||
| className={`flex items-center gap-2 text-sm text-foreground hover:bg-accent hover:text-accent-foreground p-2 rounded-md ${ | ||
| isActive("/access-control") | ||
| ? "bg-accent text-accent-foreground" | ||
| : "" | ||
| }`} | ||
| > | ||
| <Hospital size={18} /> | ||
| <span>Clients</span> | ||
| </Link> | ||
| </li> | ||
| </PermissionGuard> |
There was a problem hiding this comment.
Ensure the correct active state for the "Clients" link.
Currently, isActive("/access-control") is used for the "Clients" link, which may cause the active style to remain unset when navigating to /clients. Consider updating it to isActive("/clients") or refining the active check logic.
Proposed fix:
- className={`flex items-center gap-2 text-sm text-foreground hover:bg-accent hover:text-accent-foreground p-2 rounded-md ${
- isActive("/access-control")
- ? "bg-accent text-accent-foreground"
- : ""
- }`}
+ className={`flex items-center gap-2 text-sm text-foreground hover:bg-accent hover:text-accent-foreground p-2 rounded-md ${
+ isActive("/clients")
+ ? "bg-accent text-accent-foreground"
+ : ""
+ }`}📝 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.
| <PermissionGuard permission="CREATE_UPDATE_AND_DELETE_NETWORK_ROLES"> | |
| <li> | |
| <Link | |
| href="/clients" | |
| className={`flex items-center gap-2 text-sm text-foreground hover:bg-accent hover:text-accent-foreground p-2 rounded-md ${ | |
| isActive("/access-control") | |
| ? "bg-accent text-accent-foreground" | |
| : "" | |
| }`} | |
| > | |
| <Hospital size={18} /> | |
| <span>Clients</span> | |
| </Link> | |
| </li> | |
| </PermissionGuard> | |
| <PermissionGuard permission="CREATE_UPDATE_AND_DELETE_NETWORK_ROLES"> | |
| <li> | |
| <Link | |
| href="/clients" | |
| className={`flex items-center gap-2 text-sm text-foreground hover:bg-accent hover:text-accent-foreground p-2 rounded-md ${ | |
| isActive("/clients") | |
| ? "bg-accent text-accent-foreground" | |
| : "" | |
| }`} | |
| > | |
| <Hospital size={18} /> | |
| <span>Clients</span> | |
| </Link> | |
| </li> | |
| </PermissionGuard> |
danielmarv
changed the title
|
| }, | ||
| }, | ||
| ] | ||
|
|
There was a problem hiding this comment.
Is this a component or a page? @danielmarv
Anything added under app folder is considered a page. Now we can't have a page called columns😂😂 actually everything you put under clients is a component except for page. Kindly put these in the right folder!
And if it is a page, clients is not accessible by everyone. Refer to sites registry page to see how to add access control
netmanager-app/package.json
Outdated
| @@ -24,6 +25,7 @@ | |||
| "@reduxjs/toolkit": "^2.5.0", | |||
| "@tanstack/react-query": "^5.62.11", | |||
| "@tanstack/react-query-devtools": "^5.62.11", | |||
| "@tanstack/react-table": "^8.20.6", | |||
| "@types/leaflet": "^1.9.15", | |||
There was a problem hiding this comment.
No need for a new table library, kindly use shadcn for all your UI needs! @danielmarv
|
Clients should be a tab under profile settings and not a under the sidebar as we had discussed. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
netmanager-app/app/(authenticated)/clients/page.tsx (1)
24-37:⚠️ Potential issueFix the date validation and enhance the formatting utility.
The date formatting utility has a few issues:
- Uses unsafe
isNaN(as flagged by static analysis)- Lacks timezone handling
- Could benefit from internationalization
const formatDate = (dateString: string | undefined): string => { if (!dateString) return "N/A" const date = new Date(dateString) - if (isNaN(date.getTime())) return "Invalid Date" + if (Number.isNaN(date.getTime())) return "Invalid Date" const now = new Date() + // Normalize both dates to UTC midnight to avoid timezone issues + const utcDate = new Date(Date.UTC(date.getUTCFullYear(), date.getUTCMonth(), date.getUTCDate())) + const utcNow = new Date(Date.UTC(now.getUTCFullYear(), now.getUTCMonth(), now.getUTCDate())) - const diffTime = date.getTime() - now.getTime() + const diffTime = utcDate.getTime() - utcNow.getTime() const diffDays = Math.ceil(diffTime / (1000 * 60 * 60 * 24))🧰 Tools
🪛 Biome (1.9.4)
[error] 27-27: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
🧹 Nitpick comments (5)
netmanager-app/components/clients/dialogs.tsx (3)
12-17: Props interface looks good, but could be more descriptive.The interface is well-structured but could benefit from JSDoc comments to improve documentation.
+/** + * Props for client activation/deactivation dialogs + * @property {boolean} open - Controls dialog visibility + * @property {(open: boolean) => void} onOpenChange - Handles dialog open state changes + * @property {() => void} onConfirm - Callback fired when action is confirmed + * @property {string} [clientName] - Optional name of the client for display + */ interface DialogProps { open: boolean onOpenChange: (open: boolean) => void onConfirm: () => void clientName?: string }
19-36: Consider adding loading state and error handling.The dialog could benefit from:
- A loading state during activation
- Error handling for the confirmation action
-export function ActivateClientDialog({ open, onOpenChange, onConfirm, clientName }: DialogProps) { +export function ActivateClientDialog({ + open, + onOpenChange, + onConfirm, + clientName, + isLoading = false +}: DialogProps & { isLoading?: boolean }) { return ( <AlertDialog open={open} onOpenChange={onOpenChange}> <AlertDialogContent> <AlertDialogHeader> <AlertDialogTitle>Activate Client</AlertDialogTitle> <AlertDialogDescription> Are you sure you want to activate the client {clientName}? This action cannot be undone. </AlertDialogDescription> </AlertDialogHeader> <AlertDialogFooter> <AlertDialogCancel>Cancel</AlertDialogCancel> - <AlertDialogAction onClick={onConfirm}>Activate</AlertDialogAction> + <AlertDialogAction + onClick={onConfirm} + disabled={isLoading} + > + {isLoading ? "Activating..." : "Activate"} + </AlertDialogAction> </AlertDialogFooter> </AlertDialogContent> </AlertDialog> ) }
38-55: Consider reducing code duplication with a shared base component.Both dialog components share almost identical structure. Consider creating a base component to reduce duplication.
+interface ActionDialogProps extends DialogProps { + title: string; + actionText: string; + isLoading?: boolean; +} + +function ActionDialog({ + open, + onOpenChange, + onConfirm, + clientName, + title, + actionText, + isLoading = false +}: ActionDialogProps) { + return ( + <AlertDialog open={open} onOpenChange={onOpenChange}> + <AlertDialogContent> + <AlertDialogHeader> + <AlertDialogTitle>{title}</AlertDialogTitle> + <AlertDialogDescription> + Are you sure you want to {actionText.toLowerCase()} the client {clientName}? + This action cannot be undone. + </AlertDialogDescription> + </AlertDialogHeader> + <AlertDialogFooter> + <AlertDialogCancel>Cancel</AlertDialogCancel> + <AlertDialogAction + onClick={onConfirm} + disabled={isLoading} + > + {isLoading ? `${actionText}ing...` : actionText} + </AlertDialogAction> + </AlertDialogFooter> + </AlertDialogContent> + </AlertDialog> + ) +} + +export function ActivateClientDialog(props: DialogProps & { isLoading?: boolean }) { + return <ActionDialog {...props} title="Activate Client" actionText="Activate" /> +} + +export function DeactivateClientDialog(props: DialogProps & { isLoading?: boolean }) { + return <ActionDialog {...props} title="Deactivate Client" actionText="Deactivate" /> +}netmanager-app/app/(authenticated)/clients/page.tsx (2)
139-150: Optimize token expiry calculation and add type safety.The nearest expiring token calculation could be optimized and made more type-safe.
const nearestExpiringToken = clients - .filter((client) => client.access_token && client.access_token.expires) - .filter((client) => { + .filter((client): client is Client & { access_token: { expires: string } } => { + if (!client.access_token?.expires) return false const expiryDate = new Date(client.access_token.expires) - return expiryDate > new Date() + return !Number.isNaN(expiryDate.getTime()) && expiryDate > new Date() }) .sort((a, b) => { const dateA = new Date(a.access_token.expires).getTime() const dateB = new Date(b.access_token.expires).getTime() return dateA - dateB - })[0] + })[0] ?? null🧰 Tools
🪛 Biome (1.9.4)
[error] 140-140: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
240-248: Enhance button accessibility and loading states.The action buttons could benefit from improved accessibility and loading states.
<Button variant="ghost" + aria-label={`${client.isActive ? "Deactivate" : "Activate"} ${client.name}`} onClick={() => (client.isActive ? handleDeactivateClick(client) : handleActivateClick(client))} + disabled={loading} > {client.isActive ? "Deactivate" : "Activate"} </Button> <Button variant="ghost" + aria-label={`Copy ID for ${client.name}`} onClick={() => handleCopyClientId(client._id)} + disabled={loading} > Copy ID </Button>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
netmanager-app/app/(authenticated)/clients/page.tsx(1 hunks)netmanager-app/components/clients/dialogs.tsx(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
netmanager-app/app/(authenticated)/clients/page.tsx
[error] 140-140: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 27-27: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
| const fetchClients = async () => { | ||
| setLoading(true) | ||
| try { | ||
| const response = await getClientsApi() | ||
| console.log(response.clients) | ||
| setClients(response.clients) | ||
| } catch (error) { | ||
| toast({ | ||
| title: "Error", | ||
| description: "Failed to fetch clients", | ||
| variant: "destructive", | ||
| }) | ||
| } finally { | ||
| setLoading(false) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove console.log and enhance error handling.
The fetchClients function has a console.log statement and could benefit from more specific error handling.
const fetchClients = async () => {
setLoading(true)
try {
const response = await getClientsApi()
- console.log(response.clients)
setClients(response.clients)
} catch (error) {
+ const errorMessage = error instanceof Error ? error.message : "Failed to fetch clients"
toast({
title: "Error",
- description: "Failed to fetch clients",
+ description: errorMessage,
variant: "destructive",
})
+ console.error("Error fetching clients:", error)
} finally {
setLoading(false)
}
}📝 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 fetchClients = async () => { | |
| setLoading(true) | |
| try { | |
| const response = await getClientsApi() | |
| console.log(response.clients) | |
| setClients(response.clients) | |
| } catch (error) { | |
| toast({ | |
| title: "Error", | |
| description: "Failed to fetch clients", | |
| variant: "destructive", | |
| }) | |
| } finally { | |
| setLoading(false) | |
| } | |
| } | |
| const fetchClients = async () => { | |
| setLoading(true) | |
| try { | |
| const response = await getClientsApi() | |
| setClients(response.clients) | |
| } catch (error) { | |
| const errorMessage = error instanceof Error ? error.message : "Failed to fetch clients" | |
| toast({ | |
| title: "Error", | |
| description: errorMessage, | |
| variant: "destructive", | |
| }) | |
| console.error("Error fetching clients:", error) | |
| } finally { | |
| setLoading(false) | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
netmanager-app/app/(authenticated)/clients/page.tsx (1)
140-287: Enhance accessibility for better user experienceConsider the following accessibility improvements:
- Add aria-label to the search input
- Add aria-sort to sortable table headers
- Add aria-live region for loading state
<Input placeholder="Search clients..." className="pl-8" value={searchQuery} + aria-label="Search clients by name or email" onChange={(e) => setSearchQuery(e.target.value)} /> -<div className="flex justify-center items-center h-64"> +<div className="flex justify-center items-center h-64" aria-live="polite"> <Loader2 className="h-8 w-8 animate-spin" /> </div> -<TableHead className="w-[250px]">Client Name</TableHead> +<TableHead className="w-[250px]" aria-sort={sortField === "name" ? sortOrder : "none"}> + Client Name +</TableHead>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
netmanager-app/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
netmanager-app/app/(authenticated)/clients/page.tsx(1 hunks)netmanager-app/package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- netmanager-app/package.json
🧰 Additional context used
🪛 Biome (1.9.4)
netmanager-app/app/(authenticated)/clients/page.tsx
[error] 27-27: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
🔇 Additional comments (4)
netmanager-app/app/(authenticated)/clients/page.tsx (4)
40-50: LGTM! Well-organized state managementThe state variables are properly typed and initialized with appropriate default values.
52-66: Remove console.log and enhance error handling.The
fetchClientsfunction could benefit from more specific error handling.
120-137: LGTM! Efficient data processingThe filtering and pagination logic is well-implemented with case-insensitive search and proper calculations.
227-237: 🛠️ Refactor suggestionAdd missing action buttons as per PR comments
The implementation is missing activate/deactivate buttons in the style mentioned in the PR comments. Consider adding these buttons to match the sites registry table style.
<TableCell className="text-right"> + <div className="flex justify-end gap-2"> <Button - variant="ghost" + variant="outline" + size="sm" onClick={() => (client.isActive ? handleDeactivateClick(client) : handleActivateClick(client))} + className={client.isActive ? "text-red-600" : "text-green-600"} > {client.isActive ? "Deactivate" : "Activate"} </Button> <Button variant="ghost" onClick={() => handleCopyClientId(client._id)}> Copy ID </Button> + </div> </TableCell>
| const handleSort = (field: keyof Client) => { | ||
| if (sortField === field) { | ||
| setSortOrder(sortOrder === "asc" ? "desc" : "asc") | ||
| } else { | ||
| setSortField(field) | ||
| setSortOrder("asc") | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add type safety to sorting function
The current sorting implementation might fail for nested properties like user.email. Consider adding type safety and proper handling for nested properties.
- const sortedClients = [...filteredClients].sort((a, b) => {
- if (a[sortField] < b[sortField]) return sortOrder === "asc" ? -1 : 1
- if (a[sortField] > b[sortField]) return sortOrder === "asc" ? 1 : -1
- return 0
- })
+ const sortedClients = [...filteredClients].sort((a, b) => {
+ const getValue = (obj: Client, field: keyof Client) => {
+ if (field === "user.email") return obj.user.email
+ return obj[field]
+ }
+ const aValue = getValue(a, sortField)
+ const bValue = getValue(b, sortField)
+ return sortOrder === "asc"
+ ? String(aValue).localeCompare(String(bValue))
+ : String(bValue).localeCompare(String(aValue))
+ })Also applies to: 126-130
| const formatDate = (dateString: string | undefined): string => { | ||
| if (!dateString) return "N/A" | ||
| const date = new Date(dateString) | ||
| if (isNaN(date.getTime())) return "Invalid Date" | ||
|
|
||
| const now = new Date() | ||
| const diffTime = date.getTime() - now.getTime() | ||
| const diffDays = Math.ceil(diffTime / (1000 * 60 * 60 * 24)) | ||
|
|
||
| if (diffDays < 0) return "Expired" | ||
| if (diffDays === 0) return "Expires today" | ||
| if (diffDays === 1) return "Expires tomorrow" | ||
| return `Expires in ${diffDays} days` | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace unsafe isNaN with Number.isNaN
The current implementation uses the unsafe global isNaN which performs type coercion. Use Number.isNaN instead for more predictable type checking.
const formatDate = (dateString: string | undefined): string => {
if (!dateString) return "N/A"
const date = new Date(dateString)
- if (isNaN(date.getTime())) return "Invalid Date"
+ if (Number.isNaN(date.getTime())) return "Invalid Date"
const now = new Date()
const diffTime = date.getTime() - now.getTime()
const diffDays = Math.ceil(diffTime / (1000 * 60 * 60 * 24))📝 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 formatDate = (dateString: string | undefined): string => { | |
| if (!dateString) return "N/A" | |
| const date = new Date(dateString) | |
| if (isNaN(date.getTime())) return "Invalid Date" | |
| const now = new Date() | |
| const diffTime = date.getTime() - now.getTime() | |
| const diffDays = Math.ceil(diffTime / (1000 * 60 * 60 * 24)) | |
| if (diffDays < 0) return "Expired" | |
| if (diffDays === 0) return "Expires today" | |
| if (diffDays === 1) return "Expires tomorrow" | |
| return `Expires in ${diffDays} days` | |
| } | |
| const formatDate = (dateString: string | undefined): string => { | |
| if (!dateString) return "N/A" | |
| const date = new Date(dateString) | |
| if (Number.isNaN(date.getTime())) return "Invalid Date" | |
| const now = new Date() | |
| const diffTime = date.getTime() - now.getTime() | |
| const diffDays = Math.ceil(diffTime / (1000 * 60 * 60 * 24)) | |
| if (diffDays < 0) return "Expired" | |
| if (diffDays === 0) return "Expires today" | |
| if (diffDays === 1) return "Expires tomorrow" | |
| return `Expires in ${diffDays} days` | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 27-27: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
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 Improvements
Dependencies
Type Definitions