[Feature] Add time limits report filters #3392
Conversation
WalkthroughThe pull request introduces significant modifications across several components related to the Weekly Limit Report. Key changes include the renaming of properties in data types, the enhancement of dropdown functionality to support multi-selection, and the introduction of new components for improved reporting capabilities. Additionally, the grouping logic for report data has been updated to accommodate dynamic filtering and grouping options, allowing for more flexible data presentation. Changes
Assessment against linked issues
Possibly related issues
Possibly related PRs
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
warning "@nx/eslint-plugin-nx > @nx/[email protected]" has unmet peer dependency "nx@>= 14.1 <= 16". 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: 3
🧹 Outside diff range and nitpick comments (7)
apps/web/app/[locale]/reports/weekly-limit/components/group-by-select.tsx (2)
89-92: Ensure disabled options are visually distinctiveWhen an option is disabled to prevent users from deselecting the last remaining option, make sure it's visually apparent. This enhances usability by signaling to users why the option cannot be interacted with.
Consider adding styles to the disabled options:
<Listbox.Option disabled={selected.includes(option) && selected.length == 1} + className={({ active, disabled }) => + `relative cursor-default select-none py-2 pl-10 pr-4 ${ + active ? 'bg-primary/10 text-primary' : 'text-gray-900' + } ${disabled ? 'opacity-50 cursor-not-allowed' : ''}` + } value={option} >
71-72: Simplify translation key generationThe casting to
DottedLanguageObjectStringPathsmay be unnecessary. You can streamline the code by ensuring the translation keys match the expected format.Apply this diff to simplify:
{t( - `common.${(option == 'date' ? 'date' : option).toUpperCase()}` as DottedLanguageObjectStringPaths + `common.${option.toUpperCase()}` )}apps/web/app/[locale]/reports/weekly-limit/components/time-report-table.tsx (1)
122-150: Optimize 'groupDataByEmployee' function for performanceConsider optimizing the
groupDataByEmployeefunction. Using a reducer or more efficient data structures can improve performance, especially with large datasets.Example using a reducer:
export function groupDataByEmployee(data: ITimeLimitReport[]): ITimeLimitReportByEmployee[] { - const grouped = new Map<string, ITimeLimitReportByEmployee>(); - data.forEach((day) => { - // existing logic - }); - return Array.from(grouped.values()); + const grouped = data.reduce((acc, day) => { + day.employees.forEach((emp) => { + const empId = emp.employee.id; + if (!acc[empId]) { + acc[empId] = { + employee: emp.employee, + reports: [], + }; + } + acc[empId].reports.push({ + date: day.date, + duration: emp.duration, + durationPercentage: emp.durationPercentage, + limit: emp.limit, + }); + }); + return acc; + }, {} as { [key: string]: ITimeLimitReportByEmployee }); + return Object.values(grouped); }apps/web/app/[locale]/reports/weekly-limit/page.tsx (2)
67-68: Consolidate 'duration' and 'displayMode' calculationsBoth
durationanddisplayModeare derived fromgroupByin a similar way. Consider consolidating them to simplify state management.Apply this diff:
-const duration = useMemo(() => groupBy.find((el) => el == 'date' || el == 'week') ?? 'date', [groupBy]); -const displayMode = (groupBy.find((el) => el === 'date' || el === 'week') ?? 'date') as 'week' | 'date'; +const [displayMode] = useMemo(() => { + const mode = groupBy.find((el) => el === 'date' || el === 'week') ?? 'date'; + return [mode as 'week' | 'date']; +}, [groupBy]); +const duration = displayMode;
188-189: Offer assistance to improve paginationThe TODO comment suggests improving pagination for filtered data. I can help implement a solution to ensure pagination works correctly with the current filters.
Would you like assistance in updating the pagination logic to align with the filtered and grouped data?
apps/web/app/[locale]/reports/weekly-limit/components/data-table.tsx (2)
78-80: Consider adding aria-label for better accessibilityWhile the column definition changes look good, consider adding an aria-label to improve accessibility when the header text changes dynamically.
- header: () => <div className="">{indexTitle}</div>, + header: () => <div className="" aria-label={`Column header for ${indexTitle}`}>{indexTitle}</div>,
149-168: Consider accessibility impact when headers are hiddenWhile the conditional header rendering works well, hiding table headers might affect screen reader users. Consider adding appropriate ARIA attributes to maintain table semantics when headers are hidden.
- {showHeader && ( + {showHeader ? ( <TableHeader> {table.getHeaderGroups().map((headerGroup) => ( <TableRow key={headerGroup.id}> {headerGroup.headers.map((header) => { return ( <TableHead className=" capitalize" key={header.id}> {header.isPlaceholder ? null : flexRender( header.column.columnDef.header, header.getContext() )} </TableHead> ); })} </TableRow> ))} </TableHeader> + ) : ( + <thead className="sr-only" aria-hidden="false"> + {table.getHeaderGroups().map((headerGroup) => ( + <tr key={headerGroup.id}> + {headerGroup.headers.map((header) => ( + <th key={header.id}> + {header.isPlaceholder + ? null + : flexRender( + header.column.columnDef.header, + header.getContext() + )} + </th> + ))} + </tr> + ))} + </thead> )}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
apps/web/app/[locale]/reports/weekly-limit/components/data-table.tsx(5 hunks)apps/web/app/[locale]/reports/weekly-limit/components/group-by-select.tsx(1 hunks)apps/web/app/[locale]/reports/weekly-limit/components/time-report-table.tsx(3 hunks)apps/web/app/[locale]/reports/weekly-limit/page.tsx(7 hunks)apps/web/app/hooks/features/usePagination.ts(1 hunks)apps/web/app/interfaces/ITimeLimits.ts(1 hunks)
🔇 Additional comments (7)
apps/web/app/[locale]/reports/weekly-limit/components/time-report-table.tsx (1)
98-100: Review date formatting for week display
In the indexValue, when displayMode is 'week', the date range is formatted using item.date and moment(item.date).endOf('week').format('YYYY-MM-DD'). Ensure this correctly represents the week's start and end dates according to your locale settings.
apps/web/app/[locale]/reports/weekly-limit/page.tsx (1)
154-158: Review necessity of additional filtering on 'currentItems'
The filter applied on currentItems for displayMode === 'week' may be redundant if the data fetched already respects the duration. Verify if this filtering is necessary.
apps/web/app/interfaces/ITimeLimits.ts (1)
26-34: Approved: New interface 'ITimeLimitReportByEmployee'
The ITimeLimitReportByEmployee interface is well-defined and integrates seamlessly with existing structures.
apps/web/app/hooks/features/usePagination.ts (1)
24-25: Verify the necessity of exposing 'setItemOffset'
Adding setItemOffset to the returned object exposes internal state. Ensure this is required and won't introduce unintended side effects. If necessary, document its intended usage.
apps/web/app/[locale]/reports/weekly-limit/components/data-table.tsx (3)
44-49: LGTM! Props changes enhance component flexibility
The new props indexTitle and showHeader are well-designed additions that support the filtering and grouping requirements. The types are properly defined, and default values are handled correctly.
126-126: LGTM! Table configuration is properly set up
25-25: Verify type changes across dependent components
The rename from member to indexValue makes the type more flexible for different grouping options, which aligns with the filtering requirements. However, we should ensure all consumers of this type are updated.
✅ Verification successful
Type change from member to indexValue is safe
The rename from member to indexValue in WeeklyLimitTableDataType is safe. The search results show that while member is used in various contexts (MembersSelect component, groupBy options), there are no direct references to the old property name member within WeeklyLimitTableDataType. The change makes the type more generic to support different grouping options while maintaining backward compatibility.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining usage of the old property name 'member' in WeeklyLimitTableDataType
rg -l "WeeklyLimitTableDataType.*member" --type typescript
Length of output: 95
Script:
#!/bin/bash
# Search for any remaining usage of 'member' property in TypeScript/TSX files
rg -l "member.*WeeklyLimitTableDataType|WeeklyLimitTableDataType.*member" "*.ts" "*.tsx"
# Also search for any direct usage of 'member' in weekly-limit related files
fd -e ts -e tsx weekly-limit --exec rg -l "member"
Length of output: 232
Script:
#!/bin/bash
# Search for WeeklyLimitTableDataType usage
rg "WeeklyLimitTableDataType" --type=tsx --type=ts
# Search for member property in weekly-limit directory
rg "member" apps/web/app/\[locale\]/reports/weekly-limit/
Length of output: 2446
apps/web/app/[locale]/reports/weekly-limit/components/group-by-select.tsx
Show resolved
Hide resolved
apps/web/app/[locale]/reports/weekly-limit/components/time-report-table.tsx
Outdated
Show resolved
Hide resolved
apps/web/app/[locale]/reports/weekly-limit/components/time-report-table.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]/reports/weekly-limit/components/time-report-table.tsx (2)
58-111: Consider performance optimizations and code reusability.While the implementation is functionally correct, consider these improvements:
- Extract date formatting logic to a utility function for reusability
- Memoize the data mapping function to optimize performance with large datasets
Example implementation:
+ const formatWeekRange = (date: string) => + `${date} - ${moment(date).endOf('week').format('YYYY-MM-DD')}`; + const useMemoizedReportData = (reports: any[], displayMode: string) => + useMemo(() => reports?.map((item) => { + const limit = item.limit || organizationLimits[displayMode] || DEFAULT_WORK_HOURS_PER_DAY; + const percentageUsed = (item.duration / limit) * 100; + const remaining = limit - item.duration; + return { + indexValue: displayMode === 'week' ? formatWeekRange(item.date) : item.date, + limit, + percentageUsed, + timeSpent: item.duration, + remaining + }; + }), [reports, displayMode]);
113-150: Add error handling for edge cases.The grouping logic is efficient, but consider adding error handling for:
- Empty data array
- Missing employee information
- Invalid date formats
Example implementation:
export function groupDataByEmployee(data: ITimeLimitReport[]): ITimeLimitReportByEmployee[] { + if (!Array.isArray(data) || data.length === 0) { + return []; + } const grouped = new Map<string, ITimeLimitReportByEmployee>(); data.forEach((day) => { + if (!day?.date || !Array.isArray(day.employees)) { + console.warn('Invalid data format encountered:', day); + return; + } const date = day.date; day.employees.forEach((emp) => { + if (!emp?.employee?.id) { + console.warn('Invalid employee data:', emp); + return; + } // ... rest of the implementation }); });apps/web/app/[locale]/reports/weekly-limit/page.tsx (2)
87-94: Simplify duration mapping.The duration mapping can be simplified to avoid the ternary operator.
-duration: duration == 'date' ? 'day' : duration, +const durationMap = { date: 'day', week: 'week' }; +duration: durationMap[duration] || 'day',
188-189: Address the TODO comment for pagination improvement.The comment indicates that pagination needs improvement for filtered data. This is important for proper data handling.
Would you like me to help implement the pagination logic for filtered data or create a GitHub issue to track this task?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
apps/web/app/[locale]/reports/weekly-limit/components/time-report-table.tsx(3 hunks)apps/web/app/[locale]/reports/weekly-limit/page.tsx(7 hunks)
🧰 Additional context used
📓 Learnings (1)
apps/web/app/[locale]/reports/weekly-limit/page.tsx (1)
Learnt from: CREDO23
PR: ever-co/ever-teams#3392
File: apps/web/app/[locale]/reports/weekly-limit/components/group-by-select.tsx:42-51
Timestamp: 2024-12-05T09:04:08.049Z
Learning: In the `GroupBySelect` component (`apps/web/app/[locale]/reports/weekly-limit/components/group-by-select.tsx`), only one option can be selected at a time; two options cannot be selected simultaneously.
🔇 Additional comments (2)
apps/web/app/[locale]/reports/weekly-limit/components/time-report-table.tsx (2)
1-11: LGTM! Well-structured interfaces with clear type definitions.
The props interface is well-defined with appropriate types, and the displayMode values are consistently using lowercase.
Line range hint 26-56: LGTM! Clean implementation with proper calculations.
The component effectively:
- Handles time limit calculations
- Maps employee data correctly
- Uses proper TypeScript types
Description
Fixes #3163
Fixes #3164
Type of Change
Checklist
Current screenshots
Loom
Summary by CodeRabbit
Release Notes
New Features
DataTableWeeklyLimitswith dynamic header titles and optional header display.GroupBySelectto a multi-select dropdown with improved selection rules.TimeReportTableByMemberfor detailed employee-specific reporting.WeeklyLimitReportfor better data organization.usePaginationhook to allow external modification of item offset.Bug Fixes
Documentation