Enhancement: Support email editing on My Account page#3777
Enhancement: Support email editing on My Account page#3777ankit-mehta07 wants to merge 14 commits intoglific:masterfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR makes several coordinated changes: modifies Auth registration to update Formik values with captcha via async setValues and then calls submitForm (removing an explicit setLoading call); adds phoneNumber validation to ResetPasswordConfirmOTP; converts MyAccount profile form to Formik with validation, makes email editable, updates updateCurrentUser error handling, and adjusts GraphQL mutation to return user.phone and user.email; expands GraphQL mocks with __typename annotations and adds/updates tests for MyAccount and ResetPassword flows. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/containers/Auth/Auth.tsx (1)
74-79:⚠️ Potential issue | 🟠 MajorLoading state prematurely reset on resubmission after an error
When
loadingtransitions totrue(form resubmit) and a staleerrorMessagefrom the previous request is still set (callers such asResetPasswordConfirmOTPnever clearauthErrorbetween attempts), this effect fires, seesloading && errorMessage, and immediately callssetLoading(false)— so the spinner flashes and disappears while the API call is still in flight.
loadingshould be removed from the deps. Stopping the spinner whenevererrorMessageis set is sufficient and correct:🐛 Proposed fix
useEffect(() => { - // Stop loading if any error - if (loading && errorMessage) { + if (errorMessage) { setLoading(false); } - }, [loading, errorMessage]); + }, [errorMessage]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/containers/Auth/Auth.tsx` around lines 74 - 79, The effect in Auth.tsx currently checks both loading and errorMessage and includes loading in the dependency array, causing setLoading(false) to run on a new submit if a stale errorMessage exists; update the useEffect (the effect that references loading, errorMessage, and calls setLoading) to only depend on errorMessage and change the condition to stop the spinner when errorMessage is truthy (i.e., remove loading from deps and only call setLoading(false) when errorMessage is present) so resubmissions while an API call is in-flight don't prematurely clear loading; this touches the useEffect that interacts with loading, errorMessage and setLoading (and is relevant to callers like ResetPasswordConfirmOTP which don't clear authError).
🧹 Nitpick comments (3)
src/containers/MyAccount/MyAccount.tsx (1)
197-202: Save button is missing aloadingprop — multiple concurrent submissions possible
useMutationreturns a loading flag that is currently discarded:const [updateCurrentUser] = useMutation(…) // ↑ loading not extractedWhile the mutation is in-flight the Save button remains fully clickable, allowing duplicate submissions. The
Buttoncomponent accepts aloadingprop that disables it and shows a spinner.✨ Proposed fix
- const [updateCurrentUser] = useMutation(UPDATE_CURRENT_USER, { + const [updateCurrentUser, { loading: updateLoading }] = useMutation(UPDATE_CURRENT_USER, {<Button variant="contained" color="primary" onClick={submitForm} className={styles.Button} + loading={updateLoading} > {t('Save')} </Button>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/containers/MyAccount/MyAccount.tsx` around lines 197 - 202, The Save button can be clicked multiple times because the mutation's loading state isn't used; update the useMutation call that defines updateCurrentUser to also extract the loading flag (e.g., const [updateCurrentUser, { loading }] = useMutation(...)) and pass that loading value into the Button component as its loading prop (Button loading={loading}) so the button is disabled/shows a spinner while submitForm is in-flight.src/containers/Auth/ResetPassword/ResetPasswordConfirmOTP.test.tsx (1)
93-95: Strengthen the assertion —toBeGreaterThan(0)is too permissiveWith
phoneNumberandOTPboth empty, Yup fires "Input required" on both fields (thepasswordfield usesyupPasswordValidationwhich produces a different message). The assertion should be exact:✨ Proposed improvement
- expect(screen.getAllByText('Input required').length).toBeGreaterThan(0); + expect(screen.getAllByText('Input required')).toHaveLength(2);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/containers/Auth/ResetPassword/ResetPasswordConfirmOTP.test.tsx` around lines 93 - 95, The test currently asserts expect(screen.getAllByText('Input required').length).toBeGreaterThan(0), which is too permissive; update the assertion to assert the exact expected count of Yup "Input required" errors for the empty fields (phoneNumber and OTP) by changing it to expect(screen.getAllByText('Input required').length).toBe(2) or otherwise assert equality to 2 in ResetPasswordConfirmOTP.test.tsx (locate the usage of screen.getAllByText('Input required') in the test and replace the toBeGreaterThan(0) check with a toBe(2) assertion).src/containers/MyAccount/MyAccount.test.tsx (1)
19-32: Redundant mock — already present inupdateUserQuery
updateUserQuery(spread intomocksat line 15) already contains an identical entry for{ input: { name: 'John Doe', email: '[email protected]' } }returning the same error shape (seesrc/mocks/User.tsxlines 155–168). This inline mock is never consumed — the first matching entry from...updateUserQueryis always used.✨ Proposed clean-up
const mocks = [ getCurrentUserQuery, ...updateUserQuery, getCurrentUserQuery, getOrganizationLanguagesQuery, getOrganizationLanguagesQuery, - { - request: { - query: UPDATE_CURRENT_USER, - variables: { input: { name: 'John Doe', email: '[email protected]' } }, - }, - result: { - data: { - updateCurrentUser: { - errors: [{ key: 'email', message: 'Email already exists' }], - user: null, - }, - }, - }, - }, ];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/containers/MyAccount/MyAccount.test.tsx` around lines 19 - 32, Remove the redundant inline mock object for UPDATE_CURRENT_USER from the test mocks: the identical entry is already provided by updateUserQuery (spread into mocks), so delete the duplicate request/result block in MyAccount.test.tsx to avoid unused/never-consumed mocks and rely on updateUserQuery’s mock; keep UPDATE_CURRENT_USER and updateUserQuery references intact so the test still resolves the error response as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/containers/Auth/Auth.tsx`:
- Around line 137-142: handlePhone currently mutates the prop
initialFormValues.phone (wrong key) which is a React anti-pattern and won't
update Formik; instead, remove the prop mutation and wire handlePhone to call
Formik's setFieldValue to update "phoneNumber". Expose setFieldValue from the
Formik render prop (where Formik is instantiated) and change handlePhone to
invoke setFieldValue('phoneNumber', value) (or use the formik helper) so
ResetPasswordConfirmOTP's name: 'phoneNumber' field updates correctly without
mutating initialFormValues.
In `@src/containers/MyAccount/MyAccount.tsx`:
- Around line 175-206: The Formik form does not reinitialize when the
query-driven initialValues (userName/userPhone/userEmail derived from userData)
change after updateCurrentUser, causing dirty to remain true and the email field
to show the old value; enable Formik's reinitialization by adding
enableReinitialize={true} to the Formik component (the one with initialValues
and onSubmit) so it will pick up updated initialValues from userData and clear
dirty after a successful mutation.
---
Outside diff comments:
In `@src/containers/Auth/Auth.tsx`:
- Around line 74-79: The effect in Auth.tsx currently checks both loading and
errorMessage and includes loading in the dependency array, causing
setLoading(false) to run on a new submit if a stale errorMessage exists; update
the useEffect (the effect that references loading, errorMessage, and calls
setLoading) to only depend on errorMessage and change the condition to stop the
spinner when errorMessage is truthy (i.e., remove loading from deps and only
call setLoading(false) when errorMessage is present) so resubmissions while an
API call is in-flight don't prematurely clear loading; this touches the
useEffect that interacts with loading, errorMessage and setLoading (and is
relevant to callers like ResetPasswordConfirmOTP which don't clear authError).
---
Nitpick comments:
In `@src/containers/Auth/ResetPassword/ResetPasswordConfirmOTP.test.tsx`:
- Around line 93-95: The test currently asserts
expect(screen.getAllByText('Input required').length).toBeGreaterThan(0), which
is too permissive; update the assertion to assert the exact expected count of
Yup "Input required" errors for the empty fields (phoneNumber and OTP) by
changing it to expect(screen.getAllByText('Input required').length).toBe(2) or
otherwise assert equality to 2 in ResetPasswordConfirmOTP.test.tsx (locate the
usage of screen.getAllByText('Input required') in the test and replace the
toBeGreaterThan(0) check with a toBe(2) assertion).
In `@src/containers/MyAccount/MyAccount.test.tsx`:
- Around line 19-32: Remove the redundant inline mock object for
UPDATE_CURRENT_USER from the test mocks: the identical entry is already provided
by updateUserQuery (spread into mocks), so delete the duplicate request/result
block in MyAccount.test.tsx to avoid unused/never-consumed mocks and rely on
updateUserQuery’s mock; keep UPDATE_CURRENT_USER and updateUserQuery references
intact so the test still resolves the error response as before.
In `@src/containers/MyAccount/MyAccount.tsx`:
- Around line 197-202: The Save button can be clicked multiple times because the
mutation's loading state isn't used; update the useMutation call that defines
updateCurrentUser to also extract the loading flag (e.g., const
[updateCurrentUser, { loading }] = useMutation(...)) and pass that loading value
into the Button component as its loading prop (Button loading={loading}) so the
button is disabled/shows a spinner while submitForm is in-flight.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/containers/Auth/Auth.tsxsrc/containers/Auth/ResetPassword/ResetPasswordConfirmOTP.test.tsxsrc/containers/Auth/ResetPassword/ResetPasswordConfirmOTP.tsxsrc/containers/MyAccount/MyAccount.test.tsxsrc/containers/MyAccount/MyAccount.tsxsrc/graphql/mutations/User.tssrc/mocks/User.tsx
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/containers/Chat/ChatMessages/StatusBar/StatusBar.test.tsx (1)
63-63:⚠️ Potential issue | 🟡 MinorPre-existing: duplicate/misleading test description.
Both lines 50 and 63 use
'it should show suspended message if balance is negative', but the test at line 63 actually covers the inactive phones scenario (getInactiveStatusMock, balance14.17). This doesn't affect test execution, but it makes the test report misleading.✏️ Suggested fix
-test('it should show suspended message if balance is negative', async () => { +test('it should show inactive phones message when one or more phones are not active', async () => { render(wrapper('{"balance":14.17}', getInactiveStatusMock));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/containers/Chat/ChatMessages/StatusBar/StatusBar.test.tsx` at line 63, The test named 'it should show suspended message if balance is negative' is a duplicate/misleading description; update the test description for the second occurrence (the test that uses getInactiveStatusMock and checks balance 14.17) to accurately reflect the inactive phones scenario (e.g., "it should show inactive phones message when account has inactive phones"), so locate the test(...) invocation in StatusBar.test.tsx that uses getInactiveStatusMock and change its description string to match the behavior being asserted.src/components/UI/SearchBar/SearchBar.tsx (1)
72-79:⚠️ Potential issue | 🟡 MinorMisleading
aria-labelon the advanced search button.Line 75 has
aria-label="toggle password visibility", which appears to be a copy-paste artifact. This is a search filter button, not a password toggle. While this is likely pre-existing, it's worth fixing since you're already touching this block.♻️ Suggested fix
<IconButton data-testid="advanced-search-icon" disableFocusRipple - aria-label="toggle password visibility" + aria-label="advanced search" onClick={(e: any) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/UI/SearchBar/SearchBar.tsx` around lines 72 - 79, The IconButton with data-testid "advanced-search-icon" currently uses a misleading aria-label "toggle password visibility"; update it to an accurate, descriptive label such as "toggle advanced search" or "open advanced search" so screen readers convey its purpose; locate the IconButton (the element with onClick calling Track('Advanced search') and handleClick(e, 'search', 'update')) and replace the aria-label value accordingly.src/containers/WaGroups/GroupChatInterface/GroupInterface.test.tsx (1)
11-54:⚠️ Potential issue | 🟡 MinorMissing
__typenameon the firstsearchentry breaks cache normalization consistency.After switching from
new InMemoryCache({ addTypename: false })to the defaultnew InMemoryCache(), Apollo normalizes objects using__typename + idas the cache key. The second (waGroup_9, line 57) and third (waGroup_1, line 96) entries correctly carry__typename: 'WaConversation'and will be stored asWaConversation:waGroup_9/WaConversation:waGroup_1. The first entry (waGroup_6, line 18) has no top-level__typename, so Apollo falls back to a path-based reference (ROOT_QUERY.search.0), making it non-normalized relative to the other two. Consequences:
- Apollo may emit a
"Missing field '__typename' in {…}"warning in CI output.- Any future mutation or query result that supplies
WaConversation:waGroup_6will not merge with the path-stored entry, leaving the test reading stale data.- The test at line 187–194 relies on
waGroup_6being the first element returned ("WA Group 1"), making it fragile if search result ordering ever changes and the path key shifts.Add the missing
__typenamefield to align with the other entries and with the intent of this PR's cache migration:🐛 Proposed fix
search: [ { id: 'waGroup_6', + __typename: 'WaConversation', messages: [🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/containers/WaGroups/GroupChatInterface/GroupInterface.test.tsx` around lines 11 - 54, The first search entry written to the InMemoryCache is missing a top-level __typename which breaks Apollo normalization; update the test cache.writeQuery data for the first search item (the object with id 'waGroup_6' / label 'WA Group 1') to include "__typename: 'WaConversation'" so it matches the other entries and aligns with GROUP_SEARCH_QUERY and GROUP_QUERY_VARIABLES expectations when using new InMemoryCache().
🧹 Nitpick comments (10)
src/mocks/Collection.tsx (1)
97-97:getCollectionsQueryis missing the same__typenameadditions for consistency.
getCollectionsQuery(lines 50–85) uses the sameGET_COLLECTIONSquery and would benefit from the same treatment, otherwise tests relying on that fixture may see inconsistent Apollo cache-normalization behavior.♻️ Proposed addition to
getCollectionsQueryresult: { data: { groups: [ { + __typename: 'Group', id: '1', label: 'Staff group', isRestricted: true, }, ], }, }, }, { request: { query: GET_COLLECTIONS, variables: { groupType: 'WABA' }, }, result: { data: { groups: [ { + __typename: 'Group', id: '1', label: 'Staff group', isRestricted: true, }, ], }, }, },Also applies to: 114-114
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mocks/Collection.tsx` at line 97, The getCollectionsQuery fixture is missing __typename fields which can break Apollo cache normalization; update the getCollectionsQuery mock to include matching __typename values (e.g., for the root query, each collection object and nested Group items) the same way other fixtures do, ensuring the GET_COLLECTIONS response shape in the mock includes __typename entries for all types referenced so tests and Apollo cache behave consistently.src/mocks/Search.tsx (1)
654-654: Mockmessagescount doesn't match querymessageOpts.limit.
searchWithDateFiltersspecifiesmessageOpts: { limit: 20 }in the request variables (Line 630), but the result returnsmessages(25, 2)— generating 25 messages. While ApolloMockedProviderwill return whatever mock data you specify regardless of the limit, this mismatch can confuse future maintainers. Consider usingmessages(20, 2)to keep the mock internally consistent.Also applies to: 673-673
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mocks/Search.tsx` at line 654, The mock response for searchWithDateFilters is returning 25 messages even though the request variables set messageOpts.limit to 20; update the mock data calls messages(25, 2) to messages(20, 2) in the Search.tsx mocks used by searchWithDateFilters (and the other identical occurrence) so the mocked messages count matches messageOpts.limit and avoids confusing future maintainers.src/mocks/Contact.tsx (2)
184-185: Same__typenameplacement concern as ingetContactSampleQuery.
__typename: 'Contact'(line 184) precedes...attributes(line 185) and can be overwritten by a caller. Moving it after the spread makes it authoritative.♻️ Proposed fix
contact: { - __typename: 'Contact', ...attributes, name: 'Default User', phone: '+919820198765', maskedPhone: '+919820198765', lastMessageAt: date.toISOString(), groups: [ { __typename: 'Group', id: '1', label: 'Default collection', users: [], }, ], fields: {}, status: 'VALID', optinTime: '2021-08-19T09:28:01Z', optoutTime: null, optinMethod: 'BSP', optoutMethod: null, settings: {}, + __typename: 'Contact', },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mocks/Contact.tsx` around lines 184 - 185, The __typename property in the Contact mock is placed before the spread of attributes and can be overridden by callers; update the mock in src/mocks/Contact.tsx so that __typename: 'Contact' appears after the ...attributes spread (same change you made for getContactSampleQuery) to make the type authoritative and prevent callers from overwriting it.
89-100:__typename: 'Contact'can be silently overridden by...contactDetails.
__typename: 'Contact'(line 89) is declared before...contactDetails(line 100). Any caller passing a top-level__typenameincontactDetailswould silently replace the intended type. Current callers are safe, but moving__typenameafter the spread makes the intent unambiguous.♻️ Proposed fix
contact: { - __typename: 'Contact', id: '1', name: null, activeProfile: null, phone: '+919820198765', language: { __typename: 'Language', id: '1', label: 'English' }, groups: [], status: 'VALID', bspStatus: 'SESSION_AND_HSM', settings: {}, fields: '{}', ...contactDetails, + __typename: 'Contact', },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mocks/Contact.tsx` around lines 89 - 100, The object literal in src/mocks/Contact.tsx currently sets __typename: 'Contact' before spreading ...contactDetails, allowing a caller-provided __typename to override it; move the __typename: 'Contact' property to after the ...contactDetails spread (i.e., place __typename at the end of the returned object) so the mock's __typename cannot be silently overwritten by contactDetails while leaving the rest of the fields and the ...contactDetails spread intact.src/components/floweditor/FlowEditor.test.tsx (1)
219-219: Fixed previously-vacuous assertions — good catch.The old
waitFor(() => expect(screen.findByText(...)))pattern was a silent no-op:screen.findByTextreturns aPromise,expect(Promise)with no matcher never throws, sowaitForexited immediately without verifying anything. Every one of those assertions was a false-positive.The new
expect(await screen.findByText(...)).toBeInTheDocument()correctly awaits resolution and will fail fast if the element never appears. Applied uniformly across all 11 changed sites.Minor:
.toBeInTheDocument()after anyfindBy*query is technically redundant — iffindByText/findByTestIdresolves, the returned element is guaranteed to be in the document. It's harmless and adds readability, so no change is necessary, but worth noting for consistency if the team prefers leaner assertions.Also applies to: 232-234, 245-245, 257-257, 281-281, 312-312, 324-326, 334-334, 347-347, 361-361
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/floweditor/FlowEditor.test.tsx` at line 219, Tests were using waitFor(() => expect(screen.findByText(...))) which is a no-op because screen.findByText returns a Promise and expect(Promise) without a matcher never throws; replace those patterns by awaiting the finder (use await screen.findByText(...) or await screen.findByTestId(...)) and then assert the resolved element with toBeInTheDocument() (or another matcher) — update all occurrences of waitFor + expect(screen.findByText/FindByTestId) to use await + expect(element).toBeInTheDocument(), e.g., change waitFor(() => expect(screen.findByText('help workflow'))) to expect(await screen.findByText('help workflow')).toBeInTheDocument() and similarly for the other findBy* usages.src/mocks/User.tsx (1)
187-220: Add__typenamefields togetCurrentUserInvalidRoleQueryfor consistency withgetCurrentUserQuery.The mock currently lacks
__typenamefields on nested objects (user,contact,groups,organization,language), while the primarygetCurrentUserQueryincludes them. Since the test using this mock (Login.test.tsx) does not explicitly setaddTypename={false}on MockedProvider, the Apollo client will attempt to add__typenameautomatically, creating a mismatch that may cause cache warnings or query resolution issues.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mocks/User.tsx` around lines 187 - 220, The mock getCurrentUserInvalidRoleQuery is missing __typename fields on nested objects which causes Apollo to mismatch with the real getCurrentUserQuery; update getCurrentUserInvalidRoleQuery (the mock for GET_CURRENT_USER) to include __typename on currentUser.user and its nested objects: add __typename for the user object, for contact, for each item in groups, and for organization and language, using the same typename strings used in getCurrentUserQuery so the shapes match exactly.src/containers/Chat/ChatConversations/ChatConversations.test.tsx (1)
13-13: Consider adding__typenameto manual cache write for consistency with mocks.The
cache.writeQuerycall (lines 14–79) lacks__typenamefields on nested objects (contact,messages,sender,receiver,contextMessage), while the mocks inChatConversationMocksinclude them. This inconsistency means Apollo will store the manually written data without normalization (using the query key rather than entity IDs), whereas the mocked responses normalize properly.Since the tests are UI-focused and don't depend on cache normalization or entity lookups, this works fine. However, enriching the
writeQuerydata with__typename: 'Conversation'for the root and appropriate type names for nested objects would bring it into alignment with the rest of the codebase and avoid potential cache-related surprises if the tests evolve.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/containers/Chat/ChatConversations/ChatConversations.test.tsx` at line 13, The manual cache.writeQuery against the InMemoryCache is missing __typename fields causing inconsistent normalization; update the data object passed to cache.writeQuery (the one created alongside InMemoryCache / variable cache) to include "__typename": "Conversation" on the root and add appropriate __typename entries on nested objects (e.g., contact -> "Contact", each message -> "Message", sender -> "User", receiver -> "User", contextMessage -> its type) so the manual write mirrors the ChatConversationMocks structure and enables consistent Apollo cache normalization.src/containers/Chat/ChatInterface/ChatInterface.test.tsx (2)
345-363: These two tests are identical in behavior — one should be differentiated or removed.
"send message to whatsapp group"(lines 345-353) and"should send message to whatsapp group collections"(lines 355-363) have the exact same setup, actions, and (lack of) assertions. Neither test actually verifies WhatsApp group–specific behavior or collection-specific behavior — they both render the sameChatInterfacewith the same cache data for a regular contact.Consider either:
- Differentiating them (e.g., render with
collectionTypeprop for the collections test, and use a WA group contact in the cache for the group test).- Removing one if they're truly redundant.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/containers/Chat/ChatInterface/ChatInterface.test.tsx` around lines 345 - 363, The two tests 'send message to whatsapp group' and 'should send message to whatsapp group collections' are identical; either remove the redundant test or make the second actually exercise collection-specific behavior: update the second test to render <ChatInterface /> with a collections context/prop (e.g., set cache or pass collectionType='collections' or a WA group contact fixture) and assert WA-group/collection-specific UI or network call differences (use the same helpers setUserSession, renderWithProviders, and selectors beneficiaryName, editor, sendButton to locate elements but change the test data to a group/collection scenario) so they no longer duplicate each other.
339-343: Test session state may leak between tests.
setUserSessionat line 340 overrides the module-level session (line 197) with anAdminrole, butafterEachonly callscleanup(DOM cleanup). IfsetUserSessionpersists state (e.g., localStorage), subsequent tests that rely on the default non-admin session could be affected depending on execution order.Consider resetting the session in
afterEach:♻️ Suggested fix
-afterEach(cleanup); +afterEach(() => { + cleanup(); + setUserSession(JSON.stringify({ organization: { id: '1' } })); +});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/containers/Chat/ChatInterface/ChatInterface.test.tsx` around lines 339 - 343, This test mutates shared session state by calling setUserSession(...) and the suite only calls cleanup in afterEach, so restore or clear the session after each test to avoid leakage: add an afterEach hook that calls setUserSession with the original module-level session JSON (the same value set at top-level) or removes the persisted session (e.g., localStorage.removeItem key used by setUserSession) so tests using the default non-admin session are not affected; ensure the afterEach runs alongside the existing cleanup call.src/containers/Auth/Auth.tsx (1)
18-18: Remove commented-outPromotionimport and usage instead of leaving dead code.The
Promotionimport (line 18) and its usage (line 286) are both commented out. The component is not used anywhere else in the codebase, so delete these lines entirely rather than keeping commented-out code.♻️ Suggested cleanup
-// import { Promotion } from './Promotion/Promotion';And at line 286:
- {/* {mode === 'login' && <Promotion />} */}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/containers/Auth/Auth.tsx` at line 18, Remove the dead commented-out Promotion import and its commented usage: delete the commented import line that references Promotion and remove the commented JSX/usage near the bottom of the Auth component where Promotion is referenced (the commented block around "Promotion"). Ensure no other references to the Promotion symbol remain in the Auth component so the file has no leftover commented dead code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/containers/Auth/Auth.tsx`:
- Around line 179-181: The patch adds an unused handlePhone property on the
fieldInfo object for phone fields; remove the handlePhone assignment in the
branch where field.type === 'phone' so fieldInfo is just the spread of field
(i.e., eliminate: handlePhone: () => (value: string) => {
setFieldValue('phoneNumber', value); }) and rely on PhoneInput's existing
onChange behavior which calls setFieldValue(field.name, event); also scan for
any references to handlePhone to ensure no other code expects it.
In `@src/containers/Chat/ChatInterface/ChatInterface.test.tsx`:
- Around line 365-378: Rename or update the two tests to match their behavior:
for the "should show error if send message fails" test (uses
renderWithProviders, setUserSession, editor, sendButton) either mock the
send-message API to throw/return an error when the input value is 'error' and
add an assertion that the error UI is shown (e.g.,
expect(screen.getByText(/error/i)) or expect an error testid), or rename the
test to something like "renders chat and allows sending input" if you only
verify render and click; for the "Blocked contacts should redirect to chat page"
test either set up the blocked-contact state/mocked API or mocked navigation
(e.g., simulate blocked contact via the provider/mock and assert
navigation/redirect using the router history or mocked useNavigate), or rename
it to "renders beneficiary name when contact not blocked" to reflect the current
assertion on beneficiaryName.
In `@src/containers/MyAccount/MyAccount.tsx`:
- Around line 52-67: In the onCompleted handler inside the MyAccount component,
guard against an empty errors array before accessing errors[0]. Check that
data.updateCurrentUser.errors exists and has a length > 0 (or that errors[0] is
truthy) before reading .message; if the array is empty, fall back to a generic
error toast (e.g., "Unknown error" or t('An unexpected error occurred')) so you
don't attempt to read undefined.message in the onCompleted callback.
In `@src/mocks/Search.tsx`:
- Around line 546-571: The mock for contextMessage in src/mocks/Search.tsx uses
__typename: 'LanguageUser' for the sender which conflicts with the SEARCH_QUERY
schema expecting the sender/contact shape (id, name) and likely a 'Contact'
typename; update the mock so contextMessage.sender matches the real schema/type
returned by SEARCH_QUERY (use the same typename and fields as Contact: id and
name) and ensure any other instances of contextMessage.sender in this mock file
use the same typename to prevent cache normalization issues.
---
Outside diff comments:
In `@src/components/UI/SearchBar/SearchBar.tsx`:
- Around line 72-79: The IconButton with data-testid "advanced-search-icon"
currently uses a misleading aria-label "toggle password visibility"; update it
to an accurate, descriptive label such as "toggle advanced search" or "open
advanced search" so screen readers convey its purpose; locate the IconButton
(the element with onClick calling Track('Advanced search') and handleClick(e,
'search', 'update')) and replace the aria-label value accordingly.
In `@src/containers/Chat/ChatMessages/StatusBar/StatusBar.test.tsx`:
- Line 63: The test named 'it should show suspended message if balance is
negative' is a duplicate/misleading description; update the test description for
the second occurrence (the test that uses getInactiveStatusMock and checks
balance 14.17) to accurately reflect the inactive phones scenario (e.g., "it
should show inactive phones message when account has inactive phones"), so
locate the test(...) invocation in StatusBar.test.tsx that uses
getInactiveStatusMock and change its description string to match the behavior
being asserted.
In `@src/containers/WaGroups/GroupChatInterface/GroupInterface.test.tsx`:
- Around line 11-54: The first search entry written to the InMemoryCache is
missing a top-level __typename which breaks Apollo normalization; update the
test cache.writeQuery data for the first search item (the object with id
'waGroup_6' / label 'WA Group 1') to include "__typename: 'WaConversation'" so
it matches the other entries and aligns with GROUP_SEARCH_QUERY and
GROUP_QUERY_VARIABLES expectations when using new InMemoryCache().
---
Nitpick comments:
In `@src/components/floweditor/FlowEditor.test.tsx`:
- Line 219: Tests were using waitFor(() => expect(screen.findByText(...))) which
is a no-op because screen.findByText returns a Promise and expect(Promise)
without a matcher never throws; replace those patterns by awaiting the finder
(use await screen.findByText(...) or await screen.findByTestId(...)) and then
assert the resolved element with toBeInTheDocument() (or another matcher) —
update all occurrences of waitFor + expect(screen.findByText/FindByTestId) to
use await + expect(element).toBeInTheDocument(), e.g., change waitFor(() =>
expect(screen.findByText('help workflow'))) to expect(await
screen.findByText('help workflow')).toBeInTheDocument() and similarly for the
other findBy* usages.
In `@src/containers/Auth/Auth.tsx`:
- Line 18: Remove the dead commented-out Promotion import and its commented
usage: delete the commented import line that references Promotion and remove the
commented JSX/usage near the bottom of the Auth component where Promotion is
referenced (the commented block around "Promotion"). Ensure no other references
to the Promotion symbol remain in the Auth component so the file has no leftover
commented dead code.
In `@src/containers/Chat/ChatConversations/ChatConversations.test.tsx`:
- Line 13: The manual cache.writeQuery against the InMemoryCache is missing
__typename fields causing inconsistent normalization; update the data object
passed to cache.writeQuery (the one created alongside InMemoryCache / variable
cache) to include "__typename": "Conversation" on the root and add appropriate
__typename entries on nested objects (e.g., contact -> "Contact", each message
-> "Message", sender -> "User", receiver -> "User", contextMessage -> its type)
so the manual write mirrors the ChatConversationMocks structure and enables
consistent Apollo cache normalization.
In `@src/containers/Chat/ChatInterface/ChatInterface.test.tsx`:
- Around line 345-363: The two tests 'send message to whatsapp group' and
'should send message to whatsapp group collections' are identical; either remove
the redundant test or make the second actually exercise collection-specific
behavior: update the second test to render <ChatInterface /> with a collections
context/prop (e.g., set cache or pass collectionType='collections' or a WA group
contact fixture) and assert WA-group/collection-specific UI or network call
differences (use the same helpers setUserSession, renderWithProviders, and
selectors beneficiaryName, editor, sendButton to locate elements but change the
test data to a group/collection scenario) so they no longer duplicate each
other.
- Around line 339-343: This test mutates shared session state by calling
setUserSession(...) and the suite only calls cleanup in afterEach, so restore or
clear the session after each test to avoid leakage: add an afterEach hook that
calls setUserSession with the original module-level session JSON (the same value
set at top-level) or removes the persisted session (e.g.,
localStorage.removeItem key used by setUserSession) so tests using the default
non-admin session are not affected; ensure the afterEach runs alongside the
existing cleanup call.
In `@src/mocks/Collection.tsx`:
- Line 97: The getCollectionsQuery fixture is missing __typename fields which
can break Apollo cache normalization; update the getCollectionsQuery mock to
include matching __typename values (e.g., for the root query, each collection
object and nested Group items) the same way other fixtures do, ensuring the
GET_COLLECTIONS response shape in the mock includes __typename entries for all
types referenced so tests and Apollo cache behave consistently.
In `@src/mocks/Contact.tsx`:
- Around line 184-185: The __typename property in the Contact mock is placed
before the spread of attributes and can be overridden by callers; update the
mock in src/mocks/Contact.tsx so that __typename: 'Contact' appears after the
...attributes spread (same change you made for getContactSampleQuery) to make
the type authoritative and prevent callers from overwriting it.
- Around line 89-100: The object literal in src/mocks/Contact.tsx currently sets
__typename: 'Contact' before spreading ...contactDetails, allowing a
caller-provided __typename to override it; move the __typename: 'Contact'
property to after the ...contactDetails spread (i.e., place __typename at the
end of the returned object) so the mock's __typename cannot be silently
overwritten by contactDetails while leaving the rest of the fields and the
...contactDetails spread intact.
In `@src/mocks/Search.tsx`:
- Line 654: The mock response for searchWithDateFilters is returning 25 messages
even though the request variables set messageOpts.limit to 20; update the mock
data calls messages(25, 2) to messages(20, 2) in the Search.tsx mocks used by
searchWithDateFilters (and the other identical occurrence) so the mocked
messages count matches messageOpts.limit and avoids confusing future
maintainers.
In `@src/mocks/User.tsx`:
- Around line 187-220: The mock getCurrentUserInvalidRoleQuery is missing
__typename fields on nested objects which causes Apollo to mismatch with the
real getCurrentUserQuery; update getCurrentUserInvalidRoleQuery (the mock for
GET_CURRENT_USER) to include __typename on currentUser.user and its nested
objects: add __typename for the user object, for contact, for each item in
groups, and for organization and language, using the same typename strings used
in getCurrentUserQuery so the shapes match exactly.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (25)
src/assets/images/icons/AdvancedSearch.tsxsrc/components/UI/SearchBar/SearchBar.tsxsrc/components/floweditor/FlowEditor.test.tsxsrc/containers/Auth/Auth.tsxsrc/containers/Chat/ChatConversations/ChatConversations.test.tsxsrc/containers/Chat/ChatConversations/ConversationList/ConversationList.test.tsxsrc/containers/Chat/ChatInterface/ChatInterface.test.tsxsrc/containers/Chat/ChatMessages/ChatMessages.test.tsxsrc/containers/Chat/ChatMessages/StatusBar/StatusBar.test.tsxsrc/containers/Chat/ChatSubscription/ChatSubscription.test.tsxsrc/containers/MyAccount/MyAccount.test.tsxsrc/containers/MyAccount/MyAccount.tsxsrc/containers/Organization/TrialRegistration/TrialRegistration.test.tsxsrc/containers/WaGroups/GroupChatInterface/GroupInterface.test.tsxsrc/mocks/Chat.tsxsrc/mocks/Collection.tsxsrc/mocks/Contact.tsxsrc/mocks/Flow.tsxsrc/mocks/Organization.tsxsrc/mocks/Search.tsxsrc/mocks/Simulator.tsxsrc/mocks/StatusBar.tssrc/mocks/User.tsxsrc/routes/AuthenticatedRoute/AuthenticatedRoute.test.tsxtest_output.txt
💤 Files with no reviewable changes (1)
- src/assets/images/icons/AdvancedSearch.tsx
✅ Files skipped from review due to trivial changes (1)
- src/routes/AuthenticatedRoute/AuthenticatedRoute.test.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/containers/MyAccount/MyAccount.test.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/containers/Auth/Auth.tsx (1)
179-181:handlePhoneis still unused byPhoneInput.The
PhoneInputcomponent does not consume ahandlePhoneprop — it callssetFieldValue(field.name, event)directly in its ownonChangehandler. This assignment is dead code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/containers/Auth/Auth.tsx` around lines 179 - 181, The assignment of a handlePhone function into fieldInfo inside Auth.tsx is dead code because PhoneInput does not accept or call handlePhone; remove the handlePhone property assignment (the branch that sets fieldInfo = { ...field, handlePhone: ... } for field.type === 'phone') and rely on the existing setFieldValue usage inside PhoneInput, or alternatively modify PhoneInput to accept a handlePhone prop and call it from its onChange if you intend to delegate there; reference the fieldInfo assignment in Auth.tsx and the PhoneInput component when making the change.
🧹 Nitpick comments (3)
src/containers/Auth/Auth.tsx (3)
18-18: Remove commented-out Promotion code instead of leaving it dormant.Commented-out imports and render calls (lines 18 and 292) are dead code that adds noise. If the feature is being intentionally disabled, track it via a feature flag or issue rather than leaving stale comments in the source.
🧹 Proposed cleanup
-// import { Promotion } from './Promotion/Promotion';- {/* {mode === 'login' && <Promotion />} */}Also applies to: 292-292
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/containers/Auth/Auth.tsx` at line 18, Remove the dormant commented-out Promotion import and any commented render call for Promotion in Auth (the commented import near the top and the commented JSX at the bottom), or replace them with a proper feature flag/path controlled rendering if the feature should be toggled; specifically delete the commented line importing Promotion and the corresponding commented JSX reference so dead code is removed from Auth.tsx (or wrap Promotion usage in a conditional using a named flag like showPromotion if you intend to keep toggleable behavior).
80-85: Loading-reset effect is a no-op whenexternalLoadingis provided.When a parent supplies the
loadingprop, line 64 makesloadingreflectexternalLoading, andsetLoadingstill targetsinternalLoading. So the guardif (loading && errorMessage)can repeatedly match while only toggling a state variable that isn't consumed — harmless but misleading. Consider gating onexternalLoading === undefinedso the intent is clearer:♻️ Suggested guard
useEffect(() => { - if (loading && errorMessage) { + if (externalLoading === undefined && internalLoading && errorMessage) { setLoading(false); } -}, [loading, errorMessage]); +}, [externalLoading, internalLoading, errorMessage]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/containers/Auth/Auth.tsx` around lines 80 - 85, The effect that resets loading on error should only act on internal state when no external prop is supplying loading; update the useEffect (the one that currently checks if (loading && errorMessage)) to also guard that externalLoading is undefined (or null) before calling setLoading(false), so that when the component is controlled via the loading prop you don't toggle an unused internalLoading via setLoading; reference the useEffect, loading, externalLoading, internalLoading and setLoading identifiers and make the conditional check include externalLoading === undefined (or externalLoading == null) before invoking setLoading(false).
207-216: ThesetTimeout(resolve, 0)betweensetValuesandsubmitFormis a fragile timing workaround.
setValuesin Formik 2.x returns a promise, andsubmitForm()reads from Formik's internal state bag — so the microtask yield viasetTimeoutshouldn't be necessary in theory. In practice, known Formik issues can causesubmitFormto validate against stale values, making this workaround tempting, but a zero-delay timer provides no deterministic ordering guarantee across React batching modes (especially with React 19's automatic batching).A more robust alternative is to pass the token directly to
saveHandlerinstead of round-tripping through Formik state:♻️ Sketch of an alternative
onClick={async (token: string) => { if (token) { - // Set captcha value - await setValues({ ...values, captcha: token }); - - // Give React time to process the state update - await new Promise((resolve) => setTimeout(resolve, 0)); - - // Let Formik handle validation & submission - submitForm(); + setLoading(true); + saveHandler({ ...values, captcha: token }); } }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/containers/Auth/Auth.tsx` around lines 207 - 216, The zero-delay setTimeout is a fragile timing hack; instead of relying on Formik state being updated before submitForm(), remove the setTimeout and avoid round-tripping the captcha through Formik when submitting — call the existing saveHandler (or the function that handles form submission) directly with the token argument from the onClick handler (still update Formik via setValues if you want UI sync, but pass token into saveHandler so it doesn't read values.captcha), and remove the submitForm() call that depends on stale internal state; update the saveHandler signature/logic to accept a token parameter and use that token for submission/validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/containers/Auth/Auth.tsx`:
- Around line 241-243: Fix the typos in the JSX comment inside the Auth
component: change "neeed" to "need" and "sumbitting" to "submitting" in the
comment that explains why a hidden submit button is added (the comment block
near the form submit button inside the Auth component). Ensure the wording now
reads that the submit button is required to enable form submitting when the user
hits Enter and keep the reference URL unchanged.
---
Duplicate comments:
In `@src/containers/Auth/Auth.tsx`:
- Around line 179-181: The assignment of a handlePhone function into fieldInfo
inside Auth.tsx is dead code because PhoneInput does not accept or call
handlePhone; remove the handlePhone property assignment (the branch that sets
fieldInfo = { ...field, handlePhone: ... } for field.type === 'phone') and rely
on the existing setFieldValue usage inside PhoneInput, or alternatively modify
PhoneInput to accept a handlePhone prop and call it from its onChange if you
intend to delegate there; reference the fieldInfo assignment in Auth.tsx and the
PhoneInput component when making the change.
---
Nitpick comments:
In `@src/containers/Auth/Auth.tsx`:
- Line 18: Remove the dormant commented-out Promotion import and any commented
render call for Promotion in Auth (the commented import near the top and the
commented JSX at the bottom), or replace them with a proper feature flag/path
controlled rendering if the feature should be toggled; specifically delete the
commented line importing Promotion and the corresponding commented JSX reference
so dead code is removed from Auth.tsx (or wrap Promotion usage in a conditional
using a named flag like showPromotion if you intend to keep toggleable
behavior).
- Around line 80-85: The effect that resets loading on error should only act on
internal state when no external prop is supplying loading; update the useEffect
(the one that currently checks if (loading && errorMessage)) to also guard that
externalLoading is undefined (or null) before calling setLoading(false), so that
when the component is controlled via the loading prop you don't toggle an unused
internalLoading via setLoading; reference the useEffect, loading,
externalLoading, internalLoading and setLoading identifiers and make the
conditional check include externalLoading === undefined (or externalLoading ==
null) before invoking setLoading(false).
- Around line 207-216: The zero-delay setTimeout is a fragile timing hack;
instead of relying on Formik state being updated before submitForm(), remove the
setTimeout and avoid round-tripping the captcha through Formik when submitting —
call the existing saveHandler (or the function that handles form submission)
directly with the token argument from the onClick handler (still update Formik
via setValues if you want UI sync, but pass token into saveHandler so it doesn't
read values.captcha), and remove the submitForm() call that depends on stale
internal state; update the saveHandler signature/logic to accept a token
parameter and use that token for submission/validation.
src/containers/Auth/Auth.tsx
Outdated
| {/* We neeed to add this submit button to enable form sumbitting when user hits enter | ||
| key. This is an workaround solution till the bug in formik or react is fixed. For | ||
| more info: https://github.com/formium/formik/issues/1418 */} |
There was a problem hiding this comment.
Typos in comment: "neeed" → "need", "sumbitting" → "submitting".
✏️ Fix
- {/* We neeed to add this submit button to enable form sumbitting when user hits enter
- key. This is an workaround solution till the bug in formik or react is fixed. For
+ {/* We need to add this submit button to enable form submitting when user hits enter
+ key. This is a workaround solution till the bug in formik or react is fixed. For🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/containers/Auth/Auth.tsx` around lines 241 - 243, Fix the typos in the
JSX comment inside the Auth component: change "neeed" to "need" and "sumbitting"
to "submitting" in the comment that explains why a hidden submit button is added
(the comment block near the form submit button inside the Auth component).
Ensure the wording now reads that the submit button is required to enable form
submitting when the user hits Enter and keep the reference URL unchanged.
priyanshu6238
left a comment
priyanshu6238
left a comment
There was a problem hiding this comment.
@ankit-mehta07 Could you please review and remove any unnecessary changes in this PR that are not related to its scope?
Remove changes unrelated to glific#3435 (Support email editing on My Account page): - Revert Auth.tsx large refactor (trial flows, loading state, etc.) - Revert all Chat/WaGroups/TrialRegistration/routes test changes - Revert unrelated mock file changes (Chat, Collection, Contact, Flow, Organization, Search, Simulator, StatusBar) - Revert SearchBar, AdvancedSearch, FlowEditor changes - Revert yarn.lock and remove test_output.txt Retain only: - MyAccount.tsx: email editing with validation - MyAccount.test.tsx: new email update test - graphql/mutations/User.ts: phone+email in mutation response - mocks/User.tsx: email-editing related mocks - ResetPasswordConfirmOTP: phone validation (from same issue)
Replace non-existent translation keys with existing ones: - 'Name is required' → 'Name is required.' - 'Invalid email address' → 'Email is invalid' - 'Email is required' → 'Email is required.' - 'Profile updated successfully!' → 'Password updated successfully!' Update test assertion to match updated success message key.
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| CodeStyle | 3 minor |
🟢 Metrics 4 complexity
Metric Results Complexity 4
TIP This summary will be updated as you push new changes. Give us feedback
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/containers/MyAccount/MyAccount.tsx (1)
69-70:⚠️ Potential issue | 🟠 MajorSplit profile-save success handling from the password flow.
onSubmithere still feeds the shared success path at lines 69-70, so a successful email save currently showsPassword updated successfully!and also flipsshowOTPButtonback totrue. That couples the profile editor to the password section and can collapse an in-progress password change. At minimum, use email-specific success copy here and keep the OTP reset in the password-only path.Based on learnings: Use
setNotification()andsetErrorMessage()fromcommon/notificationto write directly to Apollo cache for in-app toast messages instead of component state.Also applies to: 179-183
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/containers/MyAccount/MyAccount.tsx` around lines 69 - 70, The shared success path in onSubmit currently calls setShowOTPButton(true) and setToastMessageInfo which incorrectly ties profile/email saves to the password flow (e.g., showing "Password updated successfully!" and resetting OTP state); update the email/profile success branch to use email-specific success copy and remove the setShowOTPButton call there, and move OTP reset and password-success toast into the password-only success path (the password update handler). Replace component state to publish to Apollo cache using setNotification() and setErrorMessage() from common/notification for toast messages instead of setToastMessageInfo; apply the same changes to the other duplicate block around the second success handling (the block noted at lines ~179-183).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/containers/MyAccount/MyAccount.tsx`:
- Around line 69-70: The shared success path in onSubmit currently calls
setShowOTPButton(true) and setToastMessageInfo which incorrectly ties
profile/email saves to the password flow (e.g., showing "Password updated
successfully!" and resetting OTP state); update the email/profile success branch
to use email-specific success copy and remove the setShowOTPButton call there,
and move OTP reset and password-success toast into the password-only success
path (the password update handler). Replace component state to publish to Apollo
cache using setNotification() and setErrorMessage() from common/notification for
toast messages instead of setToastMessageInfo; apply the same changes to the
other duplicate block around the second success handling (the block noted at
lines ~179-183).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e9a5d1d0-501c-422f-af81-54bd0ef53b41
📒 Files selected for processing (3)
src/containers/Auth/Auth.tsxsrc/containers/MyAccount/MyAccount.test.tsxsrc/containers/MyAccount/MyAccount.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- src/containers/MyAccount/MyAccount.test.tsx
- src/containers/Auth/Auth.tsx
DeepScan flags 'Return values from promise executor functions cannot be read' when setTimeout is used as an arrow-function expression body. Using a block body makes the intent explicit and resolves the warning.
- Auth.tsx: fix handlePhone indentation and expand promise executor to multi-line block body - MyAccount.tsx: use shorthand boolean prop (enableReinitialize)
- Add separate updateUserProfile mutation for email/name updates that never touches showOTPButton state - Both profile updates and language changes now use setNotification() (Apollo cache) instead of shared message state + ToastMessage - Password mutation onCompleted uses its own hardcoded success copy - Remove unused message/setMessage state - Update tests: remove inline toast assertions for notifications that now emit via setNotification (Apollo cache, global snackbar)
2 participants
Fixes #3435
Summary
keyfield in errors to match schemaValidation
Summary by CodeRabbit
New Features
Bug Fixes / UX
Tests