Initial handle change ipmlementation to use new member-api endpoint #1432
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| gap: 6px; | ||
| } | ||
|
|
||
| .dialogLoadingSpinnerContainer { |
There was a problem hiding this comment.
[design]
Using position: absolute for .dialogLoadingSpinnerContainer may cause layout issues if the parent container's dimensions or position change. Consider using a more flexible layout strategy if the spinner's position is not fixed relative to the viewport or other elements.
| props.setOpen(false) | ||
| } | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [isLoading]) |
There was a problem hiding this comment.
[correctness]
The handleClose function depends on isLoading, but props.setOpen is also used inside it. Consider adding props.setOpen to the dependency array to ensure the function is updated correctly if props.setOpen changes.
| setShowConfirm(false) | ||
| setIsLoading(true) | ||
| const data = getValues() | ||
| const nextHandle = (data.newHandle ?? '').trim() |
There was a problem hiding this comment.
[💡 maintainability]
The nextHandle is trimmed before being passed to changeUserHandle, but the form validation schema should ensure that the handle is already in the correct format. Consider validating and trimming the handle in the schema to ensure consistency and avoid redundant operations.
| .then(result => { | ||
| setIsLoading(false) | ||
| toast.success('Handle updated successfully') | ||
| props.userInfo.handle = result?.handle ?? nextHandle |
There was a problem hiding this comment.
[design]
Directly mutating props.userInfo.handle could lead to unexpected behavior if userInfo is used elsewhere. Consider using a state update or a callback to update the handle in a more controlled manner.
| @@ -309,6 +313,8 @@ export const UsersTable: FC<Props> = props => { | |||
| function onSelectOption(item: string): void { | |||
There was a problem hiding this comment.
[maintainability]
Consider using a switch statement instead of multiple else if conditions for better readability and maintainability. This will make it easier to add or modify options in the future.
| newHandle: string, | ||
| ): Promise<MemberInfo> => { | ||
| const payload = { | ||
| newHandle: newHandle.trim(), |
There was a problem hiding this comment.
[correctness]
Consider validating newHandle before trimming and sending it in the payload. This could prevent sending invalid data to the API and improve error handling.
| } | ||
|
|
||
| return xhrPatchAsync<typeof payload, MemberInfo>( | ||
| `${EnvironmentConfig.API.V6}/members/${encodeURIComponent(handle)}/change_handle`, |
There was a problem hiding this comment.
[❗❗ security]
Ensure that handle is validated before being used in the URL. This prevents potential issues with malformed URLs or unintended API calls.
| */ | ||
| export const formEditUserHandleSchema: Yup.ObjectSchema<FormEditUserHandle> | ||
| = Yup.object({ | ||
| newHandle: Yup.string() |
There was a problem hiding this comment.
[correctness]
Consider adding a .min(3, 'Handle must be at least 3 characters long.') constraint to ensure the handle is not too short, which could help prevent potential issues with handle uniqueness or readability.
Rush on this one due to support request