rename employees route, and fix employee details styling#216
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
WalkthroughThis pull request refactors various parts of the application by shifting terminology and URL paths from "people" to "employees." It updates revalidation paths, navigation links, and localization keys, while introducing new actions and React components for updating employee details, department, and status. Additionally, obsolete components and pages under the "people" namespace have been removed and replaced with their "employees" counterparts. Authentication, input validation, and revalidation flows are maintained with minor adjustments. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI
participant Action
participant DB
participant Cache
User->>UI: Initiate update (e.g., detail or status change)
UI->>Action: Invoke updateEmployeeDetails/updateEmployeeStatus
Action->>DB: Validate session & fetch employee data
DB-->>Action: Return employee data or error
Action->>DB: Update employee record (if applicable)
DB-->>Action: Acknowledge update
Action->>Cache: Trigger revalidation of paths
Action-->>UI: Return success or error response
sequenceDiagram
participant User
participant DashboardPage
participant Auth
participant Router
User->>DashboardPage: Request dashboard
DashboardPage->>Auth: Retrieve session details
Auth-->>DashboardPage: Return organizationId (or missing)
alt organizationId present
DashboardPage->>Router: Redirect to "/<orgId>/overview"
else organizationId missing
DashboardPage->>Router: Redirect to "/"
end
Possibly related PRs
Poem
✨ Finishing Touches
🪧 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: 5
🧹 Nitpick comments (14)
apps/app/src/components/tables/people/empty-states.tsx (1)
38-38: Router path updated correctly, but consider additional consistency improvementsThe navigation path has been updated from
peopletoemployeesas expected, but there are several inconsistencies that could be addressed:
- The file itself is still in a
peopledirectory (components/tables/people/)- Translation keys still use the
peopleprefix (e.g.,people.empty.no_results.title)- Component names haven't been updated (e.g.,
NoEmployeeshas always been using the term "employees")Consider updating these for complete consistency in the renaming effort.
apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/employees/layout.tsx (1)
20-23: Menu paths updated correctly, but translation keys could be updated tooThe navigation paths have been properly updated from
peopletoemployees. However, the translation keys in lines 21 and 23 still use thepeopleprefix:label: t("people.dashboard.title")label: t("people.all")Consider updating these translation keys to use an
employeesprefix for complete consistency in the renaming effort.apps/app/src/components/sheets/invite-user-sheet.tsx (1)
3-3: Import path updated but translation keys remain unchanged.The import path has been correctly updated to use "employees" instead of "people", which aligns with the PR objective. However, there are multiple translation keys throughout the component (lines 61, 76, 77, 84, 87, 95, 98, 108, 116, 137, 138) that still use the "people" namespace. Consider updating these translation keys to maintain consistency between code structure and UI text.
apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/employees/[employeeId]/actions/update-employee-status.ts (1)
9-9: Update import path to align with "employees" naming convention.The import on line 9 still references the "people" namespace:
import type { EmployeeStatusType } from "@/components/tables/people/employee-status";For consistency with the PR's goal of renaming from "people" to "employees", consider updating this import path.
apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/employees/[employeeId]/components/EmployeeDetails.tsx (4)
50-78: Consider refactoring state management and change detection logic.The component initializes multiple state variables and uses multiple useEffect hooks to track changes. This could be simplified to improve readability and maintainability.
Consider combining related state variables into a single object and using a reducer pattern:
- const [department, setDepartment] = useState<Departments | null>(null); - const [status, setStatus] = useState<EmployeeStatusType | null>(null); - const [hasChanges, setHasChanges] = useState(false); + const [state, dispatch] = useReducer(reducer, { + department: null as Departments | null, + status: null as EmployeeStatusType | null, + hasChanges: false + });This would simplify the change tracking logic and reduce the number of re-renders.
133-162: The save handler could be simplified and better error handling added.The current implementation has multiple nested conditions and could be streamlined for clarity.
Consider this revised implementation:
const handleSave = async () => { setIsSaving(true); try { // Prepare update data const updateData = { employeeId }; - // Only include changed fields - if (department && department !== employee.department) { - updateData.department = department; - } - - if (status) { - const isActive = status === "active"; - if (isActive !== employee.isActive) { - updateData.isActive = isActive; - } - } + // Only include changed fields using spread operator + if (department && department !== employee.department) { + Object.assign(updateData, { department }); + } + + if (status) { + const isActive = status === "active"; + if (isActive !== employee.isActive) { + Object.assign(updateData, { isActive }); + } + } // Execute the update await execute(updateData); } catch (error) { - toast.error("Failed to update employee details"); + console.error("Save error:", error); + toast.error(error instanceof Error ? error.message : "Failed to update employee details"); } finally { setIsSaving(false); } };
283-298: Implement the Remind button functionality.The Remind button doesn't appear to have any functionality implemented.
Would you like me to suggest an implementation for the Remind button functionality for employee tasks?
280-302: Missing empty state for tasks list.There's no UI handling for when the tasks array is empty, which could lead to a confusing user experience.
Add an empty state message when there are no tasks:
<CardContent className="flex flex-col gap-2"> + {tasks.length === 0 && ( + <div className="text-center p-4 text-muted-foreground"> + No tasks assigned to this employee. + </div> + )} {tasks.map((task) => { const isCompleted = task.status === "completed"; return ( // ... existing code ); })} </CardContent>apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/employees/[employeeId]/actions/update-employee.ts (2)
44-57: Consider adding a more specific error message for employee not found.The current error message is generic. Since you have access to the employeeId, consider adding it to the error message.
if (!employee) { return { success: false, - error: appErrors.NOT_FOUND, + error: { + ...appErrors.NOT_FOUND, + message: `Employee with ID ${employeeId} not found` + }, }; }
95-101: Consider adding more detailed error logging.The current error logging is minimal. It would be helpful to add more details for debugging purposes.
} catch (error) { - console.error("Error updating employee:", error); + console.error(`Error updating employee ${employeeId}:`, error); + if (error instanceof Error) { + console.error("Stack trace:", error.stack); + } return { success: false, error: appErrors.UNEXPECTED_ERROR, }; }apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/employees/[employeeId]/components/EditableDepartment.tsx (1)
50-53: Save handler doesn't check if department has changed.The save handler executes even if the department hasn't changed, which could lead to unnecessary API calls.
const handleSave = () => { + if (department === currentDepartment) { + return; + } execute({ employeeId, department }); };apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/employees/[employeeId]/components/EditableStatus.tsx (3)
11-11: Unused import detected.The
Buttoncomponent is imported but never used in this component. Consider removing this unused import to maintain clean code.-import { Button } from "@bubba/ui/button";
16-16: Unused import detected.The
EMPLOYEE_STATUS_TYPESis imported but never utilized in this component.-import { - EMPLOYEE_STATUS_TYPES, - getEmployeeStatusFromBoolean, -} from "@/components/tables/people/employee-status"; +import { + getEmployeeStatusFromBoolean, +} from "@/components/tables/people/employee-status";
40-50: Consider adding loading state feedback.The action status is captured but not utilized to provide visual feedback to users. Consider disabling the UI or showing a loading indicator during the update process.
const { execute, status: actionStatus } = useAction(updateEmployeeStatus, { onSuccess: () => { toast.success("Employee status updated successfully"); onSuccess?.(); }, onError: (error) => { toast.error( error?.error?.serverError || "Failed to update employee status", ); }, });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/employees/[employeeId]/actions/update-department.ts(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/employees/[employeeId]/actions/update-employee-details.ts(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/employees/[employeeId]/actions/update-employee-status.ts(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/employees/[employeeId]/actions/update-employee.ts(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/employees/[employeeId]/components/EditableDepartment.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/employees/[employeeId]/components/EditableDetails.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/employees/[employeeId]/components/EditableStatus.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/employees/[employeeId]/components/EmployeeDetails.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/employees/[employeeId]/page.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/employees/all/components/table/EmployeesTable.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/employees/layout.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/page.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/people/[employeeId]/components/EditableDepartment.tsx(0 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/people/[employeeId]/components/EmployeeDetails.tsx(0 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/people/[employeeId]/page.tsx(0 hunks)apps/app/src/components/main-menu.tsx(1 hunks)apps/app/src/components/sheets/invite-user-sheet.tsx(1 hunks)apps/app/src/components/tables/people/data-table.tsx(1 hunks)apps/app/src/components/tables/people/empty-states.tsx(1 hunks)apps/app/src/locales/layout/sidebar.ts(1 hunks)
💤 Files with no reviewable changes (3)
- apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/people/[employeeId]/components/EditableDepartment.tsx
- apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/people/[employeeId]/components/EmployeeDetails.tsx
- apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/people/[employeeId]/page.tsx
🧰 Additional context used
🧬 Code Definitions (3)
apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/employees/[employeeId]/actions/update-employee.ts (2)
apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/employees/[employeeId]/types/index.ts (1)
appErrors(45-58)apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/employees/types.ts (1)
appErrors(8-17)
apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/employees/[employeeId]/components/EditableStatus.tsx (1)
apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/employees/[employeeId]/actions/update-employee-status.ts (1)
updateEmployeeStatus(16-84)
apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/employees/[employeeId]/components/EditableDepartment.tsx (1)
apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/employees/[employeeId]/actions/update-department.ts (1)
updateEmployeeDepartment(18-80)
🔇 Additional comments (26)
apps/app/src/locales/layout/sidebar.ts (1)
9-11: Terminology update looks good!The change from "people" to "employees" is consistent with the PR objective to rename the route. The semicolon addition also improves code style consistency.
apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/employees/all/components/table/EmployeesTable.tsx (1)
30-30: Router path update looks good!The navigation path has been correctly updated from
peopletoemployeesto maintain consistency with the route renaming.apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/page.tsx (1)
1-13: Appropriate dashboard page implementation with redirection logic.The implementation of this page component correctly handles authentication and redirection. It first verifies if the user has an associated organization and redirects accordingly.
apps/app/src/components/tables/people/data-table.tsx (1)
67-67:Details
❓ Verification inconclusive
URL path updated to match new routes.
The Link component's URL has been correctly updated from "people" to "employees" to align with the route renaming in this PR.
Consider reviewing if this component should also be moved from the
tables/peopledirectory to a more appropriately named location to maintain consistency with the new "employees" terminology:
🏁 Script executed:
#!/bin/bash # Check the current file structure to see if there are other components that might need renaming find apps/app/src/components -type d -name "people" | grep -v "node_modules"Length of output: 112
Verified URL update – please review directory naming consistency.
The updated Link now correctly navigates using the new
/employees/route. However, note that the component remains in theapps/app/src/components/tables/peopledirectory. For consistency with the new naming convention, please consider relocating this component (and any related files) to a directory that reflects the "employees" terminology (e.g., renamingtables/peopletotables/employees).apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/employees/[employeeId]/actions/update-department.ts (1)
66-67: Revalidation paths updated to match new routes.The revalidation paths have been correctly updated from "people" to "employees" to maintain consistency with the route renaming.
apps/app/src/components/main-menu.tsx (1)
224-227:Details
✅ Verification successful
Menu item correctly updated to reflect new terminology.
The menu item has been appropriately updated from "people" to "employees" with the corresponding path and translation key changes.
Notice that there's a typo in the icon name on line 228 (
Icons.Peolpleinstead ofIcons.People), though this is pre-existing code:
🏁 Script executed:
#!/bin/bash # Check if the misspelled icon definition exists elsewhere grep -r "Peolple" --include="*.tsx" --include="*.ts" apps/Length of output: 182
Approved: Menu item updates are correct.
- The menu item now correctly uses "employees" with the updated path (
/:organizationId/employees/dashboard) and translation key.- The shell script confirms the typo in the icon name (
Icons.Peolple) on line 228. Since this appears to be pre-existing code, it need not be addressed in this update.apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/employees/[employeeId]/actions/update-employee-status.ts (1)
69-70: Revalidation paths correctly use "employees" route.The revalidation paths have been properly updated to use the new "employees" route naming convention, which aligns with the PR objectives.
apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/employees/[employeeId]/actions/update-employee-details.ts (1)
1-85: Well-implemented server action for updating employee details.This server action is well-structured with:
- Strong input validation using Zod schema
- Proper authentication and authorization checks
- Appropriate error handling for various scenarios
- Consistent revalidation paths using the new "employees" naming convention
The implementation follows best practices for Next.js server actions and properly integrates with the rest of the application.
apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/employees/[employeeId]/components/EmployeeDetails.tsx (7)
1-30: Client component with necessary imports looks good.The imports are well-organized and the component is correctly marked as a client component with the "use client" directive at the top of the file.
31-44: Constants for select options are well-structured.Defining the DEPARTMENTS and STATUS_OPTIONS constants outside the component is a good practice for reusability and preventing unnecessary re-renders.
46-48: Props interface is properly defined.The interface specifies the required employeeId property with the correct type.
80-91: Action hook implementation looks good.The useAction hook is properly implemented with appropriate success and error handling callbacks.
93-109: Error handling is thorough and user-friendly.The error handling includes both redirection for specific error codes and a user-friendly error message display.
111-119: Loading state is properly implemented with skeletons.Using skeleton loaders provides a good user experience during data loading.
125-131: Handler functions are clear and focused.These handler functions are appropriately named and serve a single purpose each.
apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/employees/[employeeId]/actions/update-employee.ts (4)
1-16: Server action setup and schema validation look good.The server action is properly set up with "use server" directive, and the schema validation with zod is well-defined.
17-42: Action definition and auth check are correctly implemented.The server action is properly configured with metadata for tracking and has appropriate authentication checks.
59-77: Good practice to check if changes are needed before updating.Checking if there are any changes before performing the update is an efficient approach that avoids unnecessary database operations.
79-95: Database update and path revalidation is properly implemented.The database update operation includes the necessary constraints and the paths are revalidated correctly.
apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/employees/[employeeId]/page.tsx (2)
1-7: Imports are clean and focused.The file imports only the necessary dependencies and components.
8-24: Page component is well-structured and secure.The component correctly handles authentication and redirects unauthorized users. It also properly handles locale settings.
apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/employees/[employeeId]/components/EditableDepartment.tsx (3)
1-26: Component setup and constants look good.The component is properly marked as a client component and the DEPARTMENTS constant is well-defined.
27-32: Props interface is properly defined.The interface clearly specifies required and optional properties with the correct types.
33-49: State and action hook are correctly implemented.The component correctly initializes state from props and the useAction hook has appropriate callbacks.
apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/employees/[employeeId]/components/EditableStatus.tsx (2)
25-30: LGTM! Well-structured props interface.The props interface is clear and concise with appropriate types.
21-24: LGTM! Good implementation of status options.The status options are well-defined as an array of objects with value and label properties.
| "use client"; | ||
|
|
||
| import { useState } from "react"; | ||
| import { Button } from "@bubba/ui/button"; | ||
| import { toast } from "sonner"; | ||
| import { useAction } from "next-safe-action/hooks"; | ||
| import { updateEmployeeDetails } from "../actions/update-employee-details"; | ||
|
|
||
| interface EditableDetailsProps { | ||
| employeeId: string; | ||
| currentName: string; | ||
| currentEmail: string; | ||
| onSuccess?: () => void; | ||
| } | ||
|
|
||
| export function EditableDetails({ | ||
| employeeId, | ||
| currentName, | ||
| currentEmail, | ||
| }: EditableDetailsProps) { | ||
| return ( | ||
| <div className="flex flex-col gap-4"> | ||
| <div className="flex flex-col gap-1.5"> | ||
| <span className="text-sm font-medium">Name</span> | ||
| <span className="text-sm">{currentName}</span> | ||
| </div> | ||
| <div className="flex flex-col gap-1.5"> | ||
| <span className="text-sm font-medium">Email</span> | ||
| <span className="text-sm">{currentEmail}</span> | ||
| </div> | ||
| </div> | ||
| ); | ||
| } |
There was a problem hiding this comment.
Component name "EditableDetails" doesn't match its current implementation.
This component is named "EditableDetails" but doesn't actually provide any editing functionality. It only displays the employee's name and email in a read-only format. There are several unused imports that suggest editing functionality was intended:
useStatefrom ReactButtonfrom UItoastfor notificationsuseActionfor handling the update actionupdateEmployeeDetailsactiononSuccessprop that's defined but not used
Either implement the editing functionality using these imports or rename the component to something like "EmployeeDetails" to better reflect its current read-only behavior.
| export async function generateMetadata({ | ||
| params, | ||
| }: { | ||
| params: Promise<{ locale: string; employeeId: string }>; | ||
| }): Promise<Metadata> { | ||
| const { locale } = await params; | ||
|
|
||
| setStaticParamsLocale(locale); | ||
| const t = await getI18n(); | ||
|
|
||
| return { | ||
| title: t("people.details.title"), | ||
| }; | ||
| } |
There was a problem hiding this comment.
Fix inconsistency in i18n key naming.
The title uses people.details.title but should be updated to employees.details.title to maintain consistency with the route change from "people" to "employees".
return {
- title: t("people.details.title"),
+ title: t("employees.details.title"),
};This change is needed to maintain consistency with the route change from "people" to "employees" as mentioned in the PR objectives.
📝 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 async function generateMetadata({ | |
| params, | |
| }: { | |
| params: Promise<{ locale: string; employeeId: string }>; | |
| }): Promise<Metadata> { | |
| const { locale } = await params; | |
| setStaticParamsLocale(locale); | |
| const t = await getI18n(); | |
| return { | |
| title: t("people.details.title"), | |
| }; | |
| } | |
| export async function generateMetadata({ | |
| params, | |
| }: { | |
| params: Promise<{ locale: string; employeeId: string }>; | |
| }): Promise<Metadata> { | |
| const { locale } = await params; | |
| setStaticParamsLocale(locale); | |
| const t = await getI18n(); | |
| return { | |
| title: t("employees.details.title"), | |
| }; | |
| } |
| return ( | ||
| <div> | ||
| <Select | ||
| value={department} | ||
| onValueChange={(value) => setDepartment(value as Departments)} | ||
| > | ||
| <SelectTrigger className="h-8 w-full"> | ||
| <SelectValue placeholder="Select department" /> | ||
| </SelectTrigger> | ||
| <SelectContent> | ||
| {DEPARTMENTS.map((dept) => ( | ||
| <SelectItem key={dept.value} value={dept.value}> | ||
| {dept.label} | ||
| </SelectItem> | ||
| ))} | ||
| </SelectContent> | ||
| </Select> | ||
| </div> | ||
| ); |
There was a problem hiding this comment.
The Save button is missing from the UI.
The component defines a handleSave function but doesn't include a button to trigger it, making it impossible for users to save their changes.
return (
<div>
<Select
value={department}
onValueChange={(value) => setDepartment(value as Departments)}
>
<SelectTrigger className="h-8 w-full">
<SelectValue placeholder="Select department" />
</SelectTrigger>
<SelectContent>
{DEPARTMENTS.map((dept) => (
<SelectItem key={dept.value} value={dept.value}>
{dept.label}
</SelectItem>
))}
</SelectContent>
</Select>
+ <Button
+ onClick={handleSave}
+ disabled={department === currentDepartment || status === 'loading'}
+ className="mt-2"
+ size="sm"
+ >
+ {status === 'loading' ? 'Saving...' : 'Save'}
+ </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.
| return ( | |
| <div> | |
| <Select | |
| value={department} | |
| onValueChange={(value) => setDepartment(value as Departments)} | |
| > | |
| <SelectTrigger className="h-8 w-full"> | |
| <SelectValue placeholder="Select department" /> | |
| </SelectTrigger> | |
| <SelectContent> | |
| {DEPARTMENTS.map((dept) => ( | |
| <SelectItem key={dept.value} value={dept.value}> | |
| {dept.label} | |
| </SelectItem> | |
| ))} | |
| </SelectContent> | |
| </Select> | |
| </div> | |
| ); | |
| return ( | |
| <div> | |
| <Select | |
| value={department} | |
| onValueChange={(value) => setDepartment(value as Departments)} | |
| > | |
| <SelectTrigger className="h-8 w-full"> | |
| <SelectValue placeholder="Select department" /> | |
| </SelectTrigger> | |
| <SelectContent> | |
| {DEPARTMENTS.map((dept) => ( | |
| <SelectItem key={dept.value} value={dept.value}> | |
| {dept.label} | |
| </SelectItem> | |
| ))} | |
| </SelectContent> | |
| </Select> | |
| <Button | |
| onClick={handleSave} | |
| disabled={department === currentDepartment || status === 'loading'} | |
| className="mt-2" | |
| size="sm" | |
| > | |
| {status === 'loading' ? 'Saving...' : 'Save'} | |
| </Button> | |
| </div> | |
| ); |
| const handleSave = () => { | ||
| const isActive = status === "active"; | ||
| execute({ employeeId, isActive }); | ||
| }; |
There was a problem hiding this comment.
The handleSave function is defined but never called.
There is no UI element (like a button) to trigger the handleSave function, making it impossible for users to submit status changes.
Consider adding a save button after the Select component:
return (
<div>
<Select
value={status}
onValueChange={(value) => setStatus(value as EmployeeStatusType)}
>
<SelectTrigger className="h-8 w-full">
<SelectValue placeholder="Select status" />
</SelectTrigger>
<SelectContent>
{STATUS_OPTIONS.map((option) => (
<SelectItem key={option.value} value={option.value}>
{option.label}
</SelectItem>
))}
</SelectContent>
</Select>
+ <Button
+ onClick={handleSave}
+ disabled={actionStatus === "executing" || status === initialStatus}
+ className="mt-2"
+ size="sm"
+ >
+ Save
+ </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.
| const handleSave = () => { | |
| const isActive = status === "active"; | |
| execute({ employeeId, isActive }); | |
| }; | |
| return ( | |
| <div> | |
| <Select | |
| value={status} | |
| onValueChange={(value) => setStatus(value as EmployeeStatusType)} | |
| > | |
| <SelectTrigger className="h-8 w-full"> | |
| <SelectValue placeholder="Select status" /> | |
| </SelectTrigger> | |
| <SelectContent> | |
| {STATUS_OPTIONS.map((option) => ( | |
| <SelectItem key={option.value} value={option.value}> | |
| {option.label} | |
| </SelectItem> | |
| ))} | |
| </SelectContent> | |
| </Select> | |
| <Button | |
| onClick={handleSave} | |
| disabled={actionStatus === "executing" || status === initialStatus} | |
| className="mt-2" | |
| size="sm" | |
| > | |
| Save | |
| </Button> | |
| </div> | |
| ); |
| return ( | ||
| <div> | ||
| <Select | ||
| value={status} | ||
| onValueChange={(value) => setStatus(value as EmployeeStatusType)} | ||
| > | ||
| <SelectTrigger className="h-8 w-full"> | ||
| <SelectValue placeholder="Select status" /> | ||
| </SelectTrigger> | ||
| <SelectContent> | ||
| {STATUS_OPTIONS.map((option) => ( | ||
| <SelectItem key={option.value} value={option.value}> | ||
| {option.label} | ||
| </SelectItem> | ||
| ))} | ||
| </SelectContent> | ||
| </Select> | ||
| </div> | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider auto-saving or adding save functionality.
Currently, this component allows users to select a new status but doesn't provide any way to save the change. Either implement auto-saving on selection change or add a save button to trigger the handleSave function.
Option 1: Implement auto-save on selection change:
<Select
value={status}
- onValueChange={(value) => setStatus(value as EmployeeStatusType)}
+ onValueChange={(value) => {
+ const newStatus = value as EmployeeStatusType;
+ setStatus(newStatus);
+ if (newStatus !== initialStatus) {
+ const isActive = newStatus === "active";
+ execute({ employeeId, isActive });
+ }
+ }}
>Or Option 2: Add a save button below the dropdown (as shown in the previous comment).
📝 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.
| return ( | |
| <div> | |
| <Select | |
| value={status} | |
| onValueChange={(value) => setStatus(value as EmployeeStatusType)} | |
| > | |
| <SelectTrigger className="h-8 w-full"> | |
| <SelectValue placeholder="Select status" /> | |
| </SelectTrigger> | |
| <SelectContent> | |
| {STATUS_OPTIONS.map((option) => ( | |
| <SelectItem key={option.value} value={option.value}> | |
| {option.label} | |
| </SelectItem> | |
| ))} | |
| </SelectContent> | |
| </Select> | |
| </div> | |
| ); | |
| return ( | |
| <div> | |
| <Select | |
| value={status} | |
| onValueChange={(value) => { | |
| const newStatus = value as EmployeeStatusType; | |
| setStatus(newStatus); | |
| if (newStatus !== initialStatus) { | |
| const isActive = newStatus === "active"; | |
| execute({ employeeId, isActive }); | |
| } | |
| }} | |
| > | |
| <SelectTrigger className="h-8 w-full"> | |
| <SelectValue placeholder="Select status" /> | |
| </SelectTrigger> | |
| <SelectContent> | |
| {STATUS_OPTIONS.map((option) => ( | |
| <SelectItem key={option.value} value={option.value}> | |
| {option.label} | |
| </SelectItem> | |
| ))} | |
| </SelectContent> | |
| </Select> | |
| </div> | |
| ); |
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/employees/all/page.tsx (1)
6-6: 🛠️ Refactor suggestionFunction name should be updated to match the new terminology.
The function is still named
AllPeoplePagewhile the file path and component it renders (EmployeesList) use the "employees" terminology. This is inconsistent with the PR's objective of renaming from "people" to "employees".Consider updating the function name for consistency:
-export default async function AllPeoplePage({ +export default async function AllEmployeesPage({
🧹 Nitpick comments (1)
apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/employees/dashboard/components/EmployeesOverview.tsx (1)
49-56: Clear filtering logic with helpful todo comment.The filtering logic clearly communicates the intent to only include active employees that exist in both tables. The TODO comment provides important context about this being a temporary solution, which is helpful for future maintainers.
Consider adding a reference to an issue or ticket number in the TODO comment to make it easier to track when this technical debt will be addressed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/employees/all/page.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/employees/dashboard/components/EmployeesOverview.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/employees/dashboard/page.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/employees/dashboard/page.tsx
🔇 Additional comments (5)
apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/employees/all/page.tsx (1)
28-28: LGTM! Translation key properly updated.The metadata title has been properly updated from "sidebar.people" to "sidebar.employees", which aligns with the PR objective of renaming from "people" to "employees".
apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/employees/dashboard/components/EmployeesOverview.tsx (4)
29-33: Variable renamed for clarity.Good change - renaming to
portalEmployeesbetter reflects that these users come from the portal user table. This makes the code more maintainable and easier to understand, especially in context of the subsequent changes.
35-42: Enhanced data filtering for employee records.This additional query improves data integrity by ensuring that employees returned by the component are actually present in both the portal user and employee tables. This is a good data validation approach.
44-47: Efficient status lookup with map implementation.Creating a map for quick lookups is a good performance optimization when you need to repeatedly check employee status. This approach is more efficient than filtering arrays multiple times.
58-58: Return value updated to match new filtering logic.The return value now correctly reflects the filtered active portal employees rather than all employees, which aligns with the component's purpose.
1 participant
Summary by CodeRabbit
New Features
Refactor