Feat: Add grouped display for timesheet data by date #3266
Conversation
WalkthroughThe pull request introduces several modifications across multiple files related to the timesheet feature. Key changes include the refinement of the 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: 3
🧹 Outside diff range and nitpick comments (7)
apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetFilter.tsx (2)
Line range hint
31-33: Implement proper logging or remove the comment.The empty callback with a TODO-like comment should either be implemented with proper logging or removed if not needed.
Consider implementing proper logging:
- onToggle={(label) => { - // If logging is needed, use proper logging service - }} + onToggle={(label) => { + console.debug('Status filter toggled:', label); + }}
Line range hint
39-39: Consider explicit prop passing instead of spreading.Using the spread operator (
{...initDate}) with TypeScript can bypass type checking. Consider passing props explicitly for better type safety and code clarity.- <TimesheetFilterDate t={t} {...initDate} /> + <TimesheetFilterDate + t={t} + initialRange={initDate?.initialRange} + onChange={initDate?.onChange} + maxDate={initDate?.maxDate} + minDate={initDate?.minDate} + />apps/web/app/hooks/features/useTimesheet.ts (2)
15-16: Remove consecutive empty linesConsider removing one of the consecutive empty lines to maintain consistent spacing throughout the file.
67-67: Consider memoizing grouped timesheet dataThe
groupByDatefunction is called on every render. Consider memoizing the result usinguseMemoto prevent unnecessary recalculations.+ const groupedTimesheet = useMemo(() => groupByDate(timesheet), [timesheet]); + return { loadingTimesheet, - timesheet: groupByDate(timesheet), + timesheet: groupedTimesheet, getTaskTimesheet, };Don't forget to add
useMemoto the React imports:-import { useCallback, useEffect } from 'react'; +import { useCallback, useEffect, useMemo } from 'react';apps/web/app/[locale]/timesheet/[memberId]/page.tsx (3)
Line range hint
95-112: Replace hardcoded values with dynamic data.The TimesheetCard components contain hardcoded values for counts, hours, and dates.
Consider fetching these statistics from the timesheet data:
- Pending tasks count
- Total hours worked
- Number of active members
Line range hint
134-146: Implement search functionality.The search input is currently non-functional. Consider implementing the search feature to filter timesheet entries.
Would you like me to help implement the search functionality for the timesheet data?
Line range hint
182-196: Improve accessibility in ViewToggleButton.The toggle button component needs accessibility improvements.
Consider these enhancements:
<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' )}>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetFilter.tsx(1 hunks)apps/web/app/[locale]/timesheet/[memberId]/page.tsx(3 hunks)apps/web/app/hooks/features/useTimesheet.ts(2 hunks)apps/web/lib/features/integrations/calendar/table-time-sheet.tsx(0 hunks)
💤 Files with no reviewable changes (1)
- apps/web/lib/features/integrations/calendar/table-time-sheet.tsx
🔇 Additional comments (3)
apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetFilter.tsx (1)
12-12: LGTM! Type narrowing improves interface clarity.
The change to use Pick type makes the interface more precise by explicitly declaring required properties. This follows the interface segregation principle and reduces unnecessary prop drilling.
Let's verify no other properties from TimesheetFilterDateProps are being used:
✅ Verification successful
The previous regex was too complex. Let's simplify our approach by:
- Finding where TimesheetFilterDateProps is defined
- Understanding what properties it has
- Looking at how initDate is actually used in the codebase
This will help us verify if the type narrowing is safe and doesn't break existing code.
Type narrowing is correct and safe to use
The verification confirms that:
TimesheetFilterDatePropsinterface only has 5 properties:onChange,initialRange,minDate,maxDate, andt- The
Picktype correctly includes all properties being used (initialRange,onChange,maxDate,minDate) - The actual usage in the codebase only uses these picked properties when spreading
initDateintoTimesheetFilterDatecomponent
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any usage of TimesheetFilterDateProps properties other than the picked ones
rg -l 'TimesheetFilterDateProps' | xargs rg -A 5 -B 5 '\b(?!initialRange|onChange|maxDate|minDate\b)[a-zA-Z]+(?=\s*[=:])'
Length of output: 392
Script:
#!/bin/bash
# First, let's find all files containing TimesheetFilterDateProps
rg -l "TimesheetFilterDateProps"
# Then, let's examine the interface definition
rg -A 5 "interface TimesheetFilterDateProps"
# Finally, let's look at actual usage of initDate
rg -B 3 -A 3 "initDate"
Length of output: 3640
apps/web/app/hooks/features/useTimesheet.ts (1)
Line range hint 39-41: Improve error handling in getTaskTimesheet
Currently, errors are only logged to the console, which doesn't provide feedback to the user. Consider implementing proper error handling and user notification.
}).catch((error) => {
- console.error('Error fetching timesheet:', error);
+ const errorMessage = error?.response?.data?.message || 'Failed to fetch timesheet data';
+ console.error('Error fetching timesheet:', errorMessage);
+ // Assuming you have a notification system
+ notify({
+ type: 'error',
+ message: errorMessage
+ });
});#!/bin/bash
# Check if notification system is available in the codebase
rg -l "notify.*error" --type tsapps/web/app/[locale]/timesheet/[memberId]/page.tsx (1)
11-11: Verify the removal of useDailyPlan dependency.
The transition from useDailyPlan to useTimesheet represents a significant architectural change. Let's ensure all daily plan functionalities are properly migrated.
Innocent-Akim
added
bug
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
TaskActionMenu,TaskDetails, andStatusTask.Bug Fixes
Documentation