Skip to content
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

[Refactor]: Optimize DeleteTimesheet and UpdateTimesheetStatus #3375

Merged
merged 12 commits into from
Nov 27, 2024

Conversation

Innocent-Akim
Copy link
Contributor

@Innocent-Akim Innocent-Akim commented Nov 27, 2024

Description

Please include a summary of the changes and the related issue.

Type of Change

  • Bug fix
  • New feature
  • Breaking change
  • Documentation update

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings

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

    • Introduced the ability to create and update timesheets via new API handlers.
    • Added a new state management atom for tracking individual timesheet updates.
    • Enhanced StatusTask component to directly accept timesheet data, improving clarity and functionality.
    • Implemented asynchronous handling for timesheet status updates, enhancing user interaction.
  • Bug Fixes

    • Improved error handling for timesheet creation and updates.
  • Documentation

    • Updated interfaces for better type safety and clarity in timesheet management.

These changes enhance user experience by streamlining timesheet operations and improving data handling within the application.

@Innocent-Akim Innocent-Akim self-assigned this Nov 27, 2024
Copy link
Contributor

coderabbitai bot commented Nov 27, 2024

Caution

Review failed

The pull request is closed.

Walkthrough

The changes in this pull request introduce an asynchronous POST handler for managing timesheet updates, enhancements to the useTimesheet hook, and various related interfaces and services. A new function for creating timesheets is added, and existing functions are modified to improve error handling and state management. The StatusTask component is updated to accept a TimesheetLog prop instead of an ID, reflecting a shift towards better data structure management. Overall, these changes aim to improve the functionality and organization of timesheet-related features.

Changes

File Path Change Summary
apps/web/app/api/timer/timesheet/time-log/route.ts Added asynchronous POST handler for timesheet updates, including authentication and error handling.
apps/web/app/hooks/features/useTimesheet.ts Integrated createTimesheet, added updateTimesheet, modified updateTimesheetStatus to async/await syntax, and simplified deleteTaskTimesheet.
apps/web/app/interfaces/timer/ITimerLog.ts Updated Timesheet and TimesheetLog interfaces for type safety, added UpdateTimesheet interface.
apps/web/app/services/client/api/timer/timer-log.ts Added createTimesheetFromApi and updateTimesheetFromAPi functions for creating and updating timesheet logs, updated imports.
apps/web/app/services/server/requests/timesheet.ts Added createTimesheetRequest and updateTimesheetRequest functions for creating and updating timesheet entries, updated imports.
apps/web/app/stores/time-logs.ts Added new atom timesheetUpdateState for managing TimesheetLog state.
apps/web/lib/features/integrations/calendar/table-time-sheet.tsx Updated StatusTask component to accept timesheet prop instead of ids, adjusted related logic.
apps/web/app/api/timer/timesheet/time-log/[id]/route.ts Added asynchronous PUT function for updating timesheets, including authentication and error handling.

Possibly related PRs

Suggested labels

feature, Bug fix

Suggested reviewers

  • evereq
  • Cedric921

🐰 In the world of time and logs,
We hop and skip through tasks like frogs.
With new routes and hooks, we play,
Timesheets updated, hip-hip-hooray!
So gather 'round, let’s celebrate,
Our timesheet magic, oh, it’s great! 🎉

Warning

Rate limit exceeded

@Innocent-Akim has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 2 minutes and 46 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between 474f18b and 6bfbfba.

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

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

apps/web/app/hooks/features/useTimesheet.ts

Oops! 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.


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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 10

🧹 Outside diff range and nitpick comments (7)
apps/web/app/stores/time-logs.ts (1)

19-22: Consider consolidating timesheet operation states.

The current implementation has separate atoms for different timesheet operations (timesheetDeleteState, timesheetUpdateStatus, timesheetUpdateState). This separation might lead to synchronization challenges when multiple operations are in progress.

Consider consolidating these into a single atom with a more comprehensive state structure:

type TimesheetOperationState = {
  deleting: string[];
  statusUpdates: UpdateTimesheetStatus[];
  currentUpdate: TimesheetLog | null;
  isLoading: boolean;
  error: Error | null;
};

export const timesheetOperationsState = atom<TimesheetOperationState>({
  deleting: [],
  statusUpdates: [],
  currentUpdate: null,
  isLoading: false,
  error: null
});

This approach would:

  1. Centralize timesheet operation states
  2. Make it easier to track loading and error states
  3. Prevent potential race conditions between operations
  4. Simplify state management in components
apps/web/app/services/server/requests/timesheet.ts (1)

112-120: Add JSDoc documentation for consistency.

Consider adding JSDoc documentation to match the documentation style of other functions in this file (e.g., see ITimesheetProps documentation above).

Here's a suggested documentation:

+/**
+ * Creates a new timesheet entry
+ * @param params - Timesheet update parameters
+ * @param bearer_token - Authentication token
+ * @returns Promise<TimesheetLog> - Created timesheet entry
+ */
export function createTimesheetRequest(params: UpdateTimesheet, bearer_token: string) {
apps/web/app/interfaces/timer/ITimerLog.ts (2)

73-73: Consider aligning status types across interfaces

While using TimesheetStatus type improves type safety, there's an inconsistency with the UpdateTimesheetStatus interface which still uses string literals. Consider using TimesheetStatus consistently across both interfaces.


Documentation update needed for new timesheet interfaces

The codebase lacks specific documentation for the new timesheet interfaces. While there are some JSDoc comments in the interfaces directory, the new timesheet-related interfaces are not documented. Please:

  • Add JSDoc comments to document the TimesheetLog, UpdateTimesheetStatus, and UpdateTimesheet interfaces
  • Consider adding usage examples in the web app's README.md or create a dedicated documentation file for timesheet features
  • Document the relationship between these interfaces and their role in the new async POST handler
🔗 Analysis chain

Line range hint 1-158: Verify documentation updates

The interface changes look good and align with the PR objectives. However, since this is marked as a new feature in the PR description, please ensure that the documentation is updated to reflect these interface changes.

Let's check for documentation files:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for relevant documentation files
fd -e md -e mdx | grep -i "timesheet\|timer"

Length of output: 46


Script:

#!/bin/bash
# Let's try a broader search for documentation files
fd -t f "README|CONTRIBUTING|docs|documentation" -i

# Also search for any documentation references in the codebase
rg -i "documentation" --type ts --type tsx --type js --type jsx -g "!node_modules"

# Search for comments or JSDoc that might document these interfaces
rg -i "@interface|/\*\*.*timesheet|/\*\*.*timer" --type ts

Length of output: 806


Script:

#!/bin/bash
# Let's check the content of the web app README since it's most relevant
cat apps/web/README.md

# Let's also check if there are any API documentation or type definitions in the web app
rg -i "timesheet|timer" apps/web/README.md

# Search for any TypeScript interface documentation in the interfaces directory
rg -i "/\*\*" apps/web/app/interfaces/ -A 5

Length of output: 2802

apps/web/lib/features/integrations/calendar/table-time-sheet.tsx (2)

494-497: Consider simplifying the timesheet prop type

The nested timesheet.timesheet.id access suggests a potentially redundant data structure. Consider flattening the TimesheetLog interface to avoid double nesting.


Line range hint 524-541: Implement billable status functionality

The billable status menu items are present but lack implementation for updating the billable status.

Would you like help implementing the billable status update functionality?

apps/web/app/hooks/features/useTimesheet.ts (1)

150-150: Replace 'console.log' with appropriate feedback mechanism

Using console.log is not recommended in production code. Consider replacing console.log with a proper logging mechanism or providing user feedback through the UI.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 528362c and 97b2db9.

📒 Files selected for processing (7)
  • apps/web/app/api/timer/timesheet/time-log/route.ts (1 hunks)
  • apps/web/app/hooks/features/useTimesheet.ts (6 hunks)
  • apps/web/app/interfaces/timer/ITimerLog.ts (4 hunks)
  • apps/web/app/services/client/api/timer/timer-log.ts (2 hunks)
  • apps/web/app/services/server/requests/timesheet.ts (2 hunks)
  • apps/web/app/stores/time-logs.ts (1 hunks)
  • apps/web/lib/features/integrations/calendar/table-time-sheet.tsx (3 hunks)
🔇 Additional comments (7)
apps/web/app/api/timer/timesheet/time-log/route.ts (2)

1-4: LGTM! Clean and well-organized imports

The imports are properly structured and demonstrate good separation of concerns between authentication, interfaces, and request handling.


6-6: LGTM! Proper Next.js API route handler signature

The handler follows Next.js API route conventions with correct async/await usage.

apps/web/app/services/server/requests/timesheet.ts (2)

4-4: LGTM! Import changes are appropriate.

The addition of UpdateTimesheet type import aligns with the new functionality being introduced.


112-120: Verify endpoint usage consistency.

The function implementation looks good, following the established patterns. Let's verify that the endpoint '/timesheet/time-log' is used consistently across the codebase.

✅ Verification successful

Endpoint usage is consistent across the codebase

The verification shows that the /timesheet/time-log endpoint is used consistently throughout the codebase, appearing in both server and client-side implementations with the same path structure. All usages follow similar patterns for GET, POST, and DELETE operations.

  • apps/web/app/services/server/requests/timesheet.ts: Base implementation
  • apps/web/app/services/client/api/timer/manual-time.ts: Client-side manual time logging
  • apps/web/app/services/client/api/timer/timer-log.ts: Client-side timer operations
  • apps/web/app/services/client/api/activity/time-logs.ts: Activity reporting
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for consistent usage of the timesheet endpoint
# Expected: Should show all usages of the timesheet/time-log endpoint to verify consistency

# Search for timesheet endpoint usage patterns
echo "Checking timesheet endpoint usage patterns..."
rg -A 2 "timesheet/time-log" --type ts

# Search for potential alternate endpoint definitions
echo "Checking for potential alternate endpoint definitions..."
rg -A 2 "TIMESHEET.*ENDPOINT|TIMELOG.*ENDPOINT|TIME_LOG.*ENDPOINT" --type ts

Length of output: 2224

apps/web/app/interfaces/timer/ITimerLog.ts (1)

138-158: Well-structured type definition using TypeScript utilities

The UpdateTimesheet interface is well-designed:

  • Good use of Pick utility type to ensure type safety
  • Clear separation between optional and required fields
  • Proper reuse of existing types
apps/web/app/services/client/api/timer/timer-log.ts (1)

1-2: LGTM: Import changes are appropriate

The new imports are correctly added to support the new functionality.

apps/web/app/hooks/features/useTimesheet.ts (1)

287-287: Verify the definition of 'puTimesheetStatus'

The variable puTimesheetStatus is included in the returned object, but it may not be correctly defined or might be a typo. Please verify that puTimesheetStatus is properly defined and intended to be included.

Run the following script to search for the definition and usage of puTimesheetStatus in the codebase:

✅ Verification successful

Let me search for the definition of timesheetUpdateStatus atom to fully verify this.


Let me check the type definition of UpdateTimesheetStatus to complete the verification.


Let me try searching for interface or type definition with a broader pattern.


Let me fix the regex syntax and try again.


puTimesheetStatus is properly defined and used correctly

The variable is correctly defined and used in the codebase:

  • It's defined as a Jotai atom of type UpdateTimesheetStatus[] in stores/time-logs.ts
  • The type UpdateTimesheetStatus is properly defined in interfaces/timer/ITimerLog.ts
  • It's used consistently across the timesheet-related hooks and components
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Search for definitions and usages of 'puTimesheetStatus' in TypeScript files

rg 'puTimesheetStatus' --type ts -A 5 -B 5

Length of output: 3861


Script:

#!/bin/bash
# Search for the definition of timesheetUpdateStatus atom
rg "timesheetUpdateStatus.*=.*atom" --type ts -A 2 -B 2

Length of output: 450


Script:

#!/bin/bash
# Search for UpdateTimesheetStatus type definition
rg "type UpdateTimesheetStatus" --type ts -A 5 -B 1

Length of output: 53


Script:

#!/bin/bash
# Search for UpdateTimesheetStatus interface or type definition
rg "UpdateTimesheetStatus(\s+{|^interface|^type)" --type ts -A 5 -B 1

Length of output: 224


Script:

#!/bin/bash
# Search for UpdateTimesheetStatus interface or type definition
rg "UpdateTimesheetStatus" --type ts -A 5 -B 1

Length of output: 7107

apps/web/app/stores/time-logs.ts Outdated Show resolved Hide resolved
apps/web/app/interfaces/timer/ITimerLog.ts Outdated Show resolved Hide resolved
apps/web/app/services/client/api/timer/timer-log.ts Outdated Show resolved Hide resolved
apps/web/app/hooks/features/useTimesheet.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (4)
apps/web/app/hooks/features/useTimesheet.ts (2)

164-191: Enhance error handling and user feedback in updateTimesheetStatus

The function has been improved with better state management, but could benefit from these enhancements:

 async ({ status, ids }: { status: TimesheetStatus; ids: ID[] | ID }) => {
     if (!user) return;
     const idsArray = Array.isArray(ids) ? ids : [ids];
+    if (idsArray.length === 0) {
+        throw new Error("No timesheet IDs provided");
+    }
     try {
         const response = await queryUpdateTimesheetStatus({ ids: idsArray, status });
+        if (!Array.isArray(response.data)) {
+            throw new Error("Invalid response data");
+        }
         const responseMap = new Map(response.data.map(item => [item.id, item]));
         setTimesheet(prevTimesheet =>
             prevTimesheet.map(item => {
                 const updatedItem = responseMap.get(item.timesheet.id);
                 if (updatedItem) {
                     return {
                         ...item,
                         timesheet: {
                             ...item.timesheet,
                             status: updatedItem.status
                         }
                     };
                 }
                 return item;
             })
         );
-        console.log('Timesheet status updated successfully!');
+        // TODO: Use a toast notification instead of console.log
     } catch (error) {
-        console.error('Error updating timesheet status:', error);
+        const errorMessage = error instanceof Error ? error.message : 'Unknown error';
+        throw new Error(`Failed to update timesheet status: ${errorMessage}`);
     }
 },

Changes:

  1. Added validation for empty idsArray
  2. Added response data type validation
  3. Improved error handling with proper error message propagation
  4. Suggested moving success message to a toast notification

248-256: Consider optimizing state updates in deleteTaskTimesheet

The state update logic has been improved, but consider these enhancements:

-setTimesheet(prevTimesheet =>
-    prevTimesheet.filter(item => !logIds.includes(item.timesheet.id))
-);
+setTimesheet(prevTimesheet => {
+    // Optimize for large arrays by creating a Set
+    const logIdSet = new Set(logIds);
+    return prevTimesheet.filter(item => !logIdSet.has(item.timesheet.id));
+});

This optimization uses a Set for O(1) lookups instead of Array.includes() which is O(n), improving performance when dealing with large arrays.

apps/web/lib/features/integrations/calendar/table-time-sheet.tsx (2)

224-224: Consider keeping the selection count in the dialog

The removal of ${selectTimesheet.length} from the dialog description makes it less informative for bulk deletions. Users should know how many items they're about to delete.

-description={`This action is irreversible. All related data will be lost.`}
+description={`This action is irreversible. All related data will be lost. (${selectTimesheet.length} ${selectTimesheet.length === 1 ? 'item' : 'items'})`}

Line range hint 499-521: Improve error handling in StatusTask component

The error handling implementation is incomplete and lacks user feedback.

Apply these improvements:

 onClick={async () => {
   try {
     await updateTimesheetStatus({
       status: status.label as TimesheetStatus,
       ids: [timesheet.timesheet.id]
     });
+    // Add success notification
   } catch (error) {
-    console.error('Failed to update timesheet status:');
+    console.error('Failed to update timesheet status:', error);
+    // Add error notification
   }
 }}
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 97b2db9 and dbba956.

📒 Files selected for processing (5)
  • apps/web/app/hooks/features/useTimesheet.ts (6 hunks)
  • apps/web/app/interfaces/timer/ITimerLog.ts (4 hunks)
  • apps/web/app/services/client/api/timer/timer-log.ts (2 hunks)
  • apps/web/app/stores/time-logs.ts (2 hunks)
  • apps/web/lib/features/integrations/calendar/table-time-sheet.tsx (6 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • apps/web/app/interfaces/timer/ITimerLog.ts
  • apps/web/app/stores/time-logs.ts
🧰 Additional context used
🪛 Biome (1.9.4)
apps/web/app/hooks/features/useTimesheet.ts

[error] 154-154: The catch clause that only rethrows the original error is useless.

An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.

(lint/complexity/noUselessCatch)

🔇 Additional comments (9)
apps/web/app/services/client/api/timer/timer-log.ts (3)

1-2: LGTM: Import changes are appropriate

The new imports (UpdateTimesheet and post) are correctly added to support the new timesheet creation functionality.


132-143: 🛠️ Refactor suggestion

Enhance error handling and add documentation

While the basic error handling is good, consider these improvements:

+/**
+ * Creates a new timesheet log entry
+ * @param {UpdateTimesheet} data - The timesheet data to create
+ * @returns {Promise<TimesheetLog>} The created timesheet log
+ * @throws {Error} If required parameters are missing or API call fails
+ */
-export function createTimesheetFromApi(data: UpdateTimesheet) {
+export async function createTimesheetFromApi(data: UpdateTimesheet): Promise<TimesheetLog> {
 	const organizationId = getOrganizationIdCookie();
 	const tenantId = getTenantIdCookie();
 	if (!organizationId || !tenantId) {
 		throw new Error('Required parameters missing: organizationId and tenantId are required');
 	}
+	
+	// Validate required fields in data
+	if (!data || !data.duration || !data.startedAt) {
+		throw new Error('Required timesheet data missing: duration and startedAt are required');
+	}
+
 	try {
-		return post<TimesheetLog>('/timesheet/time-log', { ...data, organizationId }, { tenantId })
+		return await post<TimesheetLog>('/timesheet/time-log', { ...data, organizationId }, { tenantId });
 	} catch (error) {
-		throw new Error('Failed to create timesheet log');
+		const message = error instanceof Error ? error.message : 'Unknown error';
+		throw new Error(`Failed to create timesheet log: ${message}`);
 	}
 }

Changes include:

  1. Added JSDoc documentation
  2. Made the function explicitly async and added return type
  3. Added validation for required timesheet data fields
  4. Enhanced error message with original error details
  5. Added proper semicolon for consistency

Likely invalid or redundant comment.


139-139: Verify endpoint path and response type handling

Let's ensure the endpoint path and response type handling are consistent across the codebase.

✅ Verification successful

Let me gather more information about the TimesheetLog type usage to ensure type consistency.


Endpoint and type usage are consistent

The verification shows that:

  1. The endpoint '/timesheet/time-log' is consistently used in both client and server implementations
  2. The TimesheetLog type is properly defined as an interface in apps/web/app/interfaces/timer/ITimerLog.ts and is consistently used across the codebase
  3. The POST response type handling matches the interface definition and is used consistently in related API calls
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check endpoint path consistency and response type handling

# Check for other usages of the timesheet/time-log endpoint
echo "Checking endpoint usage patterns:"
rg -A 2 "'/timesheet/time-log'" ./apps/web/

# Check TimesheetLog type usage in POST responses
echo "Checking response type handling:"
ast-grep --pattern 'post<$_>("/timesheet/time-log"'

Length of output: 819


Script:

#!/bin/bash
# Check TimesheetLog type definition and usage
echo "Checking TimesheetLog type definition:"
rg -p "type TimesheetLog" ./apps/web/

echo "Checking TimesheetLog interface definition:"
rg -p "interface TimesheetLog" ./apps/web/

echo "Checking other usages of TimesheetLog type:"
rg "TimesheetLog" ./apps/web/

Length of output: 5496

apps/web/app/hooks/features/useTimesheet.ts (3)

6-6: LGTM: Import and state initialization changes

The new imports and loading state are properly added to support the timesheet creation functionality.

Also applies to: 8-8, 107-107


285-287: LGTM: Hook return value updates

The new return values are properly added to expose the timesheet creation functionality to consumers of this hook.


142-158: 🛠️ Refactor suggestion

Enhance error handling and state management in createTimesheet

While the implementation is functional, there are several improvements that could be made:

 const createTimesheet = useCallback(
     async ({ ...timesheetParams }: UpdateTimesheet) => {
         if (!user) {
             throw new Error("User not authenticated");
         }
-        try {
-            const response = await queryCreateTimesheet(timesheetParams);
-            setTimesheet((prevTimesheet) => [
-                response.data,
-                ...(prevTimesheet || [])
-            ]);
-        } catch (error) {
-            throw error;
-        }
+        const response = await queryCreateTimesheet(timesheetParams);
+        if (!response.data) {
+            throw new Error("Invalid response data");
+        }
+        setTimesheet((prevTimesheet) => [
+            response.data,
+            ...(Array.isArray(prevTimesheet) ? prevTimesheet : [])
+        ]);
     },
     [queryCreateTimesheet, setTimesheet, user]
 );

Changes:

  1. Removed unnecessary try-catch block that just rethrows
  2. Added response data validation
  3. Added type checking for prevTimesheet

Likely invalid or redundant comment.

🧰 Tools
🪛 Biome (1.9.4)

[error] 154-154: The catch clause that only rethrows the original error is useless.

An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.

(lint/complexity/noUselessCatch)

apps/web/lib/features/integrations/calendar/table-time-sheet.tsx (3)

155-155: LGTM: Hook usage is properly updated

The addition of updateTimesheetStatus to the destructured hooks aligns with the PR's optimization objectives.


476-478: ⚠️ Potential issue

Add onClick handler for Delete action

The Delete menu item is missing the onClick handler to trigger the deletion dialog.

Apply this improvement:

-<DropdownMenuItem className="text-red-600 hover:!text-red-600 cursor-pointer">
+<DropdownMenuItem 
+  className="text-red-600 hover:!text-red-600 cursor-pointer"
+  onClick={() => setIsDialogOpen(true)}
+>
   {t('common.DELETE')}
 </DropdownMenuItem>

199-207: ⚠️ Potential issue

Add error handling and state management

The async status update implementation needs improvements:

  1. Error handling is missing
  2. No user feedback on success/failure
  3. State management after successful update is missing

Consider applying this improvement:

 const handleButtonClick = async (action: StatusAction) => {
   switch (action) {
     case 'Approved':
       if (selectTimesheet.length > 0) {
+        try {
           await updateTimesheetStatus({
             status: 'APPROVED',
             ids: selectTimesheet
-          })
+          });
+          setSelectTimesheet([]); // Reset selection
+          // Add success notification
+        } catch (error) {
+          console.error('Failed to update timesheet status:', error);
+          // Add error notification
+        }
       }
       break;

Likely invalid or redundant comment.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (1)
apps/web/app/hooks/features/useTimesheet.ts (1)

164-189: Consider these improvements to updateTimesheetStatus

While the implementation is solid, here are some suggested improvements:

  1. Add error handling for unauthenticated user (similar to other functions)
  2. Add type annotation for response data
  3. Consider removing or replacing console.log with proper logging
 const updateTimesheetStatus = useCallback(
     async ({ status, ids }: { status: TimesheetStatus; ids: ID[] | ID }) => {
-        if (!user) return;
+        if (!user) {
+            throw new Error("User not authenticated");
+        }
         const idsArray = Array.isArray(ids) ? ids : [ids];
         try {
-            const response = await queryUpdateTimesheetStatus({ ids: idsArray, status });
+            const response: { data: Array<{ id: string; status: TimesheetStatus }> } = 
+                await queryUpdateTimesheetStatus({ ids: idsArray, status });
             const responseMap = new Map(response.data.map(item => [item.id, item]));
             setTimesheet(prevTimesheet =>
                 prevTimesheet.map(item => {
                     const updatedItem = responseMap.get(item.timesheet.id);
                     if (updatedItem) {
                         return {
                             ...item,
                             timesheet: {
                                 ...item.timesheet,
                                 status: updatedItem.status
                             }
                         };
                     }
                     return item;
                 })
             );
-            console.log('Timesheet status updated successfully!');
+            return response.data;
         } catch (error) {
             console.error('Error updating timesheet status:', error);
+            throw error;
         }
     },
     [queryUpdateTimesheetStatus, setTimesheet, user]
 );
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between dbba956 and 050bdd4.

📒 Files selected for processing (1)
  • apps/web/app/hooks/features/useTimesheet.ts (6 hunks)
🔇 Additional comments (3)
apps/web/app/hooks/features/useTimesheet.ts (3)

6-6: LGTM: Import statement follows established patterns

The new import for createTimesheetFromApi is correctly placed with other API imports and follows the established naming convention.


248-251: LGTM: Efficient state update implementation

The state update implementation is correct and efficient:

  • Uses functional update pattern for state management
  • Correctly filters out deleted items
  • Maintains immutability

142-158: ⚠️ Potential issue

Enhance error handling in createTimesheet function

The error handling needs improvement:

  1. The error is only logged but not propagated
  2. The function's return type and value are undefined
  3. Success/failure state is not communicated to the caller

Apply this diff to improve error handling:

 const createTimesheet = useCallback(
     async ({ ...timesheetParams }: UpdateTimesheet) => {
         if (!user) {
             throw new Error("User not authenticated");
         }
         try {
             const response = await queryCreateTimesheet(timesheetParams);
             setTimesheet((prevTimesheet) => [
                 response.data,
                 ...(prevTimesheet || [])
             ]);
+            return response.data;
         } catch (error) {
             console.error('Error:', error);
+            throw error;
         }
     },
     [queryCreateTimesheet, setTimesheet, user]
 );

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (3)
apps/web/app/hooks/features/useTimesheet.ts (3)

164-191: Enhance error handling and add return value

The implementation is good but could be improved with better error handling and success tracking.

 async ({ status, ids }: { status: TimesheetStatus; ids: ID[] | ID }) => {
     if (!user) return;
     const idsArray = Array.isArray(ids) ? ids : [ids];
     try {
         const response = await queryUpdateTimesheetStatus({ ids: idsArray, status });
         const responseMap = new Map(response.data.map(item => [item.id, item]));
         setTimesheet(prevTimesheet =>
             prevTimesheet.map(item => {
                 const updatedItem = responseMap.get(item.timesheet.id);
                 if (updatedItem) {
                     return {
                         ...item,
                         timesheet: {
                             ...item.timesheet,
                             status: updatedItem.status
                         }
                     };
                 }
                 return item;
             })
         );
         console.log('Timesheet status updated successfully!');
+        return true;
     } catch (error) {
-        console.error('Error updating timesheet status:', error);
+        const errorMessage = error instanceof Error ? error.message : 'Unknown error occurred';
+        console.error(`Error updating timesheet status: ${errorMessage}`);
+        return false;
     }
 },

234-247: Add success logging and return value

The error handling is good, but let's add success logging and a return value for consistency.

 await queryDeleteTimesheet({
     organizationId: user.employee.organizationId,
     tenantId: user.tenantId ?? "",
     logIds
 });
 setTimesheet(prevTimesheet =>
     prevTimesheet.filter(item => !logIds.includes(item.timesheet.id))
 );
+console.log('Timesheets deleted successfully:', logIds);
+return true;

 } catch (error) {
     console.error('Failed to delete timesheets:', error);
     throw error;
 }

Line range hint 1-280: Consider splitting the hook for better maintainability

The hook is handling multiple responsibilities (CRUD operations, status management, grouping logic). Consider splitting it into smaller, more focused hooks:

  • useTimesheetOperations for CRUD operations
  • useTimesheetStatus for status management
  • useTimesheetGrouping for grouping logic

This would improve maintainability and make the code easier to test.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 050bdd4 and 0d621c1.

📒 Files selected for processing (1)
  • apps/web/app/hooks/features/useTimesheet.ts (6 hunks)
🔇 Additional comments (4)
apps/web/app/hooks/features/useTimesheet.ts (4)

6-6: LGTM: Import changes are well-organized

The new imports are properly structured and align with the added functionality.

Also applies to: 8-8


263-263: LGTM: Proper dependency management

Good addition of timesheetGroupByDays to the dependency array to ensure proper re-fetching when grouping changes.


142-158: 🛠️ Refactor suggestion

Enhance error handling and logging in createTimesheet

While the implementation is good, there are a few improvements needed:

 const createTimesheet = useCallback(
     async ({ ...timesheetParams }: UpdateTimesheet) => {
         if (!user) {
             throw new Error("User not authenticated");
         }
         try {
             const response = await queryCreateTimesheet(timesheetParams);
+            console.log('Timesheet created successfully:', response.data);
             setTimesheet((prevTimesheet) => [
                 response.data,
                 ...(prevTimesheet || [])
             ]);
+            return response.data;
         } catch (error) {
             console.error('Error:', error);
+            throw error;
         }
     },
     [queryCreateTimesheet, setTimesheet, user]
 );

276-278: Verify the usage of new return values

Let's verify that the new return values are properly used in the consuming components.

✅ Verification successful

New return values are properly integrated in the codebase

The verification shows that all new return values are properly used within the codebase:

  • createTimesheet is defined in useTimesheet.ts and used in API-related operations
  • loadingCreateTimesheet is correctly derived from the useQuery hook and exposed
  • puTimesheetStatus is properly integrated with the atom state management and filter options
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for components using the new return values
echo "Searching for createTimesheet usage..."
rg "createTimesheet" --type ts --type tsx

echo "Searching for loadingCreateTimesheet usage..."
rg "loadingCreateTimesheet" --type ts --type tsx

echo "Searching for puTimesheetStatus usage..."
rg "puTimesheetStatus" --type ts --type tsx

Length of output: 497


Script:

#!/bin/bash
# Search for components using the new return values with correct file type flags
echo "Searching for createTimesheet usage..."
rg "createTimesheet" -t typescript -t tsx

echo "Searching for loadingCreateTimesheet usage..."
rg "loadingCreateTimesheet" -t typescript -t tsx

echo "Searching for puTimesheetStatus usage..."
rg "puTimesheetStatus" -t typescript -t tsx

Length of output: 518


Script:

#!/bin/bash
# Search for components using the new return values without file type flags
echo "Searching for createTimesheet usage..."
rg "createTimesheet"

echo "Searching for loadingCreateTimesheet usage..."
rg "loadingCreateTimesheet"

echo "Searching for puTimesheetStatus usage..."
rg "puTimesheetStatus"

Length of output: 2073

@Innocent-Akim Innocent-Akim requested review from evereq and removed request for evereq November 27, 2024 09:05
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

🧹 Outside diff range and nitpick comments (7)
apps/web/app/services/server/requests/timesheet.ts (2)

112-120: Consider adding input validation and sanitization

While TypeScript provides type safety, consider adding runtime validation for the request payload:

  1. Validate required fields before making the request
  2. Sanitize input data to prevent potential injection attacks
  3. Consider picking only the necessary fields instead of spreading the entire params object

Example improvement:

 export function createTimesheetRequest(params: UpdateTimesheet, bearer_token: string) {
+  const { id, tenantId, organizationId, duration, ...timeLogData } = params;
+  if (!tenantId || !organizationId) {
+    throw new Error('Missing required fields: tenantId and organizationId are required');
+  }
   return serverFetch<TimesheetLog>({
     path: '/timesheet/time-log',
     method: 'POST',
-    body: { ...params },
+    body: { organizationId, tenantId, ...timeLogData },
     bearer_token,
     tenantId: params.tenantId
   })
 }

110-130: Add JSDoc documentation for new functions

Consider adding JSDoc comments for the new functions to maintain consistency with existing documentation (see ITimesheetProps documentation above). Include:

  • Function description
  • Parameter descriptions
  • Return type description
  • Example usage if applicable

Example:

/**
 * Creates a new timesheet entry
 * @param params - Timesheet creation parameters
 * @param bearer_token - Authentication token
 * @returns Promise resolving to the created TimesheetLog
 */
export function createTimesheetRequest(params: UpdateTimesheet, bearer_token: string)
apps/web/app/hooks/features/useTimesheet.ts (5)

6-6: Consider splitting long import line for better readability

The import line is quite long and could be split for better maintainability.

-import { deleteTaskTimesheetLogsApi, getTaskTimesheetLogsApi, updateStatusTimesheetFromApi, createTimesheetFromApi, updateTimesheetFromAPi } from '@/app/services/client/api/timer/timer-log';
+import {
+  deleteTaskTimesheetLogsApi,
+  getTaskTimesheetLogsApi,
+  updateStatusTimesheetFromApi,
+  createTimesheetFromApi,
+  updateTimesheetFromAPi
+} from '@/app/services/client/api/timer/timer-log';

136-152: Add return type and consider error propagation

The function implementation looks good, but could benefit from explicit typing and consistent error handling.

-    const createTimesheet = useCallback(
+    const createTimesheet = useCallback<(params: UpdateTimesheet) => Promise<void>>(
         async ({ ...timesheetParams }: UpdateTimesheet) => {
             if (!user) {
                 throw new Error("User not authenticated");
             }
             try {
                 const response = await queryCreateTimesheet(timesheetParams);
                 setTimesheet((prevTimesheet) => [
                     response.data,
                     ...(prevTimesheet || [])
                 ]);
             } catch (error) {
                 console.error('Error:', error);
+                throw error;
             }
         },
         [queryCreateTimesheet, setTimesheet, user]
     );

167-194: Consider removing console.log and returning success status

The implementation is good, but success feedback should be handled by the caller rather than logged to console.

     const updateTimesheetStatus = useCallback(
-        async ({ status, ids }: { status: TimesheetStatus; ids: ID[] | ID }) => {
+        async ({ status, ids }: { status: TimesheetStatus; ids: ID[] | ID }): Promise<boolean> => {
             if (!user) return;
             const idsArray = Array.isArray(ids) ? ids : [ids];
             try {
                 const response = await queryUpdateTimesheetStatus({ ids: idsArray, status });
                 const responseMap = new Map(response.data.map(item => [item.id, item]));
                 setTimesheet(prevTimesheet =>
                     prevTimesheet.map(item => {
                         const updatedItem = responseMap.get(item.timesheet.id);
                         if (updatedItem) {
                             return {
                                 ...item,
                                 timesheet: {
                                     ...item.timesheet,
                                     status: updatedItem.status
                                 }
                             };
                         }
                         return item;
                     })
                 );
-                console.log('Timesheet status updated successfully!');
+                return true;
             } catch (error) {
                 console.error('Error updating timesheet status:', error);
+                return false;
             }
         },
         [queryUpdateTimesheetStatus, setTimesheet, user]
     );

237-245: Remove unnecessary whitespace and improve error message

The implementation is good, but could use some minor cleanup.

             await queryDeleteTimesheet({
                 organizationId: user.employee.organizationId,
                 tenantId: user.tenantId ?? "",
                 logIds
             });
             setTimesheet(prevTimesheet =>
                 prevTimesheet.filter(item => !logIds.includes(item.timesheet.id))
-            );
-
+            );
         } catch (error) {
-            console.error('Failed to delete timesheets:', error);
+            console.error(`Failed to delete ${logIds.length} timesheet(s):`, error);
             throw error;
         }

278-284: Add explicit return type for the hook

Consider adding an explicit return type interface for better type safety and documentation.

interface UseTimesheetReturn {
  loadingTimesheet: boolean;
  timesheet: GroupedTimesheet[];
  getTaskTimesheet: (params: TimesheetParams) => void;
  loadingDeleteTimesheet: boolean;
  deleteTaskTimesheet: () => Promise<void>;
  getStatusTimesheet: (items?: TimesheetLog[]) => Record<TimesheetStatus, TimesheetLog[]>;
  timesheetGroupByDays: string;
  statusTimesheet: Record<TimesheetStatus, TimesheetLog[]>;
  updateTimesheetStatus: (params: { status: TimesheetStatus; ids: ID[] | ID }) => Promise<boolean>;
  loadingUpdateTimesheetStatus: boolean;
  puTimesheetStatus: any; // TODO: Add proper type
  createTimesheet: (params: UpdateTimesheet) => Promise<void>;
  loadingCreateTimesheet: boolean;
  updateTimesheet: (params: UpdateTimesheet) => Promise<void>;
  loadingUpdateTimesheet: boolean;
}
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between c3c2a26 and 60ab188.

📒 Files selected for processing (6)
  • apps/web/app/api/timer/timesheet/time-log/[id]/route.ts (1 hunks)
  • apps/web/app/hooks/features/useTimesheet.ts (6 hunks)
  • apps/web/app/interfaces/timer/ITimerLog.ts (4 hunks)
  • apps/web/app/services/client/api/timer/timer-log.ts (2 hunks)
  • apps/web/app/services/server/requests/timesheet.ts (2 hunks)
  • apps/web/lib/features/integrations/calendar/table-time-sheet.tsx (8 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • apps/web/app/interfaces/timer/ITimerLog.ts
  • apps/web/lib/features/integrations/calendar/table-time-sheet.tsx
🔇 Additional comments (6)
apps/web/app/api/timer/timesheet/time-log/[id]/route.ts (1)

1-4: LGTM! Imports are clean and relevant.

All imports are being used and are appropriate for the functionality.

apps/web/app/services/server/requests/timesheet.ts (2)

4-4: LGTM: Import changes are well-organized

The addition of UpdateTimesheet type to the existing imports is appropriate and maintains code organization.


122-130: Verify timesheet existence before update

The function should handle cases where the timesheet with the given ID doesn't exist. Consider:

  1. Adding error handling for 404 responses
  2. Verifying the timesheet exists before attempting update

Let's check how timesheet existence is verified in the codebase:

apps/web/app/services/client/api/timer/timer-log.ts (3)

1-1: LGTM: Import changes are appropriate

The addition of UpdateTimesheet type is consistent with the new functionality being added.


132-143: 🛠️ Refactor suggestion

Enhance error handling and add documentation

The error handling could be improved for better debugging:

+/**
+ * Creates a new timesheet log entry
+ * @param data - The timesheet data to be created
+ * @returns Promise<TimesheetLog>
+ * @throws Error if organizationId or tenantId are missing
+ * @throws Error if the API request fails
+ */
 export function createTimesheetFromApi(data: UpdateTimesheet) {
   const organizationId = getOrganizationIdCookie();
   const tenantId = getTenantIdCookie();
   if (!organizationId || !tenantId) {
     throw new Error('Required parameters missing: organizationId and tenantId are required');
   }
   try {
     return post<TimesheetLog>('/timesheet/time-log', { ...data, organizationId }, { tenantId })
   } catch (error) {
-    throw new Error('Failed to create timesheet log');
+    throw new Error(`Failed to create timesheet log: ${error instanceof Error ? error.message : 'Unknown error'}`);
   }
 }

132-157: Verify API endpoint configuration

Let's verify that the API endpoints for creating and updating timesheet logs are properly configured.

✅ Verification successful

Let me gather more information about the API endpoint configuration by checking the server-side implementation.


API endpoints are properly configured

The verification shows that both endpoints are properly configured and consistent with the codebase:

  • The /timesheet/time-log POST endpoint for creating timesheets is defined in apps/web/app/services/server/requests/timesheet.ts
  • The timesheet interfaces and types are properly defined in apps/web/app/interfaces/timer/ITimerLog.ts
  • The error handling pattern is consistent with other timesheet-related operations
  • The endpoints follow the same pattern as other timesheet endpoints (timer/start, timer/stop, etc.)
  • Required parameters (organizationId, tenantId) are handled consistently
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify API endpoint configuration for timesheet operations

# Check for route definitions
echo "Checking route definitions for timesheet endpoints..."
rg -A 5 "'/timesheet/time-log'" ./apps/web/

# Check for corresponding backend handlers
echo "Checking backend handlers..."
rg -A 5 "timesheet.*controller" ./apps/web/

# Check for consistent error handling patterns across timesheet-related functions
echo "Checking error handling patterns..."
ast-grep --pattern 'try {
  $$$
  return (post|put)<TimesheetLog>($$$)
  $$$
} catch (error) {
  $$$
}'

Length of output: 1405


Script:

#!/bin/bash
# Check for timesheet-related route configurations and controllers
echo "Checking for timesheet route configurations..."
rg -A 5 "timesheet.*router" ./apps/web/

# Check for timesheet-related API handlers or services
echo "Checking for timesheet API handlers..."
rg -A 5 "class.*TimesheetController" ./apps/web/

# Check for any other timesheet-related endpoints
echo "Checking for other timesheet endpoints..."
rg -A 5 "'/timesheet/.*'" ./apps/web/

# Check for timesheet-related types and interfaces
echo "Checking for timesheet types..."
rg -A 5 "interface.*Timesheet" ./apps/web/

Length of output: 11076

@Innocent-Akim Innocent-Akim added the Don't Merge Do not merge this PR until label not removed label Nov 27, 2024
@Innocent-Akim Innocent-Akim force-pushed the feat/create-timesheet-from-api branch from 60ab188 to 474f18b Compare November 27, 2024 17:57
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Outside diff range and nitpick comments (7)
apps/web/app/services/server/requests/timesheet.ts (3)

Line range hint 71-79: Fix malformed URL in getTaskTimesheetRequest

There's a syntax error in the URL construction. The activityLevel? appears to be incorrectly placed, resulting in a malformed URL:

`/timesheet/time-log?activityLevel?${queries.toString()}`

Apply this fix:

- path: `/timesheet/time-log?activityLevel?${queries.toString()}`,
+ path: `/timesheet/time-log?${queries.toString()}`,

112-120: Add JSDoc documentation for createTimesheetRequest

The function implementation looks good, but adding documentation would improve maintainability.

Add JSDoc documentation:

+/**
+ * Creates a new timesheet entry
+ * @param params - Timesheet creation parameters
+ * @param bearer_token - Authentication token
+ * @returns Promise<TimesheetLog> - Created timesheet entry
+ */
 export function createTimesheetRequest(params: UpdateTimesheet, bearer_token: string) {

122-130: Add JSDoc documentation for updateTimesheetRequest

The function implementation looks good, but adding documentation would improve maintainability.

Add JSDoc documentation:

+/**
+ * Updates an existing timesheet entry
+ * @param params - Timesheet update parameters including the timesheet ID
+ * @param bearer_token - Authentication token
+ * @returns Promise<TimesheetLog> - Updated timesheet entry
+ */
 export function updateTimesheetRequest(params: UpdateTimesheet, bearer_token: string) {
apps/web/lib/features/integrations/calendar/table-time-sheet.tsx (1)

231-231: Consider including the count of items being deleted

The generic description loses context about the number of items being deleted, which could be important information for the user.

Consider:

-description={`This action is irreversible. All related data will be lost.`}
+description={`This action is irreversible. ${selectTimesheet.length} item(s) and all related data will be lost.`}
apps/web/app/hooks/features/useTimesheet.ts (3)

6-6: Correct the inconsistent casing in 'updateTimesheetFromAPi'

The import updateTimesheetFromAPi has inconsistent casing in 'APi'. Consider renaming it to updateTimesheetFromApi for consistency with naming conventions.

Apply this diff to fix the casing:

-import { deleteTaskTimesheetLogsApi, getTaskTimesheetLogsApi, updateStatusTimesheetFromApi, createTimesheetFromApi, updateTimesheetFromAPi } from '@/app/services/client/api/timer/timer-log';
+import { deleteTaskTimesheetLogsApi, getTaskTimesheetLogsApi, updateStatusTimesheetFromApi, createTimesheetFromApi, updateTimesheetFromApi } from '@/app/services/client/api/timer/timer-log';

101-101: Ensure consistent casing in 'updateTimesheetFromAPi'

The variable updateTimesheetFromAPi used in useQuery has inconsistent casing. Rename it to updateTimesheetFromApi to match the corrected import.

Apply this diff:

 const { loading: loadingUpdateTimesheet, queryCall: queryUpdateTimesheet } = useQuery(
-    updateTimesheetFromAPi
+    updateTimesheetFromApi
 );

Line range hint 211-217: Simplify the 'isTimesheetStatus' function

The isTimesheetStatus function can be simplified by removing the unnecessary Object.values call since timesheetStatusValues is already an array.

Apply this diff:

 function isTimesheetStatus(status: unknown): status is TimesheetStatus {
     const timesheetStatusValues: TimesheetStatus[] = [
         "DRAFT",
         "PENDING",
         "IN REVIEW",
         "DENIED",
         "APPROVED"
     ];
-    return Object.values(timesheetStatusValues).includes(status as TimesheetStatus);
+    return timesheetStatusValues.includes(status as TimesheetStatus);
 }
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 60ab188 and 474f18b.

📒 Files selected for processing (8)
  • apps/web/app/api/timer/timesheet/time-log/[id]/route.ts (1 hunks)
  • apps/web/app/api/timer/timesheet/time-log/route.ts (1 hunks)
  • apps/web/app/hooks/features/useTimesheet.ts (6 hunks)
  • apps/web/app/interfaces/timer/ITimerLog.ts (4 hunks)
  • apps/web/app/services/client/api/timer/timer-log.ts (2 hunks)
  • apps/web/app/services/server/requests/timesheet.ts (2 hunks)
  • apps/web/app/stores/time-logs.ts (2 hunks)
  • apps/web/lib/features/integrations/calendar/table-time-sheet.tsx (7 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
  • apps/web/app/api/timer/timesheet/time-log/[id]/route.ts
  • apps/web/app/api/timer/timesheet/time-log/route.ts
  • apps/web/app/interfaces/timer/ITimerLog.ts
  • apps/web/app/stores/time-logs.ts
🔇 Additional comments (10)
apps/web/app/services/server/requests/timesheet.ts (2)

4-4: LGTM: Import statement is properly structured

The import statement correctly includes the new UpdateTimesheet type needed for the new functions.


Line range hint 81-90: Verify DELETE endpoint's handling of multiple IDs

The current implementation joins all IDs with commas, which might hit URL length limits for bulk deletions. We should verify the API's limitations.

apps/web/app/services/client/api/timer/timer-log.ts (4)

1-2: LGTM: Import statements are correctly updated

The new imports UpdateTimesheet and post are appropriately added to support the new timesheet management functions.


132-143: 🛠️ Refactor suggestion

Enhance error handling and add documentation

The function needs the following improvements:

  1. Add JSDoc documentation
  2. Validate the data parameter
  3. Include the original error message in the catch block

Apply this diff:

+/**
+ * Creates a new timesheet log entry
+ * @param data - The timesheet data to be created
+ * @returns Promise<TimesheetLog>
+ * @throws Error if organizationId or tenantId are missing
+ * @throws Error if the API request fails
+ */
 export function createTimesheetFromApi(data: UpdateTimesheet) {
+  if (!data) {
+    throw new Error('Required parameter missing: data is required');
+  }
   const organizationId = getOrganizationIdCookie();
   const tenantId = getTenantIdCookie();
   if (!organizationId || !tenantId) {
     throw new Error('Required parameters missing: organizationId and tenantId are required');
   }
   try {
     return post<TimesheetLog>('/timesheet/time-log', { ...data, organizationId }, { tenantId })
   } catch (error) {
-    throw new Error('Failed to create timesheet log');
+    throw new Error(`Failed to create timesheet log: ${error instanceof Error ? error.message : 'Unknown error'}`);
   }
 }

145-157: ⚠️ Potential issue

Fix function naming and error handling issues

The function has several issues that need to be addressed:

  1. Inconsistent naming (APi vs Api)
  2. Incorrect error message (mentions "create" instead of "update")
  3. Missing validation for the required id parameter
  4. Missing JSDoc documentation

These issues were previously identified in past review comments. Please refer to the existing review comment for the suggested fixes.


132-157: Verify integration with consuming components

Let's ensure that components using these new functions properly handle errors and required parameters.

✅ Verification successful

Integration verified with proper error handling and parameter usage

The code inspection reveals that the new timesheet functions are properly integrated:

  • The useTimesheet hook correctly implements both functions with proper error handling and required parameters
  • The functions are used within a proper try-catch block with error logging
  • Required parameters (organizationId, tenantId) are properly handled through the authenticated user context
  • The state management is properly implemented using Jotai atoms
  • The component layer (TimesheetAction) properly handles the various states and actions
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check the usage and error handling of the new timesheet functions

# Check for components using these functions
echo "Checking usage of new timesheet functions..."
rg -t typescript -t tsx "createTimesheetFromApi|updateTimesheetFromAPi" ./apps/web/

# Check error handling in components
echo "Checking error handling implementation..."
ast-grep --pattern 'try {
  $$$
  (createTimesheetFromApi|updateTimesheetFromAPi)($$$)
  $$$
} catch (error) {
  $$$
}'

# Check for potential breaking changes
echo "Checking for potential breaking changes..."
rg -t typescript -t tsx "timesheet.*api" ./apps/web/

Length of output: 619


Script:

#!/bin/bash
# Let's try with correct file extensions and simpler patterns

# Check usage of the new functions
echo "Checking usage of timesheet functions..."
rg "createTimesheetFromApi|updateTimesheetFromAPi" --type ts --type tsx

# Check for timesheet-related API calls
echo "Checking timesheet API patterns..."
rg "timesheet.*api" --type ts --type tsx

# Look for files that might use these functions
echo "Looking for timesheet-related files..."
fd -e ts -e tsx timesheet

Length of output: 1295


Script:

#!/bin/bash
# Let's examine the content of relevant files

# Check the hook implementation
echo "Checking useTimesheet hook..."
cat "apps/web/app/hooks/features/useTimesheet.ts"

# Check task timesheet store
echo "Checking task timesheet store..."
cat "apps/web/app/stores/task-timesheet.ts"

# Check timesheet components for API usage
echo "Checking TimesheetAction component..."
cat "apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetAction.tsx"

# Check timesheet interfaces
echo "Checking timesheet interfaces..."
cat "apps/web/app/interfaces/ITaskTimesheet.ts"

Length of output: 15679

apps/web/lib/features/integrations/calendar/table-time-sheet.tsx (2)

489-491: Missing onClick handler for Delete action

The Delete menu item lacks an onClick handler to trigger the deletion dialog.


207-215: ⚠️ Potential issue

Add error handling for the async operation

The async operation lacks error handling which could lead to unhandled promise rejections.

Consider implementing try-catch:

 const handleButtonClick = async (action: StatusAction) => {
   switch (action) {
     case 'Approved':
       if (selectTimesheet.length > 0) {
+        try {
           await updateTimesheetStatus({
             status: 'APPROVED',
             ids: selectTimesheet
           })
+        } catch (error) {
+          console.error('Failed to update timesheet status:', error);
+          // Add error notification/feedback to user
+        }
       }
       break;

Likely invalid or redundant comment.

apps/web/app/hooks/features/useTimesheet.ts (2)

147-149: Enhance error handling in 'createTimesheet' function

In the createTimesheet function, errors are only logged to the console without notifying the caller. Consider throwing the error after logging so that the calling code can handle it appropriately.

Apply this diff:

} catch (error) {
    console.error('Error:', error);
+   throw error;
}

156-163: Add error handling and fix potential state update issues in 'updateTimesheet' function

The updateTimesheet function lacks error handling and might cause duplicates in the state. Include a try-catch block and update the state to prevent duplicates.

Apply this diff:

 const updateTimesheet = useCallback(async ({ ...timesheet }: UpdateTimesheet) => {
-    if (!user) return;
+    if (!user) {
+        throw new Error("User not authenticated");
+    }
+    try {
         const response = await queryUpdateTimesheet(timesheet);
-        setTimesheet(prevTimesheet => [
-            response.data,
-            ...prevTimesheet,
-        ])
+        setTimesheet(prevTimesheet =>
+            prevTimesheet.map(item =>
+                item.timesheet.id === response.data.timesheet.id
+                    ? response.data
+                    : item
+            )
+        );
+    } catch (error) {
+        console.error('Error updating timesheet:', error);
+        throw error;
+    }
 }, [queryUpdateTimesheet, setTimesheet, user]);

@evereq evereq merged commit 4eccf23 into develop Nov 27, 2024
10 of 13 checks passed
@evereq evereq deleted the feat/create-timesheet-from-api branch November 27, 2024 18:03
@Innocent-Akim Innocent-Akim added bug Something isn't working WEB Web app and removed Don't Merge Do not merge this PR until label not removed labels Nov 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Ever Teams WEB Web app
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants