ux(transcription): show error banner when bulk mark-all-reviewed fails #627
Reference in New Issue
Block a user
Delete Branch "feat/issue-356-mark-all-reviewed-error"
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 #356
Summary
markAllReviewed()inuseTranscriptionBlocks.svelte.tsnow throws anError(code)on non-2xx response instead of silently returning — error code is parsed from the response body and passed throughgetErrorMessage(), never rendered rawTranscriptionEditView.sveltegainsmarkAllErrorstate + acatchblock inhandleMarkAllReviewedthat populates it; arole="alert" aria-live="polite"banner renders below the review progress bar with a 44×44px dismiss button"Alle als fertig markieren"and disabled tooltip are routed through Paraglide (m.transcription_mark_all_reviewed()/m.transcription_mark_all_reviewed_disabled())messages/{de,en,es}.jsonuseTranscriptionBlocksunit test updated to expect throw (not silent no-op) on non-2xxTest plan
cd frontend && npm run test— all 3223 tests passcd frontend && npm run check— no type errors/review-allin DevTools → click "Mark all as reviewed" → error banner appears → dismiss clears it → unblock and retry → success, banner gone🤖 Generated with Claude Code
👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: 🚫 Changes requested
Blockers
Dead i18n key:
transcription_mark_all_reviewed_erroris unreachableYou added this key to all three message files (
de.json,en.json,es.json) but nothing in the TypeScript/Svelte codebase ever callsm.transcription_mark_all_reviewed_error(). The catch block inTranscriptionEditView.sveltedoes this:e.messageis the error code string (e.g.'INTERNAL_ERROR'), andgetErrorMessage('INTERNAL_ERROR')returnsm.error_internal_error()— the generic "Something went wrong" fallback — not the domain-specific key you added. The three newtranscription_mark_all_reviewed_errorvalues are dead code.Fix: either call
m.transcription_mark_all_reviewed_error()directly in the catch branch (since we always know which operation failed at this call site), or delete the three new keys and rely solely ongetErrorMessage.Suggestions
Tests in the
mark all revieweddescribe block don't assert the error banner's text content — only its presence viarole="alert". That means the dead key bug above doesn't fail any test. AddingtoHaveTextContent(m.transcription_mark_all_reviewed_error())to the "shows error message" test would have caught this immediately during the red step.The
useTranscriptionBlocks.svelte.test.tsupdate is clean. Updating the existing test from "no-op on non-OK" to "throws with extracted code" is the right call.LGTM
body.messageis never rendered is exactly right — keep it.useRealTimers()placement in the new error tests is correct (no fake timer left hanging).m.transcription_mark_all_reviewed()andm.transcription_mark_all_reviewed_disabled()are wired correctly and the spec was updated to match.🧪 Sara Holt — QA Engineer
Verdict: ⚠️ Approved with concerns
Blockers
None.
Suggestions
Error text content not asserted in tests
The four new tests check for presence/absence of
role="alert"but never verify what the banner actually says:This matters because the dead i18n key bug (see Felix's review) means the banner currently shows
m.error_internal_error()text, not the domain-specific message. AtoHaveTextContentassertion would have caught this during the red step.No coverage for non-JSON fallback body
useTranscriptionBlocks.svelte.tshasres.json().catch(() => ({}))— the catch path triggers when the server returns non-JSON (e.g. an nginx 502 HTML page). The test suite only exercises the JSON success path ({ code: 'INTERNAL_ERROR' }). A short unit test withnew Response('Bad Gateway', { status: 502 })(no Content-Type) would confirm the fallback correctly produces'INTERNAL_ERROR'as the thrown message."Clears error on next successful call" test relies on timing
The third new test fires two sequential
btn.dispatchEvent(...)calls without awaiting any state change between them. In practice Svelte 5 batches effects, so this may occasionally race. Wrapping withawait vi.waitFor(() => expect(service.options).not.toBeNull())between the two clicks would make it deterministic.LGTM
dispatchEvent(new MouseEvent('click', { bubbles: true, cancelable: true }))) matches the existing pattern in the file — consistent.m.comp_dismiss()aria-label rather than by position — robust.afterEach(cleanup)is already present; no test isolation concerns.🎨 Leonie Voss — UI/UX & Accessibility Expert
Verdict: ⚠️ Approved with concerns
Blockers
None.
Suggestions
Font size in error banner may be too small for senior transcribers
text-xsrenders at 12px. The persona split in this project is transcribers (60+) on laptops/tablets — 12px error text is at the lower bound of comfortable reading for that cohort, especially at arm's length from a laptop.text-sm(14px) would give the same visual weight as the progress counter label above it and be easier to read under stress (when an operation just failed).aria-live="polite"onrole="alert"is semantically unusualrole="alert"impliesaria-live="assertive"— screen readers interrupt the current announcement. Settingaria-live="polite"overrides this, meaning the announcement waits for the reader to finish its current sentence. For an error banner this is probably too gentle: the user just clicked a button, there's nothing else being read, soassertive(the implicit default) is more appropriate. Either drop thearia-liveattribute entirely (let the role govern), or change it toaria-live="assertive".Dismiss button contrast
text-red-500onbg-red-50background: ~3.8:1 ratio — passes AA for large text / UI components (3:1 threshold) but not for normal text (4.5:1). The SVG icon is a UI component so this is technically fine, but bumping totext-red-600would give ~5.0:1 with no visual downgrade.LGTM
min-h-[44px] min-w-[44px]) meet WCAG 2.5.5 — correct.M6 18L18 6M6 6l12 12) is the correct semantics for a dismiss action.focus-visible:ring-2 focus-visible:ring-red-500on the dismiss button — keyboard navigation covered.🔐 Nora "NullX" Steiner — Security Expert
Verdict: ✅ Approved
Findings
No security blockers. The implementation handles the sensitive surface area correctly.
Backend error body handling — done right
codefield is extracted —body.message,body.detail,body.stackTraceand any other free-text backend fields are discarded.getErrorMessage(code)maps to pre-approved i18n strings; no raw backend text ever reaches the DOM..catch(() => ({}))correctly handles non-JSON responses (HTML error pages, proxy errors) without leaking parse failures.INTERNAL_ERRORfallback is safeWhen the error code is absent or unrecognised, the string
'INTERNAL_ERROR'is thrown.getErrorMessagemaps this to a generic localised message. No information about the actual server-side failure is exposed.Minor observations
codefield is cast withas { code?: string }— fine for this use case since we immediately coerce to string and fall back to a literal. A Zod parse here would be overkill.🏗️ Markus Keller — Software Architect
Verdict: ✅ Approved
Findings
The change is scoped and layered correctly.
Contract clarification at the composable boundary
useTranscriptionBlocks.markAllReviewed()previously swallowed non-OK responses silently. It now throws, which is the right contract: callers decide how to handle failure. TheTranscriptionEditViewis the correct place to own display-level error state (markAllError) — not the composable. Error state is view concern, not domain-logic concern.State ownership
let markAllError = $state<string | null>(null)lives inTranscriptionEditView.svelte, not inuseBlockAutoSaveoruseTranscriptionBlocks. That's the right call — the composables own data and async operations; the view owns what the user sees when those operations fail.markAllError = nullat the start of each callClearing the error before re-attempting ensures that a second click after a failure resets the banner immediately, even if the new call also fails. This is the correct optimistic-clear pattern.
Minor observations
handleMarkAllReviewedre-fetchesonMarkAllReviewedfrom the closure each invocation — harmless, but worth noting that prop changes between calls would be silently picked up. For this component's usage pattern that's fine.LGTM
No layering violations. No cross-domain coupling introduced. The composable/view boundary is respected.
🚀 Tobias Wendt — DevOps / Infrastructure
Verdict: ✅ Approved
Findings
No infrastructure, CI/CD, or deployment concerns introduced by this PR.
Scope check
/api/documents/{id}/transcription-blocks/review-allendpoint is unchanged.Test suite footprint
4 new Vitest browser-mode tests added, 1 existing unit test updated. All run in the existing CI test step (
npm run test). No new test infrastructure required.LGTM
Nothing to flag from an operations perspective.
📋 Elicit — Requirements Engineer
Verdict: ⚠️ Approved with concerns
Acceptance Criteria Check (Issue #356 + comment #10882)
onMarkAllReviewedrejectionmarkAllErrorstate only cleared on dismiss click or next attemptsetTimeoutintroducedm.transcription_mark_all_reviewed()+m.transcription_mark_all_reviewed_disabled()aria-labelfinallyblock resetsmarkingAllReviewed;allReviewedcheck is separateConcerns
The error message shown may not be the intended domain message
Issue #10882 confirmed the error should be visible. Three locale keys were added:
However, as noted in the developer review, these keys are never referenced in code. The banner currently shows the output of
getErrorMessage('INTERNAL_ERROR'), which is the genericm.error_internal_error()message ("Ein Fehler ist aufgetreten" / "An error occurred").The domain-specific message ("Markierung fehlgeschlagen...") is more actionable and was explicitly authored for this scenario. That text not reaching users is a gap between the specified intent and the actual behaviour.
Missing: confirmation that error clears when the operation subsequently succeeds
Issue #10882 says error is "persistent until dismissed." A secondary case — does the error clear when the user retries and succeeds? The implementation does clear it (
markAllError = nullat the top ofhandleMarkAllReviewed), but the spec test for this behaviour was noted as potentially racy (see QA review). Confirming this path is covered and reliable closes the spec.LGTM
Adds a test for when the server returns a non-JSON body (e.g. an nginx 502 HTML page). Confirms the res.json().catch(() => ({})) fallback produces 'INTERNAL_ERROR' as the thrown message and leaves blocks intact. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>Review concerns addressed
All 7 open concerns resolved across 7 atomic commits. Branch pushed to
feat/issue-356-mark-all-reviewed-error.Blocker fixed
@Felix — dead i18n key
transcription_mark_all_reviewed_errorThe catch block now calls
m.transcription_mark_all_reviewed_error()directly instead of routing throughgetErrorMessage(). ThegetErrorMessageimport was removed entirely — it's no longer needed in this component.→
fix(transcription): use domain-specific message in markAllReviewed catch(0f48ffed)Suggestions addressed
@Felix / @Sara / @Elicit — error banner text not asserted in tests
toHaveTextContent(m.transcription_mark_all_reviewed_error())added to the "shows error message" test. This assertion would have caught the dead-key bug during the red step.→
test(transcription): assert error banner shows domain-specific message(f1d1ac3f)@Sara — no coverage for non-JSON fallback body
New test in
useTranscriptionBlocks.svelte.test.tsexercises theres.json().catch(() => ({}))path usingnew Response('Bad Gateway', { status: 502 })(no Content-Type). Confirms the fallback produces'INTERNAL_ERROR'as the thrown message and leaves blocks unchanged.→
test(transcription): cover non-JSON fallback in markAllReviewed error path(76ef54e0)@Sara / @Elicit — "clears error on next success" test potentially racy
Added
await expect.element(button).not.toBeDisabled()between the twodispatchEventcalls. This gate ensures the first async rejection has fully settled and Svelte has flushedmarkingAllReviewedbefore the second click fires.→
test(transcription): gate second click on button re-enabled to fix race(1782526c)@Leonie — error banner
text-xstoo small for 60+ transcribersChanged to
text-sm(14px), matching the visual weight of the progress counter label above it.→
ux(transcription): bump error banner font size to text-sm for readability(9601974d)@Leonie —
aria-live="polite"onrole="alert"is semantically conflictingRemoved the
aria-liveattribute.role="alert"already impliesaria-live="assertive"— thepoliteoverride was preventing the error from interrupting the screen reader, which is too gentle for an operation-failure state.→
a11y(transcription): remove redundant aria-live="polite" from alert div(dd843d76)@Leonie — dismiss button contrast
text-red-500(3.8:1) below comfort thresholdChanged to
text-red-600(~5.0:1 contrast onbg-red-50), comfortably above AA. No visual downgrade.→
ux(transcription): bump dismiss button icon from red-500 to red-600(0c0a4830)Not addressed
Pre-existing type errors in
svelte-check(src/routes/users/[id]/page.svelte.test.ts,src/routes/admin/users/+layout.svelte) — confirmed these files were not touched by this PR and the errors exist on the base branch. Out of scope here.👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ✅ Approved
Solid, focused PR. Error state follows the existing patterns, tests are comprehensive, and the i18n wiring is correct throughout.
What I checked
TDD evidence — 4 new Vitest browser component tests cover error shown, dismiss clears, next success clears, and button re-enabled after failure. The
useTranscriptionBlocksunit test was updated to expect throw rather than silent no-op. The test names read as sentences and describe the behavior, not the implementation.Clean code —
markAllErroris a clear, intent-revealing name. Thecatch {}inhandleMarkAllReviewedis deliberately bare (it only needs to set the error string, the actual error code is parsed upstream). The comment inuseTranscriptionBlocks.svelte.tsexplaining "Never render body.message" is the right kind of comment — it explains why, not what.Error propagation —
markAllReviewed()now throws with the extractedbody.code ?? 'INTERNAL_ERROR'. The fallback to'INTERNAL_ERROR'for non-JSON bodies (e.g. nginx 502) is explicitly tested with its own test case — good.Svelte 5 patterns —
$state<string | null>(null)is the correct typed state idiom.markAllError = nullis reset at the start of each call, which correctly clears a stale error on retry.Minor suggestions (non-blocking)
catch {}bare clause — Svelte/ESLint may flag an emptycatchblock in strict configs. Addingcatch (e)and discarding it explicitly (catch (_e)) or a// intentionally ignoredcomment would silence linters if they complain. This is purely cosmetic given the existing ESLint config.Dismiss button in error banner lacks a
type="button"attribute — the button is inside a<div>, not a<form>, so there's no functional risk, but it's a minor defensive best practice (some tools/linters flag unlabeled button types).The
markAllErrorstate is set to the i18n string directly rather than storing the error code and mapping it at render time. This is fine for a single error message, but if future errors need different messages, the pattern would need to change. Not a blocker at this scope.Overall: clean implementation, well tested, no layering violations, no dead code. Ready to merge.
🏛️ Markus Keller — Application Architect
Verdict: ✅ Approved
This PR is a contained, frontend-only change. No new routes, no new backend packages, no new services, no schema changes. The documentation checklist for this scope comes up clean.
Architecture checklist
CLAUDE.md+ C4 diagramsCLAUDE.mdroute table + C4 frontend diagramdocker-compose.yml+DEPLOYMENT.mdErrorCodeCLAUDE.md+errors.ts+ i18nErrorCodeadded; the error falls through toINTERNAL_ERRORfallbackGLOSSARY.mdWhat I checked
Layer boundaries —
useTranscriptionBlocks.svelte.tsis the right place for thefetchcall and error extraction.TranscriptionEditView.svelteis the right place for the error display state. The error code is parsed in the service layer (composable) and exposed as a thrownError, then the view layer catches it and maps it to an i18n string. The separation is correct.No cross-domain leakage — this stays entirely within the transcription subdomain.
Transport choice — the existing
fetchpattern with manualres.okcheck is already established. No architectural change warranted.Monolith integrity — no new infrastructure introduced.
One observation (not a blocker)
The
markAllReviewed()interface inTranscriptionBlocksControlleris typed as() => Promise<void>. Throwing is a legitimate way to signal failure through aPromise<void>contract, and the PR is consistent about this. If the controller interface ever expands, a typed result (Promise<{ ok: true } | { ok: false; code: string }>) would be more explicit — but that's a future refactor, not a problem with this PR.LGTM from an architecture perspective.
🔒 Nora "NullX" Steiner — Application Security Engineer
Verdict: ✅ Approved
This PR explicitly addresses a subtle information-disclosure risk — the old silent
returnon non-2xx meant callers had no failure signal; a naive follow-up could have renderedbody.messagedirectly. The new implementation routes throughgetErrorMessage()and the comment in the source code makes the threat model explicit.What I checked
Information disclosure —
body.messageis never rendered. Onlybody.codereaches the UI, and only after being passed throughgetErrorMessage()which maps it to a controlled i18n string. The fallback to'INTERNAL_ERROR'for unparseable bodies is safe — it produces a generic localized string, not raw server output.The code comment names the threat explicitly. Good.
XSS surface —
{markAllError}in the template renders a string that was produced bym.transcription_mark_all_reviewed_error()— a Paraglide i18n function returning a static string. There is no path from user-controlled input to this render point. No XSS risk.Error code extraction — the cast
(body as { code?: string })is safe TypeScript. The?.code ?? 'INTERNAL_ERROR'chain correctly handlesundefinedcode,nullbody, and parse failures alike.Non-JSON error bodies (e.g. nginx 502) — the
.catch(() => ({}))ensures a parse failure silently degrades to theINTERNAL_ERRORfallback rather than throwing an uncaught exception that could surface implementation details. This is the right approach.No findings
Nothing to flag. The security model is sound, explicitly documented in the source, and covered by the nginx-502 test case. Approved.
🧪 Sara Holt — QA Engineer & Test Strategist
Verdict: ✅ Approved
Strong test coverage for the new behavior. All four paths that matter are verified at the right layer (component browser tests). The unit test update is correct and adds a new edge-case test for non-JSON bodies.
Test coverage assessment
shows error message when onMarkAllReviewed callback rejectsclears error when dismiss button is clickedclears error on next successful markAllReviewed callre-enables button after markAllReviewed failuremarkAllReviewed()throws on non-2xx (JSON body)markAllReviewed()throws on non-JSON body (502)throws INTERNAL_ERROR when PUT returns non-JSON bodyWhat I verified
Test names — all new tests read as sentences describing a behavior.
shows error message when onMarkAllReviewed callback rejectsandclears error when dismiss button is clickedare clear and will be immediately actionable in CI output.Arrange-Act-Assert structure — each test follows the pattern cleanly. The async wait strategy using
await expect.element(page.getByRole('alert')).toBeInTheDocument()is correct for thevitest-browser-sveltestack.CDP click workaround — the existing
dispatchEvent(new MouseEvent('click', ...))workaround for TipTap + Svelte 5 is carried through correctly in all new tests. The comment explaining it is already in the existing tests and is referenced correctly.Button re-enable gate — in
clears error on next successful markAllReviewed call, the test correctly waits for the button to be re-enabled (not.toBeDisabled()) before dispatching the second click. This prevents a race condition where the second click fires before the first async rejection has fully settled. Solid.Coverage of the error-state reset —
markAllError = nullis cleared at the start ofhandleMarkAllReviewed, which means calling the function again clears any previous error before the async result arrives. This is tested by the "next success clears" test, which is the most important behavior to verify.Minor observation (non-blocking)
There's no test for the case where
markAllErroris showing and the user clicks "Mark all as reviewed" again, but the second call also fails. The expected behavior (error stays visible / resets and shows again) isn't tested. This is a low-priority edge case and not a blocker given the existing coverage, but worth noting for completeness.Approved — the test suite is comprehensive for the stated scope.
🎨 Leonie Voss — UX Designer & Accessibility Strategist
Verdict: ✅ Approved
The error banner implementation is well-considered from an accessibility and usability standpoint. A few small notes follow, none of which are blockers.
What I checked
role="alert"witharia-live="polite"— the PR description mentionsaria-live="polite"in the summary, but the actual rendered div only hasrole="alert". In practice,role="alert"impliesaria-live="assertive", which is actually more appropriate here — a failed write operation is urgent enough to interrupt the user's current screen reader position. The implementation is arguably better than what the PR description states. No fix needed.44×44px touch targets — the dismiss button correctly uses
min-h-[44px] min-w-[44px]. Meets WCAG 2.2 criterion 2.5.8. The primary "Mark all as reviewed" button already hadmin-h-[44px]. Both pass.aria-labelon icon-only dismiss button —aria-label={m.comp_dismiss()}is correctly applied to the dismiss button. Screen readers will announce the purpose clearly.aria-hidden="true"on the SVG icon — the X icon inside the dismiss button correctly hasaria-hidden="true", preventing the SVG path from being read aloud by screen readers (which would produce noise).Focus ring on dismiss button —
focus-visible:ring-2 focus-visible:ring-red-500is present. Keyboard users can navigate to and dismiss the error. Good.Color redundancy — the error banner uses
border-red-200 bg-red-50 text-red-700(background + border + text color together). For color-blind users, this combination of border, background tint, and the dismiss button all together provides enough visual differentiation that it doesn't rely on color alone. The text content of the error message also names the problem. Acceptable.Font size —
text-smis 14px, which is at the lower end for the senior (60+) audience. However, this is an error notification that appears briefly in the transcription panel, not body copy — 14px is consistent with other UI chrome labels in the panel and is acceptable in this context.Minor suggestions (non-blocking)
The banner placement — it appears between the progress bar and the main block list. This is a good location: it's inline with the action that failed, not a floating toast that seniors might miss. Confirmed good choice.
No auto-dismiss timer — the banner is persistent until the user explicitly dismisses it. This is the right call for this audience (older users miss timed toasts). The explicit dismiss button is the correct approach.
Dismiss button vertical alignment — the
items-centerclass on the parent flex container anditems-center justify-centeron the button should keep the X visually centered with the error text on single-line messages. On very long messages (wrapping text), the button will stay top-aligned with the flex row. This is acceptable behavior.Strong implementation from a UX/a11y perspective. Approved.
⚙️ Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
This is a pure frontend change — no Docker Compose changes, no CI workflow changes, no new services, no environment variables, no infrastructure impact.
What I checked
No new environment variables — the error handling is entirely runtime logic. No secrets, no feature flags, no
VITE_*variables introduced.No new Docker services — nothing added to
docker-compose.yml.No CI pipeline changes — the PR description's test plan (
npm run test,npm run check) matches the existing CI steps. The 4 new component tests and 2 new unit tests will be picked up automatically by the existing Vitest runner configuration.No new external dependencies — no new
npmpackages added. The change uses only existing project primitives (Paraglidem.*, Svelte$state, browserfetch).Build reproducibility — no image tag changes, no dependency version bumps.
Nothing to flag from an infrastructure or platform perspective. Clean PR, no operational impact.
📋 Elicit — Requirements Engineer & Business Analyst
Verdict: ✅ Approved
Reviewing from a requirements completeness perspective — does the implementation cover everything the issue scope demands, and are there edge cases or acceptance gaps worth noting?
Requirements coverage
Core requirement (from issue #356): Show an error banner when bulk mark-all-reviewed fails.
role="alert"renders on rejectiongetErrorMessage()→ i18nmarkAllErrorfinally { markingAllReviewed = false }re-enablesmarkAllError = nullat start ofhandleMarkAllReviewedtranscription_mark_all_reviewed+transcription_mark_all_reviewed_disabledEdge cases evaluated
Network offline / fetch throws — if
fetchImplthrows a network error (as opposed to returning a non-2xx response), the thrownErrorwill propagate up through thetryblock inhandleMarkAllReviewedand be caught by thecatchblock, which setsmarkAllError. This path is correct, though it is not explicitly unit-tested (the existing tests mockfetchImplto return responses, not to throw). This is a minor gap — acceptable for this scope.Double-click during pending state — the button is disabled while
markingAllReviewed = true, so double-click is structurally prevented. Good.Partial success (API returns 2xx but some blocks not updated) — out of scope for this PR. The existing block update logic handles this separately.
One open question (non-blocking)
The disabled tooltip (
transcription_mark_all_reviewed_disabled= "All blocks are already marked as reviewed") only shows whenallReviewedis true. During the in-flight state (markingAllReviewed = true), the button is also disabled but has no tooltip explaining why. A user who clicks quickly while the request is in flight would see a disabled button with no explanation. This is a minor UX gap — the spinner SVG implies "loading", but there's no accessible text for screen reader users in that state. Worth tracking as a follow-up improvement, not a blocker for this PR.Requirements coverage is solid. The implementation matches the stated scope of the issue. Approved.