[Feat]: Display Timesheet Data and Refactor Code #3342
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes in this pull request involve significant updates to the handling of timesheet-related components and types across multiple files. Key modifications include the introduction of a new Changes
Possibly related PRs
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
apps/web/lib/features/task/task-card.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: 11
🧹 Outside diff range and nitpick comments (20)
apps/web/lib/components/types.ts (2)
9-38: Consider enhancing type safety and maintainabilityThe current implementation using
Record<string, StatusColorScheme>allows any string as a status key. This could lead to runtime errors if invalid status keys are used.Consider using a union type for status keys:
export type TimesheetStatus = 'PENDING' | 'APPROVED' | 'DENIED' | 'DRAFT' | 'IN_REVIEW' | 'DEFAULT'; const STATUS_COLORS: Record<TimesheetStatus, StatusColorScheme> = { // ... existing color mappings ... };
Line range hint
47-49: Enhance error handling in statusColor functionThe function silently returns an empty object when an invalid status is provided. This could mask errors in the application.
Consider adding validation and logging:
export const statusColor = (status: TimesheetStatus): StatusColorScheme => { const colorScheme = STATUS_COLORS[status]; if (!colorScheme) { console.warn(`Invalid status: ${status}, falling back to DEFAULT`); return STATUS_COLORS.DEFAULT; } return colorScheme; };apps/web/app/services/server/requests/timesheet.ts (3)
Line range hint
75-80: Fix malformed query parameter in API endpointThere's an issue with the endpoint path construction. The
activityLevel?appears to be malformed and could cause API failures.Apply this fix:
- path: `/timesheet/time-log?activityLevel?${queries.toString()}`, + path: `/timesheet/time-log?${queries.toString()}`,
Line range hint
91-97: Enhance robustness of bulk deletion endpointThe current implementation might have issues with edge cases:
- Empty logIds array could result in an invalid endpoint URL
- Large number of IDs could exceed URL length limits
Consider this improved implementation:
export function deleteTaskTimesheetRequest(params: IDeleteTimesheetProps, bearer_token: string) { const { logIds = [] } = params; + if (logIds.length === 0) { + throw new Error('No log IDs provided for deletion'); + } + // Consider batching if array is too large + if (logIds.length > 50) { + console.warn('Large number of logs for deletion, consider batching'); + } return serverFetch<TimesheetLog[]>({ path: `/timesheet/time-log/${logIds.join(',')}`, method: 'DELETE', bearer_token, tenantId: params.tenantId }); }
Line range hint
75-80: Add JSDoc documentation for consistencyWhile
ITimesheetPropshas detailed documentation, other functions likegetTaskTimesheetRequestanddeleteTaskTimesheetRequestlack similar documentation. Consider adding JSDoc comments for consistency.Example addition:
/** * Retrieves timesheet logs for specified time period * @param params - Timesheet request parameters * @param bearer_token - Authentication token * @returns Promise containing array of timesheet logs */ export function getTaskTimesheetRequest(params: ITimesheetProps, bearer_token: string) /** * Deletes multiple timesheet logs by their IDs * @param params - Delete request parameters containing log IDs * @param bearer_token - Authentication token * @returns Promise containing array of remaining timesheet logs */ export function deleteTaskTimesheetRequest(params: IDeleteTimesheetProps, bearer_token: string)Also applies to: 91-97
apps/web/app/services/client/api/timer/timer-log.ts (1)
69-69: Consider adding type validation for API responseThe return type change is consistent with the interface update. However, since this is an API response, consider adding runtime type validation to ensure the response matches the expected
TimesheetLogstructure.Example using a validation library like
zod:import { z } from 'zod'; const TimesheetLogSchema = z.object({ // define schema matching TimesheetLog interface }); return get<TimesheetLog[]>(endpoint, { tenantId }).then(response => TimesheetLogSchema.array().parse(response) );apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetAction.tsx (2)
Line range hint
11-24: Enhance button accessibility and stylingWhile the disabled prop is correctly added, consider these improvements:
- Add disabled state styling
- Include ARIA attributes for better accessibility
- Add proper type safety for the onClick handler
type ITimesheetButton = { title?: string, - onClick?: () => void, + onClick?: (event: React.MouseEvent<HTMLButtonElement>) => void, className?: string, icon?: ReactNode, disabled?: boolean } export const TimesheetButton = ({ className, icon, onClick, title, disabled }: ITimesheetButton) => { return ( <button disabled={disabled} onClick={onClick} - className={clsxm("flex items-center gap-1 text-gray-400 font-normal leading-3", className)}> + className={clsxm( + "flex items-center gap-1 text-gray-400 font-normal leading-3", + "disabled:opacity-50 disabled:cursor-not-allowed", + className + )} + aria-label={title} + role="button"> <div className="w-[16px] h-[16px] text-[#293241]"> {icon} </div> <span>{title}</span> </button> ) }
Line range hint
34-61: Improve type safety and error handling in getTimesheetButtonsSeveral improvements can be made to enhance reliability and maintainability:
- The fallback to
buttonsConfig.Deniedis implicit and could be confusing- Using index as key in the map function is not recommended
- Translation keys should be type-safe
-export const getTimesheetButtons = (status: StatusType, t: TranslationHooks, disabled: boolean, onClick: (action: StatusAction) => void) => { +export const getTimesheetButtons = ( + status: StatusType, + t: TranslationHooks, + disabled: boolean, + onClick: (action: StatusAction) => void +) => { const buttonsConfig: Record<StatusType, { icon: JSX.Element; title: string; action: StatusAction }[]> = { // ... button configurations ... }; - return (buttonsConfig[status] || buttonsConfig.Denied).map((button, index) => ( + const buttons = buttonsConfig[status]; + if (!buttons) { + console.warn(`Unknown status: ${status}, falling back to Denied status buttons`); + return buttonsConfig.Denied; + } + + return buttons.map((button) => ( <TimesheetButton className="hover:underline text-sm gap-2" disabled={disabled} - key={index} + key={`${status}-${button.action}`} icon={button.icon} onClick={() => onClick(button.action)} title={button.title} /> )); };apps/web/app/interfaces/ITask.ts (1)
139-144: Consider enhancing type safety and documentation.To improve maintainability and clarity, consider these refinements:
- Convert to an enum for better type safety and debugging
- Add JSDoc comments to document the meaning and progression of each status
+/** + * Represents the possible states of a timesheet. + * + * Progression: + * 1. DRAFT - Initial state when timesheet is being created + * 2. PENDING - Submitted for review + * 3. IN_REVIEW - Currently being reviewed + * 4. DENIED/APPROVED - Final states after review + */ -export type TimesheetStatus = - | "DRAFT" - | "PENDING" - | "IN REVIEW" - | "DENIED" - | "APPROVED"; +export enum TimesheetStatus { + DRAFT = "DRAFT", + PENDING = "PENDING", + IN_REVIEW = "IN_REVIEW", // Using underscore for consistency + DENIED = "DENIED", + APPROVED = "APPROVED" +}apps/web/lib/features/task/task-issue.tsx (3)
Line range hint
52-63: Remove commented-out codeThe commented-out "New Issue" button should be removed if it's no longer needed. Keeping commented code can lead to confusion and maintenance overhead.
- {/* <Button - onClick={openModal} - className="min-w-[100px] text-xs px-1 py-2 gap-0 w-full" - variant="outline-danger" - > - <PlusIcon className="w-4 h-4" /> - {t('common.NEW_ISSUE')} - </Button> */}
Line range hint
187-189: Implement the form submission logicThe
handleSubmitfunction is currently empty. Please implement the form submission logic to handle the creation of new issues.Would you like me to help implement the form submission logic?
Line range hint
92-121: Consider extracting validation logicThe issue type transition validation logic is complex and could benefit from being extracted into a separate hook or utility function. This would improve maintainability and testability.
Consider creating a custom hook like
useIssueTypeTransitionsto handle this logic:function useIssueTypeTransitions(items: TStatusItem[], task?: ITeamTask) { return useMemo(() => { const validTransitions: Record<IssueType, TStatusItem[]> = { [IssueType.EPIC]: [], [IssueType.STORY]: items.filter((it) => [IssueType.TASK, IssueType.BUG].includes(it.value as IssueType) ), // ... rest of the transitions }; if (!task) { return items; } let availableTransitions = task.issueType ? validTransitions[task.issueType] : items; if (task.parent?.issueType === 'Story') { availableTransitions = availableTransitions.filter(it => it.value !== 'Story'); } return availableTransitions; }, [items, task]); }apps/web/lib/features/task/task-card.tsx (2)
649-681: Fix inconsistent indentation in the TaskCardMenu componentThe indentation in this block is inconsistent with the rest of the codebase, which could affect code readability.
Apply this diff to fix the indentation:
- <> - <Divider type="HORIZONTAL" /> - <div className="mt-3"> - {!taskPlannedToday && ( - <li className="mb-2"> - <PlanTask - planMode="today" - taskId={task.id} - employeeId={profile?.member?.employeeId ?? ''} - taskPlannedToday={taskPlannedToday} - /> - </li> - )} +<> + <Divider type="HORIZONTAL" /> + <div className="mt-3"> + {!taskPlannedToday && ( + <li className="mb-2"> + <PlanTask + planMode="today" + taskId={task.id} + employeeId={profile?.member?.employeeId ?? ''} + taskPlannedToday={taskPlannedToday} + /> + </li> + )}
649-681: Consider extracting planning options into a separate componentThe planning options section in TaskCardMenu is growing in complexity. Consider extracting it into a separate component for better maintainability.
Create a new component like
TaskPlanningOptionsto handle the planning-related UI and logic:interface TaskPlanningOptionsProps { task: ITeamTask; profile?: I_UserProfilePage; taskPlannedToday?: ITeamTask; taskPlannedTomorrow?: ITeamTask; } function TaskPlanningOptions({ task, profile, taskPlannedToday, taskPlannedTomorrow }: TaskPlanningOptionsProps) { return ( <> <Divider type="HORIZONTAL" /> <div className="mt-3"> {!taskPlannedToday && ( <li className="mb-2"> <PlanTask planMode="today" taskId={task.id} employeeId={profile?.member?.employeeId ?? ''} taskPlannedToday={taskPlannedToday} /> </li> )} {/* ... rest of the planning options */} </div> </> ); }apps/web/app/interfaces/timer/ITimerLog.ts (5)
9-12: Consider usingDatetypes for timestamp propertiesThe properties
createdAt,updatedAt,deletedAt, andarchivedAtare currently typed asstring. UsingDatetypes can enhance type safety and allow for better date manipulation.Apply this diff to update the types:
interface BaseEntity { id: string; isActive: boolean; isArchived: boolean; tenantId: string; organizationId: string; - createdAt: string; - updatedAt: string; - deletedAt: string | null; - archivedAt: string | null; + createdAt: Date; + updatedAt: Date; + deletedAt: Date | null; + archivedAt: Date | null; }
56-56: UseDatetype forstartDatepropertyThe
startDateproperty is currently typed asstring | null. Changing it toDate | nullimproves type safety and consistency.Apply this diff to update the type:
interface Task extends ITeamTask { taskStatus: TaskStatus | null; number: number; description: string; - startDate: string | null; + startDate: Date | null; }
65-70: Consider usingDatetypes for timesheet date propertiesThe properties
startedAt,stoppedAt,approvedAt,submittedAt,lockedAt, andeditedAtare typed asstringorstring | null. UsingDatetypes can enhance accuracy and allow for better date operations.Apply this diff to update the types:
interface Timesheet extends BaseEntity { duration: number; keyboard: number; mouse: number; overall: number; - startedAt: string; - stoppedAt: string; - approvedAt: string | null; - submittedAt: string | null; - lockedAt: string | null; - editedAt: string | null; + startedAt: Date; + stoppedAt: Date; + approvedAt: Date | null; + submittedAt: Date | null; + lockedAt: Date | null; + editedAt: Date | null; isBilled: boolean; status: string; employeeId: string; approvedById: string | null; isEdited: boolean; }
72-72: Define a specific type forstatuspropertyThe
statusproperty is currently astring. Defining and using a specific type or enum forstatusenhances type safety and code readability.First, define the
TimesheetStatustype:type TimesheetStatus = 'DRAFT' | 'PENDING' | 'IN_REVIEW' | 'DENIED' | 'APPROVED';Then, update the
statusproperty:interface Timesheet extends BaseEntity { // ... other properties ... - status: string; + status: TimesheetStatus; employeeId: string; approvedById: string | null; isEdited: boolean; }
90-91: ExtractlogTypeandsourceinto enumsCurrently,
logTypeandsourceare union types of string literals. Defining enums for these properties improves maintainability and consistency across the codebase.Define the enums:
enum LogType { TRACKED = 'TRACKED', MANUAL = 'MANUAL', } enum Source { WEB_TIMER = 'WEB_TIMER', MOBILE_APP = 'MOBILE_APP', DESKTOP_APP = 'DESKTOP_APP', }Update the
TimesheetLoginterface:export interface TimesheetLog extends BaseEntity { startedAt: string; stoppedAt: string; editedAt: string | null; - logType: "TRACKED" | "MANUAL"; - source: "WEB_TIMER" | "MOBILE_APP" | "DESKTOP_APP"; + logType: LogType; + source: Source; description: string; reason: string | null; isBillable: boolean; isRunning: boolean; // ... other properties ... }apps/web/lib/features/integrations/calendar/table-time-sheet.tsx (1)
326-326: Simplify null checks using optional chainingYou can simplify the null check by using optional chaining (
?.) instead of&&.Apply this diff to implement the change:
- <span className="flex-1">{task.project && task.project.name}</span> + <span className="flex-1">{task.project?.name}</span>🧰 Tools
🪛 Biome
[error] 326-326: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetAction.tsx(4 hunks)apps/web/app/hooks/features/useTimesheet.ts(4 hunks)apps/web/app/interfaces/ITask.ts(1 hunks)apps/web/app/interfaces/timer/ITimerLog.ts(2 hunks)apps/web/app/services/client/api/timer/timer-log.ts(3 hunks)apps/web/app/services/server/requests/timesheet.ts(3 hunks)apps/web/app/stores/time-logs.ts(2 hunks)apps/web/lib/components/types.ts(1 hunks)apps/web/lib/features/integrations/calendar/table-time-sheet.tsx(4 hunks)apps/web/lib/features/task/task-card.tsx(1 hunks)apps/web/lib/features/task/task-issue.tsx(1 hunks)
🧰 Additional context used
🪛 Biome
apps/web/lib/features/integrations/calendar/table-time-sheet.tsx
[error] 326-326: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (18)
apps/web/app/stores/time-logs.ts (2)
3-3: Verify the TimesheetLog type definition
The import change aligns with the codebase-wide migration from ITimeSheet to TimesheetLog.
Let's verify the TimesheetLog type definition and its usage:
✅ Verification successful
Import change is valid and properly migrated
The verification confirms that:
TimesheetLogis properly defined as an interface inapps/web/app/interfaces/timer/ITimerLog.ts- The type is consistently used across the codebase in related files:
- Store definition in
time-logs.ts - API services in
timer-log.ts - Hooks in
useTimesheet.ts
- Store definition in
- No references to the old
ITimeSheettype were found, indicating complete migration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify TimesheetLog type definition and ensure complete migration
# Check TimesheetLog type definition
ast-grep --pattern 'type TimesheetLog = {
$$$
}'
# Verify no remaining ITimeSheet references
rg 'ITimeSheet' --type ts
Length of output: 78
Script:
#!/bin/bash
# Let's try a different approach to locate TimesheetLog definition and usage
# Search for TimesheetLog in interface/type definitions
ast-grep --pattern 'interface TimesheetLog {
$$$
}'
# Search for TimesheetLog in any context
rg "TimesheetLog" -A 5 --type ts
# Search for files containing both TimesheetLog and ITimeSheet
rg -l "TimesheetLog.*ITimeSheet|ITimeSheet.*TimesheetLog" --type ts
# Look for interface definitions in interfaces directory
fd "interfaces.ts$" --exec cat {}
Length of output: 7779
12-12: Verify state consumers for breaking changes
The state atom type change from ITimeSheet[] to TimesheetLog[] could affect consumers.
Let's verify the impact on consumers:
apps/web/lib/components/types.ts (2)
Line range hint 1-49: Overall implementation aligns well with PR objectives
The changes effectively support the timesheet feature implementation by providing a comprehensive and well-structured status management system. The standardization of status values and addition of new statuses (DRAFT, IN REVIEW) enhance the codebase's maintainability.
9-38: Verify color contrast ratios for accessibility
The color combinations should be verified to ensure they meet WCAG accessibility guidelines for contrast ratios.
apps/web/app/services/server/requests/timesheet.ts (1)
4-4: Verify type compatibility across the codebase
The change from ITimeSheet to TimesheetLog type is part of a larger refactoring effort. Let's verify the type compatibility across the codebase.
✅ Verification successful
Let me try a different approach to verify the type changes.
Let me check the actual interface definition and its usages with corrected file type flags.
Type change from ITimeSheet to TimesheetLog is valid and properly implemented
The verification shows that:
- The
TimesheetLoginterface is well-defined inITimerLog.tswith all necessary properties - No remaining references to
ITimeSheetexist in the codebase - The
TimesheetLogtype is consistently used across the codebase in relevant files:apps/web/app/stores/time-logs.tsapps/web/app/services/server/requests/timesheet.tsapps/web/app/services/client/api/timer/timer-log.tsapps/web/app/hooks/features/useTimesheet.ts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining ITimeSheet references and verify TimesheetLog structure
# Check for any remaining ITimeSheet references
echo "Checking for remaining ITimeSheet references..."
rg "ITimeSheet" -t ts -t tsx
# Check TimesheetLog structure and usage
echo "Checking TimesheetLog structure..."
ast-grep --pattern 'interface TimesheetLog {
$$$
}'
Length of output: 296
Script:
#!/bin/bash
# Search for TimesheetLog and ITimeSheet references with proper context
echo "Searching for TimesheetLog references..."
rg "TimesheetLog" -t typescript -A 3
echo -e "\nSearching for ITimeSheet references..."
rg "ITimeSheet" -t typescript -A 3
# Check the interface definition location
echo -e "\nFinding TimesheetLog interface definition..."
fd "ITimerLog.ts"
Length of output: 491
Script:
#!/bin/bash
# Check the TimesheetLog interface definition
echo "Content of ITimerLog.ts:"
cat apps/web/app/interfaces/timer/ITimerLog.ts
# Search for references with correct file type
echo -e "\nSearching for TimesheetLog references..."
rg "TimesheetLog" -t ts
echo -e "\nSearching for ITimeSheet references..."
rg "ITimeSheet" -t ts
Length of output: 4814
apps/web/app/services/client/api/timer/timer-log.ts (2)
56-58: LGTM! Additional relations align with feature requirements
The addition of task.taskStatus and timesheet relations will provide the necessary data for displaying timesheet information.
1-1: Verify consistent usage of TimesheetLog type
The change from ITimeSheet to TimesheetLog appears to be part of a broader refactoring effort.
✅ Verification successful
Let me check the actual content of these files to verify the type usage consistency.
Type change from ITimeSheet to TimesheetLog is correctly implemented
The verification shows that:
- No references to
ITimeSheettype exist in the codebase TimesheetLogis consistently used across the codebase:- Defined in
ITimerLog.ts - Used in imports and type annotations in all relevant files
- Properly integrated with API responses and state management
- Defined in
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that ITimeSheet is not being used elsewhere in the codebase
# and that TimesheetLog is consistently used
# Check for any remaining ITimeSheet references
echo "Checking for remaining ITimeSheet references:"
rg "ITimeSheet" -l
# Check TimesheetLog usage pattern
echo -e "\nChecking TimesheetLog usage:"
rg "TimesheetLog" -l
Length of output: 436
Script:
#!/bin/bash
# Check the content of files using TimesheetLog to verify consistent usage
echo "=== Content of ITimerLog.ts ==="
cat apps/web/app/interfaces/timer/ITimerLog.ts
echo -e "\n=== Content of time-logs.ts ==="
cat apps/web/app/stores/time-logs.ts
echo -e "\n=== Content of timesheet.ts ==="
cat apps/web/app/services/server/requests/timesheet.ts
echo -e "\n=== Content of useTimesheet.ts ==="
cat apps/web/app/hooks/features/useTimesheet.ts
# Check if there are any TypeScript type definitions or interfaces using ITimeSheet
ast-grep --pattern 'interface ITimeSheet {
$$$
}'
ast-grep --pattern 'type ITimeSheet = {
$$$
}'
Length of output: 11448
apps/web/app/interfaces/ITask.ts (1)
139-144: LGTM! The TimesheetStatus type is well-structured.
The type definition follows TypeScript best practices with clear, uppercase string literals representing distinct timesheet states.
apps/web/app/hooks/features/useTimesheet.ts (2)
8-8: LGTM: Type system improvements
The replacement of ITimeSheet with TimesheetLog and the addition of TimesheetStatus improve type safety and clarity.
Also applies to: 18-18
143-144: LGTM: Clean interface extension
The addition of getStatusTimesheet to the return object is clean and maintains the hook's interface consistency.
apps/web/lib/features/task/task-issue.tsx (2)
1-1: Verify the migration of timesheet status functionality
The removal of TaskIssueStatusTimesheet suggests that timesheet status rendering has been moved elsewhere. Let's ensure this functionality is properly handled in the new location.
#!/bin/bash
# Description: Verify the migration of timesheet status functionality
# Expected results: Find where the timesheet status rendering was moved to
# Search for new timesheet status related components
rg -t tsx "Timesheet.*Status"
# Search for files that might handle timesheet status rendering
rg -t tsx "timesheet.*status" -l2-2: Verify the removal of ITimeSheet import across the codebase
The removal of ITimeSheet import aligns with the broader changes in timesheet data models mentioned in the summary.
✅ Verification successful
Removal of ITimeSheet import is consistent with codebase changes
Based on the verification results:
- The
ITimeSheetinterface has been replaced with a newITaskTimesheetinterface (found inapps/web/app/interfaces/ITaskTimesheet.ts) - The codebase now uses
TimesheetStatustype for timesheet-related status handling - No remaining references to the old
ITimeSheettype were found - The removal in
task-issue.tsxis part of a broader refactoring where timesheet functionality has been reorganized
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the migration from ITimeSheet to TimesheetLog
# Expected results: No remaining references to ITimeSheet type
# Check for any remaining ITimeSheet references
rg "ITimeSheet" --type ts --type tsx
# Check for new TimesheetLog usage
rg "TimesheetLog" --type ts --type tsx
Length of output: 122
Script:
#!/bin/bash
# Let's try with correct file type flags and broader search patterns
# Check for ITimeSheet type references
rg "ITimeSheet" -t typescript -t typescriptreact
# Check for Timesheet related types and models
rg "interface.*Timesheet" -t typescript -t typescriptreact
# Check for TimesheetLog references
rg "TimesheetLog" -t typescript -t typescriptreact
# Look for timesheet-related files
fd -e ts -e tsx timesheet
Length of output: 973
Script:
#!/bin/bash
# Let's examine the timesheet interfaces and their usage
# Check interfaces file content
cat apps/web/app/interfaces/ITaskTimesheet.ts
# Check for any ITimeSheet references in task-issue.tsx
cat apps/web/lib/features/task/task-issue.tsx
# Look for timesheet type usage in the codebase using ripgrep with proper flags
rg --type-add 'ts:*.{ts,tsx}' --type ts 'ITimeSheet'
# Check for any timesheet status related code
rg --type-add 'ts:*.{ts,tsx}' --type ts 'TimesheetStatus|TaskIssueStatusTimesheet'
Length of output: 8957
apps/web/lib/features/task/task-card.tsx (1)
649-681: Verify the impact of custom plan mode implementation
The addition of the custom plan mode needs to be verified to ensure it works correctly with the existing planning functionality.
Let's verify the implementation:
✅ Verification successful
Let me gather more information about the custom plan mode implementation, particularly focusing on the calendar component and daily plan creation logic:
Let me check one final aspect regarding the daily plan creation and validation:
Based on the comprehensive analysis of the codebase, I can now provide the final response:
Custom plan mode implementation is properly integrated with existing functionality
The custom plan mode is correctly implemented and works harmoniously with the existing planning functionality:
- It's properly typed in the
IDailyPlanModetype definition as 'custom' alongside 'today' and 'tomorow' - The implementation includes proper validation against existing plans through
existingPlanDates - The custom calendar component is properly integrated for date selection
- The plan creation logic handles all modes consistently, including the custom mode
- The UI conditionally renders appropriate elements based on the plan mode
- It maintains consistency with the existing today/tomorrow planning features while adding the flexibility of custom date selection
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any existing custom plan mode implementations and their usage
# Search for custom plan mode implementations
echo "Searching for custom plan mode implementations..."
rg -A 5 "planMode.*custom"
# Search for IDailyPlanMode type definition
echo "Checking IDailyPlanMode type definition..."
rg "type.*IDailyPlanMode.*=.*"
Length of output: 3267
Script:
#!/bin/bash
# Check CustomCalendar component and daily plan creation implementation
echo "Checking CustomCalendar component..."
rg -A 10 "CustomCalendar"
echo "\nChecking daily plan creation logic..."
ast-grep --pattern 'function createDailyPlan($$$) {
$$$
}'
echo "\nChecking existing plan validation..."
rg -A 5 "existingPlanDates"
Length of output: 6475
Script:
#!/bin/bash
# Check daily plan creation and validation
echo "Checking daily plan creation and validation..."
rg -A 10 "createDailyPlan.*="
echo "\nChecking task planning validation..."
rg -A 5 "taskPlannedToday|taskPlannedTomorrow"
echo "\nChecking plan mode handling..."
rg -A 5 "planMode.*=.*"
Length of output: 30604
apps/web/app/interfaces/timer/ITimerLog.ts (3)
96-96: Use consistent types for versioning
The version property is typed as string | null. Confirm if versioning is consistently handled across the application and consider defining a specific type or interface if necessary.
Please ensure that version aligns with versioning standards used elsewhere in the project.
53-53:
Fix syntax error: Replace comma with semicolon
In TypeScript interfaces, properties should end with a semicolon ; instead of a comma ,.
Apply this diff to correct the syntax:
interface Task extends ITeamTask {
taskStatus: TaskStatus | null,
- taskStatus: TaskStatus | null,
+ taskStatus: TaskStatus | null;
number: number;
description: string;
startDate: string | null;
}Likely invalid or redundant comment.
107-107:
Fix syntax error: Replace comma with semicolon
The property timesheet ends with a comma. Replace it with a semicolon to correct the syntax.
Apply this diff to correct the syntax:
export interface TimesheetLog extends BaseEntity {
// ... other properties ...
employee: Employee;
- timesheet: Timesheet,
+ timesheet: Timesheet;
duration: number;
isEdited: boolean;
}Likely invalid or redundant comment.
apps/web/lib/features/integrations/calendar/table-time-sheet.tsx (2)
58-60: Imports are appropriate
The new imports for TaskNameInfoDisplay, TimesheetStatus, and dayjs are necessary for the added functionality.
178-178: Ensure imported functions are used correctly
The destructured functions deleteTaskTimesheet, loadingDeleteTimesheet, and getStatusTimesheet from useTimesheet appear to be properly integrated.
apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetAction.tsx
Outdated
Show resolved
Hide resolved
apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetAction.tsx
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: 2
🧹 Outside diff range and nitpick comments (10)
apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetAction.tsx (2)
15-19: Enhance button accessibilityWhile the disabled functionality is correctly implemented, consider adding ARIA attributes for better accessibility.
<button disabled={disabled} onClick={onClick} - className={clsxm("flex items-center gap-1 text-gray-400 font-normal leading-3", className)}> + className={clsxm("flex items-center gap-1 text-gray-400 font-normal leading-3", + disabled ? "cursor-not-allowed opacity-50" : "", + className)} + aria-disabled={disabled}>
67-67: Maintain consistent terminologyThe status label uses "Denied" but the description still uses "rejected". Consider updating the description to maintain consistent terminology.
- { label: "Denied", description: "The item has been rejected" }, + { label: "Denied", description: "The item has been denied" },apps/web/app/hooks/features/useTimesheet.ts (3)
117-126: Simplify the type guard implementationThe type guard can be simplified by directly using the TimesheetStatus enum.
- function isTimesheetStatus(status: unknown): status is TimesheetStatus { - const timesheetStatusValues: TimesheetStatus[] = [ - "DRAFT", - "PENDING", - "IN REVIEW", - "DENIED", - "APPROVED" - ]; - return Object.values(timesheetStatusValues).includes(status as TimesheetStatus); + function isTimesheetStatus(status: unknown): status is TimesheetStatus { + return Object.values(TimesheetStatus).includes(status as TimesheetStatus); }
170-171: LGTM: Return statement updated correctlyThe addition of
getStatusTimesheetto the return object is appropriate. Consider grouping related properties together (e.g., all timesheet-related functions) for better readability.return { loadingTimesheet, timesheet: groupByDate(timesheet), getTaskTimesheet, + getStatusTimesheet, loadingDeleteTimesheet, - deleteTaskTimesheet, - getStatusTimesheet + deleteTaskTimesheet };
Line range hint
141-157: Enhance error handling in deleteTaskTimesheetThe error handling in
deleteTaskTimesheetcould be improved to provide more specific error messages and better error recovery.const deleteTaskTimesheet = useCallback(async () => { if (!user) { - throw new Error('User not authenticated'); + throw new Error('Cannot delete timesheet: User not authenticated'); } if (!logIds.length) { - throw new Error('No timesheet IDs provided for deletion'); + throw new Error('Cannot delete timesheet: No entries selected'); } try { await handleDeleteTimesheet({ organizationId: user.employee.organizationId, tenantId: user.tenantId ?? "", logIds }); + // Refresh the timesheet data after successful deletion + await getTaskTimesheet({ startDate, endDate }); } catch (error) { - console.error('Failed to delete timesheets:', error); - throw error; + const errorMessage = error instanceof Error ? error.message : 'Unknown error occurred'; + console.error('Failed to delete timesheets:', errorMessage); + throw new Error(`Failed to delete timesheet: ${errorMessage}`); } }, - [user, queryDeleteTimesheet, logIds, handleDeleteTimesheet] + [user, queryDeleteTimesheet, logIds, handleDeleteTimesheet, getTaskTimesheet, startDate, endDate] );apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetFilterDate.tsx (4)
Line range hint
42-89: Consider improving date validation and preset handlingThe date handling logic could be more robust and maintainable:
- Date validation is spread across multiple handlers
- Preset handling uses direct string comparison with translated keys
- The preset logic could be simplified using a lookup object
Consider refactoring to:
+ const DATE_PRESETS = { + TODAY: 'common.FILTER_TODAY', + LAST_7_DAYS: 'common.FILTER_LAST_7_DAYS', + LAST_30_DAYS: 'common.FILTER_LAST_30_DAYS', + THIS_YEAR: 'common.FILTER_THIS_YEAR', + CUSTOM_RANGE: 'common.FILTER_CUSTOM_RANGE' + } as const; + + const getPresetRange = (preset: keyof typeof DATE_PRESETS, today = new Date()) => { + switch (preset) { + case 'TODAY': + return { from: today, to: today }; + case 'LAST_7_DAYS': + return { + from: new Date(today.getFullYear(), today.getMonth(), today.getDate() - 7), + to: today + }; + // ... other cases + } + }; const handlePresetClick = (preset: string) => { - const today = new Date(); - switch (preset) { - case t('common.FILTER_TODAY'): - setDateRange({ from: today, to: today }); - break; - // ... other cases - } + const presetKey = Object.entries(DATE_PRESETS) + .find(([_, value]) => t(value) === preset)?.[0] as keyof typeof DATE_PRESETS; + if (presetKey) { + setDateRange(getPresetRange(presetKey)); + } };
Line range hint
284-341: Consider modernizing date handling and improving performanceThe FilterCalendar component could benefit from several improvements:
- Replace moment.js with Date-fns for consistency and better performance
- Simplify date key creation
- Optimize date comparisons
Consider these improvements:
- import moment from 'moment'; + import { format, parseISO, startOfDay } from 'date-fns'; const FilterCalendar = memo(function FuturePlansCalendar<T extends { date: string | Date }>({ // ... }) { const sortedPlansByDateDesc = useMemo( () => [...plans].sort((plan1, plan2) => - new Date(plan1.date).getTime() < new Date(plan2.date).getTime() ? 1 : -1 + new Date(plan2.date).getTime() - new Date(plan1.date).getTime() ), [plans] ); - const createDateKey = (date: string | Date) => - moment(date.toString().split('T')[0]).toISOString().split('T')[0]; + const createDateKey = (date: string | Date) => + format(startOfDay(new Date(date)), 'yyyy-MM-dd'); const isDateAvailableForPlanning = useCallback( (dateToCheck: Date) => { const dateKey = createDateKey(dateToCheck); const planDates = new Set(plans.map(plan => createDateKey(plan.date))); return !planDates.has(dateKey); }, [plans] ); return ( <DatePicker // ... modifiers={{ booked: sortedPlansByDateDesc.map(plan => - moment.utc(plan.date.toString().split('T')[0]).toDate() + startOfDay(new Date(plan.date)) ), // ... }} // ... /> ); });
Line range hint
51-63: Enhance date input validationWhile the current implementation is generally safe, consider adding stronger input validation for date ranges to prevent potential edge cases and improve user experience.
Consider adding these validations:
+ const isValidDateRange = (from: Date | null, to: Date | null) => { + if (!from || !to) return false; + if (from > new Date() || to > new Date()) return false; + if (from > to) return false; + return true; + }; const handleFromChange = (fromDate: Date | null) => { + if (fromDate && !isNaN(fromDate.getTime())) { if (maxDate && fromDate > maxDate) { return; } setDateRange((prev) => ({ ...prev, from: fromDate })); onChange?.({ ...dateRange, from: fromDate }); + } };
Line range hint
92-122: Enhance accessibility supportThe component could benefit from improved accessibility features:
- More descriptive ARIA labels
- Better keyboard navigation support
- Screen reader announcements for date changes
Consider these improvements:
<Button variant={"outline"} role="combobox" - aria-label="Select date range" + aria-label={`Select date range from ${dateRange.from ? format(dateRange.from, "MMMM d, yyyy") : 'start date'} to ${dateRange.to ? format(dateRange.to, "MMMM d, yyyy") : 'end date'}`} aria-expanded="false" + aria-haspopup="dialog" + onKeyDown={(e) => { + if (e.key === 'Enter' || e.key === ' ') { + e.preventDefault(); + // trigger popover + } + }} className={cn( // ... )}>apps/web/lib/features/integrations/calendar/table-time-sheet.tsx (1)
272-346: Consider performance optimization for large datasetsThe Accordion component renders all tasks at once, which could impact performance with large datasets. Consider implementing virtualization for the task list.
🧰 Tools
🪛 Biome
[error] 326-326: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetAction.tsx(3 hunks)apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetFilterDate.tsx(1 hunks)apps/web/app/hooks/features/useTimesheet.ts(4 hunks)apps/web/app/interfaces/ITask.ts(1 hunks)apps/web/lib/features/integrations/calendar/table-time-sheet.tsx(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/app/interfaces/ITask.ts
🧰 Additional context used
🪛 Biome
apps/web/lib/features/integrations/calendar/table-time-sheet.tsx
[error] 326-326: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (8)
apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetAction.tsx (2)
11-12: LGTM: Clean type definition addition
The new optional disabled property is well-typed and follows TypeScript best practices.
52-52: Review fallback status handling
The fallback to Denied status (buttonsConfig[status] || buttonsConfig.Denied) might not be the most appropriate default. Consider if Pending would be a more logical fallback state, or if we should handle invalid states explicitly.
apps/web/app/hooks/features/useTimesheet.ts (2)
8-8: LGTM: Type system updates are consistent
The replacement of ITimeSheet with TimesheetLog and the addition of TimesheetStatus are well-structured and maintain type safety.
Also applies to: 18-18
29-46: LGTM: Error handling improvements implemented
The suggested error handling improvements from the previous review have been implemented correctly.
apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetFilterDate.tsx (1)
166-166: LGTM: Visual separator improves UI clarity
The conditional divider provides clear visual separation between the custom date picker and preset options when visible.
apps/web/lib/features/integrations/calendar/table-time-sheet.tsx (3)
269-269: Replace hardcoded time values with dynamic calculations
The time values '64:30h' and '24:30h' are hardcoded. This should be calculated from the actual timesheet data.
Also applies to: 298-298
558-573: Ensure consistent status handling in getBadgeColor
The function handles 'DENIED' but not 'REJECTED', while the UI shows 'REJECTED'. This inconsistency could lead to incorrect badge colors.
328-328:
Add null check for user image URL
Using the non-null assertion operator (!) on imageUrl could cause runtime errors if the URL is undefined.
Apply this diff to safely handle undefined URLs:
- <img className="h-8 w-8 rounded-full" src={task.employee.user.imageUrl!} alt="" />
+ <img
+ className="h-8 w-8 rounded-full"
+ src={task.employee.user.imageUrl ?? '/default-avatar.png'}
+ alt={`${task.employee.fullName}'s avatar`}
+ />Likely invalid or redundant comment.
apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetAction.tsx
Show resolved
Hide resolved
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
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
TimesheetStatustype for better categorization of timesheet states.getStatusTimesheetfunction for filtering timesheet logs by status.Improvements
TimesheetButtoncomponent with a newdisabledproperty for improved interactivity.UI Enhancements
DataTableTimeSheetcomponent using anAccordionfor better organization.TaskCardcomponent for enhanced user experience.TimesheetFilterDatecomponent for a more responsive UI.Code Cleanup
TaskIssueStatusTimesheetfunction to streamline the codebase.