[Feat]: DataTableTimeSheet and SelectFilter components for improved and status management #3223
Conversation
…roved readability and status management
WalkthroughThe changes in this pull request involve updates to several components within the timesheet feature of the application. The Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
🧹 Outside diff range and nitpick comments (14)
apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetFilter.tsx (2)
Line range hint
7-15: Consider optimizing the grid layoutThe current layout includes an empty middle column which could be better utilized. Consider adjusting the grid structure for more efficient space usage.
-<div className="grid grid-cols-3 w-full"> - <div className="col-span-1"> +<div className="grid grid-cols-2 w-full gap-4"> + <div> <FilterWithStatus activeStatus="Rejected" onToggle={(label) => { console.log(label) }} /> </div> - <div className="col-span-1"></div> - <div className="col-span-1"> + <div>
Line range hint
11-13: Remove console.log statementDevelopment logging should not be present in production code. Consider implementing proper error handling or logging service.
- onToggle={(label) => { - console.log(label) - }} + onToggle={(label) => { + // Implement proper handler + }}apps/web/lib/features/integrations/calendar/helper-calendar.ts (2)
22-22: Remove redundant text in task description.The task description contains duplicated text "& making prototype for user testing tomorrow".
- task: "#324 Working on UI Design & making prototype for user testing tomorrow & making prototype for user testing tomorrow", + task: "#324 Working on UI Design & making prototype for user testing tomorrow",
Line range hint
16-93: Consider restructuring mock data for better maintainability.The current mock data structure could be improved in several ways:
- Extract common values (like description, employee) into constants
- Use a factory function to generate mock entries
- Consider moving mock data to a separate file
Here's a suggested approach:
// mock-data/timesheet.ts const COMMON_VALUES = { description: "Members count 11", employee: "Ruslan Konviser", }; const createTimeSheetEntry = ( id: number, task: string, name: string, status: TimeSheet['status'], time: string ): TimeSheet => ({ id, task, name, status, time, ...COMMON_VALUES }); export const dataSourceTimeSheet: TimeSheet[] = [ createTimeSheetEntry( 1, "#324 Working on UI Design & making prototype for user testing tomorrow", "Ever Gauzy", "Approved", "08:00h" ), // ... other entries ];apps/web/components/ui/data-table.tsx (1)
Line range hint
18-26: Utilize the noResultsMessage prop for empty state.The interface defines a
noResultsMessageprop with heading and content, but the empty state renders a hardcoded "No results." message.Consider using the provided prop:
<TableRow> <TableCell colSpan={columns.length} className="h-24 text-center"> - No results. + {noResultsMessage ? ( + <> + <h3 className="font-medium">{noResultsMessage.heading}</h3> + <p className="text-sm text-gray-500">{noResultsMessage.content}</p> + </> + ) : ( + 'No results.' + )} </TableCell> </TableRow>Also applies to: 156-158
apps/web/lib/features/integrations/calendar/time-sheet-filter-popover.tsx (7)
Line range hint
1-10: Add missing TypeScript type imports for better type safety.Consider importing types for the components being used:
+import { ITeamTask } from '@app/interfaces'; +import { Member } from '@app/interfaces/ITeamMember';
Line range hint
25-37: Fix Project filter implementation issues.Several issues need attention:
- The items prop uses team members instead of projects
- Console.log in onValueChange needs to be replaced with actual state management
- No state tracking for selected projects
Consider implementing proper state management:
+const [selectedProjects, setSelectedProjects] = useState<Project[]>([]); <MultiSelect - items={activeTeam?.members ?? []} + items={activeTeam?.projects ?? []} - itemToString={(members) => (members ? members.employee.fullName : '')} + itemToString={(project) => (project ? project.name : '')} - itemId={(item) => item.id} + itemId={(project) => project.id} - onValueChange={(selectedItems) => console.log(selectedItems)} + onValueChange={setSelectedProjects} multiSelect={true} triggerClassName="dark:border-gray-700" />
Line range hint
64-76: Implement proper state management for task selection.The current implementation simply returns the task without managing its state:
+const [selectedTasks, setSelectedTasks] = useState<ITeamTask[]>([]); <MultiSelect items={tasks} - onValueChange={(task) => task} + onValueChange={setSelectedTasks} itemId={(task) => (task ? task.id : '')} itemToString={(task) => (task ? task.title : '')} multiSelect={true} triggerClassName="dark:border-gray-700" />
Line range hint
90-110: Add proper handling for worked hours inputs.The time inputs are currently non-functional:
- Missing onChange handlers
- No time validation
- No state management
Consider implementing proper input handling:
+const [timeRange, setTimeRange] = useState({ from: '', to: '' }); + +const handleTimeChange = (e: React.ChangeEvent<HTMLInputElement>) => { + const { name, value } = e.target; + // Add time format validation here + setTimeRange(prev => ({ ...prev, [name]: value })); +}; <Input className="border w-full dark:border-gray-700" placeholder="Time" required - value="" + value={timeRange.from} + onChange={handleTimeChange} name="from" + type="time" />
Line range hint
111-124: Implement handlers for Clear and Apply filter buttons.The action buttons are currently non-functional. They need handlers to:
- Clear all selected filters
- Apply the selected filters to the timesheet data
+const handleClearFilters = () => { + setSelectedProjects([]); + setSelectedTasks([]); + setTimeRange({ from: '', to: '' }); + // Clear other filter states +}; + +const handleApplyFilters = () => { + // Implement filter application logic + closeModal(); +}; <Button variant={'outline'} + onClick={handleClearFilters} className='flex items-center text-sm justify-center h-10 w-full rounded-lg dark:text-gray-300'> <span className="text-sm">Clear Filter</span> </Button> <Button + onClick={handleApplyFilters} className='flex items-center text-sm justify-center h-10 w-full rounded-lg bg-primary dark:bg-primary-light dark:text-gray-300'> <span className="text-sm">Apply Filter</span> </Button>
Line range hint
13-126: Consider implementing centralized state management and filter persistence.The current implementation could benefit from:
- A centralized filter state using a reducer or context
- Filter persistence across sessions
- Reusable filter components to reduce duplication
Consider implementing a custom hook for filter management:
interface FilterState { projects: Project[]; tasks: ITeamTask[]; timeRange: { from: string; to: string }; // other filter states } function useTimeSheetFilters() { const [filters, setFilters] = useState<FilterState>(() => { // Load saved filters from localStorage const saved = localStorage.getItem('timesheet-filters'); return saved ? JSON.parse(saved) : defaultFilters; }); // Save filters on change useEffect(() => { localStorage.setItem('timesheet-filters', JSON.stringify(filters)); }, [filters]); return { filters, setFilters, clearFilters: () => setFilters(defaultFilters), // other filter operations }; }
Line range hint
13-126: Enhance accessibility and user experience.Consider the following improvements:
- Add proper aria labels for accessibility
- Implement loading states during filter operations
- Add error handling and user feedback
<Modal isOpen={isOpen} closeModal={closeModal} title={"Filters"} + aria-label="Timesheet filters" className="bg-light--theme-light dark:bg-dark--theme-light p-5 rounded-xl !w-[350px] md:w-40 md:min-w-[24rem] h-[auto] justify-start shadow-xl" titleClass="font-bold" > + {isLoading && <LoadingSpinner />} + {error && <ErrorMessage message={error} />} // ... rest of the componentapps/web/lib/features/integrations/calendar/table-time-sheet.tsx (2)
94-100: Accessibility Improvement: Associate checkboxes with labelsTo enhance accessibility, associate each checkbox with its corresponding task label using the
idandhtmlForattributes. This change helps screen readers correctly identify form elements.Apply the following changes:
<div className="flex items-center gap-x-4 w-[640px]" > + <label htmlFor={`select-row-${row.id}`} className="flex items-center gap-x-4"> <Checkbox + id={`select-row-${row.id}`} checked={row.getIsSelected()} onCheckedChange={(value) => row.toggleSelected(!!value)} aria-label="Select row" /> <span className="capitalize !text-sm break-words whitespace-break-spaces sm:text-base !truncate !overflow-hidden">{row.original.task}</span> + </label> </div>
424-428: Suggestion: Improve accessibility with alt textIn the
TaskDetailscomponent, addaltattributes to images or providearia-labelswhere appropriate. This practice enhances accessibility for users relying on assistive technologies.For example:
<div className="flex items-center justify-center h-8 w-8 rounded-lg bg-primary dark:bg-primary-light text-white shadow p-2"> - <span className="lowercase font-medium text-[10px]">ever</span> + <span className="lowercase font-medium text-[10px]" aria-label="Logo">ever</span> </div>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetFilter.tsx(2 hunks)apps/web/app/[locale]/timesheet/[memberId]/components/index.tsx(1 hunks)apps/web/app/[locale]/timesheet/[memberId]/page.tsx(5 hunks)apps/web/app/helpers/date.ts(1 hunks)apps/web/components/app-sidebar.tsx(3 hunks)apps/web/components/ui/data-table.tsx(1 hunks)apps/web/lib/features/integrations/calendar/confirm-change-status.tsx(2 hunks)apps/web/lib/features/integrations/calendar/helper-calendar.ts(1 hunks)apps/web/lib/features/integrations/calendar/table-time-sheet.tsx(9 hunks)apps/web/lib/features/integrations/calendar/time-sheet-filter-popover.tsx(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 (8)
apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetFilter.tsx (2)
1-3: LGTM: Clean import organization
The imports are well-organized and the new TimeSheetFilterPopover component is properly imported.
Line range hint 23-27: Implement onClick handler for Add Time button
The button currently has a null onClick handler. Based on previous feedback, while AddManualTimeModal should not be used, a proper handler implementation is still needed.
Let's check for any existing time entry handling patterns:
apps/web/components/ui/data-table.tsx (1)
89-91: LGTM! Formatting improvement for flexRender parameters.
The parameters are correctly ordered and properly indented, improving code readability.
apps/web/lib/features/integrations/calendar/confirm-change-status.tsx (2)
4-5: LGTM! Import statements are properly structured.
96-100: 🛠️ Refactor suggestion
Consider using Tailwind's color system instead of hex values.
The current implementation replaces Tailwind's semantic color classes with direct hex values:
!bg-red-500→!bg-[#dc2626]!bg-green-500→!bg-[#30B366]!bg-orange-500→!bg-[#FBB650]
This change has several potential drawbacks:
- Loss of Tailwind's built-in dark mode support
- Reduced maintainability if colors need to be updated globally
- Potential inconsistency with the project's design system
Consider reverting to Tailwind's color system or using CSS variables for custom colors:
- return `text-black ${filterNumber ? "border-gray-200 dark:border-gray-700" : "!bg-[#dc2626]"} `;
+ return `text-black ${filterNumber ? "border-gray-200 dark:border-gray-700" : "!bg-red-500 dark:!bg-red-600"} `;
- return `text-black ${filterNumber ? "border-gray-200 dark:border-gray-700" : "!bg-[#30B366]"}`;
+ return `text-black ${filterNumber ? "border-gray-200 dark:border-gray-700" : "!bg-green-500 dark:!bg-green-600"}`;
- return `text-black ${filterNumber ? "border-gray-200 dark:border-gray-700" : "!bg-[#FBB650]"} `;
+ return `text-black ${filterNumber ? "border-gray-200 dark:border-gray-700" : "!bg-orange-500 dark:!bg-orange-600"} `;If custom colors are required, consider defining them in your Tailwind config or CSS variables:
// tailwind.config.js
module.exports = {
theme: {
extend: {
colors: {
'status-rejected': '#dc2626',
'status-approved': '#30B366',
'status-pending': '#FBB650',
}
}
}
}Let's verify if these colors are defined in the theme configuration:
apps/web/app/[locale]/timesheet/[memberId]/page.tsx (3)
8-8: LGTM! Good use of React.memo for performance optimization.
The component structure changes and new imports are well-organized. The use of React.memo is appropriate for preventing unnecessary re-renders of this complex component.
Also applies to: 17-17, 32-32
42-43: LGTM! Clean navigation implementation.
The URL parameter handling and breadcrumb navigation are well-implemented with proper dependency tracking in useMemo.
Also applies to: 48-48
34-35: 🛠️ Refactor suggestion
Consider edge cases and localization for the greeting.
While the user name fallback chain is good, consider these improvements:
- Add a default greeting when no user name is available
- Use the translation system for the greeting message to support multiple languages
Consider updating the greeting implementation:
- <h1 className='!text-[23px] font-bold text-[#282048]'>{getGreeting()}, {username} !</h1>
+ <h1 className='!text-[23px] font-bold text-[#282048]'>
+ {t('greetings.' + getGreeting())}, {username || t('common.guest')} !
+ </h1>Also applies to: 72-72
apps/web/app/[locale]/timesheet/[memberId]/components/index.tsx
Outdated
Show resolved
Hide resolved
| task: "#324 Working on UI Design & making prototype for user testing tomorrow & making prototype for user testing tomorrow", | ||
| name: "Ever Gauzy", |
There was a problem hiding this comment.
💡 Codebase verification
Mock data consistency needs improvement
The review comment is correct. The mock data shows clear inconsistency where only the first two entries (#324 and #321) have realistic task descriptions, while entries 3-8 all contain identical dependency bump messages. This creates an unrealistic test dataset.
- Entry 1-2: Real task descriptions (UI Design, Calendar views)
- Entry 3-8: Duplicate placeholder messages ("chore(deps-dev): bump karma from 5.2.3 to 6.3.16chore")
🔗 Analysis chain
Ensure consistency in mock data entries.
The first two entries have been updated with real task descriptions while the remaining entries still contain placeholder dependency bump messages. Consider updating all entries with realistic task descriptions to maintain consistency.
Let's verify if there are any actual dependency bump tasks in the codebase:
Also applies to: 31-31
Let me check the full context of these mock entries to better understand the consistency issue.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for dependency bump related commits or issues
rg -i "bump.*from.*to" --type ts
Length of output: 838
Script:
#!/bin/bash
# Get the full context of the mock data entries
rg -A 2 -B 2 "task:" apps/web/lib/features/integrations/calendar/helper-calendar.ts
Length of output: 1619
| header.column.columnDef.header, | ||
| header.getContext() | ||
| )} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving type safety for tooltip handling.
While the header rendering is correct, the tooltip handling above this section uses any type which could be improved for better type safety.
Consider defining proper types for the tooltip:
interface ColumnTooltipDef<TData, TValue> extends ColumnDef<TData, TValue> {
tooltip?: (context: any) => string | undefined;
}Then update the tooltip extraction:
- const tooltip: any = header.column.columnDef;
+ const tooltip = header.column.columnDef as ColumnTooltipDef<TData, TValue>;| @@ -67,7 +64,7 @@ function getStatusColor(status: string) { | |||
| case 'Approved': | |||
| return "text-gray-500"; | |||
| case 'Pending': | |||
| return "text-orange-400"; | |||
| return "text-[#FBB650]"; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Suggestion: Use centralized color management for status styles
The getStatusColor function returns a hardcoded hex color for the "Pending" status. Consider using a theme or a constants file to manage colors globally. This approach enhances maintainability and ensures consistency across the application.
apps/web/lib/features/integrations/calendar/table-time-sheet.tsx
Outdated
Show resolved
Hide resolved
apps/web/lib/features/integrations/calendar/table-time-sheet.tsx
Outdated
Show resolved
Hide resolved
| const groupedRows = { | ||
| Pending: table.getRowModel().rows.filter(row => row.original.status === "Pending"), | ||
| Approved: table.getRowModel().rows.filter(row => row.original.status === "Approved"), | ||
| Rejected: table.getRowModel().rows.filter(row => row.original.status === "Rejected") | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhancement: Dynamically group rows by status
Currently, groupedRows handles only predefined statuses. To make the component more flexible and accommodate future statuses, consider dynamically grouping rows based on the statuses present in the data.
Update the grouping logic:
- const groupedRows = {
- Pending: table.getRowModel().rows.filter(row => row.original.status === "Pending"),
- Approved: table.getRowModel().rows.filter(row => row.original.status === "Approved"),
- Rejected: table.getRowModel().rows.filter(row => row.original.status === "Rejected")
- };
+ const groupedRows = table.getRowModel().rows.reduce((groups, row) => {
+ const status = row.original.status;
+ if (!groups[status]) {
+ groups[status] = [];
+ }
+ groups[status].push(row);
+ return groups;
+ }, {});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const groupedRows = { | |
| Pending: table.getRowModel().rows.filter(row => row.original.status === "Pending"), | |
| Approved: table.getRowModel().rows.filter(row => row.original.status === "Approved"), | |
| Rejected: table.getRowModel().rows.filter(row => row.original.status === "Rejected") | |
| }; | |
| const groupedRows = table.getRowModel().rows.reduce((groups, row) => { | |
| const status = row.original.status; | |
| if (!groups[status]) { | |
| groups[status] = []; | |
| } | |
| groups[status].push(row); | |
| return groups; | |
| }, {}); |
apps/web/lib/features/integrations/calendar/table-time-sheet.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (1)
apps/web/lib/features/integrations/calendar/table-time-sheet.tsx (1)
216-226: Remove commented code.The commented-out header section should be either removed or implemented. Commented code can lead to confusion and maintenance issues.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
apps/web/app/[locale]/timesheet/[memberId]/components/index.tsx(1 hunks)apps/web/app/[locale]/timesheet/[memberId]/page.tsx(6 hunks)apps/web/app/helpers/date.ts(1 hunks)apps/web/lib/components/custom-select/multi-select.tsx(3 hunks)apps/web/lib/features/integrations/calendar/table-time-sheet.tsx(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/web/app/[locale]/timesheet/[memberId]/components/index.tsx
- apps/web/app/[locale]/timesheet/[memberId]/page.tsx
- apps/web/app/helpers/date.ts
🔇 Additional comments (2)
apps/web/lib/features/integrations/calendar/table-time-sheet.tsx (2)
204-208: Consider using a more dynamic row grouping approach.
The current implementation of groupedRows is hardcoded to specific status values. A more dynamic approach would be more maintainable and flexible.
401-405:
Improve accessibility of hidden description.
The description is currently hidden using display: none, which makes it inaccessible to screen readers. Consider using visually-hidden utilities that maintain accessibility.
-<div style={{ display: 'none' }}>{description}</div>
+<div className="sr-only">{description}</div>Likely invalid or redundant comment.
| @@ -134,7 +135,7 @@ export function MultiSelect<T>({ | |||
| {selectedItems.map((item) => ( | |||
| <div | |||
| key={itemId(item)} | |||
| className="flex items-center justify-between bg-gray-100 dark:bg-slate-700 px-2 py-[0.5px] rounded text-[12px] dark:text-white" | |||
| className={clsxm("flex items-center justify-between bg-gray-100 dark:bg-slate-700 px-2 py-[0.5px] rounded text-[12px] dark:text-white", statusColor(itemToString(item)).bg!)} | |||
There was a problem hiding this comment.
Improve type safety and status handling.
The current implementation has several potential issues:
- Direct string comparison with status values is fragile
- Assumes itemToString returns a status value
- Force-unwrapping the bg property could cause runtime errors
Consider this safer implementation:
- className={clsxm("flex items-center justify-between bg-gray-100 dark:bg-slate-700 px-2 py-[0.5px] rounded text-[12px] dark:text-white", statusColor(itemToString(item)).bg!)}
+ className={clsxm(
+ "flex items-center justify-between px-2 py-[0.5px] rounded text-[12px]",
+ "dark:text-white",
+ statusColor(itemToString(item))?.bg || "bg-gray-100 dark:bg-slate-700"
+ )}Also, consider adding a type-safe status enum:
export enum ItemStatus {
Pending = 'Pending',
Approved = 'Approved',
Rejected = 'Rejected'
}| const statusColor = (status: string) => { | ||
| return status === 'Pending' | ||
| ? { bg: 'bg-[#FBB650]', text: 'text-[#FBB650]', bgOpacity: 'rgba(251, 182, 80, 0.1)' } | ||
| : status === 'Approved' | ||
| ? { bg: 'bg-[#30B366]', text: 'text-[#30B366]', bgOpacity: 'rgba(48, 179, 102, 0.1)' } | ||
| : { bg: 'bg-[#dc2626]', text: 'text-[#dc2626]', bgOpacity: 'rgba(220, 38, 38, 0.1)' }; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider moving color values to a theme configuration.
The statusColor function uses hardcoded color values. For better maintainability and consistency, consider moving these values to a theme configuration file.
Create a theme configuration file and update the function:
+// theme/colors.ts
+export const statusColors = {
+ pending: {
+ bg: '#FBB650',
+ text: '#FBB650',
+ opacity: 0.1
+ },
+ approved: {
+ bg: '#30B366',
+ text: '#30B366',
+ opacity: 0.1
+ },
+ rejected: {
+ bg: '#dc2626',
+ text: '#dc2626',
+ opacity: 0.1
+ }
+};
+// table-time-sheet.tsx
+import { statusColors } from '@/theme/colors';
const statusColor = (status: string) => {
- return status === 'Pending'
- ? { bg: 'bg-[#FBB650]', text: 'text-[#FBB650]', bgOpacity: 'rgba(251, 182, 80, 0.1)' }
- : status === 'Approved'
- ? { bg: 'bg-[#30B366]', text: 'text-[#30B366]', bgOpacity: 'rgba(48, 179, 102, 0.1)' }
- : { bg: 'bg-[#dc2626]', text: 'text-[#dc2626]', bgOpacity: 'rgba(220, 38, 38, 0.1)' };
+ const color = status.toLowerCase();
+ return {
+ bg: `bg-[${statusColors[color].bg}]`,
+ text: `text-[${statusColors[color].text}]`,
+ bgOpacity: `rgba(${statusColors[color].bg}, ${statusColors[color].opacity})`
+ };
};Committable suggestion skipped: line range outside the PR's diff.
apps/web/lib/features/integrations/calendar/table-time-sheet.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (1)
apps/web/lib/features/integrations/calendar/table-time-sheet.tsx (1)
211-221: Remove commented-out code.The commented-out TableHeader code should be removed to maintain code cleanliness.
- {/* <TableHeader> - {table.getHeaderGroups().map((headerGroup) => ( - <TableRow key={headerGroup.id}> - {headerGroup.headers.map((header) => ( - <TableHead key={header.id}> - {header.isPlaceholder ? null : flexRender(header.column.````columnDef.header, header.getContext())} - </TableHead> - ))} - </TableRow> - ))} - </TableHeader> */}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
apps/web/lib/components/custom-select/multi-select.tsx(2 hunks)apps/web/lib/components/types.ts(1 hunks)apps/web/lib/features/integrations/calendar/table-time-sheet.tsx(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/lib/components/custom-select/multi-select.tsx
| type StatusColorScheme = { | ||
| bg: string; | ||
| text: string; | ||
| bgOpacity: string; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving type safety and consistency
The type definition could be more specific and consistent in its styling approach:
- Consider using union types for Tailwind classes to prevent invalid values
- Consider standardizing the color format (either all Tailwind or all rgba)
type StatusColorScheme = {
- bg: string;
- text: string;
- bgOpacity: string;
+ bg: `bg-[#${string}]`;
+ text: `text-[#${string}]`;
+ bgOpacity: `rgba(${number}, ${number}, ${number}, ${number})`;
};Committable suggestion skipped: line range outside the PR's diff.
| const STATUS_COLORS: Record<string, StatusColorScheme> = { | ||
| Pending: { | ||
| bg: 'bg-[#FBB650]', | ||
| text: 'text-[#FBB650]', | ||
| bgOpacity: 'rgba(251, 182, 80, 0.1)' | ||
| }, | ||
| Approved: { | ||
| bg: 'bg-[#30B366]', | ||
| text: 'text-[#30B366]', | ||
| bgOpacity: 'rgba(48, 179, 102, 0.1)' | ||
| }, | ||
| Rejected: { | ||
| bg: 'bg-[#dc2626]', | ||
| text: 'text-[#dc2626]', | ||
| bgOpacity: 'rgba(220, 38, 38, 0.1)' | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance type safety and completeness of status colors
The status colors implementation could be improved:
- Consider using a union type for status keys to prevent typos
- Consider adding other common statuses that might be needed
- Consider centralizing color values in a theme configuration
+type StatusType = 'Pending' | 'Approved' | 'Rejected' | 'In Progress' | 'Cancelled';
-const STATUS_COLORS: Record<string, StatusColorScheme> = {
+const STATUS_COLORS: Record<StatusType, StatusColorScheme> = {
Pending: {
bg: 'bg-[#FBB650]',
text: 'text-[#FBB650]',
bgOpacity: 'rgba(251, 182, 80, 0.1)'
},
// ... existing entries ...
+ 'In Progress': {
+ bg: 'bg-[#3B82F6]',
+ text: 'text-[#3B82F6]',
+ bgOpacity: 'rgba(59, 130, 246, 0.1)'
+ },
+ 'Cancelled': {
+ bg: 'bg-[#6B7280]',
+ text: 'text-[#6B7280]',
+ bgOpacity: 'rgba(107, 114, 128, 0.1)'
+ }
};Committable suggestion skipped: line range outside the PR's diff.
| /** | ||
| * Returns color scheme for a given status | ||
| * @param status - The status string to get colors for | ||
| * @returns StatusColorScheme object or empty object if status not found | ||
| */ | ||
| export const statusColor = (status: string): Partial<StatusColorScheme> => { | ||
| return STATUS_COLORS[status] || {}; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type safety and error handling in statusColor function
The current implementation silently fails for invalid statuses, which could lead to runtime issues:
- Consider using the StatusType union type for the status parameter
- Consider throwing an error or providing a default color scheme for invalid statuses
-export const statusColor = (status: string): Partial<StatusColorScheme> => {
+export const statusColor = (status: StatusType): StatusColorScheme => {
- return STATUS_COLORS[status] || {};
+ const colorScheme = STATUS_COLORS[status];
+ if (!colorScheme) {
+ console.warn(`Unknown status: ${status}, falling back to default scheme`);
+ return STATUS_COLORS.Pending; // Use a default scheme instead of empty object
+ }
+ return colorScheme;
};Committable suggestion skipped: line range outside the PR's diff.
| export type IVariant = 'primary' | 'outline' | 'ghost' | 'light' | 'dark'; | ||
| type StatusColorScheme = { | ||
| bg: string; | ||
| text: string; | ||
| bgOpacity: string; | ||
| }; | ||
|
|
||
| const STATUS_COLORS: Record<string, StatusColorScheme> = { | ||
| Pending: { | ||
| bg: 'bg-[#FBB650]', | ||
| text: 'text-[#FBB650]', | ||
| bgOpacity: 'rgba(251, 182, 80, 0.1)' | ||
| }, | ||
| Approved: { | ||
| bg: 'bg-[#30B366]', | ||
| text: 'text-[#30B366]', | ||
| bgOpacity: 'rgba(48, 179, 102, 0.1)' | ||
| }, | ||
| Rejected: { | ||
| bg: 'bg-[#dc2626]', | ||
| text: 'text-[#dc2626]', | ||
| bgOpacity: 'rgba(220, 38, 38, 0.1)' | ||
| } | ||
| }; | ||
|
|
||
|
|
||
| /** | ||
| * Returns color scheme for a given status | ||
| * @param status - The status string to get colors for | ||
| * @returns StatusColorScheme object or empty object if status not found | ||
| */ | ||
| export const statusColor = (status: string): Partial<StatusColorScheme> => { | ||
| return STATUS_COLORS[status] || {}; | ||
| }; |
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent status color handling detected
Several issues found with status color handling:
-
Hardcoded status colors found in files that should use the
statusColorutility:apps/web/lib/features/integrations/calendar/confirm-change-status.tsxuses hardcoded colors#dc2626,#30B366,#FBB650apps/web/app/[locale]/timesheet/[memberId]/page.tsxuses hardcoded colors#FBB650,#30B366
-
The
statusColorutility is not being imported in any files despite status values being used in multiple locations, particularly in:apps/web/lib/features/integrations/calendar/helper-calendar.ts- Various other files using status strings "Pending", "Approved", "Rejected"
Recommend:
- Replace hardcoded color values with the
statusColorutility - Import and use the
statusColorutility in all files handling these statuses
🔗 Analysis chain
Verify consistent status handling across the codebase
Let's ensure that all status-related code is using these utilities consistently.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for hardcoded status colors that should use the utility
echo "Checking for hardcoded status colors..."
rg -t tsx -t ts "bg-\[#(FBB650|30B366|dc2626)\]|rgba\(251, 182, 80|rgba\(48, 179, 102|rgba\(220, 38, 38" --glob '!**/types.ts'
# Search for status string literals to ensure they match our supported values
echo "Checking status string literals..."
rg -t tsx -t ts "status\s*[=:]\s*['\"](?:Pending|Approved|Rejected)['\"]"
Length of output: 432
Script:
#!/bin/bash
# Search for hardcoded status colors that should use the utility
echo "Checking for hardcoded status colors..."
rg "bg-\[#(FBB650|30B366|dc2626)\]|rgba\(251, 182, 80|rgba\(48, 179, 102|rgba\(220, 38, 38" --type-add 'tsx:*.tsx' --type-add 'ts:*.ts' -t tsx -t ts --glob '!**/types.ts'
# Search for status string literals to ensure they match our supported values
echo "Checking status string literals..."
rg "status\s*[=:]\s*['\"](?:Pending|Approved|Rejected)['\"]" --type-add 'tsx:*.tsx' --type-add 'ts:*.ts' -t tsx -t ts
# Find all files that import and use the statusColor utility
echo "Checking statusColor utility usage..."
rg "import.*statusColor.*from.*types" --type-add 'tsx:*.tsx' --type-add 'ts:*.ts' -t tsx -t ts
# Look for any other status-related string literals
echo "Checking other status values..."
rg "status['\"]?\s*[=:]\s*['\"][^'\"]+['\"]" --type-add 'tsx:*.tsx' --type-add 'ts:*.ts' -t tsx -t ts
Length of output: 7846
| className={clsxm( | ||
| "flex flex-row-reverse justify-end items-center w-full h-9 rounded-sm gap-x-2 hover:no-underline", | ||
| statusColor(status).text, | ||
| )} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract common styles to a reusable class.
The AccordionTrigger's className and style properties contain repeated styling logic that appears in multiple places. Consider extracting these into a reusable utility class or component prop.
-className={clsxm(
- "flex flex-row-reverse justify-end items-center w-full h-9 rounded-sm gap-x-2 hover:no-underline",
- statusColor(status).text,
-)}
+className={clsxm(
+ "accordion-trigger-base",
+ statusColor(status).text
+)}Then define the base styles in your CSS:
.accordion-trigger-base {
@apply flex flex-row-reverse justify-end items-center w-full h-9 rounded-sm gap-x-2 hover:no-underline;
}| <div className="flex items-center gap-x-2 w-40 "> | ||
| <div className="flex items-center justify-center h-8 w-8 rounded-lg bg-primary dark:bg-primary-light text-white shadow p-2"> | ||
| <span className="lowercase font-medium text-[10px]">ever</span> | ||
| </div> | ||
| <span className="capitalize font-bold !text-sm sm:text-base text-gray-800 dark:text-white leading-4 whitespace-nowrap"> | ||
| <span className="capitalize font-medium !text-sm sm:text-base text-gray-800 dark:text-white leading-4 whitespace-nowrap"> | ||
| {name} | ||
| </span> | ||
| <div style={{ display: 'none' }}>{description}</div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve semantic structure and accessibility.
The TaskDetails component has several areas for improvement:
- The hidden description should use proper ARIA attributes
- The project icon should be more semantic
- Text sizes should use design tokens for consistency
-<div className="flex items-center gap-x-2 w-40 ">
+<article className="flex items-center gap-x-2 w-40" role="listitem">
- <div className="flex items-center justify-center h-8 w-8 rounded-lg bg-primary dark:bg-primary-light text-white shadow p-2">
+ <span className="flex items-center justify-center h-8 w-8 rounded-lg bg-primary dark:bg-primary-light text-white shadow p-2" aria-hidden="true">
<span className="lowercase font-medium text-[10px]">ever</span>
- </div>
+ </span>
<span className="capitalize font-medium !text-sm sm:text-base text-gray-800 dark:text-white leading-4 whitespace-nowrap">
{name}
</span>
- <div style={{ display: 'none' }}>{description}</div>
+ <span className="sr-only" aria-label="Project description">{description}</span>
-</div>
+</article>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <div className="flex items-center gap-x-2 w-40 "> | |
| <div className="flex items-center justify-center h-8 w-8 rounded-lg bg-primary dark:bg-primary-light text-white shadow p-2"> | |
| <span className="lowercase font-medium text-[10px]">ever</span> | |
| </div> | |
| <span className="capitalize font-bold !text-sm sm:text-base text-gray-800 dark:text-white leading-4 whitespace-nowrap"> | |
| <span className="capitalize font-medium !text-sm sm:text-base text-gray-800 dark:text-white leading-4 whitespace-nowrap"> | |
| {name} | |
| </span> | |
| <div style={{ display: 'none' }}>{description}</div> | |
| <article className="flex items-center gap-x-2 w-40" role="listitem"> | |
| <span className="flex items-center justify-center h-8 w-8 rounded-lg bg-primary dark:bg-primary-light text-white shadow p-2" aria-hidden="true"> | |
| <span className="lowercase font-medium text-[10px]">ever</span> | |
| </span> | |
| <span className="capitalize font-medium !text-sm sm:text-base text-gray-800 dark:text-white leading-4 whitespace-nowrap"> | |
| {name} | |
| </span> | |
| <span className="sr-only" aria-label="Project description">{description}</span> | |
| </article> |
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Outside diff range and nitpick comments (17)
apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetView.tsx (1)
1-1: Consider adding prop type validation.While TypeScript provides type safety, consider adding runtime prop validation using
prop-typesor similar library, especially for optional props. This helps catch issues during development and provides better error messages.import { IDailyPlan } from '@/app/interfaces' +import PropTypes from 'prop-types' import { DataTableTimeSheet } from 'lib/features/integrations/calendar' export function TimesheetView({ data }: { data?: IDailyPlan[] }) {Add after the component:
TimesheetView.propTypes = { data: PropTypes.arrayOf(PropTypes.shape({ // Add IDailyPlan shape here })) }Also applies to: 4-4
apps/web/app/[locale]/timesheet/[memberId]/components/FilterWithStatus.tsx (2)
Line range hint
16-20: Consider making count values dynamic.The
buttonDataarray contains hard-coded count values which may not reflect the actual state of tasks.Consider passing these counts as props to make the component more dynamic:
type StatusCounts = { all: number; pending: number; approved: number; rejected: number; }; interface FilterWithStatusProps { activeStatus: FilterStatus; onToggle: (status: FilterStatus) => void; className?: HTMLAttributes<HTMLDivElement>; statusCounts: StatusCounts; }
Add ARIA attributes and improve accessibility
The component needs accessibility improvements:
- Add
role="group"andaria-label="Filter tasks by status"to the container div- Add
aria-pressed={activeStatus === label}to each Button- Add
aria-labelto include count information for each button (e.g., "All Tasks - 46 items")- Add
aria-current="true"to the active filter buttonExample locations requiring similar accessibility improvements:
apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetFilter.tsxapps/web/app/[locale]/timesheet/[memberId]/components/TimeSheetFilterPopover.tsxFor color contrast:
- Ensure text color
#7E7991and#71717Ameet WCAG contrast requirements against both light and dark backgrounds- Consider using CSS custom properties from the theme system as found in
apps/web/tailwind.config.jsinstead of hardcoded colors🔗 Analysis chain
Line range hint
1-37: Verify accessibility compliance.The component should be tested for:
- Color contrast ratios in both light and dark modes
- Keyboard navigation
- ARIA labels for status counts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any existing accessibility tests rg -l "accessibility|aria|@testing-library/jest-dom" --type ts --type tsx # Look for other filter components that might have accessibility patterns ast-grep --pattern 'className={$_}aria-label={$_}'Length of output: 156
Script:
#!/bin/bash # Search for any existing accessibility tests with correct file extensions rg -l "accessibility|aria|@testing-library/jest-dom" -g "*.ts" -g "*.tsx" # Look for ARIA attributes in the codebase rg -l "aria-" -g "*.tsx" -g "*.ts" # Look for other filter components fd -e tsx -e ts "Filter" --exec rg -l "Button|Select|Radio|Checkbox" # Search for color utility functions or theme configurations rg -l "theme|color|contrast" -g "tailwind.config.*"Length of output: 10468
apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetAction.tsx (1)
49-51: Implement the onClick functionality.The TODO comment indicates missing functionality that needs to be implemented.
Would you like me to help create a GitHub issue to track the implementation of the onClick handlers for each button type?
apps/web/app/[locale]/timesheet/[memberId]/components/TimeSheetFilterPopover.tsx (1)
Based on the code found, I can now provide a final response regarding the review comment:
Implement proper state management and handlers for the filter functionality.
The review comment is valid. Several implementation issues need to be addressed:
- All MultiSelect components use console.log or no-op handlers in onValueChange
- The "Clear" text in labels has very low opacity (primary/10) and lacks click handlers
- The Apply and Clear Filter buttons lack implementation
- No state management is implemented to track selected filter values
Suggested changes:
- Implement a filter state hook (e.g., useTimesheetFilter) to manage selected values for each filter type
- Add handlers for the Clear buttons in each filter section
- Implement the Apply and Clear Filter button functionality
- Connect the filter state to the parent component for filtering the actual data
🔗 Analysis chain
Line range hint
33-89: Implement filter state management and handlers.Several implementation concerns need to be addressed:
- Console.log statements in onValueChange handlers suggest incomplete implementation
- The "Clear" text appears with very low opacity (primary/10) but lacks click handlers
- No visible state management for filter values
Consider implementing proper state management:
- Add a custom hook or context for managing filter state
- Implement functional clear buttons for each filter
- Replace console.log with actual filter handlers
Would you like me to provide an example implementation of the filter state management?
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if there's any state management implementation in related files rg -l "useTimesheetFilters|TimesheetFilterContext|timesheetFilterReducer"Length of output: 141
Script:
#!/bin/bash # Search for any filter-related state management in TypeScript/JavaScript files rg -t ts -t js -t tsx -t jsx "useTimesheetFilters|TimesheetFilterContext|timesheetFilterReducer" # Look for the component implementation to understand the context better rg -t ts -t tsx "TimeSheetFilterPopover" -A 5 # Check for any existing filter-related hooks or contexts fd -e ts -e tsx -e js -e jsx | grep -i "filter" | xargs catLength of output: 105931
Script:
#!/bin/bash # Look for any filter-related state management in the component's directory fd -e tsx -e ts "TimeSheet.*" apps/web/app/\[locale\]/timesheet/ -x cat {} # Search for any filter-related hooks or contexts in the codebase rg -t ts -t tsx "useTimesheet|useFilter" apps/web/Length of output: 6309
apps/web/lib/features/integrations/calendar/helper-calendar.ts (1)
Line range hint
1-153: Consider separating types and mock data.For better maintainability and separation of concerns, consider:
- Moving type definitions to a separate
types.tsfile- Moving mock data to a dedicated
mocksdirectory- Using the new
headTimeSheettype for the mock data instead of the simplerTimeSheettypeThis separation would make it easier to maintain and test the new DataTableTimeSheet component mentioned in the PR objectives.
Also applies to: 154-238
apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetFilterDate.tsx (1)
211-213: Consider using design system tokens for colors.The modifierClassNames use hardcoded color values. Consider using design system tokens for better maintainability and consistency.
modifiersClassNames={{ - disabled: 'text-[#6989AA] cursor-not-allowed', - selected: '!rounded-full bg-primary text-white', - today: '!rounded-full bg-[#BCCAD9] text-white' + disabled: 'text-muted cursor-not-allowed', + selected: '!rounded-full bg-primary text-primary-foreground', + today: '!rounded-full bg-secondary text-secondary-foreground' }}apps/web/app/[locale]/timesheet/[memberId]/page.tsx (1)
32-38: Consider adding type safety for the user object.The implementation looks good, but consider adding type safety for the user object to prevent potential runtime errors.
- const username = user?.name || user?.firstName || user?.lastName || user?.username; + const username = user?.name || user?.firstName || user?.lastName || user?.username || 'Guest';apps/web/lib/features/integrations/calendar/table-time-sheet.tsx (3)
16-16: Consider optimizing importsThe imports seem well-organized, but some might be unused. For example,
TableCellandTableRoware imported separately while other table components are imported together. Consider consolidating table-related imports for consistency.Also applies to: 52-61
212-222: Remove commented codeThe commented table header code should be removed if it's no longer needed. If it's temporarily disabled for testing, add a TODO comment explaining why.
224-274: Enhance accordion accessibilityConsider adding ARIA labels to improve accessibility for screen readers, especially for the accordion items that show status groups.
- <Accordion type="single" collapsible> + <Accordion + type="single" + collapsible + aria-label="Timesheet entries grouped by status" + >apps/web/lib/features/user-profile-plans.tsx (6)
Line range hint
1-585: Consider splitting components into separate files for better maintainability.The file contains multiple complex components (
UserProfilePlans,AllPlans,PlanHeader,EmptyPlans) that could be moved to separate files under acomponentsdirectory. This would improve:
- Code organization and maintainability
- Component reusability
- Testing isolation
Suggested directory structure:
components/ UserProfilePlans/ index.tsx AllPlans.tsx PlanHeader.tsx EmptyPlans.tsx types.ts
Line range hint
147-166: Add missing dependencies to useEffect.The useEffect hook is missing dependencies that it uses within its body:
requirePlanhasPlanpathoutstandingPlanssetCurrentTabThis could lead to stale closures and unexpected behavior.
useEffect(() => { if (dailyPlanSuggestionModalDate != new Date().toISOString().split('T')[0] && path.split('/')[1] == 'profile') { if (estimatedTotalTime(outstandingPlans).totalTasks) { setCurrentTab('Outstanding'); } if (haveSeenDailyPlanSuggestionModal == new Date().toISOString().split('T')[0]) { if (!requirePlan || (requirePlan && hasPlan)) { window.localStorage.setItem( DAILY_PLAN_SUGGESTION_MODAL_DATE, new Date().toISOString().split('T')[0] ); } } } - }, []); + }, [dailyPlanSuggestionModalDate, path, outstandingPlans, requirePlan, hasPlan, setCurrentTab]);
447-448: Simplify className string concatenation using template literals.The current className concatenation can be simplified for better readability.
- className={`mb-6 flex ${planMode === 'Future Tasks' ? 'justify-start' : 'justify-around' - } items-center gap-5`} + className={`mb-6 flex items-center gap-5 ${ + planMode === 'Future Tasks' ? 'justify-start' : 'justify-around' + }`}
Line range hint
492-499: Optimize performance by memoizing the sumTimes function.The
sumTimesfunction is recreated on every render but its logic remains constant. Consider usinguseMemoto memoize it.- const sumTimes = useCallback((tasks: ITeamTask[], key: any) => { + const sumTimes = useMemo(() => (tasks: ITeamTask[], key: any) => { return ( tasks ?.map((task: any) => task[key]) .filter((time): time is number => typeof time === 'number') .reduce((acc, cur) => acc + cur, 0) ?? 0 ); - }, []); + }, []);
Line range hint
515-527: Add input validation for time editing.The time input field lacks proper validation and error handling:
- No maximum value constraint
- No error message display
- No handling of invalid inputs
<input min={0} + max={24} type="number" + step="0.5" + value={time} className={clsxm( 'outline-none p-0 bg-transparent border-b text-center max-w-[54px] text-xs font-medium', + time > 24 && 'border-red-500' )} onChange={(e) => { + const value = parseFloat(e.target.value); + if (value <= 24) { setTime(parseFloat(e.target.value)); + } }} /> + {time > 24 && ( + <span className="text-red-500 text-xs"> + {t('dailyPlan.INVALID_TIME')} + </span> + )}
Line range hint
312-312: Improve type safety by properly typing the profile prop.The
profileparameter is typed asany, which bypasses TypeScript's type checking. Consider creating a proper interface for the profile object.+ interface IUserProfile { + loadTaskStatsIObserverRef: React.RefObject<any>; + // Add other required profile properties + } - function AllPlans({ profile, currentTab = 'All Tasks' }: { profile: any; currentTab?: FilterTabs }) + function AllPlans({ profile, currentTab = 'All Tasks' }: { profile: IUserProfile; currentTab?: FilterTabs })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (12)
apps/web/app/[locale]/timesheet/[memberId]/components/FilterWithStatus.tsx(1 hunks)apps/web/app/[locale]/timesheet/[memberId]/components/FrequencySelect.tsx(1 hunks)apps/web/app/[locale]/timesheet/[memberId]/components/TimeSheetFilterPopover.tsx(2 hunks)apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetAction.tsx(1 hunks)apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetFilter.tsx(2 hunks)apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetFilterDate.tsx(4 hunks)apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetView.tsx(1 hunks)apps/web/app/[locale]/timesheet/[memberId]/components/index.tsx(1 hunks)apps/web/app/[locale]/timesheet/[memberId]/page.tsx(7 hunks)apps/web/lib/features/integrations/calendar/helper-calendar.ts(2 hunks)apps/web/lib/features/integrations/calendar/table-time-sheet.tsx(9 hunks)apps/web/lib/features/user-profile-plans.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/web/app/[locale]/timesheet/[memberId]/components/FrequencySelect.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetFilter.tsx
- apps/web/app/[locale]/timesheet/[memberId]/components/index.tsx
🔇 Additional comments (11)
apps/web/app/[locale]/timesheet/[memberId]/components/TimeSheetFilterPopover.tsx (1)
Line range hint 1-106: Verify integration with parent TimeSheet component.
The component seems to be part of a larger timesheet feature. We should verify:
- How filter state affects the parent TimeSheet component
- Integration with the existing frequency selector mentioned in the AI summary
#!/bin/bash
# Check how TimeSheetFilterPopover is integrated
rg -A 5 "TimeSheetFilterPopover" --type tsx
# Look for any existing filter state management
rg -A 10 "timesheet.*filter" --type tsxapps/web/lib/features/integrations/calendar/helper-calendar.ts (1)
158-159: Mock data consistency issue persists.
This issue was previously identified in the existing review comments. Please refer to the previous comment about mock data consistency where entries 3-8 contain identical dependency bump messages while only entries 1-2 have realistic task descriptions.
Also applies to: 167-167
apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetFilterDate.tsx (2)
11-11: LGTM: Clean state management implementation.
The addition of the isVisible state with useState follows React best practices for managing the visibility of the custom date range section.
Also applies to: 37-37
95-109: LGTM: Improved date range display formatting.
The button text formatting improvements with overflow handling and clear date range display logic enhance the user experience.
apps/web/app/[locale]/timesheet/[memberId]/page.tsx (5)
8-8: LGTM: Import statements are properly organized.
The new imports for authentication and greeting functionality are correctly added.
Also applies to: 17-17
75-76: Skip: Style improvements already suggested in previous review.
122-124: LGTM: Accessibility improvements implemented.
The search input now includes proper accessibility attributes as suggested.
145-145: LGTM: Proper HOC implementation.
The component is correctly wrapped with the authentication HOC and includes a proper display name for debugging.
Also applies to: 147-147
134-134: Verify the data prop type in TimesheetView component.
Ensure that the sortedPlans type matches the expected data prop type in the TimesheetView component.
✅ Verification successful
Based on the search results, I can now generate the final response since I have found the necessary information about the TimesheetView component and its data prop type:
Type verification for TimesheetView data prop is correct
The sortedPlans from useDailyPlan() hook is correctly typed and matches the expected data prop type in TimesheetView component:
- TimesheetView accepts
data?: IDailyPlan[]as prop sortedPlansfromuseDailyPlan()hook is of typeIDailyPlan[]- The
IDailyPlaninterface is properly defined with all required fields including tasks, date, workTimePlanned, etc.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for TimesheetView component definition to verify prop types
ast-grep --pattern 'interface $_ {
data: $_;
}'
Length of output: 51
Script:
#!/bin/bash
# Let's try a different approach to find TimesheetView component and its type definitions
rg "TimesheetView" -A 5 -B 5
# Also search for any type definitions related to TimesheetView
rg "type.*TimesheetView|interface.*TimesheetView" -A 3
Length of output: 6073
Script:
#!/bin/bash
# Let's check the IDailyPlan interface definition and useDailyPlan hook to verify type compatibility
rg "interface IDailyPlan" -A 10
rg "useDailyPlan" -A 5 -B 5
Length of output: 64222
apps/web/lib/features/integrations/calendar/table-time-sheet.tsx (2)
87-93: LGTM: Improved responsive design
The use of max-w-[640px] with w-full ensures better responsiveness while maintaining a reasonable maximum width.
200-204: 🛠️ Refactor suggestion
Enhance status grouping flexibility
The current implementation hardcodes status values, making it less maintainable and inflexible to status changes.
- const groupedRows = {
- Pending: table.getRowModel().rows.filter(row => row.original.status === "Pending"),
- Approved: table.getRowModel().rows.filter(row => row.original.status === "Approved"),
- Rejected: table.getRowModel().rows.filter(row => row.original.status === "Rejected")
- };
+ const groupedRows = table.getRowModel().rows.reduce((acc, row) => {
+ const status = row.original.status;
+ if (!acc[status]) acc[status] = [];
+ acc[status].push(row);
+ return acc;
+ }, {} as Record<string, typeof table.getRowModel().rows>);Likely invalid or redundant comment.
| export function TimesheetView({ data }: { data?: IDailyPlan[] }) { | ||
| return ( | ||
| <div className='grow h-full w-full bg-[#FFFFFF]'> | ||
| <DataTableTimeSheet /> | ||
| <DataTableTimeSheet data={data} /> | ||
| </div> | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider handling empty or loading states.
The component doesn't handle cases where data is undefined or empty. Consider adding appropriate loading states or empty state messages for better user experience.
export function TimesheetView({ data }: { data?: IDailyPlan[] }) {
return (
<div className='grow h-full w-full bg-[#FFFFFF]'>
- <DataTableTimeSheet data={data} />
+ {!data ? (
+ <div className="flex items-center justify-center h-full">
+ <p>Loading timesheet data...</p>
+ </div>
+ ) : data.length === 0 ? (
+ <div className="flex items-center justify-center h-full">
+ <p>No timesheet entries found</p>
+ </div>
+ ) : (
+ <DataTableTimeSheet data={data} />
+ )}
</div>
)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function TimesheetView({ data }: { data?: IDailyPlan[] }) { | |
| return ( | |
| <div className='grow h-full w-full bg-[#FFFFFF]'> | |
| <DataTableTimeSheet /> | |
| <DataTableTimeSheet data={data} /> | |
| </div> | |
| ) | |
| export function TimesheetView({ data }: { data?: IDailyPlan[] }) { | |
| return ( | |
| <div className='grow h-full w-full bg-[#FFFFFF]'> | |
| {!data ? ( | |
| <div className="flex items-center justify-center h-full"> | |
| <p>Loading timesheet data...</p> | |
| </div> | |
| ) : data.length === 0 ? ( | |
| <div className="flex items-center justify-center h-full"> | |
| <p>No timesheet entries found</p> | |
| </div> | |
| ) : ( | |
| <DataTableTimeSheet data={data} /> | |
| )} | |
| </div> | |
| ) |
| return ( | ||
| <div className='grow h-full w-full bg-[#FFFFFF]'> | ||
| <DataTableTimeSheet /> | ||
| <DataTableTimeSheet data={data} /> |
There was a problem hiding this comment.
💡 Codebase verification
Based on the code inspection, I can now provide the final response:
Add null check for data prop in DataTableTimeSheet component
The component directly uses the data prop in the table configuration without any null checks or default values. This could lead to runtime errors when data is undefined. The component should:
- Initialize
dataSourceTimeSheetwith a default empty array when data is undefined - Add type safety checks before data operations
🔗 Analysis chain
Verify DataTableTimeSheet component's handling of undefined data.
Ensure that the DataTableTimeSheet component properly handles the case when data is undefined, as the type allows for this possibility.
Let me run another script to check how the data is processed before being used in the table.
Let me try one more search to find any data transformation or null checks.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how DataTableTimeSheet handles undefined data prop
# Look for null checks or default values in the implementation
ast-grep --pattern 'function DataTableTimeSheet({ data }: { data?: $_[] }) {
$$$
}'
Length of output: 17661
Script:
#!/bin/bash
# Check for data transformation or default value assignment before table usage
ast-grep --pattern 'const dataSourceTimeSheet = $_'
Length of output: 53
Script:
#!/bin/bash
# Look for data usage and potential transformations in the component
rg -A 5 "data: " "apps/web/lib/features/integrations/calendar/table-time-sheet.tsx"
Length of output: 321
| @@ -26,11 +26,11 @@ export function FilterWithStatus({ | |||
| key={index} | |||
| className={clsxm( | |||
| 'group flex items-center justify-start h-[2.2rem] rounded-xl border w-full', | |||
| 'dark:bg-dark--theme-light dark:border-gray-700 bg-[#e2e8f0aa] text[#71717A] w-[80px]', | |||
| 'dark:bg-dark--theme-light dark:border-gray-700 bg-transparent text[#71717A] w-[80px]', | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve text color consistency and dark mode support.
The text colors are defined inconsistently:
- Button has
text[#71717A](with incorrect syntax) - Inner span has
text-[#7E7991] - No explicit dark mode text colors
Consider:
- Using consistent color tokens
- Adding proper dark mode text colors
- Fixing the text color class syntax
-'dark:bg-dark--theme-light dark:border-gray-700 bg-transparent text[#71717A] w-[80px]',
+'dark:bg-dark--theme-light dark:border-gray-700 bg-transparent text-[#71717A] dark:text-gray-300',
-<span className={clsxm('font-medium ml-1 text-[#7E7991]', `${activeStatus === label ? "text-primary" : ""}`)}>{label}</span>
+<span className={clsxm(
+ 'font-medium ml-1',
+ activeStatus === label ? "text-primary dark:text-primary" : "text-[#71717A] dark:text-gray-300"
+)}>{label}</span>Also applies to: 33-33
Fix conflicting width classes.
The button has conflicting width classes: w-full and w-[80px]. This can lead to inconsistent rendering across different browsers.
-'dark:bg-dark--theme-light dark:border-gray-700 bg-transparent text[#71717A] w-[80px]',
+'dark:bg-dark--theme-light dark:border-gray-700 bg-transparent text[#71717A]',Committable suggestion skipped: line range outside the PR's diff.
| export const getTimesheetButtons = (status: StatusType) => { | ||
| const buttonsConfig: Record<StatusType, { icon: JSX.Element; title: string }[]> = { | ||
| Pending: [ | ||
| { icon: <FaClipboardCheck className="!text-[#2932417c] rounded" />, title: "Approve Selected" }, | ||
| { icon: <IoClose className="!bg-[#2932417c] rounded" />, title: "Reject Selected" }, | ||
| { icon: <RiDeleteBin6Fill className="!text-[#2932417c] rounded" />, title: "Delete Selected" } | ||
| ], | ||
| Approved: [ | ||
| { icon: <IoClose className="!bg-[#2932417c] rounded" />, title: "Reject Selected" }, | ||
| { icon: <RiDeleteBin6Fill className="!text-[#2932417c] rounded" />, title: "Delete Selected" } | ||
| ], | ||
| Rejected: [ | ||
| { icon: <FaClipboardCheck className="!text-[#2932417c] rounded" />, title: "Approve Selected" }, | ||
| { icon: <RiDeleteBin6Fill className="!text-[#2932417c] rounded" />, title: "Delete Selected" } | ||
| ] | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract button configurations to a separate constant.
The button configurations should be extracted for better maintainability and reusability. Also, consider using a design system for consistent styling.
Create a separate constant:
export const TIMESHEET_BUTTON_CONFIGS = {
[StatusType.Pending]: [
{
id: 'approve',
icon: <FaClipboardCheck className="text-gray-600" />,
title: "Approve Selected"
},
// ... other configurations
],
// ... other status configurations
} as const;Then update the function:
-const buttonsConfig: Record<StatusType, { icon: JSX.Element; title: string }[]> = {
+const buttonsConfig = TIMESHEET_BUTTON_CONFIGS;| } | ||
|
|
||
|
|
||
| export type StatusType = "Pending" | "Approved" | "Rejected"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using an enum for StatusType.
Using an enum would provide better type safety and centralize the status values.
-export type StatusType = "Pending" | "Approved" | "Rejected";
+export enum StatusType {
+ Pending = "Pending",
+ Approved = "Approved",
+ Rejected = "Rejected"
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export type StatusType = "Pending" | "Approved" | "Rejected"; | |
| export enum StatusType { | |
| Pending = "Pending", | |
| Approved = "Approved", | |
| Rejected = "Rejected" | |
| } |
| onClick={() => { | ||
| label === 'Custom Date Range' && setIsVisible((prev) => !prev) | ||
| handlePresetClick(label) | ||
| }}> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider separating concerns in the onClick handler.
The current implementation combines visibility toggle and preset selection in a single line. Consider separating these for better maintainability and to prevent potential re-renders.
- onClick={() => {
- label === 'Custom Date Range' && setIsVisible((prev) => !prev)
- handlePresetClick(label)
- }}
+ onClick={() => {
+ if (label === 'Custom Date Range') {
+ setIsVisible((prev) => !prev);
+ return;
+ }
+ handlePresetClick(label);
+ }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| onClick={() => { | |
| label === 'Custom Date Range' && setIsVisible((prev) => !prev) | |
| handlePresetClick(label) | |
| }}> | |
| onClick={() => { | |
| if (label === 'Custom Date Range') { | |
| setIsVisible((prev) => !prev); | |
| return; | |
| } | |
| handlePresetClick(label); | |
| }}> |
| {isVisible && ( | ||
| <div className="flex flex-col p-2 gap-2 translate-x-0 justify-between"> | ||
| <div className="flex flex-col gap-2"> | ||
| <DatePickerFilter | ||
| label="From" | ||
| date={dateRange.from} | ||
| setDate={handleFromChange} | ||
| /> | ||
| <DatePickerFilter | ||
| label="To" | ||
| date={dateRange.to} | ||
| setDate={handleToChange} | ||
| minDate={dateRange.from} | ||
| /> | ||
| </div> | ||
| <div className="flex w-full justify-end items-end"> | ||
| <Button variant={'outline'} className="h-4 border-none text-primary hover:bg-transparent hover:text-primary hover:underline">Cancel</Button> | ||
| <Button variant={'outline'} className="h-4 border-none text-primary hover:bg-transparent hover:text-primary hover:underline">Apply</Button> | ||
| </div> | ||
| </div> | ||
| ) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add handlers for Cancel and Apply buttons.
The Cancel and Apply buttons are currently non-functional. They need onClick handlers to manage the date range selection.
- <Button variant={'outline'} className="h-4 border-none text-primary hover:bg-transparent hover:text-primary hover:underline">Cancel</Button>
- <Button variant={'outline'} className="h-4 border-none text-primary hover:bg-transparent hover:text-primary hover:underline">Apply</Button>
+ <Button
+ variant={'outline'}
+ className="h-4 border-none text-primary hover:bg-transparent hover:text-primary hover:underline"
+ onClick={() => {
+ setIsVisible(false);
+ setDateRange(initialRange ?? { from: new Date(), to: new Date() });
+ }}
+ >
+ Cancel
+ </Button>
+ <Button
+ variant={'outline'}
+ className="h-4 border-none text-primary hover:bg-transparent hover:text-primary hover:underline"
+ onClick={() => {
+ setIsVisible(false);
+ onChange?.(dateRange);
+ }}
+ >
+ Apply
+ </Button>Consider these improvements:
- Extract the common button styles into a shared class or variant
- Add translations for "Cancel" and "Apply" buttons if the app supports internationalization
Committable suggestion skipped: line range outside the PR's diff.
| export function DataTableTimeSheet({ data }: { data?: IDailyPlan[] }) { | ||
| const [sorting, setSorting] = React.useState<SortingState>([]) |
There was a problem hiding this comment.
Fix unused data prop
The component accepts a data prop but uses hardcoded dataSourceTimeSheet. This makes the component inflexible and ignores the provided data.
- data: dataSourceTimeSheet,
+ data: data ?? dataSourceTimeSheet,Also applies to: 182-183
| <DropdownMenuItem textValue={'Oui'} className="cursor-pointer"> | ||
| <div className="flex items-center gap-3"> | ||
| <div className="h-1 w-1 rounded-full bg-black dark:bg-white"></div> | ||
| <span>{status.label}</span> | ||
| <span>Oui</span> | ||
| </div> | ||
| </DropdownMenuItem> | ||
| ))} | ||
| </DropdownMenuSubContent> | ||
| </DropdownMenuPortal> | ||
| </DropdownMenuSub> | ||
| <DropdownMenuItem textValue={'No'} className="cursor-pointer"> | ||
| <div className="flex items-center gap-3"> | ||
| <span>No</span> | ||
| </div> | ||
| </DropdownMenuItem> |
There was a problem hiding this comment.
Improve internationalization and consistency
The component mixes French ("Oui") and English ("No") text. Consider using a consistent language and implementing proper internationalization.
- <DropdownMenuItem textValue={'Oui'} className="cursor-pointer">
+ <DropdownMenuItem textValue={'Yes'} className="cursor-pointer">
<div className="flex items-center gap-3">
- <span>Oui</span>
+ <span>Yes</span>
</div>
</DropdownMenuItem>Committable suggestion skipped: line range outside the PR's diff.
| </TableHeader> | ||
| <TableBody> | ||
| {table.getRowModel().rows?.length ? ( | ||
| table.getRowModel().rows.map((row) => ( | ||
| <TableRow | ||
| className="cursor-pointer border dark:border-gray-700 font-normal" | ||
| key={row.id} | ||
| data-state={row.getIsSelected() && "selected"} | ||
| > | ||
| {row.getVisibleCells().map((cell) => ( | ||
| <TableCell aria-checked key={cell.id}> | ||
| {flexRender( | ||
|
|
||
| cell.column.columnDef.cell, | ||
| cell.getContext() | ||
| )} | ||
| </TableCell> | ||
| ))} | ||
| </TableRow> | ||
| )) | ||
| ) : ( | ||
| <TableRow> | ||
| <TableCell | ||
| colSpan={columns.length} | ||
| className="h-24 text-center"> | ||
| No results. | ||
| </TableCell> | ||
| </TableRow> | ||
| )} | ||
| </TableBody> | ||
| </Table> | ||
| return ( | ||
| <div className="w-full"> | ||
| <div className="rounded-md"> | ||
| <Table className="border rounded-md"> | ||
| {/* <TableHeader> | ||
| {table.getHeaderGroups().map((headerGroup) => ( | ||
| <TableRow key={headerGroup.id}> | ||
| {headerGroup.headers.map((header) => ( | ||
| <TableHead key={header.id}> | ||
| {header.isPlaceholder ? null : flexRender(header.column.````columnDef.header, header.getContext())} | ||
| </TableHead> | ||
| ))} | ||
| </TableRow> | ||
| ))} | ||
| </TableHeader> */} | ||
| <TableBody className="w-full rounded-md"> | ||
| <Accordion type="single" collapsible> | ||
| {Object.entries(groupedRows).map(([status, rows]) => ( | ||
| <AccordionItem key={status} value={status} className="p-1 rounded"> | ||
| <AccordionTrigger | ||
| style={{ backgroundColor: statusColor(status).bgOpacity }} | ||
| type="button" | ||
| className={clsxm( | ||
| "flex flex-row-reverse justify-end items-center w-full h-9 rounded-sm gap-x-2 hover:no-underline px-2", | ||
| statusColor(status).text, | ||
| )} | ||
| > | ||
| <div className="flex items-center space-x-1 justify-between w-full"> | ||
| <div className="flex items-center space-x-1"> | ||
| <div className={clsxm("p-2 rounded", statusColor(status).bg)}></div> | ||
| <div className="flex items-center gap-x-1"> | ||
| <span className="text-base font-normal uppercase text-gray-400"> | ||
| {status} | ||
| </span> | ||
| <span className="text-gray-400 text-[14px]">({rows.length})</span> | ||
| </div> | ||
| <Badge variant={'outline'} className="flex items-center gap-x-2 rounded-md bg-[#E4E4E7]"> | ||
| <span className="text-[#71717A]">Total</span> | ||
| <span>24:30h</span> | ||
| </Badge> | ||
| </div> | ||
| <div className="flex items-center gap-2 p-x-1"> | ||
| {getTimesheetButtons(status as StatusType)} | ||
| </div> | ||
| </div> | ||
| </AccordionTrigger> | ||
| <AccordionContent className="flex flex-col w-full"> | ||
| {rows.length ? ( | ||
| rows.map((row) => ( | ||
| <TableRow style={{ backgroundColor: statusColor(status).bgOpacity }} | ||
| key={row.id} className="min-w-full w-auto"> | ||
| {row.getVisibleCells().map((cell) => ( | ||
| <TableCell key={cell.id} className="w-full"> | ||
| {flexRender(cell.column.columnDef.cell, cell.getContext())} | ||
| </TableCell> | ||
| ))} | ||
| </TableRow> | ||
| )) | ||
| ) : ( | ||
| <TableRow> | ||
| <TableCell colSpan={columns.length} className="text-center">No results.</TableCell> | ||
| </TableRow> | ||
| )} | ||
| </AccordionContent> | ||
| </AccordionItem> | ||
| ))} | ||
| </Accordion> | ||
| </TableBody> | ||
| </Table> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize row filtering performance
The current implementation filters rows multiple times when creating grouped rows and rendering. Consider memoizing the grouped rows to prevent unnecessary recalculations.
+ const memoizedGroupedRows = React.useMemo(() => ({
+ // ... grouping logic
+ }), [table.getRowModel().rows]);Committable suggestion skipped: line range outside the PR's diff.
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
TimeSheetFilterPopoverfor enhanced filtering options in the timesheet interface.TimesheetActioncomponent for rendering action buttons based on timesheet status.Bug Fixes
DataTablecomponent for better header cell display.Improvements
MultiSelectcomponents in the filter popover for consistent functionality.TaskDetailscomponent layout and styling for improved user experience.MultiSelectcomponent based on status.TimesheetFilterDatecomponent.Style
FilterWithStatuscomponent for a more polished appearance.