ux(transcription): show error toast when bulk "Alle als fertig markieren" fails #356

New Issue

marcel · 2026-04-27T08:12:43+02:00

marcel commented

2026-04-27 08:12:43 +02:00

Context

PR #352 ships markAllReviewed() in documents/[id]/+page.svelte with a silent failure path:

if (!res.ok) return;

No error toast, no user feedback. The @Transactional guarantee means block states are unchanged in the database on failure (all-or-nothing), but the user receives no indication the operation failed.

Raised by Felix (Developer) and Elicit (Requirements) in PR #352 Decision Queue (Item 4).

This is the same pattern as reviewToggle, but silent failure is more disorienting for a document-wide bulk action than for a single block toggle.

Acceptance criteria

When PUT /transcription-blocks/review-all returns a non-2xx response, a visible error message is shown to the user (toast or inline)
The error message is dismissible
Block state in the UI is unchanged (no optimistic update was applied)
Uses existing toast/notification infrastructure if one exists; otherwise add a minimal inline error display

Links

Feature implementation: PR #352
Original issue: feat(transcription): add "Alle als fertig markieren" bulk action to transcription panel (#345)

## Context PR #352 ships `markAllReviewed()` in `documents/[id]/+page.svelte` with a silent failure path: ```typescript if (!res.ok) return; ``` No error toast, no user feedback. The `@Transactional` guarantee means block states are unchanged in the database on failure (all-or-nothing), but the user receives no indication the operation failed. Raised by Felix (Developer) and Elicit (Requirements) in PR #352 Decision Queue (Item 4). This is the same pattern as `reviewToggle`, but silent failure is more disorienting for a document-wide bulk action than for a single block toggle. ## Acceptance criteria - [ ] When `PUT /transcription-blocks/review-all` returns a non-2xx response, a visible error message is shown to the user (toast or inline) - [ ] The error message is dismissible - [ ] Block state in the UI is unchanged (no optimistic update was applied) - [ ] Uses existing toast/notification infrastructure if one exists; otherwise add a minimal inline error display ## Links - Feature implementation: PR #352 - Original issue: #345

marcel added the P2-medium ui labels 2026-04-27 08:12:58 +02:00

marcel referenced this issue

2026-04-27 08:13:52 +02:00

feat(transcription): add "Alle als fertig markieren" bulk action (#345) #352

marcel referenced this issue

2026-04-27 08:14:09 +02:00

feat(transcription): add "Alle als fertig markieren" bulk action (#345) #352

marcel referenced this issue

2026-04-27 08:14:24 +02:00

feat(transcription): add "Alle als fertig markieren" bulk action (#345) #352

marcel referenced this issue

2026-04-27 08:14:44 +02:00

feat(transcription): add "Alle als fertig markieren" bulk action (#345) #352

marcel commented

2026-05-07 17:49:13 +02:00

👨‍💻 Markus Keller — Application Architect

Observations

The issue is well-scoped: a single missing failure path in a client-side function. No backend change is needed — the @Transactional guarantee is correctly noted.
No new infrastructure is warranted. The codebase already has a working inline error display pattern (ocrErrorMessage / ocrProgressMessage state variables rendered inline in the same component). There is no shared Toast component in src/lib/shared/primitives/, which confirms the right pattern here is inline, not a new cross-cutting toast system.
markAllReviewed lives in +page.svelte and hands state down to TranscriptionEditView. The callback chain is: +page.svelte:markAllReviewed → prop onMarkAllReviewed → TranscriptionEditView:handleMarkAllReviewed. Error state belongs in TranscriptionEditView (the component that owns the button and the markingAllReviewed guard state), not threaded up to the page.
The reviewToggle silent failure is a separate pre-existing issue. This issue correctly scopes itself to the bulk action only. Don't fix both in the same PR.

Recommendations

Add a let markAllError = $state<string | null>(null) reactive variable inside TranscriptionEditView.svelte. Set it in the catch branch of handleMarkAllReviewed, clear it on the next successful call.
Render the error inline below the review-progress bar — same pattern as ocrErrorMessage in +page.svelte. A role="alert" div with a dismiss button is sufficient and consistent with what already exists.
Do not introduce a new global toast store for this. A toast system is justified when errors need to survive navigation or stack across multiple concurrent operations. Neither applies here.
The onMarkAllReviewed prop currently has type () => Promise<void> — leave it as-is. Let the callback throw on failure (which markAllReviewed in +page.svelte currently does not), and catch in handleMarkAllReviewed. The fix belongs in +page.svelte:markAllReviewed: replace if (!res.ok) return; with if (!res.ok) throw new Error(...) so the error propagates to handleMarkAllReviewed's try/catch.

## 👨‍💻 Markus Keller — Application Architect ### Observations - The issue is well-scoped: a single missing failure path in a client-side function. No backend change is needed — the `@Transactional` guarantee is correctly noted. - No new infrastructure is warranted. The codebase already has a working inline error display pattern (`ocrErrorMessage` / `ocrProgressMessage` state variables rendered inline in the same component). There is no shared Toast component in `src/lib/shared/primitives/`, which confirms the right pattern here is inline, not a new cross-cutting toast system. - `markAllReviewed` lives in `+page.svelte` and hands state down to `TranscriptionEditView`. The callback chain is: `+page.svelte:markAllReviewed` → prop `onMarkAllReviewed` → `TranscriptionEditView:handleMarkAllReviewed`. Error state belongs in `TranscriptionEditView` (the component that owns the button and the `markingAllReviewed` guard state), not threaded up to the page. - The `reviewToggle` silent failure is a separate pre-existing issue. This issue correctly scopes itself to the bulk action only. Don't fix both in the same PR. ### Recommendations - Add a `let markAllError = $state<string | null>(null)` reactive variable inside `TranscriptionEditView.svelte`. Set it in the `catch` branch of `handleMarkAllReviewed`, clear it on the next successful call. - Render the error inline below the review-progress bar — same pattern as `ocrErrorMessage` in `+page.svelte`. A `role="alert"` div with a dismiss button is sufficient and consistent with what already exists. - Do not introduce a new global toast store for this. A toast system is justified when errors need to survive navigation or stack across multiple concurrent operations. Neither applies here. - The `onMarkAllReviewed` prop currently has type `() => Promise<void>` — leave it as-is. Let the callback throw on failure (which `markAllReviewed` in `+page.svelte` currently does not), and catch in `handleMarkAllReviewed`. The fix belongs in `+page.svelte:markAllReviewed`: replace `if (!res.ok) return;` with `if (!res.ok) throw new Error(...)` so the error propagates to `handleMarkAllReviewed`'s try/catch.

marcel commented

2026-05-07 17:49:35 +02:00

👨‍💻 Felix Brandt — Fullstack Developer

Observations

The current markAllReviewed function in +page.svelte (lines 155–165):

async function markAllReviewed() {
    const res = await fetch(`/api/documents/${doc.id}/transcription-blocks/review-all`, {
        method: 'PUT'
    });
    if (!res.ok) return;   // ← silent failure
    const updated = await res.json();
    for (const b of updated) {
        const existing = transcriptionBlocks.find((x) => x.id === b.id);
        if (existing) existing.reviewed = b.reviewed;
    }
}

Three concrete problems:

if (!res.ok) return swallows the failure — the handleMarkAllReviewed wrapper in TranscriptionEditView has a try/finally that correctly resets markingAllReviewed, but no catch to surface the error.
The button label "Alle als fertig markieren" is a hardcoded German string — not routed through Paraglide. This is pre-existing but worth fixing in the same pass.
No i18n key exists for the error message that needs to be shown. One must be added to messages/{de,en,es}.json.

The fix is three changes:

+page.svelte — throw on failure so the error propagates:

async function markAllReviewed() {
    const res = await fetch(`/api/documents/${doc.id}/transcription-blocks/review-all`, {
        method: 'PUT'
    });
    if (!res.ok) {
        const body = await res.json().catch(() => ({}));
        throw new Error(body?.code ?? 'INTERNAL_ERROR');
    }
    const updated = await res.json();
    for (const b of updated) {
        const existing = transcriptionBlocks.find((x) => x.id === b.id);
        if (existing) existing.reviewed = b.reviewed;
    }
}

TranscriptionEditView.svelte — add error state and render it:

let markAllError = $state<string | null>(null);

async function handleMarkAllReviewed() {
    if (!onMarkAllReviewed) return;
    markingAllReviewed = true;
    markAllError = null;
    try {
        await onMarkAllReviewed();
    } catch (e) {
        markAllError = getErrorMessage(e instanceof Error ? e.message : undefined);
    } finally {
        markingAllReviewed = false;
    }
}

Then render below the progress bar:

{#if markAllError}
    <div role="alert" class="mt-1 flex items-center gap-2 rounded-sm bg-red-50 px-3 py-2 font-sans text-xs text-red-700">
        <span class="flex-1">{markAllError}</span>
        <button onclick={() => (markAllError = null)} aria-label={m.dismiss()} class="shrink-0 hover:text-red-900">✕</button>
    </div>
{/if}

Recommendations

Fix the throw-vs-return in +page.svelte first — that is the root cause. The handleMarkAllReviewed wrapper already has try/finally in place; it just needs a catch branch.
Add getErrorMessage import to TranscriptionEditView.svelte — it already exists in $lib/shared/errors.
Add i18n key transcription_mark_all_reviewed_error to all three messages/*.json files. Use existing error string patterns as reference.
Use TDD: write a failing Vitest browser test for TranscriptionEditView that mocks onMarkAllReviewed to reject, then asserts the error text appears and the dismiss button clears it — before writing any production code.

## 👨‍💻 Felix Brandt — Fullstack Developer ### Observations The current `markAllReviewed` function in `+page.svelte` (lines 155–165): ```typescript async function markAllReviewed() { const res = await fetch(`/api/documents/${doc.id}/transcription-blocks/review-all`, { method: 'PUT' }); if (!res.ok) return; // ← silent failure const updated = await res.json(); for (const b of updated) { const existing = transcriptionBlocks.find((x) => x.id === b.id); if (existing) existing.reviewed = b.reviewed; } } ``` Three concrete problems: 1. `if (!res.ok) return` swallows the failure — the `handleMarkAllReviewed` wrapper in `TranscriptionEditView` has a `try/finally` that correctly resets `markingAllReviewed`, but no `catch` to surface the error. 2. The button label "Alle als fertig markieren" is a hardcoded German string — not routed through Paraglide. This is pre-existing but worth fixing in the same pass. 3. No i18n key exists for the error message that needs to be shown. One must be added to `messages/{de,en,es}.json`. The fix is three changes: **`+page.svelte`** — throw on failure so the error propagates: ```typescript async function markAllReviewed() { const res = await fetch(`/api/documents/${doc.id}/transcription-blocks/review-all`, { method: 'PUT' }); if (!res.ok) { const body = await res.json().catch(() => ({})); throw new Error(body?.code ?? 'INTERNAL_ERROR'); } const updated = await res.json(); for (const b of updated) { const existing = transcriptionBlocks.find((x) => x.id === b.id); if (existing) existing.reviewed = b.reviewed; } } ``` **`TranscriptionEditView.svelte`** — add error state and render it: ```svelte let markAllError = $state<string | null>(null); async function handleMarkAllReviewed() { if (!onMarkAllReviewed) return; markingAllReviewed = true; markAllError = null; try { await onMarkAllReviewed(); } catch (e) { markAllError = getErrorMessage(e instanceof Error ? e.message : undefined); } finally { markingAllReviewed = false; } } ``` Then render below the progress bar: ```svelte {#if markAllError} <div role="alert" class="mt-1 flex items-center gap-2 rounded-sm bg-red-50 px-3 py-2 font-sans text-xs text-red-700"> <span class="flex-1">{markAllError}</span> <button onclick={() => (markAllError = null)} aria-label={m.dismiss()} class="shrink-0 hover:text-red-900">✕</button> </div> {/if} ``` ### Recommendations - Fix the throw-vs-return in `+page.svelte` first — that is the root cause. The `handleMarkAllReviewed` wrapper already has `try/finally` in place; it just needs a `catch` branch. - Add `getErrorMessage` import to `TranscriptionEditView.svelte` — it already exists in `$lib/shared/errors`. - Add i18n key `transcription_mark_all_reviewed_error` to all three `messages/*.json` files. Use existing error string patterns as reference. - Use TDD: write a failing Vitest browser test for `TranscriptionEditView` that mocks `onMarkAllReviewed` to reject, then asserts the error text appears and the dismiss button clears it — before writing any production code.

marcel commented

2026-05-07 17:49:56 +02:00

👨‍💻 Tobias Wendt — DevOps & Platform Engineer

Observations

This issue is pure frontend — no infrastructure, no Docker, no CI pipeline change is required.
The PUT /transcription-blocks/review-all endpoint is already exercised by the backend test suite (TranscriptionBlockControllerTest, TranscriptionServiceTest). No new backend CI jobs are needed.
The acceptance criterion "uses existing toast/notification infrastructure if one exists" is correctly answered by codebase inspection: there is no shared toast component. The notificationStore / NotificationBell is for server-push notifications (SSE), not transient client-side error feedback. Using it for this would be misusing the wrong system.
No new Docker service, environment variable, or Compose change is implied by this fix.
Vitest browser tests already run in CI for frontend component tests. The new test for TranscriptionEditView will slot into the existing npm run test job without additional CI configuration.

Recommendations

No infrastructure action needed for this issue. Ship it as a pure frontend code change.
Confirm the fix does not regress the existing TranscriptionEditView.svelte.spec.ts — run npm run test with the Vitest browser runner before opening the PR.

## 👨‍💻 Tobias Wendt — DevOps & Platform Engineer ### Observations - This issue is pure frontend — no infrastructure, no Docker, no CI pipeline change is required. - The `PUT /transcription-blocks/review-all` endpoint is already exercised by the backend test suite (`TranscriptionBlockControllerTest`, `TranscriptionServiceTest`). No new backend CI jobs are needed. - The acceptance criterion "uses existing toast/notification infrastructure if one exists" is correctly answered by codebase inspection: there is no shared toast component. The `notificationStore` / `NotificationBell` is for server-push notifications (SSE), not transient client-side error feedback. Using it for this would be misusing the wrong system. - No new Docker service, environment variable, or Compose change is implied by this fix. - Vitest browser tests already run in CI for frontend component tests. The new test for `TranscriptionEditView` will slot into the existing `npm run test` job without additional CI configuration. ### Recommendations - No infrastructure action needed for this issue. Ship it as a pure frontend code change. - Confirm the fix does not regress the existing `TranscriptionEditView.svelte.spec.ts` — run `npm run test` with the Vitest browser runner before opening the PR.

marcel commented

2026-05-07 17:50:14 +02:00

👨‍💻 Elicit — Requirements Engineer

Observations

The acceptance criteria are clear and testable. Mapping them to EARS-style requirements for completeness:

REQ-TRANSC-001: When PUT /transcription-blocks/review-all returns a non-2xx response, the system shall display a visible, dismissible error message to the user within the transcription panel.
REQ-TRANSC-002: While the error message is displayed, the transcription block review states in the UI shall remain unchanged from their pre-request values (no optimistic update was applied — the issue body confirms this is already the case).
REQ-TRANSC-003: The system shall use existing UI infrastructure (inline error display) for the error message; no new global toast or notification channel shall be introduced.

Two gaps I spotted in the current acceptance criteria:

The AC says "uses existing toast/notification infrastructure if one exists; otherwise add a minimal inline error display." Codebase inspection shows no toast component exists. The AC branch condition is resolved: the implementation must use inline display. The AC should be tightened to "inline error display within the transcription panel" to remove the ambiguity.
The error message must be internationalized (de/en/es). This NFR is implied by the project's Paraglide setup but is not stated in the AC. It should be added as an explicit criterion.

The reviewToggle silent failure is explicitly deferred. The note in the issue body is the right call — mixing them would widen scope.

Recommendations

Add one AC: "The error message is rendered in the transcription panel header area, below the review progress bar, using the project's Paraglide i18n system — not a raw string."
Update the existing AC: "Uses inline error display within the transcription panel — no new toast system is required."
The issue is well-linked (PR #352, #345). It is ready to implement as-is once the two AC refinements above are noted.

Open Decisions

Scope of the hardcoded string "Alle als fertig markieren": The button label is currently a hardcoded German string, not routed through Paraglide. Is fixing this in-scope for this issue, or should it be a separate chore ticket? It is pre-existing, but the implementer will be editing the same button element — the practical cost of fixing it here is near-zero.

## 👨‍💻 Elicit — Requirements Engineer ### Observations The acceptance criteria are clear and testable. Mapping them to EARS-style requirements for completeness: - **REQ-TRANSC-001**: When `PUT /transcription-blocks/review-all` returns a non-2xx response, the system shall display a visible, dismissible error message to the user within the transcription panel. - **REQ-TRANSC-002**: While the error message is displayed, the transcription block review states in the UI shall remain unchanged from their pre-request values (no optimistic update was applied — the issue body confirms this is already the case). - **REQ-TRANSC-003**: The system shall use existing UI infrastructure (inline error display) for the error message; no new global toast or notification channel shall be introduced. Two gaps I spotted in the current acceptance criteria: 1. The AC says "uses existing toast/notification infrastructure if one exists; otherwise add a minimal inline error display." Codebase inspection shows no toast component exists. The AC branch condition is resolved: the implementation must use inline display. The AC should be tightened to "inline error display within the transcription panel" to remove the ambiguity. 2. The error message must be internationalized (de/en/es). This NFR is implied by the project's Paraglide setup but is not stated in the AC. It should be added as an explicit criterion. The `reviewToggle` silent failure is explicitly deferred. The note in the issue body is the right call — mixing them would widen scope. ### Recommendations - Add one AC: "The error message is rendered in the transcription panel header area, below the review progress bar, using the project's Paraglide i18n system — not a raw string." - Update the existing AC: "Uses inline error display within the transcription panel — no new toast system is required." - The issue is well-linked (PR #352, #345). It is ready to implement as-is once the two AC refinements above are noted. ### Open Decisions - **Scope of the hardcoded string "Alle als fertig markieren"**: The button label is currently a hardcoded German string, not routed through Paraglide. Is fixing this in-scope for this issue, or should it be a separate chore ticket? It is pre-existing, but the implementer will be editing the same button element — the practical cost of fixing it here is near-zero.

marcel commented

2026-05-07 17:50:28 +02:00

👨‍💻 Nora "NullX" Steiner — Security Engineer

Observations

No new attack surface is introduced by this fix. The endpoint already exists and is protected by @RequirePermission(Permission.WRITE_ALL).
The silent failure path (if (!res.ok) return) is a UX problem, not a security vulnerability — block states are not modified on failure, so there is no inconsistent-write risk.
One security-adjacent concern: when parsing the error body from the failed response to extract an error code, the code must not reflect raw backend error strings into the DOM. The codebase has getErrorMessage(code) in $lib/shared/errors.ts which correctly maps ErrorCode values to i18n strings. This mapping must be used — not body.message or any raw backend string.

The proposed pattern (Felix's review) correctly routes through getErrorMessage(). The only risk is if someone takes a shortcut and renders body.message directly:

<!-- WRONG — leaks backend internals -->
<p>{errorBody.message}</p>

<!-- CORRECT — maps to user-safe i18n string -->
<p>{getErrorMessage(errorBody.code)}</p>

The dismiss button does not require any special security consideration — it only clears local reactive state.
No CSRF concern: this is a credentialed same-origin fetch call, consistent with all other write operations in this page.

Recommendations

In the error-parsing code, extract only body.code (typed as ErrorCode | string), never body.message. Pass the code to getErrorMessage(). This is already the established pattern in the codebase.
Add a code comment at the error-handling site: // Never render body.message — route through getErrorMessage() to prevent leaking backend internals. One comment here is worth it because this exact mistake is easy to make under time pressure.

## 👨‍💻 Nora "NullX" Steiner — Security Engineer ### Observations - No new attack surface is introduced by this fix. The endpoint already exists and is protected by `@RequirePermission(Permission.WRITE_ALL)`. - The silent failure path (`if (!res.ok) return`) is a UX problem, not a security vulnerability — block states are not modified on failure, so there is no inconsistent-write risk. - One security-adjacent concern: when parsing the error body from the failed response to extract an error code, the code must not reflect raw backend error strings into the DOM. The codebase has `getErrorMessage(code)` in `$lib/shared/errors.ts` which correctly maps `ErrorCode` values to i18n strings. This mapping **must** be used — not `body.message` or any raw backend string. The proposed pattern (Felix's review) correctly routes through `getErrorMessage()`. The only risk is if someone takes a shortcut and renders `body.message` directly: ```svelte  <p>{errorBody.message}</p>  <p>{getErrorMessage(errorBody.code)}</p> ``` - The dismiss button does not require any special security consideration — it only clears local reactive state. - No CSRF concern: this is a credentialed same-origin `fetch` call, consistent with all other write operations in this page. ### Recommendations - In the error-parsing code, extract only `body.code` (typed as `ErrorCode | string`), never `body.message`. Pass the code to `getErrorMessage()`. This is already the established pattern in the codebase. - Add a code comment at the error-handling site: `// Never render body.message — route through getErrorMessage() to prevent leaking backend internals.` One comment here is worth it because this exact mistake is easy to make under time pressure.

marcel commented

2026-05-07 17:50:40 +02:00

👨‍💻 Sara Holt — QA Engineer & Test Strategist

Observations

Current test coverage gap:

Backend: TranscriptionBlockControllerTest and TranscriptionServiceTest both test the review-all endpoint thoroughly (401, 403, 200, empty list, idempotent, audit events). Backend is well covered.
Frontend: No test in TranscriptionEditView.svelte.spec.ts covers the error path of onMarkAllReviewed. The happy path and the markingAllReviewed spinner state may be tested, but a search across the spec files shows zero tests for the error case.

Test plan for this fix (all three layers):

Unit/component (Vitest browser — TranscriptionEditView.svelte.spec.ts):

it('shows error message when markAllReviewed callback rejects') — mock onMarkAllReviewed to return Promise.reject(new Error('INTERNAL_ERROR')), assert role="alert" is visible with a non-empty message.
it('clears error message when dismiss button is clicked') — after error appears, click dismiss button, assert alert is gone.
it('clears error message on next successful markAllReviewed call') — error shown, then mock resolves successfully, assert error is gone.
it('block states are unchanged after markAllReviewed failure') — this is guaranteed by the lack of optimistic update, but a regression test is worth having.

Server/load function (Vitest Node — +page.server.spec.ts if it exists):

No server-side load function change is implied; this is purely client-side. No server test needed.

E2E (Playwright — not required for this issue):

The error scenario requires a forced 5xx response, which would need request interception. Not worth the complexity for a P2 item. Component test coverage is sufficient.

Recommendations

Write all four component tests above before writing any production code (TDD red phase).
Use vi.fn() returning Promise.reject(...) for the error mock — do not stub fetch directly, as the error is propagated from the page's markAllReviewed via the prop.
Name the tests as sentences: shows error message when markAllReviewed callback rejects �� not testMarkAllReviewedError.
Use getByRole('alert') for the error div assertion, which also validates the role="alert" attribute is present (required for screen reader announcement).
The markingAllReviewed spinner correctly resets in the finally block — add one assertion confirming the button is re-enabled after the error, so that regression is caught.

## 👨‍💻 Sara Holt — QA Engineer & Test Strategist ### Observations **Current test coverage gap:** - Backend: `TranscriptionBlockControllerTest` and `TranscriptionServiceTest` both test the `review-all` endpoint thoroughly (401, 403, 200, empty list, idempotent, audit events). Backend is well covered. - Frontend: No test in `TranscriptionEditView.svelte.spec.ts` covers the error path of `onMarkAllReviewed`. The happy path and the `markingAllReviewed` spinner state may be tested, but a search across the spec files shows zero tests for the error case. **Test plan for this fix (all three layers):** Unit/component (Vitest browser — `TranscriptionEditView.svelte.spec.ts`): 1. `it('shows error message when markAllReviewed callback rejects')` — mock `onMarkAllReviewed` to return `Promise.reject(new Error('INTERNAL_ERROR'))`, assert `role="alert"` is visible with a non-empty message. 2. `it('clears error message when dismiss button is clicked')` — after error appears, click dismiss button, assert alert is gone. 3. `it('clears error message on next successful markAllReviewed call')` — error shown, then mock resolves successfully, assert error is gone. 4. `it('block states are unchanged after markAllReviewed failure')` — this is guaranteed by the lack of optimistic update, but a regression test is worth having. Server/load function (Vitest Node — `+page.server.spec.ts` if it exists): - No server-side load function change is implied; this is purely client-side. No server test needed. E2E (Playwright — not required for this issue): - The error scenario requires a forced 5xx response, which would need request interception. Not worth the complexity for a P2 item. Component test coverage is sufficient. ### Recommendations - Write all four component tests above before writing any production code (TDD red phase). - Use `vi.fn()` returning `Promise.reject(...)` for the error mock — do not stub fetch directly, as the error is propagated from the page's `markAllReviewed` via the prop. - Name the tests as sentences: `shows error message when markAllReviewed callback rejects` �� not `testMarkAllReviewedError`. - Use `getByRole('alert')` for the error div assertion, which also validates the `role="alert"` attribute is present (required for screen reader announcement). - The `markingAllReviewed` spinner correctly resets in the `finally` block — add one assertion confirming the button is re-enabled after the error, so that regression is caught.

marcel commented

2026-05-07 17:50:55 +02:00

👨‍💻 Leonie Voss — UI/UX Design Lead

Observations

The current handleMarkAllReviewed shows a loading spinner during the request (correct), but shows nothing at all if it fails. For the 60+ audience who use this transcription workflow on tablets, silent failure after a bulk action is particularly disorienting — they pressed a button, waited, nothing changed, and no feedback appeared.

The existing review-progress area in TranscriptionEditView.svelte (the sticky header with the "X / Y geprüft" counter and the "Alle als fertig markieren" button) is the natural home for the inline error. It is already visible at the top of the transcription panel, scrolling is not required to see it.

Two existing issues I noticed while reviewing the button:

The button label "Alle als fertig markieren" is a hardcoded German string — not routed through Paraglide. This needs fixing separately (pre-existing), but the implementer will touch this element.
The title attribute tooltip 'Alle Blöcke sind bereits als fertig markiert' (shown when disabled) is also a hardcoded German string.

For the error display itself:

Use role="alert" so screen readers announce the message immediately without user focus.
Use aria-live="polite" as a belt-and-suspenders addition — some screen reader + browser combinations handle role="alert" inconsistently.
Provide a manual dismiss button with aria-label (not just an × character) — auto-dismissing is a WCAG failure for the senior audience.
The dismiss button touch target must be at least 44×44px — use class="min-h-[44px] min-w-[44px]".
Use brand-consistent error styling: bg-red-50 text-red-700 is acceptable as it already appears in the OCR error pattern at line 492 of +page.svelte. Do not add a new color pattern.
Keep the error message to one line if possible — the transcription panel is narrow on tablet viewports.

Recommendations

Concrete Tailwind classes for the error container:

<div
    role="alert"
    aria-live="polite"
    class="mt-1.5 flex items-center gap-2 rounded-sm border border-red-200 bg-red-50 px-3 py-2 font-sans text-xs text-red-700"
>
    <span class="flex-1">{markAllError}</span>
    <button
        onclick={() => (markAllError = null)}
        aria-label={m.dismiss()}
        class="flex min-h-[44px] min-w-[44px] items-center justify-center rounded text-red-500 hover:text-red-700 focus-visible:ring-2 focus-visible:ring-red-500"
    >
        <svg class="h-4 w-4" ...><!-- X icon --></svg>
    </button>
</div>

Place this directly below the progress bar <div>, still inside the sticky header — no scroll required to see it.
Verify the error banner does not cause the sticky header to grow so tall it obscures the first transcription block. On a 768px tablet in landscape, the sticky area is already ~60px; an error banner adds ~44px. Acceptable.

Open Decisions

Auto-dismiss timeout: A 5–8 second auto-dismiss (with a manual dismiss button as well) is a common pattern for transient errors. For this audience and this action (a bulk write that failed), a persistent-until-dismissed approach is safer. However, if the error persists forever, a retry of the bulk action would leave two error banners or need the first cleared automatically. Is persistent-until-dismissed the right choice, or should the next successful markAllReviewed clear the error (which it should do regardless)?

## 👨‍💻 Leonie Voss — UI/UX Design Lead ### Observations The current `handleMarkAllReviewed` shows a loading spinner during the request (correct), but shows nothing at all if it fails. For the 60+ audience who use this transcription workflow on tablets, silent failure after a bulk action is particularly disorienting — they pressed a button, waited, nothing changed, and no feedback appeared. The existing review-progress area in `TranscriptionEditView.svelte` (the sticky header with the "X / Y geprüft" counter and the "Alle als fertig markieren" button) is the natural home for the inline error. It is already visible at the top of the transcription panel, scrolling is not required to see it. **Two existing issues I noticed while reviewing the button:** 1. The button label `"Alle als fertig markieren"` is a hardcoded German string — not routed through Paraglide. This needs fixing separately (pre-existing), but the implementer will touch this element. 2. The `title` attribute tooltip `'Alle Blöcke sind bereits als fertig markiert'` (shown when disabled) is also a hardcoded German string. **For the error display itself:** - Use `role="alert"` so screen readers announce the message immediately without user focus. - Use `aria-live="polite"` as a belt-and-suspenders addition — some screen reader + browser combinations handle `role="alert"` inconsistently. - Provide a manual dismiss button with `aria-label` (not just an × character) — auto-dismissing is a WCAG failure for the senior audience. - The dismiss button touch target must be at least 44×44px — use `class="min-h-[44px] min-w-[44px]"`. - Use brand-consistent error styling: `bg-red-50 text-red-700` is acceptable as it already appears in the OCR error pattern at line 492 of `+page.svelte`. Do not add a new color pattern. - Keep the error message to one line if possible — the transcription panel is narrow on tablet viewports. ### Recommendations Concrete Tailwind classes for the error container: ```svelte <div role="alert" aria-live="polite" class="mt-1.5 flex items-center gap-2 rounded-sm border border-red-200 bg-red-50 px-3 py-2 font-sans text-xs text-red-700" > <span class="flex-1">{markAllError}</span> <button onclick={() => (markAllError = null)} aria-label={m.dismiss()} class="flex min-h-[44px] min-w-[44px] items-center justify-center rounded text-red-500 hover:text-red-700 focus-visible:ring-2 focus-visible:ring-red-500" > <svg class="h-4 w-4" ...></svg> </button> </div> ``` - Place this directly below the progress bar `<div>`, still inside the sticky header — no scroll required to see it. - Verify the error banner does not cause the sticky header to grow so tall it obscures the first transcription block. On a 768px tablet in landscape, the sticky area is already ~60px; an error banner adds ~44px. Acceptable. ### Open Decisions - **Auto-dismiss timeout**: A 5–8 second auto-dismiss (with a manual dismiss button as well) is a common pattern for transient errors. For this audience and this action (a bulk write that failed), a persistent-until-dismissed approach is safer. However, if the error persists forever, a retry of the bulk action would leave two error banners or need the first cleared automatically. Is persistent-until-dismissed the right choice, or should the next successful `markAllReviewed` clear the error (which it should do regardless)?

marcel commented

2026-05-07 17:51:15 +02:00

Decision Queue

Two open decisions raised by the personas, grouped by theme.

Theme 1: Scope of pre-existing hardcoded strings

Raised by: Elicit, Leonie, Felix

Both the button label ("Alle als fertig markieren") and its disabled-state tooltip ("Alle Blöcke sind bereits als fertig markiert") are hardcoded German strings in TranscriptionEditView.svelte. They are pre-existing issues, but the implementer will be editing the same button element as part of this fix.

Decision needed: Fix the hardcoded strings as part of this PR (near-zero extra cost since the element is already being touched), or defer to a separate i18n chore ticket?

The recommendation from all three personas: fix in the same PR. The counter-argument for deferring is keeping this PR strictly scoped to the error feedback behavior.

Theme 2: Error persistence — auto-dismiss vs. persistent-until-dismissed

Raised by: Leonie

The error banner could:

A) Persistent until dismissed (manual dismiss button only) — safer for the senior audience, guaranteed to be seen. Risk: if the user retries markAllReviewed without dismissing, the old error persists until the retry either succeeds (clears it) or fails (replaces it). This is handled cleanly by markAllError = null at the start of handleMarkAllReviewed.
B) Auto-dismiss after 6–8 seconds, with a manual dismiss button too — common UX pattern, less cluttered. Risk: seniors may miss the message if they are not watching the panel in the moment the error appears.

The issue AC says "the error message is dismissible" — it does not prescribe persistence behavior. Option A is the safer default and requires no timer logic. Recommendation from the personas: go with persistent-until-dismissed (Option A).

Decision needed: Confirm Option A (persistent until manually dismissed or until next action clears it) is acceptable, or specify an auto-dismiss timeout.

## Decision Queue Two open decisions raised by the personas, grouped by theme. --- ### Theme 1: Scope of pre-existing hardcoded strings **Raised by:** Elicit, Leonie, Felix Both the button label ("Alle als fertig markieren") and its disabled-state tooltip ("Alle Blöcke sind bereits als fertig markiert") are hardcoded German strings in `TranscriptionEditView.svelte`. They are pre-existing issues, but the implementer will be editing the same button element as part of this fix. **Decision needed:** Fix the hardcoded strings as part of this PR (near-zero extra cost since the element is already being touched), or defer to a separate i18n chore ticket? The recommendation from all three personas: fix in the same PR. The counter-argument for deferring is keeping this PR strictly scoped to the error feedback behavior. --- ### Theme 2: Error persistence — auto-dismiss vs. persistent-until-dismissed **Raised by:** Leonie The error banner could: - **A) Persistent until dismissed** (manual dismiss button only) — safer for the senior audience, guaranteed to be seen. Risk: if the user retries `markAllReviewed` without dismissing, the old error persists until the retry either succeeds (clears it) or fails (replaces it). This is handled cleanly by `markAllError = null` at the start of `handleMarkAllReviewed`. - **B) Auto-dismiss after 6–8 seconds, with a manual dismiss button too** — common UX pattern, less cluttered. Risk: seniors may miss the message if they are not watching the panel in the moment the error appears. The issue AC says "the error message is dismissible" — it does not prescribe persistence behavior. Option A is the safer default and requires no timer logic. Recommendation from the personas: go with persistent-until-dismissed (Option A). **Decision needed:** Confirm Option A (persistent until manually dismissed or until next action clears it) is acceptable, or specify an auto-dismiss timeout.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: marcel/familienarchiv#356