From ca93ef3fb639230d526f0458aff6aee1d976ab3d Mon Sep 17 00:00:00 2001 From: Jakob Wennberg Date: Fri, 28 Aug 2026 16:57:54 +0200 Subject: [PATCH] fix(salary): surface employee-save failures and a typed 503 for the missing encryption key (#1996) (#2009) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(salary): surface employee-save failures in the dialog and type the missing encryption key (#1996) Pressing Spara in "Ny anställd" could fail without any feedback: a thrown fetch or a non-JSON 5xx body escaped handleSubmit before setSaving(false) ran, leaving the button stuck on "Sparar..." and the dialog silent. Even when the toast did fire, the Radix modal aria-hides the root-layout Toaster, so assistive tech (and the E2E driver that found this) heard nothing, and the requestId support needs was never shown anywhere. - NewEmployeeDialog: fetch + parse run in a never-throwing helper, saving is released in finally, the body is parsed with json().catch(() => null) so an HTML/plain-text error page still maps through the HTTP-status map, and the failure is rendered inline (role="alert" in the footer) with "Ärende-id: " next to the single destructive toast. - personnummer.ts: the production "key missing" throw now carries the registry code PERSONNUMMER_ENCRYPTION_NOT_CONFIGURED, and the SALARY registry gains a 503 entry naming PERSONNUMMER_ENCRYPTION_KEY with a "contact support" message and a remediation hint. withRouteContext emits the typed envelope automatically instead of INTERNAL_ERROR 500, which read as transient and invited retries that can never succeed. - Tests for the route (401, 400, 503 with requestId and no insert), the key guard, the registry entry, errorResponse dispatch on a coded Error, and getErrorMessage locale handling of the new envelope. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_015nAd8XJ2RPCmG2eKoLBdna * fix(salary): address review findings (#1996) - NewEmployeeDialog: fall back to the X-Request-Id response header when the body carries no error.requestId. The route hand-builds its 409 (duplicate personnummer) and generic insert-failure 500 bodies as flat strings, so the inline "Ärende-id" line was hidden for exactly the DB-failure class the issue names; withRouteContext sets the header on every response. - Route tests pin that the 409 and 500 insert-error arms carry X-Request-Id. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_015nAd8XJ2RPCmG2eKoLBdna --------- Co-authored-by: Jakob Wennberg <311770904+jakobwennberg-oss@users.noreply.github.com> Co-authored-by: Claude Fable 5 --- DECISIONS.md | 1 + .../salary/employees/__tests__/route.test.ts | 91 ++++++++++++- components/salary/NewEmployeeDialog.tsx | 122 +++++++++++++++--- .../__tests__/get-error-message.test.ts | 22 ++++ .../__tests__/structured-errors.test.ts | 29 +++++ lib/errors/structured-errors.ts | 18 +++ lib/salary/__tests__/personnummer.test.ts | 44 ++++++- lib/salary/personnummer.ts | 14 +- messages/en.json | 3 +- messages/sv.json | 3 +- 10 files changed, 322 insertions(+), 25 deletions(-) diff --git a/DECISIONS.md b/DECISIONS.md index 676dfb45..c930ee88 100644 --- a/DECISIONS.md +++ b/DECISIONS.md @@ -1323,5 +1323,6 @@ One line per decision: `[YYYY-MM-DD] : `. Appended by agents and [2026-08-28] AR-PDF minus fix uses ASCII hyphen formatting, not font embedding: registering a Unicode TTF for react-pdf would change the whole document's typography and bundle size to fix one glyph; formatPdfKronor keeps built-in Helvetica and sidesteps WinAnsi's missing U+2212. [2026-08-28] Same-bank warning limited to observed one-session banks (SEB only): prod shows Handelsbanken tolerates 4 concurrent sessions, and the generic warning made a user abandon a legitimate renewal. Planned sync-death visibility work was dropped: already shipped via #1271 (health probe), #1727 (stale state), #1969 (cron unstarve). [2026-08-28] Same-bank warning revised to three tiers after skeptic refutation: hard warn SEB, silent/calm only for verified multi-session banks (Handelsbanken, 4 distinct session_ids observed), legacy hedged warning for unknown banks (fail closed), shared-session siblings exempt (fan-out carries them). +[2026-08-28] Employee-save failure reported inline (role=alert in the dialog footer, carrying the requestId) in addition to the single destructive toast, and a missing PERSONNUMMER_ENCRYPTION_KEY typed as 503 PERSONNUMMER_ENCRYPTION_NOT_CONFIGURED rather than INTERNAL_ERROR (#1996): the Radix modal aria-hides the root-layout Toaster while the dialog is open, so the toast is invisible to assistive tech and E2E drivers; TOAST_LIMIT is 1, so a second toast is not an option; and the missing key is a permanent configuration gap where "try again later" is wrong and "contact support" is right (same reasoning as CUSTOMER_PERSONAL_NUMBER_UNREADABLE and INVOICE_SEND_EMAIL_NOT_CONFIGURED). The shared postAction helper was not extended (it takes no body and exposes no requestId): keeping the change local to the dialog avoids widening a helper other panels rely on. [2026-08-28] /migrate SIE guard extended to every provider (Fortnox exemption removed) as "a completed SIE import must exist for the company", not "must be part of this run", plus a wizard hint that disables Start when SIE is unchecked and never imported; chose this over forcing the checkbox on because the route is the only seam a direct API call or a stale client cannot bypass, and "must exist" keeps entities-only re-runs after a full migration working (#2000). [2026-08-28] /migrate SIE guard skips company-info-only runs (all entity flags false) and the wizard derives "SIE already imported" from the preview OR this session's successful /import-sie results: company info writes no accounts, balances or subledger rows, so the BFL rationale does not apply; and the one-shot preview went stale after phase 1 succeeded and phase 2 failed, falsely blocking an entities-only retry (#2000 review). diff --git a/app/api/salary/employees/__tests__/route.test.ts b/app/api/salary/employees/__tests__/route.test.ts index 39a97f42..b26f5187 100644 --- a/app/api/salary/employees/__tests__/route.test.ts +++ b/app/api/salary/employees/__tests__/route.test.ts @@ -12,7 +12,7 @@ * The route now runs through the withRouteContext wrapper, so we mock its * auth/company/write dependencies and inject the Supabase client via requireAuth. */ -import { beforeEach, describe, expect, it, vi } from 'vitest' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { NextResponse } from 'next/server' vi.mock('@/lib/init', () => ({ ensureInitialized: vi.fn() })) @@ -232,4 +232,93 @@ describe('POST /api/salary/employees', () => { jamkning_valid_to: null, }) }) + + it('returns 401 when unauthenticated', async () => { + vi.mocked(requireAuth).mockResolvedValue({ + user: null, + supabase: null, + error: NextResponse.json({ error: 'Unauthorized' }, { status: 401 }), + } as never) + + const res = await POST(postRequest(CREATE_BASE), params) + expect(res.status).toBe(401) + }) + + it('returns 400 on a personnummer that fails the checksum, without inserting', async () => { + const { supabase, insert } = supabaseWithInsert({ id: 'emp-new' }) + authed(supabase) + + const res = await POST(postRequest({ ...CREATE_BASE, personnummer: '199001019803' }), params) + + expect(res.status).toBe(400) + expect(insert).not.toHaveBeenCalled() + }) + + // #1996: the route hand-builds the 409 and generic insert-failure 500 bodies + // as flat strings (no error.requestId), so the dialog falls back to the + // X-Request-Id header for its "Ärende-id" line. Pin that the header is there. + describe('insert failures carry X-Request-Id for support correlation', () => { + function supabaseWithInsertError(error: { code: string; message: string }) { + const single = vi.fn(() => Promise.resolve({ data: null, error })) + const select = vi.fn(() => ({ single })) + const insert = vi.fn(() => ({ select })) + return { from: vi.fn(() => ({ insert })) } + } + + it('409 duplicate personnummer: flat error body plus X-Request-Id header', async () => { + authed(supabaseWithInsertError({ code: '23505', message: 'duplicate key value' })) + + const res = await POST(postRequest(CREATE_BASE), params) + + expect(res.status).toBe(409) + const body = await res.json() + expect(body.error).toBe('En anställd med detta personnummer finns redan') + expect(res.headers.get('X-Request-Id')).toMatch(/^req_/) + }) + + it('500 generic insert error: flat error body plus X-Request-Id header', async () => { + authed(supabaseWithInsertError({ code: '57014', message: 'canceling statement due to statement timeout' })) + + const res = await POST(postRequest(CREATE_BASE), params) + + expect(res.status).toBe(500) + const body = await res.json() + expect(typeof body.error).toBe('string') + expect(res.headers.get('X-Request-Id')).toMatch(/^req_/) + }) + }) + + // #1996: the deployment that filed the issue had no PERSONNUMMER_ENCRYPTION_KEY. + // The encrypt helper used to throw a bare Error, which the wrapper answered + // with the generic INTERNAL_ERROR 500 ("try again later") even though no + // retry can ever succeed. It now carries a registry code, so the envelope + // names the configuration gap, points at support, and keeps the requestId. + describe('missing PERSONNUMMER_ENCRYPTION_KEY in production', () => { + afterEach(() => { + vi.unstubAllEnvs() + }) + + it('answers 503 PERSONNUMMER_ENCRYPTION_NOT_CONFIGURED with a requestId and no insert', async () => { + vi.stubEnv('NODE_ENV', 'production') + vi.stubEnv('PERSONNUMMER_ENCRYPTION_KEY', '') + const { supabase, insert } = supabaseWithInsert({ id: 'emp-new' }) + authed(supabase) + + const res = await POST(postRequest(CREATE_BASE), params) + + expect(res.status).toBe(503) + const body = await res.json() + expect(body.error.code).toBe('PERSONNUMMER_ENCRYPTION_NOT_CONFIGURED') + expect(body.error.message).toContain('PERSONNUMMER_ENCRYPTION_KEY') + expect(body.error.message).toMatch(/Kontakta supporten/) + expect(body.error.message_en).toContain('PERSONNUMMER_ENCRYPTION_KEY') + expect(body.error.requestId).toMatch(/^req_/) + expect(res.headers.get('X-Request-Id')).toBe(body.error.requestId) + // The failure happens before the database is touched: nothing to clean up. + expect(supabase.from).not.toHaveBeenCalled() + expect(insert).not.toHaveBeenCalled() + // The personnummer itself must never leak into the error envelope. + expect(JSON.stringify(body)).not.toContain(NEW_PNR) + }) + }) }) diff --git a/components/salary/NewEmployeeDialog.tsx b/components/salary/NewEmployeeDialog.tsx index 945ec25a..3807282d 100644 --- a/components/salary/NewEmployeeDialog.tsx +++ b/components/salary/NewEmployeeDialog.tsx @@ -1,6 +1,7 @@ 'use client' import { useState } from 'react' +import { useLocale, useTranslations } from 'next-intl' import { useCompanySettings } from '@/lib/reference-data/hooks' import { Dialog, @@ -15,7 +16,7 @@ import { Select, SelectContent, SelectItem, SelectTrigger, SelectValue } from '@ import { Save } from 'lucide-react' import { cn } from '@/lib/utils' import { useToast } from '@/components/ui/use-toast' -import { getErrorMessage } from '@/lib/errors/get-error-message' +import { getErrorMessage, type ErrorLocale } from '@/lib/errors/get-error-message' import { validateEmployeeBankAccount, isValidClearing, @@ -100,11 +101,72 @@ export default function NewEmployeeDialog({ open, onOpenChange, onCreated }: Pro ) } +/** + * Why a failed save is reported twice (toast AND an inline line): the dialog + * is a Radix modal, which aria-hides everything outside DialogContent while + * it is open, including the root-layout Toaster. The toast is visible on + * screen but absent from the accessibility tree, so a screen reader (and the + * E2E driver that filed #1996) hears nothing. The inline role="alert" line + * lives inside the dialog and persists until the next attempt, and it is the + * one place the support reference (requestId) is shown. + */ +interface SubmitError { + message: string + requestId: string | null +} + +function readRequestId(body: unknown): string | null { + if (typeof body !== 'object' || body === null) return null + const inner = (body as { error?: unknown }).error + if (typeof inner !== 'object' || inner === null) return null + const id = (inner as { requestId?: unknown }).requestId + return typeof id === 'string' && id.trim() ? id : null +} + +/** + * POST the employee and describe the failure, if any, in one sentence. + * Returns null on success. Never throws: every arm (non-2xx envelope, a + * non-JSON body such as Vercel's plain-text FUNCTION_INVOCATION_FAILED or a + * 413 HTML page, a fetch that never completed) ends in a SubmitError so the + * caller has exactly one thing to report. + */ +async function submitEmployee(body: unknown, locale: ErrorLocale): Promise { + try { + const res = await fetch('/api/salary/employees', { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify(body), + }) + if (res.ok) return null + + // A body that is not JSON leaves null: getErrorMessage then falls back to + // the HTTP status map, still in the user's language. + const result: unknown = await res.json().catch(() => null) + // The route hand-builds its 409 (duplicate personnummer) and generic + // insert-failure 500 bodies as flat strings, so only the typed envelopes + // carry error.requestId. withRouteContext sets X-Request-Id on every + // response, so fall back to the header: those DB-failure arms are exactly + // the ones support needs a correlation id for. + return { + message: getErrorMessage(result, { context: 'salary', statusCode: res.status, locale }), + requestId: readRequestId(result) ?? res.headers.get('X-Request-Id'), + } + } catch (err) { + return { + message: getErrorMessage(err, { context: 'salary', locale }), + requestId: null, + } + } +} + // Inner component so form state resets whenever the dialog reopens (Radix // unmounts DialogContent children on close). function NewEmployeeForm({ onCreated, onCancel }: { onCreated: () => void; onCancel: () => void }) { + const t = useTranslations('employees') + const locale = useLocale() as ErrorLocale const { toast } = useToast() const [saving, setSaving] = useState(false) + const [submitError, setSubmitError] = useState(null) const [employmentType, setEmploymentType] = useState('employee') const [salaryType, setSalaryType] = useState('monthly') const [personnummer, setPersonnummer] = useState('') @@ -157,6 +219,7 @@ function NewEmployeeForm({ onCreated, onCancel }: { onCreated: () => void; onCan } setSaving(true) + setSubmitError(null) const form = new FormData(e.currentTarget) const body = { @@ -193,25 +256,31 @@ function NewEmployeeForm({ onCreated, onCancel }: { onCreated: () => void; onCan default_dimensions: dimensions, } - const res = await fetch('/api/salary/employees', { - method: 'POST', - headers: { 'Content-Type': 'application/json' }, - body: JSON.stringify(body), - }) - - if (res.ok) { - toast({ title: 'Anställd skapad' }) - onCreated() - } else { - const result = await res.json() - toast({ - title: 'Kunde inte skapa anställd', - description: getErrorMessage(result, { context: 'salary', statusCode: res.status }), - variant: 'destructive', - }) + // `saving` is released in finally no matter how the request ends: a + // thrown fetch (offline, connection reset) used to escape this handler + // before setSaving(false) ran, leaving the dialog silent with Spara stuck + // on "Sparar..." forever. #1996 + let failure: SubmitError | null + try { + failure = await submitEmployee(body, locale) + } finally { + setSaving(false) } - setSaving(false) + if (!failure) { + toast({ title: 'Anställd skapad' }) + onCreated() + return + } + + // One toast per attempt (TOAST_LIMIT is 1, a second would evict it) plus + // the inline line that the modal's aria-hiding cannot swallow. + setSubmitError(failure) + toast({ + title: 'Kunde inte skapa anställd', + description: failure.message, + variant: 'destructive', + }) } const bankName = lookupBankByClearing(clearing) @@ -426,8 +495,21 @@ function NewEmployeeForm({ onCreated, onCancel }: { onCreated: () => void; onCan {/* Solid footer outside the scroll area: always visible, never overlaps content (the body above scrolls independently). */} -
-