diff --git a/apps/api/src/auth/user.ts b/apps/api/src/auth/user.ts index b2e0c79..3a0339f 100644 --- a/apps/api/src/auth/user.ts +++ b/apps/api/src/auth/user.ts @@ -8,7 +8,7 @@ * Source: RESEARCH.md § "User upsert with color assignment" */ -import { and, eq, sql } from 'drizzle-orm' +import { and, eq } from 'drizzle-orm' import { db } from '../db/client.js' import { users } from '../db/schema.js' @@ -101,13 +101,18 @@ export async function upsertUser( return existing[0] } - // 2. Count existing users to determine round-robin color slot - const countResult = await db - .select({ count: sql`COUNT(*)` }) - .from(users) - - const count = Number(countResult[0]?.count ?? 0) - const color = COLOR_PALETTE[count % COLOR_PALETTE.length] + // 2. Assign the first palette color NOT already in use by another member. + // A plain COUNT(*) % palette collides under deletions: a deleted user + // shifts the count so the next insert reuses an in-use slot (observed in + // Gate 2 — two members both got #E8734A). Selecting the first unused color + // guarantees distinct, stable colors for up to COLOR_PALETTE.length members + // (AUTH-03). Falls back to round-robin by count only once the palette is + // exhausted (more members than colors). + const usedRows = await db.select({ color: users.color }).from(users) + const usedColors = new Set(usedRows.map((r) => r.color)) + const color = + COLOR_PALETTE.find((c) => !usedColors.has(c)) ?? + COLOR_PALETTE[usedColors.size % COLOR_PALETTE.length] // 3. Insert new user row // mysql2 has no RETURNING clause — use $returningId() then re-select diff --git a/apps/api/tests/auth/user.test.ts b/apps/api/tests/auth/user.test.ts index 0d9aae3..560f3ed 100644 --- a/apps/api/tests/auth/user.test.ts +++ b/apps/api/tests/auth/user.test.ts @@ -68,7 +68,7 @@ describe('upsertUser', () => { const sub = 'user-sub-001' // First select: no existing user - // Second select (count): count = 0 + // Second select (used colors): no existing users → no colors in use → palette[0] // Third select (re-fetch after insert): return the inserted row let selectCallCount = 0 mockDb.select.mockImplementation(() => { @@ -78,9 +78,9 @@ describe('upsertUser', () => { return makeSelectChain([]) } if (selectCallCount === 2) { - // COUNT(*) query — 0 users + // Used-colors query — no existing users return { - from: vi.fn().mockResolvedValue([{ count: 0 }]), + from: vi.fn().mockResolvedValue([]), } } // Re-fetch after insert @@ -109,9 +109,10 @@ describe('upsertUser', () => { return makeSelectChain([]) // not found } if (selectCallCount === 2) { - // COUNT(*) — 1 existing user + // Used-colors query — one existing user already holds palette[0], + // so the next member must get the first unused color: palette[1]. return { - from: vi.fn().mockResolvedValue([{ count: 1 }]), + from: vi.fn().mockResolvedValue([{ color: COLOR_PALETTE[0] }]), } } return makeSelectChain([ @@ -126,6 +127,39 @@ describe('upsertUser', () => { expect(user!.color).toBe(COLOR_PALETTE[1]) }) + // Regression (Gate 2): a new member must get a color NOT already in use, even + // after a deletion. The old COUNT(*) % palette logic reused an in-use slot + // when the user count had shifted (two members both got #E8734A). With colors + // [0] and [2] taken (slot [1] freed by a delete), the next member fills [1]. + it('assigns the first UNUSED palette color (no collision after deletions)', async () => { + const iss = 'https://auth.example.com' + const sub = 'user-sub-005' + + let selectCallCount = 0 + mockDb.select.mockImplementation(() => { + selectCallCount++ + if (selectCallCount === 1) return makeSelectChain([]) // not found + if (selectCallCount === 2) { + // palette[0] and palette[2] in use; palette[1] is free + return { + from: vi + .fn() + .mockResolvedValue([{ color: COLOR_PALETTE[0] }, { color: COLOR_PALETTE[2] }]), + } + } + return makeSelectChain([ + { id: 5, oidcIss: iss, oidcSub: sub, displayName: null, color: COLOR_PALETTE[1], createdAt: new Date() }, + ]) + }) + mockDb.insert.mockReturnValue(makeInsertChain([{ id: 5 }])) + + await upsertUser(iss, sub) + + // The inserted row's color must be the first unused palette entry (palette[1]). + const insertValues = mockDb.insert.mock.results[0]?.value?.values.mock.calls[0]?.[0] + expect(insertValues.color).toBe(COLOR_PALETTE[1]) + }) + it('returns the same user row on re-upsert (idempotent — no duplicate insert)', async () => { const iss = 'https://auth.example.com' const sub = 'user-sub-001'