[Feat]: Add Timesheet Creation Functionality #3417
Conversation
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
apps/web/app/[locale]/timesheet/[memberId]/components/AddTaskModal.tsxOops! Something went wrong! :( ESLint: 8.46.0 ESLint couldn't find the config "next/core-web-vitals" to extend from. Please check that the name of the config is correct. The config "next/core-web-vitals" was referenced from the config file in "/apps/web/.eslintrc.json". If you still have problems, please stop by https://eslint.org/chat/help to chat with the team. WalkthroughThe pull request introduces significant updates across multiple components related to timesheet management in the application. Key changes include enhancements to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (5)
apps/web/lib/features/multiple-select/index.tsx (2)
159-159: Consider using a standardized z-index scaleThe z-index value of 10000 is extremely high and could cause issues with other UI elements. Consider using a standardized z-index scale from your design system.
- <SelectContent className='z-[10000] dark:bg-dark--theme-light w-auto'> + <SelectContent className='z-[100] dark:bg-dark--theme-light w-auto'>
Line range hint
161-167: Consider performance optimization for large option listsFor better performance with large lists, consider:
- Memoizing the options mapping
- Memoizing the renderOption callback
+ const memoizedOptions = React.useMemo(() => + options.map((value) => ( + <SelectItem key={value} value={value}> + {renderOption ? renderOption(value) : value.charAt(0).toUpperCase() + value.slice(1)} + </SelectItem> + )) + , [options, renderOption]); return ( <Select ...> <SelectContent ...> <SelectGroup className={clsxm('overflow-y-auto', classNameGroup)}> - {options.map((value) => ( - <SelectItem key={value} value={value}> - {renderOption ? renderOption(value) : value.charAt(0).toUpperCase() + value.slice(1)} - </SelectItem> - ))} + {memoizedOptions} </SelectGroup> </SelectContent> </Select> );apps/web/app/[locale]/timesheet/[memberId]/components/AddTaskModal.tsx (2)
95-109: Use 'forEach' instead of 'map' for iteration without returnThe
mapfunction is used for side effects without utilizing the returned array. It's clearer and more appropriate to useforEachwhen not using the result ofmap.Apply this diff to make the change:
- formState.shifts.map((shift) => { + formState.shifts.forEach((shift) => {🧰 Tools
🪛 GitHub Check: Cspell
[warning] 104-104:
Unknown word (succes)
104-104: Fix typo in console log messageThe word
'succes'is misspelled. It should be'success'.Apply this diff to fix the typo:
- console.log('succes') + console.log('success')🧰 Tools
🪛 GitHub Check: Cspell
[warning] 104-104:
Unknown word (succes)apps/web/app/[locale]/timesheet/[memberId]/components/EditTaskModal.tsx (1)
127-127: Enhance error message with more contextWhile keeping error messages user-friendly is good, the current message might be too generic. Consider providing more specific error categories to help users understand what went wrong.
- description: "Failed to modify timesheet. Please try again.", + description: timesheetError => { + switch(timesheetError.code) { + case 'INVALID_DATE_RANGE': + return "The selected time range is invalid. Please check your start and end times."; + case 'OVERLAP': + return "This timesheet overlaps with an existing entry. Please adjust the time range."; + default: + return "Failed to modify timesheet. Please try again."; + } + },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
apps/web/app/[locale]/timesheet/[memberId]/components/AddTaskModal.tsx(10 hunks)apps/web/app/[locale]/timesheet/[memberId]/components/CompactTimesheetComponent.tsx(1 hunks)apps/web/app/[locale]/timesheet/[memberId]/components/EditTaskModal.tsx(1 hunks)apps/web/app/[locale]/timesheet/[memberId]/components/TimeSheetFilterPopover.tsx(4 hunks)apps/web/app/hooks/features/useTimesheet.ts(1 hunks)apps/web/app/interfaces/timer/ITimerLog.ts(1 hunks)apps/web/components/ui/sidebar.tsx(1 hunks)apps/web/lib/features/multiple-select/index.tsx(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/web/components/ui/sidebar.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
apps/web/app/[locale]/timesheet/[memberId]/components/TimeSheetFilterPopover.tsx
[error] 150-150: Avoid unnecessary callback in flatMap call.
You can just use flat to flatten the array.
Safe fix: Replace unnecessary flatMap call to flat instead.
(lint/correctness/noFlatMapIdentity)
🪛 GitHub Check: Cspell
apps/web/app/[locale]/timesheet/[memberId]/components/AddTaskModal.tsx
[warning] 104-104:
Unknown word (succes)
🔇 Additional comments (5)
apps/web/lib/features/multiple-select/index.tsx (2)
12-12: LGTM: Clean implementation of dynamic class management
The addition of clsxm utility and its usage for managing dynamic classes in SelectGroup follows best practices for conditional styling in React components.
Also applies to: 160-160
Line range hint 1-170: Verify integration with timesheet creation workflow
Let's verify that these select component changes properly support the timesheet creation feature.
✅ Verification successful
The select components properly support timesheet functionality
The code inspection confirms that both MultipleSelect and CustomSelect components are well-integrated into the timesheet workflow:
- In
EditTaskModal.tsx,CustomSelectis used for status selection with proper status rendering and color coding - In
AddTaskModal.tsx,CustomSelectis used for:- Task selection with proper task info display
- Employee selection with proper name display
- Both implementations include appropriate aria labels and styling for accessibility
The component changes align well with the timesheet creation feature, providing the necessary UI elements for task and employee selection with proper rendering options.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of MultipleSelect and CustomSelect in timesheet-related components
rg -l "MultipleSelect|CustomSelect" | grep -i "timesheet"
# Look for AddTaskModal implementation to verify select integration
ast-grep --pattern 'function AddTaskModal() {
$$$
return (
$$$
<CustomSelect
$$$
/>
$$$
)
}'
Length of output: 323
Script:
#!/bin/bash
# Let's examine the AddTaskModal and EditTaskModal implementations
rg -A 10 "CustomSelect|MultipleSelect" "apps/web/app/[locale]/timesheet/[memberId]/components/AddTaskModal.tsx" "apps/web/app/[locale]/timesheet/[memberId]/components/EditTaskModal.tsx"
# Also check for any task-related types or interfaces
ast-grep --pattern 'type Task = {
$$$
}'
ast-grep --pattern 'interface Task {
$$$
}'
Length of output: 7270
apps/web/app/[locale]/timesheet/[memberId]/components/CompactTimesheetComponent.tsx (1)
26-26: Removal of 'font-bold' from image className
The font-bold class has been removed from the default className of ImageWithLoader. Since font-bold applies to text, removing it from an image element is appropriate.
apps/web/app/interfaces/timer/ITimerLog.ts (1)
140-151: Addition of properties to 'UpdateTimesheet' interface
The properties id, employeeId, organizationId, tenantId, logType, and source have been added to the UpdateTimesheet interface. This provides a more comprehensive set of data for timesheet updates.
apps/web/app/[locale]/timesheet/[memberId]/components/TimeSheetFilterPopover.tsx (1)
62-86: Conditional rendering of employee filter based on 'isManage'
The employee filter is now conditionally rendered based on the isManage flag. This enhances role-based UI rendering and ensures that only managers see the employee selection.
apps/web/app/[locale]/timesheet/[memberId]/components/AddTaskModal.tsx
Outdated
Show resolved
Hide resolved
apps/web/app/[locale]/timesheet/[memberId]/components/TimeSheetFilterPopover.tsx
Outdated
Show resolved
Hide resolved
apps/web/app/[locale]/timesheet/[memberId]/components/EditTaskModal.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (2)
apps/web/app/[locale]/timesheet/[memberId]/components/AddTaskModal.tsx (2)
332-338: Refactor time conversion utilitiesThe time conversion logic is duplicated across the component. Consider extracting these utilities into a separate file for reusability and better maintainability.
// timeUtils.ts export const convertMinutesToTime = (minutes: number): string => { const hours = Math.floor(minutes / 60); const mins = minutes % 60; const period = hours >= 12 ? 'PM' : 'AM'; const formattedHours = hours % 12 || 12; return `${String(formattedHours).padStart(2, '0')}:${String(mins).padStart(2, '0')} ${period}`; }; export const convertTimeToMinutes = (time: string): number => { const [hourMinute, period] = time.split(' '); const [hours, minutes] = hourMinute.split(':').map(Number); let totalMinutes = (hours % 12) * 60 + minutes; if (period === 'PM') totalMinutes += 720; return totalMinutes; };
243-246: Enhance loading and error state UXThe loading state only shows "Processing..." without disabling the button, which could lead to multiple submissions. Also, there's no visual feedback for validation errors.
- {loadingCreateTimesheet && ( - <ReloadIcon className="w-4 h-4 mr-2 animate-spin" /> - )} - {loadingCreateTimesheet ? "Processing..." : t('common.SAVE')} + <div className="flex items-center"> + {loadingCreateTimesheet && ( + <ReloadIcon className="w-4 h-4 mr-2 animate-spin" /> + )} + <span>{loadingCreateTimesheet ? t('common.PROCESSING') : t('common.SAVE')}</span> + </div>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
apps/web/app/[locale]/timesheet/[memberId]/components/AddTaskModal.tsx(10 hunks)apps/web/app/[locale]/timesheet/[memberId]/components/EditTaskModal.tsx(1 hunks)apps/web/app/[locale]/timesheet/[memberId]/components/TimeSheetFilterPopover.tsx(4 hunks)apps/web/app/hooks/features/useTimesheet.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/web/app/[locale]/timesheet/[memberId]/components/EditTaskModal.tsx
- apps/web/app/hooks/features/useTimesheet.ts
🔇 Additional comments (4)
apps/web/app/[locale]/timesheet/[memberId]/components/TimeSheetFilterPopover.tsx (3)
11-11: LGTM!
The import statement is correctly structured and the imported statusTable is properly utilized in the component.
22-22: Consider providing explicit hook parameters
The empty object pattern ({}) as a parameter might hide the hook's configuration options. Consider explicitly declaring the expected parameters, even if they're optional, to improve code maintainability and self-documentation.
Let's check the hook's implementation and usage pattern:
150-152: Verify the shape of status items
The changes look good and align with the previous review comments. However, let's ensure that all status items consistently have the label property to prevent runtime errors.
Let's verify the shape of status items:
✅ Verification successful
Based on the search results, I can see that the statusTable is defined in apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetAction.tsx with the following shape:
const statusTable: { label: TimesheetStatus; description: string }[] = [
{ label: "PENDING", description: "Awaiting approval or review" },
{ label: "IN REVIEW", description: "The item is being reviewed" },
]Let me verify if there are any other usages or definitions of statusTable to ensure consistency:
The status items shape is consistent and safe to use
The statusTable is correctly defined with objects containing label and description properties, where label is of type TimesheetStatus. The usage in TimeSheetFilterPopover.tsx accessing the label property is safe as it's consistently defined across the codebase. The change from flatMap to flat() is also correct since statusTable is already an array of objects with the required shape.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for statusTable definition and usage to verify item shape
ast-grep --pattern 'const statusTable = $value'
# Find any other places where status items are defined or used
rg -A 2 'label.*:.*string'
Length of output: 10783
Script:
#!/bin/bash
# Search for any other definitions or usages of statusTable
rg -A 3 'statusTable.*='
# Search for TimesheetStatus type definition
ast-grep --pattern 'type TimesheetStatus = $value'
Length of output: 1619
apps/web/app/[locale]/timesheet/[memberId]/components/AddTaskModal.tsx (1)
349-353:
Fix critical issue in shift time validation
The current logic sets endTime to startTime + 5 minutes when startTime <= endTime, which is incorrect. This should only happen when creating a new shift or when endTime is before startTime.
- if (startTime <= endTime) {
+ if (startTime >= endTime) {
const startMinutes = convertToMinutesHour(startTime);
updatedShifts[index].endTime = convertMinutesToTime(startMinutes + 5);
return;
}
apps/web/app/[locale]/timesheet/[memberId]/components/TimeSheetFilterPopover.tsx
Show resolved
Hide resolved
apps/web/app/[locale]/timesheet/[memberId]/components/AddTaskModal.tsx
Outdated
Show resolved
Hide resolved
apps/web/app/[locale]/timesheet/[memberId]/components/AddTaskModal.tsx
Outdated
Show resolved
Hide resolved
apps/web/app/[locale]/timesheet/[memberId]/components/AddTaskModal.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
apps/web/app/[locale]/timesheet/[memberId]/components/AddTaskModal.tsx (2)
80-110: Extract UTC date conversion logic into a utility functionThe
handleAddTimesheetfunction is mixing multiple concerns. Consider extracting the UTC date conversion logic into a separate utility function for better maintainability and reusability.+const createUtcDateFromTime = (baseDate: Date, time: string): Date => { + const [hours, minutes] = time.split(':').map(Number); + return new Date(Date.UTC( + baseDate.getFullYear(), + baseDate.getMonth(), + baseDate.getDate(), + hours, + minutes + )); +}; const handleAddTimesheet = async () => { const payload = { isBillable: formState.isBillable, description: formState.notes, projectId: formState.projectId, logType: TimeLogType.MANUAL as any, source: TimerSource.BROWSER as any, taskId: formState.taskId, employeeId: formState.employeeId } - const createUtcDate = (baseDate: Date, time: string): Date => { - const [hours, minutes] = time.split(':').map(Number); - return new Date(Date.UTC(baseDate.getFullYear(), baseDate.getMonth(), baseDate.getDate(), hours, minutes)); - }; try { await Promise.all(formState.shifts.map(async (shift) => { const baseDate = shift.dateFrom instanceof Date ? shift.dateFrom : new Date(shift.dateFrom ?? new Date()); - const startedAt = createUtcDate(baseDate, shift.startTime.toString().slice(0, 5)); - const stoppedAt = createUtcDate(baseDate, shift.endTime.toString().slice(0, 5)); + const startedAt = createUtcDateFromTime(baseDate, shift.startTime.toString().slice(0, 5)); + const stoppedAt = createUtcDateFromTime(baseDate, shift.endTime.toString().slice(0, 5)); await createTimesheet({ ...payload, startedAt, stoppedAt, }); })); closeModal(); } catch (error) { console.error('Failed to create timesheet:', error); } }
349-353: Improve user feedback for shift time validationWhen start time equals end time, the end time is automatically adjusted by 5 minutes without user notification. This might be confusing for users.
Consider:
- Adding a toast notification when auto-adjusting the end time
- Showing validation messages when shifts overlap
- Providing visual feedback for invalid time selections
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
apps/web/app/[locale]/timesheet/[memberId]/components/AddTaskModal.tsx(10 hunks)
🔇 Additional comments (2)
apps/web/app/[locale]/timesheet/[memberId]/components/AddTaskModal.tsx (2)
19-24: Improve type safety in the Shift interface
The dateFrom property uses a union type of Date | string which reduces type safety and requires type checking/conversion in the code.
Consider using Date | null instead, as demonstrated in the previous review comment.
237-245: Add form validation before submission
The save button should be disabled when required fields are empty or invalid.
Consider implementing the form validation as suggested in the previous review comment.
Innocent-Akim
added
feature
WEB
Description
Please include a summary of the changes and the related issue.
Type of Change
Checklist
Previous screenshots
Please add here videos or images of previous status
Current screenshots
Please add here videos or images of previous status
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Improvements
Refactor