Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a centralized Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
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 |
Pull Request Test Coverage Report for Build 24021180111Details
💛 - Coveralls |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/sdk/admin/views/organizations/details/projects/use-add-project-members.tsx (1)
58-79:⚠️ Potential issue | 🟠 MajorDon’t conflate mutation failure with refetch failure in one catch.
A refetch error after successful member addition currently falls into the same handler map, which can show misleading failure messaging for an already-successful write.
Proposed fix
- try { + try { const principal = `app/user:${userId}`; const resource = `app/project:${projectId}`; await createPolicy( create(CreatePolicyRequestSchema, { body: { roleId: DEFAULT_ROLES.PROJECT_VIEWER, principal, resource, }, }), ); toast.success(`${memberLabel} added`); - await refetch(); - return projectMembers; } catch (error: unknown) { handleConnectError(error, { AlreadyExists: () => toast.error(`${memberLabel} already exists in this project`), PermissionDenied: () => toast.error("You don't have permission to perform this action"), InvalidArgument: (err) => toast.error('Invalid input', { description: err.message }), Default: (err) => toast.error('Something went wrong', { description: err.message }), }); + return; + } + + try { + await refetch(); + } catch { + toast.error(`${memberLabel} added, but failed to refresh the list`); }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ea972d5d-610a-4d8a-b513-a874a6d47283
📒 Files selected for processing (14)
web/sdk/admin/views/organizations/details/layout/invite-users-dialog.tsxweb/sdk/admin/views/organizations/details/projects/use-add-project-members.tsxweb/sdk/admin/views/users/details/security/block-user.tsxweb/sdk/admin/views/users/list/invite-users.tsxweb/sdk/hooks/index.tsweb/sdk/react/hooks/usePreferences.tsweb/sdk/react/views/members/invite-member-dialog.tsxweb/sdk/react/views/plans/hooks/usePlans.tsxweb/sdk/react/views/projects/details/delete-project-dialog.tsxweb/sdk/react/views/projects/details/project-general.tsxweb/sdk/react/views/teams/details/invite-team-member-dialog.tsxweb/sdk/react/views/teams/details/team-general.tsxweb/sdk/react/views/teams/list/add-team-dialog.tsxweb/sdk/utils/error.ts
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)
web/sdk/react/views/plans/hooks/usePlans.tsx (1)
82-129:⚠️ Potential issue | 🟠 MajorMove post-success callbacks outside
try/catchblocks to prevent masking client exceptions as RPC errors.The
onSuccess()calls at lines 118, 162, and 282 execute within the sametryblock as their respective mutations. IfonSuccessthrows after a successful RPC response, thecatchhandler will misreport it as a backend error, display a false error toast, and hide the real client bug. Extract the success-side logic outside thetry/catchfor all three functions:checkoutPlan,changePlan, andcancelSubscription.Proposed pattern
- try { - const resp = await changeSubscriptionMutation( - create(ChangeSubscriptionRequestSchema, { - id: activeSubscription?.id, - change: { - case: 'planChange', - value: { - plan: planId, - immediate: immediate - } - } - }) - ); - if (resp?.phase) { - onSuccess(); - } - } catch (error) { + let resp; + try { + resp = await changeSubscriptionMutation( + create(ChangeSubscriptionRequestSchema, { + id: activeSubscription?.id, + change: { + case: 'planChange', + value: { + plan: planId, + immediate: immediate + } + } + }) + ); + } catch (error) { handleConnectError(error, { PermissionDenied: () => toast.error("You don't have permission to perform this action"), InvalidArgument: err => toast.error('Failed to change plan', { description: err.message }), NotFound: err => toast.error('Not found', { description: err.message }), Default: err => toast.error('Failed to change plan', { description: err.message }) }); + return; + } + + if (resp?.phase) { + onSuccess(); }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 85b54ad6-8da6-4ce6-aa45-4245ec8cbf74
📒 Files selected for processing (2)
web/sdk/react/views/plans/hooks/usePlans.tsxweb/sdk/utils/error.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- web/sdk/utils/error.ts
Replace inconsistent onError callbacks and unhandled promise rejections with try/catch blocks using the new handleConnectError utility across 12 files. Maps gRPC status codes (AlreadyExists, InvalidArgument, PermissionDenied, NotFound) to appropriate user-facing error messages. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add Default handler for checkoutPlan and changePlan in usePlans - Handle string-thrown errors in handleConnectError utility Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
rsbh
force-pushed
the
fix_sdk_connect_rpc_onerror
branch
from
f45db61 to
f7396f0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
web/sdk/utils/error.ts (1)
30-32: Preserve structured context in fallback error logging.At Line 31, logging only
err.messagedrops useful debugging context (for example code and metadata). Prefer logging the full error object or a structured payload in the default handler.♻️ Suggested tweak
- const defaultHandler = - handlers?.Default ?? ((err: ConnectError) => console.error(err.message)); + const defaultHandler = + handlers?.Default ?? + ((err: ConnectError) => + console.error('frontier:sdk:: connect rpc error', { + code: err.code, + message: err.message, + error: err + }));web/sdk/admin/views/organizations/details/layout/invite-users-dialog.tsx (1)
11-12: Backfillrolewhen the async default resolves.
useFormonly appliesdefaultValueson the first render. If this dialog mounts beforeOrganizationContext.rolesis populated,defaultRoleIdnever reaches the form and the submit path falls back toroleIds: []until the user manually re-selects a role.♻️ Proposed fix
-import { useContext, useMemo } from 'react'; +import { useContext, useEffect, useMemo } from 'react'; … const methods = useForm<InviteSchemaType>({ resolver: zodResolver(inviteSchema), defaultValues: { role: defaultRoleId } }); + + useEffect(() => { + if (defaultRoleId && !methods.getValues('role')) { + methods.setValue('role', defaultRoleId, { shouldValidate: true }); + } + }, [defaultRoleId, methods]);Also applies to: 53-63, 90-94
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7107438c-15fa-42e9-94e9-7cf44c4f6155
📒 Files selected for processing (14)
web/sdk/admin/views/organizations/details/layout/invite-users-dialog.tsxweb/sdk/admin/views/organizations/details/projects/use-add-project-members.tsxweb/sdk/admin/views/users/details/security/block-user.tsxweb/sdk/admin/views/users/list/invite-users.tsxweb/sdk/hooks/index.tsweb/sdk/react/hooks/usePreferences.tsweb/sdk/react/views/members/invite-member-dialog.tsxweb/sdk/react/views/plans/hooks/usePlans.tsxweb/sdk/react/views/projects/details/delete-project-dialog.tsxweb/sdk/react/views/projects/details/project-general.tsxweb/sdk/react/views/teams/details/invite-team-member-dialog.tsxweb/sdk/react/views/teams/details/team-general.tsxweb/sdk/react/views/teams/list/add-team-dialog.tsxweb/sdk/utils/error.ts
✅ Files skipped from review due to trivial changes (1)
- web/sdk/hooks/index.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- web/sdk/admin/views/organizations/details/projects/use-add-project-members.tsx
- web/sdk/react/views/members/invite-member-dialog.tsx
- web/sdk/react/views/teams/list/add-team-dialog.tsx
- web/sdk/react/views/plans/hooks/usePlans.tsx
Replace inconsistent error handling in views-new with handleConnectError utility across 14 files. Remove onError callbacks, add try/catch blocks, and map gRPC status codes to user-facing toasts via toastManager. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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)
web/sdk/react/views-new/members/components/update-role-dialog.tsx (1)
97-106:⚠️ Potential issue | 🟠 MajorStop role update flow when policy deletions fail
At Line 104, failed deletions are only logged and execution continues to create a new policy, then shows success. This can leave stale policies while reporting a successful update. Treat deletion failures as a hard error so the catch handler can show an error toast.
💡 Suggested fix
const deleteErrors = deleteResults .filter( (result): result is PromiseRejectedResult => result.status === 'rejected' ) .map(result => result.reason); if (deleteErrors.length > 0) { - console.warn('Some policy deletions failed:', deleteErrors); + throw ( + deleteErrors[0] instanceof Error + ? deleteErrors[0] + : new Error('Failed to update member role') + ); }Also applies to: 120-125
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: db87bc19-378e-49f1-813b-307bc5222ff3
📒 Files selected for processing (14)
web/sdk/react/views-new/general/components/delete-organization-dialog.tsxweb/sdk/react/views-new/general/general-view.tsxweb/sdk/react/views-new/members/components/invite-member-dialog.tsxweb/sdk/react/views-new/members/components/remove-member-dialog.tsxweb/sdk/react/views-new/members/components/update-role-dialog.tsxweb/sdk/react/views-new/profile/profile-view.tsxweb/sdk/react/views-new/projects/components/add-project-dialog.tsxweb/sdk/react/views-new/projects/components/delete-project-dialog.tsxweb/sdk/react/views-new/projects/components/edit-project-dialog.tsxweb/sdk/react/views-new/projects/components/remove-member-dialog.tsxweb/sdk/react/views-new/projects/project-details-view.tsxweb/sdk/react/views-new/security/components/add-domain-dialog.tsxweb/sdk/react/views-new/security/components/delete-domain-dialog.tsxweb/sdk/react/views-new/security/components/verify-domain-dialog.tsx
2 participants
Summary
handleConnectErrorutility insdk/utils/error.tsthat maps gRPC status codes (AlreadyExists,InvalidArgument,PermissionDenied,NotFound) to caller-defined handlers with aDefaultfallbackmutateAsync+onErrorwas used withouttry/catcherror.message,console.erroronly, duplicate toasts) with structuredhandleConnectErrorcallshandleConnectErrorfrom@raystack/frontier/hooksfor external consumersTest plan
🤖 Generated with Claude Code