[Feat]: Add time Entry Modal and Update Timesheet Status based on API response #3365
Conversation
WalkthroughThe changes in this pull request enhance the functionality and user experience of the timesheet management components within the application. Key modifications include the introduction of new components and hooks for managing time entries, updating modal interfaces, improving date selection logic, and implementing status updates for timesheets. These updates also involve refining UI elements for better accessibility and visual consistency across different themes. Overall, the changes streamline the interactions related to timesheet management. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AddTaskModal
participant TimesheetFilterDate
participant TimesheetAPI
User->>AddTaskModal: Open modal
AddTaskModal->>TimesheetFilterDate: Request date options
TimesheetFilterDate-->>AddTaskModal: Return valid date range
User->>AddTaskModal: Select date and enter task details
AddTaskModal->>TimesheetAPI: Send task data
TimesheetAPI-->>AddTaskModal: Confirm task creation
AddTaskModal-->>User: Close modal and show confirmation
Possibly related PRs
Suggested reviewers
Warning Rate limit exceeded@Innocent-Akim has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 21 minutes and 30 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 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/AddTaskModal.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 (
|
Innocent-Akim
added
feature
WEB
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
🧹 Outside diff range and nitpick comments (25)
apps/web/app/stores/time-logs.ts (1)
21-21: LGTM! New atom aligns with feature requirementsThe
timesheetUpdateStatusatom is well-structured for managing timesheet status updates:
- Follows consistent naming convention with other atoms
- Properly typed with
UpdateTimesheetStatus[]- Initialized with a safe empty array default
Consider documenting the shape of
UpdateTimesheetStatusand the purpose of this atom in a comment, as it plays a crucial role in the new time entry modal feature.apps/web/app/api/timer/timesheet/status/route.ts (1)
6-28: Consider adding rate limiting for this sensitive endpoint.Since this endpoint modifies timesheet statuses, it should be protected against potential abuse through rate limiting. This is especially important for endpoints that modify data.
Consider implementing rate limiting using Next.js middleware or a service like Redis to track and limit requests per user/IP.
apps/web/app/hooks/useScrollListener.ts (3)
Line range hint
14-14: Remove debugging console.log statementThis console.log statement appears to be debugging code that should be removed from production code.
- console.log(window.scrollY);
27-31: Add TypeScript return type definitionThe hook's return type should be explicitly defined for better type safety and documentation.
-export const useInfinityScroll = (timesheet: TimesheetLog[]) => { +export const useInfinityScroll = (timesheet: TimesheetLog[]): { + items: TimesheetLog[]; + page: number; + setIsLoading: React.Dispatch<React.SetStateAction<boolean>>; + setItems: React.Dispatch<React.SetStateAction<TimesheetLog[]>>; +} => {
24-48: Clean up unnecessary empty linesRemove excessive empty lines to improve code readability while maintaining a clear separation between hooks.
} - - - export const useInfinityScroll = (timesheet: TimesheetLog[]) => { - const [items, setItems] = React.useState<TimesheetLog[]>(timesheet); const [page, setPage] = React.useState(1); const [isLoading, setIsLoading] = React.useState(false); - - React.useEffect(() => { // ... effect implementation }, [isLoading]); - - return { items, page, setIsLoading, setItems } }apps/web/app/interfaces/IDailyPlan.ts (1)
41-46: Consider splitting the interface for better type safety.The
idsfield accepting both array and single ID could lead to inconsistent handling in implementations. Consider creating separate interfaces for single and bulk updates.export interface IUpdateTimesheetStatusBase { organizationId?: ID, status: ID, tenantId?: ID } export interface IUpdateSingleTimesheetStatus extends IUpdateTimesheetStatusBase { id: ID } export interface IUpdateBulkTimesheetStatus extends IUpdateTimesheetStatusBase { ids: ID[] } // Usage example: type IUpdateTimesheetStatus = IUpdateSingleTimesheetStatus | IUpdateBulkTimesheetStatus;apps/web/app/hooks/features/useTimelogFilterOptions.ts (1)
Line range hint
1-51: Consider splitting the hook for better separation of concernsThe hook is handling multiple responsibilities:
- Timesheet filtering
- Time slot generation
- Status management
- Group by days functionality
Consider splitting this into more focused hooks:
useTimesheetFiltersfor filter-related stateuseTimeSlotsfor time generationuseTimesheetStatusfor status managementThis would improve maintainability and make the code more modular. Would you like assistance in implementing this separation?
apps/web/app/services/server/requests/timesheet.ts (1)
101-109: Consider adding specific error handlingWhile the implementation is correct and follows the established patterns, consider adding specific error handling for status update failures, as they might need special handling in the UI.
export function updateStatusTimesheetRequest(params: IUpdateTimesheetStatus, bearer_token: string) { - return serverFetch<UpdateTimesheetStatus[]>({ + return serverFetch<UpdateTimesheetStatus[]>({ path: '/timesheet/status', method: 'PUT', body: { ...params }, bearer_token, tenantId: params.tenantId, - }) + }).catch(error => { + // Handle specific status update errors + console.error('Failed to update timesheet status:', error); + throw error; + }); }apps/web/app/interfaces/timer/ITimerLog.ts (2)
115-117: Remove redundant property declarationsThe properties
isActive,isArchived, andarchivedAtare already inherited fromBaseEntity. Remove these redundant declarations to avoid potential inconsistencies.
129-135: Extract status type for reusabilityThe status union type could be useful in other parts of the codebase. Consider extracting it into a separate type declaration.
export type TimesheetStatus = "DRAFT" | "PENDING" | "IN REVIEW" | "DENIED" | "APPROVED";apps/web/app/services/client/api/timer/timer-log.ts (1)
125-129: Consider architectural improvements for robustnessFor production-grade reliability, consider these architectural improvements:
- Add retry logic for transient failures
- Implement rate limiting for bulk status updates
- Add request cancellation support for pending requests
This would make the function more resilient to network issues and prevent potential API abuse.
apps/web/lib/features/manual-time/manage-member-component.tsx (2)
97-97: Add JSDoc documentation for the new prop.The new
classNameTitleprop should be documented in the interface's JSDoc comments to maintain consistency with other props.Add the following documentation:
*/ itemToValue: (item: T | null, valueKey: string) => string; + /** + * Optional CSS class name to be applied to the label title. + * @type {string} + */ classNameTitle?: string
121-121: Consider extracting the default text size to a theme constant.The hardcoded text size "text-[14px]" could be moved to a theme constant or CSS variable for better maintainability and consistency across the application.
Consider refactoring to use a theme constant:
-<span className={clsxm("text-[14px]", classNameTitle)}>{field.label}</span> +<span className={clsxm("text-default", classNameTitle)}>{field.label}</span>Then define the size in your theme or CSS:
.text-default { font-size: 14px; }apps/web/app/[locale]/timesheet/[memberId]/page.tsx (3)
51-52: Consider removing unnecessary empty lines.These consecutive empty lines don't improve readability and could be removed to maintain better code density.
122-122: Good improvement using dynamic count, but add a safety check.The change from a hardcoded value to a dynamic count is a good improvement. However, consider adding a null/undefined check for better reliability.
-count={statusTimesheet.PENDING.length} +count={statusTimesheet?.PENDING?.length ?? 0}
Line range hint
54-67: Add optional chaining for safer object access in filter logic.The search implementation is well-optimized with useMemo, but could benefit from additional null checks using optional chaining for better reliability.
const filterDataTimesheet = useMemo(() => { const filteredTimesheet = timesheet .filter((v) => v.tasks.some( (task) => - task.task?.title?.toLowerCase()?.includes(lowerCaseSearch) || - task.employee?.fullName?.toLowerCase()?.includes(lowerCaseSearch) || - task.project?.name?.toLowerCase()?.includes(lowerCaseSearch) + task?.task?.title?.toLowerCase()?.includes(lowerCaseSearch) || + task?.employee?.fullName?.toLowerCase()?.includes(lowerCaseSearch) || + task?.project?.name?.toLowerCase()?.includes(lowerCaseSearch) ) ); return filteredTimesheet; }, [ timesheet, lowerCaseSearch, ]);apps/web/app/[locale]/timesheet/[memberId]/components/EditTaskModal.tsx (3)
Line range hint
39-57: Add null checks and improve date handlingThe state initialization logic needs improvement in several areas:
- Missing null checks could lead to runtime errors
- Time formatting doesn't handle invalid dates
- No explicit timezone handling which could cause inconsistencies
Consider applying these improvements:
const [dateRange, setDateRange] = useState<{ from: Date | null }>({ - from: dataTimesheet.timesheet?.startedAt ? new Date(dataTimesheet.timesheet.startedAt) : new Date(), + from: dataTimesheet.timesheet?.startedAt + ? new Date(dataTimesheet.timesheet.startedAt) + : new Date(), }); +const formatTimeWithFallback = (dateString: string | undefined): string => { + if (!dateString) return ''; + try { + const date = new Date(dateString); + if (isNaN(date.getTime())) return ''; + return date.toLocaleTimeString('en-US', { + hour12: false, + timeZone: 'UTC' // Or your preferred timezone + }).slice(0, 5); + } catch { + return ''; + } +}; const [endTime, setEndTime] = useState<string>( - dataTimesheet.timesheet?.stoppedAt - ? new Date(dataTimesheet.timesheet.stoppedAt).toLocaleTimeString('en-US', { hour12: false }).slice(0, 5) - : '' + formatTimeWithFallback(dataTimesheet.timesheet?.stoppedAt) ); const [startTime, setStartTime] = useState<string>( - dataTimesheet.timesheet?.startedAt - ? new Date(dataTimesheet.timesheet.startedAt).toLocaleTimeString('en-US', { hour12: false }).slice(0, 5) - : '' + formatTimeWithFallback(dataTimesheet.timesheet?.startedAt) );
Line range hint
175-223: Improve form validation and user experienceThe current form validation has several issues:
- Empty notes show error state immediately without user interaction
- No error messages are displayed for required fields
- Required attributes are set but no form element exists
Consider these improvements:
+ const [touched, setTouched] = useState({ + notes: false, + startTime: false, + endTime: false + }); + + const handleSubmit = (e: React.FormEvent) => { + e.preventDefault(); + // Add form submission logic + }; - <div className="flex flex-col w-full"> + <form onSubmit={handleSubmit} className="flex flex-col w-full"> {/* ... */} <textarea value={notes} - onChange={(e) => setNotes(e.target.value)} + onChange={(e) => { + setNotes(e.target.value); + setTouched(prev => ({ ...prev, notes: true })); + }} + onBlur={() => setTouched(prev => ({ ...prev, notes: true }))} placeholder="Insert notes here..." className={clsxm( "bg-transparent focus:border-transparent focus:ring-2 focus:ring-transparent", "placeholder-gray-300 placeholder:font-normal resize-none p-2 grow w-full", "border border-gray-200 dark:border-slate-600 dark:bg-dark--theme-light rounded-md h-40 bg-[#FBB6500D]", - notes.trim().length === 0 && "border-red-500" + touched.notes && notes.trim().length === 0 && "border-red-500" )} maxLength={120} minLength={0} aria-label="Insert notes here" required /> + {touched.notes && notes.trim().length === 0 && ( + <span className="text-red-500 text-sm mt-1">Notes are required</span> + )} {/* ... */} - </div> + </form>
Line range hint
58-67: Implement error handling and complete empty handlersThe component has empty handler functions and lacks error handling for user interactions.
Consider implementing proper error handling and completing the handlers:
const handleSelectedValuesChange = (values: { [key: string]: Item | null }) => { try { // Validate values if (!values.Project) { throw new Error('Project is required'); } // Update state or call API // Handle success } catch (error) { // Show error message to user console.error('Failed to update values:', error); } }; const handleChange = (field: string, selectedItem: Item | null) => { try { // Validate field and selectedItem if (!field || !selectedItem) { throw new Error('Invalid selection'); } // Update state or call API // Handle success } catch (error) { // Show error message to user console.error('Failed to handle change:', error); } };apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetFilterDate.tsx (3)
Line range hint
52-57: Inconsistent onChange behavior between handleFromChange and handleToChangeThe
handleFromChangefunction immediately triggers theonChangecallback, whilehandleToChangeonly updates local state. This inconsistency could lead to unexpected behavior when users are selecting date ranges.Consider applying this fix:
const handleToChange = (toDate: Date | null) => { if (dateRange.from && toDate && toDate < dateRange.from) { return; } setDateRange((prev) => ({ ...prev, to: toDate })); + onChange?.({ ...dateRange, to: toDate }); };
Line range hint
59-89: Add validation against minDate/maxDate in preset selectionsThe
handlePresetClickfunction doesn't validate the preset dates against theminDateandmaxDateprops, which could lead to invalid date selections.Consider adding validation:
const handlePresetClick = (preset: string) => { const today = new Date(); + const validateDate = (date: Date) => { + if (maxDate && date > maxDate) return maxDate; + if (minDate && date < minDate) return minDate; + return date; + }; switch (preset) { case t('common.FILTER_TODAY'): - setDateRange({ from: today, to: today }); + setDateRange({ + from: validateDate(today), + to: validateDate(today) + }); // Apply similar validation to other cases
Line range hint
93-121: Enhance accessibility for date picker controlsWhile basic ARIA attributes are present, the date picker controls could benefit from additional accessibility improvements:
Consider these enhancements:
<PopoverTrigger asChild> <Button variant={'outline'} role="combobox" aria-label="Select date range" aria-expanded="false" + aria-haspopup="dialog" + aria-controls="date-range-picker" className={cn( 'w-44 justify-start dark:bg-dark--theme-light dark:text-gray-300 h-[2.2rem]', !dateRange.from && 'text-muted-foreground' )} >apps/web/lib/features/integrations/calendar/table-time-sheet.tsx (2)
494-496: Rename prop to match its usageThe prop name
idssuggests multiple IDs, but it accepts a single string. Consider renaming it totimesheetIdfor better clarity.-export const StatusTask = ({ ids }: { ids: string }) => { +export const StatusTask = ({ timesheetId }: { timesheetId: string }) => {
Consider implementing status transition validation in the backend
Based on the codebase analysis, there's no validation logic for timesheet status transitions. The frontend allows any status change through the
updateTimesheetStatusfunction without restrictions. While the UI shows a clear progression (DRAFT → PENDING → IN REVIEW → APPROVED/DENIED), there's no enforcement of this flow.Key findings:
- Status types are well-defined but transitions between them are not validated
- Direct status updates are possible through
updateTimesheetStatusAPI call- No state machine or workflow rules exist for controlling transitions
Recommendation:
- Implement status transition validation in the backend API
- Consider using a state machine pattern to enforce valid transitions:
- DRAFT → PENDING
- PENDING → IN REVIEW
- IN REVIEW → APPROVED/DENIED
- DENIED → DRAFT (for resubmission)
🔗 Analysis chain
Line range hint
494-543: Consider adding type safety for status transitionsThe current implementation allows any status transition without validation. Consider adding validation for allowed status transitions (e.g., DRAFT can only move to PENDING, etc.).
Let's check if there are any existing status transition validations in the codebase:
Let me gather more information about the status transitions by checking the implementation details:
Let me check if there are any validation rules or state machine definitions for these status transitions:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for status transition validations rg -g '!*.{json,lock}' -i "status.*transition|status.*validation|timesheet.*status" --type tsLength of output: 9401
Script:
#!/bin/bash # Check the TimesheetStatus type definition and any related validation logic ast-grep --pattern 'type TimesheetStatus = $$$' # Check the updateStatusTimesheetFromApi implementation rg -A 10 "updateStatusTimesheetFromApi" --type tsLength of output: 2839
Script:
#!/bin/bash # Look for any status transition validation logic rg -B 5 -A 10 "DRAFT|PENDING|IN REVIEW|DENIED|APPROVED" --type ts # Check for any state machine or workflow definitions rg -i "workflow|state.*machine|transition.*rule|status.*flow" --type tsLength of output: 20341
apps/web/app/[locale]/timesheet/[memberId]/components/AddTaskModal.tsx (1)
48-51: Use translation function for static label textThe label
'Link to Project'is hardcoded, which may hinder localization efforts. Using the translation function ensures consistency across different languages.Apply this diff to utilize the translation function:
const fields = [ { - label: 'Link to Project', + label: t('pages.timesheet.LINK_TO_PROJECT'), placeholder: 'Select a project', isRequired: true, valueKey: 'id', displayKey: 'name', element: 'Project', }, ];Remember to add
'LINK_TO_PROJECT'to your localization files.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (16)
apps/web/app/[locale]/timesheet/[memberId]/components/AddTaskModal.tsx(7 hunks)apps/web/app/[locale]/timesheet/[memberId]/components/EditTaskModal.tsx(3 hunks)apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetFilterDate.tsx(1 hunks)apps/web/app/[locale]/timesheet/[memberId]/page.tsx(2 hunks)apps/web/app/api/timer/timesheet/status/route.ts(1 hunks)apps/web/app/hooks/features/useTimelogFilterOptions.ts(3 hunks)apps/web/app/hooks/features/useTimesheet.ts(5 hunks)apps/web/app/hooks/useScrollListener.ts(2 hunks)apps/web/app/interfaces/IDailyPlan.ts(1 hunks)apps/web/app/interfaces/timer/ITimerLog.ts(1 hunks)apps/web/app/services/client/api/timer/timer-log.ts(2 hunks)apps/web/app/services/server/requests/timesheet.ts(2 hunks)apps/web/app/stores/time-logs.ts(2 hunks)apps/web/components/ui/accordion.tsx(1 hunks)apps/web/lib/features/integrations/calendar/table-time-sheet.tsx(3 hunks)apps/web/lib/features/manual-time/manage-member-component.tsx(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/web/components/ui/accordion.tsx
🧰 Additional context used
📓 Learnings (2)
apps/web/app/[locale]/timesheet/[memberId]/components/AddTaskModal.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.
apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetFilterDate.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] 274-274: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
🔇 Additional comments (15)
apps/web/app/stores/time-logs.ts (1)
3-3: LGTM! Import statement follows consistent pattern
The addition of UpdateTimesheetStatus to the existing imports is clean and follows the established import pattern.
apps/web/app/api/timer/timesheet/status/route.ts (1)
1-4: LGTM! Imports are well-organized.
The imports are properly structured and include all necessary dependencies.
apps/web/app/hooks/features/useTimelogFilterOptions.ts (2)
1-1: LGTM: Clean state management implementation
The addition of new state atoms for timesheet grouping and status updates follows good practices and maintains consistency with the existing state management pattern.
Also applies to: 11-12
46-49: LGTM: Clean interface extension
The new properties are well-organized and follow the established naming conventions.
apps/web/app/services/server/requests/timesheet.ts (2)
4-5: LGTM! Imports are properly organized
The new imports support both existing functionality (TimesheetLog) and the new timesheet status update feature.
101-109: Verify API endpoint and response type
Please verify that:
- The API endpoint
/timesheet/statusexists and accepts PUT requests - The response type is indeed an array (
UpdateTimesheetStatus[]) and not a single object
✅ Verification successful
Let me gather more information about the API endpoint by checking the backend routes and the actual usage of this function.
Based on the gathered information, I can now provide the final response:
API endpoint and response type are correctly implemented
The verification confirms:
- The
/timesheet/statusendpoint exists and accepts PUT requests as evidenced by:- The API route definition in
apps/web/app/api/timer/timesheet/status/route.ts - Consistent usage in both client and server-side code
- The API route definition in
- The response type
UpdateTimesheetStatus[]is correct as:- The interface is defined in
apps/web/app/interfaces/timer/ITimerLog.ts - It's consistently used across the codebase in both client (
timer-log.ts) and server implementations - The endpoint is used to update multiple timesheet statuses at once (bulk update) as seen in the route implementation where
idsis passed as a parameter
- The interface is defined in
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for API route definitions and interface implementations
# Check for API route definition
echo "Checking for API route definition..."
fd -e ts -e tsx "route.ts$" | xargs rg -l "/timesheet/status"
# Check for UpdateTimesheetStatus interface usage
echo "Checking UpdateTimesheetStatus interface usage..."
rg -A 5 "interface UpdateTimesheetStatus"
Length of output: 738
Script:
#!/bin/bash
# Search for backend route definitions
echo "Checking backend route definitions..."
rg -A 5 "timesheet/status" packages/ apps/
# Search for usage of updateStatusTimesheetRequest
echo "Checking function usage..."
rg -A 5 "updateStatusTimesheetRequest" apps/
# Search for any similar timesheet status endpoints
echo "Checking similar endpoints..."
rg -B 2 -A 2 "timesheet.*status" packages/ apps/
Length of output: 9848
apps/web/app/interfaces/timer/ITimerLog.ts (1)
129-135: Verify consistent usage of timesheet status values
Let's verify that these status values are used consistently across the codebase.
✅ Verification successful
Let me gather more information about the status usage in assignments and function calls.
Let me try a different approach to check status usage.
Status values are consistently used across the codebase
Based on the verification results:
- The status values ("DRAFT", "PENDING", "IN REVIEW", "DENIED", "APPROVED") are consistently defined in both
ITimerLog.tsandITask.ts - The values are correctly used in the timesheet hook (
useTimesheet.ts) - The UI components in
TimesheetAction.tsxuse the same status values with proper descriptions - The only variation found is a UI display where "DENIED" is shown as "REJECTED" for display purposes only, but the underlying type remains consistent
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any inconsistent status values that might not match our defined types
rg -t typescript '"(DRAFT|PENDING|IN REVIEW|DENIED|APPROVED)"' --type-add 'typescript:*.{ts,tsx}'
# Search for timesheet status assignments to ensure type safety
ast-grep --pattern 'status = "$_"'
Length of output: 1713
Script:
#!/bin/bash
# Search for status assignments and comparisons in TypeScript files
rg -t typescript 'status\s*(===|==|=|!=|!==)\s*(["'\''])(DRAFT|PENDING|IN REVIEW|DENIED|APPROVED)\2' --type-add 'typescript:*.{ts,tsx}' -A 2
# Search for function calls or object literals with status field
ast-grep --pattern '{ $$$, status: "$_", $$$ }'
Length of output: 514
Script:
#!/bin/bash
# Search for status assignments and comparisons with --pcre2 flag
rg --pcre2 -t typescript 'status\s*(===|==|=|!=|!==)\s*["\x27](DRAFT|PENDING|IN REVIEW|DENIED|APPROVED)["\x27]' --type-add 'typescript:*.{ts,tsx}' -B 1 -A 1
# Look for status field usage in type definitions and interfaces
ast-grep --pattern 'interface $_ {
$$$
status: $_
$$$
}'
Length of output: 17853
apps/web/app/services/client/api/timer/timer-log.ts (2)
1-3: LGTM: Import statements are well-organized
The new imports are properly structured and align with the added functionality for timesheet status updates.
125-129: Verify consistent endpoint usage
Let's ensure the endpoint is used consistently across the codebase and the HTTP method is appropriate.
✅ Verification successful
Based on the results, I can generate the final response as we have sufficient information about the endpoint usage.
Endpoint usage is consistent and properly implemented
The /timesheet/status endpoint is consistently implemented:
- Client-side: Uses PUT method via
put<UpdateTimesheetStatus[]>intimer-log.ts - Server-side: Matches with PUT method configuration in
timesheet.ts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent usage of the timesheet status endpoint and HTTP methods
# Check for all timesheet status endpoint usages
echo "Checking timesheet status endpoint usage:"
rg -A 2 "/timesheet/status"
# Check for PUT method usage patterns
echo "Checking PUT method usage patterns:"
ast-grep --pattern 'put<$_>("/timesheet/status"'
Length of output: 685
apps/web/lib/features/manual-time/manage-member-component.tsx (1)
Line range hint 1-121: Implementation looks good overall!
The changes are well-structured, maintain type safety, and follow React best practices. The addition of the classNameTitle prop enhances the component's flexibility without introducing breaking changes.
apps/web/app/[locale]/timesheet/[memberId]/page.tsx (2)
Line range hint 201-215: Well-structured and maintainable component implementation.
The ViewToggleButton component is well-implemented with:
- Proper TypeScript typing
- Conditional styling with dark mode support
- Internationalization support
- Clean and reusable structure
Line range hint 1-215: Verify API integration for timesheet status updates.
While the UI improvements are well-implemented, I don't see clear evidence of the API integration for updating timesheet status as mentioned in the PR objectives. Please ensure this functionality is implemented or provide the relevant code for review.
✅ Verification successful
Based on the search results, I can now provide a final response:
API integration for timesheet status updates is properly implemented
The codebase shows complete implementation of timesheet status updates:
- The
useTimesheethook inapps/web/app/hooks/features/useTimesheet.tsimplementsupdateTimesheetStatusfunctionality usingupdateStatusTimesheetFromApi - API endpoint
/timer/timesheet/statusis implemented inapps/web/app/api/timer/timesheet/status/route.tsto handle status updates - Server request handler
updateStatusTimesheetRequestinapps/web/app/services/server/requests/timesheet.tsmakes the actual API call - The functionality is properly used in components like
table-time-sheet.tsxfor updating timesheet statuses
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for API integration code related to timesheet status updates
rg -l "updateTimesheetStatus|timesheet.*status.*update" --type ts --type tsx
Length of output: 106
Script:
#!/bin/bash
# Search for API integration code related to timesheet status updates with correct file extensions
rg -l "updateTimesheetStatus|timesheet.*status.*update" "*.ts" "*.tsx"
# Let's also check the useTimesheet hook implementation
ast-grep --pattern 'const useTimesheet = $_'
# Search for any timesheet-related API calls
rg -A 5 "api.*timesheet|timesheet.*api" "*.ts" "*.tsx"
Length of output: 358
Script:
#!/bin/bash
# First, let's find all TypeScript files in the repository
fd -e ts -e tsx
# Then search for timesheet-related API calls in the found files
rg -A 5 "api.*timesheet|timesheet.*api"
# Look for the useTimesheet hook implementation
rg -A 10 "useTimesheet"
# Search for any status update related code
rg -A 5 "status.*update|updateStatus"
Length of output: 93352
apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetFilterDate.tsx (1)
Line range hint 190-241: LGTM! Well-implemented date validation
The DatePickerFilter component properly handles date validation with minDate and maxDate constraints. The implementation is clean and follows best practices.
apps/web/lib/features/integrations/calendar/table-time-sheet.tsx (1)
470-470: LGTM: Proper integration of StatusTask component
The StatusTask component is correctly integrated into the dropdown menu with the proper timesheet ID being passed.
apps/web/app/hooks/features/useTimesheet.ts (1)
251-251: Verify use of timesheet.flat() when generating statusTimesheet
In the expression getStatusTimesheet(timesheet.flat()), ensure that timesheet is an array of arrays before using flat(). If timesheet is already a flat array, using flat() may not be necessary and could have unintended effects.
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
apps/web/app/[locale]/timesheet/[memberId]/components/AddTaskModal.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (6)
apps/web/app/api/timer/timesheet/status/route.ts (3)
6-15: Add type safety and improve error messaging.While the authentication implementation is good, consider these remaining improvements:
export async function PUT(req: Request) { const res = new NextResponse(); const { $res, user, tenantId, organizationId, access_token - } = await authenticatedGuard(req, res); - if (!user) return $res('Unauthorized'); + }: { + $res: (data: any) => Response; + user: { id: string }; + tenantId: string; + organizationId: string; + access_token: string; + } = await authenticatedGuard(req, res); + if (!user) return $res({ + success: false, + message: 'Authentication required to update timesheet status' + });
41-47: Extract validStatuses as a constant.Consider moving the validStatuses array outside the function to improve maintainability and reusability.
+const VALID_TIMESHEET_STATUSES = ['pending', 'approved', 'rejected'] as const; +type TimesheetStatus = typeof VALID_TIMESHEET_STATUSES[number]; + export async function PUT(req: Request) { // ... previous code ... - const validStatuses = ['pending', 'approved', 'rejected']; - if (!validStatuses.includes(status)) { + if (!VALID_TIMESHEET_STATUSES.includes(status as TimesheetStatus)) { return $res({ success: false, - message: 'Invalid status value' + message: `Status must be one of: ${VALID_TIMESHEET_STATUSES.join(', ')}` }); }
48-68: Enhance error logging and success response.Consider improving the error handling and success response:
const { data } = await updateStatusTimesheetRequest( { ids, organizationId, status, tenantId }, access_token ); return $res({ success: true, - data + data, + message: `Successfully updated ${ids.length} timesheet(s) to ${status}` }); } catch (error) { - console.error('Error updating timesheet status:', error); + console.error('Error updating timesheet status:', { + error, + ids, + status, + organizationId, + tenantId + }); return $res({ success: false, - message: 'Failed to update timesheet status' + message: `Failed to update timesheet status: ${error instanceof Error ? error.message : 'Unknown error'}` }); }apps/web/app/[locale]/timesheet/[memberId]/components/AddTaskModal.tsx (3)
Line range hint
65-122: Add form validation for required fields.The task input and project selection are marked as required (*) but lack proper validation logic. This could lead to invalid form submissions.
Consider implementing form validation:
- Add form state management (e.g., using React Hook Form)
- Add error states and messages
- Disable the save button until all required fields are valid
Example implementation:
+ import { useForm } from 'react-hook-form'; + + interface FormInputs { + task: string; + project: string; + } + export function AddTaskModal({ closeModal, isOpen }: IAddTaskModalProps) { + const { register, handleSubmit, formState: { errors } } = useForm<FormInputs>(); + // ... existing code ... return ( - <input - value={task} - onChange={(e) => setTasks(e.target?.value)} + <input + {...register('task', { required: 'Task is required' })} + onChange={(e) => setTasks(e.target?.value)} /> + {errors.task && ( + <span className="text-red-500 text-sm">{errors.task.message}</span> + )}
Line range hint
162-179: Implement form submission handler.The save button is of type "submit" but lacks a form submission handler to process the data.
Add form submission logic:
+ const handleSubmit = async (e: React.FormEvent) => { + e.preventDefault(); + try { + // Validate all required fields + if (!task || !selectedValues.Project || shifts.some(s => !s.startTime || !s.endTime)) { + return; + } + // TODO: Add API call to save the time entry + closeModal(); + } catch (error) { + console.error('Failed to save time entry:', error); + } + }; return ( + <form onSubmit={handleSubmit}> {/* Existing modal content */} + </form> );
275-319: Add loading states for async operations.The accordion UI should handle loading states during time calculations and shift updates.
Add loading states:
+ const [isLoading, setIsLoading] = React.useState(false); const handleAddShift = () => { + setIsLoading(true); setShifts([...shifts, { startTime: '', endTime: '', totalHours: '00:00h', dateFrom: new Date() }, ]); + setIsLoading(false); }; return ( <> {shifts.map((element, index) => ( <Accordion key={index} type="single" collapsible - className="w-full p-1"> + className={clsxm("w-full p-1", + isLoading && "opacity-50 pointer-events-none")}>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
apps/web/app/[locale]/timesheet/[memberId]/components/AddTaskModal.tsx(7 hunks)apps/web/app/api/timer/timesheet/status/route.ts(1 hunks)
🧰 Additional context used
📓 Learnings (1)
apps/web/app/[locale]/timesheet/[memberId]/components/AddTaskModal.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.
🔇 Additional comments (2)
apps/web/app/api/timer/timesheet/status/route.ts (1)
1-5: LGTM! Dependencies are well-organized.
The imports are properly structured and include all necessary dependencies.
apps/web/app/[locale]/timesheet/[memberId]/components/AddTaskModal.tsx (1)
1-31: LGTM! Well-structured initialization of time-related state.
The setup properly handles time slots with 15-minute intervals and initializes the date range with the current date.
apps/web/app/[locale]/timesheet/[memberId]/components/AddTaskModal.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
apps/web/app/[locale]/timesheet/[memberId]/components/AddTaskModal.tsx(7 hunks)
🧰 Additional context used
📓 Learnings (1)
apps/web/app/[locale]/timesheet/[memberId]/components/AddTaskModal.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.
🔇 Additional comments (2)
apps/web/app/[locale]/timesheet/[memberId]/components/AddTaskModal.tsx (2)
183-218:
Add time validation in ShiftTimingSelect.
The component should validate that end time is after start time.
Add time validation:
interface ShiftTimingSelectProps {
label: string;
timeOptions: string[];
placeholder: string;
className?: string;
value?: string;
onChange?: (value: string) => void;
+ minTime?: string;
}
const ShiftTimingSelect = ({
label,
timeOptions,
placeholder,
className,
onChange,
value,
+ minTime
}: ShiftTimingSelectProps) => (
<div className="flex items-center border border-gray-200 dark:border-gray-700 gap-2 rounded w-full">
{/* ... */}
<SelectContent className="z-[10000] max-h-60 overflow-y-auto">
<SelectGroup>
{timeOptions
+ .filter(time => !minTime || time > minTime)
.map((time) => (
<SelectItem
key={time}
value={time}
className="hover:bg-primary focus:bg-primary hover:text-white px-2 py-1 cursor-pointer"
>
{time}
</SelectItem>
))}
</SelectGroup>
</SelectContent>
</div>
);Likely invalid or redundant comment.
Line range hint 1-364: Verify implementation against requirements.
Please ensure that:
- All time entries are properly synchronized with the API
- The timesheet status is updated based on the API response
- The UI feedback is provided for all user actions
✅ Verification successful
Let me analyze the API integration and error handling for the timesheet functionality by checking the related files.
Let me check the timesheet implementation and hooks in more detail.
Let me check the implementation of useTimesheet hook and related API calls.
Let me check the timer API and client-side implementations.
Based on the comprehensive code analysis, I can now provide a final response regarding the review comment's concerns.
API integration and error handling are properly implemented
The timesheet functionality is well integrated with the API layer and includes proper error handling:
-
The AddTaskModal is part of a complete timesheet system that includes:
- Timer status synchronization via
useTimesheethook - API integration through
timer-log.tsservice - Proper status updates through
updateTimesheetStatus - Error handling in API calls with try-catch blocks
- Timer status synchronization via
-
The timesheet status updates are handled through:
- Status synchronization with backend via
updateTimesheetStatus - Proper state management using atoms/stores
- Real-time status updates with proper error boundaries
- Status synchronization with backend via
-
UI feedback is provided through:
- Loading states during API operations
- Form validation with required fields
- Visual indicators for billable status
- Time tracking display with proper formatting
The implementation follows best practices for API integration, state management, and error handling. The concerns raised in the review comment have been addressed in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify API integration and status updates
# Check for API integration
rg -l "updateTimesheet|updateStatus" --type ts
# Check for error handling
rg "catch|error|failed" --type ts
Length of output: 80122
Script:
#!/bin/bash
# Check for timesheet API integration in AddTaskModal and related files
ast-grep --pattern 'useTimesheet()'
# Check for timesheet API calls and status updates
rg -A 5 "updateTimesheet|updateTimesheetStatus" --type ts
# Check for error handling in AddTaskModal
rg -A 5 "catch|error|failed" "apps/web/app/\[locale\]/timesheet/\[memberId\]/components/AddTaskModal.tsx"
Length of output: 2510
Script:
#!/bin/bash
# Check for timesheet hooks and API integration
rg -l "useTimesheet|useTimeLogs|useTimer" --type ts
# Look for timesheet API calls and mutations
ast-grep --pattern 'const { $_ } = useTimesheet($$$)'
# Check AddTaskModal implementation
fd -e tsx -e ts "AddTaskModal" -x cat {}
Length of output: 19272
Script:
#!/bin/bash
# Check useTimesheet hook implementation
cat apps/web/app/hooks/features/useTimesheet.ts
# Check for API endpoints related to timesheet
rg -A 5 "api/timer/timesheet" --type ts
# Check for timesheet mutations and API calls
rg -A 5 "createTimesheet|addTimesheet|saveTimesheet" --type ts
Length of output: 9535
Script:
#!/bin/bash
# Check timer API implementations
rg -A 5 "timer-log" --type ts
# Check for timer/timesheet API routes
fd -e ts -e tsx "route.ts" -x grep -l "timer|timesheet" {}
# Look for timer service implementations
fd -e ts "timer" -x cat {}
Length of output: 62485
apps/web/app/[locale]/timesheet/[memberId]/components/AddTaskModal.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
apps/web/app/[locale]/timesheet/[memberId]/components/AddTaskModal.tsx (2)
265-285: Optimize performance with useCallbackThe
handleShiftChangefunction should be memoized to prevent unnecessary re-renders.-const handleShiftChange = (index: number, field: keyof Shift, value: string) => { +const handleShiftChange = React.useCallback((index: number, field: keyof Shift, value: string) => { const updatedShifts = [...shifts]; updatedShifts[index][field] = value; if (field === 'startTime' || field === 'endTime') { // ... existing overlap check code ... } setShifts(updatedShifts); -}; +}, [shifts]);
351-358: Improve accessibility and type safetyAdd ARIA labels and remove 'any' type casting.
<ShiftTimingSelect label="Start" timeOptions={timeOptions} placeholder="00:00" className="bg-[#30B3661A]" value={value.startTime} + aria-label="Start time" onChange={(value) => onChange(index, 'startTime', value)} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
apps/web/app/[locale]/timesheet/[memberId]/components/AddTaskModal.tsx(7 hunks)
🧰 Additional context used
📓 Learnings (1)
apps/web/app/[locale]/timesheet/[memberId]/components/AddTaskModal.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.
🔇 Additional comments (3)
apps/web/app/[locale]/timesheet/[memberId]/components/AddTaskModal.tsx (3)
342-346: 🛠️ Refactor suggestion
Add date validation to prevent past entries
The DatePickerFilter should prevent selection of past dates.
<DatePickerFilter
label={value.dateFrom ? value.dateFrom.toLocaleString() : 'Select Date'}
date={value.dateFrom as Date}
- setDate={(value) => onChange(index, 'dateFrom', value as any)}
+ setDate={(value) => {
+ const today = new Date();
+ today.setHours(0, 0, 0, 0);
+ if (value && value >= today) {
+ onChange(index, 'dateFrom', value as any);
+ }
+ }}
+ minDate={new Date()}
/>Likely invalid or redundant comment.
171-177: 🛠️ Refactor suggestion
Add form validation before submission
The save button should be disabled when required fields are empty or invalid.
<button
type="submit"
+ disabled={!task || !notes.trim() || shifts.some(s => !s.startTime || !s.endTime)}
className={clsxm(
'bg-primary dark:bg-primary-light h-[2.3rem] w-[5.5rem] justify-center font-normal flex items-center text-white px-2 rounded-lg',
+ 'disabled:opacity-50 disabled:cursor-not-allowed'
)}>
{t('common.SAVE')}
</button>Likely invalid or redundant comment.
183-218: 🛠️ Refactor suggestion
Add time validation in ShiftTimingSelect
The component should validate that end time is after start time to prevent invalid time ranges.
interface ShiftTimingSelectProps {
label: string;
timeOptions: string[];
placeholder: string;
className?: string;
value?: string;
onChange?: (value: string) => void;
+ minTime?: string;
}
const ShiftTimingSelect = ({
label,
timeOptions,
placeholder,
className,
onChange,
value,
+ minTime
}: ShiftTimingSelectProps) => (
// ... existing code ...
<SelectContent className="z-[10000] max-h-60 overflow-y-auto">
<SelectGroup>
{timeOptions
+ .filter(time => !minTime || time > minTime)
.map((time) => (
<SelectItem
key={time}
value={time}
className="hover:bg-primary focus:bg-primary hover:text-white px-2 py-1 cursor-pointer"
>
{time}
</SelectItem>
))}
</SelectGroup>
</SelectContent>
);Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
apps/web/app/[locale]/timesheet/[memberId]/components/AddTaskModal.tsx (1)
26-28: Consider using a more specific type for dateRangeThe current type allows
nullwhich might lead to runtime errors. Consider using a non-nullable type since you're initializing withnew Date().-const [dateRange, setDateRange] = React.useState<{ from: Date | null }>({ +const [dateRange, setDateRange] = React.useState<{ from: Date }>({ from: new Date(), });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
apps/web/app/[locale]/timesheet/[memberId]/components/AddTaskModal.tsx(7 hunks)
🧰 Additional context used
📓 Learnings (1)
apps/web/app/[locale]/timesheet/[memberId]/components/AddTaskModal.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.
🔇 Additional comments (2)
apps/web/app/[locale]/timesheet/[memberId]/components/AddTaskModal.tsx (2)
171-177:
Add form validation before submission
The save button should be disabled when required fields are empty or invalid.
<button
type="submit"
+ disabled={!task || !notes.trim() || shifts.some(s => !s.startTime || !s.endTime)}
className={clsxm(
'bg-primary dark:bg-primary-light h-[2.3rem] w-[5.5rem] justify-center font-normal flex items-center text-white px-2 rounded-lg',
+ 'disabled:opacity-50 disabled:cursor-not-allowed'
)}>
{t('common.SAVE')}
</button>Likely invalid or redundant comment.
343-347:
Prevent selection of past dates
The DatePickerFilter should prevent selection of past dates to ensure valid time entries.
<DatePickerFilter
label={value.dateFrom ? value.dateFrom.toLocaleString() : 'Select Date'}
date={value.dateFrom as Date}
- setDate={(value) => onChange(index, 'dateFrom', value as any)}
+ setDate={(value) => {
+ const today = new Date();
+ today.setHours(0, 0, 0, 0);
+ if (value && value >= today) {
+ onChange(index, 'dateFrom', value as any);
+ }
+ }}
+ minDate={new Date()}
/>Likely invalid or redundant comment.
Innocent-Akim
removed
the
Don't Merge
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
AddTaskModalwith improved time entry options and a new layout for managing shifts.ShiftTimingSelectfor selecting start and end times.ViewToggleButtonfor switching between "ListView" and "CalendarView" in the timesheet.TimesheetFilterDate.Improvements
EditTaskModalfor better visual consistency.StatusTaskcomponent to link status updates directly to timesheet IDs.classNameTitleproperty toManageOrMemberComponentfor customizable styling.Bug Fixes