Feat(timesheet): Add TimesheetDetailModal for displaying pending #3382
Conversation
WalkthroughThe pull request introduces several enhancements across multiple components related to timesheet management. Key modifications include the addition of a 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/TimesheetCard.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: 6
🧹 Outside diff range and nitpick comments (18)
apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetDetailModal.tsx (2)
1-5: Optimize imports for better maintainabilityThe React import is not necessary in modern React applications that use the new JSX transform.
Consider applying this diff to optimize the imports:
-import { TimesheetLog, TimesheetStatus } from '@/app/interfaces'; -import { Modal } from '@/lib/components'; -import React from 'react' -import { TimesheetCardDetail } from './TimesheetCard'; -import { useTranslations } from 'next-intl'; +import { useTranslations } from 'next-intl'; + +import { TimesheetLog, TimesheetStatus } from '@/app/interfaces'; +import { Modal } from '@/lib/components'; +import { TimesheetCardDetail } from './TimesheetCard';
17-23: Enhance modal accessibilityThe modal component could benefit from improved accessibility attributes.
<Modal isOpen={isOpen} closeModal={closeModal} title={'View Pending details'} showCloseIcon + role="dialog" + aria-modal="true" + aria-labelledby="timesheet-modal-title" className="bg-light--theme-light dark:bg-dark--theme-light p-5 rounded w-full md:w-40 md:min-w-[35rem]" - titleClass="font-bold flex justify-start w-full text-2xl"> + titleClass="font-bold flex justify-start w-full text-2xl" + id="timesheet-modal-title">apps/web/app/[locale]/timesheet/[memberId]/components/CalendarView.tsx (3)
Line range hint
13-27: Improve loading state handling and type safetyThe current implementation has some inconsistencies in how loading states are handled:
- The
loadingprop is required but not utilized in the rendering logic- Loading state is inferred from
databeing undefined, which could lead to confusionConsider this improved implementation:
-export function CalendarView({ data, loading }: { data?: GroupedTimesheet[], loading: boolean }) { +interface CalendarViewProps { + data?: GroupedTimesheet[]; + loading: boolean; +} + +export function CalendarView({ data, loading }: CalendarViewProps) { const t = useTranslations(); return ( <div className="grow h-full w-full bg-[#FFFFFF] dark:bg-dark--theme"> - {data ? ( + {loading ? ( + <div className="flex items-center justify-center h-full"> + <p>{t('pages.timesheet.LOADING')}</p> + </div> + ) : !data || data.length === 0 ? ( + <div className="flex items-center justify-center h-full min-h-[280px]"> + <p>{t('pages.timesheet.NO_ENTRIES_FOUND')}</p> + </div> + ) : ( + <CalendarDataView data={data} t={t} /> + )} - data.length > 0 ? ( - <CalendarDataView data={data} t={t} /> - ) : ( - <div className="flex items-center justify-center h-full min-h-[280px]"> - <p>{t('pages.timesheet.NO_ENTRIES_FOUND')}</p> - </div> - ) - ) : ( - <div className="flex items-center justify-center h-full"> - <p>{t('pages.timesheet.LOADING')}</p> - </div> - )} </div> ); }
Line range hint
29-143: Consider breaking down CalendarDataView for better maintainabilityThe component has high cyclomatic complexity and multiple responsibilities. This could make it harder to maintain and test.
Consider:
- Extracting the status section into a separate component:
interface TimesheetStatusSectionProps { status: string; rows: GroupedTimesheet[]; onToggle?: () => void; } const TimesheetStatusSection: React.FC<TimesheetStatusSectionProps> = ...
- Extracting the task card into a separate component:
interface TaskCardProps { task: TaskType; status: string; } const TaskCard: React.FC<TaskCardProps> = ...
Line range hint
71-76: Add aria-label to status indicator for accessibilityThe status indicator currently relies solely on color to convey information, which may not be accessible to all users.
Add proper aria labels:
-<div className={clsxm('p-2 rounded', statusColor(status).bg)}></div> +<div + className={clsxm('p-2 rounded', statusColor(status).bg)} + role="status" + aria-label={`Status: ${status === 'DENIED' ? 'REJECTED' : status}`} +></div>apps/web/app/[locale]/timesheet/[memberId]/page.tsx (1)
106-111: Consider using React.lazy for modal componentWhile the implementation is correct, consider lazy loading the modal component since it's only rendered conditionally. This can improve initial page load performance.
-import TimesheetDetailModal from './components/TimesheetDetailModal'; +const TimesheetDetailModal = React.lazy(() => import('./components/TimesheetDetailModal')); // In the render: -{isTimesheetDetailOpen && <TimesheetDetailModal +{isTimesheetDetailOpen && ( + <React.Suspense fallback={<div>Loading...</div>}> + <TimesheetDetailModal closeModal={closeTimesheetDetail} isOpen={isTimesheetDetailOpen} timesheet={statusTimesheet} - />} + /> + </React.Suspense> +)}apps/web/app/hooks/features/useTimesheet.ts (2)
Line range hint
20-45: Enhance error handling and type safety in groupByDate function.While the implementation is solid, consider these improvements:
- Add input validation for the
itemsparameter- Use a more specific type for the error in the catch block
- Consider returning a Result/Either type instead of logging errors
-const groupByDate = (items: TimesheetLog[]): GroupedTimesheet[] => { +const groupByDate = (items: TimesheetLog[] | null | undefined): GroupedTimesheet[] => { if (!items?.length) return []; type GroupedMap = Record<string, TimesheetLog[]>; const groupedByDate = items.reduce<GroupedMap>((acc, item) => { if (!item?.timesheet?.createdAt) { - console.warn('Skipping item with missing timesheet or createdAt:', item); + console.warn('[groupByDate] Invalid item structure:', { item }); return acc; } try { const date = new Date(item.timesheet.createdAt).toISOString().split('T')[0]; if (!acc[date]) acc[date] = []; acc[date].push(item); - } catch (error) { + } catch (error: unknown) { console.error( - `Failed to process date for timesheet ${item.timesheet.id}:`, - { createdAt: item.timesheet.createdAt, error } + `[groupByDate] Date processing failed:`, + { + timesheetId: item.timesheet.id, + createdAt: item.timesheet.createdAt, + error: error instanceof Error ? error.message : String(error) + } ); } return acc; }, {});
Line range hint
1-295: Consider architectural improvements for better maintainability.The codebase could benefit from the following architectural improvements:
- Implement a centralized error handling utility
- Consolidate the grouping functions using a more generic approach
- Add proper error boundaries for React components using this hook
Consider implementing a centralized error handling utility:
// errorUtils.ts type ErrorContext = Record<string, unknown>; export const logError = ( component: string, message: string, context: ErrorContext, error?: unknown ) => { console.error( `[${component}] ${message}`, { ...context, error: error instanceof Error ? error.message : String(error) } ); };Consider consolidating the grouping functions:
type GroupingStrategy = 'daily' | 'weekly' | 'monthly'; type GroupKeyGenerator = (date: Date) => string; const groupingStrategies: Record<GroupingStrategy, GroupKeyGenerator> = { daily: (date) => date.toISOString().split('T')[0], weekly: getWeekYearKey, monthly: (date) => `${date.getFullYear()}-${(date.getMonth() + 1).toString().padStart(2, '0')}` }; const groupTimesheet = ( items: TimesheetLog[], strategy: GroupingStrategy ): GroupedTimesheet[] => { const getKey = groupingStrategies[strategy]; return createGroupingFunction(getKey)(items); };apps/web/lib/features/integrations/calendar/table-time-sheet.tsx (6)
Line range hint
153-167: Improve error handling and user feedback in delete operationThe error handling in the
handleConfirmfunction could be enhanced to provide better user feedback and error recovery.Consider this implementation:
const handleConfirm = () => { - try { - deleteTaskTimesheet() - .then(() => { - setSelectTimesheet([]); - setIsDialogOpen(false); - }) - .catch((error) => { - console.error('Delete timesheet error:', error); - }); - } catch (error) { - console.error('Delete timesheet error:', error); - } + deleteTaskTimesheet() + .then(() => { + setSelectTimesheet([]); + setIsDialogOpen(false); + }) + .catch((error) => { + console.error('Delete timesheet error:', error); + // Add user feedback + toast.error('Failed to delete timesheet. Please try again.'); + }); };
Line range hint
196-198: Remove console.log from production codeThe
handleSortfunction contains a console.log statement which should not be in production code.const handleSort = (key: string, order: SortOrder) => { - console.log(`Sorting ${key} in ${order} order`); + // Implement sorting logic here };
Line range hint
449-484: Enhance accessibility for status indicatorsThe status colors should be complemented with aria labels for better accessibility.
const getColorClass = () => { + const getStatusInfo = (status: string) => { + switch (status) { + case 'Rejected': + return { color: 'text-red-500 border-gray-200', ariaLabel: 'Rejected status' }; + case 'Approved': + return { color: 'text-green-500 border-gray-200', ariaLabel: 'Approved status' }; + case 'Pending': + return { color: 'text-orange-500 border-gray-200', ariaLabel: 'Pending status' }; + default: + return { color: 'text-gray-500 border-gray-200', ariaLabel: 'Default status' }; + } + }; + const statusInfo = getStatusInfo(selected); - switch (selected) { - case 'Rejected': - return 'text-red-500 border-gray-200'; - case 'Approved': - return 'text-green-500 border-gray-200'; - case 'Pending': - return 'text-orange-500 border-gray-200'; - default: - return 'text-gray-500 border-gray-200'; - } + return statusInfo.color; };
Line range hint
509-517: Improve error handling in status updateThe catch block in the status update only logs the error without providing user feedback.
try { await updateTimesheetStatus({ status: status.label as TimesheetStatus, ids: [timesheet.timesheet.id] }); } catch (error) { - console.error('Failed to update timesheet status:'); + console.error('Failed to update timesheet status:', error); + toast.error(t('errors.STATUS_UPDATE_FAILED')); + // Optionally revert UI state + setCurrentStatus(previousStatus); }
Line range hint
587-603: Enhance maintainability of status color mappingThe switch statement could be replaced with a more maintainable object map.
-export const getBadgeColor = (timesheetStatus: TimesheetStatus | null) => { - switch (timesheetStatus) { - case 'DRAFT': - return 'bg-gray-300'; - case 'PENDING': - return 'bg-yellow-400'; - case 'IN REVIEW': - return 'bg-blue-500'; - case 'DENIED': - return 'bg-red-500'; - case 'APPROVED': - return 'bg-green-500'; - default: - return 'bg-gray-100'; - } -}; +const STATUS_COLORS: Record<TimesheetStatus, string> = { + DRAFT: 'bg-gray-300', + PENDING: 'bg-yellow-400', + 'IN REVIEW': 'bg-blue-500', + DENIED: 'bg-red-500', + APPROVED: 'bg-green-500' +}; + +export const getBadgeColor = (timesheetStatus: TimesheetStatus | null): string => { + return STATUS_COLORS[timesheetStatus ?? ''] ?? 'bg-gray-100'; +};
Line range hint
1-686: Consider splitting the file into smaller componentsThis file is quite large and handles multiple concerns. Consider splitting it into separate files for better maintainability:
components/timesheet/DataTableTimeSheet.tsxcomponents/timesheet/SelectFilter.tsxcomponents/timesheet/TaskActionMenu.tsxutils/timesheet/status.ts(for getBadgeColor and related utilities)This would improve:
- Code organization
- File maintainability
- Component reusability
- Testing isolation
apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetCard.tsx (4)
106-107: Provide unique keys toAccordionItemcomponentsUsing
statusas thekeyinAccordionItemmay cause issues if there are multiple items with the same status. Ensure each key is unique by appending an index or unique identifier.Apply this diff:
-{Object.entries(getStatusTimesheet(plan.tasks)).map(([status, rows]) => { +{Object.entries(getStatusTimesheet(plan.tasks)).map(([status, rows], index) => { -return rows.length > 0 && status && <AccordionItem - key={status} +return rows.length > 0 && status && ( + <AccordionItem + key={`${status}-${index}`}
124-124: Standardize status labels to avoid confusionThe code maps
'DENIED'to'REJECTED'for display purposes. It would be clearer to standardize the status labels throughout the application to eliminate the need for this conditional mapping.Consider updating the status values across the codebase or creating a utility function for consistent status labeling.
95-97: Adjust condition to display week information accuratelyThe condition checks if
timesheetGroupByDays === 'Weekly', but without knowing all possible values, ensure that this comparison is accurate. Also, consider internationalization for the word "Week".Apply this diff:
-{timesheetGroupByDays === 'Weekly' && ( - <span>Week {index + 1}</span> +{timesheetGroupByDays === t('timesheet.GROUP_BY_WEEKLY') && ( + <span>{t('timesheet.WEEK')} {index + 1}</span>
112-117: Simplify styles by avoiding inline stylesUsing inline styles like
style={{ backgroundColor: statusColor(status).bgOpacity }}can make the code harder to maintain. Consider using a utility class or extending your styling solution to handle dynamic styles.Consider:
- Creating a utility class that applies the dynamic background color.
- Using a CSS-in-JS solution that accepts props for styling.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
apps/web/app/[locale]/timesheet/[memberId]/components/CalendarView.tsx(1 hunks)apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetCard.tsx(2 hunks)apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetDetailModal.tsx(1 hunks)apps/web/app/[locale]/timesheet/[memberId]/page.tsx(5 hunks)apps/web/app/hooks/features/useTimesheet.ts(1 hunks)apps/web/lib/features/integrations/calendar/table-time-sheet.tsx(1 hunks)
🧰 Additional context used
📓 Learnings (1)
apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetDetailModal.tsx (1)
Learnt from: Innocent-Akim
PR: ever-co/ever-teams#3208
File: apps/web/app/[locale]/timesheet/components/TimesheetFilter.tsx:30-35
Timestamp: 2024-11-12T14:06:02.202Z
Learning: In the `TimesheetFilter` component, the `Add Time` button does not need to use the `AddManualTimeModal` component, as per the user's decision.
🔇 Additional comments (5)
apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetDetailModal.tsx (1)
13-39: Implementation aligns well with requirements
The component successfully implements the timesheet detail modal functionality as described in the PR objectives. The code is well-structured and includes proper error handling and internationalization support.
apps/web/app/[locale]/timesheet/[memberId]/page.tsx (3)
25-25: LGTM: Clean modal state management implementation
The modal setup follows React best practices with clear naming and proper state management using the useModal hook.
Also applies to: 77-82
214-222: LGTM: Consistent loading state implementation
The loading state is properly implemented and consistently applied across both view components, improving the user experience during data fetching.
142-142: Verify: Is task selection required for the modal?
The click handler opens the modal but doesn't pass specific task data. Please verify if this is intentional or if we should pass the selected pending task to the modal.
apps/web/app/hooks/features/useTimesheet.ts (1)
294-295: LGTM! Consistent with existing patterns.
The addition of loadingUpdateTimesheet and groupByDate to the return statement follows the established pattern and enhances the hook's functionality.
apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetDetailModal.tsx
Show resolved
Hide resolved
apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetDetailModal.tsx
Show resolved
Hide resolved
apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetDetailModal.tsx
Show resolved
Hide resolved
apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetCard.tsx
Outdated
Show resolved
Hide resolved
apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetCard.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetCard.tsx (4)
78-82: Consider adding TypeScript type guards and loading statesThe component could benefit from:
- Explicit type definition for the data prop
- Loading state handling
- Error boundary implementation
Consider applying this improvement:
+type TimesheetCardDetailProps = { + data?: Record<TimesheetStatus, TimesheetLog[]>; + isLoading?: boolean; +}; -export const TimesheetCardDetail = ({ data }: { data?: Record<TimesheetStatus, TimesheetLog[]> }) => { +export const TimesheetCardDetail = ({ data, isLoading = false }: TimesheetCardDetailProps) => { + if (isLoading) { + return <div className="animate-pulse">Loading...</div>; + } const { getStatusTimesheet, groupByDate } = useTimesheet({}); const { timesheetGroupByDays } = useTimelogFilterOptions(); const timesheetGroupByDate = groupByDate(data?.PENDING || [])
106-110: Improve status comparison logicThe status comparison could be more maintainable using a constant map.
Consider applying this improvement:
+const STATUS_DISPLAY_MAP: Record<string, string> = { + 'DENIED': 'REJECTED', + // Add other status mappings here +}; -return rows.length > 0 && status && <AccordionItem - key={status} - value={status === 'DENIED' ? 'REJECTED' : status} +return rows.length > 0 && status && ( + <AccordionItem + key={status} + value={STATUS_DISPLAY_MAP[status] || status}
86-104: Extract date header into a separate componentThe date header logic could be extracted into a reusable component to improve maintainability.
Consider creating a separate component:
type TimesheetDateHeaderProps = { date: string; index: number; timesheetGroupByDays: string; tasks: TimesheetLog[]; }; const TimesheetDateHeader = ({ date, index, timesheetGroupByDays, tasks }: TimesheetDateHeaderProps) => ( <div className={clsxm( 'h-[48px] flex justify-between items-center w-full', 'bg-[#ffffffcc] dark:bg-dark--theme rounded-md border-1', 'border-gray-400 px-5 text-[#71717A] font-medium' )}> <div className='flex gap-x-3'> {timesheetGroupByDays === 'Weekly' && ( <span>Week {index + 1}</span> )} <span>{formatDate(date)}</span> </div> <TotalDurationByDate timesheetLog={tasks} createdAt={formatDate(date)} /> </div> );
140-174: Extract task row into a separate componentThe task row rendering logic is complex and could be extracted into a separate component for better maintainability.
Consider creating a separate component:
type TaskRowProps = { task: TimesheetLog; status: string; }; const TaskRow = ({ task, status }: TaskRowProps) => ( <div style={{ backgroundColor: statusColor(status).bgOpacity, borderBottomColor: statusColor(status).bg }} className={clsxm( 'flex items-center border-b border-b-gray-200 dark:border-b-gray-600 space-x-4 p-1 h-[60px]' )}> {/* ... rest of the task row JSX ... */} </div> );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetCard.tsx(2 hunks)
🔇 Additional comments (1)
apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetCard.tsx (1)
165-167:
Handle potential undefined imageUrl to prevent broken images
The non-null assertion on imageUrl could lead to runtime errors.
Apply this fix:
<EmployeeAvatar
- imageUrl={task.employee.user.imageUrl!}
+ imageUrl={task.employee.user.imageUrl || '/images/default-avatar.png'}
/>
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
Enhancements
groupByDatefunction from the useTimesheet hook for better accessibility.Bug Fixes