From ba73387d509ad9faea2633cda8363c0ed04d9205 Mon Sep 17 00:00:00 2001 From: Marcel Date: Wed, 29 Apr 2026 01:20:49 +0200 Subject: [PATCH] refactor(transcription): extract saveBlockWithConflictRetry into a util MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Tester #5506 §2 + Markus #5504 §2: the 409 orchestration was inline in +page.svelte and untested. Extract into a pure module that takes the fetch function as a dependency, so the full happy path / 409 path / 500 path / refetch-fails path / UUID-guard path can be unit-tested with mock Responses. The route file now reads as 12 lines: call the helper, on conflict apply the merged snapshot to local state, re-throw. BlockConflictResolvedError now carries the merged block on its `merged` property so callers don't have to redo the refetch. 6 new unit tests cover every branch. Co-Authored-By: Claude Sonnet 4.6 --- frontend/src/lib/utils/blockConflictMerge.ts | 11 +- .../utils/saveBlockWithConflictRetry.spec.ts | 152 ++++++++++++++++++ .../lib/utils/saveBlockWithConflictRetry.ts | 60 +++++++ .../src/routes/documents/[id]/+page.svelte | 46 ++---- 4 files changed, 234 insertions(+), 35 deletions(-) create mode 100644 frontend/src/lib/utils/saveBlockWithConflictRetry.spec.ts create mode 100644 frontend/src/lib/utils/saveBlockWithConflictRetry.ts diff --git a/frontend/src/lib/utils/blockConflictMerge.ts b/frontend/src/lib/utils/blockConflictMerge.ts index c3a00039..7077d6c9 100644 --- a/frontend/src/lib/utils/blockConflictMerge.ts +++ b/frontend/src/lib/utils/blockConflictMerge.ts @@ -1,13 +1,16 @@ import type { PersonMention, TranscriptionBlockData } from '$lib/types'; /** - * Sentinel thrown by saveBlock after a 409 rename-mid-edit has been merged - * into local state. Surfaces to the autosave hook as an error (so the UI - * shows the retry indicator), but distinguishable from a genuine network - * failure via the code. + * Sentinel thrown by saveBlockWithConflictRetry after a 409 rename-mid-edit + * has been merged into local state. Surfaces to the autosave hook as an + * error (so the UI shows the retry indicator), but distinguishable from a + * genuine network failure via the code. Carries the merged block snapshot + * on its `merged` property so the caller can update local state without + * a second roundtrip. */ export class BlockConflictResolvedError extends Error { readonly code = 'CONFLICT_RESOLVED' as const; + merged?: TranscriptionBlockData; constructor(blockId: string) { super( `Block ${blockId} was rebased onto the latest server snapshot — retry to save the merged result` diff --git a/frontend/src/lib/utils/saveBlockWithConflictRetry.spec.ts b/frontend/src/lib/utils/saveBlockWithConflictRetry.spec.ts new file mode 100644 index 00000000..cc808c7d --- /dev/null +++ b/frontend/src/lib/utils/saveBlockWithConflictRetry.spec.ts @@ -0,0 +1,152 @@ +import { describe, it, expect, vi } from 'vitest'; +import { saveBlockWithConflictRetry } from './saveBlockWithConflictRetry'; +import { BlockConflictResolvedError } from './blockConflictMerge'; +import type { PersonMention } from '$lib/types'; + +const DOC = 'aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa'; +const BLK = 'bbbbbbbb-bbbb-bbbb-bbbb-bbbbbbbbbbbb'; + +const SERVER_BLOCK_AFTER_RENAME = { + id: BLK, + annotationId: 'cccccccc-cccc-cccc-cccc-cccccccccccc', + documentId: DOC, + text: 'old text from server', + label: null, + sortOrder: 0, + version: 7, + source: 'MANUAL' as const, + reviewed: false, + mentionedPersons: [{ personId: 'p-aug', displayName: 'Augusta Raddatz' }] +}; + +function mkResponse(status: number, body?: unknown): Response { + return new Response(body === undefined ? null : JSON.stringify(body), { + status, + headers: { 'Content-Type': 'application/json' } + }); +} + +describe('saveBlockWithConflictRetry', () => { + it('returns the server-saved block on a successful PUT', async () => { + const updated = { ...SERVER_BLOCK_AFTER_RENAME, text: 'persisted text' }; + const fetchImpl = vi.fn().mockResolvedValueOnce(mkResponse(200, updated)); + + const result = await saveBlockWithConflictRetry({ + fetchImpl: fetchImpl as unknown as typeof fetch, + documentId: DOC, + blockId: BLK, + text: 'persisted text', + mentionedPersons: [] + }); + + expect(result).toEqual(updated); + expect(fetchImpl).toHaveBeenCalledTimes(1); + expect(fetchImpl).toHaveBeenCalledWith( + `/api/documents/${DOC}/transcription-blocks/${BLK}`, + expect.objectContaining({ + method: 'PUT', + body: JSON.stringify({ text: 'persisted text', mentionedPersons: [] }) + }) + ); + }); + + it('throws BlockConflictResolvedError carrying the merged block on 409', async () => { + const fetchImpl = vi + .fn() + .mockResolvedValueOnce(mkResponse(409)) + .mockResolvedValueOnce(mkResponse(200, SERVER_BLOCK_AFTER_RENAME)); + + const localMentions: PersonMention[] = [{ personId: 'p-aug', displayName: 'Auguste Raddatz' }]; + + await expect( + saveBlockWithConflictRetry({ + fetchImpl: fetchImpl as unknown as typeof fetch, + documentId: DOC, + blockId: BLK, + text: 'transcriber unsaved input', + mentionedPersons: localMentions + }) + ).rejects.toThrow(BlockConflictResolvedError); + + expect(fetchImpl).toHaveBeenCalledTimes(2); + // First call PUT, second is the GET refetch. + expect(fetchImpl.mock.calls[0]?.[1]?.method).toBe('PUT'); + expect(fetchImpl.mock.calls[1]?.[1]).toBeUndefined(); + }); + + it('attaches the merged block to err.merged so callers can update local state', async () => { + const fetchImpl = vi + .fn() + .mockResolvedValueOnce(mkResponse(409)) + .mockResolvedValueOnce(mkResponse(200, SERVER_BLOCK_AFTER_RENAME)); + + const localMentions: PersonMention[] = [ + { personId: 'p-aug', displayName: 'Auguste Raddatz' } // stale displayName + ]; + try { + await saveBlockWithConflictRetry({ + fetchImpl: fetchImpl as unknown as typeof fetch, + documentId: DOC, + blockId: BLK, + text: 'transcriber unsaved input', + mentionedPersons: localMentions + }); + throw new Error('expected throw'); + } catch (err) { + expect(err).toBeInstanceOf(BlockConflictResolvedError); + const merged = (err as BlockConflictResolvedError).merged!; + // Local text wins. + expect(merged.text).toBe('transcriber unsaved input'); + // Server displayName wins for shared personId. + expect(merged.mentionedPersons).toEqual([ + { personId: 'p-aug', displayName: 'Augusta Raddatz' } + ]); + // Server version carried forward. + expect(merged.version).toBe(7); + } + }); + + it('throws BlockConflictResolvedError without merged when refetch fails', async () => { + const fetchImpl = vi + .fn() + .mockResolvedValueOnce(mkResponse(409)) + .mockResolvedValueOnce(mkResponse(500)); + + await expect( + saveBlockWithConflictRetry({ + fetchImpl: fetchImpl as unknown as typeof fetch, + documentId: DOC, + blockId: BLK, + text: 'x', + mentionedPersons: [] + }) + ).rejects.toMatchObject({ code: 'CONFLICT_RESOLVED', merged: undefined }); + }); + + it('throws Save failed for any other non-OK response', async () => { + const fetchImpl = vi.fn().mockResolvedValueOnce(mkResponse(500)); + await expect( + saveBlockWithConflictRetry({ + fetchImpl: fetchImpl as unknown as typeof fetch, + documentId: DOC, + blockId: BLK, + text: 'x', + mentionedPersons: [] + }) + ).rejects.toThrow('Save failed'); + }); + + it('rejects ids that are not UUIDs (path-injection guard)', async () => { + const fetchImpl = vi.fn(); + await expect( + saveBlockWithConflictRetry({ + fetchImpl: fetchImpl as unknown as typeof fetch, + documentId: DOC, + blockId: '../../etc/passwd', + text: 'x', + mentionedPersons: [] + }) + ).rejects.toThrow(/Invalid id/); + expect(fetchImpl).not.toHaveBeenCalled(); + }); +}); diff --git a/frontend/src/lib/utils/saveBlockWithConflictRetry.ts b/frontend/src/lib/utils/saveBlockWithConflictRetry.ts new file mode 100644 index 00000000..4daeca50 --- /dev/null +++ b/frontend/src/lib/utils/saveBlockWithConflictRetry.ts @@ -0,0 +1,60 @@ +import type { PersonMention, TranscriptionBlockData } from '$lib/types'; +import { BlockConflictResolvedError, mergeBlockOnConflict } from '$lib/utils/blockConflictMerge'; + +const UUID_RE = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i; + +type Args = { + fetchImpl: typeof fetch; + documentId: string; + blockId: string; + text: string; + mentionedPersons: PersonMention[]; +}; + +/** + * Persists a transcription block edit, with built-in handling for the + * rename-mid-edit conflict (B12b). + * + * - 200/204 → resolves with the server's updated block. + * - 409 → refetches the latest server block, merges it with the + * transcriber's unsaved input via mergeBlockOnConflict, and + * throws BlockConflictResolvedError carrying the merged + * snapshot. The caller is responsible for updating local + * state with `err.merged` before surfacing the error. + * - other → throws Error('Save failed'). + * + * Validates both ids against the UUID pattern before any fetch fires + * (Sina #5505 — defence-in-depth path-injection guard). + */ +export async function saveBlockWithConflictRetry(args: Args): Promise { + const { fetchImpl, documentId, blockId, text, mentionedPersons } = args; + if (!UUID_RE.test(documentId) || !UUID_RE.test(blockId)) { + throw new Error(`Invalid id for save: doc=${documentId} block=${blockId}`); + } + + const url = `/api/documents/${documentId}/transcription-blocks/${blockId}`; + const res = await fetchImpl(url, { + method: 'PUT', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ text, mentionedPersons }) + }); + + if (res.status === 409) { + const fresh = await fetchImpl(url); + if (!fresh.ok) { + throw new BlockConflictResolvedError(blockId); + } + const serverBlock = (await fresh.json()) as TranscriptionBlockData; + const merged = mergeBlockOnConflict({ + serverBlock, + localText: text, + localMentions: mentionedPersons + }); + const err = new BlockConflictResolvedError(blockId); + (err as BlockConflictResolvedError & { merged: TranscriptionBlockData }).merged = merged; + throw err; + } + + if (!res.ok) throw new Error('Save failed'); + return (await res.json()) as TranscriptionBlockData; +} diff --git a/frontend/src/routes/documents/[id]/+page.svelte b/frontend/src/routes/documents/[id]/+page.svelte index 0baa8985..a62fb70c 100644 --- a/frontend/src/routes/documents/[id]/+page.svelte +++ b/frontend/src/routes/documents/[id]/+page.svelte @@ -88,44 +88,28 @@ async function loadTranscriptionBlocks() { } } -const UUID_RE = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i; - async function saveBlock( blockId: string, text: string, mentionedPersons: import('$lib/types').PersonMention[] ) { - // Path-injection defence in depth (Sina #5505): both ids are server-controlled - // today, but reject anything that isn't a UUID before interpolating it into - // the URL — a future feature accepting user-supplied ids must not silently - // bypass this check. - if (!UUID_RE.test(doc.id) || !UUID_RE.test(blockId)) { - throw new Error(`Invalid id for save: doc=${doc.id} block=${blockId}`); - } - const res = await fetch(`/api/documents/${doc.id}/transcription-blocks/${blockId}`, { - method: 'PUT', - headers: { 'Content-Type': 'application/json' }, - body: JSON.stringify({ text, mentionedPersons }) - }); - if (res.status === 409) { - // Rename-mid-edit (B12b): refetch latest, merge so transcriber input survives. - const { mergeBlockOnConflict, BlockConflictResolvedError } = - await import('$lib/utils/blockConflictMerge'); - const fresh = await fetch(`/api/documents/${doc.id}/transcription-blocks/${blockId}`); - if (fresh.ok) { - const serverBlock = await fresh.json(); - const merged = mergeBlockOnConflict({ - serverBlock, - localText: text, - localMentions: mentionedPersons - }); - transcriptionBlocks = transcriptionBlocks.map((b) => (b.id === blockId ? merged : b)); + const { saveBlockWithConflictRetry } = await import('$lib/utils/saveBlockWithConflictRetry'); + const { BlockConflictResolvedError } = await import('$lib/utils/blockConflictMerge'); + try { + const updated = await saveBlockWithConflictRetry({ + fetchImpl: fetch, + documentId: doc.id, + blockId, + text, + mentionedPersons + }); + transcriptionBlocks = transcriptionBlocks.map((b) => (b.id === blockId ? updated : b)); + } catch (err) { + if (err instanceof BlockConflictResolvedError && err.merged) { + transcriptionBlocks = transcriptionBlocks.map((b) => (b.id === blockId ? err.merged! : b)); } - throw new BlockConflictResolvedError(blockId); + throw err; } - if (!res.ok) throw new Error('Save failed'); - const updated = await res.json(); - transcriptionBlocks = transcriptionBlocks.map((b) => (b.id === blockId ? updated : b)); } async function deleteBlock(blockId: string) {