[Feat]: Added Bulk Deletion Functionality For Time Entries #3402
Conversation
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
apps/web/app/hooks/features/useTimelogFilterOptions.tsOops! 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. WalkthroughThis pull request introduces several modifications across multiple components, primarily focusing on enhancing the Changes
Possibly related issues
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
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: 6
🧹 Outside diff range and nitpick comments (2)
apps/web/lib/features/integrations/calendar/table-time-sheet.tsx (2)
160-163: Consider using more descriptive variable names for modalsUsing generic names like
modalandalertConfirmationModalwith overlapping property names (isOpen,openModal, etc.) can lead to confusion and potential mistakes. Consider renaming these variables and their destructured properties to more descriptive names to improve code readability and maintainability.Suggested changes:
- const modal = useModal(); - const alertConfirmationModal = useModal(); - const { isOpen, openModal, closeModal } = modal; - const { isOpen: isOpenAlert, openModal: openAlertConfirmation, closeModal: closeAlertConfirmation } = alertConfirmationModal; + const rejectionModal = useModal(); + const deletionModal = useModal(); + const { isOpen: isRejectionOpen, openModal: openRejectionModal, closeModal: closeRejectionModal } = rejectionModal; + const { isOpen: isDeletionOpen, openModal: openDeletionModal, closeModal: closeDeletionModal } = deletionModal;
170-172: Rename parameter 'items' to 'timesheetId' for clarityIn the
handleSelectRowTimesheetfunction, the parameteritemsrepresents a single timesheet ID. Renaming it totimesheetIdwould enhance readability and make the code's intent clearer.Suggested changes:
- const handleSelectRowTimesheet = (items: string) => { - setSelectTimesheetId((prev) => prev.includes(items) ? prev.filter((filter) => filter !== items) : [...prev, items]) + const handleSelectRowTimesheet = (timesheetId: string) => { + setSelectTimesheetId((prev) => + prev.includes(timesheetId) + ? prev.filter((id) => id !== timesheetId) + : [...prev, timesheetId] + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
apps/web/app/[locale]/timesheet/[memberId]/components/EditTaskModal.tsx(1 hunks)apps/web/app/hooks/features/useTimesheet.ts(3 hunks)apps/web/lib/components/alert-dialog-confirmation.tsx(2 hunks)apps/web/lib/features/integrations/calendar/table-time-sheet.tsx(10 hunks)
🔇 Additional comments (1)
apps/web/lib/features/integrations/calendar/table-time-sheet.tsx (1)
493-495: Ensure 'Undo' action properly restores deleted timesheet
The 'Undo' action in the toast suggests that the timesheet can be restored, but it's unclear if this functionality is implemented. If the 'Undo' feature is not available, consider removing the action to prevent user confusion.
Please confirm whether the 'Undo' action in the toast notification restores the deleted timesheet. If not implemented, remove the ToastAction to avoid misleading users.
Also applies to: 498-500
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: 0
🧹 Outside diff range and nitpick comments (3)
apps/web/app/hooks/features/useTimelogFilterOptions.ts (3)
33-33: Improve type safety and naming of selection handlerThe handler correctly implements toggle functionality but could benefit from better typing and naming.
Consider these improvements:
-const handleSelectRowTimesheet = (items: string) => { +const handleToggleTimesheetSelection = (timesheetId: string) => { - setSelectTimesheetId((prev) => prev.includes(items) ? prev.filter((filter) => filter !== items) : [...prev, items]) + setSelectTimesheetId((prev) => + prev.includes(timesheetId) + ? prev.filter((id) => id !== timesheetId) + : [...prev, timesheetId] + ) }
47-47: Consider comprehensive cleanup of selection statesThe cleanup effect only resets
selectTimesheetIdbut ignores other selection states likeselectedItems.Consider cleaning up all selection-related states:
-return () => setSelectTimesheetId([]); +return () => { + setSelectTimesheetId([]); + setSelectedItems([]); + setSelectTimesheet(null); // if applicable +};
60-61: Consider grouping related state exportsThe return object exposes both old and new selection states, but they're not grouped together for better readability.
Consider grouping related state exports together:
return { statusState, employee, project, task, setEmployeeState, setProjectState, setTaskState, setStatusState, handleSelectRowTimesheet, - selectTimesheetId, - setSelectTimesheetId, handleSelectRowByStatusAndDate, selectedItems, selectTimesheet, setSelectTimesheet, + selectTimesheetId, + setSelectTimesheetId, timesheetGroupByDays, setTimesheetGroupByDays, generateTimeOptions, setPuTimesheetStatus, puTimesheetStatus };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
apps/web/app/hooks/features/useTimelogFilterOptions.ts(4 hunks)apps/web/lib/features/integrations/calendar/table-time-sheet.tsx(10 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/lib/features/integrations/calendar/table-time-sheet.tsx
🔇 Additional comments (2)
apps/web/app/hooks/features/useTimelogFilterOptions.ts (2)
Line range hint 1-72: Implementation aligns well with bulk deletion requirements
The changes effectively support the bulk deletion functionality by introducing multi-select capability while maintaining backward compatibility. The implementation is solid with good separation of concerns.
14-14: Consider consolidating timesheet selection states
The hook maintains both selectTimesheet and selectTimesheetId states for handling timesheet selection. This could lead to confusion and maintenance issues.
Consider deprecating selectTimesheet state if it's no longer needed, or document the distinct purpose of each state if both are required.
Innocent-Akim
added
Bug fix
and removed
bug
Description
#3221
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
AlertConfirmationModalfor enhanced confirmation actions.EditTaskModal.DataTableTimeSheet.Bug Fixes
Improvements