fix(ui): prevent horizontal overflow on home page mobile#38421
fix(ui): prevent horizontal overflow on home page mobile#38421sharanyamahajan wants to merge 20 commits intoRocketChat:developfrom
Conversation
fix(admin): make Custom HTML card responsive on mobile
…n-loader fix(ui): add skeleton loading states to Room Contextual Bar views
…ptions fix: prevent startup crash on invalid MONGO_OPTIONS JSON
refactor: reduce complexity in RegisterForm
|
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 |
|
|
|
WalkthroughThis PR introduces robustness improvements across multiple subsystems: caching logic with initialization and synchronization, profile form field trimming with simplified rendering, livechat store type strengthening and component modernization, sidebar item registration guards, home page layout overflow containment, data model consolidation, registration form validation hooks, and supporting documentation and test utilities. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![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)
90-101: Add required field validation and trim username input before validation.Empty usernames pass validation because the validator returns
undefined(no error) and the field has norequiredrule. Additionally, trimming happens inhandleSave(line 110) after validation, meaning whitespace-only values would be validated as entered. Trim the username at the start of validation and return a required field error if empty.Proposed fix
const validateUsername = async (username: string): Promise<string | undefined> => { + const normalized = username?.trim() || ''; + if (!normalized) { + return t('Required_field', { field: t('Username') }); + } - if (!username || username === previousUsername) { + if (normalized === previousUsername) { return; } - if (!namesRegex.test(username)) { + if (!namesRegex.test(normalized)) { return t('error-invalid-username'); } - const { result: isAvailable } = await checkUsernameAvailability({ username }); + const { result: isAvailable } = await checkUsernameAvailability({ username: normalized }); if (!isAvailable) { return t('Username_already_exist'); } };
🤖 Fix all issues with AI agents
In `@apps/meteor/client/views/account/profile/AccountProfileForm.tsx`:
- Around line 238-270: The FieldLabel htmlFor values (nicknameId, bioId,
emailId) were left without matching input ids, breaking label association;
update the Controller renderers to pass the corresponding id prop to the inputs
(e.g. in the nickname Controller render pass id={nicknameId} to TextInput, in
the bio Controller pass id={bioId} to TextAreaInput, and in the email Controller
pass id={emailId} to TextInput while preserving disabled={!allowEmailChange} and
existing validation rules). Ensure you still spread the controller's field props
({...field}) so value/onChange remain wired.
🧹 Nitpick comments (10)
apps/meteor/client/views/home/HomePageHeader.tsx (1)
15-29: Minor:width: '100%'is redundant for block-level elements.The
divis a block element and will naturally take 100% width of its container. OnlymaxWidth: '100%'andoverflowX: 'hidden'are needed to achieve the overflow fix.♻️ Suggested simplification
<div style={{ - width: '100%', maxWidth: '100%', overflowX: 'hidden', }} >apps/meteor/client/views/home/HomePage.tsx (1)
10-20: Minor: Same redundantwidth: '100%'as in HomePageHeader.For consistency, consider removing
width: '100%'here as well since block elements inherit full width by default.♻️ Suggested simplification
return ( <div style={{ - width: '100%', maxWidth: '100%', overflowX: 'hidden', }} >docs/architecture/overview.md (1)
25-29: Minor: Markdown table formatting.Static analysis flags spacing inconsistencies in the table. Consider using consistent spacing around pipe characters for better readability.
📝 Suggested fix
-| Directory | Purpose | Runtime role | -|---------|--------|--------------| -| `apps/` | Entry points and deployable applications | Client and server applications | -| `packages/` | Core and shared functionality | Loaded by Meteor at runtime | -| `ee/` | Enterprise-specific features and services | Optional, license-gated runtime modules | +| Directory | Purpose | Runtime role | +|--------------|-------------------------------------------|-----------------------------------------| +| `apps/` | Entry points and deployable applications | Client and server applications | +| `packages/` | Core and shared functionality | Loaded by Meteor at runtime | +| `ee/` | Enterprise-specific features and services | Optional, license-gated runtime modules |packages/livechat/src/store/types.ts (1)
31-31: Remove inline comment per coding guidelines.The comment
// unknown > any (safer)should be removed. As per coding guidelines: "Avoid code comments in the implementation."Suggested fix
- payload?: unknown; // unknown > any (safer) + payload?: unknown;packages/models/src/models/BaseRaw.ts (1)
170-170: Remove banner comments per coding guidelines.The section marker comments (
/* ===================== FIXED METHOD ===================== */and the closing marker) should be removed. As per coding guidelines: "Avoid code comments in the implementation."Suggested fix
- /* ===================== FIXED METHOD ===================== */ - async findOneAndDelete(filter: Filter<T>, options?: FindOneAndDeleteOptions): Promise<WithId<T> | null> {- /* ======================================================== */ - private setUpdatedAt(record: UpdateFilter<T> | InsertionModel<T>): void {Also applies to: 222-222
packages/web-ui-registration/src/hooks/useRegisterValidation.ts (1)
32-39: Consider typing thevaluesparameter more explicitly.The
valuesparameter is typed asany. For better type safety, consider using the form's payload type:♻️ Optional type improvement
- validate: (val: string, values: any) => + validate: (val: string, values: { password: string }) => values.password === val ? true : t('registration.component.form.invalidConfirmPass'),packages/web-ui-registration/src/hooks/useRegisterErrorHandling.ts (3)
4-8: Consider typingsetErrormore explicitly.The
setErrorparameter is typed asany, which loses the benefits of TypeScript's type checking. Consider using theUseFormSetErrortype fromreact-hook-form:♻️ Suggested type improvement
+import type { UseFormSetError } from 'react-hook-form'; + +type FormFields = 'email' | 'username' | 'name'; + type Params = { - setError: any; + setError: UseFormSetError<Record<FormFields, string>>; setLoginRoute: (route: 'login') => void; setServerError: (value: string | undefined) => void; };Alternatively, if the form's full type is available, use that directly for complete type safety.
14-54: Consider adding a fallback for unhandled errors.The error handler covers many specific cases but has no fallback for unexpected error types. While the mutation's error handling may cover this upstream, consider adding a catch-all to ensure visibility:
♻️ Optional fallback suggestion
if (error.error === 'error-user-registration-custom-field') { setServerError(error.message); } + + // Log unhandled errors for debugging (optional) + const handledErrors = [ + 'error-invalid-email', + 'error-user-already-exists', + 'error-too-many-requests', + 'error-user-is-not-activated', + 'error-user-registration-custom-field', + ]; + if (!handledErrors.some((e) => error.error?.includes(e) || error.errorType?.includes(e))) { + console.warn('Unhandled registration error:', error); + } };
23-40: String-based error matching is fragile.The regex tests like
/Email already exists/rely on server error message text. If server messages change, these handlers will silently stop working. Consider requesting structured error codes from the server if not already available, or document this dependency.apps/meteor/client/views/account/profile/AccountProfileForm.tsx (1)
155-156: Remove inline JSX comments to match repo guidance.These section headers are now redundant given the layout structure.
As per coding guidelines: Avoid code comments in the implementation.🧹 Proposed cleanup
- {/* Avatar */} <Field>- {/* Name + Username */} <Box display='flex' flexDirection={isMobile ? 'column' : 'row'} gap='x16'>- {/* Status */} <Field>- {/* Nickname */} <Field>- {/* Bio */} <Field>- {/* Email */} <Field>Also applies to: 173-174, 210-211, 238-239, 246-247, 259-260
| {/* Nickname */} | ||
| <Field> | ||
| <FieldLabel htmlFor={nicknameId}>{t('Nickname')}</FieldLabel> | ||
| <FieldRow> | ||
| <Controller | ||
| control={control} | ||
| name='nickname' | ||
| render={({ field }) => ( | ||
| <TextInput {...field} id={nicknameId} flexGrow={1} addon={<Icon name='edit' size='x20' alignSelf='center' />} /> | ||
| )} | ||
| /> | ||
| <Controller control={control} name='nickname' render={({ field }) => <TextInput {...field} />} /> | ||
| </FieldRow> | ||
| </Field> | ||
|
|
||
| {/* Bio */} | ||
| <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} | ||
| addon={<Icon name='edit' size='x20' alignSelf='center' />} | ||
| aria-invalid={errors.statusText ? 'true' : 'false'} | ||
| aria-describedby={`${bioId}-error`} | ||
| /> | ||
| )} | ||
| rules={{ maxLength: BIO_TEXT_MAX_LENGTH }} | ||
| render={({ field }) => <TextAreaInput {...field} rows={3} />} | ||
| /> | ||
| </FieldRow> | ||
| {errors?.bio && ( | ||
| <FieldError aria-live='assertive' id={`${bioId}-error`}> | ||
| {errors.bio.message} | ||
| </FieldError> | ||
| )} | ||
| </Field> | ||
|
|
||
| {/* Email */} | ||
| <Field> | ||
| <FieldLabel required htmlFor={emailId}> | ||
| {t('Email')} | ||
| </FieldLabel> | ||
| <FieldRow | ||
| display='flex' | ||
| flexDirection={isMobile ? 'column' : 'row'} | ||
| alignItems='stretch' | ||
| justifyContent='space-between' | ||
| className={css` | ||
| gap: 8px; | ||
| `} | ||
| > | ||
| <FieldRow> | ||
| <Controller | ||
| control={control} | ||
| name='email' | ||
| rules={{ | ||
| required: t('Required_field', { field: t('Email') }), | ||
| validate: { validateEmail: (email) => (validateEmail(email) ? undefined : t('error-invalid-email-address')) }, | ||
| }} | ||
| render={({ field }) => ( | ||
| <TextInput | ||
| {...field} | ||
| id={emailId} | ||
| flexGrow={1} | ||
| error={errors.email?.message} | ||
| addon={<Icon name={isUserVerified ? 'circle-check' : 'mail'} size='x20' />} | ||
| disabled={!allowEmailChange} | ||
| aria-required='true' | ||
| aria-invalid={errors.email ? 'true' : 'false'} | ||
| aria-describedby={`${emailId}-error ${emailId}-hint`} | ||
| /> | ||
| )} | ||
| rules={{ validate: (email) => (validateEmail(email) ? undefined : t('error-invalid-email-address')) }} | ||
| render={({ field }) => <TextInput {...field} disabled={!allowEmailChange} />} | ||
| /> |
There was a problem hiding this comment.
Restore input ids to keep labels associated.
FieldLabel htmlFor is set for nickname, bio, and email, but the inputs no longer receive the matching id, which breaks label association for assistive tech.
🛠️ Proposed fix
-<Controller control={control} name='nickname' render={({ field }) => <TextInput {...field} />} />
+<Controller control={control} name='nickname' render={({ field }) => <TextInput {...field} id={nicknameId} />} />- render={({ field }) => <TextAreaInput {...field} rows={3} />}
+ render={({ field }) => <TextAreaInput {...field} id={bioId} rows={3} />}- render={({ field }) => <TextInput {...field} disabled={!allowEmailChange} />}
+ render={({ field }) => <TextInput {...field} id={emailId} disabled={!allowEmailChange} />}📝 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.
| {/* Nickname */} | |
| <Field> | |
| <FieldLabel htmlFor={nicknameId}>{t('Nickname')}</FieldLabel> | |
| <FieldRow> | |
| <Controller | |
| control={control} | |
| name='nickname' | |
| render={({ field }) => ( | |
| <TextInput {...field} id={nicknameId} flexGrow={1} addon={<Icon name='edit' size='x20' alignSelf='center' />} /> | |
| )} | |
| /> | |
| <Controller control={control} name='nickname' render={({ field }) => <TextInput {...field} />} /> | |
| </FieldRow> | |
| </Field> | |
| {/* Bio */} | |
| <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} | |
| addon={<Icon name='edit' size='x20' alignSelf='center' />} | |
| aria-invalid={errors.statusText ? 'true' : 'false'} | |
| aria-describedby={`${bioId}-error`} | |
| /> | |
| )} | |
| rules={{ maxLength: BIO_TEXT_MAX_LENGTH }} | |
| render={({ field }) => <TextAreaInput {...field} rows={3} />} | |
| /> | |
| </FieldRow> | |
| {errors?.bio && ( | |
| <FieldError aria-live='assertive' id={`${bioId}-error`}> | |
| {errors.bio.message} | |
| </FieldError> | |
| )} | |
| </Field> | |
| {/* Email */} | |
| <Field> | |
| <FieldLabel required htmlFor={emailId}> | |
| {t('Email')} | |
| </FieldLabel> | |
| <FieldRow | |
| display='flex' | |
| flexDirection={isMobile ? 'column' : 'row'} | |
| alignItems='stretch' | |
| justifyContent='space-between' | |
| className={css` | |
| gap: 8px; | |
| `} | |
| > | |
| <FieldRow> | |
| <Controller | |
| control={control} | |
| name='email' | |
| rules={{ | |
| required: t('Required_field', { field: t('Email') }), | |
| validate: { validateEmail: (email) => (validateEmail(email) ? undefined : t('error-invalid-email-address')) }, | |
| }} | |
| render={({ field }) => ( | |
| <TextInput | |
| {...field} | |
| id={emailId} | |
| flexGrow={1} | |
| error={errors.email?.message} | |
| addon={<Icon name={isUserVerified ? 'circle-check' : 'mail'} size='x20' />} | |
| disabled={!allowEmailChange} | |
| aria-required='true' | |
| aria-invalid={errors.email ? 'true' : 'false'} | |
| aria-describedby={`${emailId}-error ${emailId}-hint`} | |
| /> | |
| )} | |
| rules={{ validate: (email) => (validateEmail(email) ? undefined : t('error-invalid-email-address')) }} | |
| render={({ field }) => <TextInput {...field} disabled={!allowEmailChange} />} | |
| /> | |
| {/* Nickname */} | |
| <Field> | |
| <FieldLabel htmlFor={nicknameId}>{t('Nickname')}</FieldLabel> | |
| <FieldRow> | |
| <Controller control={control} name='nickname' render={({ field }) => <TextInput {...field} id={nicknameId} />} /> | |
| </FieldRow> | |
| </Field> | |
| {/* Bio */} | |
| <Field> | |
| <FieldLabel htmlFor={bioId}>{t('Bio')}</FieldLabel> | |
| <FieldRow> | |
| <Controller | |
| control={control} | |
| name='bio' | |
| rules={{ maxLength: BIO_TEXT_MAX_LENGTH }} | |
| render={({ field }) => <TextAreaInput {...field} id={bioId} rows={3} />} | |
| /> | |
| </FieldRow> | |
| </Field> | |
| {/* Email */} | |
| <Field> | |
| <FieldLabel required htmlFor={emailId}> | |
| {t('Email')} | |
| </FieldLabel> | |
| <FieldRow> | |
| <Controller | |
| control={control} | |
| name='email' | |
| rules={{ validate: (email) => (validateEmail(email) ? undefined : t('error-invalid-email-address')) }} | |
| render={({ field }) => <TextInput {...field} id={emailId} disabled={!allowEmailChange} />} | |
| /> |
🤖 Prompt for AI Agents
In `@apps/meteor/client/views/account/profile/AccountProfileForm.tsx` around lines
238 - 270, The FieldLabel htmlFor values (nicknameId, bioId, emailId) were left
without matching input ids, breaking label association; update the Controller
renderers to pass the corresponding id prop to the inputs (e.g. in the nickname
Controller render pass id={nicknameId} to TextInput, in the bio Controller pass
id={bioId} to TextAreaInput, and in the email Controller pass id={emailId} to
TextInput while preserving disabled={!allowEmailChange} and existing validation
rules). Ensure you still spread the controller's field props ({...field}) so
value/onChange remain wired.
|
Hey! In Rocket.Chat, opening each PR from develop helps keep the commit list minimal and makes reviews easier for maintainers. Just wanted to share this in case it’s useful 👍 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
|
We welcome contributions, but these are starting to get spammy. Please, read of contributing guidelines and code of conduct (available on the repo) |
|
@KevLehman Understood, and apologies for this. I’ll carefully read and follow the contributing guidelines and the code of conduct before making any further contributions. Thank you for the reminder and for maintaining the standards of the repository. |
4 participants
Proposed changes (including videos or screenshots)
This PR fixes an issue where the Home page caused unwanted horizontal scrolling on small mobile screens.
The fix ensures that the Home page root container, header, and grid items respect the viewport width on mobile devices. Overflow protection is applied at the component level to prevent elements from exceeding the horizontal bounds of the screen.
This improves the mobile user experience without impacting the desktop layout or existing functionality.
Issue(s)
Related to #38398
Steps to test or reproduce
Expected result:
Further comments
The fix is intentionally scoped to the Home page components instead of using a global overflow rule. This avoids hiding unintended overflows elsewhere in the application and keeps the change minimal and safe.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.