refactor: extract validation and error handling from RegisterForm #38371
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
|
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 |
|
|
|
WalkthroughMultiple changes across Meteor client and livechat: cache lifecycle and token validation added to CachedStore; form validation centralized and trimmed; livechat store/types expanded; BaseRaw API surface reduced; new UI skeleton and registration hooks added; MongoDB TLS and options handling adjusted. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client Code
participant Cache as Local Cache
participant Store as CachedStore
participant Server as Server Sync
Client->>Store: init()
activate Store
Store->>Cache: loadFromCache()
activate Cache
Cache->>Cache: validateToken()<br/>check MAX_CACHE_TIME
alt Cache Valid
Cache->>Cache: deserializeFromCache()<br/>convert _updatedAt to Date
Cache-->>Store: return cached data
else Cache Invalid/Stale
Cache-->>Store: return null
end
deactivate Cache
alt Data Loaded from Cache
Store->>Store: setReady(true)
else No Cache / Invalid
Store->>Server: performInitialization()/sync()
activate Server
Server->>Server: fetch records<br/>collect timestamped changes
Server-->>Store: return synced data
deactivate Server
Store->>Store: apply changes in order<br/>setReady(true)
end
deactivate Store
Client->>Client: Initialization complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/meteor/packages/rocketchat-mongo-config/server/index.js (1)
20-23: Guard against undefined MONGO_URL before.includes.
process.env.MONGO_URLcan be undefined in some environments/tests, which would throw on startup. Add a safe fallback to avoid a hard crash.🔧 Proposed fix
- ...(!process.env.MONGO_URL.includes('retryWrites') && { retryWrites: false }), + ...(!((process.env.MONGO_URL ?? '').includes('retryWrites')) && { retryWrites: false }),apps/meteor/client/lib/cachedStores/CachedStore.ts (1)
368-375: Missing cleanup forreconnectionComputationin release.The
reconnectionComputationTracker autorun is set up during initialization (line 349) but is not stopped duringrelease(). This could cause the computation to continue running after the store is released, potentially leading to memory leaks or unexpected behavior when the connection status changes.Suggested fix
async release() { if (this.initializationPromise) { await this.initializationPromise; } + this.reconnectionComputation?.stop(); + this.reconnectionComputation = undefined; this.listenerUnsubscriber?.(); this.setReady(false); }packages/models/src/models/BaseRaw.ts (1)
172-216: Threadoptions.sessionthrough trash-enabled deletion path.When
options.sessionis provided (multi-document transactions), the trash path ignores it forfindOne,trash.updateOne, andcol.deleteOneoperations, causing them to execute outside the transaction. This breaks atomicity. Pass session to all three calls to maintain transactional consistency.Suggested fix
async findOneAndDelete(filter: Filter<T>, options?: FindOneAndDeleteOptions): Promise<WithId<T> | null> { if (!this.trash) { return this.col.findOneAndDelete(filter, options || {}); } + const { session } = options ?? {}; - const doc = await this.col.findOne(filter); + const doc = await this.col.findOne(filter, { session }); if (!doc) { return null; } const { _id, ...record } = doc; const trash: TDeleted = { ...record, _deletedAt: new Date(), __collection__: this.name, } as unknown as TDeleted; - await this.trash.updateOne({ _id } as Filter<TDeleted>, { $set: trash } as UpdateFilter<TDeleted>, { - upsert: true, - }); + await this.trash.updateOne({ _id } as Filter<TDeleted>, { $set: trash } as UpdateFilter<TDeleted>, { + upsert: true, + session, + }); try { - await this.col.deleteOne({ _id } as Filter<T>); + await this.col.deleteOne({ _id } as Filter<T>, { session }); } catch (originalError) {
🤖 Fix all issues with AI agents
In `@apps/meteor/client/lib/cachedStores/CachedStore.ts`:
- Around line 141-146: The computed updatedAt from deserializedRecords (variable
updatedAt, and the conditional that sets this.updatedAt = new Date(updatedAt))
is being overwritten by the later assignment this.updatedAt = data.updatedAt ||
this.updatedAt; either remove the earlier computation or stop overwriting
it—preferably keep the computed value as authoritative: remove the final
assignment (this.updatedAt = data.updatedAt || this.updatedAt) or change it to
only assign if data.updatedAt is strictly newer than the computed updatedAt;
refer to symbols updatedAt, deserializedRecords, hasId,
this.store.getState().replaceAll, and this.updatedAt to locate and adjust the
logic.
In `@apps/meteor/client/views/account/profile/AccountProfileForm.tsx`:
- Around line 240-243: The FieldLabel components (e.g., FieldLabel with
htmlFor={nicknameId}) are not associated with their inputs because the inputs
rendered by Controller (TextInput, TextArea, etc.) do not receive matching id
props; update the Controller render calls for 'nickname', 'bio', and 'email' to
spread the field props and explicitly pass id={nicknameId}, id={bioId}, and
id={emailId} respectively (or set field.id = ... before spreading) so the label
htmlFor matches the input id and restores proper accessibility linkage; locate
the Controller instances in AccountProfileForm.tsx and add the id prop to the
underlying input components (TextInput/TextArea) to fix the label-input binding.
- Around line 89-92: The validateUsername function currently returns undefined
for empty usernames which lets blank values pass client validation; update
validateUsername (referenced in AccountProfileForm.tsx) to explicitly check for
an empty string and return the same required-field validation message used
elsewhere (e.g., t('Required_field', { field: t('Username') })) instead of
returning early; keep the existing short-circuit for previousUsername, and
ensure behavior remains consistent with AdminUserForm and RegisterUsername
client-side validation before calling setUsername.
In `@apps/meteor/client/views/room/contextualBar/lib/ContextualListSkeleton.tsx`:
- Around line 8-11: The ContextualListSkeleton component uses Array.from({
length: items }) which can throw for negative or fractional items; clamp and
coerce the prop before creating the array by computing a safeCount (e.g., const
safeCount = Math.max(0, Math.floor(items ?? 6))) inside the
ContextualListSkeleton function and use Array.from({ length: safeCount })
instead; this ensures negative or non-integer items are converted to a
non-negative integer while preserving the default value behavior.
In `@packages/models/src/models/BaseRaw.ts`:
- Around line 149-153: In findOneAndUpdate ensure you detect whether the
provided update is an operator-style UpdateFilter or a full replacement document
before injecting _id: currently setUpdatedAt(update) and unconditional use of
update.$setOnInsert can add $-operators into a replacement and cause MongoDB
errors; fix by checking for operator keys (e.g., presence of any $-prefixed
properties or that update is UpdateFilter) and only assign _id into
update.$setOnInsert for operator updates, otherwise assign _id directly on the
replacement document (mutating the replacement object) when options.upsert is
true and query lacks _id.
🧹 Nitpick comments (12)
packages/web-ui-registration/src/RegisterForm.tsx (1)
40-40: Remove newly added inline comments to align with guidelines.Line 40, Line 49, and the JSX section labels at Line 124, Line 147, Line 170, Line 193, Line 217, and Line 242 introduce implementation comments that the project guidelines discourage. As per coding guidelines, please remove them.
♻️ Proposed diff
- // Settings const requireNameForRegister = useSetting('Accounts_RequireNameForSignUp', true); const requiresPasswordConfirmation = useSetting('Accounts_RequirePasswordConfirmation', true); const manuallyApproveNewUsersRequired = useSetting('Accounts_ManuallyApproveNewUsers', false); @@ - // IDs const formLabelId = useId(); const passwordVerifierId = useId(); @@ - {/* Name */} <Field> @@ - {/* Email */} <Field> @@ - {/* Username */} <Field> @@ - {/* Password */} <Field> @@ - {/* Confirm Password */} {requiresPasswordConfirmation && ( @@ - {/* Reason */} {manuallyApproveNewUsersRequired && (Based on coding guidelines, ...
Also applies to: 49-49, 124-124, 147-147, 170-170, 193-193, 217-217, 242-242
apps/meteor/packages/rocketchat-mongo-config/server/index.js (2)
38-46: Consolidate the throws and preserve the error cause.There are two consecutive throws; the second is unreachable and the first drops the
cause. Prefer a single throw that retains the original error.♻️ Proposed refactor
- throw new Error( - `Invalid MONGO_OPTIONS configuration: ${error.message}`, - ); - throw new Error('Invalid MONGO_OPTIONS environment variable: must be valid JSON.', { cause: error }); + const message = error instanceof Error ? error.message : String(error); + throw new Error(`Invalid MONGO_OPTIONS configuration: ${message}`, { cause: error });
15-17: Remove inline implementation comments.The updated inline comments conflict with the “avoid code comments in implementation” guideline for JS/TS files. Consider removing them or moving rationale to commit/ADR notes. As per coding guidelines, avoid code comments in the implementation.
Also applies to: 57-63
packages/livechat/src/store/types.ts (2)
1-5: Remove unused imports.
IMessage,ILivechatTrigger, andILivechatAgentare imported but not used in this file.Proposed fix
-import type { - IMessage, - ILivechatTrigger, - ILivechatAgent, -} from '@rocket.chat/core-typings'; -
31-31: Remove inline comment.As per coding guidelines, avoid code comments in implementation files.
Proposed fix
- payload?: unknown; // unknown > any (safer) + payload?: unknown;packages/livechat/src/store/index.tsx (2)
231-256: Provider class looks correct but consider adding explicit typing.The
Providerclass component works correctly with Preact, but thestateproperty andrendermethod lack explicit type annotations. This is minor but could improve maintainability.Optional: Add explicit type annotations
export class Provider extends Component { static override displayName = 'StoreProvider'; - override state = { + override state: StoreContextValue = { ...store.state, dispatch: store.setState.bind(store), on: store.on.bind(store), off: store.off.bind(store), };
26-31: Typo in type name: "Sytem" should be "System".
LivechatHiddenSytemMessageTypehas a typo. Rename toLivechatHiddenSystemMessageTypefor clarity and consistency with the comment and property namehiddenSystemMessages.Proposed fix
-export type LivechatHiddenSytemMessageType = +export type LivechatHiddenSystemMessageType = | 'uj' | 'ul' | 'livechat-close' | 'livechat-started' | 'livechat_transfer_history';Update usages at lines 68 and 112.
apps/meteor/client/lib/cachedStores/CachedStore.ts (3)
21-22: Type guard doesn't validate_idis a string.The type assertion claims
_id: string, but the runtime check only verifies the property exists, not its type. This could lead to runtime issues if_idis not a string.Suggested fix
const hasId = <T>(record: T): record is T & { _id: string } => - typeof record === 'object' && record !== null && '_id' in record; + typeof record === 'object' && record !== null && '_id' in record && typeof (record as { _id: unknown })._id === 'string';
229-238: Async callback without error handling could cause unhandled rejections.The async callback passed to
sdk.streammay not be awaited by the stream handler. IfhandleRecordEventthrows, the promise rejection could go unhandled.Consider adding error handling
protected setupListener() { return sdk.stream( this.eventType, [this.eventName], (async (action: 'removed' | 'changed', record: U) => { this.log('record received', action, record); - await this.handleRecordEvent(action, record); + try { + await this.handleRecordEvent(action, record); + } catch (error) { + console.error('Error handling record event:', error); + } }) as (...args: unknown[]) => void, ); }
410-412: Non-awaited asyncrelease()call.
release()is an async method that awaitsinitializationPromise, but it's called withoutawaitin the logout handler. Errors fromrelease()will result in unhandled promise rejections.Consider adding error handling
Accounts.onLogout(() => { - this.release(); + void this.release().catch(console.error); });packages/models/src/models/BaseRaw.ts (1)
170-170: Remove banner comments from implementation.The added banner comments aren’t needed and conflict with the “avoid code comments in implementation” guideline.
As per coding guidelines, ...🧹 Suggested cleanup
- /* ===================== FIXED METHOD ===================== */ ... - /* ======================================================== */Also applies to: 222-222
apps/meteor/client/views/account/profile/AccountProfileForm.tsx (1)
155-156: Remove JSX section comments.Inline section comments add noise in implementation files and conflict with the repo guideline to avoid code comments.
As per coding guidelines, avoid code comments in implementation files.Proposed cleanup
- {/* Avatar */}- {/* Name + Username */}- {/* Status */}- {/* Nickname */}- {/* Bio */}- {/* Email */}Also applies to: 173-175, 210-211, 238-239, 246-247, 259-260
| if (updatedAt > this.updatedAt.getTime()) { | ||
| this.updatedAt = new Date(updatedAt); | ||
| } | ||
|
|
||
| this.store.getState().replaceAll(deserializedRecords.filter(hasId)); | ||
|
|
||
| this.updatedAt = data.updatedAt || this.updatedAt; |
There was a problem hiding this comment.
Potential issue with updatedAt assignment order.
Lines 141-143 compute updatedAt from deserialized records, but line 146 immediately overwrites it with data.updatedAt || this.updatedAt. This makes the computation on lines 141-143 effectively unused since it will always be overwritten.
If the intent is to use data.updatedAt as the source of truth, the computation on lines 141-143 can be removed. If the computed value should take precedence, line 146 should be removed or reordered.
🤖 Prompt for AI Agents
In `@apps/meteor/client/lib/cachedStores/CachedStore.ts` around lines 141 - 146,
The computed updatedAt from deserializedRecords (variable updatedAt, and the
conditional that sets this.updatedAt = new Date(updatedAt)) is being overwritten
by the later assignment this.updatedAt = data.updatedAt || this.updatedAt;
either remove the earlier computation or stop overwriting it—preferably keep the
computed value as authoritative: remove the final assignment (this.updatedAt =
data.updatedAt || this.updatedAt) or change it to only assign if data.updatedAt
is strictly newer than the computed updatedAt; refer to symbols updatedAt,
deserializedRecords, hasId, this.store.getState().replaceAll, and this.updatedAt
to locate and adjust the logic.
| const validateUsername = async (username: string): Promise<string | undefined> => { | ||
| if (!username) { | ||
| return; | ||
| } | ||
|
|
||
| if (username === previousUsername) { | ||
| if (!username || username === previousUsername) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n apps/meteor/client/views/account/profile/AccountProfileForm.tsx | sed -n '80,110p'Repository: RocketChat/Rocket.Chat
Length of output: 1168
🏁 Script executed:
# Search for backend username validation rules
rg "username" apps/meteor/server --type ts --type js -A 3 -B 1 | head -100Repository: RocketChat/Rocket.Chat
Length of output: 7055
🏁 Script executed:
# Look for similar validation patterns with 'Required_field' error
rg "Required_field" apps/meteor/client --type ts --type js -B 2 -A 2Repository: RocketChat/Rocket.Chat
Length of output: 50378
🏁 Script executed:
# Check the form field definition for username input
rg -A 5 -B 5 "name.*username|username.*name" apps/meteor/client/views/account/profile/AccountProfileForm.tsxRepository: RocketChat/Rocket.Chat
Length of output: 2665
🏁 Script executed:
# Search for backend endpoint that handles user update
rg "updateOwnBasicInfo|users.updateOwnBasicInfo" apps/meteor/server --type ts --type js -B 2 -A 10 | head -80Repository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
# Look for username validation rules in backend
rg "username.*required|empty.*username" apps/meteor/server --type ts -B 2 -A 2 | head -60Repository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
# Check for test cases related to username validation
fd ".*\.spec\.|.*\.test\." --type f | xargs rg "validateUsername|empty.*username" 2>/dev/null | head -40Repository: RocketChat/Rocket.Chat
Length of output: 4019
🏁 Script executed:
cat -n apps/meteor/tests/unit/app/lib/server/functions/validateUsername.spec.ts | head -50Repository: RocketChat/Rocket.Chat
Length of output: 1703
🏁 Script executed:
# Find the actual backend validateUsername implementation
fd "validateUsername.ts" --type f | grep -v spec | grep -v testRepository: RocketChat/Rocket.Chat
Length of output: 122
🏁 Script executed:
# Look for the users.updateOwnBasicInfo endpoint implementation
rg "updateOwnBasicInfo" apps/meteor/server --type ts -l | head -5Repository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
cat -n apps/meteor/app/lib/server/functions/validateUsername.tsRepository: RocketChat/Rocket.Chat
Length of output: 623
🏁 Script executed:
# Search for the endpoint that handles user profile updates
rg "users.updateOwnBasicInfo" apps/meteor/app --type ts -B 5 -A 15 | head -100Repository: RocketChat/Rocket.Chat
Length of output: 1322
🏁 Script executed:
# Look for where username is updated in the backend
rg "updateOwnBasicInfo" apps/meteor/app/lib/server --type ts -B 3 -A 15Repository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
# Get the full users.updateOwnBasicInfo endpoint implementation
rg -A 30 "users.updateOwnBasicInfo" apps/meteor/app/api/server/v1/users.ts | head -80Repository: RocketChat/Rocket.Chat
Length of output: 1038
🏁 Script executed:
# Look for the validation params function
rg "isUsersUpdateOwnBasicInfoParamsPOST" apps/meteor/app --type ts -B 2 -A 10Repository: RocketChat/Rocket.Chat
Length of output: 1870
🏁 Script executed:
# Find the REST typings for updateOwnBasicInfo
fd "rest-typings" --type dRepository: RocketChat/Rocket.Chat
Length of output: 88
🏁 Script executed:
# Search for isUsersUpdateOwnBasicInfoParamsPOST definition in rest-typings
rg "isUsersUpdateOwnBasicInfoParamsPOST|UpdateOwnBasicInfoParamsPOST" packages/ --type ts -B 2 -A 10Repository: RocketChat/Rocket.Chat
Length of output: 4599
🏁 Script executed:
# Check if there's username validation in the endpoint handler itself
rg -A 50 "async post\(\)" apps/meteor/app/api/server/v1/users.ts | grep -A 50 "users.updateOwnBasicInfo" | head -70Repository: RocketChat/Rocket.Chat
Length of output: 1462
🏁 Script executed:
# Find executeSaveUserProfile implementation
rg "executeSaveUserProfile|saveUserProfile" apps/meteor/app --type ts -l | head -5Repository: RocketChat/Rocket.Chat
Length of output: 227
🏁 Script executed:
# Get the schema for UsersUpdateOwnBasicInfoParamsPostSchema
cat -n packages/rest-typings/src/v1/users/UsersUpdateOwnBasicInfoParamsPOST.tsRepository: RocketChat/Rocket.Chat
Length of output: 2009
🏁 Script executed:
# Find and examine executeSaveUserProfile implementation
rg "executeSaveUserProfile" apps/meteor/app/lib/server --type ts -B 3 -A 30 | head -80Repository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
# Also check for saveUserProfile
rg "saveUserProfile" apps/meteor/app/lib/server/functions --type ts -lRepository: RocketChat/Rocket.Chat
Length of output: 188
🏁 Script executed:
# Look for username validation in the save user profile flow
rg "validateUsername" apps/meteor/app/lib/server/functions --type ts -B 2 -A 5Repository: RocketChat/Rocket.Chat
Length of output: 3010
🏁 Script executed:
# Find where saveUserProfile is defined
fd "saveUser" apps/meteor/app/lib/server/functions --type dRepository: RocketChat/Rocket.Chat
Length of output: 112
🏁 Script executed:
# Check if saveUserProfile calls setUsername
rg "saveUserProfile|setUsername" apps/meteor/app/lib/server/functions/saveUser --type ts -B 2 -A 5 | head -50Repository: RocketChat/Rocket.Chat
Length of output: 1594
🏁 Script executed:
# Look for the main saveUserProfile function
cat -n apps/meteor/app/lib/server/functions/saveUser.ts 2>/dev/null | head -100Repository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
# List all files in saveUser directory
fd . apps/meteor/app/lib/server/functions/saveUser --type fRepository: RocketChat/Rocket.Chat
Length of output: 628
🏁 Script executed:
# Find the main export or index
rg "export.*saveUserProfile|export.*executeSaveUserProfile" apps/meteor/app/lib/server/functions --type tsRepository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
# Look for where username is handled in saveUser
rg "username" apps/meteor/app/lib/server/functions/saveUser --type ts -B 3 -A 8Repository: RocketChat/Rocket.Chat
Length of output: 10565
🏁 Script executed:
# Find isUpdateUserData definition
rg "isUpdateUserData" apps/meteor/app/lib/server/functions/saveUser --type ts -B 2 -A 5Repository: RocketChat/Rocket.Chat
Length of output: 6855
🏁 Script executed:
# Confirm that validateUserData only requires username for NEW users, not updates
cat -n apps/meteor/app/lib/server/functions/saveUser/validateUserData.ts | sed -n '1,70p'Repository: RocketChat/Rocket.Chat
Length of output: 3293
🏁 Script executed:
# Double-check the saveUserIdentity function to see if it validates empty username
rg "saveUserIdentity" apps/meteor/app/lib/server/functions --type ts -lRepository: RocketChat/Rocket.Chat
Length of output: 232
Add client-side validation to reject empty usernames in profile updates.
The validateUsername function currently returns early (undefined) for empty usernames, allowing them to pass client validation. While the backend would eventually reject an empty username through setUsername, the form should catch this on the client first. This is inconsistent with other username fields throughout the codebase (e.g., AdminUserForm, RegisterUsername) which all include explicit { required: t('Required_field', { field: t('Username') }) } validation.
Proposed fix
- if (!username || username === previousUsername) {
- return;
- }
+ if (username === previousUsername) {
+ return;
+ }
+ if (!username) {
+ return t('Required_field', { field: t('Username') });
+ }🤖 Prompt for AI Agents
In `@apps/meteor/client/views/account/profile/AccountProfileForm.tsx` around lines
89 - 92, The validateUsername function currently returns undefined for empty
usernames which lets blank values pass client validation; update
validateUsername (referenced in AccountProfileForm.tsx) to explicitly check for
an empty string and return the same required-field validation message used
elsewhere (e.g., t('Required_field', { field: t('Username') })) instead of
returning early; keep the existing short-circuit for previousUsername, and
ensure behavior remains consistent with AdminUserForm and RegisterUsername
client-side validation before calling setUsername.
| <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> |
There was a problem hiding this comment.
Bind labels to inputs for accessibility.
FieldLabel uses htmlFor, but the nickname, bio, and email inputs don’t set the matching id, which breaks label association for screen readers.
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} />}Also applies to: 248-255, 264-270
🤖 Prompt for AI Agents
In `@apps/meteor/client/views/account/profile/AccountProfileForm.tsx` around lines
240 - 243, The FieldLabel components (e.g., FieldLabel with
htmlFor={nicknameId}) are not associated with their inputs because the inputs
rendered by Controller (TextInput, TextArea, etc.) do not receive matching id
props; update the Controller render calls for 'nickname', 'bio', and 'email' to
spread the field props and explicitly pass id={nicknameId}, id={bioId}, and
id={emailId} respectively (or set field.id = ... before spreading) so the label
htmlFor matches the input id and restores proper accessibility linkage; locate
the Controller instances in AccountProfileForm.tsx and add the id prop to the
underlying input components (TextInput/TextArea) to fix the label-input binding.
| const ContextualListSkeleton: FC<ContextualListSkeletonProps> = ({ items = 6 }) => ( | ||
| <Box p='x24'> | ||
| {Array.from({ length: items }).map((_, i) => ( | ||
| <Box key={i} display='flex' alignItems='center' mb='x12'> |
There was a problem hiding this comment.
Guard against negative/fractional items to avoid RangeError.
If a caller passes a negative or non-integer value, Array.from({ length: items }) can throw or behave unexpectedly. Clamp to a non-negative integer before use.
🔧 Proposed fix
-const ContextualListSkeleton: FC<ContextualListSkeletonProps> = ({ items = 6 }) => (
- <Box p='x24'>
- {Array.from({ length: items }).map((_, i) => (
+const ContextualListSkeleton: FC<ContextualListSkeletonProps> = ({ items = 6 }) => {
+ const safeItems = Math.max(0, Math.floor(items));
+ return (
+ <Box p='x24'>
+ {Array.from({ length: safeItems }).map((_, i) => (
<Box key={i} display='flex' alignItems='center' mb='x12'>
<Skeleton variant='rect' width='x40' height='x40' borderRadius='x4' />
<Box mi='x8' flexGrow={1}>
<Skeleton variant='text' width='100%' />
<Skeleton variant='text' width='60%' />
</Box>
</Box>
- ))}
- </Box>
-);
+ ))}
+ </Box>
+ );
+};🤖 Prompt for AI Agents
In `@apps/meteor/client/views/room/contextualBar/lib/ContextualListSkeleton.tsx`
around lines 8 - 11, The ContextualListSkeleton component uses Array.from({
length: items }) which can throw for negative or fractional items; clamp and
coerce the prop before creating the array by computing a safeCount (e.g., const
safeCount = Math.max(0, Math.floor(items ?? 6))) inside the
ContextualListSkeleton function and use Array.from({ length: safeCount })
instead; this ensures negative or non-integer items are converted to a
non-negative integer while preserving the default value behavior.
| public findOneAndUpdate(query: Filter<T>, update: UpdateFilter<T> | T, options?: FindOneAndUpdateOptions): Promise<WithId<T> | null> { | ||
| this.setUpdatedAt(update); | ||
|
|
||
| if (options?.upsert && !('_id' in update || (update.$set && '_id' in update.$set)) && !('_id' in query)) { | ||
| update.$setOnInsert = { | ||
| ...(update.$setOnInsert || {}), | ||
| _id: new ObjectId().toHexString(), | ||
| } as Partial<T> & { _id: string }; | ||
| update.$setOnInsert = { ...(update.$setOnInsert || {}), _id: new ObjectId().toHexString() } as Partial<T> & { _id: string }; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd packages/models/src/models && head -200 BaseRaw.ts | tail -60Repository: RocketChat/Rocket.Chat
Length of output: 2452
🏁 Script executed:
cat -n packages/models/src/models/BaseRaw.ts | sed -n '140,165p'Repository: RocketChat/Rocket.Chat
Length of output: 1593
🌐 Web query:
MongoDB $setOnInsert replacement document operator mixing invalid
💡 Result:
That error means your update document is being treated as a replacement document (a plain { field: value, ... } document), but you’re also trying to use an update operator like $setOnInsert. MongoDB does not allow mixing replacement-style updates with operator-style updates—you must choose one form. In replacement updates, the document must contain only field/value pairs (no $... operators). [2]
Fix (most common): use an operator-style update with upsert: true—$setOnInsert only applies when an upsert results in an insert. [1]
db.collection.updateOne(
{ _id: key },
{
$set: { updatedAt: new Date() },
$setOnInsert: { createdAt: new Date(), status: "new" }
},
{ upsert: true }
)If you actually want to replace the whole document, use replaceOne() instead (but then you can’t use $setOnInsert in that same operation). [2]
Sources:
[1] MongoDB Docs: $setOnInsert behavior with upsert https://www.mongodb.com/docs/manual/reference/operator/update/setoninsert/
[2] MongoDB Docs: replacement documents cannot include update operators https://www.mongodb.com/docs/v7.1/tutorial/update-documents/
Branch update handling to avoid invalid replacement-operator mixing.
The update parameter accepts both operator-style updates (UpdateFilter<T>) and replacement documents (T). When update is a replacement document, adding $setOnInsert violates MongoDB's constraint that replacement documents cannot contain $ operators—MongoDB will reject this at runtime. Detect the update style and apply _id appropriately: for operator updates use $setOnInsert, for replacements set _id directly on the document.
Suggested fix
public findOneAndUpdate(query: Filter<T>, update: UpdateFilter<T> | T, options?: FindOneAndUpdateOptions): Promise<WithId<T> | null> {
this.setUpdatedAt(update);
if (options?.upsert && !('_id' in update || (update.$set && '_id' in update.$set)) && !('_id' in query)) {
- update.$setOnInsert = { ...(update.$setOnInsert || {}), _id: new ObjectId().toHexString() } as Partial<T> & { _id: string };
+ const updateObj = update as Record<string, any>;
+ const hasOperators = Object.keys(updateObj).some((key) => key.startsWith('$'));
+ if (hasOperators) {
+ updateObj.$setOnInsert = { ...(updateObj.$setOnInsert || {}), _id: new ObjectId().toHexString() };
+ } else {
+ (update as T)._id = new ObjectId().toHexString();
+ }
}
return this.col.findOneAndUpdate(query, update, options || {});
}📝 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.
| public findOneAndUpdate(query: Filter<T>, update: UpdateFilter<T> | T, options?: FindOneAndUpdateOptions): Promise<WithId<T> | null> { | |
| this.setUpdatedAt(update); | |
| if (options?.upsert && !('_id' in update || (update.$set && '_id' in update.$set)) && !('_id' in query)) { | |
| update.$setOnInsert = { | |
| ...(update.$setOnInsert || {}), | |
| _id: new ObjectId().toHexString(), | |
| } as Partial<T> & { _id: string }; | |
| update.$setOnInsert = { ...(update.$setOnInsert || {}), _id: new ObjectId().toHexString() } as Partial<T> & { _id: string }; | |
| } | |
| public findOneAndUpdate(query: Filter<T>, update: UpdateFilter<T> | T, options?: FindOneAndUpdateOptions): Promise<WithId<T> | null> { | |
| this.setUpdatedAt(update); | |
| if (options?.upsert && !('_id' in update || (update.$set && '_id' in update.$set)) && !('_id' in query)) { | |
| const updateObj = update as Record<string, any>; | |
| const hasOperators = Object.keys(updateObj).some((key) => key.startsWith('$')); | |
| if (hasOperators) { | |
| updateObj.$setOnInsert = { ...(updateObj.$setOnInsert || {}), _id: new ObjectId().toHexString() }; | |
| } else { | |
| (update as T)._id = new ObjectId().toHexString(); | |
| } | |
| } | |
| return this.col.findOneAndUpdate(query, update, options || {}); | |
| } |
🤖 Prompt for AI Agents
In `@packages/models/src/models/BaseRaw.ts` around lines 149 - 153, In
findOneAndUpdate ensure you detect whether the provided update is an
operator-style UpdateFilter or a full replacement document before injecting _id:
currently setUpdatedAt(update) and unconditional use of update.$setOnInsert can
add $-operators into a replacement and cause MongoDB errors; fix by checking for
operator keys (e.g., presence of any $-prefixed properties or that update is
UpdateFilter) and only assign _id into update.$setOnInsert for operator updates,
otherwise assign _id directly on the replacement document (mutating the
replacement object) when options.upsert is true and query lacks _id.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 11 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="apps/meteor/client/views/account/profile/AccountProfileForm.tsx">
<violation number="1" location="apps/meteor/client/views/account/profile/AccountProfileForm.tsx:200">
P2: Username field no longer required; empty username now passes validation and is submitted</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| required: t('Required_field', { field: t('Username') }), | ||
| validate: (username) => validateUsername(username), | ||
| }} | ||
| rules={{ validate: validateUsername }} |
There was a problem hiding this comment.
P2: Username field no longer required; empty username now passes validation and is submitted
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/client/views/account/profile/AccountProfileForm.tsx, line 200:
<comment>Username field no longer required; empty username now passes validation and is submitted</comment>
<file context>
@@ -154,218 +161,121 @@ const AccountProfileForm = (props: AllHTMLAttributes<HTMLFormElement>): ReactEle
- required: t('Required_field', { field: t('Username') }),
- validate: (username) => validateUsername(username),
- }}
+ rules={{ validate: validateUsername }}
render={({ field }) => (
- <TextInput
</file context>
3 participants
Proposed changes (including videos or screenshots)
This PR refactors
RegisterForm.tsxto reduce cyclomatic complexity and improve code maintainability, without introducing any functional or UI changes.Previously, the registration form combined form validation, API error handling, conditional logic, and UI rendering in a single component, requiring the
eslint-disable complexitydirective.This refactor improves separation of concerns by extracting logic into dedicated hooks while preserving all existing behavior.
Summary of changes:
useRegisterValidationuseRegisterErrorHandlingeslint-disable complexityfromRegisterForm.tsxThis is a refactor-only change aimed at improving readability and reducing future regression risk.
Issue(s)
Refactor to address excessive complexity and mixed concerns in
RegisterForm.tsx.Steps to test or reproduce
Further comments
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
Breaking Changes
✏️ Tip: You can customize this high-level summary in your review settings.