feat: Add validation for federation name and dynamic warning message … #512
Conversation
WalkthroughWalkthroughThe changes introduce validation logic for user input in the configuration forms, specifically for the user's name and federation name. This includes dynamically enabling or disabling the "Next" button based on the validity of the inputs. The localization file for English language settings is updated to support these changes. Changes
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (4)
Additional comments not posted (2)
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}
| const [nameHelpText, setNameHelpText] = useState<string | null>(null); | ||
|
|
||
| const validateFederationName = useCallback( | ||
| (name: string) => { | ||
| if (name.trim() === '') { | ||
| setNameHelpText(t('set-config.give-federation-name')); | ||
| } else { | ||
| setNameHelpText(null); | ||
| } | ||
| }, | ||
| [t] | ||
| ); |
There was a problem hiding this comment.
Refine validation logic for federation name.
The current implementation only checks for an empty string. Consider adding a regex pattern to ensure the federation name meets specific criteria.
Propose an enhancement to the validation logic:
- if (name.trim() === '') {
+ if (name.trim() === '' || !/^[a-zA-Z0-9-]+$/.test(name)) {Also applies to: 55-57
| @@ -38,7 +38,7 @@ export const BasicSettingsForm: React.FC<BasicSettingsFormProps> = ({ | |||
|
|
|||
| const validateName = useCallback( | |||
| (name: string) => { | |||
| const isValid = name.trim() !== '' && /^[a-zA-Z0-9]/.test(name); | |||
| const isValid = name.trim() !== 'set-config.validation-guardian-name'; | |||
There was a problem hiding this comment.
I'm fairly sure this isn't doing regex matching anymore. Not a TS/JS expert, but looks wrong. cc @Kodylow
There was a problem hiding this comment.
yeah this is broken, this check is doing "does the name match the hard coded value in "set-config": {"validation-guardian-name": VALUE}
|
can you post pictures / videos of any of the UI changes you make? |
|
yes @Kodylow, it is only to require the federation name in the leader configuration. If this is not required, it can be omitted |
|
kk can you rebase then I'll test and land |
yes sure |
Kodylow
force-pushed
the
federation-settings-improvements
branch
from
29d6643
to
e6da08e
Compare
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
| @@ -207,6 +207,7 @@ | |||
| "bft-explanation": "A {{total}} Guardian Federation creates a {{honest}}/{{total}} Threshold.", | |||
| "bft-faulty": "Your Federation will be able to tolerate {{faulty}} faulty, offline, or malicious Guardians without impacting the Federation's operations.", | |||
| "guardian-name": "Guardian name", | |||
| "validation-guardian-name": "'' && /^[a-zA-Z0-9]/", | |||
There was a problem hiding this comment.
Validation rule for guardian name needs correction.
The validation rule for the guardian name appears to be incorrect. The expression starts with an empty string followed by a logical AND operator, which doesn't make sense in this context. It seems like an attempt to ensure the name starts with an alphanumeric character, but the syntax is flawed.
Consider revising the validation rule to correctly implement the intended functionality. Here's a corrected version:
"validation-guardian-name": "/^[a-zA-Z0-9].*/"This ensures the name starts with an alphanumeric character and can be followed by any characters.
| @@ -22,18 +22,32 @@ interface BasicSettingsFormProps { | |||
| setMyName: (name: string) => void; | |||
| password: string | null; | |||
| setPassword: (password: string) => void; | |||
| isNextDisabled: (disabled: boolean) => void; | |||
There was a problem hiding this comment.
Approve addition of isNextDisabled but refine validation logic.
The addition of isNextDisabled to the BasicSettingsFormProps interface and its usage in the component are approved as they enhance control flow. However, the current validation logic in validateName only checks if the name is not equal to a specific string, which is not robust. Consider using a regex pattern to ensure the name meets specific criteria, such as being alphanumeric.
Consider enhancing the validation logic:
- const isValid = name.trim() !== 'set-config.validation-guardian-name';
+ const isValid = /^[a-zA-Z0-9]+$/.test(name.trim());Also applies to: 33-33, 39-49
| const [nameHelpText, setNameHelpText] = useState<string | null>(null); | ||
|
|
||
| const validateFederationName = useCallback( | ||
| (name: string) => { | ||
| if (name.trim() === '') { | ||
| setNameHelpText(t('set-config.give-federation-name')); | ||
| } else { | ||
| setNameHelpText(null); | ||
| } | ||
| }, | ||
| [t] | ||
| ); | ||
|
|
||
| useEffect(() => { | ||
| validateFederationName(federationName); | ||
| }, [federationName, validateFederationName]); |
There was a problem hiding this comment.
Approve real-time feedback but refine validation logic.
The implementation of real-time feedback using validateFederationName and its invocation on the onBlur event are approved. However, the validation logic should be enhanced to include checks for valid characters, not just non-empty input.
Consider enhancing the validation logic:
- if (name.trim() === '') {
+ if (name.trim() === '' || !/^[a-zA-Z0-9-]+$/.test(name)) {Also applies to: 72-80
| @@ -70,6 +70,7 @@ export const SetConfiguration: React.FC<Props> = ({ next }: Props) => { | |||
| stateNumPeers ? stateNumPeers.toString() : isSolo ? '1' : MIN_BFT_NUM_PEERS | |||
| ); | |||
|
|
|||
| const [isNextDisabled, setIsNextDisabled] = useState(true); | |||
There was a problem hiding this comment.
Refine validation logic and approve state management integration.
The integration of isNextDisabled state management based on the federation name validation is approved. However, the regex pattern used in validateFederationName is incomplete and should be enhanced to ensure it checks the entire string rather than just the beginning.
Consider enhancing the validation logic:
- const isValid = name.trim() !== '' && /^[a-zA-Z0-9]/.test(name);
+ const isValid = /^[a-zA-Z0-9]+$/.test(name.trim());Also applies to: 121-125, 159-159, 264-267
|
just tried this I don't really like it, this should be part of a larger refactor to using Toasts for all the errors and warnings instead of just leaving them on the fields/screens continuously |
|
cc @Kodylow thanks for your feedback. |
Implement validation for federation name and add dynamic warning messages
Description:
Implemented federation name validation in SetConfiguration.tsx and BasicSettingsForm.tsx.
Added a dynamic warning message in FederationSettingsForm.tsx to alert when the federation name field is empty.
These improvements ensure that users provide a valid federation name before proceeding with the configuration.
Summary by CodeRabbit
New Features
Documentation