[Feat]: Improve AddManuel with form handling and error check #3426
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. WalkthroughThis pull request introduces modifications across several files related to timesheet management and UI components. The changes primarily focus on enhancing form state management, date handling, and time-related functionalities. Key updates include a new Changes
Sequence DiagramsequenceDiagram
participant User
participant AddTaskModal
participant DateHelper
participant TimelogFilterOptions
User->>AddTaskModal: Open Modal
AddTaskModal->>DateHelper: Convert Dates
AddTaskModal->>TimelogFilterOptions: Generate Time Options
User->>AddTaskModal: Fill Form
AddTaskModal->>AddTaskModal: Validate Shifts
AddTaskModal->>AddTaskModal: Create Timesheet Entry
Possibly related PRs
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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 (
|
Innocent-Akim
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (4)
apps/web/lib/features/integrations/calendar/table-time-sheet.tsx (1)
Line range hint
86-99: Handle No Selection inhandleButtonClickFunctionIn the
Approvedcase ofhandleButtonClick, no action is taken ifselectTimesheetId.lengthis zero. Consider providing user feedback or disabling the action buttons when no timesheets are selected to enhance user experience.Apply this diff to provide user feedback:
case 'Approved': if (selectTimesheetId.length > 0) { await updateTimesheetStatus({ status: 'APPROVED', ids: selectTimesheetId }); + } else { + toast({ + title: 'No Timesheets Selected', + description: 'Please select at least one timesheet to approve.', + variant: 'warning', + }); } break;apps/web/app/[locale]/timesheet/[memberId]/components/AddTaskModal.tsx (3)
185-187: Ensure Readability with Proper Theming inCustomSelectThe
classNameGroup='max-h-[40vh] !text-white 'andclassName='w-full font-medium text-white'additions ensure that text is visible in dark mode. Consider verifying that these class names align with your theming strategy.Apply this diff to ensure consistent theming:
classNameGroup='max-h-[40vh] !text-white ' className='w-full font-medium text-white' + // Ensure the text color adapts based on the theme
354-354: InitializetotalHourswith Correct FormatWhen adding a new shift, the
totalHoursis initialized with'00:00:00 h'. Ensure it matches the format used elsewhere, such as'00:00h'.Apply this diff:
- { startTime: '', endTime: '', totalHours: '00:00:00 h', dateFrom: new Date() } + { startTime: '', endTime: '', totalHours: '00:00h', dateFrom: new Date() }
468-476: Update Placeholders to Reflect Time FormatThe placeholders in
ShiftTimingSelecthave been updated to'00:00:00', but since time options are in the format'HH:MM', consider updating them to'00:00'for consistency.Apply this diff:
- placeholder="00:00:00" + placeholder="00:00"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/web/app/[locale]/timesheet/[memberId]/components/AddTaskModal.tsx(10 hunks)apps/web/app/helpers/date.ts(1 hunks)apps/web/app/hooks/features/useTimelogFilterOptions.ts(1 hunks)apps/web/app/interfaces/timer/ITimerLog.ts(1 hunks)apps/web/lib/features/integrations/calendar/table-time-sheet.tsx(7 hunks)apps/web/lib/features/multiple-select/index.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/web/app/hooks/features/useTimelogFilterOptions.ts
🔇 Additional comments (13)
apps/web/lib/features/integrations/calendar/table-time-sheet.tsx (3)
4-4: Appropriate Import of 'MoreHorizontal' Icon
The import of MoreHorizontal from 'lucide-react' is necessary for the TaskActionMenu component to display the menu icon.
30-30: Importing 'ConfirmStatusChange' and 'statusOptions'
The addition of ConfirmStatusChange and statusOptions imports is appropriate, as they are used within the SelectFilter component.
51-51: Correct Use of Imported Interfaces
The imported interfaces IUser, TimeLogType, TimesheetLog, and TimesheetStatus are appropriately used for type annotations across the component.
apps/web/app/[locale]/timesheet/[memberId]/components/AddTaskModal.tsx (5)
26-37: Define FormState Interface for Form Management
The introduction of the FormState interface enhances form state management by providing strong typing for form fields, improving code readability and maintainability.
94-98: Encapsulate Date Conversion with createUtcDate Function
Moving the createUtcDate function inside handleAddTimesheet encapsulates its usage and prevents unnecessary exposure, promoting better function scope management.
99-140: Enhance Error Handling in handleAddTimesheet
The updated handleAddTimesheet function includes comprehensive error handling and validation, ensuring shifts are provided and time entries are valid.
268-268: Pass Correct Parameters to handleAddTimesheet
The onClick handler now correctly passes formState to handleAddTimesheet, ensuring that the form data is submitted as intended.
Line range hint 362-373: Validate Shift Times to Prevent Overlaps and Invalid Durations
The shift change handler now includes validation to prevent overlapping shifts and ensure the end time is after the start time. This enhances data integrity.
apps/web/app/interfaces/timer/ITimerLog.ts (1)
91-92: Update logType and source Properties to Use Enums
Changing the logType and source properties to use the TimeLogType and TimerSource enums allows for greater flexibility and type safety.
apps/web/lib/features/multiple-select/index.tsx (3)
Line range hint 22-23: Add classNameGroup to CustomSelectProps
Including classNameGroup in the CustomSelectProps interface allows for additional styling flexibility of the SelectGroup component.
Line range hint 146-149: Set Default ariaLabel for Accessibility
Setting the ariaLabel to default to "Select an option" improves accessibility for screen readers when no label is provided.
161-162: Avoid Duplicate Keys When Options Have Identical Values
If value is not unique across options, using it as a key may cause rendering issues. Ensure that each value is unique or consider using another unique property.
Run the following script to check for duplicate value entries in options:
apps/web/app/helpers/date.ts (1)
230-236: Consider modernizing date handling approach
While the implementation is functional, there are some architectural considerations:
-
Moment.js is in maintenance mode and not recommended for new projects. Consider using modern alternatives like:
date-fnsfor functional date manipulationluxonfor timezone handling- Native
TemporalAPI (when it becomes stable)
-
The current implementation might have inconsistent behavior with different input types. Consider:
- Documenting expected behavior for each input type
- Adding JSDoc comments with examples
- Adding type guards for more precise type safety
-
These functions should be part of a broader timezone strategy. Consider:
- Documenting the project's timezone handling approach
- Ensuring consistent timezone handling across the application
- Adding tests for various timezone scenarios
Let's verify the usage of these functions:
apps/web/lib/features/integrations/calendar/table-time-sheet.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: 2
🧹 Outside diff range and nitpick comments (5)
apps/web/lib/features/integrations/calendar/table-time-sheet.tsx (2)
Line range hint
88-105: Complete the 'Denied' status handling in handleButtonClickWhile the function handles 'Approved' and 'Deleted' cases well, the 'Denied' case only opens a modal without implementing the actual status update.
Apply this diff to complete the implementation:
case 'Denied': - openModal(); + if (selectTimesheetId.length > 0) { + openModal(); + // Handle the actual status update after modal confirmation + // This should be implemented in the RejectSelectedModal's onReject callback + } break;
Line range hint
489-517: Enhance sort indicators in HeaderColumn componentThe sort indicators (arrows) are currently static. Consider making them dynamic based on the current sort state for better user experience.
Apply this diff to enhance the indicators:
<MdKeyboardArrowUp style={{ height: 10, - color: "#71717A", + color: currentSort === "DESC" ? "#000000" : "#71717A", }} /> <MdKeyboardArrowDown style={{ height: 10, - color: "#71717A", + color: currentSort === "ASC" ? "#000000" : "#71717A", }} />apps/web/app/[locale]/timesheet/[memberId]/components/AddTaskModal.tsx (3)
26-37: Consider enhancing type safety in FormState interfaceThe interface structure is good, but could be improved with:
- More specific types for time strings
- Required/optional markers for fields
- Validation constraints through branded types
interface FormState { - isBillable: boolean; - notes: string; - projectId: string; - taskId: string; - employeeId: string; - shifts: { - dateFrom: Date; - startTime: string; - endTime: string; - }[]; + isBillable: boolean; + notes: string; + projectId: string; + taskId: string; + employeeId: string; + shifts: Array<{ + dateFrom: Date; + startTime: `${number}:${number}`; // HH:mm format + endTime: `${number}:${number}`; // HH:mm format + }>; }
Line range hint
374-392: Refactor time overlap validation logicThe time overlap checking logic should be extracted into a separate function for better maintainability and reusability.
+const isTimeOverlapping = ( + currentStart: number, + currentEnd: number, + shiftStart: number, + shiftEnd: number +): boolean => { + return ( + (currentStart < shiftEnd && currentEnd > shiftStart) || + (currentStart === shiftStart && currentEnd === shiftEnd) + ); +}; if (convertToMinutesHour(startTime) >= convertToMinutesHour(endTime)) { return; } updatedShifts[index].totalHours = calculateTotalHoursHour(startTime, endTime); -const isOverlapping = shifts.some((shift, i) => { - if (i === index || !shift.startTime || !shift.endTime) return false; - - const currentStart = convertToMinutesHour(startTime); - const currentEnd = convertToMinutesHour(endTime); - const shiftStart = convertToMinutesHour(shift.startTime); - const shiftEnd = convertToMinutesHour(shift.endTime); - return ( - (currentStart < shiftEnd && currentEnd > shiftStart) || - (currentStart === shiftStart && currentEnd === shiftEnd) - ); -}); +const isOverlapping = shifts.some((shift, i) => + i !== index && + shift.startTime && + shift.endTime && + isTimeOverlapping( + convertToMinutesHour(startTime), + convertToMinutesHour(endTime), + convertToMinutesHour(shift.startTime), + convertToMinutesHour(shift.endTime) + ) +);
469-477: Improve accessibility for time selection controlsAdd appropriate aria-labels and roles to improve accessibility.
<ShiftTimingSelect label="Start" timeOptions={timeOptions} placeholder="00:00:00" + aria-label={t('aria.START_TIME_SELECTOR')} className="bg-[#30B3661A]" value={value.startTime} onChange={(value) => onChange(index, 'startTime', value)} /> <ShiftTimingSelect label="End" timeOptions={timeOptions} placeholder="00:00:00" + aria-label={t('aria.END_TIME_SELECTOR')} className="bg-[#DA27271A]" value={value.endTime} onChange={(value) => onChange(index, 'endTime', value)} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/web/app/[locale]/timesheet/[memberId]/components/AddTaskModal.tsx(9 hunks)apps/web/app/helpers/date.ts(1 hunks)apps/web/lib/features/integrations/calendar/table-time-sheet.tsx(5 hunks)
🔇 Additional comments (5)
apps/web/app/helpers/date.ts (2)
238-240: 🛠️ Refactor suggestion
Add input validation to maintain consistency with toLocal
The toUTC function should validate input dates consistently with the toLocal function to maintain a uniform error handling approach across the codebase.
Apply this diff to add validation:
export function toUTC(date: string | Date | moment.Moment): moment.Moment {
- return moment(date).utc();
+ const utcDate = moment(date);
+ if (!utcDate.isValid()) {
+ throw new Error('Invalid date provided to toUTC');
+ }
+ return utcDate.utc();
}230-240: Verify the impact of these date conversion functions
Let's ensure that the changes to these date conversion functions won't cause breaking changes in the codebase.
apps/web/lib/features/integrations/calendar/table-time-sheet.tsx (2)
Line range hint 83-87: Implement sorting logic in handleSort function
The function currently only logs the sorting action without implementing the actual sorting logic.
Line range hint 519-597: Consider lifting sort state management
The sort state is currently managed locally in the HeaderRow component. Consider lifting it up to maintain sort state across re-renders and enable server-side sorting.
Let's verify if the parent components are prepared for this change:
apps/web/app/[locale]/timesheet/[memberId]/components/AddTaskModal.tsx (1)
355-355:
Fix inconsistent time format
The totalHours format '00:00:00 h' is inconsistent with the rest of the application.
-{ startTime: '', endTime: '', totalHours: '00:00:00 h', dateFrom: new Date() },
+{ startTime: '', endTime: '', totalHours: '00:00h', dateFrom: new Date() },Likely invalid or redundant comment.
| @@ -381,41 +258,6 @@ export function DataTableTimeSheet({ data, user }: { data?: GroupedTimesheet[], | |||
| } | |||
| )} | |||
| </div> | |||
| <div className="flex items-center justify-end p-4 space-x-2"> | |||
There was a problem hiding this comment.
@Innocent-Akim why that whole block was added and now removed? What was function of it? Did you replaced it with something else or it was not needed at all or what?
There was a problem hiding this comment.
@evereq I removed this code because it was related to the pagination functions of this Shadcn table.
| @@ -183,28 +80,7 @@ export function DataTableTimeSheet({ data, user }: { data?: GroupedTimesheet[], | |||
| }; | |||
|
|
|||
| const t = useTranslations(); | |||
| const [sorting, setSorting] = React.useState<SortingState>([]); | |||
There was a problem hiding this comment.
@Innocent-Akim why that whole block was added and now removed? What was function of it? Did you replaced it with something else or it was not needed at all or what?
There was a problem hiding this comment.
@evereq It's almost the same, except that when I used the hook to dispatch the data to the table, I analyzed how to reuse the same code. I realized it was impossible because it was tailored to the Shadcn table. Then, I understood that it was very important to remove it to make the code clearer.
| @@ -67,94 +52,6 @@ import { IUser, TimesheetLog, TimesheetStatus } from '@/app/interfaces'; | |||
| import { toast } from '@components/ui/use-toast'; | |||
| import { ToastAction } from '@components/ui/toast'; | |||
|
|
|||
| export const columns: ColumnDef<TimeSheet>[] = [ | |||
There was a problem hiding this comment.
@Innocent-Akim why that whole block was added and now removed? What was function of it? Did you replaced it with something else or it was not needed at all or what?
There was a problem hiding this comment.
@evereq Here, I simply removed the table headers. With the new design, I realized that using a Shadcn table was no longer necessary.
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
toLocal,toUTC) for better date handling.Bug Fixes
Refactor