fix(whatsapp): resolve reaction UUID targets#691
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces changes to resolve Omni message UUIDs to channel-native external IDs when sending message reactions, adding helper functions resolveReactionTarget and getReactionTargetByOmniId along with comprehensive unit tests. Feedback on the changes highlights an issue in getReactionTargetByOmniId where checking the chatId mismatch inside the try block and catching it in the same block can lead to unintended error handling behavior; moving this check outside the try-catch block is recommended to ensure consistent error wrapping and context enrichment.
| async function getReactionTargetByOmniId( | ||
| services: Services, | ||
| instanceId: string, | ||
| chatId: string, | ||
| messageId: string, | ||
| ): Promise<Awaited<ReturnType<Services['messages']['getByExternalId']>>> { | ||
| try { | ||
| const target = await services.messages.getById(messageId); | ||
| if (target.chatId !== chatId) { | ||
| throw new OmniError({ | ||
| code: ERROR_CODES.NOT_FOUND, | ||
| message: `Reaction target message not found: ${messageId}`, | ||
| context: { instanceId, chatId, messageId }, | ||
| recoverable: false, | ||
| }); | ||
| } | ||
| return target; | ||
| } catch (error) { | ||
| if (error instanceof OmniError) throw error; | ||
| throw new OmniError({ | ||
| code: ERROR_CODES.NOT_FOUND, | ||
| message: `Reaction target message not found: ${messageId}`, | ||
| context: { instanceId, chatId, messageId }, | ||
| recoverable: false, | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
In the current implementation of getReactionTargetByOmniId, throwing an error inside the try block and catching it in the same catch block leads to a bug where error instanceof OmniError is true for the newly thrown error, which is then immediately rethrown. Furthermore, if getById throws a NotFoundError (which extends OmniError), it is rethrown directly without the enriched context (instanceId, chatId, messageId) and the specific error message.
Moving the chatId mismatch check outside of the try-catch block resolves both issues, ensuring that any message-not-found error is consistently wrapped with the rich context and specific error message.
async function getReactionTargetByOmniId(
services: Services,
instanceId: string,
chatId: string,
messageId: string,
): Promise<Awaited<ReturnType<Services['messages']['getById']>>> {
let target: Awaited<ReturnType<Services['messages']['getById']>>;
try {
target = await services.messages.getById(messageId);
} catch (error) {
throw new OmniError({
code: ERROR_CODES.NOT_FOUND,
message: "Reaction target message not found: " + messageId,
context: { instanceId, chatId, messageId },
recoverable: false,
});
}
if (target.chatId !== chatId) {
throw new OmniError({
code: ERROR_CODES.NOT_FOUND,
message: "Reaction target message not found: " + messageId,
context: { instanceId, chatId, messageId },
recoverable: false,
});
}
return target;
}
namastex888
force-pushed
the
fix/whatsapp-reaction-uuid-target-dev
branch
from
dcb66bc to
e7ec629
Compare
|
Addressed the Gemini review feedback: narrowed UUID target lookup error handling so only |
1 participant
Summary
devTest Plan
bunx biome check packages/api/src/routes/v2/messages.ts packages/api/src/routes/v2/__tests__/messages-send-reaction.test.tsbun test packages/api/src/routes/v2/__tests__/messages-send-reaction.test.ts packages/cli/src/__tests__/react-context.test.tsbun run --filter @omni/api typecheckbun run --filter @automagik/omni build:serverfelipePR branch:3963 pass / 292 skip / 0 fail