diff --git a/apps/web/src/routes/api/v1/mentions/__tests__/suggest.test.ts b/apps/web/src/routes/api/v1/mentions/__tests__/suggest.test.ts index 93910998..070e788b 100644 --- a/apps/web/src/routes/api/v1/mentions/__tests__/suggest.test.ts +++ b/apps/web/src/routes/api/v1/mentions/__tests__/suggest.test.ts @@ -39,6 +39,11 @@ vi.mock('@/lib/server/db', () => ({ avatarUrl: 'avatar_url', avatarKey: 'avatar_key', }, + user: { + id: 'id', + image: 'image', + imageKey: 'image_key', + }, eq: vi.fn((col, val) => ({ _eq: [col, val] })), and: vi.fn((...args) => ({ _and: args })), inArray: vi.fn((col, vals) => ({ _inArray: [col, vals] })), @@ -88,13 +93,15 @@ interface QueryChainCapture { function makeChain(rows: unknown[], capture: QueryChainCapture) { return { from: vi.fn().mockReturnValue({ - where: vi.fn((arg: unknown) => { - capture.whereArg = arg - return { - orderBy: vi.fn().mockReturnValue({ - limit: vi.fn().mockResolvedValue(rows), - }), - } + leftJoin: vi.fn().mockReturnValue({ + where: vi.fn((arg: unknown) => { + capture.whereArg = arg + return { + orderBy: vi.fn().mockReturnValue({ + limit: vi.fn().mockResolvedValue(rows), + }), + } + }), }), }), } @@ -118,6 +125,8 @@ describe('GET /api/v1/mentions/suggest', () => { avatarUrl: 'https://avatars/jane.png', avatarKey: null, role: 'admin', + userImage: null, + userImageKey: null, }, { id: 'principal_jake', @@ -125,6 +134,8 @@ describe('GET /api/v1/mentions/suggest', () => { avatarUrl: null, avatarKey: 'avatars/jake.png', role: 'member', + userImage: null, + userImageKey: null, }, ] mockSelect.mockReturnValueOnce(makeChain(rows, capture)) @@ -154,6 +165,40 @@ describe('GET /api/v1/mentions/suggest', () => { ]) }) + it('falls back to user.imageKey / user.image when principal avatar columns are null', async () => { + // Same fallback chain the hover-card endpoint uses, so the picker and + // the card agree on which avatar to show for stale principal rows. + vi.mocked(auth.api.getSession).mockResolvedValueOnce(identifiedSession) + vi.mocked(db.query.principal.findFirst).mockResolvedValueOnce(userPrincipal) + const capture: QueryChainCapture = {} + const rows = [ + { + id: 'principal_stale', + displayName: 'Stale Principal', + avatarUrl: null, + avatarKey: null, + role: 'user', + userImage: null, + userImageKey: 'avatars/2026/03/stale-key.png', + }, + { + id: 'principal_oauth', + displayName: 'OAuth Only', + avatarUrl: null, + avatarKey: null, + role: 'user', + userImage: 'https://lh3.googleusercontent.com/a/abc', + userImageKey: null, + }, + ] + mockSelect.mockReturnValueOnce(makeChain(rows, capture)) + + const res = await handleMentionSuggest({ request: makeRequest('s') }) + const body = (await res.json()) as Array<{ principalId: string; avatarUrl: string | null }> + expect(body[0].avatarUrl).toBe('https://cdn.example.com/avatars/2026/03/stale-key.png') + expect(body[1].avatarUrl).toBe('https://lh3.googleusercontent.com/a/abc') + }) + it('returns 403 for an anonymous session', async () => { vi.mocked(auth.api.getSession).mockResolvedValueOnce(identifiedSession) vi.mocked(db.query.principal.findFirst).mockResolvedValueOnce(anonymousPrincipal) @@ -188,6 +233,8 @@ describe('GET /api/v1/mentions/suggest', () => { avatarUrl: null, avatarKey: null, role: 'admin', + userImage: null, + userImageKey: null, }, { id: 'principal_bob', @@ -195,6 +242,8 @@ describe('GET /api/v1/mentions/suggest', () => { avatarUrl: null, avatarKey: null, role: 'user', + userImage: null, + userImageKey: null, }, ] mockSelect.mockReturnValueOnce(makeChain(rows, capture)) @@ -235,10 +284,19 @@ describe('GET /api/v1/mentions/suggest', () => { await handleMentionSuggest({ request: makeRequest('JaNe') }) - // The selected columns must not include email. + // The selected columns must not include email. user.image / user.imageKey + // are joined in for the avatar fallback chain, but email never is. const selectArg = mockSelect.mock.calls[0][0] as Record expect(Object.keys(selectArg)).toEqual( - expect.arrayContaining(['id', 'displayName', 'avatarUrl', 'avatarKey', 'role']) + expect.arrayContaining([ + 'id', + 'displayName', + 'avatarUrl', + 'avatarKey', + 'role', + 'userImage', + 'userImageKey', + ]) ) expect(Object.keys(selectArg)).not.toContain('email') diff --git a/apps/web/src/routes/api/v1/mentions/suggest.ts b/apps/web/src/routes/api/v1/mentions/suggest.ts index 8e04e7c6..63d27b04 100644 --- a/apps/web/src/routes/api/v1/mentions/suggest.ts +++ b/apps/web/src/routes/api/v1/mentions/suggest.ts @@ -14,7 +14,7 @@ import { createFileRoute } from '@tanstack/react-router' import type { UserId } from '@quackback/ids' import type { SQL } from 'drizzle-orm' import { auth } from '@/lib/server/auth' -import { db, principal, eq, and, inArray, sql } from '@/lib/server/db' +import { db, principal, user, eq, and, inArray, sql } from '@/lib/server/db' import type { Role } from '@/lib/shared/roles' import { incrementBucket } from '@/lib/server/utils/redis-rate-bucket' import { getPublicUrlOrNull } from '@/lib/server/storage/s3' @@ -31,12 +31,25 @@ interface SuggestRow { role: Role } -function resolveAvatar(avatarKey: string | null, avatarUrl: string | null): string | null { - if (avatarKey) { - const s3Url = getPublicUrlOrNull(avatarKey) +// Same fallback chain as /api/v1/users/:id/card so the picker and the +// hover card agree on what avatar to show for stale principal rows +// whose `avatar_*` columns drifted from the linked user record. +function resolveSuggestAvatar(opts: { + principalAvatarKey: string | null + principalAvatarUrl: string | null + userImageKey: string | null | undefined + userImage: string | null | undefined +}): string | null { + if (opts.principalAvatarKey) { + const s3Url = getPublicUrlOrNull(opts.principalAvatarKey) if (s3Url) return s3Url } - return avatarUrl ?? null + if (opts.principalAvatarUrl) return opts.principalAvatarUrl + if (opts.userImageKey) { + const s3Url = getPublicUrlOrNull(opts.userImageKey) + if (s3Url) return s3Url + } + return opts.userImage ?? null } export async function handleMentionSuggest({ request }: { request: Request }): Promise { @@ -87,8 +100,11 @@ export async function handleMentionSuggest({ request }: { request: Request }): P avatarUrl: principal.avatarUrl, avatarKey: principal.avatarKey, role: principal.role, + userImage: user.image, + userImageKey: user.imageKey, }) .from(principal) + .leftJoin(user, eq(user.id, principal.userId)) .where(and(...predicates)) .orderBy(sql`lower(${principal.displayName})`) .limit(SUGGEST_LIMIT) @@ -96,7 +112,12 @@ export async function handleMentionSuggest({ request }: { request: Request }): P const result: SuggestRow[] = rows.map((r) => ({ principalId: r.id, displayName: r.displayName, - avatarUrl: resolveAvatar(r.avatarKey, r.avatarUrl), + avatarUrl: resolveSuggestAvatar({ + principalAvatarKey: r.avatarKey, + principalAvatarUrl: r.avatarUrl, + userImageKey: r.userImageKey, + userImage: r.userImage, + }), role: r.role as Role, })) diff --git a/apps/web/src/routes/api/v1/users/$principalId.card.ts b/apps/web/src/routes/api/v1/users/$principalId.card.ts index a9800890..89b7c16f 100644 --- a/apps/web/src/routes/api/v1/users/$principalId.card.ts +++ b/apps/web/src/routes/api/v1/users/$principalId.card.ts @@ -13,7 +13,7 @@ import { createFileRoute } from '@tanstack/react-router' import type { PrincipalId, UserId } from '@quackback/ids' import { auth } from '@/lib/server/auth' -import { db, principal, eq } from '@/lib/server/db' +import { db, principal, user, eq } from '@/lib/server/db' import { getPublicUrlOrNull } from '@/lib/server/storage/s3' interface PrincipalCardBody { @@ -24,12 +24,26 @@ interface PrincipalCardBody { joinedAt: string } -function resolveAvatar(avatarKey: string | null, avatarUrl: string | null): string | null { - if (avatarKey) { - const s3Url = getPublicUrlOrNull(avatarKey) +// Resolve in the order: principal own avatar → linked user image. The +// user fallback covers principal rows that pre-date syncPrincipalProfile +// being wired into every avatar-upload path, or any other gap where the +// principal mirror drifted from the source-of-truth user record. +function resolveCardAvatar(opts: { + principalAvatarKey: string | null + principalAvatarUrl: string | null + userImageKey: string | null | undefined + userImage: string | null | undefined +}): string | null { + if (opts.principalAvatarKey) { + const s3Url = getPublicUrlOrNull(opts.principalAvatarKey) if (s3Url) return s3Url } - return avatarUrl ?? null + if (opts.principalAvatarUrl) return opts.principalAvatarUrl + if (opts.userImageKey) { + const s3Url = getPublicUrlOrNull(opts.userImageKey) + if (s3Url) return s3Url + } + return opts.userImage ?? null } export async function handlePrincipalCard({ @@ -55,17 +69,21 @@ export async function handlePrincipalCard({ } const targetId = params.principalId as PrincipalId - const row = await db.query.principal.findFirst({ - where: eq(principal.id, targetId), - columns: { - id: true, - displayName: true, - avatarUrl: true, - avatarKey: true, - role: true, - createdAt: true, - }, - }) + const [row] = await db + .select({ + id: principal.id, + displayName: principal.displayName, + avatarUrl: principal.avatarUrl, + avatarKey: principal.avatarKey, + role: principal.role, + createdAt: principal.createdAt, + userImage: user.image, + userImageKey: user.imageKey, + }) + .from(principal) + .leftJoin(user, eq(user.id, principal.userId)) + .where(eq(principal.id, targetId)) + .limit(1) if (!row) { return Response.json({ error: 'Not Found' }, { status: 404 }) @@ -74,7 +92,12 @@ export async function handlePrincipalCard({ const body: PrincipalCardBody = { principalId: row.id, displayName: row.displayName ?? '', - avatarUrl: resolveAvatar(row.avatarKey, row.avatarUrl), + avatarUrl: resolveCardAvatar({ + principalAvatarKey: row.avatarKey, + principalAvatarUrl: row.avatarUrl, + userImageKey: row.userImageKey, + userImage: row.userImage, + }), role: row.role, joinedAt: row.createdAt.toISOString(), } diff --git a/apps/web/src/routes/api/v1/users/__tests__/principal-card.test.ts b/apps/web/src/routes/api/v1/users/__tests__/principal-card.test.ts index cdf43651..cbfcf5bd 100644 --- a/apps/web/src/routes/api/v1/users/__tests__/principal-card.test.ts +++ b/apps/web/src/routes/api/v1/users/__tests__/principal-card.test.ts @@ -14,11 +14,21 @@ vi.mock('@/lib/server/auth', () => ({ }, })) +// Target-row lookup uses a join, so we mock the select-builder chain alongside +// the findFirst the caller-lookup still uses. +const selectChain = { + from: vi.fn().mockReturnThis(), + leftJoin: vi.fn().mockReturnThis(), + where: vi.fn().mockReturnThis(), + limit: vi.fn(), +} + vi.mock('@/lib/server/db', () => ({ db: { query: { principal: { findFirst: vi.fn() }, }, + select: vi.fn(() => selectChain), }, principal: { id: 'id', @@ -30,6 +40,11 @@ vi.mock('@/lib/server/db', () => ({ avatarKey: 'avatar_key', createdAt: 'created_at', }, + user: { + id: 'id', + image: 'image', + imageKey: 'image_key', + }, eq: vi.fn((col, val) => ({ _eq: [col, val] })), })) @@ -53,25 +68,43 @@ function makeRequest(): Request { return new Request('http://localhost/api/v1/users/principal_jane/card', { method: 'GET' }) } +type TargetRow = { + id: string + displayName: string | null + avatarUrl: string | null + avatarKey: string | null + role: string + createdAt: Date + userImage: string | null + userImageKey: string | null +} + +function mockTargetRow(row: TargetRow | null): void { + selectChain.limit.mockResolvedValueOnce(row ? [row] : []) +} + describe('GET /api/v1/users/:principalId/card', () => { beforeEach(() => { vi.clearAllMocks() + selectChain.from.mockReturnValue(selectChain) + selectChain.leftJoin.mockReturnValue(selectChain) + selectChain.where.mockReturnValue(selectChain) }) it('returns 200 + body for an existing principal', async () => { vi.mocked(auth.api.getSession).mockResolvedValueOnce(identifiedSession) - // First findFirst: caller lookup vi.mocked(db.query.principal.findFirst).mockResolvedValueOnce(callerPrincipal) - // Second findFirst: target principal row const targetCreatedAt = new Date('2024-01-15T08:00:00.000Z') - vi.mocked(db.query.principal.findFirst).mockResolvedValueOnce({ + mockTargetRow({ id: 'principal_jane', displayName: 'Jane Doe', avatarUrl: null, avatarKey: 'avatars/jane.png', role: 'admin', createdAt: targetCreatedAt, - } as never) + userImage: null, + userImageKey: null, + }) const res = await handlePrincipalCard({ request: makeRequest(), @@ -90,10 +123,59 @@ describe('GET /api/v1/users/:principalId/card', () => { }) }) + it('falls back to user.imageKey when both principal avatar columns are null', async () => { + // Mirrors the production data anomaly: a principal whose own avatar + // columns drifted from the source-of-truth user record (e.g. created + // before syncPrincipalProfile was wired into every upload path). + vi.mocked(auth.api.getSession).mockResolvedValueOnce(identifiedSession) + vi.mocked(db.query.principal.findFirst).mockResolvedValueOnce(callerPrincipal) + mockTargetRow({ + id: 'principal_stale', + displayName: 'Stale Principal', + avatarUrl: null, + avatarKey: null, + role: 'user', + createdAt: new Date('2024-01-15T08:00:00.000Z'), + userImage: null, + userImageKey: 'avatars/2026/03/stale-key.png', + }) + + const res = await handlePrincipalCard({ + request: makeRequest(), + params: { principalId: 'principal_stale' }, + }) + + const body = await res.json() + expect(body.avatarUrl).toBe('https://cdn.example.com/avatars/2026/03/stale-key.png') + }) + + it('falls back to user.image when no avatar key is anywhere', async () => { + vi.mocked(auth.api.getSession).mockResolvedValueOnce(identifiedSession) + vi.mocked(db.query.principal.findFirst).mockResolvedValueOnce(callerPrincipal) + mockTargetRow({ + id: 'principal_oauth', + displayName: 'OAuth User', + avatarUrl: null, + avatarKey: null, + role: 'user', + createdAt: new Date('2024-01-15T08:00:00.000Z'), + userImage: 'https://lh3.googleusercontent.com/a/abc', + userImageKey: null, + }) + + const res = await handlePrincipalCard({ + request: makeRequest(), + params: { principalId: 'principal_oauth' }, + }) + + const body = await res.json() + expect(body.avatarUrl).toBe('https://lh3.googleusercontent.com/a/abc') + }) + it('returns 404 when the principal does not exist', async () => { vi.mocked(auth.api.getSession).mockResolvedValueOnce(identifiedSession) vi.mocked(db.query.principal.findFirst).mockResolvedValueOnce(callerPrincipal) - vi.mocked(db.query.principal.findFirst).mockResolvedValueOnce(undefined as never) + mockTargetRow(null) const res = await handlePrincipalCard({ request: makeRequest(), @@ -125,7 +207,8 @@ describe('GET /api/v1/users/:principalId/card', () => { }) expect(res.status).toBe(403) - // Only the caller-lookup query happened; the target lookup was never reached. + // Caller-lookup happened but the target select was never invoked. expect(db.query.principal.findFirst).toHaveBeenCalledTimes(1) + expect(selectChain.limit).not.toHaveBeenCalled() }) })