[Fix]: Handle Timesheet Update #3384
Conversation
WalkthroughThis pull request introduces significant updates to various components related to timesheet management. Key changes include enhancements to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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. 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: 7
🧹 Outside diff range and nitpick comments (9)
apps/web/app/hooks/features/useTimelogFilterOptions.ts (2)
22-27: Time formatting logic looks good, but consider handling edge casesThe conversion from 24-hour to 12-hour format is implemented correctly. However, there are a few suggestions for improvement:
Consider these enhancements:
const totalMinutes = i * interval; const hour24 = Math.floor(totalMinutes / 60); -const hour12 = hour24 % 12 || 12; // Convert to 12-hour format +const hour12 = hour24 === 0 ? 12 : hour24 > 12 ? hour24 - 12 : hour24; const minutes = (totalMinutes % 60).toString().padStart(2, '0'); const period = hour24 < 12 ? 'AM' : 'PM'; // Determine AM/PM -return `${hour12.toString().padStart(2, '0')}:${minutes} ${period}`; +// Ensure valid time range +if (hour24 >= 24) return null; +return `${hour12.toString().padStart(2, '0')}:${minutes} ${period}`;The changes above:
- Make the 12-hour conversion more explicit
- Add validation to prevent invalid times (beyond 24 hours)
30-30: Consider adding JSDoc documentationThe empty line at 30 could be utilized to add JSDoc documentation for the
generateTimeOptionsfunction, explaining its purpose and parameters.Add documentation like this:
+/** + * Generates an array of time options in 12-hour format (AM/PM) + * @param interval - The interval in minutes between time options (default: 15) + * @returns Array of formatted time strings or null for invalid times + */ const generateTimeOptions = (interval = 15) => {apps/web/app/interfaces/timer/ITimerLog.ts (1)
120-121: Maintain consistency with date handlingThe date type changes here match those in
TimesheetLog. If you implement the suggested date standardization, ensure it's applied consistently across both interfaces.Consider creating a common type for timesheet dates:
type TimesheetDateTime = string | Date; // Or just Date if standardizing interface TimesheetDateFields { startedAt: TimesheetDateTime; stoppedAt: TimesheetDateTime; }Then use this in both interfaces:
interface TimesheetLog extends BaseEntity, TimesheetDateFields { // ... other fields } interface UpdateTimesheetStatus extends BaseEntity, TimesheetDateFields { // ... other fields }apps/web/lib/features/manual-time/manage-member-component.tsx (1)
80-81: Add JSDoc documentation for the new properties.For consistency with other interface properties, please add JSDoc documentation for
classNameTitleanddefaultValueproperties.+ /** + * Optional CSS class name for the title element. + * @type {string} + */ classNameTitle?: string; + /** + * Optional default value for the combobox fields. + * @type {string} + */ defaultValue?: stringapps/web/app/[locale]/timesheet/[memberId]/page.tsx (2)
Line range hint
52-67: Optimize filtering logic for better performance and safetyThe current implementation of
filterDataTimesheethas potential performance and safety concerns:
- Multiple
toLowerCase()calls on each iteration- Missing null checks for nested properties
- Unnecessary array creation on each filter
Consider this optimization:
const filterDataTimesheet = useMemo(() => { + if (!timesheet.length || !lowerCaseSearch) return timesheet; const filteredTimesheet = timesheet .filter((v) => v.tasks.some( (task) => { + const title = task.task?.title?.toLowerCase() ?? ''; + const fullName = task.employee?.fullName?.toLowerCase() ?? ''; + const projectName = task.project?.name?.toLowerCase() ?? ''; + return ( - task.task?.title?.toLowerCase()?.includes(lowerCaseSearch) || - task.employee?.fullName?.toLowerCase()?.includes(lowerCaseSearch) || - task.project?.name?.toLowerCase()?.includes(lowerCaseSearch) + title.includes(lowerCaseSearch) || + fullName.includes(lowerCaseSearch) || + projectName.includes(lowerCaseSearch) + ); } ) ); return filteredTimesheet; }, [timesheet, lowerCaseSearch]);
Line range hint
213-227: Enhance ViewToggleButton componentThe component is well-implemented but could benefit from:
- Moving to a separate file for better code organization
- Adding accessibility attributes for better user experience
Consider these enhancements:
const ViewToggleButton: React.FC<ViewToggleButtonProps> = ({ mode, active, icon, onClick, t }) => ( <button onClick={onClick} + role="tab" + aria-selected={active} + aria-controls={`${mode.toLowerCase()}-panel`} className={clsxm( 'text-[#7E7991] font-medium w-[191px] h-[40px] flex items-center gap-x-4 text-[14px] px-2 rounded', active && 'border-b-primary text-primary border-b-2 dark:text-primary-light dark:border-b-primary-light bg-[#F1F5F9] dark:bg-gray-800 font-bold' )} > {icon} <span>{mode === 'ListView' ? t('pages.timesheet.VIEWS.LIST') : t('pages.timesheet.VIEWS.CALENDAR')}</span> </button> );Consider moving this component to a separate file like
components/ViewToggleButton.tsx.apps/web/app/[locale]/timesheet/[memberId]/components/EditTaskModal.tsx (1)
173-175: Use dynamic label for date pickerThe
labelprop inDatePickerFilteris hardcoded as "Oct 01 2024". It's better to display the selected date dynamically to reflect the user's current selection.Suggested fix:
<DatePickerFilter date={dateRange.date} setDate={handleFromChange} - label="Oct 01 2024" + label={dateRange.date ? dateRange.date.toLocaleDateString() : ''} />apps/web/app/[locale]/timesheet/[memberId]/components/AddTaskModal.tsx (2)
290-290: Translate comments and alert messages for consistencyThe comment
// Détection des chevauchementsand the alert message'Le shift modifié chevauche un autre shift existant.'are in French. Translating them to English or implementing localization enhances maintainability and user experience.Apply this diff to translate the comment and alert message:
- // Détection des chevauchements + // Detect overlapping shifts ... - alert('Le shift modifié chevauche un autre shift existant.'); + alert('The modified shift overlaps with an existing shift.');🧰 Tools
🪛 GitHub Check: Cspell
[warning] 290-290:
Unknown word (Détection)
[warning] 290-290:
Unknown word (chevauchements)
282-282: Translate comments to English for consistencyThe comment
// Validation des donnéesis in French. For clarity and consistency with the rest of the codebase, please translate it to English.Apply this diff:
- // Validation des données + // Data validation🧰 Tools
🪛 GitHub Check: Cspell
[warning] 282-282:
Unknown word (données)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
apps/web/app/[locale]/timesheet/[memberId]/components/AddTaskModal.tsx(4 hunks)apps/web/app/[locale]/timesheet/[memberId]/components/EditTaskModal.tsx(11 hunks)apps/web/app/[locale]/timesheet/[memberId]/page.tsx(1 hunks)apps/web/app/hooks/features/useTimelogFilterOptions.ts(1 hunks)apps/web/app/interfaces/timer/ITimerLog.ts(2 hunks)apps/web/lib/components/combobox/index.tsx(3 hunks)apps/web/lib/features/manual-time/manage-member-component.tsx(3 hunks)
🧰 Additional context used
🪛 GitHub Check: Cspell
apps/web/app/[locale]/timesheet/[memberId]/components/AddTaskModal.tsx
[warning] 282-282:
Unknown word (données)
[warning] 286-286:
Unknown word (L’heure)
[warning] 286-286:
Unknown word (début)
[warning] 286-286:
Unknown word (être)
[warning] 286-286:
Unknown word (inférieure)
[warning] 286-286:
Unknown word (l’heure)
[warning] 290-290:
Unknown word (Détection)
[warning] 290-290:
Unknown word (chevauchements)
[warning] 306-306:
Unknown word (modifié)
[warning] 306-306:
Unknown word (chevauche)
🔇 Additional comments (9)
apps/web/app/hooks/features/useTimelogFilterOptions.ts (1)
22-27: Verify the impact of time format changes
The change from 24-hour to 12-hour format might affect components that consume these time options.
Let's check for any components that might be affected:
✅ Verification successful
Time format changes are safe to proceed
The verification shows that the time options are only used for display purposes in select dropdowns and no time parsing or comparison operations depend on the format:
- The time options are used in
AddTaskModal.tsxonly for populating select dropdowns TimeSheetFilterPopover.tsxuses the hook but doesn't directly interact with the time format- Other components using dates work with Date objects and ISO strings, not the formatted time strings
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for components using generateTimeOptions or timeOptions
rg -l "generateTimeOptions|timeOptions" --type ts --type tsx
# Search for time format patterns to identify potential conflicts
rg -l "(\d{2}:\d{2})\s*(AM|PM|am|pm)" --type ts --type tsx
Length of output: 179
Script:
#!/bin/bash
# Search for components using generateTimeOptions or timeOptions without file type restrictions
rg -l "generateTimeOptions|timeOptions"
# Search for files that import or use the useTimelogFilterOptions hook
rg -l "useTimelogFilterOptions"
# Search for time format patterns to identify potential conflicts
rg -l "(\d{2}:\d{2})\s*(AM|PM|am|pm)"
Length of output: 739
Script:
#!/bin/bash
# Check how the time options are being used in components
rg -A 5 "timeOptions" "apps/web/app/[locale]/timesheet/[memberId]/components/AddTaskModal.tsx"
rg -A 5 "useTimelogFilterOptions" "apps/web/app/[locale]/timesheet/[memberId]/components/TimeSheetFilterPopover.tsx"
# Check if there's any time format parsing or comparison
rg "new Date|Date\.parse|format.*time|parse.*time" "apps/web/lib/features/integrations/calendar/table-time-sheet.tsx"
rg "new Date|Date\.parse|format.*time|parse.*time" "apps/web/app/hooks/features/useTimesheet.ts"
Length of output: 3941
apps/web/lib/components/combobox/index.tsx (1)
Line range hint 1-115: Well-structured component implementation
The overall component implementation is clean and follows React best practices:
- Proper TypeScript generics usage
- Clean UI state management
- Effective use of React hooks
- Good handling of dynamic width calculations
apps/web/app/interfaces/timer/ITimerLog.ts (3)
Line range hint 137-157: Verify type compatibility with UpdateTimesheet interface
The UpdateTimesheet interface uses Pick to select fields from TimesheetLog. Ensure that consumers of this interface are compatible with the new date types and constrained enum values.
Let's check the usage:
✅ Verification successful
Based on the search results, I can now generate the final response:
The UpdateTimesheet interface usage is consistent with its definition
The interface is used correctly across the codebase:
- In API calls (
createTimesheetFromApi,updateTimesheetFromAPi) - In hooks (
useTimesheet.ts) - In server requests (
createTimesheetRequest,updateTimesheetRequest)
All usages properly handle the required fields and maintain type compatibility. No type mismatches or incompatibilities were found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for UpdateTimesheet interface usage
rg "UpdateTimesheet.*[{<]" -A 3
# Check for any type errors in related files
ast-grep --pattern 'const $_ : UpdateTimesheet = {
$$$
}'
Length of output: 5283
88-89: Consider standardizing date type handling
The dual type string | Date for startedAt and stoppedAt could lead to inconsistent date handling across the application. Consider:
- Using a single type (preferably
Date) and handling string conversion at API boundaries - Adding validation to ensure consistent date format when strings are used
Let's check how dates are handled across the codebase:
91-92: Verify impact of constrained enum values
The hardcoded values TimeLogType.MANUAL and TimerSource.BROWSER make this interface specific to manual browser entries.
Let's verify if this constraint is compatible with all usage scenarios:
✅ Verification successful
Let's fix the syntax and search more broadly to understand the interface and enum definitions, along with their usages. This will help us verify if the constrained enum values are appropriate for all scenarios.
Hardcoded enum values are intentional and correct for this interface
The constrained enum values are appropriate here as this interface specifically represents manual time entries from the browser. This is evidenced by:
- The
TimerSourceenum shows multiple sources (MOBILE, BROWSER, DESKTOP, etc.), but this interface is specifically for browser-based entries - The usage in
useManualTime.tsconfirms this is the correct pattern for manual time entries from the browser - The
TimeLogTypeenum includes different types (TRACKED, MANUAL, IDLE, RESUMED), but this interface specifically handles manual entries
The constraints help enforce type safety for manual browser-based time entries, which is the intended use case for this interface.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for TimesheetLog implementations to verify enum usage
ast-grep --pattern 'interface $_ extends TimesheetLog {
$$$
}'
# Check for direct TimesheetLog object creations
rg "TimesheetLog.*=.*{" -A 5
Length of output: 197
Script:
#!/bin/bash
# Search for interface definitions and usages
rg "interface.*TimesheetLog" -A 5
# Search for enum definitions
rg "enum TimeLogType|enum TimerSource" -A 10
# Search for usages of these specific enum values
rg "TimeLogType\.MANUAL|TimerSource\.BROWSER" -A 2
Length of output: 3620
apps/web/lib/features/manual-time/manage-member-component.tsx (2)
98-99: LGTM!
The props destructuring is correctly implemented and maintains consistency with the interface definition.
80-81: Verify the impact on existing implementations.
The addition of the defaultValue prop might affect existing implementations of this component, particularly in timesheet-related features.
Also applies to: 98-99, 127-127
✅ Verification successful
Let me check the actual usage of this component to understand if the changes impact existing implementations.
Changes are safe and backward compatible
The verification shows that the new props (defaultValue and classNameTitle) are optional and the existing implementations remain compatible:
EditTaskModal.tsxalready uses both propsAddTaskModal.tsxandadd-manual-time-modal.tsxuse only the required props- No breaking changes are introduced as the new props are optional
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of ManageOrMemberComponent in timesheet-related files
rg -l "ManageOrMemberComponent" | rg -i "time|sheet"
# Check specific usage patterns
ast-grep --pattern 'ManageOrMemberComponent $props'
Length of output: 371
Script:
#!/bin/bash
# Check how ManageOrMemberComponent is used in the timesheet files
rg -A 5 "ManageOrMemberComponent" apps/web/lib/features/manual-time/add-manual-time-modal.tsx apps/web/app/\[locale\]/timesheet/\[memberId\]/components/EditTaskModal.tsx apps/web/app/\[locale\]/timesheet/\[memberId\]/components/AddTaskModal.tsx
Length of output: 4612
apps/web/app/[locale]/timesheet/[memberId]/page.tsx (1)
211-211: Verify loading state behavior with showPageSkeleton
The addition of showPageSkeleton: true might result in duplicate loading indicators since the component already handles loading state via loadingTimesheet. Please ensure this doesn't create a jarring user experience.
apps/web/app/[locale]/timesheet/[memberId]/components/AddTaskModal.tsx (1)
285-288:
Use 'convertToMinutesHour' for 12-hour time format comparison
The function convertToMinutes is designed for 24-hour time formats, but the times are in 12-hour format with AM/PM. Using convertToMinutesHour ensures accurate comparisons.
Apply this diff to fix the issue:
- if (convertToMinutes(startTime) >= convertToMinutes(endTime)) {
+ if (convertToMinutesHour(startTime) >= convertToMinutesHour(endTime)) {Likely invalid or redundant comment.
🧰 Tools
🪛 GitHub Check: Cspell
[warning] 286-286:
Unknown word (L’heure)
[warning] 286-286:
Unknown word (début)
[warning] 286-286:
Unknown word (être)
[warning] 286-286:
Unknown word (inférieure)
[warning] 286-286:
Unknown word (l’heure)
apps/web/app/[locale]/timesheet/[memberId]/components/EditTaskModal.tsx
Outdated
Show resolved
Hide resolved
apps/web/app/[locale]/timesheet/[memberId]/components/EditTaskModal.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 (2)
apps/web/app/[locale]/timesheet/[memberId]/components/EditTaskModal.tsx (2)
215-234: Improve form validation UXThe current validation approach for the notes field could be improved:
- The red border on empty notes is premature if the field hasn't been touched
- Character count could show remaining characters instead of current count
<textarea value={timesheetData.notes} onChange={(e) => setTimesheetData((prev) => ({ ...prev, notes: e.target.value, }))} placeholder="Insert notes here..." className={clsxm( "bg-transparent focus:border-transparent focus:ring-2 focus:ring-transparent", "placeholder-gray-300 placeholder:font-normal resize-none p-2 grow w-full", "border border-gray-200 dark:border-slate-600 dark:bg-dark--theme-light rounded-md h-40 bg-[#FBB6500D]", - timesheetData.notes.trim().length === 0 && "border-red-500" + touched.notes && timesheetData.notes.trim().length === 0 && "border-red-500" )} maxLength={120} minLength={0} aria-label="Insert notes here" required /> <div className="text-sm text-[#282048] dark:text-gray-500 text-right"> - {timesheetData.notes.length}/{120} + {120 - timesheetData.notes.length} characters remaining </div>
Line range hint
1-268: Consider using a form management libraryThe component handles complex form state and validation logic. Consider using a form management library like React Hook Form or Formik to:
- Centralize form state management
- Handle validation more effectively
- Manage form submission states
- Reduce boilerplate code
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
apps/web/app/[locale]/timesheet/[memberId]/components/AddTaskModal.tsx(4 hunks)apps/web/app/[locale]/timesheet/[memberId]/components/EditTaskModal.tsx(11 hunks)
🔇 Additional comments (4)
apps/web/app/[locale]/timesheet/[memberId]/components/EditTaskModal.tsx (2)
31-34:
Initialize timeRange with values from dataTimesheet
The timeRange state is initialized with empty strings, which could lead to validation issues when submitting the form. This is a known issue from previous reviews.
const [timeRange, setTimeRange] = useState<{ startTime: string; endTime: string }>({
- startTime: '',
- endTime: '',
+ startTime: dataTimesheet.timesheet?.startedAt
+ ? new Date(dataTimesheet.timesheet.startedAt).toLocaleTimeString('en-US', { hour12: false, hour: '2-digit', minute: '2-digit' })
+ : '',
+ endTime: dataTimesheet.timesheet?.stoppedAt
+ ? new Date(dataTimesheet.timesheet.stoppedAt).toLocaleTimeString('en-US', { hour12: false, hour: '2-digit', minute: '2-digit' })
+ : '',
});66-87:
Add validation and error handling to form submission
The form submission handler needs additional validation and error handling:
- Validate that end time is after start time
- Add error handling for the updateTimesheet call
- Add loading state during submission
const handleUpdateSubmit = useCallback(async (e: FormEvent<HTMLFormElement>) => {
e.preventDefault();
+ try {
const startedAt = new Date(dateRange.date ?? new Date());
const stoppedAt = new Date(dateRange.date ?? new Date());
const [startHours, startMinutes] = timeRange.startTime.split(':').map(Number);
startedAt.setHours(startHours, startMinutes);
const [endHours, endMinutes] = timeRange.endTime.split(':').map(Number);
stoppedAt.setHours(endHours, endMinutes);
+
+ if (stoppedAt <= startedAt) {
+ throw new Error('End time must be after start time');
+ }
+
await updateTimesheet({
// ... existing properties
});
+ closeModal();
+ } catch (error) {
+ console.error('Failed to update timesheet:', error);
+ // Add error notification here
+ }
}, [dateRange, timeRange, timesheetData, dataTimesheet, updateTimesheet]);Likely invalid or redundant comment.
apps/web/app/[locale]/timesheet/[memberId]/components/AddTaskModal.tsx (2)
63-63: Resolve conflicting width classes in modal styling
The className includes both md:w-40 (width of 10rem at medium breakpoints) and md:min-w-[32rem] (minimum width of 32rem at medium breakpoints). This conflict may cause unintended layout issues. Consider removing md:w-40 to let md:min-w-[32rem] control the modal's width.
Apply this diff to fix the conflict:
- className="bg-light--theme-light dark:bg-dark--theme-light p-5 rounded-xl w-full md:w-40 md:min-w-[32rem] justify-start h-[auto]"
+ className="bg-light--theme-light dark:bg-dark--theme-light p-5 rounded-xl w-full md:min-w-[32rem] justify-start h-[auto]"210-210: ClassName configuration is appropriate
The styling for the SelectItem component looks good.
apps/web/app/[locale]/timesheet/[memberId]/components/EditTaskModal.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: 3
🧹 Outside diff range and nitpick comments (1)
apps/web/app/[locale]/timesheet/[memberId]/components/EditTaskModal.tsx (1)
211-235: Consider optimizing state updatesMultiple separate state updates could be combined for better performance, especially for form fields that often change together.
Consider using a single state update:
-onClick={() => setTimesheetData((prev) => ({ - ...prev, - isBillable: true, -}))} +onClick={() => setTimesheetData((prev) => { + const updates = { + isBillable: true, + // Add other related fields that might change together + }; + return { ...prev, ...updates }; +})}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
apps/web/app/[locale]/timesheet/[memberId]/components/AddTaskModal.tsx(4 hunks)apps/web/app/[locale]/timesheet/[memberId]/components/EditTaskModal.tsx(9 hunks)apps/web/lib/features/manual-time/manage-member-component.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/web/lib/features/manual-time/manage-member-component.tsx
- apps/web/app/[locale]/timesheet/[memberId]/components/AddTaskModal.tsx
🔇 Additional comments (1)
apps/web/app/[locale]/timesheet/[memberId]/components/EditTaskModal.tsx (1)
159-163: Time input validation looks good!
The implementation includes proper HTML5 validation attributes as previously suggested.
apps/web/app/[locale]/timesheet/[memberId]/components/EditTaskModal.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
apps/web/app/[locale]/timesheet/[memberId]/components/AddTaskModal.tsx (2)
246-259: Add JSDoc documentation for the total hours calculationThe function logic is correct, but adding documentation would improve maintainability.
Consider adding JSDoc:
+/** + * Calculates the total hours between two times in 12-hour format + * @param start - Start time in "HH:MM AM/PM" format + * @param end - End time in "HH:MM AM/PM" format + * @returns Total duration in "HH:MMh" format + * @example + * calculateTotalHoursHour("09:00 AM", "05:00 PM") // Returns "08:00h" + * calculateTotalHoursHour("10:00 PM", "02:00 AM") // Returns "04:00h" + */ const calculateTotalHoursHour = React.useCallback( (start: string, end: string): string => { // ... existing implementation }, [] );
271-302: Refactor shift change handler for better separation of concernsThe function handles multiple responsibilities and could benefit from being split into smaller, focused functions.
Consider this refactoring:
+const isValidTimeRange = (startTime: string, endTime: string): boolean => { + if (!startTime || !endTime) return false; + return convertToMinutesHour(startTime) < convertToMinutesHour(endTime); +}; + +const hasOverlappingShifts = ( + shifts: Shift[], + currentIndex: number, + startTime: string, + endTime: string +): boolean => { + return shifts.some((shift, i) => { + if (i === currentIndex || !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 handleShiftChange = (index: number, field: keyof Shift, value: string) => { const updatedShifts = [...shifts]; updatedShifts[index][field] = value; if (field === 'startTime' || field === 'endTime') { const { startTime, endTime } = updatedShifts[index]; - if (!startTime || !endTime) return; - if (convertToMinutesHour(startTime) >= convertToMinutesHour(endTime)) { - return; - } + if (!isValidTimeRange(startTime, 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 = hasOverlappingShifts(shifts, index, startTime, endTime); if (isOverlapping) return; } setShifts(updatedShifts); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
apps/web/app/[locale]/timesheet/[memberId]/components/AddTaskModal.tsx(4 hunks)apps/web/app/[locale]/timesheet/[memberId]/components/EditTaskModal.tsx(9 hunks)
🔇 Additional comments (5)
apps/web/app/[locale]/timesheet/[memberId]/components/EditTaskModal.tsx (4)
Line range hint 136-295: Implementation looks good with proper form handling
The form implementation follows React best practices with:
- Proper form submission handling
- Input validation
- Error states
- Accessibility attributes
33-38:
Fix unsafe time string manipulation
The current implementation of time string formatting is unsafe and could fail across different timezones or date formats.
Use a more robust approach with proper date formatting:
const [timeRange, setTimeRange] = useState<{ startTime: string; endTime: string }>({
startTime: dataTimesheet.timesheet?.startedAt
- ? dataTimesheet.timesheet.startedAt.toString().slice(0, 5)
+ ? new Date(dataTimesheet.timesheet.startedAt)
+ .toLocaleTimeString('en-US', {
+ hour: '2-digit',
+ minute: '2-digit',
+ hour12: false
+ })
: '',
endTime: dataTimesheet.timesheet?.stoppedAt
- ? dataTimesheet.timesheet.stoppedAt.toString().slice(0, 5)
+ ? new Date(dataTimesheet.timesheet.stoppedAt)
+ .toLocaleTimeString('en-US', {
+ hour: '2-digit',
+ minute: '2-digit',
+ hour12: false
+ })
: '',
});69-110:
Add validation for time order
While the time format validation is good, there's no check to ensure the end time is after the start time.
Add time order validation:
const handleUpdateSubmit = useCallback(async (e: FormEvent<HTMLFormElement>) => {
e.preventDefault();
if (!timeRange.startTime || !timeRange.endTime) {
alert('Please enter valid start and end times.');
return;
}
if (!/^\d{2}:\d{2}$/.test(timeRange.startTime) || !/^\d{2}:\d{2}$/.test(timeRange.endTime)) {
alert('Time format should be HH:MM.');
return;
}
+ const [startHours, startMinutes] = timeRange.startTime.split(':').map(Number);
+ const [endHours, endMinutes] = timeRange.endTime.split(':').map(Number);
+ const startMinutesTotal = startHours * 60 + startMinutes;
+ const endMinutesTotal = endHours * 60 + endMinutes;
+ if (endMinutesTotal <= startMinutesTotal) {
+ alert('End time must be after start time');
+ return;
+ }
const baseDate = dateRange.date ?? new Date();
// ... rest of the code241-260:
Enhance notes field validation
While the UI shows validation state for empty notes, it doesn't prevent submission of whitespace-only notes.
Add proper validation:
<textarea
value={timesheetData.notes}
onChange={(e) => setTimesheetData((prev) => ({
...prev,
notes: e.target.value,
}))}
placeholder="Insert notes here..."
className={clsxm(
"bg-transparent focus:border-transparent focus:ring-2 focus:ring-transparent",
"placeholder-gray-300 placeholder:font-normal resize-none p-2 grow w-full",
"border border-gray-200 dark:border-slate-600 dark:bg-dark--theme-light rounded-md h-40 bg-[#FBB6500D]",
- timesheetData.notes.trim().length === 0 && "border-red-500"
+ (!timesheetData.notes || timesheetData.notes.trim().length === 0) && "border-red-500"
)}
maxLength={120}
minLength={0}
aria-label="Insert notes here"
required
+ onBlur={(e) => {
+ const trimmed = e.target.value.trim();
+ setTimesheetData(prev => ({
+ ...prev,
+ notes: trimmed
+ }));
+ }}
/>Also add validation in the submit handler:
const handleUpdateSubmit = useCallback(async (e: FormEvent<HTMLFormElement>) => {
e.preventDefault();
+ const trimmedNotes = timesheetData.notes.trim();
+ if (trimmedNotes.length === 0) {
+ alert('Notes field cannot be empty');
+ return;
+ }
// ... rest of the code
}, [dateRange, timeRange, timesheetData, dataTimesheet, updateTimesheet]);apps/web/app/[locale]/timesheet/[memberId]/components/AddTaskModal.tsx (1)
63-63: Resolve conflicting width classes in modal styling
The className still includes conflicting width classes: md:w-40 and md:min-w-[32rem].
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
New Features
AddTaskModalwith improved time calculation functionality, including validation for overlapping shifts.EditTaskModalfor better state management and streamlined handling of time entries.defaultValueproperty inCustomComboboxandManageOrMemberComponentfor default selections.TimeSheetcomponent to manage loading states more effectively with a new property.Bug Fixes
AddTaskModalto ensure start time is less than end time.Documentation