[fix] Update Task undefined array methods #8576
Conversation
WalkthroughThe changes in this pull request focus on the Changes
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
packages/core/src/tasks/task.service.ts (4)
97-101: LGTM! Good defensive programming to handle undefined arrays.The changes correctly prevent "undefined array" errors by providing empty array fallbacks. This is a good fix for the issue.
Consider extracting these operations into a helper method for better reusability and cleaner code:
-const memberIdSet = new Set((data.members || []).map(({ id }) => id)); -const existingMemberIdSet = new Set((taskMembers || []).map(({ id }) => id)); - -const removedMembers = (taskMembers || []).filter((member) => !memberIdSet.has(member.id)); -const newMembers = (data.members || []).filter((member) => !existingMemberIdSet.has(member.id)); +const getMemberDifferences = (newMembers = [], existingMembers = []) => { + const newIdSet = new Set(newMembers.map(({ id }) => id)); + const existingIdSet = new Set(existingMembers.map(({ id }) => id)); + + return { + removedMembers: existingMembers.filter((member) => !newIdSet.has(member.id)), + newMembers: newMembers.filter((member) => !existingIdSet.has(member.id)) + }; +}; + +const { removedMembers, newMembers } = getMemberDifferences(data.members, taskMembers);
Line range hint
192-195: Improve error handling to avoid redundancy and potential security issues.The current error handling has a few issues:
- The error message is logged twice (
${error.message}anderror.message).- Exposing the raw error object in the response could leak sensitive information.
Consider this improved error handling:
-console.error(`Error while updating task: ${error.message}`, error.message); -throw new HttpException({ message: error?.message, error }, HttpStatus.BAD_REQUEST); +console.error('Error while updating task:', error); +throw new HttpException( + { message: 'Failed to update task. Please try again.' }, + HttpStatus.BAD_REQUEST +);
Line range hint
156-187: Enhance error handling for subscription operations.The current implementation silently catches errors during subscription operations, which could lead to inconsistent states where some operations succeed while others fail.
Consider implementing a more robust error handling strategy:
-try { - await Promise.all( - removedMembers.map( - async (member) => - await this._subscriptionService.delete({ - entity: BaseEntityEnum.Task, - entityId: updatedTask.id, - userId: member.userId, - type: SubscriptionTypeEnum.ASSIGNMENT - }) - ) - ); -} catch (error) { - console.error('Error unsubscribing members from the task:', error); -} +const unsubscribeResults = await Promise.allSettled( + removedMembers.map((member) => + this._subscriptionService.delete({ + entity: BaseEntityEnum.Task, + entityId: updatedTask.id, + userId: member.userId, + type: SubscriptionTypeEnum.ASSIGNMENT + }) + ) +); + +const failedUnsubscribes = unsubscribeResults + .filter((result): result is PromiseRejectedResult => result.status === 'rejected') + .map((result) => result.reason); + +if (failedUnsubscribes.length > 0) { + console.error('Failed to unsubscribe some members:', failedUnsubscribes); + // Optionally: Throw an error or handle the partial failure +}Similar improvements should be applied to the new member subscription logic.
Line range hint
188-191: Address the TODO comment about actor type determination.The current implementation hardcodes the actor type as 'User', but the TODO comment indicates a need to support system-initiated actions (e.g., from GitHub integration).
Would you like me to help implement a dynamic actor type determination? This could include:
- Creating an enum for different actor sources
- Adding a service to determine the actor type based on the request context
- Updating the activity logging to use the dynamic actor type
☁️ Nx Cloud ReportCI is running/has finished running commands for commit 7cdfef4. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this CI Pipeline Execution ✅ Successfully ran 1 targetSent with 💌 from NxCloud. |
rahul-rocket
deleted the
fix/coderrabitai-undefined-values-task-update
branch
PR
Please note: we will close your PR without comment if you do not check the boxes above and provide ALL requested information.
Summary by CodeRabbit
Bug Fixes
Refactor
update,findById,getMyTasks, andgetAllTasksByEmployeemethods to better handle member-related data.