From 2a536a416a2110b4cf5bdacfc25612d6e98c6a10 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nathan=20=F0=9F=94=B6=20Tarbert?= <66887028+NathanTarbert@users.noreply.github.com> Date: Fri, 7 Aug 2026 12:33:20 -0400 Subject: [PATCH 1/2] fix(sync): validate labelRules and fix two mappings-dashboard nits Closes the five non-blocking findings collected in #165. Three are the labelRules half of guards statusMappings and priorityMappings already had. labelRules is now validated on read. readPersistedConfig validated the other two sections per-section but passed labelRules through raw, so a malformed value was served as-is, omitted from invalidSections, and reached LabelRulesPanel which calls ruleList.map() on it. Absence is still valid -- the section is optional -- but a present value is checked like its siblings and falls back to defaults with the section named in the log. isValidLabelRulesShape now rejects an empty per-plugin array, matching isValidMappingShape. loadLabelMapper treats [] as "nothing persisted, use defaults", so saving { linear: [] } showed an empty rule list in the dashboard while the worker kept applying built-in rules -- the UI/engine divergence the sibling check exists to block. A PUT that omits labelRules no longer wipes persisted rules. The upsert replaces the whole row, so omitting the key dropped it -- the same silent wipe the {} rejection prevents, through a different door. Omitting the key now means "leave label rules alone" and carries forward what is stored; clearing requires an explicit valid value. Dashboard: PriorityMappingEntry gained a display-only "label" field, and the priority panel renders it beside the raw key. #95 moved the human text out of the persisted key (keys are now the adapter's real '0'-'4' values), which fixed the real bug but left the tab showing bare numbers -- the persistence half landed, the rendering half did not. And handleSave now clears provenance, so the "no saved mapping configuration is in effect" notice stops sitting on screen next to "Mappings saved successfully". Tests: three added, red-green verified -- with the source reverted exactly those three fail (labelRules read validation, omit-preservation, empty-array rejection) and all 13 pass with it. The two dashboard items are render-only. Verified: typecheck 10/10, tests 10/10, build 10/10. --- .../sync-mappings-config-read.test.ts | 31 ++++++++ .../__tests__/sync-mappings-roundtrip.test.ts | 75 +++++++++++++++++++ apps/web/src/app/api/sync/mappings/route.ts | 32 +++++++- apps/web/src/app/sync/mappings/page.tsx | 5 ++ .../src/components/sync/mapping-editor.tsx | 13 +++- apps/web/src/lib/mock-sync.ts | 8 ++ 6 files changed, 161 insertions(+), 3 deletions(-) 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..b265825b 100644 --- a/apps/web/src/__tests__/sync-mappings-config-read.test.ts +++ b/apps/web/src/__tests__/sync-mappings-config-read.test.ts @@ -106,4 +106,35 @@ describe('GET /api/sync/mappings — persisted config read path', () => { 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'); + expect(body.labelRules).not.toEqual({ linear: 'not-an-array' }); + // The good sections are untouched. + expect(body.statusMappings).toEqual(VALID_CONFIG.statusMappings); + expect(errorSpy.mock.calls.flat().join(' ')).toContain('labelRules'); + errorSpy.mockRestore(); + }); + + 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..c4b1eaa4 100644 --- a/apps/web/src/__tests__/sync-mappings-roundtrip.test.ts +++ b/apps/web/src/__tests__/sync-mappings-roundtrip.test.ts @@ -125,3 +125,78 @@ 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); + }); +}); diff --git a/apps/web/src/app/api/sync/mappings/route.ts b/apps/web/src/app/api/sync/mappings/route.ts index dc26f384..a20a4b1c 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 }; } @@ -301,6 +309,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,12 +384,27 @@ export async function PUT(request: NextRequest) { return NextResponse.json({ error: 'labelRules has invalid shape' }, { status: 400 }); } + // 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", so carry forward + // whatever is already stored. Clearing them requires an explicit, valid value. + let carriedLabelRules: PersistedMappingConfig['labelRules'] | undefined; + if (body.labelRules === undefined) { + const existing = await readPersistedConfig(); + if (existing.status === 'ok') { + carriedLabelRules = existing.config.labelRules; + } + } + const config: 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(config); diff --git a/apps/web/src/app/sync/mappings/page.tsx b/apps/web/src/app/sync/mappings/page.tsx index 14932e92..f7e7104f 100644 --- a/apps/web/src/app/sync/mappings/page.tsx +++ b/apps/web/src/app/sync/mappings/page.tsx @@ -48,6 +48,11 @@ export default function MappingsPage() { }); if (res.ok) { setConfig(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} + )}