feat(transcribe): keyboard shortcuts for the transcribe power path + cheatsheet overlay (#327) #728
Reference in New Issue
Block a user
Delete Branch "feat/issue-327-transcribe-shortcuts"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes #327.
Adds the full keyboard power-path for the Transcribe panel plus a
?cheatsheet overlay, and makes the annotationDeleteaffordance discoverable (the deferred #722 addendum). TDD throughout — 46 unit/component tests green locally, an e2e + axe spec for CI;npm run checkadds 0 errors over baseline andnpm run buildpasses.What's in
transcribeShortcutsaction (lib/shared/actions/) — single-ownerwindowkeydown translator:j/kregion nav,emode toggle,ndraw-arm (edit only),ttraining mark,Delete,?cheatsheet, and theEscprecedence ladder (cheatsheet → editable no-op → close panel). Focus guard exempts only?;destroy()removes the listener. 20 tests.ShortcutCheatsheet.svelte— native<dialog aria-modal>withshowModal()focus trap, close button focused on open, 8 grouped<kbd>rows, autosave footer, reduced-motion fade. 8 tests.documents/[id]/+page.svelte) — action attached, callbacks to existing context setters, the inlineEschandler removed (single owner), cheatsheet + draw-armed hint rendered.Deletediscoverability —aria-keyshortcuts="Delete"+ extendedaria-label; AnnotationShape no longer self-handlesDelete(action is sole owner → deletes exactly once),onfocussyncs the active region. Removed the now-redundantonDeleteRequestplumbing through AnnotationLayer/PdfViewer/DocumentViewer.?tip added (#320). i18n complete de/en/es. e2etranscribe-shortcuts.spec.ts+ axe on the open cheatsheet.Two decisions where the codebase differed from the spec (please sanity-check)
tsemantics — no per-region training flag exists onmain(that's #321); "mark for training" is two fixed document-level script-type chips. Sottoggles the document'sKURRENT_RECOGNITIONenrollment, no-op unless a region is active. Follow-up once #321 lands.ndraw mode — drawing is always-on pointer-drag with no keyboard draw path, son(edit-only) arms a visible draw cue (drawArmed→ hint banner), cleared on next draw / leaving edit mode.Minor deviations from the issue's file list
transcribeShortcuts.svelte.spec.ts(not.spec.ts) so it runs in the client/browser vitest project where DOM APIs exist — matchesclickOutside.svelte.spec.ts.🤖 Generated with Claude Code
🏛️ Markus — Software Architect
✅ Approved
I reviewed this purely through the architecture lens: layer boundaries, coupling, module responsibilities, and domain separation. This is a structurally clean change — it actually reduces coupling rather than adding it, which is exactly what I want to see from a frontend feature PR.
What I checked, and why it's clean
1. The prop-plumbing removal is a real coupling win. Previously
onDeleteAnnotationRequest/onDeleteRequestwas threaded down four layers (DocumentViewer→PdfViewer→AnnotationLayer→AnnotationShape) for the sole purpose of letting the leaf shape's inlineDeletekeydown call back up to the page. That is plumbing-as-coupling: a presentational leaf component owning a domain command. The PR collapses it — deletion now lives at the page level (deleteCurrentRegion→ the still-presenthandleAnnotationDeleteRequest), and the downward callback is replaced by a thinner, more generalonAnnotationFocus(id)signal. The leaf no longer knows about deletion semantics at all (AnnotationShape.svelte:343-346documents the single-owner decision). I grepped the tree — zero dangling references to the removed props. Boundary is clean.2. Single owner for the
keydown/ Esc ladder is the right call. The old inlinedocument.addEventListener('keydown', …)in+page.svelte'sonMountcompeted with the shape's ownDeletehandler for ownership of keyboard behavior. The action (transcribeShortcuts.ts) is now the single owner of the panel's global keydown, including Esc (decision B1, documented inline attranscribeShortcuts.ts:944-953and+page.svelte:288-289). Consolidating two scattered listeners into one well-documented owner is a responsibility-boundary improvement.3. The action's interface is properly decoupled from the domain.
TranscribeShortcutOptionsis all primitives + callbacks — no transcription entity types leak in. The action owns no state and has no persistence responsibility; it's a pure input→callback translator. That keeps it independently unit-testable (the 254-line spec mocks every callback with zero Svelte/DOM-store coupling), which is exactly the testability posture I want: logic lives in a replaceable unit, not buried in a component's lifecycle.4. No accidental complexity, no backend/infra touch. Pure frontend island. No new transport, no schema, no migration, no new domain concept, no new ErrorCode/Permission, no route. Nothing on my doc-currency checklist is triggered — no diagram or ADR update is owed. i18n keys added across all three locales (de/en/es) consistently.
Suggestions (non-blocking)
transcribeShortcuts.tsplacement. It lives inshared/actions/next toclickOutside,trapFocus,radioGroupNav— but those three are domain-agnostic primitives, whereas this one is transcribe-specific (its option vocabulary isgoToNextRegion,toggleTrainingMark, etc.). The coupling is fine because the interface only takes primitives/callbacks, so I'm not asking for a move. But for "navigate by naming convention alone," a transcribe-specific action arguably belongs underdocument/transcription/alongsideShortcutCheatsheet.svelte.shared/should read as "reusable anywhere." Minor — your call; if it stays, a one-line comment noting it's transcribe-scoped-but-parked-here would help future readers. (transcribeShortcuts.ts:944)The action ignores its
nodeparameter (_node) and binds the listener towindowinstead. That's a legitimate choice for a panel-global shortcut, and the underscore signals intent — but it meansuse:transcribeShortcutson the panel div (+page.svelte:1199) is slightly misleading: the binding element is not the listener scope. A short comment ("listens on window, not node, because shortcuts are panel-global while the panel is a fixed overlay") would close the small gap between what the directive looks like it does and what it does. Non-blocking.Neither suggestion affects a module boundary or data-integrity rule, so they don't gate the merge. From an architecture standpoint this lands cleanly. Nice work pulling the delete responsibility back up to where it belongs.
🔒 Nora — Security Engineer
Verdict: LGTM. Frontend-only keyboard-shortcut feature with effectively zero new security surface — no network/auth changes, no untrusted data rendered unsafely, and the global listener lifecycle is correctly bounded. A couple of hygiene notes below, none blocking.
Blockers
None.
Suggestions
transcribeShortcuts.ts:30— listener stays attached for the whole document page, not just transcribe mode. Thewindowkeydown handler is registered on the page-root<div>(+page.svelte:288), which is unconditionally mounted on/documents/[id]. Svelte callsdestroy()on navigation away, so there is no leak — good. But the listener is live even when the panel is closed; every key on the document page runs throughhandleKeydown. The earlyisPanelOpen()guards (transcribeShortcuts.ts:25,:34) keep it a clean no-op, so this is fine as-is. Worth a one-line comment noting the guard is the thing that makes the always-on listener safe, for the next auditor. (CWE-401 considered and ruled out — flagging only as documentation hygiene.)transcribeShortcuts.ts:18-39—preventDefaultscope is appropriately narrow.e.preventDefault()only fires on keys the action actually consumes (j/k/e/n/t/Delete/?/Esc) and only after the editable-focus guard (isEditableTarget,:21) has bailed. This correctly avoids swallowing typing inside<input>/<textarea>/TipTapcontenteditable. The?-before-panel-guard ordering (:30returns early if!isPanelOpen()) means it won't steal?on unrelated pages. No key-swallowing concern.i18n strings are all static literals (
de/en/es.json) rendered via Paraglidem.*()and plainkbd/text interpolation — no user-controlled data reaches the DOM.ShortcutCheatsheet.svelteand the coach hint render only hardcoded shortcut caps and translated labels. No{@html}, noinnerHTML. XSS surface: none.ShortcutCheatsheet.svelte:38backdrop-click close comparesevent.target === dialogElon a native<dialog>witharia-modal="true"and focus moved to the close button on open — correct modal hygiene, no focus-trap escape concern.E2E seeding (
transcribe-shortcuts.spec.ts:33-84) creates the doc + blocks through the real authenticated API (same pattern asannotations.spec), not by bypassing auth or writing to the DB directly. Good — it exercises the actual permission path rather than circumventing it.E2E_BASE_URLdefaults to localhost; no secrets embedded.Nice touch removing the duplicate
Deletehandler fromAnnotationShape.svelteand making the action the single owner — that eliminates the double-delete race and is backed by a regression test (AnnotationShape.svelte.spec.ts:55). Clean.⚙️ DevOps
LGTM — frontend-only change, nothing operational to flag. No new dependencies, no CI/build/config changes, no env or secret touches. The new e2e spec fits the existing Playwright setup cleanly.
Blockers
None.
Suggestions
None blocking. Notes for the record:
package.json/package-lock.json/playwright.config.tsare untouched. The spec's only non-Playwright import,@axe-core/playwright, is already a declared devDependency (frontend/package.json:38) and is used identically by ~10 existing specs — so the i18n compile (Paraglide) and the CI image (mcr.microsoft.com/playwright:v1.60.0-noble,.gitea/workflows/ci.yml:16) are unaffected.transcribe-shortcuts.spec.tslands undertestDir: './e2e'(frontend/playwright.config.ts:8) and is picked up automatically — no workflow edit needed.annotations.spec.tspattern (API-seed two transcription blocks inbeforeAll, then drive the keyboard — no OCR, no manual draw). It reuses the committedfrontend/e2e/fixtures/minimal.pdffixture (present, 310 bytes). It waits on concrete selectors ([data-hydrated],[data-testid="annotation-…]") rather than fixed sleeps, and per-testsetTimeout(30_000)is in line with neighbours. Withworkers: 1/ sequential execution andretries: 2in CI (playwright.config.ts:22-24), the timing surface is well controlled.One minor watch-item (non-blocking): the a11y test at
frontend/e2e/transcribe-shortcuts.spec.ts:152runs AxeBuilder scoped to.include('dialog'). Axe scans can be the slowest part of an e2e run; scoping to the dialog (as done here) keeps it cheap, so this is fine as written — just the one spot to revisit if e2e wall-time ever creeps up.🎨 Leonie — UX / Accessibility
Verdict: Solid, accessible keyboard layer — native
<dialog>, real focus management,aria-keyshortcuts, reduced-motion all done right. Two real issues hold it back from a clean approve: key-cap glyphs are hardcoded German, and the whole shortcut surface is shown to touch-only users who can't use it.Blockers
Key-cap glyphs are hardcoded, not i18n'd —
ShortcutCheatsheet.svelte:19,22. Every label in the cheatsheet is a Paraglide message, but the caps'Entf'and'Esc'are German literals baked into thegroupsarray. An EN/ES user sees GermanEntfwhile reading "Delete current region".Entfis also the wrong glyph on a US/UK/ES keyboard — that key readsDel/Supr. The standard cross-keyboard label isDelete/Del. Either route the caps through messages (cheatsheet_cap_delete→Entf/Del/Supr,cheatsheet_cap_esc→Esc) or use the neutralEsc/Deleverywhere. Same literal leaks intoAnnotationShape.svelte:annotation_label_with_deleteis correctly i18n'd ("Show block, press Delete to remove"), but the fallback'Block anzeigen'on line 39 is a hardcoded German string — that's the label a non-transcribe reader's screen reader announces, in every locale. Move it to a message too.Touch-only devices are shown shortcuts they can't trigger —
ShortcutCheatsheet.svelte,TranscribeCoachEmptyState.svelte:75-82,+page.svelte?path. There's no(pointer: coarse)/(hover: none)guard anywhere (confirmed: nomatchMediafor pointer modality in any touched file). A 67-year-old transcriber on a tablet gets a?tip pointing at a key they don't have, and if a Bluetooth-less device ever shows the cheatsheet trigger it's pure noise. Per our dual-audience rule, gate the empty-state tip and any visible cheatsheet entry point behind a fine-pointer check (e.g.window.matchMedia('(any-hover: hover) and (any-pointer: fine)'), reactive tochange). The?keydown handler itself can stay unconditional — it's harmless on touch since the key never fires.Suggestions
<kbd>chips are 12px (text-xs) —ShortcutCheatsheet.svelte:74,TranscribeCoachEmptyState.svelte:78(11px). 12px is my hard floor and 11px is below it. The cheatsheet is a deliberate reading surface for the 60+ audience; bump the caps totext-sm(14px) and the empty-state inline?to at leasttext-xs. Contrast is fine (text-inkonbg-muted, both AA-documented inlayout.css).Draw hint is announced, but
role="status"re-announcement is unreliable —+page.svelte:430-437. Good call making it non-blocking (pointer-events-none, centered,z-50). But a plainrole="status"element that's conditionally mounted may not re-announce if armed twice in a row. Render the live region as a persistent node toggling its text content (or addaria-live="polite"explicitly on a stable wrapper) so the cue is announced every timenis pressed, not just first mount. Visually it's clean and doesn't overlap the cheatsheet (separate stacking, hint atbottom-4).Cheatsheet rows are single-column key-then-label, not the two-column grid the spec implied —
ShortcutCheatsheet.svelte:72-78. Not a bug; thejustify-betweenrows read fine and stay full-width on mobile (w-[calc(100%-2rem)] max-w-md). Just flagging that label-right-aligned with the cap hard-left puts a wide gap between key and meaning on a narrow phone — considerjustify-start gap-3with the label immediately after the cap so the eye doesn't have to track across the row.Close button is a proper 44×44 (
h-11 w-11) witharia-labeland focus-on-open — all correct. Backdrop-click-to-close andonclosewired toonCloseare good. One nit: the close button hashover:bg-mutedbut no visiblefocus-visible:ring — addfocus-visible:ring-2 focus-visible:ring-brand-navyso keyboard users landing on it (it's auto-focused) see the focus state, not just the hover state.👨💻 Felix Brandt — Senior Fullstack Developer
⚠️ Approved with concerns — clean, well-tested, idiomatic Svelte 5. The action design (stateless input→callback translator owning the global keydown) is exactly right, the Esc ladder is documented and tested, and the Delete single-owner refactor closes a real double-delete hole. A handful of clean-code nits, no blockers.
What I liked
52920a5ais spec+impl only). ThetranscribeShortcuts.svelte.spec.tscovers guards, edge cases (panel closed, modifier held, cheatsheet-already-open), the Esc ladder rung-by-rung, and lifecycle (destroy +update()swapping stale closures). This is the empty/error/edge discipline I want to see.{#each}is keyed everywhere ((i),(shortcut.cap),(annotation.id)downstream).sortedBlocksis a named$derived, not a$state+$effect. The two page$effects are legitimate side-effect synchronisation, not derived-value-in-effect: the cheatsheet one drives the imperative<dialog>.showModal()DOM API, and thedrawArmedreset is cross-state reconciliation. Both correct.shortcutOptionsgetter object is the right call. Methods close over$statevia getters, so the listener reads live state andupdate()never needs to refire. Good reasoning, and theupdate()test pins it.Blockers
None.
Suggestions
1. Nested ternary in
goToRegion— hard to hold in working memory (+page.svelte~L113-123)Two guard clauses read in one pass:
2. Redundant condition in the draw-hint template (
+page.svelteL430)The
$effectat L138 already forcesdrawArmed = falsewheneverpanelMode !== 'edit', sodrawArmedis true ⟹ we're in edit mode. The&& panelMode === 'edit'is dead logic in the markup —{#if drawArmed}is sufficient. (If you keep the belt-and-braces guard, that's a defensible call; just flagging it isn't load-bearing.)3. Untranslated aria-label fallback (
AnnotationShape.svelte~L40)The new branch is i18n'd via paraglide; the fallback is a hardcoded German literal. This pre-dates your PR (the old
aria-label="Block anzeigen"was already hardcoded), but since you're touching the line, am.annotation_label_plain()key would finish the job and keep es/en honest.4.
eswallows the key even when read-only (+page.sveltetoggleMode)The action always
preventDefault()se, buttoggleMode()no-ops when!canWrite. A read-only user pressingegets a silent dead key. Minor, but consider gating the action'seonpanelMode/write-capability the waynis gated, so the keypress falls through when it can't act.Note
n(startDrawMode) only arms the visual hint banner — it doesn't change the viewer's actual draw capability (drawing is already available in edit mode viacanDraw). That's the intended "cue" per your comment, so it's correct; just confirming it's cosmetic-by-design and not a missed wiring.i18n parity verified: all 15 new keys present in de/en/es. Nice work.
🧪 Sara — QA Engineer
Verdict: Approve with reservations. The action layer is genuinely well-tested (20 clean, sentence-named cases, real teardown, leak + stale-state checks). My concern is the seam: the page-level wiring that actually contains the interesting branch logic has zero direct coverage, and the headline "delete exactly once" claim is proven by construction in two separate files but never against the integrated whole.
Blockers
None that should hold merge — no broken tests, no flake I can reproduce. The items below are coverage gaps I'd want closed in this PR or a fast follow.
Suggestions
1.
goToRegionwrap-around + fresh-entry branches are untested (routes/documents/[id]/+page.svelte:96-107).The branchy logic —
current === -1 ? (delta>0?0:len-1)and the modulo wrap(current+delta+len)%len— lives here, but the action spec mocksgoToNextRegion/goToPrevRegionasvi.fn(), so none of it is exercised by unit tests. The e2e (transcribe-shortcuts.spec.ts:130-143) only doesj j kacross two blocks and stops before any boundary, so neither the end→start wrap (jon last) nor start→end wrap (kon first) nor the-1fresh-entry-from-kpath ever runs. This is the exact "untested boundary condition" pattern. ExtractgoToRegion(or a purenextIndex(current, delta, len)helper) into a testable unit and cover: empty list, fresh entry viajand viak, wrap at both ends.2. Empty-block-list early return is untested (
+page.svelte:98).if (sortedBlocks.length === 0) return;has no test. One line, one assertion —j/kwith no blocks is a no-op.3.
twith no active block is untested (+page.svelte:135-139). The action spec provestfires thetoggleTrainingMarkcallback, but the callback's own guardif (!activeAnnotationId) return;— the whole reason the comment says "no-op unless a region is active" — is never asserted. The behavior the doc-comment promises is the one with no test behind it.4. The "delete exactly once" double-bind fix is proven by construction, not integration. Two separate unit facts (
AnnotationShape.svelte.spec.ts:59"shape does not act on Delete", + action spec:198 "action fires once") together imply exactly-once, but nothing renders a realAnnotationShapeand attaches the action and presses Delete once to confirm the count is 1, not 2. The action spec at :198 even decorates the target withdata-annotation/tabindexto look like the real scenario, but since the shape no longer binds Delete, that DOM is inert — the assertion passes identically with a plaindocumenttarget, so it gives false reassurance that the double-bind is covered. A single component/integration test (shape + action, one Delete,toHaveBeenCalledOnce) would actually pin the regression you're fixing.5.
drawArmedhint + disarm-$effectare untested (+page.svelte:130, 137-139, 430-438). New visible UI state (the draw-hint banner) and the "disarm on leaving edit mode" effect have no coverage.ntoggling the hint visible, and switching mode hiding it, would be a small component test.6. e2e flake watch (not blocking, just flagging the load-bearing assumptions):
j/kcorrectness hinges entirely on the resize overlay (RESIZE_AREA_LABEL) rendering for the active region in read mode (:88-90,:135). The header comment asserts this; if that overlay ever becomes edit-only, this test silently inverts from "navigation works" to "navigation broken" with no other signal. Worth a one-line comment in the impl tying the read-mode overlay to this test, so a future refactor can't quietly break it.e-toggle (:113-128) couples to the literal string'Für Training vormerken'as its only edit-mode proxy. Fine today; adata-testidon the edit view would be more durable than a content string.What's good
makeOptions/makeEditable), realafterEachteardown, explicit leak test (:236) and stale-update()test (:244). This is the test quality bar I want.aria-modal, labelled heading, focus-on-open (:60), and axecriticalfilter in e2e (:156). Accessibility-as-gate, exactly right.aria-keyshortcuts/label coverage inAnnotationShape.svelte.spec.ts(:both branches) is thorough.Net: ship it if the wrap-around (#1) gets at least a thin unit test — that's the one genuinely branchy, genuinely untested piece of business logic in the diff. The rest are cheap follow-ups.
📋 Requirements Engineer
Verdict: Request changes — 14 of 15 acceptance rows met; one row ("touch-only devices") is unmet, and two intentional deviations are acceptable but require traceability updates so the issue stops over-claiming scope.
I reviewed strictly against issue #327's 15-item acceptance checklist: are the criteria met, is there scope creep, are there unmet requirements, and is the issue ↔ PR traceability honest. I did not assess code quality — that's for other personas.
Blockers (unmet acceptance criteria)
B1 — AC "On touch-only devices (no hardware keyboard) shortcuts are unavailable and the cheatsheet is not surfaced, to avoid confusion" is NOT satisfied.
The diff contains no device/input-capability guard (no
pointer: coarse, nomatchMedia, no keyboard-presence detection — confirmed by grep across the full diff). Two parts to disentangle:?discoverability hint inTranscribeCoachEmptyState.svelterenders unconditionally, so a touch-only transcriber (the spec explicitly calls out 60+ users on tablets) is shown a "press?" tip for a key they have no way to press. That is exactly the "confusion" the AC was written to prevent. The implementation summary marks this row green, but the checkbox is not earned.?cheatsheet hint."Deviations reviewed — both acceptable, but traceability must be corrected (not blockers, but do before close)
D1 —
ttoggles a document-levelKURRENT_RECOGNITIONenrollment, not a per-region training flag.The issue body (Proposed shortcuts + "Related → #321") promises "Toggle 'mark for training' on the current region." The per-region flag lives in #321, which isn't merged. Toggling a document-level chid is a reasonable stopgap given codebase reality and I accept it for v1 — but the acceptance row as written ("
t… on the current region") is technically unmet by the literal text. Action: edit the issue to reflect that v1tis document-scoped (recognition enrollment), with a TODO linking #321 to make it region-scoped once that lands. Otherwise the issue claims a capability the PR doesn't deliver, and the traceability matrix lies. The "tis a silent no-op when no region is active" sub-criterion is met.D2 —
narms a draw cue rather than keyboard-drawing.The issue says "Start drawing a new region (Edit mode only)." Drawing on
mainis always pointer-drag with no keyboard path, and the option type already hintedstartDrawMode"does not create a block." Arming a visible cue (drawArmed→ banner, cleared on draw or on leaving edit mode) is a faithful, non-surprising interpretation — accepted, no scope creep. The "edit-mode-only, silent no-op in read mode" sub-criterion is met. Minor: the issue text should notenarms drawing rather than performs it, so a future reader doesn't expect a keyboard-only box.Met criteria (spot-checked against the diff, for the record)
8 shortcuts present; focus guard tests
INPUT/TEXTAREA/isContentEditableand exempts only?(with the Ctrl/Alt/Meta guard, Shift allowed);Esc3-rung ladder with lower-rung suppression; inline pageEschandler removed and folded into the action (single keydown owner);destroy()removes the listener;event.preventDefault()on every handled key;?open-only; native<dialog>.showModal()with close-button focus on open; autosave footer line present; no save/discard shortcuts; i18n complete for de/en/es (verified all three files, real translations not copies);?hint in the coach card;aria-keyshortcuts="Delete"+ extendedaria-labelon the annotation, with theAnnotationShapeDeletebranch removed so the key fires exactly once. axe-core and the delete-once regression are covered by spec files I trust to CI.Suggestions (non-blocking)
Escrow usingshortcut_close_panel, whose German value is "Bereich schließen". "Bereich" = region everywhere else in the very same sheet (shortcut_next_region= "Nächster Bereich"). Reading "Bereich schließen" next to "Nächster Bereich" impliesEsccloses a region, not the panel. Rename the value to "Panel schließen" (or "Transkriptionsbereich schließen") to remove the ambiguity. en/es ("Close panel"/"Cerrar panel") are fine.Review follow-up (Leonie, Felix, Markus): bump cheatsheet key caps to text-sm for the 60+ audience, add a focus-visible ring to the close button, simplify the draw-hint guard to {#if drawArmed} (the $effect already clears it outside edit mode), and document why the transcribeShortcuts action ignores its node and binds to window. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>Review follow-up — blockers + concerns addressed
Thanks all. Pushed 5 commits (
7199575a..74124dcc) resolving every blocker and the substantive concerns. All touched specs green (57 tests: 48 browser + 9 node),npm run checkadds 0 errors over baseline,npm run buildpasses.Blockers — fixed
?hint is now gated behind[@media(pointer:coarse)]:hidden, so touch-only transcribers are never told to press a key they lack. The cheatsheet itself only opens via the?key, so it already never surfaces without a keyboard. New test pins the guard. →39ef5f2dDeletekey cap is nowkey_cap_delete(Entf / Del / Supr) and the annotation read-only label isannotation_view_label(was a hardcoded German string in all locales). →7199575aConcerns — addressed
goToRegionwrap-around untested (Sara) — extracted to a purestepRegion()inregionNavigation.tswith 9 unit tests (empty list, forward/back, both wraps, fresh-entry null + unknown id, length-1). Also kills the nested ternary Felix flagged. →194340c7AnnotationShape, attachtranscribeShortcuts, focus, pressDeleteonce → asserts the delete fires exactly once. →f7bc3ccfshortcut_close_panel"Bereich schließen" → "Panel schließen". →7199575atext-sm, focus-visible ring on the close button,{#if drawArmed}simplified, and a comment explaining the action binds towindow(ignoresnode). →74124dccAcknowledged, not changed (with rationale)
ecallspreventDefaultfor read-only users (Felix, minor) — left as-is:ehas no native behaviour to preserve outside text inputs (those are already guarded), andtoggleModeno-ops harmlessly. Adding acanToggleModeoption to the action felt like more surface than the nit warrants — happy to add it if you'd prefer.tdocument-scoped /narms-not-draws (ReqEng deviations) — both are faithful to codebase reality (no per-region training flag until #321; drawing is pointer-only). Documented in the PR body and the issue completion comment; a #321 follow-up will maketregion-scoped.shared/actions/(Markus, non-blocking) — kept there per the issue's stated path and consistency withclickOutside/trapFocus.Ready for re-review.
👨💻 Felix Brandt — Senior Fullstack Developer (re-review)
✅ Approved. All four of my round-1 items are resolved or declined with sound rationale, and the refactor that resolves them is itself clean and well-tested. No remaining blockers.
My round-1 items — verified against current head
Nested ternary in
goToRegion→ resolved. The branchy index math is now extracted into a purestepRegion(orderedIds, activeId, delta)inregionNavigation.ts. It reads in one pass — guard clause for the empty list, guard clause for the fresh/unknown-id entry, then the modulo wrap as the last line. Intent-revealing names, a doc-comment that documents the contract, and the page-levelgoToRegioncollapses to four lines (+page.svelte:105-109). This is exactly the guard-clause shape I asked for, and extracting it also closed Sara's wrap-around coverage gap — two birds. The 9 unit tests (empty, fwd/back from middle, both wraps, fresh-entry fwd+back, unknown-id-as-fresh, length-1) are the right boundary set. Ran the spec locally: 9/9 green.Redundant
&& panelMode === 'edit'in the draw hint → resolved. Now{#if drawArmed}(+page.svelte:425). The disarm$effectis the single source of truth for the invariant; the markup just reads the flag. Correct.Hardcoded German aria-label fallback → resolved. Both branches are now Paraglide messages (
m.annotation_label_with_delete()/m.annotation_view_label(),AnnotationShape.svelte:354-356). en/es honest, screen-reader output localised in every locale.epreventDefault for read-only users → decline accepted. Your rationale holds: by the time the switch runs,isEditableTargethas already bailed, so a bareehas no native behaviour left to preserve, andtoggleModeno-ops harmlessly when!canWrite. AcanToggleModeoption would add action surface for a purely cosmetic dead-key nit — not worth it. Agreed, leave as-is.Note on the new tests
The integration test you added (
AnnotationShape.svelte.spec.ts— render a real shape, attach the livetranscribeShortcutsaction, focus, press Delete once, asserttoHaveBeenCalledTimes(1)) is the right answer to the "proven by construction, not integration" concern — it pins the double-delete regression against the integrated whole, not two separate facts that merely imply it. Good.Clean work. Ship it.
🏛️ Markus — Software Architect (re-review)
✅ Approved
Re-reviewed the current head (
74124dcc) through the architecture lens only — layer boundaries, coupling, module placement, doc-currency. Round 1 was already a clean approve; the follow-up commits improved the structure further, and both of my non-blocking suggestions were addressed. Nothing remains on my side.Prior notes — both resolved
Suggestion #1 (placement of the navigation logic). Better than I asked for. Instead of relocating the whole action, you extracted the branchy region-stepping into a pure, domain-located helper:
document/transcription/regionNavigation.ts(stepRegion), colocated withregionNavigation.spec.ts. The page-levelgoToRegionis now a 4-line adapter (+page.svelte:105-109) that maps blocks → ids and delegates. That puts the logic in the right feature package (navigates by naming convention alone), keeps it independently unit-testable with zero Svelte/DOM coupling, and incidentally kills the nested ternary. This is exactly the "what can change vs. what must be stable" separation I want — and it lands the testability win Sara and I both flagged. The action staying inshared/actions/is fine now that the transcribe-specific decision logic lives undertranscription/; the action is a genuinely reusable input→callback primitive again.Suggestion #2 (the
node/windowgap). Closed.transcribeShortcuts.ts:30-33now documents that the listener is global-on-windowby design while still authored as an action sodestroy()reliably unbinds on host unmount. The directive no longer misleads the next reader.New code — no boundary issues introduced
stepRegionis pure, total, and side-effect free (empty list, fresh-entry both directions, unknown-id, wrap at both ends). No domain types leak in — it takesstring[]+string | null, not transcription entities. Correct decoupling.onAnnotationFocus(id)replacing the threadedonDeleteAnnotationRequestis the same coupling-reduction I approved in round 1, now confirmed end-to-end across all four layers (DocumentViewer → PdfViewer → AnnotationLayer → AnnotationShape). The leaf shape emits a generic focus signal and no longer owns a delete command; deletion stays single-owner at the page. Boundary is clean, grep shows no dangling refs to the removed props.annotation_view_label,key_cap_delete) are presentation-layer string moves with full de/en/es parity — no structural impact.[@media(pointer:coarse)]:hidden) on the hint — purely presentational, no new module, no capability-detection service. Right altitude for the concern.f7bc3ccf) renders a realAnnotationShape+ live action and asserts one Delete fires exactly once — pins the single-owner invariant at the seam, not just by construction.Doc-currency: pure frontend island — confirmed zero non-
frontend/files in the diff. No migration, schema, join table, new package/domain, route, Docker service, external system, auth/upload-flow change, ErrorCode, Permission, or new domain term. Nothing on my checklist is owed; no diagram or ADR update required.Lands cleanly from an architecture standpoint. Nice work pulling the navigation logic into a tested unit in its proper home — that's the kind of refactor that pays down maintenance cost rather than adding it.
🎨 Leonie — UX / Accessibility (re-review)
Verdict: ✅ Approve. Both my round-1 blockers are cleared, and every suggestion I'd hoped to see landed is in. Clean, accessible keyboard layer — ship it.
Blockers — both resolved
Hardcoded glyphs + German SR label — fixed. The
Deletecap is nowkey_cap_deleteand renders correctly per locale (Entf/Del/Supr,de/en/es.json). The annotation read-only label is now the i18n'dannotation_view_labelas a$derivedwith no hardcoded fallback (AnnotationShape.svelte:39-41) — a non-transcribe reader's screen reader now hears "View block" / "Ver bloque" in the right locale instead of German "Block anzeigen". ✓Esccap staying a literal'Esc'(ShortcutCheatsheet.svelte:22): I accept this.Escis the physical key legend in de/en/es alike — there's no locale where that cap reads differently, so routing it through a message would add a key for zero divergence. Correct call.Touch-only guard — fixed. The coach
?hint is now gated behind[@media(pointer:coarse)]:hidden(TranscribeCoachEmptyState.svelte:75), so a tablet transcriber is never told to press a key they don't have. The cheatsheet has no visible trigger — it only opens via the?keydown — so it inherently never surfaces on a keyboard-less device. The AC is genuinely earned now, not by construction. ✓Suggestions — status
text-sm/14px in the cheatsheet (:74); empty-state inline?sits attext-xs/12px (:78) — at my floor, accepted. ✓focus-visible:ring-2 focus-visible:ring-brand-mint(:60). Mint on surface is AA-fine and reads clearly; navy vs mint is brand taste, not a defect. ✓role="status"re-announce (#4): left as-is, and I accept it. BecausedrawArmedresets tofalseon leaving edit mode and re-arms on eachn, the live node remounts per press, so it does re-announce in practice. Non-blocking from the start; no objection to deferring.justify-between. Cosmetic only — the rows stay full-width and legible at 320px. Fine to leave.No remaining blockers. Nice, careful pass on the fixes.
🔒 Nora — Security Engineer (re-review)
Verdict: LGTM — approve. Re-reviewed the round-2 fixes (
7199575a..74124dcc). They are frontend-only and introduce no new security or reliability surface; my round-1 approval stands.What I re-checked against the delta:
key_cap_delete→ Entf/Del/Supr,annotation_view_label) are static literals rendered as plain text/attribute interpolation via Paraglidem.*()—ShortcutCheatsheet.svelte:19,AnnotationShape.svelte:40. No user-controlled data flows in; no{@html}, noinnerHTMLanywhere in the touched files (grep-confirmed). XSS surface: still none.[@media(pointer:coarse)]:hiddenon the coach hint (TranscribeCoachEmptyState.svelte:75). Pure presentational media query, no JS branch, nomatchMedialistener to leak, no injection vector. Clean.stepRegion()(regionNavigation.ts) is a genuinely pure helper — operates only on astring[]of ids + a delta, no DOM access, no side effects, modulo arithmetic bounded bycount. The extraction didn't change the page's listener wiring.transcribeShortcutsstill binds towindowand removes ondestroy(); round 2 only added a clarifying comment (transcribeShortcuts.ts:30-33) explaining thewindow-not-nodebinding, which is exactly the documentation-hygiene note I raised in round 1. CWE-401 (listener leak) remains ruled out.focus-visible:ringon the cheatsheet close button and the{#if drawArmed}simplification are cosmetic — no security impact.Nothing to flag. Ship it.
📋 Requirements Engineer (re-review)
Verdict: Approve — all 15 acceptance rows now met. My round-1 blocker is resolved; the two intentional deviations remain acceptably documented.
I re-reviewed strictly against issue #327's 15-row acceptance checklist, focusing on the one row I blocked on in round 1 and re-confirming the two accepted deviations. No code-quality assessment — that's for the other personas.
Round-1 blocker — RESOLVED ✅
"On touch-only devices (no hardware keyboard) shortcuts are unavailable and the cheatsheet is not surfaced, to avoid confusion."
?discoverability hint inTranscribeCoachEmptyState.svelte:75is now gated behind[@media(pointer:coarse)]:hidden, so a touch-only transcriber is no longer told to press a key they lack. The guard is pinned by a regression test (TranscribeCoachEmptyState.svelte.spec.ts:75— "hides the keyboard hint on touch-only (coarse-pointer) devices"). →39ef5f2dcheatsheetOpenis set true only via theopenCheatsheetcallback bound to the?key (+page.svelte:148); with no hardware keyboard that key can't fire, so the dialog can never open. De-facto met by construction.The row is now earned, not just checkbox-ticked. This was my only blocker — it's cleared.
Deviations — re-confirmed acceptable, traceability now honest
tis document-scoped (KURRENT_RECOGNITIONenrollment), not per-region. Faithful to codebase reality (no per-region flag until #321). Now inline-documented at+page.svelte:116-120with the #321 follow-up noted, and called out in the PR/issue completion comment. The "silent no-op when no region active" sub-criterion holds. Accepted for v1.narms a draw cue (drawArmedbanner), not keyboard-drawing. Drawing onmainis pointer-only; arming a visible, self-clearing cue is a non-surprising interpretation. "Edit-mode-only, silent no-op in read mode" holds. Accepted, no scope creep.Round-1 suggestions — both addressed
shortcut_close_panelGerman value renamed "Bereich schließen" → "Panel schließen". ✅Final tally: 15 / 15 acceptance rows met. No remaining unmet rows. No blockers.
Clean re-review. Ship it.
⚙️ DevOps (re-review)
✅ LGTM (still). The follow-up commits (
7199575a..74124dcc) add nothing operational — confirmed no dependency, CI, or config drift.git diff origin/main...HEAD --name-onlyshows zero non-frontend/files.package.json/package-lock.json/playwright.config.ts/vite.config.ts/.gitea/workflows/*all untouched in the re-review delta.regionNavigation.spec.tsneeds no config and adds no runner cost. It's a pure-function unit test (import { describe, it, expect } from 'vitest', no@vitest/browserimport), so the vitestprojectsmatcher invite.config.tsroutes it to the existing nodeserverproject (include: src/**/*.{test,spec}.{js,ts}, excludes the*.svelte.*browser specs) — no new Playwright browser instance is spawned. Auto-discovered, no workflow edit.key_cap_delete/annotation_view_labelkeys (+ theshortcut_close_panelvalue fix) are static literals with full de/en/es parity — Paraglide compile is unaffected, no build/config change.[@media(pointer:coarse)]:hiddenon the coach hint (TranscribeCoachEmptyState.svelte:75) — purely presentational, nomatchMedia/JS capability-detection service, nothing to operate.Nothing on my checklist is triggered. Ship it.
🧪 Sara — QA Engineer (re-review)
Verdict: Approve. My top ask — wrap-around coverage — is closed with a clean pure-helper unit suite, and the headline "delete exactly once" claim is now proven by a genuine integration test instead of two separate by-construction facts. The two items I'm leaving open are cosmetic guards worth at most a fast follow, not a merge gate. I re-ran the new specs locally; both green (
stepRegion9/9 node,AnnotationShape7/7 browser).Prior concerns — status
1.
goToRegionwrap-around untested (my top ask) — ✅ RESOLVED. Extracted to a purestepRegion(orderedIds, activeId, delta)inregionNavigation.ts, andregionNavigation.spec.tscovers exactly the branches I called out: forward/back wrap at both ends, fresh entry viaj(null → first) and viak(null → last), unknown-id-as-fresh, and the length-1 self-wrap. Sentence-named, AAA, no mocks needed because the logic is now side-effect free. This is the right layer for branchy logic. Ran it: 9 passed.2. Empty-block-list early return — ✅ RESOLVED.
stepRegion([], …)returnsnull(covered twice, both directions), and the callergoToRegiononly assigns when the result is truthy (+page.svelte:108). No-op proven.3.
twith no active block is untested — ⚠️ STILL OPEN (non-blocking). The guard moved out of the action intotoggleTrainingMark(+page.svelte:123 — if (!activeAnnotationId) return;), so the action spec legitimately can't reach it, and nothing else asserts it. The behaviour the doc-comment promises still has no test behind it. One trivial guard line — a fast follow, not a gate.4. "Delete exactly once" proven by integration, not construction — ✅ RESOLVED.
AnnotationShape.svelte.spec.ts:158now renders a realAnnotationShape, attaches the livetranscribeShortcutsaction to that same element, focuses, pressesDeleteonce, and assertsdeleteCurrentRegionfired exactly once. I verified the shape's onlyonkeydownhandles Enter/Space (AnnotationShape.svelte:98-100) and never Delete — so if a competing handler ever crept back in, this test would catch the double-fire. This is no longer the inert-DOM false-reassurance from round 1; it pins the regression. Ran it: 7 passed.5.
drawArmedhint + disarm-$effectuntested — ⚠️ STILL OPEN (non-blocking).narming the banner (startDrawMode → drawArmed = true) and the disarm effect (+page.svelte:133-135) still have no coverage. Per ReqEng's D2 this is a faithful cosmetic cue, so I'm not blocking — but a small component test (pressn→ banner visible; switch mode → hidden) would close it cheaply.6. e2e flake watch — ✅ mostly addressed. The header comment now ties
j/kto the read-mode resize overlay (transcribe-shortcuts.spec.ts:15), which was my durability ask. Thee-toggle still proxies on the literal'Für Training vormerken'; adata-testidon the edit view would be more robust, but it's fine as-is.Net
Ship it. Concerns #1, #2, #4 — the genuinely branchy / regression-pinning ones — are all closed with tests at the correct layer. #3 and #5 are trivial guard/cosmetic-state gaps suitable for a fast follow; neither blocks merge. No broken tests, no reproducible flake. Test quality across the new specs (pure helper for navigation logic, real-shape integration for the delete invariant) is exactly the layering I want.
Fast-follow — Sara's two remaining gaps closed (
52f0babb)Both untested page-internal bits are now extracted into pure, unit-tested helpers (
document/transcription/):tno-active-region guard →resolveTrainingMark(activeAnnotationId, currentLabels)returnsnull(no-op) when no region is active, else the recognition-enrollment toggle with the correctenrolledflip. 4 tests (no-op ×2, enrol, un-enrol, ignores unrelated labels).drawArmedarm/disarm →canArmDraw(panelMode)/shouldDisarmDraw(panelMode). The page now arms the cue only viacanArmDrawand the disarm$effectusesshouldDisarmDraw. 2 tests.10 helper unit tests in this area total (incl.
stepRegion).npm run buildgreen; full check adds 0 errors over baseline. No behaviour change — pure refactor + coverage.With this, every persona blocker and concern from both review rounds is resolved or explicitly accepted. Ready to merge.