Tweaks for bulk group creation flow and better error handling#1722
Tweaks for bulk group creation flow and better error handling#1722
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -1,3 +1,4 @@ | |||
| import _ from 'lodash' | |||
There was a problem hiding this comment.
[performance]
The use of lodash for deep property access (e.g., _.get) is convenient, but consider if it's necessary to import the entire library for this functionality. If lodash is not extensively used elsewhere, it might be more efficient to use optional chaining and nullish coalescing operators available in modern JavaScript.
| return validationResults | ||
| } catch (error) { | ||
| const errorDetails = (error && (error.message || error.toString())) || 'Unknown error' | ||
| const errorDetails = getApiErrorMessage(error, 'Unable to validate members.') |
There was a problem hiding this comment.
[correctness]
The getApiErrorMessage function is used to extract error details, which is a good practice for consistent error handling. However, ensure that this function is thoroughly tested, especially with various error object structures, to avoid unexpected behavior in production.
| return createdGroup | ||
| } catch (error) { | ||
| const errorDetails = (error && (error.message || error.toString())) || 'Unknown error' | ||
| const errorDetails = getApiErrorMessage(error, 'Unable to create group.') |
There was a problem hiding this comment.
[correctness]
Similar to the bulkSearchMembers function, ensure that the getApiErrorMessage function is robust against different error object structures to prevent misleading error messages from being displayed to the user.
| }) | ||
|
|
||
| this.props.bulkSearchUsers(parsedHandlesEmails) | ||
| this.props.bulkSearchUsers(parsedHandlesEmails).catch(() => null) |
There was a problem hiding this comment.
[maintainability]
Swallowing errors by using .catch(() => null) can make debugging difficult and might hide issues that need attention. Consider logging the error or handling it in a way that provides more visibility into what went wrong.
| selfRegister, | ||
| privateGroup | ||
| ) | ||
| ).catch(() => null) |
There was a problem hiding this comment.
[maintainability]
Using .catch(() => null) to ignore errors can lead to silent failures. It's advisable to at least log the error or handle it in a way that allows for easier debugging and maintenance.
1 participant
No description provided.