fix(admin): clear unsaved-changes guard before redirect on groups/new and users/new #567

marcel · 2026-05-14T11:59:08+02:00

marcel commented

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

Closes #527

Summary

Root cause: use:enhance on both /new pages had no callback, so on a redirect result SvelteKit internally called goto() while isDirty was still true — causing beforeNavigate to cancel the navigation and show the amber "unsaved changes" banner even though the record had been saved successfully.
Fix: add an enhance callback that calls unsaved.clearOnSuccess() before await update(), so the guard is disarmed before SvelteKit's internal goto() fires.
Cleanup: replace the inline isDirty/showUnsavedWarning/discardTarget/beforeNavigate block and inline banner with createUnsavedWarning() + <UnsavedWarningBanner> — matching the existing pattern on the edit pages ([id]/+page.svelte).

Changed files

File	Change
`frontend/src/routes/admin/groups/new/+page.svelte`	Refactor inline guard → hook + banner + enhance callback
`frontend/src/routes/admin/users/new/+page.svelte`	Same refactor
`frontend/src/routes/admin/groups/new/page.svelte.spec.ts`	New — 6 unsaved-warning behaviour tests (redirect clears, failure keeps)
`frontend/src/routes/admin/users/new/page.svelte.spec.ts`	Add `$app/navigation` mock + 6 unsaved-warning tests

Test plan

groups/new/page.svelte.spec.ts — 6 new tests all green
users/new/page.svelte.spec.ts — 6 new tests + 7 pre-existing tests all green
groups/new/page.svelte.test.ts — all pre-existing rendering tests still green
users/new/page.svelte.test.ts — all pre-existing rendering tests still green
npm run check — no new type errors introduced (pre-existing { url: URL } pattern shared with groups/[id], users/[id], tags/[id] specs)

🤖 Generated with Claude Code

Closes #527 ## Summary - **Root cause**: `use:enhance` on both `/new` pages had no callback, so on a `redirect` result SvelteKit internally called `goto()` while `isDirty` was still `true` — causing `beforeNavigate` to cancel the navigation and show the amber "unsaved changes" banner even though the record had been saved successfully. - **Fix**: add an enhance callback that calls `unsaved.clearOnSuccess()` *before* `await update()`, so the guard is disarmed before SvelteKit's internal `goto()` fires. - **Cleanup**: replace the inline `isDirty`/`showUnsavedWarning`/`discardTarget`/`beforeNavigate` block and inline banner with `createUnsavedWarning()` + `<UnsavedWarningBanner>` — matching the existing pattern on the edit pages (`[id]/+page.svelte`). ## Changed files | File | Change | |------|--------| | `frontend/src/routes/admin/groups/new/+page.svelte` | Refactor inline guard → hook + banner + enhance callback | | `frontend/src/routes/admin/users/new/+page.svelte` | Same refactor | | `frontend/src/routes/admin/groups/new/page.svelte.spec.ts` | **New** — 6 unsaved-warning behaviour tests (redirect clears, failure keeps) | | `frontend/src/routes/admin/users/new/page.svelte.spec.ts` | Add `$app/navigation` mock + 6 unsaved-warning tests | ## Test plan - [x] `groups/new/page.svelte.spec.ts` — 6 new tests all green - [x] `users/new/page.svelte.spec.ts` — 6 new tests + 7 pre-existing tests all green - [x] `groups/new/page.svelte.test.ts` — all pre-existing rendering tests still green - [x] `users/new/page.svelte.test.ts` — all pre-existing rendering tests still green - [x] `npm run check` — no new type errors introduced (pre-existing `{ url: URL }` pattern shared with `groups/[id]`, `users/[id]`, `tags/[id]` specs) 🤖 Generated with [Claude Code](https://claude.ai/claude-code)

marcel commented

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

👨‍💻 Felix Brandt — Senior Fullstack Developer

Verdict: ✅ Approved

Clean, focused fix. The root cause diagnosis is correct — beforeNavigate was cancelling SvelteKit's internal redirect goto() because isDirty was still true at the time it fired. Calling clearOnSuccess() before await update() is the right ordering. I checked the hook API and this is exactly how you defuse the guard before SvelteKit takes over.

What works well

Correct hook placement: unsaved.clearOnSuccess() runs before await update(), so the guard is disarmed before SvelteKit's internal navigation fires. If the order were reversed, the bug would persist.
vi.hoisted() pattern: Correctly used to capture the enhance function reference before vi.mock() hoisting executes. The pattern is consistent with the TipTap workarounds established in this codebase.
oninput={unsaved.markDirty} instead of oninput={() => { isDirty = true; ... }}: Passing the handler reference directly rather than wrapping it is cleaner and avoids a closure that captured the now-deleted inline state.
Test naming: All 6 test names read as sentences describing observable behavior. "clears banner when enhance callback receives a redirect result" — clear, specific, meaningful.
Test coverage of the critical invariant: Both redirect (guard clears) and failure (guard persists) cases are explicitly tested. This is exactly what TDD requires for a bugfix.

Suggestions (non-blocking)

SubmitFn type is defined inline four times — twice in groups/new/page.svelte.spec.ts, twice in users/new/page.svelte.spec.ts. It's identical each time. A shared testHelpers.ts or a local type SubmitFn = ... at the top of each spec file would eliminate the duplication. Minor, but noticeable.

// At the top of each spec file, once:
type SubmitFn = () => Promise<
    (opts: { result: { type: string; [key: string]: unknown }; update: () => Promise<void> }) => Promise<void>
>;

No blockers. The fix is minimal and correct. The refactor aligns these pages with the pattern already used on the [id]/edit pages.

## 👨‍💻 Felix Brandt — Senior Fullstack Developer **Verdict: ✅ Approved** Clean, focused fix. The root cause diagnosis is correct — `beforeNavigate` was cancelling SvelteKit's internal redirect `goto()` because `isDirty` was still true at the time it fired. Calling `clearOnSuccess()` _before_ `await update()` is the right ordering. I checked the hook API and this is exactly how you defuse the guard before SvelteKit takes over. ### What works well - **Correct hook placement**: `unsaved.clearOnSuccess()` runs before `await update()`, so the guard is disarmed before SvelteKit's internal navigation fires. If the order were reversed, the bug would persist. - **`vi.hoisted()` pattern**: Correctly used to capture the enhance function reference before `vi.mock()` hoisting executes. The pattern is consistent with the TipTap workarounds established in this codebase. - **`oninput={unsaved.markDirty}` instead of `oninput={() => { isDirty = true; ... }}`**: Passing the handler reference directly rather than wrapping it is cleaner and avoids a closure that captured the now-deleted inline state. - **Test naming**: All 6 test names read as sentences describing observable behavior. "clears banner when enhance callback receives a redirect result" — clear, specific, meaningful. - **Test coverage of the critical invariant**: Both redirect (guard clears) and failure (guard persists) cases are explicitly tested. This is exactly what TDD requires for a bugfix. ### Suggestions (non-blocking) - **`SubmitFn` type is defined inline four times** — twice in `groups/new/page.svelte.spec.ts`, twice in `users/new/page.svelte.spec.ts`. It's identical each time. A shared `testHelpers.ts` or a local `type SubmitFn = ...` at the top of each spec file would eliminate the duplication. Minor, but noticeable. ```typescript // At the top of each spec file, once: type SubmitFn = () => Promise< (opts: { result: { type: string; [key: string]: unknown }; update: () => Promise<void> }) => Promise<void> >; ``` No blockers. The fix is minimal and correct. The refactor aligns these pages with the pattern already used on the `[id]/edit` pages.

marcel commented

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

🏛️ Markus Keller — Application Architect

Verdict: ✅ Approved

Pure frontend bugfix and refactor. I checked this against every row in my doc-update table — none are triggered:

Criterion	Status
New Flyway migration	✗ no backend changes
New backend package or domain module	✗
New SvelteKit route	✗ both pages already existed
New Docker service or infra component	✗
New `ErrorCode` or `Permission`	✗
New domain concept or term	✗
Auth or upload flow change	✗
Architectural decision with lasting consequences	✗

Pattern consistency: This PR correctly brings admin/groups/new and admin/users/new into alignment with the admin/groups/[id] and admin/users/[id] edit pages, which already use createUnsavedWarning() + <UnsavedWarningBanner>. This is exactly the right abstraction reuse — the hook encapsulates the isDirty, showUnsavedWarning, discardTarget state triangle that was duplicated across at least four pages. This PR eliminates two of those duplicates.

Layer boundaries: No boundary leaks. This is a component-level change with no cross-domain coupling introduced.

Nothing to flag architecturally. The change reduces complexity.

## 🏛️ Markus Keller — Application Architect **Verdict: ✅ Approved** Pure frontend bugfix and refactor. I checked this against every row in my doc-update table — none are triggered: | Criterion | Status | |---|---| | New Flyway migration | ✗ no backend changes | | New backend package or domain module | ✗ | | New SvelteKit route | ✗ both pages already existed | | New Docker service or infra component | ✗ | | New `ErrorCode` or `Permission` | ✗ | | New domain concept or term | ✗ | | Auth or upload flow change | ✗ | | Architectural decision with lasting consequences | ✗ | **Pattern consistency**: This PR correctly brings `admin/groups/new` and `admin/users/new` into alignment with the `admin/groups/[id]` and `admin/users/[id]` edit pages, which already use `createUnsavedWarning()` + `<UnsavedWarningBanner>`. This is exactly the right abstraction reuse — the hook encapsulates the `isDirty`, `showUnsavedWarning`, `discardTarget` state triangle that was duplicated across at least four pages. This PR eliminates two of those duplicates. **Layer boundaries**: No boundary leaks. This is a component-level change with no cross-domain coupling introduced. Nothing to flag architecturally. The change reduces complexity.

marcel commented

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

🔧 Tobias Wendt — DevOps & Platform Engineer

Verdict: ✅ Approved

Nothing in my domain is touched by this PR — no CI workflow changes, no Compose file modifications, no Docker image updates, no secrets, no infrastructure configuration.

I checked:

.gitea/workflows/: not modified
docker-compose files: not modified
Any new environment variables: none introduced
Artifact upload steps: unaffected

The test additions are Vitest browser-mode tests — they run in the existing frontend unit test job, no new CI infrastructure needed.

LGTM from an infra perspective.

## 🔧 Tobias Wendt — DevOps & Platform Engineer **Verdict: ✅ Approved** Nothing in my domain is touched by this PR — no CI workflow changes, no Compose file modifications, no Docker image updates, no secrets, no infrastructure configuration. I checked: - **.gitea/workflows/**: not modified - **docker-compose files**: not modified - **Any new environment variables**: none introduced - **Artifact upload steps**: unaffected The test additions are Vitest browser-mode tests — they run in the existing frontend unit test job, no new CI infrastructure needed. LGTM from an infra perspective.

marcel commented

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

📋 Elicit — Requirements Engineer

Verdict: ✅ Approved

Reviewing against the implied acceptance criteria for issue #527.

Requirements satisfied

The fix addresses two observable behaviors:

FR-1 (implicit): After a successful form submission that results in a redirect, the unsaved-changes banner MUST NOT appear.
→ Tested: "clears banner when enhance callback receives a redirect result" ✓

FR-2 (implicit): After a failed form submission (failure result), the unsaved-changes banner MUST remain visible if the form was dirty.
→ Tested: "keeps banner when enhance callback receives a failure result" ✓

FR-3 (implicit): A clean form (no user input) MUST NOT trigger the unsaved-changes guard during navigation.
→ Tested: "does not cancel navigation when form is clean" ✓

FR-4 (implicit): A dirty form MUST cancel navigation and show the banner.
→ Tested: "cancels navigation and shows banner when form is dirty" ✓

Open question (informational, not blocking)

SvelteKit distinguishes three result types beyond redirect and failure: error, success, and redirect. The enhance callback only calls clearOnSuccess() for redirect. What about success (a 2xx response without redirect, which would keep the user on the same page)? In this form's current +page.server.ts, a 2xx success path presumably doesn't exist — the action either redirects or fails — so this is not a present gap. Worth noting if the action contract changes in future.

No blockers from a requirements perspective. The fix is well-scoped to the reported issue.

## 📋 Elicit — Requirements Engineer **Verdict: ✅ Approved** Reviewing against the implied acceptance criteria for issue #527. ### Requirements satisfied The fix addresses two observable behaviors: **FR-1 (implicit)**: After a successful form submission that results in a redirect, the unsaved-changes banner MUST NOT appear. → Tested: "clears banner when enhance callback receives a redirect result" ✓ **FR-2 (implicit)**: After a failed form submission (`failure` result), the unsaved-changes banner MUST remain visible if the form was dirty. → Tested: "keeps banner when enhance callback receives a failure result" ✓ **FR-3 (implicit)**: A clean form (no user input) MUST NOT trigger the unsaved-changes guard during navigation. → Tested: "does not cancel navigation when form is clean" ✓ **FR-4 (implicit)**: A dirty form MUST cancel navigation and show the banner. → Tested: "cancels navigation and shows banner when form is dirty" ✓ ### Open question (informational, not blocking) SvelteKit distinguishes three result types beyond `redirect` and `failure`: `error`, `success`, and `redirect`. The enhance callback only calls `clearOnSuccess()` for `redirect`. What about `success` (a 2xx response without redirect, which would keep the user on the same page)? In this form's current `+page.server.ts`, a 2xx success path presumably doesn't exist — the action either redirects or fails — so this is not a present gap. Worth noting if the action contract changes in future. No blockers from a requirements perspective. The fix is well-scoped to the reported issue.

marcel commented

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

🔐 Nora "NullX" Steiner — Application Security Engineer

Verdict: ✅ Approved

No security-relevant changes in this PR. I reviewed it through my standard lens:

Check	Result
User-controlled content rendered in banner	✗ — Banner uses `m.admin_unsaved_warning()` (Paraglide i18n key), no user input reflected
New API endpoint or auth boundary	✗ — Frontend-only change
Permission changes (`@RequirePermission`)	✗ — No backend changes
XSS risk in template	✗ — `<UnsavedWarningBanner>` renders only static i18n text and a button
CSRF surface change	✗ — The enhance callback only reads `result.type`, a locally-typed value from SvelteKit's form action framework
New external data path	✗
`localStorage`, `sessionStorage`, or cookies touched	✗

The createUnsavedWarning() hook is pure client-side state management. The enhance callback receives a result object from SvelteKit's action layer — it's not derived from untrusted input.

Nothing to flag.

## 🔐 Nora "NullX" Steiner — Application Security Engineer **Verdict: ✅ Approved** No security-relevant changes in this PR. I reviewed it through my standard lens: | Check | Result | |---|---| | User-controlled content rendered in banner | ✗ — Banner uses `m.admin_unsaved_warning()` (Paraglide i18n key), no user input reflected | | New API endpoint or auth boundary | ✗ — Frontend-only change | | Permission changes (`@RequirePermission`) | ✗ — No backend changes | | XSS risk in template | ✗ — `<UnsavedWarningBanner>` renders only static i18n text and a button | | CSRF surface change | ✗ — The enhance callback only reads `result.type`, a locally-typed value from SvelteKit's form action framework | | New external data path | ✗ | | `localStorage`, `sessionStorage`, or cookies touched | ✗ | The `createUnsavedWarning()` hook is pure client-side state management. The enhance callback receives a `result` object from SvelteKit's action layer — it's not derived from untrusted input. Nothing to flag.

marcel commented

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

🧪 Sara Holt — QA Engineer & Test Strategist

Verdict: ⚠️ Approved with concerns

Overall the test coverage is solid and correctly verifies the bug fix. I have one suggestion and one informational note.

What works well

6 targeted behavior tests per page — each has one logical assertion (or tightly coupled pair for the redirect test which verifies both state reset AND guard disabled together, which is acceptable since they are the same observable outcome).
Descriptive test names that read as sentences — pass my name-as-sentence rule.
beforeEach resets vi.clearAllMocks() and enhanceCaptureRef.submitFn — prevents test bleed across runs.
vitest-browser-svelte used throughout — real DOM, not JSDOM. The dispatchEvent(new InputEvent('input', { bubbles: true })) pattern for triggering oninput is the established workaround for this project.
Test pyramid placement: These are correctly placed at the component layer (unit-equivalent), not as E2E tests.

Suggestion (non-blocking)

SubmitFn type defined inline 4 times across 2 spec files. Each occurrence is identical. Extract it to a shared utility or at minimum define it once per file:

// At the top of the describe block or file:
type SubmitFn = () => Promise<
    (opts: { result: { type: string; [key: string]: unknown }; update: () => Promise<void> }) => Promise<void>
>;

This doesn't affect test reliability, but it reduces maintenance surface.

Informational note (not a blocker for this PR)

There is no test for result.type === 'error' (thrown by SvelteKit's error() helper, distinct from failure()). In the current action implementation this path may be unreachable, but if a future maintainer adds an error() call, the guard behavior for that case would be untested. Since the existing failure test already covers the "don't clear on non-redirect" invariant and these share the same code path (the if (result.type === 'redirect') check), this is informational only.

The pre-existing tests (groups/new/page.svelte.test.ts and users/new/page.svelte.test.ts) still pass — no regressions in rendering behavior.

## 🧪 Sara Holt — QA Engineer & Test Strategist **Verdict: ⚠️ Approved with concerns** Overall the test coverage is solid and correctly verifies the bug fix. I have one suggestion and one informational note. ### What works well - **6 targeted behavior tests** per page — each has one logical assertion (or tightly coupled pair for the redirect test which verifies both state reset AND guard disabled together, which is acceptable since they are the same observable outcome). - **Descriptive test names** that read as sentences — pass my name-as-sentence rule. - **`beforeEach` resets** `vi.clearAllMocks()` and `enhanceCaptureRef.submitFn` — prevents test bleed across runs. - **`vitest-browser-svelte`** used throughout — real DOM, not JSDOM. The `dispatchEvent(new InputEvent('input', { bubbles: true }))` pattern for triggering `oninput` is the established workaround for this project. - **Test pyramid placement**: These are correctly placed at the component layer (unit-equivalent), not as E2E tests. ### Suggestion (non-blocking) **`SubmitFn` type defined inline 4 times across 2 spec files.** Each occurrence is identical. Extract it to a shared utility or at minimum define it once per file: ```typescript // At the top of the describe block or file: type SubmitFn = () => Promise< (opts: { result: { type: string; [key: string]: unknown }; update: () => Promise<void> }) => Promise<void> >; ``` This doesn't affect test reliability, but it reduces maintenance surface. ### Informational note (not a blocker for this PR) There is no test for `result.type === 'error'` (thrown by SvelteKit's `error()` helper, distinct from `failure()`). In the current action implementation this path may be unreachable, but if a future maintainer adds an `error()` call, the guard behavior for that case would be untested. Since the existing `failure` test already covers the "don't clear on non-redirect" invariant and these share the same code path (the `if (result.type === 'redirect')` check), this is informational only. The pre-existing tests (`groups/new/page.svelte.test.ts` and `users/new/page.svelte.test.ts`) still pass — no regressions in rendering behavior.

marcel commented

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

🎨 Leonie Voss — UX Designer & Accessibility Strategist

Verdict: ✅ Approved

This is a UX regression fix — and a meaningful one. The original bug caused the amber "unsaved changes" banner to appear after a successful save, right before the redirect. For a 60+ user who just filled out a "New Group" form and submitted it, seeing an amber warning saying their changes are unsaved — immediately after saving — is genuinely confusing. It undermines trust in the interface ("did my save work?") and violates Nielsen Heuristic #1 (Visibility of system status) and Heuristic #4 (Consistency and standards — the edit pages don't have this problem).

What I verified

Component reuse: The PR uses <UnsavedWarningBanner> — the same component already used on the [id]/edit pages. Visual consistency is maintained. The amber banner style, the "Verwerfen" discard button, and the i18n keys are unchanged.
No new hardcoded colors: The deleted inline banner code used raw Tailwind amber utilities (border-amber-200 bg-amber-50 text-amber-800). The replacement <UnsavedWarningBanner> component encapsulates those — correct either way, but the component is the right abstraction.
No touch target regression: The discard button inside UnsavedWarningBanner should already meet the 44px minimum. This PR doesn't change that.
i18n: The test asserts /ungespeicherte Änderungen/i which confirms the admin_unsaved_warning key is in use — no new raw German strings introduced in the component.

One note for future work (not this PR)

The amber banner currently has no aria-live attribute to announce itself to screen readers when it appears. This pre-exists this PR and would be a separate accessibility issue to file against UnsavedWarningBanner. Not a blocker here since the banner behavior itself is unchanged.

This fix makes the admin create flows consistent with the edit flows. Good housekeeping.

## 🎨 Leonie Voss — UX Designer & Accessibility Strategist **Verdict: ✅ Approved** This is a UX regression fix — and a meaningful one. The original bug caused the amber "unsaved changes" banner to appear _after_ a successful save, right before the redirect. For a 60+ user who just filled out a "New Group" form and submitted it, seeing an amber warning saying their changes are unsaved — immediately after saving — is genuinely confusing. It undermines trust in the interface ("did my save work?") and violates Nielsen Heuristic #1 (Visibility of system status) and Heuristic #4 (Consistency and standards — the edit pages don't have this problem). ### What I verified - **Component reuse**: The PR uses `<UnsavedWarningBanner>` — the same component already used on the `[id]/edit` pages. Visual consistency is maintained. The amber banner style, the "Verwerfen" discard button, and the i18n keys are unchanged. - **No new hardcoded colors**: The deleted inline banner code used raw Tailwind amber utilities (`border-amber-200 bg-amber-50 text-amber-800`). The replacement `<UnsavedWarningBanner>` component encapsulates those — correct either way, but the component is the right abstraction. - **No touch target regression**: The discard button inside `UnsavedWarningBanner` should already meet the 44px minimum. This PR doesn't change that. - **i18n**: The test asserts `/ungespeicherte Änderungen/i` which confirms the `admin_unsaved_warning` key is in use — no new raw German strings introduced in the component. ### One note for future work (not this PR) The amber banner currently has no `aria-live` attribute to announce itself to screen readers when it appears. This pre-exists this PR and would be a separate accessibility issue to file against `UnsavedWarningBanner`. Not a blocker here since the banner behavior itself is unchanged. This fix makes the admin create flows consistent with the edit flows. Good housekeeping.

marcel added 3 commits 2026-05-14 12:09:17 +02:00

fix(admin): clear unsaved-changes guard before redirect on groups/new 30469e74c9

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>

fix(admin): clear unsaved-changes guard before redirect on users/new ffcb901376

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>

fix(test): allow extra result properties in enhance callback type

CI / Unit & Component Tests (pull_request) Successful in 3m8s

Details

CI / OCR Service Tests (pull_request) Successful in 17s

Details

CI / fail2ban Regex (pull_request) Successful in 48s

Details

CI / Compose Bucket Idempotency (pull_request) Successful in 58s

Details

CI / Backend Unit Tests (pull_request) Failing after 17m19s

Details

6fffc06c28

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>

marcel force-pushed feat/issue-527-unsaved-warning-new-pages from 07ae9b61fb to 6fffc06c28

2026-05-14 12:09:17 +02:00

Compare

marcel added 1 commit 2026-05-14 12:12:14 +02:00

refactor(test): hoist SubmitFn to file-level type in unsaved-guard specs

CI / Unit & Component Tests (pull_request) Successful in 4m9s

Details

CI / OCR Service Tests (pull_request) Successful in 20s

Details

CI / Backend Unit Tests (pull_request) Successful in 5m8s

Details

CI / fail2ban Regex (pull_request) Successful in 48s

Details

CI / Compose Bucket Idempotency (pull_request) Successful in 1m3s

Details

2ca8428be4

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

marcel commented

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

Review concerns addressed

Concern raised by @felix and @sara (non-blocking): SubmitFn type defined inline 4 times — twice in each spec file.

Fix (commit 2ca8428b): Extracted SubmitFn to a file-level type alias at the top of each spec file, above the describe blocks. Removed the 4 inline definitions. The type itself is unchanged — pure structural deduplication.

All other reviewer notes (Elicit's success-result observation, Sara's error-result note, Leonie's aria-live note) were explicitly flagged as informational / future work and are not addressed in this PR.

## Review concerns addressed **Concern raised by @felix and @sara (non-blocking):** `SubmitFn` type defined inline 4 times — twice in each spec file. **Fix (commit `2ca8428b`):** Extracted `SubmitFn` to a file-level `type` alias at the top of each spec file, above the `describe` blocks. Removed the 4 inline definitions. The type itself is unchanged — pure structural deduplication. All other reviewer notes (Elicit's `success`-result observation, Sara's `error`-result note, Leonie's `aria-live` note) were explicitly flagged as informational / future work and are not addressed in this PR.

CI / Unit & Component Tests (pull_request) Successful in 4m9s

Details

CI / OCR Service Tests (pull_request) Successful in 20s

Details

CI / Backend Unit Tests (pull_request) Successful in 5m8s

Details

CI / fail2ban Regex (pull_request) Successful in 48s

Details

CI / Compose Bucket Idempotency (pull_request) Successful in 1m3s

Details

This pull request can be merged automatically.

You are not authorized to merge this pull request.

View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u origin feat/issue-527-unsaved-warning-new-pages:feat/issue-527-unsaved-warning-new-pages

git checkout feat/issue-527-unsaved-warning-new-pages

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: marcel/familienarchiv#567