-
Notifications
You must be signed in to change notification settings - Fork 51
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix(timesheet): Correct handling of timesheet updates in state #3497
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/[locale]/timesheet/[memberId]/components/AddTaskModal.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. WalkthroughThis pull request introduces several modifications across multiple files in the timesheet management section of the web application. The changes primarily focus on improving the handling of timesheet-related components, including task addition, editing, and status management. Key updates involve refining data handling in modal components, enhancing error logging in the Changes
Sequence DiagramsequenceDiagram
participant User
participant AddTaskModal
participant useTimesheet
participant API
User->>AddTaskModal: Fill task details
AddTaskModal->>useTimesheet: Create timesheet
useTimesheet->>API: Send timesheet data
API-->>useTimesheet: Return response
useTimesheet-->>AddTaskModal: Confirm creation
AddTaskModal->>User: Close modal
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/web/app/[locale]/timesheet/[memberId]/components/AddTaskModal.tsx (1)
156-158
: Directly storing the fullvalue
object.Instead of extracting
id
alone, storing the entirevalue
object opens possibilities for future enhancements (e.g., displaying additional task details). Ensure all references toformState.taskId
still function correctly ifvalue
is an object instead of a plain string.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/web/app/[locale]/timesheet/[memberId]/components/AddTaskModal.tsx
(2 hunks)apps/web/app/[locale]/timesheet/[memberId]/components/EditTaskModal.tsx
(1 hunks)apps/web/app/[locale]/timesheet/[memberId]/components/RejectSelectedModal.tsx
(4 hunks)apps/web/app/[locale]/timesheet/[memberId]/page.tsx
(1 hunks)apps/web/app/hooks/features/useTimesheet.ts
(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: deploy
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (13)
apps/web/app/[locale]/timesheet/[memberId]/components/RejectSelectedModal.tsx (5)
4-4
: Great usage of ReloadIcon.No concerns on this import; it integrates seamlessly for loading indicators.
37-37
: Integration of the new hook variables looks good.Using
useTimesheet
to accessupdateTimesheetStatus
,loadingUpdateTimesheetStatus
, andsetSelectTimesheetId
within this component ensures a cohesive state management flow. No immediate issues detected.
49-49
: Modal closure timing is appropriate.Closing the modal immediately after resetting the
selectTimesheetId
array is consistent with a clean success flow.
101-101
: Improved button disabling condition.Disabling the button based on
loadingUpdateTimesheetStatus
prevents duplicate submissions during status updates.
109-111
: Better user feedback with a loading spinner.Displaying the
ReloadIcon
whileloadingUpdateTimesheetStatus
is true helps clearly convey a loading state.apps/web/app/[locale]/timesheet/[memberId]/page.tsx (1)
174-174
: Well-structured zero-padding formatting.Using
String(...).padStart(2, '0')
standardizes hour and minute display, improving readability for single-digit values.apps/web/app/hooks/features/useTimesheet.ts (5)
10-10
: Axios import check.Importing axios here is reasonable for handling HTTP calls and error inspection.
29-40
: Switched to directly checkingitem.createdAt
.Replacing checks for
timesheet?.createdAt
withitem?.createdAt
clarifies data access. The added warning and error logs provide improved visibility ifcreatedAt
is missing or invalid.
65-77
: Consistent grouping function updates.The logic follows the same pattern as in
groupByDate
. Ensuringitem?.createdAt
and logging errors if missing is a good approach for data integrity.
181-195
: Enhanced error handling for createTimesheet.Distinguishing between Axios and generic errors allows for more targeted debugging. Throwing a custom error message and logging the response status provides a clear audit trail.
201-230
: Improved updateTimesheet flow.
- Logging a warning when the user is unauthenticated (instead of throwing an error) is sensible if a no-op is intended.
- Verifying
response?.data?.id
before patching the local state ensures safe partial updates.- Overall, these revisions reduce the likelihood of stale or incorrect state updates.
apps/web/app/[locale]/timesheet/[memberId]/components/AddTaskModal.tsx (2)
131-131
: Explicitly mappingtaskId
in the payload.Mirroring this assignment clarifies which field is being used for timesheet creation. This small addition reduces confusion in the codebase.
135-135
: Immediate modal closure upon success.Closing the modal right after creating timesheets helps maintain a clear user flow without extra steps.
Description
#3441
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
Bug Fixes
Performance