refactor(transcribe): extract region navigation into a tested pure helper (#327)
Review follow-up (Sara): j/k wrap-around and fresh-entry had no direct coverage — the logic lived inline in the page where the action spec only mocks the callbacks. Extracted to a pure stepRegion() with 9 unit tests (empty list, forward/back, both wraps, fresh-entry null + unknown id, length-1). Also replaces the inline nested ternary Felix flagged. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
@@ -0,0 +1,48 @@
|
|||||||
|
import { describe, it, expect } from 'vitest';
|
||||||
|
import { stepRegion } from './regionNavigation';
|
||||||
|
|
||||||
|
describe('stepRegion', () => {
|
||||||
|
const ids = ['a', 'b', 'c'];
|
||||||
|
|
||||||
|
it('returns null for an empty list', () => {
|
||||||
|
expect(stepRegion([], null, 1)).toBe(null);
|
||||||
|
expect(stepRegion([], 'a', -1)).toBe(null);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('steps forward from the middle', () => {
|
||||||
|
expect(stepRegion(ids, 'a', 1)).toBe('b');
|
||||||
|
expect(stepRegion(ids, 'b', 1)).toBe('c');
|
||||||
|
});
|
||||||
|
|
||||||
|
it('steps backward from the middle', () => {
|
||||||
|
expect(stepRegion(ids, 'c', -1)).toBe('b');
|
||||||
|
expect(stepRegion(ids, 'b', -1)).toBe('a');
|
||||||
|
});
|
||||||
|
|
||||||
|
it('wraps forward past the last region to the first', () => {
|
||||||
|
expect(stepRegion(ids, 'c', 1)).toBe('a');
|
||||||
|
});
|
||||||
|
|
||||||
|
it('wraps backward past the first region to the last', () => {
|
||||||
|
expect(stepRegion(ids, 'a', -1)).toBe('c');
|
||||||
|
});
|
||||||
|
|
||||||
|
it('lands on the first region when entering fresh (no active) going forward', () => {
|
||||||
|
expect(stepRegion(ids, null, 1)).toBe('a');
|
||||||
|
});
|
||||||
|
|
||||||
|
it('lands on the last region when entering fresh (no active) going backward', () => {
|
||||||
|
expect(stepRegion(ids, null, -1)).toBe('c');
|
||||||
|
});
|
||||||
|
|
||||||
|
it('treats an unknown active id as a fresh entry', () => {
|
||||||
|
expect(stepRegion(ids, 'zzz', 1)).toBe('a');
|
||||||
|
expect(stepRegion(ids, 'zzz', -1)).toBe('c');
|
||||||
|
});
|
||||||
|
|
||||||
|
it('returns the single region for both directions (wrap of length 1)', () => {
|
||||||
|
expect(stepRegion(['only'], 'only', 1)).toBe('only');
|
||||||
|
expect(stepRegion(['only'], 'only', -1)).toBe('only');
|
||||||
|
expect(stepRegion(['only'], null, 1)).toBe('only');
|
||||||
|
});
|
||||||
|
});
|
||||||
33
frontend/src/lib/document/transcription/regionNavigation.ts
Normal file
33
frontend/src/lib/document/transcription/regionNavigation.ts
Normal file
@@ -0,0 +1,33 @@
|
|||||||
|
/**
|
||||||
|
* Region navigation for the transcribe keyboard shortcuts (j/k) — issue #327.
|
||||||
|
*
|
||||||
|
* Pure and side-effect free so the wrap-around / fresh-entry branches are
|
||||||
|
* unit-testable without mounting the page.
|
||||||
|
*/
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Pick the annotation id one step from the active region, wrapping around the
|
||||||
|
* ends. Entering fresh (no active region, or an unknown id) lands on the first
|
||||||
|
* region going forward and the last going backward.
|
||||||
|
*
|
||||||
|
* @param orderedAnnotationIds region annotation ids in display order
|
||||||
|
* @param activeId the currently active region, or null
|
||||||
|
* @param delta +1 for next (j), -1 for previous (k)
|
||||||
|
* @returns the next annotation id, or null when there are no regions
|
||||||
|
*/
|
||||||
|
export function stepRegion(
|
||||||
|
orderedAnnotationIds: string[],
|
||||||
|
activeId: string | null,
|
||||||
|
delta: 1 | -1
|
||||||
|
): string | null {
|
||||||
|
const count = orderedAnnotationIds.length;
|
||||||
|
if (count === 0) return null;
|
||||||
|
|
||||||
|
const current = activeId === null ? -1 : orderedAnnotationIds.indexOf(activeId);
|
||||||
|
if (current === -1) {
|
||||||
|
return delta > 0 ? orderedAnnotationIds[0] : orderedAnnotationIds[count - 1];
|
||||||
|
}
|
||||||
|
|
||||||
|
const next = (current + delta + count) % count;
|
||||||
|
return orderedAnnotationIds[next];
|
||||||
|
}
|
||||||
@@ -12,6 +12,7 @@ import ShortcutCheatsheet from '$lib/document/transcription/ShortcutCheatsheet.s
|
|||||||
import { transcribeShortcuts } from '$lib/shared/actions/transcribeShortcuts';
|
import { transcribeShortcuts } from '$lib/shared/actions/transcribeShortcuts';
|
||||||
import { createOcrJob } from '$lib/ocr/useOcrJob.svelte';
|
import { createOcrJob } from '$lib/ocr/useOcrJob.svelte';
|
||||||
import { createTranscriptionBlocks } from '$lib/document/transcription/useTranscriptionBlocks.svelte';
|
import { createTranscriptionBlocks } from '$lib/document/transcription/useTranscriptionBlocks.svelte';
|
||||||
|
import { stepRegion } from '$lib/document/transcription/regionNavigation';
|
||||||
import { createFileLoader } from '$lib/document/viewer/useFileLoader.svelte';
|
import { createFileLoader } from '$lib/document/viewer/useFileLoader.svelte';
|
||||||
import { scrollToCommentFromQuery } from '$lib/shared/utils/deepLinkScroll';
|
import { scrollToCommentFromQuery } from '$lib/shared/utils/deepLinkScroll';
|
||||||
import { getConfirmService } from '$lib/shared/services/confirm.svelte';
|
import { getConfirmService } from '$lib/shared/services/confirm.svelte';
|
||||||
@@ -102,15 +103,9 @@ async function createBlockFromDraw(rect: {
|
|||||||
const sortedBlocks = $derived([...transcription.blocks].sort((a, b) => a.sortOrder - b.sortOrder));
|
const sortedBlocks = $derived([...transcription.blocks].sort((a, b) => a.sortOrder - b.sortOrder));
|
||||||
|
|
||||||
function goToRegion(delta: 1 | -1) {
|
function goToRegion(delta: 1 | -1) {
|
||||||
if (sortedBlocks.length === 0) return;
|
const ids = sortedBlocks.map((b) => b.annotationId);
|
||||||
const current = sortedBlocks.findIndex((b) => b.annotationId === activeAnnotationId);
|
const next = stepRegion(ids, activeAnnotationId, delta);
|
||||||
const next =
|
if (next) activeAnnotationId = next;
|
||||||
current === -1
|
|
||||||
? delta > 0
|
|
||||||
? 0
|
|
||||||
: sortedBlocks.length - 1
|
|
||||||
: (current + delta + sortedBlocks.length) % sortedBlocks.length;
|
|
||||||
activeAnnotationId = sortedBlocks[next].annotationId;
|
|
||||||
}
|
}
|
||||||
|
|
||||||
function toggleMode() {
|
function toggleMode() {
|
||||||
|
|||||||
Reference in New Issue
Block a user