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

packages/business/__tests__/conversation-find-by-contact-with-inboxes.test.ts

@@ -0,0 +1,106 @@
+// @vitest-environment node
+import { afterEach, describe, expect, test, vi } from "vitest"
+
+const { db } = await import("@chatbotx.io/database/client")
+const { conversationModel } = await import("@chatbotx.io/database/schema")
+const { conversationService } = await import("../src/conversation/service")
+
+const spyOnFindFirst = () =>
+  vi.spyOn(db.query.conversationModel, "findFirst" as never)
+
+/**
+ * The literal text of the `orderBy` drizzle was handed.
+ *
+ * It arrives as drizzle's callback form — the only form that accepts a raw SQL
+ * expression — so it is invoked first. `queryChunks` then interleaves
+ * `StringChunk` (whose `value` is a string array) with column references, and
+ * those reference their table, so the object as a whole cannot be stringified.
+ */
+const orderBySql = (orderBy: unknown): string => {
+  const expression =
+    typeof orderBy === "function"
+      ? (orderBy as (table: unknown) => unknown)(conversationModel)
+      : orderBy
+  return ((expression as { queryChunks?: unknown[] }).queryChunks ?? [])
+    .flatMap((chunk) => (chunk as { value?: unknown }).value ?? [])
+    .filter((part): part is string => typeof part === "string")
+    .join("")
+}
+
+afterEach(() => {
+  vi.restoreAllMocks()
+})
+
+describe("conversationService.findByContactWithInboxes", () => {
+  // `lastActivityAt` is nullable with no default, and Postgres puts NULLs FIRST
+  // on a plain DESC — so the object form `{ lastActivityAt: "desc" }` would
+  // rank a conversation that has never been active above every real one.
+  test("orders by lastActivityAt with NULLS LAST, not a bare DESC", async () => {
+    const findFirst = spyOnFindFirst().mockResolvedValue({
+      id: "conv-dm",
+      contactInboxes: [],
+    } as never)
+
+    await conversationService.findByContactWithInboxes({
+      contactId: "contact-1",
+      workspaceId: "ws-1",
+    })
+
+    const { orderBy } = findFirst.mock.calls[0]?.[0] as { orderBy: unknown }
+    expect(orderBySql(orderBy)).toContain("DESC NULLS LAST")
+  })
+
+  // Every caller is a /v1/contacts/{identifier}/messages handler — send, list
+  // and get. When only the send path preferred the DM thread, an integrator
+  // could POST, get 204, then list and not find the message. There is no flag
+  // to forget, so the three cannot drift.
+  // `Conversation_contactId_dm_key` is unique on contactId where sourceId IS
+  // NULL, so this probe can match at most one row.
+  test("probes the DM thread first, with no caller opt-in", async () => {
+    const directMessage = { id: "conv-dm", sourceId: null, contactInboxes: [] }
+    const findFirst = spyOnFindFirst().mockResolvedValue(directMessage as never)
+
+    const result = await conversationService.findByContactWithInboxes({
+      contactId: "contact-1",
+      workspaceId: "ws-1",
+    })
+
+    expect(result).toEqual(directMessage)
+    expect(findFirst).toHaveBeenCalledTimes(1)
+    expect(findFirst).toHaveBeenCalledWith(
+      expect.objectContaining({
+        where: {
+          contactId: "contact-1",
+          workspaceId: "ws-1",
+          sourceId: { isNull: true },
+        },
+      }),
+    )
+  })
+
+  // A contact who has only ever commented has no DM thread to prefer. Falling
+  // through to the unfiltered lookup is what keeps them reachable.
+  test("falls back to any conversation when the contact has no DM thread", async () => {
+    const commentThread = {
+      id: "conv-comment",
+      sourceId: "page-1_post-1",
+      contactInboxes: [],
+    }
+    const findFirst = spyOnFindFirst()
+      .mockResolvedValueOnce(undefined as never)
+      .mockResolvedValueOnce(commentThread as never)
+
+    const result = await conversationService.findByContactWithInboxes({
+      contactId: "contact-1",
+      workspaceId: "ws-1",
+    })
+
+    expect(result).toEqual(commentThread)
+    expect(findFirst).toHaveBeenCalledTimes(2)
+    expect(findFirst).toHaveBeenLastCalledWith(
+      expect.objectContaining({
+        where: { contactId: "contact-1", workspaceId: "ws-1" },
+      }),
+    )
+  })
+})

packages/business/__tests__/conversation-resolve-contact-inbox-for-send.test.ts

@@ -2,6 +2,7 @@
 import { beforeEach, describe, expect, test, vi } from "vitest"
 
 const { conversationService } = await import("../src/conversation/service")
+const { contactInboxService } = await import("../src/contact-inbox/service")
 
 describe("conversationService.resolveContactInboxForSend", () => {
   beforeEach(() => {
@@ -28,22 +29,86 @@ describe("conversationService.resolveContactInboxForSend", () => {
     expect(result.conversation.id).toBe("conv-1")
   })
 
-  test("falls back to the first contact inbox when inboxId is omitted", async () => {
-    const contactInboxA = { id: "ci-a", inboxId: "inbox-a" }
-    const contactInboxB = { id: "ci-b", inboxId: "inbox-b" }
+  // The relation on the conversation row is keyed by `contactId`, so it holds
+  // every inbox the contact has across every channel. Taking `[0]` of that
+  // unordered list could address the wrong page entirely.
+  test("resolves the most recently active contact inbox when inboxId is omitted", async () => {
+    const stale = { id: "ci-stale", inboxId: "inbox-a" }
+    const recent = { id: "ci-recent", inboxId: "inbox-b" }
     vi.spyOn(conversationService, "findByContactWithInboxes").mockResolvedValue(
       {
         id: "conv-1",
-        contactInboxes: [contactInboxA, contactInboxB],
+        contactInboxes: [stale, recent],
       } as never,
     )
+    const findRecent = vi
+      .spyOn(contactInboxService, "findRecentByContactId")
+      .mockResolvedValue(recent as never)
 
     const result = await conversationService.resolveContactInboxForSend({
       contactId: "contact-1",
       workspaceId: "ws-1",
     })
 
-    expect(result.contactInbox).toEqual(contactInboxA)
+    expect(result.contactInbox).toEqual(recent)
+    expect(findRecent).toHaveBeenCalledWith({
+      workspaceId: "ws-1",
+      contactId: "contact-1",
+    })
+  })
+
+  // The DM-vs-comment-thread choice belongs to `findByContactWithInboxes` and
+  // is not a flag this caller passes — that is what keeps the send path and the
+  // list/get paths on the same conversation. Asserted here only so the send
+  // path cannot start routing around it.
+  test("takes the conversation findByContactWithInboxes settles on, passing no opt-in", async () => {
+    const findConversation = vi
+      .spyOn(conversationService, "findByContactWithInboxes")
+      .mockResolvedValue({
+        id: "conv-dm",
+        sourceId: null,
+        contactInboxes: [{ id: "ci-a", inboxId: "inbox-a" }],
+      } as never)
+    vi.spyOn(contactInboxService, "findRecentByContactId").mockResolvedValue({
+      id: "ci-a",
+      inboxId: "inbox-a",
+    } as never)
+
+    const result = await conversationService.resolveContactInboxForSend({
+      contactId: "contact-1",
+      workspaceId: "ws-1",
+    })
+
+    expect(findConversation).toHaveBeenCalledWith({
+      contactId: "contact-1",
+      workspaceId: "ws-1",
+    })
+    expect(result.conversation.id).toBe("conv-dm")
+  })
+
+  // A contact who has only ever commented has no DM conversation to prefer.
+  // Their comment conversation must still resolve, or comment-origin contacts
+  // become unreachable through the public API.
+  test("still resolves a comment conversation when the contact has no DM thread", async () => {
+    vi.spyOn(conversationService, "findByContactWithInboxes").mockResolvedValue(
+      {
+        id: "conv-comment",
+        sourceId: "page-1_post-1",
+        contactInboxes: [{ id: "ci-a", inboxId: "inbox-a" }],
+      } as never,
+    )
+    vi.spyOn(contactInboxService, "findRecentByContactId").mockResolvedValue({
+      id: "ci-a",
+      inboxId: "inbox-a",
+    } as never)
+
+    const result = await conversationService.resolveContactInboxForSend({
+      contactId: "contact-1",
+      workspaceId: "ws-1",
+    })
+
+    expect(result.conversation.id).toBe("conv-comment")
+    expect(result.contactInbox).toEqual({ id: "ci-a", inboxId: "inbox-a" })
   })
 
   test("404s when no conversation exists for the contact", async () => {
@@ -76,13 +141,16 @@ describe("conversationService.resolveContactInboxForSend", () => {
     ).rejects.toMatchObject({ code: "notFound" })
   })
 
-  test("404s when the conversation has no contact inboxes at all", async () => {
+  test("404s when the contact has no contact inbox at all", async () => {
     vi.spyOn(conversationService, "findByContactWithInboxes").mockResolvedValue(
       {
         id: "conv-1",
         contactInboxes: [],
       } as never,
     )
+    vi.spyOn(contactInboxService, "findRecentByContactId").mockResolvedValue(
+      undefined,
+    )
 
     await expect(
       conversationService.resolveContactInboxForSend({

packages/business/src/conversation/service.ts

@@ -306,24 +306,81 @@ class ConversationService extends BaseService {
     return rows.length > 0
   }
 
+  /**
+   * A contact can hold a DM conversation and one comment thread per post, so
+   * "the contact's conversation" is a choice, not a lookup. Without an order
+   * `findFirst` returned whichever row Postgres handed back — the same contact
+   * could resolve to a different conversation on two consecutive calls.
+   *
+   * The DM thread always wins. Every caller is a
+   * `/v1/contacts/{identifier}/messages` handler — send, list and get — and a
+   * direct message is what all three are about, so none of them wants a post's
+   * comment thread. Deciding it here rather than behind a per-caller flag is
+   * the point: when the send path preferred the DM thread and the read paths
+   * did not, an integrator could POST a message, get 204, then list the
+   * conversation and not find it. The three cannot drift if there is nothing
+   * to pass. A contact who has only ever commented has no DM row to prefer and
+   * still resolves to their comment thread.
+   *
+   * The DM probe is a single index hit: `Conversation_contactId_dm_key` is
+   * unique on `contactId` where `sourceId IS NULL`, so at most one row matches.
+   */
   async findByContactWithInboxes(props: {
     contactId: string
     workspaceId: string
     tx?: DatabaseClient
   }): Promise<ConversationWithContactInboxes | undefined> {
     const { tx = db, contactId, workspaceId } = props
-    return (await tx.query.conversationModel.findFirst({
-      where: { contactId, workspaceId },
-      with: { contactInboxes: true },
-    })) as ConversationWithContactInboxes | undefined
+
+    const findFirstWhere = async (where: Record<string, unknown>) =>
+      (await tx.query.conversationModel.findFirst({
+        where,
+        // Spelled as SQL because `lastActivityAt` is nullable with no default —
+        // a freshly created conversation has NULL — and Postgres puts NULLs
+        // FIRST on a plain DESC. The `{ lastActivityAt: "desc" }` object form
+        // cannot express the NULLS clause, so it would rank a conversation that
+        // has never been active above every real one. This also matches
+        // `Conversation_workspaceId_lastActivityAt_id_idx`, declared
+        // `.desc().nullsLast()`. Raw SQL reaches `orderBy` only through its
+        // callback form; the object form takes no expression.
+        orderBy: (table) => sql`${table.lastActivityAt} DESC NULLS LAST`,
+        with: { contactInboxes: true },
+      })) as ConversationWithContactInboxes | undefined
+
+    const directMessage = await findFirstWhere({
+      contactId,
+      workspaceId,
+      sourceId: { isNull: true },
+    })
+    if (directMessage) {
+      return directMessage
+    }
+
+    return await findFirstWhere({ contactId, workspaceId })
   }
 
   /**
    * Shared by the public `/v1/contacts/{identifier}/messages|auto-replies|flows`
    * handlers: resolve the contact's conversation and the specific
-   * `ContactInbox` to send through (or the first one when `inboxId` is
-   * omitted), throwing the same 404 either way instead of repeating both
-   * lookups + both `notFoundException` calls at every call site.
+   * `ContactInbox` to send through, throwing the same 404 either way instead of
+   * repeating both lookups + both `notFoundException` calls at every call site.
+   *
+   * Both halves used to be picked arbitrarily, and a send that resolved wrong
+   * failed asynchronously — the handler had already answered 204.
+   *
+   * - The conversation comes from `findByContactWithInboxes`, which settles the
+   *   DM-vs-comment-thread choice for the read paths too.
+   * - The `ContactInbox` now comes from `findRecentByContactId`. The relation
+   *   on the conversation row is keyed by `contactId`, so it carries every
+   *   inbox the contact has across every channel — taking `[0]` of an unordered
+   *   list could address the wrong page entirely. An explicit `inboxId` still
+   *   wins and is still matched against that relation.
+   *
+   * The channel is therefore 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 of their channels. So for a contact
+   * connected on more than one channel, omitting `inboxId` makes the channel a
+   * best guess ("most recently active"). Pass `inboxId` when it must be exact.
    */
   async resolveContactInboxForSend(props: {
     contactId: string
@@ -344,7 +401,10 @@ class ConversationService extends BaseService {
 
     const contactInbox = inboxId
       ? conversation.contactInboxes.find((ci) => ci.inboxId === inboxId)
-      : conversation.contactInboxes[0]
+      : await contactInboxService.findRecentByContactId({
+          workspaceId,
+          contactId,
+        })
     if (!contactInbox) {
       throw notFoundException("Conversation not found")
     }
@@ -357,9 +417,8 @@ class ConversationService extends BaseService {
    * `createMessageAction`: resolve the `ContactInbox` to send an outgoing
    * message through, scoped to an already-identified `conversationId` rather
    * than `contactId` (see `resolveContactInboxForSend` for the public-API
-   * variant, which starts from `contactId` and falls back to the
-   * conversation's first `ContactInbox` instead of the most recently
-   * active one).
+   * variant, which starts from `contactId` and resolves the conversation too).
+   * Both settle on the same `ContactInbox` for a given contact.
    */
   async resolveContactInboxForConversation(props: {
     conversation: Pick<ConversationModel, "contactId">
@@ -389,9 +448,11 @@ class ConversationService extends BaseService {
     // A contact can have multiple conversations (DM + comment threads), all
     // sharing the same ContactInbox — order by lastActivityAt so callers get
     // the conversation the contact is actually active in, not an arbitrary one.
+    // NULLS LAST is not optional here: the column is nullable with no default,
+    // so a plain DESC ranks a never-active conversation above every real one.
     return await tx.query.conversationModel.findFirst({
       where: { contactId },
-      orderBy: { lastActivityAt: "desc" },
+      orderBy: (table) => sql`${table.lastActivityAt} DESC NULLS LAST`,
     })
   }