Skip to content

fix(api): stop resolving a contact's conversation and inbox arbitrarily - #1248

Open
sung17 wants to merge 1 commit into
mainfrom
fix/contacts-api-conversation-resolution
Open

sung17 wants to merge 1 commit into
mainfrom
fix/contacts-api-conversation-resolution

Conversation

@sung17

@sung17 sung17 commented Sep 20, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • The public /v1/contacts/{identifier}/messages handlers picked both their target conversation and their target ContactInbox without an ordering, so which one they got was left to Postgres. A wrong resolution failed asynchronously — the handler had already answered 204 — which is the silent-send shape reported in Comment-originated contacts cannot be messaged via API even after the contact replies (silent failure) #879.
  • A contact holds a DM conversation plus one comment thread per post they commented on, so the same contact could resolve to a different conversation on two consecutive calls. The DM thread now wins, decided inside the lookup rather than behind a per-caller flag: with only the send path preferring it, an integrator could POST a message, get 204, then list the conversation and not find it. Send, list and get cannot drift because there is no flag to forget.
  • contactInboxes[0] is gone. That relation is keyed by contactId, so it carries every inbox the contact has across every channel — an unordered [0] could address the wrong page entirely.

Whether this accounts for the failure rate measured in #879 is still unconfirmed, so this refs the issue rather than closing it.

Changes

  • packages/business/src/conversation/service.ts

    • findByContactWithInboxes probes the DM conversation (sourceId IS NULL, a single hit on the unique Conversation_contactId_dm_key) before falling back. A contact who has only ever commented still resolves to their comment thread.
    • resolveContactInboxForSend resolves the ContactInbox via contactInboxService.findRecentByContactId, the same resolution resolveContactInboxForConversation already used. An explicit inboxId still wins.
    • Both orderings spelled as SQL with DESC NULLS LAST. lastActivityAt is nullable with no default, so a freshly created conversation holds NULL and a plain DESC would rank a never-active conversation above every real one. The { lastActivityAt: "desc" } object form cannot express the NULLS clause, and raw SQL reaches orderBy only through its callback form. This also matches Conversation_workspaceId_lastActivityAt_id_idx, declared .desc().nullsLast(). findLatestByContact carried the same latent bug and is fixed alongside.
    • Docstrings record why the channel is decided by the ContactInbox alone: a Conversation carries no inbox or channel column, and the single DM row a contact owns is shared across all their channels.
  • packages/business/__tests__/conversation-find-by-contact-with-inboxes.test.ts (new) — DM-first probe, comment-thread fallback, and the NULLS LAST ordering.

  • packages/business/__tests__/conversation-resolve-contact-inbox-for-send.test.ts — the case asserting the old contactInboxes[0] behaviour is replaced; the 404 paths now cover findRecentByContactId returning nothing.

Notes for reviewers

listMessages and getMessage are not touched: they call findByContactWithInboxes with no arguments and pick up the new behaviour automatically. That is deliberate — it is what makes the three endpoints agree by construction.

One behaviour change worth a second opinion: listMessages will no longer surface messages that live in a comment thread for a contact who also has a DM thread. Previously that was arbitrary, so it sometimes did. The endpoint has no parameter for choosing a thread; if that turns out to matter, the fix is to add a conversationId parameter rather than to go back to an arbitrary pick.

Test plan

  • pnpm --filter @chatbotx.io/business test — 274 files, 3083 tests pass
  • pnpm --filter builder test — 649 files, 5037 pass / 2 skipped
  • pnpm --filter builder check-types
  • pnpm lint
  • Manual: for a contact with both a DM thread and a comment thread, POST /v1/contacts/{id}/messages, then GET the same path and confirm the message is listed

The public `/v1/contacts/{identifier}/messages` handlers picked both halves of
their target without an ordering, so which one they got was left to Postgres.

`findByContactWithInboxes` ran `findFirst` with no `orderBy`, and a contact
holds a DM conversation plus one comment thread per post they commented on —
so the same contact could resolve to a different conversation on two
consecutive calls. It now prefers the DM thread, which is what all three
handlers are about. A contact who has only ever commented has no DM row and
still resolves to their comment thread.

Deciding that inside the lookup rather than behind a per-caller flag is the
point: with only the send path preferring the DM thread, an integrator could
POST a message, get 204, then list the conversation and not find it. There is
no flag to forget, so send, list and get cannot drift.

`resolveContactInboxForSend` took `contactInboxes[0]`. That relation is keyed
by `contactId`, so it carries every inbox the contact has across every channel
— an unordered `[0]` could address the wrong page entirely. It now comes from
`findRecentByContactId`, the same resolution `resolveContactInboxForConversation`
already used.

Both orderings are spelled as SQL with `DESC NULLS LAST`. `lastActivityAt` is
nullable with no default, so a freshly created conversation holds NULL and a
plain DESC would rank a never-active conversation above every real one; the
`{ lastActivityAt: "desc" }` object form cannot express the NULLS clause, and
raw SQL reaches `orderBy` only through its callback form. This also matches
`Conversation_workspaceId_lastActivityAt_id_idx`, declared `.desc().nullsLast()`.

A wrong resolution failed asynchronously — the handler had already answered
204 — which is the silent-send shape reported in #879. Whether it accounts for
the failure rate measured there is still unconfirmed, so this refs the issue
rather than closing it.

refs #879
@github-actions github-actions Bot added the bug Something isn't working as expected label Sep 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working as expected

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant