[Netmanager]: Profile Settings Page #2401
Conversation
📝 WalkthroughWalkthroughThis pull request introduces several new UI components, hooks, API functions, and Redux slices. New user profile and settings components are added along with supporting utilities to manage clients, toast notifications, country and timezone data, and Cloudinary image uploads. Additionally, dependency updates and minor sidebar adjustments have been implemented. The changes establish new client management flows and refine existing structures. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant CM as ClientManagement Component
participant API as API (getClientsApi)
participant Hook as useClients Hook
participant Redux as Redux Store
U->>CM: Open client management view
CM->>API: Request clients data
API-->>CM: Return list of clients
CM->>Hook: Pass clients data for state update
Hook->>Redux: Dispatch setClients action
Redux-->>Hook: Update state with clients
Hook-->>CM: Provide updated clients data
CM->>U: Render clients table
sequenceDiagram
participant C as UI Component
participant Toast as Toast System
participant L as Toast Listener
C->>Toast: Call toast({ title, description })
Toast->>Toast: Generate unique ID & update toast state
Toast->>L: Notify listener (ADD_TOAST)
C->>Toast: Optionally dismiss toast when required
Toast->>L: Notify listener (DISMISS_TOAST)
Possibly related PRs
Suggested reviewers
Poem
Tip 🌐 Web search-backed reviews and chat
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: 11
🧹 Nitpick comments (6)
netmanager-app/app/(authenticated)/profile/page.tsx (1)
3-10: Clean implementation with room for accessibility enhancement.The component structure is well-organized. Consider adding an
aria-labelto the container div to improve screen reader context.- <div className="container mx-auto px-4 py-8"> + <div className="container mx-auto px-4 py-8" aria-label="Profile Settings">netmanager-app/components/ui/textarea.tsx (1)
5-19: Enhance accessibility with aria-label support.The component implementation is solid, but consider adding explicit support for aria-label to improve accessibility.
const Textarea = React.forwardRef< HTMLTextAreaElement, React.ComponentProps<"textarea"> ->(({ className, ...props }, ref) => { +>(({ className, "aria-label": ariaLabel, ...props }, ref) => { return ( <textarea className={cn( "flex min-h-[80px] w-full rounded-md border border-input bg-background px-3 py-2 text-base ring-offset-background placeholder:text-muted-foreground focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-ring focus-visible:ring-offset-2 disabled:cursor-not-allowed disabled:opacity-50 md:text-sm", className )} + aria-label={ariaLabel} ref={ref} {...props} />netmanager-app/app/(authenticated)/profile/components/ApiTokens.tsx (2)
9-17: Consider adding JSDoc documentation and validation constraints to the Token interface.Adding documentation and constraints would improve type safety and code maintainability.
+/** + * Represents an API token with its associated metadata + */ interface Token { id: string - name: string + name: string // @min 3 characters ipAddress: string status: "Active" | "Inactive" created: string // ISO date string token: string - expires: string + expires: string // ISO date string }
66-82: Enhance form validation and error handling.Add proper validation and error feedback for better user experience.
+ const [error, setError] = useState<string>("") + <form onSubmit={handleCreateToken} className="space-y-4"> <div className="flex space-x-2"> <div className="flex-grow"> <Label htmlFor="new-token-name" className="sr-only"> New Token Name </Label> <Input id="new-token-name" placeholder="Enter new token name" value={newTokenName} - onChange={(e) => setNewTokenName(e.target.value)} + onChange={(e) => { + setNewTokenName(e.target.value) + setError("") + }} + minLength={3} + maxLength={50} + pattern="[a-zA-Z0-9\s-_]+" required /> + {error && <p className="text-red-500 text-sm mt-1">{error}</p>} </div> <Button type="submit">Create New Token</Button> </div> </form>netmanager-app/utils/timezones.ts (1)
1-8: Add TypeScript types and error handlingThe implementation looks clean, but could benefit from some TypeScript safety features:
+interface TimeZone { + utc: string[]; + text: string; +} + +interface FormattedTimeZone { + value: string; + label: string; +} + -import timeZones from "timezones.json" +import timeZones from "timezones.json" assert { type: "json" } -export const getTimezones = () => { +export const getTimezones = (): FormattedTimeZone[] => { return timeZones.map((tz) => ({ - value: tz.utc[0], + value: tz.utc[0] ?? '', // Fallback if array is empty label: `${tz.text} (${tz.utc[0]})`, })) }netmanager-app/utils/countries.ts (1)
1-12: Add TypeScript safety and optimize performanceThe implementation is good, but could be enhanced with types and memoization:
import countries from "i18n-iso-countries" import enLocale from "i18n-iso-countries/langs/en.json" +interface CountryOption { + value: string; + label: string; +} + +let cachedCountries: CountryOption[] | null = null; + countries.registerLocale(enLocale) -export const getCountries = () => { +export const getCountries = (): CountryOption[] => { + if (cachedCountries) return cachedCountries; + const countryObj = countries.getNames("en", { select: "official" }) - return Object.entries(countryObj).map(([code, name]) => ({ + cachedCountries = Object.entries(countryObj).map(([code, name]) => ({ value: code, label: name, })) + return cachedCountries; }Since the country list is static, we can memoize it to avoid unnecessary transformations on subsequent calls.
📜 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 (9)
netmanager-app/app/(authenticated)/profile/components/ApiTokens.tsx(1 hunks)netmanager-app/app/(authenticated)/profile/components/MyProfile.tsx(1 hunks)netmanager-app/app/(authenticated)/profile/components/PasswordEdit.tsx(1 hunks)netmanager-app/app/(authenticated)/profile/components/ProfileTabs.tsx(1 hunks)netmanager-app/app/(authenticated)/profile/page.tsx(1 hunks)netmanager-app/components/ui/textarea.tsx(1 hunks)netmanager-app/package.json(2 hunks)netmanager-app/utils/countries.ts(1 hunks)netmanager-app/utils/timezones.ts(1 hunks)
🧰 Additional context used
🪛 Gitleaks (8.21.2)
netmanager-app/app/(authenticated)/profile/components/ApiTokens.tsx
27-27: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
36-36: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
| export default function ProfileTabs() { | ||
| const [activeTab, setActiveTab] = useState("profile") | ||
|
|
||
| return ( | ||
| <Tabs value={activeTab} onValueChange={setActiveTab} className="w-full"> | ||
| <TabsList className="grid w-full grid-cols-3"> | ||
| <TabsTrigger value="profile">My Profile</TabsTrigger> | ||
| <TabsTrigger value="password">Password Edit</TabsTrigger> | ||
| <TabsTrigger value="api">API Access Tokens</TabsTrigger> | ||
| </TabsList> | ||
| <TabsContent value="profile"> | ||
| <MyProfile /> | ||
| </TabsContent> | ||
| <TabsContent value="password"> | ||
| <PasswordEdit /> | ||
| </TabsContent> | ||
| <TabsContent value="api"> | ||
| <ApiTokens /> | ||
| </TabsContent> | ||
| </Tabs> | ||
| ) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error boundaries and enhance tab accessibility.
While the tab implementation is clean, consider these improvements:
- Wrap tab content components with error boundaries
- Add aria-labels to improve tab navigation
+import { ErrorBoundary } from "react-error-boundary"
+
export default function ProfileTabs() {
const [activeTab, setActiveTab] = useState("profile")
return (
- <Tabs value={activeTab} onValueChange={setActiveTab} className="w-full">
+ <Tabs
+ value={activeTab}
+ onValueChange={setActiveTab}
+ className="w-full"
+ aria-label="Profile Settings Tabs"
+ >
<TabsList className="grid w-full grid-cols-3">
<TabsTrigger value="profile">My Profile</TabsTrigger>
<TabsTrigger value="password">Password Edit</TabsTrigger>
<TabsTrigger value="api">API Access Tokens</TabsTrigger>
</TabsList>
<TabsContent value="profile">
+ <ErrorBoundary fallback={<div>Error loading profile content</div>}>
<MyProfile />
+ </ErrorBoundary>
</TabsContent>
// Apply similar error boundaries to other tab contents📝 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 default function ProfileTabs() { | |
| const [activeTab, setActiveTab] = useState("profile") | |
| return ( | |
| <Tabs value={activeTab} onValueChange={setActiveTab} className="w-full"> | |
| <TabsList className="grid w-full grid-cols-3"> | |
| <TabsTrigger value="profile">My Profile</TabsTrigger> | |
| <TabsTrigger value="password">Password Edit</TabsTrigger> | |
| <TabsTrigger value="api">API Access Tokens</TabsTrigger> | |
| </TabsList> | |
| <TabsContent value="profile"> | |
| <MyProfile /> | |
| </TabsContent> | |
| <TabsContent value="password"> | |
| <PasswordEdit /> | |
| </TabsContent> | |
| <TabsContent value="api"> | |
| <ApiTokens /> | |
| </TabsContent> | |
| </Tabs> | |
| ) | |
| } | |
| import { ErrorBoundary } from "react-error-boundary" | |
| export default function ProfileTabs() { | |
| const [activeTab, setActiveTab] = useState("profile") | |
| return ( | |
| <Tabs | |
| value={activeTab} | |
| onValueChange={setActiveTab} | |
| className="w-full" | |
| aria-label="Profile Settings Tabs" | |
| > | |
| <TabsList className="grid w-full grid-cols-3"> | |
| <TabsTrigger value="profile">My Profile</TabsTrigger> | |
| <TabsTrigger value="password">Password Edit</TabsTrigger> | |
| <TabsTrigger value="api">API Access Tokens</TabsTrigger> | |
| </TabsList> | |
| <TabsContent value="profile"> | |
| <ErrorBoundary fallback={<div>Error loading profile content</div>}> | |
| <MyProfile /> | |
| </ErrorBoundary> | |
| </TabsContent> | |
| <TabsContent value="password"> | |
| <ErrorBoundary fallback={<div>Error loading password edit content</div>}> | |
| <PasswordEdit /> | |
| </ErrorBoundary> | |
| </TabsContent> | |
| <TabsContent value="api"> | |
| <ErrorBoundary fallback={<div>Error loading API tokens content</div>}> | |
| <ApiTokens /> | |
| </ErrorBoundary> | |
| </TabsContent> | |
| </Tabs> | |
| ) | |
| } |
| const handleSubmit = (e: React.FormEvent) => { | ||
| e.preventDefault() | ||
| // Here you would typically send the new password to your backend | ||
| console.log("Password change request:", passwords) | ||
| // Reset form after submission | ||
| setPasswords({ current: "", new: "", confirm: "" }) | ||
| } |
There was a problem hiding this comment.
Remove password logging and implement proper validation.
Critical security issue: Passwords should never be logged to the console, even during development.
const handleSubmit = (e: React.FormEvent) => {
e.preventDefault()
- // Here you would typically send the new password to your backend
- console.log("Password change request:", passwords)
+ if (passwords.new !== passwords.confirm) {
+ // Add error state handling
+ return
+ }
// Reset form after submission
setPasswords({ current: "", new: "", confirm: "" })
}📝 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 = (e: React.FormEvent) => { | |
| e.preventDefault() | |
| // Here you would typically send the new password to your backend | |
| console.log("Password change request:", passwords) | |
| // Reset form after submission | |
| setPasswords({ current: "", new: "", confirm: "" }) | |
| } | |
| const handleSubmit = (e: React.FormEvent) => { | |
| e.preventDefault() | |
| if (passwords.new !== passwords.confirm) { | |
| // Add error state handling | |
| return | |
| } | |
| // Reset form after submission | |
| setPasswords({ current: "", new: "", confirm: "" }) | |
| } |
| export default function PasswordEdit() { | ||
| const [passwords, setPasswords] = useState({ | ||
| current: "", | ||
| new: "", | ||
| confirm: "", | ||
| }) | ||
|
|
||
| const handleInputChange = (e: React.ChangeEvent<HTMLInputElement>) => { | ||
| setPasswords({ ...passwords, [e.target.name]: e.target.value }) | ||
| } | ||
|
|
||
| const handleSubmit = (e: React.FormEvent) => { | ||
| e.preventDefault() | ||
| // Here you would typically send the new password to your backend | ||
| console.log("Password change request:", passwords) | ||
| // Reset form after submission | ||
| setPasswords({ current: "", new: "", confirm: "" }) | ||
| } | ||
|
|
||
| return ( | ||
| <div className="space-y-6"> | ||
| <h2 className="text-2xl font-bold">Change Password</h2> | ||
| <form onSubmit={handleSubmit} className="space-y-4"> | ||
| <div className="space-y-2"> | ||
| <Label htmlFor="current-password">Current Password</Label> | ||
| <Input | ||
| id="current-password" | ||
| name="current" | ||
| type="password" | ||
| value={passwords.current} | ||
| onChange={handleInputChange} | ||
| required | ||
| /> | ||
| </div> | ||
| <div className="space-y-2"> | ||
| <Label htmlFor="new-password">New Password</Label> | ||
| <Input | ||
| id="new-password" | ||
| name="new" | ||
| type="password" | ||
| value={passwords.new} | ||
| onChange={handleInputChange} | ||
| required | ||
| /> | ||
| </div> | ||
| <div className="space-y-2"> | ||
| <Label htmlFor="confirm-password">Confirm New Password</Label> | ||
| <Input | ||
| id="confirm-password" | ||
| name="confirm" | ||
| type="password" | ||
| value={passwords.confirm} | ||
| onChange={handleInputChange} | ||
| required | ||
| /> | ||
| </div> | ||
| <Button type="submit">Change Password</Button> | ||
| </form> | ||
| </div> | ||
| ) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance password change form with validation and UX improvements.
The form needs several security and usability improvements:
- Password match validation
- Password strength requirements
- Visual feedback for validation state
Consider implementing a more robust password change form:
export default function PasswordEdit() {
const [passwords, setPasswords] = useState({
current: "",
new: "",
confirm: "",
})
+ const [errors, setErrors] = useState({
+ new: "",
+ confirm: "",
+ })
+
+ const validatePassword = (password: string) => {
+ if (password.length < 8) {
+ return "Password must be at least 8 characters long"
+ }
+ if (!/[A-Z]/.test(password)) {
+ return "Password must contain at least one uppercase letter"
+ }
+ // Add more validation rules as needed
+ return ""
+ }
+
+ const handleNewPasswordChange = (e: React.ChangeEvent<HTMLInputElement>) => {
+ const newPassword = e.target.value
+ setPasswords(prev => ({ ...prev, new: newPassword }))
+ setErrors(prev => ({
+ ...prev,
+ new: validatePassword(newPassword),
+ confirm: newPassword !== passwords.confirm ? "Passwords do not match" : ""
+ }))
+ }Committable suggestion skipped: line range outside the PR's diff.
| <Table> | ||
| <TableHeader> | ||
| <TableRow> | ||
| <TableHead>Client name</TableHead> | ||
| <TableHead>IP Address</TableHead> | ||
| <TableHead>Client Status</TableHead> | ||
| <TableHead>Created</TableHead> | ||
| <TableHead>Token</TableHead> | ||
| <TableHead>Expires</TableHead> | ||
| </TableRow> | ||
| </TableHeader> | ||
| <TableBody> | ||
| {tokens.map((token) => ( | ||
| <TableRow key={token.id}> | ||
| <TableCell>{token.name}</TableCell> | ||
| <TableCell>{token.ipAddress}</TableCell> | ||
| <TableCell>{token.status}</TableCell> | ||
| <TableCell>{token.created}</TableCell> | ||
| <TableCell>{token.token}</TableCell> | ||
| <TableCell>{token.expires}</TableCell> | ||
| </TableRow> | ||
| ))} | ||
| </TableBody> | ||
| </Table> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add token management features and improve security.
Consider adding:
- Token masking
- Copy to clipboard functionality
- Token revocation
- Last used timestamp
<Table>
<TableHeader>
<TableRow>
<TableHead>Client name</TableHead>
<TableHead>IP Address</TableHead>
<TableHead>Client Status</TableHead>
<TableHead>Created</TableHead>
<TableHead>Token</TableHead>
<TableHead>Expires</TableHead>
+ <TableHead>Actions</TableHead>
</TableRow>
</TableHeader>
<TableBody>
{tokens.map((token) => (
<TableRow key={token.id}>
<TableCell>{token.name}</TableCell>
<TableCell>{token.ipAddress}</TableCell>
<TableCell>{token.status}</TableCell>
<TableCell>{token.created}</TableCell>
- <TableCell>{token.token}</TableCell>
+ <TableCell>
+ <div className="flex items-center space-x-2">
+ <span>{token.token.substring(0, 8)}...</span>
+ <Button
+ variant="ghost"
+ size="sm"
+ onClick={() => navigator.clipboard.writeText(token.token)}
+ >
+ Copy
+ </Button>
+ </div>
+ </TableCell>
<TableCell>{token.expires}</TableCell>
+ <TableCell>
+ <Button
+ variant="destructive"
+ size="sm"
+ onClick={() => handleRevokeToken(token.id)}
+ >
+ Revoke
+ </Button>
+ </TableCell>
</TableRow>
))}
</TableBody>
</Table>Committable suggestion skipped: line range outside the PR's diff.
| const [tokens, setTokens] = useState<Token[]>([ | ||
| { | ||
| id: "1", | ||
| name: "Production API", | ||
| ipAddress: "192.168.1.1", | ||
| status: "Active", | ||
| created: "2023-01-01", | ||
| token: "abc123xyz789", | ||
| expires: "2024-01-01", | ||
| }, | ||
| { | ||
| id: "2", | ||
| name: "Development API", | ||
| ipAddress: "192.168.1.2", | ||
| status: "Active", | ||
| created: "2023-02-01", | ||
| token: "def456uvw890", | ||
| expires: "2024-02-01", | ||
| }, | ||
| ]) |
There was a problem hiding this comment.
Security: Remove hardcoded API tokens from the source code.
Exposing API tokens in the source code poses a security risk. Consider:
- Loading tokens from a secure backend API
- Using placeholder values for development
- const [tokens, setTokens] = useState<Token[]>([
- {
- id: "1",
- name: "Production API",
- ipAddress: "192.168.1.1",
- status: "Active",
- created: "2023-01-01",
- token: "abc123xyz789",
- expires: "2024-01-01",
- },
- {
- id: "2",
- name: "Development API",
- ipAddress: "192.168.1.2",
- status: "Active",
- created: "2023-02-01",
- token: "def456uvw890",
- expires: "2024-02-01",
- },
- ])
+ const [tokens, setTokens] = useState<Token[]>([])
+
+ useEffect(() => {
+ // Fetch tokens from secure backend API
+ const fetchTokens = async () => {
+ try {
+ const response = await fetch('/api/tokens');
+ const data = await response.json();
+ setTokens(data);
+ } catch (error) {
+ console.error('Failed to fetch tokens:', error);
+ }
+ };
+
+ fetchTokens();
+ }, []);📝 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 [tokens, setTokens] = useState<Token[]>([ | |
| { | |
| id: "1", | |
| name: "Production API", | |
| ipAddress: "192.168.1.1", | |
| status: "Active", | |
| created: "2023-01-01", | |
| token: "abc123xyz789", | |
| expires: "2024-01-01", | |
| }, | |
| { | |
| id: "2", | |
| name: "Development API", | |
| ipAddress: "192.168.1.2", | |
| status: "Active", | |
| created: "2023-02-01", | |
| token: "def456uvw890", | |
| expires: "2024-02-01", | |
| }, | |
| ]) | |
| const [tokens, setTokens] = useState<Token[]>([]) | |
| useEffect(() => { | |
| // Fetch tokens from secure backend API | |
| const fetchTokens = async () => { | |
| try { | |
| const response = await fetch('/api/tokens'); | |
| const data = await response.json(); | |
| setTokens(data); | |
| } catch (error) { | |
| console.error('Failed to fetch tokens:', error); | |
| } | |
| }; | |
| fetchTokens(); | |
| }, []); |
🧰 Tools
🪛 Gitleaks (8.21.2)
27-27: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
36-36: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
| const handleCreateToken = (e: React.FormEvent) => { | ||
| e.preventDefault() | ||
| // Here you would typically send a request to your backend to create a new token | ||
| const newToken: Token = { | ||
| id: String(tokens.length + 1), | ||
| name: newTokenName, | ||
| ipAddress: "192.168.1." + (tokens.length + 1), | ||
| status: "Active", | ||
| created: new Date().toISOString().split("T")[0], | ||
| token: Math.random().toString(36).substring(2, 15), | ||
| expires: new Date(Date.now() + 365 * 24 * 60 * 60 * 1000).toISOString().split("T")[0], | ||
| } | ||
| setTokens([...tokens, newToken]) | ||
| setNewTokenName("") | ||
| } |
There was a problem hiding this comment.
Security: Improve token generation and implement backend integration.
Current implementation has several security concerns:
- Using Math.random() for token generation is not cryptographically secure
- IP address generation is predictable
- Missing backend integration for token creation
const handleCreateToken = (e: React.FormEvent) => {
e.preventDefault()
- // Here you would typically send a request to your backend to create a new token
- const newToken: Token = {
- id: String(tokens.length + 1),
- name: newTokenName,
- ipAddress: "192.168.1." + (tokens.length + 1),
- status: "Active",
- created: new Date().toISOString().split("T")[0],
- token: Math.random().toString(36).substring(2, 15),
- expires: new Date(Date.now() + 365 * 24 * 60 * 60 * 1000).toISOString().split("T")[0],
- }
- setTokens([...tokens, newToken])
+ const createToken = async () => {
+ try {
+ const response = await fetch('/api/tokens', {
+ method: 'POST',
+ headers: {
+ 'Content-Type': 'application/json',
+ },
+ body: JSON.stringify({ name: newTokenName }),
+ });
+ const newToken = await response.json();
+ setTokens([...tokens, newToken]);
+ } catch (error) {
+ console.error('Failed to create token:', error);
+ }
+ };
+ createToken();
setNewTokenName("")
}Committable suggestion skipped: line range outside the PR's diff.
| const handleSubmit = (e: React.FormEvent) => { | ||
| e.preventDefault(); | ||
| console.log("Updated profile:", profile); | ||
| setIsEditing(false); | ||
| }; |
There was a problem hiding this comment.
Implement proper form submission with API integration.
Current implementation only logs to console. Add proper API integration and error handling.
const handleSubmit = (e: React.FormEvent) => {
e.preventDefault();
- console.log("Updated profile:", profile);
- setIsEditing(false);
+ const updateProfile = async () => {
+ try {
+ const response = await fetch(`/api/users/${currentuser._id}`, {
+ method: 'PUT',
+ headers: {
+ 'Content-Type': 'application/json',
+ },
+ body: JSON.stringify(profile),
+ });
+ if (!response.ok) throw new Error('Failed to update profile');
+ setIsEditing(false);
+ } catch (error) {
+ console.error('Error updating profile:', error);
+ // Show error message to user
+ }
+ };
+ updateProfile();
};Committable suggestion skipped: line range outside the PR's diff.
| const handleImageUpload = (e: React.ChangeEvent<HTMLInputElement>) => { | ||
| const file = e.target.files?.[0]; | ||
| if (file) { | ||
| const reader = new FileReader(); | ||
| reader.onloadend = () => { | ||
| setProfile({ ...profile, profilePicture: reader.result as string }); | ||
| }; | ||
| reader.readAsDataURL(file); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance image upload with validation and backend integration.
Add proper file validation and backend integration for image uploads.
+ const MAX_FILE_SIZE = 5 * 1024 * 1024; // 5MB
+ const ALLOWED_TYPES = ['image/jpeg', 'image/png', 'image/gif'];
const handleImageUpload = (e: React.ChangeEvent<HTMLInputElement>) => {
const file = e.target.files?.[0];
if (file) {
+ if (!ALLOWED_TYPES.includes(file.type)) {
+ alert('Please upload an image file (JPEG, PNG, or GIF)');
+ return;
+ }
+ if (file.size > MAX_FILE_SIZE) {
+ alert('File size should not exceed 5MB');
+ return;
+ }
+
const reader = new FileReader();
reader.onloadend = () => {
- setProfile({ ...profile, profilePicture: reader.result as string });
+ // Show preview
+ setProfile({ ...profile, profilePicture: reader.result as string });
+
+ // Upload to backend
+ const formData = new FormData();
+ formData.append('image', file);
+ fetch('/api/upload-profile-image', {
+ method: 'POST',
+ body: formData,
+ })
+ .then(response => response.json())
+ .then(data => {
+ setProfile({ ...profile, profilePicture: data.imageUrl });
+ })
+ .catch(error => {
+ console.error('Error uploading image:', error);
+ // Show error message to user
+ });
};
reader.readAsDataURL(file);
}
};📝 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 handleImageUpload = (e: React.ChangeEvent<HTMLInputElement>) => { | |
| const file = e.target.files?.[0]; | |
| if (file) { | |
| const reader = new FileReader(); | |
| reader.onloadend = () => { | |
| setProfile({ ...profile, profilePicture: reader.result as string }); | |
| }; | |
| reader.readAsDataURL(file); | |
| } | |
| }; | |
| const MAX_FILE_SIZE = 5 * 1024 * 1024; // 5MB | |
| const ALLOWED_TYPES = ['image/jpeg', 'image/png', 'image/gif']; | |
| const handleImageUpload = (e: React.ChangeEvent<HTMLInputElement>) => { | |
| const file = e.target.files?.[0]; | |
| if (file) { | |
| if (!ALLOWED_TYPES.includes(file.type)) { | |
| alert('Please upload an image file (JPEG, PNG, or GIF)'); | |
| return; | |
| } | |
| if (file.size > MAX_FILE_SIZE) { | |
| alert('File size should not exceed 5MB'); | |
| return; | |
| } | |
| const reader = new FileReader(); | |
| reader.onloadend = () => { | |
| // Show preview | |
| setProfile({ ...profile, profilePicture: reader.result as string }); | |
| // Upload to backend | |
| const formData = new FormData(); | |
| formData.append('image', file); | |
| fetch('/api/upload-profile-image', { | |
| method: 'POST', | |
| body: formData, | |
| }) | |
| .then(response => response.json()) | |
| .then(data => { | |
| setProfile({ ...profile, profilePicture: data.imageUrl }); | |
| }) | |
| .catch(error => { | |
| console.error('Error uploading image:', error); | |
| // Show error message to user | |
| }); | |
| }; | |
| reader.readAsDataURL(file); | |
| } | |
| }; |
| @@ -32,6 +32,7 @@ | |||
| "clsx": "^2.1.1", | |||
| "cmdk": "^1.0.4", | |||
| "date-fns": "^3.6.0", | |||
| "i18n-iso-countries": "^7.13.0", | |||
There was a problem hiding this comment.
💡 Codebase verification
Consider alternatives to timezones.json package
While i18n-iso-countries is healthy and well-maintained, timezones.json shows concerning signs of inactive maintenance (health score 57/100). Consider using more actively maintained alternatives like moment-timezone (91/100) or @js-joda/core (89/100).
Bundle sizes are reasonable:
- i18n-iso-countries: 6.8KB gzipped
- timezones.json: 5.4KB gzipped
🔗 Analysis chain
Verify package versions and security
The new dependencies look good, but let's verify their compatibility and security:
Also applies to: 53-53
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for known vulnerabilities and latest versions
echo "Checking npm audit for security issues..."
npm audit --json | jq '.advisories'
echo "Checking latest versions..."
npm view i18n-iso-countries version
npm view timezones.json version
echo "Checking bundle sizes..."
npm view i18n-iso-countries size
npm view timezones.json size
Length of output: 757
Script:
#!/bin/bash
# Check package security and size using public APIs
echo "Checking bundle sizes via bundlephobia..."
curl -s "https://bundlephobia.com/api/[email protected]" | jq '{name: "i18n-iso-countries", size: .size, gzip: .gzip}'
curl -s "https://bundlephobia.com/api/[email protected]" | jq '{name: "timezones.json", size: .size, gzip: .gzip}'
echo -e "\nChecking security via Snyk..."
curl -s "https://snyk.io/advisor/npm-package/i18n-iso-countries" | grep -A 2 "Health Score"
curl -s "https://snyk.io/advisor/npm-package/timezones.json" | grep -A 2 "Health Score"
Length of output: 17883
netmanager-app/app/(authenticated)/profile/components/PasswordEdit.tsx
Outdated
Show resolved
Hide resolved
| status: "Active", | ||
| created: "2023-02-01", | ||
| token: "def456uvw890", | ||
| expires: "2024-02-01", |
There was a problem hiding this comment.
Any reason for using dummy data here, the corresponding apis should be working just fine @danielmarv
| expires: new Date(Date.now() + 365 * 24 * 60 * 60 * 1000).toISOString().split("T")[0], | ||
| } | ||
| setTokens([...tokens, newToken]) | ||
| setNewTokenName("") |
There was a problem hiding this comment.
Use token creation api @danielmarv
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
netmanager-app/app/(authenticated)/profile/components/MyProfile.tsx (3)
55-58:⚠️ Potential issueImplement proper form submission with API integration.
The form submission is not functional. This needs to be implemented with proper API integration.
60-69:⚠️ Potential issueEnhance image upload with validation and backend integration.
The image upload needs proper file validation and backend integration.
114-198: 🛠️ Refactor suggestionFix form field validation and editing state.
The form fields have incorrect editing state and lack proper validation.
netmanager-app/app/(authenticated)/profile/components/ApiTokens.tsx (1)
66-116:⚠️ Potential issueMask the tokens for security and add copy functionality.
Displaying raw tokens may leak sensitive information. Consider masking them and adding a “Copy” button, adopting best practices for token management.-<TableCell>{client.access_token.token}</TableCell> +<TableCell> + <div className="flex items-center space-x-2"> + <span>{client.access_token.token.substring(0, 8)}...</span> + <Button + variant="ghost" + size="sm" + onClick={() => navigator.clipboard.writeText(client.access_token.token)} + > + Copy + </Button> + </div> +</TableCell>
🧹 Nitpick comments (5)
netmanager-app/app/(authenticated)/profile/components/MyProfile.tsx (1)
94-111: Add spacing between avatar buttons and improve UI.The avatar update and delete buttons need proper spacing, and the delete button needs confirmation dialog.
- <div> + <div className="space-y-2"> <Button type="button" variant="outline" className="mb-2"> <Label htmlFor="avatar-upload" className="cursor-pointer"> Update </Label> </Button> <Input id="avatar-upload" type="file" accept="image/*" className="hidden" onChange={handleImageUpload} - disabled={isEditing} + disabled={!isEditing} /> - <Button type="button" variant="outline" className="text-red-500"> + <Button + type="button" + variant="outline" + className="text-red-500 ml-2" + onClick={() => { + if (window.confirm('Are you sure you want to delete your profile picture?')) { + setProfile({ ...profile, profilePicture: null }); + } + }} + > Delete </Button> </div>netmanager-app/app/(authenticated)/profile/components/ApiTokens.tsx (3)
13-18: Nitpick: renamecurrentusertocurrentUser.
Renaming the variable tocurrentUserimproves consistency with conventional naming.- const currentuser = useAppSelector((state) => state.user.userDetails) + const currentUser = useAppSelector((state) => state.user.userDetails)
19-34: Add early-return check for missing user ID.
Consider returning early whenuserIDis empty, to avoid unnecessary API calls or potential undefined behavior.+ if (!userID) { + setIsLoading(false); + return; + }
36-41: Implement an actual token creation flow.
ThehandleCreateTokenhandler currently only logs that the feature is not implemented. Consider integrating the appropriate API to create tokens.const handleCreateToken = async (e: React.FormEvent) => { e.preventDefault(); - console.log("Create token feature is not implemented yet."); + try { + const response = await fetch("/api/tokens", { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ name: newTokenName }), + }); + const newToken = await response.json(); + // TODO: handle newToken (e.g., set state to show updated tokens in the table) + } catch (err) { + console.error("Error creating token:", err); + } };netmanager-app/core/apis/settings.ts (1)
7-17: Enhance error handling for improved resiliency.
Currently, errors are logged, but the function rethrows them without user-facing feedback. Consider returning a fallback or providing a user-friendly error message for better usability.export const getUserClientsApi = async (userID: string): Promise<Client[]> => { try { const response = await axiosInstance.get<Client[]>(USERS_MGT_URL, { params: { user_id: userID } }); return response.data; } catch (error) { console.error("Error fetching clients:", error); + // Optionally handle errors more gracefully: + // return []; throw error; } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
netmanager-app/app/(authenticated)/profile/components/ApiTokens.tsx(1 hunks)netmanager-app/app/(authenticated)/profile/components/MyProfile.tsx(1 hunks)netmanager-app/app/types/clients.ts(1 hunks)netmanager-app/core/apis/settings.ts(1 hunks)
🔇 Additional comments (7)
netmanager-app/app/(authenticated)/profile/components/MyProfile.tsx (1)
1-14: LGTM! Well-structured imports and setup.The component follows Next.js 13+ conventions and includes all necessary imports for UI components, utilities, and state management.
netmanager-app/app/(authenticated)/profile/components/ApiTokens.tsx (4)
1-2: No issues with the client directive.
Your"use client"directive properly indicates that this component should be rendered on the client side.
3-12: Imports appear valid and well-organized.
All imported modules are used, and there's no indication of any redundant imports.
42-49: Heading and description are clear.
This section effectively explains the purpose of API tokens and user responsibilities. Good job.
50-65: Form design follows good accessibility practices.
Using a visually hidden label (sr-only) while ensuring the input isrequiredprovides a user-friendly form. Great approach!netmanager-app/app/types/clients.ts (2)
3-15: Interface forAccessTokenlooks clear.
Defining strict field types benefits maintainability. Confirm that string fields likeexpiresandcreatedAtalign with how the API returns date/time data.
17-25:Clientinterface is well-defined.
TheClientinterface comprehensively describes an entity with anaccess_tokensub-object. No major issues found.
|
|
||
| export default function MyProfile() { | ||
| const currentuser = useAppSelector((state) => state.user.userDetails); | ||
| const [profile, setProfile] = useState<any>(null); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add proper TypeScript interface for profile state.
Using any type reduces type safety. Define a proper interface for the profile state.
+interface Profile {
+ firstName: string;
+ lastName: string;
+ email: string;
+ jobTitle?: string;
+ country?: string;
+ timezone?: string;
+ bio?: string;
+ profilePicture?: string;
+}
-const [profile, setProfile] = useState<any>(null);
+const [profile, setProfile] = useState<Profile | null>(null);📝 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 [profile, setProfile] = useState<any>(null); | |
| interface Profile { | |
| firstName: string; | |
| lastName: string; | |
| email: string; | |
| jobTitle?: string; | |
| country?: string; | |
| timezone?: string; | |
| bio?: string; | |
| profilePicture?: string; | |
| } | |
| const [profile, setProfile] = useState<Profile | null>(null); |
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (8)
netmanager-app/app/(authenticated)/profile/components/DialogWrapper.tsx (3)
7-7: Consider using a more specific type for ModalIcon.Instead of using React.ComponentType with an optional className, consider using a more specific type that better represents the expected icon component interface.
- ModalIcon?: React.ComponentType<{ className?: string }> + ModalIcon?: React.ComponentType<{ className?: string; size?: number; color?: string }>
40-40: Consider making footer alignment configurable.The footer alignment is hardcoded to 'sm:justify-start'. Consider making this configurable through props to support different alignment needs.
- <DialogFooter className="sm:justify-start"> + <DialogFooter className={`${footerClassName ?? 'sm:justify-start'}`}>
53-54: Choose a single export pattern.Having both named and default exports for the same component can lead to inconsistent import patterns across the codebase. Consider using only one export method.
-export const DialogWrapper: React.FC<DialogWrapperProps> = ({ -export default DialogWrapper +export const DialogWrapper: React.FC<DialogWrapperProps> = ({netmanager-app/app/(authenticated)/profile/components/ClientTableRow.tsx (1)
53-77: Enhance token security and copy feedback.Consider these security and UX improvements:
- Use a more secure token masking pattern
- Add visual feedback for the copy action
- {getClientToken(client._id).slice(0, 2)}.... - {getClientToken(client._id).slice(-2)} + {getClientToken(client._id).slice(0, 4)}••••• + {getClientToken(client._id).slice(-4)} <div className="w-6 h-6 bg-white rounded border border-gray-200 flex justify-center items-center gap-2 cursor-pointer" - onClick={() => onCopyToken(getClientToken(client._id))} + onClick={() => { + onCopyToken(getClientToken(client._id)); + // Add toast notification for copy feedback + toast.success('Token copied to clipboard'); + }}netmanager-app/core/apis/settings.ts (1)
60-70: Remove or restore commented code.The commented-out code should either be removed if it's no longer needed or restored if it's still required. Keeping commented code can lead to confusion.
netmanager-app/app/(authenticated)/profile/components/ApiTokens.tsx (3)
17-18: Consider consolidating error states.The component maintains separate error states for general errors and activation request errors. Consider consolidating them into a single error state manager for better maintainability.
- const [isError, setIsError] = useState({ isError: false, message: "", type: "" }) - const [isActivationRequestError, setIsActivationRequestError] = useState({ isError: false, message: "", type: "" }) + const [errors, setErrors] = useState({ + general: { isError: false, message: "", type: "" }, + activationRequest: { isError: false, message: "", type: "" } + })
29-29: Consider making itemsPerPage configurable.The itemsPerPage value is hardcoded to 4, which limits flexibility. Consider making it configurable through props or environment variables.
- const itemsPerPage = 4 + const itemsPerPage = props.itemsPerPage || process.env.NEXT_PUBLIC_DEFAULT_ITEMS_PER_PAGE || 4
78-100: Refactor client token utility functions.The client token utility functions share similar patterns and could be consolidated into a more maintainable structure.
+ const getClientData = (clientId: string) => { + return Array.isArray(clientsDetails) + ? clientsDetails?.find((client: any) => client._id === clientId) + : undefined + } + + const getClientTokenData = (clientId: string, field?: string) => { + const client = getClientData(clientId) + if (!client?.access_token) return undefined + return field ? client.access_token[field] : client.access_token + } + - const hasAccessToken = (clientId: string) => { - const client = - Array.isArray(clientsDetails) && (clientsDetails) - ? clientsDetails?.find((client: any) => client._id === clientId) - : undefined - return client && client.access_token - } + const hasAccessToken = (clientId: string) => Boolean(getClientTokenData(clientId)) + - const getClientToken = (clientID: string) => { - const client = - Array.isArray(clientsDetails) && (clientsDetails) - ? clientsDetails?.find((client: any) => client._id === clientID) - : undefined - return client && client.access_token && client.access_token.token - } + const getClientToken = (clientId: string) => getClientTokenData(clientId, 'token') + - const getClientTokenExpiryDate = (clientID: string) => { - const client = - Array.isArray(clientsDetails) && (clientsDetails) - ? clientsDetails?.find((client: any) => client._id === clientID) - : undefined - return client && client.access_token && client.access_token.expires - } + const getClientTokenExpiryDate = (clientId: string) => getClientTokenData(clientId, 'expires')🧰 Tools
🪛 Biome (1.9.4)
[error] 83-83: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 91-91: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 99-99: 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 (6)
netmanager-app/app/(authenticated)/profile/components/ApiTokens.tsx(1 hunks)netmanager-app/app/(authenticated)/profile/components/ClientTableRow.tsx(1 hunks)netmanager-app/app/(authenticated)/profile/components/DialogWrapper.tsx(1 hunks)netmanager-app/app/(authenticated)/profile/components/EditClientForm.tsx(1 hunks)netmanager-app/core/apis/settings.ts(1 hunks)netmanager-app/package.json(2 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)/profile/components/ApiTokens.tsx
[error] 13-13: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 13-13: unterminated string literal
The closing quote must be on the same line.
(parse)
[error] 83-83: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 91-91: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 99-99: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
netmanager-app/app/(authenticated)/profile/components/EditClientForm.tsx
[error] 30-30: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
🔇 Additional comments (1)
netmanager-app/app/(authenticated)/profile/components/ApiTokens.tsx (1)
109-109: Replace dummy data with proper API integration.Based on previous review comments, ensure proper backend integration for token management instead of using dummy data.
#!/bin/bash # Check for API integration implementation rg -l "getUserDetailsAccount|generateToken|activationRequest" --type ts
| const displayIPAddresses = (client) => { | ||
| return Array.isArray(client.ip_addresses) ? client.ip_addresses.join(", ") : client.ip_addresses | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add IP address format validation.
The current implementation doesn't validate IP address formats. Consider adding validation to ensure proper IP address formatting.
const displayIPAddresses = (client) => {
+ const isValidIP = (ip: string) => {
+ const ipv4Regex = /^(\d{1,3}\.){3}\d{1,3}$/;
+ return ipv4Regex.test(ip) && ip.split('.').every(num => parseInt(num) <= 255);
+ };
+ const addresses = Array.isArray(client.ip_addresses) ? client.ip_addresses : [client.ip_addresses];
+ return addresses.filter(ip => ip && isValidIP(ip)).join(", ") || "Invalid IP";
- return Array.isArray(client.ip_addresses) ? client.ip_addresses.join(", ") : client.ip_addresses
}📝 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 displayIPAddresses = (client) => { | |
| return Array.isArray(client.ip_addresses) ? client.ip_addresses.join(", ") : client.ip_addresses | |
| } | |
| const displayIPAddresses = (client) => { | |
| const isValidIP = (ip: string) => { | |
| const ipv4Regex = /^(\d{1,3}\.){3}\d{1,3}$/; | |
| return ipv4Regex.test(ip) && ip.split('.').every(num => parseInt(num) <= 255); | |
| }; | |
| const addresses = Array.isArray(client.ip_addresses) ? client.ip_addresses : [client.ip_addresses]; | |
| return addresses.filter(ip => ip && isValidIP(ip)).join(", ") || "Invalid IP"; | |
| } |
There was a problem hiding this comment.
@danielmarv ,please consider this suggestion
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| import EditIcon from "@/icons/Common/edit-pencil.svg" | ||
|
|
||
| interface ClientTableRowProps { | ||
| client: any |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace 'any' type with a proper Client interface.
Using 'any' type reduces type safety and IDE support. Consider using the Client interface that's already defined in your types.
- client: any
+ client: ClientCommittable suggestion skipped: line range outside the PR's diff.
| } catch (error: any) { | ||
| setIsError({ | ||
| isError: true, | ||
| message: error?.response?.data?.message || "Failed to Edit client", | ||
| type: "error", | ||
| }) | ||
| } finally { | ||
| setLoading(false) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling implementation.
The current error handling implementation can be enhanced:
- Replace
anywith proper error type - Make the error message timeout configurable
- Add retry mechanism for API calls
- } catch (error: any) {
+ } catch (error: Error | ApiError) {
+ const errorMessage = isApiError(error)
+ ? error.response?.data?.message
+ : error.message;
setIsError({
isError: true,
- message: error?.response?.data?.message || "Failed to Edit client",
+ message: errorMessage || "Failed to Edit client",
type: "error",
})
+ // Optional: Add retry logic here
} finally {Committable suggestion skipped: line range outside the PR's diff.
| useEffect(() => { | ||
| handleInitialData() | ||
| }, [handleInitialData]) // Updated dependency |
There was a problem hiding this comment.
Fix useEffect dependency cycle.
The current implementation creates a dependency cycle. Move the handleInitialData function inside useEffect or use useCallback.
- useEffect(() => {
- handleInitialData()
- }, [handleInitialData])
+ useEffect(() => {
+ const handleInitialData = () => {
+ setClientName(data?.name || "")
+ const ipAddresses = Array.isArray(data?.ip_addresses)
+ ? data?.ip_addresses
+ : data?.ip_addresses
+ ? [data?.ip_addresses]
+ : [""]
+ setIpAddresses(ipAddresses)
+ }
+ handleInitialData()
+ }, [data])Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome (1.9.4)
[error] 30-30: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
netmanager-app/core/apis/settings.ts
Outdated
| userId: string, | ||
| tenant: string, | ||
| userData: UserDetails | ||
| ): Promise<any> => { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace Promise with specific return types.
Using Promise<any> reduces type safety. Define and use specific return types for each API response.
-): Promise<any> => {
+): Promise<{ success: boolean; message: string; data: UserDetails }> => {
-): Promise<any> => {
+): Promise<{ success: boolean; message: string; data: Client }> => {
-export const generateTokenApi = async (data: AccessToken): Promise<any> => {
+export const generateTokenApi = async (data: AccessToken): Promise<{ success: boolean; token: string; expiresAt: string }> => {Also applies to: 49-49, 55-55
| const handleGenerateToken = async (res: any) => { | ||
| setIsLoadingToken(true) | ||
| if (!res?.isActive) { | ||
| setShowInfoModal(true) | ||
| setIsLoadingToken(false) | ||
| } else { | ||
| try { | ||
| const response = await api.generateToken(res) | ||
| if (response.success === true) { | ||
| setErrorState("Token generated", "success") | ||
| } | ||
| dispatch(performRefresh()) | ||
| } catch (error: any) { | ||
| setErrorState(error.message, "error") | ||
| } finally { | ||
| setIsLoadingToken(false) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling in token generation.
The handleGenerateToken function could benefit from more specific error handling and user feedback.
const handleGenerateToken = async (res: any) => {
setIsLoadingToken(true)
if (!res?.isActive) {
setShowInfoModal(true)
setIsLoadingToken(false)
} else {
try {
const response = await api.generateToken(res)
if (response.success === true) {
setErrorState("Token generated", "success")
+ await dispatch(performRefresh())
}
- dispatch(performRefresh())
} catch (error: any) {
- setErrorState(error.message, "error")
+ const errorMessage = error.response?.data?.message || error.message || "Failed to generate token"
+ setErrorState(errorMessage, "error")
+ console.error("Token generation failed:", error)
} finally {
setIsLoadingToken(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 handleGenerateToken = async (res: any) => { | |
| setIsLoadingToken(true) | |
| if (!res?.isActive) { | |
| setShowInfoModal(true) | |
| setIsLoadingToken(false) | |
| } else { | |
| try { | |
| const response = await api.generateToken(res) | |
| if (response.success === true) { | |
| setErrorState("Token generated", "success") | |
| } | |
| dispatch(performRefresh()) | |
| } catch (error: any) { | |
| setErrorState(error.message, "error") | |
| } finally { | |
| setIsLoadingToken(false) | |
| } | |
| } | |
| } | |
| const handleGenerateToken = async (res: any) => { | |
| setIsLoadingToken(true) | |
| if (!res?.isActive) { | |
| setShowInfoModal(true) | |
| setIsLoadingToken(false) | |
| } else { | |
| try { | |
| const response = await api.generateToken(res) | |
| if (response.success === true) { | |
| setErrorState("Token generated", "success") | |
| await dispatch(performRefresh()) | |
| } | |
| } catch (error: any) { | |
| const errorMessage = error.response?.data?.message || error.message || "Failed to generate token" | |
| setErrorState(errorMessage, "error") | |
| console.error("Token generation failed:", error) | |
| } finally { | |
| setIsLoadingToken(false) | |
| } | |
| } | |
| } |
| )} | ||
| </TableBody> | ||
| </Table> | ||
| {/* Add your Pagination component here */} |
There was a problem hiding this comment.
Implement pagination component.
The comment indicates a missing pagination component. This is essential for proper table navigation.
- {/* Add your Pagination component here */}
+ {clients?.length > itemsPerPage && (
+ <div className="flex justify-center mt-4">
+ <div className="flex space-x-2">
+ {Array.from({ length: Math.ceil(clients.length / itemsPerPage) }).map((_, index) => (
+ <Button
+ key={index}
+ variant={currentPage === index + 1 ? "default" : "outline"}
+ size="sm"
+ onClick={() => onPageChange(index + 1)}
+ >
+ {index + 1}
+ </Button>
+ ))}
+ </div>
+ </div>
+ )}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Hi @danielmarv , I hope you have been able to review this suggestion?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| {getClientToken(client._id) ? ( | ||
| <div className="flex items-center"> | ||
| <span className="mr-2"> | ||
| {getClientToken(client._id).slice(0, 2)}.... | ||
| {getClientToken(client._id).slice(-2)} | ||
| </span> | ||
| <Button | ||
| variant="ghost" | ||
| size="sm" | ||
| onClick={() => { | ||
| navigator.clipboard.writeText(getClientToken(client._id)) | ||
| setErrorState("Token copied to clipboard!", "success") | ||
| }} | ||
| > | ||
| <Copy className="h-4 w-4" /> | ||
| </Button> | ||
| </div> | ||
| ) : ( |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance token display security and UX.
The current token display implementation could be improved for better security and user experience.
<TableCell>
{getClientToken(client._id) ? (
<div className="flex items-center">
- <span className="mr-2">
- {getClientToken(client._id).slice(0, 2)}....
- {getClientToken(client._id).slice(-2)}
- </span>
+ <span className="mr-2 font-mono">
+ {getClientToken(client._id).slice(0, 4)}
+ <span className="mx-1">•••••••</span>
+ {getClientToken(client._id).slice(-4)}
+ </span>
<Button
variant="ghost"
size="sm"
onClick={() => {
navigator.clipboard.writeText(getClientToken(client._id))
- setErrorState("Token copied to clipboard!", "success")
+ setErrorState("API token copied to clipboard", "success")
}}
+ title="Copy full token"
>
<Copy className="h-4 w-4" />
</Button>
</div>📝 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.
| {getClientToken(client._id) ? ( | |
| <div className="flex items-center"> | |
| <span className="mr-2"> | |
| {getClientToken(client._id).slice(0, 2)}.... | |
| {getClientToken(client._id).slice(-2)} | |
| </span> | |
| <Button | |
| variant="ghost" | |
| size="sm" | |
| onClick={() => { | |
| navigator.clipboard.writeText(getClientToken(client._id)) | |
| setErrorState("Token copied to clipboard!", "success") | |
| }} | |
| > | |
| <Copy className="h-4 w-4" /> | |
| </Button> | |
| </div> | |
| ) : ( | |
| {getClientToken(client._id) ? ( | |
| <div className="flex items-center"> | |
| <span className="mr-2 font-mono"> | |
| {getClientToken(client._id).slice(0, 4)} | |
| <span className="mx-1">•••••••</span> | |
| {getClientToken(client._id).slice(-4)} | |
| </span> | |
| <Button | |
| variant="ghost" | |
| size="sm" | |
| onClick={() => { | |
| navigator.clipboard.writeText(getClientToken(client._id)) | |
| setErrorState("API token copied to clipboard", "success") | |
| }} | |
| title="Copy full token" | |
| > | |
| <Copy className="h-4 w-4" /> | |
| </Button> | |
| </div> | |
| ) : ( |
| import { Table, TableBody, TableCell, TableHead, TableHeader, TableRow } from "@/components/ui/table" | ||
| import { Skeleton } from "@/components/ui/skeleton" | ||
| import { Edit, Copy, Info } from "lucide-react" | ||
| import { api } from "../utils/api"tore" |
There was a problem hiding this comment.
Fix syntax error in import statement.
There's a typo in the import statement that would prevent compilation.
-import { api } from "../utils/api"tore"
+import { api } from "../utils/api"Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome (1.9.4)
[error] 13-13: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 13-13: unterminated string literal
The closing quote must be on the same line.
(parse)
… UI toast notifications
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (9)
netmanager-app/core/hooks/useClients.ts (2)
13-14: Improve type safety by replacinganytypeThe
dataparameter uses theanytype which bypasses TypeScript's type checking benefits. Consider defining a proper interface for the API response.- onSuccess: (data: any) => { + onSuccess: (data: { clients: Client[] }) => { dispatch(setClients(data)); },
16-18: Enhance error handlingConsider adding error details to help with debugging.
onError: (error: Error) => { - dispatch(setError(error.message)); + dispatch(setError(`Failed to fetch clients: ${error.message}`)); + console.error('Client fetch error:', error); },netmanager-app/core/redux/slices/clientsSlice.ts (2)
30-32: Add type safety to action payloadThe action payload type is missing which could lead to type-related bugs.
- addClients: (state, action) => { + addClients: (state, action: PayloadAction<Client[]>) => { state.clients = action.payload; },
33-35: Add type safety to clientsDetails actionSimilar to above, add type safety to the action payload.
- addClientsDetails: (state, action) => { + addClientsDetails: (state, action: PayloadAction<Client[]>) => { state.clientsDetails = action.payload; },netmanager-app/hooks/use-toast.ts (1)
96-104: Consider extracting side effect into separate actionThe comment suggests extracting the side effect into a separate action. This would improve code organization and testability.
Consider creating a separate
dismissToastaction to handle the timeout side effect.netmanager-app/app/(authenticated)/profile/components/CreateClientForm.tsx (1)
20-24: Consider adding error state management.The component could benefit from explicit error state management to handle API failures and validation errors gracefully.
const [clientName, setClientName] = useState('') const [ipAddresses, setIpAddresses] = useState(['']) const [isLoading, setIsLoading] = useState(false) +const [error, setError] = useState<{ message: string } | null>(null)netmanager-app/app/(authenticated)/profile/components/ApiTokens.tsx (3)
35-54: Add error type and improve error handling in fetchClients.The error handling could be more specific and informative.
- } catch (error) { - console.error(error) + } catch (error: unknown) { + const errorMessage = error instanceof Error ? error.message : 'An unexpected error occurred' + console.error('Failed to fetch clients:', error) toast({ title: "Error", - description: "Failed to fetch user details", + description: errorMessage, variant: "destructive", })
133-160: Remove unnecessary setTimeout in error handling.The setTimeout calls in the error handling don't serve a clear purpose and could be removed.
try { const clientID = selectedClient?._id const response = await activationRequestApi(clientID) if (response) { setShowInfoModal(false) - setTimeout(() => { - toast({ - title: "Success", - description: "Activation request sent successfully", - variant: "success", - }) - }, 3000) + toast({ + title: "Success", + description: "Activation request sent successfully", + variant: "success", + }) } } catch (error: any) { setShowInfoModal(false) - setTimeout(() => { - toast({ - title: "Error", - description: error.message || "Failed to send activation request", - variant: "destructive", - }) - }, 3000) + toast({ + title: "Error", + description: error.message || "Failed to send activation request", + variant: "destructive", + }) }
81-103: Optimize client lookup functions with optional chaining.The helper functions for token-related operations could be simplified using optional chaining.
const hasAccessToken = (clientId: string) => { - const client = - Array.isArray(clientsDetails) && clientsDetails - ? clientsDetails?.find((client: Client) => client._id === clientId) - : undefined - return client && client.access_token + return clientsDetails?.find((client: Client) => client._id === clientId)?.access_token } const getClientToken = (clientID: string) => { - const client = - Array.isArray(clientsDetails) && clientsDetails - ? clientsDetails?.find((client: Client) => client._id === clientID) - : undefined - return client && client.access_token && client.access_token.token + return clientsDetails?.find((client: Client) => client._id === clientID)?.access_token?.token } const getClientTokenExpiryDate = (clientID: string) => { - const client = - Array.isArray(clientsDetails) && clientsDetails - ? clientsDetails?.find((client: Client) => client._id === clientID) - : undefined - return client && client.access_token && client.access_token.expires + return clientsDetails?.find((client: Client) => client._id === clientID)?.access_token?.expires }🧰 Tools
🪛 Biome (1.9.4)
[error] 86-86: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 94-94: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 102-102: 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 (11)
netmanager-app/app/(authenticated)/profile/components/ApiTokens.tsx(1 hunks)netmanager-app/app/(authenticated)/profile/components/CreateClientForm.tsx(1 hunks)netmanager-app/app/(authenticated)/profile/components/EditClientForm.tsx(1 hunks)netmanager-app/components/ui/toast.tsx(9 hunks)netmanager-app/components/ui/toaster.tsx(1 hunks)netmanager-app/core/apis/settings.ts(1 hunks)netmanager-app/core/hooks/useClients.ts(1 hunks)netmanager-app/core/redux/slices/clientsSlice.ts(1 hunks)netmanager-app/core/redux/store.ts(2 hunks)netmanager-app/hooks/use-toast.ts(1 hunks)netmanager-app/package.json(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- netmanager-app/components/ui/toast.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
netmanager-app/app/(authenticated)/profile/components/ApiTokens.tsx
[error] 86-86: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 94-94: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 102-102: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (13)
netmanager-app/core/redux/store.ts (1)
Line range hint
9-18: LGTM on the store configuration!The integration of the clients reducer follows the established pattern and Redux Toolkit best practices. The structure is consistent with other reducers in the store.
netmanager-app/core/hooks/useClients.ts (1)
21-25: LGTM! Good error handling patternThe fallback to empty array and proper error typing is a good practice.
netmanager-app/components/ui/toaster.tsx (1)
3-3: LGTM! Clean implementationThe import path change and component implementation follow React best practices.
netmanager-app/core/redux/slices/clientsSlice.ts (1)
5-11: LGTM! Well-structured state interfaceThe state interface is well-defined with proper typing and initial state values.
Also applies to: 13-19
netmanager-app/hooks/use-toast.ts (2)
11-12: Review toast configuration constants
TOAST_LIMIT = 1seems very restrictive. Consider increasing it to allow multiple toasts.TOAST_REMOVE_DELAY = 1000000(~16.7 minutes) seems too long for a toast notification.Would you like to adjust these values? Common values are:
- TOAST_LIMIT: 3-5
- TOAST_REMOVE_DELAY: 3000-5000 (3-5 seconds)
174-192: LGTM! Clean hook implementationThe hook implementation is clean with proper cleanup of listeners in the useEffect.
netmanager-app/package.json (1)
55-55: Consider alternatives to timezones.json package.netmanager-app/core/apis/settings.ts (2)
50-53: 🛠️ Refactor suggestionUse consistent axios instance.
The function creates a new axios instance while other functions reuse the existing one.
- return await createAxiosInstance() + return await axiosInstance .put(`${USERS_MGT_URL}/clients/${client_id}`, data) .then((response) => response.data);Likely invalid or redundant comment.
60-68: 🛠️ Refactor suggestionAdd explicit return type definitions.
Functions are missing return type definitions which reduces type safety.
-export const getClientsApi = async () => { +export const getClientsApi = async (): Promise<Client[]> => { -export const activateUserClientApi = async (data: { _id: string; isActive: boolean }) => { +export const activateUserClientApi = async (data: { _id: string; isActive: boolean }): Promise<{ success: boolean; message: string }> => {Likely invalid or redundant comment.
netmanager-app/app/(authenticated)/profile/components/EditClientForm.tsx (1)
98-107: Improve error handling implementation.netmanager-app/app/(authenticated)/profile/components/ApiTokens.tsx (3)
1-18: LGTM! Clean and well-organized imports.The imports are logically grouped and all dependencies seem necessary for the component's functionality.
284-284: Implement the missing pagination component.The comment indicates a missing pagination component. This issue was previously identified and a solution was proposed in the past review comments.
219-256: 🛠️ Refactor suggestionEnhance token security with proper masking and clipboard handling.
The token display and clipboard functionality could be improved for better security.
<TableCell> {getClientToken(client._id) ? ( <div className="flex items-center"> <span className="mr-2 font-mono"> {getClientToken(client._id).slice(0, 4)} <span className="mx-1">•••••••</span> {getClientToken(client._id).slice(-4)} </span> <Button title="Copy full token" variant="ghost" size="sm" onClick={() => { - navigator.clipboard.writeText(getClientToken(client._id)) + const token = getClientToken(client._id) + if (token) { + navigator.clipboard.writeText(token) + .then(() => { + toast({ + title: "Success", + description: "API token copied to clipboard", + variant: "success", + }) + }) + .catch(() => { + toast({ + title: "Error", + description: "Failed to copy token to clipboard", + variant: "destructive", + }) + }) + } - toast({ - title: "Success", - description: "API token copied to clipboard", - variant: "success", - }) }} > <Copy className="h-4 w-4" />Likely invalid or redundant comment.
| @@ -12,6 +13,7 @@ export const store = configureStore({ | |||
| devices: devicesReducer, | |||
| grids: gridsReducer, | |||
| cohorts: cohortsReducer, | |||
| clients: clientsRudcer, | |||
There was a problem hiding this comment.
Update the reducer name to match the import
Once the import is fixed, update the reducer configuration to use the correct name.
- 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, |
| @@ -4,6 +4,7 @@ import sitesReducer from "./slices/sitesSlice"; | |||
| 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.
💡 Codebase verification
Fix import statement to match source file naming
The import statement has incorrect naming. The source file exports the reducer as clientSlice.reducer (singular form).
-import clientsRudcer from "./slices/clientsSlice";
+import clientReducer from "./slices/clientsSlice";🔗 Analysis chain
Fix typo in import name: clientsRudcer → clientsReducer
There's a typo in the import name that should be corrected for better code maintainability and clarity.
-import clientsRudcer from "./slices/clientsSlice";
+import clientsReducer from "./slices/clientsSlice";Let's verify if this typo exists in the source file as well:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the typo exists in the source file
rg -l "clientsRudcer" "netmanager-app/core/redux/slices/clientsSlice.ts"
Length of output: 70
Script:
#!/bin/bash
# Find the file and inspect its content
fd "clientsSlice.ts" --type f
echo "=== File content ==="
cat netmanager-app/core/redux/slices/clientsSlice.ts 2>/dev/null || echo "File not found"
Length of output: 1522
| const data = { | ||
| name: clientName, | ||
| user_id: userInfo?._id, | ||
| ip_addresses: ipAddresses.filter((ip) => ip.trim() !== ''), | ||
| } |
There was a problem hiding this comment.
Add IP address format validation.
The code filters empty IP addresses but doesn't validate the format.
+const isValidIp = (ip: string): boolean => {
+ const ipv4Regex = /^(\d{1,3}\.){3}\d{1,3}$/;
+ if (!ipv4Regex.test(ip)) return false;
+ return ip.split('.').every(num => parseInt(num) >= 0 && parseInt(num) <= 255);
+}
const data = {
name: clientName,
user_id: userInfo?._id,
- ip_addresses: ipAddresses.filter((ip) => ip.trim() !== ''),
+ ip_addresses: ipAddresses.filter((ip) => {
+ const trimmedIp = ip.trim();
+ return trimmedIp !== '' && isValidIp(trimmedIp);
+ }),
}📝 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 = { | |
| name: clientName, | |
| user_id: userInfo?._id, | |
| ip_addresses: ipAddresses.filter((ip) => ip.trim() !== ''), | |
| } | |
| const isValidIp = (ip: string): boolean => { | |
| const ipv4Regex = /^(\d{1,3}\.){3}\d{1,3}$/; | |
| if (!ipv4Regex.test(ip)) return false; | |
| return ip.split('.').every(num => parseInt(num) >= 0 && parseInt(num) <= 255); | |
| } | |
| const data = { | |
| name: clientName, | |
| user_id: userInfo?._id, | |
| ip_addresses: ipAddresses.filter((ip) => { | |
| const trimmedIp = ip.trim(); | |
| return trimmedIp !== '' && isValidIp(trimmedIp); | |
| }), | |
| } |
There was a problem hiding this comment.
Hi @danielmarv ....review this please
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| } catch (error: any) { | ||
| toast({ | ||
| title: "Error", | ||
| description: error.message || "Failed to create client", | ||
| variant: "destructive", | ||
| }) | ||
| } finally { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add type safety to error handling.
The error catch block uses any type which reduces type safety.
- } catch (error: any) {
+ } catch (error) {
+ const errorMessage = error instanceof Error
+ ? error.message
+ : "Failed to create client"
toast({
title: "Error",
- description: error.message || "Failed to create client",
+ description: errorMessage,
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.
| } catch (error: any) { | |
| toast({ | |
| title: "Error", | |
| description: error.message || "Failed to create client", | |
| variant: "destructive", | |
| }) | |
| } finally { | |
| } catch (error) { | |
| const errorMessage = error instanceof Error | |
| ? error.message | |
| : "Failed to create client" | |
| toast({ | |
| title: "Error", | |
| description: errorMessage, | |
| variant: "destructive", | |
| }) | |
| } finally { |
| const res = await users.getUserDetails(userInfo?._id || "") | ||
| const resp = await getUserClientsApi(userInfo?._id || "") | ||
| dispatch(addClients(res.users[0].clients)) | ||
| dispatch(addClientsDetails(resp.clients)) | ||
| dispatch(performRefresh()) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve API call sequence handling.
Multiple sequential API calls without individual error handling could lead to inconsistent state.
- const res = await users.getUserDetails(userInfo?._id || "")
- const resp = await getUserClientsApi(userInfo?._id || "")
- dispatch(addClients(res.users[0].clients))
- dispatch(addClientsDetails(resp.clients))
- dispatch(performRefresh())
+ try {
+ const [userDetails, userClients] = await Promise.all([
+ users.getUserDetails(userInfo?._id || ""),
+ getUserClientsApi(userInfo?._id || "")
+ ]);
+ dispatch(addClients(userDetails.users[0].clients));
+ dispatch(addClientsDetails(userClients.clients));
+ dispatch(performRefresh());
+ } catch (error) {
+ console.error('Failed to refresh data:', error);
+ // Consider showing a warning toast that data might be stale
+ }📝 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 res = await users.getUserDetails(userInfo?._id || "") | |
| const resp = await getUserClientsApi(userInfo?._id || "") | |
| dispatch(addClients(res.users[0].clients)) | |
| dispatch(addClientsDetails(resp.clients)) | |
| dispatch(performRefresh()) | |
| try { | |
| const [userDetails, userClients] = await Promise.all([ | |
| users.getUserDetails(userInfo?._id || ""), | |
| getUserClientsApi(userInfo?._id || "") | |
| ]); | |
| dispatch(addClients(userDetails.users[0].clients)); | |
| dispatch(addClientsDetails(userClients.clients)); | |
| dispatch(performRefresh()); | |
| } catch (error) { | |
| console.error('Failed to refresh data:', error); | |
| // Consider showing a warning toast that data might be stale | |
| } |
| const handleGenerateToken = async (res: Client) => { | ||
| setIsLoadingToken(true) | ||
| if (!res?.isActive) { | ||
| setShowInfoModal(true) | ||
| setIsLoadingToken(false) | ||
| } else { | ||
| try { | ||
| const response = await generateTokenApi(res) | ||
| if (response) { | ||
| toast({ | ||
| title: "Success", | ||
| description: "Token generated successfully", | ||
| variant: "success", | ||
| }) | ||
| } | ||
| dispatch(performRefresh()) | ||
| } catch (error: any) { | ||
| toast({ | ||
| title: "Error", | ||
| description: error.message || "Failed to generate token", | ||
| variant: "destructive", | ||
| }) | ||
| } finally { | ||
| setIsLoadingToken(false) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve token generation error handling and add rate limiting.
The token generation could benefit from additional security measures.
const handleGenerateToken = async (res: Client) => {
setIsLoadingToken(true)
+ const rateLimitKey = `token_gen_${res._id}`
+ const lastAttempt = localStorage.getItem(rateLimitKey)
+
+ if (lastAttempt && Date.now() - Number(lastAttempt) < 60000) {
+ toast({
+ title: "Error",
+ description: "Please wait 1 minute before generating another token",
+ variant: "destructive",
+ })
+ setIsLoadingToken(false)
+ return
+ }
+
if (!res?.isActive) {
setShowInfoModal(true)
setIsLoadingToken(false)
} else {
try {
const response = await generateTokenApi(res)
if (response) {
+ localStorage.setItem(rateLimitKey, Date.now().toString())
toast({
title: "Success",
description: "Token generated successfully",
variant: "success",
})
}📝 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 handleGenerateToken = async (res: Client) => { | |
| setIsLoadingToken(true) | |
| if (!res?.isActive) { | |
| setShowInfoModal(true) | |
| setIsLoadingToken(false) | |
| } else { | |
| try { | |
| const response = await generateTokenApi(res) | |
| if (response) { | |
| toast({ | |
| title: "Success", | |
| description: "Token generated successfully", | |
| variant: "success", | |
| }) | |
| } | |
| dispatch(performRefresh()) | |
| } catch (error: any) { | |
| toast({ | |
| title: "Error", | |
| description: error.message || "Failed to generate token", | |
| variant: "destructive", | |
| }) | |
| } finally { | |
| setIsLoadingToken(false) | |
| } | |
| } | |
| } | |
| const handleGenerateToken = async (res: Client) => { | |
| setIsLoadingToken(true) | |
| const rateLimitKey = `token_gen_${res._id}` | |
| const lastAttempt = localStorage.getItem(rateLimitKey) | |
| if (lastAttempt && Date.now() - Number(lastAttempt) < 60000) { | |
| toast({ | |
| title: "Error", | |
| description: "Please wait 1 minute before generating another token", | |
| variant: "destructive", | |
| }) | |
| setIsLoadingToken(false) | |
| return | |
| } | |
| if (!res?.isActive) { | |
| setShowInfoModal(true) | |
| setIsLoadingToken(false) | |
| } else { | |
| try { | |
| const response = await generateTokenApi(res) | |
| if (response) { | |
| localStorage.setItem(rateLimitKey, Date.now().toString()) | |
| toast({ | |
| title: "Success", | |
| description: "Token generated successfully", | |
| variant: "success", | |
| }) | |
| } | |
| dispatch(performRefresh()) | |
| } catch (error: any) { | |
| toast({ | |
| title: "Error", | |
| description: error.message || "Failed to generate token", | |
| variant: "destructive", | |
| }) | |
| } finally { | |
| setIsLoadingToken(false) | |
| } | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
netmanager-app/app/(authenticated)/profile/components/Clients.tsx (2)
24-37: Enhance date formatting utility for better safety and efficiency.The
formatDatefunction has a few areas for improvement:
- Replace unsafe
isNaNwithNumber.isNaN- Use more efficient date comparisons
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 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)
41-49: Consider consolidating related state variables.The component uses multiple independent state variables that could be combined into a single state object for better maintainability.
- const [clients, setClients] = useState<Client[]>([]) - const [loading, setLoading] = useState(false) - const [selectedClient, setSelectedClient] = useState<Client | null>(null) - const [activateDialogOpen, setActivateDialogOpen] = useState(false) - const [deactivateDialogOpen, setDeactivateDialogOpen] = useState(false) - const [searchQuery, setSearchQuery] = useState("") - const [currentPage, setCurrentPage] = useState(1) - const [sortField, setSortField] = useState<keyof Client>("name") - const [sortOrder, setSortOrder] = useState<"asc" | "desc">("asc") + const [state, setState] = useState({ + clients: [] as Client[], + loading: false, + selectedClient: null as Client | null, + dialogs: { + activate: false, + deactivate: false + }, + search: { + query: "", + currentPage: 1, + sortField: "name" as keyof Client, + sortOrder: "asc" as "asc" | "desc" + } + })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
netmanager-app/app/(authenticated)/profile/components/Clients.tsx(1 hunks)netmanager-app/app/(authenticated)/profile/components/ProfileTabs.tsx(1 hunks)netmanager-app/app/(authenticated)/profile/page.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- netmanager-app/app/(authenticated)/profile/page.tsx
- netmanager-app/app/(authenticated)/profile/components/ProfileTabs.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
netmanager-app/app/(authenticated)/profile/components/Clients.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)
| <TableCell className="text-right"> | ||
| <Button | ||
| variant="ghost" | ||
| onClick={() => (client.isActive ? handleDeactivateClick(client) : handleActivateClick(client))} | ||
| > | ||
| {client.isActive ? "Deactivate" : "Activate"} | ||
| </Button> | ||
| <Button variant="ghost" onClick={() => handleCopyClientId(client._id)}> | ||
| Copy ID | ||
| </Button> | ||
| </TableCell> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve button spacing and accessibility in actions column.
Addressing the PR comment about button spacing and enhancing accessibility:
<TableCell className="text-right">
+ <div className="space-x-2 flex justify-end">
<Button
variant="ghost"
+ aria-label={`${client.isActive ? "Deactivate" : "Activate"} ${client.name}`}
onClick={() => (client.isActive ? handleDeactivateClick(client) : handleActivateClick(client))}
>
{client.isActive ? "Deactivate" : "Activate"}
</Button>
<Button
variant="ghost"
+ aria-label={`Copy ID for ${client.name}`}
onClick={() => handleCopyClientId(client._id)}
>
Copy ID
</Button>
+ </div>
</TableCell>📝 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.
| <TableCell className="text-right"> | |
| <Button | |
| variant="ghost" | |
| onClick={() => (client.isActive ? handleDeactivateClick(client) : handleActivateClick(client))} | |
| > | |
| {client.isActive ? "Deactivate" : "Activate"} | |
| </Button> | |
| <Button variant="ghost" onClick={() => handleCopyClientId(client._id)}> | |
| Copy ID | |
| </Button> | |
| </TableCell> | |
| <TableCell className="text-right"> | |
| <div className="space-x-2 flex justify-end"> | |
| <Button | |
| variant="ghost" | |
| aria-label={`${client.isActive ? "Deactivate" : "Activate"} ${client.name}`} | |
| onClick={() => (client.isActive ? handleDeactivateClick(client) : handleActivateClick(client))} | |
| > | |
| {client.isActive ? "Deactivate" : "Activate"} | |
| </Button> | |
| <Button | |
| variant="ghost" | |
| aria-label={`Copy ID for ${client.name}`} | |
| onClick={() => handleCopyClientId(client._id)} | |
| > | |
| Copy ID | |
| </Button> | |
| </div> | |
| </TableCell> |
| const filteredClients = clients.filter( | ||
| (client) => | ||
| client.name.toLowerCase().includes(searchQuery.toLowerCase()) || | ||
| client.user.email.toLowerCase().includes(searchQuery.toLowerCase()), | ||
| ) | ||
|
|
||
| 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 | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize data processing with memoization.
The filtering and sorting operations are recalculated on every render. Consider using useMemo to cache these results.
+ const filteredClients = useMemo(() =>
- const filteredClients = clients.filter(
(client) =>
client.name.toLowerCase().includes(searchQuery.toLowerCase()) ||
client.user.email.toLowerCase().includes(searchQuery.toLowerCase()),
- )
+ ), [clients, searchQuery])
+ const sortedClients = useMemo(() =>
- const sortedClients = [...filteredClients].sort((a, b) => {
+ [...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
- })
+ }), [filteredClients, sortField, sortOrder])📝 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 filteredClients = clients.filter( | |
| (client) => | |
| client.name.toLowerCase().includes(searchQuery.toLowerCase()) || | |
| client.user.email.toLowerCase().includes(searchQuery.toLowerCase()), | |
| ) | |
| 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 filteredClients = useMemo(() => | |
| clients.filter( | |
| (client) => | |
| client.name.toLowerCase().includes(searchQuery.toLowerCase()) || | |
| client.user.email.toLowerCase().includes(searchQuery.toLowerCase()), | |
| ), [clients, searchQuery]) | |
| const sortedClients = useMemo(() => | |
| [...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 | |
| }), [filteredClients, sortField, sortOrder]) |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
netmanager-app/app/(authenticated)/profile/components/ProfileTabs.tsx (2)
11-16: Add error handling for permission check.Consider adding error handling for the permission check to gracefully handle any potential issues:
export default function ProfileTabs() { const [activeTab, setActiveTab] = useState("profile") - const { hasPermission } = usePermissions() + const { hasPermission, error } = usePermissions() + + if (error) { + return <div>Error loading permissions: {error.message}</div> + } const canManageClients = hasPermission("CREATE_UPDATE_AND_DELETE_NETWORK_USERS")
17-41: Add loading states for better UX.Consider adding loading states to improve user experience while content is being fetched:
export default function ProfileTabs() { const [activeTab, setActiveTab] = useState("profile") - const { hasPermission } = usePermissions() + const { hasPermission, isLoading } = usePermissions() + + if (isLoading) { + return <div>Loading profile settings...</div> + } const canManageClients = hasPermission("CREATE_UPDATE_AND_DELETE_NETWORK_USERS") return ( <Tabs value={activeTab} onValueChange={setActiveTab} className="w-full"> <TabsList className={`grid w-full ${canManageClients ? "grid-cols-4" : "grid-cols-3"}`}> <TabsTrigger value="profile" disabled={isLoading}>My Profile</TabsTrigger> <TabsTrigger value="password" disabled={isLoading}>Password Edit</TabsTrigger> <TabsTrigger value="api" disabled={isLoading}>API Access Tokens</TabsTrigger> {canManageClients && <TabsTrigger value="clients" disabled={isLoading}>Clients</TabsTrigger>}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
netmanager-app/app/(authenticated)/profile/components/ProfileTabs.tsx(1 hunks)
🔇 Additional comments (2)
netmanager-app/app/(authenticated)/profile/components/ProfileTabs.tsx (2)
1-10: LGTM! Well-organized imports and proper Next.js setup.The imports are logically grouped and the "use client" directive is correctly placed.
17-41: Previous accessibility improvements are still relevant.The previous review's suggestions for error boundaries and accessibility improvements haven't been addressed yet.
| <TabsContent value="profile"> | ||
| <MyProfile /> | ||
| </TabsContent> | ||
| <TabsContent value="password"> | ||
| <PasswordEdit /> | ||
| </TabsContent> | ||
| <TabsContent value="api"> | ||
| <ApiTokens /> | ||
| </TabsContent> |
There was a problem hiding this comment.
💡 Codebase verification
Implementation gaps found in profile management components
The profile management features are indeed non-functional:
- MyProfile component has a form submission that only toggles editing state without actual profile updates
- ApiTokens component appears to be missing token creation implementation entirely
🔗 Analysis chain
Verify functionality of profile management features.
Based on PR comments, there are issues with update/delete buttons and token creation. Let's verify the implementation in the child components:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check implementation of profile management features
# Check MyProfile component for update/delete functionality
echo "Checking MyProfile component..."
rg -A 10 "function.*update.*profile|function.*delete.*profile" "netmanager-app/app/(authenticated)/profile/components/MyProfile.tsx"
# Check ApiTokens component for token creation
echo "Checking ApiTokens component..."
rg -A 10 "function.*create.*token" "netmanager-app/app/(authenticated)/profile/components/ApiTokens.tsx"
Length of output: 387
Script:
#!/bin/bash
# Broader search for profile management implementations
echo "Checking MyProfile component..."
rg -A 10 "onSubmit|handleSubmit|mutation|updateProfile|deleteProfile" "netmanager-app/app/(authenticated)/profile/components/MyProfile.tsx"
echo "Checking ApiTokens component..."
rg -A 10 "onSubmit|handleSubmit|mutation|createToken|addToken" "netmanager-app/app/(authenticated)/profile/components/ApiTokens.tsx"
# Check for API endpoints usage
echo "Checking API endpoints usage..."
rg -A 5 "api/profile|api/token" "netmanager-app/app/(authenticated)/profile/components/"
# Check for form components
echo "Checking form implementations..."
ast-grep --pattern 'form onSubmit={$_}' "netmanager-app/app/(authenticated)/profile/components/"
Length of output: 1666
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
netmanager-app/app/(authenticated)/profile/components/PasswordEdit.tsx (4)
17-21: Consider ephemeral handling of password fields.
Storing passwords in state potentially keeps them in memory for longer than necessary. If feasible, implement ephemeral handling or secure input fields that don't persist password data extensively in state.
68-75: Refine password strength logic.
Currently, the strength calculation only checks length, uppercase, lowercase, and digits. It might be beneficial to integrate the same criteria used inpasswordRegex(including special characters) to keep the strength meter consistent with final validations.
77-86: Potential missing 2FA prompt or extra check.
When changing a password, consider adding a 2FA prompt or extra confirmation step for enhanced security. This ensures only authenticated users can proceed with password changes.
200-203: Align password strength message with complexity requirements.
It may be clearer to display a short descriptive message (e.g., "Weak", "Moderate", "Strong") aligning with numeric progress. This provides better usability than a raw percentage.netmanager-app/core/apis/settings.ts (2)
39-45: Return type mismatch forupdateUserPasswordApi.
This API returns a response shape with a success message (or an error string). UsingPromise<PasswordData>could be misleading if the server actually returns a different structure. Consider defining and returning a more explicit response type.
65-69: Add error handling foractivateUserClientApi.
Enhance type safety and handle potential HTTP errors for robust production scenarios. Logging or throwing within a.catch()block can prevent silent failures when the request fails.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
netmanager-app/app/(authenticated)/profile/components/PasswordEdit.tsx(1 hunks)netmanager-app/core/apis/settings.ts(1 hunks)
🔇 Additional comments (2)
netmanager-app/app/(authenticated)/profile/components/PasswordEdit.tsx (1)
37-41: Prevent overshadowing validations.
When updatingpasswordsstate, ensure the subsequentvalidateFieldcall doesn't overwrite or reset prior user error messages. For instance, consider using functional updates or merging partial validations to preserve earlier checks.netmanager-app/core/apis/settings.ts (1)
22-32: Confirm existence of user ID route.
Ensure theGETroute (${USERS_MGT_URL}/clients?user_id=${userID}) is valid. An unexpected route or mismatch can lead to silent failures or 404 errors.
netmanager-app/core/apis/settings.ts
Outdated
| export const getClientsApi = async () => { | ||
| return axiosInstance.get(`${USERS_MGT_URL}/clients`).then((response) => response.data); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Incorporate explicit return type for getClientsApi.
Currently, the function returns a promise of unknown shape. Specifying an interface (e.g., Promise<Client[]>) or a properly typed object helps avoid confusion and reduces runtime errors.
…dd Radix UI progress component for enhanced loading indicators
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
netmanager-app/components/ui/progress.tsx (1)
12-25: Consider adding bounds checking for the value prop.While the implementation is solid, we could make the transform calculation more robust by ensuring the value stays within valid bounds.
Consider this enhancement:
- style={{ transform: `translateX(-${100 - (value || 0)}%)` }} + style={{ transform: `translateX(-${100 - Math.min(Math.max(value || 0, 0), 100)}%)` }}This ensures the value stays between 0 and 100, preventing potential visual glitches.
netmanager-app/core/apis/settings.ts (1)
15-18: Consider adding password validation constraints.The
PasswordDatainterface could benefit from JSDoc comments specifying password requirements (e.g., minimum length, required characters).interface PasswordData { + /** Password must be at least 8 characters long and include uppercase, lowercase, number, and special character */ password: string; + /** Current password for verification */ old_password: string; }
📜 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 (3)
netmanager-app/components/ui/progress.tsx(1 hunks)netmanager-app/core/apis/settings.ts(1 hunks)netmanager-app/package.json(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- netmanager-app/package.json
🔇 Additional comments (6)
netmanager-app/components/ui/progress.tsx (4)
1-7: LGTM! Clean and well-organized imports.The imports are properly structured with the "use client" directive correctly placed at the top, followed by external and internal dependencies.
8-11: Well-typed component definition!Excellent use of TypeScript with proper type definitions for both the ref and component props using Radix UI's types.
26-28: LGTM! Proper component naming and export.The displayName is correctly set for better debugging experience, and the export is clean and well-structured.
1-28: Verify integration with profile settings page.Since this Progress component is part of the Profile Settings Page PR, let's verify its integration and usage.
✅ Verification successful
Integration Verified:
The Progress component is correctly integrated within the Profile Settings Page in the file:
- netmanager-app/app/(authenticated)/profile/components/PasswordEdit.tsx
This confirms that the component is being utilized as expected for profile settings.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for Progress component usage in profile-related files rg -l "Progress" netmanager-app/app/\(authenticated\)/profile/Length of output: 133
netmanager-app/core/apis/settings.ts (2)
63-65: Add return type and error handling togetClientsApi.The function is missing a return type annotation and error handling.
67-71: Add return type and error handling toactivateUserClientApi.The function is missing a return type annotation and error handling.
netmanager-app/core/apis/settings.ts
Outdated
| export const createClientApi = async (data: CreateClientData): Promise<Client> => { | ||
| return await axiosInstance.post<CreateClientData, AxiosResponse<Client>>(`${USERS_MGT_URL}/clients`, data) | ||
| .then((response) => response.data); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add consistent error handling across all API functions.
Several API functions lack error handling. Follow the pattern used in getUserClientsApi for consistency.
Example implementation:
export const createClientApi = async (data: CreateClientData): Promise<Client> => {
+ try {
return await axiosInstance.post<CreateClientData, AxiosResponse<Client>>(`${USERS_MGT_URL}/clients`, data)
.then((response) => response.data);
+ } catch (error) {
+ console.error("Error creating client:", error);
+ throw error;
+ }
};Apply similar error handling to updateUserPasswordApi, updateClientApi, generateTokenApi, and activationRequestApi.
Also applies to: 39-47, 50-56, 58-61, 73-76
netmanager-app/core/apis/settings.ts
Outdated
| export const updateUserDetailsApi = async (data: any, userID: string): Promise<UserDetails> => { | ||
| return await axiosInstance.put(`${USERS_MGT_URL}/${userID}`, data) | ||
| .then((response) => response.data); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace any type with a proper interface.
Using any type reduces type safety. Define a proper interface for the data parameter.
+interface UserUpdateData {
+ name?: string;
+ email?: string;
+ phone?: string;
+ // Add other relevant fields
+}
-export const updateUserDetailsApi = async (data: any, userID: string): Promise<UserDetails> => {
+export const updateUserDetailsApi = async (data: UserUpdateData, userID: string): Promise<UserDetails> => {
+ try {
return await axiosInstance.put(`${USERS_MGT_URL}/${userID}`, data)
.then((response) => response.data);
+ } catch (error) {
+ console.error("Error updating user details:", error);
+ throw 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.
| export const updateUserDetailsApi = async (data: any, userID: string): Promise<UserDetails> => { | |
| return await axiosInstance.put(`${USERS_MGT_URL}/${userID}`, data) | |
| .then((response) => response.data); | |
| } | |
| interface UserUpdateData { | |
| name?: string; | |
| email?: string; | |
| phone?: string; | |
| // Add other relevant fields | |
| } | |
| export const updateUserDetailsApi = async (data: UserUpdateData, userID: string): Promise<UserDetails> => { | |
| try { | |
| return await axiosInstance.put(`${USERS_MGT_URL}/${userID}`, data) | |
| .then((response) => response.data); | |
| } catch (error) { | |
| console.error("Error updating user details:", error); | |
| throw error; | |
| } | |
| } |
There was a problem hiding this comment.
Thanks @danielmarv ,
Thanks a lot, this is marvelous! :)
please take off sometime to review the code review comments by Code Rabbit tool. Just seen some pretty interesting suggestions.
| )} | ||
| </TableBody> | ||
| </Table> | ||
| {/* Add your Pagination component here */} |
There was a problem hiding this comment.
Hi @danielmarv , I hope you have been able to review this suggestion?
| const displayIPAddresses = (client) => { | ||
| return Array.isArray(client.ip_addresses) ? client.ip_addresses.join(", ") : client.ip_addresses | ||
| } |
There was a problem hiding this comment.
@danielmarv ,please consider this suggestion
| 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.
Hi @danielmarv , please consider this suggestion.
| const data = { | ||
| name: clientName, | ||
| user_id: userInfo?._id, | ||
| ip_addresses: ipAddresses.filter((ip) => ip.trim() !== ''), | ||
| } |
There was a problem hiding this comment.
Hi @danielmarv ....review this please
|
Good evening @Baalmart |
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (7)
netmanager-app/components/Settings/DialogWrapper.tsx (3)
5-14: Well-structured interface with comprehensive prop types!The
DialogWrapperPropsinterface is well-designed with appropriate optional properties. Consider adding JSDoc comments to document the purpose of each prop for better maintainability.interface DialogWrapperProps { + /** Content to be rendered inside the dialog */ children: React.ReactNode + /** Optional icon component to be displayed at the top of the dialog */ ModalIcon?: React.ComponentType<{ className?: string }> + /** Handler for the primary action button */ handleClick: () => void + /** Controls dialog visibility */ open: boolean + /** Handler for dialog close action */ onClose: () => void + /** Custom text for the primary button */ primaryButtonText?: string + /** Loading state for the primary button */ loading?: boolean + /** Optional dialog title */ title?: string }
40-47: Consider adjusting the footer button alignment.The footer uses
sm:justify-startwhich aligns buttons to the left on small screens. Consider usingsm:justify-endfor a more conventional dialog button alignment.- <DialogFooter className="sm:justify-start"> + <DialogFooter className="sm:justify-end">
53-55: Consider standardizing the export pattern.While having both named and default exports works, consider standardizing to just one export style across the codebase. Also, ensure the file ends with a newline character to avoid potential linting issues.
-export default DialogWrapper - +export default DialogWrapper +netmanager-app/components/Settings/ProfileTabs.tsx (1)
17-40: Consider enhancing error handling and accessibility.The tabs implementation could benefit from:
- Error boundaries to gracefully handle child component failures
- Loading states for asynchronous operations
- ARIA attributes for better accessibility
<Tabs value={activeTab} onValueChange={setActiveTab} className="w-full"> - <TabsList className={`grid w-full ${canManageClients ? "grid-cols-4" : "grid-cols-3"}`}> + <TabsList + className={`grid w-full ${canManageClients ? "grid-cols-4" : "grid-cols-3"}`} + aria-label="Profile settings tabs" + > <TabsTrigger value="profile">My Profile</TabsTrigger>netmanager-app/components/Settings/PasswordEdit.tsx (2)
23-23: Consider enhancing password validation readability and security.The password regex pattern could be more maintainable and secure.
Apply this diff to improve readability and add constants for better maintainability:
-const passwordRegex = /^(?=.*[a-z])(?=.*[A-Z])(?=.*\d)(?=.*[@$!%*?&])[A-Za-z\d@$!%*?&]{8,}$/ +const PASSWORD_MIN_LENGTH = 8; +const PASSWORD_SPECIAL_CHARS = '@$!%*?&'; +const passwordRegex = new RegExp( + `^(?=.*[a-z])(?=.*[A-Z])(?=.*\\d)(?=.*[${PASSWORD_SPECIAL_CHARS}])[A-Za-z\\d${PASSWORD_SPECIAL_CHARS}]{${PASSWORD_MIN_LENGTH},}$` +);
107-133: Enhance error handling specificity.The error handling could be more specific to different types of failures (e.g., network issues, validation errors, server errors).
Apply this diff to improve error handling:
try { setIsLoading(true) const response = await updateUserPasswordApi(userId, pwdData) if (response) { setPasswords({ currentPassword: "", newPassword: "", confirmNewPassword: "", }) toast({ title: "Success", description: "Password updated successfully.", }) } else { throw new Error(response.message) } } catch (error) { + const errorMessage = error instanceof Error ? error.message : 'An error occurred' + const isNetworkError = error instanceof TypeError && errorMessage.includes('network') toast({ title: "Error", - description: (error as Error).message || "An error occurred.", + description: isNetworkError + ? "Network error. Please check your connection." + : errorMessage, variant: "destructive", }) }netmanager-app/components/Settings/Clients.tsx (1)
284-285: Add pagination placeholder implementation.The comment indicates missing pagination implementation.
Would you like me to generate a pagination component implementation that integrates with your existing code?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
netmanager-app/app/(authenticated)/profile/page.tsx(1 hunks)netmanager-app/components/Settings/ApiTokens.tsx(1 hunks)netmanager-app/components/Settings/ClientTableRow.tsx(1 hunks)netmanager-app/components/Settings/Clients.tsx(1 hunks)netmanager-app/components/Settings/CreateClientForm.tsx(1 hunks)netmanager-app/components/Settings/DialogWrapper.tsx(1 hunks)netmanager-app/components/Settings/EditClientForm.tsx(1 hunks)netmanager-app/components/Settings/MyProfile.tsx(1 hunks)netmanager-app/components/Settings/PasswordEdit.tsx(1 hunks)netmanager-app/components/Settings/ProfileTabs.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- netmanager-app/app/(authenticated)/profile/page.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
netmanager-app/components/Settings/ApiTokens.tsx
[error] 86-86: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 94-94: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 102-102: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
netmanager-app/components/Settings/Clients.tsx
[error] 77-77: 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] 209-209: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 28-28: 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 (2)
netmanager-app/components/Settings/DialogWrapper.tsx (1)
16-51: Clean and well-structured component implementation!The component follows React best practices with proper prop handling and conditional rendering. The loading state management is particularly well done.
netmanager-app/components/Settings/PasswordEdit.tsx (1)
139-240: Well-implemented accessibility and user feedback!The component demonstrates good practices with ARIA attributes, password visibility toggle, and visual feedback for password strength.
| interface ClientTableRowProps { | ||
| client: any | ||
| onGenerateToken: (client: any) => void |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Define proper TypeScript interface for client prop.
Using any type reduces type safety. Consider defining a proper interface for the client object.
- client: any
+ client: {
+ _id: string;
+ name: string;
+ ip_addresses: string | string[];
+ isActive: boolean;
+ createdAt: 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 ClientTableRowProps { | |
| client: any | |
| onGenerateToken: (client: any) => void | |
| interface ClientTableRowProps { | |
| client: { | |
| _id: string; | |
| name: string; | |
| ip_addresses: string | string[]; | |
| isActive: boolean; | |
| createdAt: string; | |
| } | |
| onGenerateToken: (client: any) => void | |
| } |
| <div | ||
| className="w-6 h-6 bg-white rounded border border-gray-200 flex justify-center items-center gap-2 cursor-pointer" | ||
| onClick={() => onCopyToken(getClientToken(client._id))} | ||
| > | ||
| <CopyIcon /> | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance accessibility for the copy button.
The copy button lacks proper accessibility attributes.
<div
className="w-6 h-6 bg-white rounded border border-gray-200 flex justify-center items-center gap-2 cursor-pointer"
+ role="button"
+ aria-label="Copy token"
onClick={() => onCopyToken(getClientToken(client._id))}
>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <div | |
| className="w-6 h-6 bg-white rounded border border-gray-200 flex justify-center items-center gap-2 cursor-pointer" | |
| onClick={() => onCopyToken(getClientToken(client._id))} | |
| > | |
| <CopyIcon /> | |
| </div> | |
| <div | |
| className="w-6 h-6 bg-white rounded border border-gray-200 flex justify-center items-center gap-2 cursor-pointer" | |
| role="button" | |
| aria-label="Copy token" | |
| onClick={() => onCopyToken(getClientToken(client._id))} | |
| > | |
| <CopyIcon /> | |
| </div> |
| const handleSubmit = async () => { | ||
| setIsLoading(true) | ||
| try { | ||
| if (!clientName) { | ||
| throw new Error("Client name can't be empty") | ||
| } | ||
|
|
||
| const data = { | ||
| name: clientName, | ||
| user_id: userInfo?._id, | ||
| ip_addresses: ipAddresses.filter((ip) => ip.trim() !== ''), | ||
| } | ||
|
|
||
| const response = await createClientApi(data) | ||
| if (response) { | ||
| toast({ | ||
| title: "Success", | ||
| description: "Client created successfully", | ||
| variant: "success", | ||
| }) | ||
| onClientCreated() | ||
| onClose() | ||
| } | ||
| } catch (error: any) { | ||
| toast({ | ||
| title: "Error", | ||
| description: error.message || "Failed to create client", | ||
| variant: "destructive", | ||
| }) | ||
| } finally { | ||
| setIsLoading(false) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add IP address validation and improve error handling.
The form submission lacks proper IP address validation and could benefit from using a form validation library.
+const isValidIpAddress = (ip: string): boolean => {
+ const ipv4Regex = /^(\d{1,3}\.){3}\d{1,3}$/;
+ if (!ipv4Regex.test(ip)) return false;
+ return ip.split('.').every(num => parseInt(num) >= 0 && parseInt(num) <= 255);
+}
const handleSubmit = async () => {
setIsLoading(true)
try {
if (!clientName) {
throw new Error("Client name can't be empty")
}
+ const invalidIps = ipAddresses.filter(ip => ip.trim() !== '' && !isValidIpAddress(ip));
+ if (invalidIps.length > 0) {
+ throw new Error("Invalid IP address format detected");
+ }📝 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 () => { | |
| setIsLoading(true) | |
| try { | |
| if (!clientName) { | |
| throw new Error("Client name can't be empty") | |
| } | |
| const data = { | |
| name: clientName, | |
| user_id: userInfo?._id, | |
| ip_addresses: ipAddresses.filter((ip) => ip.trim() !== ''), | |
| } | |
| const response = await createClientApi(data) | |
| if (response) { | |
| toast({ | |
| title: "Success", | |
| description: "Client created successfully", | |
| variant: "success", | |
| }) | |
| onClientCreated() | |
| onClose() | |
| } | |
| } catch (error: any) { | |
| toast({ | |
| title: "Error", | |
| description: error.message || "Failed to create client", | |
| variant: "destructive", | |
| }) | |
| } finally { | |
| setIsLoading(false) | |
| } | |
| } | |
| const isValidIpAddress = (ip: string): boolean => { | |
| const ipv4Regex = /^(\d{1,3}\.){3}\d{1,3}$/; | |
| if (!ipv4Regex.test(ip)) return false; | |
| return ip.split('.').every(num => parseInt(num) >= 0 && parseInt(num) <= 255); | |
| } | |
| const handleSubmit = async () => { | |
| setIsLoading(true) | |
| try { | |
| if (!clientName) { | |
| throw new Error("Client name can't be empty") | |
| } | |
| const invalidIps = ipAddresses.filter(ip => ip.trim() !== '' && !isValidIpAddress(ip)); | |
| if (invalidIps.length > 0) { | |
| throw new Error("Invalid IP address format detected"); | |
| } | |
| const data = { | |
| name: clientName, | |
| user_id: userInfo?._id, | |
| ip_addresses: ipAddresses.filter((ip) => ip.trim() !== ''), | |
| } | |
| const response = await createClientApi(data) | |
| if (response) { | |
| toast({ | |
| title: "Success", | |
| description: "Client created successfully", | |
| variant: "success", | |
| }) | |
| onClientCreated() | |
| onClose() | |
| } | |
| } catch (error: any) { | |
| toast({ | |
| title: "Error", | |
| description: error.message || "Failed to create client", | |
| variant: "destructive", | |
| }) | |
| } finally { | |
| setIsLoading(false) | |
| } | |
| } |
| const res = await users.getUserDetails(userInfo?._id || "") | ||
| const resp = await getUserClientsApi(userInfo?._id || "") | ||
| dispatch(addClients(res.users[0].clients)) | ||
| dispatch(addClientsDetails(resp.clients)) | ||
| dispatch(performRefresh()) | ||
| closeModal() |
There was a problem hiding this comment.
Optimize Redux dispatches and fix undefined function.
Multiple sequential dispatches could be optimized, and there's an undefined function being called.
- const res = await users.getUserDetails(userInfo?._id || "")
- const resp = await getUserClientsApi(userInfo?._id || "")
- dispatch(addClients(res.users[0].clients))
- dispatch(addClientsDetails(resp.clients))
- dispatch(performRefresh())
- closeModal()
+ const [userDetails, userClients] = await Promise.all([
+ users.getUserDetails(userInfo?._id || ""),
+ getUserClientsApi(userInfo?._id || "")
+ ]);
+ dispatch(addClients(userDetails.users[0].clients));
+ dispatch(addClientsDetails(userClients.clients));
+ dispatch(performRefresh());
+ onClose();📝 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 res = await users.getUserDetails(userInfo?._id || "") | |
| const resp = await getUserClientsApi(userInfo?._id || "") | |
| dispatch(addClients(res.users[0].clients)) | |
| dispatch(addClientsDetails(resp.clients)) | |
| dispatch(performRefresh()) | |
| closeModal() | |
| const [userDetails, userClients] = await Promise.all([ | |
| users.getUserDetails(userInfo?._id || ""), | |
| getUserClientsApi(userInfo?._id || "") | |
| ]); | |
| dispatch(addClients(userDetails.users[0].clients)); | |
| dispatch(addClientsDetails(userClients.clients)); | |
| dispatch(performRefresh()); | |
| onClose(); |
| useEffect(() => { | ||
| if (isError.isError) { | ||
| const timer = setTimeout(() => { | ||
| setIsError({ | ||
| isError: false, | ||
| message: "", | ||
| type: "", | ||
| }) | ||
| }, 2000) | ||
|
|
||
| return () => clearTimeout(timer) | ||
| } | ||
| }, [isError.isError]) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix potential memory leak in error timeout.
The cleanup function for the error timeout should be moved outside the condition.
useEffect(() => {
+ let timer: NodeJS.Timeout;
if (isError.isError) {
- const timer = setTimeout(() => {
+ timer = setTimeout(() => {
setIsError({
isError: false,
message: "",
type: "",
})
}, 2000)
-
- return () => clearTimeout(timer)
}
+ return () => {
+ if (timer) clearTimeout(timer)
+ }
}, [isError.isError])📝 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.
| useEffect(() => { | |
| if (isError.isError) { | |
| const timer = setTimeout(() => { | |
| setIsError({ | |
| isError: false, | |
| message: "", | |
| type: "", | |
| }) | |
| }, 2000) | |
| return () => clearTimeout(timer) | |
| } | |
| }, [isError.isError]) | |
| useEffect(() => { | |
| let timer: NodeJS.Timeout; | |
| if (isError.isError) { | |
| timer = setTimeout(() => { | |
| setIsError({ | |
| isError: false, | |
| message: "", | |
| type: "", | |
| }) | |
| }, 2000) | |
| } | |
| return () => { | |
| if (timer) clearTimeout(timer) | |
| } | |
| }, [isError.isError]) |
| const handleActivateDeactivate = async (clientId: string, activate: boolean) => { | ||
| const data = { | ||
| _id: clientId, | ||
| isActive: activate ? true : false, | ||
| }; | ||
| try { | ||
| await activateUserClientApi(data) | ||
| await fetchClients() | ||
| toast({ | ||
| title: "Success", | ||
| description: `Client ${activate ? "activated" : "deactivated"} successfully`, | ||
| }) | ||
| } catch (error) { | ||
| toast({ | ||
| title: "Error", | ||
| description: `Failed to ${activate ? "activate" : "deactivate"} client`, | ||
| variant: "destructive", | ||
| }) | ||
| } finally { | ||
| setActivateDialogOpen(false) | ||
| setDeactivateDialogOpen(false) | ||
| setSelectedClient(null) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve client activation error handling and user feedback.
The handleActivateDeactivate function could provide more specific feedback and handle edge cases better.
Apply this diff to enhance error handling:
const handleActivateDeactivate = async (clientId: string, activate: boolean) => {
const data = {
_id: clientId,
- isActive: activate ? true : false,
+ isActive: activate,
};
try {
+ if (!clientId) {
+ throw new Error('Client ID is required');
+ }
await activateUserClientApi(data)
await fetchClients()
toast({
title: "Success",
description: `Client ${activate ? "activated" : "deactivated"} successfully`,
})
} catch (error) {
+ const errorMessage = error instanceof Error ? error.message : 'Unknown error occurred';
toast({
title: "Error",
- description: `Failed to ${activate ? "activate" : "deactivate"} client`,
+ description: `Failed to ${activate ? "activate" : "deactivate"} client: ${errorMessage}`,
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.
| const handleActivateDeactivate = async (clientId: string, activate: boolean) => { | |
| const data = { | |
| _id: clientId, | |
| isActive: activate ? true : false, | |
| }; | |
| try { | |
| await activateUserClientApi(data) | |
| await fetchClients() | |
| toast({ | |
| title: "Success", | |
| description: `Client ${activate ? "activated" : "deactivated"} successfully`, | |
| }) | |
| } catch (error) { | |
| toast({ | |
| title: "Error", | |
| description: `Failed to ${activate ? "activate" : "deactivate"} client`, | |
| variant: "destructive", | |
| }) | |
| } finally { | |
| setActivateDialogOpen(false) | |
| setDeactivateDialogOpen(false) | |
| setSelectedClient(null) | |
| } | |
| } | |
| const handleActivateDeactivate = async (clientId: string, activate: boolean) => { | |
| const data = { | |
| _id: clientId, | |
| isActive: activate, | |
| }; | |
| try { | |
| if (!clientId) { | |
| throw new Error('Client ID is required'); | |
| } | |
| await activateUserClientApi(data) | |
| await fetchClients() | |
| toast({ | |
| title: "Success", | |
| description: `Client ${activate ? "activated" : "deactivated"} successfully`, | |
| }) | |
| } catch (error) { | |
| const errorMessage = error instanceof Error ? error.message : 'Unknown error occurred'; | |
| toast({ | |
| title: "Error", | |
| description: `Failed to ${activate ? "activate" : "deactivate"} client: ${errorMessage}`, | |
| variant: "destructive", | |
| }) | |
| } finally { | |
| setActivateDialogOpen(false) | |
| setDeactivateDialogOpen(false) | |
| setSelectedClient(null) | |
| } | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 77-77: 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)
| const handleSubmit = async (e: React.FormEvent) => { | ||
| e.preventDefault() | ||
| if (!profile || !currentUser) return | ||
|
|
||
| setIsLoading(true) | ||
| try { | ||
| await updateUserDetailsApi(profile, currentUser._id) | ||
| const res = await users.getUserDetails(currentUser._id) | ||
| const updatedUser = res.users[0] | ||
|
|
||
| if (!updatedUser) { | ||
| throw new Error("User details not updated") | ||
| } | ||
|
|
||
| dispatch(setUserDetails({ ...currentUser, ...updatedUser })) | ||
| toast({ | ||
| title: "Success", | ||
| description: "User details updated successfully", | ||
| }) | ||
| } catch (error) { | ||
| setError("Failed to update user details") | ||
| toast({ | ||
| title: "Error", | ||
| description: "Failed to update user details", | ||
| }) | ||
| } finally { | ||
| setIsLoading(false) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add retry mechanism for profile updates.
Based on reported issues with profile updates, implement a retry mechanism for better reliability.
Apply this diff to add retry logic:
+const MAX_RETRIES = 3;
+const RETRY_DELAY = 1000;
+const wait = (ms: number) => new Promise(resolve => setTimeout(resolve, ms));
const handleSubmit = async (e: React.FormEvent) => {
e.preventDefault()
if (!profile || !currentUser) return
setIsLoading(true)
+ let retries = 0;
try {
- await updateUserDetailsApi(profile, currentUser._id)
- const res = await users.getUserDetails(currentUser._id)
+ while (retries < MAX_RETRIES) {
+ try {
+ await updateUserDetailsApi(profile, currentUser._id)
+ const res = await users.getUserDetails(currentUser._id)
+ const updatedUser = res.users[0]
+ if (!updatedUser) throw new Error("User details not updated")
+ dispatch(setUserDetails({ ...currentUser, ...updatedUser }))
+ toast({
+ title: "Success",
+ description: "User details updated successfully",
+ })
+ break;
+ } catch (error) {
+ retries++;
+ if (retries === MAX_RETRIES) throw error;
+ await wait(RETRY_DELAY);
+ }
+ }
} catch (error) {
setError("Failed to update user details")
toast({
title: "Error",
- description: "Failed to update user details",
+ description: "Failed to update user details after multiple attempts",
variant: "destructive",
})
} finally {
setIsLoading(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 handleSubmit = async (e: React.FormEvent) => { | |
| e.preventDefault() | |
| if (!profile || !currentUser) return | |
| setIsLoading(true) | |
| try { | |
| await updateUserDetailsApi(profile, currentUser._id) | |
| const res = await users.getUserDetails(currentUser._id) | |
| const updatedUser = res.users[0] | |
| if (!updatedUser) { | |
| throw new Error("User details not updated") | |
| } | |
| dispatch(setUserDetails({ ...currentUser, ...updatedUser })) | |
| toast({ | |
| title: "Success", | |
| description: "User details updated successfully", | |
| }) | |
| } catch (error) { | |
| setError("Failed to update user details") | |
| toast({ | |
| title: "Error", | |
| description: "Failed to update user details", | |
| }) | |
| } finally { | |
| setIsLoading(false) | |
| } | |
| } | |
| const MAX_RETRIES = 3; | |
| const RETRY_DELAY = 1000; | |
| const wait = (ms: number) => new Promise(resolve => setTimeout(resolve, ms)); | |
| const handleSubmit = async (e: React.FormEvent) => { | |
| e.preventDefault() | |
| if (!profile || !currentUser) return | |
| setIsLoading(true) | |
| let retries = 0; | |
| try { | |
| while (retries < MAX_RETRIES) { | |
| try { | |
| await updateUserDetailsApi(profile, currentUser._id) | |
| const res = await users.getUserDetails(currentUser._id) | |
| const updatedUser = res.users[0] | |
| if (!updatedUser) throw new Error("User details not updated") | |
| dispatch(setUserDetails({ ...currentUser, ...updatedUser })) | |
| toast({ | |
| title: "Success", | |
| description: "User details updated successfully", | |
| }) | |
| break; | |
| } catch (error) { | |
| retries++; | |
| if (retries === MAX_RETRIES) throw error; | |
| await wait(RETRY_DELAY); | |
| } | |
| } | |
| } catch (error) { | |
| setError("Failed to update user details") | |
| toast({ | |
| title: "Error", | |
| description: "Failed to update user details after multiple attempts", | |
| variant: "destructive", | |
| }) | |
| } finally { | |
| setIsLoading(false) | |
| } | |
| } |
| const handleGenerateToken = async (res: Client) => { | ||
| setIsLoadingToken(true) | ||
| if (!res?.isActive) { | ||
| setShowInfoModal(true) | ||
| setIsLoadingToken(false) | ||
| } else { | ||
| try { | ||
| const response = await generateTokenApi(res) | ||
| if (response) { | ||
| toast({ | ||
| title: "Success", | ||
| description: "Token generated successfully", | ||
| variant: "success", | ||
| }) | ||
| } | ||
| dispatch(performRefresh()) | ||
| } catch (error: any) { | ||
| toast({ | ||
| title: "Error", | ||
| description: error.message || "Failed to generate token", | ||
| variant: "destructive", | ||
| }) | ||
| } finally { | ||
| setIsLoadingToken(false) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve token generation reliability and feedback.
The token generation flow needs enhancement to address reported issues.
Apply this diff to improve the token generation process:
const handleGenerateToken = async (res: Client) => {
setIsLoadingToken(true)
if (!res?.isActive) {
setShowInfoModal(true)
setIsLoadingToken(false)
+ return;
} else {
try {
+ if (!res?._id) {
+ throw new Error("Invalid client data");
+ }
const response = await generateTokenApi(res)
- if (response) {
+ if (response?.token) {
toast({
title: "Success",
- description: "Token generated successfully",
+ description: "Token generated successfully. Copy it now as it won't be shown again.",
variant: "success",
})
+ await fetchClientDetails(); // Refresh client details to show new token
+ } else {
+ throw new Error("Token generation failed - invalid response");
}
dispatch(performRefresh())
} catch (error: any) {
+ const errorMessage = error?.response?.data?.message || error.message || "Failed to generate token";
toast({
title: "Error",
- description: error.message || "Failed to generate token",
+ description: errorMessage,
variant: "destructive",
})
} finally {
setIsLoadingToken(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 handleGenerateToken = async (res: Client) => { | |
| setIsLoadingToken(true) | |
| if (!res?.isActive) { | |
| setShowInfoModal(true) | |
| setIsLoadingToken(false) | |
| } else { | |
| try { | |
| const response = await generateTokenApi(res) | |
| if (response) { | |
| toast({ | |
| title: "Success", | |
| description: "Token generated successfully", | |
| variant: "success", | |
| }) | |
| } | |
| dispatch(performRefresh()) | |
| } catch (error: any) { | |
| toast({ | |
| title: "Error", | |
| description: error.message || "Failed to generate token", | |
| variant: "destructive", | |
| }) | |
| } finally { | |
| setIsLoadingToken(false) | |
| } | |
| } | |
| } | |
| const handleGenerateToken = async (res: Client) => { | |
| setIsLoadingToken(true) | |
| if (!res?.isActive) { | |
| setShowInfoModal(true) | |
| setIsLoadingToken(false) | |
| return; | |
| } else { | |
| try { | |
| if (!res?._id) { | |
| throw new Error("Invalid client data"); | |
| } | |
| const response = await generateTokenApi(res) | |
| if (response?.token) { | |
| toast({ | |
| title: "Success", | |
| description: "Token generated successfully. Copy it now as it won't be shown again.", | |
| variant: "success", | |
| }) | |
| await fetchClientDetails(); // Refresh client details to show new token | |
| } else { | |
| throw new Error("Token generation failed - invalid response"); | |
| } | |
| dispatch(performRefresh()) | |
| } catch (error: any) { | |
| const errorMessage = error?.response?.data?.message || error.message || "Failed to generate token"; | |
| toast({ | |
| title: "Error", | |
| description: errorMessage, | |
| variant: "destructive", | |
| }) | |
| } finally { | |
| setIsLoadingToken(false) | |
| } | |
| } | |
| } |
… variants for improved clarity
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
netmanager-app/components/Settings/ApiTokens.tsx (4)
19-34: Consider consolidating client state management.The component maintains two separate states for clients (
clientsandclientsDetails), which could lead to synchronization issues. Consider consolidating these into a single state with all required fields.- const [clients, setClients] = useState<Client[]>([]) - const [clientsDetails, setClientsDetails] = useState<Client[]>([]) + const [clients, setClients] = useState<{ + basic: Client[]; + details: Client[]; + }>({ basic: [], details: [] })
81-104: Improve null safety with optional chaining.The client lookup functions could be simplified and made safer with optional chaining.
- return client && client.access_token + return client?.access_token - return client && client.access_token && client.access_token.token + return client?.access_token?.token - return client && client.access_token && client.access_token.expires + return client?.access_token?.expires🧰 Tools
🪛 Biome (1.9.4)
[error] 86-86: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 94-94: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 102-102: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
283-283: Implement the missing pagination component.The comment indicates a missing pagination component. Consider implementing it to improve user experience with large datasets.
Would you like me to help implement a reusable pagination component?
140-146: Optimize toast notification timing.The 3-second delay for activation request notifications seems unnecessarily long and might confuse users.
- setTimeout(() => { toast({ title: "Success", description: "Activation request sent successfully", }) - }, 3000) + }, 500) - setTimeout(() => { toast({ title: "Error", description: error.message || "Failed to send activation request", variant: "destructive", }) - }, 3000) + }, 500)Also applies to: 149-156
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
netmanager-app/components/Settings/ApiTokens.tsx(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
netmanager-app/components/Settings/ApiTokens.tsx
[error] 86-86: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 94-94: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 102-102: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (2)
netmanager-app/components/Settings/ApiTokens.tsx (2)
1-18: LGTM! Well-structured imports and component setup.The imports are well-organized and the component is properly typed.
105-131: Improve token generation reliability and feedback.The token generation flow needs enhancement to address reported issues.
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (4)
netmanager-app/components/Settings/EditClientForm.tsx (2)
68-107:⚠️ Potential issueOptimize API calls and fix undefined function.
The sequential API calls could be optimized using Promise.all, and there's an undefined function being called.
Apply this diff to fix the issues:
- const res = await users.getUserDetails(userInfo?._id || "") - const resp = await settings.getUserClientsApi(userInfo?._id || "") - dispatch(addClients(res.users[0].clients)) - dispatch(addClientsDetails(resp.clients)) - dispatch(performRefresh()) - closeModal() + const [userDetails, userClients] = await Promise.all([ + users.getUserDetails(userInfo?._id || ""), + settings.getUserClientsApi(userInfo?._id || "") + ]); + dispatch(addClients(userDetails.users[0].clients)); + dispatch(addClientsDetails(userClients.clients)); + dispatch(performRefresh()); + onClose();
109-121:⚠️ Potential issueFix potential memory leak in error timeout.
The cleanup function for the error timeout should be moved outside the condition.
Apply this diff to fix the memory leak:
useEffect(() => { + let timer: NodeJS.Timeout; if (isError.isError) { - const timer = setTimeout(() => { + timer = setTimeout(() => { setIsError({ isError: false, message: "", type: "", }) }, 2000) - - return () => clearTimeout(timer) } + return () => { + if (timer) clearTimeout(timer) + } }, [isError.isError])netmanager-app/components/Settings/Clients.tsx (1)
74-97: 🛠️ Refactor suggestionImprove client activation error handling.
The function could be simplified and provide more specific error feedback.
Apply this diff to improve the function:
const handleActivateDeactivate = async (clientId: string, activate: boolean) => { const data = { _id: clientId, - isActive: activate ? true : false, + isActive: activate, }; try { + if (!clientId) { + throw new Error('Client ID is required'); + } await settings.activateUserClientApi(data) await fetchClients() toast({ title: "Success", description: `Client ${activate ? "activated" : "deactivated"} successfully`, }) } catch (error) { + const errorMessage = error instanceof Error ? error.message : 'Unknown error occurred'; toast({ title: "Error", - description: `Failed to ${activate ? "activate" : "deactivate"} client`, + description: `Failed to ${activate ? "activate" : "deactivate"} client: ${errorMessage}`, variant: "destructive", }) } }🧰 Tools
🪛 Biome (1.9.4)
[error] 77-77: 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/ApiTokens.tsx (1)
104-130: 🛠️ Refactor suggestionImprove token generation reliability and feedback.
The token generation flow needs enhancement to address reported issues.
Apply this diff to improve the token generation process:
const handleGenerateToken = async (res: Client) => { setIsLoadingToken(true) if (!res?.isActive) { setShowInfoModal(true) setIsLoadingToken(false) + return; } else { try { + if (!res?._id) { + throw new Error("Invalid client data"); + } const response = await settings.generateTokenApi(res) - if (response) { + if (response?.token) { toast({ title: "Success", - description: "Token generated successfully", + description: "Token generated successfully. Copy it now as it won't be shown again.", variant: "success", }) + await fetchClientDetails(); // Refresh client details to show new token + } else { + throw new Error("Token generation failed - invalid response"); } dispatch(performRefresh()) } catch (error: any) { const errorMessage = error?.response?.data?.message || error.message || "Failed to generate token"; toast({ title: "Error", description: errorMessage, variant: "destructive", }) } finally { setIsLoadingToken(false) } } }
🧹 Nitpick comments (9)
netmanager-app/components/Settings/MyProfile.tsx (3)
21-30: Strengthen the Profile interface.Consider making required fields non-optional and adding validation constraints.
interface Profile { firstName: string lastName: string email: string - jobTitle?: string - country?: string - timezone?: string - description?: string - profilePicture?: string + jobTitle: string | null + country: string | null + timezone: string | null + description: string | null + profilePicture: string | null }
44-77: Enhance error handling in fetchUserData.The error messages are generic and don't provide enough context for debugging.
} catch (error) { + const errorMessage = error instanceof Error ? error.message : "Unknown error occurred" setError("Failed to fetch user data") toast({ title: "Error", - description: "Failed to fetch user data", + description: `Failed to fetch user data: ${errorMessage}`, }) }
132-150: Add confirmation dialog for profile image deletion.The delete operation should require user confirmation to prevent accidental deletions.
Consider using a confirmation dialog before proceeding with deletion.
netmanager-app/components/Settings/PasswordEdit.tsx (5)
23-23: Consider simplifying the password requirements.The current regex pattern might be too complex for users to understand. Consider breaking it down into separate checks for better user experience.
- const passwordRegex = /^(?=.*[a-z])(?=.*[A-Z])(?=.*\d)(?=.*[@$!%*?&])[A-Za-z\d@$!%*?&]{8,}$/ + const passwordRequirements = { + minLength: 8, + hasLowerCase: /[a-z]/, + hasUpperCase: /[A-Z]/, + hasNumber: /\d/, + hasSpecialChar: /[@$!%*?&]/ + }
13-36: Consider extracting password-related logic into a custom hook.The password-related state and validation logic could be moved to a custom hook for better reusability and maintainability.
// usePasswordForm.ts export function usePasswordForm() { const [passwords, setPasswords] = useState({...}) const [errors, setErrors] = useState({...}) const [showPassword, setShowPassword] = useState({...}) // ... validation and handlers return { passwords, errors, showPassword, handleInputChange, validateField, togglePasswordVisibility } }
43-66: Consider using a validation library for more maintainable form validation.While the current validation logic works well, using a form validation library like Zod or Yup could make the code more maintainable and provide better type safety.
import { z } from 'zod' const passwordSchema = z.object({ currentPassword: z.string().min(1, "Current password is required"), newPassword: z.string() .min(8, "Password must be at least 8 characters") .regex(/[A-Z]/, "Must contain uppercase") .regex(/[a-z]/, "Must contain lowercase") .regex(/[0-9]/, "Must contain number") .regex(/[@$!%*?&]/, "Must contain special character"), confirmNewPassword: z.string() }).refine((data) => data.newPassword === data.confirmNewPassword, { message: "Passwords do not match", path: ["confirmNewPassword"] })
68-75: Consider using a more comprehensive password strength calculator.The current strength calculation is basic. Consider using a library like
zxcvbnfor more accurate password strength measurement.import zxcvbn from 'zxcvbn' const calculatePasswordStrength = (password: string) => { if (!password) return 0 const result = zxcvbn(password) return (result.score / 4) * 100 }
139-240: LGTM! Well-structured UI with good accessibility.The form implementation includes proper ARIA attributes and error handling. The password strength indicator provides good visual feedback.
Consider adding
aria-live="polite"to the password strength indicator for better screen reader support:<p className="text-sm text-muted-foreground" + aria-live="polite" > Password strength: {calculatePasswordStrength(passwords.newPassword)}% </p>netmanager-app/components/Settings/Clients.tsx (1)
118-122: Simplify email filtering logic with optional chaining.The email filtering logic can be made more concise.
const filteredClients = clients.filter( (client) => client.name.toLowerCase().includes(searchQuery.toLowerCase()) || - (client.user?.email?.toLowerCase().includes(searchQuery.toLowerCase()) ?? false) + client.user?.email?.toLowerCase()?.includes(searchQuery.toLowerCase()) || false )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
netmanager-app/components/Settings/ApiTokens.tsx(1 hunks)netmanager-app/components/Settings/Clients.tsx(1 hunks)netmanager-app/components/Settings/CreateClientForm.tsx(1 hunks)netmanager-app/components/Settings/EditClientForm.tsx(1 hunks)netmanager-app/components/Settings/MyProfile.tsx(1 hunks)netmanager-app/components/Settings/PasswordEdit.tsx(1 hunks)netmanager-app/core/apis/settings.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- netmanager-app/components/Settings/CreateClientForm.tsx
- netmanager-app/core/apis/settings.ts
🧰 Additional context used
🪛 Biome (1.9.4)
netmanager-app/components/Settings/Clients.tsx
[error] 77-77: 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] 209-209: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 28-28: 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/components/Settings/ApiTokens.tsx
[error] 85-85: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 93-93: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 101-101: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (9)
netmanager-app/components/Settings/MyProfile.tsx (2)
152-180: Add retry mechanism for profile updates.Based on reported issues with profile updates in the PR comments, implement a retry mechanism.
203-313: Well-structured UI implementation!The UI implementation is clean, responsive, and handles loading/error states appropriately.
netmanager-app/components/Settings/PasswordEdit.tsx (2)
1-12: LGTM! Clean imports and proper Next.js setup.The imports are well-organized and the "use client" directive is correctly placed.
135-137: LGTM! Clean implementation of password visibility toggle.The toggle functionality is well-implemented with proper typing.
netmanager-app/components/Settings/EditClientForm.tsx (2)
30-38: LGTM! Well-structured initialization logic.The function handles both array and non-array IP addresses gracefully, with proper fallback values.
44-54: LGTM! Clean input handling.The function effectively manages both client name and IP address changes with proper type checking.
netmanager-app/components/Settings/Clients.tsx (1)
74-97: 🛠️ Refactor suggestionEnhance error handling and simplify boolean assignment.
The function can be improved in two ways:
- Simplify the boolean assignment
- Add more specific error handling
const handleActivateDeactivate = async (clientId: string, activate: boolean) => { const data = { _id: clientId, - isActive: activate ? true : false, + isActive: activate, }; try { + if (!clientId) { + throw new Error('Client ID is required'); + } await settings.activateUserClientApi(data) await fetchClients() toast({ title: "Success", description: `Client ${activate ? "activated" : "deactivated"} successfully`, }) } catch (error) { + const errorMessage = error instanceof Error ? error.message : 'Unknown error occurred'; toast({ title: "Error", - description: `Failed to ${activate ? "activate" : "deactivate"} client`, + description: `Failed to ${activate ? "activate" : "deactivate"} client: ${errorMessage}`, variant: "destructive", }) } }Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 77-77: 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/ApiTokens.tsx (2)
282-282: Implement the missing pagination component.The code includes pagination logic but the UI component is not implemented. Consider using the same pagination component as in the Clients.tsx file for consistency.
Would you like me to help implement the pagination component similar to the one used in Clients.tsx?
104-130:⚠️ Potential issueEnhance token generation reliability and user feedback.
The token generation flow needs improvement to address reported issues:
- Response validation is insufficient
- Users should be notified to copy the token immediately
const handleGenerateToken = async (res: Client) => { setIsLoadingToken(true) if (!res?.isActive) { setShowInfoModal(true) setIsLoadingToken(false) + return; } else { try { + if (!res?._id) { + throw new Error("Invalid client data"); + } const response = await settings.generateTokenApi(res) - if (response) { + if (response?.token) { toast({ title: "Success", - description: "Token generated successfully", + description: "Token generated successfully. Copy it now as it won't be shown again.", variant: "success", }) + await fetchClientDetails(); // Refresh client details to show new token + } else { + throw new Error("Token generation failed - invalid response"); } dispatch(performRefresh()) } catch (error: any) { toast({ title: "Error", description: errorMessage, variant: "destructive", }) } } }Likely invalid or redundant comment.
| <div className="grid grid-cols-1 gap-4 sm:grid-cols-2"> | ||
| <div className="space-y-2"> | ||
| <Label htmlFor="firstName">First name</Label> | ||
| <Input id="firstName" name="firstName" value={profile.firstName} onChange={handleInputChange} /> | ||
| </div> | ||
| <div className="space-y-2"> | ||
| <Label htmlFor="lastName">Last name</Label> | ||
| <Input id="lastName" name="lastName" value={profile.lastName} onChange={handleInputChange} /> | ||
| </div> | ||
| <div className="space-y-2"> | ||
| <Label htmlFor="email">Email</Label> | ||
| <Input id="email" name="email" type="email" value={profile.email} onChange={handleInputChange} disabled /> | ||
| </div> | ||
| <div className="space-y-2"> | ||
| <Label htmlFor="jobTitle">Job title</Label> | ||
| <Input id="jobTitle" name="jobTitle" value={profile.jobTitle || ""} onChange={handleInputChange} /> | ||
| </div> | ||
| <div className="space-y-2"> | ||
| <Label htmlFor="country">Country</Label> | ||
| <Select value={profile.country} onValueChange={handleSelectChange("country")}> | ||
| <SelectTrigger> | ||
| <SelectValue placeholder="Select a country" /> | ||
| </SelectTrigger> | ||
| <SelectContent> | ||
| {countries.map((country) => ( | ||
| <SelectItem key={country.value} value={country.value}> | ||
| {country.label} | ||
| </SelectItem> | ||
| ))} | ||
| </SelectContent> | ||
| </Select> | ||
| </div> | ||
| <div className="space-y-2"> | ||
| <Label htmlFor="timezone">Timezone</Label> | ||
| <Select value={profile.timezone} onValueChange={handleSelectChange("timezone")}> | ||
| <SelectTrigger> | ||
| <SelectValue placeholder="Select a timezone" /> | ||
| </SelectTrigger> | ||
| <SelectContent> | ||
| {timezones.map((timezone) => ( | ||
| <SelectItem key={timezone.value} value={timezone.value}> | ||
| {timezone.label} | ||
| </SelectItem> | ||
| ))} | ||
| </SelectContent> | ||
| </Select> | ||
| </div> | ||
| </div> | ||
|
|
||
| <div className="space-y-2"> | ||
| <Label htmlFor="description">Bio</Label> | ||
| <Textarea id="description" name="description" value={profile.description || ""} onChange={handleInputChange} rows={4} /> | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add form validation.
The form lacks input validation for required fields and format validation for inputs like email.
Consider using a form validation library like Zod or Yup to enforce validation rules.
|
|
||
| const formData = new FormData() | ||
| formData.append("file", file) | ||
| formData.append("upload_preset", process.env.NEXT_PUBLIC_CLOUDINARY_PRESET || "") |
There was a problem hiding this comment.
Add environment variable validation.
The Cloudinary preset is accessed without validation, which could lead to silent failures.
Add validation:
-formData.append("upload_preset", process.env.NEXT_PUBLIC_CLOUDINARY_PRESET || "")
+const preset = process.env.NEXT_PUBLIC_CLOUDINARY_PRESET
+if (!preset) {
+ throw new Error("NEXT_PUBLIC_CLOUDINARY_PRESET is not configured")
+}
+formData.append("upload_preset", preset)📝 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.
| formData.append("upload_preset", process.env.NEXT_PUBLIC_CLOUDINARY_PRESET || "") | |
| const preset = process.env.NEXT_PUBLIC_CLOUDINARY_PRESET | |
| if (!preset) { | |
| throw new Error("NEXT_PUBLIC_CLOUDINARY_PRESET is not configured") | |
| } | |
| formData.append("upload_preset", preset) |
| const handleProfileImageUpdate = async (e: React.ChangeEvent<HTMLInputElement>) => { | ||
| const file = e.target.files?.[0] | ||
| if (!file) return | ||
|
|
||
| const formData = new FormData() | ||
| formData.append("file", file) | ||
| formData.append("upload_preset", process.env.NEXT_PUBLIC_CLOUDINARY_PRESET || "") | ||
| formData.append("folder", "profiles") | ||
|
|
||
| setProfileUploading(true) | ||
| try { | ||
| const responseData = await cloudinaryImageUpload(formData) | ||
| if (profile && currentUser) { | ||
| const updatedProfile = { ...profile, profilePicture: responseData.secure_url } | ||
| setProfile(updatedProfile) | ||
| await settings.updateUserDetailsApi({ profilePicture: responseData.secure_url }, currentUser._id) | ||
| dispatch(setUserDetails({ ...currentUser, profilePicture: responseData.secure_url })) | ||
| toast({ | ||
| title: "Success", | ||
| description: "Profile picture updated successfully", | ||
| }) | ||
| } | ||
| } catch (error) { | ||
| toast({ | ||
| title: "Error", | ||
| description: "Failed to update profile picture", | ||
| }) | ||
| } finally { | ||
| setProfileUploading(false) | ||
| } | ||
| } |
There was a problem hiding this comment.
Add file validation for profile image upload.
The current implementation lacks file size and type validation, which could lead to failed uploads or security issues.
const handleProfileImageUpdate = async (e: React.ChangeEvent<HTMLInputElement>) => {
const file = e.target.files?.[0]
if (!file) return
+ // Validate file size (e.g., 5MB limit)
+ const MAX_FILE_SIZE = 5 * 1024 * 1024
+ if (file.size > MAX_FILE_SIZE) {
+ toast({
+ title: "Error",
+ description: "File size should not exceed 5MB",
+ })
+ return
+ }
+
+ // Validate file type
+ const allowedTypes = ['image/jpeg', 'image/png', 'image/gif']
+ if (!allowedTypes.includes(file.type)) {
+ toast({
+ title: "Error",
+ description: "Only JPEG, PNG and GIF images are allowed",
+ })
+ return
+ }📝 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 handleProfileImageUpdate = async (e: React.ChangeEvent<HTMLInputElement>) => { | |
| const file = e.target.files?.[0] | |
| if (!file) return | |
| const formData = new FormData() | |
| formData.append("file", file) | |
| formData.append("upload_preset", process.env.NEXT_PUBLIC_CLOUDINARY_PRESET || "") | |
| formData.append("folder", "profiles") | |
| setProfileUploading(true) | |
| try { | |
| const responseData = await cloudinaryImageUpload(formData) | |
| if (profile && currentUser) { | |
| const updatedProfile = { ...profile, profilePicture: responseData.secure_url } | |
| setProfile(updatedProfile) | |
| await settings.updateUserDetailsApi({ profilePicture: responseData.secure_url }, currentUser._id) | |
| dispatch(setUserDetails({ ...currentUser, profilePicture: responseData.secure_url })) | |
| toast({ | |
| title: "Success", | |
| description: "Profile picture updated successfully", | |
| }) | |
| } | |
| } catch (error) { | |
| toast({ | |
| title: "Error", | |
| description: "Failed to update profile picture", | |
| }) | |
| } finally { | |
| setProfileUploading(false) | |
| } | |
| } | |
| const handleProfileImageUpdate = async (e: React.ChangeEvent<HTMLInputElement>) => { | |
| const file = e.target.files?.[0] | |
| if (!file) return | |
| // Validate file size (e.g., 5MB limit) | |
| const MAX_FILE_SIZE = 5 * 1024 * 1024 | |
| if (file.size > MAX_FILE_SIZE) { | |
| toast({ | |
| title: "Error", | |
| description: "File size should not exceed 5MB", | |
| }) | |
| return | |
| } | |
| // Validate file type | |
| const allowedTypes = ['image/jpeg', 'image/png', 'image/gif'] | |
| if (!allowedTypes.includes(file.type)) { | |
| toast({ | |
| title: "Error", | |
| description: "Only JPEG, PNG and GIF images are allowed", | |
| }) | |
| return | |
| } | |
| const formData = new FormData() | |
| formData.append("file", file) | |
| formData.append("upload_preset", process.env.NEXT_PUBLIC_CLOUDINARY_PRESET || "") | |
| formData.append("folder", "profiles") | |
| setProfileUploading(true) | |
| try { | |
| const responseData = await cloudinaryImageUpload(formData) | |
| if (profile && currentUser) { | |
| const updatedProfile = { ...profile, profilePicture: responseData.secure_url } | |
| setProfile(updatedProfile) | |
| await settings.updateUserDetailsApi({ profilePicture: responseData.secure_url }, currentUser._id) | |
| dispatch(setUserDetails({ ...currentUser, profilePicture: responseData.secure_url })) | |
| toast({ | |
| title: "Success", | |
| description: "Profile picture updated successfully", | |
| }) | |
| } | |
| } catch (error) { | |
| toast({ | |
| title: "Error", | |
| description: "Failed to update profile picture", | |
| }) | |
| } finally { | |
| setProfileUploading(false) | |
| } | |
| } |
| } else { | ||
| throw new Error(response.message) | ||
| } |
There was a problem hiding this comment.
Improve error handling for API response.
The current error handling might throw on an undefined message property. Consider using optional chaining or providing a default error message.
- throw new Error(response.message)
+ throw new Error(response?.message || "Failed to update password")📝 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.
| } else { | |
| throw new Error(response.message) | |
| } | |
| } else { | |
| throw new Error(response?.message || "Failed to update password") | |
| } |
| 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.
Use Number.isNaN for safer type checking.
Replace isNaN with Number.isNaN to avoid type coercion issues.
Apply this diff to fix the issue:
- if (isNaN(date.getTime())) return "Invalid Date"
+ if (Number.isNaN(date.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.
| 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 (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)) | |
| 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] 28-28: 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)
🛠️ Refactor suggestion
Replace global isNaN with Number.isNaN for safer date validation.
The global isNaN performs type coercion which can lead to unexpected results.
- if (isNaN(date.getTime())) return "Invalid Date"
+ if (Number.isNaN(date.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.
| 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] 28-28: 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 handleActivationRequest = async () => { | ||
| setIsLoadingActivationRequest(true) | ||
| try { | ||
| const clientID = selectedClient?._id | ||
| const response = await settings.activationRequestApi(clientID) | ||
| if (response) { | ||
| setShowInfoModal(false) | ||
| setTimeout(() => { | ||
| toast({ | ||
| title: "Success", | ||
| description: "Activation request sent successfully", | ||
| }) | ||
| }, 3000) | ||
| } | ||
| } catch (error: any) { | ||
| setShowInfoModal(false) | ||
| setTimeout(() => { | ||
| toast({ | ||
| title: "Error", | ||
| description: error.message || "Failed to send activation request", | ||
| variant: "destructive", | ||
| }) | ||
| }, 3000) | ||
| } finally { | ||
| setIsLoadingActivationRequest(false) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unnecessary timeouts and improve error handling.
The function uses setTimeout unnecessarily and could handle errors better.
Apply this diff to improve the function:
const handleActivationRequest = async () => {
setIsLoadingActivationRequest(true)
try {
const clientID = selectedClient?._id
+ if (!clientID) {
+ throw new Error("Invalid client ID");
+ }
const response = await settings.activationRequestApi(clientID)
if (response) {
setShowInfoModal(false)
- setTimeout(() => {
- toast({
- title: "Success",
- description: "Activation request sent successfully",
- })
- }, 3000)
+ toast({
+ title: "Success",
+ description: "Activation request sent successfully",
+ })
}
} catch (error: any) {
setShowInfoModal(false)
- setTimeout(() => {
- toast({
- title: "Error",
- description: error.message || "Failed to send activation request",
- variant: "destructive",
- })
- }, 3000)
+ toast({
+ title: "Error",
+ description: error?.response?.data?.message || error.message || "Failed to send activation request",
+ variant: "destructive",
+ })
} finally {
setIsLoadingActivationRequest(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 handleActivationRequest = async () => { | |
| setIsLoadingActivationRequest(true) | |
| try { | |
| const clientID = selectedClient?._id | |
| const response = await settings.activationRequestApi(clientID) | |
| if (response) { | |
| setShowInfoModal(false) | |
| setTimeout(() => { | |
| toast({ | |
| title: "Success", | |
| description: "Activation request sent successfully", | |
| }) | |
| }, 3000) | |
| } | |
| } catch (error: any) { | |
| setShowInfoModal(false) | |
| setTimeout(() => { | |
| toast({ | |
| title: "Error", | |
| description: error.message || "Failed to send activation request", | |
| variant: "destructive", | |
| }) | |
| }, 3000) | |
| } finally { | |
| setIsLoadingActivationRequest(false) | |
| } | |
| } | |
| const handleActivationRequest = async () => { | |
| setIsLoadingActivationRequest(true) | |
| try { | |
| const clientID = selectedClient?._id | |
| if (!clientID) { | |
| throw new Error("Invalid client ID"); | |
| } | |
| const response = await settings.activationRequestApi(clientID) | |
| if (response) { | |
| setShowInfoModal(false) | |
| toast({ | |
| title: "Success", | |
| description: "Activation request sent successfully", | |
| }) | |
| } | |
| } catch (error: any) { | |
| setShowInfoModal(false) | |
| toast({ | |
| title: "Error", | |
| description: error?.response?.data?.message || error.message || "Failed to send activation request", | |
| variant: "destructive", | |
| }) | |
| } finally { | |
| setIsLoadingActivationRequest(false) | |
| } | |
| } |
🛠️ Refactor suggestion
Simplify activation request handling.
The function uses unnecessary timeouts and could benefit from better error handling.
const handleActivationRequest = async () => {
setIsLoadingActivationRequest(true)
try {
const clientID = selectedClient?._id
+ if (!clientID) {
+ throw new Error('No client selected');
+ }
const response = await settings.activationRequestApi(clientID)
if (response) {
setShowInfoModal(false)
- setTimeout(() => {
- toast({
- title: "Success",
- description: "Activation request sent successfully",
- })
- }, 3000)
+ toast({
+ title: "Success",
+ description: "Activation request sent successfully",
+ })
}
} catch (error: any) {
setShowInfoModal(false)
- setTimeout(() => {
- toast({
- title: "Error",
- description: error.message || "Failed to send activation request",
- variant: "destructive",
- })
- }, 3000)
+ toast({
+ title: "Error",
+ description: error.message || "Failed to send activation request",
+ variant: "destructive",
+ })
} finally {
setIsLoadingActivationRequest(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 handleActivationRequest = async () => { | |
| setIsLoadingActivationRequest(true) | |
| try { | |
| const clientID = selectedClient?._id | |
| const response = await settings.activationRequestApi(clientID) | |
| if (response) { | |
| setShowInfoModal(false) | |
| setTimeout(() => { | |
| toast({ | |
| title: "Success", | |
| description: "Activation request sent successfully", | |
| }) | |
| }, 3000) | |
| } | |
| } catch (error: any) { | |
| setShowInfoModal(false) | |
| setTimeout(() => { | |
| toast({ | |
| title: "Error", | |
| description: error.message || "Failed to send activation request", | |
| variant: "destructive", | |
| }) | |
| }, 3000) | |
| } finally { | |
| setIsLoadingActivationRequest(false) | |
| } | |
| } | |
| const handleActivationRequest = async () => { | |
| setIsLoadingActivationRequest(true) | |
| try { | |
| const clientID = selectedClient?._id | |
| if (!clientID) { | |
| throw new Error('No client selected'); | |
| } | |
| const response = await settings.activationRequestApi(clientID) | |
| if (response) { | |
| setShowInfoModal(false) | |
| toast({ | |
| title: "Success", | |
| description: "Activation request sent successfully", | |
| }) | |
| } | |
| } catch (error: any) { | |
| setShowInfoModal(false) | |
| toast({ | |
| title: "Error", | |
| description: error.message || "Failed to send activation request", | |
| variant: "destructive", | |
| }) | |
| } finally { | |
| setIsLoadingActivationRequest(false) | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
netmanager-app/core/apis/settings.ts (1)
7-18: Add JSDoc documentation to interfaces.Consider adding JSDoc documentation to improve code maintainability and IDE support.
+/** + * Interface for creating a new client + * @property {string} name - The name of the client + * @property {string[]} ip_addresses - List of allowed IP addresses + * @property {boolean} isActive - Whether the client is active + * @property {string} client_secret - Client's secret key + * @property {string} user_id - Associated user ID + */ interface CreateClientData { name: string; ip_addresses: string[]; isActive: boolean; client_secret: string; user_id: string; } +/** + * Interface for password update data + * @property {string} password - New password + * @property {string} old_password - Current password + */ interface PasswordData { password: string; old_password: string; }netmanager-app/components/Settings/ApiTokens.tsx (2)
33-33: Move itemsPerPage to a constant.Define itemsPerPage as a constant at the top of the file for better maintainability.
+const ITEMS_PER_PAGE = 4; const UserClientsTable: React.FC = () => { // ... - const itemsPerPage = 4 + const itemsPerPage = ITEMS_PER_PAGE
296-296: Implement the missing pagination component.The pagination component is currently missing, which limits the user's ability to navigate through the client list efficiently.
Would you like me to help generate a reusable pagination component implementation?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
netmanager-app/components/Settings/ApiTokens.tsx(1 hunks)netmanager-app/core/apis/settings.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
netmanager-app/components/Settings/ApiTokens.tsx
[error] 87-87: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 112-112: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (4)
netmanager-app/core/apis/settings.ts (2)
22-33: LGTM! Well-structured API function.The implementation includes proper error handling and type safety.
35-56: 🛠️ Refactor suggestionAdd consistent error handling.
These functions should follow the same error handling pattern as
getUserClientsApi.createClientApi: async (data: CreateClientData): Promise<Client> => { + try { return await axiosInstance.post<CreateClientData, AxiosResponse<Client>>(`${USERS_MGT_URL}/clients`, data) .then((response) => response.data); + } catch (error) { + console.error("Error creating client:", error); + throw error; + } }, updateUserPasswordApi: async ( userId: string, userData: PasswordData ): Promise<PasswordData> => { + try { return await axiosInstance.put(`${USERS_MGT_URL}/updatePassword`, userData, { params: { id: userId }, }) .then((response) => response.data); + } catch (error) { + console.error("Error updating password:", error); + throw error; + } }, updateClientApi: async ( data: CreateClientData, client_id: string ): Promise<Client> => { + try { return await axiosInstance.put(`${USERS_MGT_URL}/clients/${client_id}`, data) .then((response) => response.data); + } catch (error) { + console.error("Error updating client:", error); + throw error; + } },Likely invalid or redundant comment.
netmanager-app/components/Settings/ApiTokens.tsx (2)
229-255: LGTM! Secure token display implementation.The implementation properly masks the token and provides a convenient copy feature.
143-169: 🛠️ Refactor suggestionRemove unnecessary timeouts and improve error handling.
The function uses unnecessary timeouts and could benefit from better validation.
const handleActivationRequest = async () => { setIsLoadingActivationRequest(true) try { const clientID = selectedClient?._id + if (!clientID) { + throw new Error("No client selected"); + } const response = await settings.activationRequestApi(clientID) if (response) { setShowInfoModal(false) - setTimeout(() => { - toast({ - title: "Success", - description: "Activation request sent successfully", - }) - }, 3000) + toast({ + title: "Success", + description: "Activation request sent successfully", + }) } } catch (error: any) { setShowInfoModal(false) - setTimeout(() => { - toast({ - title: "Error", - description: error.message || "Failed to send activation request", - variant: "destructive", - }) - }, 3000) + toast({ + title: "Error", + description: error?.response?.data?.message || error.message || "Failed to send activation request", + variant: "destructive", + }) } finally { setIsLoadingActivationRequest(false) } }Likely invalid or redundant comment.
| generateTokenApi: async (data: Client): Promise<Client> => { | ||
| const client_id = data._id | ||
| const formData = { client_id, name: data.name } | ||
| const response = await axiosInstance.post(`${USERS_MGT_URL}/tokens`, formData); | ||
| return response.data; | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation for required fields.
Validate required fields before making the API call to prevent potential runtime errors.
generateTokenApi: async (data: Client): Promise<Client> => {
+ if (!data?._id || !data?.name) {
+ throw new Error("Client ID and name are required");
+ }
const client_id = data._id
const formData = { client_id, name: data.name }
+ try {
const response = await axiosInstance.post(`${USERS_MGT_URL}/tokens`, formData);
return response.data;
+ } catch (error) {
+ console.error("Error generating token:", error);
+ throw 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.
| generateTokenApi: async (data: Client): Promise<Client> => { | |
| const client_id = data._id | |
| const formData = { client_id, name: data.name } | |
| const response = await axiosInstance.post(`${USERS_MGT_URL}/tokens`, formData); | |
| return response.data; | |
| }, | |
| generateTokenApi: async (data: Client): Promise<Client> => { | |
| if (!data?._id || !data?.name) { | |
| throw new Error("Client ID and name are required"); | |
| } | |
| const client_id = data._id | |
| const formData = { client_id, name: data.name } | |
| try { | |
| const response = await axiosInstance.post(`${USERS_MGT_URL}/tokens`, formData); | |
| return response.data; | |
| } catch (error) { | |
| console.error("Error generating token:", error); | |
| throw error; | |
| } | |
| }, |
| getClientsApi: async () => { | ||
| return axiosInstance.get(`${USERS_MGT_URL}/clients`).then((response) => response.data); | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add return type and error handling.
Improve type safety and error handling for better reliability.
-getClientsApi: async () => {
+getClientsApi: async (): Promise<Client[]> => {
+ try {
return axiosInstance.get(`${USERS_MGT_URL}/clients`).then((response) => response.data);
+ } catch (error) {
+ console.error("Error fetching clients:", error);
+ throw 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.
| getClientsApi: async () => { | |
| return axiosInstance.get(`${USERS_MGT_URL}/clients`).then((response) => response.data); | |
| }, | |
| getClientsApi: async (): Promise<Client[]> => { | |
| try { | |
| return axiosInstance.get(`${USERS_MGT_URL}/clients`).then((response) => response.data); | |
| } catch (error) { | |
| console.error("Error fetching clients:", error); | |
| throw error; | |
| } | |
| }, |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
netmanager-app/components/Settings/Clients.tsx (2)
137-142: Optimize client filtering logic.The current implementation filters clients on every render. Consider memoizing the filtered results.
+const filteredClients = useMemo(() => -const filteredClients = clients.filter( (client) => client.name.toLowerCase().includes(searchQuery.toLowerCase()) || (client.user?.email?.toLowerCase().includes(searchQuery.toLowerCase()) ?? false) -); +), [clients, searchQuery]);
270-273: Use optional chaining for cleaner null checks.Simplify the nested conditional check using optional chaining.
-{client.user && client.user.email - ? client.user.email - : "N/A"} +{client.user?.email ?? "N/A"}🧰 Tools
🪛 Biome (1.9.4)
[error] 270-271: 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 (1)
netmanager-app/components/Settings/Clients.tsx(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
netmanager-app/components/Settings/Clients.tsx
[error] 94-94: 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] 270-271: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 44-44: 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 (3)
netmanager-app/components/Settings/Clients.tsx (3)
41-54: Replace global isNaN with Number.isNaN for safer date validation.The global
isNaNperforms type coercion which can lead to unexpected results.Apply this diff to fix the issue:
- if (isNaN(date.getTime())) return "Invalid Date"; + if (Number.isNaN(date.getTime())) return "Invalid Date";🧰 Tools
🪛 Biome (1.9.4)
[error] 44-44: 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)
292-302: Add space between action buttons.Based on the PR comments, there should be space between edit and delete buttons.
<TableCell className="text-right"> + <div className="space-x-2"> <Button variant="ghost" onClick={() => client.isActive ? handleDeactivateClick(client) : handleActivateClick(client) } > {client.isActive ? "Deactivate" : "Activate"} </Button> + </div> </TableCell>
88-116: 🛠️ Refactor suggestionSimplify boolean logic and enhance error handling.
The activation/deactivation logic can be simplified and error handling can be improved.
Apply this diff to enhance the code:
const handleActivateDeactivate = async (clientId: string, activate: boolean) => { const data = { _id: clientId, - isActive: activate ? true : false, + isActive: activate, }; try { + if (!clientId) { + throw new Error('Client ID is required'); + } await settings.activateUserClientApi(data); await fetchClients(); toast({ title: "Success", description: `Client ${activate ? "activated" : "deactivated"} successfully`, }); } catch (error) { + const errorMessage = error instanceof Error ? error.message : 'Unknown error occurred'; toast({ title: "Error", - description: `Failed to ${activate ? "activate" : "deactivate"} client`, + description: `Failed to ${activate ? "activate" : "deactivate"} client: ${errorMessage}`, variant: "destructive", }); } };Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 94-94: 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)
| const fetchClients = async () => { | ||
| setLoading(true); | ||
| try { | ||
| const response = await getClientsApi(); | ||
| 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.
💡 Verification agent
🧩 Analysis chain
Verify error handling in fetchClients.
The error handling in fetchClients could be more specific about what went wrong.
🏁 Script executed:
#!/bin/bash
# Description: Check for all possible error scenarios in getClientsApi
ast-grep --pattern 'export const getClientsApi = async () => {
$$$
}'Length of output: 686
Action: Enhance the error specificity in fetchClients.
The current implementation catches any error thrown by getClientsApi and displays a generic toast ("Failed to fetch clients"). Our verification confirms that both implementations of getClientsApi (in netmanager/src/views/apis/analytics.js and netmanager-app/core/apis/analytics.ts) simply forward errors from the underlying Axios calls without adding any meaningful context. As a result, fetchClients cannot provide users with detailed information about what went wrong.
- API Implementation: Both versions of
getClientsApido not include any error processing (e.g., no inspection oferror.responseorerror.message), so they simply pass along the rejection. - Error Handling: The
fetchClientsfunction’s error block displays a generic message, which may not be helpful in diagnosing specific issues.
Consider updating either the API function to throw more descriptive errors or modifying fetchClients to extract useful details (e.g., using error.response?.data or error.message) for better feedback.
…ing token regeneration functionality
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
netmanager-app/components/Settings/ApiTokens.tsx (4)
35-54: Enhance error handling in fetchClients.The error message could be more informative by including the actual error details.
- console.error(error) + const errorMessage = error?.response?.data?.message || error.message || "Failed to fetch user details" toast({ title: "Error", - description: "Failed to fetch user details", + description: errorMessage, variant: "destructive", })
81-84: Simplify boolean expression.The boolean conversion is unnecessary here.
const hasAccessToken = (clientId: string): boolean => { const client = clients.find((client) => client._id === clientId) - return client?.access_token?.token ? true : false + return !!client?.access_token?.token }🧰 Tools
🪛 Biome (1.9.4)
[error] 83-83: 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)
216-224: Revise token display logic.The current implementation shows the same token twice, which might be confusing. Consider showing a truncated version instead.
- <span className="font-mono"> - {client.access_token?.token} - <span className="mx-1">•••••••</span> - {client.access_token?.token} - </span> + <span className="font-mono"> + {client.access_token?.token?.slice(0, 8)} + <span className="mx-1">•••••••</span> + {client.access_token?.token?.slice(-8)} + </span>
274-285: Add spacing between action buttons.As per PR objectives, add proper spacing between edit and other action buttons.
- <Button + <Button + className="ml-2" variant="ghost" size="sm" onClick={() => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
netmanager-app/components/Settings/ApiTokens.tsx(1 hunks)netmanager-app/components/Settings/MyProfile.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- netmanager-app/components/Settings/MyProfile.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
netmanager-app/components/Settings/ApiTokens.tsx
[error] 83-83: 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] 99-99: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (3)
netmanager-app/components/Settings/ApiTokens.tsx (3)
1-34: LGTM! Well-structured component setup.The imports are comprehensive and state management is well-organized.
130-156: Remove unnecessary timeouts and improve error handling.The function uses setTimeout unnecessarily and could handle errors better.
297-297: Implement pagination component.The comment indicates a missing pagination component. This should be implemented to ensure proper navigation through the client list.
Would you like me to help implement a basic pagination component?
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
netmanager-app/core/hooks/useClients.ts (1)
13-15: Add type safety to the success callback.The
onSuccesscallback uses theanytype which bypasses TypeScript's type checking benefits.- onSuccess: (data: any) => { + onSuccess: (data: { clients: Client[] }) => { dispatch(setClients(data)); },netmanager-app/core/hooks/useUserClients.ts (1)
18-21: Add type safety to the success callback.Similar to useClients.ts, the success callback should be properly typed.
- onSuccess: (data: any) => { + onSuccess: (data: { clients: Client[] }) => { dispatch(setUserClients(data.clients)); },netmanager-app/core/redux/slices/clientsSlice.ts (1)
32-34: Add type safety to reducer action payloads.The reducers are missing PayloadAction types which could lead to type-related bugs.
- addClients: (state, action) => { + addClients: (state, action: PayloadAction<Client[]>) => { state.clients = action.payload; }, - addClientsDetails: (state, action) => { + addClientsDetails: (state, action: PayloadAction<Client[]>) => { state.clientsDetails = action.payload; }, - setAccessToken: (state, action) => { + setAccessToken: (state, action: PayloadAction<AccessToken[]>) => { state.access_token = action.payload },Also applies to: 35-37, 38-40
netmanager-app/components/Settings/ApiTokens.tsx (1)
80-86: Simplify client lookup functions using optional chaining.The client lookup functions have repetitive code that can be simplified.
const hasAccessToken = (clientId: string): boolean => { - const client = - Array.isArray(clientsDetails) && clientsDetails - ? clientsDetails?.find((client: Client) => client._id === clientId) - : undefined - return client?.access_token?.token ? true : false + return !!clientsDetails?.find((client: Client) => + client._id === clientId + )?.access_token?.token } const getClientToken = (clientID: string) => { - const client = - Array.isArray(clientsDetails) && clientsDetails - ? clientsDetails?.find((client: Client) => client._id === clientID) - : undefined - return client?.access_token?.token + return clientsDetails?.find((client: Client) => + client._id === clientID + )?.access_token?.token }Also applies to: 88-94, 95-101, 103-109
🧰 Tools
🪛 Biome (1.9.4)
[error] 85-85: 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)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
netmanager-app/app/types/clients.ts(0 hunks)netmanager-app/components/Settings/ApiTokens.tsx(1 hunks)netmanager-app/core/hooks/useClients.ts(1 hunks)netmanager-app/core/hooks/useUserClients.ts(1 hunks)netmanager-app/core/redux/slices/clientsSlice.ts(1 hunks)
💤 Files with no reviewable changes (1)
- netmanager-app/app/types/clients.ts
🧰 Additional context used
🪛 Biome (1.9.4)
netmanager-app/components/Settings/ApiTokens.tsx
[error] 85-85: 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] 93-93: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 100-100: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 108-108: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (3)
netmanager-app/core/hooks/useClients.ts (1)
21-25: LGTM! Good error handling and default values.The hook properly handles errors and provides sensible defaults for the returned values.
netmanager-app/components/Settings/ApiTokens.tsx (2)
111-137: Improve token generation reliability and feedback.The token generation flow needs enhancement to address reported issues.
139-165: Remove unnecessary timeouts and improve error handling.The function uses setTimeout unnecessarily and could handle errors better.
| settings.getUserClientsApi( | ||
| userDetails?._id || "" | ||
| ), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate user ID before making API call.
Using an empty string as fallback for userDetails?._id could lead to unnecessary API calls.
- queryFn: () =>
- settings.getUserClientsApi(
- userDetails?._id || ""
- ),
+ queryFn: () => {
+ if (!userDetails?._id) {
+ throw new Error("User ID is required");
+ }
+ return settings.getUserClientsApi(userDetails._id);
+ },📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| settings.getUserClientsApi( | |
| userDetails?._id || "" | |
| ), | |
| queryFn: () => { | |
| if (!userDetails?._id) { | |
| throw new Error("User ID is required"); | |
| } | |
| return settings.getUserClientsApi(userDetails._id); | |
| }, |
| accees_token: [], | ||
| refresh: false, |
There was a problem hiding this comment.
Fix typo in initialState property name.
There's a typo in the accees_token property name.
- accees_token: [],
+ access_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.
| accees_token: [], | |
| refresh: false, | |
| access_token: [], | |
| refresh: false, |
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