diff --git a/apps/web/src/__tests__/api-route-auth.test.ts b/apps/web/src/__tests__/api-route-auth.test.ts index 476643fb..713dbe3d 100644 --- a/apps/web/src/__tests__/api-route-auth.test.ts +++ b/apps/web/src/__tests__/api-route-auth.test.ts @@ -14,9 +14,19 @@ const mockAccountFindMany = vi.fn(); const mockTicketGroupBy = vi.fn(); const mockExternalIdentityFindMany = vi.fn(); const mockSystemConfigFindUnique = vi.fn().mockResolvedValue(null); +const mockSystemConfigUpsert = vi.fn().mockResolvedValue({}); vi.mock('@copilotkit/outpost/db', () => ({ prisma: { + // The mappings PUT reads and writes in one transaction so a concurrent save + // cannot drop label rules; the callback receives the same mocked client. + $transaction: async (fn: (tx: unknown) => unknown) => + fn({ + systemConfig: { + findUnique: (...args: unknown[]) => mockSystemConfigFindUnique(...args), + upsert: (...args: unknown[]) => mockSystemConfigUpsert(...args), + }, + }), account: { create: (...args: unknown[]) => mockAccountCreate(...args), findMany: (...args: unknown[]) => mockAccountFindMany(...args), diff --git a/apps/web/src/__tests__/sync-api.test.ts b/apps/web/src/__tests__/sync-api.test.ts index 3dee1d19..81cb6571 100644 --- a/apps/web/src/__tests__/sync-api.test.ts +++ b/apps/web/src/__tests__/sync-api.test.ts @@ -15,6 +15,15 @@ const mockTicketExternalLinkFindFirst = vi.fn(); vi.mock('@copilotkit/outpost/db', () => ({ prisma: { + // The mappings PUT reads and writes in one transaction so a concurrent save + // cannot drop label rules; the callback receives the same mocked client. + $transaction: async (fn: (tx: unknown) => unknown) => + fn({ + systemConfig: { + findUnique: (...args: unknown[]) => mockSystemConfigFindUnique(...args), + upsert: (...args: unknown[]) => mockSystemConfigUpsert(...args), + }, + }), syncEvent: { findMany: (...args: unknown[]) => mockSyncEventFindMany(...args), findFirst: (...args: unknown[]) => mockSyncEventFindFirst(...args), diff --git a/apps/web/src/__tests__/sync-mappings-config-read.test.ts b/apps/web/src/__tests__/sync-mappings-config-read.test.ts index 6036580a..1f75ab74 100644 --- a/apps/web/src/__tests__/sync-mappings-config-read.test.ts +++ b/apps/web/src/__tests__/sync-mappings-config-read.test.ts @@ -7,9 +7,10 @@ * as "never configured", because that renders the code defaults as though they * were the admin's saved settings. */ -import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; const mockSystemConfigFindUnique = vi.fn(); +const mockSystemConfigUpsert = vi.fn(); const mockExternalIdentityFindMany = vi.fn(); vi.mock('@copilotkit/outpost/db', () => ({ @@ -20,6 +21,15 @@ vi.mock('@copilotkit/outpost/db', () => ({ externalIdentity: { findMany: (...args: unknown[]) => mockExternalIdentityFindMany(...args), }, + // The PUT path reads and writes in one transaction so a concurrent save cannot + // drop label rules. The callback receives the same mocked client. + $transaction: async (fn: (tx: unknown) => unknown) => + fn({ + systemConfig: { + findUnique: (...a: unknown[]) => mockSystemConfigFindUnique(...a), + upsert: (...a: unknown[]) => mockSystemConfigUpsert(...a), + }, + }), }, })); @@ -46,6 +56,12 @@ describe('GET /api/sync/mappings — persisted config read path', () => { mockExternalIdentityFindMany.mockResolvedValue([]); }); + // Restored here, not at the end of each test body: a failed assertion would skip an + // inline mockRestore() and leave console.error stubbed for the rest of the file. + afterEach(() => { + vi.restoreAllMocks(); + }); + it('reports defaults as defaults when no row exists', async () => { mockSystemConfigFindUnique.mockResolvedValue(null); @@ -75,7 +91,6 @@ describe('GET /api/sync/mappings — persisted config read path', () => { expect(body.configSource).toBe('defaults'); expect(body.configError).toContain('JSON'); expect(errorSpy.mock.calls.flat().join(' ')).toContain('sync.mappingConfig'); - errorSpy.mockRestore(); }); // A row written by an older version of the code, or hand-edited in the DB, @@ -104,6 +119,41 @@ describe('GET /api/sync/mappings — persisted config read path', () => { // ...while the good one is still the admin's saved config. expect(body.priorityMappings).toEqual(VALID_CONFIG.priorityMappings); expect(errorSpy.mock.calls.flat().join(' ')).toContain('statusMappings'); - errorSpy.mockRestore(); + }); + + it('validates labelRules on read instead of passing a malformed value through', async () => { + // labelRules used to be served raw: a malformed value reached LabelRulesPanel, + // which calls ruleList.map(...) on it, and it was absent from invalidSections + // so nothing reported the problem. + const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); + mockSystemConfigFindUnique.mockResolvedValue({ + value: JSON.stringify({ + ...VALID_CONFIG, + labelRules: { linear: 'not-an-array' }, + }), + }); + + const body = await (await GET()).json(); + + expect(body.invalidSections).toContain('labelRules'); + // Positive assertion against the defaults this endpoint serves when nothing is + // persisted. `not.toEqual` would also pass if labelRules were undefined, which is + // a different bug wearing the same green tick. + mockSystemConfigFindUnique.mockResolvedValue(null); + const defaults = await (await GET()).json(); + expect(body.labelRules).toEqual(defaults.labelRules); + expect(body.labelRules).toBeTruthy(); + // The good sections are untouched. + expect(body.statusMappings).toEqual(VALID_CONFIG.statusMappings); + expect(errorSpy.mock.calls.flat().join(' ')).toContain('labelRules'); + }); + + it('treats an absent labelRules as valid, since the section is optional', async () => { + mockSystemConfigFindUnique.mockResolvedValue({ value: JSON.stringify(VALID_CONFIG) }); + + const body = await (await GET()).json(); + + expect(body.invalidSections ?? []).not.toContain('labelRules'); + expect(body.configSource).not.toBe('defaults'); }); }); diff --git a/apps/web/src/__tests__/sync-mappings-roundtrip.test.ts b/apps/web/src/__tests__/sync-mappings-roundtrip.test.ts index 8515bd6b..1daa651d 100644 --- a/apps/web/src/__tests__/sync-mappings-roundtrip.test.ts +++ b/apps/web/src/__tests__/sync-mappings-roundtrip.test.ts @@ -11,7 +11,7 @@ * That is exactly what shipped: the Linear priority defaults read '0 (None)'.. * '4 (Low)' while LinearAdapter maps with String(data.priority) -> '0'..'4'. */ -import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; import { TicketPriority, TicketStatus } from '@copilotkit/outpost/shared'; import { loadStatusMap, @@ -25,6 +25,7 @@ import { const mockSystemConfigFindUnique = vi.fn(); const mockSystemConfigUpsert = vi.fn(); const mockExternalIdentityFindMany = vi.fn(); +const mockTransaction = vi.fn(); const mockGetServerSession = vi.fn(); vi.mock('@copilotkit/outpost/db', () => ({ @@ -34,6 +35,17 @@ vi.mock('@copilotkit/outpost/db', () => ({ upsert: (...a: unknown[]) => mockSystemConfigUpsert(...a), }, externalIdentity: { findMany: (...a: unknown[]) => mockExternalIdentityFindMany(...a) }, + // The PUT path reads and writes in one transaction so a concurrent save cannot + // drop label rules. The callback receives the same mocked client. + $transaction: async (fn: (tx: unknown) => unknown) => { + mockTransaction(fn); + return fn({ + systemConfig: { + findUnique: (...a: unknown[]) => mockSystemConfigFindUnique(...a), + upsert: (...a: unknown[]) => mockSystemConfigUpsert(...a), + }, + }); + }, }, })); @@ -125,3 +137,188 @@ describe('mappings round-trip: GET defaults -> PUT -> load as the worker does', expect(loaded.toOutpost(['wontfix'])).toEqual([]); }); }); + +describe('PUT /api/sync/mappings — labelRules preservation and empty-array rejection', () => { + const VALID_STATUS = { linear: [{ externalStatus: 'Done', outpostStatus: 'RESOLVED' }] }; + const VALID_PRIORITY = { linear: [{ externalPriority: '1', outpostPriority: 'CRITICAL' }] }; + const SAVED_LABEL_RULES = { github: [{ externalPrefix: 'bug', outpostPrefix: 'defect' }] }; + + beforeEach(() => { + vi.clearAllMocks(); + mockExternalIdentityFindMany.mockResolvedValue([]); + mockSystemConfigUpsert.mockResolvedValue({}); + }); + + function put(body: Record) { + return PUT( + new Request('http://localhost:3000/api/sync/mappings', { + method: 'PUT', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify(body), + }) as never, + ); + } + + function persistedValue() { + return JSON.parse(mockSystemConfigUpsert.mock.calls[0][0].update.value); + } + + it('carries forward existing labelRules when the PUT omits the key', async () => { + // The upsert replaces the whole row, so omitting labelRules used to drop + // previously persisted rules — a silent wipe reachable through a door the + // `labelRules: {}` rejection did not cover. + mockSystemConfigFindUnique.mockResolvedValue({ + value: JSON.stringify({ + statusMappings: VALID_STATUS, + priorityMappings: VALID_PRIORITY, + labelRules: SAVED_LABEL_RULES, + }), + }); + + const res = await put({ + statusMappings: VALID_STATUS, + priorityMappings: VALID_PRIORITY, + }); + + expect(res.status).toBe(200); + expect(persistedValue().labelRules).toEqual(SAVED_LABEL_RULES); + }); + + it('rejects an empty per-plugin labelRules array, as the sibling validator does', async () => { + // loadLabelMapper treats [] as "nothing persisted, use defaults", so saving + // it would show an empty list while the worker kept applying built-in rules. + mockSystemConfigFindUnique.mockResolvedValue(null); + + const res = await put({ + statusMappings: VALID_STATUS, + priorityMappings: VALID_PRIORITY, + labelRules: { linear: [] }, + }); + + expect(res.status).toBe(400); + expect(mockSystemConfigUpsert).not.toHaveBeenCalled(); + }); + + it('still writes an explicitly supplied labelRules value', async () => { + mockSystemConfigFindUnique.mockResolvedValue(null); + + const res = await put({ + statusMappings: VALID_STATUS, + priorityMappings: VALID_PRIORITY, + labelRules: SAVED_LABEL_RULES, + }); + + expect(res.status).toBe(200); + expect(persistedValue().labelRules).toEqual(SAVED_LABEL_RULES); + }); +}); + +describe('PUT /api/sync/mappings — atomicity, un-carriable rows, and label validation', () => { + const VALID_STATUS = { linear: [{ externalStatus: 'Done', outpostStatus: 'RESOLVED' }] }; + const VALID_PRIORITY = { linear: [{ externalPriority: '1', outpostPriority: 'CRITICAL' }] }; + const SAVED_LABEL_RULES = { github: [{ externalPrefix: 'bug', outpostPrefix: 'defect' }] }; + + beforeEach(() => { + vi.clearAllMocks(); + mockExternalIdentityFindMany.mockResolvedValue([]); + mockSystemConfigUpsert.mockResolvedValue({}); + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + + function put(body: Record) { + return PUT( + new Request('http://localhost:3000/api/sync/mappings', { + method: 'PUT', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify(body), + }) as never, + ); + } + + it('carries labelRules forward inside a transaction, not as two statements', async () => { + // Read-then-write as separate statements lets a concurrent PUT land between them + // and lose its rules. Pin that both happen through $transaction. + mockSystemConfigFindUnique.mockResolvedValue({ + value: JSON.stringify({ + statusMappings: VALID_STATUS, + priorityMappings: VALID_PRIORITY, + labelRules: SAVED_LABEL_RULES, + }), + }); + + const res = await put({ statusMappings: VALID_STATUS, priorityMappings: VALID_PRIORITY }); + + expect(res.status).toBe(200); + expect(mockTransaction).toHaveBeenCalledTimes(1); + const persisted = JSON.parse(mockSystemConfigUpsert.mock.calls[0][0].update.value); + expect(persisted.labelRules).toEqual(SAVED_LABEL_RULES); + }); + + it('says so when existing labelRules are unusable and cannot be carried', async () => { + const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); + mockSystemConfigFindUnique.mockResolvedValue({ + value: JSON.stringify({ + statusMappings: VALID_STATUS, + priorityMappings: VALID_PRIORITY, + labelRules: { linear: 'not-an-array' }, + }), + }); + + const res = await put({ statusMappings: VALID_STATUS, priorityMappings: VALID_PRIORITY }); + + expect(res.status).toBe(200); + const persisted = JSON.parse(mockSystemConfigUpsert.mock.calls[0][0].update.value); + // Unusable rules are dropped rather than persisted... + expect(persisted.labelRules).toBeUndefined(); + // ...but the drop is reported, so it is not silent. + expect(errorSpy.mock.calls.flat().join(' ')).toContain('labelRules'); + }); + + it('returns the persisted config so the client can store what was actually saved', async () => { + mockSystemConfigFindUnique.mockResolvedValue({ + value: JSON.stringify({ + statusMappings: VALID_STATUS, + priorityMappings: VALID_PRIORITY, + labelRules: SAVED_LABEL_RULES, + }), + }); + + const body = await ( + await put({ statusMappings: VALID_STATUS, priorityMappings: VALID_PRIORITY }) + ).json(); + + // The request omitted labelRules; the response must still show them, otherwise the + // dashboard drops rules that are saved. + expect(body.labelRules).toEqual(SAVED_LABEL_RULES); + }); + + it('rejects a non-string label on a priority entry', async () => { + mockSystemConfigFindUnique.mockResolvedValue(null); + + const res = await put({ + statusMappings: VALID_STATUS, + priorityMappings: { + linear: [{ externalPriority: '1', outpostPriority: 'CRITICAL', label: 42 }], + }, + }); + + expect(res.status).toBe(400); + expect(mockSystemConfigUpsert).not.toHaveBeenCalled(); + }); + + it('accepts a string label', async () => { + mockSystemConfigFindUnique.mockResolvedValue(null); + + const res = await put({ + statusMappings: VALID_STATUS, + priorityMappings: { + linear: [{ externalPriority: '1', outpostPriority: 'CRITICAL', label: 'Urgent' }], + }, + }); + + expect(res.status).toBe(200); + }); +}); diff --git a/apps/web/src/app/api/sync/mappings/route.ts b/apps/web/src/app/api/sync/mappings/route.ts index dc26f384..48e91499 100644 --- a/apps/web/src/app/api/sync/mappings/route.ts +++ b/apps/web/src/app/api/sync/mappings/route.ts @@ -157,6 +157,13 @@ async function readPersistedConfig(): Promise { ) { bad.push('priorityMappings'); } + // labelRules is optional, so absence is valid — but a PRESENT value must be + // validated like the other two. Passing it through raw let a malformed row + // reach LabelRulesPanel, which does `ruleList.map(...)` on it, and it was + // omitted from invalidSections so nothing reported the problem. + if (candidate?.labelRules !== undefined && !isValidLabelRulesShape(candidate.labelRules)) { + bad.push('labelRules'); + } if (bad.length > 0) { console.error( @@ -168,6 +175,7 @@ async function readPersistedConfig(): Promise { const usable: PersistedMappingConfig = { ...candidate }; if (bad.includes('statusMappings')) delete usable.statusMappings; if (bad.includes('priorityMappings')) delete usable.priorityMappings; + if (bad.includes('labelRules')) delete usable.labelRules; return { status: 'ok', config: usable, invalidSections: bad }; } @@ -270,6 +278,10 @@ function isValidMappingShape( const external = record[externalKey]; const outpost = record[outpostKey]; + // `label` is display-only and optional, but a non-string reaches the editor + // and renders as garbage, so reject it rather than pass it through. + if (record.label !== undefined && typeof record.label !== 'string') return false; + return ( typeof external === 'string' && // Non-blank: the loaders test truthiness, so '' would pass here @@ -301,6 +313,13 @@ function isValidLabelRulesShape(value: unknown): boolean { return Object.values(value as Record).every((entries) => { if (!Array.isArray(entries)) return false; + // Same reasoning as isValidMappingShape: `loadLabelMapper` treats an empty + // array as "nothing persisted, use the defaults", so saving `{ linear: [] }` + // would show an empty rule list in the dashboard while the worker kept + // applying the built-in rules. A save must not be able to produce a state + // where the UI and the engine disagree about what is in effect. + if (entries.length === 0) return false; + return entries.every((entry) => { if (typeof entry !== 'object' || entry === null) return false; const record = entry as Record; @@ -369,19 +388,58 @@ export async function PUT(request: NextRequest) { return NextResponse.json({ error: 'labelRules has invalid shape' }, { status: 400 }); } - const config: PersistedMappingConfig = { - statusMappings: body.statusMappings as PersistedMappingConfig['statusMappings'], - priorityMappings: body.priorityMappings as PersistedMappingConfig['priorityMappings'], - ...(body.labelRules - ? { labelRules: body.labelRules as PersistedMappingConfig['labelRules'] } - : {}), - }; - const value = JSON.stringify(config); - - await prisma.systemConfig.upsert({ - where: { key: MAPPING_CONFIG_KEY }, - update: { value }, - create: { key: MAPPING_CONFIG_KEY, value }, + // The upsert replaces the whole row, so omitting labelRules would drop any previously + // persisted rules — the same silent wipe the `labelRules: {}` rejection above exists to + // prevent, reachable through a different door. A PUT without the key means "leave label + // rules alone"; clearing them requires an explicit, valid value. + // + // Read and write in one transaction: as two statements, a concurrent PUT could land + // between them and its rules would be lost by whichever write came second. + const config = await prisma.$transaction(async (tx) => { + let carriedLabelRules: PersistedMappingConfig['labelRules'] | undefined; + + if (body.labelRules === undefined) { + const row = await tx.systemConfig.findUnique({ where: { key: MAPPING_CONFIG_KEY } }); + if (row) { + try { + const existing = JSON.parse(row.value) as PersistedMappingConfig; + if (isValidLabelRulesShape(existing?.labelRules)) { + carriedLabelRules = existing.labelRules; + } else if (existing?.labelRules !== undefined) { + // Present but unusable: it is not carried forward, and saying so + // matters — otherwise the rules disappear with no signal anywhere. + console.error( + `[sync/mappings] existing labelRules in "${MAPPING_CONFIG_KEY}" are ` + + `unusable and will NOT be carried forward by this save.`, + ); + } + } catch { + console.error( + `[sync/mappings] existing "${MAPPING_CONFIG_KEY}" row is not valid JSON; ` + + `label rules cannot be carried forward by this save.`, + ); + } + } + } + + const next: PersistedMappingConfig = { + statusMappings: body.statusMappings as PersistedMappingConfig['statusMappings'], + priorityMappings: body.priorityMappings as PersistedMappingConfig['priorityMappings'], + ...(body.labelRules + ? { labelRules: body.labelRules as PersistedMappingConfig['labelRules'] } + : carriedLabelRules + ? { labelRules: carriedLabelRules } + : {}), + }; + const value = JSON.stringify(next); + + await tx.systemConfig.upsert({ + where: { key: MAPPING_CONFIG_KEY }, + update: { value }, + create: { key: MAPPING_CONFIG_KEY, value }, + }); + + return next; }); return NextResponse.json(config); diff --git a/apps/web/src/app/sync/mappings/page.tsx b/apps/web/src/app/sync/mappings/page.tsx index 14932e92..6b59c74a 100644 --- a/apps/web/src/app/sync/mappings/page.tsx +++ b/apps/web/src/app/sync/mappings/page.tsx @@ -47,7 +47,17 @@ export default function MappingsPage() { body: JSON.stringify(updatedConfig), }); if (res.ok) { - setConfig(updatedConfig); + // Store what the server persisted, not what was sent. A PUT that omits + // labelRules has them carried forward server-side, so echoing the request + // body back into state would drop rules that are actually saved — they + // would vanish from the editor until the next page load. + const saved = (await res.json().catch(() => null)) as MappingConfig | null; + setConfig(saved ?? updatedConfig); + // A successful PUT means a saved configuration is now in effect, so the + // "no saved mapping configuration" / "using built-in defaults for …" + // notices no longer describe reality. Without this they sit on screen + // next to "Mappings saved successfully", contradicting it. + setProvenance({}); setSaveMessage('Mappings saved successfully.'); } else { setSaveMessage('Failed to save mappings.'); diff --git a/apps/web/src/components/sync/mapping-editor.tsx b/apps/web/src/components/sync/mapping-editor.tsx index 1dcefb58..32ee85f5 100644 --- a/apps/web/src/components/sync/mapping-editor.tsx +++ b/apps/web/src/components/sync/mapping-editor.tsx @@ -185,8 +185,17 @@ function PriorityMappingPanel({ data-testid={`priority-row-${plugin}-${i}`} className="flex items-center gap-3" > - - {entry.externalPriority} + + {entry.label ? ( + <> + {entry.label}{' '} + + ({entry.externalPriority}) + + + ) : ( + {entry.externalPriority} + )}