feat: Transcription read mode (clean split) #177 #205
Reference in New Issue
Block a user
Delete Branch "feat/issue-177-transcription-read-mode"
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?
Summary
TranscriptionPanelHeaderwith "Lesen | Bearbeiten" segmented toggle, block count, and close buttonTranscriptionReadViewrendering blocks as readable text with[unleserlich]/[...]markers styled as italic muted text (XSS-safe, no{@html})prefers-reduced-motionsupport (instant scroll, no CSS animation, 2s static highlight)splitByMarkersutility with 8 unit tests, 16 component tests totalCloses #177
Relates to #192 (deferred: editor name in status bar)
Test plan
[unleserlich]or[...]→ renders as italic muted textprefers-reduced-motion: no scroll animation, instant highlightnpm run test→ all new tests pass🤖 Generated with Claude Code
👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ⚠️ Approved with concerns
What I checked
TDD evidence, naming, function size, Svelte 5 rules, component splitting, SOLID.
Strengths
splitByMarkershas 8 unit tests written red-first. Component tests forAnnotationLayer,TranscriptionPanelHeader, andTranscriptionReadViewcover key behaviors. Good discipline.splitByMarkersis clean: pure function, single responsibility, well-tested, no{@html}— exactly what NullX recommended.TranscriptionPanelHeader(toggle + status + close),TranscriptionReadView(prose), both small and focused.{#each}blocks:(block.id)on the outer loop — correct.Concerns
Inner
{#each}uses index key(i)—TranscriptionReadView.svelte:27Index-based keys are acceptable here since the segments are derived from the block text and won't be reordered independently. But if the block text changes reactively while rendered, Svelte may not diff correctly. Since
splitByMarkersis called on every render and the list is stable for a givenblock.text, this is fine in practice. No change needed — just documenting the reasoning.prefersReducedMotionuses$derivedwithwindowaccess —+page.svelte:60This evaluates once during SSR (returns
false) and once on the client. It does not react to runtime changes to the media query (e.g., user toggles reduced motion while the page is open). If that matters, use$effectwithmatchMedia.addEventListener('change', ...). For this use case (scroll sync), the current approach is pragmatic and sufficient.loadTranscriptionBlocks().then()inside$effect—+page.svelte:189The
.then()readstranscriptionBlocks.length— but at that point the state update fromloadTranscriptionBlocks()may or may not have been applied yet depending on Svelte's batching. This works becauseloadTranscriptionBlocksdirectly assignstranscriptionBlocks = await res.json()before the promise resolves. Still, a race condition could occur if the user rapidly toggles transcribe mode. Low risk, but worth noting.onModeChangeprop shadowsmimport —+page.svelte:302The parameter
mshadows the importedmfrom Paraglide. Not a bug (the scope is tiny), but rename tonewModefor clarity.Suggestions
+page.sveltefile is growing (now ~310 lines of script). The mode state + scroll-sync logic (~40 lines) could be extracted to atranscriptionMode.svelte.tsmodule if more features are added. Not needed now — just flagging the trajectory.🏗️ Markus Keller — Application Architect
Verdict: ✅ Approved
What I checked
Layer boundaries, state ownership, prop drilling depth, component coupling, data flow direction.
Observations
No backend changes — purely frontend. The existing
GET /api/documents/{id}/transcription-blocksendpoint serves both read and edit mode. Clean scope.State ownership is correct —
panelMode,highlightBlockId,flashAnnotationId,pdfStripExpandedall live in+page.svelte(the orchestrator). Children receive them via props and emit events via callbacks. No stores, no context API, no global state. This is the right pattern for page-scoped UI state.Prop drilling depth is acceptable —
flashAnnotationIdthreads through+page.svelte→DocumentViewer→PdfViewer→AnnotationLayer(3 levels). For a single boolean/string prop, this is fine. If more props accumulate on this path, consider a context. Not needed now.TranscriptionBlockDatatype extension — addingupdatedAt?: string | nullis backwards-compatible. The?makes it optional, so existing code that doesn't return this field won't break. Good.lastEditedAtderived computation —+page.svelte:76-82computes the most recentupdatedAtacross all blocks. This is O(n) on every reactive update oftranscriptionBlocks. For the expected block counts (1-50), this is negligible. If block counts grow to hundreds, memoize or compute server-side. Not a concern now.No new API calls — the read view reuses the same block data already fetched for edit mode. No additional network requests. Good architectural economy.
No concerns. Ship it.
🧪 Sara Holt — QA Engineer & Test Strategist
Verdict: ⚠️ Approved with concerns
What I checked
Test coverage, test quality, missing edge cases, test pyramid placement.
Strengths
splitByMarkershas excellent unit coverage — 8 tests covering plain text, single marker, multiple markers, adjacent markers, empty string, and non-matching brackets. Well-structured, descriptive names.vitest-browser-sveltewith Playwright. These are real browser tests, not JSDOM approximations. Good choice for verifying DOM output.TranscriptionPanelHeadertests cover critical interactions — mode switching, disabled state, close callback, singular/plural block count. 7 tests total.TranscriptionReadViewtests cover rendering and click behavior — paragraph count, marker rendering, sort order, click callback. 6 tests.Concerns
No test for
highlightBlockIdflash behavior —TranscriptionReadViewacceptshighlightBlockIdand applies aflash-highlightCSS class, but no test verifies this. Add:No test for
flashAnnotationIdon AnnotationLayer — theannotation-flashCSS class is applied whenflashAnnotationIdmatches, but no test verifies it. The existing 3 tests only coverdimmed. Add a test for the flash class.lastEditedAtformatting not tested —TranscriptionPanelHeaderusesIntl.DateTimeFormatto format the date. A test with a knownlastEditedAtvalue would verify the formatted output appears correctly in the DOM.No test for 0 blocks status display — the header shows
{count} AbschnitteforblockCount: 0, which renders "0 Abschnitte". Is that the desired behavior, or should 0 blocks show nothing? The singular test covers 1, plural covers 5, but 0 is untested.Suggestions
*.svelte.test.tsconvention andvitest-browser-svelteusage.🔒 Nora "NullX" Steiner — Application Security Engineer
Verdict: ✅ Approved
What I checked
XSS vectors, injection surfaces, data exposure, auth boundaries, DOM manipulation safety.
Findings
No
{@html}— XSS surface eliminated — the[unleserlich]/[...]rendering uses text splitting +{#each}with plain text nodes and<em>elements. Even if block text contains<script>alert(1)</script>, it renders as escaped text. This follows my recommendation from the issue discussion exactly. Well done.No new API endpoints — no new attack surface. The read view consumes the same data as the edit view.
No
innerHTML,document.write, orinsertAdjacentHTML— all DOM manipulation is via Svelte's template system, which escapes by default.document.querySelectorusage is safe —+page.svelte:174usesdocument.querySelector('[data-block-id="${block.id}"]'). Theblock.idis a UUID from the backend, not user input. No selector injection risk.No new form inputs or user-submitted data paths — the read view is purely display. No
contenteditable, no textareas, no forms.dispatchEventin tests only — thenew MouseEvent('click', { bubbles: true })pattern appears only in test files, not production code. No concern.No security issues. Clean read-only feature.
🎨 Leonie Voss — UI/UX Design Lead
Verdict: ⚠️ Approved with concerns
What I checked
Typography, color usage, accessibility (WCAG 2.2), touch targets, mobile layout, dark mode compatibility, brand compliance.
Strengths
font-serif text-[16px] leading-[1.85]gives a generous reading experience. The 1.85 line-height exceeds the WCAG 1.5 recommendation for body text. Good for the 60+ audience.hover:bg-[rgba(0,199,177,0.06)]provides a subtle clickability cue without cluttering the reading experience.prefers-reduced-motionsupport — both CSS and JS respects the preference. The CSS media query disables animation and applies a static highlight. The JS usesbehavior: 'instant'for scroll. Thorough.Concerns
Paragraph hover at 6% opacity may be invisible on uncalibrated displays —
rgba(0,199,177,0.06)is extremely subtle. On a senior user's monitor with warm color temperature, this is likely invisible. Consider bumping to0.08or0.10, or adding a2pxleft border on hover atrgba(0,199,177,0.2)for a stronger affordance that the text is interactive.Close button touch target is 32x32px —
TranscriptionPanelHeader.svelte:77usesh-8 w-8(32px). WCAG 2.2 Target Size (Level AA) requires 24px minimum, but our project standard is 44px minimum. Increase toh-11 w-11or wrap in a larger touch area with padding.Segmented toggle buttons may be too small on mobile — the toggle uses
h-7(28px) withtext-xs(12px). On mobile, these are small tap targets. The outer pill is 28px tall. Considerh-9(36px) on mobile via responsive class.Intl.DateTimeFormathardcoded to'de-DE'—TranscriptionPanelHeader.svelte:17. The app supports de/en/es via Paraglide, but the date formatter always uses German locale. Should use the current Paraglide language tag to format dates in the user's language.No dark mode verification — the component uses semantic tokens (
bg-surface,border-line,text-ink-2) which should work in dark mode. But the flash animation uses hardcodedrgba(0, 199, 177, ...)values in bothTranscriptionReadView.svelteandAnnotationLayer.svelte. These should reference the--c-turquoiseCSS variable for dark mode compatibility, or be verified to look acceptable on dark backgrounds.Suggestions
🔧 Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
What I checked
New dependencies, bundle impact, build/CI changes, infrastructure requirements, font loading.
Findings
No new npm dependencies — zero additions to
package.json. The new components use only existing project dependencies (Svelte, Tailwind, Paraglide). No bundle size impact beyond the new component code itself.No new fonts — the read view uses
font-serifwhich is already configured inlayout.css. No new@font-facedeclarations, no new Google Fonts requests, no GDPR concerns.No infrastructure changes — no new services, no new environment variables, no Docker changes, no CI pipeline changes. Purely frontend feature.
No new external requests — the read view reuses data already fetched from the existing transcription blocks endpoint. No new API calls, no new CORS considerations.
CSS animations are GPU-composited — the flash effects use
background-colorandoutlineproperties. These are paint operations, not layout-triggering. Performance impact is negligible even on older devices.i18n files grew by 11 keys each — minimal. Paraglide tree-shakes unused messages, so only messages actually imported will be in the bundle.
No concerns. No operational impact. Ship it.
Review Concerns Addressed
10 commits pushed addressing all reviewer concerns:
@saraholt (QA)
90120ca— flash-highlight class test for TranscriptionReadView (+ negative test)b312878— annotation-flash class test for AnnotationLayer (+ negative test)1b063d4— lastEditedAt formatting test + 0 blocks status display test@leonievoss (UI/UX)
0a5c82c— Close button touch target increased from 32px to 44px (h-8 → h-11)9d9d19c— Segmented toggle height increased to 36px on mobile (h-9 md:h-7)e31b733+b01a9ef— Paragraph hover opacity bumped to 10%, then refactored to usebg-turquoise/10tokene01ef56— Date formatting usesgetLocale()instead of hardcoded 'de-DE'10fdaf7— Flash animations usecolor-mix(... var(--color-turquoise) ...)for dark mode compatibility@felixbrandt (Developer)
b5ec4eb— Renamed shadowedmparameter tonewModein onModeChange callbackTest results