Conversation
evgenibir
force-pushed
the
feat/1782-Updating-email-on-profile-should-update-login-change
branch
from
5b66e2e to
57a442e
Compare
WalkthroughThis PR integrates Privy-based email handling into the profile edit flow. The profile edit page now detects email changes using Privy's user state, queues pending form data when an email change is identified, and uses Privy's Changes
Sequence DiagramsequenceDiagram
actor User
participant Page as Profile Edit Page
participant Privy as Privy Auth Service
participant API as Backend API
participant DB as Database
User->>Page: Edit profile with email change
Page->>Page: Detect email differs from currentPrivyEmail
Page->>Page: Store form data in pendingFormDataRef
Page->>Privy: Call updateEmail()
activate Privy
Privy->>Privy: Validate & process email update
alt Email update succeeds
Privy-->>Page: onSuccess callback
Page->>API: editProfile with pending data
API->>DB: Update profile
API-->>Page: Success response
Page->>Page: Navigate to /profile
else Email update fails
Privy-->>Page: onError callback
Page->>API: editProfile with pending data (retry)
API->>DB: Update profile
API-->>Page: Success response
Page->>Page: Navigate to /profile
end
deactivate Privy
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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: 0
🧹 Nitpick comments (3)
apps/web/src/app/[lang]/profile/[personSlug]/@aside/edit/page.tsx (3)
55-57: Useconsole.errorfor error logging.Errors caught in try/catch blocks should be logged with
console.errorfor proper severity and dev tools filtering.} catch (err) { - console.log(err); + console.error('Error editing profile after email update:', err); pendingFormDataRef.current = null; }
72-73: Useconsole.errorfor consistency.Same as above — prefer
console.errorfor caught exceptions..catch((err) => { - console.log(err); + console.error('Error editing profile:', err); })
102-104: Useconsole.errorfor caught exceptions.} catch (err) { - console.log(err); + console.error('Error in onEdit:', err); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/src/app/[lang]/profile/[personSlug]/@aside/edit/page.tsx(2 hunks)
⏰ 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). (1)
- GitHub Check: deploy-preview
🔇 Additional comments (1)
apps/web/src/app/[lang]/profile/[personSlug]/@aside/edit/page.tsx (1)
95-97: The review comment is incorrect. TheupdateEmail()call is correct as written.According to Privy's official documentation,
updateEmail()takes no parameters and is designed to open a modal where the user enters and verifies their new email address. The code correctly follows the Privy pattern:
updateEmail()is called with no arguments (line 97) ✓- The
onSuccesscallback (lines 49-60) handles the post-update flow- The
pendingFormDataRefpattern properly stores the form data for use after email verification succeedsThe suggested fix in the review comment (
updateEmail({ email: newEmail })) would be incorrect and would not work with the Privy SDK.Likely an incorrect or invalid review comment.
|
|
||
| if (emailChanged) { | ||
| pendingFormDataRef.current = data; | ||
| updateEmail(); |
There was a problem hiding this comment.
Doesn't updateEmail return Promise, too?
evgenibir
force-pushed
the
feat/1782-Updating-email-on-profile-should-update-login-change
branch
from
57a442e to
6ee9411
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/web/src/app/[lang]/profile/[personSlug]/@aside/edit/page.tsx (1)
20-21: useUpdateAccount and updateEmail usage matches Privy API; no await neededAccording to Privy’s docs,
useUpdateAccountreturns{ updateEmail: () => void, updatePhone: () => void }, andupdateEmailis avoidfunction that just opens the modal (not a Promise), whileonSuccess/onErrorare invoked via callbacks. (docs.privy.io)Your usage:
- Destructuring
updateEmailfromuseUpdateAccount({ onSuccess, onError }).- Not awaiting
updateEmail()inonEdit.- Treating
onSuccess/onErroras the async boundaries for callingeditProfileand routing.is aligned with the documented hook contract and resolves the prior concern about
updateEmailreturning a Promise.Also applies to: 50-87
🧹 Nitpick comments (1)
apps/web/src/app/[lang]/profile/[personSlug]/@aside/edit/page.tsx (1)
89-105: Consider preserving non‑email edits when Privy update fails for other reasonsWhen
emailChangedis true, you stashdatainpendingFormDataRefand rely on theuseUpdateAccountcallbacks to calleditProfile. IfonErrorfires with an error that doesn’t match the two specific substrings, you simply clearpendingFormDataRefand never persist the pending profile changes.This means users who attempted an email change and hit a generic Privy error will lose all non‑email edits unless they re‑submit the form.
Consider either:
- Still calling
editProfile(pendingFormDataRef.current)for generic errors (perhaps after showing a message that the email itself was not updated), or- Surfacing an explicit error to the user and leaving
pendingFormDataRefintact so they can retry, rather than silently dropping it.Also applies to: 50-87
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/web/src/app/[lang]/profile/[personSlug]/@aside/edit/page.tsx(3 hunks)packages/epics/src/people/components/edit-person-section.tsx(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-03-19T12:11:00.004Z
Learnt from: plitzenberger
Repo: hypha-dao/hypha-web PR: 486
File: apps/web/src/app/[lang]/@aside/(.)profile/edit/loading.tsx:21-21
Timestamp: 2025-03-19T12:11:00.004Z
Learning: In the EditPersonSection component, successfulEditCallback is an optional prop that should be invoked using optional chaining (successfulEditCallback?.()) when it's used.
Applied to files:
packages/epics/src/people/components/edit-person-section.tsx
📚 Learning: 2025-03-21T11:15:37.961Z
Learnt from: plitzenberger
Repo: hypha-dao/hypha-web PR: 486
File: apps/web/src/app/[lang]/profile/edit/page.tsx:17-24
Timestamp: 2025-03-21T11:15:37.961Z
Learning: In React forms with validation schemas like Zod, required fields that shouldn't be edited by users (like IDs) should be included as hidden form fields rather than checked conditionally in submission handlers. This ensures proper form validation and prevents invalid submissions.
Applied to files:
packages/epics/src/people/components/edit-person-section.tsx
🧬 Code graph analysis (2)
apps/web/src/app/[lang]/profile/[personSlug]/@aside/edit/page.tsx (1)
packages/core/src/people/validation.ts (2)
schemaEditPersonWeb2(145-145)editPersonFiles(83-122)
packages/epics/src/people/components/edit-person-section.tsx (1)
apps/hypha-migration/hypha-react-demo/src/components/HyphaWallet.jsx (1)
isLoading(7-7)
⏰ 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). (1)
- GitHub Check: deploy-preview
🔇 Additional comments (1)
packages/epics/src/people/components/edit-person-section.tsx (1)
44-52: Email lock-out prop and UI wiring look consistentThe
cannotChangeEmailprop, its defaulting in the props destructuring, and its use to both disable the emailInputand show the explanatory block are all consistent and backward‑compatible with existing callers. No functional issues spotted here.Also applies to: 56-64, 247-248, 259-270
| const { user: privyUser, authenticated } = usePrivy(); | ||
|
|
||
| const hasEmailAccount = !!privyUser?.email?.address; | ||
| const currentPrivyEmail = privyUser?.email?.address; | ||
|
|
||
| const hasOAuthAccount = !!(privyUser?.google || privyUser?.apple); | ||
| const cannotChangeEmail = | ||
| authenticated && hasOAuthAccount && !hasEmailAccount; |
There was a problem hiding this comment.
Align cannotChangeEmail gating with OAuth provider‑agnostic copy
cannotChangeEmail is computed as:
const hasOAuthAccount = !!(privyUser?.google || privyUser?.apple);
const cannotChangeEmail = authenticated && hasOAuthAccount && !hasEmailAccount;and then passed to EditPersonSection. For Apple SSO users (where privyUser?.apple is truthy but privyUser?.google is not), this flag will also be true, but the explanatory text rendered in EditPersonSection currently talks only about “Google SSO” and “Google Account settings”.
To avoid confusing Apple‑SSO users, consider either:
- Making the message in
EditPersonSectionprovider‑agnostic, or - Passing down more specific context (e.g., which OAuth provider is in use) so the message can be tailored appropriately.
Also applies to: 128-136
evgenibir
force-pushed
the
feat/1782-Updating-email-on-profile-should-update-login-change
branch
from
6ee9411 to
75f0cf4
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/web/src/app/[lang]/profile/[personSlug]/@aside/edit/page.tsx (1)
36-43: Consider gating Privy‑derived flags onreadyto avoid transient misclassificationPrivy recommends checking
readybefore relying onauthenticated/user. Right nowhasEmailAccount,emailChanged, andcannotChangeEmaildon’t considerready, so on initial render a logged‑in user could briefly be treated as having no email account (or as unauthenticated) while Privy initializes. That’s a small edge case, but easy to harden by includingreadyin these conditions.Example refactor:
- const { user: privyUser, authenticated } = usePrivy(); + const { user: privyUser, authenticated, ready } = usePrivy(); const hasEmailAccount = !!privyUser?.email?.address; const currentPrivyEmail = privyUser?.email?.address; const hasOAuthAccount = !!(privyUser?.google || privyUser?.apple); - const cannotChangeEmail = - authenticated && hasOAuthAccount && !hasEmailAccount; + const cannotChangeEmail = + ready && authenticated && hasOAuthAccount && !hasEmailAccount; ... - const emailChanged = - authenticated && - hasEmailAccount && - currentPrivyEmail && - newEmail && - newEmail !== currentPrivyEmail; + const emailChanged = + ready && + authenticated && + hasEmailAccount && + currentPrivyEmail && + newEmail && + newEmail !== currentPrivyEmail;Also applies to: 91-97, 135-135
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/web/src/app/[lang]/profile/[personSlug]/@aside/edit/page.tsx(3 hunks)packages/epics/src/people/components/edit-person-section.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/epics/src/people/components/edit-person-section.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
apps/web/src/app/[lang]/profile/[personSlug]/@aside/edit/page.tsx (1)
packages/core/src/people/validation.ts (2)
schemaEditPersonWeb2(145-145)editPersonFiles(83-122)
⏰ 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). (1)
- GitHub Check: deploy-preview
🔇 Additional comments (1)
apps/web/src/app/[lang]/profile/[personSlug]/@aside/edit/page.tsx (1)
20-20: PrivyupdateEmailflow +pendingFormDataRefusage look correct (noawaitneeded)Per Privy’s
useUpdateAccountdocs,updateEmailis a() => voidthat opens the modal and drives completion via the suppliedonSuccess/onErrorcallbacks, not a Promise‑returning function. CallingupdateEmail()withoutawaitinonEditis therefore correct, and the pattern of stashingEditPersonFormDatainpendingFormDataRefand only invokingeditProfilefrom those callbacks gives you the intended “update Privy email first, then persist profile” behavior.You also clear
pendingFormDataRefin all code paths (success, handled error, and unhandled error), so there’s no obvious risk of stale data leaking into a later submission. From a correctness standpoint this async flow looks good as implemented.Also applies to: 50-87, 99-105
2 participants
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.