Conversation
lbjunq
requested review from
anicioalexandre and
priscilladeroode
as code owners
|
📝 WalkthroughWalkthroughIntroduces an email-change workflow: a new Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant F as Form (UI)
participant H as useRequestEmailChange (hook)
participant A as AuthApi
participant S as Server
U->>F: submit new email
F->>H: handleSubmit -> submit()
H->>A: requestEmailChange({ newEmail })
A->>S: POST /change-email
S-->>A: 200 { newEmail } / 4xx error
A-->>H: resolve / reject
alt success
H->>F: call onSuccess callbacks
else error
H->>F: map API errors to form errors (if enabled)
H->>F: call onError callbacks
end
H->>A: resendRequestEmailChange() (when invoked)
A->>S: POST /change-email/resend-confirm
S-->>A: 200 / error
A-->>H: resolve / reject
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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
🤖 Fix all issues with AI agents
In
`@packages/authentication/modules/access/useRequestEmailChange/__tests__/useRequestEmailChange.test.tsx`:
- Around line 38-42: Tests are failing because the test props use a non-existent
options property; update all test instances to use requestEmailChangeOptions
(the property defined on the UseRequestEmailChange interface) instead of options
— e.g., where you set options: { onSuccess: () => { hasOnSuccessRan = true } },
change to requestEmailChangeOptions: { onSuccess: () => { hasOnSuccessRan = true
} }; apply the same replacement for every occurrence in this test file
(including the other blocks that set onSuccess/onError callbacks) so the mock
props align with the UseRequestEmailChange interface.
🧹 Nitpick comments (4)
packages/authentication/modules/access/useRequestEmailChange/types.ts (1)
17-19: Duplicate type definition withpackages/authentication/types/auth.ts.
RequestEmailChangeRequestis also defined as an interface inpackages/authentication/types/auth.ts(lines 63-65). Consider importing from the centralized auth types instead to avoid duplication and maintain a single source of truth.🔧 Suggested fix
-export type RequestEmailChangeRequest = { - newEmail: string -} +export type { RequestEmailChangeRequest } from '../../../types/auth'packages/authentication/modules/access/useRequestEmailChange/__tests__/useRequestEmailChange.test.tsx (1)
17-19: Consider addingbeforeEachto reset the mock properly.Using
mockClear()inafterEachis fine, but if a test fails before setting up the mock, subsequent tests might behave unexpectedly. Consider usingbeforeEachwithjest.resetAllMocks()for more robust test isolation.packages/authentication/modules/access/useRequestEmailChange/constants.ts (1)
7-9: Consider reordering validators for better error messaging.With the current order
.email().min(1), an empty string will fail at.email()with "Please provide a properly formatted email address" rather than "This field is required". If you want empty values to show the "required" message, reverse the order:💡 Suggested improvement
export const DEFAULT_VALIDATION_SCHEMA = z.object({ - newEmail: z.string().email(ZOD_MESSAGE.email).min(1, ZOD_MESSAGE.required), + newEmail: z.string().min(1, ZOD_MESSAGE.required).email(ZOD_MESSAGE.email), })packages/authentication/modules/access/useRequestEmailChange/index.ts (1)
115-121: Empty catch block silently swallows errors.While the comment explains that
mutateAsyncraises errors on API failures, the empty catch block makes debugging difficult. Consider at minimum logging in development mode, or removing the try-catch if error handling is already managed by the mutation'sonErrorcallback.💡 Suggested alternatives
Option 1: Remove try-catch (errors handled by mutation's onError)
const handleSubmit: SubmitHandler<RequestEmailChangeRequest> = async (values) => { - try { - await requestEmailChangeMutation.mutateAsync(values) - } catch (error) { - // mutateAsync will raise an error if there's an API error - } + await requestEmailChangeMutation.mutateAsync(values) }Option 2: Keep catch but add development logging
const handleSubmit: SubmitHandler<RequestEmailChangeRequest> = async (values) => { try { await requestEmailChangeMutation.mutateAsync(values) } catch (error) { - // mutateAsync will raise an error if there's an API error + // Error is already handled by mutation's onError callback + if (process.env.NODE_ENV === 'development') { + console.debug('Request email change failed:', error) + } } }
...authentication/modules/access/useRequestEmailChange/__tests__/useRequestEmailChange.test.tsx
Outdated
Show resolved
Hide resolved
|
2 participants
❌ RN - Change Email in Settings
Key details
Description
Acceptance Criteria
Design Link: https://www.figma.com/design/z5ZdVlKEMmGIX4GVnjwfxA/BaseApp---NATIVE?node-id=6107-19479&t=ZYZocuoVLM11VrtX-0
Notes
Check the web app flow, so that the designs match the flow.
Approvd
https://app.approvd.io/silverlogic/BA/stories/39019
Demo: In progress...
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.