From 902172e4e2f8a68419c083e2ba76ffd25a9f9447 Mon Sep 17 00:00:00 2001
From: Marcel <marcel@familienarchiv>
Date: Fri, 17 Apr 2026 08:12:35 +0200
Subject: [PATCH] =?UTF-8?q?fix(#248):=20fix=203=20merge=20zone=20bugs=20?=
 =?UTF-8?q?=E2=80=94=20stale=20state,=20wrong=20placeholder,=20missing=20s?=
 =?UTF-8?q?uccess=20feedback?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

- TagMergeZone: add $effect to reset targetId when tag prop changes (fixes stale form after navigation)
- TagMergeZone: pass merge-specific placeholder to TagParentPicker
- TagMergeZone: show success banner on form.mergeSuccess and goto() target tag
- +page.server.ts: merge action returns { mergeSuccess, mergeTargetId } instead of redirect

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
---
 .../routes/admin/tags/[id]/+page.server.ts    |   2 +-
 .../admin/tags/[id]/TagMergeZone.svelte       |  27 ++++-
 .../tags/[id]/TagMergeZone.svelte.spec.ts     | 103 +++++++++++++-----
 .../admin/tags/[id]/page.server.spec.ts       |  20 ++--
 4 files changed, 111 insertions(+), 41 deletions(-)

diff --git a/frontend/src/routes/admin/tags/[id]/+page.server.ts b/frontend/src/routes/admin/tags/[id]/+page.server.ts
index 3288b743..da3e5e26 100644
--- a/frontend/src/routes/admin/tags/[id]/+page.server.ts
+++ b/frontend/src/routes/admin/tags/[id]/+page.server.ts
@@ -47,7 +47,7 @@ export const actions: Actions = {
 			return fail(result.response.status, { error: getErrorMessage(code) });
 		}
 
-		throw redirect(303, `/admin/tags/${result.data!.id}`);
+		return { mergeSuccess: true, mergeTargetId: result.data!.id };
 	},
 
 	delete: async ({ params, request, fetch }) => {
diff --git a/frontend/src/routes/admin/tags/[id]/TagMergeZone.svelte b/frontend/src/routes/admin/tags/[id]/TagMergeZone.svelte
index 7ebebe34..4f790978 100644
--- a/frontend/src/routes/admin/tags/[id]/TagMergeZone.svelte
+++ b/frontend/src/routes/admin/tags/[id]/TagMergeZone.svelte
@@ -1,4 +1,5 @@
 <script lang="ts">
+import { goto } from '$app/navigation';
 import { enhance } from '$app/forms';
 import { m } from '$lib/paraglide/messages.js';
 import TagParentPicker from '$lib/components/TagParentPicker.svelte';
@@ -14,12 +15,23 @@ type FlatTag = {
 interface Props {
 	tag: { id: string; name: string; documentCount: number };
 	allTags: FlatTag[];
-	form: { error?: string } | null;
+	form: { error?: string; mergeSuccess?: boolean; mergeTargetId?: string } | null;
 }
 
 let { tag, allTags, form }: Props = $props();
 
 let targetId = $state('');
+
+$effect(() => {
+	void tag.id;
+	targetId = '';
+});
+
+$effect(() => {
+	if (form?.mergeSuccess && form.mergeTargetId) {
+		goto(`/admin/tags/${form.mergeTargetId}`);
+	}
+});
 const step = $derived(targetId ? 2 : 1);
 
 // All descendants of tag.id (to exclude from picker)
@@ -60,7 +72,12 @@ const targetTag = $derived(allTags.find((t) => t.id === targetId));
 		<label for="mergePickerTarget-search" class="mb-1 block text-xs font-medium text-ink-2">
 			{m.admin_tag_merge_target_label()}
 		</label>
-		<TagParentPicker name="mergePickerTarget" bind:value={targetId} excludeIds={excludeIds} />
+		<TagParentPicker
+			name="mergePickerTarget"
+			bind:value={targetId}
+			excludeIds={excludeIds}
+			placeholder={m.admin_tag_merge_target_placeholder()}
+		/>
 	</div>
 
 	<!-- Step 2: confirm -->
@@ -108,6 +125,12 @@ const targetTag = $derived(allTags.find((t) => t.id === targetId));
 		</div>
 	{/if}
 
+	{#if form?.mergeSuccess}
+		<p data-testid="merge-success-banner" class="mt-3 text-xs text-green-700">
+			{m.admin_tag_merge_success()}
+		</p>
+	{/if}
+
 	{#if form?.error}
 		<p class="mt-3 text-xs text-red-600">{form.error}</p>
 	{/if}
diff --git a/frontend/src/routes/admin/tags/[id]/TagMergeZone.svelte.spec.ts b/frontend/src/routes/admin/tags/[id]/TagMergeZone.svelte.spec.ts
index 5a17e441..b5d5b65c 100644
--- a/frontend/src/routes/admin/tags/[id]/TagMergeZone.svelte.spec.ts
+++ b/frontend/src/routes/admin/tags/[id]/TagMergeZone.svelte.spec.ts
@@ -1,11 +1,20 @@
-import { afterEach, describe, expect, it, vi } from 'vitest';
+import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
 import { cleanup, render } from 'vitest-browser-svelte';
 import { page } from 'vitest/browser';
 import TagMergeZone from './TagMergeZone.svelte';
 
 vi.mock('$app/forms', () => ({ enhance: () => () => {} }));
+vi.mock('$app/navigation', () => ({ goto: vi.fn() }));
 
-afterEach(cleanup);
+beforeEach(() => {
+	vi.useFakeTimers();
+});
+
+afterEach(() => {
+	cleanup();
+	vi.unstubAllGlobals();
+	vi.useRealTimers();
+});
 
 const tag = { id: 't1', name: 'Familie', documentCount: 3 };
 const allTags = [
@@ -14,7 +23,7 @@ const allTags = [
 	{ id: 't3', name: 'Urlaub', documentCount: 0, parentId: 't1' }
 ];
 
-describe('TagMergeZone', () => {
+describe('TagMergeZone – rendering', () => {
 	it('renders the merge heading', async () => {
 		render(TagMergeZone, { tag, allTags, form: null });
 		await expect.element(page.getByText(/Zusammenführen/i)).toBeInTheDocument();
@@ -25,35 +34,79 @@ describe('TagMergeZone', () => {
 		await expect.element(page.getByRole('combobox')).toBeInTheDocument();
 	});
 
+	it('combobox has merge-specific placeholder text', async () => {
+		render(TagMergeZone, { tag, allTags, form: null });
+		const input = await page.getByRole('combobox').element();
+		expect(input.getAttribute('placeholder')).toBe('Ziel-Schlagwort suchen …');
+	});
+});
+
+describe('TagMergeZone – step flow', () => {
 	it('step 2 is not shown before a target is selected', async () => {
 		const { container } = render(TagMergeZone, { tag, allTags, form: null });
 		expect(container.querySelector('[data-testid="merge-step2"]')).toBeFalsy();
 	});
 
 	it('shows step 2 confirm button after target is selected', async () => {
-		const { container } = render(TagMergeZone, { tag, allTags, form: null });
+		vi.stubGlobal(
+			'fetch',
+			vi.fn().mockResolvedValue({
+				ok: true,
+				json: vi.fn().mockResolvedValue([{ id: 't2', name: 'Reise' }])
+			})
+		);
+		render(TagMergeZone, { tag, allTags, form: null });
 
-		// Simulate target selection by directly dispatching change event
-		const input = container.querySelector<HTMLInputElement>('input[type="text"]');
-		expect(input).toBeTruthy();
+		const input = page.getByRole('combobox');
+		await input.fill('R');
+		await vi.advanceTimersByTimeAsync(300);
+		await page.getByRole('option', { name: 'Reise' }).click();
+		await vi.advanceTimersByTimeAsync(0);
 
-		// Simulate the hidden input being set (mimic TagParentPicker selecting 't2')
-		const hidden = container.querySelector<HTMLInputElement>('input[name="targetId"]');
-		if (hidden) {
-			Object.defineProperty(hidden, 'value', { value: 't2', writable: true });
-			hidden.dispatchEvent(new Event('change', { bubbles: true }));
-		}
-
-		// The merge confirm button should appear in step 2
-		// We test the component in isolation: since TagParentPicker is real,
-		// focus the combobox to trigger the mock and look for the step indicator
-		await expect.element(page.getByText(/Schritt 1 von 2/i)).toBeInTheDocument();
-	});
-
-	it('form has action ?/merge', async () => {
-		const { container } = render(TagMergeZone, { tag, allTags, form: null });
-		// Select a target to trigger step 2 rendering
-		// For now just check the component renders without error
-		expect(container).toBeTruthy();
+		await expect.element(page.getByTestId('merge-step2')).toBeInTheDocument();
+	});
+});
+
+describe('TagMergeZone – stale state reset', () => {
+	it('resets target selection when tag prop changes', async () => {
+		vi.stubGlobal(
+			'fetch',
+			vi.fn().mockResolvedValue({
+				ok: true,
+				json: vi.fn().mockResolvedValue([{ id: 't2', name: 'Reise' }])
+			})
+		);
+		const { rerender } = render(TagMergeZone, { tag, allTags, form: null });
+
+		const input = page.getByRole('combobox');
+		await input.fill('R');
+		await vi.advanceTimersByTimeAsync(300);
+		await page.getByRole('option', { name: 'Reise' }).click();
+		await vi.advanceTimersByTimeAsync(0);
+
+		await expect.element(page.getByTestId('merge-step2')).toBeInTheDocument();
+
+		// Navigate to a different tag
+		await rerender({ tag: { id: 't2', name: 'Reise', documentCount: 1 }, allTags, form: null });
+		await vi.advanceTimersByTimeAsync(0);
+
+		// step 2 should be gone — targetId was reset
+		expect(document.querySelector('[data-testid="merge-step2"]')).toBeFalsy();
+	});
+});
+
+describe('TagMergeZone – success banner', () => {
+	it('shows success banner when form.mergeSuccess is true', async () => {
+		render(TagMergeZone, {
+			tag,
+			allTags,
+			form: { mergeSuccess: true, mergeTargetId: 't2' }
+		});
+		await expect.element(page.getByTestId('merge-success-banner')).toBeInTheDocument();
+	});
+
+	it('does not show success banner when form is null', async () => {
+		const { container } = render(TagMergeZone, { tag, allTags, form: null });
+		expect(container.querySelector('[data-testid="merge-success-banner"]')).toBeFalsy();
 	});
 });
diff --git a/frontend/src/routes/admin/tags/[id]/page.server.spec.ts b/frontend/src/routes/admin/tags/[id]/page.server.spec.ts
index 698d5edb..e832c436 100644
--- a/frontend/src/routes/admin/tags/[id]/page.server.spec.ts
+++ b/frontend/src/routes/admin/tags/[id]/page.server.spec.ts
@@ -53,7 +53,7 @@ describe('tags/[id] — update action', () => {
 // ─── merge action ─────────────────────────────────────────────────────────────
 
 describe('tags/[id] — merge action', () => {
-	it('redirects to target tag on successful merge', async () => {
+	it('returns mergeSuccess and mergeTargetId on successful merge', async () => {
 		mockApi.POST.mockResolvedValue({
 			response: { ok: true },
 			data: { id: 't2', name: 'Reise' }
@@ -62,19 +62,13 @@ describe('tags/[id] — merge action', () => {
 		const formData = new FormData();
 		formData.set('targetId', 't2');
 
-		let redirectUrl: string | null = null;
-		try {
-			await actions.merge({
-				params: { id: 't1' },
-				request: { formData: async () => formData },
-				fetch
-			} as never);
-		} catch (e: unknown) {
-			const r = e as { location?: string };
-			redirectUrl = r.location ?? null;
-		}
+		const result = await actions.merge({
+			params: { id: 't1' },
+			request: { formData: async () => formData },
+			fetch
+		} as never);
 
-		expect(redirectUrl).toBe('/admin/tags/t2');
+		expect(result).toEqual({ mergeSuccess: true, mergeTargetId: 't2' });
 	});
 
 	it('returns fail when merge API responds not ok', async () => {