ui(admin/system): improve mass-import card (loading state, i18n, font size) #569
Reference in New Issue
Block a user
Delete Branch "feat/issue-533-mass-import-ux"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes #533
Summary
Three discrete UX fixes to the mass-import card at
/admin/system, flagged by Leonie Voss after #526 made the card live on prod:RUNNING, renders an animated spinner (animate-spin) alongside the liveprocessedcount. Auto-poll at 2 s was already in place.statusCodestring (IMPORT_IDLE/IMPORT_RUNNING/IMPORT_DONE/IMPORT_FAILED_NO_SPREADSHEET/IMPORT_FAILED_INTERNAL) instead of a hard-coded German message. The frontend maps this to Paraglide keys; no raw backend string is ever rendered. A privateNoSpreadsheetExceptiondistinguishes "no spreadsheet file found" from other I/O failures.processedcount shown attext-base(16 px); section labels attext-xs uppercase tracking-widest. Works for both RUNNING and DONE states.Commits
2ad25f06feat(import): addstatusCodetoImportStatus+NoSpreadsheetException+ tests5dd91fa1feat(i18n): new failure keys + split DONE label (de / en / es)d8de391cfeat(admin/system):ImportStatusCardcomponent — spinner, text-base count, statusCode i18nTest plan
MassImportServiceTest(37 tests) +AdminControllerTest(12 tests) — all green locallyImportStatusCard.svelte.test.ts— run in CI (aborted locally due to browser startup time)/admin/systemcard in all three states (RUNNING / DONE / FAILED) and bothen/eslocales🤖 Generated with Claude Code
Replaces the {message} interpolation (raw German backend string) with two distinct error keys: IMPORT_FAILED_NO_SPREADSHEET and IMPORT_FAILED_INTERNAL. Also removes the {count} parameter from the done message and adds admin_system_import_status_done_label so the processed count can be rendered separately at text-base size. All three locales (de / en / es) updated. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>d8de391c10to3b2fae8188🏛️ Markus Keller — Senior Application Architect
Verdict: ✅ Approved
Documentation trigger check
ImportStatusCardis a component, not a routeErrorCodeorPermissiondocs/adr/statusCodeis an implementation detail, not a structural decisionNo doc updates required. ✅
Observations
statusCodeString alongsideStateenum — TheImportStatusrecord now carries both aStateenum and astatusCodeString. These are parallel representations:State.FAILEDmaps to two possible statusCodes. This is a deliberate choice to let the frontend distinguish failure sub-types without coupling to Java enum semantics. The mapping is fully contained withinMassImportService; no other layer knows about the statusCode values. I find this acceptable — the alternative (sub-classingStateor adding a second enum) would be heavier. Well-bounded.NoSpreadsheetExceptionas a private static inner class — Clean containment. The exception is an implementation detail of the service and doesn't escape. Correct design.ImportStatustype duplicated in+page.svelteandImportStatusCard.svelte— Per project conventions, each module defines its own input types. Duplication here is cheaper than coupling. This is correct.Suggestion (non-blocking)
The
messagefield inImportStatusis returned in the API response but carries raw exception text and internal directory paths (e.g.,"Fehler: " + e.getMessage()which can include/data/import/...). The frontend ignores this field entirely and drives display fromstatusCode. Consider whethermessagebelongs in the API contract at all — or if it should be annotated@JsonIgnoreand kept exclusively for server-side logging. A minimal API surface is preferable.👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ⚠️ Approved with concerns
Blockers
None.
Suggestions
1. Business logic in template markup (
ImportStatusCard.svelte, line ~64)The inline ternary selecting the failure message is business logic that belongs in
$derived, not in the template:Preferred:
2. Template length (
ImportStatusCard.svelte)The template runs from line 21 to line 80 — 60 lines of markup. The 40-line splitting signal is passed. Each branch (RUNNING / DONE / FAILED / IDLE) is a distinct visual region. That said, given this is an admin-only utility card with four compact states, I'd leave the split decision to the author — the cognitive overhead of four separate component files may outweigh the benefit here. Flagging as a discussion point, not a mandate.
3. Dead
messagefield in frontend typesBoth
+page.svelte:13andImportStatusCard.svelte:6declaremessage: stringin their localImportStatustype, but neither component ever accessesimportStatus.message. The backend still populates it with German strings ("Fehler: " + e.getMessage()). Two questions:messagebe removed from both frontend type definitions?What's well done
makeStatus()factory with overrides in the test file — idiomatic, readable.$props()destructuring in Svelte 5 style — correct.NoSpreadsheetExceptionas a private static inner class — the exception catches the specific case cleanly without leaking beyond the service.runImportAsynctest updated to match the new 5-argImportStatusconstructor — no signature drift.role="status"andaria-label— accessibility-aware from the start.🔧 Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
No CI/CD workflow, Docker Compose, or infrastructure changes in this PR. Nothing to flag from the platform side.
What I verified
One note
The test plan says the vitest-browser component tests were "aborted locally due to browser startup time" and are expected to run in CI instead. This is a valid pattern — browser tests often can't run on dev machines without a full Playwright install. But before merging, confirm that:
A quiet pass in CI is not the same as a confirmed pass. Check the CI run output for
ImportStatusCard.svelte.test.tsexplicitly.LGTM otherwise.
📋 Elicit — Senior Requirements Engineer
Verdict: ⚠️ Approved with concerns
Requirements coverage from issue #533
text-basefor count, section labels attext-xs uppercase tracking-widestConcerns
1. The
messagefield partially undermines the i18n requirement at the API contract levelThe PR description states: "no raw backend string is ever rendered." This is true for the current UI. However, the API response still includes
message: "Fehler: ..."— German text with potentially internal path details. Any future frontend consumer (a second admin view, an external monitoring integration, an API script) that readsmessageinstead ofstatusCodewould silently break the i18n guarantee. The requirement is satisfied at the rendering layer but not enforced at the contract boundary.Recommendation: Either exclude
messagefrom the serialized response (@JsonIgnoreon the field), or explicitly document in the API (OpenAPI description) thatmessageis a developer/log field not intended for display.2. Unspecified behavior for
importStatus = nullon initial loadBefore the first 2-second polling interval completes,
importStatusisnull. The component renders nothing in this state (just the card heading and description, no status area). Is this the intended user experience? No acceptance criterion in the issue covers this. A "Loading…" indicator or skeleton would better signal to the admin that status is being fetched. Worth a follow-up issue if intentional deferral.3. In-memory state loss on server restart
currentStatusis aprivate volatilefield — it resets toIDLEon restart. If the last import failed, an admin who connects after a restart sees IDLE with no history of the failure. The new sub-codes (IMPORT_FAILED_NO_SPREADSHEET,IMPORT_FAILED_INTERNAL) make this gap more visible since they provide actionable distinctions the admin can no longer see after a restart. This is pre-existing behavior, but the new granularity warrants a follow-up issue to track whether persistence (e.g., aimport_statusDB row) is needed.🔐 Nora "NullX" Steiner — Application Security Engineer
Verdict: ⚠️ Approved with concerns
Concern — Information Disclosure via
messagefield (CWE-209, Low-Medium)File:
MassImportService.javaThe
messagefield ofImportStatusis serialized and returned viaGET /api/admin/import-status. For the two failure cases:The
NoSpreadsheetExceptionmessage includesimportDir— a Spring configuration value that exposes the container's internal directory structure (e.g.,/data/import). This message propagates through the exception toimportStatus.messageand into the API response.Risk: Low-Medium. The endpoint requires ADMIN permission, limiting exposure to trusted users. However, this data can appear in browser DevTools, exported browser logs, or audit trails shipped to external systems.
Fix option A — sanitize the message field:
Fix option B — exclude from serialization entirely (since the frontend never uses it):
The
log.error(...)calls already capture the full detail on the server side — nothing is lost.Observation — Missing 401/403 Test Coverage
AdminControllerTest.importStatus_returns200_withStatusCode_whenAdmin()verifies the happy path, but the authorization boundary is untested:These are not blockers — the existing
@RequirePermissioninfrastructure presumably handles this correctly. But permission boundary tests are cheap insurance against future configuration drift.🧪 Sara Holt — Senior QA Engineer
Verdict: ⚠️ Approved with concerns
Backend — MassImportServiceTest
✅
getStatus_hasStatusCode_IMPORT_IDLE_byDefault()— clear behavioral name, single assertion.✅
runImportAsync_setsStatusCode_IMPORT_FAILED_NO_SPREADSHEET_whenDirIsEmpty()— covers new exception path with specific statusCode assertion. Good use of@TempDir.✅
runImportAsync_setsFailedStatus_whenImportDirectoryDoesNotExist()— extended with statusCode check, correctly identifiesIMPORT_FAILED_INTERNALfor the IOException path.✅
runImportAsync_throwsConflict_whenAlreadyRunning— constructor updated to match 5-arg signature. No drift.Backend — AdminControllerTest
✅
importStatus_returns200_withStatusCode_whenAdmin()— verifies$.statusCodeand$.statein the API response JSON. Good.Gap — authorization boundaries untested:
Testing "authorized users get 200" without testing "unauthorized users get 403/401" is half-coverage on the security boundary. Not a blocker — but these tests catch configuration drift.
Frontend — ImportStatusCard.svelte.test.ts
✅
makeStatus()factory with overrides — clean, idiomatic Vitest pattern.✅ Spinner visibility for RUNNING.
✅ Processed count visible for RUNNING and DONE.
✅ i18n string verified for
IMPORT_FAILED_NO_SPREADSHEET.Gaps:
importStatus = nullnot tested — the component renders nothing for null; this should be explicit:IDLE state with non-null
importStatusnot tested — the idle text (m.admin_system_import_status_idle()) should appear when state is'IDLE'andimportStatus !== null. Currently untested.IMPORT_FAILED_INTERNALpath not tested — onlyIMPORT_FAILED_NO_SPREADSHEETis tested. Theelsebranch of the ternary is unverified.ontriggercallback not tested — neither the "retry" button (DONE/FAILED states) nor the "start" button (IDLE state) is tested for actually callingontrigger.Locale assumption —
expect.element(getByText('No spreadsheet file found.')).toBeVisible()relies on Paraglide serving English in the test environment. Is this configured? If Paraglide defaults to German in tests, this assertion silently fails to find the element. Document or add a setup fixture.Reliability note
The test plan explicitly says browser tests were "aborted locally due to browser startup time." Confirm all 4
ImportStatusCardtest cases are green in the actual CI run before merging — do not rely on "CI will catch it" as a substitute for a confirmed passing run.🎨 Leonie Voss — UX Designer & Accessibility Strategist
Verdict: ⚠️ Approved with concerns
What works well
✅ Spinner has
role="status"+aria-label={m.admin_system_import_status_running()}— screen readers will announce the running state correctly.✅
text-base(16px) for the processed count — meets the senior audience minimum.✅ Brand-compliant spinner:
border-ink-3base ring,border-t-brand-mintspinning indicator.✅ Visual hierarchy in DONE state: count (bold,
text-base) → label (text-xs uppercase) → status line — scannable at a glance.✅ Semantic
<div>structure for the spinner row (flex items-center gap-3) — count and label are visually grouped.Concern 1 —
animate-spinwithoutprefers-reduced-motionprotection [High]animate-spinruns unconditionally. Users with vestibular disorders can experience nausea from spinning elements. Tailwind'smotion-reducemodifier adds this at zero visual cost for other users:This aligns with WCAG 2.1 SC 2.3.3 (Animation from Interactions). Given the 60+ audience that is the primary user of this app, treat this as High priority.
Concern 2 —
text-green-600contrast in DONE state [Medium]text-green-600(#16a34a) onbg-green-50(#f0fdf4) has an approximate contrast ratio of ~3.0:1 — below the WCAG AA minimum of 4.5:1 for normal text.Affected elements:
"Dokumente verarbeitet"label →text-xs font-bold tracking-widest text-green-600"Import abgeschlossen"line →text-xs text-green-600Fix: Replace
text-green-600withtext-green-800(#166534), which achieves ~6.1:1 onbg-green-50.Note:
text-green-700ontext-basefor the count has better contrast (~4.3:1) but still falls short of AA. Also recommend bumping that totext-green-800.Concern 3 — Button touch target height [Medium]
The retry/start buttons use
px-5 py-2 font-sans text-xs:py-2→ 8px top + 8px bottom = 16px vertical paddingtext-xsline-height ≈ 20pxFix: Add
min-h-[44px]to the button class (or usepy-3):Note: This same button style exists in the thumbnail backfill section of the same page. A consistent fix across all admin action buttons would be ideal.
Observation —
text-xssecondary labels [Low]The "Dokumente verarbeitet" label and spinner status text use
text-xs= 12px. This is the absolute floor. For the 60+ audience,text-sm(14px) would be preferable. Low priority since these are secondary labels, not body copy — but worth tracking for a future accessibility pass.Review concerns addressed — commits
a2e925b7+120866bfBackend (
a2e925b7)messagefield leaks internal directory paths + raw exception text via API (CWE-209)@JsonIgnoreonImportStatus.message— field is logging-only, excluded from serialized responseimportStatus_returns401_whenUnauthenticatedmissing@RequirePermission(ADMIN)against configuration driftimportStatus_returns403_whenUserLacksAdminPermissionmissingimportStatus_messageField_notPresentInApiResponse— test was red before fix@JsonIgnore, absent afterFrontend (
120866bf)const failureMessage = $derived(...)message: stringin bothImportStatustype definitionsImportStatusCard.svelteand+page.svelte— field is now@JsonIgnore'danimate-spinwithoutprefers-reduced-motion[High]motion-reduce:animate-none— WCAG 2.1 SC 2.3.3text-green-600onbg-green-50— contrast ~3.0:1 [Medium]text-green-800— contrast ~6.1:1 (WCAG AA ✅)min-h-[44px]to all three buttons — WCAG 2.2 ✅IMPORT_FAILED_INTERNALpath untestedimportStatus = nulluntestedontriggercallback untestedDeferred (not code changes)
Browser tests are running on CI now (the vitest-browser suite was not run locally due to Playwright startup overhead).
🤖 Generated with Claude Code
View command line instructions
Checkout
From your project repository, check out a new branch and test the changes.