[Feat]: Add Component for Timesheet Creation and Optimize Task Button #3359
Conversation
WalkthroughThe changes introduced in this pull request involve the addition and modification of several components related to timesheet management in a web application. A new Changes
Possibly related PRs
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 (1.23.1)
apps/web/app/[locale]/timesheet/[memberId]/components/FilterWithStatus.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: 8
🧹 Outside diff range and nitpick comments (17)
apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetFilter.tsx (2)
15-17: Add JSDoc comments for the new data propThe new
dataprop's type signature is well-structured, but adding documentation would help explain its purpose and expected format to other developers.interface ITimesheetFilter { filterStatus?: FilterStatus, + /** Record mapping timesheet status to array of timesheet logs + * Used to display count of tasks in each status category + */ data?: Record<TimesheetStatus, TimesheetLog[]> }
Line range hint
44-46: Consider updating button text for consistencySince we've switched from manual time entry to task management, consider updating the button text from
'common.ADD_TIME'to something more task-oriented like'common.ADD_TASK'.apps/web/app/[locale]/timesheet/[memberId]/components/FilterWithStatus.tsx (2)
21-28: Consider using distinct icons for different statusesCurrently, some different statuses share the same icon:
- 'In review' and 'Rejected' both use 'icon-rejected'
- 'Draft' and 'Approved' both use 'icon-approved'
This might confuse users about the actual status of their timesheets.
Consider using unique icons for each status:
const statusIcons: Record<FilterStatus, string> = { 'All Tasks': 'icon-all', Pending: 'icon-pending', Approved: 'icon-approved', - 'In review': 'icon-rejected', + 'In review': 'icon-review', - Draft: 'icon-approved', + Draft: 'icon-draft', Rejected: 'icon-rejected', };
Line range hint
8-69: Consider separating data transformation logicThe component currently handles both data transformation and presentation. Consider extracting the data transformation logic into a custom hook for better separation of concerns and reusability.
Example structure:
// useFilterStatus.ts export function useFilterStatus(data: Record<TimesheetStatus, TimesheetLog[]>) { const statusIcons = {...}; // Move statusIcons here const buttonData = React.useMemo(() => { // Move buttonData calculation here return [...]; }, [data]); return { buttonData, statusIcons }; } // FilterWithStatus.tsx export function FilterWithStatus({ data, ...props }) { const { buttonData } = useFilterStatus(data); return (...); // Focus on presentation only }apps/web/app/hooks/features/useTimesheet.ts (4)
171-172: Optimize array flattening operationThe changes look good! However, since we're only flattening the array without transforming its elements, we can optimize by using
flat()instead offlatMap().- statusTimesheet: getStatusTimesheet(timesheet.flatMap((data) => data)) + statusTimesheet: getStatusTimesheet(timesheet.flat())🧰 Tools
🪛 Biome (1.9.4)
[error] 172-172: Avoid unnecessary callback in flatMap call.
You can just use flat to flatten the array.
Safe fix: Replace unnecessary flatMap call to flat instead.(lint/correctness/noFlatMapIdentity)
Line range hint
73-89: Enhance error handling for invalid timesheet statusesThe status grouping logic is well-structured, but we could improve the error handling for invalid statuses to aid in debugging.
const getStatusTimesheet = (items: TimesheetLog[] = []) => { const STATUS_MAP: Record<TimesheetStatus, TimesheetLog[]> = { PENDING: [], APPROVED: [], DENIED: [], DRAFT: [], 'IN REVIEW': [] }; return items.reduce((acc, item) => { const status = item.timesheet.status; if (isTimesheetStatus(status)) { acc[status].push(item); } else { - console.warn(`Invalid timesheet status: ${status}`); + console.warn( + `Invalid timesheet status: ${status}`, + `for timesheet ID: ${item.timesheet.id}`, + `Expected one of: ${Object.keys(STATUS_MAP).join(', ')}` + ); } return acc; }, STATUS_MAP); }🧰 Tools
🪛 Biome (1.9.4)
[error] 172-172: Avoid unnecessary callback in flatMap call.
You can just use flat to flatten the array.
Safe fix: Replace unnecessary flatMap call to flat instead.(lint/correctness/noFlatMapIdentity)
Line range hint
91-102: Ensure consistency between status definitionsThe type guard implementation could be improved by using the same source of truth for status values.
function isTimesheetStatus(status: unknown): status is TimesheetStatus { - const timesheetStatusValues: TimesheetStatus[] = [ - "DRAFT", - "PENDING", - "IN REVIEW", - "DENIED", - "APPROVED" - ]; + const STATUS_MAP: Record<TimesheetStatus, TimesheetLog[]> = { + PENDING: [], + APPROVED: [], + DENIED: [], + DRAFT: [], + 'IN REVIEW': [] + }; + const timesheetStatusValues = Object.keys(STATUS_MAP) as TimesheetStatus[]; return Object.values(timesheetStatusValues).includes(status as TimesheetStatus); }🧰 Tools
🪛 Biome (1.9.4)
[error] 172-172: Avoid unnecessary callback in flatMap call.
You can just use flat to flatten the array.
Safe fix: Replace unnecessary flatMap call to flat instead.(lint/correctness/noFlatMapIdentity)
Line range hint
1-173: Consider architectural improvements for better maintainabilityThe hook implementation is solid, but there are a few architectural improvements that could make it more maintainable:
- Extract the status map to a constant at the module level to avoid duplication
- Consider splitting the hook into smaller, more focused hooks (e.g.,
useTimesheetStatus,useTimesheetOperations)- Implement consistent error handling across all operations
Here's how you could extract the status map:
const TIMESHEET_STATUS_MAP: Record<TimesheetStatus, TimesheetLog[]> = { PENDING: [], APPROVED: [], DENIED: [], DRAFT: [], 'IN REVIEW': [] } as const;Would you like me to help create a proposal for splitting this hook into smaller, more focused hooks?
🧰 Tools
🪛 Biome (1.9.4)
[error] 172-172: Avoid unnecessary callback in flatMap call.
You can just use flat to flatten the array.
Safe fix: Replace unnecessary flatMap call to flat instead.(lint/correctness/noFlatMapIdentity)
apps/web/app/[locale]/timesheet/[memberId]/components/EditTaskModal.tsx (3)
Line range hint
26-156: Add form validation and error handlingThe form lacks proper validation and error handling mechanisms:
- Required fields are marked with asterisks but can be submitted empty
- Time inputs need validation for valid ranges and end time > start time
- Empty handlers for
handleSelectedValuesChangeandhandleChange- No loading states or error handling for form submission
Consider implementing form validation using a form management library like
react-hook-formorformik. Example implementation:import { useForm } from 'react-hook-form'; interface TaskFormData { startTime: string; endTime: string; date: Date; project: string; notes: string; isBillable: boolean; } const { register, handleSubmit, formState: { errors, isSubmitting } } = useForm<TaskFormData>(); const onSubmit = async (data: TaskFormData) => { try { // Handle form submission } catch (error) { // Handle error } };
Line range hint
213-227: Enhance ToggleButton accessibility and stylingThe ToggleButton component needs improvements in several areas:
- Missing ARIA attributes for accessibility
- Using inline styles instead of Tailwind classes
- Lacking hover/focus states
- No keyboard navigation support
Consider this improved implementation:
export const ToggleButton = ({ isActive, onClick, label }: ToggleButtonProps) => ( - <div className="flex items-center gap-x-2"> + <div className="flex items-center gap-x-2 group"> <div - className="w-6 h-6 flex items-center bg-[#6c57f4b7] rounded-full p-1 cursor-pointer" + className={clsxm( + "w-6 h-6 flex items-center rounded-full p-1 cursor-pointer", + "focus:outline-none focus:ring-2 focus:ring-offset-2 focus:ring-primary", + "transition-colors duration-200 ease-in-out", + isActive ? "bg-gradient-to-r from-[#9d91efb7] to-[#8a7bedb7]" : "bg-gray-800", + "hover:opacity-90" + )} onClick={onClick} - style={{ - background: isActive ? 'linear-gradient(to right, #9d91efb7, #8a7bedb7)' : '#1f2937' - }} + role="switch" + aria-checked={isActive} + tabIndex={0} + onKeyDown={(e) => { + if (e.key === 'Enter' || e.key === ' ') { + e.preventDefault(); + onClick(); + } + }} > <div className={clsxm( - 'bg-[#3826A6] w-4 h-4 rounded-full shadow-md transform transition-transform translate-x-0', - isActive && 'bg-white' + 'w-4 h-4 rounded-full shadow-md transform transition-transform duration-200', + 'translate-x-0', + isActive ? 'bg-white' : 'bg-[#3826A6]' )} /> </div> - <span>{label}</span> + <span className="select-none">{label}</span> </div> );
Line range hint
39-49: Implement handlers and improve type safetyThe component has several unimplemented features and type safety issues:
- Empty handler functions
- Loose typing on selectedValues
- No cleanup for state on unmount
Consider implementing the handlers and improving type safety:
interface SelectedValues { Teams: Item | null; [key: string]: Item | null; } const [selectedValues, setSelectedValues] = useState<SelectedValues>({ Teams: null }); const handleSelectedValuesChange = (values: SelectedValues) => { setSelectedValues(values); }; const handleChange = (field: keyof SelectedValues, selectedItem: Item | null) => { setSelectedValues(prev => ({ ...prev, [field]: selectedItem })); }; // Cleanup on unmount useEffect(() => { return () => { // Cleanup state if needed }; }, []);apps/web/app/[locale]/timesheet/[memberId]/components/AddTaskModal.tsx (1)
14-19: Fix state setter naming for consistency.The state setter name
setTasksdoesn't match the singular nature of the state variabletask.-const [task, setTasks] = React.useState('') +const [task, setTask] = React.useState('')apps/web/app/[locale]/timesheet/[memberId]/page.tsx (1)
Line range hint
41-65: Consider performance optimizations.A few suggestions to improve performance:
- Debounce the search input to prevent excessive filtering on every keystroke
- Memoize the date range state to prevent unnecessary re-renders
- Consider using
useCallbackfor event handlersExample implementation for search debouncing:
import { useMemo, useCallback } from 'react'; import debounce from 'lodash/debounce'; // Memoize the debounced search handler const debouncedSetSearch = useMemo( () => debounce((value: string) => setSearch(value), 300), [] ); // Memoize the onChange handler const handleSearchChange = useCallback( (e: React.ChangeEvent<HTMLInputElement>) => { debouncedSetSearch(e.target.value); }, [debouncedSetSearch] );apps/web/lib/features/integrations/calendar/table-time-sheet.tsx (4)
Line range hint
192-201: Enhance error handling in handleConfirmThe current error handling implementation has several issues:
- No user feedback on error
- State updates proceed even after error
- Redundant error logging
Consider this improved 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 notification here + toast.error('Failed to delete timesheet. Please try again.'); + }); };
399-399: Consider extracting status-related logicWhile the internationalization is correctly implemented, there are opportunities for improvement:
- Extract status color mapping to a constant:
const STATUS_COLOR_CLASSES = { Rejected: 'text-red-500 border-gray-200', Approved: 'text-green-500 border-gray-200', Pending: 'text-orange-500 border-gray-200', default: 'text-gray-500 border-gray-200' } as const;
- Consider moving status options to a configuration file and adding translations:
export const STATUS_OPTIONS = { REJECTED: 'pages.timesheet.status.rejected', APPROVED: 'pages.timesheet.status.approved', PENDING: 'pages.timesheet.status.pending' } as const;Also applies to: 431-431
Line range hint
500-513: Improve type safety and key handling in status mappingThere are several potential issues in the status mapping implementation:
- Add type safety for status objects:
- {statusTable?.map((status, index) => ( + {statusTable?.map((status: { label: TimesheetStatus }, index) => (
- Use meaningful keys instead of array indices:
- <DropdownMenuItem key={index} textValue={status.label} className="cursor-pointer"> + <DropdownMenuItem key={status.label} textValue={status.label} className="cursor-pointer">
- Consider adding error boundaries or fallback UI for when statusTable is undefined:
if (!statusTable?.length) { return <DropdownMenuItem disabled>No status options available</DropdownMenuItem>; }
Approval logic implementation required
The verification confirms there is a TODO comment for implementing approval logic in
apps/web/lib/features/integrations/calendar/table-time-sheet.tsx. Since this is a documented placeholder and there's no existing implementation, this is a valid concern that needs to be addressed.
apps/web/lib/features/integrations/calendar/table-time-sheet.tsx: Missing approval logic implementation at the TODO comment🔗 Analysis chain
Line range hint
267-269: Verify implementation status of approval logicThe TODO comment indicates missing approval logic implementation. This could affect the functionality of the timesheet approval flow.
Let's check for any existing approval implementations:
Would you like me to help implement the approval logic or create a GitHub issue to track this task?
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for approval-related implementations echo "Searching for approval-related code..." rg -l "handleApproval|approveTimesheet" --type ts --type tsx # Search for similar TODO comments echo "Checking for related TODO comments..." rg "TODO.*implement.*approv" --type ts --type tsxLength of output: 342
Script:
#!/bin/bash # Search for approval-related implementations with correct file type flags echo "Searching for approval-related code..." rg -l "handleApproval|approveTimesheet" -t typescript -t tsx # Search for TODO comments about approval echo "Checking for related TODO comments..." rg "TODO.*approv" -t typescript -t tsx # Check the specific file for TODO comments echo "Checking TODOs in the timesheet file..." rg "TODO" "apps/web/lib/features/integrations/calendar/table-time-sheet.tsx"Length of output: 547
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
apps/web/app/[locale]/timesheet/[memberId]/components/AddTaskModal.tsx(1 hunks)apps/web/app/[locale]/timesheet/[memberId]/components/EditTaskModal.tsx(1 hunks)apps/web/app/[locale]/timesheet/[memberId]/components/FilterWithStatus.tsx(2 hunks)apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetFilter.tsx(2 hunks)apps/web/app/[locale]/timesheet/[memberId]/page.tsx(3 hunks)apps/web/app/hooks/features/useTimesheet.ts(1 hunks)apps/web/lib/features/integrations/calendar/table-time-sheet.tsx(7 hunks)
🧰 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-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.
🪛 Biome (1.9.4)
apps/web/app/[locale]/timesheet/[memberId]/components/AddTaskModal.tsx
[error] 76-76: Avoid unnecessary callback in flatMap call.
You can just use flat to flatten the array.
Safe fix: Replace unnecessary flatMap call to flat instead.
(lint/correctness/noFlatMapIdentity)
apps/web/app/hooks/features/useTimesheet.ts
[error] 172-172: Avoid unnecessary callback in flatMap call.
You can just use flat to flatten the array.
Safe fix: Replace unnecessary flatMap call to flat instead.
(lint/correctness/noFlatMapIdentity)
🪛 GitHub Check: Cspell
apps/web/app/[locale]/timesheet/[memberId]/components/AddTaskModal.tsx
[warning] 28-28:
Unknown word (Preject)
🔇 Additional comments (8)
apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetFilter.tsx (1)
5-6: LGTM! Import changes align with requirements
The replacement of AddManualTimeModal with AddTaskModal aligns with previous decisions and the PR objectives for task management optimization.
apps/web/app/[locale]/timesheet/[memberId]/components/FilterWithStatus.tsx (1)
58-58: LGTM: UI enhancement for active state
The added shadow and border improve the visual feedback for the active filter state, making it more prominent and user-friendly.
apps/web/app/[locale]/timesheet/[memberId]/components/AddTaskModal.tsx (1)
1-13: LGTM! Clean imports and well-defined interface.
The imports are properly organized and the interface clearly defines the required props.
apps/web/app/[locale]/timesheet/[memberId]/page.tsx (3)
214-214: LGTM: Improved dark mode support.
The styling changes correctly implement dark mode variants while maintaining the existing active state behavior.
47-50: Verify the useTimesheet hook implementation.
The hook now destructures an additional statusTimesheet value. Please ensure that:
- The hook properly implements this new value
- The value is correctly computed based on the date range parameters
✅ Verification successful
The useTimesheet hook implementation is correct and properly supports statusTimesheet
The hook implementation correctly provides the statusTimesheet value through the getStatusTimesheet function, which:
- Takes timesheet logs and groups them by their status (PENDING, APPROVED, DENIED, DRAFT, IN REVIEW)
- Is properly exposed in the hook's return value
- Works with the date range parameters as it processes the timesheet data fetched based on these dates
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the useTimesheet hook implementation
ast-grep --pattern 'export const useTimesheet = $_'
Length of output: 53
Script:
#!/bin/bash
# Search for useTimesheet hook implementation with a broader pattern and ripgrep
rg "useTimesheet" -A 10
# Also search for any file containing timesheet in its name that might contain the hook
fd "timesheet" --type f
Length of output: 6963
Script:
#!/bin/bash
# Let's examine the useTimesheet hook implementation
cat apps/web/app/hooks/features/useTimesheet.ts
Length of output: 5782
Line range hint 171-182: Verify TimesheetFilter component prop types.
The component now receives additional data prop containing timesheet status information. Please ensure that:
- The prop types are properly defined
- The component handles the new data prop correctly
✅ Verification successful
Let me try a different search pattern to find the TimesheetFilter component definition.
Let me try searching specifically for the TimesheetFilter component definition.
Let me check the TimesheetFilter component definition to verify its props.
Props are properly defined and match the component usage
The TimesheetFilter component has a properly defined interface ITimesheetFilter that includes all the props being passed, including:
data?: Record<TimesheetStatus, TimesheetLog[]>for the timesheet status datafilterStatus?: FilterStatusfor the filter statusonChangeStatus?: (status: FilterStatus) => voidfor status change handlinginitDatefor date range handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the TimesheetFilter component definition
ast-grep --pattern 'type $_ = {
$$$
data: $_
$$$
}'
Length of output: 898
Script:
#!/bin/bash
# Search for TimesheetFilter component and its props
ast-grep --pattern 'interface $_ {
$$$
}'
# Also search for type definitions
ast-grep --pattern 'type $_ = {
$$$
}'
# Backup search using ripgrep
rg -A 10 "TimesheetFilter.*Props"
Length of output: 100310
Script:
#!/bin/bash
# Search for TimesheetFilter component file
fd TimesheetFilter
# Also search for any file containing TimesheetFilter
rg -l "TimesheetFilter"
Length of output: 536
Script:
#!/bin/bash
# Get the content of TimesheetFilter component
cat "apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetFilter.tsx"
Length of output: 2066
apps/web/lib/features/integrations/calendar/table-time-sheet.tsx (2)
55-56: LGTM! Import changes align with new functionality
The addition of getTimesheetButtons and statusTable imports supports the enhanced status management functionality.
449-450: LGTM! Internationalization properly implemented
The translation implementation for menu items follows best practices.
Also applies to: 463-463, 468-468
apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetFilter.tsx
Show resolved
Hide resolved
apps/web/app/[locale]/timesheet/[memberId]/components/FilterWithStatus.tsx
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/AddTaskModal.tsx
Outdated
Show resolved
Hide resolved
apps/web/app/[locale]/timesheet/[memberId]/components/AddTaskModal.tsx
Outdated
Show resolved
Hide resolved
apps/web/app/[locale]/timesheet/[memberId]/components/AddTaskModal.tsx
Outdated
Show resolved
Hide resolved
Innocent-Akim
force-pushed
the
feat/timesheet-creation-task-buttons
branch
from
97521cb to
6af3dc0
Compare
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Outside diff range and nitpick comments (12)
apps/web/app/stores/time-logs.ts (1)
19-20: Consider enhancing state persistence and configurationThe new
timesheetGroupByDayStateatom is well-typed and follows proper conventions. However, consider these improvements:
- The default value could be moved to a configuration constant
- The state could be persisted across sessions if this is a user preference
Consider this refactor:
+// At the top of the file with other constants +export const DEFAULT_TIMESHEET_GROUP_BY = 'Daily' as const; -export const timesheetGroupByDayState = atom<TimesheetFilterByDays>('Daily') +export const timesheetGroupByDayState = atom<TimesheetFilterByDays>( + // Get from localStorage or fall back to default + (typeof window !== 'undefined' && localStorage.getItem('timesheetGroupBy') as TimesheetFilterByDays) + || DEFAULT_TIMESHEET_GROUP_BY +);apps/web/app/hooks/features/useTimelogFilterOptions.ts (1)
11-11: LGTM! State management implementation follows best practicesThe implementation:
- Uses Jotai's
useAtomhook consistently with other state management in the file- Follows the established pattern of exposing both state and setter
- Maintains consistent naming conventions
Consider documenting the expected values and usage of
timesheetGroupByDaysin a comment above the state initialization, as this appears to be a key feature for timesheet organization.Also applies to: 36-37
apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetAction.tsx (1)
Line range hint
34-72: Consider improving accessibility and maintainabilityA few suggestions to enhance this section:
- Move color values (e.g.,
#2932417c) to a theme system for better maintainability- Add aria-labels to icon buttons for better accessibility
Example improvement:
<TimesheetButton className="hover:underline text-sm gap-2" disabled={disabled} key={index} icon={button.icon} + aria-label={button.title} onClick={() => onClick(button.action)} title={button.title} />apps/web/app/interfaces/ITask.ts (1)
147-150: LGTM! Consider using an enum for better type safety.The new
TimesheetFilterByDaystype is well-defined and appropriately placed. However, since these are fixed values that represent distinct cases, consider using an enum instead of a union type for better type safety and maintainability.Here's a suggested enhancement:
-export type TimesheetFilterByDays = - | "Daily" - | "Weekly" - | "Monthly" +export enum TimesheetFilterByDays { + Daily = "Daily", + Weekly = "Weekly", + Monthly = "Monthly" +}This change would:
- Prevent accidental string assignments
- Enable using the enum as a type and value
- Make it easier to add new filter options in the future
- Provide better IDE support for value completion
apps/web/app/[locale]/timesheet/[memberId]/components/FrequencySelect.tsx (2)
36-52: LGTM: Clean state management implementation.The frequency selection logic is well-implemented with proper type safety. However, consider simplifying the
handleSelectChangefunction since it's a simple passthrough.Optional improvement:
-const handleSelectChange = (value: string) => { - setTimesheetGroupByDays(value as TimesheetFilterByDays); -}; <Select value={timesheetGroupByDays} - onValueChange={handleSelectChange}> + onValueChange={(value) => setTimesheetGroupByDays(value as TimesheetFilterByDays)}>
The FrequencySelect component should be removed as it's redundant with TimesheetFilterDate
The codebase shows that:
- PR [Feat]: Implement dynamic date range picker for timesheet filtering #3213 implemented the date range filtering functionality through
TimesheetFilterDatecomponentTimesheetFilterDatealready includes all the functionality present inFrequencySelect:
- Today filter
- Last 7 days
- Last 30 days
- This year
- Custom date range with date picker
The
FrequencySelectcomponent in the reviewed file appears to be a duplicate implementation that should be removed, as the proper date range filtering is already handled byTimesheetFilterDatewhich is being used in the sameTimesheetFiltercomponent.🔗 Analysis chain
Line range hint
59-146: Verify the status of date range selection implementation.Based on previous learnings (PR #3208), the date range selection functionality was planned for a future PR. However, this PR includes a basic implementation. Please clarify if:
- This is the planned implementation mentioned in PR Feat: Implement Timesheet Filter Component With Status, Date Filters #3208
- This implementation might conflict with or duplicate future changes
Let's check for any related implementations or discussions:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for similar date range implementations rg -l "CustomDateRange|FilterTaskActionMenu" --type tsx # Search for related PR discussions gh pr list --search "date range in:title,body" --state all --limit 5Length of output: 791
Script:
#!/bin/bash # Let's check the content of PR #3213 which seems directly related gh pr view 3213 --json body # Search for similar date range implementations with correct file extension rg -l "CustomDateRange|FilterTaskActionMenu" --type ts --type tsx --type jsx --type js # Look for any other date range related components rg -l "DateRange|date.*range" --type ts --type tsx --type jsx --type jsLength of output: 1730
Script:
#!/bin/bash # Try with just the file extension without --type flag rg -l "CustomDateRange|FilterTaskActionMenu" .*\.[tj]sx?$ # Search for TimesheetFilterDate mentioned in PR #3213 rg -l "TimesheetFilterDate" .*\.[tj]sx?$ # Look for any files with timesheet and filter in their path fd -e tsx -e ts -e js -e jsx . | grep -i "timesheet.*filter"Length of output: 577
Script:
#!/bin/bash # Check the content of TimesheetFilterDate.tsx which seems to be the main implementation cat "apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetFilterDate.tsx" # Check TimesheetFilter.tsx to understand the overall filter structure cat "apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetFilter.tsx" # Check if FrequencySelect.tsx is being used in these files rg "FrequencySelect" "apps/web/app/[locale]/timesheet/[memberId]/components/Timesheet.*"Length of output: 13524
🧰 Tools
🪛 Biome (1.9.4)
[error] 34-34: Unexpected empty array pattern.
(lint/correctness/noEmptyPattern)
apps/web/app/hooks/features/useTimesheet.ts (3)
54-59: Add type annotation and consider edge cases.The week calculation looks correct, but consider adding a return type annotation and handling edge cases around year boundaries.
-const getWeekYearKey = (date: Date): string => { +const getWeekYearKey = (date: Date): `${number}-W${number}` => {
222-230: Enhance memoization robustness.The memoization logic could be more robust by:
- Using a type-safe discriminator for grouping options
- Handling invalid grouping options
type GroupingOption = 'Daily' | 'Weekly' | 'Monthly'; const timesheetElementGroup = useMemo(() => { switch (timesheetGroupByDays as GroupingOption) { case 'Daily': return groupByDate(timesheet); case 'Weekly': return groupByWeek(timesheet); case 'Monthly': return groupByMonth(timesheet); default: console.warn(`Invalid grouping option: ${timesheetGroupByDays}, falling back to Daily`); return groupByDate(timesheet); } }, [timesheetGroupByDays, timesheet]);
Line range hint
123-245: Standardize error handling and improve error messages.Consider implementing a consistent error handling strategy:
- Add error boundaries for React components using this hook
- Improve error messages with more context
- Consider adding error state to the hook's return value
Example implementation:
interface TimesheetError { code: string; message: string; context?: unknown; } // Add to return object error?: TimesheetError; setError: (error: TimesheetError | undefined) => void; // Use in error handlers catch (error) { setError({ code: 'TIMESHEET_FETCH_ERROR', message: 'Failed to fetch timesheet data', context: error }); }🧰 Tools
🪛 Biome (1.9.4)
[error] 245-245: Avoid unnecessary callback in flatMap call.
You can just use flat to flatten the array.
Safe fix: Replace unnecessary flatMap call to flat instead.(lint/correctness/noFlatMapIdentity)
apps/web/lib/features/integrations/calendar/table-time-sheet.tsx (3)
508-513: Add type safety to status table mappingThe current implementation could benefit from stronger typing to prevent potential runtime errors.
Consider adding type safety:
- {statusTable?.map((status, index) => ( + {statusTable?.map((status: { label: TimesheetStatus }, index) => (
403-403: Consider centralizing translation keysWhile the translations are correctly implemented, maintaining translation keys across the file could become challenging as the component grows.
Consider creating a constant object for translation keys at the top of the file:
const TRANSLATION_KEYS = { common: { STATUS: 'common.STATUS', EDIT: 'common.EDIT', DELETE: 'common.DELETE' }, timesheet: { BILLABLE: 'pages.timesheet.BILLABLE.BILLABLE' } } as const;Then use it throughout the component:
t(TRANSLATION_KEYS.common.EDIT)Also applies to: 453-454, 471-471, 476-476, 521-521
Line range hint
156-171: Enhance error handling for timesheet deletionThe current error handling only logs to console, which doesn't provide feedback to users when operations fail.
Consider implementing proper error handling:
const handleConfirm = () => { try { deleteTaskTimesheet() .then(() => { setSelectTimesheet([]); setIsDialogOpen(false); + // Show success notification + toast.success(t('common.DELETE_SUCCESS')); }) .catch((error) => { console.error('Delete timesheet error:', error); + // Show error notification + toast.error(t('common.DELETE_ERROR')); + setIsDialogOpen(false); }); } catch (error) { console.error('Delete timesheet error:', error); + // Show error notification + toast.error(t('common.DELETE_ERROR')); + setIsDialogOpen(false); } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
apps/web/app/[locale]/timesheet/[memberId]/components/AddTaskModal.tsx(1 hunks)apps/web/app/[locale]/timesheet/[memberId]/components/FilterWithStatus.tsx(2 hunks)apps/web/app/[locale]/timesheet/[memberId]/components/FrequencySelect.tsx(1 hunks)apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetAction.tsx(3 hunks)apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetFilter.tsx(2 hunks)apps/web/app/[locale]/timesheet/[memberId]/page.tsx(3 hunks)apps/web/app/hooks/features/useTimelogFilterOptions.ts(3 hunks)apps/web/app/hooks/features/useTimesheet.ts(3 hunks)apps/web/app/interfaces/ITask.ts(1 hunks)apps/web/app/stores/time-logs.ts(2 hunks)apps/web/lib/features/integrations/calendar/table-time-sheet.tsx(10 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetFilter.tsx
- apps/web/app/[locale]/timesheet/[memberId]/page.tsx
🧰 Additional context used
📓 Learnings (1)
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-11-12T14:06:02.202Z
Learning: The date range selection functionality for the date range options in the `FilterTaskActionMenu` component will be implemented in a future PR.
🪛 Biome (1.9.4)
apps/web/app/[locale]/timesheet/[memberId]/components/AddTaskModal.tsx
[error] 77-77: Avoid unnecessary callback in flatMap call.
You can just use flat to flatten the array.
Safe fix: Replace unnecessary flatMap call to flat instead.
(lint/correctness/noFlatMapIdentity)
apps/web/app/[locale]/timesheet/[memberId]/components/FrequencySelect.tsx
[error] 34-34: Unexpected empty array pattern.
(lint/correctness/noEmptyPattern)
apps/web/app/hooks/features/useTimesheet.ts
[error] 245-245: Avoid unnecessary callback in flatMap call.
You can just use flat to flatten the array.
Safe fix: Replace unnecessary flatMap call to flat instead.
(lint/correctness/noFlatMapIdentity)
🔇 Additional comments (8)
apps/web/app/stores/time-logs.ts (1)
3-3: LGTM! Clean import addition
The import statement is well-organized, grouping the new TimesheetFilterByDays type with related interfaces.
apps/web/app/hooks/features/useTimelogFilterOptions.ts (1)
1-1: Verify the usage of timesheetGroupByDayState atom
The atom import looks correct, but let's verify its usage across the codebase to ensure proper integration.
✅ Verification successful
Based on the shell script results, I can now generate the final response as the usage of timesheetGroupByDayState is well-documented across the codebase:
The timesheetGroupByDayState atom is properly integrated and used
The atom is correctly defined in the stores and consistently used across the codebase:
- Defined as a Jotai atom in
apps/web/app/stores/time-logs.ts - Properly consumed in
useTimelogFilterOptionshook - Actively used in multiple components:
useTimesheet.tsfor grouping logic (Daily/Weekly/Monthly)FrequencySelect.tsxfor frequency selection UItable-time-sheet.tsxfor display formatting
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check usage of timesheetGroupByDayState atom
# Expected: Find references in components using this grouping functionality
# Search for direct usage of the atom
echo "Searching for direct atom usage..."
rg "timesheetGroupByDayState" -A 3
# Search for the state variables to understand component integration
echo "Searching for state variable usage..."
rg "timesheetGroupByDays" -A 3
Length of output: 6007
apps/web/app/[locale]/timesheet/[memberId]/components/FilterWithStatus.tsx (3)
5-5: LGTM: Type definitions are well-structured
The import and type definitions are clear and properly organized.
Also applies to: 7-7
11-16: Skip: Issue already identified in previous review
The nullability handling issue has been previously identified and a solution has been proposed.
58-58: LGTM: Style enhancement for active status
The addition of border styling for the active status button improves visual feedback.
apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetAction.tsx (1)
74-78: 🛠️ Refactor suggestion
Enhance DailyTable implementation
A few suggestions for improvement:
- Use translation keys for internationalization support
- Add more descriptive explanations in the descriptions
- Consider adding type validation for the label values
Here's a suggested implementation:
export const DailyTable: { label: TimesheetFilterByDays; description: string }[] = [
- { label: "Daily", description: 'Group by Daily' },
- { label: "Weekly", description: 'Group by Weekly' },
- { label: "Monthly", description: 'Group by Monthly' },
+ {
+ label: "Daily",
+ description: t('pages.timesheet.GROUP_DAILY_DESC')
+ },
+ {
+ label: "Weekly",
+ description: t('pages.timesheet.GROUP_WEEKLY_DESC')
+ },
+ {
+ label: "Monthly",
+ description: t('pages.timesheet.GROUP_MONTHLY_DESC')
+ },
];Let's verify if these strings are already defined in the translation files:
apps/web/app/[locale]/timesheet/[memberId]/components/FrequencySelect.tsx (1)
25-26: LGTM: Import statements are properly structured.
The new imports for hooks and types are correctly added and align with the component's enhanced functionality.
apps/web/app/[locale]/timesheet/[memberId]/components/AddTaskModal.tsx (1)
1-13: LGTM! Clean imports and well-defined interface.
The imports are properly organized and the interface clearly defines the required props.
apps/web/app/[locale]/timesheet/[memberId]/components/FilterWithStatus.tsx
Outdated
Show resolved
Hide resolved
apps/web/app/[locale]/timesheet/[memberId]/components/FilterWithStatus.tsx
Show resolved
Hide resolved
apps/web/app/[locale]/timesheet/[memberId]/components/FrequencySelect.tsx
Outdated
Show resolved
Hide resolved
apps/web/lib/features/integrations/calendar/table-time-sheet.tsx
Outdated
Show resolved
Hide resolved
Innocent-Akim
removed
the
Don't Merge
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
apps/web/app/hooks/features/useTimesheet.ts (1)
202-210: Enhance type safety for grouping selection.Consider using a type-safe approach for the grouping selection to prevent potential runtime errors.
+type TimesheetGroupByDays = 'Daily' | 'Weekly' | 'Monthly'; const timesheetElementGroup = useMemo(() => { - if (timesheetGroupByDays === 'Daily') { - return groupByDate(timesheet); - } - if (timesheetGroupByDays === 'Weekly') { - return groupByWeek(timesheet); - } - return groupByMonth(timesheet); + const groupingMap: Record<TimesheetGroupByDays, (items: TimesheetLog[]) => GroupedTimesheet[]> = { + 'Daily': groupByDate, + 'Weekly': groupByWeek, + 'Monthly': groupByMonth + }; + return groupingMap[timesheetGroupByDays](timesheet); }, [timesheetGroupByDays, timesheet]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
apps/web/app/[locale]/timesheet/[memberId]/components/FilterWithStatus.tsx(2 hunks)apps/web/app/[locale]/timesheet/[memberId]/components/FrequencySelect.tsx(1 hunks)apps/web/app/hooks/features/useTimesheet.ts(3 hunks)apps/web/lib/features/integrations/calendar/table-time-sheet.tsx(10 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/web/app/[locale]/timesheet/[memberId]/components/FilterWithStatus.tsx
- apps/web/app/[locale]/timesheet/[memberId]/components/FrequencySelect.tsx
- apps/web/lib/features/integrations/calendar/table-time-sheet.tsx
🔇 Additional comments (5)
apps/web/app/hooks/features/useTimesheet.ts (5)
54-59: Verify week calculation around year boundaries.
The week calculation logic might have edge cases around year boundaries. Consider using a well-tested date library like date-fns for more reliable week calculations.
-const getWeekYearKey = (date: Date): string => {
- const startOfYear = new Date(date.getFullYear(), 0, 1);
- const daysSinceStart = Math.floor((date.getTime() - startOfYear.getTime()) / (1000 * 60 * 60 * 24));
- const week = Math.ceil((daysSinceStart + startOfYear.getDay() + 1) / 7);
- return `${date.getFullYear()}-W${week}`;
-};
+import { getWeek, getYear } from 'date-fns';
+
+const getWeekYearKey = (date: Date): string => {
+ return `${getYear(date)}-W${getWeek(date)}`;
+};64-95: LGTM! Well-structured grouping functions.
The refactoring effectively addresses the code duplication issue from the previous review. The higher-order function pattern provides a clean and maintainable solution.
103-103: LGTM! Clean state management integration.
The addition of timesheetGroupByDays from useTimelogFilterOptions is well-integrated with the existing state management pattern.
215-215: LGTM! Proper effect dependencies.
The addition of timesheetGroupByDays to the effect dependencies ensures proper data synchronization when grouping changes.
219-225: LGTM! Clean return object updates.
The return object updates are well-structured and consistent with the new grouping functionality. Good job addressing the previous review comment about using flat() instead of flatMap().
Description
Please include a summary of the changes and the related issue.
#3043
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
Improvements
Bug Fixes