"Unsaved changes" banner appears after creating a group/user — users think save failed #527

New Issue

Open

opened 2026-05-11 19:06:41 +02:00 by marcel · 7 comments

marcel commented 2026-05-11 19:06:41 +02:00

Owner

Copy Link

Context

The admin "new group" and "new user" forms both register an inline beforeNavigate guard that cancels navigation while a local isDirty flag is true. The server actions complete with throw redirect(303, …). When use:enhance follows that redirect via client-side goto(), beforeNavigate fires — isDirty is still true, so navigation is cancelled and the "unsaved changes" amber banner is shown.

The POST itself succeeded: the record is in the DB, and a manual reload reveals the new group/user in the list. But from the user's perspective the save looks like it failed.

The sibling edit page (admin/groups/[id]/+page.svelte) does not have this problem because it uses the shared createUnsavedWarning() hook + clears dirty state on form?.success. The two /new pages predate that refactor and still use the inline copy.

User-facing impact

Step	What the user sees
1. Open /admin/groups/new, fill in name, tick permissions	Form fills normally
2. Click Erstellen	Page stays on /admin/groups/new, amber "Du hast ungespeicherte Änderungen" banner appears
3. User assumes save failed, retries or panics	(Group has actually been created — visible after reload or by navigating away with "Verwerfen")

Reproducible 100% of the time on both pages.

Root cause

frontend/src/routes/admin/groups/new/+page.svelte:25-31

beforeNavigate(({ cancel, to }) => {
    if (isDirty) {
        cancel();
        showUnsavedWarning = true;
        discardTarget = to?.url.href ?? null;
    }
});

The oninput handler at line 89 sets isDirty = true. use:enhance at line 88 has no callback, so on a redirect result it calls goto('/admin/groups') internally — which triggers the guard above and gets cancelled.

Identical pattern in frontend/src/routes/admin/users/new/+page.svelte:15-21 (same bug — confirmed the action throws redirect(303, '/admin/users') at +page.server.ts:42).

Affected files

• frontend/src/routes/admin/groups/new/+page.svelte
• frontend/src/routes/admin/users/new/+page.svelte

Fix

Refactor both pages to match the edit-page pattern:

1. Replace the inline isDirty / showUnsavedWarning / discardTarget / beforeNavigate block with createUnsavedWarning() from $lib/shared/hooks/useUnsavedWarning.svelte.
2. Replace the inline amber banner markup with <UnsavedWarningBanner onDiscard={unsaved.discard} /> from $lib/shared/primitives/UnsavedWarningBanner.svelte.
3. Wire oninput={unsaved.markDirty} on the form.
4. Pass an enhance callback that clears dirty state when the server returns a redirect, before update() runs:

use:enhance={() => async ({ result, update }) => {
    if (result.type === 'redirect') unsaved.clearOnSuccess();
    await update();
}}

Step 4 is the actual bug fix; steps 1–3 are cleanup that makes both /new pages consistent with [id]/+page.svelte.

Acceptance criteria (Gherkin)

Scenario: Successful group creation navigates to the list
  Given I am on /admin/groups/new
  And I have entered a name and ticked at least one permission
  When I click "Erstellen"
  Then the server creates the group
  And I am navigated to /admin/groups
  And no "unsaved changes" banner is shown

Scenario: Cancelling navigation with unsaved changes still warns
  Given I am on /admin/groups/new
  And I have entered a name
  When I click the "Abbrechen" link or any other nav target
  Then navigation is cancelled
  And the "unsaved changes" banner is shown
  And clicking "Verwerfen" navigates to the intended target

Scenario: Failed group creation keeps the user on the form
  Given I am on /admin/groups/new
  And I have entered a duplicate name
  When I click "Erstellen"
  Then I remain on /admin/groups/new
  And the server-side error message is shown
  And no "unsaved changes" banner is shown

Scenario: Same behaviour for /admin/users/new
  Given the user-creation form is filled in correctly
  When I submit
  Then I land on /admin/users with no unsaved-changes banner

Test plan

• Component spec: extend frontend/src/routes/admin/groups/new with a spec that submits the form, asserts the form action returns a redirect, and asserts no UnsavedWarningBanner is rendered. Use the existing pattern in admin/groups/[id]/page.svelte.spec.ts as a reference.
• Mirror the spec for admin/users/new.
• Manual Playwright check via dev admin (admin@familyarchive.local / admin123): create a group, confirm redirect to /admin/groups, confirm new group appears in the list without reload.

Out of scope

• Re-styling the banner or the shared hook.
• Touching the edit pages ([id]/+page.svelte) — those already work correctly.
• Any other /new pages: a grep for beforeNavigate in src/routes shows only these two inline copies remain.

## Context The admin "new group" and "new user" forms both register an inline `beforeNavigate` guard that cancels navigation while a local `isDirty` flag is `true`. The server actions complete with `throw redirect(303, …)`. When `use:enhance` follows that redirect via client-side `goto()`, `beforeNavigate` fires — `isDirty` is still `true`, so navigation is **cancelled** and the "unsaved changes" amber banner is shown. The POST itself succeeded: the record is in the DB, and a manual reload reveals the new group/user in the list. But from the user's perspective the save *looks* like it failed. The sibling edit page (`admin/groups/[id]/+page.svelte`) does not have this problem because it uses the shared `createUnsavedWarning()` hook + clears dirty state on `form?.success`. The two `/new` pages predate that refactor and still use the inline copy. ## User-facing impact | Step | What the user sees | | --- | --- | | 1. Open `/admin/groups/new`, fill in name, tick permissions | Form fills normally | | 2. Click **Erstellen** | Page stays on `/admin/groups/new`, amber "Du hast ungespeicherte Änderungen" banner appears | | 3. User assumes save failed, retries or panics | (Group has actually been created — visible after reload or by navigating away with "Verwerfen") | Reproducible 100% of the time on both pages. ## Root cause `frontend/src/routes/admin/groups/new/+page.svelte:25-31` ```svelte beforeNavigate(({ cancel, to }) => { if (isDirty) { cancel(); showUnsavedWarning = true; discardTarget = to?.url.href ?? null; } }); ``` The `oninput` handler at line 89 sets `isDirty = true`. `use:enhance` at line 88 has no callback, so on a `redirect` result it calls `goto('/admin/groups')` internally — which triggers the guard above and gets cancelled. Identical pattern in `frontend/src/routes/admin/users/new/+page.svelte:15-21` (same bug — confirmed the action `throws redirect(303, '/admin/users')` at `+page.server.ts:42`). ## Affected files - `frontend/src/routes/admin/groups/new/+page.svelte` - `frontend/src/routes/admin/users/new/+page.svelte` ## Fix Refactor both pages to match the edit-page pattern: 1. Replace the inline `isDirty` / `showUnsavedWarning` / `discardTarget` / `beforeNavigate` block with `createUnsavedWarning()` from `$lib/shared/hooks/useUnsavedWarning.svelte`. 2. Replace the inline amber banner markup with `<UnsavedWarningBanner onDiscard={unsaved.discard} />` from `$lib/shared/primitives/UnsavedWarningBanner.svelte`. 3. Wire `oninput={unsaved.markDirty}` on the form. 4. Pass an `enhance` callback that clears dirty state when the server returns a redirect, **before** `update()` runs: ```svelte use:enhance={() => async ({ result, update }) => { if (result.type === 'redirect') unsaved.clearOnSuccess(); await update(); }} ``` Step 4 is the actual bug fix; steps 1–3 are cleanup that makes both `/new` pages consistent with `[id]/+page.svelte`. ## Acceptance criteria (Gherkin) ``` Scenario: Successful group creation navigates to the list Given I am on /admin/groups/new And I have entered a name and ticked at least one permission When I click "Erstellen" Then the server creates the group And I am navigated to /admin/groups And no "unsaved changes" banner is shown Scenario: Cancelling navigation with unsaved changes still warns Given I am on /admin/groups/new And I have entered a name When I click the "Abbrechen" link or any other nav target Then navigation is cancelled And the "unsaved changes" banner is shown And clicking "Verwerfen" navigates to the intended target Scenario: Failed group creation keeps the user on the form Given I am on /admin/groups/new And I have entered a duplicate name When I click "Erstellen" Then I remain on /admin/groups/new And the server-side error message is shown And no "unsaved changes" banner is shown Scenario: Same behaviour for /admin/users/new Given the user-creation form is filled in correctly When I submit Then I land on /admin/users with no unsaved-changes banner ``` ## Test plan - Component spec: extend `frontend/src/routes/admin/groups/new` with a spec that submits the form, asserts the form action returns a redirect, and asserts no `UnsavedWarningBanner` is rendered. Use the existing pattern in `admin/groups/[id]/page.svelte.spec.ts` as a reference. - Mirror the spec for `admin/users/new`. - Manual Playwright check via dev admin (`admin@familyarchive.local` / `admin123`): create a group, confirm redirect to `/admin/groups`, confirm new group appears in the list without reload. ## Out of scope - Re-styling the banner or the shared hook. - Touching the edit pages (`[id]/+page.svelte`) — those already work correctly. - Any other `/new` pages: a grep for `beforeNavigate` in `src/routes` shows only these two inline copies remain.

marcel added the P1-highbugui labels 2026-05-11 19:06:48 +02:00

marcel commented 2026-05-14 11:11:33 +02:00

Author

Owner

Copy Link

👨‍💻 Felix Brandt — Senior Fullstack Developer

Observations

• The root cause analysis is correct and the four-step fix is complete. Step 4 (if (result.type === 'redirect') unsaved.clearOnSuccess() before await update()) is the actual bug fix; steps 1–3 are the consistency cleanup.
• The key ordering guarantee: clearOnSuccess() sets isDirty = false before update() triggers the redirect navigation. By the time SvelteKit internally calls goto(), isDirty is already false and the beforeNavigate guard lets navigation through. The fix depends on this ordering.
• The edit page ([id]/+page.svelte) clears dirty via $effect(() => { if (form?.success) unsaved.clearOnSuccess(); }). That approach works because the edit action returns a success flag. The new pages throw redirect(303, ...) — form is never updated — so the $effect approach is structurally impossible here. The enhance callback is the correct mechanism.

Recommendations

• The users/new/page.svelte.spec.ts currently has no mock for $app/navigation at all. Introducing createUnsavedWarning() will cause the spec to fail at import time because beforeNavigate won't be defined. Add the same vi.mock('$app/navigation', () => ({ beforeNavigate: vi.fn(), goto: vi.fn() })) that the edit page spec uses.
• The existing groups/new/page.svelte.test.ts mocks beforeNavigate: () => {} (a no-op, not tracked). That's compatible for rendering tests but blocks the new unsaved-warning tests. The new spec file should use vi.fn() so the registered callback can be captured and invoked, exactly as in [id]/page.svelte.spec.ts.
• The test plan says "use the existing pattern in admin/groups/[id]/page.svelte.spec.ts" but the edit spec clears dirty via rerender({ form: { success: true } }), which won't work for redirect-based flows. The enhance mock in that file (enhance: () => () => {}) discards the callback entirely. The new spec needs a mock that captures the SubmitFunction so the redirect result can be simulated. Concrete mock structure:

let capturedSubmitFn: ((args: unknown) => Promise<((opts: unknown) => Promise<void>) | void>) | null = null;

vi.mock('$app/forms', () => ({
    enhance: (_el: HTMLFormElement, fn?: typeof capturedSubmitFn) => {
        capturedSubmitFn = fn ?? null;
        return { destroy: vi.fn() };
    }
}));

// In test: simulate the enhance callback being invoked with a redirect result
it('clears unsaved warning when server responds with redirect', async () => {
    render(Page, { props: { form: null } });
    document.querySelector<HTMLInputElement>('input[name="name"]')!
        .dispatchEvent(new InputEvent('input', { bubbles: true }));
    // Trigger the beforeNavigate guard to show the banner
    const [navCb] = vi.mocked(beforeNavigate).mock.calls[0];
    navCb({ cancel: vi.fn(), to: { url: new URL('http://localhost/admin/groups') } });
    await expect.element(page.getByText(/ungespeicherte Änderungen/i)).toBeVisible();

    const innerHandler = await capturedSubmitFn!({ /* minimal SubmitFunction args */ });
    await innerHandler!({ result: { type: 'redirect', location: '/admin/groups', status: 303 }, update: vi.fn().mockResolvedValue(undefined) });

    await expect.element(page.getByText(/ungespeicherte Änderungen/i)).not.toBeInTheDocument();
});

## 👨‍💻 Felix Brandt — Senior Fullstack Developer ### Observations - The root cause analysis is correct and the four-step fix is complete. Step 4 (`if (result.type === 'redirect') unsaved.clearOnSuccess()` before `await update()`) is the actual bug fix; steps 1–3 are the consistency cleanup. - The key ordering guarantee: `clearOnSuccess()` sets `isDirty = false` *before* `update()` triggers the redirect navigation. By the time SvelteKit internally calls `goto()`, `isDirty` is already `false` and the `beforeNavigate` guard lets navigation through. The fix depends on this ordering. - The edit page (`[id]/+page.svelte`) clears dirty via `$effect(() => { if (form?.success) unsaved.clearOnSuccess(); })`. That approach works because the edit action returns a `success` flag. The new pages throw `redirect(303, ...)` — `form` is never updated — so the `$effect` approach is structurally impossible here. The enhance callback is the correct mechanism. ### Recommendations - The `users/new/page.svelte.spec.ts` currently has **no mock for `$app/navigation`** at all. Introducing `createUnsavedWarning()` will cause the spec to fail at import time because `beforeNavigate` won't be defined. Add the same `vi.mock('$app/navigation', () => ({ beforeNavigate: vi.fn(), goto: vi.fn() }))` that the edit page spec uses. - The existing `groups/new/page.svelte.test.ts` mocks `beforeNavigate: () => {}` (a no-op, not tracked). That's compatible for rendering tests but blocks the new unsaved-warning tests. The new **spec** file should use `vi.fn()` so the registered callback can be captured and invoked, exactly as in `[id]/page.svelte.spec.ts`. - The test plan says "use the existing pattern in `admin/groups/[id]/page.svelte.spec.ts`" but the edit spec clears dirty via `rerender({ form: { success: true } })`, which won't work for redirect-based flows. The `enhance` mock in that file (`enhance: () => () => {}`) discards the callback entirely. The new spec needs a mock that captures the SubmitFunction so the redirect result can be simulated. Concrete mock structure: ```ts let capturedSubmitFn: ((args: unknown) => Promise<((opts: unknown) => Promise<void>) | void>) | null = null; vi.mock('$app/forms', () => ({ enhance: (_el: HTMLFormElement, fn?: typeof capturedSubmitFn) => { capturedSubmitFn = fn ?? null; return { destroy: vi.fn() }; } })); // In test: simulate the enhance callback being invoked with a redirect result it('clears unsaved warning when server responds with redirect', async () => { render(Page, { props: { form: null } }); document.querySelector<HTMLInputElement>('input[name="name"]')! .dispatchEvent(new InputEvent('input', { bubbles: true })); // Trigger the beforeNavigate guard to show the banner const [navCb] = vi.mocked(beforeNavigate).mock.calls[0]; navCb({ cancel: vi.fn(), to: { url: new URL('http://localhost/admin/groups') } }); await expect.element(page.getByText(/ungespeicherte Änderungen/i)).toBeVisible(); const innerHandler = await capturedSubmitFn!({ /* minimal SubmitFunction args */ }); await innerHandler!({ result: { type: 'redirect', location: '/admin/groups', status: 303 }, update: vi.fn().mockResolvedValue(undefined) }); await expect.element(page.getByText(/ungespeicherte Änderungen/i)).not.toBeInTheDocument(); }); ```

marcel commented 2026-05-14 11:11:42 +02:00

Author

Owner

Copy Link

🏗️ Markus Keller — Application Architect

Observations

• This is exactly the right kind of cleanup: two inline copies of shared state logic are replaced with the single shared hook (createUnsavedWarning) that already exists for this purpose. DRY is applied correctly.
• The project now has two legitimate mechanisms for clearing dirty state on success, and both are correct for their flow type:
  • Edit pages: $effect(() => { if (form?.success) unsaved.clearOnSuccess(); }) — server action returns { success: true }, form is updated, $effect fires.
  • Create pages: enhance callback if (result.type === 'redirect') unsaved.clearOnSuccess() — server action throws redirect(303, ...), form is never updated.
    These are different server-side flow shapes, not inconsistency. No unification is needed or appropriate.
• The shared hook encapsulates the beforeNavigate registration, isDirty, showUnsavedWarning, and discardTarget state in one place. The banner is a standalone component with a single onDiscard prop. The composition pattern is clean.

Recommendations

• No doc updates required: no new routes, entities, migrations, ADRs, or architectural decisions are introduced. This is an internal refactor of existing pages.
• Verify that after the refactor no other uses of the inline pattern remain. The issue states a grep for beforeNavigate in src/routes showed only these two files — confirm this holds after the fix to avoid a third copy appearing unnoticed.

## 🏗️ Markus Keller — Application Architect ### Observations - This is exactly the right kind of cleanup: two inline copies of shared state logic are replaced with the single shared hook (`createUnsavedWarning`) that already exists for this purpose. DRY is applied correctly. - The project now has **two legitimate mechanisms** for clearing dirty state on success, and both are correct for their flow type: - Edit pages: `$effect(() => { if (form?.success) unsaved.clearOnSuccess(); })` — server action returns `{ success: true }`, `form` is updated, `$effect` fires. - Create pages: enhance callback `if (result.type === 'redirect') unsaved.clearOnSuccess()` — server action throws `redirect(303, ...)`, `form` is never updated. These are different server-side flow shapes, not inconsistency. No unification is needed or appropriate. - The shared hook encapsulates the `beforeNavigate` registration, `isDirty`, `showUnsavedWarning`, and `discardTarget` state in one place. The banner is a standalone component with a single `onDiscard` prop. The composition pattern is clean. ### Recommendations - No doc updates required: no new routes, entities, migrations, ADRs, or architectural decisions are introduced. This is an internal refactor of existing pages. - Verify that after the refactor no other uses of the inline pattern remain. The issue states a `grep` for `beforeNavigate` in `src/routes` showed only these two files — confirm this holds after the fix to avoid a third copy appearing unnoticed.

marcel commented 2026-05-14 11:11:54 +02:00

Author

Owner

Copy Link

🧪 Sara Holt — QA Engineer

Observations

• The test plan is underspecified for the most important scenario: "redirect result clears dirty state". This is the actual bug scenario and it requires a more involved mock than the reference spec uses.
• The reference file (admin/groups/[id]/page.svelte.spec.ts) tests dirty-state clearing by re-rendering with form: { success: true }, which triggers a $effect. That mechanism does not exist in the new pages (no form.success, only a redirect). The test for this case cannot reuse that pattern.
• The existing groups/new/page.svelte.test.ts already has basic coverage (rendering, checkboxes, error banner). The new spec file should extend coverage with the unsaved-warning behavior, not duplicate the rendering tests.
• users/new/page.svelte.spec.ts has no mock for $app/navigation at all. Once createUnsavedWarning() is introduced (which calls beforeNavigate at module load), the spec will throw at import time.

Recommendations

• The spec files for both pages need vi.mock('$app/navigation', () => ({ beforeNavigate: vi.fn(), goto: vi.fn() })) — not the no-op version in page.svelte.test.ts.
• The $app/forms mock must capture the SubmitFunction callback to enable redirect simulation. See Felix's comment for the concrete mock structure.
• Minimum test cases needed for the new spec (both groups/new and users/new):
  1. No unsaved warning shown initially
  2. Dirty input → navigate away → warning shown, navigation cancelled
  3. Dirty input → discard clicked → goto() called with the target URL
  4. Dirty input → enhance callback invoked with { type: 'redirect' } result → warning hidden, isDirty false
  5. Dirty input → enhance callback invoked with { type: 'failure' } result → warning still shown (dirty preserved)
• Case 5 is currently missing from the acceptance criteria. A failed submission (form.error is set) must leave the form dirty so the guard still protects unsaved work.
• The hook's own unit test (useUnsavedWarning.svelte.test.ts) already covers clearOnSuccess() directly — the component spec can stay slim by delegating hook-level assertions to that file.

## 🧪 Sara Holt — QA Engineer ### Observations - The test plan is underspecified for the most important scenario: **"redirect result clears dirty state"**. This is the actual bug scenario and it requires a more involved mock than the reference spec uses. - The reference file (`admin/groups/[id]/page.svelte.spec.ts`) tests dirty-state clearing by re-rendering with `form: { success: true }`, which triggers a `$effect`. That mechanism **does not exist** in the new pages (no `form.success`, only a redirect). The test for this case cannot reuse that pattern. - The existing `groups/new/page.svelte.test.ts` already has basic coverage (rendering, checkboxes, error banner). The new spec file should extend coverage with the unsaved-warning behavior, not duplicate the rendering tests. - `users/new/page.svelte.spec.ts` has no mock for `$app/navigation` at all. Once `createUnsavedWarning()` is introduced (which calls `beforeNavigate` at module load), the spec will throw at import time. ### Recommendations - The **spec files** for both pages need `vi.mock('$app/navigation', () => ({ beforeNavigate: vi.fn(), goto: vi.fn() }))` — not the no-op version in `page.svelte.test.ts`. - The `$app/forms` mock must capture the `SubmitFunction` callback to enable redirect simulation. See Felix's comment for the concrete mock structure. - Minimum test cases needed for the new spec (both `groups/new` and `users/new`): 1. No unsaved warning shown initially 2. Dirty input → navigate away → warning shown, navigation cancelled 3. Dirty input → discard clicked → `goto()` called with the target URL 4. **Dirty input → enhance callback invoked with `{ type: 'redirect' }` result → warning hidden, `isDirty` false** 5. Dirty input → enhance callback invoked with `{ type: 'failure' }` result → warning still shown (dirty preserved) - Case 5 is currently missing from the acceptance criteria. A failed submission (`form.error` is set) must leave the form dirty so the guard still protects unsaved work. - The hook's own unit test (`useUnsavedWarning.svelte.test.ts`) already covers `clearOnSuccess()` directly — the component spec can stay slim by delegating hook-level assertions to that file.

marcel commented 2026-05-14 11:11:59 +02:00

Author

Owner

Copy Link

🔐 Nora "NullX" Steiner — Application Security Engineer

Observations

• No security concerns in this fix. The beforeNavigate guard is purely client-side UX — it cannot prevent the form POST from reaching the server, and its bug (showing the banner after a successful redirect) has no security implications.
• Both admin/groups/new/+page.server.ts and admin/users/new/+page.server.ts already enforce server-side authorization. The groups page has an implicit auth gate via session-based access; the users page has an explicit hasAdmin check in the load function. Neither is touched by this fix.
• The fix does not introduce new data flows, API calls, form fields, or permission changes.

Recommendations

• No action needed from a security perspective. Ship with confidence.

## 🔐 Nora "NullX" Steiner — Application Security Engineer ### Observations - No security concerns in this fix. The `beforeNavigate` guard is purely client-side UX — it cannot prevent the form POST from reaching the server, and its bug (showing the banner after a successful redirect) has no security implications. - Both `admin/groups/new/+page.server.ts` and `admin/users/new/+page.server.ts` already enforce server-side authorization. The groups page has an implicit auth gate via session-based access; the users page has an explicit `hasAdmin` check in the `load` function. Neither is touched by this fix. - The fix does not introduce new data flows, API calls, form fields, or permission changes. ### Recommendations - No action needed from a security perspective. Ship with confidence.

marcel commented 2026-05-14 11:12:07 +02:00

Author

Owner

Copy Link

🎨 Leonie Voss — UX Designer & Accessibility Strategist

Observations

• This is a Critical UX failure under Nielsen's Heuristic #1 (Visibility of System Status) and Heuristic #6 (Recognition over Recall). The system successfully completed the user's action but displayed feedback that looks identical to a failure — the amber "unsaved changes" banner. Users who don't understand the mechanics will:
  1. Assume the save failed
  2. Navigate away with "Discard changes" (which does nothing since the group was already created)
  3. Or create a duplicate group by submitting again
• The fix is correct and no UX changes are needed beyond the code fix.

Recommendations

• One minor observation: the UnsavedWarningBanner component uses the i18n key person_discard_changes() for the "Discard" button. This key name implies it belongs to the persons domain. The message text itself is presumably generic enough to read naturally in the admin context — verify the German string (Verwerfen?) is appropriate here and consider renaming the key to btn_discard_changes in a follow-up if it ever causes confusion.
• The banner itself is accessible: it has visible text and a labeled button. No ARIA issues.
• After the fix: on successful creation, the user is navigated to /admin/groups (or /admin/users) without any amber banner. This is the correct, clean success state.

## 🎨 Leonie Voss — UX Designer & Accessibility Strategist ### Observations - This is a **Critical UX failure** under Nielsen's Heuristic #1 (Visibility of System Status) and Heuristic #6 (Recognition over Recall). The system successfully completed the user's action but displayed feedback that looks identical to a failure — the amber "unsaved changes" banner. Users who don't understand the mechanics will: 1. Assume the save failed 2. Navigate away with "Discard changes" (which does nothing since the group was already created) 3. Or create a duplicate group by submitting again - The fix is correct and no UX changes are needed beyond the code fix. ### Recommendations - One minor observation: the `UnsavedWarningBanner` component uses the i18n key `person_discard_changes()` for the "Discard" button. This key name implies it belongs to the persons domain. The message text itself is presumably generic enough to read naturally in the admin context — verify the German string (`Verwerfen`?) is appropriate here and consider renaming the key to `btn_discard_changes` in a follow-up if it ever causes confusion. - The banner itself is accessible: it has visible text and a labeled button. No ARIA issues. - After the fix: on successful creation, the user is navigated to `/admin/groups` (or `/admin/users`) without any amber banner. This is the correct, clean success state.

marcel commented 2026-05-14 11:12:10 +02:00

Author

Owner

Copy Link

⚙️ Tobias Wendt — DevOps & Platform Engineer

Observations

• No infrastructure impact. This is a pure frontend component refactor within two existing SvelteKit pages.
• No new dependencies, Docker services, environment variables, or CI pipeline changes required.
• The fix introduces no new build artifacts and does not affect the backend API surface.

Recommendations

• Nothing to do from the infrastructure side. The fix can ship with the regular frontend build.

## ⚙️ Tobias Wendt — DevOps & Platform Engineer ### Observations - No infrastructure impact. This is a pure frontend component refactor within two existing SvelteKit pages. - No new dependencies, Docker services, environment variables, or CI pipeline changes required. - The fix introduces no new build artifacts and does not affect the backend API surface. ### Recommendations - Nothing to do from the infrastructure side. The fix can ship with the regular frontend build.

marcel commented 2026-05-14 11:12:17 +02:00

Author

Owner

Copy Link

📋 Elicit — Requirements Engineer

Observations

• Spec quality is high for a bugfix: root cause analysis, user-facing impact table, Gherkin acceptance criteria, test plan, and explicit out-of-scope bounds are all present. The issue is implementation-ready.
• The four Gherkin scenarios cover the main cases: success, cancelled navigation with warning, failed submission, and the mirror for /admin/users/new.

Recommendations

• One implicit edge case worth adding to the acceptance criteria or confirming during manual testing: what happens when the form is submitted with a failure result (form.error is shown), and the user then tries to navigate away? The failure path does not call clearOnSuccess(), so isDirty remains true and the guard should still trigger. This is the correct behavior — confirm it's covered in the spec.
• The test plan references admin/groups/[id]/page.svelte.spec.ts as the pattern, but the mechanism for clearing dirty state is different between the edit and create flows (see Felix and Sara's comments). Clarify in the test plan that the enhance callback approach — not rerender with form.success — is how the redirect-clearing case is tested.
• Scenario 3 ("Failed group creation keeps the user on the form") should explicitly state: "And the unsaved-changes guard remains active." This makes it unambiguous that a failed save does not disarm the navigation protection.

## 📋 Elicit — Requirements Engineer ### Observations - Spec quality is high for a bugfix: root cause analysis, user-facing impact table, Gherkin acceptance criteria, test plan, and explicit out-of-scope bounds are all present. The issue is implementation-ready. - The four Gherkin scenarios cover the main cases: success, cancelled navigation with warning, failed submission, and the mirror for `/admin/users/new`. ### Recommendations - One implicit edge case worth adding to the acceptance criteria or confirming during manual testing: **what happens when the form is submitted with a failure result (`form.error` is shown), and the user then tries to navigate away?** The failure path does not call `clearOnSuccess()`, so `isDirty` remains `true` and the guard should still trigger. This is the correct behavior — confirm it's covered in the spec. - The test plan references `admin/groups/[id]/page.svelte.spec.ts` as the pattern, but the mechanism for clearing dirty state is different between the edit and create flows (see Felix and Sara's comments). Clarify in the test plan that the `enhance` callback approach — not `rerender` with `form.success` — is how the redirect-clearing case is tested. - Scenario 3 ("Failed group creation keeps the user on the form") should explicitly state: "And the unsaved-changes guard remains active." This makes it unambiguous that a failed save does not disarm the navigation protection.

marcel referenced a pull request that will close this issue 2026-05-14 11:59:08 +02:00

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

marcel referenced this issue 2026-05-14 12:02:25 +02:00

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

Sign in to join this conversation.

No Branch/Tag Specified

Branches Tags

main

feat/issue-533-mass-import-ux

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

worktree-feat+issue-557-upload-artifact-v3-pin

worktree-chore+issue-556-drop-client-branches-coverage-gate

fix/issue-514-prerender-crawl-bakes-redirects

fix/issue-472-prerender-entries

feat/issue-395-readme

feat/issue-345-bulk-mark-reviewed

feat/issue-344-bell-tooltip

feat/issue-341-annotieren-contrast

feat/issue-225-bulk-metadata-edit

feat/issue-317-bulk-upload

feat/issue-271-dashboard-redesign

docs/issue-240-mission-control-spec

refactor/issues-193-200

feat/issue-162-korrespondenz-redesign

feature/68-new-document-file-first

feat/81-discussion-discoverability

feat/62-document-bottom-panel

feature/56-backfill-file-hashes

feat/38-document-edit-history

fix/svelte5-test-delegation-and-login-auth

No results found.

Labels

Clear labels

P0-critical

Blocks a core user journey, causes data loss, or is a security/accessibility barrier. Work on this before P1+.

P1-high

Significant friction on a main user journey; workaround exists but is non-obvious. Next up after P0.

P2-medium

Noticeable improvement; doesn't block anything; schedule opportunistically.

P3-later

Cosmetic or parking-lot work; fine to stay open indefinitely.

audit

Read-only audit / assessment work; produces a report rather than changing code

bug

Something isn't working

cleanup

Removal of dead code, vague comments, naming violations, and scratch artifacts

collaboration

We want to extend the family archive to have more collaboration tools

conversation

descoped

We will do that later

devops

documentation

README, ARCHITECTURE, GLOSSARY, CONTRIBUTING, per-domain docs

epic

Marker for epic-level issues that group multiple child issues

feature

file-upload

legibility

Codebase Legibility Refactor — preparing the codebase for human evaluation and bus-factor reduction

notification

person

phase-1: security

Security hygiene — must be done first

phase-2: container-images

Production-ready multi-stage Docker images

phase-3: prod-compose

Production compose overlay + Caddy reverse proxy

phase-4: spring-prod-profile

Spring Boot production configuration profile

phase-5: backups

Database and object storage backup strategy

phase-6: deployment-docs

.env.example, DEPLOYMENT.md, runbook

phase-7: monitoring

Prometheus, Loki, Grafana, Alertmanager

refactor

Code restructuring without behaviour change

security

test

ui

UI/UX and accessibility issues

user

No Label P1-high bug ui

Milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

Jens marcel

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: marcel/familienarchiv#527