From 854c9f2b16eff67dd55e081ba8b81096fd4e7d7b Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 9 Apr 2026 11:20:59 +0000 Subject: [PATCH] fix: address review findings for email forwarding feature - P0: Move S3 object to failed/ on download error (prevents infinite retry) - P1: Return existing channel if user already has one (idempotent creation) - P1: Map EMAIL_FORWARDING_NOT_CONFIGURED to InternalServerError - P1: Remove redundant MessageChannelType guard in hasPendingConfiguration - P1: Reset modal forwardingAddress state on close - Update test to verify moveToFailed on download error https://claude.ai/code/session_01KpyF6p4cUEnuaT4h8DP5Pm --- .../SettingsAccountsEmailForwardingModal.tsx | 7 ++++++- .../SettingsAccountsListEmptyStateCard.tsx | 1 + .../SettingsAccountsRowDropdownMenu.tsx | 2 -- .../message-channel-metadata.service.ts | 19 +++++++++++++++++++ ...nnel-graphql-api-exception-handler.util.ts | 3 ++- .../messaging-messages-import.cron.job.ts | 5 ++++- .../inbound-email-import.service.spec.ts | 3 ++- .../services/inbound-email-import.service.ts | 1 + 8 files changed, 35 insertions(+), 6 deletions(-) diff --git a/packages/twenty-front/src/modules/settings/accounts/components/SettingsAccountsEmailForwardingModal.tsx b/packages/twenty-front/src/modules/settings/accounts/components/SettingsAccountsEmailForwardingModal.tsx index 2725e63cc73..75330e0c1a2 100644 --- a/packages/twenty-front/src/modules/settings/accounts/components/SettingsAccountsEmailForwardingModal.tsx +++ b/packages/twenty-front/src/modules/settings/accounts/components/SettingsAccountsEmailForwardingModal.tsx @@ -65,10 +65,12 @@ const StyledButtonContainer = styled.div` type SettingsAccountsEmailForwardingModalProps = { forwardingAddress: string; + onClose?: () => void; }; export const SettingsAccountsEmailForwardingModal = ({ forwardingAddress, + onClose, }: SettingsAccountsEmailForwardingModalProps) => { const { t } = useLingui(); const { copyToClipboard } = useCopyToClipboard(); @@ -114,7 +116,10 @@ export const SettingsAccountsEmailForwardingModal = ({ title={t`Done`} variant="primary" size="small" - onClick={() => closeModal(EMAIL_FORWARDING_MODAL_ID)} + onClick={() => { + closeModal(EMAIL_FORWARDING_MODAL_ID); + onClose?.(); + }} /> diff --git a/packages/twenty-front/src/modules/settings/accounts/components/SettingsAccountsListEmptyStateCard.tsx b/packages/twenty-front/src/modules/settings/accounts/components/SettingsAccountsListEmptyStateCard.tsx index f97b652d488..44531dbe900 100644 --- a/packages/twenty-front/src/modules/settings/accounts/components/SettingsAccountsListEmptyStateCard.tsx +++ b/packages/twenty-front/src/modules/settings/accounts/components/SettingsAccountsListEmptyStateCard.tsx @@ -120,6 +120,7 @@ export const SettingsAccountsListEmptyStateCard = () => { {forwardingAddress && ( setForwardingAddress(null)} /> )} diff --git a/packages/twenty-front/src/modules/settings/accounts/components/SettingsAccountsRowDropdownMenu.tsx b/packages/twenty-front/src/modules/settings/accounts/components/SettingsAccountsRowDropdownMenu.tsx index 9263ba7456e..3249fa258b4 100644 --- a/packages/twenty-front/src/modules/settings/accounts/components/SettingsAccountsRowDropdownMenu.tsx +++ b/packages/twenty-front/src/modules/settings/accounts/components/SettingsAccountsRowDropdownMenu.tsx @@ -4,7 +4,6 @@ import { CalendarChannelSyncStage, ConnectedAccountProvider, MessageChannelSyncStage, - MessageChannelType, SettingsPath, } from 'twenty-shared/types'; @@ -61,7 +60,6 @@ export const SettingsAccountsRowDropdownMenu = ({ !isEmailForwarding && (account.messageChannels.some( (channel) => - channel.type !== MessageChannelType.EMAIL_FORWARDING && channel.syncStage === MessageChannelSyncStage.PENDING_CONFIGURATION, ) || account.calendarChannels.some( diff --git a/packages/twenty-server/src/engine/metadata-modules/message-channel/message-channel-metadata.service.ts b/packages/twenty-server/src/engine/metadata-modules/message-channel/message-channel-metadata.service.ts index e8a8eecfc1e..611aea179b6 100644 --- a/packages/twenty-server/src/engine/metadata-modules/message-channel/message-channel-metadata.service.ts +++ b/packages/twenty-server/src/engine/metadata-modules/message-channel/message-channel-metadata.service.ts @@ -214,6 +214,25 @@ export class MessageChannelMetadataService { ); } + const existingChannel = await this.repository.findOne({ + where: { + workspaceId, + type: MessageChannelType.EMAIL_FORWARDING, + connectedAccountId: In( + await this.connectedAccountMetadataService + .getUserConnectedAccountIds({ userWorkspaceId, workspaceId }) + .then((ids) => (ids.length > 0 ? ids : ['__none__'])), + ), + }, + }); + + if (existingChannel) { + return { + messageChannel: existingChannel, + forwardingAddress: existingChannel.handle, + }; + } + const localPart = INBOUND_EMAIL_LOCAL_PART_PREFIX + randomBytes(INBOUND_EMAIL_LOCAL_PART_RANDOM_BYTES).toString('hex'); diff --git a/packages/twenty-server/src/engine/metadata-modules/message-channel/utils/message-channel-graphql-api-exception-handler.util.ts b/packages/twenty-server/src/engine/metadata-modules/message-channel/utils/message-channel-graphql-api-exception-handler.util.ts index 749debf3aad..9a45f8ca1eb 100644 --- a/packages/twenty-server/src/engine/metadata-modules/message-channel/utils/message-channel-graphql-api-exception-handler.util.ts +++ b/packages/twenty-server/src/engine/metadata-modules/message-channel/utils/message-channel-graphql-api-exception-handler.util.ts @@ -2,6 +2,7 @@ import { assertUnreachable } from 'twenty-shared/utils'; import { ForbiddenError, + InternalServerError, NotFoundError, UserInputError, } from 'src/engine/core-modules/graphql/utils/graphql-errors.util'; @@ -24,7 +25,7 @@ export const messageChannelGraphqlApiExceptionHandler = (error: Error) => { case MessageChannelExceptionCode.MESSAGE_CHANNEL_OWNERSHIP_VIOLATION: throw new ForbiddenError(error); case MessageChannelExceptionCode.EMAIL_FORWARDING_NOT_CONFIGURED: - throw new UserInputError(error); + throw new InternalServerError(error); default: { return assertUnreachable(error.code); } diff --git a/packages/twenty-server/src/modules/messaging/message-import-manager/crons/jobs/messaging-messages-import.cron.job.ts b/packages/twenty-server/src/modules/messaging/message-import-manager/crons/jobs/messaging-messages-import.cron.job.ts index 13a14e829a8..8a02cc489ac 100644 --- a/packages/twenty-server/src/modules/messaging/message-import-manager/crons/jobs/messaging-messages-import.cron.job.ts +++ b/packages/twenty-server/src/modules/messaging/message-import-manager/crons/jobs/messaging-messages-import.cron.job.ts @@ -4,7 +4,10 @@ import { isDefined } from 'twenty-shared/utils'; import { WorkspaceActivationStatus } from 'twenty-shared/workspace'; import { DataSource, Repository } from 'typeorm'; -import { MessageChannelSyncStage, MessageChannelType } from 'twenty-shared/types'; +import { + MessageChannelSyncStage, + MessageChannelType, +} from 'twenty-shared/types'; import { SentryCronMonitor } from 'src/engine/core-modules/cron/sentry-cron-monitor.decorator'; import { ExceptionHandlerService } from 'src/engine/core-modules/exception-handler/exception-handler.service'; import { InjectMessageQueue } from 'src/engine/core-modules/message-queue/decorators/message-queue.decorator'; diff --git a/packages/twenty-server/src/modules/messaging/message-import-manager/drivers/inbound-email/services/__tests__/inbound-email-import.service.spec.ts b/packages/twenty-server/src/modules/messaging/message-import-manager/drivers/inbound-email/services/__tests__/inbound-email-import.service.spec.ts index cf425ace2f3..712c2c0b8c4 100644 --- a/packages/twenty-server/src/modules/messaging/message-import-manager/drivers/inbound-email/services/__tests__/inbound-email-import.service.spec.ts +++ b/packages/twenty-server/src/modules/messaging/message-import-manager/drivers/inbound-email/services/__tests__/inbound-email-import.service.spec.ts @@ -177,7 +177,7 @@ describe('InboundEmailImportService', () => { expect(storageService.moveToProcessed).toHaveBeenCalledWith(TEST_S3_KEY); }); - it('should return "parse_failed" when download fails', async () => { + it('should return "parse_failed" and move to failed/ when download fails', async () => { storageService.getRawMessage.mockRejectedValue( new Error('S3 download error'), ); @@ -188,6 +188,7 @@ describe('InboundEmailImportService', () => { kind: 'parse_failed', error: 'S3 download error', }); + expect(storageService.moveToFailed).toHaveBeenCalledWith(TEST_S3_KEY); }); it('should return "parse_failed" when parsing fails', async () => { diff --git a/packages/twenty-server/src/modules/messaging/message-import-manager/drivers/inbound-email/services/inbound-email-import.service.ts b/packages/twenty-server/src/modules/messaging/message-import-manager/drivers/inbound-email/services/inbound-email-import.service.ts index 64180cb4a75..8883c7e4624 100644 --- a/packages/twenty-server/src/modules/messaging/message-import-manager/drivers/inbound-email/services/inbound-email-import.service.ts +++ b/packages/twenty-server/src/modules/messaging/message-import-manager/drivers/inbound-email/services/inbound-email-import.service.ts @@ -61,6 +61,7 @@ export class InboundEmailImportService { this.logger.error( `Failed to download inbound email from S3 key ${s3Key}: ${message}`, ); + await this.safeMove(s3Key, 'failed'); return { kind: 'parse_failed', error: message }; }