[Feat]: Add dark Mode Support to timesheet component #3230
Conversation
WalkthroughThe pull request introduces several modifications across various components within the timesheet application. Key changes include the addition of dark mode styling to enhance visual consistency, adjustments to component layouts, and the introduction of new TypeScript interfaces for better data modeling. Notably, a new API route for fetching timesheet data has been added, and existing components have been refined for improved usability and accessibility. Overall, these changes aim to enhance the user experience by ensuring the application is visually cohesive across different themes and improving component functionality. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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: 9
🧹 Outside diff range and nitpick comments (31)
apps/web/app/[locale]/timesheet/[memberId]/components/CalendarView.tsx (1)
4-4: Consider using Tailwind color variables for better maintainability.While the dark mode implementation is correct, using hardcoded color values (
#FFFFFF) makes theme maintenance more difficult. Consider using Tailwind's color palette variables for consistency and easier updates.-<div className='grow h-full w-full bg-[#FFFFFF] dark:bg-dark--theme'> +<div className='grow h-full w-full bg-white dark:bg-dark--theme'>apps/web/app/api/timer/timesheet/daily/route.ts (1)
15-16: Add response headers for caching and rate limiting.The API response should include appropriate headers for caching and rate limiting to improve performance and prevent abuse.
- return $res(data) + return NextResponse.json(data, { + headers: { + 'Cache-Control': 'private, max-age=300', + 'X-RateLimit-Limit': '100', + 'X-RateLimit-Remaining': '99' + } + });apps/web/app/interfaces/index.ts (1)
44-44: Consider reorganizing exports for better maintainability.The timer-related exports could be grouped with other time-tracking interfaces like
ITaskTimesheetfor better organization.Consider applying this diff:
export * from './ITaskSizes'; export * from './ITaskTimesheet'; +export * from './timer'; export * from './ITaskLabels'; export * from './ITaskRelatedIssueType'; ... export * from './ILiveKiteCredentials'; export * from './IBaseModel'; -export * from './timer'apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetFilter.tsx (1)
30-39: Consider using design tokens for consistent valuesWhile the styling changes look good, consider extracting magic numbers to design tokens for better maintainability:
- The
2.2remheight- The
primary/5opacity valueExample approach:
// In your design tokens file export const tokens = { height: { filterControls: '2.2rem' }, opacity: { primaryLight: '0.05' // for primary/5 } } as constThen update the button class to use these tokens:
- className="bg-primary/5 dark:bg-primary-light dark:border-transparent !h-[2.2rem] font-medium" + className={`bg-primary/${tokens.opacity.primaryLight} dark:bg-primary-light dark:border-transparent !h-[${tokens.height.filterControls}] font-medium`}apps/web/app/services/server/requests/timesheet.ts (1)
58-66: Implementation looks good but needs improvements.The function follows the established patterns but could benefit from:
- Stricter typing for query parameters
- Error handling
- JSDoc documentation explaining the purpose and usage
Consider applying these improvements:
+ /** + * Fetches daily timesheet report for specified tasks + * @param params - Parameters for filtering timesheet data + * @param bearer_token - Authentication token + * @returns Promise<ITimeSheet[]> - Array of timesheet entries + * @throws Error if the request fails + */ export function getTaskTimesheetRequest(params: TTasksTimesheetStatisticsParams, bearer_token: string) { const queries = qs.stringify(params); - return serverFetch<ITimeSheet[]>({ + try { + return serverFetch<ITimeSheet[]>({ path: `/timesheet/time-log/report/daily?${queries.toString()}`, method: 'GET', bearer_token, tenantId: params.tenantId - }) + }); + } catch (error) { + console.error('Failed to fetch timesheet report:', error); + throw error; + } }apps/web/app/interfaces/timer/ITimerLog.ts (4)
1-7: Consider using a dedicated type for image URLs.The
imageUrlandimageproperties appear in multiple interfaces. Consider creating a dedicated type for better type safety and consistency:type ImageAsset = { imageUrl: string; image: string | null; }; interface Project extends ImageAsset { id: string; name: string; membersCount: number; }Also applies to: 16-21, 23-30
9-14: Consider enhancing the Task interface with additional metadata.The Task interface could benefit from additional fields commonly associated with tasks:
interface Task { id: string; title: string; estimate: Duration; // Consider using a more specific type taskNumber: string; status?: 'todo' | 'in_progress' | 'done'; priority?: 'low' | 'medium' | 'high'; dueDate?: string; } type Duration = { value: number; unit: 'hours' | 'days'; } | null;
23-30: Consider removing redundant name field from User interface.The
nameproperty appears redundant since you already havefirstNameandlastName. Consider removing it or making it a computed property:interface User { id: string; firstName: string; lastName: string; imageUrl: string; image: string | null; // name can be computed: `${firstName} ${lastName}` }
40-72: Consider using more specific types for timestamps and IDs.The ITimeSheet interface uses string for timestamps and IDs. Consider using more specific types for better type safety:
type ISO8601DateTime = string; // You could create a branded type for more safety type UUID = string; // You could create a branded type for more safety export interface ITimeSheet { id: UUID; createdAt: ISO8601DateTime; updatedAt: ISO8601DateTime; deletedAt: ISO8601DateTime | null; archivedAt: ISO8601DateTime | null; startedAt: ISO8601DateTime; stoppedAt: ISO8601DateTime; editedAt: ISO8601DateTime | null; // ... rest of the fields }apps/web/app/[locale]/timesheet/[memberId]/components/FilterWithStatus.tsx (1)
29-29: Consider adjusting button background for better contrastWhile the current styling works, the buttons might blend too much with the container in dark mode since they share the same background color. Consider using a slightly different shade for better visual distinction.
-'dark:bg-gray-800 dark:border-primary-light bg-transparent text-[#71717A] w-[80px]', +'dark:bg-gray-700 dark:border-primary-light bg-transparent text-[#71717A] w-[80px]',apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetAction.tsx (3)
33-35: Use Tailwind's color system instead of hex valuesReplace hex color values with Tailwind's color system for better maintainability and consistency with the theme system.
-{ icon: <FaClipboardCheck className="!text-[#2932417c] dark:!text-gray-400 rounded" />, title: t('pages.timesheet.TIMESHEET_ACTION_APPROVE_SELECTED') }, -{ icon: <IoClose className="!bg-[#2932417c] dark:!bg-gray-400 rounded" />, title: t('pages.timesheet.TIMESHEET_ACTION_REJECT_SELECTED') }, -{ icon: <RiDeleteBin6Fill className="!text-[#2932417c] dark:!text-gray-400 rounded" />, title: t('pages.timesheet.TIMESHEET_ACTION_DELETE_SELECTED') } +{ icon: <FaClipboardCheck className="!text-gray-600/50 dark:!text-gray-400 rounded" />, title: t('pages.timesheet.TIMESHEET_ACTION_APPROVE_SELECTED') }, +{ icon: <IoClose className="!bg-gray-600/50 dark:!bg-gray-400 rounded" />, title: t('pages.timesheet.TIMESHEET_ACTION_REJECT_SELECTED') }, +{ icon: <RiDeleteBin6Fill className="!text-gray-600/50 dark:!text-gray-400 rounded" />, title: t('pages.timesheet.TIMESHEET_ACTION_DELETE_SELECTED') }
Line range hint
48-50: Implement onClick handler functionalityThe TODO comment indicates missing implementation for the button actions. While this might be out of scope for the dark mode feature, it's important to track this technical debt.
Would you like me to help create a GitHub issue to track the implementation of these actions? I can provide a suggested implementation for:
- Approve action
- Reject action
- Delete action
33-43: Add transition classes for smooth theme switchingConsider adding transition classes to ensure smooth switching between light and dark modes.
Add the following classes to each icon's className:
-className="!text-[#2932417c] dark:!text-gray-400 rounded" +className="!text-[#2932417c] dark:!text-gray-400 rounded transition-colors duration-200"apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetCard.tsx (3)
28-28: LGTM! Consider subtle shadow for depth in dark modeThe dark mode border and shadow implementation looks good. However, instead of completely removing the shadow in dark mode, consider using a very subtle shadow to maintain depth perception.
-className='w-full h-[175px] rounded-md border border-gray-200 dark:border-gray-600 flex gap-8 shadow shadow-gray-100 dark:shadow-transparent p-3'> +className='w-full h-[175px] rounded-md border border-gray-200 dark:border-gray-600 flex gap-8 shadow shadow-gray-100 dark:shadow-gray-900/10 p-3'>
32-33: LGTM! Consider contrast ratio for accessibilityThe dark mode text colors are consistent. However, gray-400 might not provide sufficient contrast in dark mode for optimal accessibility.
- <h2 className='text-base md:text-[16px] font-medium text-[#282048] dark:text-gray-400 truncate w-full'>{title}</h2> - <span className='text-sm md:text-[14px] text-[#3D5A80] dark:text-gray-400 truncate w-full'>{date ?? description}</span> + <h2 className='text-base md:text-[16px] font-medium text-[#282048] dark:text-gray-300 truncate w-full'>{title}</h2> + <span className='text-sm md:text-[14px] text-[#3D5A80] dark:text-gray-300 truncate w-full'>{date ?? description}</span>
39-42: LGTM! Consider dark mode hover stateThe button styling is well-implemented, but the hover state could be enhanced for dark mode.
- 'hover:bg-gray-50 focus:ring-2 focus:ring-offset-2 focus:ring-gray-200 dark:border-gray-600' + 'hover:bg-gray-50 dark:hover:bg-gray-700 focus:ring-2 focus:ring-offset-2 focus:ring-gray-200 dark:focus:ring-gray-600 dark:border-gray-600'apps/web/app/[locale]/timesheet/[memberId]/components/FrequencySelect.tsx (5)
42-42: Consider improving text overflow handling for accessibility.The dark mode styling changes look good. However, the current overflow handling with
overflow-hidden text-clipmight not be ideal for accessibility. Consider usingtext-ellipsisinstead to indicate truncated content visually.- <SelectTrigger className="w-36 overflow-hidden h-[2.2rem] text-clip border border-gray-200 dark:border-gray-700 bg-white dark:bg-dark--theme-light focus:ring-2 focus:ring-transparent"> + <SelectTrigger className="w-36 overflow-hidden h-[2.2rem] text-ellipsis border border-gray-200 dark:border-gray-700 bg-white dark:bg-dark--theme-light focus:ring-2 focus:ring-transparent">
Line range hint
56-82: Add dark mode support to FilterTaskActionMenu component.The component needs dark mode styling for better visibility and consistency:
- Button text color in dark mode
- Menu items hover state contrast
- <Button variant="ghost" className="text-sm font-normal"> + <Button variant="ghost" className="text-sm font-normal dark:text-gray-200"> <span className="sr-only">Open menu</span> <span className="">20-10-20</span> </Button> - <DropdownMenuItem onClick={(e) => e.preventDefault()} className="cursor-pointer hover:bg-primary" > + <DropdownMenuItem onClick={(e) => e.preventDefault()} className="cursor-pointer hover:bg-primary dark:hover:bg-primary/80 dark:text-gray-200" >
Line range hint
108-127: Add dark mode styling to CustomDateRange component.While noting that the date range functionality will be implemented in a future PR, the current component needs dark mode styling support:
- <DropdownMenuSubContent className="bg-white hover:bg-white"> + <DropdownMenuSubContent className="bg-white dark:bg-dark--theme-light hover:bg-white dark:hover:bg-dark--theme-light"> - <DropdownMenuItem className="cursor-pointer bg-white hover:bg-white"> + <DropdownMenuItem className="cursor-pointer bg-white dark:bg-dark--theme-light hover:bg-white dark:hover:bg-dark--theme-light"> <div className="flex flex-col gap-3"> <DynamicDatePicker label="From" date={dateRange.from} setDate={handleFromChange} /> <DynamicDatePicker label="To" date={dateRange.to} setDate={handleToChange} /> <div className="flex w-full justify-end gap-4"> - <button className="text-primary" onClick={cancelChanges}>Cancel</button> - <button className="text-primary" onClick={applyChanges}>Apply</button> + <button className="text-primary hover:text-primary/80" onClick={cancelChanges}>Cancel</button> + <button className="text-primary hover:text-primary/80" onClick={applyChanges}>Apply</button> </div>
Line range hint
134-146: Improve dark mode text visibility in DatePickerInput.The calendar icon has dark mode support, but the button text needs dark mode styling:
- "w-[150px] justify-start text-left font-normal text-black h-10 border border-transparent dark:border-transparent", + "w-[150px] justify-start text-left font-normal text-black dark:text-gray-200 h-10 border border-transparent dark:border-transparent",
Line range hint
148-175: Simplify className prop for better maintainability.Consider extracting the complex className into a separate constant or using the
cnutility for better readability:+ const buttonClasses = cn( + 'flex items-center w-full', + 'bg-white dark:bg-dark--theme-light', + 'border-gray-300 dark:border-slate-600', + 'justify-start text-left font-normal', + 'text-black dark:text-gray-200', + 'h-10 rounded-md' + ); return ( <div> <DatePicker buttonVariant={'link'} className="dark:bg-dark--theme-light rounded-lg bg-white" - buttonClassName={'decoration-transparent flex items-center w-full bg-white dark:bg-dark--theme-light border-gray-300 justify-start text-left font-normal text-black h-10 border dark:border-slate-600 rounded-md'} + buttonClassName={cn('decoration-transparent', buttonClasses)}apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetFilterDate.tsx (2)
197-198: Refactor DatePicker styling for better maintainability.The current implementation has:
- Duplicate height properties
- A very long, hard-to-maintain className string
- Inconsistent height units (em vs rem)
Consider extracting styles and fixing duplicates:
+const datePickerClass = { + wrapper: "dark:bg-dark-theme-light rounded-lg bg-white dark:text-gray-200", + button: "decoration-transparent flex items-center w-full h-[2.2rem] bg-white dark:text-gray-200 dark:bg-dark-theme-light border-gray-300 justify-start text-left font-normal text-black dark:border-slate-600 rounded-md" +}; -className="dark:bg-dark--theme-light rounded-lg bg-white dark:text-gray-200" -buttonClassName={'decoration-transparent flex items-center w-full h-[2.2em] bg-white dark:text-gray-200 dark:bg-dark--theme-light border-gray-300 justify-start text-left font-normal text-black h-[2.2rem] border dark:border-slate-600 rounded-md'} +className={datePickerClass.wrapper} +buttonClassName={datePickerClass.button}
Line range hint
1-220: Consider implementing a theme configuration system.The current implementation directly uses dark mode classes throughout the component. For better maintainability and consistency:
- Consider implementing a theme configuration system using CSS variables or a theme provider
- Extract common dark mode styles into reusable classes
- Implement a consistent naming convention for theme-related classes
This would make the dark mode implementation more maintainable and easier to extend in the future.
apps/web/app/[locale]/timesheet/[memberId]/page.tsx (6)
61-63: Consider extracting the magic number in height calculationThe height calculation uses a magic number (
300px). Consider extracting this to a constant or CSS variable for better maintainability.+// Add at the top of the file +const LAYOUT_HEADER_HEIGHT = 300; // pixels -childrenClassName="h-[calc(100vh-_300px)] !overflow-hidden w-full" +childrenClassName={`h-[calc(100vh-_${LAYOUT_HEADER_HEIGHT}px)] !overflow-hidden w-full`}
64-64: Remove redundant shadow propertiesThe
shadow-2xl shadow-transparent dark:shadow-transparentclasses are redundant since they effectively cancel each other out. Consider removing them if no shadow effect is intended.-<div className="top-14 fixed flex flex-col border-b-[1px] dark:border-gray-800 z-10 mx-0 w-full bg-white dark:bg-dark-high shadow-2xl shadow-transparent dark:shadow-transparent "> +<div className="top-14 fixed flex flex-col border-b-[1px] dark:border-gray-800 z-10 mx-0 w-full bg-white dark:bg-dark-high">
78-79: Use Tailwind's color system for consistencyInstead of using hardcoded hex colors, consider using Tailwind's built-in color system for better theme consistency and maintenance.
-<h1 className='!text-[23px] font-bold text-[#282048] dark:text-white'> +<h1 className='!text-[23px] font-bold text-gray-900 dark:text-white'> -<span className='text-[16px] text-[#3D5A80] dark:text-gray-500'> +<span className='text-[16px] text-blue-900 dark:text-gray-500'>
122-132: Simplify search input implementationThe current implementation has some redundancies and complexities:
- Height is duplicated between parent div and input
- Focus state handling could be simplified
-<div className='flex items-center !h-[2.2rem] w-[700px] bg-white dark:bg-dark--theme-light gap-x-2 px-2 border border-gray-200 dark:border-gray-700 rounded-sm mb-2'> +<div className='flex items-center w-[700px] bg-white dark:bg-dark--theme-light gap-x-2 px-2 border border-gray-200 dark:border-gray-700 rounded-sm mb-2'> <GoSearch className='text-[#7E7991]' /> <input role="searchbox" aria-label="Search timesheet" type="search" name="timesheet-search" id="timesheet-search" - className="!h-[2.2rem] w-full bg-transparent focus:border-transparent focus:ring-2 focus:ring-transparent placeholder-gray-500 placeholder:font-medium shadow-sm outline-none" + className="h-9 w-full bg-transparent focus:outline-none placeholder-gray-500 placeholder:font-medium" placeholder="Search.." />
135-141: Extract magic number in content height calculationSimilar to the earlier comment, the height calculation uses a magic number (
291px). Consider extracting this to a constant or CSS variable.+// Add at the top of the file +const CONTENT_OFFSET_HEIGHT = 291; // pixels -<div className='h-[calc(100vh-_291px)] mt-3 p-10 overflow-y-auto'> +<div className={`h-[calc(100vh-_${CONTENT_OFFSET_HEIGHT}px)] mt-3 p-10 overflow-y-auto`}>
168-169: Use Tailwind's color system for toggle buttonConsider using Tailwind's color system instead of hardcoded hex colors for better theme consistency.
-'text-[#7E7991] font-medium w-[191px] h-[40px] flex items-center gap-x-4 text-[14px] px-2 rounded', +'text-gray-500 font-medium w-[191px] h-10 flex items-center gap-x-4 text-sm px-2 rounded',apps/web/lib/features/integrations/calendar/table-time-sheet.tsx (2)
271-273: Improve time estimate formattingThe current implementation displays time estimates in a raw format. Consider creating a dedicated component or utility function to format the time estimates more elegantly.
Consider this improvement:
- <span className="flex-1"> - {task.estimateHours}h, {task.estimateDays}j, {task.estimateMinutes}m - </span> + <span className="flex-1"> + {formatTimeEstimate(task.estimateHours, task.estimateDays, task.estimateMinutes)} + </span>Add a utility function:
function formatTimeEstimate(hours: number, days: number, minutes: number): string { const parts = []; if (days > 0) parts.push(`${days}d`); if (hours > 0) parts.push(`${hours}h`); if (minutes > 0) parts.push(`${minutes}m`); return parts.join(' '); }
297-320: Consider extracting pagination controls into a reusable componentThe pagination controls contain repeated logic and could benefit from being extracted into a reusable component.
Consider creating a
TablePaginationControlscomponent:interface PaginationControlsProps { table: Table<any>; className?: string; } const TablePaginationControls: React.FC<PaginationControlsProps> = ({ table, className }) => { const buttons = [ { icon: MdKeyboardDoubleArrowLeft, onClick: () => table.setPageIndex(0) }, { icon: MdKeyboardArrowLeft, onClick: () => table.previousPage() }, { icon: MdKeyboardArrowRight, onClick: () => table.nextPage() }, { icon: MdKeyboardDoubleArrowRight, onClick: () => table.setPageIndex(table.getPageCount() - 1) } ]; return ( <div className={clsxm("flex items-center gap-x-3", className)}> {buttons.map((btn, idx) => ( <Button key={idx} variant="outline" onClick={btn.onClick} disabled={idx < 2 ? !table.getCanPreviousPage() : !table.getCanNextPage()} > <btn.icon /> </Button> ))} </div> ); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (18)
apps/web/app/[locale]/timesheet/[memberId]/components/CalendarView.tsx(1 hunks)apps/web/app/[locale]/timesheet/[memberId]/components/FilterWithStatus.tsx(1 hunks)apps/web/app/[locale]/timesheet/[memberId]/components/FrequencySelect.tsx(1 hunks)apps/web/app/[locale]/timesheet/[memberId]/components/TimeSheetFilterPopover.tsx(1 hunks)apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetAction.tsx(1 hunks)apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetCard.tsx(2 hunks)apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetFilter.tsx(1 hunks)apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetFilterDate.tsx(4 hunks)apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetView.tsx(1 hunks)apps/web/app/[locale]/timesheet/[memberId]/page.tsx(6 hunks)apps/web/app/api/timer/timesheet/daily/route.ts(1 hunks)apps/web/app/interfaces/index.ts(1 hunks)apps/web/app/interfaces/timer/ITimerLog.ts(1 hunks)apps/web/app/interfaces/timer/index.ts(1 hunks)apps/web/app/services/server/requests/timesheet.ts(2 hunks)apps/web/components/shared/tasks/assigned-tasks.tsx(1 hunks)apps/web/components/shared/tasks/task-input.tsx(1 hunks)apps/web/lib/features/integrations/calendar/table-time-sheet.tsx(8 hunks)
✅ Files skipped from review due to trivial changes (4)
- apps/web/app/[locale]/timesheet/[memberId]/components/TimeSheetFilterPopover.tsx
- apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetView.tsx
- apps/web/app/interfaces/timer/index.ts
- apps/web/components/shared/tasks/assigned-tasks.tsx
🧰 Additional context used
📓 Learnings (2)
apps/web/app/[locale]/timesheet/[memberId]/components/FrequencySelect.tsx (1)
Learnt from: Innocent-Akim
PR: ever-co/ever-teams#3208
File: apps/web/app/[locale]/timesheet/components/FrequencySelect.tsx:67-79
Timestamp: 2024-10-30T16:25:19.910Z
Learning: The date range selection functionality for the date range options in the `FilterTaskActionMenu` component will be implemented in a future PR.
apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetFilter.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-10-30T16:31:01.294Z
Learning: In the `TimesheetFilter` component, the `Add Time` button does not need to use the `AddManualTimeModal` component, as per the user's decision.
🪛 GitHub Check: Cspell
apps/web/lib/features/integrations/calendar/table-time-sheet.tsx
[warning] 212-212:
Unknown word (ligh)
🔇 Additional comments (16)
apps/web/app/[locale]/timesheet/[memberId]/components/CalendarView.tsx (1)
4-6: Verify the empty component implementation.
The component appears to be a placeholder with no content. Please ensure this is intentional and not an incomplete implementation.
✅ Verification successful
Let me gather more information about the CalendarView component and its implementation.
Let me check if there are any related calendar components or implementations that might indicate this is a work in progress or if there are similar patterns in the codebase.
Let me check the parent component to understand how CalendarView is being used and what it's expected to contain.
Let me check if there are any recent changes or work in progress related to the CalendarView component.
Empty CalendarView component is a work in progress
Based on the git history, this component was just recently added (Nov 4, 2024) as part of a larger feature implementation for the timesheet functionality. The empty implementation is intentional as it's part of an ongoing development, with only dark mode support added so far (Nov 5, 2024). The parent component shows it's properly integrated into a view toggle system alongside a ListView mode.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for potential content or components that should be rendered inside CalendarView
rg -l --type tsx "CalendarView" | xargs rg -A 5 "import.*CalendarView|<CalendarView"
Length of output: 1280
Script:
#!/bin/bash
# Check the full implementation of CalendarView component
cat "apps/web/app/[locale]/timesheet/[memberId]/components/CalendarView.tsx"
Length of output: 227
Script:
#!/bin/bash
# Search for calendar-related components and implementations
rg -g "*.tsx" -g "*.ts" -l "calendar|Calendar" | xargs rg -A 5 "function.*Calendar|class.*Calendar|interface.*Calendar|type.*Calendar"
Length of output: 10926
Script:
#!/bin/bash
# Check the implementation of the parent component where CalendarView is used
cat "apps/web/app/[locale]/timesheet/[memberId]/page.tsx"
Length of output: 9137
Script:
#!/bin/bash
# Check git history for CalendarView component
git log -p -- "apps/web/app/[locale]/timesheet/[memberId]/components/CalendarView.tsx"
Length of output: 1711
apps/web/app/api/timer/timesheet/daily/route.ts (2)
1-3: LGTM! Imports are appropriate and well-organized.
5-16: Verify the integration with the authentication system.
The route uses authenticatedGuard, but we should verify its integration with the overall authentication system.
✅ Verification successful
Let me generate additional scripts to verify the authentication implementation.
Let me check one more aspect to verify the authentication implementation completely.
Authentication implementation is properly integrated and consistent
The authentication system is well-implemented with proper safeguards:
authenticatedGuardis consistently used across 70+ protected routes- The guard properly validates access tokens and user authentication via
currentAuthenticatedUserRequest - Proper cookie management for tokens, tenant, and organization IDs
- Authentication middleware is integrated at both API routes and app level
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify authentication guard usage across the application
# Expected: Consistent usage of authenticatedGuard in protected routes
# Check for other route files using authenticatedGuard
echo "Checking authentication guard usage in routes:"
rg -l "authenticatedGuard" "apps/web/app/api"
# Check for any custom authentication implementations
echo -e "\nChecking for custom authentication implementations:"
rg -l "Authorization.*Bearer" "apps/web/app/api"
Length of output: 3742
Script:
#!/bin/bash
# Check the authenticatedGuard implementation
echo "Checking authenticatedGuard implementation:"
rg -A 10 "export.*authenticatedGuard"
# Check for authentication middleware or utilities
echo -e "\nChecking auth-related utilities:"
fd -e ts -e js auth middleware guard
Length of output: 2854
Script:
#!/bin/bash
# Check the currentAuthenticatedUserRequest implementation and cookie utilities
echo "Checking authentication request implementation:"
rg -A 5 "currentAuthenticatedUserRequest"
echo -e "\nChecking cookie utilities:"
rg -l "getAccessTokenCookie|getTenantIdCookie|getOrganizationIdCookie" --type ts
Length of output: 9619
apps/web/app/interfaces/index.ts (1)
44-44: Verify potential circular dependencies and interface overlaps.
The new timer export might conflict with the existing ITimer interface export. Additionally, there's a risk of circular dependencies.
Let's verify these concerns:
apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetFilter.tsx (1)
20-21: LGTM! Improved layout structure
The switch to flexbox with justify-between provides better control over the spacing and alignment of filter elements.
apps/web/app/services/server/requests/timesheet.ts (1)
4-4: LGTM: Import statement is properly placed and utilized.
The import of ITimeSheet interface is correctly added and used as the return type for the new API request function.
apps/web/app/interfaces/timer/ITimerLog.ts (1)
40-72: Verify the relationship between this interface and dark mode implementation.
This file contains type definitions but doesn't seem directly related to the PR's objective of adding dark mode support. Please ensure that these interfaces are necessary for the dark mode implementation.
apps/web/app/[locale]/timesheet/[memberId]/components/FilterWithStatus.tsx (2)
23-23: LGTM: Container dark mode styling is well implemented
The dark mode background color choice provides good contrast while maintaining the component's visual hierarchy.
30-30: LGTM: Active state styling enhances visual feedback
The enhanced shadow and consistent use of primary-light color effectively highlight the active state in dark mode.
apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetCard.tsx (1)
58-58: LGTM! Shadow implementation looks good
The dark mode shadow for the icon container provides good visual depth while maintaining dark mode aesthetics.
apps/web/components/shared/tasks/task-input.tsx (2)
143-145: LGTM! Dark mode styling implementation looks good.
The dark mode classes provide appropriate contrast and follow Tailwind's dark mode conventions. The template literal construction improves readability.
Line range hint 189-196: Verify the necessity of the active prop.
The active prop is declared in the component's interface and passed from the parent, but it's not used within the component implementation. This could indicate either:
- Dead code if the prop is no longer needed
- Missing implementation if the prop should affect the component's behavior
apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetFilterDate.tsx (1)
135-135: LGTM!
The separator's dark mode styling follows Tailwind conventions correctly.
apps/web/app/[locale]/timesheet/[memberId]/page.tsx (2)
105-105: LGTM!
The dark mode border color implementation follows best practices.
Line range hint 1-173: Dark mode implementation looks good overall!
The implementation successfully adds dark mode support to the timesheet component while maintaining existing functionality. The changes follow React and Tailwind best practices for theme switching. Consider the suggested refinements for better maintainability, but the core implementation is solid.
apps/web/lib/features/integrations/calendar/table-time-sheet.tsx (1)
Line range hint 463-476: LGTM! Well-structured dropdown menu implementation
The implementation properly handles translations and maintains consistent styling with the dark mode theme.
apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetFilter.tsx
Outdated
Show resolved
Hide resolved
apps/web/app/[locale]/timesheet/[memberId]/components/FilterWithStatus.tsx
Outdated
Show resolved
Hide resolved
apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetFilterDate.tsx
Outdated
Show resolved
Hide resolved
apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetFilterDate.tsx
Outdated
Show resolved
Hide resolved
apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetFilterDate.tsx
Outdated
Show resolved
Hide resolved
apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetFilterDate.tsx
Outdated
Show resolved
Hide resolved
apps/web/lib/features/integrations/calendar/table-time-sheet.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
apps/web/lib/features/integrations/calendar/table-time-sheet.tsx (1)
Line range hint
463-472: Enhance accessibility for dropdown menu itemsWhile the implementation works correctly, consider improving accessibility by adding
aria-labelattributes to the dropdown menu items to better describe the actions for screen readers.Apply this diff to enhance accessibility:
- <DropdownMenuItem textValue={'Yes'} className="cursor-pointer"> + <DropdownMenuItem + textValue={'Yes'} + className="cursor-pointer" + aria-label={t('pages.timesheet.BILLABLE.YES')} + > <div className="flex items-center gap-3"> <span>{t('pages.timesheet.BILLABLE.YES')}</span> </div> </DropdownMenuItem> - <DropdownMenuItem textValue={'No'} className="cursor-pointer"> + <DropdownMenuItem + textValue={'No'} + className="cursor-pointer" + aria-label={t('pages.timesheet.BILLABLE.NO')} + > <div className="flex items-center gap-3"> <span>{t('pages.timesheet.BILLABLE.NO')}</span> </div> </DropdownMenuItem>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
apps/web/lib/features/integrations/calendar/table-time-sheet.tsx(8 hunks)
🔇 Additional comments (2)
apps/web/lib/features/integrations/calendar/table-time-sheet.tsx (2)
252-276: LGTM: Task display implementation with dark mode support
The task display implementation correctly handles dark mode with appropriate class names and styling:
- Proper border color handling with
dark:border-b-gray-600 - Shadow handling with
dark:shadow-transparent - Consistent styling across light and dark themes
210-213: Overall dark mode implementation looks good
The dark mode support has been successfully implemented across the timesheet component with appropriate styling for:
- Table background and borders
- Task items and their containers
- Badges and interactive elements
- Text colors and shadows
Note: Please address the dark mode class naming issues mentioned in the previous review.
Also applies to: 241-243, 256-256, 262-262
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (8)
apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetFilter.tsx (2)
25-26: Consider implementing proper status change trackingThe empty callback with just a comment might miss important state changes. Consider implementing proper tracking for analytics or state management.
onToggle={(label) => { - // If logging is needed, use proper logging service + // Track status changes for analytics + trackStatusChange(label); }}
34-39: Review the use of fixed height with important flagWhile the dark mode styling looks good, the use of
!h-[2.2rem]with an important flag might:
- Override the design system's default styling
- Affect the button's responsiveness on different screen sizes
Consider using the design system's built-in size variants or removing the important flag if possible.
- className="bg-primary/5 dark:bg-primary-light dark:border-transparent !h-[2.2rem] font-medium" + className="bg-primary/5 dark:bg-primary-light dark:border-transparent h-[2.2rem] font-medium"apps/web/lib/features/integrations/calendar/table-time-sheet.tsx (3)
256-256: Improve dark mode border contrastThe current border color (
border-b-gray-200) might not provide enough contrast in dark mode. Consider using a darker shade for better visibility.-className="flex items-center border-b border-b-gray-200 dark:border-b-gray-600 space-x-4 p-1" +className="flex items-center border-b border-b-gray-200 dark:border-b-gray-700 space-x-4 p-1"
298-299: Add dark mode styles for disabled pagination buttonsWhile the disabled states are properly implemented, consider adding dark mode specific styles for better visual feedback when buttons are disabled.
-disabled={!table.getCanPreviousPage()} +disabled={!table.getCanPreviousPage()} +className="dark:disabled:opacity-50 dark:disabled:cursor-not-allowed"Also applies to: 305-306, 312-313, 319-320
Line range hint
346-357: Enhance status color handling for dark modeThe current color implementation doesn't account for dark mode contrast. Consider adding dark mode specific colors for better visibility.
const getColorClass = () => { switch (selected) { case "Rejected": - return "text-red-500 border-gray-200"; + return "text-red-500 dark:text-red-400 border-gray-200 dark:border-gray-700"; case "Approved": - return "text-green-500 border-gray-200"; + return "text-green-500 dark:text-green-400 border-gray-200 dark:border-gray-700"; case "Pending": - return "text-orange-500 border-gray-200"; + return "text-orange-500 dark:text-orange-400 border-gray-200 dark:border-gray-700"; default: - return "text-gray-500 border-gray-200"; + return "text-gray-500 dark:text-gray-400 border-gray-200 dark:border-gray-700"; } };apps/web/app/api/timer/timesheet/daily/route.ts (3)
6-7: Re-evaluate the necessity of creating a newNextResponseinstance.The
resobject created withnew NextResponse()is only used as a parameter inauthenticatedGuard. IfauthenticatedGuarddoesn't require aresobject in this context, consider removingconst res = new NextResponse();to simplify the code.
37-38: Use appropriate logging mechanism for error handling.Using
console.errormay not be optimal for logging errors in a production environment. Consider implementing a structured logging system to capture and manage errors effectively.
35-41: Ensure consistent error response formats.The error responses use different message structures. For consistency and better client-side handling, consider standardizing the error response format across all error handlers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
apps/web/app/[locale]/timesheet/[memberId]/components/FilterWithStatus.tsx(1 hunks)apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetFilter.tsx(1 hunks)apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetFilterDate.tsx(5 hunks)apps/web/app/api/timer/timesheet/daily/route.ts(1 hunks)apps/web/lib/features/integrations/calendar/table-time-sheet.tsx(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/web/app/[locale]/timesheet/[memberId]/components/FilterWithStatus.tsx
- apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetFilterDate.tsx
🧰 Additional context used
📓 Learnings (1)
apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetFilter.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-10-30T16:31:01.294Z
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 (4)
apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetFilter.tsx (2)
20-21: LGTM! Improved layout structure
The transition from grid to flex layout with proper alignment properties improves the component's flexibility and maintainability.
30-33: LGTM! Well-organized control layout
The flex container with consistent spacing provides a clean layout for the filter controls.
apps/web/lib/features/integrations/calendar/table-time-sheet.tsx (2)
8-8: LGTM: Import statements are properly organized.
The changes to imports are clean and align with the component's requirements.
Also applies to: 12-12, 29-29, 62-62
Line range hint 210-463: Overall dark mode implementation looks good!
The dark mode support has been properly implemented across the timesheet component using Tailwind CSS conventions. The implementation includes:
- Proper background color transitions
- Consistent text color adjustments
- Status indicator adaptations
- Proper component theming
The suggested improvements are minor and focus on enhancing contrast and visual feedback in specific scenarios.
| if (!startDate || !endDate) { | ||
| return NextResponse.json( | ||
| { error: 'startDate and endDate are required' }, | ||
| { status: 400 } | ||
| ); | ||
| } |
There was a problem hiding this comment.
Validate the format of startDate and endDate.
Currently, startDate and endDate are accepted without validation. To prevent potential errors or security issues, consider validating that these parameters are valid dates before proceeding.
Apply this diff to add date validation:
if (!startDate || !endDate) {
return NextResponse.json(
{ error: 'startDate and endDate are required' },
{ status: 400 }
);
}
+
+// Validate date format
+const isValidDate = (dateStr) => !isNaN(Date.parse(dateStr));
+if (!isValidDate(startDate) || !isValidDate(endDate)) {
+ return NextResponse.json(
+ { error: 'Invalid date format for startDate or endDate' },
+ { status: 400 }
+ );
+}📝 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.
| if (!startDate || !endDate) { | |
| return NextResponse.json( | |
| { error: 'startDate and endDate are required' }, | |
| { status: 400 } | |
| ); | |
| } | |
| if (!startDate || !endDate) { | |
| return NextResponse.json( | |
| { error: 'startDate and endDate are required' }, | |
| { status: 400 } | |
| ); | |
| } | |
| // Validate date format | |
| const isValidDate = (dateStr) => !isNaN(Date.parse(dateStr)); | |
| if (!isValidDate(startDate) || !isValidDate(endDate)) { | |
| return NextResponse.json( | |
| { error: 'Invalid date format for startDate or endDate' }, | |
| { status: 400 } | |
| ); | |
| } |
There was a problem hiding this comment.
@Innocent-Akim, understood. Let me know if there's anything else I can assist with.
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
DataTableTimeSheetcomponent with an accordion format for better task detail presentation.Bug Fixes
Improvements
TimesheetFiltercomponent using flexbox for better child element arrangement.