[Netmanager]: Profile Settings Page #2401
Conversation
📝 WalkthroughWalkthroughThis pull request introduces a comprehensive user profile management system for the netmanager application. The changes include new components for profile editing, password management, API token handling, and client management. The implementation leverages React hooks, Redux for state management, and integrates various utility functions for handling countries, timezones, and API interactions. Changes
Suggested labels
Suggested reviewers
Poem
Possibly related PRs
✨ 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"; | |
| } |
| 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.
| {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); | |
| }), | |
| } |
| } 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
…r handling; add profile image upload functionality with size validation and success/error notifications.
… improve profile picture upload handling and state management.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
netmanager-app/app/(authenticated)/profile/components/MyProfile.tsx (1)
92-95:⚠️ Potential issueImplement profile update functionality.
The handleSubmit function is incomplete and doesn't update the profile data in the backend.
🧹 Nitpick comments (4)
netmanager-app/app/(authenticated)/profile/components/MyProfile.tsx (1)
40-74: Prevent potential memory leak in useEffect.The fetch operation could complete after the component unmounts. Consider adding a cleanup function.
useEffect(() => { + let isSubscribed = true; const fetchUserData = async () => { if (!currentuser) return; setIsLoading(true); setError(null); try { const response = await users.getUserDetails(currentuser._id); const userData = response?.users?.[0]; - if (userData) { + if (userData && isSubscribed) { setProfile({ firstName: userData.firstName || "", lastName: userData.lastName || "", email: userData.email || "", jobTitle: userData.jobTitle || "", country: userData.country || "", timezone: userData.timezone || "", bio: userData.description || "", profilePicture: userData.profilePicture || "", }); } else { setError("User data not found"); } } catch (error) { - setError("Failed to fetch user data"); + if (isSubscribed) { + setError("Failed to fetch user data"); + } console.error("Error fetching user data:", error); } finally { - setIsLoading(false); + if (isSubscribed) { + setIsLoading(false); + } } }; fetchUserData(); + return () => { + isSubscribed = false; + }; }, [currentuser]);netmanager-app/core/apis/settings.ts (1)
7-13: Add JSDoc documentation to the interface.Consider adding JSDoc documentation to describe the purpose and properties of the
CreateClientDatainterface.+/** + * Interface representing the data required to create 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 - The client's secret key. + * @property {string} user_id - The associated user ID. + */ interface CreateClientData { name: string; ip_addresses: string[]; isActive: boolean; client_secret: string; user_id: string; }netmanager-app/app/(authenticated)/profile/components/Clients.tsx (2)
74-97: Simplify boolean assignment.Remove the unnecessary ternary operator for the boolean assignment.
const data = { _id: clientId, - isActive: activate ? true : false, + isActive: activate, };🧰 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)
209-209: Use optional chaining for cleaner code.Simplify the nested property access using optional chaining.
-<TableCell>{client.user && client.user.email ? client.user.email : "N/A"}</TableCell> +<TableCell>{client.user?.email ?? "N/A"}</TableCell>🧰 Tools
🪛 Biome (1.9.4)
[error] 209-209: 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 (4)
netmanager-app/app/(authenticated)/profile/components/Clients.tsx(1 hunks)netmanager-app/app/(authenticated)/profile/components/MyProfile.tsx(1 hunks)netmanager-app/components/sidebar.tsx(0 hunks)netmanager-app/core/apis/settings.ts(1 hunks)
💤 Files with no reviewable changes (1)
- netmanager-app/components/sidebar.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
netmanager-app/app/(authenticated)/profile/components/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 (4)
netmanager-app/app/(authenticated)/profile/components/MyProfile.tsx (1)
18-27: LGTM! Well-defined TypeScript interface.The Profile interface is properly typed with all necessary fields and optional properties.
netmanager-app/core/apis/settings.ts (1)
46-53: Use the existing axios instance.A new axios instance is created for this request while an existing instance is available.
export const updateClientApi = async ( data: CreateClientData, client_id: string ): Promise<Client> => { - return await createAxiosInstance() + return await axiosInstance .put(`${USERS_MGT_URL}/clients/${client_id}`, data) .then((response) => response.data); };netmanager-app/app/(authenticated)/profile/components/Clients.tsx (2)
227-234: Improve button spacing and accessibility in actions column.Add proper spacing between buttons and enhance accessibility with ARIA labels.
<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> + </div> </TableCell>
118-129: Optimize data processing with memoization.The filtering and sorting operations are recalculated on every render. Use
useMemoto cache these results.+import { useMemo } from "react" -const filteredClients = clients.filter( +const filteredClients = useMemo(() => clients.filter( (client) => client.name.toLowerCase().includes(searchQuery.toLowerCase()) || (client.user?.email?.toLowerCase().includes(searchQuery.toLowerCase()) ?? false) - ) +), [clients, searchQuery]) -const sortedClients = [...filteredClients].sort((a, b) => { +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])
| <Button type="button" variant="outline" className="text-red-500"> | ||
| Delete | ||
| </Button> |
There was a problem hiding this comment.
Implement profile picture deletion.
The delete button is present but lacks an onClick handler for removing the profile picture.
-<Button type="button" variant="outline" className="text-red-500">
+<Button
+ type="button"
+ variant="outline"
+ className="text-red-500"
+ onClick={async () => {
+ try {
+ await fetch('/api/delete-profile-image', {
+ method: 'DELETE',
+ });
+ setProfile(prev => prev ? ({
+ ...prev,
+ profilePicture: '',
+ }) : null);
+ toast({
+ title: "Success",
+ description: "Profile image deleted successfully",
+ });
+ } catch (error) {
+ toast({
+ title: "Error",
+ description: "Failed to delete profile image",
+ variant: "destructive",
+ });
+ }
+ }}
+>
Delete
</Button>📝 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.
| <Button type="button" variant="outline" className="text-red-500"> | |
| Delete | |
| </Button> | |
| <Button | |
| type="button" | |
| variant="outline" | |
| className="text-red-500" | |
| onClick={async () => { | |
| try { | |
| await fetch('/api/delete-profile-image', { | |
| method: 'DELETE', | |
| }); | |
| setProfile(prev => prev ? ({ | |
| ...prev, | |
| profilePicture: '', | |
| }) : null); | |
| toast({ | |
| title: "Success", | |
| description: "Profile image deleted successfully", | |
| }); | |
| } catch (error) { | |
| toast({ | |
| title: "Error", | |
| description: "Failed to delete profile image", | |
| variant: "destructive", | |
| }); | |
| } | |
| }} | |
| > | |
| Delete | |
| </Button> |
| const [isLoading, setIsLoading] = useState(true); | ||
| const [error, setError] = useState<string | null>(null); | ||
|
|
||
| const [isEditing, setIsEditing] = useState(false); |
There was a problem hiding this comment.
💡 Codebase verification
Issue Detected: Immediate Toggle of isEditing Without Awaiting API Response
- The
isEditingstate is initialized as shown, but in thehandleSubmitfunction it’s set tofalseimmediately after preventing the default form submission—without any asynchronous operation for API integration. - This means the form inputs meant to be disabled during submission aren’t truly protected against premature user interaction if an update fails.
- Consider adopting a dedicated submission state (e.g.,
isSubmitting) and waiting for the API response before updating the UI, ensuring a more robust user experience.
🔗 Analysis chain
Review isEditing state usage.
The isEditing state is used to disable form inputs, but it's set to false immediately in handleSubmit without waiting for the API response. This could lead to a poor user experience if the update fails.
Consider implementing proper form submission with API integration:
-const [isEditing, setIsEditing] = useState(false);
+const [isSubmitting, setIsSubmitting] = useState(false);🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
echo "=== Searching for handleSubmit usage and context ==="
rg --context 5 "handleSubmit" "netmanager-app/app/(authenticated)/profile/components/MyProfile.tsx"
echo ""
echo "=== Searching for setIsEditing usage and context ==="
rg --context 5 "setIsEditing" "netmanager-app/app/(authenticated)/profile/components/MyProfile.tsx"
Length of output: 1927
| export const activateUserClientApi = async (data: { _id: string; isActive: boolean }) => { | ||
| return axiosInstance | ||
| .post(`${USERS_MGT_URL}/clients/activate/${data._id}`, data) | ||
| .then((response) => response.data); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add return type annotation and error handling.
The function should have proper type annotations and error handling for better type safety and robustness.
-export const activateUserClientApi = async (data: { _id: string; isActive: boolean }) => {
+export const activateUserClientApi = async (data: { _id: string; isActive: boolean }): Promise<{ success: boolean; message: string }> => {
+ try {
return axiosInstance
.post(`${USERS_MGT_URL}/clients/activate/${data._id}`, data)
.then((response) => response.data);
+ } catch (error) {
+ console.error('Error activating client:', 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 activateUserClientApi = async (data: { _id: string; isActive: boolean }) => { | |
| return axiosInstance | |
| .post(`${USERS_MGT_URL}/clients/activate/${data._id}`, data) | |
| .then((response) => response.data); | |
| }; | |
| export const activateUserClientApi = async (data: { _id: string; isActive: boolean }): Promise<{ success: boolean; message: string }> => { | |
| try { | |
| return axiosInstance | |
| .post(`${USERS_MGT_URL}/clients/activate/${data._id}`, data) | |
| .then((response) => response.data); | |
| } catch (error) { | |
| console.error('Error activating client:', error); | |
| throw error; | |
| } | |
| }; |
| const formatDate = (dateString: string | undefined): string => { | ||
| if (!dateString) return "N/A" | ||
| const date = new Date(dateString) | ||
| if (isNaN(date.getTime())) return "Invalid Date" | ||
|
|
||
| const now = new Date() | ||
| const diffTime = date.getTime() - now.getTime() | ||
| const diffDays = Math.ceil(diffTime / (1000 * 60 * 60 * 24)) | ||
|
|
||
| if (diffDays < 0) return "Expired" | ||
| if (diffDays === 0) return "Expires today" | ||
| if (diffDays === 1) return "Expires tomorrow" | ||
| return `Expires in ${diffDays} days` | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use Number.isNaN for safer type checking.
Replace the unsafe isNaN function with Number.isNaN as recommended by static analysis.
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`
}📝 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)
… toast hook import
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
netmanager-app/app/(authenticated)/profile/components/MyProfile.tsx (1)
305-308: Add loading state indicator for form submission.The submit button should show a loading state during form submission.
- <Button type="submit" disabled={isLoading}> - {isLoading ? <Loader2 className="h-4 w-4 animate-spin mr-2" /> : null} + <Button type="submit" disabled={isLoading} className="w-full sm:w-auto"> + {isLoading ? ( + <> + <Loader2 className="h-4 w-4 animate-spin mr-2" /> + Saving... + </> + ) : ( + 'Save Changes' + )} </Button>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
netmanager-app/app/(authenticated)/profile/components/MyProfile.tsx(1 hunks)netmanager-app/components/ui/toaster.tsx(1 hunks)netmanager-app/core/apis/cloudinary.ts(1 hunks)netmanager-app/core/apis/settings.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- netmanager-app/components/ui/toaster.tsx
| export const cloudinaryImageUpload = async (formData: any) => { | ||
| return await fetch(`${cloudinaryUrl}/image/upload`, { | ||
| method: 'POST', | ||
| body: formData, | ||
| }).then((response) => response.json()); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add type safety and error handling to cloudinaryImageUpload.
The function could be improved with proper TypeScript types and error handling.
+interface CloudinaryResponse {
+ secure_url: string;
+ public_id: string;
+}
+
-export const cloudinaryImageUpload = async (formData: any) => {
+export const cloudinaryImageUpload = async (formData: FormData): Promise<CloudinaryResponse> => {
+ if (!process.env.NEXT_PUBLIC_CLOUDINARY_NAME) {
+ throw new Error('Cloudinary configuration is missing');
+ }
+
+ try {
return await fetch(`${cloudinaryUrl}/image/upload`, {
method: 'POST',
body: formData,
}).then((response) => response.json());
+ } catch (error) {
+ console.error('Error uploading to Cloudinary:', error);
+ throw new Error('Failed to upload image');
+ }
};📝 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 cloudinaryImageUpload = async (formData: any) => { | |
| return await fetch(`${cloudinaryUrl}/image/upload`, { | |
| method: 'POST', | |
| body: formData, | |
| }).then((response) => response.json()); | |
| }; | |
| interface CloudinaryResponse { | |
| secure_url: string; | |
| public_id: string; | |
| } | |
| export const cloudinaryImageUpload = async (formData: FormData): Promise<CloudinaryResponse> => { | |
| if (!process.env.NEXT_PUBLIC_CLOUDINARY_NAME) { | |
| throw new Error('Cloudinary configuration is missing'); | |
| } | |
| try { | |
| return await fetch(`${cloudinaryUrl}/image/upload`, { | |
| method: 'POST', | |
| body: formData, | |
| }).then((response) => response.json()); | |
| } catch (error) { | |
| console.error('Error uploading to Cloudinary:', error); | |
| throw new Error('Failed to upload image'); | |
| } | |
| }; |
| 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 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.
🛠️ Refactor suggestion
Add file validation to handleProfileImageUpdate.
The function lacks file size and type validation for image uploads.
+const MAX_FILE_SIZE = 5 * 1024 * 1024; // 5MB
+const ALLOWED_FILE_TYPES = ['image/jpeg', 'image/png', 'image/gif'];
const handleProfileImageUpdate = async (e: React.ChangeEvent<HTMLInputElement>) => {
const file = e.target.files?.[0]
if (!file) return
+ if (!ALLOWED_FILE_TYPES.includes(file.type)) {
+ toast({
+ title: "Error",
+ description: "Please upload a valid image file (JPEG, PNG, or GIF)",
+ });
+ return;
+ }
+
+ if (file.size > MAX_FILE_SIZE) {
+ toast({
+ title: "Error",
+ description: "File size should not exceed 5MB",
+ });
+ return;
+ }
const formData = new FormData()📝 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 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 MAX_FILE_SIZE = 5 * 1024 * 1024; // 5MB | |
| +const ALLOWED_FILE_TYPES = ['image/jpeg', 'image/png', 'image/gif']; | |
| const handleProfileImageUpdate = async (e: React.ChangeEvent<HTMLInputElement>) => { | |
| const file = e.target.files?.[0] | |
| if (!file) return | |
| + if (!ALLOWED_FILE_TYPES.includes(file.type)) { | |
| + toast({ | |
| + title: "Error", | |
| + description: "Please upload a valid image file (JPEG, PNG, or GIF)", | |
| + }); | |
| + return; | |
| + } | |
| + | |
| + if (file.size > MAX_FILE_SIZE) { | |
| + toast({ | |
| + title: "Error", | |
| + description: "File size should not exceed 5MB", | |
| + }); | |
| + 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 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) | |
| } | |
| } |
…indicator; enhance user feedback with loading states and error handling
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.
| 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.
| 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
| 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; | |
| } | |
| } |
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
Release Notes
New Features
Improvements
Dependencies
Technical Enhancements