chore: refactor the submit function on ProfileView#7353
Conversation
OtavioStasiak
temporarily deployed
to
approve_e2e_testing
GitHub Actions
Inactive
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📜 Recent review details⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
WalkthroughProfileView extracts payload construction to ChangesProfile save flow refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
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 |
OtavioStasiak
had a problem deploying
to
android_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
android_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
android_build
GitHub Actions
Error
OtavioStasiak
temporarily deployed
to
approve_e2e_testing
GitHub Actions
Inactive
OtavioStasiak
had a problem deploying
to
android_build
GitHub Actions
Error
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
app/views/ProfileView/index.tsx (2)
164-164: ⚡ Quick winReplace
as anycast with proper typing.Using
as anybypasses TypeScript's type safety. The parameterpshould have an explicit type based onConfirmEmailChangeActionSheetContent'sonSubmitprop signature.♻️ Suggested improvement
- onSubmit={p => { + onSubmit={(password: string) => { hideActionSheet(); - setValue('currentPassword', p as any); + setValue('currentPassword', password); submit().catch(err => { console.error('Profile submit failed:', err); }); }}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/views/ProfileView/index.tsx` at line 164, The line uses a loose cast setValue('currentPassword', p as any) which bypasses TypeScript safety; change the handler so parameter p is explicitly typed to match the ConfirmEmailChangeActionSheetContent onSubmit signature (e.g. declare the callback param as the exact type from ConfirmEmailChangeActionSheetContent['onSubmit'] or extract/define a named type for the submitted payload) and pass that typed value to setValue without using any, ensuring setValue('currentPassword', p) uses the correctly typed property (currentPassword) from that payload.
216-229: 💤 Low valueConsider adding a comment to clarify the recursive retry logic.
The function recursively calls
submit()at line 223, which will invokehandleTwoFactorChallengeagain if needed. While this is protected by thetwoFactorCodestate and modal cancellation, a brief comment would help future maintainers understand the retry flow.📝 Suggested comment
try { const code = await twoFactor({ method: e.details.method, invalid: e?.error === 'totp-invalid' && !!twoFactorCode }); setTwoFactorCode(code as any); + // Recursively retry submit with the 2FA code await submit(); return true; } catch {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/views/ProfileView/index.tsx` around lines 216 - 229, Add a short clarifying comment explaining the recursive retry flow in handleTwoFactorChallenge: note that handleTwoFactorChallenge can trigger twoFactor(...) to set twoFactorCode and then calls submit(), which may re-enter handleTwoFactorChallenge if the server still requires TOTP; also mention the protections (the twoFactorCode state prevents infinite loops and modal cancellation is handled in the catch) so future maintainers understand why this recursion is intentional. Place the comment above the handleTwoFactorChallenge function or immediately before the await submit() call and reference the twoFactorCode state and twoFactor(...) modal behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/views/ProfileView/index.tsx`:
- Line 165: The call to the async function submit() is currently a floating
promise; update the invocation to handle errors explicitly (e.g.,
submit().catch(err => processLogger?.error?.('Profile submit failed', err) or
set component error state) so the promise cannot fail silently; ensure you call
submit() with .catch(...) rather than using void or await in this location and
reference the submit() function in ProfileView to implement the error handler.
- Around line 210-212: The guard currently calls e?.message.startsWith(email)
which will throw when email is null; modify the conditional in the error
handling (the branch that calls setError) to ensure email is a non-null string
first (e.g. check typeof email === 'string' or email != null) before calling
startsWith, keeping the existing e?.error === 'error-field-unavailable' check
and leaving setError('email', ...) unchanged.
- Around line 178-203: The applySaveSuccess handler is dispatching setUser twice
and uses setValue/getValues to update form fields right before reset, causing
redundant Redux updates and wasted work; modify applySaveSuccess to compute the
merged updatedUser once, call dispatch(setUser(updatedUser)) a single time
(include customFields on the merged object if present), remove the earlier
conditional dispatch and the manual setValue loop, and then call reset(...) with
values derived from that same updatedUser so the form and store are consistent
without duplicate dispatches.
---
Nitpick comments:
In `@app/views/ProfileView/index.tsx`:
- Line 164: The line uses a loose cast setValue('currentPassword', p as any)
which bypasses TypeScript safety; change the handler so parameter p is
explicitly typed to match the ConfirmEmailChangeActionSheetContent onSubmit
signature (e.g. declare the callback param as the exact type from
ConfirmEmailChangeActionSheetContent['onSubmit'] or extract/define a named type
for the submitted payload) and pass that typed value to setValue without using
any, ensuring setValue('currentPassword', p) uses the correctly typed property
(currentPassword) from that payload.
- Around line 216-229: Add a short clarifying comment explaining the recursive
retry flow in handleTwoFactorChallenge: note that handleTwoFactorChallenge can
trigger twoFactor(...) to set twoFactorCode and then calls submit(), which may
re-enter handleTwoFactorChallenge if the server still requires TOTP; also
mention the protections (the twoFactorCode state prevents infinite loops and
modal cancellation is handled in the catch) so future maintainers understand why
this recursion is intentional. Place the comment above the
handleTwoFactorChallenge function or immediately before the await submit() call
and reference the twoFactorCode state and twoFactor(...) modal behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ebd9570f-b68b-443c-8e25-8d6398332b80
📒 Files selected for processing (2)
app/views/ProfileView/index.tsxapp/views/ProfileView/methods/buildProfileParams.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: ESLint and Test / run-eslint-and-test
- GitHub Check: format
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,jsx,tsx}: Use descriptive names for functions, variables, and classes that clearly convey their purpose
Write comments that explain the 'why' behind code decisions, not the 'what'
Keep functions small and focused on a single responsibility
Use const by default, let when reassignment is needed, and avoid var
Prefer async/await over .then() chains for handling asynchronous operations
Use explicit error handling with try/catch blocks for async operations
Avoid deeply nested code; refactor complex logic into helper functions
Files:
app/views/ProfileView/methods/buildProfileParams.tsapp/views/ProfileView/index.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript for type safety; add explicit type annotations to function parameters and return types
Prefer interfaces over type aliases for defining object shapes in TypeScript
Use enums for sets of related constants rather than magic strings or numbersUse TypeScript with strict mode and baseUrl set to app/ for import resolution
Files:
app/views/ProfileView/methods/buildProfileParams.tsapp/views/ProfileView/index.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: Use Prettier with tabs, single quotes, 130 char width, no trailing commas, arrow parens avoid, bracket same line
Use@rocket.chat/eslint-configbase with React, React Native, TypeScript, Jest plugins
Files:
app/views/ProfileView/methods/buildProfileParams.tsapp/views/ProfileView/index.tsx
app/views/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
View components (70+ screen components) should be placed in app/views/ directory
Files:
app/views/ProfileView/methods/buildProfileParams.tsapp/views/ProfileView/index.tsx
🧠 Learnings (1)
📚 Learning: 2026-04-30T17:07:51.020Z
Learnt from: diegolmello
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7274
File: app/lib/services/voip/MediaCallEvents.ts:0-0
Timestamp: 2026-04-30T17:07:51.020Z
Learning: In this Rocket.Chat React Native codebase, the ESLint rule `no-void: error` is enforced. When you see a promise returned from an async call that is not awaited (a “floating promise”), do not silence it with the `void somePromise()` pattern. Instead, handle the promise explicitly by attaching `.catch(...)` (or otherwise awaiting/handling the error) so unhandled-rejection risks are addressed in a way that satisfies the existing ESLint configuration.
Applied to files:
app/views/ProfileView/methods/buildProfileParams.tsapp/views/ProfileView/index.tsx
🔇 Additional comments (3)
app/views/ProfileView/index.tsx (2)
172-176: LGTM!
231-266: LGTM!app/views/ProfileView/methods/buildProfileParams.ts (1)
20-20: ⚡ Quick winPrevent
params.email = nullfrom reaching the API
ProfileViewvalidatesyup.string().email(...).required(...), andsubmitreturns early when validation fails, so the UI shouldn’t submit withemail: null(i.e., clearing the email field isn’t supported via this flow).- While
buildProfileParamswould setparams.email = nulland skip the password confirmation (requirePassword = !!params.email), that path shouldn’t be reachable through the normal submit button.
OtavioStasiak
had a problem deploying
to
android_build
GitHub Actions
Error
OtavioStasiak
temporarily deployed
to
approve_e2e_testing
GitHub Actions
Inactive
OtavioStasiak
temporarily deployed
to
android_build
GitHub Actions
Inactive
|
Actionable comments posted: 0 |
|
iOS Build Available Rocket.Chat 4.73.0.108990 |
OtavioStasiak
requested a deployment
to
upload_android
GitHub Actions
Waiting
|
Android Build Available Rocket.Chat 4.73.0.108995 Internal App Sharing: https://play.google.com/apps/test/RQQ8k09hlnQ/ahAO29uNSG5GKMRQJ8yDIiccgMrQi78M-x8hsHTi73qlNdY4uxuzm5gKvZuxQPy9RtVEZa7fkbnALbxbYu7T8QY-8y |
1 participant
Proposed changes
Break the submit function into smaller functions to improve the readability of the code.
Issue(s)
https://rocketchat.atlassian.net/browse/NATIVE-1037
How to test or reproduce
Screenshots
Types of changes
Checklist
Further comments
Summary by CodeRabbit
Bug Fixes
Refactor