fix(ux): improve delete account modal with inline errors#36293
fix(ux): improve delete account modal with inline errors#36293sandranymark wants to merge 8 commits intoRocketChat:developfrom
Conversation
|
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 |
|
gabriellsh
left a comment
gabriellsh
left a comment
There was a problem hiding this comment.
There's some linting errors, would it be possible to fix those?
About the overall solution: It's a nice approach, but I think we could make it a little more declarative/generic if instead of using this "dependency inversion" pattern (accepting a callback to set the error in the confirmation fn), we could throw a custom error with the correct message, and wrap the the fn invocation in a try catch.
export class InputError extends Error {
constructor(message: string) {
super(message);
}
}if (error.errorType === 'error-invalid-password') {
throw new InputError(t('Invalid_password'));
}try {
await onConfirm(inputText);
onCancel();
} catch (error) {
if (error instanceof InputError) {
setError('credential', { message: error.message });
}
}This way we decouple the modal use from it's action, and we can also emit other types of errors in the future if we wish to.
WalkthroughIntroduces a new InputError class, updates AccountProfilePage to throw InputError on invalid password and close the modal before logout, and refactors ActionConfirmModal to use react-hook-form with an async onConfirm that surfaces field-specific errors and manages focus/accessibility. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant M as ActionConfirmModal
participant P as AccountProfilePage
participant S as Server (Delete Account API)
U->>M: Submit credential
M->>P: onConfirm(credential) [async]
P->>S: deleteOwnAccount(credential)
alt Success
S-->>P: OK
P-->>M: resolve()
M->>M: close via onCancel()
M-->>U: Modal closed
P->>P: logout()
else Invalid password
S-->>P: error-invalid-password
P-->>M: reject(InputError("Invalid_password"))
M->>M: set field error + focus input
M-->>U: Inline error shown
else Other error
S-->>P: other error
P-->>M: reject(Error)
M-->>U: Generic failure (no field error)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 (5)
apps/meteor/client/views/account/profile/DeleteAccountError.ts (1)
1-4: Name and prototype fix for Error subclassSet the error name and prototype to ensure correct instanceof behavior across transpiled targets and cleaner diagnostics.
-export class InputError extends Error { constructor(message: string) { -super(message); - } -} +export class InputError extends Error { + constructor(message: string) { + super(message); + this.name = 'InputError'; + // Ensure instanceof works when transpiled + Object.setPrototypeOf(this, new.target.prototype); + } +}apps/meteor/client/views/account/profile/AccountProfilePage.tsx (2)
108-110: Harden error-shape check for invalid passwordBackends sometimes emit either errorType or error. Consider accepting both to avoid missing the inline error path.
- if (error.errorType === 'error-invalid-password') { + if (error?.errorType === 'error-invalid-password' || error?.error === 'error-invalid-password') { throw new InputError(t('Invalid_password')); }
95-116: SSO/no-password flow — verified: client hashing matches server contractThe API /v1/users.deleteOwnAccount requires a password string; server deleteUserOwnAccount() checks local-password users via Accounts._checkPasswordAsync (digest lowercased) and otherwise compares SHA256(user.username) === password.trim() (apps/meteor/app/lib/server/methods/deleteUserOwnAccount.ts / apps/meteor/app/api/server/v1/users.ts). The client (apps/meteor/client/views/account/profile/AccountProfilePage.tsx) sends { password: SHA256(passwordOrUsername) } for both flows — correct.
- Optional: handle server 'error-invalid-username' the same way as 'error-invalid-password' (InputError) to surface a clear validation error for SSO/no-password users.
apps/meteor/client/views/account/profile/ActionConfirmModal.tsx (2)
62-88: A11y polish: native required + autocomplete hintsAdd required to inputs and set autocomplete to improve UX with screen readers and password managers.
<Controller name='credential' control={control} rules={{ required: t('Invalid_field') }} render={({ field }) => isPassword ? ( <PasswordInput {...field} id={inputId} + required + autoComplete='current-password' aria-labelledby={actionTextId} aria-describedby={errors.credential ? errorId : undefined} aria-invalid={Boolean(errors.credential)} aria-required='true' /> ) : ( <TextInput {...field} id={inputId} placeholder={t('Username')} + required + autoComplete='username' aria-labelledby={actionTextId} aria-describedby={errors.credential ? errorId : undefined} aria-invalid={Boolean(errors.credential)} aria-required='true' /> ) } />
15-16: Remove stale TODOThe component now uses react-hook-form; the TODO is obsolete.
-// TODO: Use react-hook-form const ActionConfirmModal = ({ isPassword, onConfirm, onCancel }: ActionConfirmModalProps) => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
apps/meteor/client/views/account/profile/AccountProfilePage.tsx(2 hunks)apps/meteor/client/views/account/profile/ActionConfirmModal.tsx(2 hunks)apps/meteor/client/views/account/profile/DeleteAccountError.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
apps/meteor/client/views/account/profile/AccountProfilePage.tsx (1)
apps/meteor/client/views/account/profile/DeleteAccountError.ts (1)
InputError(1-4)
apps/meteor/client/views/account/profile/ActionConfirmModal.tsx (1)
apps/meteor/client/views/account/profile/DeleteAccountError.ts (1)
InputError(1-4)
🔇 Additional comments (3)
apps/meteor/client/views/account/profile/AccountProfilePage.tsx (2)
24-24: Typed input error import looks goodUsing a dedicated InputError for field-level feedback is appropriate here.
100-100: Avoid double-close races with downstream modalsThis closes the modal on success (good). However, ActionConfirmModal currently also calls onCancel after onConfirm resolves, which can double-close or accidentally close a replacement modal (e.g., ConfirmOwnerChangeModal). I’m proposing removal of that auto-close in ActionConfirmModal to avoid races; keeping this line is fine once that change is applied.
apps/meteor/client/views/account/profile/ActionConfirmModal.tsx (1)
11-11: API change: onConfirm is now async — update/verify all call sitesNo direct JSX usages were found in your search output; check wrappers/re-exports and any external consumers and ensure passed onConfirm handlers return Promise (mark handlers async or return a Promise and handle rejections).
apps/meteor/client/views/account/profile/ActionConfirmModal.tsx
Outdated
Show resolved
Hide resolved
aleksandernsilva
left a comment
aleksandernsilva
left a comment
There was a problem hiding this comment.
Hey, changes are looking good. I left comments on a couple of small improvements. I also noticed the code still has a few lint errors, could you take a look?
apps/meteor/client/views/account/profile/ActionConfirmModal.tsx
Outdated
Show resolved
Hide resolved
apps/meteor/client/views/account/profile/ActionConfirmModal.tsx
Outdated
Show resolved
Hide resolved
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
dougfabris
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #36293 +/- ##
===========================================
+ Coverage 70.55% 70.64% +0.09%
===========================================
Files 3188 3186 -2
Lines 112651 112655 +4
Branches 20400 20429 +29
===========================================
+ Hits 79476 79583 +107
+ Misses 31114 31028 -86
+ Partials 2061 2044 -17
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
gabriellsh
left a comment
gabriellsh
left a comment
There was a problem hiding this comment.
Failing tests related to changes
dougfabris
added
the
stat: QA assured
4 participants
Proposed changes (including videos or screenshots)
This PR improves the Delete My Account modal (
ActionConfirmModal) by adding inline error handling usingreact-hook-form:useStatelogic withreact-hook-form("Invalid password")under the input field.Issue(s)
Steps to test or reproduce
Navigate to My Account → Profile
Inline validation error should appear below the field
"Invalid password" message should display under the field
Account is deleted, modal closes, user is logged out, and a success toast is shown
Further comments
WA-58
Summary by CodeRabbit
Bug Fixes
Refactor