Enhancement: Resolve [604] Email Field Shouldn't Allow Spaces #613
Conversation
|
@anthonyheitzeberg is attempting to deploy a commit to the Listinai Team on Vercel. A member of the Team first needs to authorize it. |
|
Warning Rate limit exceeded@anthonyheitzeberg has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 22 minutes and 54 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe pull request updates the email input behavior by introducing an explicit Changes
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 (
|
|
Thank you! Also, the type I will let other people comment on what they think :) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/frontend/src/components/settings/teams.component.tsx(1 hunks)libraries/react-shared-libraries/src/form/input.tsx(3 hunks)
🔇 Additional comments (2)
libraries/react-shared-libraries/src/form/input.tsx (1)
29-30: LGTM! Type prop addition looks good.The type prop is correctly destructured from props and will be useful for input-specific behaviors.
apps/frontend/src/components/settings/teams.component.tsx (1)
108-113:✅ Verification successful
LGTM! Email input configuration looks good.
The type="email" attribute is correctly added to the Input component.
Let's check if AddTeamMemberDto already includes email validation:
🏁 Script executed:
#!/bin/bash # Search for AddTeamMemberDto definition and its validation decorators ast-grep --pattern 'export class AddTeamMemberDto { $$$ email$$$ $$$ }'Length of output: 79
Please verify email validation in AddTeamMemberDto.
- The
Inputcomponent changes (lines 108-113 inapps/frontend/src/components/settings/teams.component.tsx) correctly addtype="email"—this aligns with validation expectations on the frontend.- Automated searches did not return clear evidence of email validation (e.g., via
@IsEmaildecorators) in theAddTeamMemberDtodefinition. Please manually inspect or updateAddTeamMemberDtoto ensure that email inputs are suitably validated.
🏁 Script executed:
[run_scripts] #!/bin/bash # 1. Print out context around the definition of AddTeamMemberDto echo "Searching for 'class AddTeamMemberDto' with context:" rg -n "class AddTeamMemberDto" -C 10 . # 2. Look for any email validation decorator usage (e.g., '@IsEmail') in the repository: echo "Searching for '@IsEmail' in the repository:" rg -n "@IsEmail" .Length of output: 1949
LGTM! Email input configuration and validation are correctly implemented.
- The
Inputcomponent inapps/frontend/src/components/settings/teams.component.tsxnow correctly usestype="email".- Backend validation is confirmed in
AddTeamMemberDto(inlibraries/nestjs-libraries/src/dtos/settings/add.team.member.dto.ts), where the@IsEmail()decorator validates the email field.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
Hello @nevo-david thank you for your feedback, so I understand it would be better to use the preferred react-form-hook and have applied the suggested changes of code rabbit. Edit: still working on getting this to work, my bad |
anthonyheitzeberg
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
libraries/react-shared-libraries/src/form/input.tsx (1)
47-48: Remove unused email pattern.The email pattern can be moved inside the registration options since it's only used there.
Apply this diff:
-const emailPattern = /^[^\s@]+@[^\s@]+\.[^\s@]+$/; -
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
libraries/react-shared-libraries/src/form/input.tsx (1)
10-20:⚠️ Potential issueAdd type prop to component interface.
The component interface is missing the
typeprop which is needed for proper email validation.Update the interface:
export const Input: FC< DetailedHTMLProps<InputHTMLAttributes<HTMLInputElement>, HTMLInputElement> & { removeError?: boolean; error?: any; disableForm?: boolean; customUpdate?: () => void; label: string; name: string; + type?: string; icon?: ReactNode; } > = (props) => {
♻️ Duplicate comments (1)
libraries/react-shared-libraries/src/form/input.tsx (1)
61-65: 🛠️ Refactor suggestionSwitch to react-hook-form's built-in validation instead of keyDown event.
The current implementation only prevents typing spaces but doesn't handle paste events or trailing spaces. Using react-hook-form's built-in validation would be more robust.
Replace the keyDown handler with react-hook-form's built-in validation:
-onKeyDown={(e) => { - if (props.name === 'email' && e.key === ' ') { - e.preventDefault(); - } -}}And update the form registration to use built-in validation:
{...(disableForm ? {} : form.register(props.name, { ...(props.type === 'email' ? { pattern: { value: /^[^\s@]+@[^\s@]+\.[^\s@]+$/, message: 'Please enter a valid email address' }, setValueAs: (value: string) => value?.trim() } : {}) }))}
|
Hello @nevo-david, have verified that the type field exist on all email fields in the frontend, resorted to still use onkeydown to prevent the field from even receiving a space at all so no need to set the form value. Thanks for the approval @egelhaus! It did try using the onChange and setValueAs in the register options of the hook but it wasn't a smooth experience as using the onKeydown. |
|
./src/components/billing/lifetime.deal.tsx:176:16
|
|
I will be trying this again on a different PR. |
|
You add an extra even listener to every input on the page in exchange for preventing a space in the email (that doesn't pass validation anyway from the dto) (It would have been better to add the onKeyDown, only the type is email) I am closing it for now |
|
Hey, this hasn't been an Issue on a different PR, so this PR is the Issue. |
|
@nevo-david I wouldn't close it, I would make it an Draft. But your choice. |
|
Okay I see @nevo-david, i think i understand what you mean, I can open another PR , but reopening this is also fine. |
|
I closed the issue. |
|
Alright thats true, thank you @nevo-david @egelhaus, will try another issue and play around the project. |
anthonyheitzeberg
deleted the
bugfix/604-email-field-no-spaces-allowed
branch
What kind of change does this PR introduce?
Bug Fix, but this seems more like a UX enhancement regarding email input field
Why was this change needed?
Addressing this UX issue brought up here: Email Field Shouldn't Allow Spaces Issue # 604
Other information:
eg: Did you discuss this change with anybody before working on it (not required, but can be a good idea for bigger changes). Any plans for the future, etc?
Checklist:
Summary by CodeRabbit