From e94e845a9b60fa2f40aaa63eb0c5ab4cefce063e Mon Sep 17 00:00:00 2001 From: James Morton Date: Sat, 16 May 2026 15:04:21 +0100 Subject: [PATCH] fix(mentions): fall back to user.image when principal avatar columns are empty The hover card and picker rendered initials for principals whose own avatar columns were null even when the linked user record had an image. Both endpoints now LEFT JOIN user and resolve in the order: principal.avatarKey -> principal.avatarUrl -> user.imageKey -> user.image. Read-side safety net for principal rows that drifted from the source-of- truth user record (e.g. created before syncPrincipalProfile was wired into every upload path). --- .../api/v1/mentions/__tests__/suggest.test.ts | 76 +++++++++++++-- .../web/src/routes/api/v1/mentions/suggest.ts | 33 +++++-- .../routes/api/v1/users/$principalId.card.ts | 57 +++++++---- .../v1/users/__tests__/principal-card.test.ts | 95 +++++++++++++++++-- 4 files changed, 223 insertions(+), 38 deletions(-) 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() }) })