Version 10.0.0 beta04#2384
Conversation
…2383) * fix(auth): validate display name only when required in sign-up form * fix(auth): use locale-safe matcher for sign-up button in test
There was a problem hiding this comment.
Code Review
This pull request updates the form validation logic in SignUpUI.kt to only validate the display name if it is required by the provider, and adds a corresponding unit test in SignUpUITest.kt to verify this behavior. Feedback was provided regarding the use of derivedStateOf for isFormValid, which is an anti-pattern here because the inputs are plain strings rather than Compose State objects, leading to unnecessary allocations. Additionally, calling validate() inside the block causes side-effects during composition. It is recommended to simplify isFormValid to a plain Boolean using remember and update the button's enabled state accordingly.
| val isFormValid = remember(displayName, email, password, confirmPassword) { | ||
| derivedStateOf { | ||
| listOf( | ||
| displayNameValidator.validate(displayName), | ||
| !provider.isDisplayNameRequired || displayNameValidator.validate(displayName), |
There was a problem hiding this comment.
Unnecessary derivedStateOf and Side-Effects during Composition
There are two main issues with the current implementation of isFormValid:
-
derivedStateOfAnti-Pattern:
derivedStateOfis designed to derive state from other ComposeStateobjects (likeMutableState). Here, the inputs (displayName,email,password,confirmPassword) are plainStringparameters, not ComposeStates. Because they are plain values, Compose cannot track them inside thederivedStateOfblock. To make it work, they are passed as keys toremember. However, when any of these keys change, the entirerememberblock is re-executed, creating a brand newderivedStateOfinstance on every keystroke. This completely defeats the purpose ofderivedStateOfand adds unnecessary allocation and registration overhead. -
Side-Effects during Composition:
Inside thederivedStateOfblock,validate()is called on the validators (e.g.,displayNameValidator.validate(displayName)). Thevalidate()function is not pure; it mutates the internal state (_validationStatus) of the validator. Running side-effects inside a composition/calculation block is highly discouraged in Compose as it can lead to inconsistent UI states or unexpected behavior.
Recommended Solution
Simplify isFormValid to a plain Boolean using remember with keys, and update its usage in the Button to remove .value:
// 1. Simplify isFormValid to a plain Boolean
val isFormValid = remember(displayName, email, password, confirmPassword) {
listOf(
!provider.isDisplayNameRequired || displayNameValidator.validate(displayName),
emailValidator.validate(email),
passwordValidator.validate(password),
confirmPasswordValidator.validate(confirmPassword)
).all { it }
}
// 2. Update the Button's enabled state (around line 203) to use isFormValid directly:
Button(
onClick = { onSignUpClick() },
enabled = !isLoading && isFormValid, // Removed .value
) {
// ...
}
No description provided.