[Feat]: Timesheet Calendar View UI #3369
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces several enhancements to the timesheet functionality within the application. The Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
apps/web/app/[locale]/timesheet/[memberId]/components/index.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: 5
🧹 Outside diff range and nitpick comments (15)
apps/web/app/[locale]/timesheet/[memberId]/components/index.tsx (1)
12-12: LGTM! Consider organizing exports alphabetically.The new export follows the established pattern and integrates well with the existing timesheet components.
Consider organizing the exports alphabetically for better maintainability:
export * from './CalendarView'; +export * from './CompactTimesheetComponent'; export * from './EditTaskModal'; export * from './FilterWithStatus'; export * from './FrequencySelect'; export * from './RejectSelectedModal'; export * from './TimesheetAction'; export * from './TimesheetCard'; export * from './TimesheetFilter'; export * from './TimesheetFilterDate'; export * from './TimesheetView'; export * from './TimeSheetFilterPopover'; -export * from './CompactTimesheetComponent';apps/web/lib/components/types.ts (3)
4-4: Use consistent property separators in type definitionThe border property uses a comma while other properties use semicolons. Maintain consistency by using semicolons throughout.
- border: string, + border: string;
12-12: Standardize color format and optimize rgba usageThe border colors use inconsistent formats (rgb vs rgba) and some use unnecessary rgba with full opacity. Consider standardizing to a single format for better maintainability.
- border: 'rgb(251, 182, 80)', + border: '#FBB650', - border: 'rgba(48, 179, 102)', + border: '#30B366', - border: 'rgba(220, 38, 38)', + border: '#dc2626', - border: 'rgba(220, 220, 220)', + border: '#dcdcdc', - border: 'rgba(59, 130, 246)', + border: '#3b82f6', - border: 'rgba(243, 244, 246)', + border: '#f3f4f6',Also applies to: 18-18, 24-24, 30-30, 36-36, 42-42
Line range hint
9-46: Consider extracting status colors to a theme configurationThe status colors are well-organized, but consider moving them to a theme configuration file if they might be used across different features or if there's a need for theme customization in the future. This would make it easier to maintain consistent styling across the application and support potential theming requirements.
apps/web/app/[locale]/timesheet/[memberId]/components/CompactTimesheetComponent.tsx (1)
1-4: Enhance component type safety and documentationConsider improving the component's structure with:
- A proper TypeScript interface for props
- JSDoc documentation
- Default prop values
+interface EmployeeAvatarProps { + /** URL of the employee's avatar image */ + imageUrl: string; + /** Optional alt text for the image */ + alt?: string; + /** Optional size in pixels (defaults to 24) */ + size?: number; +} -export const EmployeeAvatar = ({ imageUrl }: { imageUrl: string }) => { +export const EmployeeAvatar = ({ + imageUrl, + alt = 'Employee', + size = 24 +}: EmployeeAvatarProps) => {apps/web/lib/features/task/task-displays.tsx (1)
85-85: Consider using design system color tokens instead of hardcoded hex values.The hardcoded hex values (#282048, #9b8ae1) might make it harder to maintain consistent theming. Consider using your design system's color tokens instead.
- <div className='flex items-center text-[#282048] dark:text-[#9b8ae1]'> + <div className='flex items-center text-primary dark:text-primary-dark'>apps/web/app/[locale]/timesheet/[memberId]/page.tsx (3)
Line range hint
51-64: Add missing dependencies to useMemoThe
filterDataTimesheetuseMemo is missingfilterStatusin its dependency array. This could cause stale data if the filter status changes.Apply this diff:
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) ) ); return filteredTimesheet; }, [ timesheet, lowerCaseSearch, + filterStatus, ]);
Line range hint
126-138: Enhance search input accessibilityWhile the search input has basic accessibility attributes, it could be improved with ARIA attributes and keyboard handling.
Consider applying these enhancements:
<div className="flex items-center !h-[2.2rem] w-[700px] bg-white dark:bg-dark--theme-light gap-x-2 px-2 border border-gray-200 dark:border-gray-700 rounded-sm mb-2"> <GoSearch className="text-[#7E7991]" /> <input onChange={(v) => setSearch(v.target.value)} role="searchbox" aria-label="Search timesheet" + aria-expanded="false" + aria-controls="search-results" + aria-describedby="search-description" type="search" name="timesheet-search" id="timesheet-search" className="!h-[2.2rem] w-full bg-transparent focus:border-transparent focus:ring-2 focus:ring-transparent placeholder-gray-500 placeholder:font-medium shadow-sm outline-none" placeholder={t('common.SEARCH')} /> + <span id="search-description" className="sr-only"> + {t('pages.timesheet.SEARCH_DESCRIPTION')} + </span> </div>
Line range hint
1-240: Consider adding Error BoundariesThe component handles large datasets and complex UI states but lacks error boundaries for graceful failure handling.
Consider wrapping the main content with an Error Boundary component:
import { ErrorBoundary } from 'react-error-boundary'; function ErrorFallback({error, resetErrorBoundary}) { return ( <div role="alert"> <p>Something went wrong:</p> <pre>{error.message}</pre> <button onClick={resetErrorBoundary}>Try again</button> </div> ); } // Wrap the main content <ErrorBoundary FallbackComponent={ErrorFallback} onReset={() => { // Reset the state here }} > <div className="flex flex-col w-full border-1 rounded-lg bg-[#FFFFFF] dark:bg-dark--theme px-4"> {/* ... existing content ... */} </div> </ErrorBoundary>apps/web/app/[locale]/timesheet/[memberId]/components/CalendarView.tsx (3)
13-32: Consider enhancing the loading state with a proper loading indicator.While the component handles all states correctly, the loading state could be improved for better user experience.
Consider using a loading spinner or skeleton component instead of just text:
-<div className="flex items-center justify-center h-full"> - <p>{t('pages.timesheet.LOADING')}</p> -</div> +<div className="flex items-center justify-center h-full"> + <LoadingSpinner className="w-8 h-8" /> + <p className="ml-2">{t('pages.timesheet.LOADING')}</p> +</div>
34-36: Consider adding error boundary for hook usage.The
useTimesheethook could potentially throw errors that should be handled gracefully.Consider wrapping the component with an error boundary:
class TimesheetErrorBoundary extends React.Component { // ... error boundary implementation } const CalendarDataView = ({ data, t }: Props) => { // ... existing implementation } export default function WrappedCalendarDataView(props: Props) { return ( <TimesheetErrorBoundary> <CalendarDataView {...props} /> </TimesheetErrorBoundary> ); }
91-131: Extract task card into a separate component.The task rendering logic inside AccordionContent is complex and could be extracted into a separate component for better maintainability.
Consider creating a TaskCard component:
interface TaskCardProps { task: Task; status: string; statusColor: StatusColor; } const TaskCard: React.FC<TaskCardProps> = ({ task, status, statusColor }) => { return ( <div style={{ backgroundColor: statusColor.bgOpacity, borderLeftColor: statusColor.border }} className={clsxm( 'border-l-4 rounded-l flex flex-col p-2 gap-2 items-start space-x-4 h-[100px]', )} > {/* ... existing task card JSX ... */} </div> ); };Then use it in the AccordionContent:
<AccordionContent className="flex flex-col w-full gap-y-2"> {rows.map((task) => ( <TaskCard key={task.id} task={task} status={status} statusColor={statusColor(status)} /> ))} </AccordionContent>🧰 Tools
🪛 Biome (1.9.4)
[error] 128-128: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
apps/web/lib/features/integrations/calendar/table-time-sheet.tsx (3)
Line range hint
156-167: Improve error handling for deleteTaskTimesheetThe current error handling only logs to console, which doesn't provide feedback to users when operations fail.
Consider implementing proper error handling:
try { deleteTaskTimesheet() .then(() => { setSelectTimesheet([]); setIsDialogOpen(false); + // Show success notification + showNotification({ type: 'success', message: t('timesheet.delete.success') }); }) .catch((error) => { - console.error('Delete timesheet error:', error); + showNotification({ + type: 'error', + message: t('timesheet.delete.error', { details: error.message }) + }); }); } catch (error) { - console.error('Delete timesheet error:', error); + showNotification({ + type: 'error', + message: t('timesheet.delete.error', { details: error.message }) + }); }
Line range hint
168-196: Consider breaking down the DataTableTimeSheet componentThe component is handling too many responsibilities, which makes it harder to maintain and test. Consider splitting it into smaller, more focused components.
Suggested structure:
- TimesheetHeader (handling filters and actions)
- TimesheetBody (handling the main table content)
- TimesheetPagination (handling pagination logic)
- TimesheetStatusGroup (handling status grouping)
This would improve:
- Code maintainability
- Testing capabilities
- Reusability
- Performance (through potential memoization)
Line range hint
197-203: Implement the TODO for approval logicThere's an unimplemented TODO for approval logic in the handleButtonClick function.
Would you like me to help implement the approval logic or create a GitHub issue to track this task?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
apps/web/app/[locale]/timesheet/[memberId]/components/CalendarView.tsx(1 hunks)apps/web/app/[locale]/timesheet/[memberId]/components/CompactTimesheetComponent.tsx(1 hunks)apps/web/app/[locale]/timesheet/[memberId]/components/index.tsx(1 hunks)apps/web/app/[locale]/timesheet/[memberId]/page.tsx(1 hunks)apps/web/lib/components/types.ts(1 hunks)apps/web/lib/features/integrations/calendar/table-time-sheet.tsx(2 hunks)apps/web/lib/features/task/task-displays.tsx(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
apps/web/app/[locale]/timesheet/[memberId]/components/CalendarView.tsx
[error] 128-128: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (6)
apps/web/lib/components/types.ts (1)
Line range hint 49-55: LGTM! Well-documented and robust implementation
The statusColor helper function is well-documented, handles edge cases appropriately, and uses TypeScript features effectively.
apps/web/app/[locale]/timesheet/[memberId]/components/CompactTimesheetComponent.tsx (1)
1-41: Verify component usage and potential duplicates
Let's ensure this component is used consistently and there are no duplicate avatar implementations.
✅ Verification successful
Let me try a different approach with the correct file extensions and patterns.
Let me try one final approach using just the basic file extensions and fd to first locate the relevant files.
Let me try one more search focusing specifically on the file content and avatar implementations:
Based on the search results, I can now provide a final response:
Component is properly scoped and no duplicates found
The EmployeeAvatar component is:
- Used consistently in timesheet-related components
- Has a unique implementation for loading states
- No duplicate avatar implementations with the same functionality exist (other avatar components serve different purposes)
- Used in a limited scope within the timesheet feature
The component's loading pattern is unique to its use case and doesn't need to be consolidated with other avatar implementations in the codebase, which serve different purposes (e.g., TeamAvatar, ProfileAvatar, UserAvatar, etc.).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other avatar implementations
echo "Searching for potential duplicate avatar implementations..."
rg -l "Avatar" --type tsx
# Search for usage patterns of EmployeeAvatar
echo "Checking EmployeeAvatar usage patterns..."
rg "EmployeeAvatar" --type tsx -A 2
# Look for similar image loading patterns
echo "Checking for similar image loading patterns..."
ast-grep --pattern 'img[onLoad=$_][onError=$_]'
Length of output: 492
Script:
#!/bin/bash
# Search for other avatar implementations
echo "Searching for potential duplicate avatar implementations..."
rg -l "Avatar" --type ts --type tsx --type jsx
# Search for usage patterns of EmployeeAvatar
echo "Checking EmployeeAvatar usage patterns..."
rg "EmployeeAvatar" --type ts --type tsx --type jsx -A 2
# Look for similar image loading patterns with both onLoad and onError
echo "Checking for similar image loading patterns..."
rg 'onLoad.*onError|onError.*onLoad' --type ts --type tsx --type jsx
Length of output: 583
Script:
#!/bin/bash
# First, find all TypeScript and JavaScript files
echo "Finding all TypeScript and JavaScript files..."
fd -e ts -e tsx -e js -e jsx
# Then use these file types to search for avatars
echo "Searching for Avatar implementations..."
rg -t js "Avatar" -B 2 -A 2
# Check for EmployeeAvatar usage
echo "Checking EmployeeAvatar usage..."
rg -t js "EmployeeAvatar" -B 2 -A 2
# Look for similar image loading patterns
echo "Looking for similar image loading patterns..."
rg -t js "onLoad.*onError" -B 2 -A 2
Length of output: 70147
Script:
#!/bin/bash
# Search for avatar components in the codebase
echo "Searching for avatar components..."
fd -e tsx -e ts -e js | xargs cat 2>/dev/null | grep -A 10 -B 2 "Avatar.*=.*{" || true
# Search for image loading patterns
echo "Looking for image loading patterns..."
fd -e tsx -e ts -e js | xargs cat 2>/dev/null | grep -A 5 -B 2 "onLoad.*onError" || true
# Search for EmployeeAvatar usage
echo "Looking for EmployeeAvatar usage..."
fd -e tsx -e ts -e js | xargs cat 2>/dev/null | grep -A 2 -B 2 "EmployeeAvatar" || true
Length of output: 19586
apps/web/lib/features/task/task-displays.tsx (1)
107-107: LGTM! Well-implemented styling flexibility.
The addition of the optional className prop and proper use of clsxm for class merging improves the component's reusability while maintaining default styles.
Also applies to: 115-115
apps/web/app/[locale]/timesheet/[memberId]/page.tsx (1)
200-200: LGTM: CalendarView data prop implementation
The addition of the data prop to CalendarView is well implemented and consistent with the ListView implementation.
apps/web/app/[locale]/timesheet/[memberId]/components/CalendarView.tsx (1)
1-11: LGTM! Imports are well-organized and appropriate.
The imports cover all necessary dependencies for the component's functionality.
apps/web/lib/features/integrations/calendar/table-time-sheet.tsx (1)
55-55: LGTM: Import statement is correctly placed
The EmployeeAvatar component import follows the project's conventions and is appropriately grouped with related component imports.
apps/web/app/[locale]/timesheet/[memberId]/components/CompactTimesheetComponent.tsx
Outdated
Show resolved
Hide resolved
apps/web/app/[locale]/timesheet/[memberId]/components/CompactTimesheetComponent.tsx
Show resolved
Hide resolved
apps/web/app/[locale]/timesheet/[memberId]/components/CalendarView.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
apps/web/app/[locale]/timesheet/[memberId]/components/CompactTimesheetComponent.tsx (2)
1-2: Add proper TypeScript types for component propsConsider adding proper type definitions for the component props to improve type safety and documentation.
import React from "react"; + +interface EmployeeAvatarProps { + imageUrl: string; + alt?: string; + size?: number; + onLoadError?: () => void; +}
1-46: Add unit tests for the componentsBoth components lack unit tests. Please add tests to cover:
- Image loading success/failure scenarios
- Loading state transitions
- Spinner rendering and accessibility
- Props validation
Would you like me to help generate a test suite for these components?
apps/web/app/[locale]/timesheet/[memberId]/components/CalendarView.tsx (2)
34-140: Consider breaking down the component for better maintainability.The
CalendarDataViewcomponent is quite large and handles multiple responsibilities. Consider extracting the following into separate components:
- Task item display (lines 93-131)
- Status header (lines 68-90)
Example structure:
const TaskItem = ({ task, status }) => { ... } const StatusHeader = ({ status, rows }) => { ... }🧰 Tools
🪛 Biome (1.9.4)
[error] 129-129: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
61-136: Enhance accordion accessibility.The accordion implementation could benefit from improved accessibility features:
- Add
aria-labelto describe the accordion's purpose- Include proper heading structure within accordion items
-<Accordion type="single" collapsible> +<Accordion + type="single" + collapsible + aria-label={t('pages.timesheet.TIMESHEET_STATUS_SECTIONS')} +>🧰 Tools
🪛 Biome (1.9.4)
[error] 129-129: 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 (2)
apps/web/app/[locale]/timesheet/[memberId]/components/CalendarView.tsx(1 hunks)apps/web/app/[locale]/timesheet/[memberId]/components/CompactTimesheetComponent.tsx(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
apps/web/app/[locale]/timesheet/[memberId]/components/CalendarView.tsx
[error] 129-129: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (4)
apps/web/app/[locale]/timesheet/[memberId]/components/CompactTimesheetComponent.tsx (2)
3-22:
Critical: Address previous review feedback for robustness
The current implementation needs significant improvements in several areas that were previously identified:
- Error handling is minimal (only sets loading to false)
- Missing accessibility attributes
- Fixed dimensions limit reusability
- Basic loading state management
The previous review comments provided comprehensive solutions for these issues. Please implement those suggestions, particularly:
- Proper error state management with retry mechanism
- Accessibility attributes
- Dynamic sizing
- Improved loading state handling
25-46: 🛠️ Refactor suggestion
Move LoadingSpinner to shared components and enhance accessibility
The LoadingSpinner is a generic UI component that could be reused across the application. Consider:
- Moving it to a shared components directory
- Adding proper accessibility attributes for screen readers
const LoadingSpinner = ({ className }: { className?: string }) => (
<svg
- className={`animate-spin text-gray-500 ${className}`}
+ className={`animate-spin text-gray-500 ${className ?? ''}`}
xmlns="http://www.w3.org/2000/svg"
fill="none"
viewBox="0 0 24 24"
+ role="status"
+ aria-label="Loading..."
>Let's check if this component is already used elsewhere in the codebase:
apps/web/app/[locale]/timesheet/[memberId]/components/CalendarView.tsx (2)
1-11: LGTM! Imports are well-organized and relevant.
All necessary dependencies are properly imported for the component's functionality.
13-32: LGTM! Well-structured component with proper state handling.
The component effectively handles all data states (loading, empty, and populated) with appropriate user feedback.
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
CalendarDataViewcomponent for enhanced timesheet data display with conditional rendering.EmployeeAvatarcomponent for improved loading state and error handling of employee images.CalendarViewto accept filtered timesheet data for better user experience.LoadingSpinnercomponent to provide visual feedback during image loading.Improvements
DisplayTimeForTimesheetcomponent for better visibility.TotalDurationByDatecomponent for greater flexibility.Bug Fixes
EmployeeAvatarcomponent.