Handle Postmark InactiveRecipientsError gracefully#466
Conversation
🦋 Changeset detectedLatest commit: d3fbaa6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
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:
WalkthroughAdds detection and handling for Postmark "inactive recipient" errors in magic-link sign-in and organization-invite flows: catches the specific error, returns a localized Dutch user-facing message, and cancels created invitations when email delivery fails due to an inactive recipient. (≤50 words) Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant AppServer
participant AuthAPI
participant EmailLib
participant Postmark
Client->>AppServer: Request invite or magic-link
AppServer->>AuthAPI: createInvitation / requestMagicLink
AuthAPI-->>AppServer: returns (invitation.id / ok)
AppServer->>EmailLib: send email (invitation or magic link)
EmailLib->>Postmark: deliver email
alt Postmark OK
Postmark-->>EmailLib: 200 OK
EmailLib-->>AppServer: success
AppServer-->>Client: success message / toast
else Postmark InactiveRecipientsError
Postmark-->>EmailLib: 406 InactiveRecipientsError
EmailLib-->>AppServer: throws isInactiveRecipientError
AppServer->>AuthAPI: cancelInvitation (if invitation.id)
AppServer-->>Client: localized inactive-recipient error message
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## development #466 +/- ##
============================================
Coverage 88.10% 88.10%
============================================
Files 93 93
Lines 4675 4675
Branches 1498 1498
============================================
Hits 4119 4119
Misses 556 556
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.changeset/strong-symbols-beam.md:
- Line 5: Fix the grammar in the changelog sentence by replacing the incorrect
phrase "can't be send" with "can't be sent" in the changelog entry text (the
sentence beginning "While logging in or sending an organization invitation, if
an email can't be send due to an inactive recipient error from the Postmark
API..."). Ensure the updated sentence reads "...if an email can't be sent due to
an inactive recipient error from the Postmark API..." without changing other
wording.
In `@fdm-app/app/routes/organization`.$slug.tsx:
- Around line 435-452: The generic email failure branch currently returns an
error without cancelling the created invitation, leaving it stuck in pending;
modify the catch handler so that when sendEmail throws any error (i.e., in the
non-isInactiveRecipientError branch) you call auth.api.cancelInvitation with the
same headers and body (invitationId: invitation.id) before returning
dataWithError, similar to the isInactiveRecipientError branch; reference
sendEmail, isInactiveRecipientError, auth.api.cancelInvitation, invitation.id,
dataWithError, invitationEmail and formValues.email when making the change.
- Around line 438-448: When an inactive-recipient error is detected, add a
warning log that includes the affected email before the error is propagated;
update the detection within email.server.ts (where isInactiveRecipientError is
defined) to call console.warn(`Inactive recipient: ${email}`) (or similar) when
it returns true, or alternatively add the same console.warn in the error
handlers that call it (the handlers inside organization.$slug.tsx around the
branch using isInactiveRecipientError and in signin._index.tsx). Ensure the
warning runs prior to returning dataWithError or rethrowing so the email address
is recorded for debugging.
In `@fdm-app/app/routes/signin._index.tsx`:
- Around line 1672-1681: The catch block currently handles inactive-recipient
via isInactiveRecipientError and returns dataWithError, but for all other errors
it calls handleActionError(error) without throwing, which swallows the result;
prepend throw to the call (i.e., throw handleActionError(error)) so the action
returns the expected error flow; also update the console.error("Error sending
magic link") line to include the error details (e.g., console.error("Error
sending magic link", error)) to preserve useful logs; modify the catch around
isInactiveRecipientError, dataWithError, console.error, and handleActionError
accordingly.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
fdm-app/app/routes/organization.$slug.tsx (1)
444-462: Line 445 has an inconsistent error-handling pattern, not unreachable code.The
handleActionError(e)call on line 445 returns a Response object—it does not throw. The function always returns across all code paths with no throw statements. This means lines 446–462 are reachable and will execute correctly; the invitation cancellation and error responses function as intended.However, the pattern of calling
handleActionError(e)without throwing or returning its result is inconsistent with the rest of the codebase (lines 385, 401, 414, etc.), where the return value is always used. Additionally, there is no specific logging for inactive recipient errors as suggested in the previous review.Consider restructuring to handle specific errors before generic ones, capturing the logged response flow more explicitly:
Suggested pattern
} catch (e) { - handleActionError(e) if (invitation?.id) { await auth.api.cancelInvitation({ headers: request.headers, body: { invitationId: invitation.id, }, }) } if (isInactiveRecipientError(e)) { + console.warn( + `Inactive recipient error for ${formValues.email}`, + ) return dataWithError(null, { message: `We kunnen geen e-mails naar ${formValues.email} sturen omdat het als inactief is gemarkeerd. Neem contact op met de ondersteuning voor hulp.`, }) } + return handleActionError(e) - return dataWithError(null, { - message: `We kunnen geen e-mails naar ${formValues.email} sturen. Neem contact op met de ondersteuning voor hulp.`, - }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/routes/organization`.$slug.tsx around lines 444 - 462, Handle the specific inactive-recipient case before calling the generic error handler and make sure to return the handler's response; check isInactiveRecipientError(e) first and return dataWithError(...) for that case (include formValues.email), then if invitation?.id perform auth.api.cancelInvitation(...) before returning the generic error response, and finally return the result of handleActionError(e) when you want the centralized handling/logging behavior; reference handleActionError, isInactiveRecipientError, auth.api.cancelInvitation, dataWithError, invitation, and formValues.email to locate the changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@fdm-app/app/routes/organization`.$slug.tsx:
- Around line 446-452: The call to auth.api.cancelInvitation when invitation?.id
exists can throw and currently bubbles out into the outer catch, hiding the
original user-facing error; wrap the auth.api.cancelInvitation({...}) call in
its own try/catch so any cancellation failure is caught locally, log the
cancellation error (including context: invitation.id and request.headers) but do
not rethrow, ensuring the original upstream error handling (the outer catch that
returns the inactive-recipient or email-failure response) still executes; keep
the rest of the flow unchanged.
---
Nitpick comments:
In `@fdm-app/app/routes/organization`.$slug.tsx:
- Around line 444-462: Handle the specific inactive-recipient case before
calling the generic error handler and make sure to return the handler's
response; check isInactiveRecipientError(e) first and return dataWithError(...)
for that case (include formValues.email), then if invitation?.id perform
auth.api.cancelInvitation(...) before returning the generic error response, and
finally return the result of handleActionError(e) when you want the centralized
handling/logging behavior; reference handleActionError,
isInactiveRecipientError, auth.api.cancelInvitation, dataWithError, invitation,
and formValues.email to locate the changes.
…on email also fails
|
@coderabbitai we sometimes call handleActionError without awaiting it to be able to just report errors to Sentry. For example I think it is called like that on the signin page so that the user sees a toast instead of a full page error, and doesn't get a bad first impression of the application. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 97741 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 27 🏁 Script executed: Length of output: 374 🏁 Script executed: Length of output: 34 🏁 Script executed: Length of output: 48312 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 79 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 954 🏁 Script executed: Length of output: 102 🏁 Script executed: Length of output: 63 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 1472 🏁 Script executed: Length of output: 603 🏁 Script executed: Length of output: 39 🏁 Script executed: Length of output: 3780
This is a smart approach—you get error monitoring via Sentry while the user sees a friendly toast notification instead of a jarring 500 error page. Perfect for maintaining a good first impression during critical flows like sign-in and invitations! ✏️ Learnings added
🧠 Learnings used |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@fdm-app/app/routes/organization`.$slug.tsx:
- Around line 444-466: The catch block conflates invitation-creation failures
with email-send failures and shadows the outer error variable; update the error
handling so email-specific messages only run when an invitation was actually
created (i.e., invitation?.id is truthy) and distinguish a separate
invitation-creation failure message when createInvitation throws and invitation
is undefined; also rename the inner catch parameter (e.g., from "e" to
"emailErr" or "cancelErr") in the auth.api.cancelInvitation block to avoid
shadowing the outer "e", and keep using handleActionError for both errors and
return appropriate dataWithError messages using formValues.email or a generic
invitation failure message depending on whether invitation exists.
---
Duplicate comments:
In `@fdm-app/app/routes/organization`.$slug.tsx:
- Line 445: The call to handleActionError(e) runs for every exception (including
inactive-recipient cases) causing Sentry to record full errors; modify the flow
in the action handler so you first check isInactiveRecipientError(e) and for
that case emit a warning (use console.warn with the recipient address where
available) and skip calling handleActionError; only call handleActionError(e)
for non-inactive-recipient errors so they continue to be treated as full errors.
Ensure you reference the isInactiveRecipientError(...) check and the
handleActionError(...) call and add the console.warn(...) with the recipient
identifier in the inactive branch.
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@fdm-app/app/routes/organization`.$slug.tsx:
- Around line 458-465: The error message for email delivery is shown even when
createInvitation fails; update the try/catch in the invitation flow: rename the
catch parameter sendingError to a neutral inviteError, ensure email-specific
checks (isInactiveRecipientError(inviteError) and the "We kunnen geen
e-mails..." messages) only run when invitation?.id is present (i.e., an invite
was created and an email was attempted), and add a separate error path/message
for invitation creation/rendering failures (errors from createInvitation or
renderInvitationEmail) to return a distinct, accurate error. Also keep
references to createInvitation, renderInvitationEmail, sendEmail and
isInactiveRecipientError to locate and scope the changes.
Enhancements
Closes #454
Summary by CodeRabbit