ui(admin/system): improve mass-import status card (loading state, i18n, font size) #533

New Issue

marcel · 2026-05-11T20:14:08+02:00

marcel commented

2026-05-11 20:14:08 +02:00

Type: UI/UX improvement
Priority: P1-high — must land before the first real prod import campaign
Source: review of #526 by Leonie Voss (UX) — comment #8650
Parent PR: #526 (mass-import bind mount → admin card now actually does something on prod)

Summary

Three discrete UX fixes to the mass-import card at /admin/system: (1) a real loading state while RUNNING, (2) i18n via a structured status code instead of a verbatim German backend string, (3) a senior-audience-readable font size for the progress counts.

User Story

As Marcel (operator, primary admin user), I want the mass-import card on /admin/system to show clear loading progress in my language at a readable font size, so that during a real prod import campaign I know the system is working, I can run the UI in English/Spanish for non-German family members watching, and I don't have to squint at small text.

Context

With #526 merged, /admin/system's mass-import card actually does something on staging/prod for the first time. Leonie flagged three UX concerns on the card itself that were out of scope for the infra PR but should land before the first real prod import.

Required

Real loading state during RUNNING — the card currently shows state from /api/admin/import-status as plain text. While state === 'RUNNING', render a spinner or progress indicator alongside the processed count, not just the stale-looking word "RUNNING". Auto-poll every 2–5s while running.
i18n the status messages — ImportStatus.message is currently a German String baked into MassImportService ("Import läuft...", "Import abgeschlossen. N Dokumente verarbeitet.", etc.). The frontend should not render the backend's localized string verbatim — instead, the backend should return an ErrorCode-style enum (e.g. IMPORT_RUNNING, IMPORT_DONE, IMPORT_FAILED_NO_SPREADSHEET), and the frontend maps via Paraglide (messages/{de,en,es}.json). This also helps debugging in non-German locales.
Font size for the senior audience — progress counts must be at least text-base (16px). Currently they're text-xs/text-sm per Leonie's quick check. Use the project's standard text-base for the count and text-xs uppercase tracking-widest only for the label row.

Acceptance criteria

Given an import is in progress (state === 'RUNNING'),
when the operator views /admin/system,
then a spinner or progress indicator is visible alongside the live processed count, and the card auto-polls every 2–5s.

Given the user has selected en or es as their UI language,
when the import status card renders,
then all status text is localized via Paraglide and no raw German backend string is shown.

Given any state,
when the card renders the processed / skipped counts,
then the numeric values are rendered at ≥ text-base (16px); only the section label row may use text-xs.

Checklist

vitest-browser test asserts the loading indicator is visible while state === 'RUNNING'
Backend returns a structured status code, not a German string; frontend translates via Paraglide
All three locales (de, en, es) have keys for every status code
axe-playwright check on /admin/system passes in light AND dark mode
Visual regression diff at 320 / 768 / 1440 px

Linked NFRs

Usability (senior audience): Numeric values in admin cards MUST be ≥ 16px.
i18n: No backend-localized string MAY be rendered verbatim in the UI; translation MUST happen frontend-side via Paraglide.
Accessibility: axe-playwright clean in light + dark mode.
Responsiveness: Card MUST render correctly at 320 / 768 / 1440 px breakpoints.

Backend status structure change touches MassImportService.ImportStatus — coordinate with the operator-policy follow-up #534 and the skipped-counter additions from #529 / #530.

Definition of Ready

User story explicit
Three discrete fixes scoped
Acceptance criteria in Given-When-Then
NFR thresholds named (text-base, axe-clean, three breakpoints)

🤖 Generated with Claude Code during /implement on #526

**Type:** UI/UX improvement **Priority:** P1-high — must land before the first real prod import campaign **Source:** review of #526 by Leonie Voss (UX) — comment [#8650](https://git.raddatz.cloud/marcel/familienarchiv/pulls/526#issuecomment-8650) **Parent PR:** #526 (mass-import bind mount → admin card now actually does something on prod) ## Summary Three discrete UX fixes to the mass-import card at `/admin/system`: (1) a real loading state while `RUNNING`, (2) i18n via a structured status code instead of a verbatim German backend string, (3) a senior-audience-readable font size for the progress counts. ## User Story > As Marcel (operator, primary admin user), I want the mass-import card on `/admin/system` to show clear loading progress in my language at a readable font size, so that during a real prod import campaign I know the system is working, I can run the UI in English/Spanish for non-German family members watching, and I don't have to squint at small text. ## Context With #526 merged, `/admin/system`'s mass-import card actually *does* something on staging/prod for the first time. Leonie flagged three UX concerns on the card itself that were out of scope for the infra PR but should land before the first real prod import. ## Required 1. **Real loading state during `RUNNING`** — the card currently shows `state` from `/api/admin/import-status` as plain text. While `state === 'RUNNING'`, render a spinner or progress indicator alongside the `processed` count, not just the stale-looking word "RUNNING". Auto-poll every 2–5s while running. 2. **i18n the status messages** — `ImportStatus.message` is currently a German `String` baked into `MassImportService` (`"Import läuft..."`, `"Import abgeschlossen. N Dokumente verarbeitet."`, etc.). The frontend should not render the backend's localized string verbatim — instead, the backend should return an `ErrorCode`-style enum (e.g. `IMPORT_RUNNING`, `IMPORT_DONE`, `IMPORT_FAILED_NO_SPREADSHEET`), and the frontend maps via Paraglide (`messages/{de,en,es}.json`). This also helps debugging in non-German locales. 3. **Font size for the senior audience** — progress counts must be at least `text-base` (16px). Currently they're `text-xs`/`text-sm` per Leonie's quick check. Use the project's standard `text-base` for the count and `text-xs uppercase tracking-widest` only for the *label* row. ## Acceptance criteria **Given** an import is in progress (`state === 'RUNNING'`), **when** the operator views `/admin/system`, **then** a spinner or progress indicator is visible alongside the live `processed` count, and the card auto-polls every 2–5s. **Given** the user has selected `en` or `es` as their UI language, **when** the import status card renders, **then** all status text is localized via Paraglide and no raw German backend string is shown. **Given** any state, **when** the card renders the `processed` / `skipped` counts, **then** the numeric values are rendered at ≥ `text-base` (16px); only the section label row may use `text-xs`. ### Checklist - [ ] vitest-browser test asserts the loading indicator is visible while state === 'RUNNING' - [ ] Backend returns a structured status code, not a German string; frontend translates via Paraglide - [ ] All three locales (`de`, `en`, `es`) have keys for every status code - [ ] axe-playwright check on `/admin/system` passes in light AND dark mode - [ ] Visual regression diff at 320 / 768 / 1440 px ## Linked NFRs - **Usability (senior audience):** Numeric values in admin cards MUST be ≥ 16px. - **i18n:** No backend-localized string MAY be rendered verbatim in the UI; translation MUST happen frontend-side via Paraglide. - **Accessibility:** axe-playwright clean in light + dark mode. - **Responsiveness:** Card MUST render correctly at 320 / 768 / 1440 px breakpoints. ## Related - Backend status structure change touches `MassImportService.ImportStatus` — coordinate with the operator-policy follow-up #534 and the skipped-counter additions from #529 / #530. ## Definition of Ready - [x] User story explicit - [x] Three discrete fixes scoped - [x] Acceptance criteria in Given-When-Then - [x] NFR thresholds named (`text-base`, axe-clean, three breakpoints) 🤖 Generated with [Claude Code](https://claude.com/claude-code) during /implement on #526

marcel referenced this issue

2026-05-11 20:14:42 +02:00

feat(infra): bind-mount /import for backend mass-import endpoint #526

marcel referenced this issue

2026-05-11 20:35:36 +02:00

security(import): validate PDF magic bytes in MassImportService before S3 upload #529

marcel added the P1-high ui labels 2026-05-11 20:36:43 +02:00

marcel commented

2026-05-14 11:34:32 +02:00

👨‍💻 Felix Brandt — Senior Fullstack Developer

Observations

The i18n is 75% done already. messages/{de,en,es}.json already has admin_system_import_status_idle, admin_system_import_status_running, and admin_system_import_status_done (with {count} param). The only gap is the FAILED state: admin_system_import_status_failed uses {message} as a Paraglide parameter, so the backend's German string leaks through for en/es locales. The structural work is adding granular failure status codes so the frontend can drop the {message} interpolation entirely.
The polling loop already exists. The frontend already polls /api/admin/import-status every 2s while RUNNING (page.svelte:179–220). Task 1 (loading state) is narrower than it sounds — only the visual spinner is missing.
Spinner hook: isRunning must be a $derived — const isRunning = $derived(importStatus?.state === 'RUNNING') — not a $state + $effect pair. The existing {#if isRunning} block is the correct insertion point for the spinner markup.
Backend ImportStatus record (MassImportService.java:55) carries String message — a localization concern in the service layer. The fix adds a structured ImportStatusCode enum field; the message field becomes debug-only and must not be rendered by the frontend.
After any backend change: run npm run generate:api before writing frontend code — the TypeScript types are auto-generated and the field won't exist in the client until then.
Font sizes: The card currently uses text-xs/text-sm for numeric counts. The fix matches the CLAUDE.md card pattern exactly: text-base font-serif for the value, text-xs font-bold uppercase tracking-widest text-ink-3 only for the label row.

Recommendations

Backend: Add an ImportStatusCode enum with values IDLE, RUNNING, DONE, FAILED_NO_SPREADSHEET, FAILED_GENERIC. Add it as a @Schema(requiredMode = REQUIRED) field on the ImportStatus record. Mark the existing message field @Schema(requiredMode = NOT_REQUIRED) — it stays for debugging, but the OpenAPI contract no longer guarantees it. Audit every catch block in runImportAsync() to confirm which failure paths map to which code before writing new i18n keys.
Frontend status mapping: Create a getImportStatusMessage(code: ImportStatusCode, processed?: number): string helper (pattern: errors.ts → getErrorMessage()). Each code maps to exactly one Paraglide call. The FAILED codes get their own keys (admin_system_import_status_failed_no_spreadsheet, admin_system_import_status_failed_generic) — the {message} interpolation disappears.

Spinner markup — accessible pattern:

{#if isRunning}
  <div role="status" aria-label={m.admin_system_import_status_running()}>
    <svg class="animate-spin h-5 w-5 text-brand-navy" xmlns="http://www.w3.org/2000/svg" fill="none" viewBox="0 0 24 24">
      <circle class="opacity-25" cx="12" cy="12" r="10" stroke="currentColor" stroke-width="4"/>
      <path class="opacity-75" fill="currentColor" d="M4 12a8 8 0 018-8V0C5.373 0 0 5.373 0 12h4z"/>
    </svg>
    <span class="sr-only">{m.admin_system_import_status_running()}</span>
  </div>
{/if}

role="status" announces the state to screen readers. sr-only gives a verbal label to the spinner. Use text-brand-navy so it respects the semantic token in both light and dark mode.

TDD order:
1. Update the existing RUNNING-state vitest-browser test to assert getByRole('status') is visible — watch it fail.
2. Add the spinner; go green.
3. Add failing tests for each ImportStatusCode → i18n mapping (including FAILED subcodes).
4. Update backend, run generate:api, implement mapping; go green.
5. Font-size change: assert count element has text-base class — no separate failing test needed if done as part of the same cycle.

## 👨‍💻 Felix Brandt — Senior Fullstack Developer ### Observations - **The i18n is 75% done already.** `messages/{de,en,es}.json` already has `admin_system_import_status_idle`, `admin_system_import_status_running`, and `admin_system_import_status_done` (with `{count}` param). The only gap is the FAILED state: `admin_system_import_status_failed` uses `{message}` as a Paraglide parameter, so the backend's German string leaks through for `en`/`es` locales. The structural work is adding granular failure status codes so the frontend can drop the `{message}` interpolation entirely. - **The polling loop already exists.** The frontend already polls `/api/admin/import-status` every 2s while RUNNING (`page.svelte:179–220`). Task 1 (loading state) is narrower than it sounds — only the visual spinner is missing. - **Spinner hook**: `isRunning` must be a `$derived` — `const isRunning = $derived(importStatus?.state === 'RUNNING')` — not a `$state` + `$effect` pair. The existing `{#if isRunning}` block is the correct insertion point for the spinner markup. - **Backend `ImportStatus` record** (`MassImportService.java:55`) carries `String message` — a localization concern in the service layer. The fix adds a structured `ImportStatusCode` enum field; the `message` field becomes debug-only and must not be rendered by the frontend. - **After any backend change**: run `npm run generate:api` before writing frontend code — the TypeScript types are auto-generated and the field won't exist in the client until then. - **Font sizes**: The card currently uses `text-xs`/`text-sm` for numeric counts. The fix matches the CLAUDE.md card pattern exactly: `text-base font-serif` for the value, `text-xs font-bold uppercase tracking-widest text-ink-3` only for the label row. ### Recommendations - **Backend**: Add an `ImportStatusCode` enum with values `IDLE`, `RUNNING`, `DONE`, `FAILED_NO_SPREADSHEET`, `FAILED_GENERIC`. Add it as a `@Schema(requiredMode = REQUIRED)` field on the `ImportStatus` record. Mark the existing `message` field `@Schema(requiredMode = NOT_REQUIRED)` — it stays for debugging, but the OpenAPI contract no longer guarantees it. Audit every `catch` block in `runImportAsync()` to confirm which failure paths map to which code before writing new i18n keys. - **Frontend status mapping**: Create a `getImportStatusMessage(code: ImportStatusCode, processed?: number): string` helper (pattern: `errors.ts` → `getErrorMessage()`). Each code maps to exactly one Paraglide call. The FAILED codes get their own keys (`admin_system_import_status_failed_no_spreadsheet`, `admin_system_import_status_failed_generic`) — the `{message}` interpolation disappears. - **Spinner markup** — accessible pattern: ```svelte {#if isRunning} <div role="status" aria-label={m.admin_system_import_status_running()}> <svg class="animate-spin h-5 w-5 text-brand-navy" xmlns="http://www.w3.org/2000/svg" fill="none" viewBox="0 0 24 24"> <circle class="opacity-25" cx="12" cy="12" r="10" stroke="currentColor" stroke-width="4"/> <path class="opacity-75" fill="currentColor" d="M4 12a8 8 0 018-8V0C5.373 0 0 5.373 0 12h4z"/> </svg> <span class="sr-only">{m.admin_system_import_status_running()}</span> </div> {/if} ``` `role="status"` announces the state to screen readers. `sr-only` gives a verbal label to the spinner. Use `text-brand-navy` so it respects the semantic token in both light and dark mode. - **TDD order**: 1. Update the existing RUNNING-state vitest-browser test to assert `getByRole('status')` is visible — watch it fail. 2. Add the spinner; go green. 3. Add failing tests for each `ImportStatusCode` → i18n mapping (including FAILED subcodes). 4. Update backend, run `generate:api`, implement mapping; go green. 5. Font-size change: assert count element has `text-base` class — no separate failing test needed if done as part of the same cycle.

marcel commented

2026-05-14 11:34:49 +02:00

🏛️ Markus Keller — Application Architect

Observations

Domain boundary violation in the current design. The message: String field on ImportStatus (MassImportService.java:55) is a localization artifact baked into the service layer. Localization is a presentation concern — it should never originate in the domain. This issue fixes a real architectural smell, not just a UX polish item.
Two design options for the API change:
- Option A: Expand the existing State enum with granular failure codes (IDLE, RUNNING, DONE, FAILED_NO_SPREADSHEET, FAILED_GENERIC). The state field stays as the single discriminator — the frontend switches on one field, the backend sets one field. Clean, no additional fields, no JSON schema bloat.
- Option B: Add a separate statusCode field alongside the existing state: FAILED — coarse state for simple consumers, fine code for i18n. More flexible, more complex, harder to keep in sync.
- Option A is the right call. state is already the only field driving all frontend branch decisions. Granularizing it costs nothing and removes the message field's only remaining reason to exist.
Breaking API change — scoped correctly. Renaming State values or removing message is a breaking change to the /api/admin/import-status response. This API has exactly one consumer (the frontend). The change must land atomically: backend + frontend in the same PR. No migration ceremony needed.
Coordination with #534. If #534 modifies MassImportService to add operator-policy behaviour, the two PRs will touch the same record and the same service method. The failure status code enum needs to be defined now to avoid a second breaking API change within the same sprint.
No new packages, services, or endpoints. This is an in-place change to one record, one enum, and one service's catch blocks. The module boundary (importing/ package owns MassImportService and ImportStatus) is preserved.

Recommendations

Rename State to ImportStatusCode. Values: IDLE, RUNNING, DONE, FAILED_NO_SPREADSHEET, FAILED_GENERIC. Annotate the field with @Schema(requiredMode = REQUIRED) in the ImportStatus record so TypeScript codegen emits it as non-optional.
Remove message from the OpenAPI contract (@Schema(requiredMode = NOT_REQUIRED) or omit @Schema entirely). It can stay on the Java record for debug logging but must not be part of the published API surface.
Define the full ImportStatusCode enum in coordination with any failure modes that #534 might introduce — enumerate once, not twice.
Doc impact: no diagram updates needed (no new package, no new endpoint, no new entity). No ADR needed — this is a correction, not a new architectural decision.

## 🏛️ Markus Keller — Application Architect ### Observations - **Domain boundary violation in the current design.** The `message: String` field on `ImportStatus` (`MassImportService.java:55`) is a localization artifact baked into the service layer. Localization is a presentation concern — it should never originate in the domain. This issue fixes a real architectural smell, not just a UX polish item. - **Two design options for the API change:** - **Option A**: Expand the existing `State` enum with granular failure codes (`IDLE`, `RUNNING`, `DONE`, `FAILED_NO_SPREADSHEET`, `FAILED_GENERIC`). The `state` field stays as the single discriminator — the frontend switches on one field, the backend sets one field. Clean, no additional fields, no JSON schema bloat. - **Option B**: Add a separate `statusCode` field alongside the existing `state: FAILED` — coarse state for simple consumers, fine code for i18n. More flexible, more complex, harder to keep in sync. - **Option A is the right call.** `state` is already the only field driving all frontend branch decisions. Granularizing it costs nothing and removes the `message` field's only remaining reason to exist. - **Breaking API change — scoped correctly.** Renaming `State` values or removing `message` is a breaking change to the `/api/admin/import-status` response. This API has exactly one consumer (the frontend). The change must land atomically: backend + frontend in the same PR. No migration ceremony needed. - **Coordination with #534.** If #534 modifies `MassImportService` to add operator-policy behaviour, the two PRs will touch the same record and the same service method. The failure status code enum needs to be defined now to avoid a second breaking API change within the same sprint. - **No new packages, services, or endpoints.** This is an in-place change to one record, one enum, and one service's catch blocks. The module boundary (`importing/` package owns `MassImportService` and `ImportStatus`) is preserved. ### Recommendations - Rename `State` to `ImportStatusCode`. Values: `IDLE`, `RUNNING`, `DONE`, `FAILED_NO_SPREADSHEET`, `FAILED_GENERIC`. Annotate the field with `@Schema(requiredMode = REQUIRED)` in the `ImportStatus` record so TypeScript codegen emits it as non-optional. - Remove `message` from the OpenAPI contract (`@Schema(requiredMode = NOT_REQUIRED)` or omit `@Schema` entirely). It can stay on the Java record for debug logging but must not be part of the published API surface. - Define the full `ImportStatusCode` enum in coordination with any failure modes that #534 might introduce — enumerate once, not twice. - Doc impact: no diagram updates needed (no new package, no new endpoint, no new entity). No ADR needed — this is a correction, not a new architectural decision.

marcel commented

2026-05-14 11:35:09 +02:00

🔒 Nora "NullX" Steiner — Application Security Engineer

Observations

Both admin import endpoints are correctly secured. AdminController.java carries a class-level @RequirePermission(Permission.ADMIN) annotation — GET /api/admin/import-status and POST /api/admin/trigger-import are both covered. No auth gap here.
Information disclosure via message field (OWASP A05 — low severity, admin-only surface). The message field in ImportStatus is set directly from backend logic. In the FAILED case, if the catch block in runImportAsync() writes e.getMessage() or a filesystem path (e.g., /import/documents/archive.ods) into message, that string reaches the frontend verbatim. Since this is an admin-only endpoint, the severity is low — but the pattern is wrong. An admin seeing /opt/familienarchiv/import/subfolder/... in a UI error message gets more internal topology than they need, and it normalizes leaking internals through the UI. The structured status code fix eliminates this vector completely.
Specific path to confirm: Audit runImportAsync() where currentStatus is set on the FAILED path. If it's something like:
```
currentStatus = new ImportStatus(State.FAILED, e.getMessage(), processed, startedAt);
```
that is the leak. The fix: the message field stays for server-side logging only; the API response carries only the ImportStatusCode enum value.
CSRF on the trigger endpoint. POST /api/admin/trigger-import must be covered by Spring Security's CSRF protection. Verify SecurityConfig does not have a blanket csrf().disable() that inadvertently covers this endpoint. Since the frontend uses form-based session auth (not token-based), CSRF protection is load-bearing here.
The startedAt: LocalDateTime field reveals the timestamp of the last import run. Acceptable for an admin-only endpoint — no concern.

Recommendations

Audit the FAILED branch in runImportAsync(). If e.getMessage() or any path string lands in message, document it explicitly and confirm the enum change removes it from the API response.
Add a @WebMvcTest test to AdminControllerTest.java asserting that GET /api/admin/import-status returns 403 when called with READ_ALL authority (not ADMIN). Currently AdminControllerTest.java has no test for the import status endpoint at all — the @RequirePermission is correct but untested.
Add the same 401 (unauthenticated) test for POST /api/admin/trigger-import. Both tests are one-liners with MockMvc + user(...).authorities(...).

## 🔒 Nora "NullX" Steiner — Application Security Engineer ### Observations - **Both admin import endpoints are correctly secured.** `AdminController.java` carries a class-level `@RequirePermission(Permission.ADMIN)` annotation — `GET /api/admin/import-status` and `POST /api/admin/trigger-import` are both covered. No auth gap here. - **Information disclosure via `message` field (OWASP A05 — low severity, admin-only surface).** The `message` field in `ImportStatus` is set directly from backend logic. In the FAILED case, if the catch block in `runImportAsync()` writes `e.getMessage()` or a filesystem path (e.g., `/import/documents/archive.ods`) into `message`, that string reaches the frontend verbatim. Since this is an admin-only endpoint, the severity is low — but the pattern is wrong. An admin seeing `/opt/familienarchiv/import/subfolder/...` in a UI error message gets more internal topology than they need, and it normalizes leaking internals through the UI. The structured status code fix eliminates this vector completely. - **Specific path to confirm:** Audit `runImportAsync()` where `currentStatus` is set on the `FAILED` path. If it's something like: ```java currentStatus = new ImportStatus(State.FAILED, e.getMessage(), processed, startedAt); ``` that is the leak. The fix: the `message` field stays for server-side logging only; the API response carries only the `ImportStatusCode` enum value. - **CSRF on the trigger endpoint.** `POST /api/admin/trigger-import` must be covered by Spring Security's CSRF protection. Verify `SecurityConfig` does not have a blanket `csrf().disable()` that inadvertently covers this endpoint. Since the frontend uses form-based session auth (not token-based), CSRF protection is load-bearing here. - **The `startedAt: LocalDateTime` field** reveals the timestamp of the last import run. Acceptable for an admin-only endpoint — no concern. ### Recommendations - Audit the FAILED branch in `runImportAsync()`. If `e.getMessage()` or any path string lands in `message`, document it explicitly and confirm the enum change removes it from the API response. - Add a `@WebMvcTest` test to `AdminControllerTest.java` asserting that `GET /api/admin/import-status` returns 403 when called with `READ_ALL` authority (not `ADMIN`). Currently `AdminControllerTest.java` has no test for the import status endpoint at all — the `@RequirePermission` is correct but untested. - Add the same 401 (unauthenticated) test for `POST /api/admin/trigger-import`. Both tests are one-liners with `MockMvc` + `user(...).authorities(...)`.

marcel commented

2026-05-14 11:35:30 +02:00

🧪 Sara Holt — QA Engineer & Test Automation Specialist

Observations

The checklist is solid. vitest-browser for the spinner, axe-playwright for a11y, visual regression at three breakpoints — all the right layers. A few gaps in how to implement them:
Existing RUNNING state test needs to fail first. page.svelte.test.ts:138–158 tests the RUNNING state renders — but based on the current implementation (no spinner), it asserts the text appears, not that a loading indicator is visible. TDD here means updating that test to assert getByRole('status') is present before adding the spinner. If the test is updated to pass with the current code (no spinner), the test is wrong.
i18n regression test is the most important new test. The current FAILED state renders backend string as {message} param into admin_system_import_status_failed. A dedicated test should: mock the API to return { state: 'FAILED', statusCode: 'FAILED_NO_SPREADSHEET', ... } with locale set to en, and assert no German text appears in the rendered output. This is the regression guard that proves the fix actually works across locales.
axe test is missing entirely. No axe-playwright test exists for /admin/system. The issue's checklist requires light + dark mode. Dark mode matters independently because the spinner's text-brand-navy colour needs to maintain contrast on the dark surface token.
Visual regression setup: The 320/768/1440 px tests are new work — no existing visual regression test exists for this page. Plan for the Playwright screenshot baseline to be committed on first run.
Backend unit tests for new status codes. MassImportServiceTest.java (522 lines) has getStatus_returnsIdleByDefault() and async failure tests. Each new ImportStatusCode value needs a corresponding assertion: verify the exact code is set on the currentStatus after the matching failure path executes. These are pure unit tests — @ExtendWith(MockitoExtension.class), no Spring context, runs in milliseconds.

Recommendations

Test pyramid plan for this issue:

Layer	New tests	Notes
Unit (JUnit/Mockito)	2–3	One per new `ImportStatusCode` value, asserting exact enum set in `currentStatus`
Component (vitest-browser)	3	Spinner visible while RUNNING; i18n locale assertion for FAILED subcodes; `text-base` on count element
E2E a11y (Playwright + axe)	1	`/admin/system` in light + dark mode
Visual regression (Playwright screenshots)	1	320 / 768 / 1440 px baseline

For the text-base font-size assertion: add a data-testid="import-processed-count" attribute and assert element.classList.contains('text-base') via expect.element(getByTestId('import-processed-count')).toHaveClass('text-base'). Computed style checks are fragile in vitest-browser; class-list checks are reliable.
The DONE state test at page.svelte.test.ts:160–180 uses the processed count — update it to assert the count is rendered in an element with text-base class. That test already has the mock data; just add the class assertion.
Don't disable the FAILED state test while the status code enum is in flight — update it alongside the backend change. A temporarily @Disabled test with no linked ticket is a silent regression risk.

## 🧪 Sara Holt — QA Engineer & Test Automation Specialist ### Observations - **The checklist is solid.** vitest-browser for the spinner, axe-playwright for a11y, visual regression at three breakpoints — all the right layers. A few gaps in how to implement them: - **Existing RUNNING state test needs to fail first.** `page.svelte.test.ts:138–158` tests the RUNNING state renders — but based on the current implementation (no spinner), it asserts the text appears, not that a loading indicator is visible. TDD here means updating that test to assert `getByRole('status')` is present *before* adding the spinner. If the test is updated to pass with the current code (no spinner), the test is wrong. - **i18n regression test is the most important new test.** The current FAILED state renders backend string as `{message}` param into `admin_system_import_status_failed`. A dedicated test should: mock the API to return `{ state: 'FAILED', statusCode: 'FAILED_NO_SPREADSHEET', ... }` with locale set to `en`, and assert no German text appears in the rendered output. This is the regression guard that proves the fix actually works across locales. - **axe test is missing entirely.** No axe-playwright test exists for `/admin/system`. The issue's checklist requires light + dark mode. Dark mode matters independently because the spinner's `text-brand-navy` colour needs to maintain contrast on the dark surface token. - **Visual regression setup**: The 320/768/1440 px tests are new work — no existing visual regression test exists for this page. Plan for the Playwright screenshot baseline to be committed on first run. - **Backend unit tests for new status codes.** `MassImportServiceTest.java` (522 lines) has `getStatus_returnsIdleByDefault()` and async failure tests. Each new `ImportStatusCode` value needs a corresponding assertion: verify the exact code is set on the `currentStatus` after the matching failure path executes. These are pure unit tests — `@ExtendWith(MockitoExtension.class)`, no Spring context, runs in milliseconds. ### Recommendations - **Test pyramid plan for this issue:** | Layer | New tests | Notes | |---|---|---| | Unit (JUnit/Mockito) | 2–3 | One per new `ImportStatusCode` value, asserting exact enum set in `currentStatus` | | Component (vitest-browser) | 3 | Spinner visible while RUNNING; i18n locale assertion for FAILED subcodes; `text-base` on count element | | E2E a11y (Playwright + axe) | 1 | `/admin/system` in light + dark mode | | Visual regression (Playwright screenshots) | 1 | 320 / 768 / 1440 px baseline | - **For the `text-base` font-size assertion:** add a `data-testid="import-processed-count"` attribute and assert `element.classList.contains('text-base')` via `expect.element(getByTestId('import-processed-count')).toHaveClass('text-base')`. Computed style checks are fragile in vitest-browser; class-list checks are reliable. - **The DONE state test** at `page.svelte.test.ts:160–180` uses the `processed` count — update it to assert the count is rendered in an element with `text-base` class. That test already has the mock data; just add the class assertion. - **Don't disable the FAILED state test** while the status code enum is in flight — update it alongside the backend change. A temporarily `@Disabled` test with no linked ticket is a silent regression risk.

marcel commented

2026-05-14 11:35:50 +02:00

📋 Elicit — Requirements Engineer

Observations

The issue is well above the Definition of Ready bar. User story explicit, three discrete fixes scoped and independently testable, GWT ACs present, NFR thresholds named (text-base, axe-clean, three breakpoints). This is one of the tighter issues in the backlog.
Gap: Failure mode enumeration is incomplete. The issue lists IMPORT_FAILED_NO_SPREADSHEET as an example status code, but doesn't enumerate all failure modes the backend can raise. MassImportService.runImportAsync() likely has catch blocks for at minimum: no import directory, no spreadsheet file found, file format not recognized, S3 upload failure, database constraint violation. Each of these needs a decision: own status code, or collapse to FAILED_GENERIC. Without that list, the implementer will make this decision ad-hoc mid-PR.
Gap: The skipped counter is in scope but absent from ACs. The Related section mentions coordination with #529/#530 (skipped-counter additions). If skipped is already on the ImportStatus DTO when this PR lands, its font size must be ≥ text-base per NFR. The ACs only mention processed. Either add a skipped AC now, or explicitly scope it out as "only if #529/#530 has merged."
Gap: startedAt field has no requirement attached. The API returns startedAt: LocalDateTime. No AC says what to do with it. During RUNNING, this could display elapsed time ("Running for 4:32") — useful operator reassurance. Currently it's returned but apparently ignored. This is a small-scope decision that will cause a mid-implementation question if not resolved now.
Coordination risk with #534. Issue #534 (operator-policy follow-up) is noted as a coordination dependency but not marked as a Gitea blocker. Both issues touch MassImportService. If they land in the wrong order, the ImportStatusCode enum may need a second revision. Mark #534 as "related" or "blocked-by this issue" in Gitea to make the dependency visible.

Recommendations

Add to the issue body: an explicit list of all backend failure paths and their assigned ImportStatusCode value. This is a 10-minute audit of runImportAsync() that saves a mid-PR question.
Either add AC: "Given a skipped count is present, when the card renders, then the value is displayed at ≥ text-base" — or add a [scope-out] note: "skipped display size covered by #529/#530."
Make a decision on startedAt: display elapsed time during RUNNING, or intentionally leave it unused. Document the decision in the issue so the implementer doesn't have to ask.
Link #534 in Gitea as a dependency to make the merge ordering explicit.

## 📋 Elicit — Requirements Engineer ### Observations - **The issue is well above the Definition of Ready bar.** User story explicit, three discrete fixes scoped and independently testable, GWT ACs present, NFR thresholds named (`text-base`, axe-clean, three breakpoints). This is one of the tighter issues in the backlog. - **Gap: Failure mode enumeration is incomplete.** The issue lists `IMPORT_FAILED_NO_SPREADSHEET` as an example status code, but doesn't enumerate all failure modes the backend can raise. `MassImportService.runImportAsync()` likely has catch blocks for at minimum: no import directory, no spreadsheet file found, file format not recognized, S3 upload failure, database constraint violation. Each of these needs a decision: own status code, or collapse to `FAILED_GENERIC`. Without that list, the implementer will make this decision ad-hoc mid-PR. - **Gap: The `skipped` counter is in scope but absent from ACs.** The Related section mentions coordination with #529/#530 (skipped-counter additions). If `skipped` is already on the `ImportStatus` DTO when this PR lands, its font size must be ≥ `text-base` per NFR. The ACs only mention `processed`. Either add a `skipped` AC now, or explicitly scope it out as "only if #529/#530 has merged." - **Gap: `startedAt` field has no requirement attached.** The API returns `startedAt: LocalDateTime`. No AC says what to do with it. During RUNNING, this could display elapsed time ("Running for 4:32") — useful operator reassurance. Currently it's returned but apparently ignored. This is a small-scope decision that will cause a mid-implementation question if not resolved now. - **Coordination risk with #534.** Issue #534 (operator-policy follow-up) is noted as a coordination dependency but not marked as a Gitea blocker. Both issues touch `MassImportService`. If they land in the wrong order, the `ImportStatusCode` enum may need a second revision. Mark #534 as "related" or "blocked-by this issue" in Gitea to make the dependency visible. ### Recommendations - Add to the issue body: an explicit list of all backend failure paths and their assigned `ImportStatusCode` value. This is a 10-minute audit of `runImportAsync()` that saves a mid-PR question. - Either add AC: *"Given a `skipped` count is present, when the card renders, then the value is displayed at ≥ `text-base`"* — or add a `[scope-out]` note: "`skipped` display size covered by #529/#530." - Make a decision on `startedAt`: display elapsed time during RUNNING, or intentionally leave it unused. Document the decision in the issue so the implementer doesn't have to ask. - Link #534 in Gitea as a dependency to make the merge ordering explicit.

marcel commented

2026-05-14 11:36:12 +02:00

🎨 Leonie Voss — UX Designer & Accessibility Strategist

Since this issue originates from my review comment on #526, let me be specific about what I found and what the fixes should look like.

Observations

Font size — confirmed violation of the senior-audience NFR. The processed count (and any skipped count from #529/#530) is currently at text-xs or text-sm (12–14px). The senior operator audience (60+) needs minimum 16px for numeric data. The CLAUDE.md card pattern is explicit: text-xs font-bold uppercase tracking-widest text-ink-3 is for section labels only — numeric values get text-base font-serif (16px Tinos). This is a one-line change per count element with zero design risk.
Spinner — needs to be recognizable and calm. The import can run for minutes. The spinner must:
1. Be recognizable as "the system is working" — an animated ring, not a generic loading text.
2. Stay visible for the entire RUNNING duration, not just on first render.
3. Respect prefers-reduced-motion — show the count incrementing without spinning animation for users who've opted out.
4. Use role="status" so assistive technology announces the state change without requiring the user to navigate there.
Recommended pattern:
```
{#if isRunning}
  <div role="status" class="flex items-center gap-3">
    <svg class="animate-spin h-5 w-5 text-brand-navy motion-reduce:hidden"
         xmlns="http://www.w3.org/2000/svg" fill="none" viewBox="0 0 24 24">
      <circle class="opacity-25" cx="12" cy="12" r="10" stroke="currentColor" stroke-width="4"/>
      <path class="opacity-75" fill="currentColor" d="M4 12a8 8 0 018-8V0C5.373 0 0 5.373 0 12h4z"/>
    </svg>
    <span class="sr-only">{m.admin_system_import_status_running()}</span>
  </div>
{/if}
```
motion-reduce:hidden hides the spinning SVG for reduced-motion users while role="status" and sr-only text remain for screen readers. The count still updates every 2s.
Dark mode: text-brand-navy uses --palette-navy which is defined in layout.css for both themes. The spinner will inherit the correct colour automatically if the semantic token is used — no dark-mode-specific spinner code needed.
Touch target: The trigger button (Import starten / Erneut starten) must be ≥ 44px height. Confirm it has min-h-[44px] — admin pages are often skipped in the touch-target audit because "admins use desktop." The senior operator may use a tablet.
Contrast of text-ink-3 label row: Verify text-ink-3 on bg-surface meets 4.5:1 in both light and dark mode. The card pattern in CLAUDE.md uses this for section labels — if the dark-mode surface token is very dark, text-ink-3 may fall below the AA threshold. The axe-playwright check will catch this if configured for dark mode too.

Open Decisions (omit this section entirely if none)

Elapsed time display. The API already returns startedAt. During a long import, showing "Running for 4:32" alongside the live count gives the operator confidence without requiring them to remember when they clicked start. This is a minor enhancement that fits within this issue's scope — but it's a product call, not a design call. If it's in, it renders as text-sm text-ink-3 below the count (16px count, 14px elapsed — acceptable hierarchy). If it's out, mark startedAt as intentionally unused in this issue.

## 🎨 Leonie Voss — UX Designer & Accessibility Strategist Since this issue originates from my review comment on #526, let me be specific about what I found and what the fixes should look like. ### Observations - **Font size — confirmed violation of the senior-audience NFR.** The `processed` count (and any `skipped` count from #529/#530) is currently at `text-xs` or `text-sm` (12–14px). The senior operator audience (60+) needs minimum 16px for numeric data. The CLAUDE.md card pattern is explicit: `text-xs font-bold uppercase tracking-widest text-ink-3` is for *section labels only* — numeric values get `text-base font-serif` (16px Tinos). This is a one-line change per count element with zero design risk. - **Spinner — needs to be recognizable and calm.** The import can run for minutes. The spinner must: 1. Be recognizable as "the system is working" — an animated ring, not a generic loading text. 2. Stay visible for the entire RUNNING duration, not just on first render. 3. Respect `prefers-reduced-motion` — show the count incrementing without spinning animation for users who've opted out. 4. Use `role="status"` so assistive technology announces the state change without requiring the user to navigate there. Recommended pattern: ```svelte {#if isRunning} <div role="status" class="flex items-center gap-3"> <svg class="animate-spin h-5 w-5 text-brand-navy motion-reduce:hidden" xmlns="http://www.w3.org/2000/svg" fill="none" viewBox="0 0 24 24"> <circle class="opacity-25" cx="12" cy="12" r="10" stroke="currentColor" stroke-width="4"/> <path class="opacity-75" fill="currentColor" d="M4 12a8 8 0 018-8V0C5.373 0 0 5.373 0 12h4z"/> </svg> <span class="sr-only">{m.admin_system_import_status_running()}</span> </div> {/if} ``` `motion-reduce:hidden` hides the spinning SVG for reduced-motion users while `role="status"` and `sr-only` text remain for screen readers. The count still updates every 2s. - **Dark mode**: `text-brand-navy` uses `--palette-navy` which is defined in `layout.css` for both themes. The spinner will inherit the correct colour automatically if the semantic token is used — no dark-mode-specific spinner code needed. - **Touch target**: The trigger button (`Import starten` / `Erneut starten`) must be ≥ 44px height. Confirm it has `min-h-[44px]` — admin pages are often skipped in the touch-target audit because "admins use desktop." The senior operator may use a tablet. - **Contrast of `text-ink-3` label row**: Verify `text-ink-3` on `bg-surface` meets 4.5:1 in both light and dark mode. The card pattern in CLAUDE.md uses this for section labels — if the dark-mode surface token is very dark, `text-ink-3` may fall below the AA threshold. The axe-playwright check will catch this if configured for dark mode too. ### Open Decisions _(omit this section entirely if none)_ - **Elapsed time display.** The API already returns `startedAt`. During a long import, showing "Running for 4:32" alongside the live count gives the operator confidence without requiring them to remember when they clicked start. This is a minor enhancement that fits within this issue's scope — but it's a product call, not a design call. If it's in, it renders as `text-sm text-ink-3` below the count (16px count, 14px elapsed — acceptable hierarchy). If it's out, mark `startedAt` as intentionally unused in this issue.

marcel commented

2026-05-14 11:36:27 +02:00

⚙️ Tobias Wendt — DevOps & Platform Engineer

Observations

No infrastructure changes required. This is a pure service-layer + frontend change. No new Docker services, no config changes, no CI pipeline modifications.
Polling load is negligible. The 2s poll interval to GET /api/admin/import-status generates ~30 req/min per active browser tab. MassImportService.getStatus() (MassImportService.java:59–61) reads a volatile field — zero database queries, no I/O. At single-admin load this is unmeasurable. No concern.
The issue spec says "2–5s." The frontend currently polls every 2s, which satisfies the spec and gives good live feedback. However, 3s is a reasonable middle ground — 33% fewer requests, still fast enough that the count visibly moves during a real import. Either is fine; 2s is not a problem, just worth noting for completeness.
No new env vars or secrets. The ImportStatusCode enum lives in application code — no configuration needed. The i18n keys are committed to messages/ — no deployment artifact changes.
CI impact: minimal. The backend change adds cases to MassImportServiceTest.java (existing file, 522 lines). The frontend change updates page.svelte.test.ts (existing file, 360 lines) and adds a Playwright axe test. Pipeline runtime change is negligible.
Good to note: the issue correctly scopes this as P1-high and plans to land before the first real prod import campaign. The blast radius is a single card on /admin/system. Even if something goes wrong, the worst case is a non-functional status card — the import itself runs server-side and is unaffected.

Recommendations

No action items from DevOps — this issue is well-contained. Just confirm that after the ImportStatus API change, npm run generate:api is in the PR's "how to test" notes so reviewers know to check the generated types match.
If the poll interval is bumped to 3s, update both the component code and the test mock (which likely uses a vi.useFakeTimers() or similar) to stay consistent.

## ⚙️ Tobias Wendt — DevOps & Platform Engineer ### Observations - **No infrastructure changes required.** This is a pure service-layer + frontend change. No new Docker services, no config changes, no CI pipeline modifications. - **Polling load is negligible.** The 2s poll interval to `GET /api/admin/import-status` generates ~30 req/min per active browser tab. `MassImportService.getStatus()` (`MassImportService.java:59–61`) reads a `volatile` field — zero database queries, no I/O. At single-admin load this is unmeasurable. No concern. - **The issue spec says "2–5s."** The frontend currently polls every 2s, which satisfies the spec and gives good live feedback. However, 3s is a reasonable middle ground — 33% fewer requests, still fast enough that the count visibly moves during a real import. Either is fine; 2s is not a problem, just worth noting for completeness. - **No new env vars or secrets.** The `ImportStatusCode` enum lives in application code — no configuration needed. The i18n keys are committed to `messages/` — no deployment artifact changes. - **CI impact: minimal.** The backend change adds cases to `MassImportServiceTest.java` (existing file, 522 lines). The frontend change updates `page.svelte.test.ts` (existing file, 360 lines) and adds a Playwright axe test. Pipeline runtime change is negligible. - **Good to note:** the issue correctly scopes this as P1-high and plans to land before the first real prod import campaign. The blast radius is a single card on `/admin/system`. Even if something goes wrong, the worst case is a non-functional status card — the import itself runs server-side and is unaffected. ### Recommendations - No action items from DevOps — this issue is well-contained. Just confirm that after the `ImportStatus` API change, `npm run generate:api` is in the PR's "how to test" notes so reviewers know to check the generated types match. - If the poll interval is bumped to 3s, update both the component code and the test mock (which likely uses a `vi.useFakeTimers()` or similar) to stay consistent.

marcel commented

2026-05-14 11:36:42 +02:00

🗳️ Decision Queue — Action Required

2 decisions need your input before implementation starts.

Backend Design

Failure mode enumeration. The issue lists IMPORT_FAILED_NO_SPREADSHEET as an example ImportStatusCode but doesn't enumerate all failure paths in runImportAsync(). Before writing new i18n keys, audit the catch blocks and decide: which failures get their own code vs. collapse to FAILED_GENERIC? Candidates likely include: import directory not found, no spreadsheet file, unrecognized file format, S3 upload failure, database constraint violation. (Raised by: Elicit, Felix)

UX / Scope

startedAt elapsed time display. The API already returns startedAt: LocalDateTime but no AC covers it. Options: (A) Show "Running for 4:32" alongside the live count during RUNNING — reassuring for long imports, fits this issue's scope, renders as text-sm text-ink-3 below the count. (B) Mark startedAt as intentionally unused in this issue and scope it to a follow-up. Either is fine — unresolved, it becomes a mid-implementation question. (Raised by: Leonie, Elicit)

## 🗳️ Decision Queue — Action Required _2 decisions need your input before implementation starts._ ### Backend Design - **Failure mode enumeration.** The issue lists `IMPORT_FAILED_NO_SPREADSHEET` as an example `ImportStatusCode` but doesn't enumerate all failure paths in `runImportAsync()`. Before writing new i18n keys, audit the catch blocks and decide: which failures get their own code vs. collapse to `FAILED_GENERIC`? Candidates likely include: import directory not found, no spreadsheet file, unrecognized file format, S3 upload failure, database constraint violation. _(Raised by: Elicit, Felix)_ ### UX / Scope - **`startedAt` elapsed time display.** The API already returns `startedAt: LocalDateTime` but no AC covers it. Options: (A) Show "Running for 4:32" alongside the live count during RUNNING — reassuring for long imports, fits this issue's scope, renders as `text-sm text-ink-3` below the count. (B) Mark `startedAt` as intentionally unused in this issue and scope it to a follow-up. Either is fine — unresolved, it becomes a mid-implementation question. _(Raised by: Leonie, Elicit)_

marcel commented

2026-05-14 11:45:47 +02:00

Failure mode: explore possible Enums, so we get good errors we can act upon
UX: show a counter

Failure mode: explore possible Enums, so we get good errors we can act upon UX: show a counter

marcel referenced this issue from a commit

2026-05-14 12:11:42 +02:00

feat(admin/system): extract ImportStatusCard — spinner, text-base count, statusCode i18n

marcel referenced a pull request that will close this issue

2026-05-14 12:11:57 +02:00

ui(admin/system): improve mass-import card (loading state, i18n, font size) #569

marcel commented

2026-05-14 12:12:08 +02:00

Implemented in PR #569 (feat/issue-533-mass-import-ux).

What was done:

Backend — MassImportService.ImportStatus gains a statusCode String field. A private NoSpreadsheetException distinguishes "no spreadsheet found" (IMPORT_FAILED_NO_SPREADSHEET) from other I/O failures (IMPORT_FAILED_INTERNAL). All 5 state transitions set the correct code. 37 + 12 backend tests green.
i18n — Removed {message} interpolation (raw German string) from all three locale files. Added admin_system_import_failed_no_spreadsheet, admin_system_import_failed_internal, and admin_system_import_status_done_label to de / en / es.
Frontend — Extracted ImportStatusCard.svelte from +page.svelte:
- RUNNING: animated spinner + processed count at text-base
- DONE: count at text-base, label at text-xs uppercase tracking-widest
- FAILED: maps statusCode to Paraglide message — no raw backend string ever rendered
- Vitest-browser tests cover spinner visibility, count display, and per-statusCode FAILED message selection

🤖 Generated with Claude Code

Implemented in PR #569 (`feat/issue-533-mass-import-ux`). **What was done:** 1. **Backend** — `MassImportService.ImportStatus` gains a `statusCode` String field. A private `NoSpreadsheetException` distinguishes "no spreadsheet found" (`IMPORT_FAILED_NO_SPREADSHEET`) from other I/O failures (`IMPORT_FAILED_INTERNAL`). All 5 state transitions set the correct code. 37 + 12 backend tests green. 2. **i18n** — Removed `{message}` interpolation (raw German string) from all three locale files. Added `admin_system_import_failed_no_spreadsheet`, `admin_system_import_failed_internal`, and `admin_system_import_status_done_label` to `de` / `en` / `es`. 3. **Frontend** — Extracted `ImportStatusCard.svelte` from `+page.svelte`: - RUNNING: animated spinner + `processed` count at `text-base` - DONE: count at `text-base`, label at `text-xs uppercase tracking-widest` - FAILED: maps `statusCode` to Paraglide message — no raw backend string ever rendered - Vitest-browser tests cover spinner visibility, count display, and per-statusCode FAILED message selection 🤖 Generated with [Claude Code](https://claude.com/claude-code)

marcel referenced this issue from a commit

2026-05-14 12:38:07 +02:00

feat(admin/system): extract ImportStatusCard — spinner, text-base count, statusCode i18n

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: marcel/familienarchiv#533