[Feat]: Add Timelog FilterOptions #3333
Conversation
WalkthroughThe pull request introduces significant updates to the Changes
Possibly related PRs
Suggested reviewers
Poem
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/lib/components/custom-select/multi-select.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. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Innocent-Akim
added
feature
WEB
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (16)
apps/web/app/stores/index.ts (1)
30-30: LGTM! Consider grouping with related exports.The export statement is correctly implemented. However, for better organization, consider grouping it with other time-related exports (like 'timer') or task-related exports since timesheets are closely related to these domains.
export * from './timer'; +export * from './time-logs'; export * from './invitations'; export * from './team-tasks'; - -export * from './languages'; -export * from './timezones'; -export * from './collaborative'; - -export * from './task-status'; -export * from './task-version'; -export * from './task-priorities'; -export * from './task-sizes'; -export * from './task-labels'; -export * from './issue-type'; -export * from './task-related-issue-type'; -export * from './daily-plan'; - -export * from './roles'; -export * from './role-permissions'; -export * from './request-to-join'; -export * from './public'; - -export * from './integration-tenant'; -export * from './integration-github'; -export * from './integration-types'; -export * from './integration'; -export * from './time-logs'apps/web/app/hooks/features/useTimelogFilterOptions.ts (2)
15-26: Improve return object organization and fix naming
- Remove the extra blank line at line 15
- Consider grouping related values with their setters for better maintainability
Apply this organization to improve readability:
return { - statuState, - employee, - project, - task, - setEmployeeState, - setProjectState, - setTaskState, - setStatuState + employee, + setEmployeeState, + project, + setProjectState, + status: statusState, + setStatusState, + task, + setTaskState };🧰 Tools
🪛 GitHub Check: Cspell
[warning] 17-17:
Unknown word (statu)
[warning] 24-24:
Unknown word (Statu)
5-26: Consider enhancing the hook with additional featuresTo improve the robustness and performance of this hook, consider:
- Memoizing the return object using
useMemoto prevent unnecessary re-renders- Adding validation for initial states
- Implementing error handling for invalid state values
Example implementation:
export function useTimelogFilterOptions() { const [employeeState, setEmployeeState] = useAtom(timesheetFilterEmployeeState); // ... other state declarations ... return useMemo(() => ({ employee, setEmployeeState, // ... other values and setters ... }), [employee, project, statusState, task]); }🧰 Tools
🪛 GitHub Check: Cspell
[warning] 8-8:
Unknown word (statu)
[warning] 8-8:
Unknown word (Statu)
[warning] 17-17:
Unknown word (statu)
[warning] 24-24:
Unknown word (Statu)apps/web/app/interfaces/timer/ITimerLog.ts (1)
14-14: Consider using enums or string literals for better type safetyWhile adding
issueTypeandtaskStatusas nullable strings provides flexibility, using more specific types would improve type safety and prevent potential inconsistencies.Consider refactoring to use string literals or enums:
type TaskIssueType = 'bug' | 'feature' | 'improvement' | null; type TaskStatus = 'todo' | 'in_progress' | 'done' | null; interface Task { // ... other properties issueType: TaskIssueType; taskStatus: TaskStatus; }Also applies to: 16-16
apps/web/app/hooks/index.ts (1)
43-43: Consider reorganizing the export location.While the export follows the correct naming convention, consider:
- Moving it to maintain alphabetical ordering with other feature hooks
- Grouping it with other time-related hooks like
useTimerfor better organizationApply this diff to maintain consistent ordering:
export * from './features/useTeamTasks'; export * from './features/useTimer'; +export * from './features/useTimelogFilterOptions'; export * from './features/useUser'; export * from './features/useUserProfilePage'; -export * from './features/useTimelogFilterOptions'; export * from './useCollaborative';apps/web/app/hooks/features/useTimesheet.ts (2)
63-64: Consider handling empty arrays explicitlyWhile filtering undefined values is good, consider whether empty arrays should be treated differently from undefined values in the API call. This might affect how the backend filters the results.
-employeeIds: employee?.map((member) => member.employee.id).filter((id) => id !== undefined), -projectIds: project?.map((project) => project.id).filter((id) => id !== undefined) +employeeIds: employee?.length ? employee.map((member) => member.employee.id).filter((id) => id !== undefined) : undefined, +projectIds: project?.length ? project.map((project) => project.id).filter((id) => id !== undefined) : undefined
66-69: Enhance error handling for better user experienceThe current error handling only logs to console. Consider adding proper error state management and user feedback.
}).catch((error) => { console.error('Error fetching timesheet:', error); + // Assuming you have an error state atom + setTimesheetError(error.message); + // Consider showing a user-friendly notification + showErrorNotification('Failed to fetch timesheet data'); });apps/web/lib/components/custom-select/multi-select.tsx (1)
Line range hint
1-183: Consider performance and accessibility improvementsWhile the component functions correctly, there are several opportunities for optimization:
- Memoize callback functions to prevent unnecessary re-renders:
const onClick = useCallback((item: T) => { // ... existing onClick logic }, [multiSelect, selectedItems, itemId, onValueChange]); const removeItem = useCallback((item: T) => { // ... existing removeItem logic }, [multiSelect, selectedItems, itemId, onValueChange]);
- Memoize derived state:
const selectedItemsCount = useMemo(() => selectedItems.length, [selectedItems]);
- Enhance accessibility:
<PopoverContent + role="listbox" + aria-multiselectable={multiSelect} className={cn( 'w-full max-w-full max-h-[80vh] border border-transparent dark:bg-dark--theme-light', popoverClassName )} >
- Consider extracting side effects into custom hooks:
function usePopoverWidth(triggerRef: RefObject<HTMLButtonElement>) { const [width, setWidth] = useState<number | null>(null); useEffect(() => { if (triggerRef.current) { setWidth(triggerRef.current.offsetWidth); } }, []); return width; }Would you like me to provide more detailed examples for any of these improvements?
apps/web/app/[locale]/timesheet/[memberId]/components/TimeSheetFilterPopover.tsx (4)
17-17: Consider adding prop types for memoization effectivenessWhile memoizing the component is good for performance, it would be more effective to:
- Define proper prop types for the component
- Implement a custom comparison function as the second argument to
React.memoif neededExample implementation:
type TimeSheetFilterPopoverProps = { // Add your prop types here } export const TimeSheetFilterPopover = React.memo(function TimeSheetFilterPopover(props: TimeSheetFilterPopoverProps) { // ... component implementation }, (prevProps, nextProps) => { // Add custom comparison if needed return true; });
Line range hint
24-28: Simplify the useEffect implementationThe current useEffect implementation can be simplified by directly setting the state in the clear filter button click handler instead.
Consider replacing the useEffect with a direct state update:
- React.useEffect(() => { - if (shouldRemoveItems) { - setShouldRemoveItems(false); - } - }, [shouldRemoveItems]); // In the clear filter button handler - onClick={() => setShouldRemoveItems(true)} + onClick={() => { + // Clear the filters directly + setEmployeeState([]); + setProjectState([]); + setStatusState([]); + setTaskState([]); + }}🧰 Tools
🪛 GitHub Check: Cspell
[warning] 22-22:
Unknown word (Statu)
[warning] 22-22:
Unknown word (statu)
Line range hint
49-104: Enhance accessibility and reduce code duplicationThe filter sections have several areas for improvement:
- Missing accessibility attributes for interactive elements
- Repeated MultiSelect pattern could be abstracted
- Hardcoded text sizes could use design tokens
Consider creating a reusable FilterSection component:
type FilterSectionProps<T> = { label: string; items: T[]; value: T[]; onChange: (items: T[]) => void; itemToString: (item: T) => string; itemId: (item: T) => string; }; function FilterSection<T>({ label, items, value, onChange, itemToString, itemId }: FilterSectionProps<T>) { return ( <div role="region" aria-label={label}> <label className="flex justify-between text-gray-600 mb-1 text-sm"> <span className="text-xs">{label}</span> <span role="button" tabIndex={0} className={clsxm("text-primary/10", value?.length > 0 && "text-primary dark:text-primary-light")} > Clear </span> </label> <MultiSelect items={items} onValueChange={onChange} itemToString={itemToString} itemId={itemId} multiSelect={true} triggerClassName="dark:border-gray-700" aria-label={`Select ${label}`} /> </div> ); }
Line range hint
106-120: Add proper button types and loading statesThe filter action buttons are missing proper type attributes and loading states.
<Button onClick={() => setShouldRemoveItems(true)} variant={'outline'} + type="button" className='flex items-center text-sm justify-center h-10 rounded-lg dark:text-gray-300' > <span className="text-sm">Clear Filter</span> </Button> <Button + type="submit" + disabled={isLoading} className='flex items-center text-sm justify-center h-10 rounded-lg bg-primary dark:bg-primary-light dark:text-gray-300' > - <span className="text-sm">Apply Filter</span> + <span className="text-sm">{isLoading ? 'Applying...' : 'Apply Filter'}</span> </Button>apps/web/lib/features/integrations/calendar/table-time-sheet.tsx (4)
Line range hint
248-257: Implement missing status handlersThe
handleButtonClickfunction has incomplete implementations for 'Approved' and 'Pending' states. This could lead to unexpected behavior when users interact with these buttons.Consider implementing proper handlers for all states:
const handleButtonClick = (action: StatusType) => { switch (action) { case 'Approved': - // TODO: Implement approval logic + // Add proper approval logic here + openModal(); break; case 'Rejected': openModal() break; case 'Pending': - // TODO: Implement pending logic + // Add proper pending logic here + openModal(); break; default: console.error(`Unsupported action: ${action}`); } };
Line range hint
155-156: Use provided data prop instead of hardcoded dataSourceTimeSheetThe table is using a hardcoded
dataSourceTimeSheetinstead of thedataprop passed to the component. This makes the component inflexible and harder to test.- data: dataSourceTimeSheet, + data: data?.flatMap(group => group.tasks) ?? [],
Line range hint
283-292: Remove or implement commented codeThere's commented-out code for
TaskNameInfoDisplaycomponent. Either implement this component or remove the commented code to maintain code cleanliness.Consider implementing the task name display or removing the commented section entirely.
Line range hint
238-246: Add error handling for modal actionsThe
RejectSelectedModalhas an empty implementation for the reject action. This could lead to a poor user experience when rejections fail.<RejectSelectedModal onReject={() => { - // Pending implementation + try { + // Implement rejection logic + closeModal(); + } catch (error) { + // Handle error appropriately + } }} maxReasonLength={120} minReasonLength={0} closeModal={closeModal} isOpen={isOpen} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
apps/web/app/[locale]/timesheet/[memberId]/components/TimeSheetFilterPopover.tsx(5 hunks)apps/web/app/hooks/features/useTimelogFilterOptions.ts(1 hunks)apps/web/app/hooks/features/useTimesheet.ts(3 hunks)apps/web/app/hooks/index.ts(1 hunks)apps/web/app/interfaces/timer/ITimerLog.ts(1 hunks)apps/web/app/services/client/api/timer/timer-log.ts(2 hunks)apps/web/app/stores/index.ts(1 hunks)apps/web/app/stores/time-logs.ts(1 hunks)apps/web/lib/components/custom-select/multi-select.tsx(1 hunks)apps/web/lib/features/integrations/calendar/table-time-sheet.tsx(1 hunks)
🧰 Additional context used
🪛 GitHub Check: Cspell
apps/web/app/[locale]/timesheet/[memberId]/components/TimeSheetFilterPopover.tsx
[warning] 22-22:
Unknown word (Statu)
[warning] 22-22:
Unknown word (statu)
[warning] 95-95:
Unknown word (statu)
[warning] 102-102:
Unknown word (Statu)
apps/web/app/hooks/features/useTimelogFilterOptions.ts
[warning] 8-8:
Unknown word (statu)
[warning] 8-8:
Unknown word (Statu)
[warning] 17-17:
Unknown word (statu)
[warning] 24-24:
Unknown word (Statu)
🔇 Additional comments (11)
apps/web/app/stores/time-logs.ts (2)
3-3: LGTM: Import statement is well-structured
The new type imports are properly organized and necessary for the filter state atoms.
9-18: Consider improving type definitions and adding documentation
While the implementation is functional, there are a few suggestions for improvement:
- The type pattern
T | T[] | nullinitialized with[]could lead to confusion. Consider using either:- Just array types with empty array default
- Just nullable single items
- The status filter type could be simplified using a type alias
Here's a suggested refactor:
+// Type alias for status option
+type StatusOption = { value: string; label: string };
+
+/**
+ * Filter state for employee selection in timesheet
+ * @default []
+ */
-export const timesheetFilterEmployeeState = atom<OT_Member | OT_Member[] | null>([]);
+export const timesheetFilterEmployeeState = atom<OT_Member[]>([]);
+
+/**
+ * Filter state for project selection in timesheet
+ * @default []
+ */
-export const timesheetFilterProjectState = atom<IProject | IProject[] | null>([]);
+export const timesheetFilterProjectState = atom<IProject[]>([]);
+
+/**
+ * Filter state for task selection in timesheet
+ * @default []
+ */
-export const timesheetFilterTaskState = atom<ITeamTask | ITeamTask[] | null>([]);
+export const timesheetFilterTaskState = atom<ITeamTask[]>([]);
+
+/**
+ * Filter state for status selection in timesheet
+ * @default []
+ */
-export const timesheetFilterStatusState = atom<{
- value: string;
- label: string;
-} | {
- value: string;
- label: string;
-}[] | null>([]);
+export const timesheetFilterStatusState = atom<StatusOption[]>([]);Let's verify if these atoms are used consistently across the codebase:
✅ Verification successful
Refactor is safe to implement
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of these atoms to ensure the refactor won't break existing code
rg -A 3 "timesheetFilter(Employee|Project|Task|Status)State"
Length of output: 1883
apps/web/app/hooks/features/useTimelogFilterOptions.ts (1)
1-3: LGTM! Clean and well-organized imports
The imports are properly structured, with clear separation between types, state atoms, and external dependencies.
apps/web/app/interfaces/timer/ITimerLog.ts (1)
Line range hint 1-1: Verify ITaskIssue import and usage
Let's verify the ITaskIssue interface definition and its usage patterns.
apps/web/app/services/client/api/timer/timer-log.ts (2)
23-25: LGTM! Function signature changes are well-structured.
The addition of optional projectIds and employeeIds parameters with default empty arrays is a clean approach to implementing the new filtering capabilities.
Also applies to: 31-33
59-65: Consider adding validation and optimizing parameter handling.
While the implementation works, consider these improvements for better robustness and efficiency:
- Add input validation for project and employee IDs
- Handle edge cases (empty strings, invalid IDs)
- Consider using a more efficient approach for multiple parameters
Here's a suggested improvement:
- projectIds.forEach((id, index) => {
- params.append(`projectIds[${index}]`, id);
- });
-
- employeeIds.forEach((id, index) => {
- params.append(`employeeIds[${index}]`, id);
- });
+ // Filter out invalid IDs and handle multiple parameters more efficiently
+ const validProjectIds = projectIds.filter(id => id && id.trim());
+ const validEmployeeIds = employeeIds.filter(id => id && id.trim());
+
+ if (validProjectIds.length) {
+ validProjectIds.forEach((id, index) => {
+ params.append(`projectIds[${index}]`, id.trim());
+ });
+ }
+
+ if (validEmployeeIds.length) {
+ validEmployeeIds.forEach((id, index) => {
+ params.append(`employeeIds[${index}]`, id.trim());
+ });
+ }Let's verify the API endpoint format expectations:
apps/web/app/hooks/features/useTimesheet.ts (3)
9-9: LGTM! Clean integration of filter options
The integration of useTimelogFilterOptions hook is well-structured and follows React best practices.
Also applies to: 49-49
71-71: LGTM! Proper dependency management
The dependencies array is correctly updated to include the new filter options, ensuring proper memoization behavior.
12-13: Verify the impact of optional date parameters
Making startDate and endDate optional could lead to undefined behavior in the timesheet filtering. The code should handle cases where these parameters are undefined.
✅ Verification successful
Optional date parameters are properly handled
The codebase includes necessary checks for startDate and endDate, ensuring undefined values are managed correctly.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usages of TimesheetParams to verify proper handling of undefined dates
ast-grep --pattern 'function $_(params: TimesheetParams) {
$$$
}'
# Search for direct usage of startDate/endDate
rg -A 3 'startDate|endDate'
Length of output: 24660
apps/web/lib/components/custom-select/multi-select.tsx (1)
Line range hint 9-21: Verify unique item IDs to prevent unexpected behavior
The component relies on itemId for item identification, but doesn't validate ID uniqueness. Duplicate IDs could lead to incorrect selection/deselection behavior.
Consider adding runtime validation:
function validateUniqueIds<T>(items: T[], itemId: (item: T) => string): void {
const ids = new Set<string>();
items.forEach(item => {
const id = itemId(item);
if (ids.has(id)) {
console.warn(`Duplicate item ID found: ${id}. This may cause unexpected behavior.`);
}
ids.add(id);
});
}
// Add to component:
useEffect(() => {
if (items) {
validateUniqueIds(items, itemId);
}
}, [items, itemId]);apps/web/app/[locale]/timesheet/[memberId]/components/TimeSheetFilterPopover.tsx (1)
Line range hint 1-125: Implementation aligns well with PR objectives
The changes successfully implement the useTimelogFilterOptions hook and enhance the TimeSheetFilterPopover component with proper filter functionality. The code is generally well-structured, though there are some opportunities for improvement as noted in the previous comments.
apps/web/app/[locale]/timesheet/[memberId]/components/TimeSheetFilterPopover.tsx
Outdated
Show resolved
Hide resolved
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 (15)
apps/web/app/stores/time-logs.ts (1)
9-11: Consider using null as initial value for nullable statesThe atoms are initialized with empty arrays but their types include null. For better type safety and consistency, consider using null as the initial value if no selection is intended.
-export const timesheetFilterEmployeeState = atom<OT_Member | OT_Member[] | null>([]); -export const timesheetFilterProjectState = atom<IProject | IProject[] | null>([]); -export const timesheetFilterTaskState = atom<ITeamTask | ITeamTask[] | null>([]) +export const timesheetFilterEmployeeState = atom<OT_Member | OT_Member[] | null>(null); +export const timesheetFilterProjectState = atom<IProject | IProject[] | null>(null); +export const timesheetFilterTaskState = atom<ITeamTask | ITeamTask[] | null>(null);apps/web/app/stores/index.ts (1)
30-30: Consider grouping related exports together.For better maintainability, consider grouping this export with other time/task-related exports (around line 13-17).
export * from './task-related-issue-type'; export * from './daily-plan'; +export * from './time-logs'; export * from './roles'; export * from './role-permissions'; export * from './request-to-join'; export * from './public'; export * from './integration-tenant'; export * from './integration-github'; export * from './integration-types'; export * from './integration'; -export * from './time-logs'apps/web/app/hooks/features/useTimelogFilterOptions.ts (2)
16-25: Improve return object structureThe return object can be simplified using object shorthand notation, and the status naming should be consistent.
Apply this diff to improve the code:
return { - statuState, - employee, - project, - task, - setEmployeeState, - setProjectState, - setTaskState, - setStatuState + statusState, + employee, + project, + task, + setStatusState, + setEmployeeState, + setProjectState, + setTaskState };🧰 Tools
🪛 GitHub Check: Cspell
[warning] 17-17:
Unknown word (statu)
[warning] 24-24:
Unknown word (Statu)
5-5: Add JSDoc documentationConsider adding JSDoc documentation to describe the hook's purpose, parameters, and return value. This would improve maintainability and developer experience.
Example documentation:
+/** + * Hook for managing timesheet filter options state + * @returns {Object} Object containing filter states and their setters + * @property {OT_Member[]} employee - Selected employee filters + * @property {IProject[]} project - Selected project filters + * @property {ITeamTask[]} task - Selected task filters + * @property {unknown} statusState - Selected status filters + */ export function useTimelogFilterOptions() {apps/web/app/interfaces/timer/ITimerLog.ts (1)
14-14: Consider aligning issueType types across interfacesWhile the additions enhance filtering capabilities, there's an architectural inconsistency:
Task.issueTypeis typed asstring | nullwhileITimeSheet.issueTypeusesITaskIssue. This might lead to type conversion complexity or runtime issues.Consider one of these approaches:
- Use
ITaskIssue | nullconsistently across both interfaces- Document why different types are necessary
- Create a separate type for task-specific issue types if they truly need to be different
Also applies to: 16-16
apps/web/app/services/client/api/timer/timer-log.ts (2)
59-65: Consider adding input validation and optimizing array handlingWhile the implementation is functional, consider these improvements:
- Add validation for project and employee IDs to prevent potential injection
- For large arrays, consider using a more efficient approach to construct the query parameters
Here's a suggested improvement:
- projectIds.forEach((id, index) => { - params.append(`projectIds[${index}]`, id); - }); - - employeeIds.forEach((id, index) => { - params.append(`employeeIds[${index}]`, id); - }); + // Validate and sanitize IDs + const sanitizedProjectIds = projectIds.filter(id => /^[a-zA-Z0-9-]+$/.test(id)); + const sanitizedEmployeeIds = employeeIds.filter(id => /^[a-zA-Z0-9-]+$/.test(id)); + + // More efficient array handling + if (sanitizedProjectIds.length) { + params.append('projectIds', JSON.stringify(sanitizedProjectIds)); + } + if (sanitizedEmployeeIds.length) { + params.append('employeeIds', JSON.stringify(sanitizedEmployeeIds)); + }
Line range hint
13-13: Remove unnecessary commentThe comment
// todayStart, todayEnd;appears to be a leftover note and should be removed.apps/web/app/hooks/index.ts (1)
43-43: Consider reorganizing time-related hook exports.For better code organization, consider:
- Moving this export next to other time-related hooks like
useTimer- Creating a dedicated section for time/timesheet-related hooks if there are multiple such hooks
export * from './features/useTeamTasks'; export * from './features/useTimer'; +export * from './features/useTimelogFilterOptions'; export * from './features/useUser'; export * from './features/useUserProfilePage'; -export * from './features/useTimelogFilterOptions'; export * from './useCollaborative';apps/web/app/hooks/features/useTimesheet.ts (2)
63-64: Consider handling empty filter cases explicitlyWhile filtering undefined values is good, consider these improvements:
- Empty arrays might behave differently from undefined in the API
- The mapping could be more concise
- employeeIds: employee?.map((member) => member.employee.id).filter((id) => id !== undefined), - projectIds: project?.map((project) => project.id).filter((id) => id !== undefined) + employeeIds: employee?.length ? employee.map((member) => member.employee?.id).filter(Boolean) : undefined, + projectIds: project?.length ? project.map((project) => project?.id).filter(Boolean) : undefined
66-69: Enhance error handling for better user experienceThe current error handling only logs to console. Consider adding proper error state management and user feedback.
}).catch((error) => { console.error('Error fetching timesheet:', error); + // Suggest implementing proper error state management + // setError(error); + // notifyUser('Failed to fetch timesheet data'); });apps/web/app/[locale]/timesheet/[memberId]/components/TimeSheetFilterPopover.tsx (2)
Line range hint
49-104: Consider extracting repeated MultiSelect pattern into a reusable componentThe MultiSelect sections share similar structure and behavior, leading to code duplication. This could be abstracted into a reusable component for better maintainability.
Consider creating a component like this:
type FilterSelectProps = { label: string; items: any[]; value: any[]; onValueChange: (items: any[]) => void; itemToString: (item: any) => string; itemId: (item: any) => string; removeItems: boolean; } const FilterSelect: React.FC<FilterSelectProps> = ({ label, items, value, onValueChange, itemToString, itemId, removeItems }) => ( <div> <label className="flex justify-between text-gray-600 mb-1 text-sm"> <span className="text-[12px]">{label}</span> <span className={clsxm("text-primary/10", value?.length > 0 && "text-primary dark:text-primary-light")}> Clear </span> </label> <MultiSelect removeItems={removeItems} items={items} itemToString={itemToString} itemId={itemId} onValueChange={onValueChange} multiSelect={true} triggerClassName="dark:border-gray-700" /> </div> );
Line range hint
106-120: Add missing onClick handler for Apply Filter buttonThe Apply Filter button doesn't have an onClick handler, making it non-functional. Consider adding the handler and loading states for better user feedback.
Would you like me to help implement the Apply Filter functionality?
apps/web/lib/features/integrations/calendar/table-time-sheet.tsx (3)
274-274: Consider using a more semantic disabled state stylingThe addition of
disabled:bg-blackseems too aggressive for a disabled state. Consider using more subtle styling that aligns with the application's design system.-<div className={clsxm("flex items-center gap-2 p-x-1", 'disabled:bg-black')}> +<div className={clsxm("flex items-center gap-2 p-x-1", 'disabled:opacity-50 disabled:cursor-not-allowed')}>
Line range hint
152-164: Implement pending TODO items in handleButtonClickThe function contains TODO comments for 'Approved' and 'Pending' actions that need implementation. This could lead to unexpected behavior when users interact with these actions.
Would you like help implementing these actions? I can help generate the implementation code or create GitHub issues to track these tasks.
Line range hint
152-164: Add error handling for action state changesThe switch statement's default case only logs to console. Consider adding proper error handling and user feedback.
const handleButtonClick = (action: StatusType) => { switch (action) { case 'Approved': // TODO: Implement approval logic break; case 'Rejected': openModal() break; case 'Pending': // TODO: Implement pending logic break; default: - console.error(`Unsupported action: ${action}`); + console.error(`Unsupported action: ${action}`); + // Show user feedback + toast.error(`Invalid action: ${action}`); + // Log to error tracking service + captureException(new Error(`Unsupported timesheet action: ${action}`)); } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
apps/web/app/[locale]/timesheet/[memberId]/components/TimeSheetFilterPopover.tsx(5 hunks)apps/web/app/hooks/features/useTimelogFilterOptions.ts(1 hunks)apps/web/app/hooks/features/useTimesheet.ts(3 hunks)apps/web/app/hooks/index.ts(1 hunks)apps/web/app/interfaces/timer/ITimerLog.ts(1 hunks)apps/web/app/services/client/api/timer/timer-log.ts(2 hunks)apps/web/app/stores/index.ts(1 hunks)apps/web/app/stores/time-logs.ts(1 hunks)apps/web/lib/components/custom-select/multi-select.tsx(1 hunks)apps/web/lib/features/integrations/calendar/table-time-sheet.tsx(1 hunks)
🧰 Additional context used
🪛 GitHub Check: Cspell
apps/web/app/[locale]/timesheet/[memberId]/components/TimeSheetFilterPopover.tsx
[warning] 22-22:
Unknown word (Statu)
[warning] 22-22:
Unknown word (statu)
[warning] 95-95:
Unknown word (statu)
[warning] 102-102:
Unknown word (Statu)
apps/web/app/hooks/features/useTimelogFilterOptions.ts
[warning] 8-8:
Unknown word (statu)
[warning] 8-8:
Unknown word (Statu)
[warning] 17-17:
Unknown word (statu)
[warning] 24-24:
Unknown word (Statu)
🔇 Additional comments (14)
apps/web/app/stores/time-logs.ts (2)
3-3: LGTM: Import statement properly includes required types
The new imports are correctly added and necessary for typing the new filter states.
9-18: Verify completeness of filter states
Please verify if date/time period filters are needed for the timesheet filtering feature. If they are handled elsewhere, this is fine. If not, consider adding them to complete the filtering functionality.
apps/web/app/stores/index.ts (1)
30-30: LGTM! Export addition looks good.
The new export for time-logs module aligns with the PR's objective of enhancing timesheet functionality.
apps/web/app/hooks/features/useTimelogFilterOptions.ts (1)
1-3: LGTM! Well-organized imports
The imports are properly structured, following a good pattern of separating external libraries from internal imports.
apps/web/app/interfaces/timer/ITimerLog.ts (2)
Line range hint 77-77: LGTM: Optional issueType aligns with interface design
Making issueType optional is consistent with the interface's pattern for nullable fields and maintains backward compatibility.
14-16: Verify usage of new optional fields
Let's ensure existing code handles these new optional fields appropriately.
Also applies to: 77-77
✅ Verification successful
All usages of issueType and taskStatus handle null values appropriately.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct usage of these fields to verify proper null checking
echo "Checking usage of Task.issueType and Task.taskStatus:"
rg -A 2 "\.issueType\b|\.taskStatus\b" --type ts
echo "Checking for potential type assertions or non-null assumptions:"
ast-grep --pattern 'task.issueType!' --lang ts
ast-grep --pattern 'task.taskStatus!' --lang ts
Length of output: 28414
apps/web/app/services/client/api/timer/timer-log.ts (1)
23-25: LGTM! Well-structured parameter additions
The new optional parameters are properly typed and include sensible defaults, maintaining backward compatibility.
Also applies to: 31-33
apps/web/app/hooks/index.ts (1)
43-43: Verify the new hook implementation and integration.
Let's verify the hook's implementation and its integration with the TimeSheetFilterPopover component.
✅ Verification successful
Verified the new hook implementation and its integration with the TimeSheetFilterPopover component.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the hook implementation and integration
# Check if the hook file exists
echo "Checking hook file existence..."
fd "useTimelogFilterOptions.ts" apps/web/app/hooks/features
# Check hook implementation patterns
echo -e "\nChecking hook implementation..."
rg -A 10 "export function useTimelogFilterOptions" apps/web/app/hooks/features
# Check integration with TimeSheetFilterPopover
echo -e "\nChecking component integration..."
rg "useTimelogFilterOptions" apps/web/app/\[locale\]/timesheet --type ts
Length of output: 1973
apps/web/app/hooks/features/useTimesheet.ts (2)
9-9: Clean integration of filter options
The integration of useTimelogFilterOptions hook follows React best practices and properly separates concerns for filter state management.
Also applies to: 49-49
71-71: Proper dependency management
The dependencies array has been correctly updated to include the new filter states, ensuring proper memoization of the callback.
apps/web/lib/components/custom-select/multi-select.tsx (1)
137-137: LGTM! Defensive programming approach added.
The optional chaining operator (?.) is a good defensive programming practice to prevent runtime errors when items is undefined during initial render or loading states.
apps/web/app/[locale]/timesheet/[memberId]/components/TimeSheetFilterPopover.tsx (2)
Line range hint 1-17: Good use of React.memo for performance optimization!
The memoization will prevent unnecessary re-renders of this component when parent components update with props that haven't changed.
Line range hint 24-28: LGTM: Clean effect implementation
The effect has a clear purpose and correctly handles the reset of shouldRemoveItems.
🧰 Tools
🪛 GitHub Check: Cspell
[warning] 22-22:
Unknown word (Statu)
[warning] 22-22:
Unknown word (statu)
apps/web/lib/features/integrations/calendar/table-time-sheet.tsx (1)
274-276: Verify the integration with useTimelogFilterOptions
The PR introduces useTimelogFilterOptions hook but it's not utilized in this component. Consider if this component should integrate with the new filtering functionality.
apps/web/app/[locale]/timesheet/[memberId]/components/TimeSheetFilterPopover.tsx
Outdated
Show resolved
Hide resolved
Innocent-Akim
changed the title
Innocent-Akim
removed
the
Don't Merge
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (5)
apps/web/lib/features/task/task-block-card.tsx (1)
79-79: LGTM! Consider adding a comment to explain the fallback.The change to default to
0when no tasks exist is a good defensive programming practice. This prevents potential undefined/null values from causing display issues in the time formatting.Consider adding a comment to explain the fallback logic:
(currentMember?.totalWorkedTasks && currentMember?.totalWorkedTasks?.length && currentMember?.totalWorkedTasks .filter((t) => t.id === task?.id) .reduce( (previousValue, currentValue) => previousValue + currentValue.duration, 0 )) || - 0 + 0 // Default to 0 seconds when no tasks exist or no duration is recordedapps/web/app/[locale]/timesheet/[memberId]/components/TimeSheetFilterPopover.tsx (3)
22-22: Maintain consistency in state variable namingThe state variables follow different naming patterns:
employee,project,statusState,task. Consider using consistent naming across all filter states.- const { setEmployeeState, setProjectState, setStatusState, setTaskState, employee, project, statusState, task } = useTimelogFilterOptions(); + const { setEmployeeState, setProjectState, setStatusState, setTaskState, employeeState, projectState, statusState, taskState } = useTimelogFilterOptions();
Line range hint
119-122: Implement onClick handler for Apply Filter buttonThe Apply Filter button is missing an onClick handler, making it non-functional. Consider implementing the filter application logic or removing the button if filters are applied automatically.
<Button + onClick={() => {/* Implement filter application logic */}} className='flex items-center text-sm justify-center h-10 rounded-lg bg-primary dark:bg-primary-light dark:text-gray-300' > <span className="text-sm">Apply Filter</span> </Button>
Line range hint
31-39: Enhance accessibility for filter trigger buttonConsider adding ARIA attributes to improve accessibility for screen readers.
<Button variant="outline" + aria-label="Open filter options" className='flex items-center justify-center h-[2.2rem] rounded-lg bg-white dark:bg-dark--theme-light border dark:border-gray-700 hover:bg-white p-3 gap-2'>apps/web/lib/components/custom-select/multi-select.tsx (1)
19-20: Add prop documentation and improve type safety.Consider adding JSDoc comments and improving type safety:
interface MultiSelectProps<T> { + /** Flag to clear all selected items */ removeItems?: boolean; + /** Key used for localStorage persistence. Should be unique per instance */ - localStorageKey?: string; + localStorageKey?: `multiselect-${string}`; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
apps/web/app/[locale]/timesheet/[memberId]/components/TimeSheetFilterPopover.tsx(5 hunks)apps/web/lib/components/custom-select/multi-select.tsx(2 hunks)apps/web/lib/features/task/task-block-card.tsx(1 hunks)
🔇 Additional comments (3)
apps/web/app/[locale]/timesheet/[memberId]/components/TimeSheetFilterPopover.tsx (3)
17-17: LGTM: Good use of React.memo for performance optimization
The memoization will prevent unnecessary re-renders of this component when parent components update.
Line range hint 24-28: LGTM: Effect hook implementation is clean and focused
The effect hook correctly handles the reset of shouldRemoveItems with proper dependency tracking.
53-53: LGTM: Good use of localStorageKey for filter persistence
The addition of localStorageKey for each filter ensures that user preferences are preserved across sessions.
Also applies to: 69-69, 85-85, 101-101
apps/web/app/[locale]/timesheet/[memberId]/components/TimeSheetFilterPopover.tsx
Show resolved
Hide resolved
#3044
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
Improvements
TimeSheetFilterPopoverfor better performance and user interface responsiveness.getTaskTimesheetLogsApifunction to accept additional filtering parameters for improved data retrieval.DataTableTimeSheetcomponent for improved styling flexibility.MultiSelectcomponent for better state persistence.TaskBlockCardcomponent to ensure the total worked tasks timer defaults to zero when no tasks are found.Bug Fixes
Documentation