Replace hand-copied load/action replicas with direct imports of the
real module. Mock $env/dynamic/private so the tests cover the actual
production code paths, not a duplicate that can drift.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Screen readers now announce the amber warning when it appears after
the form expands, without requiring the user to navigate to it.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
bind:group requires a writable $state variable; $derived is read-only
in Svelte 5, so every click was silently reset to unchecked, making
the group picker non-functional.
Also wraps checkboxes in <fieldset>/<legend> for WCAG 1.3.1 compliance.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- load() fetches /api/groups in parallel with /api/invites; returns
sorted groups array and groupsLoadError for partial failures
- create action forwards groupIds[] to POST /api/invites so invited
users are placed in the selected groups on registration
- +page.svelte: group checkboxes via UserGroupsSection inside the form;
amber warning banner when groups could not be loaded
- page.svelte.test.ts: groups checkboxes + warning banner tests
- page.server.test.ts: parallel fetch, sorting, error fallback,
groupIds in POST body
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds the error code to the ErrorCode union and getErrorMessage() switch.
Adds admin_new_invite_groups, admin_invite_groups_load_error, and
error_group_has_active_invites to all three locale files (de/en/es).
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds GROUP_HAS_ACTIVE_INVITES error code and guards UserService.deleteGroup()
with a 409 conflict when any active (non-revoked, non-expired, non-exhausted)
invite token still holds the group UUID.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Extract ImportStatus type to types.ts — removes duplication across
+page.svelte, ImportStatusCard.svelte, and test file (Felix blocker)
- Fix H2 to match CLAUDE.md card pattern: text-xs uppercase tracking-widest
text-ink-3 mb-5 (Leonie blocker 1)
- Add font-sans to RUNNING and DONE status labels (Leonie blocker 2)
- Add data-testid="processed-count" to count elements in both states
- Replace document.querySelector with locator API in spinner tests
- Tighten getByText('7') to getByTestId('processed-count') (Felix/Sara)
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Covers the success path — previously untested per Sara's review.
Creates a minimal empty XLSX via XSSFWorkbook so processRows returns 0.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
toBeAttached() is not in the vitest-browser matcher set; toBeVisible() was
previously ruled out because the spinner is 0x0 px. Mirror the querySelector
pattern already used for the negative case in the same file.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
CI Chromium runs with German locale so hardcoded English strings like
'No spreadsheet file found.' never matched. Use m.admin_system_import_*()
to assert whatever locale the browser resolves to.
Spinner test used toBeVisible() on an empty <span> whose dimensions come
entirely from Tailwind CSS. Without layout CSS the span is 0×0 and fails
the visibility check; toBeAttached() asserts DOM presence, which is the
right semantic here.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Three test files were written against the old API shape (raw `message` field) before
the statusCode i18n field was introduced, or used the wrong `expect` import path:
- ImportStatusCard.svelte.test.ts: `@vitest/browser/context` does not export `expect`
in this project's Vitest setup — use `vitest` like every other test file.
- page.svelte.spec.ts: FAILED mock lacked `statusCode`; assertion matched old German
raw message instead of the i18n string for IMPORT_FAILED_NO_SPREADSHEET.
- page.svelte.test.ts: same pattern — mock lacked `statusCode`; assertion checked for
raw backend string "database error" instead of the rendered i18n text.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Remove dead `message` field from both frontend ImportStatus types
(field is now @JsonIgnore'd on the backend)
- Extract failure message ternary into `$derived` — business logic off
the template (Felix)
- Add motion-reduce:animate-none to spinner — WCAG 2.1 SC 2.3.3 (Leonie)
- Replace text-green-600 with text-green-800 — WCAG AA contrast 6.1:1
on bg-green-50 (Leonie)
- Add min-h-[44px] to all three buttons — WCAG 2.2 44px touch target (Leonie)
- Add 6 missing tests: IMPORT_FAILED_INTERNAL path, IDLE state text,
null importStatus, ontrigger called on DONE/FAILED/IDLE buttons (Sara)
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- @JsonIgnore on ImportStatus.message — stops internal directory paths and
raw exception text leaking through the admin import-status endpoint (CWE-209)
- Add importStatus_messageField_notPresentInApiResponse test (red/green verified)
- Add importStatus_returns401/403 auth boundary tests — documents and guards
the @RequirePermission(ADMIN) protection against configuration drift
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Extracts the mass-import block from +page.svelte into ImportStatusCard.svelte.
Changes per the three UX fixes from issue #533:
- RUNNING: animated spinner (animate-spin) + processed count at text-base;
auto-poll at 2 s was already in place
- DONE: processed count at text-base, label at text-xs uppercase tracking-widest
- FAILED: maps statusCode (IMPORT_FAILED_NO_SPREADSHEET / IMPORT_FAILED_INTERNAL)
to Paraglide messages — no raw German backend string rendered
Adds vitest-browser tests covering spinner visibility, count display,
and per-statusCode FAILED message selection.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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>
Adds a statusCode field (IMPORT_IDLE / IMPORT_RUNNING / IMPORT_DONE /
IMPORT_FAILED_NO_SPREADSHEET / IMPORT_FAILED_INTERNAL) to ImportStatus.
The frontend will map these codes to localized strings via Paraglide
instead of rendering the backend's German message verbatim.
NoSpreadsheetException distinguishes a missing spreadsheet from other
I/O failures so the frontend can show a specific error without raw text.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
testTimeout: 30_000 causes Vitest to fail a hanging browser test
within 30 s when Chromium crashes mid-load instead of silently
occupying the CI slot for 14+ min.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Captures all 102 test results independent of log verbosity.
if: always() ensures reports are available on failure — exactly
when they're needed most.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
forkedProcessTimeoutInSeconds=120 caps the JVM on catastrophic hangs.
junit.jupiter.execution.timeout.default=90s times out each hanging
JUnit 5 test individually, letting healthy tests continue — replaces
the deprecated <timeout> alias that conflicted with the JVM ceiling.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Set logging.level.root=WARN + logging.level.org.raddatz=INFO in
backend/src/test/resources/application.properties to keep the full
test run under Gitea's 1.4 MB log cap.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Use [key: string]: unknown index signature so TS does not reject the
extra fields (location, status) passed to the redirect/failure result
in the spec helpers.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Mirror the groups/new fix: replace inline beforeNavigate/isDirty with
createUnsavedWarning() + UnsavedWarningBanner and add an enhance callback
that calls clearOnSuccess() before update() on redirect results.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Use createUnsavedWarning() + UnsavedWarningBanner to replace the inline
beforeNavigate/isDirty pattern, and add an enhance callback that calls
clearOnSuccess() before update() so the guard is disarmed before
SvelteKit's internal goto() fires on a redirect result.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
When the node_modules cache hits, npm ci is skipped and the prepare
lifecycle (svelte-kit sync) never runs. frontend/tsconfig.json extends
.svelte-kit/tsconfig.json which only exists after svelte-kit sync —
so ESLint fails at tsconfig resolution on every cache-warm run.
Adding an unconditional svelte-kit sync step after Paraglide compile
and before Lint ensures .svelte-kit/tsconfig.json is always present
regardless of cache state.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replace && with ; in test:coverage so the client vitest run is not
short-circuited when the server run exits non-zero (e.g. threshold
violation or test failure). Without this the upload-artifact step
only ever sees coverage/server.
Also updates the stale CLAUDE.md comment that said server-only.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
CDP-based Playwright clicks (locator.click()) do not reliably trigger
Svelte 5 onclick handlers — documented in commit 0c765d81 which fixed
13 other specs. The layout dropdown tests were missed in that pass.
Applies the same pattern: ((await locator.element()) as HTMLElement).click()
for button interactions, and native KeyboardEvent dispatch for the Escape
test (dispatched on the button so it bubbles to the parent div's onkeydown).
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Docker Compose interpolates all variables in the full file even when
only a subset of services is requested. The backend service uses
IMPORT_HOST_DIR with :? (hard-required), causing the idempotency job
to abort before any container starts. A dummy path satisfies the parser;
the backend service is never started in this job so the path need not exist.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The previous self-test proved the regex catches @v5 (positive case).
This adds a negative case proving @v3 is NOT flagged — guards against
a false-positive that would break every CI run permanently.
Suggested by Sara Holt in review of PR #558.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Lines 203, 230, and 332 carried comments that actively encouraged
the regression (they read as if v4 is the canonical target). Replaced
with the correct pinned-at-v3 comment referencing ADR-014.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Documents the three-incident history, the enforcement layers (inline
comments + grep guard + ADR), how to spot the symptom, and the explicit
upgrade trigger (act_runner v4 protocol support OR v3 CVE).
Cross-references ADR-011 (single-tenant Gitea runner) and #557.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Reverts the re-regression introduced in 410b91e2. Gitea Actions
(act_runner) does not implement the v4 artifact protocol — jobs report
failure even when all tests pass. Pins all three call sites back to @v3
and adds load-bearing inline comments pointing to ADR-014 / #557.
This commit makes the grep guard added in the previous commit GREEN.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds a repo-invariant check in the same 'Assert' block as the ADR-012
birpc guard. Anchored to YAML `uses:` lines so the inline self-test
fixture does not false-positive. Fails with an actionable error
referencing ADR-014 / #557.
Guard is intentionally RED at this commit — the three v4 call sites
are downgraded in the next commit.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Branches gate was blocking CI at 75% measured coverage. The 80% floor
suffers Istanbul parent/child denominator coupling (long-tail grind, per
#496) that makes the remaining gap disproportionately costly to close.
Drop branches to 75 to match current state; leave lines/functions/
statements at 80. ADR-013 documents the rationale and the ratchet rule
for raising the gate back incrementally.
Closes#556
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds a second binding invariant section to ADR-012 covering the
duplicate-id mechanism named in #553's follow-up investigation: same
resolved module URL referenced via two distinct vi.mock id strings →
@vitest/browser-playwright leaks an orphan Playwright route → birpc-closed
crash in the next session.
Records the rule (one canonical id per mocked module, prefer the spelling
production uses, no-extension for .svelte rune modules), the in-suite
detector (no-duplicate-mock-ids.test.ts), and the patch-package backport
of vitest PR #10267 with its removal trigger.
Extends the existing Consequences enforcement list from four layers to
six, adding the duplicate-id detector and the patch-package layer.
Refs: #553 · vitest-dev/vitest#9957 · vitest-dev/vitest#10267
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Installs patch-package (^8.0.0) and a postinstall script, then applies
the diff from vitest PR #10267 against @vitest/browser-playwright@4.1.0.
What the patch changes (in dist/index.js):
- createPredicate(sessionId, url) → createPredicate(url): factory becomes
pure, returns { url, predicate } instead of mutating sessionIds /
idPreficates as a side-effect.
- sessionIds value type: array → Set (deduplicates resolved URLs).
- register handler now looks up any existing predicate for the
(sessionId, resolvedUrl) pair and unroutes it BEFORE installing the
new route. This is the actual race fix: without it, the second
vi.mock for a duplicate-id leaks an orphan Playwright route that
fires after birpc closes.
- clear handler iterates the Set via spread.
Why this matters even though Layer 1 normalised the only known duplicate
in our suite: every future vi.mock call is a class of race we shouldn't
have to think about. The patch closes the upstream gap at the
route-handler level, so a contributor reintroducing the duplicate-id
pattern can't reopen the race.
When to remove: when @vitest/browser-playwright ships a release
containing PR #10267. Delete patches/@vitest+browser-playwright+4.1.0.patch
and the postinstall hook (or keep the hook if other patches accumulate).
Refs: #553 · vitest-dev/vitest#9957 · vitest-dev/vitest#10267
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Scans every src/**/*.svelte.{spec,test}.ts file for vi.mock first-arg
strings, canonicalises each by stripping a trailing .js/.ts after
.svelte, groups by canonical id, and fails if any canonical id is
referenced under two or more distinct raw spellings.
Mirrors the shape of src/__meta__/no-async-mock-factories.test.ts:
source-text regex scan (no AST parser dependency), red/green self-test
fixtures inline, then one corpus assertion that the whole suite is
clean.
This is the in-suite defence-in-depth layer for the duplicate-id birpc
race named in ADR-012 / #553 and fixed upstream by vitest PR #10267.
Harder to disable than ESLint (cross-file invariant ESLint cannot
express anyway) and harder to scope around than a CI grep.
Refs: #553
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Five test files mocked $lib/shared/services/confirm.svelte under BOTH
spellings (.svelte and .svelte.js) within the same file; two more mocked
only the .svelte.js form. Both resolve to the same module URL but register
two distinct Playwright route handlers in @vitest/browser-playwright. The
cleanup logic only removes one, leaving an orphan that fires when the next
session loads the module — crashing the run with
"[birpc] rpc is closed, cannot call resolveManualMock".
This is the exact trigger fixed upstream by vitest PR #10267 (issue #9957).
Normalise every confirm.svelte mock to the no-extension form, matching
production imports and the source file basename (confirm.svelte.ts).
After this commit: 8 confirm.svelte mocks across 8 spec files, all under
one canonical ID. A meta-test (next commit) prevents the duplicate-id
pattern from reappearing.
Refs: #553 · vitest-dev/vitest#9957 · vitest-dev/vitest#10267
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Production code referenced $lib/shared/services/confirm.svelte under two
spellings — 4 files with the .js extension and one without. Standardise on
the no-extension form to match Svelte 5 rune-module convention and the
source file basename (confirm.svelte.ts).
Why this matters: vitest browser mode's @vitest/browser-playwright resolves
both spellings to the same module URL but registers a separate Playwright
route per spelling. The route-cleanup logic only unregisters the latest,
leaving an orphan that crashes the next session with
"[birpc] rpc is closed, cannot call resolveManualMock". Fixed upstream in
vitest PR #10267 (merged, not yet released). Normalising the spelling
removes the trigger from our side.
Refs: #553. Companion test-file changes follow in the next commit.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Hover-prefetch has two surfaces in SvelteKit:
- data-sveltekit-preload-data (route loader data)
- data-sveltekit-preload-code (route JS chunks)
The original fix turned off only the loader-data side. Route-code chunks
prefetched on hover can also include manually-mocked module URLs; an
in-flight code prefetch landing after iframe teardown hits the same
Playwright route handler that resolves manual mocks, raising the
unhandled rejection. Disable both surfaces.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The previous revision allowed vi.mock for virtual modules on the "consumer
import is static" argument. #553 proved that argument wrong: a statically-
imported module with an async factory body whose dynamic import landed
after teardown still produced the race. The factory body — not the
consumer — is the failure surface.
- Drop the "residual exceptions" table.
- Add the binding invariant: factory bodies under `**/*.svelte.{test,spec}.ts`
must be synchronous (no `await`, no `import(...)`).
- Document the canonical vi.hoisted + getter pattern, with file references.
- Record the $app/stores → $app/state architectural call (Markus's
recommendation), removing one of the last two deprecated-import
outliers.
- Record the preload-data=off hardening (Tobias's recommendation) as a
pattern note.
- Update the Enforcement section to list all four defence layers (ESLint,
CI grep, in-suite meta-test, CI birpc assert) and the coverage-flake-
probe verification workflow.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Verification mechanism for the 20-run acceptance criterion of issue #553.
Triggered manually via workflow_dispatch, runs the full coverage suite 20×
in parallel against a single SHA, asserts zero `[birpc] rpc is closed`
lines in every cell.
One fire, parallel cost (~one main-job's wall-clock), deterministic signal
for the teardown race. Cheaper than 20 sequential push events and tests
the same property the AC names.
Closes the verification gap raised by Tobias and Elicit in the issue
discussion.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The pdfjs-dist literal grep added in 9260866f only caught one named
trigger of the birpc teardown race; the underlying mechanism (ADR 012 /
#553) is any async vi.mock factory whose body performs `await import(...)`.
Add a second PCRE-multiline grep matching that shape. Scoped to
**/*.{spec,test}.ts under frontend/src/, excluding __meta__ (which holds
the fixture strings exercising the meta-test). Defence in depth pairs with
the ESLint rule (saves at edit time) and the in-suite meta-test (catches
when tests run).
Verified locally with real GNU grep against a planted synthetic offender.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Generalise the no-restricted-syntax rule from the literal pdfjs-dist
selector (added in #535) to also catch the underlying mechanism named in
ADR-012 / #553: any `vi.mock(..., async () => { ... await import(...)
... })` produces a late birpc roundtrip during worker teardown.
Selector: vi.mock CallExpression whose second argument is an
ArrowFunctionExpression with async=true and whose subtree contains an
AwaitExpression > ImportExpression. Both rules coexist — the literal
pdfjs-dist rule still enforces the libLoader prop injection pattern
(catches sync forms too); the new rule enforces the sync-factory
invariant universally.
Demonstrated by planting a synthetic offender locally and watching
ESLint flag it with the new rule's message.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
