fix: prevent save button appearing for whitespace-only changes#38244
fix: prevent save button appearing for whitespace-only changes#38244Anshumancanrock wants to merge 3 commits intoRocketChat:developfrom
Conversation
🦋 Changeset detectedLatest commit: f2b4d95 The changes in this PR will be included in the next version bump. This PR includes changesets to release 40 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
WalkthroughTrim whitespace from nickname and bio on the server and update the client form to reset using backend-provided, trimmed profile values after a successful save; add a changeset documenting the patch. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #38244 +/- ##
===========================================
+ Coverage 70.39% 70.41% +0.01%
===========================================
Files 3161 3161
Lines 110654 110648 -6
Branches 19892 19885 -7
===========================================
+ Hits 77895 77910 +15
+ Misses 30731 30713 -18
+ Partials 2028 2025 -3
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/client/views/account/profile/AccountProfileForm.tsx (1)
112-140: Remove inline implementation comment.Line 113 adds a comment that restates the code and isn’t needed. Please remove it.
As per coding guidelines, avoid code comments in the implementation.🧹 Suggested cleanup
- // Trim string values before saving
🤖 Fix all issues with AI agents
In `@apps/meteor/client/views/account/profile/AccountProfilePage.tsx`:
- Around line 47-75: Remove the two inline explanatory comments inside the
AccountProfilePage component: drop the comment above the useWatch call ("//
Watch all form values to compare trimmed versions") and the comment before
stringFields ("// Check if form is truly dirty by comparing trimmed string
values"), leaving the logic for watchedValues, defaultValues, and the
isTrulyDirty useMemo (including stringFields, avatar/statusType/customFields
checks) intact; ensure only the code around useWatch, getProfileInitialValues,
and the isTrulyDirty computation is preserved without those comments.
apps/meteor/client/views/account/profile/AccountProfilePage.tsx
Outdated
Show resolved
Hide resolved
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Anshumancanrock
force-pushed
the
space
branch
from
c543225 to
89d3e7c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/meteor/client/views/account/profile/AccountProfileForm.tsx`:
- Line 113: Remove the inline explanatory comment "// Trim string values before
saving" inside the AccountProfileForm component (look for the AccountProfileForm
function/JSX where trimming is performed) so the implementation contains no
in-code comments; leave the trimming code intact but delete that comment to
comply with the coding guideline against inline comments.
🧹 Nitpick comments (1)
apps/meteor/client/views/account/profile/AccountProfileForm.tsx (1)
139-141: Consider includingavatarin the reset and improve line formatting.The reset omits
avatar, which could leave stale state if the avatar was changed. Also, this line exceeds typical length limits, making it hard to read.♻️ Suggested refactor
} finally { - reset({ email: trimmedEmail, name: trimmedName, username: trimmedUsername, statusType, statusText: trimmedStatusText, nickname: trimmedNickname, bio: trimmedBio, customFields }); + reset({ + email: trimmedEmail, + name: trimmedName, + username: trimmedUsername, + statusType, + statusText: trimmedStatusText, + nickname: trimmedNickname, + bio: trimmedBio, + customFields, + avatar: undefined, + }); }Verify that resetting
avatartoundefined(or its appropriate default) aligns with the expected behavior after a successful save.
apps/meteor/client/views/account/profile/AccountProfileForm.tsx
Outdated
Show resolved
Hide resolved
Anshumancanrock
force-pushed
the
space
branch
2 times, most recently
from
78fa457 to
d15fc31
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/client/views/account/profile/AccountProfileForm.tsx (1)
299-313: Broken JSX structure around Nickname/Bio fields.From Line 302 onward, the
bioprops are dangling (no<TextAreaInput ...>opener) and thebioerrors are rendered under the Nickname label. This will fail to compile or render correctly. Please restore a dedicated Bio controller/TextAreaInput and close the Nickname field row before it.🛠️ Suggested fix
- <FieldRow> - <Controller control={control} name='nickname' render={({ field }) => <TextInput {...field} id={nicknameId} flexGrow={1} />} /> - id={bioId} - error={errors.bio?.message} - rows={3} - flexGrow={1} - addon={<Icon name='edit' size='x20' alignSelf='center' />} - aria-invalid={errors.statusText ? 'true' : 'false'} - aria-describedby={`${bioId}-error`} - /> - )} - /> - </FieldRow> - {errors?.bio && ( - <FieldError aria-live='assertive' id={`${bioId}-error`}> - {errors.bio.message} - </FieldError> - )} - </Field> + <FieldRow> + <Controller control={control} name='nickname' render={({ field }) => <TextInput {...field} id={nicknameId} flexGrow={1} />} /> + </FieldRow> + </Field> + <Field> + <FieldLabel htmlFor={bioId}>{t('Bio')}</FieldLabel> + <FieldRow> + <Controller + control={control} + name='bio' + render={({ field }) => ( + <TextAreaInput + {...field} + id={bioId} + error={errors.bio?.message} + rows={3} + flexGrow={1} + aria-invalid={errors.bio ? 'true' : 'false'} + aria-describedby={`${bioId}-error`} + /> + )} + /> + </FieldRow> + {errors?.bio && ( + <FieldError aria-live='assertive' id={`${bioId}-error`}> + {errors.bio.message} + </FieldError> + )} + </Field>
…file Trims field values on blur to prevent whitespace-only changes from triggering the dirty state. This is more performant than watching all form values since it only runs when user leaves a field.
Anshumancanrock
force-pushed
the
space
branch
from
d15fc31 to
bd8469d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/client/views/account/profile/AccountProfileForm.tsx (1)
299-313: Fix the broken JSX in the nickname/bio section.Line 302 closes the nickname
Controller, but the subsequent bio props (id={bioId},rows, etc.) are now outside any JSX element, which will break rendering/compilation. The bio field needs its ownController/TextAreaInputblock after the nickname field is closed.🐛 Proposed fix
<FieldLabel htmlFor={nicknameId}>{t('Nickname')}</FieldLabel> <FieldRow> - <Controller control={control} name='nickname' render={({ field }) => <TextInput {...field} id={nicknameId} flexGrow={1} />} /> - id={bioId} - error={errors.bio?.message} - rows={3} - flexGrow={1} - addon={<Icon name='edit' size='x20' alignSelf='center' />} - aria-invalid={errors.statusText ? 'true' : 'false'} - aria-describedby={`${bioId}-error`} - /> - )} - /> - </FieldRow> - {errors?.bio && ( - <FieldError aria-live='assertive' id={`${bioId}-error`}> - {errors.bio.message} - </FieldError> - )} - </Field> + <Controller + control={control} + name='nickname' + render={({ field }) => <TextInput {...field} id={nicknameId} flexGrow={1} />} + /> + </FieldRow> + </Field> + <Field> + <FieldLabel htmlFor={bioId}>{t('Bio')}</FieldLabel> + <FieldRow> + <Controller + control={control} + name='bio' + rules={{ + maxLength: { + value: BIO_TEXT_MAX_LENGTH, + message: t('Max_length_is', BIO_TEXT_MAX_LENGTH), + }, + }} + render={({ field }) => ( + <TextAreaInput + {...field} + id={bioId} + error={errors.bio?.message} + rows={3} + flexGrow={1} + aria-invalid={errors.bio ? 'true' : 'false'} + aria-describedby={`${bioId}-error`} + /> + )} + /> + </FieldRow> + {errors?.bio && ( + <FieldError aria-live='assertive' id={`${bioId}-error`}> + {errors.bio.message} + </FieldError> + )} + </Field>
Anshumancanrock
force-pushed
the
space
branch
2 times, most recently
from
83c0e1c to
f7bda30
Compare
MartinSchoeler
left a comment
MartinSchoeler
left a comment
There was a problem hiding this comment.
This PR tries to do 2 things that cancel each other out, you either trim the data or disable the button, if you do both one of the cases will never happen.
But all the other forms do not concern themselves with trimming data, since most of the endpoints already do that, so maybe a backend solution would be more appropriate in this case
| const watchedValues = useWatch({ control }); | ||
| const defaultValues = useMemo(() => getProfileInitialValues(user), [user]); | ||
|
|
||
| const isTrulyDirty = useMemo(() => { |
There was a problem hiding this comment.
You are basically re implementing the validations done in react-hook-form
Please check it's documentation https://www.react-hook-form.com/api/useform/register/
It would better to try to create a validation callback. But before that check the main review comment
| const updateAvatar = useUpdateAvatar(avatar, user?._id || ''); | ||
|
|
||
| const handleSave = async ({ email, name, username, statusType, statusText, nickname, bio, customFields }: AccountProfileFormValues) => { | ||
| const trimmedName = name.trim(); |
There was a problem hiding this comment.
Don't create a bunch of variables just for the trimmed data, either trim it directly or iterate and filter the data beforehand
Anshumancanrock
force-pushed
the
space
branch
from
f7bda30 to
8de7f38
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/client/views/account/profile/AccountProfileForm.tsx (1)
113-133: Remove inline comment at line 131.The inline comment violates the coding guideline: avoid code comments in the implementation. The
reset(getProfileInitialValues(updatedUser))call correctly preservescustomFieldssincegetProfileInitialValuesexplicitly includescustomFields: user?.customFields ?? {}andupdateOwnBasicInforeturns the complete user object from the backend.Diff
- // Reset form with backend response (which has trimmed whitespace) reset(getProfileInitialValues(updatedUser));
Anshumancanrock
force-pushed
the
space
branch
4 times, most recently
from
a56dfa4 to
e9580d8
Compare
Anshumancanrock
force-pushed
the
space
branch
from
7f4f507 to
4744d3b
Compare
|
@MartinSchoeler Thanks for the feedback! . I've simplified the approach to fix this on the backend like you suggested. I also moved the length validation to check the trimmed value instead of the raw input, which actually fixes a bug where users could bypass the max length by padding with spaces. The form resets with the API response now, so the save button behavior works correctly. Much cleaner this way! Let me know if it looks good or you would like any changes. Recording.2026-01-30.005625.mp4 |
3 participants
Proposed changes (including videos or screenshots)
This PR fixes an issue where the "Save Changes" button appears in the profile settings when users add only whitespace (spaces/tabs) to text fields, even though no meaningful changes have been made.
Changes:
isTrulyDirtylogic inAccountProfilePage.tsxthat compares trimmed values of form fields instead of raw valueshandleSaveinAccountProfileForm.tsxto ensure clean data is saved and the form state is reset with trimmed valuesIssue(s)
Closes #38243
Steps to test or reproduce
Before Fix:
Recording.2026-01-18.220530.mp4
After Fix:
Recording.2026-01-30.005625.mp4
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.