Sync partner profile and account#4064
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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:
📝 WalkthroughWalkthroughAdds optional bidirectional identity synchronization between partner profiles and user login accounts. A new ChangesPartner–User Identity Sync
Sequence Diagram(s)sequenceDiagram
participant User
participant ProfileForm as Partner Profile Form
participant SettingsPage as Account Settings
participant PartnerAction as updatePartnerProfileAction
participant UserAPI as PATCH /api/user
participant SyncLib as sync-partner-identity
participant ConfirmEmail as Email Confirmation
participant Prisma
User->>ProfileForm: submit profile (name/image/email changed)
ProfileForm->>ProfileForm: getProfileSyncCandidates vs session
alt sync candidates exist
ProfileForm->>User: show identity-sync modal
User->>ProfileForm: "Update both" choice
end
ProfileForm->>PartnerAction: updatePartnerProfileAction {syncIdentity}
alt email changed + syncIdentity
PartnerAction->>SyncLib: assertEmailAvailableForIdentitySync
PartnerAction->>SyncLib: requestSyncedEmailChange
SyncLib->>ConfirmEmail: confirmEmailChange with syncIdentity=true
end
PartnerAction->>SyncLib: syncNameAndImageToUser
User->>SettingsPage: submit name/image/email
SettingsPage->>SettingsPage: getAccountSyncCandidates
alt on partner domain + sync candidates
SettingsPage->>User: show identity-sync modal
User->>SettingsPage: "Update both" choice
end
SettingsPage->>UserAPI: PATCH {syncIdentity: true}
UserAPI->>SyncLib: syncNameAndImageToPartner
UserAPI->>SyncLib: isImageReferencedByPartner (guard deletion)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ 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: 6
🤖 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
`@apps/web/app/`(ee)/partners.dub.co/(dashboard)/profile/profile-details-form.tsx:
- Around line 480-488: The validation pattern attribute set to
"^[a-z0-9]+(?:-[a-z0-9]+)*$" only allows lowercase letters, numbers, and
hyphens, but the help text in the p element below it claims underscores are
allowed. Update the help text to remove the mention of underscores and
accurately reflect what the pattern validation actually permits - only lowercase
letters, numbers, and hyphens.
- Around line 308-310: The session is not being refreshed when syncIdentity is
true but needsEmailVerification is also true, which leaves the login cache stale
for name/avatar changes that were synced immediately. In the if statement
checking syncIdentity and result.data.needsEmailVerification, remove the
negation condition on needsEmailVerification so that updateSession() is called
whenever syncIdentity is true, regardless of whether email verification is
pending. This ensures name and avatar changes are reflected in the session cache
immediately even when an email change requires verification.
In `@apps/web/app/app.dub.co/`(dashboard)/account/settings/page-client.tsx:
- Around line 86-87: The `isPartnerDomain` assignment on line 87 uses `subdomain
!== "app"` which incorrectly treats the initial null value as a partner domain
and would also incorrectly classify other subdomains like "admin" as partners.
Change the condition to explicitly check if `subdomain` equals the specific
partners domain value instead of checking if it's not equal to "app", ensuring
partner-sync behavior is only enabled for the actual partners subdomain.
In `@apps/web/lib/auth/confirm-email-change.ts`:
- Around line 66-67: The `partnerId` is not being persisted with the email
change request, causing confirmation to use the wrong partner if the user
switches partners before clicking the email link. Modify the
`requestSyncedEmailChange` function to accept `partnerId` as an input parameter,
add it to the object spread pattern alongside `syncIdentity` and `redirectTo`
(using the same conditional spread syntax), store the `partnerId` in Redis with
the sync identity request, and add validation to ensure `partnerId` is provided
when `syncIdentity` is true. Then update the `confirmEmailChange` function to
retrieve and use the stored `partnerId` instead of relying on the current
session's defaultPartnerId when confirming the email change.
In `@apps/web/lib/partners/sync-partner-identity.ts`:
- Around line 34-37: The `assertEmailAvailableForIdentitySync` function is
throwing a plain Error when detecting a partner email conflict, which causes it
to be treated as a generic 500 error instead of a proper conflict response.
Replace the plain Error throw statement with a typed conflict error from the
shared helper so that when this function is called from `PATCH /api/user`, the
conflict is properly handled and returns the expected conflict response code
instead of a 500 error.
In `@apps/web/ui/modals/confirm-modal.tsx`:
- Around line 79-87: The cancel button in the confirm modal is not disabled
while the async onCancel operation is in progress, allowing it to be clicked
multiple times and invoke the handler repeatedly. Add a disabled prop to the
cancel button that evaluates the isLoading state (set to true at the start of
the onClick handler), similar to how the confirm button likely handles it, to
prevent multiple concurrent invocations of the onCancel function.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5bb2258a-ec9d-40ec-bcc4-216c48e6e303
📒 Files selected for processing (10)
apps/web/app/(ee)/partners.dub.co/(dashboard)/profile/profile-details-form.tsxapps/web/app/api/user/route.tsapps/web/app/app.dub.co/(auth)/auth/confirm-email-change/[token]/page-client.tsxapps/web/app/app.dub.co/(auth)/auth/confirm-email-change/[token]/page.tsxapps/web/app/app.dub.co/(dashboard)/account/settings/page-client.tsxapps/web/lib/actions/partners/update-partner-profile.tsapps/web/lib/auth/confirm-email-change.tsapps/web/lib/partners/sync-partner-identity.tsapps/web/ui/account/upload-avatar.tsxapps/web/ui/modals/confirm-modal.tsx
|
@coderabbitai full review. any breaking changes or security issues? |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Read all key files changed in this PR
echo "=== sync-partner-identity.ts ==="
fd "sync-partner-identity.ts" --exec cat {}
echo ""
echo "=== update-partner-profile.ts ==="
fd "update-partner-profile.ts" --exec cat {}
echo ""
echo "=== confirm-email-change.ts (lib/auth) ==="
fd "confirm-email-change.ts" apps/web/lib/auth --exec cat {}
echo ""
echo "=== confirm-email-change page.tsx ==="
fd "page.tsx" apps/web/app/app.dub.co/\(auth\)/auth/confirm-email-change --exec cat {}
echo ""
echo "=== confirm-email-change page-client.tsx ==="
fd "page-client.tsx" apps/web/app/app.dub.co/\(auth\)/auth/confirm-email-change --exec cat {}
echo ""
echo "=== user/route.ts ==="
fd "route.ts" apps/web/app/api/user --exec cat {}
echo ""
echo "=== profile-details-form.tsx ==="
fd "profile-details-form.tsx" --exec cat {}
echo ""
echo "=== account settings page-client.tsx ==="
fd "page-client.tsx" "apps/web/app/app.dub.co/(dashboard)/account/settings" --exec cat {}
echo ""
echo "=== upload-avatar.tsx ==="
fd "upload-avatar.tsx" --exec cat {}
echo ""
echo "=== confirm-modal.tsx ==="
fd "confirm-modal.tsx" --exec cat {}Length of output: 81755
Breaking ChangesNone identified. All additions are backward-compatible:
Security & Logic Issues🔴 High –
|
| Severity | File | Issue |
|---|---|---|
| 🔴 High | sync-partner-identity.ts |
Image clear (null) silently no-op'd in sync helpers |
| 🟠 Medium | app/api/user/route.ts |
isPartnersDomain derived from spoofable host header |
| 🟠 Medium | update-partner-profile.ts |
partner.email! assertion can throw if email is null |
| 🟡 Low | confirm-email-change/page.tsx |
Token identifier not explicitly validated against session |
| 🟡 Low | confirm-email-change/page.tsx |
Role not re-checked at confirmation for syncIdentity |
| 🟡 Low | confirm-email-change/page.tsx |
Sync change notified as non-partner update via EmailUpdated |
✅ Action performed
Full review finished.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
apps/web/app/app.dub.co/(auth)/auth/confirm-email-change/[token]/page.tsx (2)
84-91: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winBind the Redis payload to the verification token identifier.
The lookup should use
tokenFound.identifierafter verifying it belongs to the current session. Deriving the key from the current default partner/user can read the wrong pending request if the default partner changes or another request exists.🐛 Proposed fix shape
+ const tokenIdentifier = tokenFound.identifier; + const sessionIdentifier = tokenIdentifier.startsWith("pn_") + ? partnerId + : userId; + + if (sessionIdentifier !== tokenIdentifier) { + return ( + <EmptyState + icon={InputPassword} + title="Unauthorized" + description="This email change request does not belong to the current session." + /> + ); + } + const data = await redis.get<{ email: string; newEmail: string; isPartnerProfile?: boolean; syncIdentity?: boolean; partnerId?: string; redirectTo?: "/profile" | "/account/settings"; - }>(`email-change-request:user:${identifier}`); + }>(`email-change-request:user:${tokenIdentifier}`);🤖 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 `@apps/web/app/app.dub.co/`(auth)/auth/confirm-email-change/[token]/page.tsx around lines 84 - 91, The Redis lookup for the email change request data is using the current session's identifier when constructing the key `email-change-request:user:${identifier}`, but it should use the identifier from the verified token instead. After confirming that tokenFound exists and belongs to the current session, replace the identifier variable in the Redis key construction with tokenFound.identifier to ensure the correct pending request is retrieved for the specific token being confirmed.
194-201: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winClassify synced-email notifications as partner profile updates.
For
data.syncIdentity, the transaction updates the partner email too, but the notification template receivesisPartnerProfile: false.🐛 Proposed fix
react: EmailUpdated({ oldEmail: data.email, newEmail: data.newEmail, - isPartnerProfile: !!data.isPartnerProfile, + isPartnerProfile: !!data.isPartnerProfile || !!data.syncIdentity, }),🤖 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 `@apps/web/app/app.dub.co/`(auth)/auth/confirm-email-change/[token]/page.tsx around lines 194 - 201, The `isPartnerProfile` parameter passed to the `EmailUpdated` component in the `sendEmail` call only checks `data.isPartnerProfile`, but when `data.syncIdentity` is true, the partner email is also updated in the transaction. Update the `isPartnerProfile` parameter in the `EmailUpdated` function call to account for both conditions - it should be true when either `data.isPartnerProfile` is true OR when `data.syncIdentity` is true, ensuring the email template correctly identifies when a partner profile email has been synced and updated.apps/web/app/api/user/route.ts (2)
14-18: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDefault unrecognized hosts to the app flow.
!APP_HOSTNAMES.has(hostName)treats any unknown or spoofed Host as the partners domain, which can select the partners confirmation URL and enable the partner-sync branch. Only the explicit partners host should opt into partners behavior.🛡️ Proposed fix
import { APP_DOMAIN, - APP_HOSTNAMES, PARTNERS_DOMAIN, R2_URL, nanoid, @@ const hostName = req.headers.get("host") || ""; - const isPartnersDomain = !APP_HOSTNAMES.has(hostName); + const isPartnersDomain = hostName === new URL(PARTNERS_DOMAIN).host; @@ await confirmEmailChange({ email: session.user.email, newEmail: email, identifier: session.user.id, - hostName: APP_HOSTNAMES.has(hostName) ? APP_DOMAIN : PARTNERS_DOMAIN, + hostName: isPartnersDomain ? PARTNERS_DOMAIN : APP_DOMAIN, });Also applies to: 78-82, 166-171
🤖 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 `@apps/web/app/api/user/route.ts` around lines 14 - 18, The logic for detecting the partners domain flow uses a negative check on APP_HOSTNAMES that defaults unknown/spoofed hosts to partners behavior, which is a security issue. Replace all instances of conditions that use !APP_HOSTNAMES.has(hostName) throughout the file (including around lines 78-82 and 166-171) with explicit positive checks that only activate partners behavior when hostName explicitly matches PARTNERS_DOMAIN. This ensures only legitimate partners hosts can opt into the partners behavior while unknown hosts default to the standard app flow.
175-216: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve
nullavatar updates through save, sync, and cleanup.
image: nullpasses validation, but the truthy spreads skip clearinguser.image, skip sending the clear to the partner sync helper, and skip old-avatar cleanup.🐛 Proposed fix
data: { ...(name && { name }), - ...(image && { image }), + ...(image !== undefined && { image }), ...(source && { source }), ...(defaultWorkspace && { defaultWorkspace }), }, @@ await syncNameAndImageToPartner({ partnerId, ...(name && { name }), - ...(image && { image }), + ...(image !== undefined && { image }), }); } @@ if ( - image && + image !== undefined && session.user.image && session.user.image.startsWith(`${R2_URL}/avatars/${session.user.id}`) ) {🤖 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 `@apps/web/app/api/user/route.ts` around lines 175 - 216, The issue is that the conditional spreads using the AND operator (&&) skip null values, preventing image from being cleared. When image is null, the spreads in the prisma.user.update data object, the syncNameAndImageToPartner call, and the cleanup logic all skip the null value because null is falsy. Instead of checking if the values are truthy, you need to check if they were explicitly provided in the request. Refactor the conditional logic to distinguish between properties that were not provided versus properties that were intentionally set to null, so that null values for name, image, source, and defaultWorkspace are properly included in the database update, passed to syncNameAndImageToPartner, and accounted for in the old avatar cleanup logic within the waitUntil function.
🤖 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 `@apps/web/app/app.dub.co/`(auth)/auth/confirm-email-change/[token]/page.tsx:
- Around line 116-153: The code checks if a user is a partnerUser member but
does not verify the user has the partner_profile.update permission before
applying the email changes in the prisma.$transaction call. A user's role could
be downgraded between the initial membership check and the actual update,
allowing them to modify the partner email without proper authorization. Add a
re-check of the partner_profile.update permission immediately before the
prisma.$transaction block that updates both the user email and partner email to
ensure the user still has the required permission at the time of applying the
changes.
In `@apps/web/lib/actions/partners/update-partner-profile.ts`:
- Around line 195-202: The non-null assertion operators on user.email and
partner.email in the requestSyncedEmailChange call do not validate that these
values are actually non-null at runtime, only suppress TypeScript checks. Add
explicit null/undefined checks for both user.email and partner.email before
calling requestSyncedEmailChange to ensure neither value is null before passing
them to the function. If either email is null, handle this case appropriately
(such as returning early or throwing a user-friendly error) rather than allowing
null values to be queued in the confirmation payload.
- Around line 228-233: The conditional spread operator for the imageUrl
parameter in the syncNameAndImageToUser function call is preventing null values
from being passed through, which means avatar clears are not synced to linked
users. Instead of using the conditional spread pattern `...(imageUrl && { image:
imageUrl })`, directly pass the image property to the syncNameAndImageToUser
call so that explicit null values are included and properly communicated to the
sync helper, allowing it to clear the avatar when needed.
In `@apps/web/lib/partners/sync-partner-identity.ts`:
- Around line 81-99: The early return condition at the start of the function
treats `image === null` (explicit avatar deletion) the same as no image being
provided, which causes avatar clear operations to be silently skipped.
Additionally, the conditional spread in the Prisma update data does not include
explicit null values for the image field. Modify the logic to distinguish
between "image not provided" and "image explicitly set to null" - the function
should only return early if both name and image are undefined (not provided),
and the Prisma data object should include `image: null` when image is explicitly
null to ensure avatar deletions are synced to the database.
In `@apps/web/ui/modals/confirm-modal.tsx`:
- Around line 55-60: The handleCancel function awaits onCancel on line 60, but
the onCancel prop in the PromptModalProps type definition around line 8 is
declared as a synchronous callback. Update the onCancel property type in
PromptModalProps to support both sync and async callbacks by changing it to
accept a function that returns either void or Promise<void>, matching the
existing type signature of the onConfirm prop.
---
Outside diff comments:
In `@apps/web/app/api/user/route.ts`:
- Around line 14-18: The logic for detecting the partners domain flow uses a
negative check on APP_HOSTNAMES that defaults unknown/spoofed hosts to partners
behavior, which is a security issue. Replace all instances of conditions that
use !APP_HOSTNAMES.has(hostName) throughout the file (including around lines
78-82 and 166-171) with explicit positive checks that only activate partners
behavior when hostName explicitly matches PARTNERS_DOMAIN. This ensures only
legitimate partners hosts can opt into the partners behavior while unknown hosts
default to the standard app flow.
- Around line 175-216: The issue is that the conditional spreads using the AND
operator (&&) skip null values, preventing image from being cleared. When image
is null, the spreads in the prisma.user.update data object, the
syncNameAndImageToPartner call, and the cleanup logic all skip the null value
because null is falsy. Instead of checking if the values are truthy, you need to
check if they were explicitly provided in the request. Refactor the conditional
logic to distinguish between properties that were not provided versus properties
that were intentionally set to null, so that null values for name, image,
source, and defaultWorkspace are properly included in the database update,
passed to syncNameAndImageToPartner, and accounted for in the old avatar cleanup
logic within the waitUntil function.
In `@apps/web/app/app.dub.co/`(auth)/auth/confirm-email-change/[token]/page.tsx:
- Around line 84-91: The Redis lookup for the email change request data is using
the current session's identifier when constructing the key
`email-change-request:user:${identifier}`, but it should use the identifier from
the verified token instead. After confirming that tokenFound exists and belongs
to the current session, replace the identifier variable in the Redis key
construction with tokenFound.identifier to ensure the correct pending request is
retrieved for the specific token being confirmed.
- Around line 194-201: The `isPartnerProfile` parameter passed to the
`EmailUpdated` component in the `sendEmail` call only checks
`data.isPartnerProfile`, but when `data.syncIdentity` is true, the partner email
is also updated in the transaction. Update the `isPartnerProfile` parameter in
the `EmailUpdated` function call to account for both conditions - it should be
true when either `data.isPartnerProfile` is true OR when `data.syncIdentity` is
true, ensuring the email template correctly identifies when a partner profile
email has been synced and updated.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: efe9cbf6-a00a-49ee-a385-0f1cb3b4c033
📒 Files selected for processing (10)
apps/web/app/(ee)/partners.dub.co/(dashboard)/profile/profile-details-form.tsxapps/web/app/api/user/route.tsapps/web/app/app.dub.co/(auth)/auth/confirm-email-change/[token]/page-client.tsxapps/web/app/app.dub.co/(auth)/auth/confirm-email-change/[token]/page.tsxapps/web/app/app.dub.co/(dashboard)/account/settings/page-client.tsxapps/web/lib/actions/partners/update-partner-profile.tsapps/web/lib/auth/confirm-email-change.tsapps/web/lib/partners/sync-partner-identity.tsapps/web/ui/account/upload-avatar.tsxapps/web/ui/modals/confirm-modal.tsx
|
@coderabbitai I've applied all the security changes. Please verify all security issues again. Any breaking changes? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== sync-partner-identity.ts ==="
fd "sync-partner-identity.ts" --exec cat {}
echo ""
echo "=== update-partner-profile.ts ==="
fd "update-partner-profile.ts" --exec cat {}
echo ""
echo "=== user/route.ts ==="
fd "route.ts" apps/web/app/api/user --exec cat {}
echo ""
echo "=== confirm-email-change/page.tsx ==="
fd "page.tsx" apps/web/app/app.dub.co/\(auth\)/auth/confirm-email-change --exec cat {}Length of output: 37360
Security Issues – Re-verification🔴 High – Image
|
| Severity | Issue | Status |
|---|---|---|
| 🔴 High | sync-partner-identity.ts – null image no-op |
✅ Fixed |
| 🟠 Medium | api/user/route.ts – host header spoofing |
✅ Mitigated (positive allowlist) |
| 🟠 Medium | update-partner-profile.ts – partner.email! assertion |
✅ Fixed |
| 🟡 Low | confirm-email-change/page.tsx – token identifier validation |
✅ Fixed |
| 🟡 Low | confirm-email-change/page.tsx – role not re-checked at confirmation |
✅ Fixed |
| 🟡 Low | confirm-email-change/page.tsx – sync change email notification |
✅ Fixed |
All issues are addressed. 🎉
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/web/ui/modals/identity-sync-confirm-modal.tsx (2)
53-74: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting shared Tailwind classes.
Lines 55–59 and 66–70 repeat base classes with only conditional modifiers. You can simplify with a template literal or
cn()utility.♻️ Optional refactor
{showName && snapshot.name && ( <div - className={ - isNameContextOnly - ? "text-[14px] font-semibold leading-4 text-neutral-900 opacity-50" - : "text-[14px] font-semibold leading-4 text-neutral-900" - } + className={`text-[14px] font-semibold leading-4 text-neutral-900 ${ + isNameContextOnly ? "opacity-50" : "" + }`} > {snapshot.name} </div> )} {showEmail && snapshot.email && ( <div - className={ - showName && snapshot.name - ? "mt-1 text-sm font-medium leading-5 text-neutral-500" - : "text-sm font-medium leading-5 text-neutral-900" - } + className={`text-sm font-medium leading-5 ${ + showName && snapshot.name + ? "mt-1 text-neutral-500" + : "text-neutral-900" + }`} > {snapshot.email} </div>🤖 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 `@apps/web/ui/modals/identity-sync-confirm-modal.tsx` around lines 53 - 74, The conditional className logic for both the name and email display sections contains repeated patterns that can be simplified. Extract the className definitions for both the name div (currently using isNameContextOnly condition) and the email div (currently using the showName && snapshot.name condition) into separate variables at the top of the component or inline, using either template literals or a cn() utility function to build the conditional class strings more cleanly. This will reduce verbosity and make the conditional logic easier to maintain and read.
142-164: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider adding error handling to async event handlers.
Both
handleConfirmandhandleCancelawait user-provided callbacks but don't catch errors. IfonConfirmoronCancelthrow, the error becomes an unhandled promise rejection. React error boundaries cannot catch async errors from event handlers. Either add try-catch blocks to log/display errors, or document thatonConfirm/onCancelmust handle their own errors.♻️ Proposed error-handling pattern
const handleConfirm = async () => { if (isLoading) return; setIsLoading(true); try { await onConfirm(); setShowModal(false); + } catch (error) { + console.error("Identity sync confirmation failed:", error); + // Optionally: show toast notification or set error state } finally { setIsLoading(false); } }; const handleCancel = async () => { if (isLoading) return; setIsLoading(true); try { await onCancel?.(); setShowModal(false); + } catch (error) { + console.error("Identity sync cancellation failed:", error); + // Optionally: show toast notification or set error state } finally { setIsLoading(false); } };🤖 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 `@apps/web/ui/modals/identity-sync-confirm-modal.tsx` around lines 142 - 164, The handleConfirm and handleCancel functions currently have try-finally blocks without catch blocks to handle errors from the onConfirm and onCancel callbacks respectively. Add a catch block in both handlers after the try block to catch and handle any errors thrown by these callbacks. In the catch block, you should log the error and optionally display it to the user through an error state or notification mechanism, ensuring proper error handling occurs before the loading state is reset in the finally block.
🤖 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.
Nitpick comments:
In `@apps/web/ui/modals/identity-sync-confirm-modal.tsx`:
- Around line 53-74: The conditional className logic for both the name and email
display sections contains repeated patterns that can be simplified. Extract the
className definitions for both the name div (currently using isNameContextOnly
condition) and the email div (currently using the showName && snapshot.name
condition) into separate variables at the top of the component or inline, using
either template literals or a cn() utility function to build the conditional
class strings more cleanly. This will reduce verbosity and make the conditional
logic easier to maintain and read.
- Around line 142-164: The handleConfirm and handleCancel functions currently
have try-finally blocks without catch blocks to handle errors from the onConfirm
and onCancel callbacks respectively. Add a catch block in both handlers after
the try block to catch and handle any errors thrown by these callbacks. In the
catch block, you should log the error and optionally display it to the user
through an error state or notification mechanism, ensuring proper error handling
occurs before the loading state is reset in the finally block.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5ab47f51-3542-4c6c-8b27-2167ca8fd204
📒 Files selected for processing (3)
apps/web/app/(ee)/partners.dub.co/(dashboard)/profile/profile-details-form.tsxapps/web/app/app.dub.co/(dashboard)/account/settings/page-client.tsxapps/web/ui/modals/identity-sync-confirm-modal.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/web/app/app.dub.co/(dashboard)/account/settings/page-client.tsx
- apps/web/app/(ee)/partners.dub.co/(dashboard)/profile/profile-details-form.tsx
1 participant
Summary by CodeRabbit