Address Sourcery code review: fix employment type badge, refactor Framer Motion, and improve dialog documentation Co-authored-by: Harshit16g <73606353+Harshit16g@users.noreply.github.com> - #5
Open
Harshit16g wants to merge 11 commits into
Conversation
Co-authored-by: Harshit16g <73606353+Harshit16g@users.noreply.github.com>
…ns, and data management Co-authored-by: Harshit16g <73606353+Harshit16g@users.noreply.github.com>
…ync cookies usage Co-authored-by: Harshit16g <73606353+Harshit16g@users.noreply.github.com>
Co-authored-by: Harshit16g <73606353+Harshit16g@users.noreply.github.com>
…improved error handling Co-authored-by: Harshit16g <73606353+Harshit16g@users.noreply.github.com>
Co-authored-by: Harshit16g <73606353+Harshit16g@users.noreply.github.com>
Co-authored-by: Harshit16g <73606353+Harshit16g@users.noreply.github.com>
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
…issues Co-authored-by: Harshit16g <73606353+Harshit16g@users.noreply.github.com>
…mer Motion, and improve dialog documentation Co-authored-by: Harshit16g <73606353+Harshit16g@users.noreply.github.com>
|
Meticulous was unable to execute a test run for this PR because the most recent commit is associated with multiple PRs. To execute a test run, please try pushing up a new commit that is only associated with this PR. Last updated for commit 0912427. This comment will update as new commits are pushed. |
Contributor
Reviewer's GuideThis PR refactors the admin dashboard animations into shared Framer Motion variants, streamlines project and experience management using a reusable DataTable with a CRUD hook, replaces localStorage auth with JWT-based secure cookie flows and rate-limited API routes, introduces a reusable FormDialog and setup documentation, and updates global font loading. Sequence diagram for new admin authentication flow (JWT, API routes, secure cookies)sequenceDiagram
actor AdminUser as Admin User
participant AdminLoginPage as Admin Login Page
participant API_Login as /api/admin/login
participant JWT as JWT Token
participant Cookie as Secure Cookie
participant API_Verify as /api/admin/verify
participant AdminDashboard as Admin Dashboard
AdminUser->>AdminLoginPage: Enter password
AdminLoginPage->>API_Login: POST /api/admin/login {password}
API_Login->>API_Login: Validate password, check rate limit
API_Login->>JWT: Sign JWT if valid
API_Login->>Cookie: Set HTTP-only cookie (admin-token)
API_Login-->>AdminLoginPage: {success: true}
AdminLoginPage->>API_Verify: GET /api/admin/verify (cookie sent)
API_Verify->>JWT: Verify JWT from cookie
API_Verify-->>AdminLoginPage: {authenticated: true}
AdminLoginPage->>AdminDashboard: Render dashboard if authenticated
AdminUser->>AdminDashboard: Click Logout
AdminDashboard->>API_Logout: POST /api/admin/logout
API_Logout->>Cookie: Delete admin-token cookie
API_Logout-->>AdminDashboard: {success: true}
AdminDashboard->>AdminLoginPage: Redirect to login
Class diagram for new shared CRUD and DataTable architectureclassDiagram
class useCrudManager {
+items: T[]
+filteredItems: T[]
+loading: boolean
+searchTerm: string
+selectedItem: T | null
+isCreateDialogOpen: boolean
+isEditDialogOpen: boolean
+formData: any
+setSearchTerm()
+setFormData()
+openCreateDialog()
+openEditDialog(item)
+closeDialogs()
+handleCreate()
+handleUpdate()
+handleDelete(item)
+loadData()
}
class DataTable {
+title: string
+description: string
+data: T[]
+columns: Column<T>[]
+loading: boolean
+searchTerm: string
+onSearchChange(term)
+onAdd()
+addButtonText: string
+renderActions(item)
}
useCrudManager <.. DataTable : provides data & actions
Class diagram for updated AdminAuthContext and providerclassDiagram
class AdminAuthContextType {
+isAuthenticated: boolean
+login(password): Promise<boolean>
+logout(): Promise<void>
+loading: boolean
+error: string | null
}
class AdminAuthProvider {
+isAuthenticated: boolean
+loading: boolean
+error: string | null
+login(password): Promise<boolean>
+logout(): Promise<void>
+checkAuthStatus()
}
AdminAuthProvider --> AdminAuthContextType : provides
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey @Harshit16g - I've reviewed your changes - here's some feedback:
Blocking issues:
- Detected a Generic API Key, potentially exposing access to various services and sensitive operations. (link)
General comments:
- Consider using prefers-reduced-motion or framer-motion’s reduceMotion feature so users who opt-out of animations aren’t overwhelmed by the transitions.
- The DataTable keys each row with
item.id || index, which can lead to unstable or duplicate keys—ensure every item has a unique identifier or throw an error ifidis missing. - For large datasets, debounce or throttle the search input in DataTable (or memoize the filtered results) to avoid running the filter function on every keystroke.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider using prefers-reduced-motion or framer-motion’s reduceMotion feature so users who opt-out of animations aren’t overwhelmed by the transitions.
- The DataTable keys each row with `item.id || index`, which can lead to unstable or duplicate keys—ensure every item has a unique identifier or throw an error if `id` is missing.
- For large datasets, debounce or throttle the search input in DataTable (or memoize the filtered results) to avoid running the filter function on every keystroke.
## Individual Comments
### Comment 1
<location> `components/admin/experiences-management.tsx:36` </location>
<code_context>
+ title: "",
+ company: "",
+ location: "",
+ description: "",
+ start_date: "",
+ end_date: "",
</code_context>
<issue_to_address>
Initial formData omits 'location' field.
Ensure 'location' is included in initialFormData to match expected fields and prevent issues in create/edit dialogs.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
const initialFormData = {
title: "",
company: "",
description: "",
start_date: "",
end_date: "",
is_current: false,
sort_order: 0,
}
=======
const initialFormData = {
title: "",
company: "",
location: "",
description: "",
start_date: "",
end_date: "",
is_current: false,
sort_order: 0,
}
>>>>>>> REPLACE
</suggested_fix>
### Comment 2
<location> `components/admin/experiences-management.tsx:177` </location>
<code_context>
+ {
+ key: 'employment_type',
+ header: 'Type',
+ render: () => (
+ <span className="text-muted-foreground text-sm italic">
+ Not available
</code_context>
<issue_to_address>
Employment type column always displays 'Not available'.
If employment type data is unavailable, consider removing this column to prevent user confusion.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
{
key: 'employment_type',
header: 'Type',
render: () => (
<span className="text-muted-foreground text-sm italic">
Not available
</span>
), // Employment type field doesn't exist in current database schema
},
=======
>>>>>>> REPLACE
</suggested_fix>
### Comment 3
<location> `components/admin/projects-management.tsx:113` </location>
<code_context>
+ )
+
+ // Project image/title renderer
+ const renderProjectTitle = (project: ProjectWithTechnologies) => (
+ <div className="flex items-center gap-2">
+ {project.image_url ? (
</code_context>
<issue_to_address>
Project image alt text may be empty if title is missing.
If project.title is undefined, the alt text becomes 'undefined logo'. Please add a default alt text when the title is missing.
</issue_to_address>
### Comment 4
<location> `components/admin/shared/data-table.tsx:42` </location>
<code_context>
+ addButtonText = "Add Item",
+ renderActions,
+}: DataTableProps<T>) {
+ const getCellValue = (item: T, column: Column<T>) => {
+ if (column.render) {
+ return column.render(item)
</code_context>
<issue_to_address>
getCellValue function may return empty string for falsy values.
Using the nullish coalescing operator (`??`) will return an empty string for values like 0 or false. To preserve these values, check explicitly for `undefined` instead.
Suggested implementation:
```typescript
const getCellValue = (item: T, column: Column<T>) => {
if (column.render) {
const rendered = column.render(item)
return rendered === undefined ? "" : rendered
}
if (typeof column.key === 'string' && column.key.includes('.')) {
// Handle nested properties like 'user.name'
const keys = column.key.split('.')
let value = item
try {
for (const key of keys) {
if (value === null || value === undefined) {
```
```typescript
try {
for (const key of keys) {
if (value === null || value === undefined) {
return ""
}
value = value[key]
}
return value === undefined ? "" : value
} catch {
return ""
}
}
const cellValue = item[column.key as keyof T]
return cellValue === undefined ? "" : cellValue
}
```
</issue_to_address>
### Comment 5
<location> `components/admin/shared/data-table.tsx:139` </location>
<code_context>
+ <AnimatePresence>
+ {data.map((item, index) => (
+ <motion.tr
+ key={item.id || index}
+ initial={{ opacity: 0, y: 20 }}
+ animate={{ opacity: 1, y: 0 }}
</code_context>
<issue_to_address>
Table row key may be unstable if 'id' is missing.
Relying on the index as a fallback key can lead to rendering issues if the list order changes. It's best to ensure each item has a unique 'id'.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
{data.map((item, index) => (
<motion.tr
key={item.id || index}
initial={{ opacity: 0, y: 20 }}
animate={{ opacity: 1, y: 0 }}
exit={{ opacity: 0, y: -20 }}
transition={{ duration: 0.2, delay: index * 0.05 }}
className="border-b"
>
=======
{data.map((item, index) => {
if (!item.id) {
console.warn('DataTable: Each item in data should have a unique "id" property.', item);
}
return (
<motion.tr
key={item.id}
initial={{ opacity: 0, y: 20 }}
animate={{ opacity: 1, y: 0 }}
exit={{ opacity: 0, y: -20 }}
transition={{ duration: 0.2, delay: index * 0.05 }}
className="border-b"
>
);
})}
>>>>>>> REPLACE
</suggested_fix>
## Security Issues
### Issue 1
<location> `ADMIN_SETUP.md:16` </location>
<issue_to_address>
**security (generic-api-key):** Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
*Source: gitleaks*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+33
to
+41
| const initialFormData = { | ||
| title: "", | ||
| company: "", | ||
| description: "", | ||
| start_date: "", | ||
| end_date: "", | ||
| is_current: false, | ||
| sort_order: 0, | ||
| } |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): Initial formData omits 'location' field.
Ensure 'location' is included in initialFormData to match expected fields and prevent issues in create/edit dialogs.
Suggested change
| const initialFormData = { | |
| title: "", | |
| company: "", | |
| description: "", | |
| start_date: "", | |
| end_date: "", | |
| is_current: false, | |
| sort_order: 0, | |
| } | |
| const initialFormData = { | |
| title: "", | |
| company: "", | |
| location: "", | |
| description: "", | |
| start_date: "", | |
| end_date: "", | |
| is_current: false, | |
| sort_order: 0, | |
| } |
Comment on lines
+174
to
+182
| { | ||
| key: 'employment_type', | ||
| header: 'Type', | ||
| render: () => ( | ||
| <span className="text-muted-foreground text-sm italic"> | ||
| Not available | ||
| </span> | ||
| ), // Employment type field doesn't exist in current database schema | ||
| }, |
Contributor
There was a problem hiding this comment.
suggestion: Employment type column always displays 'Not available'.
If employment type data is unavailable, consider removing this column to prevent user confusion.
Suggested change
| { | |
| key: 'employment_type', | |
| header: 'Type', | |
| render: () => ( | |
| <span className="text-muted-foreground text-sm italic"> | |
| Not available | |
| </span> | |
| ), // Employment type field doesn't exist in current database schema | |
| }, |
| ) | ||
|
|
||
| // Project image/title renderer | ||
| const renderProjectTitle = (project: ProjectWithTechnologies) => ( |
Contributor
There was a problem hiding this comment.
nitpick (bug_risk): Project image alt text may be empty if title is missing.
If project.title is undefined, the alt text becomes 'undefined logo'. Please add a default alt text when the title is missing.
| addButtonText = "Add Item", | ||
| renderActions, | ||
| }: DataTableProps<T>) { | ||
| const getCellValue = (item: T, column: Column<T>) => { |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): getCellValue function may return empty string for falsy values.
Using the nullish coalescing operator (??) will return an empty string for values like 0 or false. To preserve these values, check explicitly for undefined instead.
Suggested implementation:
const getCellValue = (item: T, column: Column<T>) => {
if (column.render) {
const rendered = column.render(item)
return rendered === undefined ? "" : rendered
}
if (typeof column.key === 'string' && column.key.includes('.')) {
// Handle nested properties like 'user.name'
const keys = column.key.split('.')
let value = item
try {
for (const key of keys) {
if (value === null || value === undefined) { try {
for (const key of keys) {
if (value === null || value === undefined) {
return ""
}
value = value[key]
}
return value === undefined ? "" : value
} catch {
return ""
}
}
const cellValue = item[column.key as keyof T]
return cellValue === undefined ? "" : cellValue
}
Comment on lines
+137
to
+145
| {data.map((item, index) => ( | ||
| <motion.tr | ||
| key={item.id || index} | ||
| initial={{ opacity: 0, y: 20 }} | ||
| animate={{ opacity: 1, y: 0 }} | ||
| exit={{ opacity: 0, y: -20 }} | ||
| transition={{ duration: 0.2, delay: index * 0.05 }} | ||
| className="border-b" | ||
| > |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): Table row key may be unstable if 'id' is missing.
Relying on the index as a fallback key can lead to rendering issues if the list order changes. It's best to ensure each item has a unique 'id'.
Suggested change
| {data.map((item, index) => ( | |
| <motion.tr | |
| key={item.id || index} | |
| initial={{ opacity: 0, y: 20 }} | |
| animate={{ opacity: 1, y: 0 }} | |
| exit={{ opacity: 0, y: -20 }} | |
| transition={{ duration: 0.2, delay: index * 0.05 }} | |
| className="border-b" | |
| > | |
| {data.map((item, index) => { | |
| if (!item.id) { | |
| console.warn('DataTable: Each item in data should have a unique "id" property.', item); | |
| } | |
| return ( | |
| <motion.tr | |
| key={item.id} | |
| initial={{ opacity: 0, y: 20 }} | |
| animate={{ opacity: 1, y: 0 }} | |
| exit={{ opacity: 0, y: -20 }} | |
| transition={{ duration: 0.2, delay: index * 0.05 }} | |
| className="border-b" | |
| > | |
| ); | |
| })} |
| - **Purpose**: Secret key for signing JWT tokens (authentication) | ||
| - **Required**: Yes | ||
| - **Generate with**: `openssl rand -base64 32` | ||
| - **Example**: `ADMIN_SECRET_KEY=6Q7VdKxvuUqnP8YrL2+m5AzWj9nE3Fg1HsKpRtYoL7X=` |
Contributor
There was a problem hiding this comment.
security (generic-api-key): Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
Source: gitleaks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Standardize motion animations, consolidate CRUD management UIs, secure admin authentication via JWT, fix badge styling, and improve dialog and setup documentation
New Features:
Bug Fixes:
Enhancements:
Documentation: