feat: add trailing whitespace warning on login username field (#2040)#2177
feat: add trailing whitespace warning on login username field (#2040)#2177GeoffreyCoulaud wants to merge 1 commit intoseerr-team:developfrom
Conversation
GeoffreyCoulaud
force-pushed
the
warn-whitespace-after-username
branch
from
b0f9812 to
2923af0
Compare
|
This pull request has merge conflicts. Please resolve the conflicts so the PR can be successfully reviewed and merged. |
GeoffreyCoulaud
force-pushed
the
warn-whitespace-after-username
branch
from
2923af0 to
05a9d95
Compare
github-actions
bot
removed
the
merge conflict
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe changes add visual warnings in login forms to alert users when their username or email field contains trailing whitespace. This includes importing icon components, adding internationalized warning messages, updating UI to display warnings conditionally, and introducing a corresponding CSS styling class. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/components/Login/JellyfinLogin.tsx (1)
76-79: Consider trimming the username before submission for consistency.Similar to the LocalLogin component, the username value is passed to the API without trimming. For consistency and to fully address issue
#2040:💡 Suggested enhancement
await axios.post('/api/v1/auth/jellyfin', { - username: values.username, + username: values.username.trim(), password: values.password, - email: values.username, + email: values.username.trim(), });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Login/JellyfinLogin.tsx` around lines 76 - 79, The JellyfinLogin component sends values.username directly to the API; trim the username before submission so the payload uses a trimmed value for username and email. Update the axios.post call in JellyfinLogin (the object containing username, password, email) to use values.username.trim() (and assign the trimmed value to both username and email) so the API receives a consistent, whitespace-free username.src/components/Login/LocalLogin.tsx (1)
59-61: Consider trimming the email/username before submission.The original issue
#2040requests that usernames be trimmed before login to prevent failures caused by trailing spaces (especially from mobile keyboard autocomplete). The current implementation only warns users but still sends the untrimmed value to the API.Consider also trimming the value before submission to fully address the UX issue:
💡 Suggested enhancement
try { await axios.post('/api/v1/auth/local', { - email: values.email, + email: values.email.trim(), password: values.password, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Login/LocalLogin.tsx` around lines 59 - 61, The submitted email/username is not being trimmed before the POST, so trailing spaces can cause login failures; update the payload sent in the function that calls axios.post('/api/v1/auth/local') (in LocalLogin.tsx) to send trimmed values (e.g., use values.email.trim() and/or values.username?.trim()) instead of the raw values, and if the form state is reused elsewhere also update the form field or local variable prior to submission so the trimmed value is consistently used.src/components/Login/UsernameInput.tsx (1)
17-18: Consider adding theform-input-fieldwrapper for styling consistency.The parent components (
LocalLoginandJellyfinLogin) wrap the password field in<div className="form-input-field">, which applies flexbox layout, a max-width constraint, rounded corners, and shadow styling. The username input field inUsernameInputlacks this wrapper, creating a visual inconsistency between the two fields.💡 Suggested enhancement
return ( <div> - <input type="text" {...field} {...props} /> + <div className="form-input-field"> + <input type="text" {...field} {...props} /> + </div> {touched[field.name] && hasTrailingWhitespace(field.value) ? (🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Login/UsernameInput.tsx` around lines 17 - 18, The UsernameInput component is missing the consistent wrapper used elsewhere; wrap the existing <input> (which spreads {...field} and {...props}) inside a div with className="form-input-field" to match LocalLogin and JellyfinLogin styling, preserving the current props/field spreads and structure in the UsernameInput component so flex layout, max-width, rounded corners, and shadow are applied consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/Login/UsernameInput.tsx`:
- Around line 14-27: The current check uses the `in` operator on `touched` which
only tests key existence; update the UsernameInput component to check that the
field is actually touched by using a truthy lookup (e.g.,
`!!touched[field.name]` or `Boolean(touched[field.name])`) instead of
`field.name in touched`, so the trailing-whitespace warning
(hasTrailingWhitespace(field.value)) only renders when the field is truly
touched; modify the conditional that currently reads `field.name in touched &&
hasTrailingWhitespace(field.value)` to use the truthy lookup of
`touched[field.name]`.
---
Nitpick comments:
In `@src/components/Login/JellyfinLogin.tsx`:
- Around line 76-79: The JellyfinLogin component sends values.username directly
to the API; trim the username before submission so the payload uses a trimmed
value for username and email. Update the axios.post call in JellyfinLogin (the
object containing username, password, email) to use values.username.trim() (and
assign the trimmed value to both username and email) so the API receives a
consistent, whitespace-free username.
In `@src/components/Login/LocalLogin.tsx`:
- Around line 59-61: The submitted email/username is not being trimmed before
the POST, so trailing spaces can cause login failures; update the payload sent
in the function that calls axios.post('/api/v1/auth/local') (in LocalLogin.tsx)
to send trimmed values (e.g., use values.email.trim() and/or
values.username?.trim()) instead of the raw values, and if the form state is
reused elsewhere also update the form field or local variable prior to
submission so the trimmed value is consistently used.
In `@src/components/Login/UsernameInput.tsx`:
- Around line 17-18: The UsernameInput component is missing the consistent
wrapper used elsewhere; wrap the existing <input> (which spreads {...field} and
{...props}) inside a div with className="form-input-field" to match LocalLogin
and JellyfinLogin styling, preserving the current props/field spreads and
structure in the UsernameInput component so flex layout, max-width, rounded
corners, and shadow are applied consistently.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/components/Login/JellyfinLogin.tsxsrc/components/Login/LocalLogin.tsxsrc/components/Login/UsernameInput.tsxsrc/i18n/locale/en.jsonsrc/styles/globals.css
GeoffreyCoulaud
force-pushed
the
warn-whitespace-after-username
branch
from
05a9d95 to
c4c424d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/Login/UsernameInput.tsx`:
- Around line 14-18: The component UsernameInput is currently capturing Formik's
meta via the rest spread and accidentally spreading it onto the <input>, causing
invalid DOM prop warnings; fix by explicitly destructuring meta from the props
(e.g., ({ field, form: { touched }, meta, ...props }: FieldProps)) and then
spread only field and props into the <input> (do not spread meta), so meta is
removed from the DOM attributes while preserving other HTML props.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/components/Login/JellyfinLogin.tsxsrc/components/Login/LocalLogin.tsxsrc/components/Login/UsernameInput.tsxsrc/i18n/locale/en.jsonsrc/styles/globals.css
🚧 Files skipped from review as they are similar to previous changes (2)
- src/i18n/locale/en.json
- src/components/Login/LocalLogin.tsx
| const UsernameInput = ({ field, form: { touched }, ...props }: FieldProps) => { | ||
| const intl = useIntl(); | ||
| return ( | ||
| <div> | ||
| <input type="text" {...field} {...props} /> |
There was a problem hiding this comment.
Avoid spreading Formik's meta prop onto the DOM element.
The rest destructuring ...props captures Formik's meta object (along with valid HTML attributes). Spreading meta onto the <input> will cause React warnings about invalid DOM props.
🐛 Proposed fix to exclude meta from spread
-const UsernameInput = ({ field, form: { touched }, ...props }: FieldProps) => {
+const UsernameInput = ({
+ field,
+ form: { touched },
+ meta: _meta,
+ ...props
+}: FieldProps) => {
const intl = useIntl();
return (
<div>
<input type="text" {...field} {...props} />📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const UsernameInput = ({ field, form: { touched }, ...props }: FieldProps) => { | |
| const intl = useIntl(); | |
| return ( | |
| <div> | |
| <input type="text" {...field} {...props} /> | |
| const UsernameInput = ({ | |
| field, | |
| form: { touched }, | |
| meta: _meta, | |
| ...props | |
| }: FieldProps) => { | |
| const intl = useIntl(); | |
| return ( | |
| <div> | |
| <input type="text" {...field} {...props} /> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/Login/UsernameInput.tsx` around lines 14 - 18, The component
UsernameInput is currently capturing Formik's meta via the rest spread and
accidentally spreading it onto the <input>, causing invalid DOM prop warnings;
fix by explicitly destructuring meta from the props (e.g., ({ field, form: {
touched }, meta, ...props }: FieldProps)) and then spread only field and props
into the <input> (do not spread meta), so meta is removed from the DOM
attributes while preserving other HTML props.
There was a problem hiding this comment.
Pull request overview
Adds a reusable login username/email input component that displays a warning when trailing whitespace is present, and applies it to both local and Jellyfin login flows.
Changes:
- Introduce
UsernameInputcomponent that detects trailing whitespace and renders an inline warning under the field - Reuse
UsernameInputinLocalLoginandJellyfinLogin - Add global
.warningstyling and an i18n string for the warning text
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/styles/globals.css | Adds a .warning utility class for warning text styling |
| src/i18n/locale/en.json | Adds the localized warning message key |
| src/components/Login/UsernameInput.tsx | New component wrapping a Formik field and showing a trailing-whitespace warning |
| src/components/Login/LocalLogin.tsx | Switches username/email Field to use UsernameInput |
| src/components/Login/JellyfinLogin.tsx | Switches username Field to use UsernameInput |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <div> | ||
| <input type="text" {...field} {...props} /> | ||
| {touched[field.name] && hasTrailingWhitespace(field.value) ? ( | ||
| <div className="warning label-tip flex items-center"> | ||
| <ExclamationTriangleIcon className="mr-1 h-4 w-4" /> | ||
| {intl.formatMessage(messages.tipUsernameHasTrailingWhitespace)} | ||
| </div> |
There was a problem hiding this comment.
UsernameInput no longer applies the form-input-field wrapper styling that the login form previously relied on (max width + shadow). This likely causes a visual/layout regression vs the password field and prior behavior. Consider wrapping the <input> in a form-input-field container (or re-introducing that wrapper where the component is used) to preserve consistent styling.
| tipUsernameHasTrailingWhitespace: 'The username/email ends with a space', | ||
| }); | ||
|
|
||
| const hasTrailingWhitespace = (value: string): boolean => { | ||
| return /\s$/.test(value); |
There was a problem hiding this comment.
The warning text says the value ends with a "space", but the detection uses /\s$/ which matches any trailing whitespace (tabs, newlines, non‑breaking spaces, etc.). Either tighten the check to a literal space or change the message to say "whitespace" so the UI message matches the actual condition.
src/components/Login/LocalLogin.tsx
Outdated
| <Field | ||
| id="email" | ||
| name="email" | ||
| placeholder={`${intl.formatMessage( | ||
| messages.email | ||
| )} / ${intl.formatMessage(messages.username)}`} | ||
| type="text" | ||
| inputMode="email" | ||
| data-testid="email" | ||
| data-form-type="username,email" | ||
| className="!bg-gray-700/80 placeholder:text-gray-400" | ||
| component={UsernameInput} | ||
| /> |
There was a problem hiding this comment.
This field used to be wrapped in a form-input-field div (adds max width + shadow). With component={UsernameInput} the wrapper was removed, so the username/email input may render inconsistently compared to other login inputs (e.g., password). Consider restoring the wrapper here, or ensuring UsernameInput provides equivalent wrapper styling.
| <Field | |
| id="email" | |
| name="email" | |
| placeholder={`${intl.formatMessage( | |
| messages.email | |
| )} / ${intl.formatMessage(messages.username)}`} | |
| type="text" | |
| inputMode="email" | |
| data-testid="email" | |
| data-form-type="username,email" | |
| className="!bg-gray-700/80 placeholder:text-gray-400" | |
| component={UsernameInput} | |
| /> | |
| <div className="form-input-field"> | |
| <Field | |
| id="email" | |
| name="email" | |
| placeholder={`${intl.formatMessage( | |
| messages.email | |
| )} / ${intl.formatMessage(messages.username)}`} | |
| type="text" | |
| inputMode="email" | |
| data-testid="email" | |
| data-form-type="username,email" | |
| className="!bg-gray-700/80 placeholder:text-gray-400" | |
| component={UsernameInput} | |
| /> | |
| </div> |
| <Field | ||
| id="username" | ||
| name="username" | ||
| type="text" | ||
| placeholder={intl.formatMessage(messages.username)} | ||
| className="!bg-gray-700/80 placeholder:text-gray-400" | ||
| data-form-type="username" | ||
| component={UsernameInput} | ||
| /> |
There was a problem hiding this comment.
Yes, please keep the <div className="form-input-field">
|
I can do what the AIs suggested if you think it's meaningful, don't hesitate to tell me :) |
gauthier-th
left a comment
gauthier-th
left a comment
There was a problem hiding this comment.
I find the new behavior with UsernameInput.tsx a bit confusing. It adds complexity and abstraction only for a small warning.
Couldn't you instead:
- Keep the current username input field as it is
- Optional: create a small component for this warning, but I don't think it's necessary since it's used in only 2 places
- Add the warning depending on the username value, like
{username.match(...) && <WarningTooltip>}
| <Field | ||
| id="username" | ||
| name="username" | ||
| type="text" | ||
| placeholder={intl.formatMessage(messages.username)} | ||
| className="!bg-gray-700/80 placeholder:text-gray-400" | ||
| data-form-type="username" | ||
| component={UsernameInput} | ||
| /> |
There was a problem hiding this comment.
Yes, please keep the <div className="form-input-field">
GeoffreyCoulaud
force-pushed
the
warn-whitespace-after-username
branch
from
c4c424d to
ee94461
Compare
GeoffreyCoulaud
force-pushed
the
warn-whitespace-after-username
branch
from
ee94461 to
30e0b69
Compare
|
I edited the code as requested. |
Description
Adds a
UsernameInputcomponent and uses it in both local and jellyfin login.If trailing whitespace is detected, a warning is displayed under the input.
No AI tools were used to create this PR.Edit: I got help from Claude code to apply requested changes.
Screenshot (if UI-related)
To-Dos
pnpm buildpnpm i18n:extractIssues Fixed or Closed
Summary by CodeRabbit