refactor(transcription): extract saveBlockWithConflictRetry into a util
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 <noreply@anthropic.com>
This commit is contained in:
@@ -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`
|
||||
|
||||
152
frontend/src/lib/utils/saveBlockWithConflictRetry.spec.ts
Normal file
152
frontend/src/lib/utils/saveBlockWithConflictRetry.spec.ts
Normal file
@@ -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();
|
||||
});
|
||||
});
|
||||
60
frontend/src/lib/utils/saveBlockWithConflictRetry.ts
Normal file
60
frontend/src/lib/utils/saveBlockWithConflictRetry.ts
Normal file
@@ -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<TranscriptionBlockData> {
|
||||
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;
|
||||
}
|
||||
@@ -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) {
|
||||
|
||||
Reference in New Issue
Block a user