-
Notifications
You must be signed in to change notification settings - Fork 51
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Feat]: Implement Timesheet logs API integration with State management and query handling #3254
Conversation
…nd query handling
Caution Review failedThe pull request is closed. WalkthroughThe changes primarily enhance the timesheet functionality across several components. The Changes
Possibly related PRs
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 10
🧹 Outside diff range and nitpick comments (18)
apps/web/app/stores/time-logs.ts (1)
Line range hint
7-8
: Consider renaming the state atom for consistency.The new state atom uses "rapport" (French) while the existing atom uses "report" (English). Consider renaming for consistency with the existing naming convention:
-export const timesheetRapportState = atom<ITimeSheet[]>([]) +export const timesheetReportState = atom<ITimeSheet[]>([])apps/web/app/api/timer/timesheet/daily/route.ts (2)
Line range hint
8-15
: Enhance date parameter validationWhile the presence check is good, consider adding more robust validation:
- Validate date formats
- Ensure endDate is not before startDate
- Consider limiting the date range to prevent excessive data queries
const startDate = searchParams.get('startDate'); const endDate = searchParams.get('endDate'); -if (!startDate || !endDate) { +if (!startDate || !endDate || !isValidDateFormat(startDate) || !isValidDateFormat(endDate)) { return NextResponse.json( - { error: 'startDate and endDate are required' }, + { error: 'Valid startDate and endDate are required in YYYY-MM-DD format' }, { status: 400 } ); } +if (new Date(endDate) < new Date(startDate)) { + return NextResponse.json( + { error: 'endDate must not be before startDate' }, + { status: 400 } + ); +}
Line range hint
28-42
: Consider enhancing error response structureWhile the error handling is comprehensive, consider standardizing the error response format to include error codes for better client-side handling.
- return NextResponse.json( - { error: 'No data found' }, - { status: 404 } - ); + return NextResponse.json({ + error: { + code: 'TIMESHEET_NOT_FOUND', + message: 'No timesheet data found for the specified date range', + status: 404 + } + }, { status: 404 });apps/web/app/services/client/api/timer/timer-log.ts (2)
34-46
: Extract relations array as a constantThe relations array should be extracted as a constant for better maintainability and reusability.
Consider refactoring:
+const TIMESHEET_RELATIONS = [ + 'project', + 'task', + 'organizationContact', + 'employee.user' +] as const; + const params = new URLSearchParams({ 'activityLevel[start]': '0', 'activityLevel[end]': '100', organizationId, tenantId, startDate: start, endDate: end, timeZone: timeZone || '', - 'relations[0]': 'project', - 'relations[1]': 'task', - 'relations[2]': 'organizationContact', - 'relations[3]': 'employee.user' + ...TIMESHEET_RELATIONS.reduce((acc, relation, index) => ({ + ...acc, + [`relations[${index}]`]: relation + }), {}) });
48-50
: Consider implementing request cachingGiven that this API fetches timesheet data which might be frequently accessed, consider implementing request caching to improve performance.
You could use React Query or a similar caching solution to cache the responses based on the query parameters.
apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetFilter.tsx (3)
9-10
: Add JSDoc documentation for the interface propertiesConsider adding JSDoc comments to document the purpose and expected values of each interface property, especially the new
initDate
prop.interface ITimesheetFilter { + /** Controls the visibility of the modal */ isOpen: boolean, + /** Callback to open the modal */ openModal: () => void, + /** Callback to close the modal */ closeModal: () => void + /** Optional initial date configuration for the timesheet filter */ initDate?: TimesheetFilterDateProps }
11-11
: Remove unnecessary trailing comma in function parametersThere's an unnecessary trailing comma after the parameter list.
-export function TimesheetFilter({ closeModal, isOpen, openModal, initDate }: ITimesheetFilter,) { +export function TimesheetFilter({ closeModal, isOpen, openModal, initDate }: ITimesheetFilter) {
33-33
: Consider safer prop handling for TimesheetFilterDateSpreading an optional prop directly could lead to undefined being spread when
initDate
is not provided. Consider adding default values or conditional spreading.-<TimesheetFilterDate {...initDate} /> +<TimesheetFilterDate {...(initDate || {})} />apps/web/app/services/server/requests/timesheet.ts (1)
65-72
: Add error handling and response type validationThe function lacks proper error handling and type validation for the API response.
Consider enhancing the implementation:
-export function getTaskTimesheetRequest(params: ITimesheetProps, bearer_token: string) { +export async function getTaskTimesheetRequest(params: ITimesheetProps, bearer_token: string): Promise<ITimeSheet[]> { const queries = qs.stringify(params); - return serverFetch<ITimeSheet[]>({ - path: `/timesheet/time-log/activityLevel?${queries}`, - method: 'GET', - bearer_token, - tenantId: params.tenantId - }) + try { + const response = await serverFetch<ITimeSheet[]>({ + path: `/timesheet/time-log/activityLevel?${queries}`, + method: 'GET', + bearer_token, + tenantId: params.tenantId + }); + + if (!Array.isArray(response)) { + throw new Error('Invalid response format'); + } + + return response; + } catch (error) { + console.error('Failed to fetch timesheet data:', error); + throw error; + } }apps/web/app/hooks/features/useTimesheet.ts (3)
9-12
: Consider strengthening type safety for date parametersThe interface allows both
Date
andstring
types which could lead to inconsistent date handling. Consider enforcing a single type and validating date strings if needed.interface TimesheetParams { - startDate: Date | string; - endDate: Date | string; + startDate: Date; + endDate: Date; }
1-8
: Consider replacing Moment.js with a more modern alternativeMoment.js is now considered legacy. Consider using more modern alternatives like
date-fns
or the nativeIntl
API for date formatting.
42-44
: Consider debouncing date change effectsThe effect runs immediately on every date change, which could lead to excessive API calls if dates are updated frequently (e.g., through a date picker).
+ const debouncedGetTimesheet = useCallback( + debounce(({ startDate, endDate }: TimesheetParams) => { + getTaskTimesheet({ startDate, endDate }); + }, 300), + [getTaskTimesheet] + ); + useEffect(() => { - getTaskTimesheet({ startDate, endDate }); + debouncedGetTimesheet({ startDate, endDate }); + return () => { + debouncedGetTimesheet.cancel(); + }; }, [getTaskTimesheet, startDate, endDate]);apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetFilterDate.tsx (2)
11-11
: LGTM! Consider making onChange required.The interface changes and exports look good. The new props enhance the component's flexibility. However, since
onChange
is crucial for the component's functionality, consider making it required by removing the optional modifier.export interface TimesheetFilterDateProps { - onChange?: (range: { from: Date | null; to: Date | null }) => void; + onChange: (range: { from: Date | null; to: Date | null }) => void; initialRange?: { from: Date | null; to: Date | null }; minDate?: Date; maxDate?: Date; }Also applies to: 19-24
Line range hint
1-214
: Consider architectural improvements for better maintainability.The component handles multiple responsibilities and could benefit from the following improvements:
- Extract preset date ranges to a constant:
const DATE_PRESETS = [ "Today", "Last 7 days", "Last 30 days", `This year (${new Date().getFullYear()})`, "Custom Date Range" ] as const;
- Consider extracting date validation logic into a custom hook:
function useDateValidation(minDate?: Date, maxDate?: Date) { return React.useCallback((date: Date) => { if (minDate && date < minDate) return true; if (maxDate && date > maxDate) return true; return false; }, [minDate, maxDate]); }
- Consider using a reducer for complex state management instead of multiple useState calls.
Would you like me to provide a complete implementation of these suggestions?
apps/web/app/[locale]/timesheet/[memberId]/page.tsx (4)
5-6
: Remove redundant import.The
TranslationHooks
import appears to be redundant sinceuseTranslations
is already imported from 'next-intl'. Consider removing this import and using the type from the existing import if needed.
55-55
: Remove debug console.log statement.Debug console.log statements should not be committed to production code.
-console.log("=SPACE===============>", timesheet)
151-156
: Consider debouncing date range changes.The date range onChange handler might trigger too many API calls when users are selecting dates rapidly. Consider implementing debouncing.
+const debouncedSetDateRange = useMemo( + () => debounce((range: { from: Date | null; to: Date | null }) => { + setDateRange(range); + }, 300), + [] +); initDate={{ initialRange: dateRange, - onChange(range) { - setDateRange(range) - }, + onChange: debouncedSetDateRange, }}
Line range hint
73-74
: Improve layout responsiveness.The use of hardcoded height calculations (
h-[calc(100vh-_300px)]
andh-[calc(100vh-_291px)]
) could break responsiveness on different screen sizes or when the viewport height changes.Consider:
- Using CSS Grid or Flexbox for dynamic height calculations
- Implementing a more robust responsive layout system
- Adding media queries for different viewport sizes
Also applies to: 162-163
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetFilter.tsx
(2 hunks)apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetFilterDate.tsx
(2 hunks)apps/web/app/[locale]/timesheet/[memberId]/page.tsx
(3 hunks)apps/web/app/api/timer/timesheet/daily/route.ts
(1 hunks)apps/web/app/hooks/features/useTimesheet.ts
(1 hunks)apps/web/app/services/client/api/timer/timer-log.ts
(2 hunks)apps/web/app/services/server/requests/timesheet.ts
(1 hunks)apps/web/app/stores/time-logs.ts
(1 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-10-30T16:31:01.294Z
Learning: In the `TimesheetFilter` component, the `Add Time` button does not need to use the `AddManualTimeModal` component, as per the user's decision.
🔇 Additional comments (10)
apps/web/app/stores/time-logs.ts (1)
3-3
: LGTM!
The import statement is properly structured and consistent with the existing import style.
apps/web/app/api/timer/timesheet/daily/route.ts (2)
17-18
: LGTM! Authentication setup is secure
The authentication guard properly extracts all necessary properties and handles unauthorized access appropriately.
21-25
: Verify the removal of employeeIds parameter
The employeeIds parameter has been removed from the request. Please confirm if this is intentional and won't affect any existing functionality that might depend on employee filtering.
✅ Verification successful
Based on the output showing the API endpoints and implementations, I can now generate the final response:
Removal of employeeIds parameter appears to be correct
The timesheet endpoints shown in the codebase (/timesheet/time-log
) don't show any dependency on employeeIds parameter. The filtering appears to be handled through query parameters and relations at the API level, with employee data being accessed through relations[3]
: 'employee.user' when needed.
- The endpoint
/timesheet/time-log
is consistently used across different implementations without employeeIds - Employee filtering appears to be handled through the relations parameter instead
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any references to employeeIds in timesheet-related components
rg -l 'employeeIds.*timesheet' --type ts --type tsx
# Check the implementation of getTaskTimesheetRequest
ast-grep --pattern 'function getTaskTimesheetRequest' -A 10
Length of output: 143
Script:
#!/bin/bash
# Check for any references to employeeIds in timesheet-related files
rg -l 'employeeIds.*timesheet' --type typescript
# Look for the implementation and usage of timesheet request
rg -A 5 'getTaskTimesheetRequest|employeeIds.*timesheet' --type typescript
# Find all timesheet-related files to understand the context
fd timesheet --type f --extension ts
Length of output: 465
Script:
#!/bin/bash
# Check the content of timesheet-related files
cat apps/web/app/interfaces/ITaskTimesheet.ts
cat apps/web/app/services/server/requests/timesheet.ts
cat apps/mobile/app/services/client/requests/timesheet.ts
# Search for employeeIds in these specific files
rg 'employeeIds' apps/web/app/interfaces/ITaskTimesheet.ts apps/web/app/services/server/requests/timesheet.ts apps/mobile/app/services/client/requests/timesheet.ts
Length of output: 3838
apps/web/app/services/client/api/timer/timer-log.ts (2)
1-1
: LGTM: Import statement updated correctly
The addition of ITimeSheet
import is appropriate for the new function's return type.
18-50
: ```shell
#!/bin/bash
Let's try a broader search for timesheet API usage patterns
rg "timesheet.*time-log" --type ts -A 5 -B 5
Search for any timesheet-related tests or documentation
fd "timesheet" --type f --extension ts --extension md
Look for any performance configurations or settings related to API
rg "performance|timeout" --type ts apps/web/app/services/client/api
The previous search might have been too specific. Let's broaden our search to understand the timesheet API usage patterns, related tests, and any performance configurations that might exist in the API service layer.
</details>
<details>
<summary>apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetFilter.tsx (2)</summary>
`2-2`: **LGTM: Import statement is correctly updated**
The TimesheetFilterDateProps type import is appropriately added to the existing import statement.
---
Line range hint `11-43`: **Verify state management integration**
The component changes suggest it's part of a larger state management implementation. Let's verify the integration points to ensure proper setup.
<details>
<summary>✅ Verification successful</summary>
**No state management concerns found in TimesheetFilter component**
The component is properly integrated with state management:
- It receives state management props (`closeModal`, `isOpen`, `openModal`, `initDate`) from its parent page component
- The parent page component (`[memberId]/page.tsx`) correctly manages modal state using the `useModal` hook
- Date range state is properly handled through the `initDate` prop with `onChange` callback
- The component acts as a presentational component, delegating state management to its parent
</details>
<details>
<summary>🏁 Scripts executed</summary>
The following scripts were executed for the analysis:
Script:
```shell
#!/bin/bash
# Description: Verify state management integration for TimesheetFilter
# Check for state management setup in parent components
echo "Checking for state management imports and usage..."
rg -l "(useTimesheet|useState|useReducer|createContext)" "apps/web/app/\[locale\]/timesheet/"
# Check for date range handling
echo "Checking date range handling implementation..."
rg "onChange.*dateRange" "apps/web/app/\[locale\]/timesheet/"
Length of output: 523
Script:
#!/bin/bash
# Let's try with correct path escaping and broader search
echo "Checking for state management imports and usage..."
rg -l "(useTimesheet|useState|useReducer|createContext)" "apps/web/app/[[]locale]/timesheet"
echo "\nChecking component's props and state handling..."
ast-grep --pattern 'export function TimesheetFilter($$$) {
$$$
}'
echo "\nChecking for related components and their state management..."
fd -e tsx -e ts . apps/web/app --exec rg -l "(useState|useReducer|createContext|store)" {}
Length of output: 9143
Script:
#!/bin/bash
# Let's check the specific hooks and components related to timesheet functionality
echo "Checking TimesheetFilter's related hooks..."
rg -A 5 "useTimesheet|useTimer|useTimeLogs" "apps/web/app/hooks/features/use(Timesheet|Timer|TimeLogs).ts"
echo "\nChecking parent component's state management..."
rg -A 5 "TimesheetFilter" "apps/web/app/[locale]/timesheet/[memberId]/page.tsx"
echo "\nChecking modal state management..."
ast-grep --pattern 'const $_ = useModal($$$)'
Length of output: 12321
apps/web/app/hooks/features/useTimesheet.ts (2)
14-50
: Implementation looks good with suggested improvements
The hook implementation is well-structured and follows React best practices. The core functionality for timesheet management is properly implemented with good separation of concerns.
Consider implementing the suggested improvements for better robustness and user experience:
- Stronger type safety for date parameters
- More robust timezone handling
- Better error handling with user feedback
- Debounced date change handling
32-32
: Validate tenantId before API call
The code uses optional chaining with a fallback empty string for tenantId. This could lead to invalid API calls if tenantId is required.
apps/web/app/[locale]/timesheet/[memberId]/page.tsx (1)
39-48
: Add unit tests for date range handling.
To ensure reliability of the timesheet functionality, consider adding unit tests for:
- Date range state management
- API integration with useTimesheet hook
- Edge cases in date handling
apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetFilterDate.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
apps/web/app/services/client/api/timer/timer-log.ts (3)
16-17
: Remove unnecessary commentThe comment "todayStart, todayEnd;" appears to be a remnant and serves no clear purpose.
-// todayStart, todayEnd; -
18-30
: Add JSDoc documentation for the API functionConsider adding documentation to describe the function's purpose, parameters, and return type.
+/** + * Retrieves timesheet logs for a specific organization and time range + * @param {Object} params - The parameters for the API request + * @param {string} params.organizationId - The ID of the organization + * @param {string} params.tenantId - The tenant ID + * @param {string|Date} params.startDate - The start date for the timesheet logs + * @param {string|Date} params.endDate - The end date for the timesheet logs + * @param {string} [params.timeZone] - Optional timezone for the request + * @returns {Promise<ITimeSheet[]>} Array of timesheet entries + * @throws {Error} When required parameters are missing or dates are invalid + */ export async function getTaskTimesheetLogsApi({
41-53
: Consider improving timezone handlingThe current implementation falls back to an empty string for undefined timezone, which might not be the best approach. Consider:
- Using a default timezone (e.g., 'UTC')
- Making the parameter required if timezone-specific data is crucial
const params = new URLSearchParams({ 'activityLevel[start]': '0', 'activityLevel[end]': '100', organizationId, tenantId, startDate: start, endDate: end, - timeZone: timeZone || '', + timeZone: timeZone || 'UTC', // Provide a meaningful default 'relations[0]': 'project', 'relations[1]': 'task', 'relations[2]': 'organizationContact', 'relations[3]': 'employee.user' });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetFilterDate.tsx
(2 hunks)apps/web/app/[locale]/timesheet/[memberId]/page.tsx
(3 hunks)apps/web/app/services/client/api/timer/timer-log.ts
(2 hunks)apps/web/app/services/server/requests/timesheet.ts
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetFilterDate.tsx
- apps/web/app/services/server/requests/timesheet.ts
🧰 Additional context used
🪛 Biome
apps/web/app/services/client/api/timer/timer-log.ts
[error] 37-37: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
[error] 37-37: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
🔇 Additional comments (6)
apps/web/app/services/client/api/timer/timer-log.ts (2)
1-1
: LGTM: Import statement is correctly updated
The addition of ITimeSheet
to the imports is appropriate for the new API function's return type.
55-57
: LGTM: API endpoint construction and return type
The endpoint construction and return type are well implemented. The use of URLSearchParams for query string construction is a good practice.
apps/web/app/[locale]/timesheet/[memberId]/page.tsx (4)
5-6
: LGTM: Imports are well-organized and properly used.
The new imports are correctly structured and all are utilized within the component.
Also applies to: 19-21, 23-24
40-43
: LGTM: Date range state is properly initialized.
Good use of date-fns utilities to ensure proper date boundaries and consistent date handling.
46-49
: Previous review comment about error handling and type safety is still applicable.
The concerns raised in the previous review about proper error handling and type safety haven't been addressed.
152-157
: LGTM: TimesheetFilter props are well-structured.
The initDate prop implementation follows React best practices with proper state management and event handling.
@Innocent-Akim watch out for DeepScan things and fix those (https://deepscan.io/dashboard/#view=project&tid=3293&pid=25855&bid=814579&subview=pull-request&prid=2027442). |
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
TimesheetFilter
component with enhanced initialization options for date filtering.useTimesheet
for improved retrieval and management of timesheet data based on user-defined date ranges.timesheetRapportState
.Enhancements
TimesheetFilterDate
component to respond dynamically to date changes.Bug Fixes